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.
