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

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