Re: [PATCH 01/10] drm/i915: Move map-and-fenceable tracking to the VMA

Joonas Lahtinen
2016-08-15T08:03:39Z

On pe, 2016-08-12 at 11:28 +0100, Chris Wilson wrote:
> @@ -2843,8 +2843,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>   GEM_BUG_ON(obj->bind_count == 0);
>   GEM_BUG_ON(!obj->pages);
>
> - if (i915_vma_is_ggtt(vma) &&
> -     vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {

Maybe make a comment here, as the test feel out-of-place quickly
glancing. Especially wrt. what it replaces. Although you mentioned in
IRC this will soon be eliminated?

> + if (i915_vma_is_map_and_fenceable(vma)) {
>   i915_gem_object_finish_gtt(obj);
>
>   /* release the fence reg _after_ flushing */

<SNIP>

> @@ -2864,13 +2864,9 @@ int i915_vma_unbind(struct i915_vma *vma)
>   drm_mm_remove_node(&vma->node);
>   list_move_tail(&vma->vm_link, &vma->vm->unbound_list);
>
> - if (i915_vma_is_ggtt(vma)) {
> - if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
> - obj->map_and_fenceable = false;
> - } else if (vma->pages) {
> - sg_free_table(vma->pages);
> - kfree(vma->pages);
> - }

Not sure if there should be a comment that for 1:1 mappings vma->pages
is just obj->pages so it should not be freed. Or maybe you could even
make the test if vma->pages != vma->obj->pages? More self-documenting.

> + if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> + sg_free_table(vma->pages);
> + kfree(vma->pages);
>   }
>   vma->pages = NULL;

<SNIP>

> @@ -3693,7 +3687,10 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)

This might also clear, so function name should be
update_map_and_fenceable, really.

> @@ -2262,11 +2262,11 @@ void intel_unpin_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>   WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
>
>   intel_fill_fb_ggtt_view(&view, fb, rotation);
> + vma = i915_gem_object_to_ggtt(obj, &view);
>
> - if (view.type == I915_GGTT_VIEW_NORMAL)
> + if (i915_vma_is_map_and_fenceable(vma))
>   i915_gem_object_unpin_fence(obj);
>
> - vma = i915_gem_object_to_ggtt(obj, &view);
>   i915_gem_object_unpin_from_display_plane(vma);

This did not have NULL protection previously either, so should be OK.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation