| Message ID | 20220922075948.111558-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/psr: Fix PSR_IMR/IIR field handling | expand |
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? 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. */ 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. > > v2: Improve commit message (José) > > Cc: Mika Kahola <mika.kahola@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@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);
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@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > Signed-off-by: Jouni Högander <jouni.hogander@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); >
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+. Also please include a fixes tag pointing to 8241cfbe67f4082eee5fc72e5a8025c5b58c2ddf so this gets backported. > > > > > > > v2: Improve commit message (José) > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@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); > > >
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? I would still consider current approach as forcing 0 shifting using EDP_PSR_TRANS_* keeps it pretty simple. > > Also please include a fixes tag pointing to > 8241cfbe67f4082eee5fc72e5a8025c5b58c2ddf so this gets backported. > > > > > > > > > > > v2: Improve commit message (José) > > > > > > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@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); > > > > > >
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@intel.com> > > > > > Cc: José Roberto de Souza <jose.souza@intel.com> > > > > > > > > > > Signed-off-by: Jouni Högander <jouni.hogander@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); > > > > > > > > > >
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);
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 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. v2: Improve commit message (José) Cc: Mika Kahola <mika.kahola@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Jouni Högander <jouni.hogander@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)