Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3)

[Egbert Eich <eich@freedesktop.org>]

On Thu, Apr 11, 2013 at 01:44:51PM +0300, Jani Nikula wrote:
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +	for (i = (HPD_NONE + 1); i < HPD_NUM_PINS; i++) {
> > +		struct drm_connector *connector;
> > +
> > +		if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
> 
> I think this is wrong, skipping HPD_DISABLED.

Right, this was indeed wrong.

> 
> > +			continue;
> > +
> > +		dev_priv->hpd_stats[i].hpd_mark = HPD_ENABLED;
> > +
> > +		list_for_each_entry(connector, &mode_config->connector_list, head) {
> > +			struct intel_connector *intel_connector = to_intel_connector(connector);
> > +
> > +			if (intel_connector->encoder->hpd_pin == i) {
> > +				if (connector->polled != intel_connector->polled)
> > +					DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
> > +							 drm_get_connector_name(connector));
> > +				connector->polled = intel_connector->polled;
> > +				if (!connector->polled)
> > +					connector->polled = DRM_CONNECTOR_POLL_HPD;
> > +			}
> > +		}
> > +		dev_priv->display.hpd_irq_setup(dev);
> 
> You don't need to call this at each iteration, do you?

Right, you are right here as well.
> 
> > +	}
> 
> In fact, couldn't you just call intel_hpd_init(), or modify it to do
> what you want? Keep it simple.

Well, intel_hpd_init() is initializing every to dev_priv->hpd_stats[i].hpd_mark
to HPD_ENABLED. Can we rule out that the timer runs between the interrupt
handler and the worker - as in this state this variable might me set to
HPD_MARK_DISABLED?
In fact it would probably not even hurt if we changed the HPD_MARK_DISABLED
to HPD_ENABLED as in a storm condition this condition will soon reappear but
I'd rather avoid it.
Of course we could pass an argument to the function to distinguish both
conditions. This is a simplification which can still be introduced - when
I'm in fact able to do some testing.

Thanks a lot for the review!

Cheers,
	Egbert.