diff mbox series

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

Message ID 20240826095041.75416-3-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/xe: Align all VRAM scanout buffers to 64k physical pages when needed. | expand

Commit Message

Maarten Lankhorst Aug. 26, 2024, 9:50 a.m. UTC 
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(-)

Comments

Rodrigo Vivi Aug. 26, 2024, 1:31 p.m. UTC | #1 
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
>
Zbigniew Kempczyński Aug. 26, 2024, 1:52 p.m. UTC | #2 
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
>
diff mbox series

Patch

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;