Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Chris Wilson]

Quoting Michel Thierry (2018-02-07 22:34:00)
> On 2/7/2018 1:05 PM, Chris Wilson wrote:
> > If we remove some hardcoded assumptions about the preempt context having
> > a fixed id, reserved from use by normal user contexts, we may only
> > allocate the i915_gem_context when required. Then the subsequent
> > decisions on using preemption reduce to having the preempt context
> > available.
> > 
> > v2: Include an assert that we don't allocate the preempt context twice.
> > 
> > Signed-off-by: Chris Wilson 
> > Cc: Michal Winiarski 
> > Cc: Tvrtko Ursulin 
> > Cc: Arkadiusz Hiler 
> > Cc: Mika Kuoppala 
> > Cc: Daniele Ceraolo Spurio 
> > Acked-by: Daniele Ceraolo Spurio 
> 
> Only this one was missing a r-b, but all 3:
> 
> Reviewed-by: Michel Thierry 

Pushed, thanks. Hopefully this makes the icl bifurcation tidier.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Chris Wilson]

Quoting Michel Thierry (2018-02-07 22:34:00)
> On 2/7/2018 1:05 PM, Chris Wilson wrote:
> > +static bool needs_preempt_context(struct drm_i915_private *i915)
> > +{
> > + return HAS_LOGICAL_RING_PREEMPTION(i915);
> > +}
> > +
> 
> Wasn't adding a function just for this an overkill?

Maybe over-engineering. Based on the discussion that we may not need a
preempt context for icl (I'm still skeptical), I thought having a
convenient slot to hook in the extra logic was worth the seeming
tautology.

> I agree it makes gem_contexts_init self-explanatory but there are still 
> some HAS_LOGICAL_RING_PREEMPTION in guc_init/fini_wq and gvt/scheduler.

A few ordering problems^W challenges prevented killing those remaining
HAS_LOGICAL_RING_PREEMPTION; afaict guc_init is making some decisions
before the contexts have been initialised.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Michel Thierry]

On 2/7/2018 1:05 PM, Chris Wilson wrote:

If we remove some hardcoded assumptions about the preempt context having
a fixed id, reserved from use by normal user contexts, we may only
allocate the i915_gem_context when required. Then the subsequent
decisions on using preemption reduce to having the preempt context
available.

v2: Include an assert that we don't allocate the preempt context twice.

Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Tvrtko Ursulin 
Cc: Arkadiusz Hiler 
Cc: Mika Kuoppala 
Cc: Daniele Ceraolo Spurio 
Acked-by: Daniele Ceraolo Spurio 


Only this one was missing a r-b, but all 3:

Reviewed-by: Michel Thierry 


---
  drivers/gpu/drm/i915/i915_gem_context.c  | 31 
  drivers/gpu/drm/i915/intel_engine_cs.c   |  6 ++---
  drivers/gpu/drm/i915/intel_guc_submission.c  | 24 +-
  drivers/gpu/drm/i915/intel_lrc.c | 17 -
  drivers/gpu/drm/i915/intel_ringbuffer.h  |  5 
  drivers/gpu/drm/i915/selftests/mock_gem_device.c |  6 -
  6 files changed, 48 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
b/drivers/gpu/drm/i915/i915_gem_context.c
index 8337d15bb0e5..dd9efb9d0e0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -449,12 +449,18 @@ destroy_kernel_context(struct i915_gem_context **ctxp)
i915_gem_context_free(ctx);
  }
  
+static bool needs_preempt_context(struct drm_i915_private *i915)

+{
+   return HAS_LOGICAL_RING_PREEMPTION(i915);
+}
+


Wasn't adding a function just for this an overkill?

I agree it makes gem_contexts_init self-explanatory but there are still 
some HAS_LOGICAL_RING_PREEMPTION in guc_init/fini_wq and gvt/scheduler.



  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
  {
struct i915_gem_context *ctx;
-   int err;
  
+	/* Reassure ourselves we are only called once */

GEM_BUG_ON(dev_priv->kernel_context);
+   GEM_BUG_ON(dev_priv->preempt_context);
  
  	INIT_LIST_HEAD(_priv->contexts.list);

INIT_WORK(_priv->contexts.free_work, contexts_free_worker);
@@ -468,8 +474,7 @@ int i915_gem_contexts_init(struct drm_i915_private 
*dev_priv)
ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
if (IS_ERR(ctx)) {
DRM_ERROR("Failed to create default global context\n");
-   err = PTR_ERR(ctx);
-   goto err;
+   return PTR_ERR(ctx);
}
/*
 * For easy recognisablity, we want the kernel context to be 0 and then
@@ -479,23 +484,18 @@ int i915_gem_contexts_init(struct drm_i915_private 
*dev_priv)
dev_priv->kernel_context = ctx;
  
  	/* highest priority; preempting task */

-   ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
-   if (IS_ERR(ctx)) {
-   DRM_ERROR("Failed to create default preempt context\n");
-   err = PTR_ERR(ctx);
-   goto err_kernel_context;
+   if (needs_preempt_context(dev_priv)) {
+   ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
+   if (!IS_ERR(ctx))
+   dev_priv->preempt_context = ctx;
+   else
+   DRM_ERROR("Failed to create preempt context; disabling 
preemption\n");
}
-   dev_priv->preempt_context = ctx;
  
  	DRM_DEBUG_DRIVER("%s context support initialized\n",

 dev_priv->engine[RCS]->context_size ? "logical" :
 "fake");
return 0;
-
-err_kernel_context:
-   destroy_kernel_context(_priv->kernel_context);
-err:
-   return err;
  }
  
  void i915_gem_contexts_lost(struct drm_i915_private *dev_priv)

@@ -521,7 +521,8 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
  {
lockdep_assert_held(>drm.struct_mutex);
  
-	destroy_kernel_context(>preempt_context);

+   if (i915->preempt_context)
+   destroy_kernel_context(>preempt_context);
destroy_kernel_context(>kernel_context);
  
  	/* Must free all deferred contexts (via flush_workqueue) first */

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
b/drivers/gpu/drm/i915/intel_engine_cs.c
index 7eebfbb95e89..bf634432c9c6 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -631,7 +631,7 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 * Similarly the preempt context must always be available so that
 * we can interrupt the engine at any time.
 */
-   if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
+   if (engine->i915->preempt_context) {
ring = 

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Chris Wilson]

Quoting Mika Kuoppala (2018-01-31 14:38:02)
> > @@ -1988,6 +1988,11 @@ static int logical_ring_init(struct intel_engine_cs 
> > *engine)
> >   engine->execlists.elsp =
> >   engine->i915->regs + i915_mmio_reg_offset(RING_ELSP(engine));
> >  
> > + engine->execlists.preempt_complete_status = ~0u;
> 
> This is to ensure that we catch a rogue status? Atleast we will
> bug on of preempting with this status as the EXECLIST_ACTIVE_PREEMPT
> will be false.

0 is the value for the i915->kernel_context. ~0u is currently invalid
and that looks to be the case (that at least it will never match an
active context) for the near future.

> > + if (engine->i915->preempt_context)
> > + engine->execlists.preempt_complete_status =
> > + 
> > upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
> 
> 
> We have not upgraded the descriptor yet, so just use preempt_context->hw_id;

The upper_32_bits should be set; and I think it is important that the
usage is consistent. If we change the upper_32_bits later, we should try
to remember to update the preempt_complete_status. I.e. I feel it is
vital to remember that the complete_status is the
upper_32_bits(lrc_desc) that just happens to be hw_id due to our limited
definition today.

> I would also like duplicate, from i915_gem_context.c, the assertion that
> hw_id needs to be <= INT_MAX but didn't find a good spot.

For what purpose? When defining lrc_desc checking that hw_id fits within
the field width would be sensible.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Mika Kuoppala]

Chris Wilson  writes:

> If we remove some hardcoded assumptions about the preempt context having
> a fixed id, reserved from use by normal user contexts, we may only
> allocate the i915_gem_context when required. Then the subsequent
> decisions on using preemption reduce to having the preempt context
> available.
>
> Signed-off-by: Chris Wilson 
> Cc: Michal Winiarski 
> Cc: Tvrtko Ursulin 
> Cc: Arkadiusz Hiler 
> Cc: Mika Kuoppala 
> Cc: Daniele Ceraolo Spurio 
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c  | 29 
> 
>  drivers/gpu/drm/i915/intel_engine_cs.c   |  6 ++---
>  drivers/gpu/drm/i915/intel_guc_submission.c  | 24 +++-
>  drivers/gpu/drm/i915/intel_lrc.c | 17 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  5 
>  drivers/gpu/drm/i915/selftests/mock_gem_device.c |  6 -
>  6 files changed, 46 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 648e7536ff51..d69c8f1a4e78 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -449,10 +449,14 @@ destroy_kernel_context(struct i915_gem_context **ctxp)
>   i915_gem_context_free(ctx);
>  }
>  
> +static bool needs_preempt_context(struct drm_i915_private *i915)
> +{
> + return HAS_LOGICAL_RING_PREEMPTION(i915);
> +}
> +
>  int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
>  {
>   struct i915_gem_context *ctx;
> - int err;
>  
>   GEM_BUG_ON(dev_priv->kernel_context);

Please consider adding
GEM_BUG_ON(dev_priv->preempt_context);

here even tho the kernel_context should act as a master guard for init ordering
errors. Even if no other than documenting that we expect a clean slate
in this regard also.

>  
> @@ -468,8 +472,7 @@ int i915_gem_contexts_init(struct drm_i915_private 
> *dev_priv)
>   ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
>   if (IS_ERR(ctx)) {
>   DRM_ERROR("Failed to create default global context\n");
> - err = PTR_ERR(ctx);
> - goto err;
> + return PTR_ERR(ctx);
>   }
>   /*
>* For easy recognisablity, we want the kernel context to be 0 and then
> @@ -479,23 +482,18 @@ int i915_gem_contexts_init(struct drm_i915_private 
> *dev_priv)
>   dev_priv->kernel_context = ctx;
>  
>   /* highest priority; preempting task */
> - ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
> - if (IS_ERR(ctx)) {
> - DRM_ERROR("Failed to create default preempt context\n");
> - err = PTR_ERR(ctx);
> - goto err_kernel_context;

It seems this error path has been wrong all along.

> + if (needs_preempt_context(dev_priv)) {
> + ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
> + if (!IS_ERR(ctx))
> + dev_priv->preempt_context = ctx;
> + else
> + DRM_ERROR("Failed to create preempt context; disabling 
> preemption\n");
>   }
> - dev_priv->preempt_context = ctx;
>  
>   DRM_DEBUG_DRIVER("%s context support initialized\n",
>dev_priv->engine[RCS]->context_size ? "logical" :
>"fake");
>   return 0;
> -
> -err_kernel_context:
> - destroy_kernel_context(_priv->kernel_context);
> -err:
> - return err;
>  }
>  
>  void i915_gem_contexts_lost(struct drm_i915_private *dev_priv)
> @@ -521,7 +519,8 @@ void i915_gem_contexts_fini(struct drm_i915_private *i915)
>  {
>   lockdep_assert_held(>drm.struct_mutex);
>  
> - destroy_kernel_context(>preempt_context);
> + if (i915->preempt_context)
> + destroy_kernel_context(>preempt_context);
>   destroy_kernel_context(>kernel_context);
>  
>   /* Must free all deferred contexts (via flush_workqueue) first */
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c 
> b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 7eebfbb95e89..bf634432c9c6 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -631,7 +631,7 @@ int intel_engine_init_common(struct intel_engine_cs 
> *engine)
>* Similarly the preempt context must always be available so that
>* we can interrupt the engine at any time.
>*/
> - if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
> + if (engine->i915->preempt_context) {
>   ring = engine->context_pin(engine,
>  engine->i915->preempt_context);
>   if (IS_ERR(ring)) {
> @@ -656,7 +656,7 @@ int intel_engine_init_common(struct intel_engine_cs 
> *engine)
>  err_breadcrumbs:
>