Re: [PATCH 03/17] drm/i915: Allow the user to pass a context to any ring

John Harrison
2016-08-25T15:28:41Z

[-- Attachment #1.1: Type: text/plain, Size: 3212 bytes --]

On 23/08/2016 14:33, John Harrison wrote:
> On 23/08/2016 14:28, John Harrison wrote:
>> On 22/08/2016 13:23, Chris Wilson wrote:
>>> On Mon, Aug 22, 2016 at 02:23:28PM +0300, Joonas Lahtinen wrote:
>>>> On ma, 2016-08-22 at 09:03 +0100, Chris Wilson wrote:
>>>>> With full-ppgtt, we want the user to have full control over their memory
>>>>> layout, with a separate instance per context. Forcing them to use a
>>>>> shared memory layout for !RCS not only duplicates the amount of work we
>>>>> have to do, but also defeats the memory segregation on offer.
>>>>>
>>>>> Signed-off-by: Chris Wilson<chris@chris-wilson.co.uk>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 +----
>>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> index 8f9d5ad0cfd8..fb1a64738fb8 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>>> @@ -1250,12 +1250,9 @@ static struct i915_gem_context *
>>>>> i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>>>>> struct intel_engine_cs *engine, const u32 ctx_id)
>>>>> {
>>>>> - struct i915_gem_context *ctx = NULL;
>>>>> + struct i915_gem_context *ctx;
>>>>> struct i915_ctx_hang_stats *hs;
>>>>>
>>>>> - if (engine->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
>>>>> - return ERR_PTR(-EINVAL);
>>>>> -
>>>> One would think this existed due to lack of testing or bugs in early
>>>> hardware. Do we need to use IS_GEN or some other means of validation?
>>> No.
>>>
>>> This has nothing to do with the hardware logical state (that is found
>>> within intel_context and only enabled where appropriate). The
>>> i915_gem_context is the driver's segregation between clients. Not only
>>> is t required for tracking clients independently (currently hangstats,
>>> but the context would be the first place we start enforcing cgroups like
>>> controls), but it is vital for clients who want to control their memory
>>> layout without conflicts (with themselves and others).
>>> -Chris
>>>
>> It is also important for clients that want to submit lots of work in
>> parallel from a single application by using multiple contexts. Other
>> internal teams have been running with this patch for quite some time.
>> I believe the only reason it has not been merged upstream before (it
>> has been on the mailing list at least twice before that I know of)
>> was the argument of no open source user.
>>
>> Reviewed-by: John Harrison <john.c.harrison@intel.com>
>>
>
> Actually, just found a previous instance. It had an r-b from Daniel
> Thomas but Tvrtko vetoed it on the grounds of needing IGT coverage
> first - message id '<565EF558.5050705@linux.intel.com>' on Dec 2nd 2015.
>

Just had a quick look at gem_ctx_switch and that seems to notice the
change with this patch. Without it skips non-render engines, with it
runs a bunch of non-default context tests across all engines. Is that
sufficient to satisfy the IGT coverage requirement? Maybe with an update
to make it fail rather than skip if it can't use a non-default context?

John.

[-- Attachment #1.2: Type: text/html, Size: 4441 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]