Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: Enable DATABUS_WIDEN for DSI command mode
From: Abhinav Kumar
Date: Thu Jun 22 2023 - 20:02:24 EST
On 6/14/2023 2:56 AM, Marijn Suijten wrote:
On 2023-06-13 18:57:13, Jessica Zhang wrote:
DSI 6G v2.5.x+ supports a data-bus widen mode that allows DSI to send
48 bits of compressed data per pclk instead of 24.
For all chipsets that support this mode, enable it whenever DSC is
enabled as recommend by the hardware programming guide.
Only enable this for command mode as we are currently unable to validate
it for video mode.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
Note: The dsi.xml.h changes were generated using the headergen2 script in
envytools [1], but the changes to the copyright and rules-ng-ng source file
paths were dropped.
[1] https://github.com/freedreno/envytools/
More interesting would be a link to the Mesa MR upstreaming this
bitfield to dsi.xml [2] (which I have not found on my own yet).
[2]: https://gitlab.freedesktop.org/mesa/mesa/-/blame/main/src/freedreno/registers/dsi/dsi.xml
Thats because we havent submitted a MR yet for this on mesa.
Generally, our team does not have legal permissions yet for mesa MRs
other than mesa drm because we got permissions for the modetest.
Rob/Dmitry, can one of you pls help with the corresponding mesa MR for this?
The xml file change was autogenerated so this patch can go in.
drivers/gpu/drm/msm/dsi/dsi.xml.h | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.xml.h b/drivers/gpu/drm/msm/dsi/dsi.xml.h
index a4a154601114..2a7d980e12c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.xml.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.xml.h
@@ -664,6 +664,7 @@ static inline uint32_t DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP(enum dsi_rgb_swap v
return ((val) << DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__SHIFT) & DSI_CMD_MODE_MDP_CTRL2_INPUT_RGB_SWAP__MASK;
}
#define DSI_CMD_MODE_MDP_CTRL2_BURST_MODE 0x00010000
+#define DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN 0x00100000
#define REG_DSI_CMD_MODE_MDP_STREAM2_CTRL 0x000001b8
#define DSI_CMD_MODE_MDP_STREAM2_CTRL_DATA_TYPE__MASK 0x0000003f
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 5d7b4409e4e9..1da5238e7105 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -927,6 +927,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
u32 hdisplay = mode->hdisplay;
u32 wc;
int ret;
+ bool widebus_supported = msm_host->cfg_hnd->major == MSM_DSI_VER_MAJOR_6G &&
+ msm_host->cfg_hnd->minor >= MSM_DSI_6G_VER_MINOR_V2_5_0;
+
DBG("");
@@ -973,8 +976,15 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
*
* hdisplay will be divided by 3 here to account for the fact
* that DPU sends 3 bytes per pclk cycle to DSI.
+ *
+ * If widebus is supported, set DATABUS_WIDEN register and divide hdisplay by 6
+ * instead of 3
So this should allow us to divide pclk by 2, or have much lower latency?
Otherwise it'll tick enough times to transmit the data twice.
Note that I brought up the exact same concerns when you used the 3:1
ratio from dsi_bpp / dsc_bpp in your pclk reduction patch (instad of the
number of bits/bytes that DPU sends to DSI per pclk), but no-one has
replied to my inquiry yet.
Ideally yes, we could have done pclk/2 on uncompressed pixels but we are
not going to add support for widebus on DSI without DSC as that is not
recommended in our docs.
So this cannot be done.
We tried our best to respond and explain to all your queries both on the
bug and the patch but i guess it just kept coming :)
I am going to try one more time to explain it in the documentation change.
*/
- hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
+ if (!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO) && widebus_supported)
+ hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 6);
+ else
+ hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
Nit: I wonder if this is more concise when written as:
u32 bytes_per_pclk;
...
if (!video && widebus)
bytes_per_pclk = 6;
else
bytes_per_pclk = 3;
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc),
bytes_per_pclk);
That is less duplication, **and** the value becomes self-documenting!
Sure, no concerns with making this change.
+
h_total += hdisplay;
ha_end = ha_start + hdisplay;
}
@@ -1027,6 +1037,13 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
dsi_write(msm_host, REG_DSI_CMD_MDP_STREAM0_TOTAL,
DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL(hdisplay) |
DSI_CMD_MDP_STREAM0_TOTAL_V_TOTAL(mode->vdisplay));
+
+ if (msm_host->dsc && widebus_supported) {
Can we also support widebus for uncompressed streams (sending 2 pixels
of bpp=24 per pclk), and if so, is that something you want to add in the
future (a comment would be nice)?
No, we cannot support widebus on uncompressed streams on DSI so we wont
be adding that.
+ u32 mdp_ctrl2 = dsi_read(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2);
+
+ mdp_ctrl2 |= DSI_CMD_MODE_MDP_CTRL2_DATABUS_WIDEN;
+ dsi_write(msm_host, REG_DSI_CMD_MODE_MDP_CTRL2, mdp_ctrl2);
+ }
Same comment as on your BURST_MODE patch (which this'll conflict with):
does this belong to the timing setup or is it better moved to
dsi_ctrl_config?
- Marijn
}
}
--
2.40.1