Re: [Intel-gfx] [PATCH v5 3/3] drm/i915: Make PSR registers relative to transcoders
On Fri, 2019-06-14 at 21:27 -0700, Dhinakaran Pandiyan wrote:
> "drm/i915/psr" in the subject.
Done
>
> On Sat, 2019-04-20 at 13:55 -0700, José Roberto de Souza wrote:
> > PSR registers are a mess, some have the full address while others
> > just
> > have the additional offset from psr_mmio_base.
> >
> > For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
> > 0x800 and using it makes more difficult for people with an PSR
> > register address or PSR register name from from BSpec as i915 also
> > don't match the BSpec names.
> > For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers
> > are
> > only available in DDIA.
> >
> > Other reason to make relative to transcoder is that since BDW every
> > transcoder have PSR registers, so in theory it should be possible
> > to
> > have PSR enabled in a non-eDP transcoder.
> >
> > So for BDW+ we can use _TRANS2() to get the register offset of any
> > PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
> > that will calculate the register offset for the single PSR
> > instance,
> > noting that we are already guarded about trying to enable PSR in
> > other
> > port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)'
> > in
> > intel_psr_compute_config(), this check should only be valid for HSW
> > and will be changed in future.
> > PSR2 registers and PSR_EVENT was added after Haswell so that is why
> > _PSR_ADJ() is not used in some macros.
> >
> > The only registers that can not be relative to transcoder are
> > PSR_IMR and PSR_IIR that are not relative to anything, so keeping
> > it
> > hardcoded.
> >
> > Also removing BDW_EDP_PSR_BASE from GVT because it is not used as
> > it
> > is the only PSR register that GVT have.
> >
> > v5:
> > - Macros changed to be more explicit about HSW (Dhinakaran)
> > - Squashed with the patch that added the tran parameter to the
> > macros (Dhinakaran)
> >
> > Cc: Dhinakaran Pandiyan
> > Cc: Rodrigo Vivi
> > Cc: Jani Nikula
> > Cc: Ville Syrjälä
> > Cc: Zhi Wang
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/gvt/handlers.c | 1 -
> > drivers/gpu/drm/i915/i915_debugfs.c | 18 +
> > drivers/gpu/drm/i915/i915_drv.h | 3 +-
> > drivers/gpu/drm/i915/i915_reg.h | 57 -
> >
> > drivers/gpu/drm/i915/intel_psr.c| 55 -
> > ---
> > 5 files changed, 83 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > b/drivers/gpu/drm/i915/gvt/handlers.c
> > index 18f01eeb2510..749e3e4204f2 100644
> > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > @@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct
> > intel_gvt
> > *gvt)
> > MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
> >
> > MMIO_D(WM_MISC, D_BDW);
> > - MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
> Change this to _SRD_CTL_EDP to keep the change non-functional? Any
> case, we'll
> need an ack to modify this.
Ping Zhi?
Do you think we should replace with _SRD_CTL_EDP or is okay to remove
it?
>
> > MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 5823ffb17821..2a0f5871e9a8 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2470,7 +2470,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv,
> > struct seq_file *m)
> > "BUF_ON",
> > "TG_ON"
> > };
> > - val = I915_READ(EDP_PSR2_STATUS);
> > + val = I915_READ(EDP_PSR2_STATUS(dev_priv-
> > >psr.transcoder));
> > status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> > EDP_PSR2_STATUS_STATE_SHIFT;
> > if (status_val < ARRAY_SIZE(live_status))
> > @@ -2486,7 +2486,7 @@ psr_source_status(struct drm_i915_private
> > *dev_priv,
> > struct seq_file *m)
> > "SRDOFFACK",
> > "SRDENT_ON",
> > };
> > - val = I915_READ(EDP_PSR_STATUS);
> > + val = I915_READ(EDP_PSR_STATUS(dev_priv-
> > >psr.transcoder));
> > status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> > EDP_PSR_STATUS_STATE_SHIFT;
> > if (status_val < ARRAY_SIZE(live_status))
> > @@ -2529,10 +2529,10 @@ static int i915_edp_psr_status(struct
> > seq_file *m,
> > void *data)
> > goto unlock;
> >
> > if (psr->psr2_enabled) {
> > - val = I915_READ(EDP_PSR2_CTL);
> > + val = I915_READ(EDP_PSR2_CTL(dev_priv-
> > >psr.transcoder));
> > enabled = val & EDP_PSR2_ENABLE;
> > } else {
> > - val = I915_READ(EDP_PSR_CTL);
> > + val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> > enabled = val & EDP_PSR_ENABLE;
> > }
> > seq_printf(m, "Source PSR ctl: %s
Re: [Intel-gfx] [PATCH v5 3/3] drm/i915: Make PSR registers relative to transcoders
"drm/i915/psr" in the subject.
On Sat, 2019-04-20 at 13:55 -0700, José Roberto de Souza wrote:
> PSR registers are a mess, some have the full address while others just
> have the additional offset from psr_mmio_base.
>
> For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
> 0x800 and using it makes more difficult for people with an PSR
> register address or PSR register name from from BSpec as i915 also
> don't match the BSpec names.
> For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers are
> only available in DDIA.
>
> Other reason to make relative to transcoder is that since BDW every
> transcoder have PSR registers, so in theory it should be possible to
> have PSR enabled in a non-eDP transcoder.
>
> So for BDW+ we can use _TRANS2() to get the register offset of any
> PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
> that will calculate the register offset for the single PSR instance,
> noting that we are already guarded about trying to enable PSR in other
> port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)' in
> intel_psr_compute_config(), this check should only be valid for HSW
> and will be changed in future.
> PSR2 registers and PSR_EVENT was added after Haswell so that is why
> _PSR_ADJ() is not used in some macros.
>
> The only registers that can not be relative to transcoder are
> PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
> hardcoded.
>
> Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
> is the only PSR register that GVT have.
>
> v5:
> - Macros changed to be more explicit about HSW (Dhinakaran)
> - Squashed with the patch that added the tran parameter to the
> macros (Dhinakaran)
>
> Cc: Dhinakaran Pandiyan
> Cc: Rodrigo Vivi
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Cc: Zhi Wang
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/gvt/handlers.c | 1 -
> drivers/gpu/drm/i915/i915_debugfs.c | 18 +
> drivers/gpu/drm/i915/i915_drv.h | 3 +-
> drivers/gpu/drm/i915/i915_reg.h | 57 -
> drivers/gpu/drm/i915/intel_psr.c| 55
> 5 files changed, 83 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> b/drivers/gpu/drm/i915/gvt/handlers.c
> index 18f01eeb2510..749e3e4204f2 100644
> --- a/drivers/gpu/drm/i915/gvt/handlers.c
> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> @@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct intel_gvt
> *gvt)
> MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
>
> MMIO_D(WM_MISC, D_BDW);
> - MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
Change this to _SRD_CTL_EDP to keep the change non-functional? Any case, we'll
need an ack to modify this.
> MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5823ffb17821..2a0f5871e9a8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2470,7 +2470,7 @@ psr_source_status(struct drm_i915_private *dev_priv,
> struct seq_file *m)
> "BUF_ON",
> "TG_ON"
> };
> - val = I915_READ(EDP_PSR2_STATUS);
> + val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
> status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
> EDP_PSR2_STATUS_STATE_SHIFT;
> if (status_val < ARRAY_SIZE(live_status))
> @@ -2486,7 +2486,7 @@ psr_source_status(struct drm_i915_private *dev_priv,
> struct seq_file *m)
> "SRDOFFACK",
> "SRDENT_ON",
> };
> - val = I915_READ(EDP_PSR_STATUS);
> + val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
> status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
> EDP_PSR_STATUS_STATE_SHIFT;
> if (status_val < ARRAY_SIZE(live_status))
> @@ -2529,10 +2529,10 @@ static int i915_edp_psr_status(struct seq_file *m,
> void *data)
> goto unlock;
>
> if (psr->psr2_enabled) {
> - val = I915_READ(EDP_PSR2_CTL);
> + val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
> enabled = val & EDP_PSR2_ENABLE;
> } else {
> - val = I915_READ(EDP_PSR_CTL);
> + val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
> enabled = val & EDP_PSR_ENABLE;
> }
> seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
> @@ -2545,7 +2545,8 @@ static int i915_edp_psr_status(struct seq_file *m, void
> *data)
>* SKL+ Perf counter is reset to 0 everytime DC state is entered
>*/
> if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> - val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
> + val =
[Intel-gfx] [PATCH v5 3/3] drm/i915: Make PSR registers relative to transcoders
PSR registers are a mess, some have the full address while others just
have the additional offset from psr_mmio_base.
For BDW+ psr_mmio_base is nothing more than TRANSCODER_EDP_OFFSET +
0x800 and using it makes more difficult for people with an PSR
register address or PSR register name from from BSpec as i915 also
don't match the BSpec names.
For HSW psr_mmio_base is _DDI_BUF_CTL_A + 0x800 and PSR registers are
only available in DDIA.
Other reason to make relative to transcoder is that since BDW every
transcoder have PSR registers, so in theory it should be possible to
have PSR enabled in a non-eDP transcoder.
So for BDW+ we can use _TRANS2() to get the register offset of any
PSR register in any transcoder while for HSW we have _HSW_PSR_ADJ
that will calculate the register offset for the single PSR instance,
noting that we are already guarded about trying to enable PSR in other
port than DDIA on HSW by the 'if (dig_port->base.port != PORT_A)' in
intel_psr_compute_config(), this check should only be valid for HSW
and will be changed in future.
PSR2 registers and PSR_EVENT was added after Haswell so that is why
_PSR_ADJ() is not used in some macros.
The only registers that can not be relative to transcoder are
PSR_IMR and PSR_IIR that are not relative to anything, so keeping it
hardcoded.
Also removing BDW_EDP_PSR_BASE from GVT because it is not used as it
is the only PSR register that GVT have.
v5:
- Macros changed to be more explicit about HSW (Dhinakaran)
- Squashed with the patch that added the tran parameter to the
macros (Dhinakaran)
Cc: Dhinakaran Pandiyan
Cc: Rodrigo Vivi
Cc: Jani Nikula
Cc: Ville Syrjälä
Cc: Zhi Wang
Signed-off-by: José Roberto de Souza
---
drivers/gpu/drm/i915/gvt/handlers.c | 1 -
drivers/gpu/drm/i915/i915_debugfs.c | 18 +
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/i915_reg.h | 57 -
drivers/gpu/drm/i915/intel_psr.c| 55
5 files changed, 83 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
b/drivers/gpu/drm/i915/gvt/handlers.c
index 18f01eeb2510..749e3e4204f2 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -2776,7 +2776,6 @@ static int init_broadwell_mmio_info(struct intel_gvt *gvt)
MMIO_D(CHICKEN_PIPESL_1(PIPE_C), D_BDW_PLUS);
MMIO_D(WM_MISC, D_BDW);
- MMIO_D(_MMIO(BDW_EDP_PSR_BASE), D_BDW);
MMIO_D(_MMIO(0x6671c), D_BDW_PLUS);
MMIO_D(_MMIO(0x66c00), D_BDW_PLUS);
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5823ffb17821..2a0f5871e9a8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2470,7 +2470,7 @@ psr_source_status(struct drm_i915_private *dev_priv,
struct seq_file *m)
"BUF_ON",
"TG_ON"
};
- val = I915_READ(EDP_PSR2_STATUS);
+ val = I915_READ(EDP_PSR2_STATUS(dev_priv->psr.transcoder));
status_val = (val & EDP_PSR2_STATUS_STATE_MASK) >>
EDP_PSR2_STATUS_STATE_SHIFT;
if (status_val < ARRAY_SIZE(live_status))
@@ -2486,7 +2486,7 @@ psr_source_status(struct drm_i915_private *dev_priv,
struct seq_file *m)
"SRDOFFACK",
"SRDENT_ON",
};
- val = I915_READ(EDP_PSR_STATUS);
+ val = I915_READ(EDP_PSR_STATUS(dev_priv->psr.transcoder));
status_val = (val & EDP_PSR_STATUS_STATE_MASK) >>
EDP_PSR_STATUS_STATE_SHIFT;
if (status_val < ARRAY_SIZE(live_status))
@@ -2529,10 +2529,10 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
goto unlock;
if (psr->psr2_enabled) {
- val = I915_READ(EDP_PSR2_CTL);
+ val = I915_READ(EDP_PSR2_CTL(dev_priv->psr.transcoder));
enabled = val & EDP_PSR2_ENABLE;
} else {
- val = I915_READ(EDP_PSR_CTL);
+ val = I915_READ(EDP_PSR_CTL(dev_priv->psr.transcoder));
enabled = val & EDP_PSR_ENABLE;
}
seq_printf(m, "Source PSR ctl: %s [0x%08x]\n",
@@ -2545,7 +2545,8 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
* SKL+ Perf counter is reset to 0 everytime DC state is entered
*/
if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
- val = I915_READ(EDP_PSR_PERF_CNT) & EDP_PSR_PERF_CNT_MASK;
+ val = I915_READ(EDP_PSR_PERF_CNT(dev_priv->psr.transcoder));
+ val &= EDP_PSR_PERF_CNT_MASK;
seq_printf(m, "Performance counter: %u\n", val);
}
@@ -2563,8 +2564,11 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
* Reading all 3 registers before hand to minimize
