| Message ID | 20171016132928.6498-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > if (!commit) > return -ENOMEM; > > -- > 2.14.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. But if we ever did allow moving crtc's, it's up for debate what crtc we want to use here..
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. > 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.
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?
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.
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); if (!commit) return -ENOMEM;
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(-)