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.

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