| Message ID | 1481812185-19098-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > This was somehow lost between v3 and the merged version in Maarten's > patch merged as: > > commit f2d580b9a8149735cbc4b59c4a8df60173658140 > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Date: Wed May 4 14:38:26 2016 +0200 > > drm/core: Do not preserve framebuffer on rmfb, v4. > > Actual code copied from Maarten's patch, but with the slight change to > just use dev->mode_config.funcs->atomic_commit to decide whether to > use the atomic path or not. > > v2: > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > - Add WARN_ON when atomic_remove_fb fails. > - Always call drm_atomic_state_put. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Would be great if someone else could r-b this, I've proven pretty well that I don't understand the complexity here :( -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_internal.h | 1 + > drivers/gpu/drm/drm_framebuffer.c | 7 ++++ > 3 files changed, 72 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index d1d252261bf1..23a3845542e1 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device *dev, > kfree(fence_state); > } > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_device *dev = fb->dev; > + struct drm_atomic_state *state; > + struct drm_plane *plane; > + int ret = 0; > + unsigned plane_mask; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return -ENOMEM; > + > + drm_modeset_acquire_init(&ctx, 0); > + state->acquire_ctx = &ctx; > + > +retry: > + plane_mask = 0; > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret) > + goto unlock; > + > + drm_for_each_plane(plane, dev) { > + struct drm_plane_state *plane_state; > + > + if (plane->state->fb != fb) > + continue; > + > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) { > + ret = PTR_ERR(plane_state); > + goto unlock; > + } > + > + drm_atomic_set_fb_for_plane(plane_state, NULL); > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > + if (ret) > + goto unlock; > + > + plane_mask |= BIT(drm_plane_index(plane)); > + > + plane->old_fb = plane->fb; > + } > + > + if (plane_mask) > + ret = drm_atomic_commit(state); > + > +unlock: > + if (plane_mask) > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > + > + if (ret == -EDEADLK) { > + drm_modeset_backoff(&ctx); > + goto retry; > + } > + > + drm_atomic_state_put(state); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return ret; > +} > + > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv) > { > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > index cdf6860c9d22..121e250853d2 100644 > --- a/drivers/gpu/drm/drm_crtc_internal.h > +++ b/drivers/gpu/drm/drm_crtc_internal.h > @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, > struct drm_property *property, uint64_t *val); > int drm_mode_atomic_ioctl(struct drm_device *dev, > void *data, struct drm_file *file_priv); > +int drm_atomic_remove_fb(struct drm_framebuffer *fb); > > > /* drm_plane.c */ > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > index cbf0c893f426..c358bf8280a8 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > * in this manner. > */ > if (drm_framebuffer_read_refcount(fb) > 1) { > + if (dev->mode_config.funcs->atomic_commit) { > + int ret = drm_atomic_remove_fb(fb); > + WARN(ret, "atomic remove_fb failed with %i\n", ret); > + goto out; > + } > + > drm_modeset_lock_all(dev); > /* remove from any CRTC */ > drm_for_each_crtc(crtc, dev) { > @@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > drm_modeset_unlock_all(dev); > } > > +out: > drm_framebuffer_unreference(fb); > } > EXPORT_SYMBOL(drm_framebuffer_remove); > -- > 2.7.4 >
On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: > On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > This was somehow lost between v3 and the merged version in Maarten's > > patch merged as: > > > > commit f2d580b9a8149735cbc4b59c4a8df60173658140 > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Date: Wed May 4 14:38:26 2016 +0200 > > > > drm/core: Do not preserve framebuffer on rmfb, v4. > > > > Actual code copied from Maarten's patch, but with the slight change to > > just use dev->mode_config.funcs->atomic_commit to decide whether to > > use the atomic path or not. > > > > v2: > > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > > - Add WARN_ON when atomic_remove_fb fails. > > - Always call drm_atomic_state_put. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Would be great if someone else could r-b this, I've proven pretty well > that I don't understand the complexity here :( > -Daniel It looks like this will change the behavior slightly in that rmfb will cause primary planes to be disabled, but no longer cause the entire CRTC to be turned off. You'll probably want to note that in the commit message, along with the justification on why this is okay ABI-wise. I know that 13803132818c ("drm/core: Preserve the framebuffer after removing it.") was initially trying to not only leave the CRTC on, but also preserve the framebuffer and leave the planes on; that wound up causing some kind of regression for vmwgfx, but I'm unclear on the details there. I'd suggest getting an Ack from one of the vmware guys to ensure that the less drastic change in behavior here won't cause them any problems. Your code looks correct to me, so with an expanded commit message, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> General observation; we specifically test for the presence of mode_config->funcs.atomic_commit here rather than DRIVER_ATOMIC because we care about a driver's internal atomic plumbing rather than whether it exposes atomic to userspace or not. I can see that causing confusion in the future, so I wonder if adding macros like DRM_SUPPORTS_ATOMIC_INTERNAL(dev) vs DRM_SUPPORTS_ATOMIC_USERSPACE(dev) might help overclarify exactly what/why we're testing in various places in the driver. Matt > > > --- > > drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/drm_crtc_internal.h | 1 + > > drivers/gpu/drm/drm_framebuffer.c | 7 ++++ > > 3 files changed, 72 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index d1d252261bf1..23a3845542e1 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device *dev, > > kfree(fence_state); > > } > > > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb) > > +{ > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_device *dev = fb->dev; > > + struct drm_atomic_state *state; > > + struct drm_plane *plane; > > + int ret = 0; > > + unsigned plane_mask; > > + > > + state = drm_atomic_state_alloc(dev); > > + if (!state) > > + return -ENOMEM; > > + > > + drm_modeset_acquire_init(&ctx, 0); > > + state->acquire_ctx = &ctx; > > + > > +retry: > > + plane_mask = 0; > > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > > + if (ret) > > + goto unlock; > > + > > + drm_for_each_plane(plane, dev) { > > + struct drm_plane_state *plane_state; > > + > > + if (plane->state->fb != fb) > > + continue; > > + > > + plane_state = drm_atomic_get_plane_state(state, plane); > > + if (IS_ERR(plane_state)) { > > + ret = PTR_ERR(plane_state); > > + goto unlock; > > + } > > + > > + drm_atomic_set_fb_for_plane(plane_state, NULL); > > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > > + if (ret) > > + goto unlock; > > + > > + plane_mask |= BIT(drm_plane_index(plane)); > > + > > + plane->old_fb = plane->fb; > > + } > > + > > + if (plane_mask) > > + ret = drm_atomic_commit(state); > > + > > +unlock: > > + if (plane_mask) > > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > > + > > + if (ret == -EDEADLK) { > > + drm_modeset_backoff(&ctx); > > + goto retry; > > + } > > + > > + drm_atomic_state_put(state); > > + > > + drm_modeset_drop_locks(&ctx); > > + drm_modeset_acquire_fini(&ctx); > > + > > + return ret; > > +} > > + > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > void *data, struct drm_file *file_priv) > > { > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > > index cdf6860c9d22..121e250853d2 100644 > > --- a/drivers/gpu/drm/drm_crtc_internal.h > > +++ b/drivers/gpu/drm/drm_crtc_internal.h > > @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, > > struct drm_property *property, uint64_t *val); > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > void *data, struct drm_file *file_priv); > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb); > > > > > > /* drm_plane.c */ > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > index cbf0c893f426..c358bf8280a8 100644 > > --- a/drivers/gpu/drm/drm_framebuffer.c > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > * in this manner. > > */ > > if (drm_framebuffer_read_refcount(fb) > 1) { > > + if (dev->mode_config.funcs->atomic_commit) { > > + int ret = drm_atomic_remove_fb(fb); > > + WARN(ret, "atomic remove_fb failed with %i\n", ret); > > + goto out; > > + } > > + > > drm_modeset_lock_all(dev); > > /* remove from any CRTC */ > > drm_for_each_crtc(crtc, dev) { > > @@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > drm_modeset_unlock_all(dev); > > } > > > > +out: > > drm_framebuffer_unreference(fb); > > } > > EXPORT_SYMBOL(drm_framebuffer_remove); > > -- > > 2.7.4 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: > On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: > > On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: > > > From: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > > This was somehow lost between v3 and the merged version in Maarten's > > > patch merged as: > > > > > > commit f2d580b9a8149735cbc4b59c4a8df60173658140 > > > Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Date: Wed May 4 14:38:26 2016 +0200 > > > > > > drm/core: Do not preserve framebuffer on rmfb, v4. > > > > > > Actual code copied from Maarten's patch, but with the slight change to > > > just use dev->mode_config.funcs->atomic_commit to decide whether to > > > use the atomic path or not. > > > > > > v2: > > > - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > > > - Add WARN_ON when atomic_remove_fb fails. > > > - Always call drm_atomic_state_put. > > > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > > Would be great if someone else could r-b this, I've proven pretty well > > that I don't understand the complexity here :( > > -Daniel > > It looks like this will change the behavior slightly in that rmfb will > cause primary planes to be disabled, but no longer cause the entire CRTC > to be turned off. You'll probably want to note that in the commit > message, along with the justification on why this is okay ABI-wise. > > I know that 13803132818c ("drm/core: Preserve the framebuffer after > removing it.") was initially trying to not only leave the CRTC on, but > also preserve the framebuffer and leave the planes on; that wound up > causing some kind of regression for vmwgfx, but I'm unclear on the > details there. I'd suggest getting an Ack from one of the vmware guys > to ensure that the less drastic change in behavior here won't cause them > any problems. Hm, this is a good point since with some atomic drivers (the ones using the simple pipe helpers at least) disabling the primary plane alone doesn't work. We might need a fallback path if the atomic_commit fails, which also disables the crtc on top. I guess that needs to be address? > Your code looks correct to me, so with an expanded commit message, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > General observation; we specifically test for the presence of > mode_config->funcs.atomic_commit here rather than DRIVER_ATOMIC because > we care about a driver's internal atomic plumbing rather than whether it > exposes atomic to userspace or not. I can see that causing confusion in > the future, so I wonder if adding macros like > DRM_SUPPORTS_ATOMIC_INTERNAL(dev) vs DRM_SUPPORTS_ATOMIC_USERSPACE(dev) > might help overclarify exactly what/why we're testing in various places > in the driver. This already exists and is called drm_drv_uses_atomic_modeset(). Maarten, can you pls update the patch? -Daniel > > > Matt > > > > > > --- > > > drivers/gpu/drm/drm_atomic.c | 64 +++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/drm_crtc_internal.h | 1 + > > > drivers/gpu/drm/drm_framebuffer.c | 7 ++++ > > > 3 files changed, 72 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > > index d1d252261bf1..23a3845542e1 100644 > > > --- a/drivers/gpu/drm/drm_atomic.c > > > +++ b/drivers/gpu/drm/drm_atomic.c > > > @@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device *dev, > > > kfree(fence_state); > > > } > > > > > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb) > > > +{ > > > + struct drm_modeset_acquire_ctx ctx; > > > + struct drm_device *dev = fb->dev; > > > + struct drm_atomic_state *state; > > > + struct drm_plane *plane; > > > + int ret = 0; > > > + unsigned plane_mask; > > > + > > > + state = drm_atomic_state_alloc(dev); > > > + if (!state) > > > + return -ENOMEM; > > > + > > > + drm_modeset_acquire_init(&ctx, 0); > > > + state->acquire_ctx = &ctx; > > > + > > > +retry: > > > + plane_mask = 0; > > > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > > > + if (ret) > > > + goto unlock; > > > + > > > + drm_for_each_plane(plane, dev) { > > > + struct drm_plane_state *plane_state; > > > + > > > + if (plane->state->fb != fb) > > > + continue; > > > + > > > + plane_state = drm_atomic_get_plane_state(state, plane); > > > + if (IS_ERR(plane_state)) { > > > + ret = PTR_ERR(plane_state); > > > + goto unlock; > > > + } > > > + > > > + drm_atomic_set_fb_for_plane(plane_state, NULL); > > > + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); > > > + if (ret) > > > + goto unlock; > > > + > > > + plane_mask |= BIT(drm_plane_index(plane)); > > > + > > > + plane->old_fb = plane->fb; > > > + } > > > + > > > + if (plane_mask) > > > + ret = drm_atomic_commit(state); > > > + > > > +unlock: > > > + if (plane_mask) > > > + drm_atomic_clean_old_fb(dev, plane_mask, ret); > > > + > > > + if (ret == -EDEADLK) { > > > + drm_modeset_backoff(&ctx); > > > + goto retry; > > > + } > > > + > > > + drm_atomic_state_put(state); > > > + > > > + drm_modeset_drop_locks(&ctx); > > > + drm_modeset_acquire_fini(&ctx); > > > + > > > + return ret; > > > +} > > > + > > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > > void *data, struct drm_file *file_priv) > > > { > > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h > > > index cdf6860c9d22..121e250853d2 100644 > > > --- a/drivers/gpu/drm/drm_crtc_internal.h > > > +++ b/drivers/gpu/drm/drm_crtc_internal.h > > > @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, > > > struct drm_property *property, uint64_t *val); > > > int drm_mode_atomic_ioctl(struct drm_device *dev, > > > void *data, struct drm_file *file_priv); > > > +int drm_atomic_remove_fb(struct drm_framebuffer *fb); > > > > > > > > > /* drm_plane.c */ > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c > > > index cbf0c893f426..c358bf8280a8 100644 > > > --- a/drivers/gpu/drm/drm_framebuffer.c > > > +++ b/drivers/gpu/drm/drm_framebuffer.c > > > @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > > * in this manner. > > > */ > > > if (drm_framebuffer_read_refcount(fb) > 1) { > > > + if (dev->mode_config.funcs->atomic_commit) { > > > + int ret = drm_atomic_remove_fb(fb); > > > + WARN(ret, "atomic remove_fb failed with %i\n", ret); > > > + goto out; > > > + } > > > + > > > drm_modeset_lock_all(dev); > > > /* remove from any CRTC */ > > > drm_for_each_crtc(crtc, dev) { > > > @@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) > > > drm_modeset_unlock_all(dev); > > > } > > > > > > +out: > > > drm_framebuffer_unreference(fb); > > > } > > > EXPORT_SYMBOL(drm_framebuffer_remove); > > > -- > > > 2.7.4 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795
On 01/25/2017 05:54 AM, Daniel Vetter wrote: > On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: >> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: >>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: >>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> >>>> This was somehow lost between v3 and the merged version in Maarten's >>>> patch merged as: >>>> >>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 >>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Date: Wed May 4 14:38:26 2016 +0200 >>>> >>>> drm/core: Do not preserve framebuffer on rmfb, v4. >>>> >>>> Actual code copied from Maarten's patch, but with the slight change to >>>> just use dev->mode_config.funcs->atomic_commit to decide whether to >>>> use the atomic path or not. >>>> >>>> v2: >>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. >>>> - Add WARN_ON when atomic_remove_fb fails. >>>> - Always call drm_atomic_state_put. >>>> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Would be great if someone else could r-b this, I've proven pretty well >>> that I don't understand the complexity here :( >>> -Daniel >> It looks like this will change the behavior slightly in that rmfb will >> cause primary planes to be disabled, but no longer cause the entire CRTC >> to be turned off. You'll probably want to note that in the commit >> message, along with the justification on why this is okay ABI-wise. >> >> I know that 13803132818c ("drm/core: Preserve the framebuffer after >> removing it.") was initially trying to not only leave the CRTC on, but >> also preserve the framebuffer and leave the planes on; that wound up >> causing some kind of regression for vmwgfx, but I'm unclear on the >> details there. I'd suggest getting an Ack from one of the vmware guys >> to ensure that the less drastic change in behavior here won't cause them >> any problems. The vmware Xorg driver is currently relying on rmfb to turn all attached crtcs off. Even if we were to fix that in the Xorg driver now, older Xorgs with newer kernels still would break. /Thomas
Op 25-01-17 om 09:09 schreef Thomas Hellstrom: > On 01/25/2017 05:54 AM, Daniel Vetter wrote: >> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: >>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: >>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: >>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> >>>>> This was somehow lost between v3 and the merged version in Maarten's >>>>> patch merged as: >>>>> >>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 >>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>> Date: Wed May 4 14:38:26 2016 +0200 >>>>> >>>>> drm/core: Do not preserve framebuffer on rmfb, v4. >>>>> >>>>> Actual code copied from Maarten's patch, but with the slight change to >>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to >>>>> use the atomic path or not. >>>>> >>>>> v2: >>>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. >>>>> - Add WARN_ON when atomic_remove_fb fails. >>>>> - Always call drm_atomic_state_put. >>>>> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> Would be great if someone else could r-b this, I've proven pretty well >>>> that I don't understand the complexity here :( >>>> -Daniel >>> It looks like this will change the behavior slightly in that rmfb will >>> cause primary planes to be disabled, but no longer cause the entire CRTC >>> to be turned off. You'll probably want to note that in the commit >>> message, along with the justification on why this is okay ABI-wise. >>> >>> I know that 13803132818c ("drm/core: Preserve the framebuffer after >>> removing it.") was initially trying to not only leave the CRTC on, but >>> also preserve the framebuffer and leave the planes on; that wound up >>> causing some kind of regression for vmwgfx, but I'm unclear on the >>> details there. I'd suggest getting an Ack from one of the vmware guys >>> to ensure that the less drastic change in behavior here won't cause them >>> any problems. > The vmware Xorg driver is currently relying on rmfb to turn all attached > crtcs off. Even if we were to fix that in the Xorg driver now, older > Xorgs with newer kernels still would break. Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled? If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow. However for right now vmwgfx uses the legacy rmfb, which does disable all crtc's. :) ~Maarten
On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote: > Op 25-01-17 om 09:09 schreef Thomas Hellstrom: > > On 01/25/2017 05:54 AM, Daniel Vetter wrote: > >> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: > >>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: > >>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: > >>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>> > >>>>> This was somehow lost between v3 and the merged version in Maarten's > >>>>> patch merged as: > >>>>> > >>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 > >>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>> Date: Wed May 4 14:38:26 2016 +0200 > >>>>> > >>>>> drm/core: Do not preserve framebuffer on rmfb, v4. > >>>>> > >>>>> Actual code copied from Maarten's patch, but with the slight change to > >>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to > >>>>> use the atomic path or not. > >>>>> > >>>>> v2: > >>>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > >>>>> - Add WARN_ON when atomic_remove_fb fails. > >>>>> - Always call drm_atomic_state_put. > >>>>> > >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>> Would be great if someone else could r-b this, I've proven pretty well > >>>> that I don't understand the complexity here :( > >>>> -Daniel > >>> It looks like this will change the behavior slightly in that rmfb will > >>> cause primary planes to be disabled, but no longer cause the entire CRTC > >>> to be turned off. You'll probably want to note that in the commit > >>> message, along with the justification on why this is okay ABI-wise. > >>> > >>> I know that 13803132818c ("drm/core: Preserve the framebuffer after > >>> removing it.") was initially trying to not only leave the CRTC on, but > >>> also preserve the framebuffer and leave the planes on; that wound up > >>> causing some kind of regression for vmwgfx, but I'm unclear on the > >>> details there. I'd suggest getting an Ack from one of the vmware guys > >>> to ensure that the less drastic change in behavior here won't cause them > >>> any problems. > > The vmware Xorg driver is currently relying on rmfb to turn all attached > > crtcs off. Even if we were to fix that in the Xorg driver now, older > > Xorgs with newer kernels still would break. > Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled? > > If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow. FYI, we are in the process of converting things to atomic. This may happen around 4.12
Op 25-01-17 om 19:05 schreef Sinclair Yeh: > On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote: >> Op 25-01-17 om 09:09 schreef Thomas Hellstrom: >>> On 01/25/2017 05:54 AM, Daniel Vetter wrote: >>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: >>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: >>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: >>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>> >>>>>>> This was somehow lost between v3 and the merged version in Maarten's >>>>>>> patch merged as: >>>>>>> >>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 >>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>> Date: Wed May 4 14:38:26 2016 +0200 >>>>>>> >>>>>>> drm/core: Do not preserve framebuffer on rmfb, v4. >>>>>>> >>>>>>> Actual code copied from Maarten's patch, but with the slight change to >>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to >>>>>>> use the atomic path or not. >>>>>>> >>>>>>> v2: >>>>>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. >>>>>>> - Add WARN_ON when atomic_remove_fb fails. >>>>>>> - Always call drm_atomic_state_put. >>>>>>> >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>> Would be great if someone else could r-b this, I've proven pretty well >>>>>> that I don't understand the complexity here :( >>>>>> -Daniel >>>>> It looks like this will change the behavior slightly in that rmfb will >>>>> cause primary planes to be disabled, but no longer cause the entire CRTC >>>>> to be turned off. You'll probably want to note that in the commit >>>>> message, along with the justification on why this is okay ABI-wise. >>>>> >>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after >>>>> removing it.") was initially trying to not only leave the CRTC on, but >>>>> also preserve the framebuffer and leave the planes on; that wound up >>>>> causing some kind of regression for vmwgfx, but I'm unclear on the >>>>> details there. I'd suggest getting an Ack from one of the vmware guys >>>>> to ensure that the less drastic change in behavior here won't cause them >>>>> any problems. >>> The vmware Xorg driver is currently relying on rmfb to turn all attached >>> crtcs off. Even if we were to fix that in the Xorg driver now, older >>> Xorgs with newer kernels still would break. >> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled? >> >> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow. > FYI, we are in the process of converting things to atomic. This may happen > around 4.12 > Will the driver allow the crtc to be enabled without primary plane?
On Thu, Jan 26, 2017 at 10:55:51AM +0100, Maarten Lankhorst wrote: > Op 25-01-17 om 19:05 schreef Sinclair Yeh: > > On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote: > >> Op 25-01-17 om 09:09 schreef Thomas Hellstrom: > >>> On 01/25/2017 05:54 AM, Daniel Vetter wrote: > >>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: > >>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: > >>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: > >>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>>>> > >>>>>>> This was somehow lost between v3 and the merged version in Maarten's > >>>>>>> patch merged as: > >>>>>>> > >>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 > >>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>>> Date: Wed May 4 14:38:26 2016 +0200 > >>>>>>> > >>>>>>> drm/core: Do not preserve framebuffer on rmfb, v4. > >>>>>>> > >>>>>>> Actual code copied from Maarten's patch, but with the slight change to > >>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to > >>>>>>> use the atomic path or not. > >>>>>>> > >>>>>>> v2: > >>>>>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. > >>>>>>> - Add WARN_ON when atomic_remove_fb fails. > >>>>>>> - Always call drm_atomic_state_put. > >>>>>>> > >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>>>>> Would be great if someone else could r-b this, I've proven pretty well > >>>>>> that I don't understand the complexity here :( > >>>>>> -Daniel > >>>>> It looks like this will change the behavior slightly in that rmfb will > >>>>> cause primary planes to be disabled, but no longer cause the entire CRTC > >>>>> to be turned off. You'll probably want to note that in the commit > >>>>> message, along with the justification on why this is okay ABI-wise. > >>>>> > >>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after > >>>>> removing it.") was initially trying to not only leave the CRTC on, but > >>>>> also preserve the framebuffer and leave the planes on; that wound up > >>>>> causing some kind of regression for vmwgfx, but I'm unclear on the > >>>>> details there. I'd suggest getting an Ack from one of the vmware guys > >>>>> to ensure that the less drastic change in behavior here won't cause them > >>>>> any problems. > >>> The vmware Xorg driver is currently relying on rmfb to turn all attached > >>> crtcs off. Even if we were to fix that in the Xorg driver now, older > >>> Xorgs with newer kernels still would break. > >> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled? > >> > >> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow. > > FYI, we are in the process of converting things to atomic. This may happen > > around 4.12 > > > Will the driver allow the crtc to be enabled without primary plane? Give me a few days to get back to you. I'm reworking some patches right now.
Op 26-01-17 om 19:39 schreef Sinclair Yeh: > On Thu, Jan 26, 2017 at 10:55:51AM +0100, Maarten Lankhorst wrote: >> Op 25-01-17 om 19:05 schreef Sinclair Yeh: >>> On Wed, Jan 25, 2017 at 09:36:36AM +0100, Maarten Lankhorst wrote: >>>> Op 25-01-17 om 09:09 schreef Thomas Hellstrom: >>>>> On 01/25/2017 05:54 AM, Daniel Vetter wrote: >>>>>> On Tue, Jan 24, 2017 at 01:44:54PM -0800, Matt Roper wrote: >>>>>>> On Wed, Jan 11, 2017 at 05:15:47PM +0100, Daniel Vetter wrote: >>>>>>>> On Thu, Dec 15, 2016 at 03:29:45PM +0100, Maarten Lankhorst wrote: >>>>>>>>> From: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>>>> >>>>>>>>> This was somehow lost between v3 and the merged version in Maarten's >>>>>>>>> patch merged as: >>>>>>>>> >>>>>>>>> commit f2d580b9a8149735cbc4b59c4a8df60173658140 >>>>>>>>> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>>>> Date: Wed May 4 14:38:26 2016 +0200 >>>>>>>>> >>>>>>>>> drm/core: Do not preserve framebuffer on rmfb, v4. >>>>>>>>> >>>>>>>>> Actual code copied from Maarten's patch, but with the slight change to >>>>>>>>> just use dev->mode_config.funcs->atomic_commit to decide whether to >>>>>>>>> use the atomic path or not. >>>>>>>>> >>>>>>>>> v2: >>>>>>>>> - Remove plane->fb assignment, done by drm_atomic_clean_old_fb. >>>>>>>>> - Add WARN_ON when atomic_remove_fb fails. >>>>>>>>> - Always call drm_atomic_state_put. >>>>>>>>> >>>>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>>>>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >>>>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>>>>>> Would be great if someone else could r-b this, I've proven pretty well >>>>>>>> that I don't understand the complexity here :( >>>>>>>> -Daniel >>>>>>> It looks like this will change the behavior slightly in that rmfb will >>>>>>> cause primary planes to be disabled, but no longer cause the entire CRTC >>>>>>> to be turned off. You'll probably want to note that in the commit >>>>>>> message, along with the justification on why this is okay ABI-wise. >>>>>>> >>>>>>> I know that 13803132818c ("drm/core: Preserve the framebuffer after >>>>>>> removing it.") was initially trying to not only leave the CRTC on, but >>>>>>> also preserve the framebuffer and leave the planes on; that wound up >>>>>>> causing some kind of regression for vmwgfx, but I'm unclear on the >>>>>>> details there. I'd suggest getting an Ack from one of the vmware guys >>>>>>> to ensure that the less drastic change in behavior here won't cause them >>>>>>> any problems. >>>>> The vmware Xorg driver is currently relying on rmfb to turn all attached >>>>> crtcs off. Even if we were to fix that in the Xorg driver now, older >>>>> Xorgs with newer kernels still would break. >>>> Is it allowed for vmwgfx to keep the crtc enabled, but the primary plane disabled? >>>> >>>> If so, when vmwgfx is eventually converted to atomic then we need to special-case rmfb for them somehow. >>> FYI, we are in the process of converting things to atomic. This may happen >>> around 4.12 >>> >> Will the driver allow the crtc to be enabled without primary plane? > Give me a few days to get back to you. I'm reworking some patches right now. > > Any update on this? ~Maarten
I've verified that it doesn't break our existing code, but I'm in the process of rebasing my atomic enabling patch series onto drm-next along with this. I should be able to get this done by tomorrow morning.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index d1d252261bf1..23a3845542e1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device *dev, kfree(fence_state); } +int drm_atomic_remove_fb(struct drm_framebuffer *fb) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_device *dev = fb->dev; + struct drm_atomic_state *state; + struct drm_plane *plane; + int ret = 0; + unsigned plane_mask; + + state = drm_atomic_state_alloc(dev); + if (!state) + return -ENOMEM; + + drm_modeset_acquire_init(&ctx, 0); + state->acquire_ctx = &ctx; + +retry: + plane_mask = 0; + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (ret) + goto unlock; + + drm_for_each_plane(plane, dev) { + struct drm_plane_state *plane_state; + + if (plane->state->fb != fb) + continue; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + ret = PTR_ERR(plane_state); + goto unlock; + } + + drm_atomic_set_fb_for_plane(plane_state, NULL); + ret = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (ret) + goto unlock; + + plane_mask |= BIT(drm_plane_index(plane)); + + plane->old_fb = plane->fb; + } + + if (plane_mask) + ret = drm_atomic_commit(state); + +unlock: + if (plane_mask) + drm_atomic_clean_old_fb(dev, plane_mask, ret); + + if (ret == -EDEADLK) { + drm_modeset_backoff(&ctx); + goto retry; + } + + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; +} + int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index cdf6860c9d22..121e250853d2 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj, struct drm_property *property, uint64_t *val); int drm_mode_atomic_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_atomic_remove_fb(struct drm_framebuffer *fb); /* drm_plane.c */ diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index cbf0c893f426..c358bf8280a8 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) * in this manner. */ if (drm_framebuffer_read_refcount(fb) > 1) { + if (dev->mode_config.funcs->atomic_commit) { + int ret = drm_atomic_remove_fb(fb); + WARN(ret, "atomic remove_fb failed with %i\n", ret); + goto out; + } + drm_modeset_lock_all(dev); /* remove from any CRTC */ drm_for_each_crtc(crtc, dev) { @@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_modeset_unlock_all(dev); } +out: drm_framebuffer_unreference(fb); } EXPORT_SYMBOL(drm_framebuffer_remove);