diff mbox

drm/i915: Fix sprite offset on HSW

Message ID 1351181401-18088-1-git-send-email-damien.lespiau@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Damien Lespiau Oct. 25, 2012, 4:10 p.m. UTC
From: Damien Lespiau <damien.lespiau@intel.com>

HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
register.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h     |    3 +++
 drivers/gpu/drm/i915/intel_sprite.c |   18 +++++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

Comments

Mika Kuoppala Oct. 26, 2012, 7:33 a.m. UTC | #1
On Thu, 25 Oct 2012 17:10:01 +0100, Damien Lespiau <damien.lespiau@gmail.com> wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
> register.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |    3 +++
>  drivers/gpu/drm/i915/intel_sprite.c |   18 +++++++++++++-----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be22aeb..2a6c0b6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3187,6 +3187,7 @@
>  #define _SPRA_SURF		0x7029c
>  #define _SPRA_KEYMAX		0x702a0
>  #define _SPRA_TILEOFF		0x702a4
> +#define _SPRA_OFFSET		0x702a4
>  #define _SPRA_SCALE		0x70304
>  #define   SPRITE_SCALE_ENABLE	(1<<31)
>  #define   SPRITE_FILTER_MASK	(3<<29)
> @@ -3207,6 +3208,7 @@
>  #define _SPRB_SURF		0x7129c
>  #define _SPRB_KEYMAX		0x712a0
>  #define _SPRB_TILEOFF		0x712a4
> +#define _SPRB_OFFSET		0x712a4
>  #define _SPRB_SCALE		0x71304
>  #define _SPRB_GAMC		0x71400
>  
> @@ -3220,6 +3222,7 @@
>  #define SPRSURF(pipe) _PIPE(pipe, _SPRA_SURF, _SPRB_SURF)
>  #define SPRKEYMAX(pipe) _PIPE(pipe, _SPRA_KEYMAX, _SPRB_KEYMAX)
>  #define SPRTILEOFF(pipe) _PIPE(pipe, _SPRA_TILEOFF, _SPRB_TILEOFF)
> +#define SPROFFSET(pipe) _PIPE(pipe, _SPRA_OFFSET, _SPRB_OFFSET)
>  #define SPRSCALE(pipe) _PIPE(pipe, _SPRA_SCALE, _SPRB_SCALE)
>  #define SPRGAMC(pipe) _PIPE(pipe, _SPRA_GAMC, _SPRB_GAMC)
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 176c462..24b8231 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -127,13 +127,21 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
>  
>  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> -	if (obj->tiling_mode != I915_TILING_NONE) {
> -		I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> +
> +	if (IS_HASWELL(dev)) {
> +		/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single
> +		 * SPROFFSET register */

I don't know if upper layers sanitize already but
x should be < 8192 and y < 4096 in here, and both <4096 for ivb.
Emit warning and clamp if they are not in range?

> +		I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
>  	} else {
> -		unsigned long offset;
> +		if (obj->tiling_mode != I915_TILING_NONE) {
> +			I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> +		} else {
> +			unsigned long offset;
>  
> -		offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
> -		I915_WRITE(SPRLINOFF(pipe), offset);
> +			offset = y * fb->pitches[0] +
> +				 x * (fb->bits_per_pixel / 8);
> +			I915_WRITE(SPRLINOFF(pipe), offset);
> +		}
>  	}
>  	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
>  	if (intel_plane->can_scale)
> -- 
> 1.7.7.5

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Chris Wilson Oct. 26, 2012, 9:48 a.m. UTC | #2
On Fri, 26 Oct 2012 10:33:00 +0300, Mika Kuoppala <mika.kuoppala@linux.intel.com> wrote:
> On Thu, 25 Oct 2012 17:10:01 +0100, Damien Lespiau <damien.lespiau@gmail.com> wrote:
> > @@ -127,13 +127,21 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
> >  
> >  	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
> >  	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> > -	if (obj->tiling_mode != I915_TILING_NONE) {
> > -		I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> > +
> > +	if (IS_HASWELL(dev)) {
> > +		/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single
> > +		 * SPROFFSET register */
> 
> I don't know if upper layers sanitize already but
> x should be < 8192 and y < 4096 in here, and both <4096 for ivb.
> Emit warning and clamp if they are not in range?

Should be -EINVAL on entry, and BUG_ON here if truly paranoid -
knowledge of the hardware is already spread across the layers.
-Chris
Daniel Vetter Oct. 26, 2012, 9:49 a.m. UTC | #3
On Fri, Oct 26, 2012 at 9:33 AM, Mika Kuoppala
<mika.kuoppala@linux.intel.com> wrote:
> I don't know if upper layers sanitize already but
> x should be < 8192 and y < 4096 in here, and both <4096 for ivb.
> Emit warning and clamp if they are not in range?

We probably need to do the same trick we're already doing in the main
plane code by adjusting the the base address, see:

commit c2c75131244507c93f812862fdbd4f3a37139401
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jul 5 12:17:30 2012 +0200

    drm/i915: adjust framebuffer base address on gen4+

Damien, could you throw this in on top?

Thanks, Daniel
Paulo Zanoni Oct. 26, 2012, 1:18 p.m. UTC | #4
Hi

2012/10/25 Damien Lespiau <damien.lespiau@gmail.com>:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> HSW consolidates SPRTILEOFF and SPRLINOFF into a single SPROFFSET
> register.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h     |    3 +++
>  drivers/gpu/drm/i915/intel_sprite.c |   18 +++++++++++++-----
>  2 files changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be22aeb..2a6c0b6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3187,6 +3187,7 @@
>  #define _SPRA_SURF             0x7029c
>  #define _SPRA_KEYMAX           0x702a0
>  #define _SPRA_TILEOFF          0x702a4
> +#define _SPRA_OFFSET           0x702a4
>  #define _SPRA_SCALE            0x70304
>  #define   SPRITE_SCALE_ENABLE  (1<<31)
>  #define   SPRITE_FILTER_MASK   (3<<29)
> @@ -3207,6 +3208,7 @@
>  #define _SPRB_SURF             0x7129c
>  #define _SPRB_KEYMAX           0x712a0
>  #define _SPRB_TILEOFF          0x712a4
> +#define _SPRB_OFFSET           0x712a4
>  #define _SPRB_SCALE            0x71304
>  #define _SPRB_GAMC             0x71400
>
> @@ -3220,6 +3222,7 @@
>  #define SPRSURF(pipe) _PIPE(pipe, _SPRA_SURF, _SPRB_SURF)
>  #define SPRKEYMAX(pipe) _PIPE(pipe, _SPRA_KEYMAX, _SPRB_KEYMAX)
>  #define SPRTILEOFF(pipe) _PIPE(pipe, _SPRA_TILEOFF, _SPRB_TILEOFF)
> +#define SPROFFSET(pipe) _PIPE(pipe, _SPRA_OFFSET, _SPRB_OFFSET)
>  #define SPRSCALE(pipe) _PIPE(pipe, _SPRA_SCALE, _SPRB_SCALE)
>  #define SPRGAMC(pipe) _PIPE(pipe, _SPRA_GAMC, _SPRB_GAMC)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 176c462..24b8231 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -127,13 +127,21 @@ ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
>
>         I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
>         I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> -       if (obj->tiling_mode != I915_TILING_NONE) {
> -               I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> +
> +       if (IS_HASWELL(dev)) {
> +               /* HSW consolidates SPRTILEOFF and SPRLINOFF into a single
> +                * SPROFFSET register */
> +               I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
>         } else {
> -               unsigned long offset;
> +               if (obj->tiling_mode != I915_TILING_NONE) {
> +                       I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> +               } else {
> +                       unsigned long offset;
>
> -               offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
> -               I915_WRITE(SPRLINOFF(pipe), offset);
> +                       offset = y * fb->pitches[0] +
> +                                x * (fb->bits_per_pixel / 8);
> +                       I915_WRITE(SPRLINOFF(pipe), offset);
> +               }

<bikeshed>
Doing "if (is_hsw) { code; } else if (obj->tiling_mode) { code; } else
{ code; }" would probably make reading the patch easier :)
But I don't really think you need to resend just because of this.
</bikeshed>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'd love to see the same fix for the "Primary Plane" registers since
this will get us rid of many "Unclaimed write" messages.

>         }
>         I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
>         if (intel_plane->can_scale)
> --
> 1.7.7.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index be22aeb..2a6c0b6 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3187,6 +3187,7 @@ 
 #define _SPRA_SURF		0x7029c
 #define _SPRA_KEYMAX		0x702a0
 #define _SPRA_TILEOFF		0x702a4
+#define _SPRA_OFFSET		0x702a4
 #define _SPRA_SCALE		0x70304
 #define   SPRITE_SCALE_ENABLE	(1<<31)
 #define   SPRITE_FILTER_MASK	(3<<29)
@@ -3207,6 +3208,7 @@ 
 #define _SPRB_SURF		0x7129c
 #define _SPRB_KEYMAX		0x712a0
 #define _SPRB_TILEOFF		0x712a4
+#define _SPRB_OFFSET		0x712a4
 #define _SPRB_SCALE		0x71304
 #define _SPRB_GAMC		0x71400
 
@@ -3220,6 +3222,7 @@ 
 #define SPRSURF(pipe) _PIPE(pipe, _SPRA_SURF, _SPRB_SURF)
 #define SPRKEYMAX(pipe) _PIPE(pipe, _SPRA_KEYMAX, _SPRB_KEYMAX)
 #define SPRTILEOFF(pipe) _PIPE(pipe, _SPRA_TILEOFF, _SPRB_TILEOFF)
+#define SPROFFSET(pipe) _PIPE(pipe, _SPRA_OFFSET, _SPRB_OFFSET)
 #define SPRSCALE(pipe) _PIPE(pipe, _SPRA_SCALE, _SPRB_SCALE)
 #define SPRGAMC(pipe) _PIPE(pipe, _SPRA_GAMC, _SPRB_GAMC)
 
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 176c462..24b8231 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -127,13 +127,21 @@  ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
 
 	I915_WRITE(SPRSTRIDE(pipe), fb->pitches[0]);
 	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
-	if (obj->tiling_mode != I915_TILING_NONE) {
-		I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
+
+	if (IS_HASWELL(dev)) {
+		/* HSW consolidates SPRTILEOFF and SPRLINOFF into a single
+		 * SPROFFSET register */
+		I915_WRITE(SPROFFSET(pipe), (y << 16) | x);
 	} else {
-		unsigned long offset;
+		if (obj->tiling_mode != I915_TILING_NONE) {
+			I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
+		} else {
+			unsigned long offset;
 
-		offset = y * fb->pitches[0] + x * (fb->bits_per_pixel / 8);
-		I915_WRITE(SPRLINOFF(pipe), offset);
+			offset = y * fb->pitches[0] +
+				 x * (fb->bits_per_pixel / 8);
+			I915_WRITE(SPRLINOFF(pipe), offset);
+		}
 	}
 	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
 	if (intel_plane->can_scale)