| Message ID | 20180601095215.29279-2-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Lionel Landwerlin (2018-06-01 10:52:14) > We currently using GuC as a proxy to the hardware. When Guc is used in > such mode, it consumes the bit 20 of the hw_id to indicate that the > workload was submitted by proxy. > > So far we probably haven't seen the issue because we need to allocate > 1048576+ contexts to hit this issue. Still, we should avoid allocating > the hw_id on that bit and restriction to bits [0:19] (i.e 20bits > instead of 21). > > v2: Leave the max hw_id computation in i915_gem_context.c (Michel) > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > BSpec: 1237 > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem_context.c | 14 +++++++++++--- > drivers/gpu/drm/i915/intel_lrc.c | 2 +- > 3 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 38157df6ff5c..7088a1c3b6ad 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1841,6 +1841,7 @@ struct drm_i915_private { > */ > struct ida hw_ida; > #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ > +#define MAX_GUC_CONTEXT_HW_ID (1<<20) /* exclusive */ > #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ > } contexts; > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 81f086397d10..fa732592e221 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -208,10 +208,18 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out) > int ret; > unsigned int max; > > - if (INTEL_GEN(dev_priv) >= 11) > + if (INTEL_GEN(dev_priv) >= 11) { > max = GEN11_MAX_CONTEXT_HW_ID; > - else > - max = MAX_CONTEXT_HW_ID; > + } else { > + /* > + * When using GuC in proxy submission, GuC consumes the > + * highest bit in the context id to indicate proxy submission. > + */ > + max = USES_GUC_SUBMISSION(dev_priv) ? > + MAX_GUC_CONTEXT_HW_ID : > + MAX_CONTEXT_HW_ID; I'm just going to say mixing if() and ternary ?: isn't great style. Acked-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 6/1/2018 8:10 AM, Chris Wilson wrote: > Quoting Lionel Landwerlin (2018-06-01 10:52:14) >> We currently using GuC as a proxy to the hardware. When Guc is used in >> such mode, it consumes the bit 20 of the hw_id to indicate that the >> workload was submitted by proxy. >> >> So far we probably haven't seen the issue because we need to allocate >> 1048576+ contexts to hit this issue. Still, we should avoid allocating >> the hw_id on that bit and restriction to bits [0:19] (i.e 20bits >> instead of 21). >> >> v2: Leave the max hw_id computation in i915_gem_context.c (Michel) >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> BSpec: 1237 >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/i915_gem_context.c | 14 +++++++++++--- >> drivers/gpu/drm/i915/intel_lrc.c | 2 +- >> 3 files changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 38157df6ff5c..7088a1c3b6ad 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1841,6 +1841,7 @@ struct drm_i915_private { >> */ >> struct ida hw_ida; >> #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ >> +#define MAX_GUC_CONTEXT_HW_ID (1<<20) /* exclusive */ >> #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ >> } contexts; >> >> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c >> index 81f086397d10..fa732592e221 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -208,10 +208,18 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out) >> int ret; >> unsigned int max; >> >> - if (INTEL_GEN(dev_priv) >= 11) >> + if (INTEL_GEN(dev_priv) >= 11) { >> max = GEN11_MAX_CONTEXT_HW_ID; >> - else >> - max = MAX_CONTEXT_HW_ID; >> + } else { >> + /* >> + * When using GuC in proxy submission, GuC consumes the >> + * highest bit in the context id to indicate proxy submission. >> + */ >> + max = USES_GUC_SUBMISSION(dev_priv) ? >> + MAX_GUC_CONTEXT_HW_ID : >> + MAX_CONTEXT_HW_ID; > > I'm just going to say mixing if() and ternary ?: isn't great style. > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> My r-b from v1 still applies, but since I'm already writing this... Reviewed-by: Michel Thierry <michel.thierry@intel.com>
On 01/06/18 20:24, Michel Thierry wrote: > On 6/1/2018 8:10 AM, Chris Wilson wrote: >> Quoting Lionel Landwerlin (2018-06-01 10:52:14) >>> We currently using GuC as a proxy to the hardware. When Guc is used in >>> such mode, it consumes the bit 20 of the hw_id to indicate that the >>> workload was submitted by proxy. >>> >>> So far we probably haven't seen the issue because we need to allocate >>> 1048576+ contexts to hit this issue. Still, we should avoid allocating >>> the hw_id on that bit and restriction to bits [0:19] (i.e 20bits >>> instead of 21). >>> >>> v2: Leave the max hw_id computation in i915_gem_context.c (Michel) >>> >>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> BSpec: 1237 >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 1 + >>> drivers/gpu/drm/i915/i915_gem_context.c | 14 +++++++++++--- >>> drivers/gpu/drm/i915/intel_lrc.c | 2 +- >>> 3 files changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h >>> b/drivers/gpu/drm/i915/i915_drv.h >>> index 38157df6ff5c..7088a1c3b6ad 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -1841,6 +1841,7 @@ struct drm_i915_private { >>> */ >>> struct ida hw_ida; >>> #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ >>> +#define MAX_GUC_CONTEXT_HW_ID (1<<20) /* exclusive */ >>> #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ >>> } contexts; >>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c >>> b/drivers/gpu/drm/i915/i915_gem_context.c >>> index 81f086397d10..fa732592e221 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>> @@ -208,10 +208,18 @@ static int assign_hw_id(struct >>> drm_i915_private *dev_priv, unsigned *out) >>> int ret; >>> unsigned int max; >>> - if (INTEL_GEN(dev_priv) >= 11) >>> + if (INTEL_GEN(dev_priv) >= 11) { >>> max = GEN11_MAX_CONTEXT_HW_ID; >>> - else >>> - max = MAX_CONTEXT_HW_ID; >>> + } else { >>> + /* >>> + * When using GuC in proxy submission, GuC consumes the >>> + * highest bit in the context id to indicate proxy >>> submission. >>> + */ >>> + max = USES_GUC_SUBMISSION(dev_priv) ? >>> + MAX_GUC_CONTEXT_HW_ID : >>> + MAX_CONTEXT_HW_ID; >> >> I'm just going to say mixing if() and ternary ?: isn't great style. >> >> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > My r-b from v1 still applies, but since I'm already writing this... > > Reviewed-by: Michel Thierry <michel.thierry@intel.com> > > > Thanks, I should have added it. -Lionel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 38157df6ff5c..7088a1c3b6ad 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1841,6 +1841,7 @@ struct drm_i915_private { */ struct ida hw_ida; #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */ +#define MAX_GUC_CONTEXT_HW_ID (1<<20) /* exclusive */ #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */ } contexts; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 81f086397d10..fa732592e221 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -208,10 +208,18 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out) int ret; unsigned int max; - if (INTEL_GEN(dev_priv) >= 11) + if (INTEL_GEN(dev_priv) >= 11) { max = GEN11_MAX_CONTEXT_HW_ID; - else - max = MAX_CONTEXT_HW_ID; + } else { + /* + * When using GuC in proxy submission, GuC consumes the + * highest bit in the context id to indicate proxy submission. + */ + max = USES_GUC_SUBMISSION(dev_priv) ? + MAX_GUC_CONTEXT_HW_ID : + MAX_CONTEXT_HW_ID; + } + ret = ida_simple_get(&dev_priv->contexts.hw_ida, 0, max, GFP_KERNEL); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 517e92c6a70b..d09d2b79552f 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -200,7 +200,7 @@ static inline bool need_preempt(const struct intel_engine_cs *engine, * * bits 0-11: flags, GEN8_CTX_* (cached in ctx->desc_template) * bits 12-31: LRCA, GTT address of (the HWSP of) this context - * bits 32-52: ctx ID, a globally unique tag + * bits 32-52: ctx ID, a globally unique tag (highest bit used by GuC) * bits 53-54: mbz, reserved for use by hardware * bits 55-63: group ID, currently unused and set to 0 *
We currently using GuC as a proxy to the hardware. When Guc is used in such mode, it consumes the bit 20 of the hw_id to indicate that the workload was submitted by proxy. So far we probably haven't seen the issue because we need to allocate 1048576+ contexts to hit this issue. Still, we should avoid allocating the hw_id on that bit and restriction to bits [0:19] (i.e 20bits instead of 21). v2: Leave the max hw_id computation in i915_gem_context.c (Michel) Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> BSpec: 1237 --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_context.c | 14 +++++++++++--- drivers/gpu/drm/i915/intel_lrc.c | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-)