* [PATCH RFC 1/2] drm: extract closefb logic in separate function @ 2021-10-07 13:15 Simon Ser 2021-10-07 13:15 ` [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL Simon Ser 0 siblings, 1 reply; 4+ messages in thread From: Simon Ser @ 2021-10-07 13:15 UTC (permalink / raw) To: dri-devel Cc: Hans de Goede, Dennis Filder, Daniel Vetter, Pekka Paalanen, Rob Clark, Sean Paul drm_mode_rmfb performs two operations: drop the FB from the file_priv->fbs list, and make sure the FB is no longer used on a plane. In the next commit an IOCTL which only does so former will be introduced, so let's split it into a separate function. No functional change, only refactoring. Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Dennis Filder <d.filder@web.de> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Pekka Paalanen <ppaalanen@gmail.com> Cc: Rob Clark <robdclark@gmail.com> Cc: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_framebuffer.c | 51 +++++++++++++++++++------------ 1 file changed, 31 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 07f5abc875e9..2352972ba6ac 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -412,6 +412,31 @@ static void drm_mode_rmfb_work_fn(struct work_struct *w) } } +static int drm_mode_closefb(struct drm_framebuffer *fb, + struct drm_file *file_priv) +{ + struct drm_framebuffer *fbl = NULL; + bool found = false; + + mutex_lock(&file_priv->fbs_lock); + list_for_each_entry(fbl, &file_priv->fbs, filp_head) + if (fb == fbl) + found = true; + + if (!found) { + mutex_unlock(&file_priv->fbs_lock); + return -ENOENT; + } + + list_del_init(&fb->filp_head); + mutex_unlock(&file_priv->fbs_lock); + + /* Drop the reference that was stored in the fbs list */ + drm_framebuffer_put(fb); + + return 0; +} + /** * drm_mode_rmfb - remove an FB from the configuration * @dev: drm device @@ -429,8 +454,7 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id, struct drm_file *file_priv) { struct drm_framebuffer *fb = NULL; - struct drm_framebuffer *fbl = NULL; - int found = 0; + int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; @@ -439,23 +463,14 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id, if (!fb) return -ENOENT; - mutex_lock(&file_priv->fbs_lock); - list_for_each_entry(fbl, &file_priv->fbs, filp_head) - if (fb == fbl) - found = 1; - if (!found) { - mutex_unlock(&file_priv->fbs_lock); - goto fail_unref; + ret = drm_mode_closefb(fb, file_priv); + if (ret != 0) { + drm_framebuffer_put(fb); + return ret; } - list_del_init(&fb->filp_head); - mutex_unlock(&file_priv->fbs_lock); - - /* drop the reference we picked up in framebuffer lookup */ - drm_framebuffer_put(fb); - /* - * we now own the reference that was stored in the fbs list + * We now own the reference we picked up in drm_framebuffer_lookup. * * drm_framebuffer_remove may fail with -EINTR on pending signals, * so run this in a separate stack as there's no way to correctly @@ -475,10 +490,6 @@ int drm_mode_rmfb(struct drm_device *dev, u32 fb_id, drm_framebuffer_put(fb); return 0; - -fail_unref: - drm_framebuffer_put(fb); - return -ENOENT; } int drm_mode_rmfb_ioctl(struct drm_device *dev, -- 2.33.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL 2021-10-07 13:15 [PATCH RFC 1/2] drm: extract closefb logic in separate function Simon Ser @ 2021-10-07 13:15 ` Simon Ser 2021-10-08 7:29 ` Pekka Paalanen 0 siblings, 1 reply; 4+ messages in thread From: Simon Ser @ 2021-10-07 13:15 UTC (permalink / raw) To: dri-devel Cc: Hans de Goede, Dennis Filder, Daniel Vetter, Pekka Paalanen, Rob Clark, Sean Paul This new IOCTL allows callers to close a framebuffer without disabling planes or CRTCs. This takes inspiration from Rob Clark's unref_fb IOCTL [1] and DRM_MODE_FB_PERSIST [2]. [1]: https://lore.kernel.org/dri-devel/20170509153654.23464-1-robdclark@gmail.com/ [2]: https://lore.kernel.org/dri-devel/20211006151921.312714-1-contact@emersion.fr/ Signed-off-by: Simon Ser <contact@emersion.fr> Cc: Hans de Goede <hdegoede@redhat.com> Cc: Dennis Filder <d.filder@web.de> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Pekka Paalanen <ppaalanen@gmail.com> Cc: Rob Clark <robdclark@gmail.com> Cc: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_crtc_internal.h | 2 ++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++++++++++++++++ drivers/gpu/drm/drm_ioctl.c | 1 + include/uapi/drm/drm.h | 19 +++++++++++++++++++ 4 files changed, 41 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index 63279e984342..c6377b9ba981 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -221,6 +221,8 @@ int drm_mode_addfb2_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_rmfb_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_mode_closefb_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv); int drm_mode_getfb(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_getfb2_ioctl(struct drm_device *dev, diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 2352972ba6ac..15f7c22dbd4a 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -500,6 +500,25 @@ int drm_mode_rmfb_ioctl(struct drm_device *dev, return drm_mode_rmfb(dev, *fb_id, file_priv); } +int drm_mode_closefb_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + uint32_t *fb_id = data; + struct drm_framebuffer *fb; + int ret; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EOPNOTSUPP; + + fb = drm_framebuffer_lookup(dev, file_priv, *fb_id); + if (!fb) + return -ENOENT; + + ret = drm_mode_closefb(fb, file_priv); + drm_framebuffer_put(fb); + return ret; +} + /** * drm_mode_getfb - get FB info * @dev: drm device for the ioctl diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 8b8744dcf691..545762bc16d0 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -670,6 +670,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb_ioctl, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2_ioctl, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb_ioctl, 0), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CLOSEFB, drm_mode_closefb_ioctl, 0), DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, 0), diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 3b810b53ba8b..8726f003f382 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -1097,6 +1097,25 @@ extern "C" { #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) #define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCE, struct drm_mode_fb_cmd2) +/** + * DRM_IOCTL_MODE_CLOSEFB - Close a framebuffer. + * + * This closes a framebuffer previously added via ADDFB/ADDFB2. The IOCTL + * argument is a framebuffer object ID. + * + * This IOCTL is similar to &DRM_IOCTL_MODE_RMFB, except it doesn't disable + * planes and CRTCs. As long as the framebuffer is used by a plane, it's kept + * alive. When the plane no longer uses the framebuffer (because the + * framebuffer is replaced with another one, or the plane is disabled), the + * framebuffer is cleaned up. + * + * This is useful to implement flicker-free transitions between two processes. + * + * Depending on the threat model, user-space may want to ensure that the + * framebuffer doesn't expose any sensitive user information: closed + * framebuffers attached to a plane can be read back by the next DRM master. + */ +#define DRM_IOCTL_MODE_CLOSEFB DRM_IOWR(0xCF, unsigned int) /* * Device specific ioctls should only be in their respective headers -- 2.33.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL 2021-10-07 13:15 ` [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL Simon Ser @ 2021-10-08 7:29 ` Pekka Paalanen 2021-10-18 8:52 ` Simon Ser 0 siblings, 1 reply; 4+ messages in thread From: Pekka Paalanen @ 2021-10-08 7:29 UTC (permalink / raw) To: Simon Ser Cc: dri-devel, Hans de Goede, Dennis Filder, Daniel Vetter, Rob Clark, Sean Paul [-- Attachment #1: Type: text/plain, Size: 3534 bytes --] On Thu, 07 Oct 2021 13:15:25 +0000 Simon Ser <contact@emersion.fr> wrote: > This new IOCTL allows callers to close a framebuffer without > disabling planes or CRTCs. This takes inspiration from Rob Clark's > unref_fb IOCTL [1] and DRM_MODE_FB_PERSIST [2]. > > [1]: https://lore.kernel.org/dri-devel/20170509153654.23464-1-robdclark@gmail.com/ > [2]: https://lore.kernel.org/dri-devel/20211006151921.312714-1-contact@emersion.fr/ > > Signed-off-by: Simon Ser <contact@emersion.fr> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Dennis Filder <d.filder@web.de> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Pekka Paalanen <ppaalanen@gmail.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Sean Paul <seanpaul@chromium.org> > --- > drivers/gpu/drm/drm_crtc_internal.h | 2 ++ > drivers/gpu/drm/drm_framebuffer.c | 19 +++++++++++++++++++ > drivers/gpu/drm/drm_ioctl.c | 1 + > include/uapi/drm/drm.h | 19 +++++++++++++++++++ > 4 files changed, 41 insertions(+) ... > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index 8b8744dcf691..545762bc16d0 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -670,6 +670,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb_ioctl, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB2, drm_mode_addfb2_ioctl, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb_ioctl, 0), > + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CLOSEFB, drm_mode_closefb_ioctl, 0), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, 0), > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 3b810b53ba8b..8726f003f382 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -1097,6 +1097,25 @@ extern "C" { > #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct drm_syncobj_timeline_array) > > #define DRM_IOCTL_MODE_GETFB2 DRM_IOWR(0xCE, struct drm_mode_fb_cmd2) > +/** > + * DRM_IOCTL_MODE_CLOSEFB - Close a framebuffer. > + * > + * This closes a framebuffer previously added via ADDFB/ADDFB2. The IOCTL > + * argument is a framebuffer object ID. > + * > + * This IOCTL is similar to &DRM_IOCTL_MODE_RMFB, except it doesn't disable > + * planes and CRTCs. As long as the framebuffer is used by a plane, it's kept > + * alive. When the plane no longer uses the framebuffer (because the > + * framebuffer is replaced with another one, or the plane is disabled), the > + * framebuffer is cleaned up. > + * > + * This is useful to implement flicker-free transitions between two processes. > + * > + * Depending on the threat model, user-space may want to ensure that the > + * framebuffer doesn't expose any sensitive user information: closed > + * framebuffers attached to a plane can be read back by the next DRM master. > + */ LGTM! Semantics Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> > +#define DRM_IOCTL_MODE_CLOSEFB DRM_IOWR(0xCF, unsigned int) Should it have a structure with 'flags' for future-proofing? ISTR some rule of thumb that everything new must have 'flags' field enforced to be zero just in case. I only now saw that RMFB cannot have flags. Thanks, pq > > /* > * Device specific ioctls should only be in their respective headers [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL 2021-10-08 7:29 ` Pekka Paalanen @ 2021-10-18 8:52 ` Simon Ser 0 siblings, 0 replies; 4+ messages in thread From: Simon Ser @ 2021-10-18 8:52 UTC (permalink / raw) To: Pekka Paalanen Cc: dri-devel, Hans de Goede, Dennis Filder, Daniel Vetter, Rob Clark, Sean Paul Thanks for having a look at this patch, Pekka! On Friday, October 8th, 2021 at 09:29, Pekka Paalanen <ppaalanen@gmail.com> wrote: > > +#define DRM_IOCTL_MODE_CLOSEFB DRM_IOWR(0xCF, unsigned int) > > Should it have a structure with 'flags' for future-proofing? > > ISTR some rule of thumb that everything new must have 'flags' field > enforced to be zero just in case. I only now saw that RMFB cannot have > flags. I wonder. I don't mind adding a struct with flags or padding, not sure what the policy is for the DRM subsystem. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-18 8:52 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-07 13:15 [PATCH RFC 1/2] drm: extract closefb logic in separate function Simon Ser 2021-10-07 13:15 ` [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL Simon Ser 2021-10-08 7:29 ` Pekka Paalanen 2021-10-18 8:52 ` Simon Ser
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox