On Thu, 11 Apr 2013, Egbert Eich <eich at freedesktop.org> wrote:
> 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.
Yeah, let's go with this for now. R-b added.
One idea is to reuse the per-pin hpd_last_jiffies to check if enough
time has passed from the last storm on each pin. That could be done
here, or we could even throw out the timer, and check hpd_last_jiffies
on polling detect callbacks. Or maybe the latter is too spread out all
over the place. Perhaps you can think of something nice based on
this. ;)
> Thanks a lot for the review!
Thanks for doing this!
BR,
Jani.
>
> Cheers,
> Egbert.
