On Tue, Dec 23, 2025 at 06:08:49PM +0530, Nautiyal, Ankit K wrote:
>
> On 12/23/2025 3:16 PM, Imre Deak wrote:
> > On Tue, Dec 23, 2025 at 11:05:59AM +0530, Nautiyal, Ankit K wrote:
> > > On 12/22/2025 6:26 PM, Imre Deak wrote:
> > > > On Mon, Dec 22, 2025 at 10:14:43AM +0530, Ankit Nautiyal wrote:
> > > > > When DSC is enabled on a pipe, the pipe pixel rate input to the
> > > > > CDCLK frequency and pipe joining calculation needs an adjustment to
> > > > > account for compression overhead "bubbles" added at each horizontal
> > > > > slice boundary.
> > > > >
> > > > > Account for this overhead while computing min cdclk required for DSC.
> > > > >
> > > > > v2: Get rid of the scaling factor and return unchanged pixel-rate
> > > > > instead of 0.
> > > > >
> > > > > Bspec:68912
> > > > > Signed-off-by: Ankit Nautiyal <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/i915/display/intel_vdsc.c | 45
> > > > > +++++++++++++++++++++--
> > > > > 1 file changed, 41 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > index ad5fe841e4b3..b91cd009be9d 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > > > @@ -1050,15 +1050,52 @@ void intel_vdsc_state_dump(struct drm_printer
> > > > > *p, int indent,
> > > > > drm_dsc_dump_config(p, indent, &crtc_state->dsc.config);
> > > > > }
> > > > > +static
> > > > > +int intel_dsc_get_pixel_rate_with_dsc_bubbles(int pixel_rate, int
> > > > > htotal,
> > > > > + int dsc_horizontal_slices)
> > > > > +{
> > > > > + int dsc_slice_bubbles;
> > > > > + u64 num;
> > > > > +
> > > > > + if (!htotal)
> > > > Should this also warn as !slice_width below?
> > > Yeah can add add warn here..
> > >
> > > > > + return pixel_rate;
> > > > > +
> > > > > + dsc_slice_bubbles = 14 * dsc_horizontal_slices;
> > > > > + num = (u64)pixel_rate * (u64)(htotal + dsc_slice_bubbles);
> > > > Better to use mul_u32_u32() to avoid the casts and 64-bit x 64-bit
> > > > multiplication.
> > > Ok sure will use mul_u32_u32 to avoid casts here.
> > >
> > > > > +
> > > > > + return (int)DIV_ROUND_UP_ULL(num, (u64)htotal);
> > > > Both casts are ensured by the compiler already, so no need for doing
> > > > them explicitly.
> > > Got it.
> > >
> > > > > +}
> > > > > +
> > > > > +static
> > > > > +int pixel_rate_with_dsc_bubbles(const struct intel_crtc_state
> > > > > *crtc_state, int pixel_rate)
> > > > > +{
> > > > > + struct intel_display *display = to_intel_display(crtc_state);
> > > > > + const struct drm_display_mode *adjusted_mode =
> > > > > &crtc_state->hw.adjusted_mode;
> > > > > + const struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> > > > > + int dsc_horizontal_slices;
> > > > > +
> > > > > + if (drm_WARN_ON(display->drm, !vdsc_cfg->slice_width))
> > > > > + return pixel_rate;
> > > > > +
> > > > > + dsc_horizontal_slices = vdsc_cfg->pic_width /
> > > > > vdsc_cfg->slice_width;
> > > > The above looks to be the same as crtc_state->dsc.slice_count, aka the
> > > > slices per scanline, could you use that instead?
> > > Apparently we are not filling dsc.slice_count. We are filling
> > > dsc.slice_width and the pic_width.
> > dsc.slice_count is computed for the given encoder and dsc.slice_width is
> > computed based on that (as vdsc->pic_width / crtc_state->dsc.slice_count).
>
> Sorry I got confused between dsc.slice_count and dsc.config.slice_count (we
> do not fill this).
>
> I was also assuming we already got
> intel_dsc_line_slice_count(dsc.slice_config) merged, but that is still to be
> reviewed.
>
> So yes you are right we can use dsc.slice_count, which will be later
> replaced by the intel_dsc_line_slice_count() AFAIU.
Right, I meant intel_crtc_state::dsc.slice_count, which is replaced by
intel_dsc_line_slice_count() by a follow-up patchset as you described. I
think it makes sense to merge now your patch and I can rebase then the
follow-up patchset converting this place as well to use
intel_dsc_line_slice_count() instead of
intel_crtc_state::dsc.slice_count.
> Thanks for clearing this up. I will make the suggested change.
>
>
> Regards,
>
> Ankit
>
> >
> > > This parameter seems to be unused, perhaps can be dropped?
> > It's slice_count what is computed for a particular encoder and the rest
> > of DSC parameters are only derived from slice_count the same way for all
> > encoders.
> >
> > > Regards,
> > > Ankit
> > >
> > > > > +
> > > > > + return intel_dsc_get_pixel_rate_with_dsc_bubbles(pixel_rate,
> > > > > +
> > > > > adjusted_mode->crtc_htotal,
> > > > > +
> > > > > dsc_horizontal_slices);
> > > > > +}
> > > > > +
> > > > > int intel_vdsc_min_cdclk(const struct intel_crtc_state *crtc_state)
> > > > > {
> > > > > struct intel_display *display = to_intel_display(crtc_state);
> > > > > int num_vdsc_instances =
> > > > > intel_dsc_get_num_vdsc_instances(crtc_state);
> > > > > + int pixel_rate;
> > > > > int min_cdclk;
> > > > > if (!crtc_state->dsc.compression_enable)
> > > > > return 0;
> > > > > + pixel_rate = pixel_rate_with_dsc_bubbles(crtc_state,
> > > > > crtc_state->pixel_rate);
> > > > > +
> > > > > /*
> > > > > * When we decide to use only one VDSC engine, since
> > > > > * each VDSC operates with 1 ppc throughput, pixel clock
> > > > > @@ -1066,7 +1103,7 @@ int intel_vdsc_min_cdclk(const struct
> > > > > intel_crtc_state *crtc_state)
> > > > > * If there 2 VDSC engines, then pixel clock can't be higher
> > > > > than
> > > > > * VDSC clock(cdclk) * 2 and so on.
> > > > > */
> > > > > - min_cdclk = DIV_ROUND_UP(crtc_state->pixel_rate,
> > > > > num_vdsc_instances);
> > > > > + min_cdclk = DIV_ROUND_UP(pixel_rate, num_vdsc_instances);
> > > > > if (crtc_state->joiner_pipes) {
> > > > > int pixel_clock =
> > > > > intel_dp_mode_to_fec_clock(crtc_state->hw.adjusted_mode.clock);
> > > > > @@ -1084,9 +1121,9 @@ int intel_vdsc_min_cdclk(const struct
> > > > > intel_crtc_state *crtc_state)
> > > > > * => CDCLK >= compressed_bpp * Pixel clock / 2 *
> > > > > Bigjoiner Interface bits
> > > > > */
> > > > > int bigjoiner_interface_bits = DISPLAY_VER(display) >=
> > > > > 14 ? 36 : 24;
> > > > > - int min_cdclk_bj =
> > > > > -
> > > > > (fxp_q4_to_int_roundup(crtc_state->dsc.compressed_bpp_x16) *
> > > > > - pixel_clock) / (2 * bigjoiner_interface_bits);
> > > > > + int adjusted_pixel_rate =
> > > > > pixel_rate_with_dsc_bubbles(crtc_state, pixel_clock);
> > > > > + int min_cdclk_bj =
> > > > > (fxp_q4_to_int_roundup(crtc_state->dsc.compressed_bpp_x16) *
> > > > > + adjusted_pixel_rate) / (2 *
> > > > > bigjoiner_interface_bits);
> > > > > min_cdclk = max(min_cdclk, min_cdclk_bj);
> > > > > }
> > > > > --
> > > > > 2.45.2
> > > > >
