From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: Fixup hpd irq register setup ordering Date: Tue, 11 Dec 2012 13:06:47 +0100 Message-ID: <20121211120647.GR11556@phenom.ffwll.local> References: <1355080498-3870-1-git-send-email-daniel.vetter@ffwll.ch> <1355082121-15869-1-git-send-email-daniel.vetter@ffwll.ch> <20121210130143.5e8abcd0@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 1EC13E6285 for ; Tue, 11 Dec 2012 04:05:15 -0800 (PST) Received: by mail-ee0-f49.google.com with SMTP id c4so2134086eek.36 for ; Tue, 11 Dec 2012 04:05:15 -0800 (PST) Content-Disposition: inline In-Reply-To: <20121210130143.5e8abcd0@jbarnes-desktop> 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: Jesse Barnes Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Mon, Dec 10, 2012 at 01:01:43PM -0800, Jesse Barnes wrote: > On Sun, 9 Dec 2012 20:42:01 +0100 > Daniel Vetter wrote: > > > For GMCH platforms we set up the hpd irq registers in the irq > > postinstall hook. But since we only enable the irq sources we actually > > need in PORT_HOTPLUG_EN/STATUS, taking dev_priv->hotplug_supported_mask > > into account, no hpd interrupt sources is enabled since > > > > commit 52d7ecedac3f96fb562cb482c139015372728638 > > Author: Daniel Vetter > > Date: Sat Dec 1 21:03:22 2012 +0100 > > > > drm/i915: reorder setup sequence to have irqs for output setup > > > > Wrongly set-up interrupts also lead broken hw-based load-detection on > > at least GM45, resulting in ghost VGA/TV-out outputs. > > ..."can lead to broken"... or "also leads to" ? > > > + > > +void intel_hpd_init(struct drm_device *dev) > > +{ > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + if (dev_priv->display.hpd_irq_setup) > > + dev_priv->display.hpd_irq_setup(dev); > > + > > + /* > > + * Some ports require correctly set-up hpd registers for detection to > > + * work properly, e.g. VGA on gm45. Hence we can only set up the initial > > + * fbdev config after hpd irqs are fully enabled. Now we should scan for > > + * the initial config only once hotplug handling is enabled, but due to > > + * screwed-up locking around kms/fbdev init we can't protect the fdbev > > + * initial config scanning against hotplug events. Hence do this first > > + * and ignore the tiny window where we will loose hotplug notifactions. > > + */ > > + intel_fbdev_initial_config(dev); > > + > > + /* Only enable hotplug handling once the fbdev is fully set up. */ > > + dev_priv->enable_hotplug_processing = true; > > + > > + drm_kms_helper_poll_init(dev); > > +} > > I'd rather see these calls in load_modeset_init(). Having > fbdev_initial_config here at least is confusing, and since poll_init > depends on it... > > With those two nits: > > Reviewed-by: Jesse Barnes Bikeshedded patch queued for -next, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch