Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Egbert Eich <eich@suse.de>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup
Date: Wed, 27 Mar 2013 16:42:01 +0100	[thread overview]
Message-ID: <20130327154201.GC2228@phenom.ffwll.local> (raw)
In-Reply-To: <20130327150554.GD4469@intel.com>

On Wed, Mar 27, 2013 at 05:05:54PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 27, 2013 at 03:55:01PM +0100, Daniel Vetter wrote:
> > This fixes a regression introduced in
> > 
> > commit e5868a318d1ae28f760f77bb91ce5deb751733fd
> > Author: Egbert Eich <eich@suse.de>
> > Date:   Thu Feb 28 04:17:12 2013 -0500
> > 
> >     DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encode
> > 
> > Due to the irq setup rework in 3.9, see
> > 
> > commit 20afbda209d708be66944907966486d0c1331cb8
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Tue Dec 11 14:05:07 2012 +0100
> > 
> >     drm/i915: Fixup hpd irq register setup ordering
> > 
> > Egbert Eich's hpd rework blows up on pch-split platforms - it walks
> > the encoder list before that has been set up completely. The new init
> > sequence is:
> > 
> > 1. irq enabling
> > 2. modeset init
> > 3. hpd setup
> > 
> > We need to move around the ibx setup a bit to fix this.
> > 
> > Ville Syrjälä pointed out in his review that we can't touch SDEIER
> > after the interrupt handler is set up, since that'll race with Paulo
> > Zanoni's PCH interrupt race fix:
> > 
> > commit 44498aea293b37af1d463acd9658cdce1ecdf427
> > Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Date:   Fri Feb 22 17:05:28 2013 -0300
> > 
> >     drm/i915: also disable south interrupts when handling them
> > 
> > We fix that by unconditionally enabling all interrupts in SDEIER, but
> > masking them as-needed in SDEIMR. Since only the single-threaded
> > setup/teardown (or suspend/resume) code touches that, no further
> > locking is required.
> > 
> > While at it also simplify the mask handling - we start out with all
> > interrupts cleared in the postinstall hook, and never enable a hpd
> > interrupt before hpd_irq_setup is called.
> > 
> > And finally, for consistency rename the ibx hpd setup function to
> > ibx_hpd_irq_setup.
> > 
> > v2: Fix race around SDEIER writes (Ville).
> > 
> > v3: Remove the superflous posting read for SDEIER, spotted by Ville.
> > 
> > Ville also wondered whether we shouldn't clear SDEIIR, since now
> > SDE interrupts are enabled before we have an irq handler installed.
> > But the master interrupt control bit in DEIER is still cleared, so we
> > should be fine.
> > 
> > Cc: Egbert Eich <eich@suse.de>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Makes sense to me.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Queued for -next, thanks for the patch. I've just pushed down the patch a
bit to keep the bisect regression window small, but not squashed - keeping
a record of our discussion seemed beneficial.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch