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

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