Re: [Intel-gfx] [PATCH] drm/i915: Fix the 12 BPC bits for PIPE_MISC reg

["Shankar, Uma" <uma.shankar@intel.com>]

> -----Original Message-----
> From: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>
> Sent: Monday, July 19, 2021 9:55 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma <uma.shankar@intel.com>; Nikula, Jani <jani.nikula@intel.com>;
> ville.syrjala@linux.intel.com
> Subject: [PATCH] drm/i915: Fix the 12 BPC bits for PIPE_MISC reg

Append display in header.
> 
> From: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> Till DISPLAY12 the PIPE_MISC bits 5-7 are used to set the Dithering BPC, with valid
> values of 6, 8, 10 BPC.
> For ADLP+ these bits are used to set the PORT OUTPUT BPC, with valid values of: 6,
> 8, 10, 12 BPC, and need to be programmed whether dithering is enabled or not.
> 
> This patch:
> -corrects the bits 5-7 for PIPE MISC register for 12 BPC.
> -renames the bits and mask to have generic names for these bits for dithering bpc
> and port output bpc.
> 

I guess we have 2 issues here. One is wrong definition of 12bpc for dither which seems not
even there in hw before display 12 platforms.  Other one is the port output bpc fix.
Would suggest to split this patch in 2 addressing these issues separately. Send 1st patch as
Fixme (with commit it is fixing), and other as normal feature addition.


> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 23 +++++++++++++----------
>  drivers/gpu/drm/i915/i915_reg.h              | 16 +++++++++++-----
>  2 files changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 65ddb6c..dc4869f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5760,16 +5760,18 @@ static void bdw_set_pipemisc(const struct
> intel_crtc_state *crtc_state)
> 
>  	switch (crtc_state->pipe_bpp) {
>  	case 18:
> -		val |= PIPEMISC_DITHER_6_BPC;
> +		val |= PIPEMISC_6_BPC;
>  		break;
>  	case 24:
> -		val |= PIPEMISC_DITHER_8_BPC;
> +		val |= PIPEMISC_8_BPC;
>  		break;
>  	case 30:
> -		val |= PIPEMISC_DITHER_10_BPC;
> +		val |= PIPEMISC_10_BPC;
>  		break;
>  	case 36:
> -		val |= PIPEMISC_DITHER_12_BPC;
> +		/* Port output 12BPC defined for ADLP+ */
> +		if (DISPLAY_VER(dev_priv) > 12)
> +			val |= PIPEMISC_12_BPC_ADLP;

We have 12 bpc in TRANS_DDI_FUNC_CTRL for BDW+, so what happens to dithering
if we have 12bpc enabled. We should confirm this.

>  		break;
>  	default:
>  		MISSING_CASE(crtc_state->pipe_bpp);
> @@ -5822,15 +5824,16 @@ int bdw_get_pipemisc_bpp(struct intel_crtc *crtc)
> 
>  	tmp = intel_de_read(dev_priv, PIPEMISC(crtc->pipe));
> 
> -	switch (tmp & PIPEMISC_DITHER_BPC_MASK) {
> -	case PIPEMISC_DITHER_6_BPC:
> +	switch (tmp & PIPEMISC_BPC_MASK) {
> +	case PIPEMISC_6_BPC:
>  		return 18;
> -	case PIPEMISC_DITHER_8_BPC:
> +	case PIPEMISC_8_BPC:
>  		return 24;
> -	case PIPEMISC_DITHER_10_BPC:
> +	case PIPEMISC_10_BPC:
>  		return 30;
> -	case PIPEMISC_DITHER_12_BPC:
> -		return 36;
> +	/* PORT OUTPUT 12 BPC defined for ADLP+ */
> +	case PIPEMISC_12_BPC_ADLP:
> +		return DISPLAY_VER(dev_priv) > 12 ? 36 : 0;

Returning 0 seems odd, as this will give bpp as 0 which is not right.
We should throw a WARN since getting 12bpc set in pipe_misc is not expected (as not supported)
on pre gen12.

To me there is some discrepancy here, as transcoder output supports 12bpc so dithering
also should have that. If not we have a issue with DSI since it tries to get pipe_bpp from
bdw_get_pipemisc. Please check this out once.

>  	default:
>  		MISSING_CASE(tmp);
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 943fe48..963d87d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6166,11 +6166,17 @@ enum {
>  #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_DITHER_BPC_MASK	(7 << 5)
> -#define   PIPEMISC_DITHER_8_BPC		(0 << 5)
> -#define   PIPEMISC_DITHER_10_BPC	(1 << 5)
> -#define   PIPEMISC_DITHER_6_BPC		(2 << 5)
> -#define   PIPEMISC_DITHER_12_BPC	(3 << 5)
> +/*
> + * 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)
> --
> 2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx