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@xxxxxxxxx>
Sent: Monday, July 19, 2021 9:55 AM
To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>;
ville.syrjala@xxxxxxxxxxxxxxx
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
