[PATCH RFC] drm: introduce DRM_MODE_FB_PERSIST

[Pekka Paalanen]

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

> On Wed, Oct 6, 2021 at 5:19 PM Simon Ser <contact at 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 at emersion.fr>
> > Cc: Hans de Goede <hdegoede at redhat.com>
> > Cc: Dennis Filder <d.filder at web.de>
> > Cc: Daniel Vetter <daniel at ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen at 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
> >
> >  
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20211007/69aa6841/attachment.sig>