Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/02/16 11:36, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:28AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Purpose is to catch places which iterate the object VMA list
>> without holding the big lock.
>>
>> Implemented by open coding list_for_each_entry to make the
>> macro compatible with existing call sites.
>>
>> v2: Error capture runs without the mutex so iterate directly from there.
>> v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
>> v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>>   drivers/gpu/drm/i915/i915_drv.h          | 11 +++++++++++
>>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>   5 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 863012a2602e..ff444f09ea98 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>   	u64 size = 0;
>>   	struct i915_vma *vma;
>>
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		if (i915_is_ggtt(vma->vm) &&
>>   		    drm_mm_node_allocated(&vma->node))
>>   			size += vma->node.size;
>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>   	if (obj->base.name)
>>   		seq_printf(m, " (name: %d)", obj->base.name);
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		if (vma->pin_count > 0)
>>   			pin_count++;
>>   	}
>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   		seq_printf(m, " (display)");
>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>>   			   vma->node.start, vma->node.size);
>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>   		stats->shared += obj->base.size;
>>
>>   	if (USES_FULL_PPGTT(obj->base.dev)) {
>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +		i915_gem_obj_for_each_vma(vma, obj) {
>>   			struct i915_hw_ppgtt *ppgtt;
>>
>>   			if (!drm_mm_node_allocated(&vma->node))
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 905e90f25957..05ef750386df 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>>
>> +#ifdef CONFIG_DRM_I915_DEBUG
>> +  #define i915_gem_obj_for_each_vma(vma, obj) \
>
> This is the wrong name, i915_gem_object is the object on which we are
> iterating over, so i915_gem_object_.

To me obj is just a shorthand for object, sorry. :) It sounds like bad 
naming if obj and object mean something functionally different. Imagine 
if request, req and rq meant something different. :D

>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>
> I disagree that we should be doing i915-specific lockdep debugging and
> *ignoring* the core facilities. If we locked at RCU, they introduced a
> new set of _protected/_check interfaces, an idea that could also be used
> here to add this or something like this to the core.
> Something like
>
> #define list_for_each_entry_check(pos, list, member, lock) \
> for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
>       pos = list_first_entry(head, typeof(*pos), member); \
>       &pos->member != (head); \
>       pos = list_next_entry(pos, member))
>
> #define i915_gem_object_for_each_vma(vma, obj) \
> 	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
> 	
> could be lifted easily, and makes i915_gem_object_for_each_vma() easier
> to understand (i.e. serves better as documentation).

Don't know, needs buy-in from the relevant people, and depends on how 
useful to outside of i915 it would be. And if you can make 
lockdep_assert_held work in this context.

But I am also not sure all i915 debugging should be tied to lockdep 
debugging so to me it is not so clear-cut.

>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>> +	     &vma->vma_link != (&(obj)->vma_list); \
>> +	     vma = list_next_entry(vma, vma_link))
>> +#else
>> +  #define i915_gem_obj_for_each_vma(vma, obj) \
>> +		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
>> +#endif
>> +
>>   /* Flags used by pin/bind&friends. */
>>   #define PIN_MAPPABLE	(1<<0)
>>   #define PIN_NONBLOCK	(1<<1)
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index c558887b2084..ce9d0544b42c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>>   	list_move_tail(&obj->global_list,
>>   		       &to_i915(obj->base.dev)->mm.bound_list);
>>
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>
> This and the majority of the conversions simply should not exist and
> only do so because of what I consider to be bad API. After they are

Bad API or what you really meant was GEM internals should not do it? 
What is the harm anyway? More use, if it is painless, means less 
likelyhood for copy&paste errors in the future.

> removed, there are only a few list iterators left. That said, there is
> value in documenting the current locking.

The last bit meaning exactly?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx