[1/3] drm/i915: Actually respect DSPSURF alignment restrictions
diff mbox

Message ID 1434029476-958-1-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä June 11, 2015, 1:31 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently intel_gen4_compute_page_offset() simply picks the closest
page boundary below the linear offset. That however may not be suitably
aligned to satisfy any hardware specific restrictions. So let's make
sure the page boundary we choose is properly aligned.

Also to play it a bit safer lets split the remaining linear offset into
x and y values instead of just x. This should make no difference for
most platforms since we convert the x and y offsets back into a linear
offset before feeding them to the hardware. HSW+ are different however
and use x and y offsets even with linear buffers, so they might have
trouble if either the x or y get too big.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++-
 drivers/gpu/drm/i915/intel_sprite.c  |  9 ++++++---
 3 files changed, 31 insertions(+), 18 deletions(-)

Comments

Chris Wilson June 11, 2015, 1:51 p.m. UTC | #1
On Thu, Jun 11, 2015 at 04:31:14PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently intel_gen4_compute_page_offset() simply picks the closest
> page boundary below the linear offset. That however may not be suitably
> aligned to satisfy any hardware specific restrictions. So let's make
> sure the page boundary we choose is properly aligned.
> 
> Also to play it a bit safer lets split the remaining linear offset into
> x and y values instead of just x. This should make no difference for
> most platforms since we convert the x and y offsets back into a linear
> offset before feeding them to the hardware. HSW+ are different however
> and use x and y offsets even with linear buffers, so they might have
> trouble if either the x or y get too big.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---

> @@ -2455,12 +2461,13 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
>  
>  		return tile_rows * pitch * 8 + tiles * 4096;
>  	} else {
> +		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
>  		unsigned int offset;
>  
>  		offset = *y * pitch + *x * cpp;
> -		*y = 0;
> -		*x = (offset & 4095) / cpp;
> -		return offset & -4096;
> +		*y = (offset & alignment) / pitch;
> +		*x = ((offset & alignment) - *y * pitch) / cpp;
> +		return offset & ~alignment;

Calculation looks solid. I presume we have a igt/kms test that combines
linear/tiled, large surfaces and large offsets?

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Ville Syrjälä June 12, 2015, 10:50 a.m. UTC | #2
On Thu, Jun 11, 2015 at 02:51:19PM +0100, Chris Wilson wrote:
> On Thu, Jun 11, 2015 at 04:31:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently intel_gen4_compute_page_offset() simply picks the closest
> > page boundary below the linear offset. That however may not be suitably
> > aligned to satisfy any hardware specific restrictions. So let's make
> > sure the page boundary we choose is properly aligned.
> > 
> > Also to play it a bit safer lets split the remaining linear offset into
> > x and y values instead of just x. This should make no difference for
> > most platforms since we convert the x and y offsets back into a linear
> > offset before feeding them to the hardware. HSW+ are different however
> > and use x and y offsets even with linear buffers, so they might have
> > trouble if either the x or y get too big.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> 
> > @@ -2455,12 +2461,13 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
> >  
> >  		return tile_rows * pitch * 8 + tiles * 4096;
> >  	} else {
> > +		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
> >  		unsigned int offset;
> >  
> >  		offset = *y * pitch + *x * cpp;
> > -		*y = 0;
> > -		*x = (offset & 4095) / cpp;
> > -		return offset & -4096;
> > +		*y = (offset & alignment) / pitch;
> > +		*x = ((offset & alignment) - *y * pitch) / cpp;
> > +		return offset & ~alignment;
> 
> Calculation looks solid. I presume we have a igt/kms test that combines
> linear/tiled, large surfaces and large offsets?

kms_plane has some kind of panning tests. Probably not as good as it
could/should be. I have a few custom tests I created to hunt for the
VLV/CHV bug, but those aren't really useable as regular igt tests as
is. Would take a bit of extra effort to turn them into such.

> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Daniel Vetter June 15, 2015, 4:04 p.m. UTC | #3
On Fri, Jun 12, 2015 at 01:50:13PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 11, 2015 at 02:51:19PM +0100, Chris Wilson wrote:
> > On Thu, Jun 11, 2015 at 04:31:14PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Currently intel_gen4_compute_page_offset() simply picks the closest
> > > page boundary below the linear offset. That however may not be suitably
> > > aligned to satisfy any hardware specific restrictions. So let's make
> > > sure the page boundary we choose is properly aligned.
> > > 
> > > Also to play it a bit safer lets split the remaining linear offset into
> > > x and y values instead of just x. This should make no difference for
> > > most platforms since we convert the x and y offsets back into a linear
> > > offset before feeding them to the hardware. HSW+ are different however
> > > and use x and y offsets even with linear buffers, so they might have
> > > trouble if either the x or y get too big.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > 
> > > @@ -2455,12 +2461,13 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
> > >  
> > >  		return tile_rows * pitch * 8 + tiles * 4096;
> > >  	} else {
> > > +		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
> > >  		unsigned int offset;
> > >  
> > >  		offset = *y * pitch + *x * cpp;
> > > -		*y = 0;
> > > -		*x = (offset & 4095) / cpp;
> > > -		return offset & -4096;
> > > +		*y = (offset & alignment) / pitch;
> > > +		*x = ((offset & alignment) - *y * pitch) / cpp;
> > > +		return offset & ~alignment;
> > 
> > Calculation looks solid. I presume we have a igt/kms test that combines
> > linear/tiled, large surfaces and large offsets?
> 
> kms_plane has some kind of panning tests. Probably not as good as it
> could/should be. I have a few custom tests I created to hunt for the
> VLV/CHV bug, but those aren't really useable as regular igt tests as
> is. Would take a bit of extra effort to turn them into such.

We'd need a crc based testcase where we first draw a test picture at (0,0)
and then with a hilariously large buffer at some offset. Evil values seems
to be anything > 2048 iirc.

Testcase for this would be really good since I think skl is all broken
too.
-Daniel

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5cc2263..0ac213a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2325,6 +2325,18 @@  intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb,
 	return 0;
 }
 
+static unsigned int intel_linear_alignment(struct drm_i915_private *dev_priv)
+{
+	if (INTEL_INFO(dev_priv)->gen >= 9)
+		return 256 * 1024;
+	else if (IS_BROADWATER(dev_priv) || IS_CRESTLINE(dev_priv))
+		return 128 * 1024;
+	else if (INTEL_INFO(dev_priv)->gen >= 4)
+		return 4 * 1024;
+	else
+		return 64 * 1024;
+}
+
 int
 intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 			   struct drm_framebuffer *fb,
@@ -2342,14 +2354,7 @@  intel_pin_and_fence_fb_obj(struct drm_plane *plane,
 
 	switch (fb->modifier[0]) {
 	case DRM_FORMAT_MOD_NONE:
-		if (INTEL_INFO(dev)->gen >= 9)
-			alignment = 256 * 1024;
-		else if (IS_BROADWATER(dev) || IS_CRESTLINE(dev))
-			alignment = 128 * 1024;
-		else if (INTEL_INFO(dev)->gen >= 4)
-			alignment = 4 * 1024;
-		else
-			alignment = 64 * 1024;
+		alignment = intel_linear_alignment(dev_priv);
 		break;
 	case I915_FORMAT_MOD_X_TILED:
 		if (INTEL_INFO(dev)->gen >= 9)
@@ -2439,7 +2444,8 @@  static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per pixel
  * is assumed to be a power-of-two. */
-unsigned long intel_gen4_compute_page_offset(int *x, int *y,
+unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv,
+					     int *x, int *y,
 					     unsigned int tiling_mode,
 					     unsigned int cpp,
 					     unsigned int pitch)
@@ -2455,12 +2461,13 @@  unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 
 		return tile_rows * pitch * 8 + tiles * 4096;
 	} else {
+		unsigned int alignment = intel_linear_alignment(dev_priv) - 1;
 		unsigned int offset;
 
 		offset = *y * pitch + *x * cpp;
-		*y = 0;
-		*x = (offset & 4095) / cpp;
-		return offset & -4096;
+		*y = (offset & alignment) / pitch;
+		*x = ((offset & alignment) - *y * pitch) / cpp;
+		return offset & ~alignment;
 	}
 }
 
@@ -2729,7 +2736,8 @@  static void i9xx_update_primary_plane(struct drm_crtc *crtc,
 
 	if (INTEL_INFO(dev)->gen >= 4) {
 		intel_crtc->dspaddr_offset =
-			intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
+			intel_gen4_compute_page_offset(dev_priv,
+						       &x, &y, obj->tiling_mode,
 						       pixel_size,
 						       fb->pitches[0]);
 		linear_offset -= intel_crtc->dspaddr_offset;
@@ -2830,7 +2838,8 @@  static void ironlake_update_primary_plane(struct drm_crtc *crtc,
 
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	intel_crtc->dspaddr_offset =
-		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
+		intel_gen4_compute_page_offset(dev_priv,
+					       &x, &y, obj->tiling_mode,
 					       pixel_size,
 					       fb->pitches[0]);
 	linear_offset -= intel_crtc->dspaddr_offset;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b28029a..c28d117 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1113,7 +1113,8 @@  void assert_fdi_rx_pll(struct drm_i915_private *dev_priv,
 void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe, bool state);
 #define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
 #define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
-unsigned long intel_gen4_compute_page_offset(int *x, int *y,
+unsigned long intel_gen4_compute_page_offset(struct drm_i915_private *dev_priv,
+					     int *x, int *y,
 					     unsigned int tiling_mode,
 					     unsigned int bpp,
 					     unsigned int pitch);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8193a35..28f6972 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -411,7 +411,8 @@  vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	crtc_h--;
 
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
-	sprsurf_offset = intel_gen4_compute_page_offset(&x, &y,
+	sprsurf_offset = intel_gen4_compute_page_offset(dev_priv,
+							&x, &y,
 							obj->tiling_mode,
 							pixel_size,
 							fb->pitches[0]);
@@ -546,7 +547,8 @@  ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	sprsurf_offset =
-		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
+		intel_gen4_compute_page_offset(dev_priv,
+					       &x, &y, obj->tiling_mode,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= sprsurf_offset;
 
@@ -682,7 +684,8 @@  ilk_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	linear_offset = y * fb->pitches[0] + x * pixel_size;
 	dvssurf_offset =
-		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
+		intel_gen4_compute_page_offset(dev_priv,
+					       &x, &y, obj->tiling_mode,
 					       pixel_size, fb->pitches[0]);
 	linear_offset -= dvssurf_offset;