From mboxrd@z Thu Jan  1 00:00:00 1970
From: Egbert Eich <eich@freedesktop.org>
Subject: Re: [PATCH v3 5/7] drm/i915: Add Reenable Timer to turn
 Hotplug Detection back on (v3)
Date: Thu, 11 Apr 2013 15:10:03 +0200
Message-ID: <20130411131003.GA17947@debian>
References: <1365499470-28646-1-git-send-email-eich@freedesktop.org>
	<1365499470-28646-6-git-send-email-eich@freedesktop.org>
	<8738uxpcrg.fsf@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from moutng.kundenserver.de (moutng.kundenserver.de
	[212.227.126.187])
	by gabe.freedesktop.org (Postfix) with ESMTP id 5E654E65FF
	for <intel-gfx@lists.freedesktop.org>;
	Thu, 11 Apr 2013 06:10:08 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <8738uxpcrg.fsf@intel.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=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: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Egbert Eich <eich@suse.de>, Daniel Vetter <daniel.vetter@intel.com>, intel-gfx@lists.freedesktop.org
List-Id: intel-gfx@lists.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.