Re: [PATCH v2 5/5] drm/i915: Use partial view in mmap fault handler

Tvrtko Ursulin
2015-05-05T09:07:46Z

On 05/04/2015 12:51 PM, Joonas Lahtinen wrote:
> On to, 2015-04-30 at 15:54 +0100, Tvrtko Ursulin wrote:
>> On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
>>>
>>> Use partial view for huge BOs (bigger than the mappable aperture)
>>> in fault handler so that they can be accessed without trying to make
>>> room for them by evicting other objects.
>>>
>>> v2:
>>> - Only use partial views in the case where early rejection was
>>> previously done.
>>> - Account variable type changes from previous reroll.
>>>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_gem.c | 69 +++++++++++++++++++++++++++--------------
>>> 1 file changed, 46 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index a020836..2f3fa0b 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>>> struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
>>> struct drm_device *dev = obj->base.dev;
>>> struct drm_i915_private *dev_priv = dev->dev_private;
>>> + struct i915_ggtt_view view = i915_ggtt_view_normal;
>>> pgoff_t page_offset;
>>> unsigned long pfn;
>>> int ret = 0;
>>> @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>>> goto unlock;
>>> }
>>>
>>> - /* Now bind it into the GTT if needed */
>>> - ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
>>> + /* Use a partial view if the object is bigger than the aperture. */
>>> + if (obj->base.size >= dev_priv->gtt.mappable_end) {
>>> + static const unsigned int chunk_size = 256; // 1 MiB
>>> + memset(&view, 0, sizeof(view));
>>> + view.type = I915_GGTT_VIEW_PARTIAL;
>>> + view.params.partial.offset = rounddown(page_offset, chunk_size);
>>> + view.params.partial.size =
>>> + min_t(unsigned int,
>>> + chunk_size,
>>> + (vma->vm_end - vma->vm_start)/PAGE_SIZE -
>>> + view.params.partial.offset);
>>> + }
>>> +
>>> + /* Now pin it into the GTT if needed */
>>> + ret = i915_gem_object_ggtt_pin(obj, &view, 0, PIN_MAPPABLE);
>>> if (ret)
>>> goto unlock;
>>>
>>> @@ -1681,30 +1695,44 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
>>> goto unpin;
>>>
>>> /* Finally, remap it using the new GTT offset */
>>> - pfn = dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj);
>>> + pfn = dev_priv->gtt.mappable_base +
>>> + i915_gem_obj_ggtt_offset_view(obj, &view);
>>> pfn >>= PAGE_SHIFT;
>>>
>>> - if (!obj->fault_mappable) {
>>> - unsigned long size = min_t(unsigned long,
>>> - vma->vm_end - vma->vm_start,
>>> - obj->base.size);
>>> - int i;
>>> + if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
>>> + unsigned long base = vma->vm_start +
>>> + (view.params.partial.offset << PAGE_SHIFT);
>>> + unsigned int i;
>>>
>>> - for (i = 0; i < size >> PAGE_SHIFT; i++) {
>>> - ret = vm_insert_pfn(vma,
>>> - (unsigned long)vma->vm_start + i * PAGE_SIZE,
>>> - pfn + i);
>>> + for (i = 0; i < view.params.partial.size; i++) {
>>> + ret = vm_insert_pfn(vma, base + i * PAGE_SIZE, pfn + i);
>>> if (ret)
>>> break;
>>> }
>>> -
>>> obj->fault_mappable = true;
>>> - } else
>>> - ret = vm_insert_pfn(vma,
>>> - (unsigned long)vmf->virtual_address,
>>> - pfn + page_offset);
>>
>> If I read the diff correctly you don't have equivalent handling (as the
>> normal view) for when the case when the pre-fault fails somewhere in the
>> middle?
>>
>
> True so, the flag fault_mappable is used for the normal view to track
> whether all pages were inserted and it makes sense to just insert the
> faulted one. I just didn't want to add another flag to track the same
> for each vma.

But it is safe to do it multiple times?

Either way I would put a comment in explaining the difference between
code paths.

Regards,

Tvrtko