| Message ID | 1487685102-31991-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 21, 2017 at 02:51:42PM +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. Since vmwgfx is > a legacy driver we can safely only disable the plane with atomic. > In the conversion to atomic the driver will reject the config > without primary plane. > > If this commit is rejected by the driver then we will still fall > back to the old behavior and turn off the crtc. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- > 1 file changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 285e1c23e8c9..0d5b0065208e 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > struct drm_connector *conn; > struct drm_connector_state *conn_state; > int i, ret = 0; > - unsigned plane_mask; > + unsigned plane_mask, disable_crtcs = false; > > state = drm_atomic_state_alloc(dev); > if (!state) > @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > goto unlock; > } > > - if (plane_state->crtc->primary == plane) { > + /* > + * Some drivers do not support keeping crtc active with the > + * primary plane disabled. If we fail to commit with -EINVAL > + * then we will try to perform the same commit but with all > + * crtc's disabled for primary planes as well. > + */ > + 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); > @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > plane->old_fb = plane->fb; > } > > + /* This list is only not empty when disable_crtcs is set. */ > for_each_connector_in_state(state, conn, conn_state, i) { > ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); > > @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) > > drm_atomic_state_put(state); > > + if (ret == -EINVAL && !disable_crtcs) { > + disable_crtcs = true; > + > + state = drm_atomic_state_alloc(dev); > + if (state) { > + state->acquire_ctx = &ctx; > + goto retry; Re-using the retry label is a little fishy here. You need to re-allocate state, but we don't drop the locks, which makes things a little unbalanced (I realize modeset_lock will s/EALREADY/0/). Perhaps something like this above would be more clear: + drm_modeset_acquire_init(&ctx, 0); +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + +retry_disable: + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + state->acquire_ctx = &ctx; + Then you can do: + if (ret == -EINVAL && !disable_crtcs) { + disable_crtcs = true; + goto retry_disable; + } (you would also need to ensure you moved the state_put above the EDEADLK handling). Thoughts? Sean > + } > + ret = -ENOMEM; > + } > + > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 23-02-17 om 16:24 schreef Sean Paul: > On Tue, Feb 21, 2017 at 02:51:42PM +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. Since vmwgfx is >> a legacy driver we can safely only disable the plane with atomic. >> In the conversion to atomic the driver will reject the config >> without primary plane. >> >> If this commit is rejected by the driver then we will still fall >> back to the old behavior and turn off the crtc. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- >> 1 file changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 285e1c23e8c9..0d5b0065208e 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> struct drm_connector *conn; >> struct drm_connector_state *conn_state; >> int i, ret = 0; >> - unsigned plane_mask; >> + unsigned plane_mask, disable_crtcs = false; >> >> state = drm_atomic_state_alloc(dev); >> if (!state) >> @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> goto unlock; >> } >> >> - if (plane_state->crtc->primary == plane) { >> + /* >> + * Some drivers do not support keeping crtc active with the >> + * primary plane disabled. If we fail to commit with -EINVAL >> + * then we will try to perform the same commit but with all >> + * crtc's disabled for primary planes as well. >> + */ >> + 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); >> @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> plane->old_fb = plane->fb; >> } >> >> + /* This list is only not empty when disable_crtcs is set. */ >> for_each_connector_in_state(state, conn, conn_state, i) { >> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); >> >> @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) >> >> drm_atomic_state_put(state); >> >> + if (ret == -EINVAL && !disable_crtcs) { >> + disable_crtcs = true; >> + >> + state = drm_atomic_state_alloc(dev); >> + if (state) { >> + state->acquire_ctx = &ctx; >> + goto retry; > Re-using the retry label is a little fishy here. You need to re-allocate state, > but we don't drop the locks, which makes things a little unbalanced (I realize > modeset_lock will s/EALREADY/0/). drm_modeset_lock can be called any arbitrary numer of times. I'm not worried about it. :) > Perhaps something like this above would be more clear: > > + drm_modeset_acquire_init(&ctx, 0); > +retry: > + plane_mask = 0; > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret) > + goto unlock; > + > +retry_disable: > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + state->acquire_ctx = &ctx; > + > > Then you can do: > > > + if (ret == -EINVAL && !disable_crtcs) { > + disable_crtcs = true; > + goto retry_disable; > + } > > (you would also need to ensure you moved the state_put above the EDEADLK > handling). > > Thoughts? In general we try to call drm_atomic_state_clear instead of drm_atomic_state_put, this would be a source of confusion for being different from other users of atomic. ~Maarten
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 285e1c23e8c9..0d5b0065208e 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2070,7 +2070,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) struct drm_connector *conn; struct drm_connector_state *conn_state; int i, ret = 0; - unsigned plane_mask; + unsigned plane_mask, disable_crtcs = false; state = drm_atomic_state_alloc(dev); if (!state) @@ -2097,7 +2097,13 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) goto unlock; } - if (plane_state->crtc->primary == plane) { + /* + * Some drivers do not support keeping crtc active with the + * primary plane disabled. If we fail to commit with -EINVAL + * then we will try to perform the same commit but with all + * crtc's disabled for primary planes as well. + */ + 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); @@ -2122,6 +2128,7 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) plane->old_fb = plane->fb; } + /* This list is only not empty when disable_crtcs is set. */ for_each_connector_in_state(state, conn, conn_state, i) { ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); @@ -2143,6 +2150,17 @@ int drm_atomic_remove_fb(struct drm_framebuffer *fb) drm_atomic_state_put(state); + if (ret == -EINVAL && !disable_crtcs) { + disable_crtcs = true; + + state = drm_atomic_state_alloc(dev); + if (state) { + state->acquire_ctx = &ctx; + goto retry; + } + ret = -ENOMEM; + } + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);
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. Since vmwgfx is a legacy driver we can safely only disable the plane with atomic. In the conversion to atomic the driver will reject the config without primary plane. If this commit is rejected by the driver then we will still fall back to the old behavior and turn off the crtc. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/drm_atomic.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-)