diff mbox

[v6,4/5] drm/i915: Mark shadow batch buffers as purgeable

Message ID 1418078030-17690-5-git-send-email-michael.h.nguyen@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Michael H. Nguyen Dec. 8, 2014, 10:33 p.m. UTC 
From: Brad Volkin <bradley.d.volkin@intel.com>

By adding a new exec_entry flag, we cleanly mark the shadow objects
as purgeable after they are on the active list.

v2:
- Move 'shadow_batch_obj->madv = I915_MADV_WILLNEED' inside _get
  fnc (danvet, from v4 6/7 feedback)

Issue: VIZ-4719
Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_batch_pool.c |  2 ++
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Dec. 9, 2014, 1:21 p.m. UTC | #1 
On Mon, Dec 08, 2014 at 02:33:49PM -0800, michael.h.nguyen@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> By adding a new exec_entry flag, we cleanly mark the shadow objects
> as purgeable after they are on the active list.
> 
> v2:
> - Move 'shadow_batch_obj->madv = I915_MADV_WILLNEED' inside _get
>   fnc (danvet, from v4 6/7 feedback)

Seems duplicated now.
-Daniel

> 
> Issue: VIZ-4719
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_batch_pool.c |  2 ++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> index e9349e3..9e0ec4b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> @@ -128,5 +128,7 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
>  		list_add_tail(&obj->batch_pool_list, &pool->cache_list);
>  	}
>  
> +	obj->madv = I915_MADV_WILLNEED;
> +
>  	return obj;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index fccfff5..2071938 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -37,6 +37,7 @@
>  #define  __EXEC_OBJECT_HAS_FENCE (1<<30)
>  #define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
>  #define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
> +#define  __EXEC_OBJECT_PURGEABLE (1<<27)
>  
>  #define BATCH_OFFSET_BIAS (256*1024)
>  
> @@ -226,7 +227,12 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
>  	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
>  		vma->pin_count--;
>  
> -	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
> +	if (entry->flags & __EXEC_OBJECT_PURGEABLE)
> +		obj->madv = I915_MADV_DONTNEED;
> +
> +	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
> +			  __EXEC_OBJECT_HAS_PIN |
> +			  __EXEC_OBJECT_PURGEABLE);
>  }
>  
>  static void eb_destroy(struct eb_vmas *eb)
> @@ -1410,6 +1416,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  		}
>  
> +		shadow_batch_obj->madv = I915_MADV_WILLNEED;
> +
>  		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>  		if (ret)
>  			goto err;
> @@ -1433,6 +1441,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  
>  			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>  			vma->exec_entry = &shadow_exec_entry;
> +			vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
>  			drm_gem_object_reference(&shadow_batch_obj->base);
>  			list_add_tail(&vma->exec_list, &eb->vmas);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Michael H. Nguyen Dec. 9, 2014, 9:34 p.m. UTC | #2 
On 12/09/2014 05:21 AM, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 02:33:49PM -0800, michael.h.nguyen@intel.com wrote:
>> From: Brad Volkin <bradley.d.volkin@intel.com>
>>
>> By adding a new exec_entry flag, we cleanly mark the shadow objects
>> as purgeable after they are on the active list.
>>
>> v2:
>> - Move 'shadow_batch_obj->madv = I915_MADV_WILLNEED' inside _get
>>    fnc (danvet, from v4 6/7 feedback)
>
> Seems duplicated now.
Good eyes! The duplicate does go away in 5/5 as part of moving much of 
the cmdparser code out of do_execbuffer() but it still doesn't make 4/5 
right. Will fix.

Thanks,
Mike
> -Daniel
>
>>
>> Issue: VIZ-4719
>> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_batch_pool.c |  2 ++
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ++++++++++-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> index e9349e3..9e0ec4b 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> @@ -128,5 +128,7 @@ i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
>>   		list_add_tail(&obj->batch_pool_list, &pool->cache_list);
>>   	}
>>
>> +	obj->madv = I915_MADV_WILLNEED;
>> +
>>   	return obj;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index fccfff5..2071938 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -37,6 +37,7 @@
>>   #define  __EXEC_OBJECT_HAS_FENCE (1<<30)
>>   #define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
>>   #define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
>> +#define  __EXEC_OBJECT_PURGEABLE (1<<27)
>>
>>   #define BATCH_OFFSET_BIAS (256*1024)
>>
>> @@ -226,7 +227,12 @@ i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
>>   	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
>>   		vma->pin_count--;
>>
>> -	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
>> +	if (entry->flags & __EXEC_OBJECT_PURGEABLE)
>> +		obj->madv = I915_MADV_DONTNEED;
>> +
>> +	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
>> +			  __EXEC_OBJECT_HAS_PIN |
>> +			  __EXEC_OBJECT_PURGEABLE);
>>   }
>>
>>   static void eb_destroy(struct eb_vmas *eb)
>> @@ -1410,6 +1416,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   			goto err;
>>   		}
>>
>> +		shadow_batch_obj->madv = I915_MADV_WILLNEED;
>> +
>>   		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>>   		if (ret)
>>   			goto err;
>> @@ -1433,6 +1441,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>
>>   			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>>   			vma->exec_entry = &shadow_exec_entry;
>> +			vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
>>   			drm_gem_object_reference(&shadow_batch_obj->base);
>>   			list_add_tail(&vma->exec_list, &eb->vmas);
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Bloomfield, Jon Dec. 11, 2014, 1:26 p.m. UTC | #3 
> -----Original Message-----
> From: Nguyen, Michael H
> Sent: Monday, December 08, 2014 10:34 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Bloomfield, Jon; Brad Volkin
> Subject: [PATCH v6 4/5] drm/i915: Mark shadow batch buffers as purgeable
> 
> From: Brad Volkin <bradley.d.volkin@intel.com>
> 
> By adding a new exec_entry flag, we cleanly mark the shadow objects as
> purgeable after they are on the active list.
> 
> v2:
> - Move 'shadow_batch_obj->madv = I915_MADV_WILLNEED' inside _get
>   fnc (danvet, from v4 6/7 feedback)
> 
> Issue: VIZ-4719
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_batch_pool.c |  2 ++
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> index e9349e3..9e0ec4b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
> @@ -128,5 +128,7 @@ i915_gem_batch_pool_get(struct
> i915_gem_batch_pool *pool,
>  		list_add_tail(&obj->batch_pool_list, &pool->cache_list);
>  	}
> 
> +	obj->madv = I915_MADV_WILLNEED;
> +
>  	return obj;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index fccfff5..2071938 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -37,6 +37,7 @@
>  #define  __EXEC_OBJECT_HAS_FENCE (1<<30)  #define
> __EXEC_OBJECT_NEEDS_MAP (1<<29)  #define
> __EXEC_OBJECT_NEEDS_BIAS (1<<28)
> +#define  __EXEC_OBJECT_PURGEABLE (1<<27)
> 
>  #define BATCH_OFFSET_BIAS (256*1024)
> 
> @@ -226,7 +227,12 @@ i915_gem_execbuffer_unreserve_vma(struct
> i915_vma *vma)
>  	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
>  		vma->pin_count--;
> 
> -	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
> __EXEC_OBJECT_HAS_PIN);
> +	if (entry->flags & __EXEC_OBJECT_PURGEABLE)
> +		obj->madv = I915_MADV_DONTNEED;
> +
> +	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
> +			  __EXEC_OBJECT_HAS_PIN |
> +			  __EXEC_OBJECT_PURGEABLE);
>  }
> 
>  static void eb_destroy(struct eb_vmas *eb) @@ -1410,6 +1416,8 @@
> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  		}
> 
> +		shadow_batch_obj->madv = I915_MADV_WILLNEED;
> +

Hasn't i915_gem_batch_pool_get() has already marked the buffer as WILLNEED ?


>  		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>  		if (ret)
>  			goto err;
> @@ -1433,6 +1441,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> void *data,
> 
>  			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>  			vma->exec_entry = &shadow_exec_entry;
> +			vma->exec_entry->flags =
> __EXEC_OBJECT_PURGEABLE;
>  			drm_gem_object_reference(&shadow_batch_obj-
> >base);
>  			list_add_tail(&vma->exec_list, &eb->vmas);
> 
> --
> 1.9.1
Michael H. Nguyen Dec. 11, 2014, 6:56 p.m. UTC | #4 
On 12/11/2014 05:26 AM, Bloomfield, Jon wrote:
>> -----Original Message-----
>> From: Nguyen, Michael H
>> Sent: Monday, December 08, 2014 10:34 PM
>> To: intel-gfx@lists.freedesktop.org
>> Cc: Bloomfield, Jon; Brad Volkin
>> Subject: [PATCH v6 4/5] drm/i915: Mark shadow batch buffers as purgeable
>>
>> From: Brad Volkin <bradley.d.volkin@intel.com>
>>
>> By adding a new exec_entry flag, we cleanly mark the shadow objects as
>> purgeable after they are on the active list.
>>
>> v2:
>> - Move 'shadow_batch_obj->madv = I915_MADV_WILLNEED' inside _get
>>    fnc (danvet, from v4 6/7 feedback)
>>
>> Issue: VIZ-4719
>> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_batch_pool.c |  2 ++
>> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 ++++++++++-
>>   2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> index e9349e3..9e0ec4b 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
>> @@ -128,5 +128,7 @@ i915_gem_batch_pool_get(struct
>> i915_gem_batch_pool *pool,
>>   		list_add_tail(&obj->batch_pool_list, &pool->cache_list);
>>   	}
>>
>> +	obj->madv = I915_MADV_WILLNEED;
>> +
>>   	return obj;
>>   }
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index fccfff5..2071938 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -37,6 +37,7 @@
>>   #define  __EXEC_OBJECT_HAS_FENCE (1<<30)  #define
>> __EXEC_OBJECT_NEEDS_MAP (1<<29)  #define
>> __EXEC_OBJECT_NEEDS_BIAS (1<<28)
>> +#define  __EXEC_OBJECT_PURGEABLE (1<<27)
>>
>>   #define BATCH_OFFSET_BIAS (256*1024)
>>
>> @@ -226,7 +227,12 @@ i915_gem_execbuffer_unreserve_vma(struct
>> i915_vma *vma)
>>   	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
>>   		vma->pin_count--;
>>
>> -	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
>> __EXEC_OBJECT_HAS_PIN);
>> +	if (entry->flags & __EXEC_OBJECT_PURGEABLE)
>> +		obj->madv = I915_MADV_DONTNEED;
>> +
>> +	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
>> +			  __EXEC_OBJECT_HAS_PIN |
>> +			  __EXEC_OBJECT_PURGEABLE);
>>   }
>>
>>   static void eb_destroy(struct eb_vmas *eb) @@ -1410,6 +1416,8 @@
>> i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>>   			goto err;
>>   		}
>>
>> +		shadow_batch_obj->madv = I915_MADV_WILLNEED;
>> +
>
> Hasn't i915_gem_batch_pool_get() has already marked the buffer as WILLNEED ?
Yes, good eyes. It was actually corrected in 5/5 but that doesn't make 
this patch okay. Will fix.

Thanks,
Mike
>
>
>>   		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
>>   		if (ret)
>>   			goto err;
>> @@ -1433,6 +1441,7 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>> void *data,
>>
>>   			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
>>   			vma->exec_entry = &shadow_exec_entry;
>> +			vma->exec_entry->flags =
>> __EXEC_OBJECT_PURGEABLE;
>>   			drm_gem_object_reference(&shadow_batch_obj-
>>> base);
>>   			list_add_tail(&vma->exec_list, &eb->vmas);
>>
>> --
>> 1.9.1
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_batch_pool.c b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
index e9349e3..9e0ec4b 100644
--- a/drivers/gpu/drm/i915/i915_gem_batch_pool.c
+++ b/drivers/gpu/drm/i915/i915_gem_batch_pool.c
@@ -128,5 +128,7 @@  i915_gem_batch_pool_get(struct i915_gem_batch_pool *pool,
 		list_add_tail(&obj->batch_pool_list, &pool->cache_list);
 	}
 
+	obj->madv = I915_MADV_WILLNEED;
+
 	return obj;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index fccfff5..2071938 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -37,6 +37,7 @@ 
 #define  __EXEC_OBJECT_HAS_FENCE (1<<30)
 #define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
 #define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
+#define  __EXEC_OBJECT_PURGEABLE (1<<27)
 
 #define BATCH_OFFSET_BIAS (256*1024)
 
@@ -226,7 +227,12 @@  i915_gem_execbuffer_unreserve_vma(struct i915_vma *vma)
 	if (entry->flags & __EXEC_OBJECT_HAS_PIN)
 		vma->pin_count--;
 
-	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE | __EXEC_OBJECT_HAS_PIN);
+	if (entry->flags & __EXEC_OBJECT_PURGEABLE)
+		obj->madv = I915_MADV_DONTNEED;
+
+	entry->flags &= ~(__EXEC_OBJECT_HAS_FENCE |
+			  __EXEC_OBJECT_HAS_PIN |
+			  __EXEC_OBJECT_PURGEABLE);
 }
 
 static void eb_destroy(struct eb_vmas *eb)
@@ -1410,6 +1416,8 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			goto err;
 		}
 
+		shadow_batch_obj->madv = I915_MADV_WILLNEED;
+
 		ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 4096, 0);
 		if (ret)
 			goto err;
@@ -1433,6 +1441,7 @@  i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 			vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
 			vma->exec_entry = &shadow_exec_entry;
+			vma->exec_entry->flags = __EXEC_OBJECT_PURGEABLE;
 			drm_gem_object_reference(&shadow_batch_obj->base);
 			list_add_tail(&vma->exec_list, &eb->vmas);