Re: [Intel-gfx] [PATCH v5 12/35] drm/i915: Added deferred work handler for scheduler

2016-03-01 Thread John Harrison

On 01/03/2016 09:16, Joonas Lahtinen wrote:

On to, 2016-02-18 at 14:27 +, john.c.harri...@intel.com wrote:

From: John Harrison 

The scheduler needs to do interrupt triggered work that is too complex
to do in the interrupt handler. Thus it requires a deferred work
handler to process such tasks asynchronously.

v2: Updated to reduce mutex lock usage. The lock is now only held for
the minimum time within the remove function rather than for the whole
of the worker thread's operation.

v5: Removed objectionable white space and added some documentation.
[Joonas Lahtinen]

For: VIZ-1587
Signed-off-by: John Harrison 
Cc: Joonas Lahtinen 
---
  drivers/gpu/drm/i915/i915_dma.c   |  3 +++
  drivers/gpu/drm/i915/i915_drv.h   | 10 ++
  drivers/gpu/drm/i915/i915_gem.c   |  2 ++
  drivers/gpu/drm/i915/i915_scheduler.c | 29 +++--
  drivers/gpu/drm/i915/i915_scheduler.h |  1 +
  5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 678adc7..c3d382d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1158,6 +1158,9 @@ int i915_driver_unload(struct drm_device *dev)
WARN_ON(unregister_oom_notifier(_priv->mm.oom_notifier));
unregister_shrinker(_priv->mm.shrinker);
  
+	/* Cancel the scheduler work handler, which should be idle now. */

+   cancel_work_sync(_priv->mm.scheduler_work);
+
io_mapping_free(dev_priv->gtt.mappable);
arch_phys_wc_del(dev_priv->gtt.mtrr);
  
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h

index 03add1a..4d544f1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1291,6 +1291,16 @@ struct i915_gem_mm {
struct delayed_work retire_work;
  
  	/**

+* New scheme is to get an interrupt after every work packet
+* in order to allow the low latency scheduling of pending
+* packets. The idea behind adding new packets to a pending
+* queue rather than directly into the hardware ring buffer
+* is to allow high priority packets to over take low priority
+* ones.
+*/
+   struct work_struct scheduler_work;
+
+   /**
 * When we detect an idle GPU, we want to turn on
 * powersaving features. So once we see that there
 * are no more requests outstanding and no more
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c3b7def..1ab7256 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5427,6 +5427,8 @@ i915_gem_load(struct drm_device *dev)
  i915_gem_retire_work_handler);
INIT_DELAYED_WORK(_priv->mm.idle_work,
  i915_gem_idle_work_handler);
+   INIT_WORK(_priv->mm.scheduler_work,
+   i915_scheduler_work_handler);
init_waitqueue_head(_priv->gpu_error.reset_queue);
  
  	dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;

diff --git a/drivers/gpu/drm/i915/i915_scheduler.c 
b/drivers/gpu/drm/i915/i915_scheduler.c
index ab5007a..3986890 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -697,7 +697,9 @@ static int i915_scheduler_remove_dependent(struct 
i915_scheduler *scheduler,
   */
  void i915_scheduler_wakeup(struct drm_device *dev)
  {
-   /* XXX: Need to call i915_scheduler_remove() via work handler. */
+   struct drm_i915_private *dev_priv = to_i915(dev);
+
+   queue_work(dev_priv->wq, _priv->mm.scheduler_work);
  }
  
  /**

@@ -827,7 +829,7 @@ static bool i915_scheduler_remove(struct i915_scheduler 
*scheduler,
return do_submit;
  }
  
-void i915_scheduler_process_work(struct intel_engine_cs *ring)

+static void i915_scheduler_process_work(struct intel_engine_cs *ring)

This is an odd change, maybe should have been static void to begin
with?
The problems of introducing code in patch A and plumbing it to the 
driver in patch B. Making it static from the start gives an unused 
function compiler warning.



  {
struct drm_i915_private *dev_priv = ring->dev->dev_private;
struct i915_scheduler *scheduler = dev_priv->scheduler;
@@ -874,6 +876,29 @@ void i915_scheduler_process_work(struct intel_engine_cs 
*ring)
  }
  
  /**

+ * i915_scheduler_work_handler - scheduler's work handler callback.
+ * @work: Work structure
+ * A lot of the scheduler's work must be done asynchronously in response to
+ * an interrupt or other event. However, that work cannot be done at
+ * interrupt time or in the context of the event signaller (which might in
+ * fact be an interrupt). Thus a worker thread is required. This function
+ * will cause the thread to wake up and do its processing.
+ */
+void i915_scheduler_work_handler(struct 

Re: [Intel-gfx] [PATCH v5 12/35] drm/i915: Added deferred work handler for scheduler

2016-03-01 Thread Joonas Lahtinen
On to, 2016-02-18 at 14:27 +, john.c.harri...@intel.com wrote:
> From: John Harrison 
> 
> The scheduler needs to do interrupt triggered work that is too complex
> to do in the interrupt handler. Thus it requires a deferred work
> handler to process such tasks asynchronously.
> 
> v2: Updated to reduce mutex lock usage. The lock is now only held for
> the minimum time within the remove function rather than for the whole
> of the worker thread's operation.
> 
> v5: Removed objectionable white space and added some documentation.
> [Joonas Lahtinen]
> 
> For: VIZ-1587
> Signed-off-by: John Harrison 
> Cc: Joonas Lahtinen 
> ---
>  drivers/gpu/drm/i915/i915_dma.c   |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h   | 10 ++
>  drivers/gpu/drm/i915/i915_gem.c   |  2 ++
>  drivers/gpu/drm/i915/i915_scheduler.c | 29 +++--
>  drivers/gpu/drm/i915/i915_scheduler.h |  1 +
>  5 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 678adc7..c3d382d 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1158,6 +1158,9 @@ int i915_driver_unload(struct drm_device *dev)
>   WARN_ON(unregister_oom_notifier(_priv->mm.oom_notifier));
>   unregister_shrinker(_priv->mm.shrinker);
>  
> + /* Cancel the scheduler work handler, which should be idle now. */
> + cancel_work_sync(_priv->mm.scheduler_work);
> +
>   io_mapping_free(dev_priv->gtt.mappable);
>   arch_phys_wc_del(dev_priv->gtt.mtrr);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 03add1a..4d544f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1291,6 +1291,16 @@ struct i915_gem_mm {
>   struct delayed_work retire_work;
>  
>   /**
> +  * New scheme is to get an interrupt after every work packet
> +  * in order to allow the low latency scheduling of pending
> +  * packets. The idea behind adding new packets to a pending
> +  * queue rather than directly into the hardware ring buffer
> +  * is to allow high priority packets to over take low priority
> +  * ones.
> +  */
> + struct work_struct scheduler_work;
> +
> + /**
>    * When we detect an idle GPU, we want to turn on
>    * powersaving features. So once we see that there
>    * are no more requests outstanding and no more
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c3b7def..1ab7256 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5427,6 +5427,8 @@ i915_gem_load(struct drm_device *dev)
>     i915_gem_retire_work_handler);
>   INIT_DELAYED_WORK(_priv->mm.idle_work,
>     i915_gem_idle_work_handler);
> + INIT_WORK(_priv->mm.scheduler_work,
> + i915_scheduler_work_handler);
>   init_waitqueue_head(_priv->gpu_error.reset_queue);
>  
>   dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c 
> b/drivers/gpu/drm/i915/i915_scheduler.c
> index ab5007a..3986890 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -697,7 +697,9 @@ static int i915_scheduler_remove_dependent(struct 
> i915_scheduler *scheduler,
>   */
>  void i915_scheduler_wakeup(struct drm_device *dev)
>  {
> - /* XXX: Need to call i915_scheduler_remove() via work handler. */
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + queue_work(dev_priv->wq, _priv->mm.scheduler_work);
>  }
>  
>  /**
> @@ -827,7 +829,7 @@ static bool i915_scheduler_remove(struct i915_scheduler 
> *scheduler,
>   return do_submit;
>  }
>  
> -void i915_scheduler_process_work(struct intel_engine_cs *ring)
> +static void i915_scheduler_process_work(struct intel_engine_cs *ring)

This is an odd change, maybe should have been static void to begin
with?

>  {
>   struct drm_i915_private *dev_priv = ring->dev->dev_private;
>   struct i915_scheduler *scheduler = dev_priv->scheduler;
> @@ -874,6 +876,29 @@ void i915_scheduler_process_work(struct intel_engine_cs 
> *ring)
>  }
>  
>  /**
> + * i915_scheduler_work_handler - scheduler's work handler callback.
> + * @work: Work structure
> + * A lot of the scheduler's work must be done asynchronously in response to
> + * an interrupt or other event. However, that work cannot be done at
> + * interrupt time or in the context of the event signaller (which might in
> + * fact be an interrupt). Thus a worker thread is required. This function
> + * will cause the thread to wake up and do its processing.
> + */
> +void i915_scheduler_work_handler(struct work_struct *work)
> +{
> + struct intel_engine_cs *ring;
> 

[Intel-gfx] [PATCH v5 12/35] drm/i915: Added deferred work handler for scheduler

2016-02-18 Thread John . C . Harrison
From: John Harrison 

The scheduler needs to do interrupt triggered work that is too complex
to do in the interrupt handler. Thus it requires a deferred work
handler to process such tasks asynchronously.

v2: Updated to reduce mutex lock usage. The lock is now only held for
the minimum time within the remove function rather than for the whole
of the worker thread's operation.

v5: Removed objectionable white space and added some documentation.
[Joonas Lahtinen]

For: VIZ-1587
Signed-off-by: John Harrison 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_dma.c   |  3 +++
 drivers/gpu/drm/i915/i915_drv.h   | 10 ++
 drivers/gpu/drm/i915/i915_gem.c   |  2 ++
 drivers/gpu/drm/i915/i915_scheduler.c | 29 +++--
 drivers/gpu/drm/i915/i915_scheduler.h |  1 +
 5 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 678adc7..c3d382d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1158,6 +1158,9 @@ int i915_driver_unload(struct drm_device *dev)
WARN_ON(unregister_oom_notifier(_priv->mm.oom_notifier));
unregister_shrinker(_priv->mm.shrinker);
 
+   /* Cancel the scheduler work handler, which should be idle now. */
+   cancel_work_sync(_priv->mm.scheduler_work);
+
io_mapping_free(dev_priv->gtt.mappable);
arch_phys_wc_del(dev_priv->gtt.mtrr);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 03add1a..4d544f1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1291,6 +1291,16 @@ struct i915_gem_mm {
struct delayed_work retire_work;
 
/**
+* New scheme is to get an interrupt after every work packet
+* in order to allow the low latency scheduling of pending
+* packets. The idea behind adding new packets to a pending
+* queue rather than directly into the hardware ring buffer
+* is to allow high priority packets to over take low priority
+* ones.
+*/
+   struct work_struct scheduler_work;
+
+   /**
 * When we detect an idle GPU, we want to turn on
 * powersaving features. So once we see that there
 * are no more requests outstanding and no more
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c3b7def..1ab7256 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5427,6 +5427,8 @@ i915_gem_load(struct drm_device *dev)
  i915_gem_retire_work_handler);
INIT_DELAYED_WORK(_priv->mm.idle_work,
  i915_gem_idle_work_handler);
+   INIT_WORK(_priv->mm.scheduler_work,
+   i915_scheduler_work_handler);
init_waitqueue_head(_priv->gpu_error.reset_queue);
 
dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
diff --git a/drivers/gpu/drm/i915/i915_scheduler.c 
b/drivers/gpu/drm/i915/i915_scheduler.c
index ab5007a..3986890 100644
--- a/drivers/gpu/drm/i915/i915_scheduler.c
+++ b/drivers/gpu/drm/i915/i915_scheduler.c
@@ -697,7 +697,9 @@ static int i915_scheduler_remove_dependent(struct 
i915_scheduler *scheduler,
  */
 void i915_scheduler_wakeup(struct drm_device *dev)
 {
-   /* XXX: Need to call i915_scheduler_remove() via work handler. */
+   struct drm_i915_private *dev_priv = to_i915(dev);
+
+   queue_work(dev_priv->wq, _priv->mm.scheduler_work);
 }
 
 /**
@@ -827,7 +829,7 @@ static bool i915_scheduler_remove(struct i915_scheduler 
*scheduler,
return do_submit;
 }
 
-void i915_scheduler_process_work(struct intel_engine_cs *ring)
+static void i915_scheduler_process_work(struct intel_engine_cs *ring)
 {
struct drm_i915_private *dev_priv = ring->dev->dev_private;
struct i915_scheduler *scheduler = dev_priv->scheduler;
@@ -874,6 +876,29 @@ void i915_scheduler_process_work(struct intel_engine_cs 
*ring)
 }
 
 /**
+ * i915_scheduler_work_handler - scheduler's work handler callback.
+ * @work: Work structure
+ * A lot of the scheduler's work must be done asynchronously in response to
+ * an interrupt or other event. However, that work cannot be done at
+ * interrupt time or in the context of the event signaller (which might in
+ * fact be an interrupt). Thus a worker thread is required. This function
+ * will cause the thread to wake up and do its processing.
+ */
+void i915_scheduler_work_handler(struct work_struct *work)
+{
+   struct intel_engine_cs *ring;
+   struct drm_i915_private *dev_priv;
+   struct drm_device *dev;
+   int i;
+
+   dev_priv = container_of(work, struct drm_i915_private, 
mm.scheduler_work);
+   dev = dev_priv->dev;
+
+   for_each_ring(ring, dev_priv, i)
+   i915_scheduler_process_work(ring);
+}
+