[PATCH v5 0/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[PATCH v5 0/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Maarten Lankhorst]

Introduce a second patch to move the changes to intel_fb instead.

Maarten Lankhorst (2):
  drm/xe: Align all VRAM scanout buffers to 64k physical pages when
    needed.
  drm/i915/display: Add function for checking 64k physical alignment
    workaround

 drivers/gpu/drm/i915/display/intel_fb.c  | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fb.h  |  2 ++
 drivers/gpu/drm/xe/display/intel_fb_bo.c |  6 ++++++
 drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
 drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
 5 files changed, 44 insertions(+), 2 deletions(-)

-- 
2.45.2

[PATCH v5 1/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Maarten Lankhorst]

For CCS formats on affected platforms, CCS can be used freely, but
display engine requires a multiple of 64k physical pages. No other
changes are needed.

At the BO creation time we don't know if the BO will be used for CCS
or not. If the scanout flag is set, and the BO is a multiple of 64k,
we take the safe route and force the physical alignment of 64k pages.

If the BO is not a multiple of 64k, or the scanout flag was not set
at BO creation, we reject it for usage as CCS in display. The physical
pages are likely not aligned correctly, and this will cause corruption
when used as FB.

The scanout flag and size being a multiple of 64k are used together
to enforce 64k physical placement.

VM_BIND is completely unaffected, mappings to a VM can still be aligned
to 4k, just like for normal buffers.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Co-developed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
---
 drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++++
 drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
 drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
index f835492f73fb4..6775c2557b9df 100644
--- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
+++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
@@ -28,6 +28,11 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
 	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
 	int ret;
 
+	/* Only this specific format is affected, and it's only available on VRAM */
+	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
+			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
+		return -EINVAL;
+
 	xe_bo_get(bo);
 
 	ret = ttm_bo_reserve(&bo->ttm, true, false, NULL);
diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
index cbe7bf098970f..41297b5797173 100644
--- a/drivers/gpu/drm/xe/xe_bo.c
+++ b/drivers/gpu/drm/xe/xe_bo.c
@@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
 
 	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
 
+	/* CCS formats need physical placement at a 64K alignment in VRAM. */
+	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
+	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
+	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
+	    !(args->size % SZ_64K))
+		bo_flags |= XE_BO_FLAG_NEEDS_64K;
+
 	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
 		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_VRAM_MASK)))
 			return -EINVAL;
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 4cc13eddb6b32..3eb76d874eb28 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2878,7 +2878,16 @@ static int xe_vm_bind_ioctl_validate_bo(struct xe_device *xe, struct xe_bo *bo,
 		return -EINVAL;
 	}
 
-	if (bo->flags & XE_BO_FLAG_INTERNAL_64K) {
+	/*
+	 * Some platforms require 64k VM_BIND alignment,
+	 * specifically those with XE_VRAM_FLAGS_NEED64K.
+	 *
+	 * Other platforms may have BO's set to 64k physical placement,
+	 * but can be mapped at 4k offsets anyway. This check is only
+	 * there for the former case.
+	 */
+	if ((bo->flags & XE_BO_FLAG_INTERNAL_64K) &&
+	    (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)) {
 		if (XE_IOCTL_DBG(xe, obj_offset &
 				 XE_64K_PAGE_MASK) ||
 		    XE_IOCTL_DBG(xe, addr & XE_64K_PAGE_MASK) ||
-- 
2.45.2

[PATCH v5 2/2] drm/i915/display: Add function for checking 64k physical alignment workaround

[Maarten Lankhorst]

Instead of hardcoding the modifier in xe, the alternative approach is
setting a bit in the modifier description for 64k phys requirement.

This removes the hardcoding for the modifier in CCS.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

---
This might make the changes more palatable, but I personally believe
that hardcoding for a one-off check is fine. My optimism is that when
creating a new tiling format, the previous lessons will be taken into
account and this case will be tested.

Yeah, maybe just go for this patch too..
---
 drivers/gpu/drm/i915/display/intel_fb.c  | 20 +++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fb.h  |  2 ++
 drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++--
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index d2716915d046d..5139172a8a3d1 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -169,7 +169,7 @@ static const struct intel_modifier_desc intel_modifiers[] = {
 	}, {
 		.modifier = I915_FORMAT_MOD_4_TILED_BMG_CCS,
 		.display_ver = { 14, -1 },
-		.plane_caps = INTEL_PLANE_CAP_TILING_4,
+		.plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_NEED64K_PHYS_WA,
 	}, {
 		.modifier = I915_FORMAT_MOD_4_TILED_MTL_MC_CCS,
 		.display_ver = { 14, 14 },
@@ -420,6 +420,24 @@ bool intel_fb_is_mc_ccs_modifier(u64 modifier)
 				      INTEL_PLANE_CAP_CCS_MC);
 }
 
+/**
+ * intel_fb_needs_64k_phys_wa: Check if modifier requires 64k phys placement workaround.
+ * @modifier: Modifier to check
+ *
+ * Returns:
+ * Returns %true if @modifier requires 64k physical page alignment.
+ */
+bool intel_fb_needs_64k_phys_wa(u64 modifier)
+{
+	const struct intel_modifier_desc *md = lookup_modifier_or_null(modifier);
+
+	if (!md)
+		return false;
+
+	return plane_caps_contain_any(md->plane_caps,
+				      INTEL_PLANE_CAP_NEED64K_PHYS_WA);
+}
+
 static bool check_modifier_display_ver_range(const struct intel_modifier_desc *md,
 					     u8 display_ver_from, u8 display_ver_until)
 {
diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
index 6dee0c8b7f226..8a84b9abf9b91 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.h
+++ b/drivers/gpu/drm/i915/display/intel_fb.h
@@ -28,11 +28,13 @@ struct intel_plane_state;
 #define INTEL_PLANE_CAP_TILING_Y	BIT(4)
 #define INTEL_PLANE_CAP_TILING_Yf	BIT(5)
 #define INTEL_PLANE_CAP_TILING_4	BIT(6)
+#define INTEL_PLANE_CAP_NEED64K_PHYS_WA	BIT(7)
 
 bool intel_fb_is_tiled_modifier(u64 modifier);
 bool intel_fb_is_ccs_modifier(u64 modifier);
 bool intel_fb_is_rc_ccs_cc_modifier(u64 modifier);
 bool intel_fb_is_mc_ccs_modifier(u64 modifier);
+bool intel_fb_needs_64k_phys_wa(u64 modifier);
 
 bool intel_fb_is_ccs_aux_plane(const struct drm_framebuffer *fb, int color_plane);
 int intel_fb_rc_ccs_cc_plane(const struct drm_framebuffer *fb);
diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
index 6775c2557b9df..64bcd10d3f538 100644
--- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
+++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
@@ -7,6 +7,7 @@
 #include <drm/ttm/ttm_bo.h>
 
 #include "intel_display_types.h"
+#include "intel_fb.h"
 #include "intel_fb_bo.h"
 #include "xe_bo.h"
 
@@ -28,8 +29,8 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
 	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
 	int ret;
 
-	/* Only this specific format is affected, and it's only available on VRAM */
-	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
+	/* Some modifiers require aligned 64k phys pages. */
+	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys_wa(mode_cmd->modifier[0]) &&
 			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
 		return -EINVAL;
 
-- 
2.45.2

✗ Fi.CI.CHECKPATCH: warning for drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Patchwork]

== Series Details ==

Series: drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.
URL   : https://patchwork.freedesktop.org/series/137785/
State : warning

== Summary ==

Error: dim checkpatch failed
b3895b347e5e drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.
-:30: WARNING:BAD_SIGN_OFF: Co-developed-by: must be immediately followed by Signed-off-by:
#30: 
Co-developed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>

-:47: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#47: FILE: drivers/gpu/drm/xe/display/intel_fb_bo.c:33:
+	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
+			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))

total: 0 errors, 1 warnings, 1 checks, 41 lines checked
eee56e82b172 drm/i915/display: Add function for checking 64k physical alignment workaround
-:90: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#90: FILE: drivers/gpu/drm/xe/display/intel_fb_bo.c:34:
+	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys_wa(mode_cmd->modifier[0]) &&
 			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))

total: 0 errors, 0 warnings, 1 checks, 62 lines checked

Re: [PATCH v5 2/2] drm/i915/display: Add function for checking 64k physical alignment workaround

[Rodrigo Vivi]

On Mon, Aug 26, 2024 at 11:50:41AM +0200, Maarten Lankhorst wrote:
> Instead of hardcoding the modifier in xe, the alternative approach is
> setting a bit in the modifier description for 64k phys requirement.
> 
> This removes the hardcoding for the modifier in CCS.

Please change this commit message to explain the change and why we need
that, instead of comparing this to the alternative option.

> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> ---
> This might make the changes more palatable, but I personally believe
> that hardcoding for a one-off check is fine. My optimism is that when
> creating a new tiling format, the previous lessons will be taken into
> account and this case will be tested.
> 
> Yeah, maybe just go for this patch too..
> ---
>  drivers/gpu/drm/i915/display/intel_fb.c  | 20 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_fb.h  |  2 ++
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++--
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index d2716915d046d..5139172a8a3d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -169,7 +169,7 @@ static const struct intel_modifier_desc intel_modifiers[] = {
>  	}, {
>  		.modifier = I915_FORMAT_MOD_4_TILED_BMG_CCS,
>  		.display_ver = { 14, -1 },
> -		.plane_caps = INTEL_PLANE_CAP_TILING_4,
> +		.plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_NEED64K_PHYS_WA,

This is not a Workaround, but a design decision. Let's avoid the 'WA'.


>  	}, {
>  		.modifier = I915_FORMAT_MOD_4_TILED_MTL_MC_CCS,
>  		.display_ver = { 14, 14 },
> @@ -420,6 +420,24 @@ bool intel_fb_is_mc_ccs_modifier(u64 modifier)
>  				      INTEL_PLANE_CAP_CCS_MC);
>  }
>  
> +/**
> + * intel_fb_needs_64k_phys_wa: Check if modifier requires 64k phys placement workaround.
> + * @modifier: Modifier to check
> + *
> + * Returns:
> + * Returns %true if @modifier requires 64k physical page alignment.
> + */
> +bool intel_fb_needs_64k_phys_wa(u64 modifier)
> +{
> +	const struct intel_modifier_desc *md = lookup_modifier_or_null(modifier);
> +
> +	if (!md)
> +		return false;
> +
> +	return plane_caps_contain_any(md->plane_caps,
> +				      INTEL_PLANE_CAP_NEED64K_PHYS_WA);
> +}
> +
>  static bool check_modifier_display_ver_range(const struct intel_modifier_desc *md,
>  					     u8 display_ver_from, u8 display_ver_until)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 6dee0c8b7f226..8a84b9abf9b91 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -28,11 +28,13 @@ struct intel_plane_state;
>  #define INTEL_PLANE_CAP_TILING_Y	BIT(4)
>  #define INTEL_PLANE_CAP_TILING_Yf	BIT(5)
>  #define INTEL_PLANE_CAP_TILING_4	BIT(6)
> +#define INTEL_PLANE_CAP_NEED64K_PHYS_WA	BIT(7)

My concern with this version is that this doesn't seems to fit
the 'capability' bits. This is not a 'capability', but a limitation.

Wit this in mind I wouldn't mind having the previous patch where
the limitation was recorded.

But well, this case here make it more expansible for any future
platform with similar design decisions.

>  
>  bool intel_fb_is_tiled_modifier(u64 modifier);
>  bool intel_fb_is_ccs_modifier(u64 modifier);
>  bool intel_fb_is_rc_ccs_cc_modifier(u64 modifier);
>  bool intel_fb_is_mc_ccs_modifier(u64 modifier);
> +bool intel_fb_needs_64k_phys_wa(u64 modifier);
>  
>  bool intel_fb_is_ccs_aux_plane(const struct drm_framebuffer *fb, int color_plane);
>  int intel_fb_rc_ccs_cc_plane(const struct drm_framebuffer *fb);
> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> index 6775c2557b9df..64bcd10d3f538 100644
> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> @@ -7,6 +7,7 @@
>  #include <drm/ttm/ttm_bo.h>
>  
>  #include "intel_display_types.h"
> +#include "intel_fb.h"
>  #include "intel_fb_bo.h"
>  #include "xe_bo.h"
>  
> @@ -28,8 +29,8 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> -	/* Only this specific format is affected, and it's only available on VRAM */
> -	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
> +	/* Some modifiers require aligned 64k phys pages. */
> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys_wa(mode_cmd->modifier[0]) &&
>  			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
>  		return -EINVAL;
>  
> -- 
> 2.45.2
> 

✓ Fi.CI.BAT: success for drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

== Series Details ==

Series: drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.
URL   : https://patchwork.freedesktop.org/series/137785/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_15291 -> Patchwork_137785v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137785v1/index.html

Participating hosts (37 -> 33)
------------------------------

  Missing    (4): bat-dg1-7 bat-arlh-2 bat-jsl-1 fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_137785v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@hangcheck:
    - bat-arls-2:         [PASS][1] -> [DMESG-WARN][2] ([i915#11349])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15291/bat-arls-2/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137785v1/bat-arls-2/igt@i915_selftest@live@hangcheck.html
    - bat-arls-1:         [PASS][3] -> [DMESG-WARN][4] ([i915#11349])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15291/bat-arls-1/igt@i915_selftest@live@hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137785v1/bat-arls-1/igt@i915_selftest@live@hangcheck.html

  
#### Possible fixes ####

  * igt@gem_exec_fence@basic-await@bcs0:
    - bat-twl-2:          [FAIL][5] -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15291/bat-twl-2/igt@gem_exec_fence@basic-await@bcs0.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137785v1/bat-twl-2/igt@gem_exec_fence@basic-await@bcs0.html

  
  [i915#11349]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11349


Build changes
-------------

  * Linux: CI_DRM_15291 -> Patchwork_137785v1

  CI-20190529: 20190529
  CI_DRM_15291: 3180bea570dadc713d6f1cc50662818d684cfb4a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7991: e5cbe548dbd6ee44200a83745a605643a1a4c714 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_137785v1: 3180bea570dadc713d6f1cc50662818d684cfb4a @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137785v1/index.html

[-- Attachment #2: Type: text/html, Size: 3021 bytes --]

Re: [PATCH v5 1/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Zbigniew Kempczyński]

On Mon, Aug 26, 2024 at 11:50:40AM +0200, Maarten Lankhorst wrote:
> For CCS formats on affected platforms, CCS can be used freely, but
> display engine requires a multiple of 64k physical pages. No other
> changes are needed.
> 
> At the BO creation time we don't know if the BO will be used for CCS
> or not. If the scanout flag is set, and the BO is a multiple of 64k,
> we take the safe route and force the physical alignment of 64k pages.
> 
> If the BO is not a multiple of 64k, or the scanout flag was not set
> at BO creation, we reject it for usage as CCS in display. The physical
> pages are likely not aligned correctly, and this will cause corruption
> when used as FB.
> 
> The scanout flag and size being a multiple of 64k are used together
> to enforce 64k physical placement.
> 
> VM_BIND is completely unaffected, mappings to a VM can still be aligned
> to 4k, just like for normal buffers.

For me patch and explanation looks correct. According to submitting
patches rules please add my s-o-b before the merge.

--
Zbigniew


> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Co-developed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
> ---
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++++
>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> index f835492f73fb4..6775c2557b9df 100644
> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> @@ -28,6 +28,11 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> +	/* Only this specific format is affected, and it's only available on VRAM */
> +	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> +		return -EINVAL;
> +
>  	xe_bo_get(bo);
>  
>  	ret = ttm_bo_reserve(&bo->ttm, true, false, NULL);
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index cbe7bf098970f..41297b5797173 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>  
>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>  
> +	/* CCS formats need physical placement at a 64K alignment in VRAM. */
> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
> +	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
> +	    !(args->size % SZ_64K))
> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
> +
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
>  		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_VRAM_MASK)))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 4cc13eddb6b32..3eb76d874eb28 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2878,7 +2878,16 @@ static int xe_vm_bind_ioctl_validate_bo(struct xe_device *xe, struct xe_bo *bo,
>  		return -EINVAL;
>  	}
>  
> -	if (bo->flags & XE_BO_FLAG_INTERNAL_64K) {
> +	/*
> +	 * Some platforms require 64k VM_BIND alignment,
> +	 * specifically those with XE_VRAM_FLAGS_NEED64K.
> +	 *
> +	 * Other platforms may have BO's set to 64k physical placement,
> +	 * but can be mapped at 4k offsets anyway. This check is only
> +	 * there for the former case.
> +	 */
> +	if ((bo->flags & XE_BO_FLAG_INTERNAL_64K) &&
> +	    (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)) {
>  		if (XE_IOCTL_DBG(xe, obj_offset &
>  				 XE_64K_PAGE_MASK) ||
>  		    XE_IOCTL_DBG(xe, addr & XE_64K_PAGE_MASK) ||
> -- 
> 2.45.2
> 

Re: [PATCH v5 2/2] drm/i915/display: Add function for checking 64k physical alignment workaround

[Zbigniew Kempczyński]

On Mon, Aug 26, 2024 at 11:50:41AM +0200, Maarten Lankhorst wrote:
> Instead of hardcoding the modifier in xe, the alternative approach is
> setting a bit in the modifier description for 64k phys requirement.
> 
> This removes the hardcoding for the modifier in CCS.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> ---
> This might make the changes more palatable, but I personally believe
> that hardcoding for a one-off check is fine. My optimism is that when
> creating a new tiling format, the previous lessons will be taken into
> account and this case will be tested.
> 
> Yeah, maybe just go for this patch too..

It's likely it is not one-time check and it will be used in the future
so I would like to avoid hardcoding and changing in two places instead
of set appropriate cap once. I asked about something like that and with
respect to Rodrigo comment according to cap name:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>

--
Zbigniew

> ---
>  drivers/gpu/drm/i915/display/intel_fb.c  | 20 +++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_fb.h  |  2 ++
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++--
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index d2716915d046d..5139172a8a3d1 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -169,7 +169,7 @@ static const struct intel_modifier_desc intel_modifiers[] = {
>  	}, {
>  		.modifier = I915_FORMAT_MOD_4_TILED_BMG_CCS,
>  		.display_ver = { 14, -1 },
> -		.plane_caps = INTEL_PLANE_CAP_TILING_4,
> +		.plane_caps = INTEL_PLANE_CAP_TILING_4 | INTEL_PLANE_CAP_NEED64K_PHYS_WA,
>  	}, {
>  		.modifier = I915_FORMAT_MOD_4_TILED_MTL_MC_CCS,
>  		.display_ver = { 14, 14 },
> @@ -420,6 +420,24 @@ bool intel_fb_is_mc_ccs_modifier(u64 modifier)
>  				      INTEL_PLANE_CAP_CCS_MC);
>  }
>  
> +/**
> + * intel_fb_needs_64k_phys_wa: Check if modifier requires 64k phys placement workaround.
> + * @modifier: Modifier to check
> + *
> + * Returns:
> + * Returns %true if @modifier requires 64k physical page alignment.
> + */
> +bool intel_fb_needs_64k_phys_wa(u64 modifier)
> +{
> +	const struct intel_modifier_desc *md = lookup_modifier_or_null(modifier);
> +
> +	if (!md)
> +		return false;
> +
> +	return plane_caps_contain_any(md->plane_caps,
> +				      INTEL_PLANE_CAP_NEED64K_PHYS_WA);
> +}
> +
>  static bool check_modifier_display_ver_range(const struct intel_modifier_desc *md,
>  					     u8 display_ver_from, u8 display_ver_until)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 6dee0c8b7f226..8a84b9abf9b91 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -28,11 +28,13 @@ struct intel_plane_state;
>  #define INTEL_PLANE_CAP_TILING_Y	BIT(4)
>  #define INTEL_PLANE_CAP_TILING_Yf	BIT(5)
>  #define INTEL_PLANE_CAP_TILING_4	BIT(6)
> +#define INTEL_PLANE_CAP_NEED64K_PHYS_WA	BIT(7)
>  
>  bool intel_fb_is_tiled_modifier(u64 modifier);
>  bool intel_fb_is_ccs_modifier(u64 modifier);
>  bool intel_fb_is_rc_ccs_cc_modifier(u64 modifier);
>  bool intel_fb_is_mc_ccs_modifier(u64 modifier);
> +bool intel_fb_needs_64k_phys_wa(u64 modifier);
>  
>  bool intel_fb_is_ccs_aux_plane(const struct drm_framebuffer *fb, int color_plane);
>  int intel_fb_rc_ccs_cc_plane(const struct drm_framebuffer *fb);
> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> index 6775c2557b9df..64bcd10d3f538 100644
> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> @@ -7,6 +7,7 @@
>  #include <drm/ttm/ttm_bo.h>
>  
>  #include "intel_display_types.h"
> +#include "intel_fb.h"
>  #include "intel_fb_bo.h"
>  #include "xe_bo.h"
>  
> @@ -28,8 +29,8 @@ int intel_fb_bo_framebuffer_init(struct intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> -	/* Only this specific format is affected, and it's only available on VRAM */
> -	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
> +	/* Some modifiers require aligned 64k phys pages. */
> +	if (XE_IOCTL_DBG(xe, intel_fb_needs_64k_phys_wa(mode_cmd->modifier[0]) &&
>  			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
>  		return -EINVAL;
>  
> -- 
> 2.45.2
> 

Re: [PATCH v5 1/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Thomas Hellström]

On Mon, 2024-08-26 at 11:50 +0200, Maarten Lankhorst wrote:
> For CCS formats on affected platforms, CCS can be used freely, but
> display engine requires a multiple of 64k physical pages. No other
> changes are needed.
> 
> At the BO creation time we don't know if the BO will be used for CCS
> or not. If the scanout flag is set, and the BO is a multiple of 64k,
> we take the safe route and force the physical alignment of 64k pages.
> 
> If the BO is not a multiple of 64k, or the scanout flag was not set
> at BO creation, we reject it for usage as CCS in display. The
> physical
> pages are likely not aligned correctly, and this will cause
> corruption
> when used as FB.
> 
> The scanout flag and size being a multiple of 64k are used together
> to enforce 64k physical placement.
> 
> VM_BIND is completely unaffected, mappings to a VM can still be
> aligned
> to 4k, just like for normal buffers.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Co-developed-by: Zbigniew Kempczyński
> <zbigniew.kempczynski@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
> ---
>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++++
>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> index f835492f73fb4..6775c2557b9df 100644
> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
> @@ -28,6 +28,11 @@ int intel_fb_bo_framebuffer_init(struct
> intel_framebuffer *intel_fb,
>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>  	int ret;
>  
> +	/* Only this specific format is affected, and it's only
> available on VRAM */

A first time reader would wonder "Affected by what". Could we rephrase
like "This specific format, only available with DGFX needs .." or
something similar self-contained?

> +	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] ==
> I915_FORMAT_MOD_4_TILED_BMG_CCS &&
> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
> +		return -EINVAL;
> +
>  	xe_bo_get(bo);
>  
>  	ret = ttm_bo_reserve(&bo->ttm, true, false, NULL);
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index cbe7bf098970f..41297b5797173 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device
> *dev, void *data,
>  
>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>  
> +	/* CCS formats need physical placement at a 64K alignment in
> VRAM. */
> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
> +	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
> +	    !(args->size % SZ_64K))

This might probably fail on 32-bit compiles? Can we use the IS_ALIGNED
macro?


> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
> +
>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
> {
>  		if (XE_IOCTL_DBG(xe, !(bo_flags &
> XE_BO_FLAG_VRAM_MASK)))
>  			return -EINVAL;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 4cc13eddb6b32..3eb76d874eb28 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2878,7 +2878,16 @@ static int xe_vm_bind_ioctl_validate_bo(struct
> xe_device *xe, struct xe_bo *bo,
>  		return -EINVAL;
>  	}
>  
> -	if (bo->flags & XE_BO_FLAG_INTERNAL_64K) {
> +	/*
> +	 * Some platforms require 64k VM_BIND alignment,
> +	 * specifically those with XE_VRAM_FLAGS_NEED64K.
> +	 *
> +	 * Other platforms may have BO's set to 64k physical
> placement,
> +	 * but can be mapped at 4k offsets anyway. This check is
> only
> +	 * there for the former case.
> +	 */
> +	if ((bo->flags & XE_BO_FLAG_INTERNAL_64K) &&
> +	    (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)) {
>  		if (XE_IOCTL_DBG(xe, obj_offset &
>  				 XE_64K_PAGE_MASK) ||
>  		    XE_IOCTL_DBG(xe, addr & XE_64K_PAGE_MASK) ||

Otherwise LGTM.
/Thomas

Re: [PATCH v5 1/2] drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed.

[Maarten Lankhorst]

Hey,

Den 2024-08-26 kl. 16:01, skrev Thomas Hellström:
> On Mon, 2024-08-26 at 11:50 +0200, Maarten Lankhorst wrote:
>> For CCS formats on affected platforms, CCS can be used freely, but
>> display engine requires a multiple of 64k physical pages. No other
>> changes are needed.
>>
>> At the BO creation time we don't know if the BO will be used for CCS
>> or not. If the scanout flag is set, and the BO is a multiple of 64k,
>> we take the safe route and force the physical alignment of 64k pages.
>>
>> If the BO is not a multiple of 64k, or the scanout flag was not set
>> at BO creation, we reject it for usage as CCS in display. The
>> physical
>> pages are likely not aligned correctly, and this will cause
>> corruption
>> when used as FB.
>>
>> The scanout flag and size being a multiple of 64k are used together
>> to enforce 64k physical placement.
>>
>> VM_BIND is completely unaffected, mappings to a VM can still be
>> aligned
>> to 4k, just like for normal buffers.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Co-developed-by: Zbigniew Kempczyński
>> <zbigniew.kempczynski@intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
>> ---
>>  drivers/gpu/drm/xe/display/intel_fb_bo.c |  5 +++++
>>  drivers/gpu/drm/xe/xe_bo.c               |  7 +++++++
>>  drivers/gpu/drm/xe/xe_vm.c               | 11 ++++++++++-
>>  3 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/intel_fb_bo.c
>> b/drivers/gpu/drm/xe/display/intel_fb_bo.c
>> index f835492f73fb4..6775c2557b9df 100644
>> --- a/drivers/gpu/drm/xe/display/intel_fb_bo.c
>> +++ b/drivers/gpu/drm/xe/display/intel_fb_bo.c
>> @@ -28,6 +28,11 @@ int intel_fb_bo_framebuffer_init(struct
>> intel_framebuffer *intel_fb,
>>  	struct xe_device *xe = to_xe_device(bo->ttm.base.dev);
>>  	int ret;
>>  
>> +	/* Only this specific format is affected, and it's only
>> available on VRAM */
> 
> A first time reader would wonder "Affected by what". Could we rephrase
> like "This specific format, only available with DGFX needs .." or
> something similar self-contained?
Yeah, I will reorder with next patch, then this comment goes away.

>> +	if (XE_IOCTL_DBG(xe, mode_cmd->modifier[0] ==
>> I915_FORMAT_MOD_4_TILED_BMG_CCS &&
>> +			     !(bo->flags & XE_BO_FLAG_NEEDS_64K)))
>> +		return -EINVAL;
>> +
>>  	xe_bo_get(bo);
>>  
>>  	ret = ttm_bo_reserve(&bo->ttm, true, false, NULL);
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index cbe7bf098970f..41297b5797173 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -2019,6 +2019,13 @@ int xe_gem_create_ioctl(struct drm_device
>> *dev, void *data,
>>  
>>  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
>>  
>> +	/* CCS formats need physical placement at a 64K alignment in
>> VRAM. */
>> +	if ((bo_flags & XE_BO_FLAG_VRAM_MASK) &&
>> +	    (bo_flags & XE_BO_FLAG_SCANOUT) &&
>> +	    !(xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K) &&
>> +	    !(args->size % SZ_64K))
> 
> This might probably fail on 32-bit compiles? Can we use the IS_ALIGNED
> macro?
Good point!


> 
>> +		bo_flags |= XE_BO_FLAG_NEEDS_64K;
>> +
>>  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)
>> {
>>  		if (XE_IOCTL_DBG(xe, !(bo_flags &
>> XE_BO_FLAG_VRAM_MASK)))
>>  			return -EINVAL;
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 4cc13eddb6b32..3eb76d874eb28 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -2878,7 +2878,16 @@ static int xe_vm_bind_ioctl_validate_bo(struct
>> xe_device *xe, struct xe_bo *bo,
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (bo->flags & XE_BO_FLAG_INTERNAL_64K) {
>> +	/*
>> +	 * Some platforms require 64k VM_BIND alignment,
>> +	 * specifically those with XE_VRAM_FLAGS_NEED64K.
>> +	 *
>> +	 * Other platforms may have BO's set to 64k physical
>> placement,
>> +	 * but can be mapped at 4k offsets anyway. This check is
>> only
>> +	 * there for the former case.
>> +	 */
>> +	if ((bo->flags & XE_BO_FLAG_INTERNAL_64K) &&
>> +	    (xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K)) {
>>  		if (XE_IOCTL_DBG(xe, obj_offset &
>>  				 XE_64K_PAGE_MASK) ||
>>  		    XE_IOCTL_DBG(xe, addr & XE_64K_PAGE_MASK) ||
> 
> Otherwise LGTM.
> /Thomas
> 
Thanks,
~Maarten