On 6/11/2023 3:03 PM, Marijn Suijten wrote:
On 2023-06-09 15:57:18, Jessica Zhang wrote:
Add documentation comments explaining the pclk_rate and hdisplay math
related to DSC.
Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index fb1d3a25765f..aeaadc18bc7b 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -564,6 +564,13 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
const struct drm_dsc_config *dsc)
{
+ /*
+ * Adjust the pclk rate by calculating a new hdisplay proportional to
+ * the compression ratio such that:
+ * new_hdisplay = old_hdisplay * target_bpp / source_bpp
+ *
+ * Porches need not be adjusted during compression.
+ */
int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
dsc->bits_per_component * 3);
I won't reiterate my original troubles with this logic and the comment
as that has well been described in v5 replies.
Just want to ask why this comment couldn't be added in patch 5/6
immediately when the logic is introduced? Now readers won't have a clue
what is going on until they skip one patch ahead.
Both myself and Dmitry discussed that in this particular case, we will
add the documentation as a follow-up patch and merge it together. Not
usually the process, but in this case, just decided to do it this way.
The series will still be merged as one.
Furthermore it is lacking any explanation that this is a workaround for
cmd-mode, and that porches are currently used to represent "transfer
time" until those calculations are implemented. At that point there is
no concept of "not adjusting porches for compressed signals" anymore.
This is a much bigger topic and goes out of scope of this patch and
series and I dont want to explain all that in this documentation patch.
If we explain that this is specific to command mode, what would the
panel drivers fill out for porches . Obviously they cannot fill out a 0.
Coming to transfer time. Even if current panel drivers use 0 porches,
the clock you get should still be sufficient for 60fps or a transfer
time of 16.66ms.
Transfer time was a concept introduced for some specific command mode
panels where we needed to finish transferring the frame even faster than
16.66ms like 12ms or 13ms.
Yes, without that, upstream and downstream math doesnt match. But that
doesnt mean its going to break the panels or that upstream math is
wrong. If you think command mode porches should be 0, then this will
give you the clk for 60fps. If you add some random porches, it will just
give a faster clock.
Porches can be used instead of transfer time till we add that math but
again, thats only needed for panels which need a faster transfer time
than 16.66ms.
So we dont need to call this a workaround in my opinion at all (and hack
as you called in v5 is totally out of proportion).
One could even argue that if the panel needs a transfer time faster than
16.66ms, then the mode->clock should also be bumped up. Panels dont do
that today either.
Hence, I am going to consider transfer time as an enhancement and not
going to take that up in this series so I am not for adding that comment
here.
And as I have explained, this patch is not a workaround either. Its just
calculating the clock based on what we have today in the panel drivers.
@@ -961,6 +968,9 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
/* Divide the display by 3 but keep back/font porch and
* pulse width same
+ *
+ * hdisplay will be divided by 3 here to account for the fact
+ * that DPU sends 3 bytes per pclk cycle to DSI.
*/
h_total -= hdisplay;
hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
Still very glad to have this, thank you for adding it. Note that it
only further undermines the pclk adjustments, as I just explained in v5
review.
- Marijn
--
2.40.1
