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

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

[Dmitry Baryshkov]

Provide actual documentation for the pclk and hdisplay calculations in
the case of DSC compression being used.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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
+ * 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.
+ * - For the CMD mode the are no actual porches. Instead they 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.
+ */
 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
+		 * 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 pyxels in case bpp is
+		 * greater than 8.
+		 *
+		 * The back/font porch and pulse width are kept intact.  They
+		 * represent timing parameters rather than actual data
+		 * transfer.
+		 *
+		 * 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

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

[Dmitry Baryshkov]

Provide actual documentation for the pclk and hdisplay calculations in
the case of DSC compression being used.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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
+ * 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.
+ * - For the CMD mode the are no actual porches. Instead they 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.
+ */
 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
+		 * 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 pyxels in case bpp is
+		 * greater than 8.
+		 *
+		 * The back/font porch and pulse width are kept intact.  They
+		 * represent timing parameters rather than actual data
+		 * transfer.
+		 *
+		 * 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

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

[Marijn Suijten]

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

    /**
     * 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

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.  Let's make sure that folks read the FIXME
below by perhaps rewording it right into this entry?

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

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

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

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

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

[Marijn Suijten]

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

    /**
     * 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

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.  Let's make sure that folks read the FIXME
below by perhaps rewording it right into this entry?

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

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

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

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

Re: [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@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

Re: [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@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

Re: [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@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

Re: [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@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