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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

> 
> 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx