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

[Marijn Suijten <marijn.suijten@somainline.org>]

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@quicinc.com>
> ---
>  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.

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.

>  
> @@ -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
>