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

- Marijn