Hi, On 10/7/21 9:45 AM, Pekka Paalanen wrote: > On Wed, 6 Oct 2021 21:24:44 +0200 > Daniel Vetter <daniel@xxxxxxxx> wrote: > >> On Wed, Oct 6, 2021 at 5:19 PM Simon Ser <contact@xxxxxxxxxxx> 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@xxxxxxxxxxx> >>> Cc: Hans de Goede <hdegoede@xxxxxxxxxx> >>> Cc: Dennis Filder <d.filder@xxxxxx> >>> Cc: Daniel Vetter <daniel@xxxxxxxx> >>> Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx> >>> --- >>> >>> 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
