Re: [PATCH 05/33] drm/i915: Reduce amount of duplicate buffer information captured on error

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> @@ -414,18 +403,25 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
>  			error_print_engine(m, &error->engine[i]);
>  	}
>  
> -	for (i = 0; i < error->vm_count; i++) {
> -		err_printf(m, "vm[%d]\n", i);
> +	for (i = 0; i < I915_NUM_ENGINES; i++) {
> +		if (!error->active_vm[i])
> +			break;
>  
> -		print_error_buffers(m, "Active",
> +		err_printf(m, "Active vm[%d]\n", i);
> +		for (j = 0; j < I915_NUM_ENGINES; j++) {
> +			if (error->engine[j].vm == error->active_vm[i])

break here and then print outside loop?

> +				err_printf(m, "    %s\n",
> +					   dev_priv->engine[j].name);
> +		}
> +

<SNIP>

>  static void i915_gem_capture_vm(struct drm_i915_private *dev_priv,
>  				struct drm_i915_error_state *error,
>  				struct i915_address_space *vm,
>  				const int ndx)
>  {
> -	struct drm_i915_error_buffer *active_bo = NULL, *pinned_bo = NULL;
> -	struct drm_i915_gem_object *obj;
> +	struct drm_i915_error_buffer *active_bo;
>  	struct i915_vma *vma;
>  	int i;

'count';

>  
>  	i = 0;
>  	list_for_each_entry(vma, &vm->active_list, vm_link)
>  		i++;
> -	error->active_bo_count[ndx] = i;
> -
> -	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> -		list_for_each_entry(vma, &obj->vma_list, obj_link)
> -			if (vma->vm == vm && i915_vma_is_pinned(vma))
> -				i++;
> -	}
> -	error->pinned_bo_count[ndx] = i - error->active_bo_count[ndx];
>  
> -	if (i) {
> +	active_bo = NULL;

Could be initialized at declaration for better readability.

> +	if (i)
>  		active_bo = kcalloc(i, sizeof(*active_bo), GFP_ATOMIC);
> -		if (active_bo)
> -			pinned_bo = active_bo + error->active_bo_count[ndx];
> -	}
> -
>  	if (active_bo)
> -		error->active_bo_count[ndx] =
> -			capture_active_bo(active_bo,
> -					  error->active_bo_count[ndx],
> -					  &vm->active_list);
> -
> -	if (pinned_bo)
> -		error->pinned_bo_count[ndx] =
> -			capture_pinned_bo(pinned_bo,
> -					  error->pinned_bo_count[ndx],
> -					  &dev_priv->mm.bound_list, vm);
> +		i = capture_error_bo(active_bo, i, &vm->active_list, false);
> +	else
> +		i = 0;
> +
>  	error->active_bo[ndx] = active_bo;
> -	error->pinned_bo[ndx] = pinned_bo;
> +	error->active_bo_count[ndx] = i;

While at it, make the i variable 'count' and initialize it at the
declaration too. And maybe make the ndx variable something reasonable
like 'engine' or 'eid'.

> +	error->active_vm[ndx] = vm;
>  }
> 

<SNIP>

>  
> +	for (i = 0; i < I915_NUM_ENGINES; i++) {
> +		struct drm_i915_error_engine *ee = &error->engine[i];
> +
> +		if (!ee->vm)
> +			continue;
> +
> +		for (j = 0; j < i; j++)
> +			if (error->engine[j].vm == ee->vm)
> +				break;
> +		if (j != i)
> +			continue;

Maybe add a comment that we wan't to avoid capturing same vm twice.

> +
> +		i915_gem_capture_vm(dev_priv, error, ee->vm, cnt++);
>  	}
>  }
>  
> +static void i915_capture_pinned_buffers(struct drm_i915_private *dev_priv,
> +					struct drm_i915_error_state *error)
> +{
> +	struct i915_address_space *vm = &dev_priv->ggtt.base;
> +	struct drm_i915_error_buffer *bo;
> +	struct i915_vma *vma;
> +	int i, j;

count_active, count_inactive? i and j are iteration variables.

> +
> +	i = 0;
> +	list_for_each_entry(vma, &vm->active_list, vm_link)
> +		i++;
> +
> +	j = 0;
> +	list_for_each_entry(vma, &vm->inactive_list, vm_link)
> +		j++;
> +
> +	bo = NULL;

Initialize at declaration as this is one-shot.

>  /* Capture all registers which don't fit into another category. */
>  static void i915_capture_reg_state(struct drm_i915_private *dev_priv,
>  				   struct drm_i915_error_state *error)
> @@ -1436,10 +1402,12 @@ void i915_capture_error_state(struct drm_i915_private *dev_priv,
>  
>  	i915_capture_gen_state(dev_priv, error);
>  	i915_capture_reg_state(dev_priv, error);
> -	i915_gem_capture_buffers(dev_priv, error);
>  	i915_gem_record_fences(dev_priv, error);
>  	i915_gem_record_rings(dev_priv, error);
>  
> +	i915_capture_active_buffers(dev_priv, error);
> +	i915_capture_pinned_buffers(dev_priv, error);
> +

Any specific reason for reordering here?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx