Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Fix PSR_IMR/IIR field handling
On Thu, 2022-09-29 at 06:16 -0700, José Roberto de Souza wrote:
> On Tue, 2022-09-27 at 14:09 +0300, Jouni Högander wrote:
> > Current PSR code is supposed to use TRANSCODER_EDP to force 0 shift for
> > bits in PSR_IMR/IIR registers:
> >
> > /*
> > * gen12+ has registers relative to transcoder and one per transcoder
> > * using the same bit definition: handle it as TRANSCODER_EDP to force
> > * 0 shift in bit definition
> > */
> >
> > At the time of writing the code assumption "TRANSCODER_EDP == 0" was made.
> > This is not the case and all fields in PSR_IMR and PSR_IIR are shifted
> > incorrectly if DISPLAY_VER >= 12.
> >
> > Fix this by adding separate register field defines for >=12 and add bit
> > getter functions to keep code readability.
> >
> > v3:
> > - Add separate register field defines (José)
> > - Add bit getter functions (José)
> > v2:
> > - Improve commit message (José)
Also missing the Fixes tag, so this gets backported to stable kernels.
> >
> > Signed-off-by: Jouni Högander
> > ---
> > drivers/gpu/drm/i915/display/intel_psr.c | 84 ++--
> > drivers/gpu/drm/i915/i915_reg.h | 16 +++--
> > 2 files changed, 62 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9def8d9fade6..d7b08a7da9e9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -116,34 +116,56 @@ static bool psr2_global_enabled(struct intel_dp
> > *intel_dp)
> > }
> > }
> >
> > +static u32 psr_irq_psr_error_bit_get(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +
> > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_ERROR :
> > + EDP_PSR_ERROR(intel_dp->psr.transcoder);
>
> Drop the "_EDP", just go with TGL_PSR_ERROR... there is no reference to EDP
> or any transcoder in TGL+ it is one register per transcoder.
>
> > +}
> > +
> > +static u32 psr_irq_post_exit_bit_get(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +
> > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_POST_EXIT :
> > + EDP_PSR_POST_EXIT(intel_dp->psr.transcoder);
> > +}
> > +
> > +static u32 psr_irq_pre_entry_bit_get(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +
> > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_PRE_ENTRY :
> > + EDP_PSR_PRE_ENTRY(intel_dp->psr.transcoder);
> > +}
> > +
> > +static u32 psr_irq_mask_get(struct intel_dp *intel_dp)
> > +{
> > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +
> > + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_MASK :
> > + EDP_PSR_MASK(intel_dp->psr.transcoder);
> > +}
> > +
> > static void psr_irq_control(struct intel_dp *intel_dp)
> > {
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > - enum transcoder trans_shift;
> > i915_reg_t imr_reg;
> > u32 mask, val;
> >
> > - /*
> > -* gen12+ has registers relative to transcoder and one per transcoder
> > -* using the same bit definition: handle it as TRANSCODER_EDP to force
> > -* 0 shift in bit definition
> > -*/
> > - if (DISPLAY_VER(dev_priv) >= 12) {
> > - trans_shift = 0;
> > + if (DISPLAY_VER(dev_priv) >= 12)
> > imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> > - } else {
> > - trans_shift = intel_dp->psr.transcoder;
> > + else
> > imr_reg = EDP_PSR_IMR;
> > - }
> >
> > - mask = EDP_PSR_ERROR(trans_shift);
> > + mask = psr_irq_psr_error_bit_get(intel_dp);
> > if (intel_dp->psr.debug & I915_PSR_DEBUG_IRQ)
> > - mask |= EDP_PSR_POST_EXIT(trans_shift) |
> > - EDP_PSR_PRE_ENTRY(trans_shift);
> > + mask |= psr_irq_post_exit_bit_get(intel_dp) |
> > + psr_irq_pre_entry_bit_get(intel_dp);
> >
> > - /* Warning: it is masking/setting reserved bits too */
> > val = intel_de_read(dev_priv, imr_reg);
> > - val &= ~EDP_PSR_TRANS_MASK(trans_shift);
> > + val &= ~psr_irq_mask_get(intel_dp);
> > val |= ~mask;
> > intel_de_write(dev_priv, imr_reg, val);
> > }
> > @@ -191,25 +213,21 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp,
> > u32 psr_iir)
> > enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > ktime_t time_ns = ktime_get();
> > - enum transcoder trans_shift;
> > i915_reg_t imr_reg;
> >
> > - if (DISPLAY_VER(dev_priv) >= 12) {
> > - trans_shift = 0;
> > + if (DISPLAY_VER(dev_priv) >= 12)
> > imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> > - } else {
> > - trans_shift = intel_dp->psr.transcoder;
> > + else
> > imr_reg = EDP_PSR_IMR;
> > - }
> >
> > - if
Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Fix PSR_IMR/IIR field handling
On Tue, 2022-09-27 at 14:09 +0300, Jouni Högander wrote:
> Current PSR code is supposed to use TRANSCODER_EDP to force 0 shift for
> bits in PSR_IMR/IIR registers:
>
> /*
> * gen12+ has registers relative to transcoder and one per transcoder
> * using the same bit definition: handle it as TRANSCODER_EDP to force
> * 0 shift in bit definition
> */
>
> At the time of writing the code assumption "TRANSCODER_EDP == 0" was made.
> This is not the case and all fields in PSR_IMR and PSR_IIR are shifted
> incorrectly if DISPLAY_VER >= 12.
>
> Fix this by adding separate register field defines for >=12 and add bit
> getter functions to keep code readability.
>
> v3:
> - Add separate register field defines (José)
> - Add bit getter functions (José)
> v2:
> - Improve commit message (José)
>
> Signed-off-by: Jouni Högander
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 84 ++--
> drivers/gpu/drm/i915/i915_reg.h | 16 +++--
> 2 files changed, 62 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 9def8d9fade6..d7b08a7da9e9 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -116,34 +116,56 @@ static bool psr2_global_enabled(struct intel_dp
> *intel_dp)
> }
> }
>
> +static u32 psr_irq_psr_error_bit_get(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_ERROR :
> + EDP_PSR_ERROR(intel_dp->psr.transcoder);
Drop the "_EDP", just go with TGL_PSR_ERROR... there is no reference to EDP or
any transcoder in TGL+ it is one register per transcoder.
> +}
> +
> +static u32 psr_irq_post_exit_bit_get(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_POST_EXIT :
> + EDP_PSR_POST_EXIT(intel_dp->psr.transcoder);
> +}
> +
> +static u32 psr_irq_pre_entry_bit_get(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_PRE_ENTRY :
> + EDP_PSR_PRE_ENTRY(intel_dp->psr.transcoder);
> +}
> +
> +static u32 psr_irq_mask_get(struct intel_dp *intel_dp)
> +{
> + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +
> + return DISPLAY_VER(dev_priv) >= 12 ? TGL_EDP_PSR_MASK :
> + EDP_PSR_MASK(intel_dp->psr.transcoder);
> +}
> +
> static void psr_irq_control(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> - enum transcoder trans_shift;
> i915_reg_t imr_reg;
> u32 mask, val;
>
> - /*
> - * gen12+ has registers relative to transcoder and one per transcoder
> - * using the same bit definition: handle it as TRANSCODER_EDP to force
> - * 0 shift in bit definition
> - */
> - if (DISPLAY_VER(dev_priv) >= 12) {
> - trans_shift = 0;
> + if (DISPLAY_VER(dev_priv) >= 12)
> imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> - } else {
> - trans_shift = intel_dp->psr.transcoder;
> + else
> imr_reg = EDP_PSR_IMR;
> - }
>
> - mask = EDP_PSR_ERROR(trans_shift);
> + mask = psr_irq_psr_error_bit_get(intel_dp);
> if (intel_dp->psr.debug & I915_PSR_DEBUG_IRQ)
> - mask |= EDP_PSR_POST_EXIT(trans_shift) |
> - EDP_PSR_PRE_ENTRY(trans_shift);
> + mask |= psr_irq_post_exit_bit_get(intel_dp) |
> + psr_irq_pre_entry_bit_get(intel_dp);
>
> - /* Warning: it is masking/setting reserved bits too */
> val = intel_de_read(dev_priv, imr_reg);
> - val &= ~EDP_PSR_TRANS_MASK(trans_shift);
> + val &= ~psr_irq_mask_get(intel_dp);
> val |= ~mask;
> intel_de_write(dev_priv, imr_reg, val);
> }
> @@ -191,25 +213,21 @@ void intel_psr_irq_handler(struct intel_dp *intel_dp,
> u32 psr_iir)
> enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> ktime_t time_ns = ktime_get();
> - enum transcoder trans_shift;
> i915_reg_t imr_reg;
>
> - if (DISPLAY_VER(dev_priv) >= 12) {
> - trans_shift = 0;
> + if (DISPLAY_VER(dev_priv) >= 12)
> imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> - } else {
> - trans_shift = intel_dp->psr.transcoder;
> + else
> imr_reg = EDP_PSR_IMR;
> - }
>
> - if (psr_iir & EDP_PSR_PRE_ENTRY(trans_shift)) {
> + if (psr_iir & psr_irq_pre_entry_bit_get(intel_dp)) {
> intel_dp->psr.last_entry_attempt = time_ns;
> drm_dbg_kms(_priv->drm,
> "[transcoder %s] PSR entry attempt in 2 vblanks\n",
>
