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 <[email protected]>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
