Re: [Freedreno] [PATCH v6 6/6] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations

[Abhinav Kumar]

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