>
> On Thu, 02 Feb 2023, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> wrote:
> >>
> >> 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
>
> Do you think it's fine to duplicate a whole function if their sole difference is
> the starting point of a for loop?
>
Well that wont be the only difference after I add the code to fallback to the older dp code going forward
Instead of returning 0 as pointed out by you earlier. If I consolidate this function just for dp and hdmi
There will be a connector type check for those two as dsi and edp have slice_height filled by vbt and this could
look bad by placing it in intel_vdsc.c where I assume we want to keep things agnostic.
Regards,
Suraj Kandpal
> BR,
> Jani.
>
> >
> > 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
>
> --
> Jani Nikula, Intel Open Source Graphics Center
