Re: [Intel-gfx] [PATCH 02/11] drm/i915: Introduce intel_connector_hpd_pin()

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

On Tue, 21 Jan 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Simplify the hotplug code connector->encoder->hpd_pin handling
> by introducing a helper for exactly this purpose.
>
> In the helper we can neatly deal with the potential lack of an
> attached encoder on fresh MST connectors leaving the rest of the
> hpd code oblivious to such details.

I might want to see a WARN_ON(connector->mst_port && encoder->hpd_pin !=
HPD_NONE) added somewhere. But not necessarily in this patch.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 50 +++++++++++---------
>  1 file changed, 28 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index 042d98bae1ea..8a3e9e901cf7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -120,6 +120,20 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  #define HPD_STORM_REENABLE_DELAY	(2 * 60 * 1000)
>  #define HPD_RETRY_DELAY			1000
>  
> +static enum hpd_pin
> +intel_connector_hpd_pin(struct intel_connector *connector)
> +{
> +	struct intel_encoder *encoder = intel_attached_encoder(connector);
> +
> +	/*
> +	 * MST connectors get their encoder attached dynamically
> +	 * so need to make sure we have an encoder here. But since
> +	 * MST encoders have their hpd_pin set to HPD_NONE we don't
> +	 * have to special case them beyond that.
> +	 */
> +	return encoder ? encoder->hpd_pin : HPD_NONE;
> +}
> +
>  /**
>   * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a pin
>   * @dev_priv: private driver data pointer
> @@ -193,17 +207,12 @@ intel_hpd_irq_storm_switch_to_polling(struct drm_i915_private *dev_priv)
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	for_each_intel_connector_iter(connector, &conn_iter) {
> -		struct intel_encoder *encoder;
>  		enum hpd_pin pin;
>  
>  		if (connector->base.polled != DRM_CONNECTOR_POLL_HPD)
>  			continue;
>  
> -		encoder = intel_attached_encoder(connector);
> -		if (!encoder)
> -			continue;
> -
> -		pin = encoder->hpd_pin;
> +		pin = intel_connector_hpd_pin(connector);
>  		if (pin == HPD_NONE ||
>  		    dev_priv->hotplug.stats[pin].state != HPD_MARK_DISABLED)
>  			continue;
> @@ -250,9 +259,7 @@ static void intel_hpd_irq_storm_reenable_work(struct work_struct *work)
>  
>  		drm_connector_list_iter_begin(dev, &conn_iter);
>  		for_each_intel_connector_iter(connector, &conn_iter) {
> -			/* Don't check MST ports, they don't have pins */
> -			if (!connector->mst_port &&
> -			    intel_attached_encoder(connector)->hpd_pin == pin) {
> +			if (intel_connector_hpd_pin(connector) == pin) {
>  				if (connector->base.polled != connector->polled)
>  					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
>  							 connector->base.name);
> @@ -381,17 +388,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	for_each_intel_connector_iter(connector, &conn_iter) {
> -		struct intel_encoder *encoder =
> -			intel_attached_encoder(connector);
> +		enum hpd_pin pin;
>  		u32 hpd_bit;
>  
> -		if (!encoder)
> +		pin = intel_connector_hpd_pin(connector);
> +		if (pin == HPD_NONE)
>  			continue;
>  
> -		hpd_bit = BIT(encoder->hpd_pin);
> +		hpd_bit = BIT(pin);
>  		if ((hpd_event_bits | hpd_retry_bits) & hpd_bit) {
> +			struct intel_encoder *encoder =
> +				intel_attached_encoder(connector);
> +
>  			DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug event.\n",
> -				      connector->base.name, encoder->hpd_pin);
> +				      connector->base.name, pin);
>  
>  			switch (encoder->hotplug(encoder, connector,
>  						 hpd_event_bits & hpd_bit)) {
> @@ -606,20 +616,16 @@ static void i915_hpd_poll_init_work(struct work_struct *work)
>  
>  	drm_connector_list_iter_begin(dev, &conn_iter);
>  	for_each_intel_connector_iter(connector, &conn_iter) {
> +		enum hpd_pin pin = intel_connector_hpd_pin(connector);
> +
>  		connector->base.polled = connector->polled;
>  
> -		/* MST has a dynamic intel_connector->encoder and it's reprobing
> -		 * is all handled by the MST helpers. */
> -		if (connector->mst_port)
> -			continue;
> -
> -		if (!connector->base.polled && I915_HAS_HOTPLUG(dev_priv) &&
> -		    intel_attached_encoder(connector)->hpd_pin > HPD_NONE) {
> +		if (pin != HPD_NONE && I915_HAS_HOTPLUG(dev_priv) &&
> +		    !connector->base.polled)
>  			connector->base.polled = enabled ?
>  				DRM_CONNECTOR_POLL_CONNECT |
>  				DRM_CONNECTOR_POLL_DISCONNECT :
>  				DRM_CONNECTOR_POLL_HPD;
> -		}
>  	}
>  	drm_connector_list_iter_end(&conn_iter);

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