[PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[Chris Wilson]

In the next patch, we may only conditionally allocate the preempt-client
if there is a global preempt context and so we need to be prepared in
case the preempt-client itself is NULL.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 1f3a8786bbdc..4ea65df05e02 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -832,10 +832,12 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
 	if (ret)
 		return ret;
 
-	ret = create_doorbell(guc->preempt_client);
-	if (ret) {
-		destroy_doorbell(guc->execbuf_client);
-		return ret;
+	if (guc->preempt_client) {
+		ret = create_doorbell(guc->preempt_client);
+		if (ret) {
+			destroy_doorbell(guc->execbuf_client);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -848,8 +850,11 @@ static void guc_clients_doorbell_fini(struct intel_guc *guc)
 	 * Instead of trying (in vain) to communicate with it, let's just
 	 * cleanup the doorbell HW and our internal state.
 	 */
-	__destroy_doorbell(guc->preempt_client);
-	__update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
+	if (guc->preempt_client) {
+		__destroy_doorbell(guc->preempt_client);
+		__update_doorbell_desc(guc->preempt_client,
+				       GUC_DOORBELL_INVALID);
+	}
 	__destroy_doorbell(guc->execbuf_client);
 	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
 }
@@ -998,10 +1003,11 @@ static void guc_clients_destroy(struct intel_guc *guc)
 {
 	struct intel_guc_client *client;
 
-	client = fetch_and_zero(&guc->execbuf_client);
-	guc_client_free(client);
-
 	client = fetch_and_zero(&guc->preempt_client);
+	if (client)
+		guc_client_free(client);
+
+	client = fetch_and_zero(&guc->execbuf_client);
 	guc_client_free(client);
 }
 
@@ -1160,7 +1166,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 	GEM_BUG_ON(!guc->execbuf_client);
 
 	guc_reset_wq(guc->execbuf_client);
-	guc_reset_wq(guc->preempt_client);
+	if (guc->preempt_client)
+		guc_reset_wq(guc->preempt_client);
 
 	err = intel_guc_sample_forcewake(guc);
 	if (err)
-- 
2.15.1

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

[PATCH v2 2/3] drm/i915: Move the scheduler feature bits into the purview of the engines

[Chris Wilson]

Rather than having the high level ioctl interface guess the underlying
implementation details, having the implementation declare what
capabilities it exports. We define an intel_driver_caps, similar to the
intel_device_info, which instead of trying to describe the HW gives
details on what the driver itself supports. This is then populated by
the engine backend for the new scheduler capability field for use
elsewhere.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tomasz Lis <tomasz.lis@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
 drivers/gpu/drm/i915/i915_drv.h          | 2 ++
 drivers/gpu/drm/i915/i915_gem.c          | 3 +++
 drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
 drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
 drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
 drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
 7 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1ec12add34b2..733f71637914 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = i915_gem_mmap_gtt_version();
 		break;
 	case I915_PARAM_HAS_SCHEDULER:
-		value = 0;
-		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
-			value |= I915_SCHEDULER_CAP_ENABLED;
-			value |= I915_SCHEDULER_CAP_PRIORITY;
-			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
-				value |= I915_SCHEDULER_CAP_PREEMPTION;
-		}
+		value = dev_priv->caps.scheduler;
 		break;
 
 	case I915_PARAM_MMAP_VERSION:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 388b72cccde4..ae434c9d2b4f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -468,6 +468,7 @@ struct i915_gpu_state {
 	u32 reset_count;
 	u32 suspend_count;
 	struct intel_device_info device_info;
+	struct intel_driver_caps driver_caps;
 	struct i915_params params;
 
 	struct i915_error_uc {
@@ -1808,6 +1809,7 @@ struct drm_i915_private {
 	struct kmem_cache *priorities;
 
 	const struct intel_device_info info;
+	struct intel_driver_caps caps;
 
 	/**
 	 * Data Stolen Memory - aka "i915 stolen memory" gives us the start and
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 062b21408698..ad7bd0dc57ee 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3223,8 +3223,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
 		 * start to complete all requests.
 		 */
 		engine->submit_request = nop_complete_submit_request;
+		engine->schedule = NULL;
 	}
 
+	i915->caps.scheduler = 0;
+
 	/*
 	 * Make sure no request can slip through without getting completed by
 	 * either this call here to intel_engine_init_global_seqno, or the one
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index a81351d9e3a6..a92b0c0377c7 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -573,11 +573,13 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
 }
 
 static void err_print_capabilities(struct drm_i915_error_state_buf *m,
-				   const struct intel_device_info *info)
+				   const struct intel_device_info *info,
+				   const struct intel_driver_caps *caps)
 {
 	struct drm_printer p = i915_error_printer(m);
 
 	intel_device_info_dump_flags(info, &p);
+	intel_driver_caps_print(caps, &p);
 }
 
 static void err_print_params(struct drm_i915_error_state_buf *m,
@@ -800,7 +802,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	if (error->display)
 		intel_display_print_error_state(m, error->display);
 
-	err_print_capabilities(m, &error->device_info);
+	err_print_capabilities(m, &error->device_info, &error->driver_caps);
 	err_print_params(m, &error->params);
 	err_print_uc(m, &error->uc);
 
@@ -1731,6 +1733,7 @@ static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
 	memcpy(&error->device_info,
 	       INTEL_INFO(dev_priv),
 	       sizeof(error->device_info));
+	error->driver_caps = dev_priv->caps;
 }
 
 static __always_inline void dup_param(const char *type, void *x)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index a2c16140169f..298f8996cc54 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -586,3 +586,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
 	/* Initialize command stream timestamp frequency */
 	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
 }
+
+void intel_driver_caps_print(const struct intel_driver_caps *caps,
+			     struct drm_printer *p)
+{
+	drm_printf(p, "scheduler: %x\n", caps->scheduler);
+}
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 9542018d11d0..71fdfb0451ef 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -167,6 +167,10 @@ struct intel_device_info {
 	} color;
 };
 
+struct intel_driver_caps {
+	unsigned int scheduler;
+};
+
 static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 {
 	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
@@ -182,4 +186,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
 void intel_device_info_dump_runtime(const struct intel_device_info *info,
 				    struct drm_printer *p);
 
+void intel_driver_caps_print(const struct intel_driver_caps *caps,
+			     struct drm_printer *p);
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2fa328d512fc..5390894001f0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1906,6 +1906,12 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->unpark = NULL;
 
 	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
+
+	engine->i915->caps.scheduler =
+		I915_SCHEDULER_CAP_ENABLED |
+		I915_SCHEDULER_CAP_PRIORITY;
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
 }
 
 static void
-- 
2.15.1

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

Re: [PATCH v2 2/3] drm/i915: Move the scheduler feature bits into the purview of the engines

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Rather than having the high level ioctl interface guess the underlying
> implementation details, having the implementation declare what
> capabilities it exports. We define an intel_driver_caps, similar to the
> intel_device_info, which instead of trying to describe the HW gives
> details on what the driver itself supports. This is then populated by
> the engine backend for the new scheduler capability field for use
> elsewhere.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
>  drivers/gpu/drm/i915/i915_drv.h          | 2 ++
>  drivers/gpu/drm/i915/i915_gem.c          | 3 +++
>  drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
>  drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
>  drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
>  drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
>  7 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ec12add34b2..733f71637914 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		value = i915_gem_mmap_gtt_version();
>  		break;
>  	case I915_PARAM_HAS_SCHEDULER:
> -		value = 0;
> -		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
> -			value |= I915_SCHEDULER_CAP_ENABLED;
> -			value |= I915_SCHEDULER_CAP_PRIORITY;
> -			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
> -				value |= I915_SCHEDULER_CAP_PREEMPTION;
> -		}
> +		value = dev_priv->caps.scheduler;

Use the shiny CAP_PRIORITY instead of rcs->schedule on the
I915_CONTEXT_PARAM_PRIORITY validation?

-Mika

>  		break;
>  
>  	case I915_PARAM_MMAP_VERSION:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 388b72cccde4..ae434c9d2b4f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -468,6 +468,7 @@ struct i915_gpu_state {
>  	u32 reset_count;
>  	u32 suspend_count;
>  	struct intel_device_info device_info;
> +	struct intel_driver_caps driver_caps;
>  	struct i915_params params;
>  
>  	struct i915_error_uc {
> @@ -1808,6 +1809,7 @@ struct drm_i915_private {
>  	struct kmem_cache *priorities;
>  
>  	const struct intel_device_info info;
> +	struct intel_driver_caps caps;
>  
>  	/**
>  	 * Data Stolen Memory - aka "i915 stolen memory" gives us the start and
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 062b21408698..ad7bd0dc57ee 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3223,8 +3223,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
>  		 * start to complete all requests.
>  		 */
>  		engine->submit_request = nop_complete_submit_request;
> +		engine->schedule = NULL;
>  	}
>  
> +	i915->caps.scheduler = 0;
> +
>  	/*
>  	 * Make sure no request can slip through without getting completed by
>  	 * either this call here to intel_engine_init_global_seqno, or the one
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index a81351d9e3a6..a92b0c0377c7 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -573,11 +573,13 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
>  }
>  
>  static void err_print_capabilities(struct drm_i915_error_state_buf *m,
> -				   const struct intel_device_info *info)
> +				   const struct intel_device_info *info,
> +				   const struct intel_driver_caps *caps)
>  {
>  	struct drm_printer p = i915_error_printer(m);
>  
>  	intel_device_info_dump_flags(info, &p);
> +	intel_driver_caps_print(caps, &p);
>  }
>  
>  static void err_print_params(struct drm_i915_error_state_buf *m,
> @@ -800,7 +802,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>  	if (error->display)
>  		intel_display_print_error_state(m, error->display);
>  
> -	err_print_capabilities(m, &error->device_info);
> +	err_print_capabilities(m, &error->device_info, &error->driver_caps);
>  	err_print_params(m, &error->params);
>  	err_print_uc(m, &error->uc);
>  
> @@ -1731,6 +1733,7 @@ static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
>  	memcpy(&error->device_info,
>  	       INTEL_INFO(dev_priv),
>  	       sizeof(error->device_info));
> +	error->driver_caps = dev_priv->caps;
>  }
>  
>  static __always_inline void dup_param(const char *type, void *x)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index a2c16140169f..298f8996cc54 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -586,3 +586,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
>  	/* Initialize command stream timestamp frequency */
>  	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
>  }
> +
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p)
> +{
> +	drm_printf(p, "scheduler: %x\n", caps->scheduler);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 9542018d11d0..71fdfb0451ef 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -167,6 +167,10 @@ struct intel_device_info {
>  	} color;
>  };
>  
> +struct intel_driver_caps {
> +	unsigned int scheduler;
> +};
> +
>  static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>  {
>  	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
> @@ -182,4 +186,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
>  void intel_device_info_dump_runtime(const struct intel_device_info *info,
>  				    struct drm_printer *p);
>  
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 2fa328d512fc..5390894001f0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1906,6 +1906,12 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>  	engine->unpark = NULL;
>  
>  	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> +
> +	engine->i915->caps.scheduler =
> +		I915_SCHEDULER_CAP_ENABLED |
> +		I915_SCHEDULER_CAP_PRIORITY;
> +	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
>  }
>  
>  static void
> -- 
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/3] drm/i915: Move the scheduler feature bits into the purview of the engines

[Chris Wilson]

Quoting Mika Kuoppala (2018-01-31 13:58:17)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Rather than having the high level ioctl interface guess the underlying
> > implementation details, having the implementation declare what
> > capabilities it exports. We define an intel_driver_caps, similar to the
> > intel_device_info, which instead of trying to describe the HW gives
> > details on what the driver itself supports. This is then populated by
> > the engine backend for the new scheduler capability field for use
> > elsewhere.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tomasz Lis <tomasz.lis@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
> >  drivers/gpu/drm/i915/i915_drv.h          | 2 ++
> >  drivers/gpu/drm/i915/i915_gem.c          | 3 +++
> >  drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
> >  drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
> >  drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
> >  drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
> >  7 files changed, 30 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1ec12add34b2..733f71637914 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >               value = i915_gem_mmap_gtt_version();
> >               break;
> >       case I915_PARAM_HAS_SCHEDULER:
> > -             value = 0;
> > -             if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
> > -                     value |= I915_SCHEDULER_CAP_ENABLED;
> > -                     value |= I915_SCHEDULER_CAP_PRIORITY;
> > -                     if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
> > -                             value |= I915_SCHEDULER_CAP_PREEMPTION;
> > -             }
> > +             value = dev_priv->caps.scheduler;
> 
> Use the shiny CAP_PRIORITY instead of rcs->schedule on the
> I915_CONTEXT_PARAM_PRIORITY validation?

Yeah, that makes sense.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Move the scheduler feature bits into the purview of the engines

[Chris Wilson]

Rather than having the high level ioctl interface guess the underlying
implementation details, having the implementation declare what
capabilities it exports. We define an intel_driver_caps, similar to the
intel_device_info, which instead of trying to describe the HW gives
details on what the driver itself supports. This is then populated by
the engine backend for the new scheduler capability field for use
elsewhere.

v2: Use caps.scheduler for validating CONTEXT_PARAM_SET_PRIORITY (Mika)
    One less assumption of engine[RCS] \o/

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tomasz Lis <tomasz.lis@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      | 1 +
 drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
 drivers/gpu/drm/i915/i915_drv.h          | 2 ++
 drivers/gpu/drm/i915/i915_gem.c          | 3 +++
 drivers/gpu/drm/i915/i915_gem_context.c  | 2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
 drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
 drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
 drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
 9 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9e44adef30f0..2bdce9fea671 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -49,6 +49,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
 
 	intel_device_info_dump_flags(info, &p);
 	intel_device_info_dump_runtime(info, &p);
+	intel_driver_caps_print(&dev_priv->caps, &p);
 
 	kernel_param_lock(THIS_MODULE);
 	i915_params_dump(&i915_modparams, &p);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1ec12add34b2..733f71637914 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = i915_gem_mmap_gtt_version();
 		break;
 	case I915_PARAM_HAS_SCHEDULER:
-		value = 0;
-		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
-			value |= I915_SCHEDULER_CAP_ENABLED;
-			value |= I915_SCHEDULER_CAP_PRIORITY;
-			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
-				value |= I915_SCHEDULER_CAP_PREEMPTION;
-		}
+		value = dev_priv->caps.scheduler;
 		break;
 
 	case I915_PARAM_MMAP_VERSION:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c676269ed843..24333042e1e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -468,6 +468,7 @@ struct i915_gpu_state {
 	u32 reset_count;
 	u32 suspend_count;
 	struct intel_device_info device_info;
+	struct intel_driver_caps driver_caps;
 	struct i915_params params;
 
 	struct i915_error_uc {
@@ -1809,6 +1810,7 @@ struct drm_i915_private {
 	struct kmem_cache *priorities;
 
 	const struct intel_device_info info;
+	struct intel_driver_caps caps;
 
 	/**
 	 * Data Stolen Memory - aka "i915 stolen memory" gives us the start and
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c049496e8757..8d732f97e23e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3222,8 +3222,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
 		 * start to complete all requests.
 		 */
 		engine->submit_request = nop_complete_submit_request;
+		engine->schedule = NULL;
 	}
 
+	i915->caps.scheduler = 0;
+
 	/*
 	 * Make sure no request can slip through without getting completed by
 	 * either this call here to intel_engine_init_global_seqno, or the one
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 648e7536ff51..8337d15bb0e5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -807,7 +807,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 
 			if (args->size)
 				ret = -EINVAL;
-			else if (!to_i915(dev)->engine[RCS]->schedule)
+			else if (!(to_i915(dev)->caps.scheduler & I915_SCHEDULER_CAP_PRIORITY))
 				ret = -ENODEV;
 			else if (priority > I915_CONTEXT_MAX_USER_PRIORITY ||
 				 priority < I915_CONTEXT_MIN_USER_PRIORITY)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index a81351d9e3a6..a92b0c0377c7 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -573,11 +573,13 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
 }
 
 static void err_print_capabilities(struct drm_i915_error_state_buf *m,
-				   const struct intel_device_info *info)
+				   const struct intel_device_info *info,
+				   const struct intel_driver_caps *caps)
 {
 	struct drm_printer p = i915_error_printer(m);
 
 	intel_device_info_dump_flags(info, &p);
+	intel_driver_caps_print(caps, &p);
 }
 
 static void err_print_params(struct drm_i915_error_state_buf *m,
@@ -800,7 +802,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
 	if (error->display)
 		intel_display_print_error_state(m, error->display);
 
-	err_print_capabilities(m, &error->device_info);
+	err_print_capabilities(m, &error->device_info, &error->driver_caps);
 	err_print_params(m, &error->params);
 	err_print_uc(m, &error->uc);
 
@@ -1731,6 +1733,7 @@ static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
 	memcpy(&error->device_info,
 	       INTEL_INFO(dev_priv),
 	       sizeof(error->device_info));
+	error->driver_caps = dev_priv->caps;
 }
 
 static __always_inline void dup_param(const char *type, void *x)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index a2c16140169f..298f8996cc54 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -586,3 +586,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
 	/* Initialize command stream timestamp frequency */
 	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
 }
+
+void intel_driver_caps_print(const struct intel_driver_caps *caps,
+			     struct drm_printer *p)
+{
+	drm_printf(p, "scheduler: %x\n", caps->scheduler);
+}
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 9542018d11d0..71fdfb0451ef 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -167,6 +167,10 @@ struct intel_device_info {
 	} color;
 };
 
+struct intel_driver_caps {
+	unsigned int scheduler;
+};
+
 static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
 {
 	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
@@ -182,4 +186,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
 void intel_device_info_dump_runtime(const struct intel_device_info *info,
 				    struct drm_printer *p);
 
+void intel_driver_caps_print(const struct intel_driver_caps *caps,
+			     struct drm_printer *p);
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 41419f249fc2..a52bd2525398 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1922,6 +1922,12 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->unpark = NULL;
 
 	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
+
+	engine->i915->caps.scheduler =
+		I915_SCHEDULER_CAP_ENABLED |
+		I915_SCHEDULER_CAP_PRIORITY;
+	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
 }
 
 static void
-- 
2.15.1

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

Re: [PATCH v2] drm/i915: Move the scheduler feature bits into the purview of the engines

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Rather than having the high level ioctl interface guess the underlying
> implementation details, having the implementation declare what
> capabilities it exports. We define an intel_driver_caps, similar to the
> intel_device_info, which instead of trying to describe the HW gives
> details on what the driver itself supports. This is then populated by
> the engine backend for the new scheduler capability field for use
> elsewhere.
>
> v2: Use caps.scheduler for validating CONTEXT_PARAM_SET_PRIORITY (Mika)
>     One less assumption of engine[RCS] \o/
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      | 1 +
>  drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
>  drivers/gpu/drm/i915/i915_drv.h          | 2 ++
>  drivers/gpu/drm/i915/i915_gem.c          | 3 +++
>  drivers/gpu/drm/i915/i915_gem_context.c  | 2 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
>  drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
>  drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
>  drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
>  9 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9e44adef30f0..2bdce9fea671 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -49,6 +49,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
>  
>  	intel_device_info_dump_flags(info, &p);
>  	intel_device_info_dump_runtime(info, &p);
> +	intel_driver_caps_print(&dev_priv->caps, &p);
>  
>  	kernel_param_lock(THIS_MODULE);
>  	i915_params_dump(&i915_modparams, &p);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ec12add34b2..733f71637914 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		value = i915_gem_mmap_gtt_version();
>  		break;
>  	case I915_PARAM_HAS_SCHEDULER:
> -		value = 0;
> -		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
> -			value |= I915_SCHEDULER_CAP_ENABLED;
> -			value |= I915_SCHEDULER_CAP_PRIORITY;
> -			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
> -				value |= I915_SCHEDULER_CAP_PREEMPTION;
> -		}
> +		value = dev_priv->caps.scheduler;
>  		break;
>  
>  	case I915_PARAM_MMAP_VERSION:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c676269ed843..24333042e1e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -468,6 +468,7 @@ struct i915_gpu_state {
>  	u32 reset_count;
>  	u32 suspend_count;
>  	struct intel_device_info device_info;
> +	struct intel_driver_caps driver_caps;
>  	struct i915_params params;
>  
>  	struct i915_error_uc {
> @@ -1809,6 +1810,7 @@ struct drm_i915_private {
>  	struct kmem_cache *priorities;
>  
>  	const struct intel_device_info info;
> +	struct intel_driver_caps caps;
>  
>  	/**
>  	 * Data Stolen Memory - aka "i915 stolen memory" gives us the start and
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c049496e8757..8d732f97e23e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3222,8 +3222,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
>  		 * start to complete all requests.
>  		 */
>  		engine->submit_request = nop_complete_submit_request;
> +		engine->schedule = NULL;
>  	}
>  
> +	i915->caps.scheduler = 0;
> +
>  	/*
>  	 * Make sure no request can slip through without getting completed by
>  	 * either this call here to intel_engine_init_global_seqno, or the one
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 648e7536ff51..8337d15bb0e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -807,7 +807,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  
>  			if (args->size)
>  				ret = -EINVAL;
> -			else if (!to_i915(dev)->engine[RCS]->schedule)
> +			else if (!(to_i915(dev)->caps.scheduler & I915_SCHEDULER_CAP_PRIORITY))
>  				ret = -ENODEV;
>  			else if (priority > I915_CONTEXT_MAX_USER_PRIORITY ||
>  				 priority < I915_CONTEXT_MIN_USER_PRIORITY)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index a81351d9e3a6..a92b0c0377c7 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -573,11 +573,13 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
>  }
>  
>  static void err_print_capabilities(struct drm_i915_error_state_buf *m,
> -				   const struct intel_device_info *info)
> +				   const struct intel_device_info *info,
> +				   const struct intel_driver_caps *caps)
>  {
>  	struct drm_printer p = i915_error_printer(m);
>  
>  	intel_device_info_dump_flags(info, &p);
> +	intel_driver_caps_print(caps, &p);
>  }
>  
>  static void err_print_params(struct drm_i915_error_state_buf *m,
> @@ -800,7 +802,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>  	if (error->display)
>  		intel_display_print_error_state(m, error->display);
>  
> -	err_print_capabilities(m, &error->device_info);
> +	err_print_capabilities(m, &error->device_info, &error->driver_caps);
>  	err_print_params(m, &error->params);
>  	err_print_uc(m, &error->uc);
>  
> @@ -1731,6 +1733,7 @@ static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
>  	memcpy(&error->device_info,
>  	       INTEL_INFO(dev_priv),
>  	       sizeof(error->device_info));
> +	error->driver_caps = dev_priv->caps;
>  }
>  
>  static __always_inline void dup_param(const char *type, void *x)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index a2c16140169f..298f8996cc54 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -586,3 +586,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
>  	/* Initialize command stream timestamp frequency */
>  	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
>  }
> +
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p)
> +{
> +	drm_printf(p, "scheduler: %x\n", caps->scheduler);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 9542018d11d0..71fdfb0451ef 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -167,6 +167,10 @@ struct intel_device_info {
>  	} color;
>  };
>  
> +struct intel_driver_caps {
> +	unsigned int scheduler;
> +};
> +
>  static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>  {
>  	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
> @@ -182,4 +186,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
>  void intel_device_info_dump_runtime(const struct intel_device_info *info,
>  				    struct drm_printer *p);
>  
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p);
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 41419f249fc2..a52bd2525398 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1922,6 +1922,12 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>  	engine->unpark = NULL;
>  
>  	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> +
> +	engine->i915->caps.scheduler =
> +		I915_SCHEDULER_CAP_ENABLED |
> +		I915_SCHEDULER_CAP_PRIORITY;
> +	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
>  }
>  
>  static void
> -- 
> 2.15.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Move the scheduler feature bits into the purview of the engines

[Lis, Tomasz]

So the functional purpose of this patch is to provide capabilities 
(including preemption status) within error information. I agree this is 
required.


On 2018-02-01 20:02, Chris Wilson wrote:
> Rather than having the high level ioctl interface guess the underlying
> implementation details, having the implementation declare what
> capabilities it exports. We define an intel_driver_caps, similar to the
> intel_device_info, which instead of trying to describe the HW gives
> details on what the driver itself supports. This is then populated by
> the engine backend for the new scheduler capability field for use
> elsewhere.
>
> v2: Use caps.scheduler for validating CONTEXT_PARAM_SET_PRIORITY (Mika)
>      One less assumption of engine[RCS] \o/
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Tomasz Lis <tomasz.lis@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      | 1 +
>   drivers/gpu/drm/i915/i915_drv.c          | 8 +-------
>   drivers/gpu/drm/i915/i915_drv.h          | 2 ++
>   drivers/gpu/drm/i915/i915_gem.c          | 3 +++
>   drivers/gpu/drm/i915/i915_gem_context.c  | 2 +-
>   drivers/gpu/drm/i915/i915_gpu_error.c    | 7 +++++--
>   drivers/gpu/drm/i915/intel_device_info.c | 6 ++++++
>   drivers/gpu/drm/i915/intel_device_info.h | 7 +++++++
>   drivers/gpu/drm/i915/intel_lrc.c         | 6 ++++++
>   9 files changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9e44adef30f0..2bdce9fea671 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -49,6 +49,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
>   
>   	intel_device_info_dump_flags(info, &p);
>   	intel_device_info_dump_runtime(info, &p);
> +	intel_driver_caps_print(&dev_priv->caps, &p);
>   
>   	kernel_param_lock(THIS_MODULE);
>   	i915_params_dump(&i915_modparams, &p);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1ec12add34b2..733f71637914 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -372,13 +372,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
>   		value = i915_gem_mmap_gtt_version();
>   		break;
>   	case I915_PARAM_HAS_SCHEDULER:
> -		value = 0;
> -		if (dev_priv->engine[RCS] && dev_priv->engine[RCS]->schedule) {
> -			value |= I915_SCHEDULER_CAP_ENABLED;
> -			value |= I915_SCHEDULER_CAP_PRIORITY;
> -			if (HAS_LOGICAL_RING_PREEMPTION(dev_priv))
> -				value |= I915_SCHEDULER_CAP_PREEMPTION;
> -		}
> +		value = dev_priv->caps.scheduler;
>   		break;
>   
>   	case I915_PARAM_MMAP_VERSION:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c676269ed843..24333042e1e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -468,6 +468,7 @@ struct i915_gpu_state {
>   	u32 reset_count;
>   	u32 suspend_count;
>   	struct intel_device_info device_info;
> +	struct intel_driver_caps driver_caps;
>   	struct i915_params params;
>   
>   	struct i915_error_uc {
> @@ -1809,6 +1810,7 @@ struct drm_i915_private {
>   	struct kmem_cache *priorities;
>   
>   	const struct intel_device_info info;
> +	struct intel_driver_caps caps;
>   
>   	/**
>   	 * Data Stolen Memory - aka "i915 stolen memory" gives us the start and
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c049496e8757..8d732f97e23e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3222,8 +3222,11 @@ void i915_gem_set_wedged(struct drm_i915_private *i915)
>   		 * start to complete all requests.
>   		 */
>   		engine->submit_request = nop_complete_submit_request;
> +		engine->schedule = NULL;
>   	}
>   
> +	i915->caps.scheduler = 0;
> +
>   	/*
>   	 * Make sure no request can slip through without getting completed by
>   	 * either this call here to intel_engine_init_global_seqno, or the one
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 648e7536ff51..8337d15bb0e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -807,7 +807,7 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>   
>   			if (args->size)
>   				ret = -EINVAL;
> -			else if (!to_i915(dev)->engine[RCS]->schedule)
> +			else if (!(to_i915(dev)->caps.scheduler & I915_SCHEDULER_CAP_PRIORITY))
>   				ret = -ENODEV;
>   			else if (priority > I915_CONTEXT_MAX_USER_PRIORITY ||
>   				 priority < I915_CONTEXT_MIN_USER_PRIORITY)
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index a81351d9e3a6..a92b0c0377c7 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -573,11 +573,13 @@ static void print_error_obj(struct drm_i915_error_state_buf *m,
>   }
>   
>   static void err_print_capabilities(struct drm_i915_error_state_buf *m,
> -				   const struct intel_device_info *info)
> +				   const struct intel_device_info *info,
> +				   const struct intel_driver_caps *caps)
>   {
>   	struct drm_printer p = i915_error_printer(m);
>   
>   	intel_device_info_dump_flags(info, &p);
> +	intel_driver_caps_print(caps, &p);
>   }
>   
>   static void err_print_params(struct drm_i915_error_state_buf *m,
> @@ -800,7 +802,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>   	if (error->display)
>   		intel_display_print_error_state(m, error->display);
>   
> -	err_print_capabilities(m, &error->device_info);
> +	err_print_capabilities(m, &error->device_info, &error->driver_caps);
>   	err_print_params(m, &error->params);
>   	err_print_uc(m, &error->uc);
>   
> @@ -1731,6 +1733,7 @@ static void i915_capture_gen_state(struct drm_i915_private *dev_priv,
>   	memcpy(&error->device_info,
>   	       INTEL_INFO(dev_priv),
>   	       sizeof(error->device_info));
> +	error->driver_caps = dev_priv->caps;
>   }
>   
>   static __always_inline void dup_param(const char *type, void *x)
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index a2c16140169f..298f8996cc54 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -586,3 +586,9 @@ void intel_device_info_runtime_init(struct intel_device_info *info)
>   	/* Initialize command stream timestamp frequency */
>   	info->cs_timestamp_frequency_khz = read_timestamp_frequency(dev_priv);
>   }
> +
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p)
> +{
> +	drm_printf(p, "scheduler: %x\n", caps->scheduler);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> index 9542018d11d0..71fdfb0451ef 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.h
> +++ b/drivers/gpu/drm/i915/intel_device_info.h
> @@ -167,6 +167,10 @@ struct intel_device_info {
>   	} color;
>   };
>   
> +struct intel_driver_caps {
> +	unsigned int scheduler;
> +};
> +
>   static inline unsigned int sseu_subslice_total(const struct sseu_dev_info *sseu)
>   {
>   	return hweight8(sseu->slice_mask) * hweight8(sseu->subslice_mask);
> @@ -182,4 +186,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
>   void intel_device_info_dump_runtime(const struct intel_device_info *info,
>   				    struct drm_printer *p);
>   
> +void intel_driver_caps_print(const struct intel_driver_caps *caps,
> +			     struct drm_printer *p);
> +
>   #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 41419f249fc2..a52bd2525398 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1922,6 +1922,12 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>   	engine->unpark = NULL;
>   
>   	engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> +
> +	engine->i915->caps.scheduler =
> +		I915_SCHEDULER_CAP_ENABLED |
> +		I915_SCHEDULER_CAP_PRIORITY;
> +	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
>   }
>   
>   static void

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

[PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 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);
 
@@ -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;
+	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(&dev_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(&i915->drm.struct_mutex);
 
-	destroy_kernel_context(&i915->preempt_context);
+	if (i915->preempt_context)
+		destroy_kernel_context(&i915->preempt_context);
 	destroy_kernel_context(&i915->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:
 	intel_engine_fini_breadcrumbs(engine);
 err_unpin_preempt:
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 err_unpin_kernel:
 	engine->context_unpin(engine, engine->i915->kernel_context);
@@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	if (engine->default_state)
 		i915_gem_object_put(engine->default_state);
 
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 4ea65df05e02..b43b58cc599b 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
 		goto unlock;
 
 	if (port_isset(port)) {
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
+		if (engine->i915->preempt_context) {
 			struct guc_preempt_work *preempt_work =
 				&engine->i915->guc.preempt_work[engine->id];
 
@@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
 	}
 	guc->execbuf_client = client;
 
-	client = guc_client_alloc(dev_priv,
-				  INTEL_INFO(dev_priv)->ring_mask,
-				  GUC_CLIENT_PRIORITY_KMD_HIGH,
-				  dev_priv->preempt_context);
-	if (IS_ERR(client)) {
-		DRM_ERROR("Failed to create GuC client for preemption!\n");
-		guc_client_free(guc->execbuf_client);
-		guc->execbuf_client = NULL;
-		return PTR_ERR(client);
+	if (dev_priv->preempt_context) {
+		client = guc_client_alloc(dev_priv,
+					  INTEL_INFO(dev_priv)->ring_mask,
+					  GUC_CLIENT_PRIORITY_KMD_HIGH,
+					  dev_priv->preempt_context);
+		if (IS_ERR(client)) {
+			DRM_ERROR("Failed to create GuC client for preemption!\n");
+			guc_client_free(guc->execbuf_client);
+			guc->execbuf_client = NULL;
+			return PTR_ERR(client);
+		}
+		guc->preempt_client = client;
 	}
-	guc->preempt_client = client;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 5390894001f0..221b62d72c72 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -161,7 +161,6 @@
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
 #define WA_TAIL_DWORDS 2
 #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
-#define PREEMPT_ID 0x1
 
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine);
@@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 		&engine->i915->preempt_context->engine[engine->id];
 	unsigned int n;
 
-	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
+	GEM_BUG_ON(engine->execlists.preempt_complete_status !=
+		   upper_32_bits(ce->lrc_desc));
 	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
 
 	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
@@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
 			goto unlock;
 
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
+		if (engine->i915->preempt_context &&
 		    rb_entry(rb, struct i915_priolist, node)->priority >
 		    max(last->priotree.priority, 0)) {
 			/*
@@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
 			if (status & GEN8_CTX_STATUS_COMPLETE &&
-			    buf[2*head + 1] == PREEMPT_ID) {
+			    buf[2*head + 1] == execlists->preempt_complete_status) {
 				GEM_TRACE("%s preempt-idle\n", engine->name);
 
 				execlists_cancel_port_requests(execlists);
@@ -1910,7 +1910,7 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->i915->caps.scheduler =
 		I915_SCHEDULER_CAP_ENABLED |
 		I915_SCHEDULER_CAP_PRIORITY;
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
 }
 
@@ -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;
+	if (engine->i915->preempt_context)
+		engine->execlists.preempt_complete_status =
+			upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
+
 	return 0;
 
 error:
@@ -2250,7 +2255,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 	if (!engine->default_state)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
-	if (ctx->hw_id == PREEMPT_ID)
+	if (ctx == ctx->i915->preempt_context)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 					   CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c5ff203e42d6..03be8995f415 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -279,6 +279,11 @@ struct intel_engine_execlists {
 	 * @csb_use_mmio: access csb through mmio, instead of hwsp
 	 */
 	bool csb_use_mmio;
+
+	/**
+	 * @preempt_complete_status: expected CSB upon completing preemption
+	 */
+	u32 preempt_complete_status;
 };
 
 #define INTEL_ENGINE_CS_MAX_NAME 8
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 1bc61f3f76fc..3175db70cc6e 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
 	if (!i915->kernel_context)
 		goto err_engine;
 
-	i915->preempt_context = mock_context(i915, NULL);
-	if (!i915->preempt_context)
-		goto err_kernel_context;
-
 	WARN_ON(i915_gemfs_init(i915));
 
 	return i915;
 
-err_kernel_context:
-	i915_gem_context_put(i915->kernel_context);
 err_engine:
 	for_each_engine(engine, i915, id)
 		mock_engine_free(engine);
-- 
2.15.1

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

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

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> 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 <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  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(&dev_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(&i915->drm.struct_mutex);
>  
> -	destroy_kernel_context(&i915->preempt_context);
> +	if (i915->preempt_context)
> +		destroy_kernel_context(&i915->preempt_context);
>  	destroy_kernel_context(&i915->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:
>  	intel_engine_fini_breadcrumbs(engine);
>  err_unpin_preempt:
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>  		engine->context_unpin(engine, engine->i915->preempt_context);
>  err_unpin_kernel:
>  	engine->context_unpin(engine, engine->i915->kernel_context);
> @@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>  	if (engine->default_state)
>  		i915_gem_object_put(engine->default_state);
>  
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>  		engine->context_unpin(engine, engine->i915->preempt_context);
>  	engine->context_unpin(engine, engine->i915->kernel_context);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 4ea65df05e02..b43b58cc599b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
>  		goto unlock;
>  
>  	if (port_isset(port)) {
> -		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
> +		if (engine->i915->preempt_context) {
>  			struct guc_preempt_work *preempt_work =
>  				&engine->i915->guc.preempt_work[engine->id];
>  
> @@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
>  	}
>  	guc->execbuf_client = client;
>  
> -	client = guc_client_alloc(dev_priv,
> -				  INTEL_INFO(dev_priv)->ring_mask,
> -				  GUC_CLIENT_PRIORITY_KMD_HIGH,
> -				  dev_priv->preempt_context);
> -	if (IS_ERR(client)) {
> -		DRM_ERROR("Failed to create GuC client for preemption!\n");
> -		guc_client_free(guc->execbuf_client);
> -		guc->execbuf_client = NULL;
> -		return PTR_ERR(client);
> +	if (dev_priv->preempt_context) {
> +		client = guc_client_alloc(dev_priv,
> +					  INTEL_INFO(dev_priv)->ring_mask,
> +					  GUC_CLIENT_PRIORITY_KMD_HIGH,
> +					  dev_priv->preempt_context);
> +		if (IS_ERR(client)) {
> +			DRM_ERROR("Failed to create GuC client for preemption!\n");
> +			guc_client_free(guc->execbuf_client);
> +			guc->execbuf_client = NULL;
> +			return PTR_ERR(client);
> +		}
> +		guc->preempt_client = client;
>  	}
> -	guc->preempt_client = client;
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5390894001f0..221b62d72c72 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -161,7 +161,6 @@
>  #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
>  #define WA_TAIL_DWORDS 2
>  #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> -#define PREEMPT_ID 0x1
>  
>  static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>  					    struct intel_engine_cs *engine);
> @@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>  		&engine->i915->preempt_context->engine[engine->id];
>  	unsigned int n;
>  
> -	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
> +	GEM_BUG_ON(engine->execlists.preempt_complete_status !=
> +		   upper_32_bits(ce->lrc_desc));
>  	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
>  
>  	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
> @@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
>  			goto unlock;
>  
> -		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
> +		if (engine->i915->preempt_context &&
>  		    rb_entry(rb, struct i915_priolist, node)->priority >
>  		    max(last->priotree.priority, 0)) {
>  			/*
> @@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long data)
>  			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
>  
>  			if (status & GEN8_CTX_STATUS_COMPLETE &&
> -			    buf[2*head + 1] == PREEMPT_ID) {
> +			    buf[2*head + 1] == execlists->preempt_complete_status) {
>  				GEM_TRACE("%s preempt-idle\n", engine->name);
>  
>  				execlists_cancel_port_requests(execlists);
> @@ -1910,7 +1910,7 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>  	engine->i915->caps.scheduler =
>  		I915_SCHEDULER_CAP_ENABLED |
>  		I915_SCHEDULER_CAP_PRIORITY;
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>  		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
>  }
>  
> @@ -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.

> +	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;

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.

-Mika

> +
>  	return 0;
>  
>  error:
> @@ -2250,7 +2255,7 @@ populate_lr_context(struct i915_gem_context *ctx,
>  	if (!engine->default_state)
>  		regs[CTX_CONTEXT_CONTROL + 1] |=
>  			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
> -	if (ctx->hw_id == PREEMPT_ID)
> +	if (ctx == ctx->i915->preempt_context)
>  		regs[CTX_CONTEXT_CONTROL + 1] |=
>  			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
>  					   CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index c5ff203e42d6..03be8995f415 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -279,6 +279,11 @@ struct intel_engine_execlists {
>  	 * @csb_use_mmio: access csb through mmio, instead of hwsp
>  	 */
>  	bool csb_use_mmio;
> +
> +	/**
> +	 * @preempt_complete_status: expected CSB upon completing preemption
> +	 */
> +	u32 preempt_complete_status;
>  };
>  
>  #define INTEL_ENGINE_CS_MAX_NAME 8
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 1bc61f3f76fc..3175db70cc6e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
>  	if (!i915->kernel_context)
>  		goto err_engine;
>  
> -	i915->preempt_context = mock_context(i915, NULL);
> -	if (!i915->preempt_context)
> -		goto err_kernel_context;
> -
>  	WARN_ON(i915_gemfs_init(i915));
>  
>  	return i915;
>  
> -err_kernel_context:
> -	i915_gem_context_put(i915->kernel_context);
>  err_engine:
>  	for_each_engine(engine, i915, id)
>  		mock_engine_free(engine);
> -- 
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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

[PATCH v2] drm/i915: Only allocate preempt context when required

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 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);
+}
+
 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(&dev_priv->contexts.list);
 	INIT_WORK(&dev_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(&dev_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(&i915->drm.struct_mutex);
 
-	destroy_kernel_context(&i915->preempt_context);
+	if (i915->preempt_context)
+		destroy_kernel_context(&i915->preempt_context);
 	destroy_kernel_context(&i915->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:
 	intel_engine_fini_breadcrumbs(engine);
 err_unpin_preempt:
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 err_unpin_kernel:
 	engine->context_unpin(engine, engine->i915->kernel_context);
@@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	if (engine->default_state)
 		i915_gem_object_put(engine->default_state);
 
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 4ea65df05e02..b43b58cc599b 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
 		goto unlock;
 
 	if (port_isset(port)) {
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
+		if (engine->i915->preempt_context) {
 			struct guc_preempt_work *preempt_work =
 				&engine->i915->guc.preempt_work[engine->id];
 
@@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
 	}
 	guc->execbuf_client = client;
 
-	client = guc_client_alloc(dev_priv,
-				  INTEL_INFO(dev_priv)->ring_mask,
-				  GUC_CLIENT_PRIORITY_KMD_HIGH,
-				  dev_priv->preempt_context);
-	if (IS_ERR(client)) {
-		DRM_ERROR("Failed to create GuC client for preemption!\n");
-		guc_client_free(guc->execbuf_client);
-		guc->execbuf_client = NULL;
-		return PTR_ERR(client);
+	if (dev_priv->preempt_context) {
+		client = guc_client_alloc(dev_priv,
+					  INTEL_INFO(dev_priv)->ring_mask,
+					  GUC_CLIENT_PRIORITY_KMD_HIGH,
+					  dev_priv->preempt_context);
+		if (IS_ERR(client)) {
+			DRM_ERROR("Failed to create GuC client for preemption!\n");
+			guc_client_free(guc->execbuf_client);
+			guc->execbuf_client = NULL;
+			return PTR_ERR(client);
+		}
+		guc->preempt_client = client;
 	}
-	guc->preempt_client = client;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index fb2dd2f3bc8d..ee7390f69e93 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -161,7 +161,6 @@
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
 #define WA_TAIL_DWORDS 2
 #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
-#define PREEMPT_ID 0x1
 
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine);
@@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 		&engine->i915->preempt_context->engine[engine->id];
 	unsigned int n;
 
-	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
+	GEM_BUG_ON(engine->execlists.preempt_complete_status !=
+		   upper_32_bits(ce->lrc_desc));
 	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
 
 	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
@@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
 			goto unlock;
 
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
+		if (engine->i915->preempt_context &&
 		    rb_entry(rb, struct i915_priolist, node)->priority >
 		    max(last->priotree.priority, 0)) {
 			/*
@@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
 			if (status & GEN8_CTX_STATUS_COMPLETE &&
-			    buf[2*head + 1] == PREEMPT_ID) {
+			    buf[2*head + 1] == execlists->preempt_complete_status) {
 				GEM_TRACE("%s preempt-idle\n", engine->name);
 
 				execlists_cancel_port_requests(execlists);
@@ -1931,7 +1931,7 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->i915->caps.scheduler =
 		I915_SCHEDULER_CAP_ENABLED |
 		I915_SCHEDULER_CAP_PRIORITY;
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
 }
 
@@ -2009,6 +2009,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;
+	if (engine->i915->preempt_context)
+		engine->execlists.preempt_complete_status =
+			upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
+
 	return 0;
 
 error:
@@ -2271,7 +2276,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 	if (!engine->default_state)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
-	if (ctx->hw_id == PREEMPT_ID)
+	if (ctx == ctx->i915->preempt_context)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 					   CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index c5ff203e42d6..03be8995f415 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -279,6 +279,11 @@ struct intel_engine_execlists {
 	 * @csb_use_mmio: access csb through mmio, instead of hwsp
 	 */
 	bool csb_use_mmio;
+
+	/**
+	 * @preempt_complete_status: expected CSB upon completing preemption
+	 */
+	u32 preempt_complete_status;
 };
 
 #define INTEL_ENGINE_CS_MAX_NAME 8
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 1bc61f3f76fc..3175db70cc6e 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
 	if (!i915->kernel_context)
 		goto err_engine;
 
-	i915->preempt_context = mock_context(i915, NULL);
-	if (!i915->preempt_context)
-		goto err_kernel_context;
-
 	WARN_ON(i915_gemfs_init(i915));
 
 	return i915;
 
-err_kernel_context:
-	i915_gem_context_put(i915->kernel_context);
 err_engine:
 	for_each_engine(engine, i915, id)
 		mock_engine_free(engine);
-- 
2.15.1

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

✗ Fi.CI.BAT: warning for series starting with [v2,1/3] drm/i915/guc: Allow preempt-client to be NULL

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/3] drm/i915/guc: Allow preempt-client to be NULL
URL   : https://patchwork.freedesktop.org/series/37395/
State : warning

== Summary ==

Series 37395v1 series starting with [v2,1/3] drm/i915/guc: Allow preempt-client to be NULL
https://patchwork.freedesktop.org/api/1.0/series/37395/revisions/1/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989 +6
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (fi-elk-e7500) fdo#104058
Test gem_sync:
        Subgroup basic-all:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-each:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-many-each:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-store-all:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-store-each:
                pass       -> SKIP       (fi-elk-e7500)
Test gem_tiled_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-elk-e7500)
Test gem_tiled_fence_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-elk-e7500)
Test gem_wait:
        Subgroup basic-busy-all:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-wait-all:
                pass       -> SKIP       (fi-elk-e7500)
        Subgroup basic-await-all:
                pass       -> SKIP       (fi-elk-e7500)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:414s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:371s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:486s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:486s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:466s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:575s
fi-elk-e7500     total:229  pass:161  dwarn:7   dfail:1   fail:0   skip:59 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:505s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:396s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:408s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:444s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:460s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:496s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:574s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:424s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:515s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:527s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:483s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:416s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:428s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:392s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:468s

5d84aeda7cd418fe9d81419a42d1e7a46264e209 drm-tip: 2018y-01m-31d-10h-16m-14s UTC integration manifest
d7ab3f9a2164 drm/i915: Only allocate preempt context when required
b7a9c5fa91e5 drm/i915: Move the scheduler feature bits into the purview of the engines
8852942ea5bd drm/i915/guc: Allow preempt-client to be NULL

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7836/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> In the next patch, we may only conditionally allocate the preempt-client
> if there is a global preempt context and so we need to be prepared in
> case the preempt-client itself is NULL.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>

You need to prepare the debugfs to handle the null also.

Does our simple igt/debugfs test include reading through all the nodes
it finds?

-Mika

> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 1f3a8786bbdc..4ea65df05e02 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -832,10 +832,12 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
>  	if (ret)
>  		return ret;
>  
> -	ret = create_doorbell(guc->preempt_client);
> -	if (ret) {
> -		destroy_doorbell(guc->execbuf_client);
> -		return ret;
> +	if (guc->preempt_client) {
> +		ret = create_doorbell(guc->preempt_client);
> +		if (ret) {
> +			destroy_doorbell(guc->execbuf_client);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -848,8 +850,11 @@ static void guc_clients_doorbell_fini(struct intel_guc *guc)
>  	 * Instead of trying (in vain) to communicate with it, let's just
>  	 * cleanup the doorbell HW and our internal state.
>  	 */
> -	__destroy_doorbell(guc->preempt_client);
> -	__update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
> +	if (guc->preempt_client) {
> +		__destroy_doorbell(guc->preempt_client);
> +		__update_doorbell_desc(guc->preempt_client,
> +				       GUC_DOORBELL_INVALID);
> +	}
>  	__destroy_doorbell(guc->execbuf_client);
>  	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
>  }
> @@ -998,10 +1003,11 @@ static void guc_clients_destroy(struct intel_guc *guc)
>  {
>  	struct intel_guc_client *client;
>  
> -	client = fetch_and_zero(&guc->execbuf_client);
> -	guc_client_free(client);
> -
>  	client = fetch_and_zero(&guc->preempt_client);
> +	if (client)
> +		guc_client_free(client);
> +
> +	client = fetch_and_zero(&guc->execbuf_client);
>  	guc_client_free(client);
>  }
>  
> @@ -1160,7 +1166,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>  	GEM_BUG_ON(!guc->execbuf_client);
>  
>  	guc_reset_wq(guc->execbuf_client);
> -	guc_reset_wq(guc->preempt_client);
> +	if (guc->preempt_client)
> +		guc_reset_wq(guc->preempt_client);
>  
>  	err = intel_guc_sample_forcewake(guc);
>  	if (err)
> -- 
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[Chris Wilson]

Quoting Mika Kuoppala (2018-01-31 11:31:09)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > In the next patch, we may only conditionally allocate the preempt-client
> > if there is a global preempt context and so we need to be prepared in
> > case the preempt-client itself is NULL.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Michel Thierry <michel.thierry@intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
> >  1 file changed, 17 insertions(+), 10 deletions(-)
> >
> 
> You need to prepare the debugfs to handle the null also.
> 
> Does our simple igt/debugfs test include reading through all the nodes
> it finds?

It does, but we don't actually end up with NULLs.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2018-01-31 11:31:09)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > In the next patch, we may only conditionally allocate the preempt-client
>> > if there is a global preempt context and so we need to be prepared in
>> > case the preempt-client itself is NULL.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Michal Winiarski <michal.winiarski@intel.com>
>> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> > Cc: Michel Thierry <michel.thierry@intel.com>
>> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
>> >  1 file changed, 17 insertions(+), 10 deletions(-)
>> >
>> 
>> You need to prepare the debugfs to handle the null also.
>> 
>> Does our simple igt/debugfs test include reading through all the nodes
>> it finds?
>
> It does, but we don't actually end up with NULLs.

Not with this patch alone but the commit message promises
that we prepare for it :)

-Mika

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

[PATCH v2] drm/i915/guc: Allow preempt-client to be NULL

[Chris Wilson]

In the next patch, we may only conditionally allocate the preempt-client
if there is a global preempt context and so we need to be prepared in
case the preempt-client itself is NULL.

v2: Grep for more preempt_client.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c         |  8 +++++---
 drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
 drivers/gpu/drm/i915/selftests/intel_guc.c  | 20 +++++++++++---------
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3849ded354e3..9e44adef30f0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2338,7 +2338,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 		return -ENODEV;
 
 	GEM_BUG_ON(!guc->execbuf_client);
-	GEM_BUG_ON(!guc->preempt_client);
 
 	seq_printf(m, "Doorbell map:\n");
 	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
@@ -2346,8 +2345,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
 
 	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
 	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
-	seq_printf(m, "\nGuC preempt client @ %p:\n", guc->preempt_client);
-	i915_guc_client_info(m, dev_priv, guc->preempt_client);
+	if (guc->preempt_client) {
+		seq_printf(m, "\nGuC preempt client @ %p:\n",
+			   guc->preempt_client);
+		i915_guc_client_info(m, dev_priv, guc->preempt_client);
+	}
 
 	i915_guc_log_info(m, dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 1f3a8786bbdc..4ea65df05e02 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -832,10 +832,12 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
 	if (ret)
 		return ret;
 
-	ret = create_doorbell(guc->preempt_client);
-	if (ret) {
-		destroy_doorbell(guc->execbuf_client);
-		return ret;
+	if (guc->preempt_client) {
+		ret = create_doorbell(guc->preempt_client);
+		if (ret) {
+			destroy_doorbell(guc->execbuf_client);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -848,8 +850,11 @@ static void guc_clients_doorbell_fini(struct intel_guc *guc)
 	 * Instead of trying (in vain) to communicate with it, let's just
 	 * cleanup the doorbell HW and our internal state.
 	 */
-	__destroy_doorbell(guc->preempt_client);
-	__update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
+	if (guc->preempt_client) {
+		__destroy_doorbell(guc->preempt_client);
+		__update_doorbell_desc(guc->preempt_client,
+				       GUC_DOORBELL_INVALID);
+	}
 	__destroy_doorbell(guc->execbuf_client);
 	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
 }
@@ -998,10 +1003,11 @@ static void guc_clients_destroy(struct intel_guc *guc)
 {
 	struct intel_guc_client *client;
 
-	client = fetch_and_zero(&guc->execbuf_client);
-	guc_client_free(client);
-
 	client = fetch_and_zero(&guc->preempt_client);
+	if (client)
+		guc_client_free(client);
+
+	client = fetch_and_zero(&guc->execbuf_client);
 	guc_client_free(client);
 }
 
@@ -1160,7 +1166,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 	GEM_BUG_ON(!guc->execbuf_client);
 
 	guc_reset_wq(guc->execbuf_client);
-	guc_reset_wq(guc->preempt_client);
+	if (guc->preempt_client)
+		guc_reset_wq(guc->preempt_client);
 
 	err = intel_guc_sample_forcewake(guc);
 	if (err)
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 3f9016466dea..fb74e2cf8a0a 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -87,7 +87,7 @@ static int validate_client(struct intel_guc_client *client,
 
 static bool client_doorbell_in_sync(struct intel_guc_client *client)
 {
-	return doorbell_ok(client->guc, client->doorbell_id);
+	return !client || doorbell_ok(client->guc, client->doorbell_id);
 }
 
 /*
@@ -137,7 +137,6 @@ static int igt_guc_clients(void *args)
 		goto unlock;
 	}
 	GEM_BUG_ON(!guc->execbuf_client);
-	GEM_BUG_ON(!guc->preempt_client);
 
 	err = validate_client(guc->execbuf_client,
 			      GUC_CLIENT_PRIORITY_KMD_NORMAL, false);
@@ -146,16 +145,18 @@ static int igt_guc_clients(void *args)
 		goto out;
 	}
 
-	err = validate_client(guc->preempt_client,
-			      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
-	if (err) {
-		pr_err("preempt client validation failed\n");
-		goto out;
+	if (guc->preempt_client) {
+		err = validate_client(guc->preempt_client,
+				      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
+		if (err) {
+			pr_err("preempt client validation failed\n");
+			goto out;
+		}
 	}
 
 	/* each client should now have reserved a doorbell */
 	if (!has_doorbell(guc->execbuf_client) ||
-	    !has_doorbell(guc->preempt_client)) {
+	    (guc->preempt_client && !has_doorbell(guc->preempt_client))) {
 		pr_err("guc_clients_create didn't reserve doorbells\n");
 		err = -EINVAL;
 		goto out;
@@ -224,7 +225,8 @@ static int igt_guc_clients(void *args)
 	 * clients during unload.
 	 */
 	destroy_doorbell(guc->execbuf_client);
-	destroy_doorbell(guc->preempt_client);
+	if (guc->preempt_client)
+		destroy_doorbell(guc->preempt_client);
 	guc_clients_destroy(guc);
 	guc_clients_create(guc);
 	guc_clients_doorbell_init(guc);
-- 
2.15.1

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

Re: [PATCH v2] drm/i915/guc: Allow preempt-client to be NULL

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> In the next patch, we may only conditionally allocate the preempt-client
> if there is a global preempt context and so we need to be prepared in
> case the preempt-client itself is NULL.
>
> v2: Grep for more preempt_client.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c         |  8 +++++---
>  drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
>  drivers/gpu/drm/i915/selftests/intel_guc.c  | 20 +++++++++++---------
>  3 files changed, 33 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3849ded354e3..9e44adef30f0 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2338,7 +2338,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  		return -ENODEV;
>  
>  	GEM_BUG_ON(!guc->execbuf_client);
> -	GEM_BUG_ON(!guc->preempt_client);
>  
>  	seq_printf(m, "Doorbell map:\n");
>  	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
> @@ -2346,8 +2345,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
>  
>  	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
>  	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
> -	seq_printf(m, "\nGuC preempt client @ %p:\n", guc->preempt_client);
> -	i915_guc_client_info(m, dev_priv, guc->preempt_client);
> +	if (guc->preempt_client) {
> +		seq_printf(m, "\nGuC preempt client @ %p:\n",
> +			   guc->preempt_client);
> +		i915_guc_client_info(m, dev_priv, guc->preempt_client);
> +	}
>  
>  	i915_guc_log_info(m, dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 1f3a8786bbdc..4ea65df05e02 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -832,10 +832,12 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
>  	if (ret)
>  		return ret;
>  
> -	ret = create_doorbell(guc->preempt_client);
> -	if (ret) {
> -		destroy_doorbell(guc->execbuf_client);
> -		return ret;
> +	if (guc->preempt_client) {
> +		ret = create_doorbell(guc->preempt_client);
> +		if (ret) {
> +			destroy_doorbell(guc->execbuf_client);
> +			return ret;
> +		}
>  	}
>  
>  	return 0;
> @@ -848,8 +850,11 @@ static void guc_clients_doorbell_fini(struct intel_guc *guc)
>  	 * Instead of trying (in vain) to communicate with it, let's just
>  	 * cleanup the doorbell HW and our internal state.
>  	 */
> -	__destroy_doorbell(guc->preempt_client);
> -	__update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
> +	if (guc->preempt_client) {
> +		__destroy_doorbell(guc->preempt_client);
> +		__update_doorbell_desc(guc->preempt_client,
> +				       GUC_DOORBELL_INVALID);
> +	}
>  	__destroy_doorbell(guc->execbuf_client);
>  	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
>  }
> @@ -998,10 +1003,11 @@ static void guc_clients_destroy(struct intel_guc *guc)
>  {
>  	struct intel_guc_client *client;
>  
> -	client = fetch_and_zero(&guc->execbuf_client);
> -	guc_client_free(client);
> -
>  	client = fetch_and_zero(&guc->preempt_client);
> +	if (client)
> +		guc_client_free(client);
> +
> +	client = fetch_and_zero(&guc->execbuf_client);
>  	guc_client_free(client);
>  }
>  
> @@ -1160,7 +1166,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
>  	GEM_BUG_ON(!guc->execbuf_client);
>  
>  	guc_reset_wq(guc->execbuf_client);
> -	guc_reset_wq(guc->preempt_client);
> +	if (guc->preempt_client)
> +		guc_reset_wq(guc->preempt_client);
>  
>  	err = intel_guc_sample_forcewake(guc);
>  	if (err)
> diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
> index 3f9016466dea..fb74e2cf8a0a 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_guc.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
> @@ -87,7 +87,7 @@ static int validate_client(struct intel_guc_client *client,
>  
>  static bool client_doorbell_in_sync(struct intel_guc_client *client)
>  {
> -	return doorbell_ok(client->guc, client->doorbell_id);
> +	return !client || doorbell_ok(client->guc, client->doorbell_id);
>  }
>  
>  /*
> @@ -137,7 +137,6 @@ static int igt_guc_clients(void *args)
>  		goto unlock;
>  	}
>  	GEM_BUG_ON(!guc->execbuf_client);
> -	GEM_BUG_ON(!guc->preempt_client);
>  
>  	err = validate_client(guc->execbuf_client,
>  			      GUC_CLIENT_PRIORITY_KMD_NORMAL, false);
> @@ -146,16 +145,18 @@ static int igt_guc_clients(void *args)
>  		goto out;
>  	}
>  
> -	err = validate_client(guc->preempt_client,
> -			      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
> -	if (err) {
> -		pr_err("preempt client validation failed\n");
> -		goto out;
> +	if (guc->preempt_client) {
> +		err = validate_client(guc->preempt_client,
> +				      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
> +		if (err) {
> +			pr_err("preempt client validation failed\n");
> +			goto out;
> +		}
>  	}
>  
>  	/* each client should now have reserved a doorbell */
>  	if (!has_doorbell(guc->execbuf_client) ||
> -	    !has_doorbell(guc->preempt_client)) {
> +	    (guc->preempt_client && !has_doorbell(guc->preempt_client))) {
>  		pr_err("guc_clients_create didn't reserve doorbells\n");
>  		err = -EINVAL;
>  		goto out;
> @@ -224,7 +225,8 @@ static int igt_guc_clients(void *args)
>  	 * clients during unload.
>  	 */
>  	destroy_doorbell(guc->execbuf_client);
> -	destroy_doorbell(guc->preempt_client);
> +	if (guc->preempt_client)
> +		destroy_doorbell(guc->preempt_client);
>  	guc_clients_destroy(guc);
>  	guc_clients_create(guc);
>  	guc_clients_doorbell_init(guc);
> -- 
> 2.15.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev2)
URL   : https://patchwork.freedesktop.org/series/37395/
State : success

== Summary ==

Series 37395v2 series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL
https://patchwork.freedesktop.org/api/1.0/series/37395/revisions/2/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                dmesg-warn -> DMESG-FAIL (fi-elk-e7500) fdo#103989

fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:415s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:370s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:485s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:281s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:482s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:465s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:565s
fi-elk-e7500     total:224  pass:168  dwarn:9   dfail:1   fail:0   skip:45 
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:280s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:389s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:398s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:412s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:493s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:572s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:429s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:506s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:526s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:489s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:429s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:398s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:469s

badc0398a06018a07f6f50a1ff5a5fd8a30b9944 drm-tip: 2018y-01m-31d-12h-17m-12s UTC integration manifest
b566d4c192e0 drm/i915: Only allocate preempt context when required
9d317ed56342 drm/i915: Move the scheduler feature bits into the purview of the engines
c74489d84b5e drm/i915/guc: Allow preempt-client to be NULL

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7838/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev2)
URL   : https://patchwork.freedesktop.org/series/37395/
State : success

== Summary ==

Test perf_pmu:
        Subgroup semaphore-wait-bcs0:
                fail       -> PASS       (shard-apl) fdo#104829
Test gem_eio:
        Subgroup in-flight-suspend:
                pass       -> FAIL       (shard-hsw) fdo#104676

fdo#104829 https://bugs.freedesktop.org/show_bug.cgi?id=104829
fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676

shard-apl        total:1638 pass:1017 dwarn:1   dfail:0   fail:16  skip:604 time:7460s
shard-hsw        total:2838 pass:1735 dwarn:1   dfail:0   fail:11  skip:1090 time:11932s
shard-snb        total:2838 pass:1330 dwarn:1   dfail:0   fail:10  skip:1497 time:6594s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7838/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev3)
URL   : https://patchwork.freedesktop.org/series/37395/
State : failure

== Summary ==

Series 37395v3 series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL
https://patchwork.freedesktop.org/api/1.0/series/37395/revisions/3/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test gem_sync:
        Subgroup basic-many-each:
                pass       -> DMESG-FAIL (fi-bsw-n3050)
        Subgroup basic-store-all:
                pass       -> SKIP       (fi-bsw-n3050)
        Subgroup basic-store-each:
                pass       -> SKIP       (fi-bsw-n3050)
Test gem_tiled_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-bsw-n3050)
Test gem_tiled_fence_blits:
        Subgroup basic:
                pass       -> SKIP       (fi-bsw-n3050)
Test gem_wait:
        Subgroup basic-busy-all:
                pass       -> SKIP       (fi-bsw-n3050)
        Subgroup basic-wait-all:
                pass       -> SKIP       (fi-bsw-n3050)
        Subgroup basic-await-all:
                pass       -> SKIP       (fi-bsw-n3050)
Test gem_workarounds:
        Subgroup basic-read:
                pass       -> SKIP       (fi-bsw-n3050)
Test kms_busy:
        Subgroup basic-flip-c:
                pass       -> SKIP       (fi-bsw-n3050)
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                pass       -> SKIP       (fi-bsw-n3050)
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> SKIP       (fi-bsw-n3050)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> SKIP       (fi-bsw-n3050) fdo#104571
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-FAIL (fi-bsw-n3050)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104571 https://bugs.freedesktop.org/show_bug.cgi?id=104571

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:417s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:227  dwarn:1   dfail:2   fail:0   skip:58  time:479s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:282s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:490s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:474s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:460s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:566s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:413s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:277s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:510s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:412s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:452s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:413s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:457s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:498s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:498s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:576s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:510s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:531s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:492s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:496s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:415s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:526s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:467s

cc6c1df8bef5c07d1b05bebf56e66a9f10a4702b drm-tip: 2018y-02m-01d-16h-46m-26s UTC integration manifest
8a388728ae1f drm/i915: Only allocate preempt context when required
e2891884a12d drm/i915: Move the scheduler feature bits into the purview of the engines
5f390ed3a415 drm/i915/guc: Allow preempt-client to be NULL

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7855/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev4)
URL   : https://patchwork.freedesktop.org/series/37395/
State : success

== Summary ==

Series 37395v4 series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL
https://patchwork.freedesktop.org/api/1.0/series/37395/revisions/4/mbox/

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:420s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:423s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:372s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:485s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:283s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:469s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:454s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:412s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:278s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:392s
fi-hsw-4770r     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:401s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:414s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:451s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:412s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:456s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:496s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:581s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:431s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:530s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:483s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:431s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:400s
Blacklisted hosts:
fi-glk-dsi       total:288  pass:152  dwarn:1   dfail:4   fail:0   skip:131 time:263s

2e76a2952923eba64c4f9baf461613bc42ee997a drm-tip: 2018y-02m-02d-20h-33m-12s UTC integration manifest
d28ada97885a drm/i915: Only allocate preempt context when required
6166a85814ef drm/i915: Move the scheduler feature bits into the purview of the engines
3c85087dcfae drm/i915/guc: Allow preempt-client to be NULL

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7879/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915/guc: Allow preempt-client to be NULL (rev4)
URL   : https://patchwork.freedesktop.org/series/37395/
State : failure

== Summary ==

Test gem_eio:
        Subgroup in-flight-contexts:
                dmesg-warn -> PASS       (shard-snb) fdo#104058
Test perf:
        Subgroup oa-exponents:
                pass       -> FAIL       (shard-apl) fdo#102254
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                fail       -> PASS       (shard-hsw)
        Subgroup 2x-flip-vs-expired-vblank-interruptible:
                fail       -> PASS       (shard-hsw) fdo#102887
Test perf_pmu:
        Subgroup busy-double-start-vcs0:
                pass       -> INCOMPLETE (shard-apl)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (shard-apl) fdo#103481
Test kms_cursor_legacy:
        Subgroup 2x-long-flip-vs-cursor-legacy:
                pass       -> FAIL       (shard-hsw) fdo#104873
        Subgroup flip-vs-cursor-toggle:
                fail       -> PASS       (shard-hsw) fdo#102670 +1
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-hsw) fdo#99912

fdo#104058 https://bugs.freedesktop.org/show_bug.cgi?id=104058
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103481 https://bugs.freedesktop.org/show_bug.cgi?id=103481
fdo#104873 https://bugs.freedesktop.org/show_bug.cgi?id=104873
fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-apl        total:2795 pass:1723 dwarn:1   dfail:0   fail:21  skip:1049 time:12048s
shard-hsw        total:2836 pass:1733 dwarn:1   dfail:0   fail:11  skip:1090 time:11710s
shard-snb        total:2836 pass:1328 dwarn:1   dfail:0   fail:10  skip:1497 time:6382s
Blacklisted hosts:
shard-kbl        total:2836 pass:1872 dwarn:1   dfail:0   fail:21  skip:942 time:9340s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7879/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] drm/i915/guc: Allow preempt-client to be NULL

[Chris Wilson]

In the next patch, we may only conditionally allocate the preempt-client
if there is a global preempt context and so we need to be prepared in
case the preempt-client itself is NULL.

v2: Grep for more preempt_client.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c         |  8 +++++---
 drivers/gpu/drm/i915/intel_guc_submission.c | 27 +++++++++++++++++----------
 drivers/gpu/drm/i915/selftests/intel_guc.c  | 20 +++++++++++---------
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3849ded354e3..9e44adef30f0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2338,7 +2338,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 		return -ENODEV;
 
 	GEM_BUG_ON(!guc->execbuf_client);
-	GEM_BUG_ON(!guc->preempt_client);
 
 	seq_printf(m, "Doorbell map:\n");
 	seq_printf(m, "\t%*pb\n", GUC_NUM_DOORBELLS, guc->doorbell_bitmap);
@@ -2346,8 +2345,11 @@ static int i915_guc_info(struct seq_file *m, void *data)
 
 	seq_printf(m, "\nGuC execbuf client @ %p:\n", guc->execbuf_client);
 	i915_guc_client_info(m, dev_priv, guc->execbuf_client);
-	seq_printf(m, "\nGuC preempt client @ %p:\n", guc->preempt_client);
-	i915_guc_client_info(m, dev_priv, guc->preempt_client);
+	if (guc->preempt_client) {
+		seq_printf(m, "\nGuC preempt client @ %p:\n",
+			   guc->preempt_client);
+		i915_guc_client_info(m, dev_priv, guc->preempt_client);
+	}
 
 	i915_guc_log_info(m, dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 1f3a8786bbdc..4ea65df05e02 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -832,10 +832,12 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
 	if (ret)
 		return ret;
 
-	ret = create_doorbell(guc->preempt_client);
-	if (ret) {
-		destroy_doorbell(guc->execbuf_client);
-		return ret;
+	if (guc->preempt_client) {
+		ret = create_doorbell(guc->preempt_client);
+		if (ret) {
+			destroy_doorbell(guc->execbuf_client);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -848,8 +850,11 @@ static void guc_clients_doorbell_fini(struct intel_guc *guc)
 	 * Instead of trying (in vain) to communicate with it, let's just
 	 * cleanup the doorbell HW and our internal state.
 	 */
-	__destroy_doorbell(guc->preempt_client);
-	__update_doorbell_desc(guc->preempt_client, GUC_DOORBELL_INVALID);
+	if (guc->preempt_client) {
+		__destroy_doorbell(guc->preempt_client);
+		__update_doorbell_desc(guc->preempt_client,
+				       GUC_DOORBELL_INVALID);
+	}
 	__destroy_doorbell(guc->execbuf_client);
 	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
 }
@@ -998,10 +1003,11 @@ static void guc_clients_destroy(struct intel_guc *guc)
 {
 	struct intel_guc_client *client;
 
-	client = fetch_and_zero(&guc->execbuf_client);
-	guc_client_free(client);
-
 	client = fetch_and_zero(&guc->preempt_client);
+	if (client)
+		guc_client_free(client);
+
+	client = fetch_and_zero(&guc->execbuf_client);
 	guc_client_free(client);
 }
 
@@ -1160,7 +1166,8 @@ int intel_guc_submission_enable(struct intel_guc *guc)
 	GEM_BUG_ON(!guc->execbuf_client);
 
 	guc_reset_wq(guc->execbuf_client);
-	guc_reset_wq(guc->preempt_client);
+	if (guc->preempt_client)
+		guc_reset_wq(guc->preempt_client);
 
 	err = intel_guc_sample_forcewake(guc);
 	if (err)
diff --git a/drivers/gpu/drm/i915/selftests/intel_guc.c b/drivers/gpu/drm/i915/selftests/intel_guc.c
index 3f9016466dea..fb74e2cf8a0a 100644
--- a/drivers/gpu/drm/i915/selftests/intel_guc.c
+++ b/drivers/gpu/drm/i915/selftests/intel_guc.c
@@ -87,7 +87,7 @@ static int validate_client(struct intel_guc_client *client,
 
 static bool client_doorbell_in_sync(struct intel_guc_client *client)
 {
-	return doorbell_ok(client->guc, client->doorbell_id);
+	return !client || doorbell_ok(client->guc, client->doorbell_id);
 }
 
 /*
@@ -137,7 +137,6 @@ static int igt_guc_clients(void *args)
 		goto unlock;
 	}
 	GEM_BUG_ON(!guc->execbuf_client);
-	GEM_BUG_ON(!guc->preempt_client);
 
 	err = validate_client(guc->execbuf_client,
 			      GUC_CLIENT_PRIORITY_KMD_NORMAL, false);
@@ -146,16 +145,18 @@ static int igt_guc_clients(void *args)
 		goto out;
 	}
 
-	err = validate_client(guc->preempt_client,
-			      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
-	if (err) {
-		pr_err("preempt client validation failed\n");
-		goto out;
+	if (guc->preempt_client) {
+		err = validate_client(guc->preempt_client,
+				      GUC_CLIENT_PRIORITY_KMD_HIGH, true);
+		if (err) {
+			pr_err("preempt client validation failed\n");
+			goto out;
+		}
 	}
 
 	/* each client should now have reserved a doorbell */
 	if (!has_doorbell(guc->execbuf_client) ||
-	    !has_doorbell(guc->preempt_client)) {
+	    (guc->preempt_client && !has_doorbell(guc->preempt_client))) {
 		pr_err("guc_clients_create didn't reserve doorbells\n");
 		err = -EINVAL;
 		goto out;
@@ -224,7 +225,8 @@ static int igt_guc_clients(void *args)
 	 * clients during unload.
 	 */
 	destroy_doorbell(guc->execbuf_client);
-	destroy_doorbell(guc->preempt_client);
+	if (guc->preempt_client)
+		destroy_doorbell(guc->preempt_client);
 	guc_clients_destroy(guc);
 	guc_clients_create(guc);
 	guc_clients_doorbell_init(guc);
-- 
2.16.1

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

[PATCH v2 3/3] drm/i915: Only allocate preempt context when required

[Chris Wilson]

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 <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 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);
+}
+
 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(&dev_priv->contexts.list);
 	INIT_WORK(&dev_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(&dev_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(&i915->drm.struct_mutex);
 
-	destroy_kernel_context(&i915->preempt_context);
+	if (i915->preempt_context)
+		destroy_kernel_context(&i915->preempt_context);
 	destroy_kernel_context(&i915->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:
 	intel_engine_fini_breadcrumbs(engine);
 err_unpin_preempt:
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 err_unpin_kernel:
 	engine->context_unpin(engine, engine->i915->kernel_context);
@@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	if (engine->default_state)
 		i915_gem_object_put(engine->default_state);
 
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
 }
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 4ea65df05e02..b43b58cc599b 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
 		goto unlock;
 
 	if (port_isset(port)) {
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
+		if (engine->i915->preempt_context) {
 			struct guc_preempt_work *preempt_work =
 				&engine->i915->guc.preempt_work[engine->id];
 
@@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
 	}
 	guc->execbuf_client = client;
 
-	client = guc_client_alloc(dev_priv,
-				  INTEL_INFO(dev_priv)->ring_mask,
-				  GUC_CLIENT_PRIORITY_KMD_HIGH,
-				  dev_priv->preempt_context);
-	if (IS_ERR(client)) {
-		DRM_ERROR("Failed to create GuC client for preemption!\n");
-		guc_client_free(guc->execbuf_client);
-		guc->execbuf_client = NULL;
-		return PTR_ERR(client);
+	if (dev_priv->preempt_context) {
+		client = guc_client_alloc(dev_priv,
+					  INTEL_INFO(dev_priv)->ring_mask,
+					  GUC_CLIENT_PRIORITY_KMD_HIGH,
+					  dev_priv->preempt_context);
+		if (IS_ERR(client)) {
+			DRM_ERROR("Failed to create GuC client for preemption!\n");
+			guc_client_free(guc->execbuf_client);
+			guc->execbuf_client = NULL;
+			return PTR_ERR(client);
+		}
+		guc->preempt_client = client;
 	}
-	guc->preempt_client = client;
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 449fd1e95f1f..c2c8380a0121 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -161,7 +161,6 @@
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
 #define WA_TAIL_DWORDS 2
 #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
-#define PREEMPT_ID 0x1
 
 static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 					    struct intel_engine_cs *engine);
@@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
 		&engine->i915->preempt_context->engine[engine->id];
 	unsigned int n;
 
-	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
+	GEM_BUG_ON(engine->execlists.preempt_complete_status !=
+		   upper_32_bits(ce->lrc_desc));
 	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
 
 	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
@@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
 		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
 			goto unlock;
 
-		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
+		if (engine->i915->preempt_context &&
 		    rb_entry(rb, struct i915_priolist, node)->priority >
 		    max(last->priotree.priority, 0)) {
 			/*
@@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long data)
 			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
 
 			if (status & GEN8_CTX_STATUS_COMPLETE &&
-			    buf[2*head + 1] == PREEMPT_ID) {
+			    buf[2*head + 1] == execlists->preempt_complete_status) {
 				GEM_TRACE("%s preempt-idle\n", engine->name);
 
 				execlists_cancel_port_requests(execlists);
@@ -1967,7 +1967,7 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
 	engine->i915->caps.scheduler =
 		I915_SCHEDULER_CAP_ENABLED |
 		I915_SCHEDULER_CAP_PRIORITY;
-	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
+	if (engine->i915->preempt_context)
 		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
 }
 
@@ -2045,6 +2045,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;
+	if (engine->i915->preempt_context)
+		engine->execlists.preempt_complete_status =
+			upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
+
 	return 0;
 
 error:
@@ -2307,7 +2312,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 	if (!engine->default_state)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
-	if (ctx->hw_id == PREEMPT_ID)
+	if (ctx == ctx->i915->preempt_context)
 		regs[CTX_CONTEXT_CONTROL + 1] |=
 			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 					   CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index ac03936162f2..8b27afa3a6a7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -279,6 +279,11 @@ struct intel_engine_execlists {
 	 * @csb_use_mmio: access csb through mmio, instead of hwsp
 	 */
 	bool csb_use_mmio;
+
+	/**
+	 * @preempt_complete_status: expected CSB upon completing preemption
+	 */
+	u32 preempt_complete_status;
 };
 
 #define INTEL_ENGINE_CS_MAX_NAME 8
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 1bc61f3f76fc..3175db70cc6e 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
 	if (!i915->kernel_context)
 		goto err_engine;
 
-	i915->preempt_context = mock_context(i915, NULL);
-	if (!i915->preempt_context)
-		goto err_kernel_context;
-
 	WARN_ON(i915_gemfs_init(i915));
 
 	return i915;
 
-err_kernel_context:
-	i915_gem_context_put(i915->kernel_context);
 err_engine:
 	for_each_engine(engine, i915, id)
 		mock_engine_free(engine);
-- 
2.16.1

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

Re: [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 <chris@chris-wilson.co.uk>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

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

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> ---
>   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(&dev_priv->contexts.list);
>   	INIT_WORK(&dev_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(&dev_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(&i915->drm.struct_mutex);
>   
> -	destroy_kernel_context(&i915->preempt_context);
> +	if (i915->preempt_context)
> +		destroy_kernel_context(&i915->preempt_context);
>   	destroy_kernel_context(&i915->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:
>   	intel_engine_fini_breadcrumbs(engine);
>   err_unpin_preempt:
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>   		engine->context_unpin(engine, engine->i915->preempt_context);
>   err_unpin_kernel:
>   	engine->context_unpin(engine, engine->i915->kernel_context);
> @@ -686,7 +686,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
>   	if (engine->default_state)
>   		i915_gem_object_put(engine->default_state);
>   
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>   		engine->context_unpin(engine, engine->i915->preempt_context);
>   	engine->context_unpin(engine, engine->i915->kernel_context);
>   }
> diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> index 4ea65df05e02..b43b58cc599b 100644
> --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> @@ -688,7 +688,7 @@ static void guc_dequeue(struct intel_engine_cs *engine)
>   		goto unlock;
>   
>   	if (port_isset(port)) {
> -		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915)) {
> +		if (engine->i915->preempt_context) {
>   			struct guc_preempt_work *preempt_work =
>   				&engine->i915->guc.preempt_work[engine->id];
>   
> @@ -984,17 +984,19 @@ static int guc_clients_create(struct intel_guc *guc)
>   	}
>   	guc->execbuf_client = client;
>   
> -	client = guc_client_alloc(dev_priv,
> -				  INTEL_INFO(dev_priv)->ring_mask,
> -				  GUC_CLIENT_PRIORITY_KMD_HIGH,
> -				  dev_priv->preempt_context);
> -	if (IS_ERR(client)) {
> -		DRM_ERROR("Failed to create GuC client for preemption!\n");
> -		guc_client_free(guc->execbuf_client);
> -		guc->execbuf_client = NULL;
> -		return PTR_ERR(client);
> +	if (dev_priv->preempt_context) {
> +		client = guc_client_alloc(dev_priv,
> +					  INTEL_INFO(dev_priv)->ring_mask,
> +					  GUC_CLIENT_PRIORITY_KMD_HIGH,
> +					  dev_priv->preempt_context);
> +		if (IS_ERR(client)) {
> +			DRM_ERROR("Failed to create GuC client for preemption!\n");
> +			guc_client_free(guc->execbuf_client);
> +			guc->execbuf_client = NULL;
> +			return PTR_ERR(client);
> +		}
> +		guc->preempt_client = client;
>   	}
> -	guc->preempt_client = client;
>   
>   	return 0;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 449fd1e95f1f..c2c8380a0121 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -161,7 +161,6 @@
>   #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
>   #define WA_TAIL_DWORDS 2
>   #define WA_TAIL_BYTES (sizeof(u32) * WA_TAIL_DWORDS)
> -#define PREEMPT_ID 0x1
>   
>   static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>   					    struct intel_engine_cs *engine);
> @@ -448,7 +447,8 @@ static void inject_preempt_context(struct intel_engine_cs *engine)
>   		&engine->i915->preempt_context->engine[engine->id];
>   	unsigned int n;
>   
> -	GEM_BUG_ON(engine->i915->preempt_context->hw_id != PREEMPT_ID);
> +	GEM_BUG_ON(engine->execlists.preempt_complete_status !=
> +		   upper_32_bits(ce->lrc_desc));
>   	GEM_BUG_ON(!IS_ALIGNED(ce->ring->size, WA_TAIL_BYTES));
>   
>   	memset(ce->ring->vaddr + ce->ring->tail, 0, WA_TAIL_BYTES);
> @@ -528,7 +528,7 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>   		if (!execlists_is_active(execlists, EXECLISTS_ACTIVE_HWACK))
>   			goto unlock;
>   
> -		if (HAS_LOGICAL_RING_PREEMPTION(engine->i915) &&
> +		if (engine->i915->preempt_context &&
>   		    rb_entry(rb, struct i915_priolist, node)->priority >
>   		    max(last->priotree.priority, 0)) {
>   			/*
> @@ -844,7 +844,7 @@ static void execlists_submission_tasklet(unsigned long data)
>   			GEM_BUG_ON(status & GEN8_CTX_STATUS_IDLE_ACTIVE);
>   
>   			if (status & GEN8_CTX_STATUS_COMPLETE &&
> -			    buf[2*head + 1] == PREEMPT_ID) {
> +			    buf[2*head + 1] == execlists->preempt_complete_status) {
>   				GEM_TRACE("%s preempt-idle\n", engine->name);
>   
>   				execlists_cancel_port_requests(execlists);
> @@ -1967,7 +1967,7 @@ static void execlists_set_default_submission(struct intel_engine_cs *engine)
>   	engine->i915->caps.scheduler =
>   		I915_SCHEDULER_CAP_ENABLED |
>   		I915_SCHEDULER_CAP_PRIORITY;
> -	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
> +	if (engine->i915->preempt_context)
>   		engine->i915->caps.scheduler |= I915_SCHEDULER_CAP_PREEMPTION;
>   }
>   
> @@ -2045,6 +2045,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;
> +	if (engine->i915->preempt_context)
> +		engine->execlists.preempt_complete_status =
> +			upper_32_bits(engine->i915->preempt_context->engine[engine->id].lrc_desc);
> +
>   	return 0;
>   
>   error:
> @@ -2307,7 +2312,7 @@ populate_lr_context(struct i915_gem_context *ctx,
>   	if (!engine->default_state)
>   		regs[CTX_CONTEXT_CONTROL + 1] |=
>   			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
> -	if (ctx->hw_id == PREEMPT_ID)
> +	if (ctx == ctx->i915->preempt_context)
>   		regs[CTX_CONTEXT_CONTROL + 1] |=
>   			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
>   					   CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index ac03936162f2..8b27afa3a6a7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -279,6 +279,11 @@ struct intel_engine_execlists {
>   	 * @csb_use_mmio: access csb through mmio, instead of hwsp
>   	 */
>   	bool csb_use_mmio;
> +
> +	/**
> +	 * @preempt_complete_status: expected CSB upon completing preemption
> +	 */
> +	u32 preempt_complete_status;
>   };
>   
>   #define INTEL_ENGINE_CS_MAX_NAME 8
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 1bc61f3f76fc..3175db70cc6e 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -243,16 +243,10 @@ struct drm_i915_private *mock_gem_device(void)
>   	if (!i915->kernel_context)
>   		goto err_engine;
>   
> -	i915->preempt_context = mock_context(i915, NULL);
> -	if (!i915->preempt_context)
> -		goto err_kernel_context;
> -
>   	WARN_ON(i915_gemfs_init(i915));
>   
>   	return i915;
>   
> -err_kernel_context:
> -	i915_gem_context_put(i915->kernel_context);
>   err_engine:
>   	for_each_engine(engine, i915, id)
>   		mock_engine_free(engine);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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: [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 <chris@chris-wilson.co.uk>
> > Cc: Michal Winiarski <michal.winiarski@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Acked-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> 
> Only this one was missing a r-b, but all 3:
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>

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