Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Tue, 9 Nov 2021 10:24:33 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Nov 09, 2021 at 08:56:10AM +0000, Simon Ser wrote:
> > There are a few details specific to the GETFB2 IOCTL.
> > 
> > It's not immediately clear how user-space should check for the
> > number of planes. Suggest using the pitches field.
> > 
> > The modifier array is filled with zeroes, ie. DRM_FORMAT_MOD_LINEAR.
> > So explicitly tell user-space to not look at it unless the flag is
> > set.
> > 
> > Signed-off-by: Simon Ser <contact@emersion.fr>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Pekka Paalanen <ppaalanen@gmail.com>
> > ---
> >  include/uapi/drm/drm.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)

Hi Simon,

Regardless of your debate with danvet, this sounds fine to me, so

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


> > 
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 3b810b53ba8b..9809078b0f51 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -1096,6 +1096,22 @@ extern "C" {
> >  #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
> >  #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
> >  
> > +/**
> > + * DRM_IOCTL_MODE_GETFB2 - Get framebuffer metadata.
> > + *
> > + * This queries metadata about a framebuffer. User-space fills
> > + * &drm_mode_fb_cmd2.fb_id as the input, and the kernels fills the rest of the
> > + * struct as the output.
> > + *
> > + * If the client is not DRM master and doesn't have &CAP_SYS_ADMIN,
> > + * &drm_mode_fb_cmd2.handles will be zeroed. Planes are valid until one has a
> > + * zero &drm_mode_fb_cmd2.pitches -- this can be used to compute the number of
> > + * planes.  
> 
> Maybe explain that when actually importing the buffer userspace should
> look for non-zeor handles instead, since some drivers/formats leave a
> silly pitch value behind. Or at least I think that can happen. Just for
> additional robustness?

Sounds like pitch is literally undefined sometimes as well. What else
would one call "silly" than undefined. Maybe implementation defined,
but that's not better.

> 
> > + *
> > + * If the framebuffer has a format modifier, &DRM_MODE_FB_MODIFIERS will be set
> > + * in &drm_mode_fb_cmd2.flags. Otherwise, &drm_mode_fb_cmd2.modifier has
> > + * undefined contents.  
> 
> Uh is this true? We should always clear values to avoid accidental leaks
> and stuff.

This is userspace facing documentation, so saying "is undefined" is
perfectly fine. It just means "expect garbage here, so don't even
look", not that the kernel must literally leave that memory
uninitialized.


Thanks,
pq

> 
> > + */  
> 
> I think kerneldoc for drm_mode_fb_cmd2 would be neat too, so we can
> document how pitch is supposed to work and all that stuff.
> 
> Anyway lgtm otherwise.
> -Daniel
> 
> >  #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
> >  
> >  /*
> > -- 
> > 2.33.1
> > 
> >   
> 


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