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

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

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