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

[Anshuman Gupta <anshuman.gupta@intel.com>]

On 2020-03-05 at 08:47:42 +0530, Anshuman Gupta wrote:
> 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@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@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?
Hi Ville/Jani,
AFAIU for v2 preparation i should ignore the entire HPD if there is no eDP power nor vdd at
in intel_hpd_irq_handler(), or it is something additional would be required.
Thanks,
Anshuman.

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