[Freedreno] [PATCH v5 6/8] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters

[Dmitry Baryshkov]

On 16/05/2023 21:46, Kandpal, Suraj wrote:
>>
>> The array of rc_parameters contains a mixture of parameters from DSC 1.1
>> and DSC 1.2 standards. Split these tow configuration arrays in preparation to
>> adding more configuration data.
>>
> 
> Hi ,
> Needed to add some more comments apart from the previous ones already given
> 

[skipped]


>> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> index d4340b18c18d..bd9116d2cd76 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
>> @@ -226,7 +226,15 @@ int intel_dsc_compute_params(struct
>> intel_crtc_state *pipe_config)
>>   	if (DISPLAY_VER(dev_priv) >= 13) {
>>   		calculate_rc_params(vdsc_cfg);
>>   	} else {
>> -		ret = drm_dsc_setup_rc_params(vdsc_cfg);
>> +		if ((compressed_bpp == 8 ||
>> +		     compressed_bpp == 12) &&
>> +		    (vdsc_cfg->bits_per_component == 8 ||
>> +		     vdsc_cfg->bits_per_component == 10 ||
>> +		     vdsc_cfg->bits_per_component == 12))
>> +			ret = drm_dsc_setup_rc_params(vdsc_cfg,
>> DRM_DSC_1_1_PRE_SCR);
>> +		else
>> +			ret = drm_dsc_setup_rc_params(vdsc_cfg,
>> DRM_DSC_1_2_444);
>> +
> 
> I do not think this kind of assignment works as you will also be adding
> DRM_DSC_1_2_422 and DRM_DSC_1_2_420 in further patches and AFAICS
> There is no where in patch 8 that you have accounted for when 422 or 420 will be used.
> Maybe you can add an if case inside the else block to check pipe_config->output_format
> to pass the rc_param_data in patch 8

I don't think this is necessary for now. The driver doesn't support YUV 
422. The YUV 420 is supported only for DISPLAY_VER(dev_priv) >= 14, 
however these helpers are only used for DISPLAY_VER(dev_priv) < 13.

I did not move RC calculation to drm_dsc_helpers.c (yet ?), which is 
used for DISPLAY_VER >= 13.

> 
> Regards,
> Suraj Kandpal
>>   		if (ret)
>>   			return ret;
>>
>> diff --git a/include/drm/display/drm_dsc_helper.h
>> b/include/drm/display/drm_dsc_helper.h
>> index 1681791f65a5..c634bb2935d3 100644
>> --- a/include/drm/display/drm_dsc_helper.h
>> +++ b/include/drm/display/drm_dsc_helper.h
>> @@ -10,12 +10,17 @@
>>
>>   #include <drm/display/drm_dsc.h>
>>
>> +enum drm_dsc_params_kind {
>> +	DRM_DSC_1_2_444,
>> +	DRM_DSC_1_1_PRE_SCR, /* legacy params from DSC 1.1 */ };
>> +
>>   void drm_dsc_dp_pps_header_init(struct dp_sdp_header *pps_header);  int
>> drm_dsc_dp_rc_buffer_size(u8 rc_buffer_block_size, u8 rc_buffer_size);
>> void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set
>> *pps_sdp,
>>   			      const struct drm_dsc_config *dsc_cfg);  void
>> drm_dsc_set_rc_buf_thresh(struct drm_dsc_config *vdsc_cfg); -int
>> drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg);
>> +int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg, enum
>> +drm_dsc_params_kind kind);
>>   int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>>
>>   #endif /* _DRM_DSC_HELPER_H_ */
>> --
>> 2.39.2
> 

-- 
With best wishes
Dmitry