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, 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...?

> +		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.

Also makes me wonder why we have intel_hdmi_dsc_get_slice_height()
separately, with almost identical implementation. Maybe we should
consolidate.

> +}
> +
>  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