[Intel-gfx] [PATCH v2] drm/i915/psr: Fix PSR_IMR/IIR field handling

[Hogander, Jouni]

On Thu, 2022-09-22 at 13:08 +0000, Souza, Jose wrote:
> On Thu, 2022-09-22 at 10:59 +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.
> 
> From where are you getting that TRANSCODER_EDP == 0?

It's not. That is my point. If you look at this commit:

https://github.com/freedesktop/drm-tip/commit/8241cfbe67f4082eee5fc72e5a8025c5b58c2ddf

adding this comment:

+       /*
+        * 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
+        */

and the code added by this commit is doing

...
+               trans_shift = 0;
mask = EDP_PSR_ERROR(trans_shift);
...

+       mask = EDP_PSR_ERROR(trans_shift);
...

and if we look at EDP_PSR_ERROR definition:

...
#define   _EDP_PSR_TRANS_SHIFT(trans)		((trans) ==
TRANSCODER_EDP ? \
						 0 : ((trans) -
TRANSCODER_A + 1) * 8)
...
#define   EDP_PSR_ERROR(trans)			(0x4 <<
_EDP_PSR_TRANS_SHIFT(trans))
...

EDP_PSR_ERROR(0) is 0x400 which is wrong for e.g. TGL. Using
TRANSCODER_EDP as mentioned in the added comment:
EDP_PSR_ERROR(TRANSCODER_EDP) is 0x4 which is correct.

My patch is doing what is in the comment. There are other way to fix
this, but to my opinion original idea using TRANSCODER_EDP in commit 
8241cfbe67f4082eee5fc72e5a8025c5b58c2ddf to force 0 shift keeps the
code pretty clean.

> 
> enum pipe {
>         INVALID_PIPE = -1,
> 
>         PIPE_A = 0,
>         PIPE_B,
>         PIPE_C,
>         PIPE_D,
>         _PIPE_EDP,
> 
>         I915_MAX_PIPES = _PIPE_EDP
> };
> 
> #define pipe_name(p) ((p) + 'A')
> 
> enum transcoder {
>         INVALID_TRANSCODER = -1,
>         /*
>          * The following transcoders have a 1:1 transcoder -> pipe
> mapping,
>          * keep their values fixed: the code assumes that
> TRANSCODER_A=0, the
>          * rest have consecutive values and match the enum values of
> the pipes
>          * they map to.EDP_PSR_TRANS_
>          */
>         TRANSCODER_A = PIPE_A,
>         TRANSCODER_B = PIPE_B,
>         TRANSCODER_C = PIPE_C,
>         TRANSCODER_D = PIPE_D,
> 
>         /*
>          * The following transcoders can map to any pipe, their enum
> value
>          * doesn't need to stay fixed.
>          */
>         TRANSCODER_EDP,
> 
> https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/i915/display/intel_display.h#n87
> 
> > 
> > Fix this by using TRANSCODER_EDP definition instead of 0. Even
> > thought
> > TRANSCODER_EDP doesn't exist in display_ver >= 12 doing it this way
> > keeps
> > code clean and readable.
> 
> trans_shift = 0 is fine, we needed this because display12+ splited
> from a single register with all transcoder to one register per
> transcoder.
> 

No, it's not. See the definition of  _EDP_PSR_TRANS_SHIFT and
EDP_PSR_TRANS_*. Maybe renaming trans_shift to transcoder would prevent
misunderstanding here.

> > 
> > v2: Improve commit message (José)
> > 
> > Cc: Mika Kahola <mika.kahola at intel.com>
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > 
> > Signed-off-by: Jouni Högander <jouni.hogander at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_psr.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9def8d9fade6..9ecf1a9a1120 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -129,7 +129,7 @@ static void psr_irq_control(struct intel_dp
> > *intel_dp)
> >          * 0 shift in bit definition
> >          */
> >         if (DISPLAY_VER(dev_priv) >= 12) {
> > -               trans_shift = 0;
> > +               trans_shift = TRANSCODER_EDP;
> >                 imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> >         } else {
> >                 trans_shift = intel_dp->psr.transcoder;
> > @@ -195,7 +195,7 @@ void intel_psr_irq_handler(struct intel_dp
> > *intel_dp, u32 psr_iir)
> >         i915_reg_t imr_reg;
> >  
> >         if (DISPLAY_VER(dev_priv) >= 12) {
> > -               trans_shift = 0;
> > +               trans_shift = TRANSCODER_EDP;
> >                 imr_reg = TRANS_PSR_IMR(intel_dp->psr.transcoder);
> >         } else {
> >                 trans_shift = intel_dp->psr.transcoder;
> > @@ -1197,7 +1197,7 @@ static bool psr_interrupt_error_check(struct
> > intel_dp *intel_dp)
> >         if (DISPLAY_VER(dev_priv) >= 12) {
> >                 val = intel_de_read(dev_priv,
> >                                     TRANS_PSR_IIR(intel_dp-
> > >psr.transcoder));
> > -               val &= EDP_PSR_ERROR(0);
> > +               val &= EDP_PSR_ERROR(TRANSCODER_EDP);
> >         } else {
> >                 val = intel_de_read(dev_priv, EDP_PSR_IIR);
> >                 val &= EDP_PSR_ERROR(intel_dp->psr.transcoder);
>