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

[Dmitry Baryshkov]

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.

> 
> 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
>> +		 * 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.

> 
>> +		 * 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.

> 
>> +		 * greater than 8.
>> +		 *
>> +		 * The back/font porch and pulse width are kept intact.  They
>> +		 * represent timing parameters rather than actual data
>> +		 * transfer.
> 
> See FIXME above on dsi_adjust_pclk_for_compression()?
> 
> Thanks so much for finally putting some of this to paper.
> 
> - 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