Em Ter, 2016-11-08 às 19:09 +0200, ville.syrj...@linux.intel.com escreveu: > From: Ville Syrjälä <ville.syrj...@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.syrj...@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.zan...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx