Re: [PATCH 1/2] drm/atomic: Make atomic helper track newly assigned planes correctly.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 16, 2017 at 05:28:27PM +0200, Maarten Lankhorst wrote:
> Op 16-10-17 om 16:48 schreef Ville Syrjälä:
> > On Mon, Oct 16, 2017 at 03:59:38PM +0200, Maarten Lankhorst wrote:
> >> Op 16-10-17 om 15:42 schreef Ville Syrjälä:
> >>> On Mon, Oct 16, 2017 at 03:29:27PM +0200, Maarten Lankhorst wrote:
> >>>> Commit 669c9215afea ("drm/atomic: Make async plane update checks work as
> >>>> intended, v2.") forced planes to always be tracked, but forgot to
> >>>> explicitly get the crtc commit from the new crtc when available.
> >>>>
> >>>> This broke plane commit tracking, and caused kms_atomic_transitions
> >>>> to randomly fail with -EBUSY.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Fixes: 669c9215afea ("drm/atomic: Make async plane update checks work as intended, v2.")
> >>>> Cc: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102671
> >>>> Testcase: kms_atomic_transitions
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index d59441f1dcd4..b64c8f5bc940 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -1804,7 +1804,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> >>>>  		    !try_wait_for_completion(&old_plane_state->commit->flip_done))
> >>>>  			return -EBUSY;
> >>>>  
> >>>> -		commit = crtc_or_fake_commit(state, old_plane_state->crtc);
> >>>> +		commit = crtc_or_fake_commit(state, old_plane_state->crtc ?: new_plane_state->crtc);
> >>> Shouldn't old vs. new state be the other way around?
> >> Hmm to be honest, could be. We don't allow crtc's to switch planes directly. So in practice it doesn't matter.
> > Not sure where we actually prevent that. A quick trawl through the code
> > didn't reveal anything like that.
> plane_switching_crtc(), called from drm_atomic_check_only->drm_atomic_plane_check().

Ah, that's where it was hiding.

> 
> So I wouldn't worry. :)
> >> But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..
> > new is the one it'd be hanging off at the end so that seems like the
> > right choice. It would also match what we do in i915 code.
> Ok new one it is, with that changed do I have your r-b?
> 

Yes.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx