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

[Dave Gordon <david.s.gordon@intel.com>]

On 11/02/16 15:02, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> 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.
>
> Now causes a splat in intel_logical_ring_cleanup when unloading module.
>
> Best to revert and rework on top of Dave's cleanup set?
> -Mika

This whole patchset was already superseded by a newer version before 
Daniel merged it. The newer version doesn't have Chris's R-B on this 
(context/engine) patch, only on two others that are still as they were
when he reviewed them.

Please go and look instead at [v5, 11 patches] posted 2016-02-05.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx