| Message ID | 20171101150433.10777-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > This introduces a slight behavioral change to rmfb. Instead of > disabling a crtc when the primary plane is disabled, we try to > preserve it. > > Apart from old versions of the vmwgfx xorg driver, there is > nothing depending on rmfb disabling a crtc. > > Vmwgfx' and simple kms helper atomic implementation rejects CRTC > enabled without plane, so we can do this safely. > > If the atomic commit is rejected by the driver then we will still > fall back to the old behavior and turn off the crtc. > > Changes since v1: > - Restart completely when rmfb with crtc on fails (Sean Paul). > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 2affe53f3fda..f0679468f421 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > struct drm_plane *plane; > struct drm_connector *conn; > struct drm_connector_state *conn_state; > - int i, ret = 0; > + int i, ret; > unsigned plane_mask; > + bool disable_crtcs = false; > > - state = drm_atomic_state_alloc(dev); > - if (!state) > - return -ENOMEM; > - > +retry_disable: > drm_modeset_acquire_init(&ctx, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto out; > + } > state->acquire_ctx = &ctx; > > retry: > @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > } > > - if (plane_state->crtc->primary == plane) { > + if (disable_crtcs && plane_state->crtc->primary == plane) { > struct drm_crtc_state *crtc_state; > > crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > plane->old_fb = plane->fb; > } > > + /* This list is only filled when disable_crtcs is set. */ > for_each_new_connector_in_state(state, conn, conn_state, i) { WARN_ON(!disable_crtcs) maybe? > ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > > @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > drm_atomic_state_put(state); > > +out: > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > + if (ret == -EINVAL && !disable_crtcs) { Hmm. -EINVAL seems rather specific. Not sure if we could just check for any error? Or... I'm not sure if we have any central place where we do the "can I disable the primary plane w/o disabling the crtc?" check. If we do then we could also add a comment there informing people that the -EINVAL is important. > + disable_crtcs = true; > + goto retry_disable; > + } > + > return ret; > } > > -- > 2.14.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 01-11-17 om 16:29 schreef Ville Syrjälä: > On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: >> This introduces a slight behavioral change to rmfb. Instead of >> disabling a crtc when the primary plane is disabled, we try to >> preserve it. >> >> Apart from old versions of the vmwgfx xorg driver, there is >> nothing depending on rmfb disabling a crtc. >> >> Vmwgfx' and simple kms helper atomic implementation rejects CRTC >> enabled without plane, so we can do this safely. >> >> If the atomic commit is rejected by the driver then we will still >> fall back to the old behavior and turn off the crtc. >> >> Changes since v1: >> - Restart completely when rmfb with crtc on fails (Sean Paul). >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >> index 2affe53f3fda..f0679468f421 100644 >> --- a/drivers/gpu/drm/drm_framebuffer.c >> +++ b/drivers/gpu/drm/drm_framebuffer.c >> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >> struct drm_plane *plane; >> struct drm_connector *conn; >> struct drm_connector_state *conn_state; >> - int i, ret = 0; >> + int i, ret; >> unsigned plane_mask; >> + bool disable_crtcs = false; >> >> - state = drm_atomic_state_alloc(dev); >> - if (!state) >> - return -ENOMEM; >> - >> +retry_disable: >> drm_modeset_acquire_init(&ctx, 0); >> + >> + state = drm_atomic_state_alloc(dev); >> + if (!state) { >> + ret = -ENOMEM; >> + goto out; >> + } >> state->acquire_ctx = &ctx; >> >> retry: >> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >> goto unlock; >> } >> >> - if (plane_state->crtc->primary == plane) { >> + if (disable_crtcs && plane_state->crtc->primary == plane) { >> struct drm_crtc_state *crtc_state; >> >> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >> plane->old_fb = plane->fb; >> } >> >> + /* This list is only filled when disable_crtcs is set. */ >> for_each_new_connector_in_state(state, conn, conn_state, i) { > WARN_ON(!disable_crtcs) maybe? Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :) >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); >> >> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >> >> drm_atomic_state_put(state); >> >> +out: >> drm_modeset_drop_locks(&ctx); >> drm_modeset_acquire_fini(&ctx); >> >> + if (ret == -EINVAL && !disable_crtcs) { > Hmm. -EINVAL seems rather specific. Not sure if we could just check for > any error? > > Or... I'm not sure if we have any central place where we do the > "can I disable the primary plane w/o disabling the crtc?" check. If we > do then we could also add a comment there informing people that the > -EINVAL is important. We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :)
On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: > Op 01-11-17 om 16:29 schreef Ville Syrjälä: > > On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > >> This introduces a slight behavioral change to rmfb. Instead of > >> disabling a crtc when the primary plane is disabled, we try to > >> preserve it. > >> > >> Apart from old versions of the vmwgfx xorg driver, there is > >> nothing depending on rmfb disabling a crtc. > >> > >> Vmwgfx' and simple kms helper atomic implementation rejects CRTC > >> enabled without plane, so we can do this safely. The code for those seems a bit inconsistent. The crtc check requires that the crtc state and plane state match. But the plane check allows the plane to be enabled w/o the crtc being enabled. I guess it doesn't matter really since you can't enable the plane without a crtc, and the crtc check would then catch the case where the crtc would be disabled. Oh and looks like drm_plane_helper_check_state() is a bit buggy. It still uses crtc->enabled instead of crtc_state->enable to check the state of the crtc. I guess to keep drm_plane_helper_check_update() working we may have to pass in the crtc state manually. The vmwgfx plane check looks a bit bogus in other ways too. I guess I'll have to fire off a couple of patches. > >> > >> If the atomic commit is rejected by the driver then we will still > >> fall back to the old behavior and turn off the crtc. > >> > >> Changes since v1: > >> - Restart completely when rmfb with crtc on fails (Sean Paul). > >> > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Sean Paul <seanpaul@chromium.org> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> --- > >> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ > >> 1 file changed, 17 insertions(+), 6 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > >> index 2affe53f3fda..f0679468f421 100644 > >> --- a/drivers/gpu/drm/drm_framebuffer.c > >> +++ b/drivers/gpu/drm/drm_framebuffer.c > >> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > >> struct drm_plane *plane; > >> struct drm_connector *conn; > >> struct drm_connector_state *conn_state; > >> - int i, ret = 0; > >> + int i, ret; > >> unsigned plane_mask; > >> + bool disable_crtcs = false; > >> > >> - state = drm_atomic_state_alloc(dev); > >> - if (!state) > >> - return -ENOMEM; > >> - > >> +retry_disable: > >> drm_modeset_acquire_init(&ctx, 0); > >> + > >> + state = drm_atomic_state_alloc(dev); > >> + if (!state) { > >> + ret = -ENOMEM; > >> + goto out; > >> + } > >> state->acquire_ctx = &ctx; > >> > >> retry: > >> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > >> goto unlock; > >> } > >> > >> - if (plane_state->crtc->primary == plane) { > >> + if (disable_crtcs && plane_state->crtc->primary == plane) { > >> struct drm_crtc_state *crtc_state; > >> > >> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > >> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > >> plane->old_fb = plane->fb; > >> } > >> > >> + /* This list is only filled when disable_crtcs is set. */ > >> for_each_new_connector_in_state(state, conn, conn_state, i) { > > WARN_ON(!disable_crtcs) maybe? > Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :) It would serve as a way to document that fact, even without the comment. But I won't insist on it. > >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > >> > >> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > >> > >> drm_atomic_state_put(state); > >> > >> +out: > >> drm_modeset_drop_locks(&ctx); > >> drm_modeset_acquire_fini(&ctx); > >> > >> + if (ret == -EINVAL && !disable_crtcs) { > > Hmm. -EINVAL seems rather specific. Not sure if we could just check for > > any error? > > > > Or... I'm not sure if we have any central place where we do the > > "can I disable the primary plane w/o disabling the crtc?" check. If we > > do then we could also add a comment there informing people that the > > -EINVAL is important. > We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :) Oh well. I guess people just have to be careful with their error values. I suppoe anyone depending on the retry will notice this issue rather quickly.
Op 01-11-17 om 18:00 schreef Ville Syrjälä: > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: >> Op 01-11-17 om 16:29 schreef Ville Syrjälä: >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: >>>> This introduces a slight behavioral change to rmfb. Instead of >>>> disabling a crtc when the primary plane is disabled, we try to >>>> preserve it. >>>> >>>> Apart from old versions of the vmwgfx xorg driver, there is >>>> nothing depending on rmfb disabling a crtc. >>>> >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC >>>> enabled without plane, so we can do this safely. > The code for those seems a bit inconsistent. The crtc check requires > that the crtc state and plane state match. But the plane check allows > the plane to be enabled w/o the crtc being enabled. I guess it doesn't > matter really since you can't enable the plane without a crtc, and the > crtc check would then catch the case where the crtc would be disabled. > > Oh and looks like drm_plane_helper_check_state() is a bit buggy. It > still uses crtc->enabled instead of crtc_state->enable to check the > state of the crtc. I guess to keep drm_plane_helper_check_update() > working we may have to pass in the crtc state manually. This is the transitional helper. i915 gets away with it because it passes the flag that ignores crtc->enabled. > The vmwgfx plane check looks a bit bogus in other ways too. I guess > I'll have to fire off a couple of patches. > >>>> If the atomic commit is rejected by the driver then we will still >>>> fall back to the old behavior and turn off the crtc. >>>> >>>> Changes since v1: >>>> - Restart completely when rmfb with crtc on fails (Sean Paul). >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Sean Paul <seanpaul@chromium.org> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> --- >>>> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ >>>> 1 file changed, 17 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >>>> index 2affe53f3fda..f0679468f421 100644 >>>> --- a/drivers/gpu/drm/drm_framebuffer.c >>>> +++ b/drivers/gpu/drm/drm_framebuffer.c >>>> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> struct drm_plane *plane; >>>> struct drm_connector *conn; >>>> struct drm_connector_state *conn_state; >>>> - int i, ret = 0; >>>> + int i, ret; >>>> unsigned plane_mask; >>>> + bool disable_crtcs = false; >>>> >>>> - state = drm_atomic_state_alloc(dev); >>>> - if (!state) >>>> - return -ENOMEM; >>>> - >>>> +retry_disable: >>>> drm_modeset_acquire_init(&ctx, 0); >>>> + >>>> + state = drm_atomic_state_alloc(dev); >>>> + if (!state) { >>>> + ret = -ENOMEM; >>>> + goto out; >>>> + } >>>> state->acquire_ctx = &ctx; >>>> >>>> retry: >>>> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> goto unlock; >>>> } >>>> >>>> - if (plane_state->crtc->primary == plane) { >>>> + if (disable_crtcs && plane_state->crtc->primary == plane) { >>>> struct drm_crtc_state *crtc_state; >>>> >>>> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >>>> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> plane->old_fb = plane->fb; >>>> } >>>> >>>> + /* This list is only filled when disable_crtcs is set. */ >>>> for_each_new_connector_in_state(state, conn, conn_state, i) { >>> WARN_ON(!disable_crtcs) maybe? >> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :) > It would serve as a way to document that fact, even without the comment. > But I won't insist on it. > >>>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); >>>> >>>> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> >>>> drm_atomic_state_put(state); >>>> >>>> +out: >>>> drm_modeset_drop_locks(&ctx); >>>> drm_modeset_acquire_fini(&ctx); >>>> >>>> + if (ret == -EINVAL && !disable_crtcs) { >>> Hmm. -EINVAL seems rather specific. Not sure if we could just check for >>> any error? >>> >>> Or... I'm not sure if we have any central place where we do the >>> "can I disable the primary plane w/o disabling the crtc?" check. If we >>> do then we could also add a comment there informing people that the >>> -EINVAL is important. >> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :) > Oh well. I guess people just have to be careful with their error > values. I suppoe anyone depending on the retry will notice this > issue rather quickly. Yes. :)
Op 01-11-17 om 18:00 schreef Ville Syrjälä: > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: >> Op 01-11-17 om 16:29 schreef Ville Syrjälä: >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: >>>> This introduces a slight behavioral change to rmfb. Instead of >>>> disabling a crtc when the primary plane is disabled, we try to >>>> preserve it. >>>> >>>> Apart from old versions of the vmwgfx xorg driver, there is >>>> nothing depending on rmfb disabling a crtc. >>>> >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC >>>> enabled without plane, so we can do this safely. > The code for those seems a bit inconsistent. The crtc check requires > that the crtc state and plane state match. But the plane check allows > the plane to be enabled w/o the crtc being enabled. I guess it doesn't > matter really since you can't enable the plane without a crtc, and the > crtc check would then catch the case where the crtc would be disabled. Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't be invoked. Hence it's the most accurate way of making sure that crtc enabled <=> primary plane bound. If the check was done in the primary plane, an atomic modeset could enable the crtc without enabling the primary plane, which shouldn't be allowed but a plane check won't catch it. This has been a bug in simple-kms-helper, fixed in the below commit: commit 765831dc27ab141b3a0be1ab55b922b012427902 Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Date: Wed Jul 12 10:13:29 2017 +0200 drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled. > Oh and looks like drm_plane_helper_check_state() is a bit buggy. It > still uses crtc->enabled instead of crtc_state->enable to check the > state of the crtc. I guess to keep drm_plane_helper_check_update() > working we may have to pass in the crtc state manually. > > The vmwgfx plane check looks a bit bogus in other ways too. I guess > I'll have to fire off a couple of patches. > >>>> If the atomic commit is rejected by the driver then we will still >>>> fall back to the old behavior and turn off the crtc. >>>> >>>> Changes since v1: >>>> - Restart completely when rmfb with crtc on fails (Sean Paul). >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Cc: Sean Paul <seanpaul@chromium.org> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> --- >>>> drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ >>>> 1 file changed, 17 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >>>> index 2affe53f3fda..f0679468f421 100644 >>>> --- a/drivers/gpu/drm/drm_framebuffer.c >>>> +++ b/drivers/gpu/drm/drm_framebuffer.c >>>> @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> struct drm_plane *plane; >>>> struct drm_connector *conn; >>>> struct drm_connector_state *conn_state; >>>> - int i, ret = 0; >>>> + int i, ret; >>>> unsigned plane_mask; >>>> + bool disable_crtcs = false; >>>> >>>> - state = drm_atomic_state_alloc(dev); >>>> - if (!state) >>>> - return -ENOMEM; >>>> - >>>> +retry_disable: >>>> drm_modeset_acquire_init(&ctx, 0); >>>> + >>>> + state = drm_atomic_state_alloc(dev); >>>> + if (!state) { >>>> + ret = -ENOMEM; >>>> + goto out; >>>> + } >>>> state->acquire_ctx = &ctx; >>>> >>>> retry: >>>> @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> goto unlock; >>>> } >>>> >>>> - if (plane_state->crtc->primary == plane) { >>>> + if (disable_crtcs && plane_state->crtc->primary == plane) { >>>> struct drm_crtc_state *crtc_state; >>>> >>>> crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >>>> @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> plane->old_fb = plane->fb; >>>> } >>>> >>>> + /* This list is only filled when disable_crtcs is set. */ >>>> for_each_new_connector_in_state(state, conn, conn_state, i) { >>> WARN_ON(!disable_crtcs) maybe? >> Would be overkill, nothing before it adds connector state, and if atomic check does then that's fine, but it won't be run here. :) > It would serve as a way to document that fact, even without the comment. > But I won't insist on it. > >>>> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); >>>> >>>> @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) >>>> >>>> drm_atomic_state_put(state); >>>> >>>> +out: >>>> drm_modeset_drop_locks(&ctx); >>>> drm_modeset_acquire_fini(&ctx); >>>> >>>> + if (ret == -EINVAL && !disable_crtcs) { >>> Hmm. -EINVAL seems rather specific. Not sure if we could just check for >>> any error? >>> >>> Or... I'm not sure if we have any central place where we do the >>> "can I disable the primary plane w/o disabling the crtc?" check. If we >>> do then we could also add a comment there informing people that the >>> -EINVAL is important. >> We don't have a central place, I check for EINVAL since that is the generic atomic_check() failed error. If it fails for any other reason then we don't have to retry, but pass it along. :) > Oh well. I guess people just have to be careful with their error > values. I suppoe anyone depending on the retry will notice this > issue rather quickly. >
On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote: > Op 01-11-17 om 18:00 schreef Ville Syrjälä: > > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: > >> Op 01-11-17 om 16:29 schreef Ville Syrjälä: > >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > >>>> This introduces a slight behavioral change to rmfb. Instead of > >>>> disabling a crtc when the primary plane is disabled, we try to > >>>> preserve it. > >>>> > >>>> Apart from old versions of the vmwgfx xorg driver, there is > >>>> nothing depending on rmfb disabling a crtc. > >>>> > >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC > >>>> enabled without plane, so we can do this safely. > > The code for those seems a bit inconsistent. The crtc check requires > > that the crtc state and plane state match. But the plane check allows > > the plane to be enabled w/o the crtc being enabled. I guess it doesn't > > matter really since you can't enable the plane without a crtc, and the > > crtc check would then catch the case where the crtc would be disabled. > > Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't > be invoked. Hence it's the most accurate way of making sure that > crtc enabled <=> primary plane bound. > > If the check was done in the primary plane, an atomic modeset could enable > the crtc without enabling the primary plane, which shouldn't be allowed but > a plane check won't catch it. > > This has been a bug in simple-kms-helper, fixed in the below commit: > > commit 765831dc27ab141b3a0be1ab55b922b012427902 > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Date: Wed Jul 12 10:13:29 2017 +0200 > > drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled. Hmm. OK that part looks OK. What does seem a bit inconsistent is the fact that we pass can_update_disabled=true, but later on we reject the update when visible==false. The old code would have accepted that because it didn't even call drm_plane_helper_check_state() when the crtc (and thus also the plane) was disabled.
On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote: > On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote: > > Op 01-11-17 om 18:00 schreef Ville Syrjälä: > > > On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: > > >> Op 01-11-17 om 16:29 schreef Ville Syrjälä: > > >>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > > >>>> This introduces a slight behavioral change to rmfb. Instead of > > >>>> disabling a crtc when the primary plane is disabled, we try to > > >>>> preserve it. > > >>>> > > >>>> Apart from old versions of the vmwgfx xorg driver, there is > > >>>> nothing depending on rmfb disabling a crtc. > > >>>> > > >>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC > > >>>> enabled without plane, so we can do this safely. > > > The code for those seems a bit inconsistent. The crtc check requires > > > that the crtc state and plane state match. But the plane check allows > > > the plane to be enabled w/o the crtc being enabled. I guess it doesn't > > > matter really since you can't enable the plane without a crtc, and the > > > crtc check would then catch the case where the crtc would be disabled. > > > > Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't > > be invoked. Hence it's the most accurate way of making sure that > > crtc enabled <=> primary plane bound. > > > > If the check was done in the primary plane, an atomic modeset could enable > > the crtc without enabling the primary plane, which shouldn't be allowed but > > a plane check won't catch it. > > > > > This has been a bug in simple-kms-helper, fixed in the below commit: > > > > commit 765831dc27ab141b3a0be1ab55b922b012427902 > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Date: Wed Jul 12 10:13:29 2017 +0200 > > > > drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled. > > Hmm. OK that part looks OK. What does seem a bit inconsistent is the > fact that we pass can_update_disabled=true, but later on we reject the > update when visible==false. The old code would have accepted that > because it didn't even call drm_plane_helper_check_state() when the > crtc (and thus also the plane) was disabled. Actually how does this work at all? If you turn off both the crtc and plane, then the plane check will always return -EINVAL on account of visible==false?
Op 06-11-17 om 15:06 schreef Ville Syrjälä: > On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote: >> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote: >>> Op 01-11-17 om 18:00 schreef Ville Syrjälä: >>>> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: >>>>> Op 01-11-17 om 16:29 schreef Ville Syrjälä: >>>>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: >>>>>>> This introduces a slight behavioral change to rmfb. Instead of >>>>>>> disabling a crtc when the primary plane is disabled, we try to >>>>>>> preserve it. >>>>>>> >>>>>>> Apart from old versions of the vmwgfx xorg driver, there is >>>>>>> nothing depending on rmfb disabling a crtc. >>>>>>> >>>>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC >>>>>>> enabled without plane, so we can do this safely. >>>> The code for those seems a bit inconsistent. The crtc check requires >>>> that the crtc state and plane state match. But the plane check allows >>>> the plane to be enabled w/o the crtc being enabled. I guess it doesn't >>>> matter really since you can't enable the plane without a crtc, and the >>>> crtc check would then catch the case where the crtc would be disabled. >>> Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't >>> be invoked. Hence it's the most accurate way of making sure that >>> crtc enabled <=> primary plane bound. >>> >>> If the check was done in the primary plane, an atomic modeset could enable >>> the crtc without enabling the primary plane, which shouldn't be allowed but >>> a plane check won't catch it. >>> This has been a bug in simple-kms-helper, fixed in the below commit: >>> >>> commit 765831dc27ab141b3a0be1ab55b922b012427902 >>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Date: Wed Jul 12 10:13:29 2017 +0200 >>> >>> drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled. >> Hmm. OK that part looks OK. What does seem a bit inconsistent is the >> fact that we pass can_update_disabled=true, but later on we reject the >> update when visible==false. The old code would have accepted that >> because it didn't even call drm_plane_helper_check_state() when the >> crtc (and thus also the plane) was disabled. > Actually how does this work at all? If you turn off both the crtc and > plane, then the plane check will always return -EINVAL on account of > visible==false? > If the crtc is turned off, enabled = false and the primary plane is not in crtc_state->plane_mask. Visibility is ignored in this check, that part is handled in plane check that the framebuffer has to span the entire crtc if enabled.
On Mon, Nov 06, 2017 at 04:43:17PM +0100, Maarten Lankhorst wrote: > Op 06-11-17 om 15:06 schreef Ville Syrjälä: > > On Mon, Nov 06, 2017 at 04:01:20PM +0200, Ville Syrjälä wrote: > >> On Thu, Nov 02, 2017 at 09:55:40AM +0100, Maarten Lankhorst wrote: > >>> Op 01-11-17 om 18:00 schreef Ville Syrjälä: > >>>> On Wed, Nov 01, 2017 at 04:55:06PM +0100, Maarten Lankhorst wrote: > >>>>> Op 01-11-17 om 16:29 schreef Ville Syrjälä: > >>>>>> On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > >>>>>>> This introduces a slight behavioral change to rmfb. Instead of > >>>>>>> disabling a crtc when the primary plane is disabled, we try to > >>>>>>> preserve it. > >>>>>>> > >>>>>>> Apart from old versions of the vmwgfx xorg driver, there is > >>>>>>> nothing depending on rmfb disabling a crtc. > >>>>>>> > >>>>>>> Vmwgfx' and simple kms helper atomic implementation rejects CRTC > >>>>>>> enabled without plane, so we can do this safely. > >>>> The code for those seems a bit inconsistent. The crtc check requires > >>>> that the crtc state and plane state match. But the plane check allows > >>>> the plane to be enabled w/o the crtc being enabled. I guess it doesn't > >>>> matter really since you can't enable the plane without a crtc, and the > >>>> crtc check would then catch the case where the crtc would be disabled. > >>> Exactly. :-) Only when a plane is unbound and stays unbound, the crtc check won't > >>> be invoked. Hence it's the most accurate way of making sure that > >>> crtc enabled <=> primary plane bound. > >>> > >>> If the check was done in the primary plane, an atomic modeset could enable > >>> the crtc without enabling the primary plane, which shouldn't be allowed but > >>> a plane check won't catch it. > >>> This has been a bug in simple-kms-helper, fixed in the below commit: > >>> > >>> commit 765831dc27ab141b3a0be1ab55b922b012427902 > >>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Date: Wed Jul 12 10:13:29 2017 +0200 > >>> > >>> drm/simple-kms-helper: Fix the check for the mismatch between plane and CRTC enabled. > >> Hmm. OK that part looks OK. What does seem a bit inconsistent is the > >> fact that we pass can_update_disabled=true, but later on we reject the > >> update when visible==false. The old code would have accepted that > >> because it didn't even call drm_plane_helper_check_state() when the > >> crtc (and thus also the plane) was disabled. > > Actually how does this work at all? If you turn off both the crtc and > > plane, then the plane check will always return -EINVAL on account of > > visible==false? > > > If the crtc is turned off, enabled = false and the primary plane is not in crtc_state->plane_mask. > > Visibility is ignored in this check, that part is handled in plane check that the framebuffer has to span the entire crtc if enabled. Hmm. I thought the !crtc->enabled check disappeared from drm_simple_kms_plane_atomic_check() but looks like I was wrong and it's still there. OK, just totally ignore what I wrote before. I guess one shouldn't try to figure out what the code is going to be doing while in the middle of an unrelated bisect. Though for some extra consistency we might want to change that to check to look for !fb after calling drm_plane_helper_check_state(). That way we wouldn't have to change drivers/simple_kms_helper if come up with something that has to be checked even for disabled planes in drm_plane_helper_check_state().
On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: > This introduces a slight behavioral change to rmfb. Instead of > disabling a crtc when the primary plane is disabled, we try to > preserve it. > > Apart from old versions of the vmwgfx xorg driver, there is > nothing depending on rmfb disabling a crtc. > > Vmwgfx' and simple kms helper atomic implementation rejects CRTC > enabled without plane, so we can do this safely. > > If the atomic commit is rejected by the driver then we will still > fall back to the old behavior and turn off the crtc. > > Changes since v1: > - Restart completely when rmfb with crtc on fails (Sean Paul). > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index 2affe53f3fda..f0679468f421 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > struct drm_plane *plane; > struct drm_connector *conn; > struct drm_connector_state *conn_state; > - int i, ret = 0; > + int i, ret; > unsigned plane_mask; > + bool disable_crtcs = false; > > - state = drm_atomic_state_alloc(dev); > - if (!state) > - return -ENOMEM; > - > +retry_disable: > drm_modeset_acquire_init(&ctx, 0); > + > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto out; > + } > state->acquire_ctx = &ctx; > > retry: > @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > } > > - if (plane_state->crtc->primary == plane) { > + if (disable_crtcs && plane_state->crtc->primary == plane) { > struct drm_crtc_state *crtc_state; > > crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > plane->old_fb = plane->fb; > } > > + /* This list is only filled when disable_crtcs is set. */ > for_each_new_connector_in_state(state, conn, conn_state, i) { > ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > > @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) > > drm_atomic_state_put(state); > > +out: > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > + if (ret == -EINVAL && !disable_crtcs) { > + disable_crtcs = true; > + goto retry_disable; > + } > + > return ret; > } > > -- > 2.14.1 >
Op 07-11-17 om 15:31 schreef Daniel Vetter: > On Wed, Nov 01, 2017 at 04:04:33PM +0100, Maarten Lankhorst wrote: >> This introduces a slight behavioral change to rmfb. Instead of >> disabling a crtc when the primary plane is disabled, we try to >> preserve it. >> >> Apart from old versions of the vmwgfx xorg driver, there is >> nothing depending on rmfb disabling a crtc. >> >> Vmwgfx' and simple kms helper atomic implementation rejects CRTC >> enabled without plane, so we can do this safely. >> >> If the atomic commit is rejected by the driver then we will still >> fall back to the old behavior and turn off the crtc. >> >> Changes since v1: >> - Restart completely when rmfb with crtc on fails (Sean Paul). >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Sean Paul <seanpaul@chromium.org> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thanks, pushed. :)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 2affe53f3fda..f0679468f421 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -765,14 +765,18 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) struct drm_plane *plane; struct drm_connector *conn; struct drm_connector_state *conn_state; - int i, ret = 0; + int i, ret; unsigned plane_mask; + bool disable_crtcs = false; - state = drm_atomic_state_alloc(dev); - if (!state) - return -ENOMEM; - +retry_disable: drm_modeset_acquire_init(&ctx, 0); + + state = drm_atomic_state_alloc(dev); + if (!state) { + ret = -ENOMEM; + goto out; + } state->acquire_ctx = &ctx; retry: @@ -793,7 +797,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) goto unlock; } - if (plane_state->crtc->primary == plane) { + if (disable_crtcs && plane_state->crtc->primary == plane) { struct drm_crtc_state *crtc_state; crtc_state = drm_atomic_get_existing_crtc_state(state, plane_state->crtc); @@ -818,6 +822,7 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) plane->old_fb = plane->fb; } + /* This list is only filled when disable_crtcs is set. */ for_each_new_connector_in_state(state, conn, conn_state, i) { ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); @@ -840,9 +845,15 @@ static int atomic_remove_fb(struct drm_framebuffer *fb) drm_atomic_state_put(state); +out: drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); + if (ret == -EINVAL && !disable_crtcs) { + disable_crtcs = true; + goto retry_disable; + } + return ret; }
This introduces a slight behavioral change to rmfb. Instead of disabling a crtc when the primary plane is disabled, we try to preserve it. Apart from old versions of the vmwgfx xorg driver, there is nothing depending on rmfb disabling a crtc. Vmwgfx' and simple kms helper atomic implementation rejects CRTC enabled without plane, so we can do this safely. If the atomic commit is rejected by the driver then we will still fall back to the old behavior and turn off the crtc. Changes since v1: - Restart completely when rmfb with crtc on fails (Sean Paul). Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_framebuffer.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)