[Intel-gfx] [PATCH] drm/i915: Gracefully handle obj not bound to GGTT in is_pin_display

[Mateo Lozano, Oscar]

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47


> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Monday, May 12, 2014 11:09 AM
> To: Mateo Lozano, Oscar
> Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Gracefully handle obj not bound to
> GGTT in is_pin_display
> 
> On Mon, May 12, 2014 at 09:05:45AM +0000, Mateo Lozano, Oscar wrote:
> > Hi Daniel,
> >
> > Sorry, this fell through the cracks:
> >
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Gracefully handle obj not
> > > bound to GGTT in is_pin_display
> > >
> > > On Wed, Apr 02, 2014 at 07:21:01PM +0100, oscar.mateo at intel.com
> wrote:
> > > > From: Oscar Mateo <oscar.mateo at intel.com>
> > > >
> > > > Otherwise, we do a NULL pointer dereference.
> > > >
> > > > I've seen this happen while handling an error in
> > > > i915_gem_object_pin_to_display_plane():
> > > >
> > > > If i915_gem_object_set_cache_level() fails, we call
> > > > is_pin_display() to handle the error. At this point, the object is
> > > > still not pinned to GGTT and maybe not even bound, so we have to
> > > > check before we dereference its GGTT vma.
> > > >
> > > > Issue: VIZ-3772
> > > > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > >
> > > Have you looked into provoking this with an igt testcase? On a hunch
> > > a busy load (to extend the race window) plus the usual interruptor
> > > trick to jump out of wait_seqno calls should be able to make this go
> > > kaboom on command. But I haven't analyzed the bug in detail.
> >
> > AFAICT, the only sequence where this likely to happen (because we are
> handling a recently created object) is:
> >
> > intelfb_alloc -> intel_pin_and_fence_fb_obj ->
> > i915_gem_object_pin_to_display_plane ->
> > i915_gem_object_set_cache_level -> is_pin_display
> >
> > Now, I think intelfb_alloc only happens during bootup, so reproducing it on
> IGT would be difficult.
> > I still feel the fix is valid though :)
> 
> Did you consider my alternative fix of restoring the old value in the error path?
> -Chris

Is that directed to Daniel or me? Restoring the old value is way easier, but I thought you wanted to keep is_pin_display as a theory of operation? And Daniel might still want an IGT test, I suppose...