[Freedreno] [PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations

[Jessica Zhang]

On 5/24/2023 12:05 PM, Marijn Suijten wrote:
> On 2023-05-24 10:45:14, Jessica Zhang wrote:
>> Add helpers to calculate det_thresh_flatness and initial_scale_value as
>> these calculations are defined within the DSC spec.
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten at somainline.org>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>> ---
>>   drivers/gpu/drm/display/drm_dsc_helper.c | 24 ++++++++++++++++++++++++
>>   include/drm/display/drm_dsc_helper.h     |  2 ++
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
>> index fc187a8d8873..4efb6236d22c 100644
>> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>> @@ -1413,3 +1413,27 @@ int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg)
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL(drm_dsc_compute_rc_parameters);
>> +
>> +/**
>> + * drm_dsc_initial_scale_value() - Calculate the initial scale value for the given DSC config
>> + * @dsc: Pointer to DRM DSC config struct
>> + *
>> + * Return: Calculated initial scale value
> 
> Perhaps just drop Calculated from Return:?
> 
>> + */
>> +u8 drm_dsc_initial_scale_value(const struct drm_dsc_config *dsc)
>> +{
>> +	return 8 * dsc->rc_model_size / (dsc->rc_model_size - dsc->initial_offset);
>> +}
>> +EXPORT_SYMBOL(drm_dsc_initial_scale_value);
>> +
>> +/**
>> + * drm_dsc_flatness_det_thresh() - Calculate the flatness_det_thresh for the given DSC config
> 
> You've written out the word ("flatness det thresh" and "initial scale
> value") entirely elsewhere, why the underscores in the doc comment here?
> 
> Instead we should have the full meaning here (and in the Return: below),
> please correct me if I'm wrong but in VESA DSC v1.2a spec 6.8.5.1
> Encoder Flatness Decision I think this variable means "flatness
> determination threshold"?  If so, use that in the doc comment :)
> 
> (and drop the leading "the", so just "Calculate flatness determination
> threshold for the given DSC config")
> 
>> + * @dsc: Pointer to DRM DSC config struct
>> + *
>> + * Return: Calculated flatness det thresh value
> 
> Nit: perhaps we can just drop "calculated" here?


Hi Marijn,

Sure, I will make these changes if a v15 is necessary.

In the future, can we try to group comments on wording/grammar/patch 
formatting with comments on the code itself?

I really appreciate your feedback and help in improving the 
documentation around this feature, however I don't find it very 
productive to have revisions where the only changes are on (in my 
opinion) small wording details.

Thanks,

Jessica Zhang

> 
> - Marijn
> 
>> + */
>> +u32 drm_dsc_flatness_det_thresh(const struct drm_dsc_config *dsc)
>> +{
>> +	return 2 << (dsc->bits_per_component - 8);
>> +}
>> +EXPORT_SYMBOL(drm_dsc_flatness_det_thresh);
>> diff --git a/include/drm/display/drm_dsc_helper.h b/include/drm/display/drm_dsc_helper.h
>> index fc2104415dcb..71789fb34e17 100644
>> --- a/include/drm/display/drm_dsc_helper.h
>> +++ b/include/drm/display/drm_dsc_helper.h
>> @@ -24,6 +24,8 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_sdp,
>>   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, enum drm_dsc_params_type type);
>>   int drm_dsc_compute_rc_parameters(struct drm_dsc_config *vdsc_cfg);
>> +u8 drm_dsc_initial_scale_value(const struct drm_dsc_config *dsc);
>> +u32 drm_dsc_flatness_det_thresh(const struct drm_dsc_config *dsc);
>>   
>>   #endif /* _DRM_DSC_HELPER_H_ */
>>   
>>
>> -- 
>> 2.40.1
>>