Re: [PATCH v4 6/6] drm/i915: fix context/engine cleanup order

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>,
	intel-gfx@lists.freedesktop.org,
	Nick Hoath <nicholas.hoath@intel.com>,
	Mika Kuoppala <mika.kuoppala@linux.intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH v4 6/6] drm/i915: fix context/engine cleanup order
Date: Thu, 11 Feb 2016 13:36:22 +0000	[thread overview]
Message-ID: <20160211133622.GG29294@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20160130111707.GU24534@nuc-i3427.alporthouse.com>

On Sat, Jan 30, 2016 at 11:17:07AM +0000, Chris Wilson wrote:
> On Fri, Jan 29, 2016 at 07:19:31PM +0000, Dave Gordon wrote:
> > From: Nick Hoath <nicholas.hoath@intel.com>
> > 
> > Swap the order of context & engine cleanup, so that contexts are cleaned
> > up first, and *then* engines. This is a more sensible order anyway, but
> > in particular has become necessary since the 'intel_ring_initialized()
> > must be simple and inline' patch, which now uses ring->dev as an
> > 'initialised' flag, so it can now be NULL after engine teardown. This in
> > turn can cause a problem in the context code, which (used to) check the
> > ring->dev->struct_mutex -- causing a fault if ring->dev was NULL.
> > 
> > Also rename the cleanup function to reflect what it actually does
> > (cleanup engines, not a ringbuffer), and fix an annoying whitespace
> > issue.
> > 
> > v2: Also make the fix in i915_load_modeset_init, not just in
> >     i915_driver_unload (Chris Wilson)
> > v3: Had extra stuff in it.
> > v4: Reverted extra stuff (so we're back to v2).
> >     Rebased and updated commentary above (Dave Gordon).
> > 
> > Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
> > Signed-off-by: David Gordon <david.s.gordon@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Have to drop that, with the recent context changes.
> 
> You have to move the gpu-reset now for execlists.
> 
> Basically pull it out into:
> 
> static void i915_unload_gem(struct drm_device *dev)
> {
>        /*
>         * Neither the BIOS, ourselves or any other kernel
>         * expects the system to be in execlists mode on startup,
>         * so we need to reset the GPU back to legacy mode. And the only
>         * known way to disable logical contexts is through a GPU reset.
>         *
>         * So in order to leave the system in a known default configration,
>         * always reset the GPU upon unload. This also cleans up the GEM
>         * state tracking, flushing off the requests and leaving the system
>         * idle.
>         *
>         * Note that is of the upmost importance that the GPU is idle and
>         * all stray writes are flushed *before* we dismantle the backing
>         * storage for the pinned objects.
>         */
>        i915_reset(dev);
> 
>        mutex_lock(&dev->struct_mutex);
>        i915_gem_context_fini(dev);
>        i915_gem_cleanup_ringbuffer(dev);
>        mutex_unlock(&dev->struct_mutex);
> }
> 
> and then kill the intel_gpu_reset along both the cleanup pathsh

It appears this patch was applied without dropping my r-b for the issue
I pointed out above.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx