Re: [PATCH 05/10] drm/i915: Fix partial GGTT faulting

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

On pe, 2016-08-12 at 11:28 +0100, Chris Wilson wrote:
> @@ -1717,26 +1716,30 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>  	}
>  
>  	/* Use a partial view if the object is bigger than the aperture. */

Move this comment down to where partial view is actually created.

> -	if (obj->base.size >= ggtt->mappable_end &&
> -	    !i915_gem_object_is_tiled(obj)) {
> +	/* Now pin it into the GTT if needed */
> +	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> +				       PIN_MAPPABLE | PIN_NONBLOCK);
> +	if (IS_ERR(vma)) {
> +		struct i915_ggtt_view partial;

'view' still makes more sense, less repeating of the word partial down.

> @@ -1754,26 +1757,7 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>  	pfn = ggtt->mappable_base + i915_ggtt_offset(vma);
>  	pfn >>= PAGE_SHIFT;
>  
> -	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
> -		/* Overriding existing pages in partial view does not cause
> -		 * us any trouble as TLBs are still valid because the fault
> -		 * is due to userspace losing part of the mapping or never
> -		 * having accessed it before (at this partials' range).
> -		 */
> -		unsigned long base = area->vm_start +
> -				     (view.params.partial.offset << PAGE_SHIFT);
> -		unsigned int i;
> -
> -		for (i = 0; i < view.params.partial.size; i++) {
> -			ret = vm_insert_pfn(area,
> -					    base + i * PAGE_SIZE,
> -					    pfn + i);
> -			if (ret)
> -				break;
> -		}
> -
> -		obj->fault_mappable = true;
> -	} else {
> +	if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {

likely() ?

>  		if (!obj->fault_mappable) {
>  			unsigned long size =
>  				min_t(unsigned long,
> @@ -1789,13 +1773,31 @@ int i915_gem_fault(struct vm_area_struct *area, struct vm_fault *vmf)
>  				if (ret)
>  					break;
>  			}
> -
> -			obj->fault_mappable = true;
>  		} else
>  			ret = vm_insert_pfn(area,
>  					    (unsigned long)vmf->virtual_address,
>  					    pfn + page_offset);
> +	} else {
> +		/* Overriding existing pages in partial view does not cause
> +		 * us any trouble as TLBs are still valid because the fault
> +		 * is due to userspace losing part of the mapping or never
> +		 * having accessed it before (at this partials' range).
> +		 */
> +		const struct i915_ggtt_view *view = &vma->ggtt_view;

I now see why you did the rename. Do not have a better idea really, so;

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