| Message ID | 20161209141944.22121-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote: > 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. > > FIXME: This seems to break audio rpm refcounting somehow! See: > > commit 0dcac5008fcf57cce66ef091204efbde86956c7a > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Jul 14 15:16:34 2016 +0200 > > Revert "drm: Resurrect atomic rmfb code" > > v2: Use new atomic state refcounting. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-email-daniel.vetter@ffwll.ch intel-gfx CI told me this breaks the world. I guess back to the drawing board. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 66 +++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/drm_crtc_internal.h | 1 + > drivers/gpu/drm/drm_framebuffer.c | 6 ++++ > 3 files changed, 73 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 72fa5b20baf1..6cc6c0f7609a 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -2009,6 +2009,72 @@ 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; > + plane->fb = NULL; > + } > + > + 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; > + } > + > + if (ret || !plane_mask) > + 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..533ce991d446 100644 > --- a/drivers/gpu/drm/drm_framebuffer.c > +++ b/drivers/gpu/drm/drm_framebuffer.c > @@ -770,6 +770,11 @@ 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) { > + drm_atomic_remove_fb(fb); > + goto out; > + } > + > drm_modeset_lock_all(dev); > /* remove from any CRTC */ > drm_for_each_crtc(crtc, dev) { > @@ -787,6 +792,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.10.2 >
Op 09-12-16 om 21:59 schreef Daniel Vetter: > On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote: >> 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. >> >> FIXME: This seems to break audio rpm refcounting somehow! See: >> >> commit 0dcac5008fcf57cce66ef091204efbde86956c7a >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> >> Date: Thu Jul 14 15:16:34 2016 +0200 >> >> Revert "drm: Resurrect atomic rmfb code" >> >> v2: Use new atomic state refcounting. >> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-email-daniel.vetter@ffwll.ch > intel-gfx CI told me this breaks the world. I guess back to the drawing > board. What happens when we add an argument to __drm_framebuffer_remove whether crtc has to be disabled or not, and make drm_framebuffer_remove always say it has to disable crtc, while being called from rmfb_work_fn tries to keep crtc enabled? ~Maarten
On Mon, Dec 12, 2016 at 09:46:59AM +0100, Maarten Lankhorst wrote: > Op 09-12-16 om 21:59 schreef Daniel Vetter: > > On Fri, Dec 09, 2016 at 03:19:44PM +0100, Daniel Vetter wrote: > >> 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. > >> > >> FIXME: This seems to break audio rpm refcounting somehow! See: > >> > >> commit 0dcac5008fcf57cce66ef091204efbde86956c7a > >> Author: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Date: Thu Jul 14 15:16:34 2016 +0200 > >> > >> Revert "drm: Resurrect atomic rmfb code" > >> > >> v2: Use new atomic state refcounting. > >> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-email-daniel.vetter@ffwll.ch > > intel-gfx CI told me this breaks the world. I guess back to the drawing > > board. > What happens when we add an argument to __drm_framebuffer_remove whether > crtc has to be disabled or not, and make drm_framebuffer_remove always > say it has to disable crtc, while being called from rmfb_work_fn tries > to keep crtc enabled? tbh I'm not sure what the regression this time around was. But I guess long-term we want some kind of atomic rmfb, since iirc it blocks the re-adding of ww_acquire_done? I've thrown this patch out of my pile for now, got burned too often ;-) -Daniel
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 72fa5b20baf1..6cc6c0f7609a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -2009,6 +2009,72 @@ 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; + plane->fb = NULL; + } + + 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; + } + + if (ret || !plane_mask) + 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..533ce991d446 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -770,6 +770,11 @@ 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) { + drm_atomic_remove_fb(fb); + goto out; + } + drm_modeset_lock_all(dev); /* remove from any CRTC */ drm_for_each_crtc(crtc, dev) { @@ -787,6 +792,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) drm_modeset_unlock_all(dev); } +out: drm_framebuffer_unreference(fb); } EXPORT_SYMBOL(drm_framebuffer_remove);
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. FIXME: This seems to break audio rpm refcounting somehow! See: commit 0dcac5008fcf57cce66ef091204efbde86956c7a Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Jul 14 15:16:34 2016 +0200 Revert "drm: Resurrect atomic rmfb code" v2: Use new atomic state refcounting. Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1465388359-8070-24-git-send-email-daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 66 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c | 6 ++++ 3 files changed, 73 insertions(+)