[v2,6/9] drm/i915: Use enum plane_id in VLV/CHV sprite code

Message ID	1478624950-31488-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: ville.syrjala@linux.intel.com To: intel-gfx@lists.freedesktop.org Date: Tue, 8 Nov 2016 19:09:10 +0200 Message-Id: <1478624950-31488-1-git-send-email-ville.syrjala@linux.intel.com> In-Reply-To: <1478616439-10150-7-git-send-email-ville.syrjala@linux.intel.com> References: <1478616439-10150-7-git-send-email-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Use enum plane_id in VLV/CHV sprite code Precedence: list Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Message ID

1478624950-31488-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)

State

New, archived

Headers

From: ville.syrjala@linux.intel.com
To: intel-gfx@lists.freedesktop.org
Date: Tue,  8 Nov 2016 19:09:10 +0200
Message-Id: <1478624950-31488-1-git-send-email-ville.syrjala@linux.intel.com>
In-Reply-To: <1478616439-10150-7-git-send-email-ville.syrjala@linux.intel.com>
References: <1478616439-10150-7-git-send-email-ville.syrjala@linux.intel.com>
MIME-Version: 1.0
Subject: [Intel-gfx] [PATCH v2 6/9] drm/i915: Use enum plane_id in VLV/CHV
	sprite code
Precedence: list
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
Errors-To: intel-gfx-bounces@lists.freedesktop.org
Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Commit Message

Ville Syrjala Nov. 8, 2016, 5:09 p.m. UTC

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

Use intel_plane->id to derive the VLV/CHV sprite register offsets
instead of abusing plane->plane which is really meant to for
primary planes only.

v2: Convert assert_sprites_disabled() over as well

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 58 +++++++++++++++-------------
 drivers/gpu/drm/i915/intel_display.c |  2 +-
 drivers/gpu/drm/i915/intel_sprite.c  | 74 ++++++++++++++++++------------------
 3 files changed, 70 insertions(+), 64 deletions(-)

Comments

Zanoni, Paulo R Nov. 17, 2016, 8:07 p.m. UTC | #1

Em Ter, 2016-11-08 às 19:09 +0200, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use intel_plane->id to derive the VLV/CHV sprite register offsets
> instead of abusing plane->plane which is really meant to for
> primary planes only.

I think this patch is kinda an argument against the series due to the
new SPRITE0 subtractions, but I still think we're better with the
series applied.

A little more below:

> 
> v2: Convert assert_sprites_disabled() over as well
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 58 +++++++++++++++-----------
> --
>  drivers/gpu/drm/i915/intel_display.c |  2 +-
>  drivers/gpu/drm/i915/intel_sprite.c  | 74 ++++++++++++++++++------
> ------------
>  3 files changed, 70 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 3361d7ffc63e..9739e97c6263 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5374,18 +5374,21 @@ enum {
>  #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
>  #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
>  
> -#define SPCNTR(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPACNTR, _SPBCNTR)
> -#define SPLINOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPALINOFF, _SPBLINOFF)
> -#define SPSTRIDE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPASTRIDE, _SPBSTRIDE)
> -#define SPPOS(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAPOS,
> _SPBPOS)
> -#define SPSIZE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPASIZE, _SPBSIZE)
> -#define SPKEYMINVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPAKEYMINVAL, _SPBKEYMINVAL)
> -#define SPKEYMSK(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPAKEYMSK, _SPBKEYMSK)
> -#define SPSURF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPASURF, _SPBSURF)
> -#define SPKEYMAXVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> -#define SPTILEOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPATILEOFF, _SPBTILEOFF)
> -#define SPCONSTALPHA(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPACONSTALPHA, _SPBCONSTALPHA)
> -#define SPGAMC(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> _SPAGAMC, _SPBGAMC)
> +#define _MMIO_VLV_SPR(pipe, plane, reg_a, reg_b) \
> +	_MMIO_PIPE((pipe) * 2 + (plane) - PLANE_SPRITE0, (reg_a),
> (reg_b))

But but but you nicely renamed everything to plane_id and now you
create a macro with plane instead of plane_id? I'll have to call Ville
to fix this inconsistency.

(this applies to all changes in this file)

> +
> +#define SPCNTR(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPACNTR, _SPBCNTR)
> +#define SPLINOFF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPALINOFF, _SPBLINOFF)
> +#define SPSTRIDE(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPASTRIDE, _SPBSTRIDE)
> +#define SPPOS(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPAPOS, _SPBPOS)
> +#define SPSIZE(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPASIZE, _SPBSIZE)
> +#define SPKEYMINVAL(pipe, plane)	_MMIO_VLV_SPR((pipe),
> (plane), _SPAKEYMINVAL, _SPBKEYMINVAL)
> +#define SPKEYMSK(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPAKEYMSK, _SPBKEYMSK)
> +#define SPSURF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPASURF, _SPBSURF)
> +#define SPKEYMAXVAL(pipe, plane)	_MMIO_VLV_SPR((pipe),
> (plane), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> +#define SPTILEOFF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPATILEOFF, _SPBTILEOFF)
> +#define SPCONSTALPHA(pipe, plane)	_MMIO_VLV_SPR((pipe),
> (plane), _SPACONSTALPHA, _SPBCONSTALPHA)
> +#define SPGAMC(pipe, plane)		_MMIO_VLV_SPR((pipe),
> (plane), _SPAGAMC, _SPBGAMC)
>  
>  /*
>   * CHV pipe B sprite CSC
> @@ -5394,29 +5397,32 @@ enum {
>   * |yg| = |c3 c4 c5| x |yg + yg_ioff| + |yg_ooff|
>   * |cb|   |c6 c7 c8|   |cb + cr_ioff|   |cb_ooff|
>   */
> -#define SPCSCYGOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d900 +
> (sprite) * 0x1000)
> -#define SPCSCCBOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d904 +
> (sprite) * 0x1000)
> -#define SPCSCCROFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d908 +
> (sprite) * 0x1000)
> +#define _MMIO_CHV_SPCSC(plane, reg) \
> +	_MMIO(VLV_DISPLAY_BASE + ((plane) - PLANE_SPRITE0) * 0x1000
> + (reg))
> +
> +#define SPCSCYGOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d900)
> +#define SPCSCCBOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d904)
> +#define SPCSCCROFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d908)
>  #define  SPCSC_OOFF(x)		(((x) & 0x7ff) << 16) /* s11
> */
>  #define  SPCSC_IOFF(x)		(((x) & 0x7ff) << 0) /* s11 */
>  
> -#define SPCSCC01(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d90c +
> (sprite) * 0x1000)
> -#define SPCSCC23(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d910 +
> (sprite) * 0x1000)
> -#define SPCSCC45(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d914 +
> (sprite) * 0x1000)
> -#define SPCSCC67(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d918 +
> (sprite) * 0x1000)
> -#define SPCSCC8(sprite)		_MMIO(VLV_DISPLAY_BASE +
> 0x6d91c + (sprite) * 0x1000)
> +#define SPCSCC01(plane)		_MMIO_CHV_SPCSC(plane,
> 0x6d90c)
> +#define SPCSCC23(plane)		_MMIO_CHV_SPCSC(plane,
> 0x6d910)
> +#define SPCSCC45(plane)		_MMIO_CHV_SPCSC(plane,
> 0x6d914)
> +#define SPCSCC67(plane)		_MMIO_CHV_SPCSC(plane,
> 0x6d918)
> +#define SPCSCC8(plane)		_MMIO_CHV_SPCSC(plane,
> 0x6d91c)

Why is 1 "01" while 8 is "8"? :)

With or without changes:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


>  #define  SPCSC_C1(x)		(((x) & 0x7fff) << 16) /* s3.12
> */
>  #define  SPCSC_C0(x)		(((x) & 0x7fff) << 0) /* s3.12
> */
>  
> -#define SPCSCYGICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d920 + (sprite) * 0x1000)
> -#define SPCSCCBICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d924 + (sprite) * 0x1000)
> -#define SPCSCCRICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d928 + (sprite) * 0x1000)
> +#define SPCSCYGICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d920)
> +#define SPCSCCBICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d924)
> +#define SPCSCCRICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d928)
>  #define  SPCSC_IMAX(x)		(((x) & 0x7ff) << 16) /* s11
> */
>  #define  SPCSC_IMIN(x)		(((x) & 0x7ff) << 0) /* s11 */
>  
> -#define SPCSCYGOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d92c + (sprite) * 0x1000)
> -#define SPCSCCBOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d930 + (sprite) * 0x1000)
> -#define SPCSCCROCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> 0x6d934 + (sprite) * 0x1000)
> +#define SPCSCYGOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d92c)
> +#define SPCSCCBOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d930)
> +#define SPCSCCROCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d934)
>  #define  SPCSC_OMAX(x)		((x) << 16) /* u10 */
>  #define  SPCSC_OMIN(x)		((x) << 0) /* u10 */
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 95644c8cc568..a11c0f1d3f2e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1331,7 +1331,7 @@ static void assert_sprites_disabled(struct
> drm_i915_private *dev_priv,
>  		}
>  	} else if (IS_VALLEYVIEW(dev_priv) ||
> IS_CHERRYVIEW(dev_priv)) {
>  		for_each_sprite(dev_priv, pipe, sprite) {
> -			u32 val = I915_READ(SPCNTR(pipe, sprite));
> +			u32 val = I915_READ(SPCNTR(pipe,
> PLANE_SPRITE0 + sprite));
>  			I915_STATE_WARN(val & SP_ENABLE,
>  			     "sprite %c assertion failure, should be
> off on pipe %c but is still active\n",
>  			     sprite_name(pipe, sprite),
> pipe_name(pipe));
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 91d47d19f4a9..a922c6382fc7 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -314,7 +314,7 @@ static void
>  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_plane-
> >base.dev);
> -	int plane = intel_plane->plane;
> +	enum plane_id plane_id = intel_plane->id;
>  
>  	/* Seems RGB data bypasses the CSC always */
>  	if (!format_is_yuv(format))
> @@ -330,23 +330,23 @@ chv_update_csc(struct intel_plane *intel_plane,
> uint32_t format)
>  	 * Cb and Cr apparently come in as signed already, so no
>  	 * need for any offset. For Y we need to remove the offset.
>  	 */
> -	I915_WRITE(SPCSCYGOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(-
> 64));
> -	I915_WRITE(SPCSCCBOFF(plane), SPCSC_OOFF(0) |
> SPCSC_IOFF(0));
> -	I915_WRITE(SPCSCCROFF(plane), SPCSC_OOFF(0) |
> SPCSC_IOFF(0));
> -
> -	I915_WRITE(SPCSCC01(plane), SPCSC_C1(4769) |
> SPCSC_C0(6537));
> -	I915_WRITE(SPCSCC23(plane), SPCSC_C1(-3330) | SPCSC_C0(0));
> -	I915_WRITE(SPCSCC45(plane), SPCSC_C1(-1605) |
> SPCSC_C0(4769));
> -	I915_WRITE(SPCSCC67(plane), SPCSC_C1(4769) | SPCSC_C0(0));
> -	I915_WRITE(SPCSCC8(plane), SPCSC_C0(8263));
> -
> -	I915_WRITE(SPCSCYGICLAMP(plane), SPCSC_IMAX(940) |
> SPCSC_IMIN(64));
> -	I915_WRITE(SPCSCCBICLAMP(plane), SPCSC_IMAX(448) |
> SPCSC_IMIN(-448));
> -	I915_WRITE(SPCSCCRICLAMP(plane), SPCSC_IMAX(448) |
> SPCSC_IMIN(-448));
> -
> -	I915_WRITE(SPCSCYGOCLAMP(plane), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
> -	I915_WRITE(SPCSCCBOCLAMP(plane), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
> -	I915_WRITE(SPCSCCROCLAMP(plane), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
> +	I915_WRITE(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) |
> SPCSC_IOFF(-64));
> +	I915_WRITE(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) |
> SPCSC_IOFF(0));
> +	I915_WRITE(SPCSCCROFF(plane_id), SPCSC_OOFF(0) |
> SPCSC_IOFF(0));
> +
> +	I915_WRITE(SPCSCC01(plane_id), SPCSC_C1(4769) |
> SPCSC_C0(6537));
> +	I915_WRITE(SPCSCC23(plane_id), SPCSC_C1(-3330) |
> SPCSC_C0(0));
> +	I915_WRITE(SPCSCC45(plane_id), SPCSC_C1(-1605) |
> SPCSC_C0(4769));
> +	I915_WRITE(SPCSCC67(plane_id), SPCSC_C1(4769) |
> SPCSC_C0(0));
> +	I915_WRITE(SPCSCC8(plane_id), SPCSC_C0(8263));
> +
> +	I915_WRITE(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) |
> SPCSC_IMIN(64));
> +	I915_WRITE(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) |
> SPCSC_IMIN(-448));
> +	I915_WRITE(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) |
> SPCSC_IMIN(-448));
> +
> +	I915_WRITE(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
> +	I915_WRITE(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
> +	I915_WRITE(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) |
> SPCSC_OMIN(0));
>  }
>  
>  static void
> @@ -358,8 +358,8 @@ vlv_update_plane(struct drm_plane *dplane,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
>  	struct drm_framebuffer *fb = plane_state->base.fb;
> -	int pipe = intel_plane->pipe;
> -	int plane = intel_plane->plane;
> +	enum pipe pipe = intel_plane->pipe;
> +	enum plane_id plane_id = intel_plane->id;
>  	u32 sprctl;
>  	u32 sprsurf_offset, linear_offset;
>  	unsigned int rotation = plane_state->base.rotation;
> @@ -446,9 +446,9 @@ vlv_update_plane(struct drm_plane *dplane,
>  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
>  
>  	if (key->flags) {
> -		I915_WRITE(SPKEYMINVAL(pipe, plane), key-
> >min_value);
> -		I915_WRITE(SPKEYMAXVAL(pipe, plane), key-
> >max_value);
> -		I915_WRITE(SPKEYMSK(pipe, plane), key-
> >channel_mask);
> +		I915_WRITE(SPKEYMINVAL(pipe, plane_id), key-
> >min_value);
> +		I915_WRITE(SPKEYMAXVAL(pipe, plane_id), key-
> >max_value);
> +		I915_WRITE(SPKEYMSK(pipe, plane_id), key-
> >channel_mask);
>  	}
>  
>  	if (key->flags & I915_SET_COLORKEY_SOURCE)
> @@ -457,21 +457,21 @@ vlv_update_plane(struct drm_plane *dplane,
>  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
>  		chv_update_csc(intel_plane, fb->pixel_format);
>  
> -	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> -	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
> +	I915_WRITE(SPSTRIDE(pipe, plane_id), fb->pitches[0]);
> +	I915_WRITE(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
>  
>  	if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> -		I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
> +		I915_WRITE(SPTILEOFF(pipe, plane_id), (y << 16) |
> x);
>  	else
> -		I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
> +		I915_WRITE(SPLINOFF(pipe, plane_id), linear_offset);
>  
> -	I915_WRITE(SPCONSTALPHA(pipe, plane), 0);
> +	I915_WRITE(SPCONSTALPHA(pipe, plane_id), 0);
>  
> -	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> -	I915_WRITE(SPCNTR(pipe, plane), sprctl);
> -	I915_WRITE(SPSURF(pipe, plane),
> +	I915_WRITE(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> +	I915_WRITE(SPCNTR(pipe, plane_id), sprctl);
> +	I915_WRITE(SPSURF(pipe, plane_id),
>  		   intel_fb_gtt_offset(fb, rotation) +
> sprsurf_offset);
> -	POSTING_READ(SPSURF(pipe, plane));
> +	POSTING_READ(SPSURF(pipe, plane_id));
>  }
>  
>  static void
> @@ -480,13 +480,13 @@ vlv_disable_plane(struct drm_plane *dplane,
> struct drm_crtc *crtc)
>  	struct drm_device *dev = dplane->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> -	int pipe = intel_plane->pipe;
> -	int plane = intel_plane->plane;
> +	enum pipe pipe = intel_plane->pipe;
> +	enum plane_id plane_id = intel_plane->id;
>  
> -	I915_WRITE(SPCNTR(pipe, plane), 0);
> +	I915_WRITE(SPCNTR(pipe, plane_id), 0);
>  
> -	I915_WRITE(SPSURF(pipe, plane), 0);
> -	POSTING_READ(SPSURF(pipe, plane));
> +	I915_WRITE(SPSURF(pipe, plane_id), 0);
> +	POSTING_READ(SPSURF(pipe, plane_id));
>  }
>  
>  static void

Ville Syrjala Nov. 17, 2016, 8:19 p.m. UTC | #2

On Thu, Nov 17, 2016 at 06:07:36PM -0200, Paulo Zanoni wrote:
> Em Ter, 2016-11-08 às 19:09 +0200, ville.syrjala@linux.intel.com
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use intel_plane->id to derive the VLV/CHV sprite register offsets
> > instead of abusing plane->plane which is really meant to for
> > primary planes only.
> 
> I think this patch is kinda an argument against the series due to the
> new SPRITE0 subtractions, but I still think we're better with the
> series applied.

I really don't want to have two different kinds of per-pipe plane
identifiers. So the subtractions are IMO an accpeptable tradeoff for
that.

> 
> A little more below:
> 
> > 
> > v2: Convert assert_sprites_disabled() over as well
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 58 +++++++++++++++-----------
> > --
> >  drivers/gpu/drm/i915/intel_display.c |  2 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  | 74 ++++++++++++++++++------
> > ------------
> >  3 files changed, 70 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 3361d7ffc63e..9739e97c6263 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5374,18 +5374,21 @@ enum {
> >  #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
> >  #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
> >  
> > -#define SPCNTR(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPACNTR, _SPBCNTR)
> > -#define SPLINOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPALINOFF, _SPBLINOFF)
> > -#define SPSTRIDE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPASTRIDE, _SPBSTRIDE)
> > -#define SPPOS(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAPOS,
> > _SPBPOS)
> > -#define SPSIZE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPASIZE, _SPBSIZE)
> > -#define SPKEYMINVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPAKEYMINVAL, _SPBKEYMINVAL)
> > -#define SPKEYMSK(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPAKEYMSK, _SPBKEYMSK)
> > -#define SPSURF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPASURF, _SPBSURF)
> > -#define SPKEYMAXVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> > -#define SPTILEOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPATILEOFF, _SPBTILEOFF)
> > -#define SPCONSTALPHA(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPACONSTALPHA, _SPBCONSTALPHA)
> > -#define SPGAMC(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane),
> > _SPAGAMC, _SPBGAMC)
> > +#define _MMIO_VLV_SPR(pipe, plane, reg_a, reg_b) \
> > +	_MMIO_PIPE((pipe) * 2 + (plane) - PLANE_SPRITE0, (reg_a),
> > (reg_b))
> 
> But but but you nicely renamed everything to plane_id and now you
> create a macro with plane instead of plane_id?

Rather I kept the 'plane' naming here because, well, I'm lazy :)

> I'll have to call Ville
> to fix this inconsistency.

I can re-sed it a bit.

> 
> (this applies to all changes in this file)
> 
> > +
> > +#define SPCNTR(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPACNTR, _SPBCNTR)
> > +#define SPLINOFF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPALINOFF, _SPBLINOFF)
> > +#define SPSTRIDE(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPASTRIDE, _SPBSTRIDE)
> > +#define SPPOS(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPAPOS, _SPBPOS)
> > +#define SPSIZE(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPASIZE, _SPBSIZE)
> > +#define SPKEYMINVAL(pipe, plane)	_MMIO_VLV_SPR((pipe),
> > (plane), _SPAKEYMINVAL, _SPBKEYMINVAL)
> > +#define SPKEYMSK(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPAKEYMSK, _SPBKEYMSK)
> > +#define SPSURF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPASURF, _SPBSURF)
> > +#define SPKEYMAXVAL(pipe, plane)	_MMIO_VLV_SPR((pipe),
> > (plane), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
> > +#define SPTILEOFF(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPATILEOFF, _SPBTILEOFF)
> > +#define SPCONSTALPHA(pipe, plane)	_MMIO_VLV_SPR((pipe),
> > (plane), _SPACONSTALPHA, _SPBCONSTALPHA)
> > +#define SPGAMC(pipe, plane)		_MMIO_VLV_SPR((pipe),
> > (plane), _SPAGAMC, _SPBGAMC)
> >  
> >  /*
> >   * CHV pipe B sprite CSC
> > @@ -5394,29 +5397,32 @@ enum {
> >   * |yg| = |c3 c4 c5| x |yg + yg_ioff| + |yg_ooff|
> >   * |cb|   |c6 c7 c8|   |cb + cr_ioff|   |cb_ooff|
> >   */
> > -#define SPCSCYGOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d900 +
> > (sprite) * 0x1000)
> > -#define SPCSCCBOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d904 +
> > (sprite) * 0x1000)
> > -#define SPCSCCROFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d908 +
> > (sprite) * 0x1000)
> > +#define _MMIO_CHV_SPCSC(plane, reg) \
> > +	_MMIO(VLV_DISPLAY_BASE + ((plane) - PLANE_SPRITE0) * 0x1000
> > + (reg))
> > +
> > +#define SPCSCYGOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d900)
> > +#define SPCSCCBOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d904)
> > +#define SPCSCCROFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d908)
> >  #define  SPCSC_OOFF(x)		(((x) & 0x7ff) << 16) /* s11
> > */
> >  #define  SPCSC_IOFF(x)		(((x) & 0x7ff) << 0) /* s11 */
> >  
> > -#define SPCSCC01(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d90c +
> > (sprite) * 0x1000)
> > -#define SPCSCC23(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d910 +
> > (sprite) * 0x1000)
> > -#define SPCSCC45(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d914 +
> > (sprite) * 0x1000)
> > -#define SPCSCC67(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d918 +
> > (sprite) * 0x1000)
> > -#define SPCSCC8(sprite)		_MMIO(VLV_DISPLAY_BASE +
> > 0x6d91c + (sprite) * 0x1000)
> > +#define SPCSCC01(plane)		_MMIO_CHV_SPCSC(plane,
> > 0x6d90c)
> > +#define SPCSCC23(plane)		_MMIO_CHV_SPCSC(plane,
> > 0x6d910)
> > +#define SPCSCC45(plane)		_MMIO_CHV_SPCSC(plane,
> > 0x6d914)
> > +#define SPCSCC67(plane)		_MMIO_CHV_SPCSC(plane,
> > 0x6d918)
> > +#define SPCSCC8(plane)		_MMIO_CHV_SPCSC(plane,
> > 0x6d91c)
> 
> Why is 1 "01" while 8 is "8"? :)

01 means "0 and 1" whereas 8 is just "8"

> 
> With or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 
> >  #define  SPCSC_C1(x)		(((x) & 0x7fff) << 16) /* s3.12
> > */
> >  #define  SPCSC_C0(x)		(((x) & 0x7fff) << 0) /* s3.12
> > */
> >  
> > -#define SPCSCYGICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d920 + (sprite) * 0x1000)
> > -#define SPCSCCBICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d924 + (sprite) * 0x1000)
> > -#define SPCSCCRICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d928 + (sprite) * 0x1000)
> > +#define SPCSCYGICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d920)
> > +#define SPCSCCBICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d924)
> > +#define SPCSCCRICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d928)
> >  #define  SPCSC_IMAX(x)		(((x) & 0x7ff) << 16) /* s11
> > */
> >  #define  SPCSC_IMIN(x)		(((x) & 0x7ff) << 0) /* s11 */
> >  
> > -#define SPCSCYGOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d92c + (sprite) * 0x1000)
> > -#define SPCSCCBOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d930 + (sprite) * 0x1000)
> > -#define SPCSCCROCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE +
> > 0x6d934 + (sprite) * 0x1000)
> > +#define SPCSCYGOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d92c)
> > +#define SPCSCCBOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d930)
> > +#define SPCSCCROCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d934)
> >  #define  SPCSC_OMAX(x)		((x) << 16) /* u10 */
> >  #define  SPCSC_OMIN(x)		((x) << 0) /* u10 */
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 95644c8cc568..a11c0f1d3f2e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1331,7 +1331,7 @@ static void assert_sprites_disabled(struct
> > drm_i915_private *dev_priv,
> >  		}
> >  	} else if (IS_VALLEYVIEW(dev_priv) ||
> > IS_CHERRYVIEW(dev_priv)) {
> >  		for_each_sprite(dev_priv, pipe, sprite) {
> > -			u32 val = I915_READ(SPCNTR(pipe, sprite));
> > +			u32 val = I915_READ(SPCNTR(pipe,
> > PLANE_SPRITE0 + sprite));
> >  			I915_STATE_WARN(val & SP_ENABLE,
> >  			     "sprite %c assertion failure, should be
> > off on pipe %c but is still active\n",
> >  			     sprite_name(pipe, sprite),
> > pipe_name(pipe));
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index 91d47d19f4a9..a922c6382fc7 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -314,7 +314,7 @@ static void
> >  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(intel_plane-
> > >base.dev);
> > -	int plane = intel_plane->plane;
> > +	enum plane_id plane_id = intel_plane->id;
> >  
> >  	/* Seems RGB data bypasses the CSC always */
> >  	if (!format_is_yuv(format))
> > @@ -330,23 +330,23 @@ chv_update_csc(struct intel_plane *intel_plane,
> > uint32_t format)
> >  	 * Cb and Cr apparently come in as signed already, so no
> >  	 * need for any offset. For Y we need to remove the offset.
> >  	 */
> > -	I915_WRITE(SPCSCYGOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(-
> > 64));
> > -	I915_WRITE(SPCSCCBOFF(plane), SPCSC_OOFF(0) |
> > SPCSC_IOFF(0));
> > -	I915_WRITE(SPCSCCROFF(plane), SPCSC_OOFF(0) |
> > SPCSC_IOFF(0));
> > -
> > -	I915_WRITE(SPCSCC01(plane), SPCSC_C1(4769) |
> > SPCSC_C0(6537));
> > -	I915_WRITE(SPCSCC23(plane), SPCSC_C1(-3330) | SPCSC_C0(0));
> > -	I915_WRITE(SPCSCC45(plane), SPCSC_C1(-1605) |
> > SPCSC_C0(4769));
> > -	I915_WRITE(SPCSCC67(plane), SPCSC_C1(4769) | SPCSC_C0(0));
> > -	I915_WRITE(SPCSCC8(plane), SPCSC_C0(8263));
> > -
> > -	I915_WRITE(SPCSCYGICLAMP(plane), SPCSC_IMAX(940) |
> > SPCSC_IMIN(64));
> > -	I915_WRITE(SPCSCCBICLAMP(plane), SPCSC_IMAX(448) |
> > SPCSC_IMIN(-448));
> > -	I915_WRITE(SPCSCCRICLAMP(plane), SPCSC_IMAX(448) |
> > SPCSC_IMIN(-448));
> > -
> > -	I915_WRITE(SPCSCYGOCLAMP(plane), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> > -	I915_WRITE(SPCSCCBOCLAMP(plane), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> > -	I915_WRITE(SPCSCCROCLAMP(plane), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> > +	I915_WRITE(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) |
> > SPCSC_IOFF(-64));
> > +	I915_WRITE(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) |
> > SPCSC_IOFF(0));
> > +	I915_WRITE(SPCSCCROFF(plane_id), SPCSC_OOFF(0) |
> > SPCSC_IOFF(0));
> > +
> > +	I915_WRITE(SPCSCC01(plane_id), SPCSC_C1(4769) |
> > SPCSC_C0(6537));
> > +	I915_WRITE(SPCSCC23(plane_id), SPCSC_C1(-3330) |
> > SPCSC_C0(0));
> > +	I915_WRITE(SPCSCC45(plane_id), SPCSC_C1(-1605) |
> > SPCSC_C0(4769));
> > +	I915_WRITE(SPCSCC67(plane_id), SPCSC_C1(4769) |
> > SPCSC_C0(0));
> > +	I915_WRITE(SPCSCC8(plane_id), SPCSC_C0(8263));
> > +
> > +	I915_WRITE(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) |
> > SPCSC_IMIN(64));
> > +	I915_WRITE(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) |
> > SPCSC_IMIN(-448));
> > +	I915_WRITE(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) |
> > SPCSC_IMIN(-448));
> > +
> > +	I915_WRITE(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> > +	I915_WRITE(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> > +	I915_WRITE(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) |
> > SPCSC_OMIN(0));
> >  }
> >  
> >  static void
> > @@ -358,8 +358,8 @@ vlv_update_plane(struct drm_plane *dplane,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> >  	struct drm_framebuffer *fb = plane_state->base.fb;
> > -	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	enum pipe pipe = intel_plane->pipe;
> > +	enum plane_id plane_id = intel_plane->id;
> >  	u32 sprctl;
> >  	u32 sprsurf_offset, linear_offset;
> >  	unsigned int rotation = plane_state->base.rotation;
> > @@ -446,9 +446,9 @@ vlv_update_plane(struct drm_plane *dplane,
> >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> >  
> >  	if (key->flags) {
> > -		I915_WRITE(SPKEYMINVAL(pipe, plane), key-
> > >min_value);
> > -		I915_WRITE(SPKEYMAXVAL(pipe, plane), key-
> > >max_value);
> > -		I915_WRITE(SPKEYMSK(pipe, plane), key-
> > >channel_mask);
> > +		I915_WRITE(SPKEYMINVAL(pipe, plane_id), key-
> > >min_value);
> > +		I915_WRITE(SPKEYMAXVAL(pipe, plane_id), key-
> > >max_value);
> > +		I915_WRITE(SPKEYMSK(pipe, plane_id), key-
> > >channel_mask);
> >  	}
> >  
> >  	if (key->flags & I915_SET_COLORKEY_SOURCE)
> > @@ -457,21 +457,21 @@ vlv_update_plane(struct drm_plane *dplane,
> >  	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> >  		chv_update_csc(intel_plane, fb->pixel_format);
> >  
> > -	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
> > -	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
> > +	I915_WRITE(SPSTRIDE(pipe, plane_id), fb->pitches[0]);
> > +	I915_WRITE(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
> >  
> >  	if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
> > -		I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
> > +		I915_WRITE(SPTILEOFF(pipe, plane_id), (y << 16) |
> > x);
> >  	else
> > -		I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
> > +		I915_WRITE(SPLINOFF(pipe, plane_id), linear_offset);
> >  
> > -	I915_WRITE(SPCONSTALPHA(pipe, plane), 0);
> > +	I915_WRITE(SPCONSTALPHA(pipe, plane_id), 0);
> >  
> > -	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
> > -	I915_WRITE(SPCNTR(pipe, plane), sprctl);
> > -	I915_WRITE(SPSURF(pipe, plane),
> > +	I915_WRITE(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
> > +	I915_WRITE(SPCNTR(pipe, plane_id), sprctl);
> > +	I915_WRITE(SPSURF(pipe, plane_id),
> >  		   intel_fb_gtt_offset(fb, rotation) +
> > sprsurf_offset);
> > -	POSTING_READ(SPSURF(pipe, plane));
> > +	POSTING_READ(SPSURF(pipe, plane_id));
> >  }
> >  
> >  static void
> > @@ -480,13 +480,13 @@ vlv_disable_plane(struct drm_plane *dplane,
> > struct drm_crtc *crtc)
> >  	struct drm_device *dev = dplane->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_plane *intel_plane = to_intel_plane(dplane);
> > -	int pipe = intel_plane->pipe;
> > -	int plane = intel_plane->plane;
> > +	enum pipe pipe = intel_plane->pipe;
> > +	enum plane_id plane_id = intel_plane->id;
> >  
> > -	I915_WRITE(SPCNTR(pipe, plane), 0);
> > +	I915_WRITE(SPCNTR(pipe, plane_id), 0);
> >  
> > -	I915_WRITE(SPSURF(pipe, plane), 0);
> > -	POSTING_READ(SPSURF(pipe, plane));
> > +	I915_WRITE(SPSURF(pipe, plane_id), 0);
> > +	POSTING_READ(SPSURF(pipe, plane_id));
> >  }
> >  
> >  static void

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3361d7ffc63e..9739e97c6263 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5374,18 +5374,21 @@  enum {
 #define _SPBCONSTALPHA		(VLV_DISPLAY_BASE + 0x722a8)
 #define _SPBGAMC		(VLV_DISPLAY_BASE + 0x722f4)
 
-#define SPCNTR(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPACNTR, _SPBCNTR)
-#define SPLINOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPALINOFF, _SPBLINOFF)
-#define SPSTRIDE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASTRIDE, _SPBSTRIDE)
-#define SPPOS(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAPOS, _SPBPOS)
-#define SPSIZE(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASIZE, _SPBSIZE)
-#define SPKEYMINVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMINVAL, _SPBKEYMINVAL)
-#define SPKEYMSK(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMSK, _SPBKEYMSK)
-#define SPSURF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPASURF, _SPBSURF)
-#define SPKEYMAXVAL(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
-#define SPTILEOFF(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPATILEOFF, _SPBTILEOFF)
-#define SPCONSTALPHA(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPACONSTALPHA, _SPBCONSTALPHA)
-#define SPGAMC(pipe, plane) _MMIO_PIPE((pipe) * 2 + (plane), _SPAGAMC, _SPBGAMC)
+#define _MMIO_VLV_SPR(pipe, plane, reg_a, reg_b) \
+	_MMIO_PIPE((pipe) * 2 + (plane) - PLANE_SPRITE0, (reg_a), (reg_b))
+
+#define SPCNTR(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPACNTR, _SPBCNTR)
+#define SPLINOFF(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPALINOFF, _SPBLINOFF)
+#define SPSTRIDE(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPASTRIDE, _SPBSTRIDE)
+#define SPPOS(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPAPOS, _SPBPOS)
+#define SPSIZE(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPASIZE, _SPBSIZE)
+#define SPKEYMINVAL(pipe, plane)	_MMIO_VLV_SPR((pipe), (plane), _SPAKEYMINVAL, _SPBKEYMINVAL)
+#define SPKEYMSK(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPAKEYMSK, _SPBKEYMSK)
+#define SPSURF(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPASURF, _SPBSURF)
+#define SPKEYMAXVAL(pipe, plane)	_MMIO_VLV_SPR((pipe), (plane), _SPAKEYMAXVAL, _SPBKEYMAXVAL)
+#define SPTILEOFF(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPATILEOFF, _SPBTILEOFF)
+#define SPCONSTALPHA(pipe, plane)	_MMIO_VLV_SPR((pipe), (plane), _SPACONSTALPHA, _SPBCONSTALPHA)
+#define SPGAMC(pipe, plane)		_MMIO_VLV_SPR((pipe), (plane), _SPAGAMC, _SPBGAMC)
 
 /*
  * CHV pipe B sprite CSC
@@ -5394,29 +5397,32 @@  enum {
  * |yg| = |c3 c4 c5| x |yg + yg_ioff| + |yg_ooff|
  * |cb|   |c6 c7 c8|   |cb + cr_ioff|   |cb_ooff|
  */
-#define SPCSCYGOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d900 + (sprite) * 0x1000)
-#define SPCSCCBOFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d904 + (sprite) * 0x1000)
-#define SPCSCCROFF(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d908 + (sprite) * 0x1000)
+#define _MMIO_CHV_SPCSC(plane, reg) \
+	_MMIO(VLV_DISPLAY_BASE + ((plane) - PLANE_SPRITE0) * 0x1000 + (reg))
+
+#define SPCSCYGOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d900)
+#define SPCSCCBOFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d904)
+#define SPCSCCROFF(plane)	_MMIO_CHV_SPCSC(plane, 0x6d908)
 #define  SPCSC_OOFF(x)		(((x) & 0x7ff) << 16) /* s11 */
 #define  SPCSC_IOFF(x)		(((x) & 0x7ff) << 0) /* s11 */
 
-#define SPCSCC01(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d90c + (sprite) * 0x1000)
-#define SPCSCC23(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d910 + (sprite) * 0x1000)
-#define SPCSCC45(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d914 + (sprite) * 0x1000)
-#define SPCSCC67(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d918 + (sprite) * 0x1000)
-#define SPCSCC8(sprite)		_MMIO(VLV_DISPLAY_BASE + 0x6d91c + (sprite) * 0x1000)
+#define SPCSCC01(plane)		_MMIO_CHV_SPCSC(plane, 0x6d90c)
+#define SPCSCC23(plane)		_MMIO_CHV_SPCSC(plane, 0x6d910)
+#define SPCSCC45(plane)		_MMIO_CHV_SPCSC(plane, 0x6d914)
+#define SPCSCC67(plane)		_MMIO_CHV_SPCSC(plane, 0x6d918)
+#define SPCSCC8(plane)		_MMIO_CHV_SPCSC(plane, 0x6d91c)
 #define  SPCSC_C1(x)		(((x) & 0x7fff) << 16) /* s3.12 */
 #define  SPCSC_C0(x)		(((x) & 0x7fff) << 0) /* s3.12 */
 
-#define SPCSCYGICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d920 + (sprite) * 0x1000)
-#define SPCSCCBICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d924 + (sprite) * 0x1000)
-#define SPCSCCRICLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d928 + (sprite) * 0x1000)
+#define SPCSCYGICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d920)
+#define SPCSCCBICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d924)
+#define SPCSCCRICLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d928)
 #define  SPCSC_IMAX(x)		(((x) & 0x7ff) << 16) /* s11 */
 #define  SPCSC_IMIN(x)		(((x) & 0x7ff) << 0) /* s11 */
 
-#define SPCSCYGOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d92c + (sprite) * 0x1000)
-#define SPCSCCBOCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d930 + (sprite) * 0x1000)
-#define SPCSCCROCLAMP(sprite)	_MMIO(VLV_DISPLAY_BASE + 0x6d934 + (sprite) * 0x1000)
+#define SPCSCYGOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d92c)
+#define SPCSCCBOCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d930)
+#define SPCSCCROCLAMP(plane)	_MMIO_CHV_SPCSC(plane, 0x6d934)
 #define  SPCSC_OMAX(x)		((x) << 16) /* u10 */
 #define  SPCSC_OMIN(x)		((x) << 0) /* u10 */
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 95644c8cc568..a11c0f1d3f2e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1331,7 +1331,7 @@  static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 		}
 	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
 		for_each_sprite(dev_priv, pipe, sprite) {
-			u32 val = I915_READ(SPCNTR(pipe, sprite));
+			u32 val = I915_READ(SPCNTR(pipe, PLANE_SPRITE0 + sprite));
 			I915_STATE_WARN(val & SP_ENABLE,
 			     "sprite %c assertion failure, should be off on pipe %c but is still active\n",
 			     sprite_name(pipe, sprite), pipe_name(pipe));
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 91d47d19f4a9..a922c6382fc7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -314,7 +314,7 @@  static void
 chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_plane->base.dev);
-	int plane = intel_plane->plane;
+	enum plane_id plane_id = intel_plane->id;
 
 	/* Seems RGB data bypasses the CSC always */
 	if (!format_is_yuv(format))
@@ -330,23 +330,23 @@  chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 	 * Cb and Cr apparently come in as signed already, so no
 	 * need for any offset. For Y we need to remove the offset.
 	 */
-	I915_WRITE(SPCSCYGOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
-	I915_WRITE(SPCSCCBOFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
-	I915_WRITE(SPCSCCROFF(plane), SPCSC_OOFF(0) | SPCSC_IOFF(0));
-
-	I915_WRITE(SPCSCC01(plane), SPCSC_C1(4769) | SPCSC_C0(6537));
-	I915_WRITE(SPCSCC23(plane), SPCSC_C1(-3330) | SPCSC_C0(0));
-	I915_WRITE(SPCSCC45(plane), SPCSC_C1(-1605) | SPCSC_C0(4769));
-	I915_WRITE(SPCSCC67(plane), SPCSC_C1(4769) | SPCSC_C0(0));
-	I915_WRITE(SPCSCC8(plane), SPCSC_C0(8263));
-
-	I915_WRITE(SPCSCYGICLAMP(plane), SPCSC_IMAX(940) | SPCSC_IMIN(64));
-	I915_WRITE(SPCSCCBICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-	I915_WRITE(SPCSCCRICLAMP(plane), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
-
-	I915_WRITE(SPCSCYGOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
-	I915_WRITE(SPCSCCBOCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
-	I915_WRITE(SPCSCCROCLAMP(plane), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCYGOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(-64));
+	I915_WRITE(SPCSCCBOFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
+	I915_WRITE(SPCSCCROFF(plane_id), SPCSC_OOFF(0) | SPCSC_IOFF(0));
+
+	I915_WRITE(SPCSCC01(plane_id), SPCSC_C1(4769) | SPCSC_C0(6537));
+	I915_WRITE(SPCSCC23(plane_id), SPCSC_C1(-3330) | SPCSC_C0(0));
+	I915_WRITE(SPCSCC45(plane_id), SPCSC_C1(-1605) | SPCSC_C0(4769));
+	I915_WRITE(SPCSCC67(plane_id), SPCSC_C1(4769) | SPCSC_C0(0));
+	I915_WRITE(SPCSCC8(plane_id), SPCSC_C0(8263));
+
+	I915_WRITE(SPCSCYGICLAMP(plane_id), SPCSC_IMAX(940) | SPCSC_IMIN(64));
+	I915_WRITE(SPCSCCBICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+	I915_WRITE(SPCSCCRICLAMP(plane_id), SPCSC_IMAX(448) | SPCSC_IMIN(-448));
+
+	I915_WRITE(SPCSCYGOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCCBOCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
+	I915_WRITE(SPCSCCROCLAMP(plane_id), SPCSC_OMAX(1023) | SPCSC_OMIN(0));
 }
 
 static void
@@ -358,8 +358,8 @@  vlv_update_plane(struct drm_plane *dplane,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	struct drm_framebuffer *fb = plane_state->base.fb;
-	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	enum pipe pipe = intel_plane->pipe;
+	enum plane_id plane_id = intel_plane->id;
 	u32 sprctl;
 	u32 sprsurf_offset, linear_offset;
 	unsigned int rotation = plane_state->base.rotation;
@@ -446,9 +446,9 @@  vlv_update_plane(struct drm_plane *dplane,
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
 	if (key->flags) {
-		I915_WRITE(SPKEYMINVAL(pipe, plane), key->min_value);
-		I915_WRITE(SPKEYMAXVAL(pipe, plane), key->max_value);
-		I915_WRITE(SPKEYMSK(pipe, plane), key->channel_mask);
+		I915_WRITE(SPKEYMINVAL(pipe, plane_id), key->min_value);
+		I915_WRITE(SPKEYMAXVAL(pipe, plane_id), key->max_value);
+		I915_WRITE(SPKEYMSK(pipe, plane_id), key->channel_mask);
 	}
 
 	if (key->flags & I915_SET_COLORKEY_SOURCE)
@@ -457,21 +457,21 @@  vlv_update_plane(struct drm_plane *dplane,
 	if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
 		chv_update_csc(intel_plane, fb->pixel_format);
 
-	I915_WRITE(SPSTRIDE(pipe, plane), fb->pitches[0]);
-	I915_WRITE(SPPOS(pipe, plane), (crtc_y << 16) | crtc_x);
+	I915_WRITE(SPSTRIDE(pipe, plane_id), fb->pitches[0]);
+	I915_WRITE(SPPOS(pipe, plane_id), (crtc_y << 16) | crtc_x);
 
 	if (fb->modifier[0] == I915_FORMAT_MOD_X_TILED)
-		I915_WRITE(SPTILEOFF(pipe, plane), (y << 16) | x);
+		I915_WRITE(SPTILEOFF(pipe, plane_id), (y << 16) | x);
 	else
-		I915_WRITE(SPLINOFF(pipe, plane), linear_offset);
+		I915_WRITE(SPLINOFF(pipe, plane_id), linear_offset);
 
-	I915_WRITE(SPCONSTALPHA(pipe, plane), 0);
+	I915_WRITE(SPCONSTALPHA(pipe, plane_id), 0);
 
-	I915_WRITE(SPSIZE(pipe, plane), (crtc_h << 16) | crtc_w);
-	I915_WRITE(SPCNTR(pipe, plane), sprctl);
-	I915_WRITE(SPSURF(pipe, plane),
+	I915_WRITE(SPSIZE(pipe, plane_id), (crtc_h << 16) | crtc_w);
+	I915_WRITE(SPCNTR(pipe, plane_id), sprctl);
+	I915_WRITE(SPSURF(pipe, plane_id),
 		   intel_fb_gtt_offset(fb, rotation) + sprsurf_offset);
-	POSTING_READ(SPSURF(pipe, plane));
+	POSTING_READ(SPSURF(pipe, plane_id));
 }
 
 static void
@@ -480,13 +480,13 @@  vlv_disable_plane(struct drm_plane *dplane, struct drm_crtc *crtc)
 	struct drm_device *dev = dplane->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
-	int pipe = intel_plane->pipe;
-	int plane = intel_plane->plane;
+	enum pipe pipe = intel_plane->pipe;
+	enum plane_id plane_id = intel_plane->id;
 
-	I915_WRITE(SPCNTR(pipe, plane), 0);
+	I915_WRITE(SPCNTR(pipe, plane_id), 0);
 
-	I915_WRITE(SPSURF(pipe, plane), 0);
-	POSTING_READ(SPSURF(pipe, plane));
+	I915_WRITE(SPSURF(pipe, plane_id), 0);
+	POSTING_READ(SPSURF(pipe, plane_id));
 }
 
 static void

[v2,6/9] drm/i915: Use enum plane_id in VLV/CHV sprite code

Commit Message

Comments

Patch