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

[Jani Nikula <jani.nikula@intel.com>]

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?

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