[Intel-gfx] [PATCH v2 6/9] drm/i915: Use enum plane_id in VLV/CHV sprite code
Paulo Zanoni
paulo.r.zanoni at intel.com
Thu Nov 17 20:07:36 UTC 2016
Em Ter, 2016-11-08 às 19:09 +0200, ville.syrjala at linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala at 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 at 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 at 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
More information about the Intel-gfx
mailing list