Re: [PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations
On 2023-05-24 18:05:51, Jessica Zhang wrote:
> >> +/**
> >> + * 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
Re: [PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations
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
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
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
Re: [PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations
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
> Reviewed-by: Dmitry Baryshkov
> Signed-off-by: Jessica Zhang
> ---
> 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
>
[PATCH v14 1/9] drm/display/dsc: Add flatness and initial scale value calculations
Add helpers to calculate det_thresh_flatness and initial_scale_value as
these calculations are defined within the DSC spec.
Reviewed-by: Marijn Suijten
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Jessica Zhang
---
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
+ */
+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_ */
--
2.40.1
