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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
