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

[Marijn Suijten]

On 2023-06-20 17:27:46, Dmitry Baryshkov wrote:
> On 20/06/2023 15:05, Marijn Suijten wrote:
> > On 2023-06-20 00:06:47, 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>
> >> ---
> >>
> >> Changes since v1:
> >> - Converted dsi_adjust_pclk_for_compression() into kerneldoc (Marijn)
> >> - Added a pointer from dsi_timing_setup() docs to
> >>    dsi_adjust_pclk_for_compression() (Marijn)
> >> - Fixed two typo (Marijn)
> >>
> >> ---
> >>   drivers/gpu/drm/msm/dsi/dsi_host.c | 40 ++++++++++++++++++++++++++++--
> >>   1 file changed, 38 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..a8a31c3dd168 100644
> >> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> >> @@ -528,6 +528,25 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
> >>   	clk_disable_unprepare(msm_host->byte_clk);
> >>   }
> >>   
> >> +/**
> >> + * dsi_adjust_pclk_for_compression() - Adjust the pclk rate for compression case
> >> + * @mode: the selected mode for the DSI output
> > 
> > The
> > 
> >> + * @dsc: DRM DSC configuration for this DSI output
> >> + *
> >> + * Adjust the pclk rate by calculating a new hdisplay proportional to
> >> + * the compression ratio such that:
> >> + *     new_hdisplay = old_hdisplay * compressed_bpp / uncompressed_bpp
> > 
> > And in v1 you explained that it is _not_ about bpp...
> 
> Hmm, this bit stayed intact, so I'm slightly confused here.

It did, because no-one bothered to respond to my questions about this
BPP rate versus "pixels/bytes per pclk" inquiry.  So I just keep
repeating it every patch that touches/mentions it, until someone
replies.

>   This function is about BPP and compressed rate. dsi_timing_setup() is 
> about bytes.

The question is whether that is _correct_.  The discussion all this time
is about pclk - even for compressed mode - counting the number of
pixels, not the bpp ratio.

> >> + *
> >> + * Porches do not need to be adjusted:
> >> + * - For the VIDEO mode they are not compressed by DSC and are passed as is.
> >> + * - For the CMD mode there are no actual porches. Instead these fields
> > 
> > I feel like "For VIDEO mode" and "For CMD mode" reads more naturally, no
> > need for "the", but I don't know the grammar rule that states so.
> 
> Ack
> 
> > 
> >> + *   currently represent the 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.
> > 
> > Nit: well, refresh rate is already part of this calculation (that's how
> > drm_display_mode's clock member comes to be, and how drm_mode_vrefresh()
> > figures out fps after the fact).  It's all about the per-CMD transfer
> > overhead in bytes that is currently not part of the calculation.
> 
> Please correct me if I'm wrong, we start from mode->clock. Refresh rate 
> isn't even a part of struct drm_display_mode.

It is not a separate field because it is _embedded in mode->clock_ (see
how drm_mode_vrefresh() is implemented to divide ->clock by htotal and
vtotal to get the fps again), and hence already part of the values used
in this calculation.

> >> + */
> >>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
> >>   		const struct drm_dsc_config *dsc)
> >>   {
> >> @@ -926,8 +945,25 @@ 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
> > 
> > Should this be pixels (1 pixel), not bytes, just like in the compressed
> > scenario?
> 
> No.

So if the uncomrpessed BPP changes, pclk _also changes_?

- Marijn

> >> +		 * 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
> >> +		 * cycle.
> >> +		 *
> >> +		 * If widebus is enabled, bus width is extended to 6 bytes.
> >> +		 * This way the DPU can transfer 6 compressed pixels with bpp
> >> +		 * less or equal to 8 or 3 compressed pixels in case bpp is
> >> +		 * greater than 8.
> > 
> > Okay, so one can not send more than 6 pixels even if the bpp is less
> > than 8, is what this comes down to.
> 
> Yes.
> 
> > 
> >> +		 *
> >> +		 * The back/font porch and pulse width are kept intact.  They
> >> +		 * represent timing parameters rather than actual data
> >> +		 * transfer. See the documentation of
> >> +		 * dsi_adjust_pclk_for_compression().
> > 
> > Nit: note that this is only for VIDEO mode, h_total and ha_end are
> > accurately unused in the CMDmode path below.
> > 
> > - Marijn
> > 
> >> +		 *
> >> +		 * XXX: widebus is not supported by the driver (yet).
> >>   		 */
> >>   		h_total -= hdisplay;
> >>   		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> >> -- 
> >> 2.39.2
> >>
> 
> -- 
> With best wishes
> Dmitry
>