Re: [PATCH v2 16/23] drm/i915/dsc: Compute Rate Control parameters for DSC

[Manasi Navare <manasi.d.navare@intel.com>]

Gaurav,

I needed to make following changes to teh RC parameter values during testing
to get this to work:

On Tue, Jul 31, 2018 at 02:07:12PM -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.
> 
> 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>
> ---
>  drivers/gpu/drm/i915/intel_vdsc.c | 129 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> index ecd270c..23ba083 100644
> --- a/drivers/gpu/drm/i915/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> @@ -335,6 +335,132 @@ 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;
> +
> +	/* RC_MODEL_SIZE is a constant across all configurations */
> +	vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
> +	/* Number of groups used to code each line of a slice */
> +	groups_per_line = DIV_ROUND_UP(vdsc_cfg->slice_width,
> +				       DP_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);
                                                                          ^^8 * 16
This is what you had in the initial patches and thats the value that works. The DSC
spec actually has it as just 8, could you tell how you got the 8*16 in your original
patches?
					   
> +
> +	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 /
> +			(8 * vdsc_cfg->initial_scale_value - 8);

                        ^^vdsc_cfg->initial_scale_value - 8
This is how it was set in your original patches.
              
> +	else
> +		vdsc_cfg->scale_decrement_interval =
> +					DP_DSC_SCALE_DECREMENT_INTERVAL_MAX;
> +
> +	vdsc_cfg->final_offset = vdsc_cfg->rc_model_size -
> +		(vdsc_cfg->initial_xmit_delay *
> +		 vdsc_cfg->bits_per_pixel) + num_extra_mux_bits;

          ^^^^^vdsc_cfg->bits_per_pixel + 8) / 16 + num_extra_mux_bits;
Which was in your original patch.

Could you change these back to the original patch values and have a comment
about how they are obtained.

Manasi

> +
> +	if (vdsc_cfg->final_offset >= vdsc_cfg->rc_model_size) {
> +		DRM_ERROR("FinalOfs < RcModelSze for this InitialXmitDelay\n");
> +		return -1;
> +	}
> +
> +	final_scale = (vdsc_cfg->rc_model_size << 3) /
> +		(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_ERROR("NflBpgOffset is too large for this slice height\n");
> +		return -1;
> +	}
> +
> +	/* 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 > 0x9) {
> +		/*
> +		 * 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_ERROR("ScaleIncrementInterval is large for slice height\n");
> +		return -1;
> +	}
> +
> +	/*
> +	 * 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 +577,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 -1;
> +
>  	return 0;
>  }
> -- 
> 2.7.4
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx