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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> >
> > 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.
>
> 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
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
