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