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

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

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