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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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().

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?


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx