diff mbox

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

Message ID 1403803475-16337-14-git-send-email-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

John Harrison June 26, 2014, 5:24 p.m. UTC
From: John Harrison <John.C.Harrison@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(+)

Comments

Jesse Barnes July 2, 2014, 6:20 p.m. UTC | #1
On Thu, 26 Jun 2014 18:24:04 +0100
John.C.Harrison@Intel.com wrote:

> From: John Harrison <John.C.Harrison@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...
John Harrison July 23, 2014, 3:10 p.m. UTC | #2
On 02/07/2014 19:20, Jesse Barnes wrote:
> On Thu, 26 Jun 2014 18:24:04 +0100
> John.C.Harrison@Intel.com wrote:
>
>> From: John Harrison <John.C.Harrison@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.
Jesse Barnes July 23, 2014, 3:39 p.m. UTC | #3
On Wed, 23 Jul 2014 16:10:32 +0100
John Harrison <John.C.Harrison@Intel.com> wrote:

> On 02/07/2014 19:20, Jesse Barnes wrote:
> > On Thu, 26 Jun 2014 18:24:04 +0100
> > John.C.Harrison@Intel.com wrote:
> >
> >> From: John Harrison <John.C.Harrison@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.
diff mbox

Patch

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