On to, 2016-02-18 at 14:26 +0000, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> The scheduler decouples the submission of batch buffers to the driver
> with submission of batch buffers to the hardware. Thus it is possible
> for an application to close its DRM file handle while there is still
> work outstanding. That means the scheduler needs to know about file
> close events so it can remove the file pointer from such orphaned
> batch buffers and not attempt to dereference it later.
>
> v3: Updated to not wait for outstanding work to complete but merely
> remove the file handle reference. The wait was getting excessively
> complicated with inter-ring dependencies, pre-emption, and other such
> issues.
>
> v4: Changed some white space to keep the style checker happy.
>
> v5: Added function documentation and removed apparently objectionable
> white space. [Joonas Lahtinen]
>
> Used lighter weight spinlocks.
>
> For: VIZ-1587
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 3 +++
> drivers/gpu/drm/i915/i915_scheduler.c | 48 +++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_scheduler.h | 2 ++
> 3 files changed, 53 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a0f5659..678adc7 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -46,6 +46,7 @@
> #include
> #include
> #include
> +#include "i915_scheduler.h"
> #include
> #include
> #include
> @@ -1258,6 +1259,8 @@ void i915_driver_lastclose(struct drm_device *dev)
>
> void i915_driver_preclose(struct drm_device *dev, struct drm_file *file)
> {
> + i915_scheduler_closefile(dev, file);
Any reason not to put this inside the struct_mutex lock?
> +
> mutex_lock(&dev->struct_mutex);
> i915_gem_context_close(dev, file);
> i915_gem_release(dev, file);
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c
> index fc23ee7..ab5007a 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> @@ -872,3 +872,51 @@ void i915_scheduler_process_work(struct intel_engine_cs *ring)
> if (do_submit)
> intel_runtime_pm_put(dev_priv);
> }
> +
> +/**
> + * i915_scheduler_closefile - notify the scheduler that a DRM file handle
> + * has been closed.
> + * @dev: DRM device
> + * @file: file being closed
> + *
> + * Goes through the scheduler's queues and removes all connections to the
> + * disappearing file handle that still exist. There is an argument to say
> + * that this should also flush such outstanding work through the hardware.
> + * However, with pre-emption, TDR and other such complications doing so
> + * becomes a locking nightmare. So instead, just warn with a debug message
> + * if the application is leaking uncompleted work and make sure a null
> + * pointer dereference will not follow.
> + */
> +int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file)
Return value not used, should be void if this can not fail.
Other than that,
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> +{
> + struct i915_scheduler_queue_entry *node;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct i915_scheduler *scheduler = dev_priv->scheduler;
> + struct intel_engine_cs *ring;
> + int i;
> +
> + if (!scheduler)
> + return 0;
> +
> + spin_lock_irq(&scheduler->lock);
> +
> + for_each_ring(ring, dev_priv, i) {
> + list_for_each_entry(node, &scheduler->node_queue[ring->id], link) {
> + if (node->params.file != file)
> + continue;
> +
> + if (!I915_SQS_IS_COMPLETE(node))
> + DRM_DEBUG_DRIVER("Closing file handle with outstanding work: %d:%d/%d on %s\n",
> + node->params.request->uniq,
> + node->params.request->seqno,
> + node->status,
> + ring->name);
> +
> + node->params.file = NULL;
> + }
> + }
> +
> + spin_unlock_irq(&scheduler->lock);
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> index 415fec8..0e8b6a9 100644
> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> @@ -87,6 +87,8 @@ enum {
>
> bool i915_scheduler_is_enabled(struct drm_device *dev);
> int i915_scheduler_init(struct drm_device *dev);
> +int i915_scheduler_closefile(struct drm_device *dev,
> + struct drm_file *file);
> void i915_scheduler_clean_node(struct i915_scheduler_queue_entry *node);
> int i915_scheduler_queue_execbuffer(struct i915_scheduler_queue_entry *qe);
> bool i915_scheduler_notify_request(struct drm_i915_gem_request *req);
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
