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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

Hi Uma,

Thanks for the comments it made me have a look at the things again, 
especially for DSI case.
For Dithering with 12bpc, the bspec doesn’t say anything. In 
intel_display.c we are enabling dithering only for 6bpc panels. However, 
we do write the BPC in Pipe MISC based on pipe_bpp, even if we enable 
dithering or not.
This helps in case of populating crtc_state->pipe_bpp during readout.
For DSI the trans ctl ddi register does not have a BPC field, and so 
readout for crtc_state->bipe_ppp we have to rely on PIPE MISC discussed 
by Ville in [1].
Now that the PIPE_MISC's dithering bpc bits also represent port output 
bpc, it has added to the confusion.

So a simple solution would be to just rename the bits simply as 
PIPE_MISC_#BPC and correct the value of 12 BPC macro which was 
introduced in [2].
So even for older platforms, we keep on writing 12 bpc in PIPE_MISC. 
Since we do not enable dithering, these bits would not mean anything for 
dithering, but continue to serve for reading out of crtc_state->pipe_bpp 
as before.

For newer platform we'll fill PIPE_MISC port output bpc information 
similar to TRANS_DDI_FUNC_CTL bits per color field for all BPC.

[1] 
https://cgit.freedesktop.org/drm-tip/commit/?id=e1b7058ece718c0350ad2e5bfbdab17885bd9f39
[2] 
https://cgit.freedesktop.org/drm-tip/commit/?id=756f85cffef2bc84aa85b25031c1c97721928bd2

Please find my response inline:


On 7/29/2021 1:31 PM, Shankar, Uma wrote:
>
>> -----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.


Thanks, I will take care of this in next version of the patch.

>> 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.

Agreed will add the fixes tag for the 12 bpc correction with commit 
mentioned in the link [2] above.

But I think, removing the 12 BPC bit in one patch and reintroducing the 
correct 12 bpc value might break something in DSI for first patch, 
because although the value written in pipe misc is incorrect we still 
use it and translate to correct bpp.

A single patch with correction and the meaning for 12 bpc and putting in 
place why this is required for older platform, as mentioned above should 
be sufficient.


>
>
>> 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.

As mentioned above, bspec doesnt say anything abut dithering with 12 bpc 
and also we enable dithering for 6bpc panels only.


>
>>   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.

You are right, as mentioned above, its better to keep using the pipe 
misc bpp bits for 12 bpc as well to avoid breaking DSI.

Since, we do not enable dithering in such case, it shouldn't be a problem.

I will remove the platform check from here and simply return 36.

Regards,

Ankit

>>   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_TRUNCREG_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