[Nouveau] [RFC PATCH v2 0/3] Add support modifiers for drivers whose planes only support linear layout

[Nouveau] [RFC PATCH v2 0/3] Add support modifiers for drivers whose planes only support linear layout

[Tomohito Esaki]

Some drivers whose planes only support linear layout fb do not support format
modifiers.
These drivers should support modifiers, however the DRM core should handle this
rather than open-coding in every driver.

In this patch series, these drivers expose format modifiers based on the
following suggestion[1].

On Thu, Nov 18, 2021 at 01:02:11PM +0000, Daniel Stone wrote:
> I think the best way forward here is:
>   - add a new mode_config.cannot_support_modifiers flag, and enable
> this in radeon (plus any other drivers in the same boat)
>   - change drm_universal_plane_init() to advertise the LINEAR modifier
> when NULL is passed as the modifier list (including installing a
> default .format_mod_supported hook)
>   - remove the mode_config.allow_fb_modifiers hook and always
> advertise modifier support, unless
> mode_config.cannot_support_modifiers is set


[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20190509054518.10781-1-etom@igel.co.jp/#24602575


v2:
* rebase to the latest master branch (5.16.0+) + "drm/plane: Make format_mod_supported truly optional" patch [2]
  [2] https://patchwork.freedesktop.org/patch/467940/?series=98255&rev=3

v1: https://www.spinics.net/lists/dri-devel/msg327352.html
* The initial patch set


Tomohito Esaki (3):
  drm: add support modifiers for drivers whose planes only support
    linear layout
  drm: set fb_modifiers_not_supported flag in legacy drivers
  drm: replace allow_fb_modifiers with fb_modifiers_not_supported

 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c   |  6 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c        |  2 ++
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c        |  2 ++
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c         |  1 +
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c         |  2 ++
 drivers/gpu/drm/drm_framebuffer.c             |  6 ++---
 drivers/gpu/drm/drm_ioctl.c                   |  2 +-
 drivers/gpu/drm/drm_plane.c                   | 22 +++++++++----------
 drivers/gpu/drm/nouveau/nouveau_display.c     |  6 +++--
 drivers/gpu/drm/radeon/radeon_display.c       |  2 ++
 .../gpu/drm/selftests/test-drm_framebuffer.c  |  1 -
 include/drm/drm_mode_config.h                 | 18 +++++----------
 include/drm/drm_plane.h                       |  3 +++
 13 files changed, 40 insertions(+), 33 deletions(-)

-- 
2.25.1

[Nouveau] [RFC PATCH v2 1/3] drm: add support modifiers for drivers whose planes only support linear layout

[Tomohito Esaki]

The LINEAR modifier is advertised as default if a driver doesn't specify
modifiers. However, there are legacy drivers such as radeon that do not
support modifiers but infer the actual layout of the underlying buffer.
Therefore, a new flag not_support_fb_modifires is introduced for these
legacy drivers. Allow_fb_modifiers will be replaced with this new flag.

Signed-off-by: Tomohito Esaki <etom@igel.co.jp>
---
 drivers/gpu/drm/drm_plane.c   | 15 ++++++++++++---
 include/drm/drm_mode_config.h | 10 ++++++++++
 include/drm/drm_plane.h       |  3 +++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index deeec60a3315..5aa7e241971e 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -237,6 +237,10 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 				      const char *name, va_list ap)
 {
 	struct drm_mode_config *config = &dev->mode_config;
+	const uint64_t default_modifiers[] = {
+		DRM_FORMAT_MOD_LINEAR,
+		DRM_FORMAT_MOD_INVALID
+	};
 	unsigned int format_modifier_count = 0;
 	int ret;
 
@@ -277,6 +281,11 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 
 		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
 			format_modifier_count++;
+	} else {
+		if (!dev->mode_config.fb_modifiers_not_supported) {
+			format_modifiers = default_modifiers;
+			format_modifier_count = 1;
+		}
 	}
 
 	/* autoset the cap and check for consistency across all planes */
@@ -341,7 +350,7 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 		drm_object_attach_property(&plane->base, config->prop_src_h, 0);
 	}
 
-	if (config->allow_fb_modifiers)
+	if (format_modifier_count)
 		create_in_format_blob(dev, plane);
 
 	return 0;
@@ -368,8 +377,8 @@ 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.
+ * For drivers supporting modifiers, all planes will advertise
+ * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set.
  *
  * 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 48b7de80daf5..c56f298c55bd 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -920,6 +920,16 @@ struct drm_mode_config {
 	 */
 	bool allow_fb_modifiers;
 
+	/**
+	 * @fb_modifiers_not_supported:
+	 *
+	 * This flag is for legacy drivers such as radeon that do not support
+	 * modifiers but infer the actual layout of the underlying buffer.
+	 * Generally, each drivers must support modifiers, this flag should not
+	 * be set.
+	 */
+	bool fb_modifiers_not_supported;
+
 	/**
 	 * @normalize_zpos:
 	 *
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 0c1102dc4d88..cad641b1f797 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -803,6 +803,9 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
  *
  * The @drm_plane_funcs.destroy hook must be NULL.
  *
+ * For drivers supporting modifiers, all planes will advertise
+ * DRM_FORMAT_MOD_LINEAR support, if @format_modifiers is not set.
+ *
  * Returns:
  * Pointer to new plane, or ERR_PTR on failure.
  */
-- 
2.25.1

[Nouveau] [RFC PATCH v2 2/3] drm: set fb_modifiers_not_supported flag in legacy drivers

[Tomohito Esaki]

Set fb_modifiers_not_supported flag in legacy drivers whose planes
support non-linear layouts but does not support modifiers, and replace
allow_fb_modifiers with fb_modifiers_not_supported.

Signed-off-by: Tomohito Esaki <etom@igel.co.jp>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 +++---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c      | 2 ++
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c      | 2 ++
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c       | 1 +
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c       | 2 ++
 drivers/gpu/drm/nouveau/nouveau_display.c   | 6 ++++--
 drivers/gpu/drm/radeon/radeon_display.c     | 2 ++
 7 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index dc50c05f23fc..cbaea9c6cfda 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -958,7 +958,7 @@ static int amdgpu_display_verify_sizes(struct amdgpu_framebuffer *rfb)
 	int ret;
 	unsigned int i, block_width, block_height, block_size_log2;
 
-	if (!rfb->base.dev->mode_config.allow_fb_modifiers)
+	if (rfb->base.dev->mode_config.fb_modifiers_not_supported)
 		return 0;
 
 	for (i = 0; i < format_info->num_planes; ++i) {
@@ -1145,7 +1145,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	if (!dev->mode_config.allow_fb_modifiers) {
+	if (dev->mode_config.fb_modifiers_not_supported) {
 		drm_WARN_ONCE(dev, adev->family >= AMDGPU_FAMILY_AI,
 			      "GFX9+ requires FB check based on format modifier\n");
 		ret = check_tiling_flags_gfx6(rfb);
@@ -1153,7 +1153,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
 			return ret;
 	}
 
-	if (dev->mode_config.allow_fb_modifiers &&
+	if (!dev->mode_config.fb_modifiers_not_supported &&
 	    !(rfb->base.flags & DRM_MODE_FB_MODIFIERS)) {
 		ret = convert_tiling_flags_to_modifier(rfb);
 		if (ret) {
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index d1570a462a51..fb61c0814115 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2798,6 +2798,8 @@ static int dce_v10_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.preferred_depth = 24;
 	adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 
+	adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
+
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
 	r = amdgpu_display_modeset_create_props(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 18a7b3bd633b..17942a11366d 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2916,6 +2916,8 @@ static int dce_v11_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.preferred_depth = 24;
 	adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 
+	adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
+
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
 	r = amdgpu_display_modeset_create_props(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index c7803dc2b2d5..2ec99ec8e1a3 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -2674,6 +2674,7 @@ static int dce_v6_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.max_height = 16384;
 	adev_to_drm(adev)->mode_config.preferred_depth = 24;
 	adev_to_drm(adev)->mode_config.prefer_shadow = 1;
+	adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
 	r = amdgpu_display_modeset_create_props(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index b200b9e722d9..8369336cec90 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2699,6 +2699,8 @@ static int dce_v8_0_sw_init(void *handle)
 	adev_to_drm(adev)->mode_config.preferred_depth = 24;
 	adev_to_drm(adev)->mode_config.prefer_shadow = 1;
 
+	adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
+
 	adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
 
 	r = amdgpu_display_modeset_create_props(adev);
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index 929de41c281f..1ecad7fa3e8a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -711,10 +711,12 @@ nouveau_display_create(struct drm_device *dev)
 				     &disp->disp);
 		if (ret == 0) {
 			nouveau_display_create_properties(dev);
-			if (disp->disp.object.oclass < NV50_DISP)
+			if (disp->disp.object.oclass < NV50_DISP) {
+				dev->mode_config.fb_modifiers_not_supported = true;
 				ret = nv04_display_create(dev);
-			else
+			} else {
 				ret = nv50_display_create(dev);
+			}
 		}
 	} else {
 		ret = 0;
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 573154268d43..b9a07677a71e 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -1596,6 +1596,8 @@ int radeon_modeset_init(struct radeon_device *rdev)
 	rdev->ddev->mode_config.preferred_depth = 24;
 	rdev->ddev->mode_config.prefer_shadow = 1;
 
+	rdev->ddev->mode_config.fb_modifiers_not_supported = true;
+
 	rdev->ddev->mode_config.fb_base = rdev->mc.aper_base;
 
 	ret = radeon_modeset_create_props(rdev);
-- 
2.25.1

[Nouveau] [RFC PATCH v2 3/3] drm: replace allow_fb_modifiers with fb_modifiers_not_supported

[Tomohito Esaki]

Since almost drivers support fb modifiers, allow_fb_modifiers is
replaced with fb_modifiers_not_supported and removed.

Signed-off-by: Tomohito Esaki <etom@igel.co.jp>
---
 drivers/gpu/drm/drm_framebuffer.c                |  6 +++---
 drivers/gpu/drm/drm_ioctl.c                      |  2 +-
 drivers/gpu/drm/drm_plane.c                      |  9 ---------
 drivers/gpu/drm/selftests/test-drm_framebuffer.c |  1 -
 include/drm/drm_mode_config.h                    | 16 ----------------
 5 files changed, 4 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 07f5abc875e9..4562a8b86579 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -309,7 +309,7 @@ drm_internal_framebuffer_create(struct drm_device *dev,
 	}
 
 	if (r->flags & DRM_MODE_FB_MODIFIERS &&
-	    !dev->mode_config.allow_fb_modifiers) {
+	    dev->mode_config.fb_modifiers_not_supported) {
 		DRM_DEBUG_KMS("driver does not support fb modifiers\n");
 		return ERR_PTR(-EINVAL);
 	}
@@ -594,7 +594,7 @@ int drm_mode_getfb2_ioctl(struct drm_device *dev,
 	r->pixel_format = fb->format->format;
 
 	r->flags = 0;
-	if (dev->mode_config.allow_fb_modifiers)
+	if (!dev->mode_config.fb_modifiers_not_supported)
 		r->flags |= DRM_MODE_FB_MODIFIERS;
 
 	for (i = 0; i < ARRAY_SIZE(r->handles); i++) {
@@ -607,7 +607,7 @@ int drm_mode_getfb2_ioctl(struct drm_device *dev,
 	for (i = 0; i < fb->format->num_planes; i++) {
 		r->pitches[i] = fb->pitches[i];
 		r->offsets[i] = fb->offsets[i];
-		if (dev->mode_config.allow_fb_modifiers)
+		if (!dev->mode_config.fb_modifiers_not_supported)
 			r->modifier[i] = fb->modifier;
 	}
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8b8744dcf691..51fcf1298023 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -297,7 +297,7 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 			req->value = 64;
 		break;
 	case DRM_CAP_ADDFB2_MODIFIERS:
-		req->value = dev->mode_config.allow_fb_modifiers;
+		req->value = !dev->mode_config.fb_modifiers_not_supported;
 		break;
 	case DRM_CAP_CRTC_IN_VBLANK_EVENT:
 		req->value = 1;
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 5aa7e241971e..89a3d044ab59 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -288,15 +288,6 @@ static int __drm_universal_plane_init(struct drm_device *dev,
 		}
 	}
 
-	/* autoset the cap and check for consistency across all planes */
-	if (format_modifier_count) {
-		drm_WARN_ON(dev, !config->allow_fb_modifiers &&
-			    !list_empty(&config->plane_list));
-		config->allow_fb_modifiers = true;
-	} else {
-		drm_WARN_ON(dev, config->allow_fb_modifiers);
-	}
-
 	plane->modifier_count = format_modifier_count;
 	plane->modifiers = kmalloc_array(format_modifier_count,
 					 sizeof(format_modifiers[0]),
diff --git a/drivers/gpu/drm/selftests/test-drm_framebuffer.c b/drivers/gpu/drm/selftests/test-drm_framebuffer.c
index 61b44d3a6a61..f6d66285c5fc 100644
--- a/drivers/gpu/drm/selftests/test-drm_framebuffer.c
+++ b/drivers/gpu/drm/selftests/test-drm_framebuffer.c
@@ -323,7 +323,6 @@ static struct drm_device mock_drm_device = {
 		.max_width = MAX_WIDTH,
 		.min_height = MIN_HEIGHT,
 		.max_height = MAX_HEIGHT,
-		.allow_fb_modifiers = true,
 		.funcs = &mock_config_funcs,
 	},
 };
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index c56f298c55bd..6fd13d6510f1 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -904,22 +904,6 @@ struct drm_mode_config {
 	 */
 	bool async_page_flip;
 
-	/**
-	 * @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:
-	 *
-	 * If this is set the driver must fill out the full implicit modifier
-	 * information in their &drm_mode_config_funcs.fb_create hook for legacy
-	 * userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
-	 * broken for modifier aware userspace.
-	 */
-	bool allow_fb_modifiers;
-
 	/**
 	 * @fb_modifiers_not_supported:
 	 *
-- 
2.25.1

Re: [Nouveau] [RFC PATCH v2 0/3] Add support modifiers for drivers whose planes only support linear layout

[Daniel Stone]

Hi Esaki-san,

On Thu, 13 Jan 2022 at 09:44, Tomohito Esaki <etom@igel.co.jp> wrote:
> Some drivers whose planes only support linear layout fb do not support format
> modifiers.
> These drivers should support modifiers, however the DRM core should handle this
> rather than open-coding in every driver.
>
> In this patch series, these drivers expose format modifiers based on the
> following suggestion[1].

Thanks for the series, it looks like the right thing to do.

Can you please change the patch ordering though? At the moment there
will be a bisection break at patch #1, because the legacy drivers will
suddenly start gaining modifier support, before it is removed in patch
#2.

I think a better order would be:
  1: add fb_modifiers_not_supported flag to core and drivers
  2: add default modifiers (and set allow_fb_modifiers) if
fb_modifiers_not_supported flag is not set
  3: remove allow_fb_modifiers flag

Cheers,
Daniel

Re: [Nouveau] [RFC PATCH v2 2/3] drm: set fb_modifiers_not_supported flag in legacy drivers

[Bas Nieuwenhuizen]

I think we'll also want to do a conditional disable for DC
(drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c) since it only
enables modifiers on newer HW.  Something like "if (modifiers == NULL)
fb_modifiers_not_supported = true;" in amdgpu_dm_plane_init.

On Thu, Jan 13, 2022 at 10:44 AM Tomohito Esaki <etom@igel.co.jp> wrote:
>
> Set fb_modifiers_not_supported flag in legacy drivers whose planes
> support non-linear layouts but does not support modifiers, and replace
> allow_fb_modifiers with fb_modifiers_not_supported.
>
> Signed-off-by: Tomohito Esaki <etom@igel.co.jp>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 +++---
>  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c      | 2 ++
>  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c      | 2 ++
>  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c       | 1 +
>  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c       | 2 ++
>  drivers/gpu/drm/nouveau/nouveau_display.c   | 6 ++++--
>  drivers/gpu/drm/radeon/radeon_display.c     | 2 ++
>  7 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index dc50c05f23fc..cbaea9c6cfda 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -958,7 +958,7 @@ static int amdgpu_display_verify_sizes(struct amdgpu_framebuffer *rfb)
>         int ret;
>         unsigned int i, block_width, block_height, block_size_log2;
>
> -       if (!rfb->base.dev->mode_config.allow_fb_modifiers)
> +       if (rfb->base.dev->mode_config.fb_modifiers_not_supported)
>                 return 0;
>
>         for (i = 0; i < format_info->num_planes; ++i) {
> @@ -1145,7 +1145,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
>         if (ret)
>                 return ret;
>
> -       if (!dev->mode_config.allow_fb_modifiers) {
> +       if (dev->mode_config.fb_modifiers_not_supported) {
>                 drm_WARN_ONCE(dev, adev->family >= AMDGPU_FAMILY_AI,
>                               "GFX9+ requires FB check based on format modifier\n");
>                 ret = check_tiling_flags_gfx6(rfb);
> @@ -1153,7 +1153,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
>                         return ret;
>         }
>
> -       if (dev->mode_config.allow_fb_modifiers &&
> +       if (!dev->mode_config.fb_modifiers_not_supported &&
>             !(rfb->base.flags & DRM_MODE_FB_MODIFIERS)) {
>                 ret = convert_tiling_flags_to_modifier(rfb);
>                 if (ret) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> index d1570a462a51..fb61c0814115 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
> @@ -2798,6 +2798,8 @@ static int dce_v10_0_sw_init(void *handle)
>         adev_to_drm(adev)->mode_config.preferred_depth = 24;
>         adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>
> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
> +
>         adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>
>         r = amdgpu_display_modeset_create_props(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> index 18a7b3bd633b..17942a11366d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
> @@ -2916,6 +2916,8 @@ static int dce_v11_0_sw_init(void *handle)
>         adev_to_drm(adev)->mode_config.preferred_depth = 24;
>         adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>
> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
> +
>         adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>
>         r = amdgpu_display_modeset_create_props(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> index c7803dc2b2d5..2ec99ec8e1a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
> @@ -2674,6 +2674,7 @@ static int dce_v6_0_sw_init(void *handle)
>         adev_to_drm(adev)->mode_config.max_height = 16384;
>         adev_to_drm(adev)->mode_config.preferred_depth = 24;
>         adev_to_drm(adev)->mode_config.prefer_shadow = 1;
> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
>         adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>
>         r = amdgpu_display_modeset_create_props(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> index b200b9e722d9..8369336cec90 100644
> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> @@ -2699,6 +2699,8 @@ static int dce_v8_0_sw_init(void *handle)
>         adev_to_drm(adev)->mode_config.preferred_depth = 24;
>         adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>
> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
> +
>         adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>
>         r = amdgpu_display_modeset_create_props(adev);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 929de41c281f..1ecad7fa3e8a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
> @@ -711,10 +711,12 @@ nouveau_display_create(struct drm_device *dev)
>                                      &disp->disp);
>                 if (ret == 0) {
>                         nouveau_display_create_properties(dev);
> -                       if (disp->disp.object.oclass < NV50_DISP)
> +                       if (disp->disp.object.oclass < NV50_DISP) {
> +                               dev->mode_config.fb_modifiers_not_supported = true;
>                                 ret = nv04_display_create(dev);
> -                       else
> +                       } else {
>                                 ret = nv50_display_create(dev);
> +                       }
>                 }
>         } else {
>                 ret = 0;
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 573154268d43..b9a07677a71e 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -1596,6 +1596,8 @@ int radeon_modeset_init(struct radeon_device *rdev)
>         rdev->ddev->mode_config.preferred_depth = 24;
>         rdev->ddev->mode_config.prefer_shadow = 1;
>
> +       rdev->ddev->mode_config.fb_modifiers_not_supported = true;
> +
>         rdev->ddev->mode_config.fb_base = rdev->mc.aper_base;
>
>         ret = radeon_modeset_create_props(rdev);
> --
> 2.25.1
>

Re: [Nouveau] [RFC PATCH v2 0/3] Add support modifiers for drivers whose planes only support linear layout

[Esaki Tomohito]

Hi Daniel-san,

Thank you for your comments.

On 2022/01/13 22:44, Daniel Stone wrote:
> Hi Esaki-san,
> 
> On Thu, 13 Jan 2022 at 09:44, Tomohito Esaki <etom@igel.co.jp> wrote:
>> Some drivers whose planes only support linear layout fb do not support format
>> modifiers.
>> These drivers should support modifiers, however the DRM core should handle this
>> rather than open-coding in every driver.
>>
>> In this patch series, these drivers expose format modifiers based on the
>> following suggestion[1].
> 
> Thanks for the series, it looks like the right thing to do.
> 
> Can you please change the patch ordering though? At the moment there
> will be a bisection break at patch #1, because the legacy drivers will
> suddenly start gaining modifier support, before it is removed in patch
> #2.
> 
> I think a better order would be:
>    1: add fb_modifiers_not_supported flag to core and drivers
>    2: add default modifiers (and set allow_fb_modifiers) if
> fb_modifiers_not_supported flag is not set
>    3: remove allow_fb_modifiers flag
> 
I agree to your proposal.
I will fix these patches.

-- 
--------------------------
株式会社イーゲル
江崎　朋人
etom@igel.co.jp

Re: [Nouveau] [RFC PATCH v2 2/3] drm: set fb_modifiers_not_supported flag in legacy drivers

[Esaki Tomohito]

Hi,

Thank you for your comment.

On 2022/01/14 2:56, Bas Nieuwenhuizen wrote:
> I think we'll also want to do a conditional disable for DC
> (drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c) since it only
> enables modifiers on newer HW.  Something like "if (modifiers == NULL)
> fb_modifiers_not_supported = true;" in amdgpu_dm_plane_init.

Ok, I'll add a conditional disable in amdgpu_dm_plane_init().

Thanks,
Tomohito Esaki

> On Thu, Jan 13, 2022 at 10:44 AM Tomohito Esaki <etom@igel.co.jp> wrote:
>>
>> Set fb_modifiers_not_supported flag in legacy drivers whose planes
>> support non-linear layouts but does not support modifiers, and replace
>> allow_fb_modifiers with fb_modifiers_not_supported.
>>
>> Signed-off-by: Tomohito Esaki <etom@igel.co.jp>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 +++---
>>   drivers/gpu/drm/amd/amdgpu/dce_v10_0.c      | 2 ++
>>   drivers/gpu/drm/amd/amdgpu/dce_v11_0.c      | 2 ++
>>   drivers/gpu/drm/amd/amdgpu/dce_v6_0.c       | 1 +
>>   drivers/gpu/drm/amd/amdgpu/dce_v8_0.c       | 2 ++
>>   drivers/gpu/drm/nouveau/nouveau_display.c   | 6 ++++--
>>   drivers/gpu/drm/radeon/radeon_display.c     | 2 ++
>>   7 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> index dc50c05f23fc..cbaea9c6cfda 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
>> @@ -958,7 +958,7 @@ static int amdgpu_display_verify_sizes(struct amdgpu_framebuffer *rfb)
>>          int ret;
>>          unsigned int i, block_width, block_height, block_size_log2;
>>
>> -       if (!rfb->base.dev->mode_config.allow_fb_modifiers)
>> +       if (rfb->base.dev->mode_config.fb_modifiers_not_supported)
>>                  return 0;
>>
>>          for (i = 0; i < format_info->num_planes; ++i) {
>> @@ -1145,7 +1145,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
>>          if (ret)
>>                  return ret;
>>
>> -       if (!dev->mode_config.allow_fb_modifiers) {
>> +       if (dev->mode_config.fb_modifiers_not_supported) {
>>                  drm_WARN_ONCE(dev, adev->family >= AMDGPU_FAMILY_AI,
>>                                "GFX9+ requires FB check based on format modifier\n");
>>                  ret = check_tiling_flags_gfx6(rfb);
>> @@ -1153,7 +1153,7 @@ int amdgpu_display_framebuffer_init(struct drm_device *dev,
>>                          return ret;
>>          }
>>
>> -       if (dev->mode_config.allow_fb_modifiers &&
>> +       if (!dev->mode_config.fb_modifiers_not_supported &&
>>              !(rfb->base.flags & DRM_MODE_FB_MODIFIERS)) {
>>                  ret = convert_tiling_flags_to_modifier(rfb);
>>                  if (ret) {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> index d1570a462a51..fb61c0814115 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
>> @@ -2798,6 +2798,8 @@ static int dce_v10_0_sw_init(void *handle)
>>          adev_to_drm(adev)->mode_config.preferred_depth = 24;
>>          adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>>
>> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
>> +
>>          adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>>
>>          r = amdgpu_display_modeset_create_props(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> index 18a7b3bd633b..17942a11366d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
>> @@ -2916,6 +2916,8 @@ static int dce_v11_0_sw_init(void *handle)
>>          adev_to_drm(adev)->mode_config.preferred_depth = 24;
>>          adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>>
>> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
>> +
>>          adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>>
>>          r = amdgpu_display_modeset_create_props(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
>> index c7803dc2b2d5..2ec99ec8e1a3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
>> @@ -2674,6 +2674,7 @@ static int dce_v6_0_sw_init(void *handle)
>>          adev_to_drm(adev)->mode_config.max_height = 16384;
>>          adev_to_drm(adev)->mode_config.preferred_depth = 24;
>>          adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
>>          adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>>
>>          r = amdgpu_display_modeset_create_props(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> index b200b9e722d9..8369336cec90 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
>> @@ -2699,6 +2699,8 @@ static int dce_v8_0_sw_init(void *handle)
>>          adev_to_drm(adev)->mode_config.preferred_depth = 24;
>>          adev_to_drm(adev)->mode_config.prefer_shadow = 1;
>>
>> +       adev_to_drm(adev)->mode_config.fb_modifiers_not_supported = true;
>> +
>>          adev_to_drm(adev)->mode_config.fb_base = adev->gmc.aper_base;
>>
>>          r = amdgpu_display_modeset_create_props(adev);
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
>> index 929de41c281f..1ecad7fa3e8a 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>> @@ -711,10 +711,12 @@ nouveau_display_create(struct drm_device *dev)
>>                                       &disp->disp);
>>                  if (ret == 0) {
>>                          nouveau_display_create_properties(dev);
>> -                       if (disp->disp.object.oclass < NV50_DISP)
>> +                       if (disp->disp.object.oclass < NV50_DISP) {
>> +                               dev->mode_config.fb_modifiers_not_supported = true;
>>                                  ret = nv04_display_create(dev);
>> -                       else
>> +                       } else {
>>                                  ret = nv50_display_create(dev);
>> +                       }
>>                  }
>>          } else {
>>                  ret = 0;
>> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
>> index 573154268d43..b9a07677a71e 100644
>> --- a/drivers/gpu/drm/radeon/radeon_display.c
>> +++ b/drivers/gpu/drm/radeon/radeon_display.c
>> @@ -1596,6 +1596,8 @@ int radeon_modeset_init(struct radeon_device *rdev)
>>          rdev->ddev->mode_config.preferred_depth = 24;
>>          rdev->ddev->mode_config.prefer_shadow = 1;
>>
>> +       rdev->ddev->mode_config.fb_modifiers_not_supported = true;
>> +
>>          rdev->ddev->mode_config.fb_base = rdev->mc.aper_base;
>>
>>          ret = radeon_modeset_create_props(rdev);
>> --
>> 2.25.1
>>