On Mon, 10 Jul 2023, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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. Moreover, are you sure a verification with a warning belongs at the low level read function? They belong at different abstraction layers. Add the low level read function, then wrap it with something that verifies. BR, Jani. > > 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