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 2/5] drm/i915/vdsc: Add function to read any PPS register
Date: Mon, 10 Jul 2023 13:52:20 +0300 [thread overview]
Message-ID: <875y6sjaxn.fsf@intel.com> (raw)
In-Reply-To: <20230710100911.2736389-3-suraj.kandpal@intel.com>
On Mon, 10 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.
>
> Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_vdsc.c | 293 ++++++++++++++++++++--
> 1 file changed, 272 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index 1a8272324c36..53eac8d9c80f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -940,16 +940,284 @@ void intel_dsc_disable(const struct intel_crtc_state *old_crtc_state)
> }
> }
>
> +static void intel_dsc_read_pps_reg(struct intel_crtc_state *crtc_state,
> + enum intel_dsc_pps pps, u32 *pps_val)
> +{
> + 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;
> + u32 pps_temp;
> +
> + switch (pps) {
> + case PPS_0:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_0);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_0);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_0(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
This function is just a huge amount of duplicated copy-paste.
What you need is a function to return the *registers* for each pps
index. And yeah, I don't think the enum is needed.
We should also switch the negative !is_pipe_dsc() positive
is_pipe_dsc().
Refactoring should never introduce functional changes. You should
separate the two. This adds the if (crtc_state->dsc.dsc_split)
conditions which doesn't exist in the original.
BR,
Jani.
> + break;
> + case PPS_1:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_1);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_1);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_1(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_1(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_2:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_2);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_2);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_2(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_2(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_3:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_3);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_3);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_3(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_3(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_4:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_4);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_4);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_4(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_4(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_5:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_5);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_5);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_5(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_5(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_6:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_6);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_6);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_6(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_6(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_7:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_7);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_7);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_7(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_7(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_8:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_8);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_8);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_8(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_8(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_9:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_9);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_9);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_9(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_9(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_10:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_10);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_10);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_10(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_10(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + case PPS_16:
> + if (!is_pipe_dsc(crtc, cpu_transcoder)) {
> + *pps_val = intel_de_read(i915, DSCA_PICTURE_PARAMETER_SET_16);
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp = intel_de_read(i915, DSCC_PICTURE_PARAMETER_SET_16);
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + } else {
> + *pps_val = intel_de_read(i915,
> + ICL_DSC0_PICTURE_PARAMETER_SET_16(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + ICL_DSC1_PICTURE_PARAMETER_SET_16(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + }
> + break;
> + /* Since PPS_17 and PPS_18 were introduced from MTL dsc check need not be done */
> + case PPS_17:
> + *pps_val = intel_de_read(i915,
> + MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + break;
> + case PPS_18:
> + *pps_val = intel_de_read(i915,
> + MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe));
> + if (crtc_state->dsc.dsc_split) {
> + pps_temp =
> + intel_de_read(i915,
> + MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe));
> + drm_WARN_ON(&i915->drm, *pps_val != pps_temp);
> + }
> + break;
> + default:
> + drm_err(&i915->drm, "PPS register does not exist\n");
> + break;
> + }
> +}
> +
> +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_pps_reg(crtc_state, PPS_0, &pps_temp1);
> + intel_dsc_read_pps_reg(crtc_state, PPS_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;
> + u32 dss_ctl1, dss_ctl2;
>
> if (!intel_dsc_source_support(crtc_state))
> return;
> @@ -970,24 +1238,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);
> - } 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));
> - }
> -
> - 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-10 10:52 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-10 10:09 [Intel-gfx] [PATCH 0/5] Add DSC PPS readout Suraj Kandpal
2023-07-10 10:09 ` [Intel-gfx] [PATCH 1/5] drm/i915/dsc: Add PPS enum Suraj Kandpal
2023-07-10 10:45 ` Jani Nikula
2023-07-10 10:45 ` Jani Nikula
2023-07-11 6:12 ` Kandpal, Suraj
2023-07-10 10:09 ` [Intel-gfx] [PATCH 2/5] drm/i915/vdsc: Add function to read any PPS register Suraj Kandpal
2023-07-10 10:52 ` Jani Nikula [this message]
2023-07-10 10:56 ` Jani Nikula
2023-07-10 10:09 ` [Intel-gfx] [PATCH 3/5] drm/i915/vdsc: Add function to write in PPS registers Suraj Kandpal
2023-07-10 10:57 ` Jani Nikula
2023-07-10 10:09 ` [Intel-gfx] [PATCH 4/5] drm/i915/vdsc: Fill the intel_dsc_get_pps_config function Suraj Kandpal
2023-07-10 11:11 ` Jani Nikula
2023-07-11 9:42 ` Kandpal, Suraj
2023-07-10 10:09 ` [Intel-gfx] [PATCH 5/5] drm/i915/display: Compare the readout dsc pps params Suraj Kandpal
2023-07-10 11:15 ` Jani Nikula
2023-07-11 5:15 ` Kandpal, Suraj
2023-07-10 13:45 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for Add DSC PPS readout 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=875y6sjaxn.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.