* [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable @ 2017-12-07 3:01 Gurchetan Singh 2017-12-07 9:16 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Gurchetan Singh @ 2017-12-07 3:01 UTC (permalink / raw) To: dri-devel; +Cc: Gurchetan Singh, laurent.pinchart, daniel.vetter When a CRTC is disabled and we used an internally created framebuffer, this patch disables the cursor plane and drops the reference that was introduced when we called drm_internal_framebuffer_create. Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> --- drivers/gpu/drm/drm_crtc.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f0556e654116..d732cca4879f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index); */ int drm_crtc_force_disable(struct drm_crtc *crtc) { + struct drm_framebuffer *fb; struct drm_mode_set set = { .crtc = crtc, }; WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); + if (crtc->cursor && crtc->cursor->fb && crtc->cursor->fb->internal) { + fb = crtc->cursor->fb; + drm_plane_force_disable(crtc->cursor); + drm_framebuffer_unreference(fb); + } + return drm_mode_set_config_internal(&set); } EXPORT_SYMBOL(drm_crtc_force_disable); -- 2.13.5 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable 2017-12-07 3:01 [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable Gurchetan Singh @ 2017-12-07 9:16 ` Daniel Vetter 2017-12-08 22:17 ` Gurchetan Singh 0 siblings, 1 reply; 4+ messages in thread From: Daniel Vetter @ 2017-12-07 9:16 UTC (permalink / raw) To: Gurchetan Singh; +Cc: daniel.vetter, laurent.pinchart, dri-devel On Wed, Dec 06, 2017 at 07:01:59PM -0800, Gurchetan Singh wrote: > When a CRTC is disabled and we used an internally created framebuffer, > this patch disables the cursor plane and drops the reference that was > introduced when we called drm_internal_framebuffer_create. > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> What kind of bug are you trying to fix here? plane and cursor uapi is that when you re-enable the crtc, all the planes will be there again. Only exception is the primary plane, but that's only because set_config both disables the crtc _and_ the primary plane. Without more detail of what's going on I have no idea what exactly you're trying to achieve here. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index f0556e654116..d732cca4879f 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index); > */ > int drm_crtc_force_disable(struct drm_crtc *crtc) > { > + struct drm_framebuffer *fb; > struct drm_mode_set set = { > .crtc = crtc, > }; > > WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); > > + if (crtc->cursor && crtc->cursor->fb && crtc->cursor->fb->internal) { > + fb = crtc->cursor->fb; > + drm_plane_force_disable(crtc->cursor); > + drm_framebuffer_unreference(fb); > + } > + > return drm_mode_set_config_internal(&set); > } > EXPORT_SYMBOL(drm_crtc_force_disable); > -- > 2.13.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable 2017-12-07 9:16 ` Daniel Vetter @ 2017-12-08 22:17 ` Gurchetan Singh 2017-12-11 9:13 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Gurchetan Singh @ 2017-12-08 22:17 UTC (permalink / raw) To: Daniel Vetter; +Cc: ML dri-devel [-- Attachment #1.1: Type: text/plain, Size: 2575 bytes --] The problem I'm trying to solve is that the internal cursor fb is leaky and even present after closing the drm driver fd. This can be seen by running modetest after this test case: https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/master/drm_cursor_test.c However, as you mentioned, the approach in this patch is not consistent with the uapi. Adding the internal fb to file_priv->fbs is another approach, though I haven't gotten it to work in 100% of all cases. What do you recommend (if anything at all)? On Thu, Dec 7, 2017 at 1:16 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Dec 06, 2017 at 07:01:59PM -0800, Gurchetan Singh wrote: > > When a CRTC is disabled and we used an internally created framebuffer, > > this patch disables the cursor plane and drops the reference that was > > introduced when we called drm_internal_framebuffer_create. > > > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> > > What kind of bug are you trying to fix here? plane and cursor uapi is that > when you re-enable the crtc, all the planes will be there again. Only > exception is the primary plane, but that's only because set_config both > disables the crtc _and_ the primary plane. > > Without more detail of what's going on I have no idea what exactly you're > trying to achieve here. > -Daniel > > --- > > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index f0556e654116..d732cca4879f 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index); > > */ > > int drm_crtc_force_disable(struct drm_crtc *crtc) > > { > > + struct drm_framebuffer *fb; > > struct drm_mode_set set = { > > .crtc = crtc, > > }; > > > > WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); > > > > + if (crtc->cursor && crtc->cursor->fb && > crtc->cursor->fb->internal) { > > + fb = crtc->cursor->fb; > > + drm_plane_force_disable(crtc->cursor); > > + drm_framebuffer_unreference(fb); > > + } > > + > > return drm_mode_set_config_internal(&set); > > } > > EXPORT_SYMBOL(drm_crtc_force_disable); > > -- > > 2.13.5 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > [-- Attachment #1.2: Type: text/html, Size: 3876 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable 2017-12-08 22:17 ` Gurchetan Singh @ 2017-12-11 9:13 ` Daniel Vetter 0 siblings, 0 replies; 4+ messages in thread From: Daniel Vetter @ 2017-12-11 9:13 UTC (permalink / raw) To: Gurchetan Singh; +Cc: ML dri-devel On Fri, Dec 08, 2017 at 02:17:39PM -0800, Gurchetan Singh wrote: > The problem I'm trying to solve is that the internal cursor fb is leaky and > even present after closing the drm driver fd. This can be seen by running > modetest after this test case: > > https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/master/drm_cursor_test.c > > However, as you mentioned, the approach in this patch is not consistent > with the uapi. Adding the internal fb to file_priv->fbs is another > approach, though I haven't gotten it to work in 100% of all cases. What do > you recommend (if anything at all)? Definitely don't do that - userspace doesn't know to clean that up. And we'd need to then track this fake cursor internally in the kernel so that e.g. a cursor animation doesn't result in an endless stream of leaked fbs (which will get cleaned up once you close the fb, but there's people who easily don't restart their compositor for weeks - no way we can just leak for that long). So the uapi is that the cursor stays on when the fb closes. Has been like that since forever (cursor on fbcon is famous!), and universal cursors simply emulate that. If you disable the cursor, your leak will be gone too. Backwards compat uapi forever has some lols sometimes :-) Imo just fix modetest to not be this broken. -Daniel > > On Thu, Dec 7, 2017 at 1:16 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Wed, Dec 06, 2017 at 07:01:59PM -0800, Gurchetan Singh wrote: > > > When a CRTC is disabled and we used an internally created framebuffer, > > > this patch disables the cursor plane and drops the reference that was > > > introduced when we called drm_internal_framebuffer_create. > > > > > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> > > > > What kind of bug are you trying to fix here? plane and cursor uapi is that > > when you re-enable the crtc, all the planes will be there again. Only > > exception is the primary plane, but that's only because set_config both > > disables the crtc _and_ the primary plane. > > > > Without more detail of what's going on I have no idea what exactly you're > > trying to achieve here. > > -Daniel > > > --- > > > drivers/gpu/drm/drm_crtc.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index f0556e654116..d732cca4879f 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -101,12 +101,19 @@ EXPORT_SYMBOL(drm_crtc_from_index); > > > */ > > > int drm_crtc_force_disable(struct drm_crtc *crtc) > > > { > > > + struct drm_framebuffer *fb; > > > struct drm_mode_set set = { > > > .crtc = crtc, > > > }; > > > > > > WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); > > > > > > + if (crtc->cursor && crtc->cursor->fb && > > crtc->cursor->fb->internal) { > > > + fb = crtc->cursor->fb; > > > + drm_plane_force_disable(crtc->cursor); > > > + drm_framebuffer_unreference(fb); > > > + } > > > + > > > return drm_mode_set_config_internal(&set); > > > } > > > EXPORT_SYMBOL(drm_crtc_force_disable); > > > -- > > > 2.13.5 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > -- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-12-11 9:13 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-07 3:01 [PATCH 2/2] drm: clean up internally created framebuffer on CRTC disable Gurchetan Singh 2017-12-07 9:16 ` Daniel Vetter 2017-12-08 22:17 ` Gurchetan Singh 2017-12-11 9:13 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.