diff mbox series

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

Message ID 20230329-rfc-msm-dsc-helper-v14-1-bafc7be95691@quicinc.com (mailing list archive)
State Not Applicable
Headers show
Series Introduce MSM-specific DSC helpers | expand

Commit Message

Jessica Zhang May 24, 2023, 5:45 p.m. UTC 
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@somainline.org>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Jessica Zhang <quic_jesszhan@quicinc.com>
---
 drivers/gpu/drm/display/drm_dsc_helper.c | 24 ++++++++++++++++++++++++
 include/drm/display/drm_dsc_helper.h     |  2 ++
 2 files changed, 26 insertions(+)

Comments

Marijn Suijten May 24, 2023, 7:05 p.m. UTC | #1 
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@somainline.org>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Jessica Zhang <quic_jesszhan@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?

- 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
>
Jessica Zhang May 25, 2023, 1:05 a.m. UTC | #2 
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@somainline.org>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@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
>>
Marijn Suijten May 25, 2023, 8:33 p.m. UTC | #3 
On 2023-05-24 18:05:51, Jessica Zhang wrote:
<snip>
> >> +/**
> >> + * 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?

Can you clarify what you mean?  v14 here is the first series including
this doc comment so there was no way for me to have reviewed this
earlier.  Code contents were already successfully reviewed many
revisions ago.

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

It is also down to you to have some patience and collect more review
from other maintainers and batch up changes, instead of spinning another
revision quickly after a review comment.

But this request can also be turned around: review and scan your own
series for simple inconsistencies before sending it to the lists, that
will surely make the time spent by reviewers much more "productive" as
well.
(Note that this goes hand in hand with the request to slow down
 consecutive revisions!)

And finally, as already said before: you can always decide to ignore my
review nits.  I am not a maintainer and don't have final say on whatever
is blocking for a patch to get merged.
But, when another revision is needed, the things I pointed out can at
least be incorporated, which is why they were shared in the first place.

Thanks for understanding.

- Marijn
diff mbox series

Patch

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
+ */
+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
+ * @dsc: Pointer to DRM DSC config struct
+ *
+ * Return: Calculated flatness det thresh value
+ */
+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_ */