From: Jani Nikula <jani.nikula@linux.intel.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
Rob Clark <robdclark@gmail.com>,
Abhinav Kumar <quic_abhinavk@quicinc.com>,
Sean Paul <sean@poorly.run>,
Marijn Suijten <marijn.suijten@somainline.org>
Cc: linux-arm-msm@vger.kernel.org, intel-gfx@lists.freedesktop.org,
freedreno@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 06/10] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters
Date: Wed, 08 Mar 2023 12:14:30 +0200 [thread overview]
Message-ID: <874jqvczy1.fsf@intel.com> (raw)
In-Reply-To: <20230307134901.322560-7-dmitry.baryshkov@linaro.org>
On Tue, 07 Mar 2023, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> drivers/gpu/drm/display/drm_dsc_helper.c | 127 ++++++++++++++++++----
> drivers/gpu/drm/i915/display/intel_vdsc.c | 10 +-
> include/drm/display/drm_dsc_helper.h | 7 +-
> 3 files changed, 119 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index acb93d4116e0..35b39f3109c4 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -324,11 +324,81 @@ struct rc_parameters_data {
>
> #define DSC_BPP(bpp) ((bpp) << 4)
>
> +static const struct rc_parameters_data rc_parameters_pre_scr[] = {
> + {
> + .bpp = DSC_BPP(8), .bpc = 8,
> + { 512, 12, 6144, 3, 12, 11, 11, {
> + { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> + { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> + { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> + { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + }
> + }
> + },
> + {
> + .bpp = DSC_BPP(8), .bpc = 10,
> + { 512, 12, 6144, 7, 16, 15, 15, {
> + /*
> + * DSC model/pre-SCR-cfg has 8 for range_max_qp[0], however
> + * VESA DSC 1.1 Table E-5 sets it to 4.
> + */
> + { 0, 4, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
> + { 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> + { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> + { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + }
> + }
> + },
> + {
> + .bpp = DSC_BPP(8), .bpc = 12,
> + { 512, 12, 6144, 11, 20, 19, 19, {
> + { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> + { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> + { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> + { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> + { 21, 23, -12 }
> + }
> + }
> + },
> + {
> + .bpp = DSC_BPP(12), .bpc = 8,
> + { 341, 15, 2048, 3, 12, 11, 11, {
> + { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> + { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> + { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> + { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + }
> + }
> + },
> + {
> + .bpp = DSC_BPP(12), .bpc = 10,
> + { 341, 15, 2048, 7, 16, 15, 15, {
> + { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> + { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> + { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> + { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + }
> + }
> + },
> + {
> + .bpp = DSC_BPP(12), .bpc = 12,
> + { 341, 15, 2048, 11, 20, 19, 19, {
> + { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> + { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> + { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> + { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> + { 21, 23, -12 }
> + }
> + }
> + },
> + { /* sentinel */ }
> +};
> +
> /*
> * Selected Rate Control Related Parameter Recommended Values
> * from DSC_v1.11 spec & C Model release: DSC_model_20161212
> */
The comment is no longer accurate, is it?
There are various ways to determine the parameters to use. There's even
an application note "VESA DSC v1.2a Guidance on Deriving DSC Rate
Control Parameters" that lists the options. They are all valid and
should "provide visually lossless quality".
Would it be simplest to always use the C model parameters in the tables
here, referencing the zip file name with date above each table? That
could at least be consistent, and drivers could override parameters
using other methods if they desire. And it would be easiest to review.
I'm having a hard time finding time to review all this in a timely
fashion. Would be good to try to get other folks to review the rest,
it's really not very i915 specific anyway. In the mean time I think
patches 1-5 are okay to merge via drm-misc.
BR,
Jani.
> -static const struct rc_parameters_data rc_parameters[] = {
> +static const struct rc_parameters_data rc_parameters_1_2_444[] = {
> {
> .bpp = DSC_BPP(6), .bpc = 8,
> { 768, 15, 6144, 3, 13, 11, 11, {
> @@ -388,22 +458,18 @@ static const struct rc_parameters_data rc_parameters[] = {
> { 512, 12, 6144, 3, 12, 11, 11, {
> { 0, 4, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> - { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 }, { 5, 12, -12 },
> - { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + { 3, 9, -8 }, { 3, 10, -10 }, { 5, 10, -10 }, { 5, 11, -12 },
> + { 5, 11, -12 }, { 9, 12, -12 }, { 12, 13, -12 }
> }
> }
> },
> {
> .bpp = DSC_BPP(8), .bpc = 10,
> { 512, 12, 6144, 7, 16, 15, 15, {
> - /*
> - * DSC model/pre-SCR-cfg has 8 for range_max_qp[0], however
> - * VESA DSC 1.1 Table E-5 sets it to 4.
> - */
> - { 0, 4, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
> + { 0, 8, 2 }, { 4, 8, 0 }, { 5, 9, 0 }, { 5, 10, -2 },
> { 7, 11, -4 }, { 7, 11, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> - { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> - { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + { 7, 13, -8 }, { 7, 14, -10 }, { 9, 14, -10 }, { 9, 15, -12 },
> + { 9, 15, -12 }, { 13, 16, -12 }, { 16, 17, -12 }
> }
> }
> },
> @@ -412,9 +478,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> { 512, 12, 6144, 11, 20, 19, 19, {
> { 0, 12, 2 }, { 4, 12, 0 }, { 9, 13, 0 }, { 9, 14, -2 },
> { 11, 15, -4 }, { 11, 15, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> - { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> - { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> - { 21, 23, -12 }
> + { 11, 17, -8 }, { 11, 18, -10 }, { 13, 18, -10 },
> + { 13, 19, -12 }, { 13, 19, -12 }, { 17, 20, -12 },
> + { 20, 21, -12 }
> }
> }
> },
> @@ -498,8 +564,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> { 341, 15, 2048, 3, 12, 11, 11, {
> { 0, 2, 2 }, { 0, 4, 0 }, { 1, 5, 0 }, { 1, 6, -2 },
> { 3, 7, -4 }, { 3, 7, -6 }, { 3, 7, -8 }, { 3, 8, -8 },
> - { 3, 9, -8 }, { 3, 10, -10 }, { 5, 11, -10 },
> - { 5, 12, -12 }, { 5, 13, -12 }, { 7, 13, -12 }, { 13, 15, -12 }
> + { 3, 8, -8 }, { 3, 9, -10 }, { 5, 9, -10 }, { 5, 9, -12 },
> + { 5, 9, -12 }, { 7, 10, -12 }, { 10, 11, -12 }
> }
> }
> },
> @@ -508,8 +574,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> { 341, 15, 2048, 7, 16, 15, 15, {
> { 0, 2, 2 }, { 2, 5, 0 }, { 3, 7, 0 }, { 4, 8, -2 },
> { 6, 9, -4 }, { 7, 10, -6 }, { 7, 11, -8 }, { 7, 12, -8 },
> - { 7, 13, -8 }, { 7, 14, -10 }, { 9, 15, -10 }, { 9, 16, -12 },
> - { 9, 17, -12 }, { 11, 17, -12 }, { 17, 19, -12 }
> + { 7, 12, -8 }, { 7, 13, -10 }, { 9, 13, -10 }, { 9, 13, -12 },
> + { 9, 13, -12 }, { 11, 14, -12 }, { 14, 15, -12 }
> }
> }
> },
> @@ -518,9 +584,9 @@ static const struct rc_parameters_data rc_parameters[] = {
> { 341, 15, 2048, 11, 20, 19, 19, {
> { 0, 6, 2 }, { 4, 9, 0 }, { 7, 11, 0 }, { 8, 12, -2 },
> { 10, 13, -4 }, { 11, 14, -6 }, { 11, 15, -8 }, { 11, 16, -8 },
> - { 11, 17, -8 }, { 11, 18, -10 }, { 13, 19, -10 },
> - { 13, 20, -12 }, { 13, 21, -12 }, { 15, 21, -12 },
> - { 21, 23, -12 }
> + { 11, 16, -8 }, { 11, 17, -10 }, { 13, 17, -10 },
> + { 13, 17, -12 }, { 13, 17, -12 }, { 15, 18, -12 },
> + { 18, 19, -12 }
> }
> }
> },
> @@ -602,7 +668,8 @@ static const struct rc_parameters_data rc_parameters[] = {
> { /* sentinel */ }
> };
>
> -static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> +static const struct rc_parameters *get_rc_params(const struct rc_parameters_data *rc_parameters,
> + u16 dsc_bpp,
> u8 bits_per_component)
> {
> int i;
> @@ -622,11 +689,13 @@ static const struct rc_parameters *get_rc_params(u16 dsc_bpp,
> * function.
> *
> * @vdsc_cfg: DSC Configuration data partially filled by driver
> + * @kind: operating mode and standard to follow
> *
> * Return: 0 or -error code in case of an error
> */
> -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)
> {
> + const struct rc_parameters_data *data;
> const struct rc_parameters *rc_params;
> int i;
>
> @@ -634,7 +703,19 @@ int drm_dsc_setup_rc_params(struct drm_dsc_config *vdsc_cfg)
> !vdsc_cfg->bits_per_component))
> return -EINVAL;
>
> - rc_params = get_rc_params(vdsc_cfg->bits_per_pixel,
> + switch (kind) {
> + case DRM_DSC_1_2_444:
> + data = rc_parameters_1_2_444;
> + break;
> + case DRM_DSC_1_1_PRE_SCR:
> + data = rc_parameters_pre_scr;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + rc_params = get_rc_params(data,
> + vdsc_cfg->bits_per_pixel,
> vdsc_cfg->bits_per_component);
> if (!rc_params)
> return -EINVAL;
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 20a4c2f343fe..a4d1d2ba71bb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -157,7 +157,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);
> +
> 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_ */
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2023-03-08 10:14 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-07 13:48 [Intel-gfx] [PATCH v2 00/10] drm/i915: move DSC RC tables to drm_dsc_helper.c Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 01/10] drm/i915/dsc: change DSC param tables to follow the DSC model Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 02/10] drm/i915/dsc: move rc_buf_thresh values to common helper Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 03/10] drm/i915/dsc: move DSC tables to DRM DSC helper Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 04/10] drm/i915/dsc: stop using interim structure for calculated params Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 05/10] drm/display/dsc: use flat array for rc_parameters lookup Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 06/10] drm/display/dsc: split DSC 1.2 and DSC 1.1 (pre-SCR) parameters Dmitry Baryshkov
2023-03-08 10:14 ` Jani Nikula [this message]
2023-03-08 12:59 ` Dmitry Baryshkov
2023-03-08 13:19 ` Jani Nikula
2023-03-08 16:50 ` Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 07/10] drm/display/dsc: include the rest of pre-SCR parameters Dmitry Baryshkov
2023-03-07 13:48 ` [Intel-gfx] [PATCH v2 08/10] drm/display/dsc: add YCbCr 4:2:2 and 4:2:0 RC parameters Dmitry Baryshkov
2023-03-07 13:49 ` [Intel-gfx] [PATCH v2 09/10] drm/display/dsc: add helper to set semi-const parameters Dmitry Baryshkov
2023-03-07 13:49 ` [Intel-gfx] [PATCH v2 10/10] drm/msm/dsi: use new helpers for DSC setup Dmitry Baryshkov
2023-03-14 8:45 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: move DSC RC tables to drm_dsc_helper.c (rev2) Patchwork
2023-03-28 23:22 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: move DSC RC tables to drm_dsc_helper.c (rev3) Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=874jqvczy1.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=joonas.lahtinen@linux.intel.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=quic_abhinavk@quicinc.com \
--cc=robdclark@gmail.com \
--cc=rodrigo.vivi@intel.com \
--cc=sean@poorly.run \
--cc=tvrtko.ursulin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox