Re: [PATCH] drm/i915: Force PSR exit when IRQ_HPD is detected on eDP.

Vivi, Rodrigo
2015-12-02T17:29:10Z

On Wed, 2015-12-02 at 11:42 +0200, Ville Syrjälä wrote:
> On Tue, Dec 01, 2015 at 07:44:00PM +0000, Vivi, Rodrigo wrote:
> > On Tue, 2015-12-01 at 20:56 +0200, Ville Syrjälä wrote:
> > > On Wed, Nov 18, 2015 at 11:19:06AM -0800, Rodrigo Vivi wrote:
> > > > According to VESA spec: "If a Source device receives and
> > > > IRQ_HPD
> > > > while in a PSR active state, and cannot identify what caused
> > > > the
> > > > IRQ_HPD to be generated, based on Sink device status registers,
> > > > the Source device can take implementation-specific action.
> > > > One such action can be to exit and then re-enter a PSR active
> > > > state."
> > > >
> > > > Since we aren't checking for any sink status registers and we
> > > > aren't looking for any other implementation-specific action,
> > > > in case we receive any IRQ_HPD and psr is active let's force
> > > > the exit and reschedule it back.
> > > >
> > > > v2: IRQ_HPD means short HPD so handle it correctly
> > > > as Ville pointed out.
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >
> > > Do we have a device that needs this for something?
> >
> > hopefully not. and I'm glad that I never had a case with PSR that I
> > needed it.... I was just trying to follow VESA spec a bit to avoid
> > issues.
> >
> > I've seeing short pulses with some KBL, but also that came along
> > with
> > bad flickerings so PSR shouldn't be the worst problem when this
> > short
> > pulse on port A happens ;)
> >
> > > Since it's totally
> > > implementation specific I would expect that something like VBT
> > > would
> > > tell us if we need to do something on random short HPDs from the
> > > panel.
> >
> > hm, I was more afraid from the Sink implementation side... if they
> > were
> > generating IRQ_HPD for some errors where they believe HW will send
> > a
> > frame update.
> >
> > >
> > > As far as the implementation goes, can't we just call
> > > intel_psr_flush()
> > > so that the frontbuffer tracking keeps in sync with the actual
> > > PSR
> > > state?
> >
> > that is a good idea... I believe when I first added this patch we
> > weren't relying fully on flush being inactivate+flush, but now with
> > all
> > other patches merged I believe this would be better.
> >
> > But what are your overall opinion now? should I come with a v3
> > using
> > the flush function or should we just ignore this patch?
>
> Dunno. I suppose it can't do too much harm. Well, unless there's a
> sink
> that keeps signalling something unrelated to us that we don't
> understand,
> and thus would kick it out of PSR all the time.

yeah, good point... So I will hold this one for now if we need I can
rework the v3 with flush and send again in the future.

Thanks,
Rodrigo.

>
> >
> > Thanks for your review and comments!
> >
> > >
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 3 +++
> > > > drivers/gpu/drm/i915/intel_drv.h | 1 +
> > > > drivers/gpu/drm/i915/intel_psr.c | 33
> > > > +++++++++++++++++++++++++++++++++
> > > > 3 files changed, 37 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index bec443a..ca7a798 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -5004,6 +5004,9 @@ intel_dp_hpd_pulse(struct
> > > > intel_digital_port
> > > > *intel_dig_port, bool long_hpd)
> > > > goto mst_fail;
> > > > }
> > > > } else {
> > > > + if (intel_dig_port->base.type ==
> > > > INTEL_OUTPUT_EDP)
> > > > + intel_psr_irq_hpd(dev);
> > > > +
> > > > if (intel_dp->is_mst) {
> > > > if
> > > > (intel_dp_check_mst_status(intel_dp) ==
> > > > -EINVAL)
> > > > goto mst_fail;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index ab5c147..1d61551 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1401,6 +1401,7 @@ void intel_psr_flush(struct drm_device
> > > > *dev,
> > > > void intel_psr_init(struct drm_device *dev);
> > > > void intel_psr_single_frame_update(struct drm_device *dev,
> > > > unsigned frontbuffer_bits);
> > > > +void intel_psr_irq_hpd(struct drm_device *dev);
> > > >
> > > > /* intel_runtime_pm.c */
> > > > int intel_power_domains_init(struct drm_i915_private *);
> > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > > b/drivers/gpu/drm/i915/intel_psr.c
> > > > index bc5ea2a..465d36b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -765,6 +765,39 @@ void intel_psr_flush(struct drm_device
> > > > *dev,
> > > > }
> > > >
> > > > /**
> > > > + * intel_psr_irq_hpd - Let PSR aware of IRQ_HPD
> > > > + * @dev: DRM device
> > > > + *
> > > > + * This function is called when IRQ_HPD is received on eDP.
> > > > + */
> > > > +void intel_psr_irq_hpd(struct drm_device *dev)
> > > > +{
> > > > + struct drm_i915_private *dev_priv = dev->dev_private;
> > > > + int delay_ms = HAS_DDI(dev) ? 100 : 500;
> > > > +
> > > > + mutex_lock(&dev_priv->psr.lock);
> > > > +
> > > > + /*
> > > > + * According to VESA spec "If a Source device receives
> > > > and
> > > > IRQ_HPD
> > > > + * while in a PSR active state, and cannot identify
> > > > what
> > > > caused the
> > > > + * IRQ_HPD to be generated, based on Sink device
> > > > status
> > > > registers,
> > > > + * the Source device can take implementation-specific
> > > > action.
> > > > + * One such action can be to exit and then re-enter a
> > > > PSR
> > > > active
> > > > + * state." Since we aren't checking for any sink
> > > > status
> > > > registers
> > > > + * and we aren't looking for any other implementation
> > > > -specific
> > > > + * action, in case we receive any IRQ_HPD and psr is
> > > > active let's
> > > > + * force the exit and reschedule it back.
> > > > + */
> > > > + if (dev_priv->psr.active) {
> > > > + intel_psr_exit(dev);
> > > > + schedule_delayed_work(&dev_priv->psr.work,
> > > > +
> > > > msecs_to_jiffies(delay_ms));
> > > > + }
> > > > +
> > > > + mutex_unlock(&dev_priv->psr.lock);
> > > > +}
> > > > +
> > > > +/**
> > > > * intel_psr_init - Init basic PSR work and mutex.
> > > > * @dev: DRM device
> > > > *
> > > > --
> > > > 2.4.3
> > >