On Tue, 9 Nov 2021 10:24:33 +0100 Daniel Vetter <dan...@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 <cont...@emersion.fr>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > Cc: Pekka Paalanen <ppaala...@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.paala...@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
> >
> >
>
pgp3TtZEIQKsC.pgp
Description: OpenPGP digital signature
