Re: [PATCH v3 4/4] drm/msm/dpu: fix video mode DSC INTF timing width calculation

[Alexander Koskovich]

On Thursday, March 19th, 2026 at 9:45 PM, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx> wrote:

> On Thu, Mar 19, 2026 at 01:23:03PM -0400, Jonathan Marek wrote:
> > On 3/19/26 10:54 AM, Neil Armstrong wrote:
> > > On 3/19/26 15:40, Alexander Koskovich wrote:
> > > > On Thursday, March 19th, 2026 at 10:13 AM, Neil Armstrong
> > > > <neil.armstrong@xxxxxxxxxx> wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On 3/19/26 12:58, Alexander Koskovich wrote:
> > > > > > Using bits_per_component * 3 as the divisor for the compressed INTF
> > > > > > timing width produces constant FIFO errors for the BOE BF068MWM-TD0
> > > > > > panel due to bits_per_component being 10 which results in a divisor
> > > > > > of 30 instead of 24.
> > > > > >
> > > > > > Regardless of the compression ratio and pixel depth, 24 bits of
> > > > > > compressed data are transferred per pclk, so the divisor should
> > > > > > always be 24.
> > > > >
> > > > > Not true with widebus, specify why 24 and because DSI widebus is
> > > > > not implemented yet.
> > > > >
> >
> > DSI widebus is implemented, and always used when available. The adjustment
> > for DSI widebus just doesn't happen in this function.
> >
> > > > > >
> > > > > > Signed-off-by: Alexander Koskovich <akoskovich@xxxxx>
> > > > > > ---
> > > > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 ++++-----
> > > > > >    1 file changed, 4 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git
> > > > > > a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > > > index 0ba777bda253..5419ef0be137 100644
> > > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> > > > > > @@ -122,19 +122,18 @@ static void drm_mode_to_intf_timing_params(
> > > > > >        }
> > > > > >
> > > > > >        /*
> > > > > > -     * for DSI, if compression is enabled, then divide the
> > > > > > horizonal active
> > > > > > -     * timing parameters by compression ratio. bits of 3
> > > > > > components(R/G/B)
> > > > > > -     * is compressed into bits of 1 pixel.
> > > > > > +     * For DSI, if DSC is enabled, 24 bits of compressed data are
> > > > > > +     * transferred per pclk regardless of the source pixel depth.
> > > > > >         */
> > > > > >        if (phys_enc->hw_intf->cap->type != INTF_DP &&
> > > > > > timing->compression_en) {
> > > > > >            struct drm_dsc_config *dsc =
> > > > > >                   dpu_encoder_get_dsc_config(phys_enc->parent);
> > > > > > +
> > > > > Drop this change
> > > > >
> > > > > >            /*
> > > > > >             * TODO: replace drm_dsc_get_bpp_int with logic to handle
> > > > > >             * fractional part if there is fraction
> > > > > >             */
> > > > > > -        timing->width = timing->width * drm_dsc_get_bpp_int(dsc) /
> > > > > > -                (dsc->bits_per_component * 3);
> > > > > > +        timing->width = timing->width * drm_dsc_get_bpp_int(dsc) / 24;
> > > > >
> > > > > It would be helpful to somehow show that 24 is 8 * 3, 8 being
> > > > > the byte width and 3 the compression ratio.
> > > >
> > > > Can you clarify what the 3 represents? My panel should have a 3.75:1
> > > > compression
> > > > ratio (30/8) so the final divisor of 24 would be wrong for my panel
> > > > if its the
> > > > compression ratio?
> > >
> > > So my guess is that while the exact ratio on the DSI lanes is 3.75:1,
> > > the ratio
> > > used to calculate the INTF timings is 3, then the DSC encoder probably
> > > does the
> > > magic to feed 10bpp into a 3.75:1 ratio over the DSI lanes.
> > >
> >
> > That's not how it works. INTF (which feeds DSI) is after DSC compression.
> >
> > INTF timings are always in RGB888 (24-bit) units. Ignoring widebus details,
> > the INTF timing should match what is programmed on the DSI side (hdisplay,
> > which is calculated as bytes per line / 3).
> >
> > (fwiw, the current "timing->width = ..." calculation here blames to me, but
> > what I wrote originally was just "timing->width = timing->width / 3" with a
> > comment about being incomplete.)
> >
> Okay. After reading the docs (sorry, it took a while).
> 
> - When widebus is not enabled, the transfer is always 24 bit of
>   compressed data. Thus if it is not in play, pclk and timing->width
>   should be scaled by source_pixel_depth / compression_ratio / 24. In
>   case of the code it is 'drm_dsc_get_bpp_int(dsc) / 24'.
> 
>   For RGB101010 / 8bpp DSC this should result in the PCLK being lowered
>   by the factor of 3 (= 24 / (30 / 3.75))
> 
> - When widebus is in play (MDSS 6.x+, DSI 2.4+), the transfer takes
>   more than 24 bits. In this case the PCLK and timing->width should be
>   scaled exactly by the DSC compression ratio, which is
>   'drm_dsc_get_bpp_int(dsc) / (3 * dsc->bits_per_component).
> 
> So, this piece of code needs to be adjusted to check for the widebus
> being enabled or not.
> 

Modified drm_mode_to_intf_timing_params & dsi_adjust_pclk_for_compression to account for widebus, but the hdisplay I get is different from stock with widebus factored in. Getting 288 instead of 360 now which produces constant FIFO errors.

This is with widebus enabled, using 3 * dsc->bits_per_component. Should it be 24 regardless of widebus?

> > > In dsi_adjust_pclk_for_compression, the pclk is adjusted to take in
> > > account bits_per_component,
> > > so I presume the actual DSI pclk _is_  timing->width *
> > > drm_dsc_get_bpp_int(dsc) / (dsc->bits_per_component * 3),
> > > which is your 3.75:1, but the INTF needs to generate timing->width *
> > > drm_dsc_get_bpp_int(dsc) / (8 * 3) pixels
> > > to the DSC encoder which will emit timing->width *
> > > drm_dsc_get_bpp_int(dsc) / (dsc->bits_per_component * 3) pixels.
> > >
> >
> > The hdisplay calculation in dsi_adjust_pclk_for_compression (which only
> > affects the clock rate) seems to be wrong, and I think Alexander's panel
> > must be running at a 20% lower clock because of it. dsi_timing_setup has the
> > right hdisplay adjustment.
> 
> That function also needs to be adjusted accordingly. I think only the
> dsi_timing_setup() is correct at this point. Note, widebus / not-widebus
> cases should be handled separately.
> 
> > > In any case, 24 _is_ 3 * 8, 3 being the DSC compression ratio on the
> > > INTF side.
> 
> In this case DSC compression ratio is 3.75, so it's not true.
> 
> --
> With best wishes
> Dmitry
> 

Thanks,
Alex