[PATCH 1/8] drm/arm: Don't set allow_fb_modifiers explicitly

[PATCH 1/8] drm/arm: Don't set allow_fb_modifiers explicitly

[Daniel Vetter]

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here for both komeda and
malidp.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "James (Qian) Wang" <james.qian.wang@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mihail Atanassov <mihail.atanassov@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 1 -
 drivers/gpu/drm/arm/malidp_drv.c                | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index aeda4e5ec4f4..ff45f23f3d56 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -247,7 +247,6 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
 	config->min_height	= 0;
 	config->max_width	= 4096;
 	config->max_height	= 4096;
-	config->allow_fb_modifiers = true;
 
 	config->funcs = &komeda_mode_config_funcs;
 	config->helper_private = &komeda_mode_config_helpers;
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d83c7366b348..de59f3302516 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -403,7 +403,6 @@ static int malidp_init(struct drm_device *drm)
 	drm->mode_config.max_height = hwdev->max_line_size;
 	drm->mode_config.funcs = &malidp_mode_config_funcs;
 	drm->mode_config.helper_private = &malidp_mode_config_helpers;
-	drm->mode_config.allow_fb_modifiers = true;
 
 	ret = malidp_crtc_init(drm);
 	if (ret)
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/arm/malidp: Always list modifiers

[Daniel Vetter]

Even when all we support is linear, make that explicit. Otherwise the
uapi is rather confusing.

Cc: stable@vger.kernel.org
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/arm/malidp_planes.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
index ddbba67f0283..8c2ab3d653b7 100644
--- a/drivers/gpu/drm/arm/malidp_planes.c
+++ b/drivers/gpu/drm/arm/malidp_planes.c
@@ -927,6 +927,11 @@ static const struct drm_plane_helper_funcs malidp_de_plane_helper_funcs = {
 	.atomic_disable = malidp_de_plane_disable,
 };
 
+static const uint64_t linear_only_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
 int malidp_de_planes_init(struct drm_device *drm)
 {
 	struct malidp_drm *malidp = drm->dev_private;
@@ -990,8 +995,8 @@ int malidp_de_planes_init(struct drm_device *drm)
 		 */
 		ret = drm_universal_plane_init(drm, &plane->base, crtcs,
 				&malidp_de_plane_funcs, formats, n,
-				(id == DE_SMART) ? NULL : modifiers, plane_type,
-				NULL);
+				(id == DE_SMART) ? linear_only_modifiers : modifiers,
+				plane_type, NULL);
 
 		if (ret < 0)
 			goto cleanup;
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/8] drm/i915: Don't set allow_fb_modifiers explicitly

[Daniel Vetter]

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Karthik B S <karthik.b.s@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4bbc81d8d649..d2c6959190ab 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -11731,8 +11731,6 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
 	mode_config->preferred_depth = 24;
 	mode_config->prefer_shadow = 1;
 
-	mode_config->allow_fb_modifiers = true;
-
 	mode_config->funcs = &intel_mode_funcs;
 
 	mode_config->async_page_flip = has_async_flips(i915);
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm/msm/dpu1: Don't set allow_fb_modifiers explicitly

[Daniel Vetter]

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here.

v2: Rebase.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Kalyan Thota <kalyant@codeaurora.org>
Cc: Jordan Crouse <jordan@cosmicpenguin.net>
Cc: Eric Anholt <eric@anholt.net>
Cc: Tanmay Shah <tanmay@codeaurora.org>
Cc: Rajendra Nayak <rnayak@codeaurora.org>
Cc: Jeykumar Sankaran <jsanka@codeaurora.org>
Cc: Qinglang Miao <miaoqinglang@huawei.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 88e9cc38c13b..93bc3575bf53 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1020,11 +1020,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 			dpu_kms->catalog->caps->max_mixer_width * 2;
 	dev->mode_config.max_height = 4096;
 
-	/*
-	 * Support format modifiers for compression etc.
-	 */
-	dev->mode_config.allow_fb_modifiers = true;
-
 	dev->max_vblank_count = 0xffffffff;
 	/* Disable vblank irqs aggressively for power-saving */
 	dev->vblank_disable_immediate = true;
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/8] drm/msm/mdp4: Fix modifier support enabling

[Daniel Vetter]

Setting the cap without the modifier list is very confusing to
userspace. Fix that by listing the ones we support explicitly.

Stable backport so that userspace can rely on this working in a
reasonable way, i.e. that the cap set implies IN_FORMATS is available.

Cc: stable@vger.kernel.org
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Rob Clark <robdclark@chromium.org>
Cc: Jordan Crouse <jordan@cosmicpenguin.net>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c   | 2 --
 drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c | 8 +++++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 3d729270bde1..4a5b518288b0 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -88,8 +88,6 @@ static int mdp4_hw_init(struct msm_kms *kms)
 	if (mdp4_kms->rev > 1)
 		mdp4_write(mdp4_kms, REG_MDP4_RESET_STATUS, 1);
 
-	dev->mode_config.allow_fb_modifiers = true;
-
 out:
 	pm_runtime_put_sync(dev->dev);
 
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
index 9aecca919f24..49bdabea8ed5 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_plane.c
@@ -349,6 +349,12 @@ enum mdp4_pipe mdp4_plane_pipe(struct drm_plane *plane)
 	return mdp4_plane->pipe;
 }
 
+static const uint64_t supported_format_modifiers[] = {
+	DRM_FORMAT_MOD_SAMSUNG_64_32_TILE,
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
 /* initialize plane */
 struct drm_plane *mdp4_plane_init(struct drm_device *dev,
 		enum mdp4_pipe pipe_id, bool private_plane)
@@ -377,7 +383,7 @@ struct drm_plane *mdp4_plane_init(struct drm_device *dev,
 	type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
 	ret = drm_universal_plane_init(dev, plane, 0xff, &mdp4_plane_funcs,
 				 mdp4_plane->formats, mdp4_plane->nformats,
-				 NULL, type, NULL);
+				 supported_format_modifiers, type, NULL);
 	if (ret)
 		goto fail;
 
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/8] drm/nouveau: Don't set allow_fb_modifiers explicitly

[Daniel Vetter]

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here.

Note that this fixes an inconsistency: We've set the cap everywhere,
but only nv50+ supports modifiers. Hence cc stable, but not further
back then the patch from Paul.

Cc: stable@vger.kernel.org # v5.1 +
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Ben Skeggs <bskeggs@redhat.com>
Cc: nouveau@lists.freedesktop.org
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 14101bd2a0ff..929de41c281f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -697,7 +697,6 @@ nouveau_display_create(struct drm_device *dev)
 
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.prefer_shadow = 1;
-	dev->mode_config.allow_fb_modifiers = true;
 
 	if (drm->client.device.info.chipset < 0x11)
 		dev->mode_config.async_page_flip = false;
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/8] drm/stm: Don't set allow_fb_modifiers explicitly

[Daniel Vetter]

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Yannick Fertre <yannick.fertre@foss.st.com>
Cc: Philippe Cornu <philippe.cornu@foss.st.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/stm/ltdc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 65c3c79ad1d5..e99771b947b6 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -1326,8 +1326,6 @@ int ltdc_load(struct drm_device *ddev)
 		goto err;
 	}
 
-	ddev->mode_config.allow_fb_modifiers = true;
-
 	ret = ltdc_crtc_init(ddev, crtc);
 	if (ret) {
 		DRM_ERROR("Failed to init crtc\n");
-- 
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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 <maxime@cerno.tech>
Cc: Simon Ser <contact@emersion.fr>
Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Ser]

Reviewed-by: Simon Ser <contact@emersion.fr>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

Hi Daniel,

On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <emil.velikov@collabora.com>

HTH
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

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 <daniel.vetter@ffwll.ch> 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 <emil.velikov@collabora.com>
> 
> HTH
> -Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/8] drm/arm: Don't set allow_fb_modifiers explicitly

[Liviu Dudau]

On Tue, Apr 27, 2021 at 11:20:11AM +0200, Daniel Vetter wrote:
> Since
> 
> commit 890880ddfdbe256083170866e49c87618b706ac7
> Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Date:   Fri Jan 4 09:56:10 2019 +0100
> 
>     drm: Auto-set allow_fb_modifiers when given modifiers at plane init
> 
> this is done automatically as part of plane init, if drivers set the
> modifier list correctly. Which is the case here for both komeda and
> malidp.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: "James (Qian) Wang" <james.qian.wang@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> ---
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 1 -
>  drivers/gpu/drm/arm/malidp_drv.c                | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index aeda4e5ec4f4..ff45f23f3d56 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -247,7 +247,6 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
>  	config->min_height	= 0;
>  	config->max_width	= 4096;
>  	config->max_height	= 4096;
> -	config->allow_fb_modifiers = true;
>  
>  	config->funcs = &komeda_mode_config_funcs;
>  	config->helper_private = &komeda_mode_config_helpers;
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index d83c7366b348..de59f3302516 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -403,7 +403,6 @@ static int malidp_init(struct drm_device *drm)
>  	drm->mode_config.max_height = hwdev->max_line_size;
>  	drm->mode_config.funcs = &malidp_mode_config_funcs;
>  	drm->mode_config.helper_private = &malidp_mode_config_helpers;
> -	drm->mode_config.allow_fb_modifiers = true;
>  
>  	ret = malidp_crtc_init(drm);
>  	if (ret)
> -- 
> 2.31.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/arm/malidp: Always list modifiers

[Liviu Dudau]

On Tue, Apr 27, 2021 at 11:20:12AM +0200, Daniel Vetter wrote:
> Even when all we support is linear, make that explicit. Otherwise the
> uapi is rather confusing.

:)

> 
> Cc: stable@vger.kernel.org
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/malidp_planes.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_planes.c b/drivers/gpu/drm/arm/malidp_planes.c
> index ddbba67f0283..8c2ab3d653b7 100644
> --- a/drivers/gpu/drm/arm/malidp_planes.c
> +++ b/drivers/gpu/drm/arm/malidp_planes.c
> @@ -927,6 +927,11 @@ static const struct drm_plane_helper_funcs malidp_de_plane_helper_funcs = {
>  	.atomic_disable = malidp_de_plane_disable,
>  };
>  
> +static const uint64_t linear_only_modifiers[] = {
> +	DRM_FORMAT_MOD_LINEAR,
> +	DRM_FORMAT_MOD_INVALID
> +};
> +
>  int malidp_de_planes_init(struct drm_device *drm)
>  {
>  	struct malidp_drm *malidp = drm->dev_private;
> @@ -990,8 +995,8 @@ int malidp_de_planes_init(struct drm_device *drm)
>  		 */
>  		ret = drm_universal_plane_init(drm, &plane->base, crtcs,
>  				&malidp_de_plane_funcs, formats, n,
> -				(id == DE_SMART) ? NULL : modifiers, plane_type,
> -				NULL);
> +				(id == DE_SMART) ? linear_only_modifiers : modifiers,
> +				plane_type, NULL);
>  
>  		if (ret < 0)
>  			goto cleanup;
> -- 
> 2.31.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 7/8] drm/stm: Don't set allow_fb_modifiers explicitly

[Philippe CORNU - foss]

Hi Daniel,
Many thanks for your patch,
Acked-by: Philippe Cornu <philippe.cornu@st.com>
Philippe :-)

________________________________________
De : Daniel Vetter <daniel.vetter@ffwll.ch>
Envoyé : mardi 27 avril 2021 11:20
À : DRI Development
Cc : Intel Graphics Development; Daniel Vetter; Daniel Vetter; Yannick FERTRE - foss; Philippe CORNU - foss; Benjamin Gaignard; Maxime Coquelin; Alexandre TORGUE - foss; linux-stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org
Objet : [PATCH 7/8] drm/stm: Don't set allow_fb_modifiers explicitly

Since

commit 890880ddfdbe256083170866e49c87618b706ac7
Author: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Date:   Fri Jan 4 09:56:10 2019 +0100

    drm: Auto-set allow_fb_modifiers when given modifiers at plane init

this is done automatically as part of plane init, if drivers set the
modifier list correctly. Which is the case here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Yannick Fertre <yannick.fertre@foss.st.com>
Cc: Philippe Cornu <philippe.cornu@foss.st.com>
Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/stm/ltdc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 65c3c79ad1d5..e99771b947b6 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -1326,8 +1326,6 @@ int ltdc_load(struct drm_device *ddev)
                goto err;
        }

-       ddev->mode_config.allow_fb_modifiers = true;
-
        ret = ltdc_crtc_init(ddev, crtc);
        if (ret) {
                DRM_ERROR("Failed to init crtc\n");
--
2.31.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 3989 bytes --]

On Tue, 27 Apr 2021 11:20:18 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> 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 <maxime@cerno.tech>
> Cc: Simon Ser <contact@emersion.fr>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  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 <pekka.paalanen@collabora.com>

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


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

Hi Daniel,

Thanks for the extra clarification.

On Tue, 27 Apr 2021 at 13:22, Daniel Vetter <daniel@ffwll.ch> 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 <daniel.vetter@ffwll.ch> 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/CAKMK7uGNP5us8KFffnPwq7g4b0-B2q-m7deqz_rPHtCrh_qUTw@mail.gmail.com/

> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Simon Ser]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Emil Velikov]

On Tue, 4 May 2021 at 15:58, Simon Ser <contact@emersion.fr> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Tue, Apr 27, 2021 at 11:20 AM Daniel Vetter <daniel.vetter@ffwll.ch> 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 <maxime@cerno.tech>
> Cc: Simon Ser <contact@emersion.fr>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>

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/20210427092018.832258-8-daniel.vetter@ffwll.ch/
we should probably use drm_WARN_ON() here instead
<Lyude> with that fixed: Reviewed-by: Lyude Paul <lyude@redhat.com>

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
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

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 <maxime@cerno.tech>
> Cc: Simon Ser <contact@emersion.fr>
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>

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
> 

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