Re: [PATCH v2] drm/i915/psr: Fix PSR_IMR/IIR field handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2022-09-23 at 12:45 +0000, Hogander, Jouni wrote:
> On Fri, 2022-09-23 at 12:37 +0000, Souza, Jose wrote:
> > On Fri, 2022-09-23 at 06:11 +0000, Hogander, Jouni wrote:
> > > 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.
> > 
> > Okay now I understood.
> > In my opinion the proper fix would be add TGL_X macros to be used in
> > diplay12+ paths and drop the EDP transcoder concept that do not exist
> > in TGL+.
> 
> Ok, I started to look at this originally and it gets a bit messy as
> each bit in PSR_IMR/PSR_ISR needs separate handling. If we choose this
> then I was thinking adding similar _bit_get functions as we have for
> man_trk_ctl bits. What do you think?

If the code gets simpler go ahead with functions to return the bits.

> 
> I would still consider current approach as forcing 0 shifting using
> EDP_PSR_TRANS_* keeps it pretty simple.

But it is terrible for readability, took me a while to get what you wanted to fix.

> 
> > 
> > Also please include a fixes tag pointing to
> > 8241cfbe67f4082eee5fc72e5a8025c5b58c2ddf so this gets backported.
> > 
> > > 
> > > > > 
> > > > > v2: Improve commit message (José)
> > > > > 
> > > > > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > > > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
> > > > > ---
> > > > >  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);
> > > > 
> > > 
> > 
> 





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux