[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 at linaro.org>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes since v1:
>>>>>>> - Converted dsi_adjust_pclk_for_compression() into kerneldoc (Marijn)
>>>>>>> - Added a pointer from dsi_timing_setup() docs to
>>>>>>>      dsi_adjust_pclk_for_compression() (Marijn)
>>>>>>> - Fixed two typo (Marijn)
>>>>>>>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/msm/dsi/dsi_host.c | 40 ++++++++++++++++++++++++++++--
>>>>>>>     1 file changed, 38 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> index 3f6dfb4f9d5a..a8a31c3dd168 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>>> @@ -528,6 +528,25 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host
>>>>>>> *msm_host)
>>>>>>>         clk_disable_unprepare(msm_host->byte_clk);
>>>>>>>     }
>>>>>>> +/**
>>>>>>> + * dsi_adjust_pclk_for_compression() - Adjust the pclk rate for
>>>>>>> compression case
>>>>>>> + * @mode: the selected mode for the DSI output
>>>>>>> + * @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