diff mbox series

[v8,10/19] drm/i915/dsc: Compute Rate Control parameters for DSC

Message ID 20181102213138.301-11-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series DSC enabling remaining patches respin | expand

Commit Message

Navare, Manasi Nov. 2, 2018, 9:31 p.m. UTC 
From: Gaurav K Singh <gaurav.k.singh@intel.com>

This computation of RC params happens in the atomic commit phase
during compute_config() to validate if display stream compression
can be enabled for the requested mode.

v7 (From Manasi):
* Use DRM_DEBUG instead of DRM_ERROR (Ville)
* Use Error numberinstead of -1 (Ville)
v6 (From Manasi):
* Use 9 instead of 0x9 for consistency (Anusha)

v5 (From Manasi):
* Fix dim checkpatch warnings/checks
v4(From Gaurav):
* No change.Rebase on drm-tip

v3 (From Gaurav):
* Rebase on top of Manasi's latest series
* Return -ve value in case of failure scenarios (Manasi)

Fix review comments from Ville:
* Remove unnecessary comments
* Remove unnecessary paranthesis
* Add comments for few RC params calculations

v2 (From Manasi):
* Rebase Gaurav's patch from intel-gfx to gfx-internal
* Use struct drm_dsc_cfg instead of struct intel_dp
as a parameter

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

Comments

Ville Syrjala Nov. 6, 2018, 2:33 p.m. UTC | #1 
On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> From: Gaurav K Singh <gaurav.k.singh@intel.com>
> 
> This computation of RC params happens in the atomic commit phase
> during compute_config() to validate if display stream compression
> can be enabled for the requested mode.
> 
> v7 (From Manasi):
> * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> * Use Error numberinstead of -1 (Ville)
> v6 (From Manasi):
> * Use 9 instead of 0x9 for consistency (Anusha)
> 
> v5 (From Manasi):
> * Fix dim checkpatch warnings/checks
> v4(From Gaurav):
> * No change.Rebase on drm-tip
> 
> v3 (From Gaurav):
> * Rebase on top of Manasi's latest series
> * Return -ve value in case of failure scenarios (Manasi)
> 
> Fix review comments from Ville:
> * Remove unnecessary comments
> * Remove unnecessary paranthesis
> * Add comments for few RC params calculations
> 
> v2 (From Manasi):
> * Rebase Gaurav's patch from intel-gfx to gfx-internal
> * Use struct drm_dsc_cfg instead of struct intel_dp
> as a parameter
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> index 0a1918f2f643..a76f78b9c0ee 100644
> --- a/drivers/gpu/drm/i915/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8 bits_per_component)
>  	}
>  }
>  
> +static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> +{
> +	unsigned long groups_per_line = 0;
> +	unsigned long groups_total = 0;
> +	unsigned long num_extra_mux_bits = 0;
> +	unsigned long slice_bits = 0;
> +	unsigned long hrd_delay = 0;
> +	unsigned long final_scale = 0;
> +	unsigned long rbs_min = 0;
> +
> +	/* Number of groups used to code each line of a slice */
> +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> +				       DSC_RC_PIXELS_PER_GROUP);
> +
> +	/* chunksize in Bytes */
> +	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> +						  vdsc_cfg->bits_per_pixel,
> +						  (8 * 16));
> +
> +	if (vdsc_cfg->convert_rgb)
> +		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> +					  (4 * vdsc_cfg->bits_per_component + 4)
> +					  - 2);
> +	else
> +		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> +			(4 * vdsc_cfg->bits_per_component + 4) +
> +			2 * (4 * vdsc_cfg->bits_per_component) - 2;
> +	/* Number of bits in one Slice */
> +	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> +
> +	while ((num_extra_mux_bits > 0) &&
> +	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> +		num_extra_mux_bits--;
> +
> +	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> +		vdsc_cfg->initial_scale_value = groups_per_line + 8;
> +
> +	/* scale_decrement_interval calculation according to DSC spec 1.11 */
> +	if (vdsc_cfg->initial_scale_value > 8)
> +		vdsc_cfg->scale_decrement_interval = groups_per_line /
> +			(vdsc_cfg->initial_scale_value - 8);
> +	else
> +		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
> +
> +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> +		(vdsc_cfg->initial_xmit_delay *
> +		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> +
> +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> +		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> +		return -EINVAL;
> +	}
> +
> +	final_scale = (vdsc_cfg->rc_model_size * 8) /
> +		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> +	if (vdsc_cfg->slice_height > 1)
> +		/*
> +		 * NflBpgOffset is 16 bit value with 11 fractional bits
> +		 * hence we multiply by 2^11 for preserving the
> +		 * fractional part
> +		 */
> +		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> +							(vdsc_cfg->slice_height - 1));
> +	else
> +		vdsc_cfg->nfl_bpg_offset = 0;
> +
> +	/* 2^16 - 1 */
> +	if (vdsc_cfg->nfl_bpg_offset > 65535) {
> +		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Number of groups used to code the entire slice */
> +	groups_total = groups_per_line * vdsc_cfg->slice_height;
> +
> +	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
> +	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
> +						    vdsc_cfg->initial_offset +
> +						    num_extra_mux_bits) << 11),
> +						  groups_total);
> +
> +	if (final_scale > 9) {
> +		/*
> +		 * ScaleIncrementInterval =
> +		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
> +		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
> +		 * we need divide by 2^11 from pstDscCfg values
> +		 */
> +		vdsc_cfg->scale_increment_interval =
> +				(vdsc_cfg->final_offset * (1 << 11)) /
> +				((vdsc_cfg->nfl_bpg_offset +
> +				vdsc_cfg->slice_bpg_offset)*
> +				(final_scale - 9));
> +	} else {
> +		/*
> +		 * If finalScaleValue is less than or equal to 9, a value of 0 should
> +		 * be used to disable the scale increment at the end of the slice
> +		 */
> +		vdsc_cfg->scale_increment_interval = 0;
> +	}
> +
> +	if (vdsc_cfg->scale_increment_interval > 65535) {
> +		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * DSC spec mentions that bits_per_pixel specifies the target
> +	 * bits/pixel (bpp) rate that is used by the encoder,
> +	 * in steps of 1/16 of a bit per pixel
> +	 */
> +	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
> +		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
> +			     vdsc_cfg->bits_per_pixel, 16) +
> +		groups_per_line * vdsc_cfg->first_line_bpg_offset;
> +
> +	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
> +	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
> +	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
> +
> +	return 0;
> +}
> +
> +
>  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
>  				struct intel_crtc_state *pipe_config)
>  {
> @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
>  	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
>  		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
>  
> +	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
> +		return -EINVAL;

One should pretty much always pass on the errno rather than inventing a
new one on the spot.

> +
>  	return 0;
>  }
> -- 
> 2.18.0
Navare, Manasi Nov. 6, 2018, 4:52 p.m. UTC | #2 
On Tue, Nov 06, 2018 at 04:33:38PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> > From: Gaurav K Singh <gaurav.k.singh@intel.com>
> > 
> > This computation of RC params happens in the atomic commit phase
> > during compute_config() to validate if display stream compression
> > can be enabled for the requested mode.
> > 
> > v7 (From Manasi):
> > * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> > * Use Error numberinstead of -1 (Ville)
> > v6 (From Manasi):
> > * Use 9 instead of 0x9 for consistency (Anusha)
> > 
> > v5 (From Manasi):
> > * Fix dim checkpatch warnings/checks
> > v4(From Gaurav):
> > * No change.Rebase on drm-tip
> > 
> > v3 (From Gaurav):
> > * Rebase on top of Manasi's latest series
> > * Return -ve value in case of failure scenarios (Manasi)
> > 
> > Fix review comments from Ville:
> > * Remove unnecessary comments
> > * Remove unnecessary paranthesis
> > * Add comments for few RC params calculations
> > 
> > v2 (From Manasi):
> > * Rebase Gaurav's patch from intel-gfx to gfx-internal
> > * Use struct drm_dsc_cfg instead of struct intel_dp
> > as a parameter
> > 
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
> >  1 file changed, 127 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> > index 0a1918f2f643..a76f78b9c0ee 100644
> > --- a/drivers/gpu/drm/i915/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> > @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8 bits_per_component)
> >  	}
> >  }
> >  
> > +static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> > +{
> > +	unsigned long groups_per_line = 0;
> > +	unsigned long groups_total = 0;
> > +	unsigned long num_extra_mux_bits = 0;
> > +	unsigned long slice_bits = 0;
> > +	unsigned long hrd_delay = 0;
> > +	unsigned long final_scale = 0;
> > +	unsigned long rbs_min = 0;
> > +
> > +	/* Number of groups used to code each line of a slice */
> > +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> > +				       DSC_RC_PIXELS_PER_GROUP);
> > +
> > +	/* chunksize in Bytes */
> > +	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> > +						  vdsc_cfg->bits_per_pixel,
> > +						  (8 * 16));
> > +
> > +	if (vdsc_cfg->convert_rgb)
> > +		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> > +					  (4 * vdsc_cfg->bits_per_component + 4)
> > +					  - 2);
> > +	else
> > +		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> > +			(4 * vdsc_cfg->bits_per_component + 4) +
> > +			2 * (4 * vdsc_cfg->bits_per_component) - 2;
> > +	/* Number of bits in one Slice */
> > +	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> > +
> > +	while ((num_extra_mux_bits > 0) &&
> > +	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> > +		num_extra_mux_bits--;
> > +
> > +	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> > +		vdsc_cfg->initial_scale_value = groups_per_line + 8;
> > +
> > +	/* scale_decrement_interval calculation according to DSC spec 1.11 */
> > +	if (vdsc_cfg->initial_scale_value > 8)
> > +		vdsc_cfg->scale_decrement_interval = groups_per_line /
> > +			(vdsc_cfg->initial_scale_value - 8);
> > +	else
> > +		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
> > +
> > +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> > +		(vdsc_cfg->initial_xmit_delay *
> > +		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> > +
> > +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> > +		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	final_scale = (vdsc_cfg->rc_model_size * 8) /
> > +		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> > +	if (vdsc_cfg->slice_height > 1)
> > +		/*
> > +		 * NflBpgOffset is 16 bit value with 11 fractional bits
> > +		 * hence we multiply by 2^11 for preserving the
> > +		 * fractional part
> > +		 */
> > +		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> > +							(vdsc_cfg->slice_height - 1));
> > +	else
> > +		vdsc_cfg->nfl_bpg_offset = 0;
> > +
> > +	/* 2^16 - 1 */
> > +	if (vdsc_cfg->nfl_bpg_offset > 65535) {
> > +		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Number of groups used to code the entire slice */
> > +	groups_total = groups_per_line * vdsc_cfg->slice_height;
> > +
> > +	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
> > +	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
> > +						    vdsc_cfg->initial_offset +
> > +						    num_extra_mux_bits) << 11),
> > +						  groups_total);
> > +
> > +	if (final_scale > 9) {
> > +		/*
> > +		 * ScaleIncrementInterval =
> > +		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
> > +		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
> > +		 * we need divide by 2^11 from pstDscCfg values
> > +		 */
> > +		vdsc_cfg->scale_increment_interval =
> > +				(vdsc_cfg->final_offset * (1 << 11)) /
> > +				((vdsc_cfg->nfl_bpg_offset +
> > +				vdsc_cfg->slice_bpg_offset)*
> > +				(final_scale - 9));
> > +	} else {
> > +		/*
> > +		 * If finalScaleValue is less than or equal to 9, a value of 0 should
> > +		 * be used to disable the scale increment at the end of the slice
> > +		 */
> > +		vdsc_cfg->scale_increment_interval = 0;
> > +	}
> > +
> > +	if (vdsc_cfg->scale_increment_interval > 65535) {
> > +		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/*
> > +	 * DSC spec mentions that bits_per_pixel specifies the target
> > +	 * bits/pixel (bpp) rate that is used by the encoder,
> > +	 * in steps of 1/16 of a bit per pixel
> > +	 */
> > +	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
> > +		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
> > +			     vdsc_cfg->bits_per_pixel, 16) +
> > +		groups_per_line * vdsc_cfg->first_line_bpg_offset;
> > +
> > +	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
> > +	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
> > +	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
> > +
> > +	return 0;
> > +}
> > +
> > +
> >  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> >  				struct intel_crtc_state *pipe_config)
> >  {
> > @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> >  	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
> >  		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
> >  
> > +	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
> > +		return -EINVAL;
> 
> One should pretty much always pass on the errno rather than inventing a
> new one on the spot.

But compute_rc_parameters() function returns EINVAL everywhere for error cases

Manasi

> 
> > +
> >  	return 0;
> >  }
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala Nov. 6, 2018, 5 p.m. UTC | #3 
On Tue, Nov 06, 2018 at 08:52:41AM -0800, Manasi Navare wrote:
> On Tue, Nov 06, 2018 at 04:33:38PM +0200, Ville Syrjälä wrote:
> > On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> > > From: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > 
> > > This computation of RC params happens in the atomic commit phase
> > > during compute_config() to validate if display stream compression
> > > can be enabled for the requested mode.
> > > 
> > > v7 (From Manasi):
> > > * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> > > * Use Error numberinstead of -1 (Ville)
> > > v6 (From Manasi):
> > > * Use 9 instead of 0x9 for consistency (Anusha)
> > > 
> > > v5 (From Manasi):
> > > * Fix dim checkpatch warnings/checks
> > > v4(From Gaurav):
> > > * No change.Rebase on drm-tip
> > > 
> > > v3 (From Gaurav):
> > > * Rebase on top of Manasi's latest series
> > > * Return -ve value in case of failure scenarios (Manasi)
> > > 
> > > Fix review comments from Ville:
> > > * Remove unnecessary comments
> > > * Remove unnecessary paranthesis
> > > * Add comments for few RC params calculations
> > > 
> > > v2 (From Manasi):
> > > * Rebase Gaurav's patch from intel-gfx to gfx-internal
> > > * Use struct drm_dsc_cfg instead of struct intel_dp
> > > as a parameter
> > > 
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
> > >  1 file changed, 127 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> > > index 0a1918f2f643..a76f78b9c0ee 100644
> > > --- a/drivers/gpu/drm/i915/intel_vdsc.c
> > > +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> > > @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8 bits_per_component)
> > >  	}
> > >  }
> > >  
> > > +static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> > > +{
> > > +	unsigned long groups_per_line = 0;
> > > +	unsigned long groups_total = 0;
> > > +	unsigned long num_extra_mux_bits = 0;
> > > +	unsigned long slice_bits = 0;
> > > +	unsigned long hrd_delay = 0;
> > > +	unsigned long final_scale = 0;
> > > +	unsigned long rbs_min = 0;
> > > +
> > > +	/* Number of groups used to code each line of a slice */
> > > +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> > > +				       DSC_RC_PIXELS_PER_GROUP);
> > > +
> > > +	/* chunksize in Bytes */
> > > +	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> > > +						  vdsc_cfg->bits_per_pixel,
> > > +						  (8 * 16));
> > > +
> > > +	if (vdsc_cfg->convert_rgb)
> > > +		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> > > +					  (4 * vdsc_cfg->bits_per_component + 4)
> > > +					  - 2);
> > > +	else
> > > +		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> > > +			(4 * vdsc_cfg->bits_per_component + 4) +
> > > +			2 * (4 * vdsc_cfg->bits_per_component) - 2;
> > > +	/* Number of bits in one Slice */
> > > +	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> > > +
> > > +	while ((num_extra_mux_bits > 0) &&
> > > +	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> > > +		num_extra_mux_bits--;
> > > +
> > > +	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> > > +		vdsc_cfg->initial_scale_value = groups_per_line + 8;
> > > +
> > > +	/* scale_decrement_interval calculation according to DSC spec 1.11 */
> > > +	if (vdsc_cfg->initial_scale_value > 8)
> > > +		vdsc_cfg->scale_decrement_interval = groups_per_line /
> > > +			(vdsc_cfg->initial_scale_value - 8);
> > > +	else
> > > +		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
> > > +
> > > +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> > > +		(vdsc_cfg->initial_xmit_delay *
> > > +		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> > > +
> > > +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> > > +		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	final_scale = (vdsc_cfg->rc_model_size * 8) /
> > > +		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> > > +	if (vdsc_cfg->slice_height > 1)
> > > +		/*
> > > +		 * NflBpgOffset is 16 bit value with 11 fractional bits
> > > +		 * hence we multiply by 2^11 for preserving the
> > > +		 * fractional part
> > > +		 */
> > > +		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> > > +							(vdsc_cfg->slice_height - 1));
> > > +	else
> > > +		vdsc_cfg->nfl_bpg_offset = 0;
> > > +
> > > +	/* 2^16 - 1 */
> > > +	if (vdsc_cfg->nfl_bpg_offset > 65535) {
> > > +		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Number of groups used to code the entire slice */
> > > +	groups_total = groups_per_line * vdsc_cfg->slice_height;
> > > +
> > > +	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
> > > +	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
> > > +						    vdsc_cfg->initial_offset +
> > > +						    num_extra_mux_bits) << 11),
> > > +						  groups_total);
> > > +
> > > +	if (final_scale > 9) {
> > > +		/*
> > > +		 * ScaleIncrementInterval =
> > > +		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
> > > +		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
> > > +		 * we need divide by 2^11 from pstDscCfg values
> > > +		 */
> > > +		vdsc_cfg->scale_increment_interval =
> > > +				(vdsc_cfg->final_offset * (1 << 11)) /
> > > +				((vdsc_cfg->nfl_bpg_offset +
> > > +				vdsc_cfg->slice_bpg_offset)*
> > > +				(final_scale - 9));
> > > +	} else {
> > > +		/*
> > > +		 * If finalScaleValue is less than or equal to 9, a value of 0 should
> > > +		 * be used to disable the scale increment at the end of the slice
> > > +		 */
> > > +		vdsc_cfg->scale_increment_interval = 0;
> > > +	}
> > > +
> > > +	if (vdsc_cfg->scale_increment_interval > 65535) {
> > > +		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/*
> > > +	 * DSC spec mentions that bits_per_pixel specifies the target
> > > +	 * bits/pixel (bpp) rate that is used by the encoder,
> > > +	 * in steps of 1/16 of a bit per pixel
> > > +	 */
> > > +	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
> > > +		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
> > > +			     vdsc_cfg->bits_per_pixel, 16) +
> > > +		groups_per_line * vdsc_cfg->first_line_bpg_offset;
> > > +
> > > +	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
> > > +	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
> > > +	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +
> > >  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > >  				struct intel_crtc_state *pipe_config)
> > >  {
> > > @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > >  	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
> > >  		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
> > >  
> > > +	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
> > > +		return -EINVAL;
> > 
> > One should pretty much always pass on the errno rather than inventing a
> > new one on the spot.
> 
> But compute_rc_parameters() function returns EINVAL everywhere for error cases

Why should I have to know that?
Navare, Manasi Nov. 6, 2018, 8:14 p.m. UTC | #4 
On Tue, Nov 06, 2018 at 07:00:50PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 06, 2018 at 08:52:41AM -0800, Manasi Navare wrote:
> > On Tue, Nov 06, 2018 at 04:33:38PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> > > > From: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > 
> > > > This computation of RC params happens in the atomic commit phase
> > > > during compute_config() to validate if display stream compression
> > > > can be enabled for the requested mode.
> > > > 
> > > > v7 (From Manasi):
> > > > * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> > > > * Use Error numberinstead of -1 (Ville)
> > > > v6 (From Manasi):
> > > > * Use 9 instead of 0x9 for consistency (Anusha)
> > > > 
> > > > v5 (From Manasi):
> > > > * Fix dim checkpatch warnings/checks
> > > > v4(From Gaurav):
> > > > * No change.Rebase on drm-tip
> > > > 
> > > > v3 (From Gaurav):
> > > > * Rebase on top of Manasi's latest series
> > > > * Return -ve value in case of failure scenarios (Manasi)
> > > > 
> > > > Fix review comments from Ville:
> > > > * Remove unnecessary comments
> > > > * Remove unnecessary paranthesis
> > > > * Add comments for few RC params calculations
> > > > 
> > > > v2 (From Manasi):
> > > > * Rebase Gaurav's patch from intel-gfx to gfx-internal
> > > > * Use struct drm_dsc_cfg instead of struct intel_dp
> > > > as a parameter
> > > > 
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
> > > >  1 file changed, 127 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> > > > index 0a1918f2f643..a76f78b9c0ee 100644
> > > > --- a/drivers/gpu/drm/i915/intel_vdsc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> > > > @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8 bits_per_component)
> > > >  	}
> > > >  }
> > > >  
> > > > +static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> > > > +{
> > > > +	unsigned long groups_per_line = 0;
> > > > +	unsigned long groups_total = 0;
> > > > +	unsigned long num_extra_mux_bits = 0;
> > > > +	unsigned long slice_bits = 0;
> > > > +	unsigned long hrd_delay = 0;
> > > > +	unsigned long final_scale = 0;
> > > > +	unsigned long rbs_min = 0;
> > > > +
> > > > +	/* Number of groups used to code each line of a slice */
> > > > +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> > > > +				       DSC_RC_PIXELS_PER_GROUP);
> > > > +
> > > > +	/* chunksize in Bytes */
> > > > +	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> > > > +						  vdsc_cfg->bits_per_pixel,
> > > > +						  (8 * 16));
> > > > +
> > > > +	if (vdsc_cfg->convert_rgb)
> > > > +		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> > > > +					  (4 * vdsc_cfg->bits_per_component + 4)
> > > > +					  - 2);
> > > > +	else
> > > > +		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> > > > +			(4 * vdsc_cfg->bits_per_component + 4) +
> > > > +			2 * (4 * vdsc_cfg->bits_per_component) - 2;
> > > > +	/* Number of bits in one Slice */
> > > > +	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> > > > +
> > > > +	while ((num_extra_mux_bits > 0) &&
> > > > +	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> > > > +		num_extra_mux_bits--;
> > > > +
> > > > +	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> > > > +		vdsc_cfg->initial_scale_value = groups_per_line + 8;
> > > > +
> > > > +	/* scale_decrement_interval calculation according to DSC spec 1.11 */
> > > > +	if (vdsc_cfg->initial_scale_value > 8)
> > > > +		vdsc_cfg->scale_decrement_interval = groups_per_line /
> > > > +			(vdsc_cfg->initial_scale_value - 8);
> > > > +	else
> > > > +		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
> > > > +
> > > > +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> > > > +		(vdsc_cfg->initial_xmit_delay *
> > > > +		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> > > > +
> > > > +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> > > > +		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	final_scale = (vdsc_cfg->rc_model_size * 8) /
> > > > +		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> > > > +	if (vdsc_cfg->slice_height > 1)
> > > > +		/*
> > > > +		 * NflBpgOffset is 16 bit value with 11 fractional bits
> > > > +		 * hence we multiply by 2^11 for preserving the
> > > > +		 * fractional part
> > > > +		 */
> > > > +		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> > > > +							(vdsc_cfg->slice_height - 1));
> > > > +	else
> > > > +		vdsc_cfg->nfl_bpg_offset = 0;
> > > > +
> > > > +	/* 2^16 - 1 */
> > > > +	if (vdsc_cfg->nfl_bpg_offset > 65535) {
> > > > +		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/* Number of groups used to code the entire slice */
> > > > +	groups_total = groups_per_line * vdsc_cfg->slice_height;
> > > > +
> > > > +	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
> > > > +	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
> > > > +						    vdsc_cfg->initial_offset +
> > > > +						    num_extra_mux_bits) << 11),
> > > > +						  groups_total);
> > > > +
> > > > +	if (final_scale > 9) {
> > > > +		/*
> > > > +		 * ScaleIncrementInterval =
> > > > +		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
> > > > +		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
> > > > +		 * we need divide by 2^11 from pstDscCfg values
> > > > +		 */
> > > > +		vdsc_cfg->scale_increment_interval =
> > > > +				(vdsc_cfg->final_offset * (1 << 11)) /
> > > > +				((vdsc_cfg->nfl_bpg_offset +
> > > > +				vdsc_cfg->slice_bpg_offset)*
> > > > +				(final_scale - 9));
> > > > +	} else {
> > > > +		/*
> > > > +		 * If finalScaleValue is less than or equal to 9, a value of 0 should
> > > > +		 * be used to disable the scale increment at the end of the slice
> > > > +		 */
> > > > +		vdsc_cfg->scale_increment_interval = 0;
> > > > +	}
> > > > +
> > > > +	if (vdsc_cfg->scale_increment_interval > 65535) {
> > > > +		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
> > > > +		return -EINVAL;
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * DSC spec mentions that bits_per_pixel specifies the target
> > > > +	 * bits/pixel (bpp) rate that is used by the encoder,
> > > > +	 * in steps of 1/16 of a bit per pixel
> > > > +	 */
> > > > +	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
> > > > +		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
> > > > +			     vdsc_cfg->bits_per_pixel, 16) +
> > > > +		groups_per_line * vdsc_cfg->first_line_bpg_offset;
> > > > +
> > > > +	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
> > > > +	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
> > > > +	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +
> > > >  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > > >  				struct intel_crtc_state *pipe_config)
> > > >  {
> > > > @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > > >  	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
> > > >  		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
> > > >  
> > > > +	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
> > > > +		return -EINVAL;
> > > 
> > > One should pretty much always pass on the errno rather than inventing a
> > > new one on the spot.
> > 
> > But compute_rc_parameters() function returns EINVAL everywhere for error cases
> 
> Why should I have to know that?

After doing errno --list and looking at the available errnos, the most logical ones to return
from intel_compute_rc_parameters is ERANGE since the errors are because the parameters of out of valid range.
And here I can just do return intel_compute_rc_parameters()

Does this look good?

Manasi

> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjala Nov. 6, 2018, 8:30 p.m. UTC | #5 
On Tue, Nov 06, 2018 at 12:14:50PM -0800, Manasi Navare wrote:
> On Tue, Nov 06, 2018 at 07:00:50PM +0200, Ville Syrjälä wrote:
> > On Tue, Nov 06, 2018 at 08:52:41AM -0800, Manasi Navare wrote:
> > > On Tue, Nov 06, 2018 at 04:33:38PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 02, 2018 at 02:31:29PM -0700, Manasi Navare wrote:
> > > > > From: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > > 
> > > > > This computation of RC params happens in the atomic commit phase
> > > > > during compute_config() to validate if display stream compression
> > > > > can be enabled for the requested mode.
> > > > > 
> > > > > v7 (From Manasi):
> > > > > * Use DRM_DEBUG instead of DRM_ERROR (Ville)
> > > > > * Use Error numberinstead of -1 (Ville)
> > > > > v6 (From Manasi):
> > > > > * Use 9 instead of 0x9 for consistency (Anusha)
> > > > > 
> > > > > v5 (From Manasi):
> > > > > * Fix dim checkpatch warnings/checks
> > > > > v4(From Gaurav):
> > > > > * No change.Rebase on drm-tip
> > > > > 
> > > > > v3 (From Gaurav):
> > > > > * Rebase on top of Manasi's latest series
> > > > > * Return -ve value in case of failure scenarios (Manasi)
> > > > > 
> > > > > Fix review comments from Ville:
> > > > > * Remove unnecessary comments
> > > > > * Remove unnecessary paranthesis
> > > > > * Add comments for few RC params calculations
> > > > > 
> > > > > v2 (From Manasi):
> > > > > * Rebase Gaurav's patch from intel-gfx to gfx-internal
> > > > > * Use struct drm_dsc_cfg instead of struct intel_dp
> > > > > as a parameter
> > > > > 
> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_vdsc.c | 127 ++++++++++++++++++++++++++++++
> > > > >  1 file changed, 127 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> > > > > index 0a1918f2f643..a76f78b9c0ee 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_vdsc.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> > > > > @@ -317,6 +317,130 @@ static int get_column_index_for_rc_params(u8 bits_per_component)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > +static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
> > > > > +{
> > > > > +	unsigned long groups_per_line = 0;
> > > > > +	unsigned long groups_total = 0;
> > > > > +	unsigned long num_extra_mux_bits = 0;
> > > > > +	unsigned long slice_bits = 0;
> > > > > +	unsigned long hrd_delay = 0;
> > > > > +	unsigned long final_scale = 0;
> > > > > +	unsigned long rbs_min = 0;
> > > > > +
> > > > > +	/* Number of groups used to code each line of a slice */
> > > > > +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> > > > > +				       DSC_RC_PIXELS_PER_GROUP);
> > > > > +
> > > > > +	/* chunksize in Bytes */
> > > > > +	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
> > > > > +						  vdsc_cfg->bits_per_pixel,
> > > > > +						  (8 * 16));
> > > > > +
> > > > > +	if (vdsc_cfg->convert_rgb)
> > > > > +		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
> > > > > +					  (4 * vdsc_cfg->bits_per_component + 4)
> > > > > +					  - 2);
> > > > > +	else
> > > > > +		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
> > > > > +			(4 * vdsc_cfg->bits_per_component + 4) +
> > > > > +			2 * (4 * vdsc_cfg->bits_per_component) - 2;
> > > > > +	/* Number of bits in one Slice */
> > > > > +	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
> > > > > +
> > > > > +	while ((num_extra_mux_bits > 0) &&
> > > > > +	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
> > > > > +		num_extra_mux_bits--;
> > > > > +
> > > > > +	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
> > > > > +		vdsc_cfg->initial_scale_value = groups_per_line + 8;
> > > > > +
> > > > > +	/* scale_decrement_interval calculation according to DSC spec 1.11 */
> > > > > +	if (vdsc_cfg->initial_scale_value > 8)
> > > > > +		vdsc_cfg->scale_decrement_interval = groups_per_line /
> > > > > +			(vdsc_cfg->initial_scale_value - 8);
> > > > > +	else
> > > > > +		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
> > > > > +
> > > > > +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> > > > > +		(vdsc_cfg->initial_xmit_delay *
> > > > > +		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
> > > > > +
> > > > > +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> > > > > +		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	final_scale = (vdsc_cfg->rc_model_size * 8) /
> > > > > +		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
> > > > > +	if (vdsc_cfg->slice_height > 1)
> > > > > +		/*
> > > > > +		 * NflBpgOffset is 16 bit value with 11 fractional bits
> > > > > +		 * hence we multiply by 2^11 for preserving the
> > > > > +		 * fractional part
> > > > > +		 */
> > > > > +		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
> > > > > +							(vdsc_cfg->slice_height - 1));
> > > > > +	else
> > > > > +		vdsc_cfg->nfl_bpg_offset = 0;
> > > > > +
> > > > > +	/* 2^16 - 1 */
> > > > > +	if (vdsc_cfg->nfl_bpg_offset > 65535) {
> > > > > +		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	/* Number of groups used to code the entire slice */
> > > > > +	groups_total = groups_per_line * vdsc_cfg->slice_height;
> > > > > +
> > > > > +	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
> > > > > +	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
> > > > > +						    vdsc_cfg->initial_offset +
> > > > > +						    num_extra_mux_bits) << 11),
> > > > > +						  groups_total);
> > > > > +
> > > > > +	if (final_scale > 9) {
> > > > > +		/*
> > > > > +		 * ScaleIncrementInterval =
> > > > > +		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
> > > > > +		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
> > > > > +		 * we need divide by 2^11 from pstDscCfg values
> > > > > +		 */
> > > > > +		vdsc_cfg->scale_increment_interval =
> > > > > +				(vdsc_cfg->final_offset * (1 << 11)) /
> > > > > +				((vdsc_cfg->nfl_bpg_offset +
> > > > > +				vdsc_cfg->slice_bpg_offset)*
> > > > > +				(final_scale - 9));
> > > > > +	} else {
> > > > > +		/*
> > > > > +		 * If finalScaleValue is less than or equal to 9, a value of 0 should
> > > > > +		 * be used to disable the scale increment at the end of the slice
> > > > > +		 */
> > > > > +		vdsc_cfg->scale_increment_interval = 0;
> > > > > +	}
> > > > > +
> > > > > +	if (vdsc_cfg->scale_increment_interval > 65535) {
> > > > > +		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > > +	/*
> > > > > +	 * DSC spec mentions that bits_per_pixel specifies the target
> > > > > +	 * bits/pixel (bpp) rate that is used by the encoder,
> > > > > +	 * in steps of 1/16 of a bit per pixel
> > > > > +	 */
> > > > > +	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
> > > > > +		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
> > > > > +			     vdsc_cfg->bits_per_pixel, 16) +
> > > > > +		groups_per_line * vdsc_cfg->first_line_bpg_offset;
> > > > > +
> > > > > +	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
> > > > > +	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
> > > > > +	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +
> > > > >  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > > > >  				struct intel_crtc_state *pipe_config)
> > > > >  {
> > > > > @@ -451,5 +575,8 @@ int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
> > > > >  	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
> > > > >  		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
> > > > >  
> > > > > +	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
> > > > > +		return -EINVAL;
> > > > 
> > > > One should pretty much always pass on the errno rather than inventing a
> > > > new one on the spot.
> > > 
> > > But compute_rc_parameters() function returns EINVAL everywhere for error cases
> > 
> > Why should I have to know that?
> 
> After doing errno --list and looking at the available errnos, the most logical ones to return
> from intel_compute_rc_parameters is ERANGE since the errors are because the parameters of out of valid range.

I guess it doesn't really matter which one we pick here since
it won't go all the way out to userspace anyway.

> And here I can just do return intel_compute_rc_parameters()

Yes, that seems good.

> 
> Does this look good?
> 
> Manasi
> 
> > 
> > -- 
> > Ville Syrjälä
> > Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
index 0a1918f2f643..a76f78b9c0ee 100644
--- a/drivers/gpu/drm/i915/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/intel_vdsc.c
@@ -317,6 +317,130 @@  static int get_column_index_for_rc_params(u8 bits_per_component)
 	}
 }
 
+static int intel_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
+{
+	unsigned long groups_per_line = 0;
+	unsigned long groups_total = 0;
+	unsigned long num_extra_mux_bits = 0;
+	unsigned long slice_bits = 0;
+	unsigned long hrd_delay = 0;
+	unsigned long final_scale = 0;
+	unsigned long rbs_min = 0;
+
+	/* Number of groups used to code each line of a slice */
+	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
+				       DSC_RC_PIXELS_PER_GROUP);
+
+	/* chunksize in Bytes */
+	vdsc_cfg->slice_chunk_size = DIV_ROUND_UP(vdsc_cfg->slice_width *
+						  vdsc_cfg->bits_per_pixel,
+						  (8 * 16));
+
+	if (vdsc_cfg->convert_rgb)
+		num_extra_mux_bits = 3 * (vdsc_cfg->mux_word_size +
+					  (4 * vdsc_cfg->bits_per_component + 4)
+					  - 2);
+	else
+		num_extra_mux_bits = 3 * vdsc_cfg->mux_word_size +
+			(4 * vdsc_cfg->bits_per_component + 4) +
+			2 * (4 * vdsc_cfg->bits_per_component) - 2;
+	/* Number of bits in one Slice */
+	slice_bits = 8 * vdsc_cfg->slice_chunk_size * vdsc_cfg->slice_height;
+
+	while ((num_extra_mux_bits > 0) &&
+	       ((slice_bits - num_extra_mux_bits) % vdsc_cfg->mux_word_size))
+		num_extra_mux_bits--;
+
+	if (groups_per_line < vdsc_cfg->initial_scale_value - 8)
+		vdsc_cfg->initial_scale_value = groups_per_line + 8;
+
+	/* scale_decrement_interval calculation according to DSC spec 1.11 */
+	if (vdsc_cfg->initial_scale_value > 8)
+		vdsc_cfg->scale_decrement_interval = groups_per_line /
+			(vdsc_cfg->initial_scale_value - 8);
+	else
+		vdsc_cfg->scale_decrement_interval = DSC_SCALE_DECREMENT_INTERVAL_MAX;
+
+	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
+		(vdsc_cfg->initial_xmit_delay *
+		 vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
+
+	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
+		DRM_DEBUG_KMS("FinalOfs < RcModelSze for this InitialXmitDelay\n");
+		return -EINVAL;
+	}
+
+	final_scale = (vdsc_cfg->rc_model_size * 8) /
+		(vdsc_cfg->rc_model_size - vdsc_cfg->final_offset);
+	if (vdsc_cfg->slice_height > 1)
+		/*
+		 * NflBpgOffset is 16 bit value with 11 fractional bits
+		 * hence we multiply by 2^11 for preserving the
+		 * fractional part
+		 */
+		vdsc_cfg->nfl_bpg_offset = DIV_ROUND_UP((vdsc_cfg->first_line_bpg_offset << 11),
+							(vdsc_cfg->slice_height - 1));
+	else
+		vdsc_cfg->nfl_bpg_offset = 0;
+
+	/* 2^16 - 1 */
+	if (vdsc_cfg->nfl_bpg_offset > 65535) {
+		DRM_DEBUG_KMS("NflBpgOffset is too large for this slice height\n");
+		return -EINVAL;
+	}
+
+	/* Number of groups used to code the entire slice */
+	groups_total = groups_per_line * vdsc_cfg->slice_height;
+
+	/* slice_bpg_offset is 16 bit value with 11 fractional bits */
+	vdsc_cfg->slice_bpg_offset = DIV_ROUND_UP(((vdsc_cfg->rc_model_size -
+						    vdsc_cfg->initial_offset +
+						    num_extra_mux_bits) << 11),
+						  groups_total);
+
+	if (final_scale > 9) {
+		/*
+		 * ScaleIncrementInterval =
+		 * finaloffset/((NflBpgOffset + SliceBpgOffset)*8(finalscale - 1.125))
+		 * as (NflBpgOffset + SliceBpgOffset) has 11 bit fractional value,
+		 * we need divide by 2^11 from pstDscCfg values
+		 */
+		vdsc_cfg->scale_increment_interval =
+				(vdsc_cfg->final_offset * (1 << 11)) /
+				((vdsc_cfg->nfl_bpg_offset +
+				vdsc_cfg->slice_bpg_offset)*
+				(final_scale - 9));
+	} else {
+		/*
+		 * If finalScaleValue is less than or equal to 9, a value of 0 should
+		 * be used to disable the scale increment at the end of the slice
+		 */
+		vdsc_cfg->scale_increment_interval = 0;
+	}
+
+	if (vdsc_cfg->scale_increment_interval > 65535) {
+		DRM_DEBUG_KMS("ScaleIncrementInterval is large for slice height\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * DSC spec mentions that bits_per_pixel specifies the target
+	 * bits/pixel (bpp) rate that is used by the encoder,
+	 * in steps of 1/16 of a bit per pixel
+	 */
+	rbs_min = vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset +
+		DIV_ROUND_UP(vdsc_cfg->initial_xmit_delay *
+			     vdsc_cfg->bits_per_pixel, 16) +
+		groups_per_line * vdsc_cfg->first_line_bpg_offset;
+
+	hrd_delay = DIV_ROUND_UP((rbs_min * 16), vdsc_cfg->bits_per_pixel);
+	vdsc_cfg->rc_bits = (hrd_delay * vdsc_cfg->bits_per_pixel) / 16;
+	vdsc_cfg->initial_dec_delay = hrd_delay - vdsc_cfg->initial_xmit_delay;
+
+	return 0;
+}
+
+
 int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config)
 {
@@ -451,5 +575,8 @@  int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,
 	vdsc_cfg->initial_scale_value = (vdsc_cfg->rc_model_size << 3) /
 		(vdsc_cfg->rc_model_size - vdsc_cfg->initial_offset);
 
+	if (intel_compute_rc_parameters(vdsc_cfg) < 0)
+		return -EINVAL;
+
 	return 0;
 }