From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn Hotplug Detection back on (v3) Date: Thu, 11 Apr 2013 17:48:09 +0300 Message-ID: <87txndcedy.fsf@intel.com> References: <1365499470-28646-1-git-send-email-eich@freedesktop.org> <1365499470-28646-6-git-send-email-eich@freedesktop.org> <8738uxpcrg.fsf@intel.com> <20130411131003.GA17947@debian> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 99BF3E614E for ; Thu, 11 Apr 2013 07:49:21 -0700 (PDT) In-Reply-To: <20130411131003.GA17947@debian> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Egbert Eich Cc: Egbert Eich , Daniel Vetter , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 11 Apr 2013, Egbert Eich 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.