Re: [Intel-gfx] [PATCH 2/9] drm/i915: Clean up PIPEMISC register defines

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 15, 2021 at 02:12:47PM -0500, Rodrigo Vivi wrote:
> On Fri, Nov 12, 2021 at 09:38:06PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use REG_BIT() & co. for PIPEMISC* bits, and while at it
> > fill in the missing dithering bits since we already had some
> > of them defined.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 18 +++++-----
> >  drivers/gpu/drm/i915/i915_reg.h              | 35 +++++++++++---------
> >  2 files changed, 28 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 6073f94632ab..e293241450b1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -3724,18 +3724,18 @@ static void bdw_set_pipemisc(const struct intel_crtc_state *crtc_state)
> >  
> >  	switch (crtc_state->pipe_bpp) {
> >  	case 18:
> > -		val |= PIPEMISC_6_BPC;
> > +		val |= PIPEMISC_BPC_6;
> >  		break;
> >  	case 24:
> > -		val |= PIPEMISC_8_BPC;
> > +		val |= PIPEMISC_BPC_8;
> >  		break;
> >  	case 30:
> > -		val |= PIPEMISC_10_BPC;
> > +		val |= PIPEMISC_BPC_10;
> >  		break;
> >  	case 36:
> >  		/* Port output 12BPC defined for ADLP+ */
> >  		if (DISPLAY_VER(dev_priv) > 12)
> > -			val |= PIPEMISC_12_BPC_ADLP;
> > +			val |= PIPEMISC_BPC_12_ADLP;
> 
> while on it, I wonder if we could remove this "ADLP" suffix.
> 
> First because prefix seems to be the most used case for the platform.

I don't like prefix for such things since it screws up the namespace for
the bits of the same register. So suffix is better IMO.

> But also because there's a clear if here and a clear comment in the definition.

Yeah, I've been removing some similar things in other places for that exact
reason. The only case where we might really need platform designators is
when the bit(s) move etc.

Though I was even pondering removing the 'if' since this only controls
dithering on older platforms and we don't enable dithering for anything
but 6bpc output. Although the whole pipe_bpp concept is kinda wrong and
we should perhaps split it up into pipe_bpp vs. port_bpp, or something 
along those lines.

> (Btw, I'd prefer the ver >= 13 than > 12 to be really clear :/)

Yeah, >= is what we should use.

> 
> >  		break;
> >  	default:
> >  		MISSING_CASE(crtc_state->pipe_bpp);
> > @@ -3771,7 +3771,7 @@ static void bdw_set_pipemisc(const struct intel_crtc_state *crtc_state)
> >  		}
> >  
> >  		intel_de_rmw(dev_priv, PIPE_MISC2(crtc->pipe),
> > -			     PIPE_MISC2_UNDERRUN_BUBBLE_COUNTER_MASK,
> > +			     PIPE_MISC2_BUBBLE_COUNTER_MASK,
> >  			     scaler_in_use ? PIPE_MISC2_BUBBLE_COUNTER_SCALER_EN :
> >  			     PIPE_MISC2_BUBBLE_COUNTER_SCALER_DIS);
> >  	}
> > @@ -3787,11 +3787,11 @@ int bdw_get_pipemisc_bpp(struct intel_crtc *crtc)
> >  	tmp = intel_de_read(dev_priv, PIPEMISC(crtc->pipe));
> >  
> >  	switch (tmp & PIPEMISC_BPC_MASK) {
> > -	case PIPEMISC_6_BPC:
> > +	case PIPEMISC_BPC_6:
> >  		return 18;
> > -	case PIPEMISC_8_BPC:
> > +	case PIPEMISC_BPC_8:
> >  		return 24;
> > -	case PIPEMISC_10_BPC:
> > +	case PIPEMISC_BPC_10:
> >  		return 30;
> >  	/*
> >  	 * PORT OUTPUT 12 BPC defined for ADLP+.
> > @@ -3803,7 +3803,7 @@ int bdw_get_pipemisc_bpp(struct intel_crtc *crtc)
> >  	 * on older platforms, need to find a workaround for 12 BPC
> >  	 * MIPI DSI HW readout.
> >  	 */
> > -	case PIPEMISC_12_BPC_ADLP:
> > +	case PIPEMISC_BPC_12_ADLP:
> >  		if (DISPLAY_VER(dev_priv) > 12)
> >  			return 36;
> >  		fallthrough;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index f5d54ed2efc1..e300a202ce2d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6308,32 +6308,35 @@ enum {
> >  
> >  #define _PIPE_MISC_A			0x70030
> >  #define _PIPE_MISC_B			0x71030
> > -#define   PIPEMISC_YUV420_ENABLE	(1 << 27) /* glk+ */
> > -#define   PIPEMISC_YUV420_MODE_FULL_BLEND (1 << 26) /* glk+ */
> > -#define   PIPEMISC_HDR_MODE_PRECISION	(1 << 23) /* icl+ */
> > -#define   PIPEMISC_OUTPUT_COLORSPACE_YUV  (1 << 11)
> > -#define   PIPEMISC_PIXEL_ROUNDING_TRUNC	REG_BIT(8) /* tgl+ */
> > +#define   PIPEMISC_YUV420_ENABLE		REG_BIT(27) /* glk+ */
> > +#define   PIPEMISC_YUV420_MODE_FULL_BLEND	REG_BIT(26) /* glk+ */
> > +#define   PIPEMISC_HDR_MODE_PRECISION		REG_BIT(23) /* icl+ */
> > +#define   PIPEMISC_OUTPUT_COLORSPACE_YUV	REG_BIT(11)
> > +#define   PIPEMISC_PIXEL_ROUNDING_TRUNC		REG_BIT(8) /* tgl+ */
> >  /*
> >   * For Display < 13, Bits 5-7 of PIPE MISC represent DITHER BPC with
> >   * valid values of: 6, 8, 10 BPC.
> >   * ADLP+, the bits 5-7 represent PORT OUTPUT BPC with valid values of:
> >   * 6, 8, 10, 12 BPC.
> >   */
> > -#define   PIPEMISC_BPC_MASK		(7 << 5)
> > -#define   PIPEMISC_8_BPC		(0 << 5)
> > -#define   PIPEMISC_10_BPC		(1 << 5)
> > -#define   PIPEMISC_6_BPC		(2 << 5)
> > -#define   PIPEMISC_12_BPC_ADLP		(4 << 5) /* adlp+ */
> > -#define   PIPEMISC_DITHER_ENABLE	(1 << 4)
> > -#define   PIPEMISC_DITHER_TYPE_MASK	(3 << 2)
> > -#define   PIPEMISC_DITHER_TYPE_SP	(0 << 2)
> > +#define   PIPEMISC_BPC_MASK			REG_GENMASK(7, 5)
> > +#define   PIPEMISC_BPC_8			REG_FIELD_PREP(PIPEMISC_BPC_MASK, 0)
> > +#define   PIPEMISC_BPC_10			REG_FIELD_PREP(PIPEMISC_BPC_MASK, 1)
> > +#define   PIPEMISC_BPC_6			REG_FIELD_PREP(PIPEMISC_BPC_MASK, 2)
> > +#define   PIPEMISC_BPC_12_ADLP			REG_FIELD_PREP(PIPEMISC_BPC_MASK, 4) /* adlp+ */
> > +#define   PIPEMISC_DITHER_ENABLE		REG_BIT(4)
> > +#define   PIPEMISC_DITHER_TYPE_MASK		REG_GENMASK(3, 2)
> > +#define   PIPEMISC_DITHER_TYPE_SP		REG_FIELD_PREP(PIPEMISC_DITHER_TYPE_MASK, 0)
> > +#define   PIPEMISC_DITHER_TYPE_ST1		REG_FIELD_PREP(PIPEMISC_DITHER_TYPE_MASK, 1)
> > +#define   PIPEMISC_DITHER_TYPE_ST2		REG_FIELD_PREP(PIPEMISC_DITHER_TYPE_MASK, 2)
> > +#define   PIPEMISC_DITHER_TYPE_TEMP		REG_FIELD_PREP(PIPEMISC_DITHER_TYPE_MASK, 3)
> >  #define PIPEMISC(pipe)			_MMIO_PIPE2(pipe, _PIPE_MISC_A)
> >  
> >  #define _PIPE_MISC2_A					0x7002C
> >  #define _PIPE_MISC2_B					0x7102C
> > -#define   PIPE_MISC2_BUBBLE_COUNTER_SCALER_EN		(0x50 << 24)
> > -#define   PIPE_MISC2_BUBBLE_COUNTER_SCALER_DIS		(0x14 << 24)
> > -#define   PIPE_MISC2_UNDERRUN_BUBBLE_COUNTER_MASK	(0xff << 24)
> > +#define   PIPE_MISC2_BUBBLE_COUNTER_MASK	REG_GENMASK(31, 24)
> > +#define   PIPE_MISC2_BUBBLE_COUNTER_SCALER_EN	REG_FIELD_PREP(PIPE_MISC2_BUBBLE_COUNTER_MASK, 80)
> > +#define   PIPE_MISC2_BUBBLE_COUNTER_SCALER_DIS	REG_FIELD_PREP(PIPE_MISC2_BUBBLE_COUNTER_MASK, 20)
> >  #define PIPE_MISC2(pipe)					_MMIO_PIPE2(pipe, _PIPE_MISC2_A)
> >  
> >  /* Skylake+ pipe bottom (background) color */
> > -- 
> > 2.32.0
> > 

-- 
Ville Syrjälä
Intel