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)
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;
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; }
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 initthis 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;
}
On Tue, Apr 20, 2021 at 03:31:27PM +0900, Inki Dae wrote:
- 오후 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 initthis 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;
}
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);
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);
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);
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);
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);
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 initthis 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);
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 initthis 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);
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);
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
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;
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;
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");
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;
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 initthis 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);@@ -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
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 initthis 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);@@ -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
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)
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 initthis 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
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: *
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: *
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: *
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 structdrm_format_modifier_blob. Without this property the plane doesn'tsupport 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
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 structdrm_format_modifier_blob. Without this property the plane doesn'tsupport 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
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 structdrm_format_modifier_blob. Without this property the plane doesn'tsupport 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
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 structdrm_format_modifier_blob. Without this property the plane doesn'tsupport 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
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
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
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: *
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 structdrm_format_modifier_blob. Without this property the plane doesn'tsupport 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
Reviewed-by: Simon Ser contact@emersion.fr
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Inki Dae -
Liu Ying -
Lucas Stach -
Maxime Ripard -
Pekka Paalanen -
Simon Ser -
Thierry Reding