[Intel-gfx] [RFC 13/44] drm/i915: Added scheduler hook when closing DRM file handles

[Jesse Barnes]

On Wed, 23 Jul 2014 16:10:32 +0100
John Harrison <John.C.Harrison at Intel.com> wrote:

> On 02/07/2014 19:20, Jesse Barnes wrote:
> > On Thu, 26 Jun 2014 18:24:04 +0100
> > John.C.Harrison at Intel.com wrote:
> >
> >> From: John Harrison <John.C.Harrison at Intel.com>
> >>
> >> 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 submit work, then close the DRM handle and free up all the
> >> resources that piece of work wishes to use before the work has even been
> >> submitted to the hardware. To prevent this, the scheduler needs to be informed
> >> of the DRM close event so that it can force through any outstanding work
> >> attributed to that file handle.
> >> ---
> >>   drivers/gpu/drm/i915/i915_dma.c       |    3 +++
> >>   drivers/gpu/drm/i915/i915_scheduler.c |   18 ++++++++++++++++++
> >>   drivers/gpu/drm/i915/i915_scheduler.h |    2 ++
> >>   3 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >> index 494b156..6c9ce82 100644
> >> --- a/drivers/gpu/drm/i915/i915_dma.c
> >> +++ b/drivers/gpu/drm/i915/i915_dma.c
> >> @@ -42,6 +42,7 @@
> >>   #include <linux/vga_switcheroo.h>
> >>   #include <linux/slab.h>
> >>   #include <acpi/video.h>
> >> +#include "i915_scheduler.h"
> >>   #include <linux/pm.h>
> >>   #include <linux/pm_runtime.h>
> >>   #include <linux/oom.h>
> >> @@ -1930,6 +1931,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);
> >> +
> >>   	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 d9c1879..66a6568 100644
> >> --- a/drivers/gpu/drm/i915/i915_scheduler.c
> >> +++ b/drivers/gpu/drm/i915/i915_scheduler.c
> >> @@ -78,6 +78,19 @@ bool i915_scheduler_is_seqno_in_flight(struct intel_engine_cs *ring,
> >>   	return found;
> >>   }
> >>   
> >> +int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file)
> >> +{
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct i915_scheduler   *scheduler = dev_priv->scheduler;
> >> +
> >> +	if (!scheduler)
> >> +		return 0;
> >> +
> >> +	/* Do stuff... */
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   #else   /* CONFIG_DRM_I915_SCHEDULER */
> >>   
> >>   int i915_scheduler_init(struct drm_device *dev)
> >> @@ -85,4 +98,9 @@ int i915_scheduler_init(struct drm_device *dev)
> >>   	return 0;
> >>   }
> >>   
> >> +int i915_scheduler_closefile(struct drm_device *dev, struct drm_file *file)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >>   #endif  /* CONFIG_DRM_I915_SCHEDULER */
> >> diff --git a/drivers/gpu/drm/i915/i915_scheduler.h b/drivers/gpu/drm/i915/i915_scheduler.h
> >> index 4044b6e..95641f6 100644
> >> --- a/drivers/gpu/drm/i915/i915_scheduler.h
> >> +++ b/drivers/gpu/drm/i915/i915_scheduler.h
> >> @@ -27,6 +27,8 @@
> >>   
> >>   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);
> >>   
> >>   #ifdef CONFIG_DRM_I915_SCHEDULER
> >>   
> > Yeah I guess the client could have passed a ref to some other process
> > for tracking the outstanding work, so we need to complete it.
> >
> > But shouldn't that happen as part of the clearing of the outstanding
> > requests in i915_gem_suspend() which is called from lastclose()?  We do
> > a gpu_idle() and retire_requests() in there already...
> >
> 
> Note that this is per file close not the global close.  Individual DRM 
> file handles are closed whenever a user land app stops using DRM. When 
> that happens, the scheduler needs to clean up all references to that 
> handle. It is not just to ensure all work belonging to that handle has 
> completed but also to ensure the scheduler does not attempt to deference 
> dodgy file pointers later on.

Ah yeah sorry, mixed it up with lastclose.  Looks fine for per-client
close.

-- 
Jesse Barnes, Intel Open Source Technology Center