[PATCH 0/9] drm/i915: Polish plane surface alignment handling

[PATCH 0/9] drm/i915: Polish plane surface alignment handling

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_surf_alignment() in particular has devolved into
a complete mess. Redesign the code so that we can handle
alignment restrictions in a nicer. Also adjust alignment
for TGL+ to actually match the hardware requirements.

Ville Syrjälä (9):
  drm: Rename drm_plane_check_pixel_format() to drm_plane_has_format()
  drm: Export drm_plane_has_format()
  drm/i915: Introduce plane->min_alignment() vfunc
  drm/i915: Introduce fb->min_alignment
  drm/i915: Split cursor alignment to per-platform vfuncs
  drm/i915: Split pre-skl platforms out from intel_surf_alignment()
  drm/i915: Move intel_surf_alignment() into skl_univerals_plane.c
  drm/i915: Update plane alignment requirements for TGL+
  drm/i915: Nuke the TGL+ chroma plane tile row alignment stuff

 drivers/gpu/drm/drm_atomic.c                  |   7 +-
 drivers/gpu/drm/drm_crtc.c                    |   6 +-
 drivers/gpu/drm/drm_crtc_internal.h           |   2 -
 drivers/gpu/drm/drm_plane.c                   |  23 ++-
 drivers/gpu/drm/i915/display/i9xx_plane.c     |  75 ++++++++-
 drivers/gpu/drm/i915/display/intel_cursor.c   |  38 +++++
 .../drm/i915/display/intel_display_types.h    |   5 +
 drivers/gpu/drm/i915/display/intel_fb.c       | 145 ++++--------------
 drivers/gpu/drm/i915/display/intel_fb.h       |   3 -
 drivers/gpu/drm/i915/display/intel_fb_pin.c   |  63 ++++++--
 drivers/gpu/drm/i915/display/intel_fb_pin.h   |   3 +-
 drivers/gpu/drm/i915/display/intel_fbdev.c    |   5 +-
 drivers/gpu/drm/i915/display/intel_sprite.c   |  26 ++++
 .../drm/i915/display/skl_universal_plane.c    |  82 +++++++++-
 drivers/gpu/drm/xe/display/xe_fb_pin.c        |   3 +-
 drivers/gpu/drm/xe/display/xe_plane_initial.c |   4 +-
 include/drm/drm_plane.h                       |   2 +
 17 files changed, 324 insertions(+), 168 deletions(-)

-- 
2.43.2

[PATCH 1/9] drm: Rename drm_plane_check_pixel_format() to drm_plane_has_format()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Rename drm_plane_check_pixel_format() to drm_plane_has_format()
and change the return type accordingly. Allows one to write
more natural code.

Also matches drm_any_plane_has_format() better.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic.c        |  7 ++-----
 drivers/gpu/drm/drm_crtc.c          |  6 ++----
 drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
 drivers/gpu/drm/drm_plane.c         | 22 ++++++++++------------
 4 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index a91737adf8e7..e22560213b8e 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -608,7 +608,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 	unsigned int fb_width, fb_height;
 	struct drm_mode_rect *clips;
 	uint32_t num_clips;
-	int ret;
 
 	/* either *both* CRTC and FB must be set, or neither */
 	if (crtc && !fb) {
@@ -635,14 +634,12 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, fb->format->format,
-					   fb->modifier);
-	if (ret) {
+	if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
 		drm_dbg_atomic(plane->dev,
 			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
 			       plane->base.id, plane->name,
 			       &fb->format->format, fb->modifier);
-		return ret;
+		return -EINVAL;
 	}
 
 	/* Give drivers some help against integer overflows */
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 483969b84a30..3488ff067c69 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -789,12 +789,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
 		 * case.
 		 */
 		if (!plane->format_default) {
-			ret = drm_plane_check_pixel_format(plane,
-							   fb->format->format,
-							   fb->modifier);
-			if (ret) {
+			if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
 				drm_dbg_kms(dev, "Invalid pixel format %p4cc, modifier 0x%llx\n",
 					    &fb->format->format, fb->modifier);
+				ret = -EINVAL;
 				goto out;
 			}
 		}
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 25aaae937ceb..898e0e8b51be 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -272,8 +272,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 /* drm_plane.c */
 int drm_plane_register_all(struct drm_device *dev);
 void drm_plane_unregister_all(struct drm_device *dev);
-int drm_plane_check_pixel_format(struct drm_plane *plane,
-				 u32 format, u64 modifier);
+bool drm_plane_has_format(struct drm_plane *plane,
+			  u32 format, u64 modifier);
 struct drm_mode_rect *
 __drm_plane_get_damage_clips(const struct drm_plane_state *state);
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 57662a1fd345..268aa2299df5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -877,8 +877,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
 	return 0;
 }
 
-int drm_plane_check_pixel_format(struct drm_plane *plane,
-				 u32 format, u64 modifier)
+bool drm_plane_has_format(struct drm_plane *plane,
+			  u32 format, u64 modifier)
 {
 	unsigned int i;
 
@@ -887,24 +887,24 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
 			break;
 	}
 	if (i == plane->format_count)
-		return -EINVAL;
+		return false;
 
 	if (plane->funcs->format_mod_supported) {
 		if (!plane->funcs->format_mod_supported(plane, format, modifier))
-			return -EINVAL;
+			return false;
 	} else {
 		if (!plane->modifier_count)
-			return 0;
+			return true;
 
 		for (i = 0; i < plane->modifier_count; i++) {
 			if (modifier == plane->modifiers[i])
 				break;
 		}
 		if (i == plane->modifier_count)
-			return -EINVAL;
+			return false;
 	}
 
-	return 0;
+	return true;
 }
 
 static int __setplane_check(struct drm_plane *plane,
@@ -924,12 +924,10 @@ static int __setplane_check(struct drm_plane *plane,
 	}
 
 	/* Check whether this plane supports the fb pixel format. */
-	ret = drm_plane_check_pixel_format(plane, fb->format->format,
-					   fb->modifier);
-	if (ret) {
+	if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
 		DRM_DEBUG_KMS("Invalid pixel format %p4cc, modifier 0x%llx\n",
 			      &fb->format->format, fb->modifier);
-		return ret;
+		return -EINVAL;
 	}
 
 	/* Give drivers some help against integer overflows */
@@ -964,7 +962,7 @@ bool drm_any_plane_has_format(struct drm_device *dev,
 	struct drm_plane *plane;
 
 	drm_for_each_plane(plane, dev) {
-		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
+		if (drm_plane_has_format(plane, format, modifier))
 			return true;
 	}
 
-- 
2.43.2

[PATCH 2/9] drm: Export drm_plane_has_format()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Export drm_plane_has_format() so that drivers can use it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_crtc_internal.h | 2 --
 drivers/gpu/drm/drm_plane.c         | 1 +
 include/drm/drm_plane.h             | 2 ++
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
index 898e0e8b51be..e207759ca045 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -272,8 +272,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 /* drm_plane.c */
 int drm_plane_register_all(struct drm_device *dev);
 void drm_plane_unregister_all(struct drm_device *dev);
-bool drm_plane_has_format(struct drm_plane *plane,
-			  u32 format, u64 modifier);
 struct drm_mode_rect *
 __drm_plane_get_damage_clips(const struct drm_plane_state *state);
 
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 268aa2299df5..a51d4dd3f7de 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -906,6 +906,7 @@ bool drm_plane_has_format(struct drm_plane *plane,
 
 	return true;
 }
+EXPORT_SYMBOL(drm_plane_has_format);
 
 static int __setplane_check(struct drm_plane *plane,
 			    struct drm_crtc *crtc,
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 9507542121fa..dd718c62ac31 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -972,6 +972,8 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 #define drm_for_each_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
 
+bool drm_plane_has_format(struct drm_plane *plane,
+			  u32 format, u64 modifier);
 bool drm_any_plane_has_format(struct drm_device *dev,
 			      u32 format, u64 modifier);
 
-- 
2.43.2

[PATCH 3/9] drm/i915: Introduce plane->min_alignment() vfunc

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Different hardware generations have different scanout alignment
requirements. Introduce a new vfunc that will allow us to
make that distinction without horrible if-ladders.

For now we directly plug in the existing intel_surf_alignment()
and intel_cursor_alignment() functions.

For fbdev we (temporarily) introduce intel_fbdev_min_alignment()
that simply queries the alignment from the primary plane of
the first crtc.

TODO: someone will need to fix xe's alignment handling

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/i9xx_plane.c     |  8 ++--
 drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +
 .../drm/i915/display/intel_display_types.h    |  3 ++
 drivers/gpu/drm/i915/display/intel_fb.c       | 22 +++++-----
 drivers/gpu/drm/i915/display/intel_fb.h       |  7 +++-
 drivers/gpu/drm/i915/display/intel_fb_pin.c   | 40 ++++++++++++++-----
 drivers/gpu/drm/i915/display/intel_fb_pin.h   |  3 +-
 drivers/gpu/drm/i915/display/intel_fbdev.c    | 21 +++++++++-
 drivers/gpu/drm/i915/display/intel_sprite.c   |  2 +
 .../drm/i915/display/skl_universal_plane.c    | 11 +++--
 drivers/gpu/drm/xe/display/xe_fb_pin.c        |  3 +-
 drivers/gpu/drm/xe/display/xe_plane_initial.c |  4 +-
 12 files changed, 89 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
index ea4d8ba55ad8..85dbf5b950e2 100644
--- a/drivers/gpu/drm/i915/display/i9xx_plane.c
+++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
@@ -224,8 +224,8 @@ static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
 
 int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
 {
-	struct drm_i915_private *dev_priv =
-		to_i915(plane_state->uapi.plane->dev);
+	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	const struct drm_framebuffer *fb = plane_state->hw.fb;
 	int src_x, src_y, src_w;
 	u32 offset;
@@ -266,7 +266,7 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
 	 * despite them not using the linear offset anymore.
 	 */
 	if (DISPLAY_VER(dev_priv) >= 4 && fb->modifier == I915_FORMAT_MOD_X_TILED) {
-		unsigned int alignment = intel_surf_alignment(fb, 0);
+		unsigned int alignment = plane->min_alignment(plane, fb, 0);
 		int cpp = fb->format->cpp[0];
 
 		while ((src_x + src_w) * cpp > plane_state->view.color_plane[0].mapping_stride) {
@@ -867,6 +867,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 			plane->max_stride = ilk_primary_max_stride;
 	}
 
+	plane->min_alignment = intel_surf_alignment;
+
 	if (IS_I830(dev_priv) || IS_I845G(dev_priv)) {
 		plane->update_arm = i830_plane_update_arm;
 	} else {
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index 2118b87ccb10..026975f569a7 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -896,6 +896,8 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 		cursor->check_plane = i9xx_check_cursor;
 	}
 
+	cursor->min_alignment = intel_cursor_alignment;
+
 	cursor->cursor.base = ~0;
 	cursor->cursor.cntl = ~0;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index fec3de25ea54..40d6e5f4c350 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1550,6 +1550,9 @@ struct intel_plane {
 	int (*max_height)(const struct drm_framebuffer *fb,
 			  int color_plane,
 			  unsigned int rotation);
+	unsigned int (*min_alignment)(struct intel_plane *plane,
+				      const struct drm_framebuffer *fb,
+				      int color_plane);
 	unsigned int (*max_stride)(struct intel_plane *plane,
 				   u32 pixel_format, u64 modifier,
 				   unsigned int rotation);
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index b6638726949d..3f3a9cd534f4 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -775,8 +775,12 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
 		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
 }
 
-unsigned int intel_cursor_alignment(const struct drm_i915_private *i915)
+unsigned int intel_cursor_alignment(struct intel_plane *plane,
+				    const struct drm_framebuffer *fb,
+				    int color_plane)
 {
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
 	if (IS_I830(i915))
 		return 16 * 1024;
 	else if (IS_I85X(i915))
@@ -800,10 +804,11 @@ static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_pr
 		return 0;
 }
 
-unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
+unsigned int intel_surf_alignment(struct intel_plane *plane,
+				  const struct drm_framebuffer *fb,
 				  int color_plane)
 {
-	struct drm_i915_private *dev_priv = to_i915(fb->dev);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 
 	if (intel_fb_uses_dpt(fb)) {
 		/* AUX_DIST needs only 4K alignment */
@@ -1098,17 +1103,12 @@ u32 intel_plane_compute_aligned_offset(int *x, int *y,
 				       const struct intel_plane_state *state,
 				       int color_plane)
 {
-	struct intel_plane *intel_plane = to_intel_plane(state->uapi.plane);
-	struct drm_i915_private *i915 = to_i915(intel_plane->base.dev);
+	struct intel_plane *plane = to_intel_plane(state->uapi.plane);
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
 	const struct drm_framebuffer *fb = state->hw.fb;
 	unsigned int rotation = state->hw.rotation;
 	unsigned int pitch = state->view.color_plane[color_plane].mapping_stride;
-	unsigned int alignment;
-
-	if (intel_plane->id == PLANE_CURSOR)
-		alignment = intel_cursor_alignment(i915);
-	else
-		alignment = intel_surf_alignment(fb, color_plane);
+	unsigned int alignment = plane->min_alignment(plane, fb, color_plane);
 
 	return intel_compute_aligned_offset(i915, x, y, fb, color_plane,
 					    pitch, rotation, alignment);
diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
index 23db6628f53e..86c01a3ce81e 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.h
+++ b/drivers/gpu/drm/i915/display/intel_fb.h
@@ -60,8 +60,11 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
 unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
 unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
 				   int color_plane, unsigned int height);
-unsigned int intel_cursor_alignment(const struct drm_i915_private *i915);
-unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
+unsigned int intel_cursor_alignment(struct intel_plane *plane,
+				    const struct drm_framebuffer *fb,
+				    int color_plane);
+unsigned int intel_surf_alignment(struct intel_plane *plane,
+				  const struct drm_framebuffer *fb,
 				  int color_plane);
 
 void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index 1acc11fa19f4..9b0f1ea41b70 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -103,8 +103,9 @@ intel_fb_pin_to_dpt(const struct drm_framebuffer *fb,
 
 struct i915_vma *
 intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
-		     bool phys_cursor,
 		     const struct i915_gtt_view *view,
+		     unsigned int alignment,
+		     unsigned int phys_alignment,
 		     bool uses_fence,
 		     unsigned long *out_flags)
 {
@@ -113,7 +114,6 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
 	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
 	intel_wakeref_t wakeref;
 	struct i915_gem_ww_ctx ww;
-	unsigned int alignment;
 	struct i915_vma *vma;
 	unsigned int pinctl;
 	int ret;
@@ -121,10 +121,6 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
 	if (drm_WARN_ON(dev, !i915_gem_object_is_framebuffer(obj)))
 		return ERR_PTR(-EINVAL);
 
-	if (phys_cursor)
-		alignment = intel_cursor_alignment(dev_priv);
-	else
-		alignment = intel_surf_alignment(fb, 0);
 	if (drm_WARN_ON(dev, alignment && !is_power_of_2(alignment)))
 		return ERR_PTR(-EINVAL);
 
@@ -162,8 +158,8 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
 	i915_gem_ww_ctx_init(&ww, true);
 retry:
 	ret = i915_gem_object_lock(obj, &ww);
-	if (!ret && phys_cursor)
-		ret = i915_gem_object_attach_phys(obj, alignment);
+	if (!ret && phys_alignment)
+		ret = i915_gem_object_attach_phys(obj, phys_alignment);
 	else if (!ret && HAS_LMEM(dev_priv))
 		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
 	if (!ret)
@@ -234,6 +230,27 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
 	i915_vma_put(vma);
 }
 
+static unsigned int
+intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	const struct drm_framebuffer *fb = plane_state->hw.fb;
+
+	return plane->min_alignment(plane, fb, 0);
+}
+
+static unsigned int
+intel_plane_fb_min_phys_alignment(const struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
+	const struct drm_framebuffer *fb = plane_state->hw.fb;
+
+	if (!intel_plane_needs_physical(plane))
+		return 0;
+
+	return plane->min_alignment(plane, fb, 0);
+}
+
 int intel_plane_pin_fb(struct intel_plane_state *plane_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
@@ -242,8 +259,9 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state)
 	struct i915_vma *vma;
 
 	if (!intel_fb_uses_dpt(&fb->base)) {
-		vma = intel_fb_pin_to_ggtt(&fb->base, intel_plane_needs_physical(plane),
-					   &plane_state->view.gtt,
+		vma = intel_fb_pin_to_ggtt(&fb->base, &plane_state->view.gtt,
+					   intel_plane_fb_min_alignment(plane_state),
+					   intel_plane_fb_min_phys_alignment(plane_state),
 					   intel_plane_uses_fence(plane_state),
 					   &plane_state->flags);
 		if (IS_ERR(vma))
@@ -261,7 +279,7 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state)
 			plane_state->phys_dma_addr =
 				i915_gem_object_get_dma_address(intel_fb_obj(&fb->base), 0);
 	} else {
-		unsigned int alignment = intel_surf_alignment(&fb->base, 0);
+		unsigned int alignment = intel_plane_fb_min_alignment(plane_state);
 
 		vma = intel_dpt_pin_to_ggtt(fb->dpt_vm, alignment / 512);
 		if (IS_ERR(vma))
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.h b/drivers/gpu/drm/i915/display/intel_fb_pin.h
index 3f8245edcd15..ac0319b53af0 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.h
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.h
@@ -15,8 +15,9 @@ struct i915_gtt_view;
 
 struct i915_vma *
 intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
-		     bool phys_cursor,
 		     const struct i915_gtt_view *view,
+		     unsigned int alignment,
+		     unsigned int phys_alignment,
 		     bool uses_fence,
 		     unsigned long *out_flags);
 
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 5ad0b4c8a0fd..ff685aebbd1a 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -46,6 +46,7 @@
 #include "gem/i915_gem_mman.h"
 
 #include "i915_drv.h"
+#include "intel_crtc.h"
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_fb_pin.h"
@@ -171,6 +172,21 @@ static const struct fb_ops intelfb_ops = {
 
 __diag_pop();
 
+static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
+{
+	struct drm_i915_private *i915 = to_i915(fb->dev);
+	struct intel_plane *plane;
+	struct intel_crtc *crtc;
+
+	crtc = intel_first_crtc(i915);
+	if (!crtc)
+		return 0;
+
+	plane = to_intel_plane(crtc->base.primary);
+
+	return plane->min_alignment(plane, fb, 0);
+}
+
 static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
@@ -227,8 +243,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * This also validates that any existing fb inherited from the
 	 * BIOS is suitable for own access.
 	 */
-	vma = intel_fb_pin_to_ggtt(&fb->base, false,
-				   &view, false, &flags);
+	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
+				   intel_fbdev_min_alignment(&fb->base), 0,
+				   false, &flags);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		goto out_unlock;
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index 36a253a19c74..1727d98d1339 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -1623,6 +1623,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		}
 	}
 
+	plane->min_alignment = intel_surf_alignment;
+
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
 		supported_rotations =
 			DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180 |
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 7b79704fa674..27782f5060ad 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -1615,11 +1615,12 @@ skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state,
 			       int main_x, int main_y, u32 main_offset,
 			       int ccs_plane)
 {
+	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
 	const struct drm_framebuffer *fb = plane_state->hw.fb;
 	int aux_x = plane_state->view.color_plane[ccs_plane].x;
 	int aux_y = plane_state->view.color_plane[ccs_plane].y;
 	u32 aux_offset = plane_state->view.color_plane[ccs_plane].offset;
-	unsigned int alignment = intel_surf_alignment(fb, ccs_plane);
+	unsigned int alignment = plane->min_alignment(plane, fb, ccs_plane);
 	int hsub;
 	int vsub;
 
@@ -1663,7 +1664,7 @@ int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state,
 	const struct drm_framebuffer *fb = plane_state->hw.fb;
 	int aux_plane = skl_main_to_aux_plane(fb, 0);
 	u32 aux_offset = plane_state->view.color_plane[aux_plane].offset;
-	unsigned int alignment = intel_surf_alignment(fb, 0);
+	unsigned int alignment = plane->min_alignment(plane, fb, 0);
 	int w = drm_rect_width(&plane_state->uapi.src) >> 16;
 
 	intel_add_fb_offsets(x, y, plane_state, 0);
@@ -1719,7 +1720,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
 	int min_width = intel_plane_min_width(plane, fb, 0, rotation);
 	int max_width = intel_plane_max_width(plane, fb, 0, rotation);
 	int max_height = intel_plane_max_height(plane, fb, 0, rotation);
-	unsigned int alignment = intel_surf_alignment(fb, 0);
+	unsigned int alignment = plane->min_alignment(plane, fb, 0);
 	int aux_plane = skl_main_to_aux_plane(fb, 0);
 	u32 offset;
 	int ret;
@@ -1808,7 +1809,7 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
 
 	if (ccs_plane) {
 		u32 aux_offset = plane_state->view.color_plane[ccs_plane].offset;
-		unsigned int alignment = intel_surf_alignment(fb, uv_plane);
+		unsigned int alignment = plane->min_alignment(plane, fb, uv_plane);
 
 		if (offset > aux_offset)
 			offset = intel_plane_adjust_aligned_offset(&x, &y,
@@ -2366,6 +2367,8 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	else
 		plane->max_stride = skl_plane_max_stride;
 
+	plane->min_alignment = intel_surf_alignment;
+
 	if (DISPLAY_VER(dev_priv) >= 11) {
 		plane->update_noarm = icl_plane_update_noarm;
 		plane->update_arm = icl_plane_update_arm;
diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
index 36e15c4961c1..0319941b7e1e 100644
--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
@@ -334,8 +334,9 @@ static void __xe_unpin_fb_vma(struct i915_vma *vma)
 
 struct i915_vma *
 intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
-		     bool phys_cursor,
 		     const struct i915_gtt_view *view,
+		     unsigned int alignment,
+		     unsigned int phys_alignment,
 		     bool uses_fence,
 		     unsigned long *out_flags)
 {
diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
index 9eaa29e733e1..a9d6c31cb683 100644
--- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
+++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
@@ -211,8 +211,8 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
 	intel_fb_fill_view(to_intel_framebuffer(fb),
 			   plane_state->uapi.rotation, &plane_state->view);
 
-	vma = intel_fb_pin_to_ggtt(fb, false, &plane_state->view.gtt,
-				   false, &plane_state->flags);
+	vma = intel_fb_pin_to_ggtt(fb, &plane_state->view.gtt,
+				   0, 0, false, &plane_state->flags);
 	if (IS_ERR(vma))
 		goto nofb;
 
-- 
2.43.2

[PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Different planes could have different alignment requirements
even for the same format/modifier. Collect the alignment
requirements across all planes capable of scanning out the
fb such that the alignment used when pinning the normal ggtt
view is satisfactory to all those planes.

When pinning per-plane views we only have to satisfy the
alignment requirements of the specific plane.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/intel_display_types.h    |  2 ++
 drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
 drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
 4 files changed, 51 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 40d6e5f4c350..58bb65832adf 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -145,6 +145,8 @@ struct intel_framebuffer {
 	};
 
 	struct i915_address_space *dpt_vm;
+
+	unsigned int min_alignment;
 };
 
 enum intel_hotplug_state {
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index 3f3a9cd534f4..c5bae05cbbc3 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -10,6 +10,7 @@
 #include <linux/dma-resv.h>
 
 #include "i915_drv.h"
+#include "intel_atomic_plane.h"
 #include "intel_display.h"
 #include "intel_display_types.h"
 #include "intel_dpt.h"
@@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
 	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
 }
 
+static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
+{
+	struct drm_i915_private *i915 = to_i915(fb->dev);
+	struct intel_plane *plane;
+	unsigned int min_alignment = 0;
+
+	for_each_intel_plane(&i915->drm, plane) {
+		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
+			continue;
+
+		if (intel_plane_needs_physical(plane))
+			continue;
+
+		min_alignment = max(min_alignment,
+				    plane->min_alignment(plane, fb, 0));
+	}
+
+	return min_alignment;
+}
+
 int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
 {
 	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
@@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
 		return -EINVAL;
 	}
 
+	fb->min_alignment = intel_fb_min_alignment(&fb->base);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
index 9b0f1ea41b70..1ae02de906f5 100644
--- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
+++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
@@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
 	i915_vma_put(vma);
 }
 
+static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
+{
+	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
+
+	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
+	    intel_fb_needs_pot_stride_remap(fb))
+		return false;
+
+	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;
+}
+
 static unsigned int
 intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
 {
 	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
-	const struct drm_framebuffer *fb = plane_state->hw.fb;
+	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
 
-	return plane->min_alignment(plane, fb, 0);
+	/*
+	 * Only use plane specific alignment for binding
+	 * a per-plane gtt view (remapped or rotated),
+	 * otherwise make sure the alignment is suitable
+	 * for all planes.
+	 */
+	if (!gtt_view_is_per_plane(plane_state))
+		return fb->min_alignment;
+
+	if (intel_plane_needs_physical(plane))
+		return 0;
+
+	return plane->min_alignment(plane, &fb->base, 0);
 }
 
 static unsigned int
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
index ff685aebbd1a..124aac172acb 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -46,7 +46,6 @@
 #include "gem/i915_gem_mman.h"
 
 #include "i915_drv.h"
-#include "intel_crtc.h"
 #include "intel_display_types.h"
 #include "intel_fb.h"
 #include "intel_fb_pin.h"
@@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = {
 
 __diag_pop();
 
-static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
-{
-	struct drm_i915_private *i915 = to_i915(fb->dev);
-	struct intel_plane *plane;
-	struct intel_crtc *crtc;
-
-	crtc = intel_first_crtc(i915);
-	if (!crtc)
-		return 0;
-
-	plane = to_intel_plane(crtc->base.primary);
-
-	return plane->min_alignment(plane, fb, 0);
-}
-
 static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
@@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * BIOS is suitable for own access.
 	 */
 	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
-				   intel_fbdev_min_alignment(&fb->base), 0,
+				   fb->min_alignment, 0,
 				   false, &flags);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
-- 
2.43.2

[PATCH 5/9] drm/i915: Split cursor alignment to per-platform vfuncs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Split intel_cursor_alignment() into per-platform variants.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cursor.c | 40 +++++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_fb.c     | 16 ---------
 drivers/gpu/drm/i915/display/intel_fb.h     |  3 --
 3 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index 026975f569a7..737d53c50901 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -193,6 +193,13 @@ i845_cursor_max_stride(struct intel_plane *plane,
 	return 2048;
 }
 
+static unsigned int i845_cursor_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	return 32;
+}
+
 static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
 	u32 cntl = 0;
@@ -343,6 +350,28 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
 	return plane->base.dev->mode_config.cursor_width * 4;
 }
 
+static unsigned int i830_cursor_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	/* "AlmadorM Errata – Requires 32-bpp cursor data to be 16KB aligned." */
+	return 16 * 1024; /* physical */
+}
+
+static unsigned int i85x_cursor_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	return 256; /* physical */
+}
+
+static unsigned int i9xx_cursor_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	return 4 * 1024; /* physical for i915/i945 */
+}
+
 static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -884,20 +913,27 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
 
 	if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
 		cursor->max_stride = i845_cursor_max_stride;
+		cursor->min_alignment = i845_cursor_min_alignment;
 		cursor->update_arm = i845_cursor_update_arm;
 		cursor->disable_arm = i845_cursor_disable_arm;
 		cursor->get_hw_state = i845_cursor_get_hw_state;
 		cursor->check_plane = i845_check_cursor;
 	} else {
 		cursor->max_stride = i9xx_cursor_max_stride;
+
+		if (IS_I830(dev_priv))
+			cursor->min_alignment = i830_cursor_min_alignment;
+		else if (IS_I85X(dev_priv))
+			cursor->min_alignment = i85x_cursor_min_alignment;
+		else
+			cursor->min_alignment = i9xx_cursor_min_alignment;
+
 		cursor->update_arm = i9xx_cursor_update_arm;
 		cursor->disable_arm = i9xx_cursor_disable_arm;
 		cursor->get_hw_state = i9xx_cursor_get_hw_state;
 		cursor->check_plane = i9xx_check_cursor;
 	}
 
-	cursor->min_alignment = intel_cursor_alignment;
-
 	cursor->cursor.base = ~0;
 	cursor->cursor.cntl = ~0;
 
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index c5bae05cbbc3..c84ecae3a57c 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -776,22 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
 		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
 }
 
-unsigned int intel_cursor_alignment(struct intel_plane *plane,
-				    const struct drm_framebuffer *fb,
-				    int color_plane)
-{
-	struct drm_i915_private *i915 = to_i915(plane->base.dev);
-
-	if (IS_I830(i915))
-		return 16 * 1024;
-	else if (IS_I85X(i915))
-		return 256;
-	else if (IS_I845G(i915) || IS_I865G(i915))
-		return 32;
-	else
-		return 4 * 1024;
-}
-
 static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_priv)
 {
 	if (DISPLAY_VER(dev_priv) >= 9)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
index 86c01a3ce81e..16ebb573643f 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.h
+++ b/drivers/gpu/drm/i915/display/intel_fb.h
@@ -60,9 +60,6 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
 unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
 unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
 				   int color_plane, unsigned int height);
-unsigned int intel_cursor_alignment(struct intel_plane *plane,
-				    const struct drm_framebuffer *fb,
-				    int color_plane);
 unsigned int intel_surf_alignment(struct intel_plane *plane,
 				  const struct drm_framebuffer *fb,
 				  int color_plane);
-- 
2.43.2

[PATCH 6/9] drm/i915: Split pre-skl platforms out from intel_surf_alignment()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Extract the necessary chunks from intel_surf_alignment()
into per-platform variants for all pre-skl primary/sprite
planes.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/i9xx_plane.c   | 69 ++++++++++++++++++++-
 drivers/gpu/drm/i915/display/intel_fb.c     | 17 +----
 drivers/gpu/drm/i915/display/intel_sprite.c | 28 ++++++++-
 3 files changed, 96 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
index 85dbf5b950e2..0d64176c1e6f 100644
--- a/drivers/gpu/drm/i915/display/i9xx_plane.c
+++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
@@ -762,6 +762,66 @@ i8xx_plane_max_stride(struct intel_plane *plane,
 		return 8 * 1024;
 }
 
+static unsigned int vlv_primary_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	switch (fb->modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		if (HAS_ASYNC_FLIPS(i915))
+			return 256 * 1024;
+		return 4 * 1024;
+	case DRM_FORMAT_MOD_LINEAR:
+		return 128 * 1024;
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
+static unsigned int g4x_primary_min_alignment(struct intel_plane *plane,
+					      const struct drm_framebuffer *fb,
+					      int color_plane)
+{
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+
+	switch (fb->modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		if (HAS_ASYNC_FLIPS(i915))
+			return 256 * 1024;
+		return 4 * 1024;
+	case DRM_FORMAT_MOD_LINEAR:
+		return 4 * 1024;
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
+static unsigned int i965_plane_min_alignment(struct intel_plane *plane,
+					     const struct drm_framebuffer *fb,
+					     int color_plane)
+{
+	switch (fb->modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		return 4 * 1024;
+	case DRM_FORMAT_MOD_LINEAR:
+		return 128 * 1024;
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
+static unsigned int i9xx_plane_min_alignment(struct intel_plane *plane,
+					     const struct drm_framebuffer *fb,
+					     int color_plane)
+{
+	return 0;
+}
+
 static const struct drm_plane_funcs i965_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
@@ -867,7 +927,14 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 			plane->max_stride = ilk_primary_max_stride;
 	}
 
-	plane->min_alignment = intel_surf_alignment;
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+		plane->min_alignment = vlv_primary_min_alignment;
+	else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
+		plane->min_alignment = g4x_primary_min_alignment;
+	else if (DISPLAY_VER(dev_priv) == 4)
+		plane->min_alignment = i965_plane_min_alignment;
+	else
+		plane->min_alignment = i9xx_plane_min_alignment;
 
 	if (IS_I830(dev_priv) || IS_I845G(dev_priv)) {
 		plane->update_arm = i830_plane_update_arm;
diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index c84ecae3a57c..eea93d84a16e 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -776,19 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
 		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
 }
 
-static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_priv)
-{
-	if (DISPLAY_VER(dev_priv) >= 9)
-		return 256 * 1024;
-	else if (IS_I965G(dev_priv) || IS_I965GM(dev_priv) ||
-		 IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
-		return 128 * 1024;
-	else if (DISPLAY_VER(dev_priv) >= 4)
-		return 4 * 1024;
-	else
-		return 0;
-}
-
 unsigned int intel_surf_alignment(struct intel_plane *plane,
 				  const struct drm_framebuffer *fb,
 				  int color_plane)
@@ -824,7 +811,7 @@ unsigned int intel_surf_alignment(struct intel_plane *plane,
 		 */
 		if (DISPLAY_VER(dev_priv) >= 12) {
 			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
-				return intel_linear_alignment(dev_priv);
+				return 256 * 1024;
 
 			return intel_tile_row_size(fb, color_plane);
 		}
@@ -836,7 +823,7 @@ unsigned int intel_surf_alignment(struct intel_plane *plane,
 
 	switch (fb->modifier) {
 	case DRM_FORMAT_MOD_LINEAR:
-		return intel_linear_alignment(dev_priv);
+		return 256 * 1024;
 	case I915_FORMAT_MOD_X_TILED:
 		if (HAS_ASYNC_FLIPS(dev_priv))
 			return 256 * 1024;
diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index 1727d98d1339..2b8fa59c409f 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -254,6 +254,21 @@ int vlv_plane_min_cdclk(const struct intel_crtc_state *crtc_state,
 	return DIV_ROUND_UP(pixel_rate * num, den);
 }
 
+static unsigned int vlv_sprite_min_alignment(struct intel_plane *plane,
+					     const struct drm_framebuffer *fb,
+					     int color_plane)
+{
+	switch (fb->modifier) {
+	case I915_FORMAT_MOD_X_TILED:
+		return 4 * 1024;
+	case DRM_FORMAT_MOD_LINEAR:
+		return 128 * 1024;
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
 static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
 	u32 sprctl = 0;
@@ -965,6 +980,13 @@ hsw_sprite_max_stride(struct intel_plane *plane,
 	return min(8192 * cpp, 16 * 1024);
 }
 
+static unsigned int g4x_sprite_min_alignment(struct intel_plane *plane,
+					     const struct drm_framebuffer *fb,
+					     int color_plane)
+{
+	return 4 * 1024;
+}
+
 static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
 	u32 dvscntr = 0;
@@ -1571,6 +1593,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		plane->get_hw_state = vlv_sprite_get_hw_state;
 		plane->check_plane = vlv_sprite_check;
 		plane->max_stride = i965_plane_max_stride;
+		plane->min_alignment = vlv_sprite_min_alignment;
 		plane->min_cdclk = vlv_plane_min_cdclk;
 
 		if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
@@ -1597,6 +1620,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 			plane->min_cdclk = ivb_sprite_min_cdclk;
 		}
 
+		plane->min_alignment = g4x_sprite_min_alignment;
+
 		formats = snb_sprite_formats;
 		num_formats = ARRAY_SIZE(snb_sprite_formats);
 
@@ -1608,6 +1633,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		plane->get_hw_state = g4x_sprite_get_hw_state;
 		plane->check_plane = g4x_sprite_check;
 		plane->max_stride = g4x_sprite_max_stride;
+		plane->min_alignment = g4x_sprite_min_alignment;
 		plane->min_cdclk = g4x_sprite_min_cdclk;
 
 		if (IS_SANDYBRIDGE(dev_priv)) {
@@ -1623,8 +1649,6 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
 		}
 	}
 
-	plane->min_alignment = intel_surf_alignment;
-
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
 		supported_rotations =
 			DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180 |
-- 
2.43.2

[PATCH 7/9] drm/i915: Move intel_surf_alignment() into skl_univerals_plane.c

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that all pre-skl platforms have their own .min_alignment()
functions the remainder of intel_surf_alignment() can be hoisted
into skl_univerals_plane.c (and renamed appropriately).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb.c       | 77 +------------------
 drivers/gpu/drm/i915/display/intel_fb.h       |  4 +-
 .../drm/i915/display/skl_universal_plane.c    | 77 ++++++++++++++++++-
 3 files changed, 78 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index eea93d84a16e..c80f866f3fb6 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -584,7 +584,7 @@ static bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int color_pl
 	return intel_fb_rc_ccs_cc_plane(fb) == color_plane;
 }
 
-static bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
+bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
 {
 	return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
 		color_plane == 1;
@@ -776,81 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
 		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
 }
 
-unsigned int intel_surf_alignment(struct intel_plane *plane,
-				  const struct drm_framebuffer *fb,
-				  int color_plane)
-{
-	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-
-	if (intel_fb_uses_dpt(fb)) {
-		/* AUX_DIST needs only 4K alignment */
-		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
-			return 512 * 4096;
-
-		/*
-		 * FIXME ADL sees GGTT/DMAR faults with async
-		 * flips unless we align to 16k at least.
-		 * Figure out what's going on here...
-		 */
-		if (IS_ALDERLAKE_P(dev_priv) &&
-		    !intel_fb_is_ccs_modifier(fb->modifier) &&
-		    HAS_ASYNC_FLIPS(dev_priv))
-			return 512 * 16 * 1024;
-
-		return 512 * 4096;
-	}
-
-	/* AUX_DIST needs only 4K alignment */
-	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
-		return 4096;
-
-	if (is_semiplanar_uv_plane(fb, color_plane)) {
-		/*
-		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
-		 * alignment for linear UV planes on all platforms.
-		 */
-		if (DISPLAY_VER(dev_priv) >= 12) {
-			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
-				return 256 * 1024;
-
-			return intel_tile_row_size(fb, color_plane);
-		}
-
-		return 4096;
-	}
-
-	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
-
-	switch (fb->modifier) {
-	case DRM_FORMAT_MOD_LINEAR:
-		return 256 * 1024;
-	case I915_FORMAT_MOD_X_TILED:
-		if (HAS_ASYNC_FLIPS(dev_priv))
-			return 256 * 1024;
-		return 0;
-	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
-	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
-	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
-	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
-	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
-	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
-		return 16 * 1024;
-	case I915_FORMAT_MOD_Y_TILED_CCS:
-	case I915_FORMAT_MOD_Yf_TILED_CCS:
-	case I915_FORMAT_MOD_Y_TILED:
-	case I915_FORMAT_MOD_4_TILED:
-	case I915_FORMAT_MOD_Yf_TILED:
-		return 1 * 1024 * 1024;
-	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
-	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
-	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
-		return 16 * 1024;
-	default:
-		MISSING_CASE(fb->modifier);
-		return 0;
-	}
-}
-
 void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
 				    const struct drm_framebuffer *fb,
 				    int color_plane)
diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
index 16ebb573643f..1b1fef2dc39a 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.h
+++ b/drivers/gpu/drm/i915/display/intel_fb.h
@@ -34,6 +34,7 @@ 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 is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
 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);
 
@@ -60,9 +61,6 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
 unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
 unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
 				   int color_plane, unsigned int height);
-unsigned int intel_surf_alignment(struct intel_plane *plane,
-				  const struct drm_framebuffer *fb,
-				  int color_plane);
 
 void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
 				    const struct drm_framebuffer *fb,
diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 27782f5060ad..1ecd7c691317 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -502,6 +502,81 @@ skl_plane_max_stride(struct intel_plane *plane,
 				max_pixels, max_bytes);
 }
 
+static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
+					    const struct drm_framebuffer *fb,
+					    int color_plane)
+{
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+
+	if (intel_fb_uses_dpt(fb)) {
+		/* AUX_DIST needs only 4K alignment */
+		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
+			return 512 * 4096;
+
+		/*
+		 * FIXME ADL sees GGTT/DMAR faults with async
+		 * flips unless we align to 16k at least.
+		 * Figure out what's going on here...
+		 */
+		if (IS_ALDERLAKE_P(dev_priv) &&
+		    !intel_fb_is_ccs_modifier(fb->modifier) &&
+		    HAS_ASYNC_FLIPS(dev_priv))
+			return 512 * 16 * 1024;
+
+		return 512 * 4096;
+	}
+
+	/* AUX_DIST needs only 4K alignment */
+	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
+		return 4096;
+
+	if (is_semiplanar_uv_plane(fb, color_plane)) {
+		/*
+		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
+		 * alignment for linear UV planes on all platforms.
+		 */
+		if (DISPLAY_VER(dev_priv) >= 12) {
+			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
+				return 256 * 1024;
+
+			return intel_tile_row_size(fb, color_plane);
+		}
+
+		return 4096;
+	}
+
+	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
+
+	switch (fb->modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+		return 256 * 1024;
+	case I915_FORMAT_MOD_X_TILED:
+		if (HAS_ASYNC_FLIPS(dev_priv))
+			return 256 * 1024;
+		return 0;
+	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
+	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
+	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
+	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
+		return 16 * 1024;
+	case I915_FORMAT_MOD_Y_TILED_CCS:
+	case I915_FORMAT_MOD_Yf_TILED_CCS:
+	case I915_FORMAT_MOD_Y_TILED:
+	case I915_FORMAT_MOD_4_TILED:
+	case I915_FORMAT_MOD_Yf_TILED:
+		return 1 * 1024 * 1024;
+	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
+	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
+	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
+		return 16 * 1024;
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
 /* Preoffset values for YUV to RGB Conversion */
 #define PREOFF_YUV_TO_RGB_HI		0x1800
 #define PREOFF_YUV_TO_RGB_ME		0x0000
@@ -2367,7 +2442,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	else
 		plane->max_stride = skl_plane_max_stride;
 
-	plane->min_alignment = intel_surf_alignment;
+	plane->min_alignment = skl_plane_min_alignment;
 
 	if (DISPLAY_VER(dev_priv) >= 11) {
 		plane->update_noarm = icl_plane_update_noarm;
-- 
2.43.2

[PATCH 8/9] drm/i915: Update plane alignment requirements for TGL+

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we still use the SKL+ PLANE_SURF alignment even
for TGL+ even though the hardware no longer needs it.
Introduce a separate tgl_plane_min_alignment() and update
it to more accurately reflect the hardware requirements.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../drm/i915/display/skl_universal_plane.c    | 103 ++++++++++--------
 1 file changed, 55 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index 1ecd7c691317..ca7fc9fae990 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -502,75 +502,79 @@ skl_plane_max_stride(struct intel_plane *plane,
 				max_pixels, max_bytes);
 }
 
-static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
-					    const struct drm_framebuffer *fb,
-					    int color_plane)
+static u32 tgl_plane_min_alignment(struct intel_plane *plane,
+				   const struct drm_framebuffer *fb,
+				   int color_plane)
 {
-	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
-
-	if (intel_fb_uses_dpt(fb)) {
-		/* AUX_DIST needs only 4K alignment */
-		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
-			return 512 * 4096;
-
-		/*
-		 * FIXME ADL sees GGTT/DMAR faults with async
-		 * flips unless we align to 16k at least.
-		 * Figure out what's going on here...
-		 */
-		if (IS_ALDERLAKE_P(dev_priv) &&
-		    !intel_fb_is_ccs_modifier(fb->modifier) &&
-		    HAS_ASYNC_FLIPS(dev_priv))
-			return 512 * 16 * 1024;
-
-		return 512 * 4096;
-	}
+	struct drm_i915_private *i915 = to_i915(plane->base.dev);
+	/* PLANE_SURF GGTT -> DPT alignment */
+	int mult = intel_fb_uses_dpt(fb) ? 512 : 1;
 
 	/* AUX_DIST needs only 4K alignment */
 	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
-		return 4096;
+		return mult * 4 * 1024;
 
 	if (is_semiplanar_uv_plane(fb, color_plane)) {
 		/*
 		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
 		 * alignment for linear UV planes on all platforms.
 		 */
-		if (DISPLAY_VER(dev_priv) >= 12) {
-			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
-				return 256 * 1024;
-
-			return intel_tile_row_size(fb, color_plane);
-		}
-
-		return 4096;
-	}
-
-	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
-
-	switch (fb->modifier) {
-	case DRM_FORMAT_MOD_LINEAR:
-		return 256 * 1024;
-	case I915_FORMAT_MOD_X_TILED:
-		if (HAS_ASYNC_FLIPS(dev_priv))
+		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
 			return 256 * 1024;
-		return 0;
+
+		return intel_tile_row_size(fb, color_plane);
+	}
+
+	switch (fb->modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+	case I915_FORMAT_MOD_X_TILED:
+	case I915_FORMAT_MOD_Y_TILED:
+	case I915_FORMAT_MOD_4_TILED:
+		/*
+		 * FIXME ADL sees GGTT/DMAR faults with async
+		 * flips unless we align to 16k at least.
+		 * Figure out what's going on here...
+		 */
+		if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))
+			return mult * 16 * 1024;
+		return mult * 4 * 1024;
 	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
 	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
+	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
+	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
+	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
 	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
 	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
 	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
-		return 16 * 1024;
+		/* 4x1 main surface tiles (16K) match 64B of AUX */
+		return max(mult * 4 * 1024, 16 * 1024);
+	default:
+		MISSING_CASE(fb->modifier);
+		return 0;
+	}
+}
+
+static u32 skl_plane_min_alignment(struct intel_plane *plane,
+				   const struct drm_framebuffer *fb,
+				   int color_plane)
+{
+	/*
+	 * AUX_DIST needs only 4K alignment,
+	 * as does ICL UV PLANE_SURF.
+	 */
+	if (color_plane != 0)
+		return 4 * 1024;
+
+	switch (fb->modifier) {
+	case DRM_FORMAT_MOD_LINEAR:
+	case I915_FORMAT_MOD_X_TILED:
+		return 256 * 1024;
 	case I915_FORMAT_MOD_Y_TILED_CCS:
 	case I915_FORMAT_MOD_Yf_TILED_CCS:
 	case I915_FORMAT_MOD_Y_TILED:
-	case I915_FORMAT_MOD_4_TILED:
 	case I915_FORMAT_MOD_Yf_TILED:
 		return 1 * 1024 * 1024;
-	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
-	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
-	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
-		return 16 * 1024;
 	default:
 		MISSING_CASE(fb->modifier);
 		return 0;
@@ -2442,7 +2446,10 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
 	else
 		plane->max_stride = skl_plane_max_stride;
 
-	plane->min_alignment = skl_plane_min_alignment;
+	if (DISPLAY_VER(dev_priv) >= 12)
+		plane->min_alignment = tgl_plane_min_alignment;
+	else
+		plane->min_alignment = skl_plane_min_alignment;
 
 	if (DISPLAY_VER(dev_priv) >= 11) {
 		plane->update_noarm = icl_plane_update_noarm;
-- 
2.43.2

[PATCH 9/9] drm/i915: Nuke the TGL+ chroma plane tile row alignment stuff

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I don't think the display hardware really has such chroma
plane tile row alignment requirements as outlined in
commit d156135e6a54 ("drm/i915/tgl: Make sure a semiplanar
UV plane is tile row size aligned")

Bspec had the same exact thing to say about earlier hardware
as well, but we never cared and things work just fine.

The one thing mentioned in that commit that is definitely
true however is the fence alignment issue. But we don't
deal with that on earlier hardware either. We do have code
to deal with that issue for the first color plane, but not
the chroma planes. So I think if we did want to check this
more extensively we should do it in the same places where
we already check the first color plane (namely
convert_plane_offset_to_xy() and intel_fb_bo_framebuffer_init()).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fb.c            | 12 +-----------
 drivers/gpu/drm/i915/display/intel_fb.h            |  1 -
 drivers/gpu/drm/i915/display/skl_universal_plane.c | 11 -----------
 3 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
index c80f866f3fb6..fc18da3106fd 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.c
+++ b/drivers/gpu/drm/i915/display/intel_fb.c
@@ -584,12 +584,6 @@ static bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int color_pl
 	return intel_fb_rc_ccs_cc_plane(fb) == color_plane;
 }
 
-bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
-{
-	return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
-		color_plane == 1;
-}
-
 bool is_surface_linear(const struct drm_framebuffer *fb, int color_plane)
 {
 	return fb->modifier == DRM_FORMAT_MOD_LINEAR ||
@@ -1019,11 +1013,7 @@ static int intel_fb_offset_to_xy(int *x, int *y,
 	struct drm_i915_private *i915 = to_i915(fb->dev);
 	unsigned int height, alignment, unused;
 
-	if (DISPLAY_VER(i915) >= 12 &&
-	    !intel_fb_needs_pot_stride_remap(to_intel_framebuffer(fb)) &&
-	    is_semiplanar_uv_plane(fb, color_plane))
-		alignment = intel_tile_row_size(fb, color_plane);
-	else if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
+	if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
 		alignment = intel_tile_size(i915);
 	else
 		alignment = 0;
diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
index 1b1fef2dc39a..6dee0c8b7f22 100644
--- a/drivers/gpu/drm/i915/display/intel_fb.h
+++ b/drivers/gpu/drm/i915/display/intel_fb.h
@@ -34,7 +34,6 @@ 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 is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
 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/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
index ca7fc9fae990..476f5b7d9497 100644
--- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
+++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
@@ -514,17 +514,6 @@ static u32 tgl_plane_min_alignment(struct intel_plane *plane,
 	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
 		return mult * 4 * 1024;
 
-	if (is_semiplanar_uv_plane(fb, color_plane)) {
-		/*
-		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
-		 * alignment for linear UV planes on all platforms.
-		 */
-		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
-			return 256 * 1024;
-
-		return intel_tile_row_size(fb, color_plane);
-	}
-
 	switch (fb->modifier) {
 	case DRM_FORMAT_MOD_LINEAR:
 	case I915_FORMAT_MOD_X_TILED:
-- 
2.43.2

✗ Fi.CI.SPARSE: warning for drm/i915: Polish plane surface alignment handling

[Patchwork]

== Series Details ==

Series: drm/i915: Polish plane surface alignment handling
URL   : https://patchwork.freedesktop.org/series/133564/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

✗ Fi.CI.BAT: failure for drm/i915: Polish plane surface alignment handling

[Patchwork]

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

== Series Details ==

Series: drm/i915: Polish plane surface alignment handling
URL   : https://patchwork.freedesktop.org/series/133564/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_14756 -> Patchwork_133564v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_133564v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_133564v1, please notify your bug team ('I915-ci-infra@lists.freedesktop.org') to allow them
  to document this new failure mode, which will reduce false positives in CI.

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

Participating hosts (42 -> 43)
------------------------------

  Additional (2): bat-dg2-11 fi-kbl-8809g 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_133564v1:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_fence@basic-await@vcs0:
    - bat-adln-1:         [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14756/bat-adln-1/igt@gem_exec_fence@basic-await@vcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-adln-1/igt@gem_exec_fence@basic-await@vcs0.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][3] ([i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][4] ([i915#4613]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/fi-kbl-8809g/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_mmap@basic:
    - bat-dg2-11:         NOTRUN -> [SKIP][5] ([i915#4083])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg2-11:         NOTRUN -> [SKIP][6] ([i915#4079]) +1 other test skip
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg2-11:         NOTRUN -> [SKIP][7] ([i915#4077]) +2 other tests skip
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@gem_tiled_fence_blits@basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-11:         NOTRUN -> [SKIP][8] ([i915#6621])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [PASS][9] -> [ABORT][10] ([i915#10594])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14756/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  * igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy:
    - bat-dg2-11:         NOTRUN -> [SKIP][11] ([i915#4212]) +7 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_addfb_basic@addfb25-x-tiled-mismatch-legacy.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-11:         NOTRUN -> [SKIP][12] ([i915#5190])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-11:         NOTRUN -> [SKIP][13] ([i915#4215] / [i915#5190])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - bat-dg2-11:         NOTRUN -> [SKIP][14] ([i915#4103] / [i915#4213]) +1 other test skip
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_dsc@dsc-basic:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][15] +30 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/fi-kbl-8809g/igt@kms_dsc@dsc-basic.html
    - bat-dg2-11:         NOTRUN -> [SKIP][16] ([i915#3555] / [i915#3840])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-11:         NOTRUN -> [SKIP][17]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-11:         NOTRUN -> [SKIP][18] ([i915#5274])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-dg2-11:         NOTRUN -> [SKIP][19] ([i915#5354])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-sprite-plane-onoff:
    - bat-dg2-11:         NOTRUN -> [SKIP][20] ([i915#1072] / [i915#9732]) +3 other tests skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_psr@psr-sprite-plane-onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-11:         NOTRUN -> [SKIP][21] ([i915#3555])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-11:         NOTRUN -> [SKIP][22] ([i915#3708])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-gtt:
    - bat-dg2-11:         NOTRUN -> [SKIP][23] ([i915#3708] / [i915#4077]) +1 other test skip
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-11:         NOTRUN -> [SKIP][24] ([i915#3291] / [i915#3708]) +2 other tests skip
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/bat-dg2-11/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - fi-bsw-n3050:       [ABORT][25] ([i915#10594]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14756/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_133564v1/fi-bsw-n3050/igt@i915_selftest@live@execlists.html

  
  [i915#10594]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10594
  [i915#1072]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1072
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#3291]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3708
  [i915#3840]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3840
  [i915#4077]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4215
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#5190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5354
  [i915#6621]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/6621
  [i915#9732]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9732


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

  * Linux: CI_DRM_14756 -> Patchwork_133564v1

  CI-20190529: 20190529
  CI_DRM_14756: 3d00e04692d45ab076e9e493252361bb18373a0e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7849: c09431db6f7d5a2ddeb42e10a119d1cb7fa071d0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_133564v1: 3d00e04692d45ab076e9e493252361bb18373a0e @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

Re: [PATCH 1/9] drm: Rename drm_plane_check_pixel_format() to drm_plane_has_format()

[Jani Nikula]

On Mon, 13 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Rename drm_plane_check_pixel_format() to drm_plane_has_format()
> and change the return type accordingly. Allows one to write
> more natural code.
>
> Also matches drm_any_plane_has_format() better.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_atomic.c        |  7 ++-----
>  drivers/gpu/drm/drm_crtc.c          |  6 ++----
>  drivers/gpu/drm/drm_crtc_internal.h |  4 ++--
>  drivers/gpu/drm/drm_plane.c         | 22 ++++++++++------------
>  4 files changed, 16 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a91737adf8e7..e22560213b8e 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -608,7 +608,6 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>  	unsigned int fb_width, fb_height;
>  	struct drm_mode_rect *clips;
>  	uint32_t num_clips;
> -	int ret;
>  
>  	/* either *both* CRTC and FB must be set, or neither */
>  	if (crtc && !fb) {
> @@ -635,14 +634,12 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>  	}
>  
>  	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> -					   fb->modifier);
> -	if (ret) {
> +	if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
>  		drm_dbg_atomic(plane->dev,
>  			       "[PLANE:%d:%s] invalid pixel format %p4cc, modifier 0x%llx\n",
>  			       plane->base.id, plane->name,
>  			       &fb->format->format, fb->modifier);
> -		return ret;
> +		return -EINVAL;
>  	}
>  
>  	/* Give drivers some help against integer overflows */
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 483969b84a30..3488ff067c69 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -789,12 +789,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  		 * case.
>  		 */
>  		if (!plane->format_default) {
> -			ret = drm_plane_check_pixel_format(plane,
> -							   fb->format->format,
> -							   fb->modifier);
> -			if (ret) {
> +			if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
>  				drm_dbg_kms(dev, "Invalid pixel format %p4cc, modifier 0x%llx\n",
>  					    &fb->format->format, fb->modifier);
> +				ret = -EINVAL;
>  				goto out;
>  			}
>  		}
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 25aaae937ceb..898e0e8b51be 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -272,8 +272,8 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  /* drm_plane.c */
>  int drm_plane_register_all(struct drm_device *dev);
>  void drm_plane_unregister_all(struct drm_device *dev);
> -int drm_plane_check_pixel_format(struct drm_plane *plane,
> -				 u32 format, u64 modifier);
> +bool drm_plane_has_format(struct drm_plane *plane,
> +			  u32 format, u64 modifier);
>  struct drm_mode_rect *
>  __drm_plane_get_damage_clips(const struct drm_plane_state *state);
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 57662a1fd345..268aa2299df5 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -877,8 +877,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data,
>  	return 0;
>  }
>  
> -int drm_plane_check_pixel_format(struct drm_plane *plane,
> -				 u32 format, u64 modifier)
> +bool drm_plane_has_format(struct drm_plane *plane,
> +			  u32 format, u64 modifier)
>  {
>  	unsigned int i;
>  
> @@ -887,24 +887,24 @@ int drm_plane_check_pixel_format(struct drm_plane *plane,
>  			break;
>  	}
>  	if (i == plane->format_count)
> -		return -EINVAL;
> +		return false;
>  
>  	if (plane->funcs->format_mod_supported) {
>  		if (!plane->funcs->format_mod_supported(plane, format, modifier))
> -			return -EINVAL;
> +			return false;
>  	} else {
>  		if (!plane->modifier_count)
> -			return 0;
> +			return true;
>  
>  		for (i = 0; i < plane->modifier_count; i++) {
>  			if (modifier == plane->modifiers[i])
>  				break;
>  		}
>  		if (i == plane->modifier_count)
> -			return -EINVAL;
> +			return false;
>  	}
>  
> -	return 0;
> +	return true;
>  }
>  
>  static int __setplane_check(struct drm_plane *plane,
> @@ -924,12 +924,10 @@ static int __setplane_check(struct drm_plane *plane,
>  	}
>  
>  	/* Check whether this plane supports the fb pixel format. */
> -	ret = drm_plane_check_pixel_format(plane, fb->format->format,
> -					   fb->modifier);
> -	if (ret) {
> +	if (!drm_plane_has_format(plane, fb->format->format, fb->modifier)) {
>  		DRM_DEBUG_KMS("Invalid pixel format %p4cc, modifier 0x%llx\n",
>  			      &fb->format->format, fb->modifier);
> -		return ret;
> +		return -EINVAL;
>  	}
>  
>  	/* Give drivers some help against integer overflows */
> @@ -964,7 +962,7 @@ bool drm_any_plane_has_format(struct drm_device *dev,
>  	struct drm_plane *plane;
>  
>  	drm_for_each_plane(plane, dev) {
> -		if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> +		if (drm_plane_has_format(plane, format, modifier))
>  			return true;
>  	}

-- 
Jani Nikula, Intel

Re: [PATCH 2/9] drm: Export drm_plane_has_format()

[Jani Nikula]

On Mon, 13 May 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Export drm_plane_has_format() so that drivers can use it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/drm_crtc_internal.h | 2 --
>  drivers/gpu/drm/drm_plane.c         | 1 +
>  include/drm/drm_plane.h             | 2 ++
>  3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> index 898e0e8b51be..e207759ca045 100644
> --- a/drivers/gpu/drm/drm_crtc_internal.h
> +++ b/drivers/gpu/drm/drm_crtc_internal.h
> @@ -272,8 +272,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  /* drm_plane.c */
>  int drm_plane_register_all(struct drm_device *dev);
>  void drm_plane_unregister_all(struct drm_device *dev);
> -bool drm_plane_has_format(struct drm_plane *plane,
> -			  u32 format, u64 modifier);
>  struct drm_mode_rect *
>  __drm_plane_get_damage_clips(const struct drm_plane_state *state);
>  
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 268aa2299df5..a51d4dd3f7de 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -906,6 +906,7 @@ bool drm_plane_has_format(struct drm_plane *plane,
>  
>  	return true;
>  }
> +EXPORT_SYMBOL(drm_plane_has_format);
>  
>  static int __setplane_check(struct drm_plane *plane,
>  			    struct drm_crtc *crtc,
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 9507542121fa..dd718c62ac31 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -972,6 +972,8 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
>  #define drm_for_each_plane(plane, dev) \
>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>  
> +bool drm_plane_has_format(struct drm_plane *plane,
> +			  u32 format, u64 modifier);
>  bool drm_any_plane_has_format(struct drm_device *dev,
>  			      u32 format, u64 modifier);

-- 
Jani Nikula, Intel

Re: [PATCH 3/9] drm/i915: Introduce plane->min_alignment() vfunc

[Imre Deak]

On Mon, May 13, 2024 at 08:59:36PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Different hardware generations have different scanout alignment
> requirements. Introduce a new vfunc that will allow us to
> make that distinction without horrible if-ladders.
> 
> For now we directly plug in the existing intel_surf_alignment()
> and intel_cursor_alignment() functions.
> 
> For fbdev we (temporarily) introduce intel_fbdev_min_alignment()
> that simply queries the alignment from the primary plane of
> the first crtc.
> 
> TODO: someone will need to fix xe's alignment handling
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c     |  8 ++--
>  drivers/gpu/drm/i915/display/intel_cursor.c   |  2 +
>  .../drm/i915/display/intel_display_types.h    |  3 ++
>  drivers/gpu/drm/i915/display/intel_fb.c       | 22 +++++-----
>  drivers/gpu/drm/i915/display/intel_fb.h       |  7 +++-
>  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 40 ++++++++++++++-----
>  drivers/gpu/drm/i915/display/intel_fb_pin.h   |  3 +-
>  drivers/gpu/drm/i915/display/intel_fbdev.c    | 21 +++++++++-
>  drivers/gpu/drm/i915/display/intel_sprite.c   |  2 +
>  .../drm/i915/display/skl_universal_plane.c    | 11 +++--
>  drivers/gpu/drm/xe/display/xe_fb_pin.c        |  3 +-
>  drivers/gpu/drm/xe/display/xe_plane_initial.c |  4 +-
>  12 files changed, 89 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index ea4d8ba55ad8..85dbf5b950e2 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -224,8 +224,8 @@ static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
>  
>  int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
>  {
> -	struct drm_i915_private *dev_priv =
> -		to_i915(plane_state->uapi.plane->dev);
> +	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	int src_x, src_y, src_w;
>  	u32 offset;
> @@ -266,7 +266,7 @@ int i9xx_check_plane_surface(struct intel_plane_state *plane_state)
>  	 * despite them not using the linear offset anymore.
>  	 */
>  	if (DISPLAY_VER(dev_priv) >= 4 && fb->modifier == I915_FORMAT_MOD_X_TILED) {
> -		unsigned int alignment = intel_surf_alignment(fb, 0);
> +		unsigned int alignment = plane->min_alignment(plane, fb, 0);
>  		int cpp = fb->format->cpp[0];
>  
>  		while ((src_x + src_w) * cpp > plane_state->view.color_plane[0].mapping_stride) {
> @@ -867,6 +867,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  			plane->max_stride = ilk_primary_max_stride;
>  	}
>  
> +	plane->min_alignment = intel_surf_alignment;
> +
>  	if (IS_I830(dev_priv) || IS_I845G(dev_priv)) {
>  		plane->update_arm = i830_plane_update_arm;
>  	} else {
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 2118b87ccb10..026975f569a7 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -896,6 +896,8 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>  		cursor->check_plane = i9xx_check_cursor;
>  	}
>  
> +	cursor->min_alignment = intel_cursor_alignment;
> +
>  	cursor->cursor.base = ~0;
>  	cursor->cursor.cntl = ~0;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index fec3de25ea54..40d6e5f4c350 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1550,6 +1550,9 @@ struct intel_plane {
>  	int (*max_height)(const struct drm_framebuffer *fb,
>  			  int color_plane,
>  			  unsigned int rotation);
> +	unsigned int (*min_alignment)(struct intel_plane *plane,
> +				      const struct drm_framebuffer *fb,
> +				      int color_plane);
>  	unsigned int (*max_stride)(struct intel_plane *plane,
>  				   u32 pixel_format, u64 modifier,
>  				   unsigned int rotation);
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index b6638726949d..3f3a9cd534f4 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -775,8 +775,12 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
>  		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
>  }
>  
> -unsigned int intel_cursor_alignment(const struct drm_i915_private *i915)
> +unsigned int intel_cursor_alignment(struct intel_plane *plane,
> +				    const struct drm_framebuffer *fb,
> +				    int color_plane)
>  {
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
>  	if (IS_I830(i915))
>  		return 16 * 1024;
>  	else if (IS_I85X(i915))
> @@ -800,10 +804,11 @@ static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_pr
>  		return 0;
>  }
>  
> -unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> +unsigned int intel_surf_alignment(struct intel_plane *plane,
> +				  const struct drm_framebuffer *fb,
>  				  int color_plane)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(fb->dev);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  
>  	if (intel_fb_uses_dpt(fb)) {
>  		/* AUX_DIST needs only 4K alignment */
> @@ -1098,17 +1103,12 @@ u32 intel_plane_compute_aligned_offset(int *x, int *y,
>  				       const struct intel_plane_state *state,
>  				       int color_plane)
>  {
> -	struct intel_plane *intel_plane = to_intel_plane(state->uapi.plane);
> -	struct drm_i915_private *i915 = to_i915(intel_plane->base.dev);
> +	struct intel_plane *plane = to_intel_plane(state->uapi.plane);
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
>  	const struct drm_framebuffer *fb = state->hw.fb;
>  	unsigned int rotation = state->hw.rotation;
>  	unsigned int pitch = state->view.color_plane[color_plane].mapping_stride;
> -	unsigned int alignment;
> -
> -	if (intel_plane->id == PLANE_CURSOR)
> -		alignment = intel_cursor_alignment(i915);
> -	else
> -		alignment = intel_surf_alignment(fb, color_plane);
> +	unsigned int alignment = plane->min_alignment(plane, fb, color_plane);
>  
>  	return intel_compute_aligned_offset(i915, x, y, fb, color_plane,
>  					    pitch, rotation, alignment);
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 23db6628f53e..86c01a3ce81e 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -60,8 +60,11 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
>  unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>  				   int color_plane, unsigned int height);
> -unsigned int intel_cursor_alignment(const struct drm_i915_private *i915);
> -unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> +unsigned int intel_cursor_alignment(struct intel_plane *plane,
> +				    const struct drm_framebuffer *fb,
> +				    int color_plane);
> +unsigned int intel_surf_alignment(struct intel_plane *plane,
> +				  const struct drm_framebuffer *fb,
>  				  int color_plane);
>  
>  void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index 1acc11fa19f4..9b0f1ea41b70 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -103,8 +103,9 @@ intel_fb_pin_to_dpt(const struct drm_framebuffer *fb,
>  
>  struct i915_vma *
>  intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> -		     bool phys_cursor,
>  		     const struct i915_gtt_view *view,
> +		     unsigned int alignment,
> +		     unsigned int phys_alignment,
>  		     bool uses_fence,
>  		     unsigned long *out_flags)
>  {
> @@ -113,7 +114,6 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
>  	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>  	intel_wakeref_t wakeref;
>  	struct i915_gem_ww_ctx ww;
> -	unsigned int alignment;
>  	struct i915_vma *vma;
>  	unsigned int pinctl;
>  	int ret;
> @@ -121,10 +121,6 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
>  	if (drm_WARN_ON(dev, !i915_gem_object_is_framebuffer(obj)))
>  		return ERR_PTR(-EINVAL);
>  
> -	if (phys_cursor)
> -		alignment = intel_cursor_alignment(dev_priv);
> -	else
> -		alignment = intel_surf_alignment(fb, 0);
>  	if (drm_WARN_ON(dev, alignment && !is_power_of_2(alignment)))
>  		return ERR_PTR(-EINVAL);
>  
> @@ -162,8 +158,8 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
>  	i915_gem_ww_ctx_init(&ww, true);
>  retry:
>  	ret = i915_gem_object_lock(obj, &ww);
> -	if (!ret && phys_cursor)
> -		ret = i915_gem_object_attach_phys(obj, alignment);
> +	if (!ret && phys_alignment)
> +		ret = i915_gem_object_attach_phys(obj, phys_alignment);
>  	else if (!ret && HAS_LMEM(dev_priv))
>  		ret = i915_gem_object_migrate(obj, &ww, INTEL_REGION_LMEM_0);
>  	if (!ret)
> @@ -234,6 +230,27 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
>  	i915_vma_put(vma);
>  }
>  
> +static unsigned int
> +intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> +{
> +	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	const struct drm_framebuffer *fb = plane_state->hw.fb;
> +
> +	return plane->min_alignment(plane, fb, 0);
> +}
> +
> +static unsigned int
> +intel_plane_fb_min_phys_alignment(const struct intel_plane_state *plane_state)
> +{
> +	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> +	const struct drm_framebuffer *fb = plane_state->hw.fb;
> +
> +	if (!intel_plane_needs_physical(plane))
> +		return 0;
> +
> +	return plane->min_alignment(plane, fb, 0);
> +}
> +
>  int intel_plane_pin_fb(struct intel_plane_state *plane_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> @@ -242,8 +259,9 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state)
>  	struct i915_vma *vma;
>  
>  	if (!intel_fb_uses_dpt(&fb->base)) {
> -		vma = intel_fb_pin_to_ggtt(&fb->base, intel_plane_needs_physical(plane),
> -					   &plane_state->view.gtt,
> +		vma = intel_fb_pin_to_ggtt(&fb->base, &plane_state->view.gtt,
> +					   intel_plane_fb_min_alignment(plane_state),
> +					   intel_plane_fb_min_phys_alignment(plane_state),
>  					   intel_plane_uses_fence(plane_state),
>  					   &plane_state->flags);
>  		if (IS_ERR(vma))
> @@ -261,7 +279,7 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state)
>  			plane_state->phys_dma_addr =
>  				i915_gem_object_get_dma_address(intel_fb_obj(&fb->base), 0);
>  	} else {
> -		unsigned int alignment = intel_surf_alignment(&fb->base, 0);
> +		unsigned int alignment = intel_plane_fb_min_alignment(plane_state);
>  
>  		vma = intel_dpt_pin_to_ggtt(fb->dpt_vm, alignment / 512);
>  		if (IS_ERR(vma))
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.h b/drivers/gpu/drm/i915/display/intel_fb_pin.h
> index 3f8245edcd15..ac0319b53af0 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.h
> @@ -15,8 +15,9 @@ struct i915_gtt_view;
>  
>  struct i915_vma *
>  intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> -		     bool phys_cursor,
>  		     const struct i915_gtt_view *view,
> +		     unsigned int alignment,
> +		     unsigned int phys_alignment,
>  		     bool uses_fence,
>  		     unsigned long *out_flags);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 5ad0b4c8a0fd..ff685aebbd1a 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -46,6 +46,7 @@
>  #include "gem/i915_gem_mman.h"
>  
>  #include "i915_drv.h"
> +#include "intel_crtc.h"
>  #include "intel_display_types.h"
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
> @@ -171,6 +172,21 @@ static const struct fb_ops intelfb_ops = {
>  
>  __diag_pop();
>  
> +static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
> +{
> +	struct drm_i915_private *i915 = to_i915(fb->dev);
> +	struct intel_plane *plane;
> +	struct intel_crtc *crtc;
> +
> +	crtc = intel_first_crtc(i915);
> +	if (!crtc)
> +		return 0;
> +
> +	plane = to_intel_plane(crtc->base.primary);
> +
> +	return plane->min_alignment(plane, fb, 0);
> +}
> +
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -227,8 +243,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	 * This also validates that any existing fb inherited from the
>  	 * BIOS is suitable for own access.
>  	 */
> -	vma = intel_fb_pin_to_ggtt(&fb->base, false,
> -				   &view, false, &flags);
> +	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
> +				   intel_fbdev_min_alignment(&fb->base), 0,
> +				   false, &flags);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
>  		goto out_unlock;
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 36a253a19c74..1727d98d1339 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -1623,6 +1623,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  		}
>  	}
>  
> +	plane->min_alignment = intel_surf_alignment;
> +
>  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
>  		supported_rotations =
>  			DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180 |
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 7b79704fa674..27782f5060ad 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1615,11 +1615,12 @@ skl_check_main_ccs_coordinates(struct intel_plane_state *plane_state,
>  			       int main_x, int main_y, u32 main_offset,
>  			       int ccs_plane)
>  {
> +	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	int aux_x = plane_state->view.color_plane[ccs_plane].x;
>  	int aux_y = plane_state->view.color_plane[ccs_plane].y;
>  	u32 aux_offset = plane_state->view.color_plane[ccs_plane].offset;
> -	unsigned int alignment = intel_surf_alignment(fb, ccs_plane);
> +	unsigned int alignment = plane->min_alignment(plane, fb, ccs_plane);
>  	int hsub;
>  	int vsub;
>  
> @@ -1663,7 +1664,7 @@ int skl_calc_main_surface_offset(const struct intel_plane_state *plane_state,
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
>  	int aux_plane = skl_main_to_aux_plane(fb, 0);
>  	u32 aux_offset = plane_state->view.color_plane[aux_plane].offset;
> -	unsigned int alignment = intel_surf_alignment(fb, 0);
> +	unsigned int alignment = plane->min_alignment(plane, fb, 0);
>  	int w = drm_rect_width(&plane_state->uapi.src) >> 16;
>  
>  	intel_add_fb_offsets(x, y, plane_state, 0);
> @@ -1719,7 +1720,7 @@ static int skl_check_main_surface(struct intel_plane_state *plane_state)
>  	int min_width = intel_plane_min_width(plane, fb, 0, rotation);
>  	int max_width = intel_plane_max_width(plane, fb, 0, rotation);
>  	int max_height = intel_plane_max_height(plane, fb, 0, rotation);
> -	unsigned int alignment = intel_surf_alignment(fb, 0);
> +	unsigned int alignment = plane->min_alignment(plane, fb, 0);
>  	int aux_plane = skl_main_to_aux_plane(fb, 0);
>  	u32 offset;
>  	int ret;
> @@ -1808,7 +1809,7 @@ static int skl_check_nv12_aux_surface(struct intel_plane_state *plane_state)
>  
>  	if (ccs_plane) {
>  		u32 aux_offset = plane_state->view.color_plane[ccs_plane].offset;
> -		unsigned int alignment = intel_surf_alignment(fb, uv_plane);
> +		unsigned int alignment = plane->min_alignment(plane, fb, uv_plane);
>  
>  		if (offset > aux_offset)
>  			offset = intel_plane_adjust_aligned_offset(&x, &y,
> @@ -2366,6 +2367,8 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	else
>  		plane->max_stride = skl_plane_max_stride;
>  
> +	plane->min_alignment = intel_surf_alignment;
> +
>  	if (DISPLAY_VER(dev_priv) >= 11) {
>  		plane->update_noarm = icl_plane_update_noarm;
>  		plane->update_arm = icl_plane_update_arm;
> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index 36e15c4961c1..0319941b7e1e 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> @@ -334,8 +334,9 @@ static void __xe_unpin_fb_vma(struct i915_vma *vma)
>  
>  struct i915_vma *
>  intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> -		     bool phys_cursor,
>  		     const struct i915_gtt_view *view,
> +		     unsigned int alignment,
> +		     unsigned int phys_alignment,
>  		     bool uses_fence,
>  		     unsigned long *out_flags)
>  {
> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> index 9eaa29e733e1..a9d6c31cb683 100644
> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
> @@ -211,8 +211,8 @@ intel_find_initial_plane_obj(struct intel_crtc *crtc,
>  	intel_fb_fill_view(to_intel_framebuffer(fb),
>  			   plane_state->uapi.rotation, &plane_state->view);
>  
> -	vma = intel_fb_pin_to_ggtt(fb, false, &plane_state->view.gtt,
> -				   false, &plane_state->flags);
> +	vma = intel_fb_pin_to_ggtt(fb, &plane_state->view.gtt,
> +				   0, 0, false, &plane_state->flags);
>  	if (IS_ERR(vma))
>  		goto nofb;
>  
> -- 
> 2.43.2
> 

Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Imre Deak]

On Mon, May 13, 2024 at 08:59:37PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Different planes could have different alignment requirements
> even for the same format/modifier. Collect the alignment
> requirements across all planes capable of scanning out the
> fb such that the alignment used when pinning the normal ggtt
> view is satisfactory to all those planes.
> 
> When pinning per-plane views we only have to satisfy the
> alignment requirements of the specific plane.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 ++
>  drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
>  4 files changed, 51 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 40d6e5f4c350..58bb65832adf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -145,6 +145,8 @@ struct intel_framebuffer {
>  	};
>  
>  	struct i915_address_space *dpt_vm;
> +
> +	unsigned int min_alignment;
>  };
>  
>  enum intel_hotplug_state {
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index 3f3a9cd534f4..c5bae05cbbc3 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -10,6 +10,7 @@
>  #include <linux/dma-resv.h>
>  
>  #include "i915_drv.h"
> +#include "intel_atomic_plane.h"
>  #include "intel_display.h"
>  #include "intel_display_types.h"
>  #include "intel_dpt.h"
> @@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
>  	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
>  }
>  
> +static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
> +{
> +	struct drm_i915_private *i915 = to_i915(fb->dev);
> +	struct intel_plane *plane;
> +	unsigned int min_alignment = 0;
> +
> +	for_each_intel_plane(&i915->drm, plane) {
> +		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
> +			continue;
> +
> +		if (intel_plane_needs_physical(plane))
> +			continue;
> +
> +		min_alignment = max(min_alignment,
> +				    plane->min_alignment(plane, fb, 0));
> +	}
> +
> +	return min_alignment;
> +}
> +
>  int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
>  {
>  	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
> @@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
>  		return -EINVAL;
>  	}
>  
> +	fb->min_alignment = intel_fb_min_alignment(&fb->base);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> index 9b0f1ea41b70..1ae02de906f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> @@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
>  	i915_vma_put(vma);
>  }
>  
> +static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
> +{
> +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> +
> +	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
> +	    intel_fb_needs_pot_stride_remap(fb))
> +		return false;

The above view is created only once for the FB, I suppose this is how
you differentiate it from views created each time due to a stride
limit (based on intel_plane_needs_remap()).

> +
> +	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;

So the rotated and remapped views created for a stride limit are
per-plane based on the above. There is also a rotated view created only
once for the FB, is it intentional to regard this kind of view as
per-plane as well?

> +}
> +
>  static unsigned int
>  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
>  {
>  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
>  
> -	return plane->min_alignment(plane, fb, 0);
> +	/*
> +	 * Only use plane specific alignment for binding
> +	 * a per-plane gtt view (remapped or rotated),
> +	 * otherwise make sure the alignment is suitable
> +	 * for all planes.
> +	 */
> +	if (!gtt_view_is_per_plane(plane_state))
> +		return fb->min_alignment;
> +
> +	if (intel_plane_needs_physical(plane))
> +		return 0;

I guess the above is ok, though looks like an unrelated change: the
cursor plane min_alignment() for relevant platforms is <= 4k, which will
be rounded up to 4k anyway when binding the vma.

> +
> +	return plane->min_alignment(plane, &fb->base, 0);

The commit could've had more details about the rational for the above.
As I understand it avoids having to rebind the vma for different plane
types, though this is already handled to some degree by
i915_vma::display_alignment.

>  }
>  
>  static unsigned int
> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index ff685aebbd1a..124aac172acb 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -46,7 +46,6 @@
>  #include "gem/i915_gem_mman.h"
>  
>  #include "i915_drv.h"
> -#include "intel_crtc.h"
>  #include "intel_display_types.h"
>  #include "intel_fb.h"
>  #include "intel_fb_pin.h"
> @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = {
>  
>  __diag_pop();
>  
> -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
> -{
> -	struct drm_i915_private *i915 = to_i915(fb->dev);
> -	struct intel_plane *plane;
> -	struct intel_crtc *crtc;
> -
> -	crtc = intel_first_crtc(i915);
> -	if (!crtc)
> -		return 0;
> -
> -	plane = to_intel_plane(crtc->base.primary);
> -
> -	return plane->min_alignment(plane, fb, 0);
> -}
> -
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	 * BIOS is suitable for own access.
>  	 */
>  	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
> -				   intel_fbdev_min_alignment(&fb->base), 0,
> +				   fb->min_alignment, 0,
>  				   false, &flags);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
> -- 
> 2.43.2
> 

Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Imre Deak]

On Tue, May 28, 2024 at 02:27:58PM +0300, Imre Deak wrote:
> [...]
> > +}
> > +
> >  static unsigned int
> >  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> >  {
> >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> >  
> > -	return plane->min_alignment(plane, fb, 0);
> > +	/*
> > +	 * Only use plane specific alignment for binding
> > +	 * a per-plane gtt view (remapped or rotated),
> > +	 * otherwise make sure the alignment is suitable
> > +	 * for all planes.
> > +	 */
> > +	if (!gtt_view_is_per_plane(plane_state))
> > +		return fb->min_alignment;
> > +
> > +	if (intel_plane_needs_physical(plane))
> > +		return 0;
> 
> I guess the above is ok, though looks like an unrelated change: the
> cursor plane min_alignment() for relevant platforms is <= 4k, which will
> be rounded up to 4k anyway when binding the vma.

Hm, actually this would change the current 16k vma alignment for cursors on i830?

> 
> > +
> > +	return plane->min_alignment(plane, &fb->base, 0);
> 
> The commit could've had more details about the rational for the above.
> As I understand it avoids having to rebind the vma for different plane
> types, though this is already handled to some degree by
> i915_vma::display_alignment.
> 
> >  }
> >  
> >  static unsigned int
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index ff685aebbd1a..124aac172acb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -46,7 +46,6 @@
> >  #include "gem/i915_gem_mman.h"
> >  
> >  #include "i915_drv.h"
> > -#include "intel_crtc.h"
> >  #include "intel_display_types.h"
> >  #include "intel_fb.h"
> >  #include "intel_fb_pin.h"
> > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = {
> >  
> >  __diag_pop();
> >  
> > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
> > -{
> > -	struct drm_i915_private *i915 = to_i915(fb->dev);
> > -	struct intel_plane *plane;
> > -	struct intel_crtc *crtc;
> > -
> > -	crtc = intel_first_crtc(i915);
> > -	if (!crtc)
> > -		return 0;
> > -
> > -	plane = to_intel_plane(crtc->base.primary);
> > -
> > -	return plane->min_alignment(plane, fb, 0);
> > -}
> > -
> >  static int intelfb_create(struct drm_fb_helper *helper,
> >  			  struct drm_fb_helper_surface_size *sizes)
> >  {
> > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	 * BIOS is suitable for own access.
> >  	 */
> >  	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
> > -				   intel_fbdev_min_alignment(&fb->base), 0,
> > +				   fb->min_alignment, 0,
> >  				   false, &flags);
> >  	if (IS_ERR(vma)) {
> >  		ret = PTR_ERR(vma);
> > -- 
> > 2.43.2
> > 

Re: [PATCH 5/9] drm/i915: Split cursor alignment to per-platform vfuncs

[Imre Deak]

On Mon, May 13, 2024 at 08:59:38PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Split intel_cursor_alignment() into per-platform variants.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c | 40 +++++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_fb.c     | 16 ---------
>  drivers/gpu/drm/i915/display/intel_fb.h     |  3 --
>  3 files changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index 026975f569a7..737d53c50901 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -193,6 +193,13 @@ i845_cursor_max_stride(struct intel_plane *plane,
>  	return 2048;
>  }
>  
> +static unsigned int i845_cursor_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	return 32;
> +}
> +
>  static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 cntl = 0;
> @@ -343,6 +350,28 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
>  	return plane->base.dev->mode_config.cursor_width * 4;
>  }
>  
> +static unsigned int i830_cursor_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	/* "AlmadorM Errata – Requires 32-bpp cursor data to be 16KB aligned." */
> +	return 16 * 1024; /* physical */
> +}
> +
> +static unsigned int i85x_cursor_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	return 256; /* physical */
> +}
> +
> +static unsigned int i9xx_cursor_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	return 4 * 1024; /* physical for i915/i945 */
> +}
> +
>  static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -884,20 +913,27 @@ intel_cursor_plane_create(struct drm_i915_private *dev_priv,
>  
>  	if (IS_I845G(dev_priv) || IS_I865G(dev_priv)) {
>  		cursor->max_stride = i845_cursor_max_stride;
> +		cursor->min_alignment = i845_cursor_min_alignment;
>  		cursor->update_arm = i845_cursor_update_arm;
>  		cursor->disable_arm = i845_cursor_disable_arm;
>  		cursor->get_hw_state = i845_cursor_get_hw_state;
>  		cursor->check_plane = i845_check_cursor;
>  	} else {
>  		cursor->max_stride = i9xx_cursor_max_stride;
> +
> +		if (IS_I830(dev_priv))
> +			cursor->min_alignment = i830_cursor_min_alignment;
> +		else if (IS_I85X(dev_priv))
> +			cursor->min_alignment = i85x_cursor_min_alignment;
> +		else
> +			cursor->min_alignment = i9xx_cursor_min_alignment;
> +
>  		cursor->update_arm = i9xx_cursor_update_arm;
>  		cursor->disable_arm = i9xx_cursor_disable_arm;
>  		cursor->get_hw_state = i9xx_cursor_get_hw_state;
>  		cursor->check_plane = i9xx_check_cursor;
>  	}
>  
> -	cursor->min_alignment = intel_cursor_alignment;
> -
>  	cursor->cursor.base = ~0;
>  	cursor->cursor.cntl = ~0;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index c5bae05cbbc3..c84ecae3a57c 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -776,22 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
>  		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
>  }
>  
> -unsigned int intel_cursor_alignment(struct intel_plane *plane,
> -				    const struct drm_framebuffer *fb,
> -				    int color_plane)
> -{
> -	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> -
> -	if (IS_I830(i915))
> -		return 16 * 1024;
> -	else if (IS_I85X(i915))
> -		return 256;
> -	else if (IS_I845G(i915) || IS_I865G(i915))
> -		return 32;
> -	else
> -		return 4 * 1024;
> -}
> -
>  static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_priv)
>  {
>  	if (DISPLAY_VER(dev_priv) >= 9)
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 86c01a3ce81e..16ebb573643f 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -60,9 +60,6 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
>  unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>  				   int color_plane, unsigned int height);
> -unsigned int intel_cursor_alignment(struct intel_plane *plane,
> -				    const struct drm_framebuffer *fb,
> -				    int color_plane);
>  unsigned int intel_surf_alignment(struct intel_plane *plane,
>  				  const struct drm_framebuffer *fb,
>  				  int color_plane);
> -- 
> 2.43.2
> 

Re: [PATCH 6/9] drm/i915: Split pre-skl platforms out from intel_surf_alignment()

[Imre Deak]

On Mon, May 13, 2024 at 08:59:39PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Extract the necessary chunks from intel_surf_alignment()
> into per-platform variants for all pre-skl primary/sprite
> planes.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c   | 69 ++++++++++++++++++++-
>  drivers/gpu/drm/i915/display/intel_fb.c     | 17 +----
>  drivers/gpu/drm/i915/display/intel_sprite.c | 28 ++++++++-
>  3 files changed, 96 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index 85dbf5b950e2..0d64176c1e6f 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -762,6 +762,66 @@ i8xx_plane_max_stride(struct intel_plane *plane,
>  		return 8 * 1024;
>  }
>  
> +static unsigned int vlv_primary_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	switch (fb->modifier) {
> +	case I915_FORMAT_MOD_X_TILED:
> +		if (HAS_ASYNC_FLIPS(i915))

Nit: the function is used on VLV and CHV and both support async flips.

> +			return 256 * 1024;
> +		return 4 * 1024;

This changes the current 0 alignment to 4k, but these seem to be
equivalent.

Regardless of the above nit:
Reviewed-by: Imre Deak <imre.deak@intel.com>


> +	case DRM_FORMAT_MOD_LINEAR:
> +		return 128 * 1024;
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
> +static unsigned int g4x_primary_min_alignment(struct intel_plane *plane,
> +					      const struct drm_framebuffer *fb,
> +					      int color_plane)
> +{
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +
> +	switch (fb->modifier) {
> +	case I915_FORMAT_MOD_X_TILED:
> +		if (HAS_ASYNC_FLIPS(i915))
> +			return 256 * 1024;
> +		return 4 * 1024;
> +	case DRM_FORMAT_MOD_LINEAR:
> +		return 4 * 1024;
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
> +static unsigned int i965_plane_min_alignment(struct intel_plane *plane,
> +					     const struct drm_framebuffer *fb,
> +					     int color_plane)
> +{
> +	switch (fb->modifier) {
> +	case I915_FORMAT_MOD_X_TILED:
> +		return 4 * 1024;
> +	case DRM_FORMAT_MOD_LINEAR:
> +		return 128 * 1024;
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
> +static unsigned int i9xx_plane_min_alignment(struct intel_plane *plane,
> +					     const struct drm_framebuffer *fb,
> +					     int color_plane)
> +{
> +	return 0;
> +}
> +
>  static const struct drm_plane_funcs i965_plane_funcs = {
>  	.update_plane = drm_atomic_helper_update_plane,
>  	.disable_plane = drm_atomic_helper_disable_plane,
> @@ -867,7 +927,14 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
>  			plane->max_stride = ilk_primary_max_stride;
>  	}
>  
> -	plane->min_alignment = intel_surf_alignment;
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		plane->min_alignment = vlv_primary_min_alignment;
> +	else if (DISPLAY_VER(dev_priv) >= 5 || IS_G4X(dev_priv))
> +		plane->min_alignment = g4x_primary_min_alignment;
> +	else if (DISPLAY_VER(dev_priv) == 4)
> +		plane->min_alignment = i965_plane_min_alignment;
> +	else
> +		plane->min_alignment = i9xx_plane_min_alignment;
>  
>  	if (IS_I830(dev_priv) || IS_I845G(dev_priv)) {
>  		plane->update_arm = i830_plane_update_arm;
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index c84ecae3a57c..eea93d84a16e 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -776,19 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
>  		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
>  }
>  
> -static unsigned int intel_linear_alignment(const struct drm_i915_private *dev_priv)
> -{
> -	if (DISPLAY_VER(dev_priv) >= 9)
> -		return 256 * 1024;
> -	else if (IS_I965G(dev_priv) || IS_I965GM(dev_priv) ||
> -		 IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		return 128 * 1024;
> -	else if (DISPLAY_VER(dev_priv) >= 4)
> -		return 4 * 1024;
> -	else
> -		return 0;
> -}
> -
>  unsigned int intel_surf_alignment(struct intel_plane *plane,
>  				  const struct drm_framebuffer *fb,
>  				  int color_plane)
> @@ -824,7 +811,7 @@ unsigned int intel_surf_alignment(struct intel_plane *plane,
>  		 */
>  		if (DISPLAY_VER(dev_priv) >= 12) {
>  			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> -				return intel_linear_alignment(dev_priv);
> +				return 256 * 1024;
>  
>  			return intel_tile_row_size(fb, color_plane);
>  		}
> @@ -836,7 +823,7 @@ unsigned int intel_surf_alignment(struct intel_plane *plane,
>  
>  	switch (fb->modifier) {
>  	case DRM_FORMAT_MOD_LINEAR:
> -		return intel_linear_alignment(dev_priv);
> +		return 256 * 1024;
>  	case I915_FORMAT_MOD_X_TILED:
>  		if (HAS_ASYNC_FLIPS(dev_priv))
>  			return 256 * 1024;
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 1727d98d1339..2b8fa59c409f 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -254,6 +254,21 @@ int vlv_plane_min_cdclk(const struct intel_crtc_state *crtc_state,
>  	return DIV_ROUND_UP(pixel_rate * num, den);
>  }
>  
> +static unsigned int vlv_sprite_min_alignment(struct intel_plane *plane,
> +					     const struct drm_framebuffer *fb,
> +					     int color_plane)
> +{
> +	switch (fb->modifier) {
> +	case I915_FORMAT_MOD_X_TILED:
> +		return 4 * 1024;
> +	case DRM_FORMAT_MOD_LINEAR:
> +		return 128 * 1024;
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
>  static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 sprctl = 0;
> @@ -965,6 +980,13 @@ hsw_sprite_max_stride(struct intel_plane *plane,
>  	return min(8192 * cpp, 16 * 1024);
>  }
>  
> +static unsigned int g4x_sprite_min_alignment(struct intel_plane *plane,
> +					     const struct drm_framebuffer *fb,
> +					     int color_plane)
> +{
> +	return 4 * 1024;
> +}
> +
>  static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 dvscntr = 0;
> @@ -1571,6 +1593,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  		plane->get_hw_state = vlv_sprite_get_hw_state;
>  		plane->check_plane = vlv_sprite_check;
>  		plane->max_stride = i965_plane_max_stride;
> +		plane->min_alignment = vlv_sprite_min_alignment;
>  		plane->min_cdclk = vlv_plane_min_cdclk;
>  
>  		if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
> @@ -1597,6 +1620,8 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  			plane->min_cdclk = ivb_sprite_min_cdclk;
>  		}
>  
> +		plane->min_alignment = g4x_sprite_min_alignment;
> +
>  		formats = snb_sprite_formats;
>  		num_formats = ARRAY_SIZE(snb_sprite_formats);
>  
> @@ -1608,6 +1633,7 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  		plane->get_hw_state = g4x_sprite_get_hw_state;
>  		plane->check_plane = g4x_sprite_check;
>  		plane->max_stride = g4x_sprite_max_stride;
> +		plane->min_alignment = g4x_sprite_min_alignment;
>  		plane->min_cdclk = g4x_sprite_min_cdclk;
>  
>  		if (IS_SANDYBRIDGE(dev_priv)) {
> @@ -1623,8 +1649,6 @@ intel_sprite_plane_create(struct drm_i915_private *dev_priv,
>  		}
>  	}
>  
> -	plane->min_alignment = intel_surf_alignment;
> -
>  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B) {
>  		supported_rotations =
>  			DRM_MODE_ROTATE_0 | DRM_MODE_ROTATE_180 |
> -- 
> 2.43.2
> 

Re: [PATCH 7/9] drm/i915: Move intel_surf_alignment() into skl_univerals_plane.c

[Imre Deak]

On Mon, May 13, 2024 at 08:59:40PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that all pre-skl platforms have their own .min_alignment()
> functions the remainder of intel_surf_alignment() can be hoisted
> into skl_univerals_plane.c (and renamed appropriately).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_fb.c       | 77 +------------------
>  drivers/gpu/drm/i915/display/intel_fb.h       |  4 +-
>  .../drm/i915/display/skl_universal_plane.c    | 77 ++++++++++++++++++-
>  3 files changed, 78 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index eea93d84a16e..c80f866f3fb6 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -584,7 +584,7 @@ static bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int color_pl
>  	return intel_fb_rc_ccs_cc_plane(fb) == color_plane;
>  }
>  
> -static bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
> +bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
>  {
>  	return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
>  		color_plane == 1;
> @@ -776,81 +776,6 @@ bool intel_fb_uses_dpt(const struct drm_framebuffer *fb)
>  		intel_fb_modifier_uses_dpt(to_i915(fb->dev), fb->modifier);
>  }
>  
> -unsigned int intel_surf_alignment(struct intel_plane *plane,
> -				  const struct drm_framebuffer *fb,
> -				  int color_plane)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -
> -	if (intel_fb_uses_dpt(fb)) {
> -		/* AUX_DIST needs only 4K alignment */
> -		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> -			return 512 * 4096;
> -
> -		/*
> -		 * FIXME ADL sees GGTT/DMAR faults with async
> -		 * flips unless we align to 16k at least.
> -		 * Figure out what's going on here...
> -		 */
> -		if (IS_ALDERLAKE_P(dev_priv) &&
> -		    !intel_fb_is_ccs_modifier(fb->modifier) &&
> -		    HAS_ASYNC_FLIPS(dev_priv))
> -			return 512 * 16 * 1024;
> -
> -		return 512 * 4096;
> -	}
> -
> -	/* AUX_DIST needs only 4K alignment */
> -	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> -		return 4096;
> -
> -	if (is_semiplanar_uv_plane(fb, color_plane)) {
> -		/*
> -		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> -		 * alignment for linear UV planes on all platforms.
> -		 */
> -		if (DISPLAY_VER(dev_priv) >= 12) {
> -			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> -				return 256 * 1024;
> -
> -			return intel_tile_row_size(fb, color_plane);
> -		}
> -
> -		return 4096;
> -	}
> -
> -	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> -
> -	switch (fb->modifier) {
> -	case DRM_FORMAT_MOD_LINEAR:
> -		return 256 * 1024;
> -	case I915_FORMAT_MOD_X_TILED:
> -		if (HAS_ASYNC_FLIPS(dev_priv))
> -			return 256 * 1024;
> -		return 0;
> -	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> -	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> -	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> -	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
> -	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
> -	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
> -		return 16 * 1024;
> -	case I915_FORMAT_MOD_Y_TILED_CCS:
> -	case I915_FORMAT_MOD_Yf_TILED_CCS:
> -	case I915_FORMAT_MOD_Y_TILED:
> -	case I915_FORMAT_MOD_4_TILED:
> -	case I915_FORMAT_MOD_Yf_TILED:
> -		return 1 * 1024 * 1024;
> -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> -	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> -		return 16 * 1024;
> -	default:
> -		MISSING_CASE(fb->modifier);
> -		return 0;
> -	}
> -}
> -
>  void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
>  				    const struct drm_framebuffer *fb,
>  				    int color_plane)
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 16ebb573643f..1b1fef2dc39a 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -34,6 +34,7 @@ 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 is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
>  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);
>  
> @@ -60,9 +61,6 @@ unsigned int intel_tile_height(const struct drm_framebuffer *fb, int color_plane
>  unsigned int intel_tile_row_size(const struct drm_framebuffer *fb, int color_plane);
>  unsigned int intel_fb_align_height(const struct drm_framebuffer *fb,
>  				   int color_plane, unsigned int height);
> -unsigned int intel_surf_alignment(struct intel_plane *plane,
> -				  const struct drm_framebuffer *fb,
> -				  int color_plane);
>  
>  void intel_fb_plane_get_subsampling(int *hsub, int *vsub,
>  				    const struct drm_framebuffer *fb,
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 27782f5060ad..1ecd7c691317 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -502,6 +502,81 @@ skl_plane_max_stride(struct intel_plane *plane,
>  				max_pixels, max_bytes);
>  }
>  
> +static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
> +					    const struct drm_framebuffer *fb,
> +					    int color_plane)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +
> +	if (intel_fb_uses_dpt(fb)) {
> +		/* AUX_DIST needs only 4K alignment */
> +		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> +			return 512 * 4096;
> +
> +		/*
> +		 * FIXME ADL sees GGTT/DMAR faults with async
> +		 * flips unless we align to 16k at least.
> +		 * Figure out what's going on here...
> +		 */
> +		if (IS_ALDERLAKE_P(dev_priv) &&
> +		    !intel_fb_is_ccs_modifier(fb->modifier) &&
> +		    HAS_ASYNC_FLIPS(dev_priv))
> +			return 512 * 16 * 1024;
> +
> +		return 512 * 4096;
> +	}
> +
> +	/* AUX_DIST needs only 4K alignment */
> +	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> +		return 4096;
> +
> +	if (is_semiplanar_uv_plane(fb, color_plane)) {
> +		/*
> +		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> +		 * alignment for linear UV planes on all platforms.
> +		 */
> +		if (DISPLAY_VER(dev_priv) >= 12) {
> +			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> +				return 256 * 1024;
> +
> +			return intel_tile_row_size(fb, color_plane);
> +		}
> +
> +		return 4096;
> +	}
> +
> +	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> +
> +	switch (fb->modifier) {
> +	case DRM_FORMAT_MOD_LINEAR:
> +		return 256 * 1024;
> +	case I915_FORMAT_MOD_X_TILED:
> +		if (HAS_ASYNC_FLIPS(dev_priv))
> +			return 256 * 1024;
> +		return 0;
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> +	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
> +	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
> +	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
> +		return 16 * 1024;
> +	case I915_FORMAT_MOD_Y_TILED_CCS:
> +	case I915_FORMAT_MOD_Yf_TILED_CCS:
> +	case I915_FORMAT_MOD_Y_TILED:
> +	case I915_FORMAT_MOD_4_TILED:
> +	case I915_FORMAT_MOD_Yf_TILED:
> +		return 1 * 1024 * 1024;
> +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> +	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> +		return 16 * 1024;
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
>  /* Preoffset values for YUV to RGB Conversion */
>  #define PREOFF_YUV_TO_RGB_HI		0x1800
>  #define PREOFF_YUV_TO_RGB_ME		0x0000
> @@ -2367,7 +2442,7 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	else
>  		plane->max_stride = skl_plane_max_stride;
>  
> -	plane->min_alignment = intel_surf_alignment;
> +	plane->min_alignment = skl_plane_min_alignment;
>  
>  	if (DISPLAY_VER(dev_priv) >= 11) {
>  		plane->update_noarm = icl_plane_update_noarm;
> -- 
> 2.43.2
> 

Re: [PATCH 8/9] drm/i915: Update plane alignment requirements for TGL+

[Imre Deak]

On Mon, May 13, 2024 at 08:59:41PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we still use the SKL+ PLANE_SURF alignment even
> for TGL+ even though the hardware no longer needs it.
> Introduce a separate tgl_plane_min_alignment() and update
> it to more accurately reflect the hardware requirements.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/skl_universal_plane.c    | 103 ++++++++++--------
>  1 file changed, 55 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 1ecd7c691317..ca7fc9fae990 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -502,75 +502,79 @@ skl_plane_max_stride(struct intel_plane *plane,
>  				max_pixels, max_bytes);
>  }
>  
> -static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
> -					    const struct drm_framebuffer *fb,
> -					    int color_plane)
> +static u32 tgl_plane_min_alignment(struct intel_plane *plane,
> +				   const struct drm_framebuffer *fb,
> +				   int color_plane)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> -
> -	if (intel_fb_uses_dpt(fb)) {
> -		/* AUX_DIST needs only 4K alignment */
> -		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> -			return 512 * 4096;
> -
> -		/*
> -		 * FIXME ADL sees GGTT/DMAR faults with async
> -		 * flips unless we align to 16k at least.
> -		 * Figure out what's going on here...
> -		 */
> -		if (IS_ALDERLAKE_P(dev_priv) &&
> -		    !intel_fb_is_ccs_modifier(fb->modifier) &&
> -		    HAS_ASYNC_FLIPS(dev_priv))
> -			return 512 * 16 * 1024;
> -
> -		return 512 * 4096;
> -	}
> +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +	/* PLANE_SURF GGTT -> DPT alignment */
> +	int mult = intel_fb_uses_dpt(fb) ? 512 : 1;
>  
>  	/* AUX_DIST needs only 4K alignment */
>  	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> -		return 4096;
> +		return mult * 4 * 1024;
>  
>  	if (is_semiplanar_uv_plane(fb, color_plane)) {
>  		/*
>  		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
>  		 * alignment for linear UV planes on all platforms.
>  		 */
> -		if (DISPLAY_VER(dev_priv) >= 12) {
> -			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> -				return 256 * 1024;
> -
> -			return intel_tile_row_size(fb, color_plane);
> -		}
> -
> -		return 4096;
> -	}
> -
> -	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> -
> -	switch (fb->modifier) {
> -	case DRM_FORMAT_MOD_LINEAR:
> -		return 256 * 1024;
> -	case I915_FORMAT_MOD_X_TILED:
> -		if (HAS_ASYNC_FLIPS(dev_priv))
> +		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
>  			return 256 * 1024;
> -		return 0;
> +
> +		return intel_tile_row_size(fb, color_plane);
> +	}
> +
> +	switch (fb->modifier) {
> +	case DRM_FORMAT_MOD_LINEAR:
> +	case I915_FORMAT_MOD_X_TILED:
> +	case I915_FORMAT_MOD_Y_TILED:
> +	case I915_FORMAT_MOD_4_TILED:
> +		/*
> +		 * FIXME ADL sees GGTT/DMAR faults with async
> +		 * flips unless we align to 16k at least.
> +		 * Figure out what's going on here...
> +		 */
> +		if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))

On ADL HAS_ASYNC_FLIPS() is always true, otherwise looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> +			return mult * 16 * 1024;
> +		return mult * 4 * 1024;
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
>  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> +	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
>  	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
>  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
>  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
> -		return 16 * 1024;
> +		/* 4x1 main surface tiles (16K) match 64B of AUX */
> +		return max(mult * 4 * 1024, 16 * 1024);
> +	default:
> +		MISSING_CASE(fb->modifier);
> +		return 0;
> +	}
> +}
> +
> +static u32 skl_plane_min_alignment(struct intel_plane *plane,
> +				   const struct drm_framebuffer *fb,
> +				   int color_plane)
> +{
> +	/*
> +	 * AUX_DIST needs only 4K alignment,
> +	 * as does ICL UV PLANE_SURF.
> +	 */
> +	if (color_plane != 0)
> +		return 4 * 1024;
> +
> +	switch (fb->modifier) {
> +	case DRM_FORMAT_MOD_LINEAR:
> +	case I915_FORMAT_MOD_X_TILED:
> +		return 256 * 1024;
>  	case I915_FORMAT_MOD_Y_TILED_CCS:
>  	case I915_FORMAT_MOD_Yf_TILED_CCS:
>  	case I915_FORMAT_MOD_Y_TILED:
> -	case I915_FORMAT_MOD_4_TILED:
>  	case I915_FORMAT_MOD_Yf_TILED:
>  		return 1 * 1024 * 1024;
> -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> -	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> -		return 16 * 1024;
>  	default:
>  		MISSING_CASE(fb->modifier);
>  		return 0;
> @@ -2442,7 +2446,10 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
>  	else
>  		plane->max_stride = skl_plane_max_stride;
>  
> -	plane->min_alignment = skl_plane_min_alignment;
> +	if (DISPLAY_VER(dev_priv) >= 12)
> +		plane->min_alignment = tgl_plane_min_alignment;
> +	else
> +		plane->min_alignment = skl_plane_min_alignment;
>  
>  	if (DISPLAY_VER(dev_priv) >= 11) {
>  		plane->update_noarm = icl_plane_update_noarm;
> -- 
> 2.43.2
> 

Re: [PATCH 9/9] drm/i915: Nuke the TGL+ chroma plane tile row alignment stuff

[Imre Deak]

On Mon, May 13, 2024 at 08:59:42PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I don't think the display hardware really has such chroma
> plane tile row alignment requirements as outlined in
> commit d156135e6a54 ("drm/i915/tgl: Make sure a semiplanar
> UV plane is tile row size aligned")
> 
> Bspec had the same exact thing to say about earlier hardware
> as well, but we never cared and things work just fine.
> 
> The one thing mentioned in that commit that is definitely
> true however is the fence alignment issue. But we don't
> deal with that on earlier hardware either. We do have code
> to deal with that issue for the first color plane, but not
> the chroma planes. So I think if we did want to check this
> more extensively we should do it in the same places where
> we already check the first color plane (namely
> convert_plane_offset_to_xy() and intel_fb_bo_framebuffer_init()).

Imo a correct alignment should be required to help users figure out why
a given config doesn't work (even if an incorrect alignment doesn't
cause other HW issues). But agreed that it should be the same then for
all platforms, so ok to remove it in its current form.

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_fb.c            | 12 +-----------
>  drivers/gpu/drm/i915/display/intel_fb.h            |  1 -
>  drivers/gpu/drm/i915/display/skl_universal_plane.c | 11 -----------
>  3 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index c80f866f3fb6..fc18da3106fd 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -584,12 +584,6 @@ static bool is_gen12_ccs_cc_plane(const struct drm_framebuffer *fb, int color_pl
>  	return intel_fb_rc_ccs_cc_plane(fb) == color_plane;
>  }
>  
> -bool is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane)
> -{
> -	return intel_format_info_is_yuv_semiplanar(fb->format, fb->modifier) &&
> -		color_plane == 1;
> -}
> -
>  bool is_surface_linear(const struct drm_framebuffer *fb, int color_plane)
>  {
>  	return fb->modifier == DRM_FORMAT_MOD_LINEAR ||
> @@ -1019,11 +1013,7 @@ static int intel_fb_offset_to_xy(int *x, int *y,
>  	struct drm_i915_private *i915 = to_i915(fb->dev);
>  	unsigned int height, alignment, unused;
>  
> -	if (DISPLAY_VER(i915) >= 12 &&
> -	    !intel_fb_needs_pot_stride_remap(to_intel_framebuffer(fb)) &&
> -	    is_semiplanar_uv_plane(fb, color_plane))
> -		alignment = intel_tile_row_size(fb, color_plane);
> -	else if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
> +	if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
>  		alignment = intel_tile_size(i915);
>  	else
>  		alignment = 0;
> diff --git a/drivers/gpu/drm/i915/display/intel_fb.h b/drivers/gpu/drm/i915/display/intel_fb.h
> index 1b1fef2dc39a..6dee0c8b7f22 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.h
> +++ b/drivers/gpu/drm/i915/display/intel_fb.h
> @@ -34,7 +34,6 @@ 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 is_semiplanar_uv_plane(const struct drm_framebuffer *fb, int color_plane);
>  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/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index ca7fc9fae990..476f5b7d9497 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -514,17 +514,6 @@ static u32 tgl_plane_min_alignment(struct intel_plane *plane,
>  	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
>  		return mult * 4 * 1024;
>  
> -	if (is_semiplanar_uv_plane(fb, color_plane)) {
> -		/*
> -		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> -		 * alignment for linear UV planes on all platforms.
> -		 */
> -		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> -			return 256 * 1024;
> -
> -		return intel_tile_row_size(fb, color_plane);
> -	}

The above will also use the correct 2MB for DPT, which the previous
patch should've kept already. Other than that looks ok:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> -
>  	switch (fb->modifier) {
>  	case DRM_FORMAT_MOD_LINEAR:
>  	case I915_FORMAT_MOD_X_TILED:
> -- 
> 2.43.2
> 

Re: [PATCH 8/9] drm/i915: Update plane alignment requirements for TGL+

[Ville Syrjälä]

On Tue, May 28, 2024 at 04:22:59PM +0300, Imre Deak wrote:
> On Mon, May 13, 2024 at 08:59:41PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we still use the SKL+ PLANE_SURF alignment even
> > for TGL+ even though the hardware no longer needs it.
> > Introduce a separate tgl_plane_min_alignment() and update
> > it to more accurately reflect the hardware requirements.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/skl_universal_plane.c    | 103 ++++++++++--------
> >  1 file changed, 55 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 1ecd7c691317..ca7fc9fae990 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -502,75 +502,79 @@ skl_plane_max_stride(struct intel_plane *plane,
> >  				max_pixels, max_bytes);
> >  }
> >  
> > -static unsigned int skl_plane_min_alignment(struct intel_plane *plane,
> > -					    const struct drm_framebuffer *fb,
> > -					    int color_plane)
> > +static u32 tgl_plane_min_alignment(struct intel_plane *plane,
> > +				   const struct drm_framebuffer *fb,
> > +				   int color_plane)
> >  {
> > -	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > -
> > -	if (intel_fb_uses_dpt(fb)) {
> > -		/* AUX_DIST needs only 4K alignment */
> > -		if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> > -			return 512 * 4096;
> > -
> > -		/*
> > -		 * FIXME ADL sees GGTT/DMAR faults with async
> > -		 * flips unless we align to 16k at least.
> > -		 * Figure out what's going on here...
> > -		 */
> > -		if (IS_ALDERLAKE_P(dev_priv) &&
> > -		    !intel_fb_is_ccs_modifier(fb->modifier) &&
> > -		    HAS_ASYNC_FLIPS(dev_priv))
> > -			return 512 * 16 * 1024;
> > -
> > -		return 512 * 4096;
> > -	}
> > +	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> > +	/* PLANE_SURF GGTT -> DPT alignment */
> > +	int mult = intel_fb_uses_dpt(fb) ? 512 : 1;
> >  
> >  	/* AUX_DIST needs only 4K alignment */
> >  	if (intel_fb_is_ccs_aux_plane(fb, color_plane))
> > -		return 4096;
> > +		return mult * 4 * 1024;
> >  
> >  	if (is_semiplanar_uv_plane(fb, color_plane)) {
> >  		/*
> >  		 * TODO: cross-check wrt. the bspec stride in bytes * 64 bytes
> >  		 * alignment for linear UV planes on all platforms.
> >  		 */
> > -		if (DISPLAY_VER(dev_priv) >= 12) {
> > -			if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> > -				return 256 * 1024;
> > -
> > -			return intel_tile_row_size(fb, color_plane);
> > -		}
> > -
> > -		return 4096;
> > -	}
> > -
> > -	drm_WARN_ON(&dev_priv->drm, color_plane != 0);
> > -
> > -	switch (fb->modifier) {
> > -	case DRM_FORMAT_MOD_LINEAR:
> > -		return 256 * 1024;
> > -	case I915_FORMAT_MOD_X_TILED:
> > -		if (HAS_ASYNC_FLIPS(dev_priv))
> > +		if (fb->modifier == DRM_FORMAT_MOD_LINEAR)
> >  			return 256 * 1024;
> > -		return 0;
> > +
> > +		return intel_tile_row_size(fb, color_plane);
> > +	}
> > +
> > +	switch (fb->modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +	case I915_FORMAT_MOD_X_TILED:
> > +	case I915_FORMAT_MOD_Y_TILED:
> > +	case I915_FORMAT_MOD_4_TILED:
> > +		/*
> > +		 * FIXME ADL sees GGTT/DMAR faults with async
> > +		 * flips unless we align to 16k at least.
> > +		 * Figure out what's going on here...
> > +		 */
> > +		if (IS_ALDERLAKE_P(i915) && HAS_ASYNC_FLIPS(i915))
> 
> On ADL HAS_ASYNC_FLIPS() is always true, otherwise looks ok:

I've been using HAS_ASYNC_FLIPS() to just flag the async flip
specific restrictions. So mainly an aide memoire, but it can
technically be used to also test with less alignment
by just neutering HAS_ASYNC_FLIPS(), without having go trawl
the specs for the specific number again.

Though I'm not super happy how this looks when combine
with the async flip modifier restrictions. Haven't yet
figured out how it actually should look in the though.

> 
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> > +			return mult * 16 * 1024;
> > +		return mult * 4 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_MC_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED_GEN12_RC_CCS_CC:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> > +	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_MC_CCS:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS:
> >  	case I915_FORMAT_MOD_4_TILED_MTL_RC_CCS_CC:
> > -		return 16 * 1024;
> > +		/* 4x1 main surface tiles (16K) match 64B of AUX */
> > +		return max(mult * 4 * 1024, 16 * 1024);
> > +	default:
> > +		MISSING_CASE(fb->modifier);
> > +		return 0;
> > +	}
> > +}
> > +
> > +static u32 skl_plane_min_alignment(struct intel_plane *plane,
> > +				   const struct drm_framebuffer *fb,
> > +				   int color_plane)
> > +{
> > +	/*
> > +	 * AUX_DIST needs only 4K alignment,
> > +	 * as does ICL UV PLANE_SURF.
> > +	 */
> > +	if (color_plane != 0)
> > +		return 4 * 1024;
> > +
> > +	switch (fb->modifier) {
> > +	case DRM_FORMAT_MOD_LINEAR:
> > +	case I915_FORMAT_MOD_X_TILED:
> > +		return 256 * 1024;
> >  	case I915_FORMAT_MOD_Y_TILED_CCS:
> >  	case I915_FORMAT_MOD_Yf_TILED_CCS:
> >  	case I915_FORMAT_MOD_Y_TILED:
> > -	case I915_FORMAT_MOD_4_TILED:
> >  	case I915_FORMAT_MOD_Yf_TILED:
> >  		return 1 * 1024 * 1024;
> > -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS:
> > -	case I915_FORMAT_MOD_4_TILED_DG2_RC_CCS_CC:
> > -	case I915_FORMAT_MOD_4_TILED_DG2_MC_CCS:
> > -		return 16 * 1024;
> >  	default:
> >  		MISSING_CASE(fb->modifier);
> >  		return 0;
> > @@ -2442,7 +2446,10 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
> >  	else
> >  		plane->max_stride = skl_plane_max_stride;
> >  
> > -	plane->min_alignment = skl_plane_min_alignment;
> > +	if (DISPLAY_VER(dev_priv) >= 12)
> > +		plane->min_alignment = tgl_plane_min_alignment;
> > +	else
> > +		plane->min_alignment = skl_plane_min_alignment;
> >  
> >  	if (DISPLAY_VER(dev_priv) >= 11) {
> >  		plane->update_noarm = icl_plane_update_noarm;
> > -- 
> > 2.43.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Ville Syrjälä]

On Tue, May 28, 2024 at 02:27:52PM +0300, Imre Deak wrote:
> On Mon, May 13, 2024 at 08:59:37PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Different planes could have different alignment requirements
> > even for the same format/modifier. Collect the alignment
> > requirements across all planes capable of scanning out the
> > fb such that the alignment used when pinning the normal ggtt
> > view is satisfactory to all those planes.
> > 
> > When pinning per-plane views we only have to satisfy the
> > alignment requirements of the specific plane.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  2 ++
> >  drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
> >  drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
> >  4 files changed, 51 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 40d6e5f4c350..58bb65832adf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -145,6 +145,8 @@ struct intel_framebuffer {
> >  	};
> >  
> >  	struct i915_address_space *dpt_vm;
> > +
> > +	unsigned int min_alignment;
> >  };
> >  
> >  enum intel_hotplug_state {
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > index 3f3a9cd534f4..c5bae05cbbc3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/dma-resv.h>
> >  
> >  #include "i915_drv.h"
> > +#include "intel_atomic_plane.h"
> >  #include "intel_display.h"
> >  #include "intel_display_types.h"
> >  #include "intel_dpt.h"
> > @@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
> >  	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
> >  }
> >  
> > +static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(fb->dev);
> > +	struct intel_plane *plane;
> > +	unsigned int min_alignment = 0;
> > +
> > +	for_each_intel_plane(&i915->drm, plane) {
> > +		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
> > +			continue;
> > +
> > +		if (intel_plane_needs_physical(plane))
> > +			continue;
> > +
> > +		min_alignment = max(min_alignment,
> > +				    plane->min_alignment(plane, fb, 0));
> > +	}
> > +
> > +	return min_alignment;
> > +}
> > +
> >  int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
> >  {
> >  	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
> > @@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
> >  		return -EINVAL;
> >  	}
> >  
> > +	fb->min_alignment = intel_fb_min_alignment(&fb->base);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > index 9b0f1ea41b70..1ae02de906f5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > @@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> >  	i915_vma_put(vma);
> >  }
> >  
> > +static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
> > +{
> > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > +
> > +	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
> > +	    intel_fb_needs_pot_stride_remap(fb))
> > +		return false;
> 
> The above view is created only once for the FB, I suppose this is how
> you differentiate it from views created each time due to a stride
> limit (based on intel_plane_needs_remap()).
> 
> > +
> > +	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;
> 
> So the rotated and remapped views created for a stride limit are
> per-plane based on the above. There is also a rotated view created only
> once for the FB, is it intentional to regard this kind of view as
> per-plane as well?

No. Forgot those exist. I guess I should just remove this per-plane
alignment logic for now. Need to think of a proper way to add it back,
or just quietly bury the whole idea.

> 
> > +}
> > +
> >  static unsigned int
> >  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> >  {
> >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> >  
> > -	return plane->min_alignment(plane, fb, 0);
> > +	/*
> > +	 * Only use plane specific alignment for binding
> > +	 * a per-plane gtt view (remapped or rotated),
> > +	 * otherwise make sure the alignment is suitable
> > +	 * for all planes.
> > +	 */
> > +	if (!gtt_view_is_per_plane(plane_state))
> > +		return fb->min_alignment;
> > +
> > +	if (intel_plane_needs_physical(plane))
> > +		return 0;
> 
> I guess the above is ok, though looks like an unrelated change: the
> cursor plane min_alignment() for relevant platforms is <= 4k, which will
> be rounded up to 4k anyway when binding the vma.

When intel_plane_needs_physical()==true the alignment declared
by the plane only applies to the physical/dma address, as that
is what the hardware will consume. Doesn't actually matter
where it gets bound in the ggtt.

> 
> > +
> > +	return plane->min_alignment(plane, &fb->base, 0);
> 
> The commit could've had more details about the rational for the above.
> As I understand it avoids having to rebind the vma for different plane
> types, though this is already handled to some degree by
> i915_vma::display_alignment.

It was also semi-handled by intel_surf_alignment() trying to
return the max alignment for all the planes more or less.
But it was a fairly big mess.

I did't really remember that the display_alignment
thing even exists. Hmm. I wonder if it's even needed 
anymore. I suppose we can have cases where the display was
previously using the vma, which was subsequency unbound,
and later rebound due to some other reason. We probably
want to preserve the alignment to make sure it'll again
be ready to be used by the display without an extra
rebind.  I guess that only really happens with pre-ppgtt
platforms and/or software rendering via ggtt.

Hmm. We could perpaps change the implementation to adjust
display_alignment when the framebuffer is created instead
of doing it when the vma actually gets pinned. Might not
matter for real world uses cases though.

> 
> >  }
> >  
> >  static unsigned int
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index ff685aebbd1a..124aac172acb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -46,7 +46,6 @@
> >  #include "gem/i915_gem_mman.h"
> >  
> >  #include "i915_drv.h"
> > -#include "intel_crtc.h"
> >  #include "intel_display_types.h"
> >  #include "intel_fb.h"
> >  #include "intel_fb_pin.h"
> > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = {
> >  
> >  __diag_pop();
> >  
> > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
> > -{
> > -	struct drm_i915_private *i915 = to_i915(fb->dev);
> > -	struct intel_plane *plane;
> > -	struct intel_crtc *crtc;
> > -
> > -	crtc = intel_first_crtc(i915);
> > -	if (!crtc)
> > -		return 0;
> > -
> > -	plane = to_intel_plane(crtc->base.primary);
> > -
> > -	return plane->min_alignment(plane, fb, 0);
> > -}
> > -
> >  static int intelfb_create(struct drm_fb_helper *helper,
> >  			  struct drm_fb_helper_surface_size *sizes)
> >  {
> > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	 * BIOS is suitable for own access.
> >  	 */
> >  	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
> > -				   intel_fbdev_min_alignment(&fb->base), 0,
> > +				   fb->min_alignment, 0,
> >  				   false, &flags);
> >  	if (IS_ERR(vma)) {
> >  		ret = PTR_ERR(vma);
> > -- 
> > 2.43.2
> > 

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Imre Deak]

On Tue, May 28, 2024 at 10:38:06PM +0300, Ville Syrjälä wrote:
> On Tue, May 28, 2024 at 02:27:52PM +0300, Imre Deak wrote:
> > On Mon, May 13, 2024 at 08:59:37PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Different planes could have different alignment requirements
> > > even for the same format/modifier. Collect the alignment
> > > requirements across all planes capable of scanning out the
> > > fb such that the alignment used when pinning the normal ggtt
> > > view is satisfactory to all those planes.
> > > 
> > > When pinning per-plane views we only have to satisfy the
> > > alignment requirements of the specific plane.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_types.h    |  2 ++
> > >  drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
> > >  drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
> > >  4 files changed, 51 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 40d6e5f4c350..58bb65832adf 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -145,6 +145,8 @@ struct intel_framebuffer {
> > >  	};
> > >  
> > >  	struct i915_address_space *dpt_vm;
> > > +
> > > +	unsigned int min_alignment;
> > >  };
> > >  
> > >  enum intel_hotplug_state {
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > index 3f3a9cd534f4..c5bae05cbbc3 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > @@ -10,6 +10,7 @@
> > >  #include <linux/dma-resv.h>
> > >  
> > >  #include "i915_drv.h"
> > > +#include "intel_atomic_plane.h"
> > >  #include "intel_display.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_dpt.h"
> > > @@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
> > >  	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
> > >  }
> > >  
> > > +static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
> > > +{
> > > +	struct drm_i915_private *i915 = to_i915(fb->dev);
> > > +	struct intel_plane *plane;
> > > +	unsigned int min_alignment = 0;
> > > +
> > > +	for_each_intel_plane(&i915->drm, plane) {
> > > +		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
> > > +			continue;
> > > +
> > > +		if (intel_plane_needs_physical(plane))
> > > +			continue;
> > > +
> > > +		min_alignment = max(min_alignment,
> > > +				    plane->min_alignment(plane, fb, 0));
> > > +	}
> > > +
> > > +	return min_alignment;
> > > +}
> > > +
> > >  int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
> > >  {
> > >  	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
> > > @@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	fb->min_alignment = intel_fb_min_alignment(&fb->base);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > index 9b0f1ea41b70..1ae02de906f5 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > @@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> > >  	i915_vma_put(vma);
> > >  }
> > >  
> > > +static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
> > > +{
> > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > > +
> > > +	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
> > > +	    intel_fb_needs_pot_stride_remap(fb))
> > > +		return false;
> > 
> > The above view is created only once for the FB, I suppose this is how
> > you differentiate it from views created each time due to a stride
> > limit (based on intel_plane_needs_remap()).
> > 
> > > +
> > > +	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;
> > 
> > So the rotated and remapped views created for a stride limit are
> > per-plane based on the above. There is also a rotated view created only
> > once for the FB, is it intentional to regard this kind of view as
> > per-plane as well?
> 
> No. Forgot those exist. I guess I should just remove this per-plane
> alignment logic for now. Need to think of a proper way to add it back,
> or just quietly bury the whole idea.

Another variation is that intel_fb_pin_to_ggtt() etc. tries to bind the
vma in max -> min alignment order considering all the plane types here
always, though that's almost the opposite to what you want to solve
I suppose (avoid to overalign?).

> > > +}
> > > +
> > >  static unsigned int
> > >  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> > >  {
> > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > >  
> > > -	return plane->min_alignment(plane, fb, 0);
> > > +	/*
> > > +	 * Only use plane specific alignment for binding
> > > +	 * a per-plane gtt view (remapped or rotated),
> > > +	 * otherwise make sure the alignment is suitable
> > > +	 * for all planes.
> > > +	 */
> > > +	if (!gtt_view_is_per_plane(plane_state))
> > > +		return fb->min_alignment;
> > > +
> > > +	if (intel_plane_needs_physical(plane))
> > > +		return 0;
> > 
> > I guess the above is ok, though looks like an unrelated change: the
> > cursor plane min_alignment() for relevant platforms is <= 4k, which will
> > be rounded up to 4k anyway when binding the vma.
> 
> When intel_plane_needs_physical()==true the alignment declared
> by the plane only applies to the physical/dma address, as that
> is what the hardware will consume. Doesn't actually matter
> where it gets bound in the ggtt.

Ok, it's unclear why the same physical alignment was used then to bind
the vma as well, but didn't probably matter in practice since the
alignment was small. The commit log could still mention this change.

> > > +
> > > +	return plane->min_alignment(plane, &fb->base, 0);
> > 
> > The commit could've had more details about the rational for the above.
> > As I understand it avoids having to rebind the vma for different plane
> > types, though this is already handled to some degree by
> > i915_vma::display_alignment.
> 
> It was also semi-handled by intel_surf_alignment() trying to
> return the max alignment for all the planes more or less.
> But it was a fairly big mess.
> 
> I did't really remember that the display_alignment
> thing even exists. Hmm. I wonder if it's even needed 
> anymore. I suppose we can have cases where the display was
> previously using the vma, which was subsequency unbound,
> and later rebound due to some other reason. We probably
> want to preserve the alignment to make sure it'll again
> be ready to be used by the display without an extra
> rebind.  I guess that only really happens with pre-ppgtt
> platforms and/or software rendering via ggtt.

Okay, thanks for clarifying. So the idea behind it wasn't using the same
vma for multiple plane types. It makes sense to optimize the alignment
upfront as done in this patch in any case and regard display_alignment
as independent to this logic.

> Hmm. We could perpaps change the implementation to adjust
> display_alignment when the framebuffer is created instead
> of doing it when the vma actually gets pinned. Might not
> matter for real world uses cases though.
> 
> > 
> > >  }
> > >  
> > >  static unsigned int
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > index ff685aebbd1a..124aac172acb 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > > @@ -46,7 +46,6 @@
> > >  #include "gem/i915_gem_mman.h"
> > >  
> > >  #include "i915_drv.h"
> > > -#include "intel_crtc.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_fb.h"
> > >  #include "intel_fb_pin.h"
> > > @@ -172,21 +171,6 @@ static const struct fb_ops intelfb_ops = {
> > >  
> > >  __diag_pop();
> > >  
> > > -static unsigned int intel_fbdev_min_alignment(const struct drm_framebuffer *fb)
> > > -{
> > > -	struct drm_i915_private *i915 = to_i915(fb->dev);
> > > -	struct intel_plane *plane;
> > > -	struct intel_crtc *crtc;
> > > -
> > > -	crtc = intel_first_crtc(i915);
> > > -	if (!crtc)
> > > -		return 0;
> > > -
> > > -	plane = to_intel_plane(crtc->base.primary);
> > > -
> > > -	return plane->min_alignment(plane, fb, 0);
> > > -}
> > > -
> > >  static int intelfb_create(struct drm_fb_helper *helper,
> > >  			  struct drm_fb_helper_surface_size *sizes)
> > >  {
> > > @@ -244,7 +228,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	 * BIOS is suitable for own access.
> > >  	 */
> > >  	vma = intel_fb_pin_to_ggtt(&fb->base, &view,
> > > -				   intel_fbdev_min_alignment(&fb->base), 0,
> > > +				   fb->min_alignment, 0,
> > >  				   false, &flags);
> > >  	if (IS_ERR(vma)) {
> > >  		ret = PTR_ERR(vma);
> > > -- 
> > > 2.43.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Re: [PATCH 4/9] drm/i915: Introduce fb->min_alignment

[Ville Syrjälä]

On Wed, May 29, 2024 at 12:37:30AM +0300, Imre Deak wrote:
> On Tue, May 28, 2024 at 10:38:06PM +0300, Ville Syrjälä wrote:
> > On Tue, May 28, 2024 at 02:27:52PM +0300, Imre Deak wrote:
> > > On Mon, May 13, 2024 at 08:59:37PM +0300, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Different planes could have different alignment requirements
> > > > even for the same format/modifier. Collect the alignment
> > > > requirements across all planes capable of scanning out the
> > > > fb such that the alignment used when pinning the normal ggtt
> > > > view is satisfactory to all those planes.
> > > > 
> > > > When pinning per-plane views we only have to satisfy the
> > > > alignment requirements of the specific plane.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_display_types.h    |  2 ++
> > > >  drivers/gpu/drm/i915/display/intel_fb.c       | 23 ++++++++++++++++
> > > >  drivers/gpu/drm/i915/display/intel_fb_pin.c   | 27 +++++++++++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_fbdev.c    | 18 +------------
> > > >  4 files changed, 51 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 40d6e5f4c350..58bb65832adf 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -145,6 +145,8 @@ struct intel_framebuffer {
> > > >  	};
> > > >  
> > > >  	struct i915_address_space *dpt_vm;
> > > > +
> > > > +	unsigned int min_alignment;
> > > >  };
> > > >  
> > > >  enum intel_hotplug_state {
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > index 3f3a9cd534f4..c5bae05cbbc3 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <linux/dma-resv.h>
> > > >  
> > > >  #include "i915_drv.h"
> > > > +#include "intel_atomic_plane.h"
> > > >  #include "intel_display.h"
> > > >  #include "intel_display_types.h"
> > > >  #include "intel_dpt.h"
> > > > @@ -1616,6 +1617,26 @@ bool intel_fb_supports_90_270_rotation(const struct intel_framebuffer *fb)
> > > >  	       fb->base.modifier == I915_FORMAT_MOD_Yf_TILED;
> > > >  }
> > > >  
> > > > +static unsigned int intel_fb_min_alignment(const struct drm_framebuffer *fb)
> > > > +{
> > > > +	struct drm_i915_private *i915 = to_i915(fb->dev);
> > > > +	struct intel_plane *plane;
> > > > +	unsigned int min_alignment = 0;
> > > > +
> > > > +	for_each_intel_plane(&i915->drm, plane) {
> > > > +		if (!drm_plane_has_format(&plane->base, fb->format->format, fb->modifier))
> > > > +			continue;
> > > > +
> > > > +		if (intel_plane_needs_physical(plane))
> > > > +			continue;
> > > > +
> > > > +		min_alignment = max(min_alignment,
> > > > +				    plane->min_alignment(plane, fb, 0));
> > > > +	}
> > > > +
> > > > +	return min_alignment;
> > > > +}
> > > > +
> > > >  int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *fb)
> > > >  {
> > > >  	struct drm_i915_gem_object *obj = intel_fb_obj(&fb->base);
> > > > @@ -1698,6 +1719,8 @@ int intel_fill_fb_info(struct drm_i915_private *i915, struct intel_framebuffer *
> > > >  		return -EINVAL;
> > > >  	}
> > > >  
> > > > +	fb->min_alignment = intel_fb_min_alignment(&fb->base);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > index 9b0f1ea41b70..1ae02de906f5 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > > > @@ -230,13 +230,36 @@ void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> > > >  	i915_vma_put(vma);
> > > >  }
> > > >  
> > > > +static bool gtt_view_is_per_plane(const struct intel_plane_state *plane_state)
> > > > +{
> > > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > > > +
> > > > +	if (plane_state->view.gtt.type == I915_GTT_VIEW_REMAPPED &&
> > > > +	    intel_fb_needs_pot_stride_remap(fb))
> > > > +		return false;
> > > 
> > > The above view is created only once for the FB, I suppose this is how
> > > you differentiate it from views created each time due to a stride
> > > limit (based on intel_plane_needs_remap()).
> > > 
> > > > +
> > > > +	return plane_state->view.gtt.type != I915_GTT_VIEW_NORMAL;
> > > 
> > > So the rotated and remapped views created for a stride limit are
> > > per-plane based on the above. There is also a rotated view created only
> > > once for the FB, is it intentional to regard this kind of view as
> > > per-plane as well?
> > 
> > No. Forgot those exist. I guess I should just remove this per-plane
> > alignment logic for now. Need to think of a proper way to add it back,
> > or just quietly bury the whole idea.
> 
> Another variation is that intel_fb_pin_to_ggtt() etc. tries to bind the
> vma in max -> min alignment order considering all the plane types here
> always, though that's almost the opposite to what you want to solve
> I suppose (avoid to overalign?).
> 
> > > > +}
> > > > +
> > > >  static unsigned int
> > > >  intel_plane_fb_min_alignment(const struct intel_plane_state *plane_state)
> > > >  {
> > > >  	struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
> > > > -	const struct drm_framebuffer *fb = plane_state->hw.fb;
> > > > +	const struct intel_framebuffer *fb = to_intel_framebuffer(plane_state->hw.fb);
> > > >  
> > > > -	return plane->min_alignment(plane, fb, 0);
> > > > +	/*
> > > > +	 * Only use plane specific alignment for binding
> > > > +	 * a per-plane gtt view (remapped or rotated),
> > > > +	 * otherwise make sure the alignment is suitable
> > > > +	 * for all planes.
> > > > +	 */
> > > > +	if (!gtt_view_is_per_plane(plane_state))
> > > > +		return fb->min_alignment;
> > > > +
> > > > +	if (intel_plane_needs_physical(plane))
> > > > +		return 0;
> > > 
> > > I guess the above is ok, though looks like an unrelated change: the
> > > cursor plane min_alignment() for relevant platforms is <= 4k, which will
> > > be rounded up to 4k anyway when binding the vma.
> > 
> > When intel_plane_needs_physical()==true the alignment declared
> > by the plane only applies to the physical/dma address, as that
> > is what the hardware will consume. Doesn't actually matter
> > where it gets bound in the ggtt.
> 
> Ok, it's unclear why the same physical alignment was used then to bind
> the vma as well, but didn't probably matter in practice since the
> alignment was small. The commit log could still mention this change.

I guess if I just drop the per-plane tricks then we should just 
always use fb->min_alignment for the ggtt. Although I think that
is going to be end up being 0 since currently we expose the ARGB
format only for cursor planes on these platforms.

-- 
Ville Syrjälä
Intel