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

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

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;
>  	}

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx