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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 18, 2013 at 05:31:14PM +0200, Ville Syrjälä wrote:
> 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?

I'd vote for the second approach of first waking everyone up, and then
doing the modeset updates. I guess you could even use
drm_modeset_lock_all, not really worth optimizing things here imo. Also
please add a comment to explain this ordering constraint in the reset
code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch