[PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Simon Ser]

This new ADDFB2 flag allows callers to mark a framebuffer as
"persistent", and no longer have RMFB semantics when the DRM
file is closed.

[1]: https://lore.kernel.org/dri-devel/YTJypepF1Hpc2YYT@reader/

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

I'm not sure this is enough, but posting this to get initial
feedback and allow to start e.g. Plymouth experiments. I'll
follow up with an IGT test soon.

 drivers/gpu/drm/drm_framebuffer.c |  6 ++++--
 include/uapi/drm/drm_mode.h       | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e9..9b398838e1f4 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -292,7 +292,8 @@ drm_internal_framebuffer_create(struct drm_device *dev,
 	struct drm_framebuffer *fb;
 	int ret;
 
-	if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
+	if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS |
+			 DRM_MODE_FB_PERSIST)) {
 		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
 		return ERR_PTR(-EINVAL);
 	}
@@ -789,7 +790,8 @@ void drm_fb_release(struct drm_file *priv)
 	 * at it any more.
 	 */
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-		if (drm_framebuffer_read_refcount(fb) > 1) {
+		if (drm_framebuffer_read_refcount(fb) > 1 &&
+		    !(fb->flags & DRM_MODE_FB_PERSIST)) {
 			list_move_tail(&fb->filp_head, &arg.fbs);
 		} else {
 			list_del_init(&fb->filp_head);
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e1e351682872..c7a7089ec31e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -662,6 +662,21 @@ struct drm_mode_fb_cmd {
 
 #define DRM_MODE_FB_INTERLACED	(1<<0) /* for interlaced framebuffers */
 #define DRM_MODE_FB_MODIFIERS	(1<<1) /* enables ->modifer[] */
+/**
+ * DRM_MODE_FB_PERSIST
+ *
+ * DRM framebuffers are normally implicitly removed when their owner closes the
+ * DRM FD. Passing this flag will make the framebuffer persistent: it will not
+ * be implicitly removed. This is useful to implement flicker-free transitions
+ * between two processes.
+ *
+ * This flag doesn't change the behavior of &DRM_IOCTL_MODE_RMFB.
+ *
+ * User-space should ensure the framebuffer doesn't expose any sensitive user
+ * information: persistent framebuffers can be read back by the next DRM
+ * master.
+ */
+#define DRM_MODE_FB_PERSIST (1 << 2)
 
 struct drm_mode_fb_cmd2 {
 	__u32 fb_id;
-- 
2.33.0

Re: [PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Daniel Vetter]

On Wed, Oct 6, 2021 at 5:19 PM Simon Ser <contact@emersion.fr> wrote:
> This new ADDFB2 flag allows callers to mark a framebuffer as
> "persistent", and no longer have RMFB semantics when the DRM
> file is closed.
>
> [1]: https://lore.kernel.org/dri-devel/YTJypepF1Hpc2YYT@reader/
>
> 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>
> ---
>
> I'm not sure this is enough, but posting this to get initial
> feedback and allow to start e.g. Plymouth experiments. I'll
> follow up with an IGT test soon.
>
>  drivers/gpu/drm/drm_framebuffer.c |  6 ++++--
>  include/uapi/drm/drm_mode.h       | 15 +++++++++++++++
>  2 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 07f5abc875e9..9b398838e1f4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -292,7 +292,8 @@ drm_internal_framebuffer_create(struct drm_device *dev,
>         struct drm_framebuffer *fb;
>         int ret;
>
> -       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
> +       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS |
> +                        DRM_MODE_FB_PERSIST)) {
>                 DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
>                 return ERR_PTR(-EINVAL);
>         }
> @@ -789,7 +790,8 @@ void drm_fb_release(struct drm_file *priv)
>          * at it any more.
>          */
>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> -               if (drm_framebuffer_read_refcount(fb) > 1) {
> +               if (drm_framebuffer_read_refcount(fb) > 1 &&
> +                   !(fb->flags & DRM_MODE_FB_PERSIST)) {
>                         list_move_tail(&fb->filp_head, &arg.fbs);
>                 } else {
>                         list_del_init(&fb->filp_head);
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index e1e351682872..c7a7089ec31e 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -662,6 +662,21 @@ struct drm_mode_fb_cmd {
>
>  #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */
>  #define DRM_MODE_FB_MODIFIERS  (1<<1) /* enables ->modifer[] */
> +/**
> + * DRM_MODE_FB_PERSIST
> + *
> + * DRM framebuffers are normally implicitly removed when their owner closes the
> + * DRM FD. Passing this flag will make the framebuffer persistent: it will not
> + * be implicitly removed. This is useful to implement flicker-free transitions
> + * between two processes.
> + *
> + * This flag doesn't change the behavior of &DRM_IOCTL_MODE_RMFB.
> + *
> + * User-space should ensure the framebuffer doesn't expose any sensitive user
> + * information: persistent framebuffers can be read back by the next DRM
> + * master.

Should probably explain here that the persistent fb stays around for
as long as it's still in use by a plane, but will disappear
automatically when it's no longer in active use.

Also I guess there was some discussion I've missed on why we exclude
rmfb from this (unlike the CLOSEFB thing robclark proposed ages ago).
The nice thing about closefb is that you can give it persistent
semantics retroactively, so don't need to re-wrap an gem_bo and do a
page flip when you quit.
-Daniel

> + */
> +#define DRM_MODE_FB_PERSIST (1 << 2)
>
>  struct drm_mode_fb_cmd2 {
>         __u32 fb_id;
> --
> 2.33.0
>
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5184 bytes --]

On Wed, 6 Oct 2021 21:24:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Oct 6, 2021 at 5:19 PM Simon Ser <contact@emersion.fr> wrote:
> > This new ADDFB2 flag allows callers to mark a framebuffer as
> > "persistent", and no longer have RMFB semantics when the DRM
> > file is closed.
> >
> > [1]: https://lore.kernel.org/dri-devel/YTJypepF1Hpc2YYT@reader/
> >
> > 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>
> > ---
> >
> > I'm not sure this is enough, but posting this to get initial
> > feedback and allow to start e.g. Plymouth experiments. I'll
> > follow up with an IGT test soon.
> >
> >  drivers/gpu/drm/drm_framebuffer.c |  6 ++++--
> >  include/uapi/drm/drm_mode.h       | 15 +++++++++++++++
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 07f5abc875e9..9b398838e1f4 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -292,7 +292,8 @@ drm_internal_framebuffer_create(struct drm_device *dev,
> >         struct drm_framebuffer *fb;
> >         int ret;
> >
> > -       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
> > +       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS |
> > +                        DRM_MODE_FB_PERSIST)) {
> >                 DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
> >                 return ERR_PTR(-EINVAL);
> >         }
> > @@ -789,7 +790,8 @@ void drm_fb_release(struct drm_file *priv)
> >          * at it any more.
> >          */
> >         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> > -               if (drm_framebuffer_read_refcount(fb) > 1) {
> > +               if (drm_framebuffer_read_refcount(fb) > 1 &&
> > +                   !(fb->flags & DRM_MODE_FB_PERSIST)) {
> >                         list_move_tail(&fb->filp_head, &arg.fbs);
> >                 } else {
> >                         list_del_init(&fb->filp_head);
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index e1e351682872..c7a7089ec31e 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -662,6 +662,21 @@ struct drm_mode_fb_cmd {
> >
> >  #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */
> >  #define DRM_MODE_FB_MODIFIERS  (1<<1) /* enables ->modifer[] */
> > +/**
> > + * DRM_MODE_FB_PERSIST
> > + *
> > + * DRM framebuffers are normally implicitly removed when their owner closes the
> > + * DRM FD. Passing this flag will make the framebuffer persistent: it will not
> > + * be implicitly removed. This is useful to implement flicker-free transitions
> > + * between two processes.
> > + *
> > + * This flag doesn't change the behavior of &DRM_IOCTL_MODE_RMFB.
> > + *
> > + * User-space should ensure the framebuffer doesn't expose any sensitive user
> > + * information: persistent framebuffers can be read back by the next DRM
> > + * master.  
> 
> Should probably explain here that the persistent fb stays around for
> as long as it's still in use by a plane, but will disappear
> automatically when it's no longer in active use.

Yes, I think that is an important detail.

> Also I guess there was some discussion I've missed on why we exclude
> rmfb from this (unlike the CLOSEFB thing robclark proposed ages ago).

What does that mean? Was the CLOSEFB proposal saying that doing an RMFB
on a persistent FB does not actually RM the FB? If so, what effect did
it have, did it only invalidate the userspace FB ID?

That is an interesting thought. Userspace would not need to
deliberately "leak" the FB ID, it could still RMFB it which feels more
clean to me.

What if persistence was a flag on RMFB instead? If userspace forgets to
call RMFB at all, then closing the device removes the FB and avoids
information leaks. Only by doing special RMFB would allow the FB to
remain after closing the device. That should also encourage userspace
to track their FBs better.

> The nice thing about closefb is that you can give it persistent
> semantics retroactively, so don't need to re-wrap an gem_bo and do a
> page flip when you quit.

When you quit, you are going to need to draw once more anyway to get
rid of any potentially sensitive information for sure, so the re-wrap
does not seem much extra to me. OTOH, I guess userspace code would be a
little simpler if it does not need to re-wrap (assuming the code
already keeps FB IDs around and does not re-ADDFB on every flip -
weston does this caching).

I think my order of favourites is:
 1. RMFB flag
 2. ioctl to set an existing FB as persistent
 3. ADDFB flag

They all seem workable to me.


Thanks,
pq

> -Daniel
> 
> > + */
> > +#define DRM_MODE_FB_PERSIST (1 << 2)
> >
> >  struct drm_mode_fb_cmd2 {
> >         __u32 fb_id;
> > --
> > 2.33.0
> >
> >  
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Hans de Goede]

Hi,

On 10/7/21 9:45 AM, Pekka Paalanen wrote:
> On Wed, 6 Oct 2021 21:24:44 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
>> On Wed, Oct 6, 2021 at 5:19 PM Simon Ser <contact@emersion.fr> wrote:
>>> This new ADDFB2 flag allows callers to mark a framebuffer as
>>> "persistent", and no longer have RMFB semantics when the DRM
>>> file is closed.
>>>
>>> [1]: https://lore.kernel.org/dri-devel/YTJypepF1Hpc2YYT@reader/
>>>
>>> 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>
>>> ---
>>>
>>> I'm not sure this is enough, but posting this to get initial
>>> feedback and allow to start e.g. Plymouth experiments. I'll
>>> follow up with an IGT test soon.
>>>
>>>  drivers/gpu/drm/drm_framebuffer.c |  6 ++++--
>>>  include/uapi/drm/drm_mode.h       | 15 +++++++++++++++
>>>  2 files changed, 19 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
>>> index 07f5abc875e9..9b398838e1f4 100644
>>> --- a/drivers/gpu/drm/drm_framebuffer.c
>>> +++ b/drivers/gpu/drm/drm_framebuffer.c
>>> @@ -292,7 +292,8 @@ drm_internal_framebuffer_create(struct drm_device *dev,
>>>         struct drm_framebuffer *fb;
>>>         int ret;
>>>
>>> -       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
>>> +       if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS |
>>> +                        DRM_MODE_FB_PERSIST)) {
>>>                 DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
>>>                 return ERR_PTR(-EINVAL);
>>>         }
>>> @@ -789,7 +790,8 @@ void drm_fb_release(struct drm_file *priv)
>>>          * at it any more.
>>>          */
>>>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
>>> -               if (drm_framebuffer_read_refcount(fb) > 1) {
>>> +               if (drm_framebuffer_read_refcount(fb) > 1 &&
>>> +                   !(fb->flags & DRM_MODE_FB_PERSIST)) {
>>>                         list_move_tail(&fb->filp_head, &arg.fbs);
>>>                 } else {
>>>                         list_del_init(&fb->filp_head);
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index e1e351682872..c7a7089ec31e 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -662,6 +662,21 @@ struct drm_mode_fb_cmd {
>>>
>>>  #define DRM_MODE_FB_INTERLACED (1<<0) /* for interlaced framebuffers */
>>>  #define DRM_MODE_FB_MODIFIERS  (1<<1) /* enables ->modifer[] */
>>> +/**
>>> + * DRM_MODE_FB_PERSIST
>>> + *
>>> + * DRM framebuffers are normally implicitly removed when their owner closes the
>>> + * DRM FD. Passing this flag will make the framebuffer persistent: it will not
>>> + * be implicitly removed. This is useful to implement flicker-free transitions
>>> + * between two processes.
>>> + *
>>> + * This flag doesn't change the behavior of &DRM_IOCTL_MODE_RMFB.
>>> + *
>>> + * User-space should ensure the framebuffer doesn't expose any sensitive user
>>> + * information: persistent framebuffers can be read back by the next DRM
>>> + * master.  
>>
>> Should probably explain here that the persistent fb stays around for
>> as long as it's still in use by a plane, but will disappear
>> automatically when it's no longer in active use.
> 
> Yes, I think that is an important detail.
> 
>> Also I guess there was some discussion I've missed on why we exclude
>> rmfb from this (unlike the CLOSEFB thing robclark proposed ages ago).
> 
> What does that mean? Was the CLOSEFB proposal saying that doing an RMFB
> on a persistent FB does not actually RM the FB? If so, what effect did
> it have, did it only invalidate the userspace FB ID?
> 
> That is an interesting thought. Userspace would not need to
> deliberately "leak" the FB ID, it could still RMFB it which feels more
> clean to me.
> 
> What if persistence was a flag on RMFB instead? If userspace forgets to
> call RMFB at all, then closing the device removes the FB and avoids
> information leaks. Only by doing special RMFB would allow the FB to
> remain after closing the device. That should also encourage userspace
> to track their FBs better.
> 
>> The nice thing about closefb is that you can give it persistent
>> semantics retroactively, so don't need to re-wrap an gem_bo and do a
>> page flip when you quit.
> 
> When you quit, you are going to need to draw once more anyway to get
> rid of any potentially sensitive information for sure, so the re-wrap
> does not seem much extra to me.

Right that was my though too.

> OTOH, I guess userspace code would be a
> little simpler if it does not need to re-wrap (assuming the code
> already keeps FB IDs around and does not re-ADDFB on every flip -
> weston does this caching).
> 
> I think my order of favourites is:
>  1. RMFB flag
>  2. ioctl to set an existing FB as persistent
>  3. ADDFB flag
> 
> They all seem workable to me.

I fully agree with the above.

Regards,

Hans

Re: [PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Simon Ser]

On Thursday, October 7th, 2021 at 10:10, Hans de Goede <hdegoede@redhat.com> wrote:

> > I think my order of favourites is:
> >  1. RMFB flag
> >  2. ioctl to set an existing FB as persistent
> >  3. ADDFB flag
> >
> > They all seem workable to me.
>
> I fully agree with the above.

Thanks for the feedback! It seems like everyone agrees Rob Clark's CLOSEFB is
the best solution. Sent a new RFC at [1].

[1]: https://lore.kernel.org/dri-devel/20211007131507.149734-1-contact@emersion.fr/T/