Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 1/3] drm/i915: Wake up pending_flip_queue as part of reset handling
Date: Mon, 18 Feb 2013 17:31:14 +0200	[thread overview]
Message-ID: <20130218153114.GP9135@intel.com> (raw)
In-Reply-To: <20130218124100.GO9135@intel.com>

On Mon, Feb 18, 2013 at 02:41:00PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 18, 2013 at 11:58:23AM +0200, Ville Syrjälä wrote:
> > On Fri, Feb 15, 2013 at 11:53:14PM +0000, Chris Wilson wrote:
> > > On Fri, Feb 15, 2013 at 05:07:44PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Someone may be waiting for a flip that will never complete due to a GPU
> > > > reset. Wake up all such waiters after the GPU reset processing has
> > > > finished.
> > > > 
> > > > v2: Dropped the wake_up_all() from i915_handle_error() since
> > > >     we no longer wait for pending flips with struct_mutex held.
> > > 
> > > Isn't the wake_up(pending_flip_queue) superseded by performing the
> > > explicit do_intel_finish_page_flip() in patch 3?
> > 
> > Yes that's correct. But I actually forgot to remove the wake_up patch
> > from my tree when I tested this. I'll run a few more tests just to make
> > sure it still works.
> 
> I just tried it w/o the wake_up_all() and unfortunately it hung :(
> 
> Need to think about it a bit more I suppose.

Well after a bit more though I think I know what happened:

1. page flip submitted on crtc which is not the first crtc in the list
2. mode set operation on any crtc (will grab all crtc locks)
3. reset handling loops over crtcs
 3.1 first crtc doesn't have a pending flip, so pending_flip_queue is not woken up
 3.2 code tries to grab first crtc mutex to update the base address
  -> deadlock

So either I need to keep the wake_up_all() call in the reset handling,
or I need to do two loops over the crtcs, first loop would complete all
page flips, and second loop would update the base address registers.
I don't really care which way we go. Anyone else have a preference?

-- 
Ville Syrjälä
Intel OTC