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

[PATCH v2] 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>
---

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
+ * @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
+ *
+ * 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
+ *   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.
+ */
 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
+		 * 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.
+		 *
+		 * 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().
+		 *
+		 * 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 v2] drm/msm/dsi: Document DSC related pclk_rate and hdisplay calculations

[Marijn Suijten]

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

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

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

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

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

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

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

[Dmitry Baryshkov]

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

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

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

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

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

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

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

[Abhinav Kumar]

On 6/19/2023 2:06 PM, 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>
> ---
> 
> 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
> + * @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
> + *
> + * 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
> + *   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.
> + */
>   static unsigned long dsi_adjust_pclk_for_compression(const struct drm_display_mode *mode,
>   		const struct drm_dsc_config *dsc)

I am fine with this part of the doc.

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

I dont want to talk about data alignment nor formats and I will not ack 
any references to those.

I would like to keep this simple and say that DPU sends 3 bytes of 
compressed data / pclk (6 with widebus enabled) and all this math is 
doing is that its calculating number of bytes and diving it by 3 OR 6 
with widebus to calculate the pclk cycles. Thats it.

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

this part is fine.

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

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

[Dmitry Baryshkov]

On 23/06/2023 03:14, Abhinav Kumar wrote:
> 
> 
> On 6/19/2023 2:06 PM, 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>
>> ---
>>
>> 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
>> + * @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
>> + *
>> + * 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
>> + *   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.
>> + */
>>   static unsigned long dsi_adjust_pclk_for_compression(const struct 
>> drm_display_mode *mode,
>>           const struct drm_dsc_config *dsc)
> 
> I am fine with this part of the doc.
> 
>>   {
>> @@ -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
>> +         * 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.
>> +         *
> 
> I dont want to talk about data alignment nor formats and I will not ack 
> any references to those.
> 
> I would like to keep this simple and say that DPU sends 3 bytes of 
> compressed data / pclk (6 with widebus enabled) and all this math is 
> doing is that its calculating number of bytes and diving it by 3 OR 6 
> with widebus to calculate the pclk cycles. Thats it.

This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp 
/ 24.  My description might be inaccurate as I don't have hw docs at 
hand, but simple description is not enough.

> 
>> +         * 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.
>> +         *
>> +         * 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().
> 
> this part is fine.
> 
>> +         *
>> +         * 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);

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
> On 23/06/2023 03:14, Abhinav Kumar wrote:
>>
>>
>> On 6/19/2023 2:06 PM, 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>
>>> ---
>>>
>>> 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
>>> + * @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
>>> + *
>>> + * 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
>>> + *   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.
>>> + */
>>>   static unsigned long dsi_adjust_pclk_for_compression(const struct 
>>> drm_display_mode *mode,
>>>           const struct drm_dsc_config *dsc)
>>
>> I am fine with this part of the doc.
>>
>>>   {
>>> @@ -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
>>> +         * 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.
>>> +         *
>>
>> I dont want to talk about data alignment nor formats and I will not 
>> ack any references to those.
>>
>> I would like to keep this simple and say that DPU sends 3 bytes of 
>> compressed data / pclk (6 with widebus enabled) and all this math is 
>> doing is that its calculating number of bytes and diving it by 3 OR 6 
>> with widebus to calculate the pclk cycles. Thats it.
> 
> This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp 
> / 24.  My description might be inaccurate as I don't have hw docs at 
> hand, but simple description is not enough.
> 

Why is it unclear? With compression, we are saying we process at 3 
compressed bytes / pclk and this math is accurately giving the pclk cycles.

You are once again trying to arrive at 3 with compression factor in mind 
by calculating target_bpp / src_bpp.

I am saying that its independent of that. Whenever we do compression 
rate is 3 bytes of compressed data (and 6 with widebus) irrespective of 
what your dsc_bpp was.

>>
>>> +         * 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.
>>> +         *
>>> +         * 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().
>>
>> this part is fine.
>>
>>> +         *
>>> +         * 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);
> 

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

[Dmitry Baryshkov]

On 23/06/2023 03:32, Abhinav Kumar wrote:
> 
> 
> On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
>> On 23/06/2023 03:14, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/19/2023 2:06 PM, 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>
>>>> ---
>>>>
>>>> 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
>>>> + * @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
>>>> + *
>>>> + * 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
>>>> + *   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.
>>>> + */
>>>>   static unsigned long dsi_adjust_pclk_for_compression(const struct 
>>>> drm_display_mode *mode,
>>>>           const struct drm_dsc_config *dsc)
>>>
>>> I am fine with this part of the doc.
>>>
>>>>   {
>>>> @@ -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
>>>> +         * 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.
>>>> +         *
>>>
>>> I dont want to talk about data alignment nor formats and I will not 
>>> ack any references to those.
>>>
>>> I would like to keep this simple and say that DPU sends 3 bytes of 
>>> compressed data / pclk (6 with widebus enabled) and all this math is 
>>> doing is that its calculating number of bytes and diving it by 3 OR 6 
>>> with widebus to calculate the pclk cycles. Thats it.
>>
>> This makes it unclear, why do we simply by 3 rather than doing * 
>> dsc_bpp / 24.  My description might be inaccurate as I don't have hw 
>> docs at hand, but simple description is not enough.
>>
> 
> Why is it unclear? With compression, we are saying we process at 3 
> compressed bytes / pclk and this math is accurately giving the pclk cycles.
> 
> You are once again trying to arrive at 3 with compression factor in mind 
> by calculating target_bpp / src_bpp.
> 
> I am saying that its independent of that. Whenever we do compression 
> rate is 3 bytes of compressed data (and 6 with widebus) irrespective of 
> what your dsc_bpp was.

3 bytes of compressed data = 4 * 6bpp pixels. So hdisplay should have 
been divided by 4 in such case. So, this is not just about bytes.

> 
>>>
>>>> +         * 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.
>>>> +         *
>>>> +         * 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().
>>>
>>> this part is fine.
>>>
>>>> +         *
>>>> +         * 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);
>>

-- 
With best wishes
Dmitry

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

[Marijn Suijten]

On 2023-06-22 17:32:17, Abhinav Kumar wrote:
> 
> 
> On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
> > On 23/06/2023 03:14, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/19/2023 2:06 PM, 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>
> >>> ---
> >>>
> >>> 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
> >>> + * @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
> >>> + *
> >>> + * 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
> >>> + *   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.
> >>> + */
> >>>   static unsigned long dsi_adjust_pclk_for_compression(const struct 
> >>> drm_display_mode *mode,
> >>>           const struct drm_dsc_config *dsc)
> >>
> >> I am fine with this part of the doc.
> >>
> >>>   {
> >>> @@ -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
> >>> +         * 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.
> >>> +         *
> >>
> >> I dont want to talk about data alignment nor formats and I will not 
> >> ack any references to those.
> >>
> >> I would like to keep this simple and say that DPU sends 3 bytes of 
> >> compressed data / pclk (6 with widebus enabled) and all this math is 
> >> doing is that its calculating number of bytes and diving it by 3 OR 6 
> >> with widebus to calculate the pclk cycles. Thats it.
> > 
> > This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp 
> > / 24.  My description might be inaccurate as I don't have hw docs at 
> > hand, but simple description is not enough.
> > 
> 
> Why is it unclear? With compression, we are saying we process at 3 
> compressed bytes / pclk and this math is accurately giving the pclk cycles.
> 
> You are once again trying to arrive at 3 with compression factor in mind 
> by calculating target_bpp / src_bpp.
> 
> I am saying that its independent of that. Whenever we do compression 
> rate is 3 bytes of compressed data (and 6 with widebus) irrespective of 
> what your dsc_bpp was.

Abhinav, this is exactly what the confusion the pclk series is about.
There it was said (and committed to mainline now!) that pclk is based on
the compression factor of target_bpp / src_bpp.  Now you are saying
there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
and DSI.

Is pclk used for more purposes besides just ticking for the data
transfer between DPU and DSI?

- Marijn

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

[Abhinav Kumar]

On 6/23/2023 12:26 AM, Marijn Suijten wrote:
> On 2023-06-22 17:32:17, Abhinav Kumar wrote:
>>
>>
>> On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 03:14, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/19/2023 2:06 PM, 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>
>>>>> ---
>>>>>
>>>>> 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
>>>>> + * @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
>>>>> + *
>>>>> + * 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
>>>>> + *   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.
>>>>> + */
>>>>>    static unsigned long dsi_adjust_pclk_for_compression(const struct
>>>>> drm_display_mode *mode,
>>>>>            const struct drm_dsc_config *dsc)
>>>>
>>>> I am fine with this part of the doc.
>>>>
>>>>>    {
>>>>> @@ -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
>>>>> +         * 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.
>>>>> +         *
>>>>
>>>> I dont want to talk about data alignment nor formats and I will not
>>>> ack any references to those.
>>>>
>>>> I would like to keep this simple and say that DPU sends 3 bytes of
>>>> compressed data / pclk (6 with widebus enabled) and all this math is
>>>> doing is that its calculating number of bytes and diving it by 3 OR 6
>>>> with widebus to calculate the pclk cycles. Thats it.
>>>
>>> This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp
>>> / 24.  My description might be inaccurate as I don't have hw docs at
>>> hand, but simple description is not enough.
>>>
>>
>> Why is it unclear? With compression, we are saying we process at 3
>> compressed bytes / pclk and this math is accurately giving the pclk cycles.
>>
>> You are once again trying to arrive at 3 with compression factor in mind
>> by calculating target_bpp / src_bpp.
>>
>> I am saying that its independent of that. Whenever we do compression
>> rate is 3 bytes of compressed data (and 6 with widebus) irrespective of
>> what your dsc_bpp was.
> 
> Abhinav, this is exactly what the confusion the pclk series is about.
> There it was said (and committed to mainline now!) that pclk is based on
> the compression factor of target_bpp / src_bpp.  Now you are saying
> there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
> and DSI.
> 
> Is pclk used for more purposes besides just ticking for the data
> transfer between DPU and DSI?
> 

There is no confusion between what was said earlier and now.

This line is calculating the number of pclks needed to transmit one line 
of the compressed data:

hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);

msm_dsc_get_bytes_per_line() is calculating the number of compressed 
bytes as it uses the target bits_per_pixel

126 	 * @bits_per_pixel:
127 	 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
128 	 */
129 	u16 bits_per_pixel;

(like I have said a few times, hdisplay is perhaps confusing us)

If you calculate the bytes this way you are already accounting for the 
compression, so where is the confusion.

The pclk calculation does the same thing of using the ratio instead.

> - Marijn

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

[Marijn Suijten]

On 2023-06-23 12:54:17, Abhinav Kumar wrote:
> 
> 
> On 6/23/2023 12:26 AM, Marijn Suijten wrote:
> > On 2023-06-22 17:32:17, Abhinav Kumar wrote:
> >>
> >>
> >> On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
> >>> On 23/06/2023 03:14, Abhinav Kumar wrote:
> >>>>
> >>>>
> >>>> On 6/19/2023 2:06 PM, 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>
> >>>>> ---
> >>>>>
> >>>>> 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
> >>>>> + * @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
> >>>>> + *
> >>>>> + * 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
> >>>>> + *   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.
> >>>>> + */
> >>>>>    static unsigned long dsi_adjust_pclk_for_compression(const struct
> >>>>> drm_display_mode *mode,
> >>>>>            const struct drm_dsc_config *dsc)
> >>>>
> >>>> I am fine with this part of the doc.
> >>>>
> >>>>>    {
> >>>>> @@ -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
> >>>>> +         * 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.
> >>>>> +         *
> >>>>
> >>>> I dont want to talk about data alignment nor formats and I will not
> >>>> ack any references to those.
> >>>>
> >>>> I would like to keep this simple and say that DPU sends 3 bytes of
> >>>> compressed data / pclk (6 with widebus enabled) and all this math is
> >>>> doing is that its calculating number of bytes and diving it by 3 OR 6
> >>>> with widebus to calculate the pclk cycles. Thats it.
> >>>
> >>> This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp
> >>> / 24.  My description might be inaccurate as I don't have hw docs at
> >>> hand, but simple description is not enough.
> >>>
> >>
> >> Why is it unclear? With compression, we are saying we process at 3
> >> compressed bytes / pclk and this math is accurately giving the pclk cycles.
> >>
> >> You are once again trying to arrive at 3 with compression factor in mind
> >> by calculating target_bpp / src_bpp.
> >>
> >> I am saying that its independent of that. Whenever we do compression
> >> rate is 3 bytes of compressed data (and 6 with widebus) irrespective of
> >> what your dsc_bpp was.
> > 
> > Abhinav, this is exactly what the confusion the pclk series is about.
> > There it was said (and committed to mainline now!) that pclk is based on
> > the compression factor of target_bpp / src_bpp.  Now you are saying
> > there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
> > and DSI.
> > 
> > Is pclk used for more purposes besides just ticking for the data
> > transfer between DPU and DSI?
> > 
> 
> There is no confusion between what was said earlier and now.
> 
> This line is calculating the number of pclks needed to transmit one line 
> of the compressed data:
> 
> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> 
> msm_dsc_get_bytes_per_line() is calculating the number of compressed 
> bytes as it uses the target bits_per_pixel
> 
> 126 	 * @bits_per_pixel:
> 127 	 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
> 128 	 */
> 129 	u16 bits_per_pixel;
> 
> (like I have said a few times, hdisplay is perhaps confusing us)
> 
> If you calculate the bytes this way you are already accounting for the 
> compression, so where is the confusion.
> 
> The pclk calculation does the same thing of using the ratio instead.

This is not answering my question whether the ratio for pclk calculation
should also be adjusted to account for widebus.  And if the ratio is
fixed, why use a fixed factor here but the ratio between
src_bpp:target_bpp here?  It only adds extra confusion.

- Marijn

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

[Dmitry Baryshkov]

On 23/06/2023 23:02, Marijn Suijten wrote:
> On 2023-06-23 12:54:17, Abhinav Kumar wrote:
>>
>>
>> On 6/23/2023 12:26 AM, Marijn Suijten wrote:
>>> On 2023-06-22 17:32:17, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/22/2023 5:17 PM, Dmitry Baryshkov wrote:
>>>>> On 23/06/2023 03:14, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/19/2023 2:06 PM, 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>
>>>>>>> ---
>>>>>>>
>>>>>>> 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
>>>>>>> + * @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
>>>>>>> + *
>>>>>>> + * 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
>>>>>>> + *   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.
>>>>>>> + */
>>>>>>>     static unsigned long dsi_adjust_pclk_for_compression(const struct
>>>>>>> drm_display_mode *mode,
>>>>>>>             const struct drm_dsc_config *dsc)
>>>>>>
>>>>>> I am fine with this part of the doc.
>>>>>>
>>>>>>>     {
>>>>>>> @@ -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
>>>>>>> +         * 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.
>>>>>>> +         *
>>>>>>
>>>>>> I dont want to talk about data alignment nor formats and I will not
>>>>>> ack any references to those.
>>>>>>
>>>>>> I would like to keep this simple and say that DPU sends 3 bytes of
>>>>>> compressed data / pclk (6 with widebus enabled) and all this math is
>>>>>> doing is that its calculating number of bytes and diving it by 3 OR 6
>>>>>> with widebus to calculate the pclk cycles. Thats it.
>>>>>
>>>>> This makes it unclear, why do we simply by 3 rather than doing * dsc_bpp
>>>>> / 24.  My description might be inaccurate as I don't have hw docs at
>>>>> hand, but simple description is not enough.
>>>>>
>>>>
>>>> Why is it unclear? With compression, we are saying we process at 3
>>>> compressed bytes / pclk and this math is accurately giving the pclk cycles.
>>>>
>>>> You are once again trying to arrive at 3 with compression factor in mind
>>>> by calculating target_bpp / src_bpp.
>>>>
>>>> I am saying that its independent of that. Whenever we do compression
>>>> rate is 3 bytes of compressed data (and 6 with widebus) irrespective of
>>>> what your dsc_bpp was.
>>>
>>> Abhinav, this is exactly what the confusion the pclk series is about.
>>> There it was said (and committed to mainline now!) that pclk is based on
>>> the compression factor of target_bpp / src_bpp.  Now you are saying
>>> there is a fixed number of bytes sent by the (wide)bus between DPU-DSC
>>> and DSI.
>>>
>>> Is pclk used for more purposes besides just ticking for the data
>>> transfer between DPU and DSI?
>>>
>>
>> There is no confusion between what was said earlier and now.
>>
>> This line is calculating the number of pclks needed to transmit one line
>> of the compressed data:
>>
>> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>
>> msm_dsc_get_bytes_per_line() is calculating the number of compressed
>> bytes as it uses the target bits_per_pixel
>>
>> 126 	 * @bits_per_pixel:
>> 127 	 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
>> 128 	 */
>> 129 	u16 bits_per_pixel;
>>
>> (like I have said a few times, hdisplay is perhaps confusing us)
>>
>> If you calculate the bytes this way you are already accounting for the
>> compression, so where is the confusion.
>>
>> The pclk calculation does the same thing of using the ratio instead.
> 
> This is not answering my question whether the ratio for pclk calculation
> should also be adjusted to account for widebus.  And if the ratio is
> fixed, why use a fixed factor here but the ratio between
> src_bpp:target_bpp here?  It only adds extra confusion.

Wide bus is dicussed separately. I think the question you are trying to 
ask is "why are we not using msm_dsc_get_bytes_per_line() in 
dsi_adjust_pclk_for_compression()?"

-- 
With best wishes
Dmitry

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

[Marijn Suijten]

On 2023-06-23 23:10:56, Dmitry Baryshkov wrote:
<snip>
> >> There is no confusion between what was said earlier and now.
> >>
> >> This line is calculating the number of pclks needed to transmit one line
> >> of the compressed data:
> >>
> >> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
> >>
> >> msm_dsc_get_bytes_per_line() is calculating the number of compressed
> >> bytes as it uses the target bits_per_pixel
> >>
> >> 126 	 * @bits_per_pixel:
> >> 127 	 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
> >> 128 	 */
> >> 129 	u16 bits_per_pixel;
> >>
> >> (like I have said a few times, hdisplay is perhaps confusing us)
> >>
> >> If you calculate the bytes this way you are already accounting for the
> >> compression, so where is the confusion.
> >>
> >> The pclk calculation does the same thing of using the ratio instead.
> > 
> > This is not answering my question whether the ratio for pclk calculation
> > should also be adjusted to account for widebus.  And if the ratio is
> > fixed, why use a fixed factor here but the ratio between
> > src_bpp:target_bpp here?  It only adds extra confusion.
> 
> Wide bus is dicussed separately. I think the question you are trying to 
> ask is "why are we not using msm_dsc_get_bytes_per_line() in 
> dsi_adjust_pclk_for_compression()?"

I have asked that question before, and the answer was something
incomprehensible.  But indeed, it would look more natural if
dsi_adjust_pclk_for_compression() replaces:

    int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
        dsc->bits_per_component * 3)

With:

    int new_hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc), 3);

Which is the same value as we have here.

And then it becomes more clear how widebus affects this calculation.

- Marijn

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

[Abhinav Kumar]

On 6/23/2023 1:18 PM, Marijn Suijten wrote:
> On 2023-06-23 23:10:56, Dmitry Baryshkov wrote:
> <snip>
>>>> There is no confusion between what was said earlier and now.
>>>>
>>>> This line is calculating the number of pclks needed to transmit one line
>>>> of the compressed data:
>>>>
>>>> hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), 3);
>>>>
>>>> msm_dsc_get_bytes_per_line() is calculating the number of compressed
>>>> bytes as it uses the target bits_per_pixel
>>>>
>>>> 126 	 * @bits_per_pixel:
>>>> 127 	 * Target bits per pixel with 4 fractional bits, bits_per_pixel << 4
>>>> 128 	 */
>>>> 129 	u16 bits_per_pixel;
>>>>
>>>> (like I have said a few times, hdisplay is perhaps confusing us)
>>>>
>>>> If you calculate the bytes this way you are already accounting for the
>>>> compression, so where is the confusion.
>>>>
>>>> The pclk calculation does the same thing of using the ratio instead.
>>>
>>> This is not answering my question whether the ratio for pclk calculation
>>> should also be adjusted to account for widebus.  And if the ratio is
>>> fixed, why use a fixed factor here but the ratio between
>>> src_bpp:target_bpp here?  It only adds extra confusion.
>>
>> Wide bus is dicussed separately. I think the question you are trying to
>> ask is "why are we not using msm_dsc_get_bytes_per_line() in
>> dsi_adjust_pclk_for_compression()?"
> 
> I have asked that question before, and the answer was something
> incomprehensible.  But indeed, it would look more natural if
> dsi_adjust_pclk_for_compression() replaces:
> 
>      int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
>          dsc->bits_per_component * 3)
> 
> With:
> 
>      int new_hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(dsc), 3);
> 
> Which is the same value as we have here.
> 
> And then it becomes more clear how widebus affects this calculation.
> 
> - Marijn

Ok, I finally got your question I think. That "why don't we account for 
widebus in the pixel clock calculation but only for calculating pclk 
cycles for REG_DSI_CMD_MDP_STREAM0_TOTAL"

Even though DPU shall transmit 48 bits worth of data , DSI will be able 
to consume only one uncompressed pixel worth of data. Thats the rule.

Hence even though we are able to account for widebus in the 
stream0_total to indicate DPU's increased data transfer rate with 
widebus, since DSI is still going to consume only one uncompressed pixel 
worth of data, we can only scale the pixel clock with compression ratio.

Hope that clarifies it.