Hi, Jesse, Some comments:
Jesse Barnes wrote: > Here's an updated set of GTT mapping patches against Eric's drm-gem-merge > branch from earlier today and DRM master for the libdrm bits. I fixed up a > couple of bugs, but it looks like UXA/EXA still don't work against GEM even > w/o these changes applied. I'm not quite sure about the zap_vma_ptes code, > that'll need review from one of the MM guys (Nick can you take a look at the > i915-gtt-mapping patch?). > > Other than that, I think the code is looking fairly complete, so at this > point > we just need to stabilize GEM and play with using GTT mapping for the general > dri_bo_map case to see if we want to do it by default. > > Thanks, > > ------------------------------------------------------------------------ > > ------------------------------------------------------------------------- > 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 + 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? + + 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. + 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? +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? @@ -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? /Thomas ------------------------------------------------------------------------- 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
