[Intel-gfx] [PATCH] drm/i915/edp: Ignore short pulse when panel powered off

[Anshuman Gupta]

On 2020-03-04 at 20:45:20 +0200, Ville Syrjälä wrote:
> On Wed, Mar 04, 2020 at 03:33:03PM +0200, Jani Nikula wrote:
> > On Wed, 04 Mar 2020, Anshuman Gupta <anshuman.gupta at intel.com> wrote:
> > > Few edp panels like Sharp is triggering short and long
> > > hpd pulse after panel is getting powered off.
> > > Currently driver is already ignoring long pulse for eDP
> > > panel but in order to process the short pulse, it turns on
> > > the VDD which requires panel power_cycle_delay + panel_power_on_delay
> > > these delay on Sharp panel introduced the responsiveness overhead
> > > of 800ms in the modeset sequence and as well is in suspend
> > > sequence.
> > > Ignoring any short pulse once panel is powered off.
> > >
> > > FIXME: It requires to wait for panel_power_off_delay in order
> > > to check the panel status, as panel triggers short pulse immediately
> > > after writing PP_OFF to PP_CTRL register.
> > >
> > > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 0a417cd2af2b..93de015f5322 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -6763,10 +6763,24 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = {
> > >  	.destroy = intel_dp_encoder_destroy,
> > >  };
> > >  
> > > +static bool is_edp_powered_off(struct intel_dp *intel_dp)
> > 
> > We have a number of existing edp_ prefixed functions, with intel_
> > prefixed wrappers. Please make this intel_edp_have_panel_power(). Early
> > return false for non-eDP.
> > 
> > Also handle intel_dp_is_edp() in the caller so it's clear what's being
> > done.
> > 
> > > +{
> > > +	intel_wakeref_t wakeref;
> > > +	bool powerd_off = false;
> > > +
> > > +	if (intel_dp_is_edp(intel_dp)) {
> > > +		with_pps_lock(intel_dp, wakeref)
> > > +			powerd_off = !edp_have_panel_power(intel_dp);
> > > +	}
> > > +
> > > +	return powerd_off;
> > > +}
> > > +
> > >  enum irqreturn
> > >  intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > >  {
> > >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > >  
> > >  	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP) {
> > >  		/*
> > > @@ -6810,6 +6824,11 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> > >  	if (!intel_dp->is_mst) {
> > >  		bool handled;
> > >  
> > > +		if (is_edp_powered_off(intel_dp)) {
> > 
> > I would move this to the beginning of the function in the same if
> > statement as the long_hpd handling:
> > 
> > 	if (intel_dp_is_edp(intel_dp) &&
> > 	    (long_hpd || !intel_edp_have_panel_power(intel_dp)))
> > 
> > But makes me wonder if that should be changed to ignore all hpd from eDP
> > if there's no panel power nor vdd. Ville?
> 
> From what I've seen for eDP no vdd implies HPD being deasserted, hence
> there is no way to signal short HPDs after vdd has been removed (apart
> from some glitchy HPD lines I guess). This case sounds like there could
> be a glitch of some sort on the HPD line while it is being deasserted
> and that triggers a short HPD. Either that or the panel is just stupid.
Hi Ville ,
I have observed that, similar panel doesn't trigger any short/long pulse on ICL but on TGL
it triggers both short and long pulse.
Thanks,
Anshuman.
> 
> Anyways, vdd and panel power are the same thing from the panel POV, so
> checking both would make sense. I can't actually imagine this even
> working correctly otherwise, unless there is an actual bug in our code
> and the spurious HPD only happens when we disable vdd via the state
> machine rather than the override bit. So which is it?
> 
> > 
> > > +			drm_info(&i915->drm, "edp panel is off, ignoring the short pulse\n");
> > 
> > drm_dbg_kms() will be enough.
> > 
> > > +			return IRQ_HANDLED;
> > > +		}
> > > +
> > >  		handled = intel_dp_short_pulse(intel_dp);
> > >  
> > >  		if (!handled)
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Ville Syrjälä
> Intel