diff mbox series

drm/doc: Document that modifiers are always required for fb

Message ID 20200917164721.2038541-1-daniel.vetter@ffwll.ch
State New, archived
Headers show
Series drm/doc: Document that modifiers are always required for fb | expand

Commit Message

Daniel Vetter Sept. 17, 2020, 4:47 p.m. UTC
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(+)

Comments

Bas Nieuwenhuizen Sept. 17, 2020, 5:24 p.m. UTC | #1
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
>
Daniel Vetter Sept. 23, 2020, 3:53 p.m. UTC | #2
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
> >
diff mbox series

Patch

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;