On 26/01/16 15:10, Chris Wilson wrote:
On Tue, Jan 26, 2016 at 02:53:31PM +0000, Tvrtko Ursulin wrote:From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> --- drivers/gpu/drm/i915/i915_gem.c | 96 ++++++++++++++++++++++++++++++++++++++--- include/uapi/drm/i915_drm.h | 3 ++ 2 files changed, 93 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index dacf6a0013c5..039d55a49fc6 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1954,6 +1954,60 @@ out: return i915_gem_ret_to_vm_ret(dev_priv, ret); } +static int +i915_gem_cpu_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; + bool write = !!(vmf->flags & FAULT_FLAG_WRITE); + pgoff_t page_offset; + struct page *page; + int ret; + + /* We don't use vmf->pgoff since that has the fake offset */ + page_offset = ((unsigned long)vmf->virtual_address - vma->vm_start) >> + PAGE_SHIFT; + + trace_i915_gem_object_fault(obj, page_offset, true, write); + + intel_runtime_pm_get(dev_priv); + + ret = i915_mutex_lock_interruptible(dev); + if (ret) + goto out; + + ret = i915_gem_object_set_to_cpu_domain(obj, write); + if (ret) + goto out_unlock;That was a mistake in the GTT gem_fault(). If you do this, we also want the nonblocking wait for obvious reasons.
You suggest leaving it for userspace? And how would a non-blocking wait work?
+ ret = i915_gem_object_get_pages(obj); + if (ret) + goto out_unlock; + + page = i915_gem_object_get_page(obj, page_offset); + if (!page) { + ret = -ERANGE; + goto out_unlock; + } + + mutex_unlock(&dev->struct_mutex); + + ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address, + page_to_pfn(page));We don't have a page ref at this point, so this obj+page could be freed (via the shrinker at least) before we insert it.
Oh yeah, need to pin the pages..
I would also be more interested in having a version that faulted the entire object at once - though maybe we will see more random access in future.
Yeah I did not want to concern myself with more code since this was a proof of concept anyway.
+ intel_runtime_pm_put(dev_priv); + + return i915_gem_ret_to_vm_ret(dev_priv, ret); + +out_unlock: + mutex_unlock(&dev->struct_mutex); +out: + intel_runtime_pm_put(dev_priv); + + return i915_gem_ret_to_vm_ret(dev_priv, ret); +} + /** * i915_gem_release_mmap - remove physical page mappings * @obj: obj in question @@ -2078,11 +2132,18 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj) drm_gem_free_mmap_offset(&obj->base); } -int -i915_gem_mmap_gtt(struct drm_file *file, - struct drm_device *dev, - uint32_t handle, - uint64_t *offset) +static const struct vm_operations_struct i915_gem_cpu_vm_ops = { + .fault = i915_gem_cpu_fault, + .open = drm_gem_vm_open, + .close = drm_gem_vm_close, +}; + +static int +i915_gem_mmap(struct drm_file *file, + struct drm_device *dev, + uint32_t handle, + uint32_t flags, + uint64_t *offset) { struct drm_i915_gem_object *obj; int ret; @@ -2103,10 +2164,23 @@ i915_gem_mmap_gtt(struct drm_file *file, goto out; } + if (!obj->base.filp && (flags & I915_MMAP2_CPU)) { + DRM_DEBUG("Attempting to mmap non-shm based object via CPU!\n"); + ret = -EINVAL; + goto out; + } + ret = i915_gem_object_create_mmap_offset(obj); if (ret) goto out; + if (flags & I915_MMAP2_CPU) { + ret = drm_vma_node_set_vm_ops(&obj->base.vma_node, + &i915_gem_cpu_vm_ops); + if (ret) + goto out; + }We would also need a WC equivalent. It looks fairly sane. I wanted this just a short while ago, but figured out a way of using regular mmap() to give me the inheritance instead.
So would it be useful to cleanup and finish this work or not? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
