dri-devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly
@ 2021-04-13  9:48 Daniel Vetter
  2021-04-13  9:48 ` [PATCH 02/12] drm/arm/malidp: Always list modifiers Daniel Vetter
                   ` (10 more replies)
  0 siblings, 11 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Daniel Vetter, Intel Graphics Development, Liviu Dudau,
	James (Qian) Wang, Daniel Vetter, Mihail Atanassov

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 02/12] drm/arm/malidp: Always list modifiers
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13  9:48 ` [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Pekka Paalanen, Daniel Vetter, Intel Graphics Development,
	Liviu Dudau, stable, 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
  2021-04-13  9:48 ` [PATCH 02/12] drm/arm/malidp: Always list modifiers Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-20  6:31   ` Inki Dae
  2021-04-13  9:48 ` [PATCH 04/12] drm/i915: " Daniel Vetter
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: linux-samsung-soc, Joonyoung Shim, Daniel Vetter,
	Intel Graphics Development, Seung-Woo Kim, Krzysztof Kozlowski,
	Kyungmin Park, Daniel Vetter, linux-arm-kernel

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: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-samsung-soc@vger.kernel.org
---
 drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 64370b634cca..79fa3649185c 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -177,7 +177,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
 	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
 	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
 
-	dev->mode_config.allow_fb_modifiers = true;
-
 	dev->mode_config.normalize_zpos = true;
 }
-- 
2.31.0

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 04/12] drm/i915: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
  2021-04-13  9:48 ` [PATCH 02/12] drm/arm/malidp: Always list modifiers Daniel Vetter
  2021-04-13  9:48 ` [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13  9:48 ` [PATCH 05/12] drm/imx: " Daniel Vetter
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Jani Nikula, Daniel Vetter, Intel Graphics Development,
	Karthik B S, José Roberto de Souza, Chris Wilson,
	Manasi Navare, Dave Airlie, 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 de8546a46872..2d1aa35adb0a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -11732,8 +11732,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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (2 preceding siblings ...)
  2021-04-13  9:48 ` [PATCH 04/12] drm/i915: " Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13 11:47   ` Lucas Stach
  2021-04-13  9:48 ` [PATCH 06/12] drm/msm/dpu1: " Daniel Vetter
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Pengutronix Kernel Team, Daniel Vetter,
	Intel Graphics Development, NXP Linux Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

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.

This one actually set it twice on top of what drm_plane_init does, so
double-redundant!

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
 drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
index b549ce5e7607..37ae68a7fba5 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
@@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
 	config->min_height = 1;
 	config->max_width = 4096;
 	config->max_height = 4096;
-	config->allow_fb_modifiers = true;
 	config->normalize_zpos = true;
 
 	config->funcs = &dcss_drm_mode_config_funcs;
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 2ded8e4f32d0..8be4edaec958 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
 	drm->mode_config.max_height = 4096;
 	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
 	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
-	drm->mode_config.allow_fb_modifiers = true;
 	drm->mode_config.normalize_zpos = true;
 
 	ret = drmm_mode_config_init(drm);
-- 
2.31.0

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 06/12] drm/msm/dpu1: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (3 preceding siblings ...)
  2021-04-13  9:48 ` [PATCH 05/12] drm/imx: " Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13  9:48 ` [PATCH 07/12] drm/msm/mdp4: Fix modifier support enabling Daniel Vetter
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Rob Clark, Rajendra Nayak, Daniel Vetter,
	Intel Graphics Development, Tanmay Shah, Jordan Crouse,
	Qinglang Miao, Daniel Vetter, Kalyan Thota

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: 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 85f2c3564c96..074fb37ed49f 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;
-
 	/*
 	 * _dpu_kms_drm_obj_init should create the DRM related objects
 	 * i.e. CRTCs, planes, encoders, connectors and so forth
-- 
2.31.0

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 07/12] drm/msm/mdp4: Fix modifier support enabling
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (4 preceding siblings ...)
  2021-04-13  9:48 ` [PATCH 06/12] drm/msm/dpu1: " Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13  9:48 ` [PATCH 08/12] drm/nouveau: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Rob Clark, Pekka Paalanen, Daniel Vetter,
	Intel Graphics Development, stable, Jordan Crouse, Daniel Vetter,
	Sam Ravnborg, Emil Velikov

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 08/12] drm/nouveau: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (5 preceding siblings ...)
  2021-04-13  9:48 ` [PATCH 07/12] drm/msm/mdp4: Fix modifier support enabling Daniel Vetter
@ 2021-04-13  9:48 ` Daniel Vetter
  2021-04-13  9:49 ` [PATCH 09/12] drm/stm: " Daniel Vetter
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:48 UTC (permalink / raw)
  To: DRI Development
  Cc: Pekka Paalanen, Daniel Vetter, Intel Graphics Development,
	stable, Ben Skeggs, nouveau, 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 09/12] drm/stm: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (6 preceding siblings ...)
  2021-04-13  9:48 ` [PATCH 08/12] drm/nouveau: Don't set allow_fb_modifiers explicitly Daniel Vetter
@ 2021-04-13  9:49 ` Daniel Vetter
  2021-04-13  9:49 ` [PATCH 10/12] drm/tegra: " Daniel Vetter
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:49 UTC (permalink / raw)
  To: DRI Development
  Cc: Maxime Coquelin, Daniel Vetter, Intel Graphics Development,
	Alexandre Torgue, linux-stm32, Philippe Cornu, Benjamin Gaignard,
	Daniel Vetter, Yannick Fertre, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 10/12] drm/tegra: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (7 preceding siblings ...)
  2021-04-13  9:49 ` [PATCH 09/12] drm/stm: " Daniel Vetter
@ 2021-04-13  9:49 ` Daniel Vetter
  2021-04-13 11:37   ` Thierry Reding
  2021-04-13  9:49 ` [PATCH 11/12] drm/vc4: " Daniel Vetter
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
  10 siblings, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:49 UTC (permalink / raw)
  To: DRI Development
  Cc: Pekka Paalanen, Daniel Vetter, Intel Graphics Development,
	stable, Jonathan Hunter, Thierry Reding, linux-tegra,
	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.

It was slightly inconsistently though, since planes with only linear
modifier support haven't listed that explicitly. Fix that, and cc:
stable to allow userspace to rely on this. Again don't backport
further than where Paul's patch got added.

Cc: stable@vger.kernel.org # v5.1 +
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: linux-tegra@vger.kernel.org
---
 drivers/gpu/drm/tegra/dc.c  | 10 ++++++++--
 drivers/gpu/drm/tegra/drm.c |  2 --
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index c9385cfd0fc1..f9845a50f866 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -959,6 +959,11 @@ static const struct drm_plane_helper_funcs tegra_cursor_plane_helper_funcs = {
 	.atomic_disable = tegra_cursor_atomic_disable,
 };
 
+static const uint64_t linear_modifiers[] = {
+	DRM_FORMAT_MOD_LINEAR,
+	DRM_FORMAT_MOD_INVALID
+};
+
 static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
 						      struct tegra_dc *dc)
 {
@@ -987,7 +992,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
 
 	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
 				       &tegra_plane_funcs, formats,
-				       num_formats, NULL,
+				       num_formats, linear_modifiers,
 				       DRM_PLANE_TYPE_CURSOR, NULL);
 	if (err < 0) {
 		kfree(plane);
@@ -1106,7 +1111,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
 
 	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
 				       &tegra_plane_funcs, formats,
-				       num_formats, NULL, type, NULL);
+				       num_formats, linear_modifiers,
+				       type, NULL);
 	if (err < 0) {
 		kfree(plane);
 		return ERR_PTR(err);
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 90709c38c993..136fe98f9459 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1125,8 +1125,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
 	drm->mode_config.max_width = 4096;
 	drm->mode_config.max_height = 4096;
 
-	drm->mode_config.allow_fb_modifiers = true;
-
 	drm->mode_config.normalize_zpos = true;
 
 	drm->mode_config.funcs = &tegra_drm_mode_config_funcs;
-- 
2.31.0

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 11/12] drm/vc4: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (8 preceding siblings ...)
  2021-04-13  9:49 ` [PATCH 10/12] drm/tegra: " Daniel Vetter
@ 2021-04-13  9:49 ` Daniel Vetter
  2021-04-14  8:51   ` Maxime Ripard
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
  10 siblings, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:49 UTC (permalink / raw)
  To: DRI Development; +Cc: Daniel Vetter, Intel Graphics Development, 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: Eric Anholt <eric@anholt.net>
Cc: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index bb5529a7a9c2..f29ac64a5aa5 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -899,7 +899,6 @@ int vc4_kms_load(struct drm_device *dev)
 	dev->mode_config.helper_private = &vc4_mode_config_helpers;
 	dev->mode_config.preferred_depth = 24;
 	dev->mode_config.async_page_flip = true;
-	dev->mode_config.allow_fb_modifiers = true;
 
 	ret = vc4_ctm_obj_init(vc4);
 	if (ret)
-- 
2.31.0

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
                   ` (9 preceding siblings ...)
  2021-04-13  9:49 ` [PATCH 11/12] drm/vc4: " Daniel Vetter
@ 2021-04-13  9:49 ` Daniel Vetter
  2021-04-13 11:54   ` Lucas Stach
                     ` (4 more replies)
  10 siblings, 5 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13  9:49 UTC (permalink / raw)
  To: DRI Development
  Cc: Pekka Paalanen, David Airlie, Daniel Vetter,
	Intel Graphics Development, Thomas Zimmermann, 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.

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   | 16 +++++++++++++++-
 include/drm/drm_mode_config.h |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0dd43882fe7c..16a7e3e57f7f 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -128,6 +128,11 @@
  *     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.
  */
 
 static unsigned int drm_num_planes(struct drm_device *dev)
@@ -277,8 +282,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 +371,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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 10/12] drm/tegra: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:49 ` [PATCH 10/12] drm/tegra: " Daniel Vetter
@ 2021-04-13 11:37   ` Thierry Reding
  2021-04-15 11:33     ` Daniel Vetter
  0 siblings, 1 reply; 34+ messages in thread
From: Thierry Reding @ 2021-04-13 11:37 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Pekka Paalanen, Intel Graphics Development, stable,
	Jonathan Hunter, DRI Development, linux-tegra, Daniel Vetter

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

On Tue, Apr 13, 2021 at 11:49:01AM +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.
> 
> It was slightly inconsistently though, since planes with only linear
> modifier support haven't listed that explicitly. Fix that, and cc:
> stable to allow userspace to rely on this. Again don't backport
> further than where Paul's patch got added.
> 
> Cc: stable@vger.kernel.org # v5.1 +
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: linux-tegra@vger.kernel.org
> ---
>  drivers/gpu/drm/tegra/dc.c  | 10 ++++++++--
>  drivers/gpu/drm/tegra/drm.c |  2 --
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index c9385cfd0fc1..f9845a50f866 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -959,6 +959,11 @@ static const struct drm_plane_helper_funcs tegra_cursor_plane_helper_funcs = {
>  	.atomic_disable = tegra_cursor_atomic_disable,
>  };
>  
> +static const uint64_t linear_modifiers[] = {
> +	DRM_FORMAT_MOD_LINEAR,
> +	DRM_FORMAT_MOD_INVALID
> +};
> +
>  static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>  						      struct tegra_dc *dc)
>  {
> @@ -987,7 +992,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>  
>  	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
>  				       &tegra_plane_funcs, formats,
> -				       num_formats, NULL,
> +				       num_formats, linear_modifiers,
>  				       DRM_PLANE_TYPE_CURSOR, NULL);
>  	if (err < 0) {
>  		kfree(plane);
> @@ -1106,7 +1111,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
>  
>  	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
>  				       &tegra_plane_funcs, formats,
> -				       num_formats, NULL, type, NULL);
> +				       num_formats, linear_modifiers,
> +				       type, NULL);

I think we can do better than linear_modifiers for overlay planes, but
given that this doesn't change existing behaviour, I'll do that in a
separate patch.

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 ` [PATCH 05/12] drm/imx: " Daniel Vetter
@ 2021-04-13 11:47   ` Lucas Stach
  2021-04-13 14:04     ` Daniel Vetter
  2021-04-15 11:35     ` Daniel Vetter
  0 siblings, 2 replies; 34+ messages in thread
From: Lucas Stach @ 2021-04-13 11:47 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, NXP Linux Team,
	Pengutronix Kernel Team, Daniel Vetter, Shawn Guo, Sascha Hauer,
	linux-arm-kernel

Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> 
> This one actually set it twice on top of what drm_plane_init does, so
> double-redundant!

That's not true. imx-dcss and imx-drm are two totally separate drivers.
Maybe we should move imx-drm into its own ipuv3 directory one day to
make this more clear. Change is still correct, though.

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
>  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> index b549ce5e7607..37ae68a7fba5 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
>  	config->min_height = 1;
>  	config->max_width = 4096;
>  	config->max_height = 4096;
> -	config->allow_fb_modifiers = true;
>  	config->normalize_zpos = true;
>  
> 
> 
> 
>  	config->funcs = &dcss_drm_mode_config_funcs;
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 2ded8e4f32d0..8be4edaec958 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
>  	drm->mode_config.max_height = 4096;
>  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
>  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> -	drm->mode_config.allow_fb_modifiers = true;
>  	drm->mode_config.normalize_zpos = true;
>  
> 
> 
> 
>  	ret = drmm_mode_config_init(drm);


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
@ 2021-04-13 11:54   ` Lucas Stach
  2021-04-13 14:17     ` Daniel Vetter
  2021-04-13 11:56   ` Pekka Paalanen
                     ` (3 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: Lucas Stach @ 2021-04-13 11:54 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	Pekka Paalanen, Thomas Zimmermann

Am Dienstag, dem 13.04.2021 um 11:49 +0200 schrieb 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).

For a lot of the embedded display drivers that never had any out-of-
band tiling meta shared with the GPU part, the implied modifier is
actually DRM_FORMAT_MOD_LINEAR, so maybe that's where some of the
confusion about needing to specify the modifier list comes from.

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

This clarification looks good to me.

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   | 16 +++++++++++++++-
>  include/drm/drm_mode_config.h |  2 ++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0dd43882fe7c..16a7e3e57f7f 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -128,6 +128,11 @@
>   *     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.
>   */
>  
> 
> 
> 
> 
> 
> 
> 
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -277,8 +282,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 +371,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:
>  	 *


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
  2021-04-13 11:54   ` Lucas Stach
@ 2021-04-13 11:56   ` Pekka Paalanen
  2021-04-13 14:11     ` Daniel Vetter
  2021-04-13 15:22   ` Simon Ser
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: Pekka Paalanen @ 2021-04-13 11:56 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	Thomas Zimmermann, DRI Development

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

On Tue, 13 Apr 2021 11:49:03 +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.
> 
> 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   | 16 +++++++++++++++-
>  include/drm/drm_mode_config.h |  2 ++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 0dd43882fe7c..16a7e3e57f7f 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -128,6 +128,11 @@
>   *     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.

Ooh, that's even better. But isn't that changing the meaning of the
cap? Isn't the cap older than IN_FORMATS?

What about the opposite? Is it allowed to have even a single IN_FORMATS
if you don't have the cap?

>   */
>  
>  static unsigned int drm_num_planes(struct drm_device *dev)
> @@ -277,8 +282,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));

What does this mean?

>  		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 +371,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.

Good.

> + *
>   * 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:
>  	 *

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13 11:47   ` Lucas Stach
@ 2021-04-13 14:04     ` Daniel Vetter
  2021-04-13 14:14       ` Lucas Stach
  2021-04-15 11:35     ` Daniel Vetter
  1 sibling, 1 reply; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13 14:04 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	NXP Linux Team, Pengutronix Kernel Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

On Tue, Apr 13, 2021 at 01:47:28PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> > 
> > This one actually set it twice on top of what drm_plane_init does, so
> > double-redundant!
> 
> That's not true. imx-dcss and imx-drm are two totally separate drivers.
> Maybe we should move imx-drm into its own ipuv3 directory one day to
> make this more clear. Change is still correct, though.

Hm I greeped for drm_universal_plane_init and didn't find anythinf for the
imx main driver ... where are planes set up for that? Need to review that
they have the modifiers listed in all cases.
-Daniel

> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > ---
> >  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
> >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
> >  2 files changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > index b549ce5e7607..37ae68a7fba5 100644
> > --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
> >  	config->min_height = 1;
> >  	config->max_width = 4096;
> >  	config->max_height = 4096;
> > -	config->allow_fb_modifiers = true;
> >  	config->normalize_zpos = true;
> >  
> > 
> > 
> > 
> >  	config->funcs = &dcss_drm_mode_config_funcs;
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index 2ded8e4f32d0..8be4edaec958 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
> >  	drm->mode_config.max_height = 4096;
> >  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> >  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> > -	drm->mode_config.allow_fb_modifiers = true;
> >  	drm->mode_config.normalize_zpos = true;
> >  
> > 
> > 
> > 
> >  	ret = drmm_mode_config_init(drm);
> 
> 

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13 11:56   ` Pekka Paalanen
@ 2021-04-13 14:11     ` Daniel Vetter
  2021-04-13 14:19       ` Daniel Vetter
  2021-04-14  7:45       ` Pekka Paalanen
  0 siblings, 2 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13 14:11 UTC (permalink / raw)
  To: Pekka Paalanen
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	DRI Development, Thomas Zimmermann, Daniel Vetter

On Tue, Apr 13, 2021 at 02:56:02PM +0300, Pekka Paalanen wrote:
> On Tue, 13 Apr 2021 11:49:03 +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.
> > 
> > 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   | 16 +++++++++++++++-
> >  include/drm/drm_mode_config.h |  2 ++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 0dd43882fe7c..16a7e3e57f7f 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -128,6 +128,11 @@
> >   *     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.
> 
> Ooh, that's even better. But isn't that changing the meaning of the
> cap? Isn't the cap older than IN_FORMATS?

Hm indeed. But also how exactly are you going to user modifiers without
IN_FORMATS ... it's a bit hard. I think this is all because we've enabled
modifiers piece-by-piece and never across the entire thing (e.g. with
compositor and protocols), so the missing pieces only became apparent
later on.

I'm not sure whether compositors really want to support this, I guess
worst case we could disable the cap on these old kernels.

> What about the opposite? Is it allowed to have even a single IN_FORMATS
> if you don't have the cap?

That direction is enforced since 5.1, because some drivers screwed it up
and confusion in userspace ensued.

Should I add a bug that on kernels older than 5.1 the situation is more
murky and there's lots of bugs?

> 
> >   */
> >  
> >  static unsigned int drm_num_planes(struct drm_device *dev)
> > @@ -277,8 +282,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));
> 
> What does this mean?

If allow_fb_modifiers isn't set yet (we do that in the line below) and we
are _not_ the first plane that gets added to the driver (that's done
towards the end of the function) then that means there's already a plane
registered without modifiers and hence IN_FORMAT. Which we then warn
about.

> 
> >  		config->allow_fb_modifiers = true;
> > +	} else {
> > +		WARN_ON(config->allow_fb_modifiers);

This warning here checks the other case of an earlier plane with
modifiers, but the one we're adding now doesn't have them.
-Daniel

> > +	}
> >  
> >  	plane->modifier_count = format_modifier_count;
> >  	plane->modifiers = kmalloc_array(format_modifier_count,
> > @@ -360,6 +371,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.
> 
> Good.
> 
> > + *
> >   * 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:
> >  	 *
> 
> Thanks,
> pq



-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13 14:04     ` Daniel Vetter
@ 2021-04-13 14:14       ` Lucas Stach
  2021-04-14  2:24         ` Liu Ying
  0 siblings, 1 reply; 34+ messages in thread
From: Lucas Stach @ 2021-04-13 14:14 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	NXP Linux Team, Pengutronix Kernel Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

Am Dienstag, dem 13.04.2021 um 16:04 +0200 schrieb Daniel Vetter:
> On Tue, Apr 13, 2021 at 01:47:28PM +0200, Lucas Stach wrote:
> > Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> > > 
> > > This one actually set it twice on top of what drm_plane_init does, so
> > > double-redundant!
> > 
> > That's not true. imx-dcss and imx-drm are two totally separate drivers.
> > Maybe we should move imx-drm into its own ipuv3 directory one day to
> > make this more clear. Change is still correct, though.
> 
> Hm I greeped for drm_universal_plane_init and didn't find anythinf for the
> imx main driver ... where are planes set up for that? Need to review that
> they have the modifiers listed in all cases.

That's in drivers/gpu/drm/imx/ipuv3-plane.c and modifiers are always
set on plane init.

Regards,
Lucas

> 
> > 
> > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > Cc: Fabio Estevam <festevam@gmail.com>
> > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > ---
> > >  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
> > >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
> > >  2 files changed, 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > index b549ce5e7607..37ae68a7fba5 100644
> > > --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
> > >  	config->min_height = 1;
> > >  	config->max_width = 4096;
> > >  	config->max_height = 4096;
> > > -	config->allow_fb_modifiers = true;
> > >  	config->normalize_zpos = true;
> > >  
> > > 
> > > 
> > > 
> > >  	config->funcs = &dcss_drm_mode_config_funcs;
> > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > index 2ded8e4f32d0..8be4edaec958 100644
> > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
> > >  	drm->mode_config.max_height = 4096;
> > >  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> > >  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> > > -	drm->mode_config.allow_fb_modifiers = true;
> > >  	drm->mode_config.normalize_zpos = true;
> > >  
> > > 
> > > 
> > > 
> > >  	ret = drmm_mode_config_init(drm);
> > 
> > 
> 


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13 11:54   ` Lucas Stach
@ 2021-04-13 14:17     ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13 14:17 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Pekka Paalanen, David Airlie, Daniel Vetter,
	Intel Graphics Development, DRI Development, Thomas Zimmermann,
	Daniel Vetter

On Tue, Apr 13, 2021 at 01:54:00PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.04.2021 um 11:49 +0200 schrieb 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).
> 
> For a lot of the embedded display drivers that never had any out-of-
> band tiling meta shared with the GPU part, the implied modifier is
> actually DRM_FORMAT_MOD_LINEAR, so maybe that's where some of the
> confusion about needing to specify the modifier list comes from.

Yeah that entire discussion last few days is why I wanted to audit all the
drivers and make sure that going forward we're more explicit&consistent
with all this. There's huge confusion around implied modifier vs "no
IN_FORMATS blob property" and what that exactly means.
-Daniel

> > Now that this is all done consistently across all drivers, document
> > the rules and enforce it in the drm core.
> 
> This clarification looks good to me.
> 
> 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   | 16 +++++++++++++++-
> >  include/drm/drm_mode_config.h |  2 ++
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > index 0dd43882fe7c..16a7e3e57f7f 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -128,6 +128,11 @@
> >   *     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.
> >   */
> >  
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> >  static unsigned int drm_num_planes(struct drm_device *dev)
> > @@ -277,8 +282,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 +371,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:
> >  	 *
> 
> 

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13 14:11     ` Daniel Vetter
@ 2021-04-13 14:19       ` Daniel Vetter
  2021-04-14  7:45       ` Pekka Paalanen
  1 sibling, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-13 14:19 UTC (permalink / raw)
  To: Pekka Paalanen
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	DRI Development, Thomas Zimmermann, Daniel Vetter

On Tue, Apr 13, 2021 at 04:11:29PM +0200, Daniel Vetter wrote:
> On Tue, Apr 13, 2021 at 02:56:02PM +0300, Pekka Paalanen wrote:
> > On Tue, 13 Apr 2021 11:49:03 +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.
> > > 
> > > 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   | 16 +++++++++++++++-
> > >  include/drm/drm_mode_config.h |  2 ++
> > >  2 files changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 0dd43882fe7c..16a7e3e57f7f 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -128,6 +128,11 @@
> > >   *     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.
> > 
> > Ooh, that's even better. But isn't that changing the meaning of the
> > cap? Isn't the cap older than IN_FORMATS?
> 
> Hm indeed. But also how exactly are you going to user modifiers without
> IN_FORMATS ... it's a bit hard. I think this is all because we've enabled
> modifiers piece-by-piece and never across the entire thing (e.g. with
> compositor and protocols), so the missing pieces only became apparent
> later on.

Ok I worked git log -Gallow_fb_modifiers and there's 3 drivers which
enabled modifiers before IN_FORMATS was merged:
- i915
- msm/mdp4 (for the tiled NV12 format thing)
- tegra

> I'm not sure whether compositors really want to support this, I guess
> worst case we could disable the cap on these old kernels.
> 
> > What about the opposite? Is it allowed to have even a single IN_FORMATS
> > if you don't have the cap?
> 
> That direction is enforced since 5.1, because some drivers screwed it up
> and confusion in userspace ensued.
> 
> Should I add a bug that on kernels older than 5.1 the situation is more
> murky and there's lots of bugs?

I guess we should recommend to userspace that if they spot an
inconsistency between IN_FORMATS across planes and the cap then maybe they
want to disable modifier support because it might be all kinds of broken?
-Daniel

> 
> > 
> > >   */
> > >  
> > >  static unsigned int drm_num_planes(struct drm_device *dev)
> > > @@ -277,8 +282,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));
> > 
> > What does this mean?
> 
> If allow_fb_modifiers isn't set yet (we do that in the line below) and we
> are _not_ the first plane that gets added to the driver (that's done
> towards the end of the function) then that means there's already a plane
> registered without modifiers and hence IN_FORMAT. Which we then warn
> about.
> 
> > 
> > >  		config->allow_fb_modifiers = true;
> > > +	} else {
> > > +		WARN_ON(config->allow_fb_modifiers);
> 
> This warning here checks the other case of an earlier plane with
> modifiers, but the one we're adding now doesn't have them.
> -Daniel
> 
> > > +	}
> > >  
> > >  	plane->modifier_count = format_modifier_count;
> > >  	plane->modifiers = kmalloc_array(format_modifier_count,
> > > @@ -360,6 +371,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.
> > 
> > Good.
> > 
> > > + *
> > >   * 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:
> > >  	 *
> > 
> > Thanks,
> > pq
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
  2021-04-13 11:54   ` Lucas Stach
  2021-04-13 11:56   ` Pekka Paalanen
@ 2021-04-13 15:22   ` Simon Ser
  2021-04-14  8:52   ` Maxime Ripard
  2021-04-14  9:08   ` [PATCH] " Daniel Vetter
  4 siblings, 0 replies; 34+ messages in thread
From: Simon Ser @ 2021-04-13 15:22 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Pekka Paalanen, David Airlie, Intel Graphics Development,
	DRI Development, Thomas Zimmermann, Daniel Vetter

On Tuesday, April 13th, 2021 at 11:49 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> + *     Note that userspace can check the DRM_CAP_ADDFB2_MODIFIERS driver

Prepend an ampersand like so and a hyperlink will be rendered:

    Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13 14:14       ` Lucas Stach
@ 2021-04-14  2:24         ` Liu Ying
  2021-04-14  9:10           ` Daniel Vetter
  0 siblings, 1 reply; 34+ messages in thread
From: Liu Ying @ 2021-04-14  2:24 UTC (permalink / raw)
  To: Lucas Stach, Daniel Vetter
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	NXP Linux Team, Pengutronix Kernel Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

Hi Daniel,

On Tue, 2021-04-13 at 16:14 +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.04.2021 um 16:04 +0200 schrieb Daniel Vetter:
> > On Tue, Apr 13, 2021 at 01:47:28PM +0200, Lucas Stach wrote:
> > > Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> > > > 
> > > > This one actually set it twice on top of what drm_plane_init does, so
> > > > double-redundant!
> > > 
> > > That's not true. imx-dcss and imx-drm are two totally separate drivers.
> > > Maybe we should move imx-drm into its own ipuv3 directory one day to
> > > make this more clear. Change is still correct, though.
> > 
> > Hm I greeped for drm_universal_plane_init and didn't find anythinf for the
> > imx main driver ... where are planes set up for that? Need to review that
> > they have the modifiers listed in all cases.
> 
> That's in drivers/gpu/drm/imx/ipuv3-plane.c and modifiers are always
> set on plane init.
> 
> Regards,
> Lucas
> 
> > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > > 
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > ---
> > > >  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
> > > >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -

Nit: Since this patch touches two totally separate drivers(imx-dcss and
imx-drm), it would be good to split it into two patches.

Thanks,
Liu Ying

> > > >  2 files changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > index b549ce5e7607..37ae68a7fba5 100644
> > > > --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
> > > >  	config->min_height = 1;
> > > >  	config->max_width = 4096;
> > > >  	config->max_height = 4096;
> > > > -	config->allow_fb_modifiers = true;
> > > >  	config->normalize_zpos = true;
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	config->funcs = &dcss_drm_mode_config_funcs;
> > > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > index 2ded8e4f32d0..8be4edaec958 100644
> > > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
> > > >  	drm->mode_config.max_height = 4096;
> > > >  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> > > >  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> > > > -	drm->mode_config.allow_fb_modifiers = true;
> > > >  	drm->mode_config.normalize_zpos = true;
> > > >  
> > > > 
> > > > 
> > > > 
> > > >  	ret = drmm_mode_config_init(drm);
> 
> 

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13 14:11     ` Daniel Vetter
  2021-04-13 14:19       ` Daniel Vetter
@ 2021-04-14  7:45       ` Pekka Paalanen
  1 sibling, 0 replies; 34+ messages in thread
From: Pekka Paalanen @ 2021-04-14  7:45 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: David Airlie, Daniel Vetter, Intel Graphics Development,
	DRI Development, Thomas Zimmermann, Daniel Vetter

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

On Tue, 13 Apr 2021 16:11:29 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Apr 13, 2021 at 02:56:02PM +0300, Pekka Paalanen wrote:
> > On Tue, 13 Apr 2021 11:49:03 +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.
> > > 
> > > 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   | 16 +++++++++++++++-
> > >  include/drm/drm_mode_config.h |  2 ++
> > >  2 files changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > > index 0dd43882fe7c..16a7e3e57f7f 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -128,6 +128,11 @@
> > >   *     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.  
> > 
> > Ooh, that's even better. But isn't that changing the meaning of the
> > cap? Isn't the cap older than IN_FORMATS?  
> 
> Hm indeed. But also how exactly are you going to user modifiers without
> IN_FORMATS ... it's a bit hard.

Easy for at least one specific case, as Daniel Stone said in IRC. Use
GBM to allocate using the no-modifiers API but specify USE_LINEAR. That
basically gives you MOD_LINEAR buffer. Then you can try to make a DRM
FB for it using AddFB2-with-modifiers.

Does anyone do this, I have no idea.

Actually, I think this semantic change is fine. Old userspace did not
know that the cap means all planes have IN_FORMATS, so they can deal
with IN_FORMATS missing, but if it is never missing, no problem.

It could be a problem with new userspace and old kernel, but that's by
definition not a kernel bug, right? Just... inconvenient for userspace
as they can't make full use of the flag and need to keep the fallback
path for missing IN_FORMATS.

As long as there are KMS drivers that don't support modifiers, generic
userspace probably needs the fallback path anyway.

> I think this is all because we've enabled
> modifiers piece-by-piece and never across the entire thing (e.g. with
> compositor and protocols), so the missing pieces only became apparent
> later on.
> 
> I'm not sure whether compositors really want to support this, I guess
> worst case we could disable the cap on these old kernels.
> 
> > What about the opposite? Is it allowed to have even a single IN_FORMATS
> > if you don't have the cap?  
> 
> That direction is enforced since 5.1, because some drivers screwed it up
> and confusion in userspace ensued.
> 
> Should I add a bug that on kernels older than 5.1 the situation is more
> murky and there's lots of bugs?

Yes, that would help to set expectations.

I'm currently on Debian stable FWIW, so 4.19 based kernel with I don't
know what patches.

On Tue, 13 Apr 2021 16:19:10 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Apr 13, 2021 at 04:11:29PM +0200, Daniel Vetter wrote:
> > 
> > Should I add a bug that on kernels older than 5.1 the situation is more
> > murky and there's lots of bugs?  
> 
> I guess we should recommend to userspace that if they spot an
> inconsistency between IN_FORMATS across planes and the cap then maybe they
> want to disable modifier support because it might be all kinds of broken?

Yes please!


------

> >   
> > >   */
> > >  
> > >  static unsigned int drm_num_planes(struct drm_device *dev)
> > > @@ -277,8 +282,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));  
> > 
> > What does this mean?  
> 
> If allow_fb_modifiers isn't set yet (we do that in the line below) and we
> are _not_ the first plane that gets added to the driver (that's done
> towards the end of the function) then that means there's already a plane
> registered without modifiers and hence IN_FORMAT. Which we then warn
> about.

Ah, ok! Would have taken a while for me to decipher that, and
impossible with just this patch context.

> >   
> > >  		config->allow_fb_modifiers = true;
> > > +	} else {
> > > +		WARN_ON(config->allow_fb_modifiers);  
> 
> This warning here checks the other case of an earlier plane with
> modifiers, but the one we're adding now doesn't have them.

Cool.


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 11/12] drm/vc4: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:49 ` [PATCH 11/12] drm/vc4: " Daniel Vetter
@ 2021-04-14  8:51   ` Maxime Ripard
  0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2021-04-14  8:51 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development

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

On Tue, Apr 13, 2021 at 11:49:02AM +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.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Maxime Ripard <mripard@kernel.org>

Acked-by: Maxime Ripard <maxime@cerno.tech>

Thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
                     ` (2 preceding siblings ...)
  2021-04-13 15:22   ` Simon Ser
@ 2021-04-14  8:52   ` Maxime Ripard
  2021-04-14  9:08   ` [PATCH] " Daniel Vetter
  4 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2021-04-14  8:52 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Pekka Paalanen, David Airlie, Intel Graphics Development,
	DRI Development, Thomas Zimmermann, Daniel Vetter

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

On Tue, Apr 13, 2021 at 11:49:03AM +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.
> 
> 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>

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
                     ` (3 preceding siblings ...)
  2021-04-14  8:52   ` Maxime Ripard
@ 2021-04-14  9:08   ` Daniel Vetter
  2021-04-14 12:14     ` Pekka Paalanen
  2021-04-16  6:30     ` Simon Ser
  4 siblings, 2 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-14  9:08 UTC (permalink / raw)
  To: DRI Development
  Cc: Pekka Paalanen, David Airlie, Daniel Vetter,
	Intel Graphics Development, Maxime Ripard, Thomas Zimmermann,
	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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-14  2:24         ` Liu Ying
@ 2021-04-14  9:10           ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-14  9:10 UTC (permalink / raw)
  To: Liu Ying
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	NXP Linux Team, Pengutronix Kernel Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

On Wed, Apr 14, 2021 at 10:24:22AM +0800, Liu Ying wrote:
> Hi Daniel,
> 
> On Tue, 2021-04-13 at 16:14 +0200, Lucas Stach wrote:
> > Am Dienstag, dem 13.04.2021 um 16:04 +0200 schrieb Daniel Vetter:
> > > On Tue, Apr 13, 2021 at 01:47:28PM +0200, Lucas Stach wrote:
> > > > Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> > > > > 
> > > > > This one actually set it twice on top of what drm_plane_init does, so
> > > > > double-redundant!
> > > > 
> > > > That's not true. imx-dcss and imx-drm are two totally separate drivers.
> > > > Maybe we should move imx-drm into its own ipuv3 directory one day to
> > > > make this more clear. Change is still correct, though.
> > > 
> > > Hm I greeped for drm_universal_plane_init and didn't find anythinf for the
> > > imx main driver ... where are planes set up for that? Need to review that
> > > they have the modifiers listed in all cases.
> > 
> > That's in drivers/gpu/drm/imx/ipuv3-plane.c and modifiers are always
> > set on plane init.

Oh I didn't grep for the new drmm_universal_plane_alloc. Thanks for
pointing this out.

> > 
> > Regards,
> > Lucas
> > 
> > > > Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> > > > 
> > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > > ---
> > > > >  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
> > > > >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
> 
> Nit: Since this patch touches two totally separate drivers(imx-dcss and
> imx-drm), it would be good to split it into two patches.

I think if you expect that, then you need to move the imx-drm driver into
a subdirectory like dcss. And like e.g. drm/msm works too. As-is this is
just kinda confusing for people not involved in imx.
-Daniel

> 
> Thanks,
> Liu Ying
> 
> > > > >  2 files changed, 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > > index b549ce5e7607..37ae68a7fba5 100644
> > > > > --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > > +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > > > > @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
> > > > >  	config->min_height = 1;
> > > > >  	config->max_width = 4096;
> > > > >  	config->max_height = 4096;
> > > > > -	config->allow_fb_modifiers = true;
> > > > >  	config->normalize_zpos = true;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > >  	config->funcs = &dcss_drm_mode_config_funcs;
> > > > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > > index 2ded8e4f32d0..8be4edaec958 100644
> > > > > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > > > > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > > > > @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
> > > > >  	drm->mode_config.max_height = 4096;
> > > > >  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> > > > >  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> > > > > -	drm->mode_config.allow_fb_modifiers = true;
> > > > >  	drm->mode_config.normalize_zpos = true;
> > > > >  
> > > > > 
> > > > > 
> > > > > 
> > > > >  	ret = drmm_mode_config_init(drm);
> > 
> > 
> 

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-14  9:08   ` [PATCH] " Daniel Vetter
@ 2021-04-14 12:14     ` Pekka Paalanen
  2021-04-16  6:30     ` Simon Ser
  1 sibling, 0 replies; 34+ messages in thread
From: Pekka Paalanen @ 2021-04-14 12:14 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: David Airlie, Intel Graphics Development, DRI Development,
	Maxime Ripard, Thomas Zimmermann, Daniel Vetter

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

On Wed, 14 Apr 2021 11:08:15 +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:
>  	 *

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 10/12] drm/tegra: Don't set allow_fb_modifiers explicitly
  2021-04-13 11:37   ` Thierry Reding
@ 2021-04-15 11:33     ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-15 11:33 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Pekka Paalanen, Daniel Vetter, Intel Graphics Development,
	stable, Jonathan Hunter, DRI Development, linux-tegra,
	Daniel Vetter

On Tue, Apr 13, 2021 at 01:37:38PM +0200, Thierry Reding wrote:
> On Tue, Apr 13, 2021 at 11:49:01AM +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.
> > 
> > It was slightly inconsistently though, since planes with only linear
> > modifier support haven't listed that explicitly. Fix that, and cc:
> > stable to allow userspace to rely on this. Again don't backport
> > further than where Paul's patch got added.
> > 
> > Cc: stable@vger.kernel.org # v5.1 +
> > Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: Jonathan Hunter <jonathanh@nvidia.com>
> > Cc: linux-tegra@vger.kernel.org
> > ---
> >  drivers/gpu/drm/tegra/dc.c  | 10 ++++++++--
> >  drivers/gpu/drm/tegra/drm.c |  2 --
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index c9385cfd0fc1..f9845a50f866 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -959,6 +959,11 @@ static const struct drm_plane_helper_funcs tegra_cursor_plane_helper_funcs = {
> >  	.atomic_disable = tegra_cursor_atomic_disable,
> >  };
> >  
> > +static const uint64_t linear_modifiers[] = {
> > +	DRM_FORMAT_MOD_LINEAR,
> > +	DRM_FORMAT_MOD_INVALID
> > +};
> > +
> >  static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
> >  						      struct tegra_dc *dc)
> >  {
> > @@ -987,7 +992,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
> >  
> >  	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
> >  				       &tegra_plane_funcs, formats,
> > -				       num_formats, NULL,
> > +				       num_formats, linear_modifiers,
> >  				       DRM_PLANE_TYPE_CURSOR, NULL);
> >  	if (err < 0) {
> >  		kfree(plane);
> > @@ -1106,7 +1111,8 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
> >  
> >  	err = drm_universal_plane_init(drm, &plane->base, possible_crtcs,
> >  				       &tegra_plane_funcs, formats,
> > -				       num_formats, NULL, type, NULL);
> > +				       num_formats, linear_modifiers,
> > +				       type, NULL);
> 
> I think we can do better than linear_modifiers for overlay planes, but
> given that this doesn't change existing behaviour, I'll do that in a
> separate patch.
> 
> Acked-by: Thierry Reding <treding@nvidia.com>

Applied to drm-misc-next, thanks for taking a look.
-Daniel
-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 05/12] drm/imx: Don't set allow_fb_modifiers explicitly
  2021-04-13 11:47   ` Lucas Stach
  2021-04-13 14:04     ` Daniel Vetter
@ 2021-04-15 11:35     ` Daniel Vetter
  1 sibling, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-15 11:35 UTC (permalink / raw)
  To: Lucas Stach
  Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
	NXP Linux Team, Pengutronix Kernel Team, Daniel Vetter,
	Shawn Guo, Sascha Hauer, linux-arm-kernel

On Tue, Apr 13, 2021 at 01:47:28PM +0200, Lucas Stach wrote:
> Am Dienstag, dem 13.04.2021 um 11:48 +0200 schrieb 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.
> > 
> > This one actually set it twice on top of what drm_plane_init does, so
> > double-redundant!
> 
> That's not true. imx-dcss and imx-drm are two totally separate drivers.
> Maybe we should move imx-drm into its own ipuv3 directory one day to
> make this more clear. Change is still correct, though.

I've fixed the commit message to reflect reality and merged to
drm-misc-next. Thanks for taking a look.
-Daniel

> 
> Reviewed-by: Lucas Stach <l.stach@pengutronix.de>
> 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Cc: Shawn Guo <shawnguo@kernel.org>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > Cc: NXP Linux Team <linux-imx@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > ---
> >  drivers/gpu/drm/imx/dcss/dcss-kms.c | 1 -
> >  drivers/gpu/drm/imx/imx-drm-core.c  | 1 -
> >  2 files changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > index b549ce5e7607..37ae68a7fba5 100644
> > --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> > @@ -52,7 +52,6 @@ static void dcss_kms_mode_config_init(struct dcss_kms_dev *kms)
> >  	config->min_height = 1;
> >  	config->max_width = 4096;
> >  	config->max_height = 4096;
> > -	config->allow_fb_modifiers = true;
> >  	config->normalize_zpos = true;
> >  
> > 
> > 
> > 
> >  	config->funcs = &dcss_drm_mode_config_funcs;
> > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> > index 2ded8e4f32d0..8be4edaec958 100644
> > --- a/drivers/gpu/drm/imx/imx-drm-core.c
> > +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> > @@ -209,7 +209,6 @@ static int imx_drm_bind(struct device *dev)
> >  	drm->mode_config.max_height = 4096;
> >  	drm->mode_config.funcs = &imx_drm_mode_config_funcs;
> >  	drm->mode_config.helper_private = &imx_drm_mode_config_helpers;
> > -	drm->mode_config.allow_fb_modifiers = true;
> >  	drm->mode_config.normalize_zpos = true;
> >  
> > 
> > 
> > 
> >  	ret = drmm_mode_config_init(drm);
> 
> 

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
  2021-04-14  9:08   ` [PATCH] " Daniel Vetter
  2021-04-14 12:14     ` Pekka Paalanen
@ 2021-04-16  6:30     ` Simon Ser
  1 sibling, 0 replies; 34+ messages in thread
From: Simon Ser @ 2021-04-16  6:30 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Pekka Paalanen, David Airlie, Intel Graphics Development,
	DRI Development, Maxime Ripard, Thomas Zimmermann, Daniel Vetter

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

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly
  2021-04-13  9:48 ` [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly Daniel Vetter
@ 2021-04-20  6:31   ` Inki Dae
  2021-04-20  8:41     ` Daniel Vetter
  0 siblings, 1 reply; 34+ messages in thread
From: Inki Dae @ 2021-04-20  6:31 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: linux-samsung-soc, Joonyoung Shim, Intel Graphics Development,
	Seung-Woo Kim, Krzysztof Kozlowski, Kyungmin Park, Daniel Vetter,
	linux-arm-kernel



21. 4. 13. 오후 6:48에 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>

Acked-by: Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index 64370b634cca..79fa3649185c 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -177,7 +177,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
>  	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
>  	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
>  
> -	dev->mode_config.allow_fb_modifiers = true;
> -
>  	dev->mode_config.normalize_zpos = true;
>  }
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly
  2021-04-20  6:31   ` Inki Dae
@ 2021-04-20  8:41     ` Daniel Vetter
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Vetter @ 2021-04-20  8:41 UTC (permalink / raw)
  To: Inki Dae
  Cc: linux-samsung-soc, Joonyoung Shim, Daniel Vetter,
	Intel Graphics Development, Seung-Woo Kim, DRI Development,
	Kyungmin Park, Krzysztof Kozlowski, Daniel Vetter,
	linux-arm-kernel

On Tue, Apr 20, 2021 at 03:31:27PM +0900, Inki Dae wrote:
> 
> 
> 21. 4. 13. 오후 6:48에 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>
> 
> Acked-by: Inki Dae <inki.dae@samsung.com>

Thanks for taking a look, pushed to drm-misc-next.
-Daniel

> 
> Thanks,
> Inki Dae
> 
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-samsung-soc@vger.kernel.org
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_fb.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> > index 64370b634cca..79fa3649185c 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> > @@ -177,7 +177,5 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
> >  	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
> >  	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
> >  
> > -	dev->mode_config.allow_fb_modifiers = true;
> > -
> >  	dev->mode_config.normalize_zpos = true;
> >  }
> > 

-- 
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

^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, back to index

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13  9:48 [PATCH 01/12] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter
2021-04-13  9:48 ` [PATCH 02/12] drm/arm/malidp: Always list modifiers Daniel Vetter
2021-04-13  9:48 ` [PATCH 03/12] drm/exynos: Don't set allow_fb_modifiers explicitly Daniel Vetter
2021-04-20  6:31   ` Inki Dae
2021-04-20  8:41     ` Daniel Vetter
2021-04-13  9:48 ` [PATCH 04/12] drm/i915: " Daniel Vetter
2021-04-13  9:48 ` [PATCH 05/12] drm/imx: " Daniel Vetter
2021-04-13 11:47   ` Lucas Stach
2021-04-13 14:04     ` Daniel Vetter
2021-04-13 14:14       ` Lucas Stach
2021-04-14  2:24         ` Liu Ying
2021-04-14  9:10           ` Daniel Vetter
2021-04-15 11:35     ` Daniel Vetter
2021-04-13  9:48 ` [PATCH 06/12] drm/msm/dpu1: " Daniel Vetter
2021-04-13  9:48 ` [PATCH 07/12] drm/msm/mdp4: Fix modifier support enabling Daniel Vetter
2021-04-13  9:48 ` [PATCH 08/12] drm/nouveau: Don't set allow_fb_modifiers explicitly Daniel Vetter
2021-04-13  9:49 ` [PATCH 09/12] drm/stm: " Daniel Vetter
2021-04-13  9:49 ` [PATCH 10/12] drm/tegra: " Daniel Vetter
2021-04-13 11:37   ` Thierry Reding
2021-04-15 11:33     ` Daniel Vetter
2021-04-13  9:49 ` [PATCH 11/12] drm/vc4: " Daniel Vetter
2021-04-14  8:51   ` Maxime Ripard
2021-04-13  9:49 ` [PATCH 12/12] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter
2021-04-13 11:54   ` Lucas Stach
2021-04-13 14:17     ` Daniel Vetter
2021-04-13 11:56   ` Pekka Paalanen
2021-04-13 14:11     ` Daniel Vetter
2021-04-13 14:19       ` Daniel Vetter
2021-04-14  7:45       ` Pekka Paalanen
2021-04-13 15:22   ` Simon Ser
2021-04-14  8:52   ` Maxime Ripard
2021-04-14  9:08   ` [PATCH] " Daniel Vetter
2021-04-14 12:14     ` Pekka Paalanen
2021-04-16  6:30     ` Simon Ser

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git
	git clone --mirror https://lore.kernel.org/dri-devel/1 dri-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git