Re: [PATCH v3 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event

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

On Tue, 09 Apr 2013, Egbert Eich <eich@freedesktop.org> wrote:
> From: Egbert Eich <eich@suse.de>
>
> Instead of calling into the DRM helper layer to poll all connectors for
> changes in connected displays probe only those connectors which have
> received a hotplug event.

Thanks, we've all been waiting for someone(tm) to do this.

Apart from the bikesheds below,
Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 92041b9..7788536 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -334,6 +334,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
>  						     crtc);
>  }
>  
> +/**
> + * drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held
> + */

That kernel-doc line should include a one line description what the
function does, not what the preconditions are. You can achieve the same
with:

	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));

I'd also like to bikeshed the function name, because we'll be seeing it
a lot in the dmesgs through DRM_DEBUG_KMS. Just intel_hpd_irq_event()?

> +
> +static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct drm_connector *connector)
> +{
> +	enum drm_connector_status old_status;
> +
> +	old_status = connector->status;
> +
> +	connector->status = connector->funcs->detect(connector, false);
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n",
> +		      connector->base.id,
> +		      drm_get_connector_name(connector),
> +		      old_status, connector->status);

And while at it, another bikeshed about having different messages for
when old_status == connector->status vs. not. I've always found the
"status updated from 1 to 1" messages a bit dumb... ;)

> +	return (old_status != connector->status);
> +}
> +
>  /*
>   * Handle hotplug events outside the interrupt handler proper.
>   */
> @@ -348,6 +366,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  	struct drm_connector *connector;
>  	unsigned long irqflags;
>  	bool connector_disabled = false;
> +	bool changed = false;
>  	u32 hpd_event_bits;
>  
>  	/* HPD irq before everything is fully set up. */
> @@ -387,14 +406,20 @@ static void i915_hotplug_work_func(struct work_struct *work)
>  
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
> -	list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> -		if (intel_encoder->hot_plug)
> -			intel_encoder->hot_plug(intel_encoder);
> -
> +	list_for_each_entry(connector, &mode_config->connector_list, head) {
> +		intel_connector = to_intel_connector(connector);
> +		intel_encoder = intel_connector->encoder;
> +		if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> +			if (intel_encoder->hot_plug)
> +				intel_encoder->hot_plug(intel_encoder);
> +			if (intel_hpd_irq_single_connector_event(dev, connector))
> +				changed = true;
> +		}
> +	}
>  	mutex_unlock(&mode_config->mutex);
>  
> -	/* Just fire off a uevent and let userspace tell us what to do */
> -	drm_helper_hpd_irq_event(dev);
> +	if (changed)
> +		drm_kms_helper_hotplug_event(dev);
>  }
>  
>  static void ironlake_handle_rps_change(struct drm_device *dev)
> -- 
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx