From: Jani Nikula <jani.nikula@linux.intel.com>
To: Suraj Kandpal <suraj.kandpal@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v2 3/6] drm/i915/vdsc: Add function to read any PPS register
Date: Thu, 13 Jul 2023 15:47:33 +0300 [thread overview]
Message-ID: <87h6q8f062.fsf@intel.com> (raw)
In-Reply-To: <20230713062924.2894736-4-suraj.kandpal@intel.com>
On Thu, 13 Jul 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote:
> Add function to read any PPS register based on the
> intel_dsc_pps enum provided. Add a function which will call the
> new pps read function and place it in crtc state. Only PPS0 and
> PPS1 are readout the rest of the registers will be read in upcoming
> patches.
>
> --v2
> -Changes in read function as PPS enum is removed
> -Initialize pps_val as 0 in pps_read func itself [Jani]
> -Create a function that gets the required register and call that
> in the common read function [Jani]
> -Move the drm_WARN_ON one abstraction layer above [Jani]
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_vdsc.c | 276 +++++++++++++++++++---
> 1 file changed, 242 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index d48b8306bfc3..48273a3618c5 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -303,6 +303,196 @@ int intel_dsc_get_num_vdsc_instances(const struct intel_crtc_state *crtc_state)
> return num_vdsc_instances;
> }
>
> +static void intel_dsc_get_pps_reg(struct intel_crtc_state *crtc_state, int pps,
> + int dsc_eng_no, i915_reg_t *dsc_reg)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + enum pipe pipe = crtc->pipe;
> + bool pipe_dsc;
> +
> + pipe_dsc = is_pipe_dsc(crtc, cpu_transcoder);
> +
> + switch (pps) {
> + case 0:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_0;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_0;
> + }
> + break;
> + case 1:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_1;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_1;
> + }
> + break;
> + case 2:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_2;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_2;
> + }
> + break;
> + case 3:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_3;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_3;
> + }
> + break;
> + case 4:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_4;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_4;
> + }
> + break;
> + case 5:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_5;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_5;
> + }
> + break;
> + case 6:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_6;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_6;
> + }
> + break;
> + case 7:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_7;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_7;
> + }
> + break;
> + case 8:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_8;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_8;
> + }
> + break;
> + case 9:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_9;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_9;
> + }
> + break;
> + case 10:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_10;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_10;
> + }
> + break;
> + case 16:
> + if (pipe_dsc) {
> + if (dsc_eng_no == 2)
> + *dsc_reg = ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe);
> + else
> + *dsc_reg = ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe);
> + } else {
> + if (dsc_eng_no == 2)
> + *dsc_reg = DSCA_PICTURE_PARAMETER_SET_16;
> + else
> + *dsc_reg = DSCC_PICTURE_PARAMETER_SET_16;
> + }
> + break;
> + /*
> + * Since PPS_17 and PPS_18 were introduced from MTL dsc check
> + * need not be done
> + */
> + case 17:
> + if (dsc_eng_no == 2)
> + *dsc_reg = MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe);
> + else
> + *dsc_reg = MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe);
> + break;
> + case 18:
> + if (dsc_eng_no == 2)
> + *dsc_reg = MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe);
> + else
> + *dsc_reg = MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe);
> + break;
> + default:
> + drm_err(&i915->drm, "PPS register does not exist\n");
> + break;
> + }
> +}
> +
> static void intel_dsc_pps_configure(const struct intel_crtc_state *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -930,16 +1120,64 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
> }
> }
>
> +static bool intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
> + int pps, u32 *pps_val)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + int num_vdsc_instances = intel_dsc_get_num_vdsc_instances(crtc_state);
> + i915_reg_t dsc_reg;
> + u32 pps_temp;
> +
> + *pps_val = 0;
> +
> + intel_dsc_get_pps_reg(crtc_state, pps, 0, &dsc_reg);
> + *pps_val = intel_de_read(i915, dsc_reg);
> + if (num_vdsc_instances > 1) {
Btw going from (crtc_state->dsc.dsc_split) to if (num_vdsc_instances >
1) should also be a separate change, instead of baked into this one.
BR,
Jani.
> + intel_dsc_get_pps_reg(crtc_state, pps, 2, &dsc_reg);
> + pps_temp = intel_de_read(i915, dsc_reg);
> + if (*pps_val != pps_temp)
> + return true;
> + }
> + return false;
> +}
> +
> +static void intel_dsc_read_and_verify_pps_reg(struct intel_crtc_state *crtc_state,
> + int pps, u32 *pps_val)
> +{
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> + bool is_dsc_diff;
> +
> + is_dsc_diff = intel_dsc_read_pps_reg(crtc_state, pps, pps_val);
> + drm_WARN_ON(&i915->drm, is_dsc_diff);
> +}
> +
> +static void intel_dsc_get_pps_config(struct intel_crtc_state *crtc_state)
> +{
> + struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> + u32 pps_temp1, pps_temp2;
> +
> + /* Readout PPS_0 and PPS_1 registers */
> + intel_dsc_read_and_verify_pps_reg(crtc_state, 0, &pps_temp1);
> + intel_dsc_read_and_verify_pps_reg(crtc_state, 1, &pps_temp2);
> +
> + vdsc_cfg->bits_per_pixel = pps_temp2;
> +
> + if (pps_temp1 & DSC_NATIVE_420_ENABLE)
> + vdsc_cfg->bits_per_pixel >>= 1;
> +
> + crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> +}
> +
> void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> - struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> - enum pipe pipe = crtc->pipe;
> enum intel_display_power_domain power_domain;
> intel_wakeref_t wakeref;
> - u32 dss_ctl1, dss_ctl2, pps0 = 0, pps1 = 0, pps_temp0 = 0, pps_temp1 = 1;
> + u32 dss_ctl1, dss_ctl2;
>
> if (!intel_dsc_source_support(crtc_state))
> return;
> @@ -960,37 +1198,7 @@ void intel_dsc_get_config(struct intel_crtc_state *crtc_state)
> crtc_state->dsc.dsc_split = (dss_ctl2 & RIGHT_BRANCH_VDSC_ENABLE) &&
> (dss_ctl1 & JOINER_ENABLE);
>
> - /* FIXME: add more state readout as needed */
> -
> - /* PPS0 & PPS1 */
> - if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> - pps1 = intel_de_read(dev_priv, DSCA_PICTURE_PARAMETER_SET_1);
> - if (crtc_state->dsc.dsc_split) {
> - pps_temp1 = intel_de_read(dev_priv, DSCC_PICTURE_PARAMETER_SET_1);
> - drm_WARN_ON(&dev_priv->drm, pps1 != pps_temp1);
> - }
> -
> - } else {
> - pps0 = intel_de_read(dev_priv,
> - ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
> - pps1 = intel_de_read(dev_priv,
> - ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
> - if (crtc_state->dsc.dsc_split) {
> - pps_temp0 = intel_de_read(dev_priv,
> - ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
> - pps_temp1 = intel_de_read(dev_priv,
> - ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
> - drm_WARN_ON(&dev_priv->drm, pps0 != pps_temp0);
> - drm_WARN_ON(&dev_priv->drm, pps1 != pps_temp1);
> - }
> - }
> -
> - vdsc_cfg->bits_per_pixel = pps1;
> -
> - if (pps0 & DSC_NATIVE_420_ENABLE)
> - vdsc_cfg->bits_per_pixel >>= 1;
> -
> - crtc_state->dsc.compressed_bpp = vdsc_cfg->bits_per_pixel >> 4;
> + intel_dsc_get_pps_config(crtc_state);
> out:
> intel_display_power_put(dev_priv, power_domain, wakeref);
> }
--
Jani Nikula, Intel Open Source Graphics Center
next prev parent reply other threads:[~2023-07-13 12:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-13 6:29 [Intel-gfx] [PATCH v2 0/6] Add DSC PPS readout Suraj Kandpal
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 1/6] drm/i915/vdsc: Refactor dsc register field macro Suraj Kandpal
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 2/6] drm/i915/vdsc: Add a check for dsc split cases Suraj Kandpal
2023-07-13 12:29 ` Jani Nikula
2023-07-13 13:03 ` Kandpal, Suraj
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 3/6] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
2023-07-13 12:38 ` Jani Nikula
2023-07-13 12:42 ` Jani Nikula
2023-07-13 12:47 ` Jani Nikula [this message]
2023-07-13 13:06 ` Kandpal, Suraj
2023-07-13 13:10 ` Nautiyal, Ankit K
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 4/6] drm/i915/vdsc: Add function to write in " Suraj Kandpal
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 5/6] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
2023-07-13 6:29 ` [Intel-gfx] [PATCH v2 6/6] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
2023-07-13 9:36 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Add DSC PPS readout (rev2) Patchwork
2023-07-13 9:36 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2023-07-13 9:49 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2023-07-13 12:30 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
2023-07-13 13:15 ` [Intel-gfx] [PATCH v2 0/6] Add DSC PPS readout Jani Nikula
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=87h6q8f062.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=suraj.kandpal@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.