>
> On Thu, 02 Feb 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> > According to Bpec: 49259 VDSC spec implies that 108 lines is an
> > optimal slice height, but any size can be used as long as vertical
> > active integer multiple and maximum vertical slice count requirements are
> met.
>
> The commit message and subject should really indicate that this increases
> the slice height considerably. It's a 13.5x increase at a minimum, could be
> much more. Seems misleading to call it "fix logic", as if there's a small issue
> somewhere.
>
> Bspec references should be here:
>
> Bspec: 49259
> > Cc: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> > Cc: Swati Sharma <swati2.sharma@xxxxxxxxx>
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 62cbab7402e9..7bd2e56ef0fa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1415,6 +1415,22 @@ static int
> intel_dp_sink_dsc_version_minor(struct intel_dp *intel_dp)
> > DP_DSC_MINOR_SHIFT;
> > }
> >
> > +static int intel_dp_get_slice_height(int vactive)
>
> intel_dp_dsc_get_slice_height
>
> > +{
> > + int slice_height;
> > +
> > + /*
> > + * VDSC spec implies that 108 lines is an optimal slice height,
>
> Please be more specific with spec references than vague "VSDC spec". Spec
> version is required at a minimum. Section and section title are a nice bonus.
>
> > + * but any size can be used as long as vertical active integer
> > + * multiple and maximum vertical slice count requirements are met.
> > + */
> > + for (slice_height = 108; slice_height <= vactive; slice_height += 2)
>
> Where does it say 108 is a minimum, and you should go up only...?
So in VDSC 1.2a section 3.8 option for slices it says
"a slice height of 108 lines typically provides better
performance than a slice height of 8 lines."
It also states the following
"Also it says There is no cost associated with slice height because
there is no additional buffering or any other additional resources required"
that's why I decided to move up from slice height of 108
>
> > + if (!(vactive % slice_height))
>
> Matter of taste, but please use (vactive % slice_height == 0) for clarity on
> computations like this.
>
> > + return slice_height;
> > +
> > + return 0;
>
> I guess it's unlikely we ever hit here, but you could have the old code as
> fallback and never return 0. Because you don't check for 0 in the caller
> anyway.
I will do this
>
> Also makes me wonder why we have intel_hdmi_dsc_get_slice_height()
> separately, with almost identical implementation. Maybe we should
> consolidate.
That's separate because the minimum there starts from slice_height of 96 as indicated in
HDMI spec
Regards,
Suraj Kandpal
>
> > +}
> > +
> > static int intel_dp_dsc_compute_params(struct intel_encoder *encoder,
> > struct intel_crtc_state *crtc_state) { @@
> -1433,17
> > +1449,7 @@ static int intel_dp_dsc_compute_params(struct intel_encoder
> *encoder,
> > vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> > vdsc_cfg->pic_height = crtc_state->hw.adjusted_mode.crtc_vdisplay;
> >
> > - /*
> > - * Slice Height of 8 works for all currently available panels. So start
> > - * with that if pic_height is an integral multiple of 8. Eventually add
> > - * logic to try multiple slice heights.
> > - */
> > - if (vdsc_cfg->pic_height % 8 == 0)
> > - vdsc_cfg->slice_height = 8;
> > - else if (vdsc_cfg->pic_height % 4 == 0)
> > - vdsc_cfg->slice_height = 4;
> > - else
> > - vdsc_cfg->slice_height = 2;
> > + vdsc_cfg->slice_height =
> > +intel_dp_get_slice_height(vdsc_cfg->pic_height);
> >
> > ret = intel_dsc_compute_params(crtc_state);
> > if (ret)
>
> --
> Jani Nikula, Intel Open Source Graphics Center
