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

[Ville Syrjälä]

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.

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