Re: [Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
On Wed, Aug 28, 2019 at 07:29:38PM +0300, Imre Deak wrote:
On Tue, Aug 27, 2019 at 09:50:24AM -0700, Lucas De Marchi wrote:
On Mon, Aug 26, 2019 at 08:28:33PM +0300, Imre Deak wrote:
> On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
> > From: José Roberto de Souza
> >
> > It was enabling and checking PSR interruptions in every transcoder
> > while it should keep the interruptions on the non-used transcoders
> > masked.
> >
> > This also already prepares for future when more than one PSR instance
> > will be allowed.
> >
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > Signed-off-by: Lucas De Marchi
> > ---
> > drivers/gpu/drm/i915/display/intel_psr.c | 140 +--
> > drivers/gpu/drm/i915/i915_reg.h | 13 +--
> > 2 files changed, 59 insertions(+), 94 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 28b62e587204..81e3619cd905 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -88,48 +88,23 @@ static bool intel_psr2_enabled(struct drm_i915_private
*dev_priv,
> > }
> > }
> >
> > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > +static void psr_irq_control(struct drm_i915_private *dev_priv)
> > {
> > - switch (cpu_transcoder) {
> > - case TRANSCODER_A:
> > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > - case TRANSCODER_B:
> > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > - case TRANSCODER_C:
> > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > - default:
> > - MISSING_CASE(cpu_transcoder);
> > - /* fallthrough */
> > - case TRANSCODER_EDP:
> > - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > - }
> > -}
> > + enum transcoder trans = dev_priv->psr.transcoder;
>
> This is called from intel_psr_debug_set() where psr.transcoder may be
> uninited.
>
> > + u32 val, mask;
> >
> > -static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> > -{
> > - u32 debug_mask, mask;
> > - enum transcoder cpu_transcoder;
> > - u32 transcoders = BIT(TRANSCODER_EDP);
> > -
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - transcoders |= BIT(TRANSCODER_A) |
> > -BIT(TRANSCODER_B) |
> > -BIT(TRANSCODER_C);
> > -
> > - debug_mask = 0;
> > - mask = 0;
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders)
{
> > - int shift = edp_psr_shift(cpu_transcoder);
> > -
> > - mask |= EDP_PSR_ERROR(shift);
> > - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > - EDP_PSR_PRE_ENTRY(shift);
> > - }
> > + mask = EDP_PSR_ERROR(trans);
> > + if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> > + mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
> >
> > - if (debug & I915_PSR_DEBUG_IRQ)
> > - mask |= debug_mask;
> > -
> > - I915_WRITE(EDP_PSR_IMR, ~mask);
> > + /*
> > + * TODO: when handling multiple PSR instances a global spinlock will
be
> > + * needed to synchronize the value of shared register
> > + */
> > + val = I915_READ(EDP_PSR_IMR);
> > + val &= ~EDP_PSR_TRANS_MASK(trans);
> > + val |= ~mask;
> > + I915_WRITE(EDP_PSR_IMR, val);
> > }
> >
> > static void psr_event_print(u32 val, bool psr2_enabled)
> > @@ -171,63 +146,54 @@ static void psr_event_print(u32 val, bool
psr2_enabled)
> >
> > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> > {
> > - u32 transcoders = BIT(TRANSCODER_EDP);
> > - enum transcoder cpu_transcoder;
> > + enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> > ktime_t time_ns = ktime_get();
> > - u32 mask = 0;
> >
> > - if (INTEL_GEN(dev_priv) >= 8)
> > - transcoders |= BIT(TRANSCODER_A) |
> > -BIT(TRANSCODER_B) |
> > -BIT(TRANSCODER_C);
> > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > + u32 val;
> >
> > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders)
{
>
> I think we should still catch all interrupts, log the unexpected ones
> and react only on the expected one in intel_psr_work().
could you expand more on this? there is only one PSR instance hence only
one possible transcoder coming from dev_priv->psr.transcoder. Looping here just
to
warn seems wasteful.
I think we should do what the HW tells us and make sure we clear all the
interrupts that may have happened rather than assume that the interrupt
that happened was the one corresponding to psr.transcoder.
if psr can only be enabled in a single transcoder, how the transcoder
could be any different for that interrupt?
psr.transcoder is also only protected by a mutex, so we can't use its
value in
Re: [Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
On Tue, Aug 27, 2019 at 09:50:24AM -0700, Lucas De Marchi wrote:
> On Mon, Aug 26, 2019 at 08:28:33PM +0300, Imre Deak wrote:
> > On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
> > > From: José Roberto de Souza
> > >
> > > It was enabling and checking PSR interruptions in every transcoder
> > > while it should keep the interruptions on the non-used transcoders
> > > masked.
> > >
> > > This also already prepares for future when more than one PSR instance
> > > will be allowed.
> > >
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: José Roberto de Souza
> > > Signed-off-by: Lucas De Marchi
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 140 +--
> > > drivers/gpu/drm/i915/i915_reg.h | 13 +--
> > > 2 files changed, 59 insertions(+), 94 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 28b62e587204..81e3619cd905 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -88,48 +88,23 @@ static bool intel_psr2_enabled(struct
> > > drm_i915_private *dev_priv,
> > > }
> > > }
> > >
> > > -static int edp_psr_shift(enum transcoder cpu_transcoder)
> > > +static void psr_irq_control(struct drm_i915_private *dev_priv)
> > > {
> > > - switch (cpu_transcoder) {
> > > - case TRANSCODER_A:
> > > - return EDP_PSR_TRANSCODER_A_SHIFT;
> > > - case TRANSCODER_B:
> > > - return EDP_PSR_TRANSCODER_B_SHIFT;
> > > - case TRANSCODER_C:
> > > - return EDP_PSR_TRANSCODER_C_SHIFT;
> > > - default:
> > > - MISSING_CASE(cpu_transcoder);
> > > - /* fallthrough */
> > > - case TRANSCODER_EDP:
> > > - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> > > - }
> > > -}
> > > + enum transcoder trans = dev_priv->psr.transcoder;
> >
> > This is called from intel_psr_debug_set() where psr.transcoder may be
> > uninited.
> >
> > > + u32 val, mask;
> > >
> > > -static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> > > -{
> > > - u32 debug_mask, mask;
> > > - enum transcoder cpu_transcoder;
> > > - u32 transcoders = BIT(TRANSCODER_EDP);
> > > -
> > > - if (INTEL_GEN(dev_priv) >= 8)
> > > - transcoders |= BIT(TRANSCODER_A) |
> > > -BIT(TRANSCODER_B) |
> > > -BIT(TRANSCODER_C);
> > > -
> > > - debug_mask = 0;
> > > - mask = 0;
> > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> > > - int shift = edp_psr_shift(cpu_transcoder);
> > > -
> > > - mask |= EDP_PSR_ERROR(shift);
> > > - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> > > - EDP_PSR_PRE_ENTRY(shift);
> > > - }
> > > + mask = EDP_PSR_ERROR(trans);
> > > + if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> > > + mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
> > >
> > > - if (debug & I915_PSR_DEBUG_IRQ)
> > > - mask |= debug_mask;
> > > -
> > > - I915_WRITE(EDP_PSR_IMR, ~mask);
> > > + /*
> > > + * TODO: when handling multiple PSR instances a global spinlock will be
> > > + * needed to synchronize the value of shared register
> > > + */
> > > + val = I915_READ(EDP_PSR_IMR);
> > > + val &= ~EDP_PSR_TRANS_MASK(trans);
> > > + val |= ~mask;
> > > + I915_WRITE(EDP_PSR_IMR, val);
> > > }
> > >
> > > static void psr_event_print(u32 val, bool psr2_enabled)
> > > @@ -171,63 +146,54 @@ static void psr_event_print(u32 val, bool
> > > psr2_enabled)
> > >
> > > void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32
> > > psr_iir)
> > > {
> > > - u32 transcoders = BIT(TRANSCODER_EDP);
> > > - enum transcoder cpu_transcoder;
> > > + enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> > > ktime_t time_ns = ktime_get();
> > > - u32 mask = 0;
> > >
> > > - if (INTEL_GEN(dev_priv) >= 8)
> > > - transcoders |= BIT(TRANSCODER_A) |
> > > -BIT(TRANSCODER_B) |
> > > -BIT(TRANSCODER_C);
> > > + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> > > + u32 val;
> > >
> > > - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> >
> > I think we should still catch all interrupts, log the unexpected ones
> > and react only on the expected one in intel_psr_work().
>
> could you expand more on this? there is only one PSR instance hence only
> one possible transcoder coming from dev_priv->psr.transcoder. Looping here
> just to
> warn seems wasteful.
I think we should do what the HW tells us and make sure we clear all the
interrupts that may have happened rather than assume that the interrupt
that happened was the one corresponding to psr.transcoder.
psr.transcoder is also only protected by a mutex, so we can't use its
value in the interrupt handler.
It's weird that there is no per-PSR instance IIRs in the misc interrupt
register. Because of that we'd need a PSR
Re: [Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
On Mon, Aug 26, 2019 at 08:28:33PM +0300, Imre Deak wrote:
On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
From: José Roberto de Souza
It was enabling and checking PSR interruptions in every transcoder
while it should keep the interruptions on the non-used transcoders
masked.
This also already prepares for future when more than one PSR instance
will be allowed.
Cc: Dhinakaran Pandiyan
Signed-off-by: José Roberto de Souza
Signed-off-by: Lucas De Marchi
---
drivers/gpu/drm/i915/display/intel_psr.c | 140 +--
drivers/gpu/drm/i915/i915_reg.h | 13 +--
2 files changed, 59 insertions(+), 94 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
b/drivers/gpu/drm/i915/display/intel_psr.c
index 28b62e587204..81e3619cd905 100644
--- a/drivers/gpu/drm/i915/display/intel_psr.c
+++ b/drivers/gpu/drm/i915/display/intel_psr.c
@@ -88,48 +88,23 @@ static bool intel_psr2_enabled(struct drm_i915_private
*dev_priv,
}
}
-static int edp_psr_shift(enum transcoder cpu_transcoder)
+static void psr_irq_control(struct drm_i915_private *dev_priv)
{
- switch (cpu_transcoder) {
- case TRANSCODER_A:
- return EDP_PSR_TRANSCODER_A_SHIFT;
- case TRANSCODER_B:
- return EDP_PSR_TRANSCODER_B_SHIFT;
- case TRANSCODER_C:
- return EDP_PSR_TRANSCODER_C_SHIFT;
- default:
- MISSING_CASE(cpu_transcoder);
- /* fallthrough */
- case TRANSCODER_EDP:
- return EDP_PSR_TRANSCODER_EDP_SHIFT;
- }
-}
+ enum transcoder trans = dev_priv->psr.transcoder;
This is called from intel_psr_debug_set() where psr.transcoder may be
uninited.
+ u32 val, mask;
-static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
-{
- u32 debug_mask, mask;
- enum transcoder cpu_transcoder;
- u32 transcoders = BIT(TRANSCODER_EDP);
-
- if (INTEL_GEN(dev_priv) >= 8)
- transcoders |= BIT(TRANSCODER_A) |
- BIT(TRANSCODER_B) |
- BIT(TRANSCODER_C);
-
- debug_mask = 0;
- mask = 0;
- for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
- int shift = edp_psr_shift(cpu_transcoder);
-
- mask |= EDP_PSR_ERROR(shift);
- debug_mask |= EDP_PSR_POST_EXIT(shift) |
- EDP_PSR_PRE_ENTRY(shift);
- }
+ mask = EDP_PSR_ERROR(trans);
+ if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
+ mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
- if (debug & I915_PSR_DEBUG_IRQ)
- mask |= debug_mask;
-
- I915_WRITE(EDP_PSR_IMR, ~mask);
+ /*
+* TODO: when handling multiple PSR instances a global spinlock will be
+* needed to synchronize the value of shared register
+*/
+ val = I915_READ(EDP_PSR_IMR);
+ val &= ~EDP_PSR_TRANS_MASK(trans);
+ val |= ~mask;
+ I915_WRITE(EDP_PSR_IMR, val);
}
static void psr_event_print(u32 val, bool psr2_enabled)
@@ -171,63 +146,54 @@ static void psr_event_print(u32 val, bool psr2_enabled)
void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
{
- u32 transcoders = BIT(TRANSCODER_EDP);
- enum transcoder cpu_transcoder;
+ enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
ktime_t time_ns = ktime_get();
- u32 mask = 0;
- if (INTEL_GEN(dev_priv) >= 8)
- transcoders |= BIT(TRANSCODER_A) |
- BIT(TRANSCODER_B) |
- BIT(TRANSCODER_C);
+ if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
+ u32 val;
- for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
I think we should still catch all interrupts, log the unexpected ones
and react only on the expected one in intel_psr_work().
could you expand more on this? there is only one PSR instance hence only
one possible transcoder coming from dev_priv->psr.transcoder. Looping here just
to
warn seems wasteful.
- int shift = edp_psr_shift(cpu_transcoder);
+ DRM_WARN("[transcoder %s] PSR aux error\n",
+transcoder_name(cpu_transcoder));
- if (psr_iir & EDP_PSR_ERROR(shift)) {
- DRM_WARN("[transcoder %s] PSR aux error\n",
-transcoder_name(cpu_transcoder));
+ dev_priv->psr.irq_aux_error = true;
- dev_priv->psr.irq_aux_error = true;
+ /*
+* If this interruption is not masked it will keep
+* interrupting so fast that it prevents the scheduled
+* work to run.
+* Also after a PSR error, we don't want to arm PSR
+* again so we don't care about unmask the interruption
+
Re: [Intel-gfx] [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
On Fri, Aug 23, 2019 at 01:20:35AM -0700, Lucas De Marchi wrote:
> From: José Roberto de Souza
>
> It was enabling and checking PSR interruptions in every transcoder
> while it should keep the interruptions on the non-used transcoders
> masked.
>
> This also already prepares for future when more than one PSR instance
> will be allowed.
>
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> Signed-off-by: Lucas De Marchi
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 140 +--
> drivers/gpu/drm/i915/i915_reg.h | 13 +--
> 2 files changed, 59 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index 28b62e587204..81e3619cd905 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -88,48 +88,23 @@ static bool intel_psr2_enabled(struct drm_i915_private
> *dev_priv,
> }
> }
>
> -static int edp_psr_shift(enum transcoder cpu_transcoder)
> +static void psr_irq_control(struct drm_i915_private *dev_priv)
> {
> - switch (cpu_transcoder) {
> - case TRANSCODER_A:
> - return EDP_PSR_TRANSCODER_A_SHIFT;
> - case TRANSCODER_B:
> - return EDP_PSR_TRANSCODER_B_SHIFT;
> - case TRANSCODER_C:
> - return EDP_PSR_TRANSCODER_C_SHIFT;
> - default:
> - MISSING_CASE(cpu_transcoder);
> - /* fallthrough */
> - case TRANSCODER_EDP:
> - return EDP_PSR_TRANSCODER_EDP_SHIFT;
> - }
> -}
> + enum transcoder trans = dev_priv->psr.transcoder;
This is called from intel_psr_debug_set() where psr.transcoder may be
uninited.
> + u32 val, mask;
>
> -static void psr_irq_control(struct drm_i915_private *dev_priv, u32 debug)
> -{
> - u32 debug_mask, mask;
> - enum transcoder cpu_transcoder;
> - u32 transcoders = BIT(TRANSCODER_EDP);
> -
> - if (INTEL_GEN(dev_priv) >= 8)
> - transcoders |= BIT(TRANSCODER_A) |
> -BIT(TRANSCODER_B) |
> -BIT(TRANSCODER_C);
> -
> - debug_mask = 0;
> - mask = 0;
> - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
> - int shift = edp_psr_shift(cpu_transcoder);
> -
> - mask |= EDP_PSR_ERROR(shift);
> - debug_mask |= EDP_PSR_POST_EXIT(shift) |
> - EDP_PSR_PRE_ENTRY(shift);
> - }
> + mask = EDP_PSR_ERROR(trans);
> + if (dev_priv->psr.debug & I915_PSR_DEBUG_IRQ)
> + mask |= EDP_PSR_POST_EXIT(trans) | EDP_PSR_PRE_ENTRY(trans);
>
> - if (debug & I915_PSR_DEBUG_IRQ)
> - mask |= debug_mask;
> -
> - I915_WRITE(EDP_PSR_IMR, ~mask);
> + /*
> + * TODO: when handling multiple PSR instances a global spinlock will be
> + * needed to synchronize the value of shared register
> + */
> + val = I915_READ(EDP_PSR_IMR);
> + val &= ~EDP_PSR_TRANS_MASK(trans);
> + val |= ~mask;
> + I915_WRITE(EDP_PSR_IMR, val);
> }
>
> static void psr_event_print(u32 val, bool psr2_enabled)
> @@ -171,63 +146,54 @@ static void psr_event_print(u32 val, bool psr2_enabled)
>
> void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir)
> {
> - u32 transcoders = BIT(TRANSCODER_EDP);
> - enum transcoder cpu_transcoder;
> + enum transcoder cpu_transcoder = dev_priv->psr.transcoder;
> ktime_t time_ns = ktime_get();
> - u32 mask = 0;
>
> - if (INTEL_GEN(dev_priv) >= 8)
> - transcoders |= BIT(TRANSCODER_A) |
> -BIT(TRANSCODER_B) |
> -BIT(TRANSCODER_C);
> + if (psr_iir & EDP_PSR_ERROR(cpu_transcoder)) {
> + u32 val;
>
> - for_each_cpu_transcoder_masked(dev_priv, cpu_transcoder, transcoders) {
I think we should still catch all interrupts, log the unexpected ones
and react only on the expected one in intel_psr_work().
> - int shift = edp_psr_shift(cpu_transcoder);
> + DRM_WARN("[transcoder %s] PSR aux error\n",
> + transcoder_name(cpu_transcoder));
>
> - if (psr_iir & EDP_PSR_ERROR(shift)) {
> - DRM_WARN("[transcoder %s] PSR aux error\n",
> - transcoder_name(cpu_transcoder));
> + dev_priv->psr.irq_aux_error = true;
>
> - dev_priv->psr.irq_aux_error = true;
> + /*
> + * If this interruption is not masked it will keep
> + * interrupting so fast that it prevents the scheduled
> + * work to run.
> + * Also after a PSR error, we don't want to arm PSR
> + * again so we don't care about unmask the interruption
> + * or unset irq_aux_error.
> + *
> + * TODO: when handling multiple PSR instances a global spinlock
> +
