Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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>

<snip>

-- 
Ville Syrjälä
Intel OTC