From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Harrison Subject: Re: [PATCH 03/17] drm/i915: Allow the user to pass a context to any ring Date: Thu, 25 Aug 2016 16:28:38 +0100 Message-ID: <9a443765-4d36-efd1-7e41-ea0ec2b183fa@Intel.com> References: <20160822080350.4964-1-chris@chris-wilson.co.uk> <20160822080350.4964-4-chris@chris-wilson.co.uk> <1471865008.3601.12.camel@linux.intel.com> <20160822122326.GD856@nuc-i3427.alporthouse.com> <34bf93f4-a812-89f5-add1-4e399825e268@Intel.com> <29ae9aa0-7d4f-e643-6cd9-e80b7a468e51@Intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0842903058==" Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6E83E6E9B5 for ; Thu, 25 Aug 2016 15:28:41 +0000 (UTC) In-Reply-To: <29ae9aa0-7d4f-e643-6cd9-e80b7a468e51@Intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0842903058== Content-Type: multipart/alternative; boundary="------------09BA6180CDFD2041B5538DFE" This is a multi-part message in MIME format. --------------09BA6180CDFD2041B5538DFE Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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 >>>>> --- >>>>> 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 >> > > 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. --------------09BA6180CDFD2041B5538DFE Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit 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.

--------------09BA6180CDFD2041B5538DFE-- --===============0842903058== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0842903058==--