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

[John Harrison <John.C.Harrison@Intel.com>]

[-- 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 --]

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