Even for legacy userspace, since otherwise GETFB2 is broken and if you switch between modifier-less and modifier-aware compositors, smooth transitions break.
Also it's just best practice to make sure modifiers are invariant for a given drm_fb, and that a modifier-aware kms drivers only has one place to store them, ignoring any old implicit bo flags or whatever else might float around.
Motivated by some irc discussion with Bas about amdgpu modifier support.
Acked-by: Simon Ser contact@emersion.fr Acked-by: Daniel Stone daniels@collabora.com Acked-by: Pekka Paalanen pekka.paalanen@collabora.com Fixes: 455e00f1412f ("drm: Add getfb2 ioctl") Cc: Daniel Stone daniels@collabora.com Cc: Juston Li juston.li@intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl Cc: Marek Olšák maraeo@gmail.com Cc: "Wentland, Harry" harry.wentland@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- Sending this out again to double-check there's no objections or concerns.
From what I understand looking at the code the latest modifier patches for
amdgpu follow this now.
Cheers, Daniel --- include/drm/drm_mode_config.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index a18f73eb3cf6..5ffbb4ed5b35 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -58,6 +58,12 @@ struct drm_mode_config_funcs { * actual modifier used if the request doesn't have it specified, * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0. * + * IMPORTANT: These implied modifiers for legacy userspace must be + * stored in struct &drm_framebuffer, including all relevant metadata + * like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the + * modifier enables additional planes beyond the fourcc pixel format + * code. This is required by the GETFB2 ioctl. + * * If the parameters are deemed valid and the backing storage objects in * the underlying memory manager all exist, then the driver allocates * a new &drm_framebuffer structure, subclassed to contain @@ -915,6 +921,13 @@ struct drm_mode_config { * @allow_fb_modifiers: * * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call. + * + * IMPORTANT: + * + * If this is set the driver must fill out the full implicit modifier + * information in their &drm_mode_config_funcs.fb_create hook for legacy + * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is + * broken for modifier aware userspace. */ bool allow_fb_modifiers;
Acked-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl
On Thu, Sep 17, 2020 at 6:48 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
Even for legacy userspace, since otherwise GETFB2 is broken and if you switch between modifier-less and modifier-aware compositors, smooth transitions break.
Also it's just best practice to make sure modifiers are invariant for a given drm_fb, and that a modifier-aware kms drivers only has one place to store them, ignoring any old implicit bo flags or whatever else might float around.
Motivated by some irc discussion with Bas about amdgpu modifier support.
Acked-by: Simon Ser contact@emersion.fr Acked-by: Daniel Stone daniels@collabora.com Acked-by: Pekka Paalanen pekka.paalanen@collabora.com Fixes: 455e00f1412f ("drm: Add getfb2 ioctl") Cc: Daniel Stone daniels@collabora.com Cc: Juston Li juston.li@intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl Cc: Marek Olšák maraeo@gmail.com Cc: "Wentland, Harry" harry.wentland@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Sending this out again to double-check there's no objections or concerns. From what I understand looking at the code the latest modifier patches for amdgpu follow this now.
Cheers, Daniel
include/drm/drm_mode_config.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index a18f73eb3cf6..5ffbb4ed5b35 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -58,6 +58,12 @@ struct drm_mode_config_funcs { * actual modifier used if the request doesn't have it specified, * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0. *
* IMPORTANT: These implied modifiers for legacy userspace must be* stored in struct &drm_framebuffer, including all relevant metadata* like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the* modifier enables additional planes beyond the fourcc pixel format* code. This is required by the GETFB2 ioctl.* * If the parameters are deemed valid and the backing storage objects in * the underlying memory manager all exist, then the driver allocates * a new &drm_framebuffer structure, subclassed to contain@@ -915,6 +921,13 @@ struct drm_mode_config { * @allow_fb_modifiers: * * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
** IMPORTANT:** If this is set the driver must fill out the full implicit modifier* information in their &drm_mode_config_funcs.fb_create hook for legacy* userspace which does not set modifiers. Otherwise the GETFB2 ioctl is* broken for modifier aware userspace. */ bool allow_fb_modifiers;-- 2.28.0
On Thu, Sep 17, 2020 at 07:24:58PM +0200, Bas Nieuwenhuizen wrote:
Acked-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl
On Thu, Sep 17, 2020 at 6:48 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:
Even for legacy userspace, since otherwise GETFB2 is broken and if you switch between modifier-less and modifier-aware compositors, smooth transitions break.
Also it's just best practice to make sure modifiers are invariant for a given drm_fb, and that a modifier-aware kms drivers only has one place to store them, ignoring any old implicit bo flags or whatever else might float around.
Motivated by some irc discussion with Bas about amdgpu modifier support.
Acked-by: Simon Ser contact@emersion.fr Acked-by: Daniel Stone daniels@collabora.com Acked-by: Pekka Paalanen pekka.paalanen@collabora.com Fixes: 455e00f1412f ("drm: Add getfb2 ioctl") Cc: Daniel Stone daniels@collabora.com Cc: Juston Li juston.li@intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl Cc: Marek Olšák maraeo@gmail.com Cc: "Wentland, Harry" harry.wentland@amd.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
Pushed to drm-misc-next, thanks for all the feedback. -Daniel
Sending this out again to double-check there's no objections or concerns. From what I understand looking at the code the latest modifier patches for amdgpu follow this now.
Cheers, Daniel
include/drm/drm_mode_config.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index a18f73eb3cf6..5ffbb4ed5b35 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -58,6 +58,12 @@ struct drm_mode_config_funcs { * actual modifier used if the request doesn't have it specified, * ie. when (@mode_cmd->flags & DRM_MODE_FB_MODIFIERS) == 0. *
* IMPORTANT: These implied modifiers for legacy userspace must be* stored in struct &drm_framebuffer, including all relevant metadata* like &drm_framebuffer.pitches and &drm_framebuffer.offsets if the* modifier enables additional planes beyond the fourcc pixel format* code. This is required by the GETFB2 ioctl.* * If the parameters are deemed valid and the backing storage objects in * the underlying memory manager all exist, then the driver allocates * a new &drm_framebuffer structure, subclassed to contain@@ -915,6 +921,13 @@ struct drm_mode_config { * @allow_fb_modifiers: * * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
** IMPORTANT:** If this is set the driver must fill out the full implicit modifier* information in their &drm_mode_config_funcs.fb_create hook for legacy* userspace which does not set modifiers. Otherwise the GETFB2 ioctl is* broken for modifier aware userspace. */ bool allow_fb_modifiers;-- 2.28.0
dri-devel@lists.freedesktop.org
-
Bas Nieuwenhuizen -
Daniel Vetter -
Daniel Vetter