[Intel-gfx] [PATCH v3 1/3] drm/i915: simplify allocation of driver-internal requests

[Chris Wilson]

On Wed, Jan 13, 2016 at 06:46:08PM +0000, Dave Gordon wrote:
> On 13/01/16 13:41, Chris Wilson wrote:
> >On Wed, Jan 13, 2016 at 01:27:51PM +0000, Dave Gordon wrote:
> >>On 12/01/16 14:27, Chris Wilson wrote:
> >>>On Tue, Jan 12, 2016 at 01:56:48PM +0000, Chris Wilson wrote:
> >>>>But we were removing the engine->default_context as it complicated the
> >>>>rest of the code. I strongly prefer keeping the contexts explicit as
> >>>>context separation should be first and foremost in the driver.
> >>>
> >>>$ git grep kernel_context -- drivers/gpu/drm/i915/ | grep request_alloc
> >>>drivers/gpu/drm/i915/i915_gem_evict.c:		req = i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >>>drivers/gpu/drm/i915/intel_overlay.c:	return i915_gem_request_alloc(ring, dev_priv->kernel_context);
> >>>
> >>>Changing those *two* callsites to pass NULL seems on the odd side, and
> >>>at least for the eviction case discards important information.
> >>>-Chris
> >>
> >>Those specific lines won't be touched by my patch, as *they don't
> >>actually exist in today's drm-intel-nightly* branch. If you want to
> >>add *new* calls to i915_gem_request_alloc() such as the above then
> >>you're quite free to pass any context you want, whether it's a real
> >>user context, the default kernel context explicitly, if you think
> >>it's important that the reader know that that specific context will
> >>be used; or NULL if you don't care what context is used.
> >
> >They are the same calls as the ones you are patching. They are not new
> >calls, they are the only users of the kernel_context for emission. Which
> >is why I am suggesting a different series of steps to take in tidying
> >this up.
> >-Chris
> 
> As of now (i.e. pre-conversion of the default_context pointer),
> there are *eight* calls to i915_gem_request_alloc(), but NONE in
> i915_gem_evict.c:
> 
> drm-intel-nightly$ git grep -c
> 'i915_gem_request_alloc(.*default_context' -- drivers/gpu/drm/i915/
> drivers/gpu/drm/i915/i915_gem.c:3
> drivers/gpu/drm/i915/intel_display.c:1
> drivers/gpu/drm/i915/intel_overlay.c:4
> 
> So you must be looking in a different tree, presumably one where
> you've already done some other bunch of cleanups in a different
> order.

Exactly. I disagree with the notion of an implicit context, and have
outlined a course of action where there is only a single active user of
the kernel context (overlay). And if we talk nicely to Ville we could
eliminate the overlay as well. The other use of that kernel context for
request construction is then to provide a means to flush other contexts
from the GPU - we don't use it for anything other than pinned storage.

For execlists we can do away with the kernel context entirely, we don't
even need it for the HWS if we just switch to per-context seqno. (It is
not required for flushing the last context.) I have no idea what the guc
is doing with it though, or whether it is being set up in hardware just
because it is there (even though we never use it).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre