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

[Marijn Suijten]

On 2023-06-19 23:57:22, Dmitry Baryshkov wrote:
> On 16/06/2023 15:25, Marijn Suijten wrote:
> > On 2023-06-16 12:41:52, Dmitry Baryshkov wrote:
> >> Provide actual documentation for the pclk and hdisplay calculations in
> >> the case of DSC compression being used.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 35 ++++++++++++++++++++++++++++--
> >>   1 file changed, 33 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> index 3f6dfb4f9d5a..72c377c9c7be 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -528,6 +528,21 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
> >>   	clk_disable_unprepare(msm_host->byte_clk);
> >>   }
> >>   
> >> +/*
> >> + * Adjust the pclk rate by calculating a new hdisplay proportional to
> > 
> > Make this a kerneldoc with:
> 
> Ack
> 
> > 
> >      /**
> >       * dsi_adjust_pclk_for_compression() - Adjust ...
> > 
> >> + * the compression ratio such that:
> >> + *     new_hdisplay = old_hdisplay * compressed_bpp / uncompressed_bpp
> >> + *
> >> + * Porches do not need to be adjusted:
> >> + * - For the VIDEO mode they are not compressed by DSC and are passed as is.
> > 
> > as-is
> 
> Cambridge dictionary gives this "as is", without dash.

I think you are right, it depends on whether this compound adjective
comes before or after (in this case after) the verb it modifies, and it
shouldn't be hyphenated if it comes after [1].

[1]: https://ell.stackexchange.com/questions/24014/as-it-is-and-as-is-as-as-a-conjunction-and-a-pronoun#comment45615_24105

> > Though this was never tested nor confirmed by QUIC, but we can assume it
> > is the case for now?
> > 
> >> + * - For the CMD mode the are no actual porches. Instead they represent the
> > 
> > the are no -> these are not
> > 
> > they currently* represent.  
> 
> Ack
> 
> > Let's make sure that folks read the FIXME
> > below by perhaps rewording it right into this entry?
> 
> I kept it separately, so that the FIXME can be removed once CMD handling 
> is reworked.
> 
> > 
> >> + *   overhead to the image data transfer. As such, they are calculated for the
> >> + *   final mode parameters (after the compression) and are not to be adjusted
> >> + *   too.
> >> + *
> >> + *  FIXME: Reconsider this if/when CMD mode handling is rewritten to use
> >> + *  refresh rate and data overhead as a starting point of the calculations.
> >> + */
> >>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
> >>   		const struct drm_dsc_config *dsc)
> >>   {
> >> @@ -926,8 +941,24 @@ static void dsi_timing_setup(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> >>   		if (ret)
> >>   			return;
> >>   
> >> -		/* Divide the display by 3 but keep back/font porch and
> >> -		 * pulse width same
> >> +		/*
> >> +		 * DPU sends 3 bytes per pclk cycle to DSI. If compression is

Following your comment below, this should then also be "3 compressed
pixels"?

> >> +		 * not used, a single pixel is transferred at each pulse, no
> >> +		 * matter what bpp or pixel format is used. In case of DSC
> >> +		 * compression this results (due to data alignment
> >> +		 * requirements) in a transfer of 3 compressed pixel per pclk
> > 
> > 3 compressed bytes*, not pixels.
> 
> No, that's the point. With 6bpp one can think that 4 pixels would fit, 
> but they don't.

But that is not what Jessica was doing in the function that adjusts the
pclk **based on the bpp ratio** rather than this fixed factor relating
to the number of pixels you are bringing up here.  All this time the
code, discussion, and code-comments are contradictory and it makes it
impossible to understand.

And likewise, if we have bpp=10 or bpp=12, what will happen to pclk and
hdisplay?

(It makes sense from how DSC is implemented, processing groups of 3
 pixels at a time)

 In any case, then it is the plural pixels* not "3 compressed pixel".

> >> +		 * cycle.
> >> +		 *
> >> +		 * If widebus is enabled, bus width is extended to 6 bytes.
> >> +		 * This way the DPU can transfer 6 compressed pixels with bpp
> > 
> > pixels -> bytes?
> 
> Same comment, no.
> 
> > 
> >> +		 * less or equal to 8 or 3 compressed pyxels in case bpp is
> > 
> > pixels*... bytes?
> > 
> > And I will ask this **again**: does this mean we can halve pclk?
> 
> My guess would be no, since all other data transfers are not scaled by 
> wide bus.

Is the fact that we *don't scale it yet* definitive for saying that we
shouldn't?

- Marijn