[8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS

Commit Message

Daniel Vetter April 27, 2021, 9:20 a.m. UTC

It's very confusing for userspace to have to deal with inconsistencies
here, and some drivers screwed this up a bit. Most just ommitted the
format list when they meant to say that only linear modifier is
allowed, but some also meant that only implied modifiers are
acceptable (because actually none of the planes registered supported
modifiers).

Now that this is all done consistently across all drivers, document
the rules and enforce it in the drm core.

v2:
- Make the capability a link (Simon)
- Note that all is lost before 5.1.

Acked-by: Maxime Ripard <[e-mail]>
Cc: Simon Ser <[e-mail]>
Reviewed-by: Lucas Stach <[e-mail]>
Cc: Pekka Paalanen <[e-mail]>
Signed-off-by: Daniel Vetter <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
---
 drivers/gpu/drm/drm_plane.c   | 18 +++++++++++++++++-
 include/drm/drm_mode_config.h |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

Comments

Simon Ser April 27, 2021, 9:30 a.m. UTC | #1

Reviewed-by: Simon Ser <[e-mail]>

Emil Velikov April 27, 2021, 11:32 a.m. UTC | #2

Hi Daniel,

On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <[e-mail]> wrote:

> @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   * drm_universal_plane_init() to let the DRM managed resource infrastructure
>   * take care of cleanup and deallocation.
>   *
> + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> + * even those that only support DRM_FORMAT_MOD_LINEAR.
> + *
The comment says "must", yet we have an "if (format_modifiers)" in the codebase.
Shouldn't we add a WARN_ON() + return -EINVAL (or similar) so people
can see and fix their drivers?

As a follow-up one could even go a step further, by erroring out when
the driver hasn't provided valid modifier(s) and even removing
config::allow_fb_modifiers all together.

Although for stable - this series + WARN_ON (no return since it might
break buggy drivers) sounds good.

> @@ -909,6 +909,8 @@ struct drm_mode_config {
>          * @allow_fb_modifiers:
>          *
>          * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> +        * Note that drivers should not set this directly, it is automatically
> +        * set in drm_universal_plane_init().
>          *
>          * IMPORTANT:
>          *
The new note and the existing IMPORTANT are in a weird mix.
Quoting the latter since it doesn't show in the diff.

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.

In particular:
As the new note says "don't set it" and the existing note one says "if
it's set". Yet no drivers do "if (config->allow_fb_modifiers)".

Sadly, nothing comes to mind atm wrt alternative wording.

With the WARN_ON() added or s/must/should/ in the documentation, the series is:
Reviewed-by: Emil Velikov <[e-mail]>

HTH
-Emil

Daniel Vetter April 27, 2021, 12:22 p.m. UTC | #3

On Tue, Apr 27, 2021 at 12:32:19PM +0100, Emil Velikov wrote:
> Hi Daniel,
> 
> On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <[e-mail]> wrote:
> 
> > @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
> >   * drm_universal_plane_init() to let the DRM managed resource infrastructure
> >   * take care of cleanup and deallocation.
> >   *
> > + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> > + * even those that only support DRM_FORMAT_MOD_LINEAR.
> > + *
> The comment says "must", yet we have an "if (format_modifiers)" in the codebase.
> Shouldn't we add a WARN_ON() + return -EINVAL (or similar) so people
> can see and fix their drivers?

This is a must only for drivers supporting modifiers, not all drivers.
Hence the check in the if. I did add WARN_ON for the combos that get stuff
wrong though (like only supply one side of the modifier info, not both).

> As a follow-up one could even go a step further, by erroring out when
> the driver hasn't provided valid modifier(s) and even removing
> config::allow_fb_modifiers all together.

Well that currently only exists to avoid walking the plane list (which we
need to do for validation that all planes are the same). It's quite tricky
code for tiny benefit, so I don't think it's worth it trying to remove
allow_fb_modifiers completely.

> Although for stable - this series + WARN_ON (no return since it might
> break buggy drivers) sounds good.
> 
> > @@ -909,6 +909,8 @@ struct drm_mode_config {
> >          * @allow_fb_modifiers:
> >          *
> >          * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > +        * Note that drivers should not set this directly, it is automatically
> > +        * set in drm_universal_plane_init().
> >          *
> >          * IMPORTANT:
> >          *
> The new note and the existing IMPORTANT are in a weird mix.
> Quoting the latter since it doesn't show in the diff.
> 
> 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.
> 
> In particular:
> As the new note says "don't set it" and the existing note one says "if
> it's set". Yet no drivers do "if (config->allow_fb_modifiers)".
> 
> Sadly, nothing comes to mind atm wrt alternative wording.

Yeah it's a bit disappointing.

> With the WARN_ON() added or s/must/should/ in the documentation, the series is:

With my clarification, can you please recheck whether as-is it's not
correct?

Thanks, Daniel

> Reviewed-by: Emil Velikov <[e-mail]>
> 
> HTH
> -Emil

Pekka Paalanen May 4, 2021, 1:38 p.m. UTC | #4

On Tue, 27 Apr 2021 11:20:18 +0200
Daniel Vetter <[e-mail]> wrote:

> It's very confusing for userspace to have to deal with inconsistencies
> here, and some drivers screwed this up a bit. Most just ommitted the
> format list when they meant to say that only linear modifier is
> allowed, but some also meant that only implied modifiers are
> acceptable (because actually none of the planes registered supported
> modifiers).
> 
> Now that this is all done consistently across all drivers, document
> the rules and enforce it in the drm core.
> 
> v2:
> - Make the capability a link (Simon)
> - Note that all is lost before 5.1.
> 
> Acked-by: Maxime Ripard <[e-mail]>
> Cc: Simon Ser <[e-mail]>
> Reviewed-by: Lucas Stach <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> ---
>  drivers/gpu/drm/drm_plane.c   | 18 +++++++++++++++++-
>  include/drm/drm_mode_config.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0dd43882fe7c..20c7a1665414 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -128,6 +128,13 @@
>   *     pairs supported by this plane. The blob is a struct
>   *     drm_format_modifier_blob. Without this property the plane doesn't
>   *     support buffers with modifiers. Userspace cannot change this property.
> + *
> + *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
> + *     capability for general modifier support. If this flag is set then every
> + *     plane will have the IN_FORMATS property, even when it only supports
> + *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
> + *     various bugs in this area with inconsistencies between the capability
> + *     flag and per-plane properties.
>   */
>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -277,8 +284,14 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>  			format_modifier_count++;
>  	}
>  
> -	if (format_modifier_count)
> +	/* autoset the cap and check for consistency across all planes */
> +	if (format_modifier_count) {
> +		WARN_ON(!config->allow_fb_modifiers &&
> +			!list_empty(&config->plane_list));
>  		config->allow_fb_modifiers = true;
> +	} else {
> +		WARN_ON(config->allow_fb_modifiers);
> +	}
>  
>  	plane->modifier_count = format_modifier_count;
>  	plane->modifiers = kmalloc_array(format_modifier_count,
> @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   * drm_universal_plane_init() to let the DRM managed resource infrastructure
>   * take care of cleanup and deallocation.
>   *
> + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> + * even those that only support DRM_FORMAT_MOD_LINEAR.
> + *
>   * Returns:
>   * Zero on success, error code on failure.
>   */
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..1ddf7783fdf7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -909,6 +909,8 @@ struct drm_mode_config {
>  	 * @allow_fb_modifiers:
>  	 *
>  	 * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> +	 * Note that drivers should not set this directly, it is automatically
> +	 * set in drm_universal_plane_init().
>  	 *
>  	 * IMPORTANT:
>  	 *

I can only say about the doc parts, but:

Acked-by: Pekka Paalanen <[e-mail]>

For patches 2 and 5 too, on the grounds that the idea is good.


Thanks,
pq

Emil Velikov May 4, 2021, 2:10 p.m. UTC | #5

Hi Daniel,

Thanks for the extra clarification.

On Tue, 27 Apr 2021 at 13:22, Daniel Vetter <[e-mail]> wrote:
>
> On Tue, Apr 27, 2021 at 12:32:19PM +0100, Emil Velikov wrote:
> > Hi Daniel,
> >
> > On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <[e-mail]> wrote:
> >
> > > @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
> > >   * drm_universal_plane_init() to let the DRM managed resource infrastructure
> > >   * take care of cleanup and deallocation.
> > >   *
> > > + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> > > + * even those that only support DRM_FORMAT_MOD_LINEAR.
> > > + *
> > The comment says "must", yet we have an "if (format_modifiers)" in the codebase.
> > Shouldn't we add a WARN_ON() + return -EINVAL (or similar) so people
> > can see and fix their drivers?
>
> This is a must only for drivers supporting modifiers, not all drivers.
> Hence the check in the if. I did add WARN_ON for the combos that get stuff
> wrong though (like only supply one side of the modifier info, not both).
>
Hmm you're spot on - the arm/malidp patch threw me off for a minute.

> > As a follow-up one could even go a step further, by erroring out when
> > the driver hasn't provided valid modifier(s) and even removing
> > config::allow_fb_modifiers all together.
>
> Well that currently only exists to avoid walking the plane list (which we
> need to do for validation that all planes are the same). It's quite tricky
> code for tiny benefit, so I don't think it's worth it trying to remove
> allow_fb_modifiers completely.
>
Pardon if I'm saying something painfully silly - it's been a while
since I've looked closely at KMS.

From some grepping around, removing ::allow_fb_modifiers would be OK
although it's a secondary goal. It feels like the bigger win will be
simpler modifier handling in DRM.

In particular, one could always "inject" the linear modifier within
drm_universal_plane_init() and always expose DRM_CAP_ADDFB2_MODIFIERS.
Some drivers mxsfb, mgag200, stm and likely others already advertise
the CAP, even though they seemingly lack any modifiers.

The linear/invalid cargo-cult to drm_universal_plane_init() seems
strong and this series adds even more.

Another plus of always exposing the CAP, is that one could mandate (or
nuke) optional .format_mod_supported that you/Ville discussed
earlier[1].
Currently things are weird, since it's required to create IN_FORMAT
blob, yet drivers lack it while simultaneously exposing the CAP to
userspace.

One such example is exynos... Although recently it recently dropped
`allow_fb_modifiers = true` and there are no modifiers passed to
drm_universal_plane_init(), so the CAP is no longer supported.
Inki you might want to check, if that broke your userspace.


Tl:Dr: There _might_ be value in simplifying the modifier handling
_after_ these fixes land.


[1] https://lore.kernel.org/dri-devel/[e-mail]/

> > Although for stable - this series + WARN_ON (no return since it might
> > break buggy drivers) sounds good.
> >
> > > @@ -909,6 +909,8 @@ struct drm_mode_config {
> > >          * @allow_fb_modifiers:
> > >          *
> > >          * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> > > +        * Note that drivers should not set this directly, it is automatically
> > > +        * set in drm_universal_plane_init().
> > >          *
> > >          * IMPORTANT:
> > >          *
> > The new note and the existing IMPORTANT are in a weird mix.
> > Quoting the latter since it doesn't show in the diff.
> >
> > 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.
> >
> > In particular:
> > As the new note says "don't set it" and the existing note one says "if
> > it's set". Yet no drivers do "if (config->allow_fb_modifiers)".
> >
> > Sadly, nothing comes to mind atm wrt alternative wording.
>
> Yeah it's a bit disappointing.
>
> > With the WARN_ON() added or s/must/should/ in the documentation, the series is:
>
> With my clarification, can you please recheck whether as-is it's not
> correct?
>
Indeed - with the series as-is my RB stands.

Thanks
-Emil

Simon Ser May 4, 2021, 2:58 p.m. UTC | #6

Continuing on that idea to push for enabling the cap in more cases: do
we have a policy to require new drivers to always support modifiers?

That would be nice, even if it's just about enabling LINEAR.

Emil Velikov May 4, 2021, 3:48 p.m. UTC | #7

On Tue, 4 May 2021 at 15:58, Simon Ser <[e-mail]> wrote:
>
> Continuing on that idea to push for enabling the cap in more cases: do
> we have a policy to require new drivers to always support modifiers?
>
> That would be nice, even if it's just about enabling LINEAR.

Sounds perfectly reasonable IMHO. I think we ought to document this
policy (requirement ideally) somewhere - say alongside the "all new
KMS drivers must support atomic modeset", which lives in ...

-Emil

Daniel Vetter May 5, 2021, 7:24 p.m. UTC | #8

On Tue, Apr 27, 2021 at 11:20 AM Daniel Vetter <[e-mail]> wrote:
>
> It's very confusing for userspace to have to deal with inconsistencies
> here, and some drivers screwed this up a bit. Most just ommitted the
> format list when they meant to say that only linear modifier is
> allowed, but some also meant that only implied modifiers are
> acceptable (because actually none of the planes registered supported
> modifiers).
>
> Now that this is all done consistently across all drivers, document
> the rules and enforce it in the drm core.
>
> v2:
> - Make the capability a link (Simon)
> - Note that all is lost before 5.1.
>
> Acked-by: Maxime Ripard <[e-mail]>
> Cc: Simon Ser <[e-mail]>
> Reviewed-by: Lucas Stach <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>

Lyude's irc review:

<Lyude> btw danvet (sorry I didn't reply in-line, for some reason I
can't actually seem to find the emails for your allow_fb_modifiers
series in my email client), I just looked over your allow_fb_modifiers
series and everything looks fine with one comment:
https://lore.kernel.org/dri-devel/[e-mail]/
we should probably use drm_WARN_ON() here instead
<Lyude> with that fixed: Reviewed-by: Lyude Paul <[e-mail]>

I'll merge the driver patches and respin this one afterwards.
-Daniel

> ---
>  drivers/gpu/drm/drm_plane.c   | 18 +++++++++++++++++-
>  include/drm/drm_mode_config.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0dd43882fe7c..20c7a1665414 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -128,6 +128,13 @@
>   *     pairs supported by this plane. The blob is a struct
>   *     drm_format_modifier_blob. Without this property the plane doesn't
>   *     support buffers with modifiers. Userspace cannot change this property.
> + *
> + *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
> + *     capability for general modifier support. If this flag is set then every
> + *     plane will have the IN_FORMATS property, even when it only supports
> + *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
> + *     various bugs in this area with inconsistencies between the capability
> + *     flag and per-plane properties.
>   */
>
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -277,8 +284,14 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>                         format_modifier_count++;
>         }
>
> -       if (format_modifier_count)
> +       /* autoset the cap and check for consistency across all planes */
> +       if (format_modifier_count) {
> +               WARN_ON(!config->allow_fb_modifiers &&
> +                       !list_empty(&config->plane_list));
>                 config->allow_fb_modifiers = true;
> +       } else {
> +               WARN_ON(config->allow_fb_modifiers);
> +       }
>
>         plane->modifier_count = format_modifier_count;
>         plane->modifiers = kmalloc_array(format_modifier_count,
> @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   * drm_universal_plane_init() to let the DRM managed resource infrastructure
>   * take care of cleanup and deallocation.
>   *
> + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> + * even those that only support DRM_FORMAT_MOD_LINEAR.
> + *
>   * Returns:
>   * Zero on success, error code on failure.
>   */
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..1ddf7783fdf7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -909,6 +909,8 @@ struct drm_mode_config {
>          * @allow_fb_modifiers:
>          *
>          * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> +        * Note that drivers should not set this directly, it is automatically
> +        * set in drm_universal_plane_init().
>          *
>          * IMPORTANT:
>          *
> --
> 2.31.0
>

Daniel Vetter May 6, 2021, 9:55 a.m. UTC | #9

On Tue, Apr 27, 2021 at 11:20:18AM +0200, Daniel Vetter wrote:
> It's very confusing for userspace to have to deal with inconsistencies
> here, and some drivers screwed this up a bit. Most just ommitted the
> format list when they meant to say that only linear modifier is
> allowed, but some also meant that only implied modifiers are
> acceptable (because actually none of the planes registered supported
> modifiers).
> 
> Now that this is all done consistently across all drivers, document
> the rules and enforce it in the drm core.
> 
> v2:
> - Make the capability a link (Simon)
> - Note that all is lost before 5.1.
> 
> Acked-by: Maxime Ripard <[e-mail]>
> Cc: Simon Ser <[e-mail]>
> Reviewed-by: Lucas Stach <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Signed-off-by: Daniel Vetter <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>

I merged all the driver patches to drm-misc-next, I'll resend v3 of this
one shortly.
-Daniel

> ---
>  drivers/gpu/drm/drm_plane.c   | 18 +++++++++++++++++-
>  include/drm/drm_mode_config.h |  2 ++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0dd43882fe7c..20c7a1665414 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -128,6 +128,13 @@
>   *     pairs supported by this plane. The blob is a struct
>   *     drm_format_modifier_blob. Without this property the plane doesn't
>   *     support buffers with modifiers. Userspace cannot change this property.
> + *
> + *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
> + *     capability for general modifier support. If this flag is set then every
> + *     plane will have the IN_FORMATS property, even when it only supports
> + *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
> + *     various bugs in this area with inconsistencies between the capability
> + *     flag and per-plane properties.
>   */
>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -277,8 +284,14 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>  			format_modifier_count++;
>  	}
>  
> -	if (format_modifier_count)
> +	/* autoset the cap and check for consistency across all planes */
> +	if (format_modifier_count) {
> +		WARN_ON(!config->allow_fb_modifiers &&
> +			!list_empty(&config->plane_list));
>  		config->allow_fb_modifiers = true;
> +	} else {
> +		WARN_ON(config->allow_fb_modifiers);
> +	}
>  
>  	plane->modifier_count = format_modifier_count;
>  	plane->modifiers = kmalloc_array(format_modifier_count,
> @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
>   * drm_universal_plane_init() to let the DRM managed resource infrastructure
>   * take care of cleanup and deallocation.
>   *
> + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> + * even those that only support DRM_FORMAT_MOD_LINEAR.
> + *
>   * Returns:
>   * Zero on success, error code on failure.
>   */
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index ab424ddd7665..1ddf7783fdf7 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -909,6 +909,8 @@ struct drm_mode_config {
>  	 * @allow_fb_modifiers:
>  	 *
>  	 * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> +	 * Note that drivers should not set this directly, it is automatically
> +	 * set in drm_universal_plane_init().
>  	 *
>  	 * IMPORTANT:
>  	 *
> -- 
> 2.31.0
>

Patch

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0dd43882fe7c..20c7a1665414 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -128,6 +128,13 @@ 
  *     pairs supported by this plane. The blob is a struct
  *     drm_format_modifier_blob. Without this property the plane doesn't
  *     support buffers with modifiers. Userspace cannot change this property.
+ *
+ *     Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
+ *     capability for general modifier support. If this flag is set then every
+ *     plane will have the IN_FORMATS property, even when it only supports
+ *     DRM_FORMAT_MOD_LINEAR. Before linux kernel release v5.1 there have been
+ *     various bugs in this area with inconsistencies between the capability
+ *     flag and per-plane properties.
  */
 
 static unsigned int drm_num_planes(struct drm_device *dev)
@@ -277,8 +284,14 @@  static int __drm_universal_plane_init(struct drm_device *dev,
 			format_modifier_count++;
 	}
 
-	if (format_modifier_count)
+	/* autoset the cap and check for consistency across all planes */
+	if (format_modifier_count) {
+		WARN_ON(!config->allow_fb_modifiers &&
+			!list_empty(&config->plane_list));
 		config->allow_fb_modifiers = true;
+	} else {
+		WARN_ON(config->allow_fb_modifiers);
+	}
 
 	plane->modifier_count = format_modifier_count;
 	plane->modifiers = kmalloc_array(format_modifier_count,
@@ -360,6 +373,9 @@  static int __drm_universal_plane_init(struct drm_device *dev,
  * drm_universal_plane_init() to let the DRM managed resource infrastructure
  * take care of cleanup and deallocation.
  *
+ * Drivers supporting modifiers must set @format_modifiers on all their planes,
+ * even those that only support DRM_FORMAT_MOD_LINEAR.
+ *
  * Returns:
  * Zero on success, error code on failure.
  */
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index ab424ddd7665..1ddf7783fdf7 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -909,6 +909,8 @@  struct drm_mode_config {
 	 * @allow_fb_modifiers:
 	 *
 	 * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
+	 * Note that drivers should not set this directly, it is automatically
+	 * set in drm_universal_plane_init().
 	 *
 	 * IMPORTANT:
 	 *