Re: [PATCH RFC 2/2] drm: introduce CLOSEFB IOCTL

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- 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 --]