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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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?

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