Re: [Intel-gfx] [PATCH 1/1] drm/i915/dp: Fix logic to fetch slice_height

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 02 Feb 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>> 
>> On Thu, 02 Feb 2023, Suraj Kandpal <suraj.kandpal@intel.com> 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@intel.com>
>> > Cc: Swati Sharma <swati2.sharma@intel.com>
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
>> >
>> > 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?

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