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

Joonas Lahtinen
2016-08-15T09:29:44Z

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