[PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Simon Ser]

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(+)

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.
+ *
+ * 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.
+ */
 #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
 
 /*
-- 
2.33.1

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Daniel Vetter]

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(+)
> 
> 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?

> + *
> + * 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.

> + */

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Simon Ser]

On Tuesday, November 9th, 2021 at 10:24, 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(+)
> >
> > 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?

The IOCTL zeroes out the arrays at the start, so should be fine.

handles will all be zero if user-space is unprivileged, so can't use them to
figure out the number of planes.

> > + *
> > + * 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.

See the commit message: the modifier array is zeroed, which means it's filled
with DRM_FORMAT_MOD_LINEAR instead of DRM_FORMAT_MOD_INVALID -- this as good as
undefined contents for me.

Since this is a pretty good footgun, maybe we should change this? But the docs
would still need to tell userspace to not look at the array because older
kernels will still exist.

> > + */
>
> 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.

Agreed, maybe will take a stab at this once this one is merged.

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Daniel Vetter]

On Tue, Nov 09, 2021 at 09:29:54AM +0000, Simon Ser wrote:
> On Tuesday, November 9th, 2021 at 10:24, 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(+)
> > >
> > > 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?
> 
> The IOCTL zeroes out the arrays at the start, so should be fine.
> 
> handles will all be zero if user-space is unprivileged, so can't use them to
> figure out the number of planes.

Yeah maybe just mention that unused/undefined fields are zeroed or
something like that.

> > > + *
> > > + * 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.
> 
> See the commit message: the modifier array is zeroed, which means it's filled
> with DRM_FORMAT_MOD_LINEAR instead of DRM_FORMAT_MOD_INVALID -- this as good as
> undefined contents for me.
> 
> Since this is a pretty good footgun, maybe we should change this? But the docs
> would still need to tell userspace to not look at the array because older
> kernels will still exist.

Hm I think that's maybe better to document in the struct, since the entire
"no modifier means implied modifier with driver-specific rules" thing
applies to both addfb and getfb. Maybe we should also merge Daniel's "how
to modifier" so that we could link at in-depth discussion of these topics
in all the places where that's needed.

> 
> > > + */
> >
> > 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.
> 
> Agreed, maybe will take a stab at this once this one is merged.

With a line about "undefined/unused stuff is cleared to zero" added this
is:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Pekka Paalanen]

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

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Daniel Stone]

On Tue, 9 Nov 2021 at 08:56, Simon Ser <contact@emersion.fr> 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.

In fairness it is perfectly clear, it's just that I never considered
calling it without master/admin because why would you ever do that?
Anyway, the docs look correct and the more the better for sure, so
this is:
Acked-by: Daniel Stone <daniels@collabora.com>

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Simon Ser]

On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote:

> In fairness it is perfectly clear, it's just that I never considered
> calling it without master/admin because why would you ever do that?

FWIW, drm_info does it to display the current buffers metadata. Pretty
useful when debugging.

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Daniel Vetter]

On Mon, Nov 15, 2021 at 12:09:47PM +0000, Simon Ser wrote:
> On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote:
> 
> > In fairness it is perfectly clear, it's just that I never considered
> > calling it without master/admin because why would you ever do that?
> 
> FWIW, drm_info does it to display the current buffers metadata. Pretty
> useful when debugging.

We also have the state debugfs file for this, which should dump everything
we can decode. If it doesn't we should extend it there ideally, instead of
hand-rolling some tool that users/devs then don't have handy. debugfs
tends to be there (at least for developers).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2

[Simon Ser]

On Monday, November 15th, 2021 at 15:20, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Nov 15, 2021 at 12:09:47PM +0000, Simon Ser wrote:
> > On Thursday, November 11th, 2021 at 12:50, Daniel Stone <daniel@fooishbar.org> wrote:
> >
> > > In fairness it is perfectly clear, it's just that I never considered
> > > calling it without master/admin because why would you ever do that?
> >
> > FWIW, drm_info does it to display the current buffers metadata. Pretty
> > useful when debugging.
>
> We also have the state debugfs file for this, which should dump everything
> we can decode. If it doesn't we should extend it there ideally, instead of
> hand-rolling some tool that users/devs then don't have handy. debugfs
> tends to be there (at least for developers).

The debugfs output isn't as nice as drm_info, and can't produce structured
output like JSON.