| Message ID | 20200313185227.21900-1-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/edp: Ignore short pulse when panel powered off | expand |
On Sat, 14 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. > > v2: > - checking vdd along with panel power to ignore the hpd. [Jani,Ville] > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 27 +++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 0a417cd2af2b..3475791d3bea 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -6763,21 +6763,36 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = { > .destroy = intel_dp_encoder_destroy, > }; > > +static bool intel_edp_have_power(struct intel_dp *intel_dp) > +{ > + intel_wakeref_t wakeref; > + bool powerd_on = false; Nitpick, "powerd" seems like a typo. "have_power" to reflect the name of the function? > + > + with_pps_lock(intel_dp, wakeref) { > + powerd_on = edp_have_panel_power(intel_dp) && > + edp_have_panel_vdd(intel_dp); > + } > + > + return powerd_on; > +} > + > 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) { > + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP && > + !intel_edp_have_power(intel_dp)) { This *enables* long hpd handling when eDP *does* have power. Not sure if we want that. Erring on the safe side, this might be a better option: if (intel_dig_port->base.type == INTEL_OUTPUT_EDP && (long_hpd || !intel_edp_have_power(intel_dp))) > /* > - * vdd off can generate a long pulse on eDP which > + * vdd off can generate a hpd pulse on eDP which > * would require vdd on to handle it, and thus we > * would end up in an endless cycle of > - * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..." > + * "vdd off -> hpd -> vdd on -> detect -> vdd off -> ..." > */ > - DRM_DEBUG_KMS("ignoring long hpd on eDP [ENCODER:%d:%s]\n", > - intel_dig_port->base.base.base.id, > - intel_dig_port->base.base.name); > + drm_dbg_kms(&i915->drm, "ignoring hpd on eDP [ENCODER:%d:%s]\n", > + intel_dig_port->base.base.base.id, > + intel_dig_port->base.base.name); I think we may want to know whether it was a short or long hpd that was ignored if we ever need to debug this again. (We will. ;) Something along the lines of "ignoring %s hpd ...", long_hpd ? "long" : "short". Please keep the old logging macro here, in case this needs backporting to older kernels. (In general not a huge fan of doing two independent things at once.) BR, Jani. > return IRQ_HANDLED; > }
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 0a417cd2af2b..3475791d3bea 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -6763,21 +6763,36 @@ static const struct drm_encoder_funcs intel_dp_enc_funcs = { .destroy = intel_dp_encoder_destroy, }; +static bool intel_edp_have_power(struct intel_dp *intel_dp) +{ + intel_wakeref_t wakeref; + bool powerd_on = false; + + with_pps_lock(intel_dp, wakeref) { + powerd_on = edp_have_panel_power(intel_dp) && + edp_have_panel_vdd(intel_dp); + } + + return powerd_on; +} + 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) { + if (intel_dig_port->base.type == INTEL_OUTPUT_EDP && + !intel_edp_have_power(intel_dp)) { /* - * vdd off can generate a long pulse on eDP which + * vdd off can generate a hpd pulse on eDP which * would require vdd on to handle it, and thus we * would end up in an endless cycle of - * "vdd off -> long hpd -> vdd on -> detect -> vdd off -> ..." + * "vdd off -> hpd -> vdd on -> detect -> vdd off -> ..." */ - DRM_DEBUG_KMS("ignoring long hpd on eDP [ENCODER:%d:%s]\n", - intel_dig_port->base.base.base.id, - intel_dig_port->base.base.name); + drm_dbg_kms(&i915->drm, "ignoring hpd on eDP [ENCODER:%d:%s]\n", + intel_dig_port->base.base.base.id, + intel_dig_port->base.base.name); return IRQ_HANDLED; }
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. v2: - checking vdd along with panel power to ignore the hpd. [Jani,Ville] Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 27 +++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)