On Friday, September 12, 2008 12:29 am Thomas Hellström wrote:
> + if (drm_mm_init(&dev->offset_manager, DRM_FILE_PAGE_OFFSET_START,
> + DRM_FILE_PAGE_OFFSET_SIZE)) {
> + drm_ht_remove(&dev->map_hash);
> + return -ENOMEM;
> + }
>
> A driver not using GEM would probably not want the map_hash in the core
> data structure. What about making a small struct that the driver can
> include in its driver_private? Likewise, what about a gem-enabled VM system
> that the driver can hook in instead of normal drm mmap, that detects GEM
> objects and calls normal drm mmap otherwise?
So bypassing all the other unused drm_mmap stuff? Yeah that would make things
cleaner. I'll do that and separate out the map_hash too.
> +
> + vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>
> You'd be much happier with a write-combined page protection. Also, it's not
> really allowed to alter the vma->vm_page_prot without the mm_sem taken in
> write mode. In the fault() function it's taken in read mode. In your case
> it should be easy to set up the vma->vm_page_prot member correctly in the
> mmap() call, since you're only dealing with write-combined mappings.
Venki is in the process of adding a pgprot_writecombine; Dave said he had a
DRM specific pgprot_* function for me to use in the meantime. But setting it
up at mmap time would be an improvement, I'll fix that.
> + pfn = ((dev->agp->base + obj_priv->gtt_offset) >> PAGE_SHIFT) +
> + page_offset;
> + /* Finally, remap it using the new GTT offset */
> + ret = vm_insert_pfn(vma, (unsigned long)vmf->virtual_address, pfn);
>
> Perhaps speculatively fault a couple of more pages for performance?
Yeah I was thinking about that; I guess it depends on the typical use case.
Right now, we're mainly using this for X fallbacks, and with tiling hopefully
we won't be touching many pages each time, but some instrumentation would
help here.
> +int
> +i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_i915_gem_mmap_gtt *args = data;
> + struct drm_gem_object *obj;
> + struct drm_i915_gem_object *obj_priv;
> +
> + if (!(dev->driver->driver_features & DRIVER_GEM))
> + return -ENODEV;
> +
> + obj = drm_gem_object_lookup(dev, file_priv, args->handle);
> + if (obj == NULL)
> + return -EBADF;
> +
> + obj_priv = obj->driver_private;
> +
> + mutex_lock(&dev->struct_mutex);
> + drm_gem_object_unreference(obj);
> + mutex_unlock(&dev->struct_mutex);
> +
> + args->addr_ptr = obj_priv->mmap_offset;
> +
> + obj_priv->gtt_alignment = args->alignment;
> +
> + return 0;
> +}
>
> I'm not sure I understand what this function is doing?
Basically just returning the fake mmap offset of a given object to userspace
so it can use it for subsequent mmap calls. It also grabs the preferred
alignment so we know how to GTT map it when the time comes. Is there some
easier way of getting that info? I suppose we could just return it at create
time instead since we initialize everything there anyway...
> @@ -940,6 +1029,10 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
>
> BUG_ON(obj_priv->active);
>
> + /* blow away mappings if mapped through GTT */
> + if (vma)
> + zap_vma_ptes(vma, vma->vm_start, vma->vm_end - vma->vm_start);
> +
> i915_gem_object_free_page_list(obj);
>
>
> What stops this racing with fault(), and furthermore how are multiple vmas
> to the same object handled? That happens if multiple processes map the same
> object or if you hit a fork(). What are the intended fork() semantics for a
> GTT mapped GEM object?
Oh, I hadn't thought about that case; I'll have to create a list of VMAs and
maybe take an appropriate mutex.
Thanks a lot for looking it over.
--
Jesse Barnes, Intel Open Source Technology Center
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
