Re: Gem GTT mmaps..

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Friday, February 6, 2009 2:39 pm Thomas Hellström wrote:
> Jesse Barnes wrote:
> > On Friday, February 6, 2009 1:35 pm Thomas Hellström wrote:
> >> Jesse Barnes wrote:
> >>> On Thursday, February 5, 2009 10:37 am Jesse Barnes wrote:
> >>>> So if we leave the lookup reference around from the GTT mapping ioctl,
> >>>> that would take care of new mappings.  And if we added/removed
> >>>> references at VM open/close time, we should be covered for fork.  But
> >>>> is it ok to add a new unref in the finish ioctl for GTT mapped
> >>>> objects?  I don't think so, because we don't know for sure if the
> >>>> caller was the one that created the new fake offset (which would be
> >>>> one way of detecting whether it was GTT mapped). Seems like we need a
> >>>> new unmap ioctl?  Or we could put the mapping ref/unref in libdrm,
> >>>> where it would be tracked on a per-process basis...
> >>>
> >>> Ah but maybe we should just tear down the fake offset at unmap time;
> >>> then we'd be able to use it as an existence test for the mapping and
> >>> get the refcounting right.  The last thing I thought of was whether
> >>> we'd be ok in a map_gtt -> crash case.  I *think* the vm_close code
> >>> will deal with that, if we do a deref there?
> >>
> >> Yes, an mmap() is always paired with a vm_close(), and the vm_close()
> >> also happens in a crash situation.
> >
> > This one should cover the cases you found.
> >   - ref at map time will keep the object around so fault shouldn't fail
> >   - additional threads will take their refs in vm_open/close
> >   - unmap will unref and remove mmap_offset allowing object to be freed
>
> Jesse,
>
> Yes, it looks OK to me.
> A short question, though, when is i915_gem_sw_finish_ioctl called? Is it
> possible that the client may still think its mmap offset is valid?

It's called at drm_bo_unmap time, but only if the swrast bit is set, so we'd
need to do that in libdrm as well.  So if the program is written properly it
shouldn't think the mapping is valid (we should probably set bo->virtual to
0 in unmap as well, to catch those errors).

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/libdrm/intel/intel_bufmgr_gem.c b/libdrm/intel/intel_bufmgr_gem.c
index f578a67..1ea4761 100644
--- a/libdrm/intel/intel_bufmgr_gem.c
+++ b/libdrm/intel/intel_bufmgr_gem.c
@@ -670,6 +670,7 @@ drm_intel_gem_bo_map_gtt(drm_intel_bo *bo)
        }
     }

+    bo_gem->swrast = 1;
     bo->virtual = bo_gem->virtual;

     DBG("bo_map: %d (%s) -> %p\n", bo_gem->gem_handle, bo_gem->name,
@@ -716,6 +717,8 @@ drm_intel_gem_bo_unmap(drm_intel_bo *bo)
        } while (ret == -1 && errno == EINTR);
        bo_gem->swrast = 0;
     }
+    bo_gem->virtual = NULL;
+    bo->virtual = NULL;
     pthread_mutex_unlock(&bufmgr_gem->lock);
     return 0;
 }