From: Imre Deak <imre.deak@intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use
Date: Wed, 28 Aug 2019 19:29:38 +0300 [thread overview]
Message-ID: <20190828162938.GA10584@ideak-desk.fi.intel.com> (raw)
In-Reply-To: <20190827165024.lwevu6e6sohr6lnh@ldmartin-desk1>
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 <jose.souza@intel.com>
> > >
> > > 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 <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > > 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 software IRQ mask that could
be set from psr_irq_control(). We also have to make sure to clear/mask a
transcoder's PSR interupts and sync against the interrupt handler when
turning off the transcoder power well. It looks like the transcoder
power well is the same as that of the transcoder's pipe power well, so
we could do this in gen8_irq_power_well_pre_disable().
By doing the above (not use psr.transcoder in the interrupt handler,
rather use a separate psr_irq software mask) we could also keep the
interrupt handling independent of the modeset code (which is what sets
psr.transcoder).
>
> >
> > > - 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
> > > + * will be needed to synchronize the value of shared register
>
> I'm not really a fan of these TODO for multiple PSR instances. When/if
> we add them, we won't really be able to rely on these TODO comments and
> will rather need to evaluate the whole scenario.
>
> > > + */
> > > + val = I915_READ(EDP_PSR_IMR);
> > > + val |= EDP_PSR_ERROR(cpu_transcoder);
> > > + I915_WRITE(EDP_PSR_IMR, val);
> > >
> > > - /*
> > > - * 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.
> > > - */
> > > - mask |= EDP_PSR_ERROR(shift);
> > > - }
> > > + schedule_work(&dev_priv->psr.work);
> >
> > Would be better not to reorder intel_psr_work() and printing the events
> > below.
> >
> > > + }
> > >
> > > - if (psr_iir & EDP_PSR_PRE_ENTRY(shift)) {
> > > - dev_priv->psr.last_entry_attempt = time_ns;
> > > - DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> > > - transcoder_name(cpu_transcoder));
> > > - }
> > > + if (psr_iir & EDP_PSR_PRE_ENTRY(cpu_transcoder)) {
> > > + dev_priv->psr.last_entry_attempt = time_ns;
> > > + DRM_DEBUG_KMS("[transcoder %s] PSR entry attempt in 2 vblanks\n",
> > > + transcoder_name(cpu_transcoder));
> > > + }
> > >
> > > - if (psr_iir & EDP_PSR_POST_EXIT(shift)) {
> > > - dev_priv->psr.last_exit = time_ns;
> > > - DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > - transcoder_name(cpu_transcoder));
> > > + if (psr_iir & EDP_PSR_POST_EXIT(cpu_transcoder)) {
> > > + dev_priv->psr.last_exit = time_ns;
> > > + DRM_DEBUG_KMS("[transcoder %s] PSR exit completed\n",
> > > + transcoder_name(cpu_transcoder));
> > >
> > > - if (INTEL_GEN(dev_priv) >= 9) {
> > > - u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> > > - bool psr2_enabled = dev_priv->psr.psr2_enabled;
> > > + if (INTEL_GEN(dev_priv) >= 9) {
> > > + u32 val = I915_READ(PSR_EVENT(cpu_transcoder));
> > > + bool psr2_enabled = dev_priv->psr.psr2_enabled;
> > >
> > > - I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> > > - psr_event_print(val, psr2_enabled);
> > > - }
> > > + I915_WRITE(PSR_EVENT(cpu_transcoder), val);
> > > + psr_event_print(val, psr2_enabled);
> > > }
> > > }
> > > -
> > > - if (mask) {
> > > - mask |= I915_READ(EDP_PSR_IMR);
> > > - I915_WRITE(EDP_PSR_IMR, mask);
> > > -
> > > - schedule_work(&dev_priv->psr.work);
> > > - }
> > > }
> > >
> > > static bool intel_dp_get_alpm_status(struct intel_dp *intel_dp)
> > > @@ -737,7 +703,7 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
> > >
> > > I915_WRITE(EDP_PSR_DEBUG(dev_priv->psr.transcoder), mask);
> > >
> > > - psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > + psr_irq_control(dev_priv);
> > > }
> > >
> > > static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> > > @@ -762,7 +728,7 @@ static void intel_psr_enable_locked(struct drm_i915_private *dev_priv,
> > > * to avoid any rendering problems.
> > > */
> > > val = I915_READ(EDP_PSR_IIR);
> > > - val &= EDP_PSR_ERROR(edp_psr_shift(dev_priv->psr.transcoder));
> > > + val &= EDP_PSR_ERROR(dev_priv->psr.transcoder);
> > > if (val) {
> > > dev_priv->psr.sink_not_reliable = true;
> > > DRM_DEBUG_KMS("PSR interruption error set, not enabling PSR\n");
> > > @@ -1110,7 +1076,7 @@ int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 val)
> > >
> > > old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK;
> > > dev_priv->psr.debug = val;
> > > - psr_irq_control(dev_priv, dev_priv->psr.debug);
> > > + psr_irq_control(dev_priv);
> > >
> > > mutex_unlock(&dev_priv->psr.lock);
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 02e1ef10c47e..1c6d99944630 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4225,13 +4225,12 @@ enum {
> > > /* Bspec claims those aren't shifted but stay at 0x64800 */
> > > #define EDP_PSR_IMR _MMIO(0x64834)
> > > #define EDP_PSR_IIR _MMIO(0x64838)
> > > -#define EDP_PSR_ERROR(shift) (1 << ((shift) + 2))
> > > -#define EDP_PSR_POST_EXIT(shift) (1 << ((shift) + 1))
> > > -#define EDP_PSR_PRE_ENTRY(shift) (1 << (shift))
> > > -#define EDP_PSR_TRANSCODER_C_SHIFT 24
> > > -#define EDP_PSR_TRANSCODER_B_SHIFT 16
> > > -#define EDP_PSR_TRANSCODER_A_SHIFT 8
> > > -#define EDP_PSR_TRANSCODER_EDP_SHIFT 0
> > > +#define _EDP_PSR_TRANS_SHIFT(trans) ((trans) == TRANSCODER_EDP ? \
> > > + 0 : ((trans) + 1) * 8)
> >
> > (trans - TRANSCODER_A) + 1
> >
> > to not depend on the enum value of TRANSCODER_A.
>
> agreed
>
> thanks
> Lucas De Marchi
>
> >
> > > +#define EDP_PSR_TRANS_MASK(trans) (0x7 << _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define EDP_PSR_ERROR(trans) (0x4 << _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define EDP_PSR_POST_EXIT(trans) (0x2 << _EDP_PSR_TRANS_SHIFT(trans))
> > > +#define EDP_PSR_PRE_ENTRY(trans) (0x1 << _EDP_PSR_TRANS_SHIFT(trans))
> > >
> > > #define _SRD_AUX_CTL_A 0x60810
> > > #define _SRD_AUX_CTL_EDP 0x6f810
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-08-28 16:30 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-23 8:20 [PATCH v3 00/23] Tiger Lake batch 3 Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 01/23] drm/i915/tgl: Move GTCR register to cope with GAM MMIO address remap Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 02/23] drm/i915/tgl: Enable VD HCP/MFX sub-pipe power gating Lucas De Marchi
2019-09-13 7:06 ` Chris Wilson
2019-08-23 8:20 ` [PATCH v3 03/23] drm/i915/psr: Only handle interruptions of the transcoder in use Lucas De Marchi
2019-08-26 17:28 ` Imre Deak
2019-08-27 16:50 ` Lucas De Marchi
2019-08-28 16:29 ` Imre Deak [this message]
2019-08-28 22:16 ` Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 04/23] drm/i915/bdw+: Enable PSR in any eDP port Lucas De Marchi
2019-08-26 13:41 ` Imre Deak
2019-08-26 17:43 ` Runyan, Arthur J
2019-08-27 16:36 ` Lucas De Marchi
2019-08-27 17:55 ` Souza, Jose
2019-08-23 8:20 ` [PATCH v3 05/23] drm/i915: Guard and warn if more than one eDP panel is present Lucas De Marchi
2019-08-26 6:41 ` Anshuman Gupta
2019-08-23 8:20 ` [PATCH v3 06/23] drm/i915: Do not read PSR2 register in transcoders without PSR2 Lucas De Marchi
2019-08-26 14:21 ` Imre Deak
2019-08-26 16:32 ` Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 07/23] drm/i915/tgl: PSR link standby is not supported anymore Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 08/23] drm/i915/tgl: Access the right register when handling PSR interruptions Lucas De Marchi
2019-08-26 9:53 ` Anshuman Gupta
2019-08-26 16:56 ` Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 09/23] drm/i915/tgl: Add maximum resolution supported by PSR2 HW Lucas De Marchi
2019-08-24 11:06 ` Anshuman Gupta
2019-08-26 17:10 ` Lucas De Marchi
2019-08-26 17:17 ` Souza, Jose
2019-08-26 17:29 ` Lucas De Marchi
2019-08-26 17:33 ` Gupta, Anshuman
2019-08-23 8:20 ` [PATCH v3 10/23] drm/i915: Add for_each_new_intel_connector_in_state() Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 11/23] drm: Add for_each_oldnew_intel_crtc_in_state_reverse() Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 12/23] drm/i915: Disable pipes in reverse order Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 13/23] FIXME: drm/i915/tgl: Select master transcoder in DP MST Lucas De Marchi
2019-08-23 13:02 ` Ville Syrjälä
2019-08-23 8:20 ` [PATCH v3 14/23] drm/i915/tgl: move DP_TP_* to transcoder Lucas De Marchi
2019-08-23 12:25 ` Ville Syrjälä
2019-08-23 12:39 ` Ville Syrjälä
2019-08-23 8:20 ` [PATCH v3 15/23] drm/i915/tgl: Implement TGL DisplayPort training sequence Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 16/23] drm/i915/tgl: Do not apply WaIncreaseDefaultTLBEntries from GEN12 onwards Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 17/23] FIXME: drm/i915/tgl: Register state context definition for Gen12 Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 18/23] drm/i915/tgl/perf: use the same oa ctx_id format as icl Lucas De Marchi
2019-08-23 8:30 ` Lionel Landwerlin
2019-08-23 18:16 ` Umesh Nerlige Ramappa
2019-08-23 8:20 ` [PATCH v3 19/23] drm/i915/tgl: Gen-12 display loses Yf tiling and legacy CCS support Lucas De Marchi
2019-08-28 23:04 ` Matt Roper
2019-08-28 23:59 ` Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 20/23] drm/framebuffer/tgl: Format modifier for Intel Gen-12 render compression Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 21/23] drm/i915/tgl: Gen-12 render decompression Lucas De Marchi
2019-08-29 0:33 ` Matt Roper
2019-09-13 0:31 ` Sripada, Radhakrishna
2019-08-23 8:20 ` [PATCH v3 22/23] drm/framebuffer/tgl: Format modifier for Intel Gen-12 media compression Lucas De Marchi
2019-08-23 8:20 ` [PATCH v3 23/23] drm/i915/tgl: " Lucas De Marchi
2019-08-23 13:24 ` ✗ Fi.CI.CHECKPATCH: warning for Tiger Lake batch 3 (rev5) Patchwork
2019-08-23 13:28 ` ✗ Fi.CI.SPARSE: " Patchwork
2019-08-23 13:53 ` ✓ Fi.CI.BAT: success " Patchwork
2019-08-24 11:50 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190828162938.GA10584@ideak-desk.fi.intel.com \
--to=imre.deak@intel.com \
--cc=dhinakaran.pandiyan@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=lucas.demarchi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox