There is a typo so deadlocks on error instead of unlocking.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@gmail.com>
... not only when the dma-buf is freshly created. In contrived
examples someone else could have exported/imported the dma-buf already
and handed us the gem object with a flink name. If such on object gets
reexported as a dma_buf we won't have it in the handle cache already,
which breaks the guarantee that for dma-buf imports we always hand
back an existing handle if there is one.
This is exercised by igt/prime_self_import/with_one_bo_two_files
Now if we extend the locked sections just a notch more we can also
plug th racy buf/handle cache setup in handle_to_fd:
If evil userspace races a concurrent gem close against a prime export
operation we can end up tearing down the gem handle before the dma buf
handle cache is set up. When handle_to_fd gets around to adding the
handle to the cache there will be no one left to clean it up,
effectily leaking the bo (and the dma-buf, since the handle cache
holds a ref on the dma-buf):
Thread A Thread B
handle_to_fd:
lookup gem object from handle
creates new dma_buf
gem_close on the same handle
obj->dma_buf is set, but file priv buf
handle cache has no entry
obj->handle_count drops to 0
drm_prime_add_buf_handle sets up the handle cache
-> We have a dma-buf reference in the handle cache, but since the
handle_count of the gem object already dropped to 0 no on will clean
it up. When closing the drm device fd we'll hit the WARN_ON in
drm_prime_destroy_file_private.
The important change is to extend the critical section of the
filp->prime.lock to cover the gem handle lookup. This serializes with
a concurrent gem handle close.
This leak is exercised by igt/prime_self_import/export-vs-gem_close-race
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
... and move it to the top of the function to avoid a forward
declaration.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The export dma-buf cache is semantically similar to an flink name. So
semantically it makes sense to treat it the same and remove the name
(i.e. the dma_buf pointer) and its references when the last gem handle
disappears.
Again we need to be careful, but double so: Not just could someone
race and export with a gem close ioctl (so we need to recheck
obj->handle_count again when assigning the new name), but multiple
exports can also race against each another. This is prevented by
holding the dev->object_name_lock across the entire section which
touches obj->dma_buf.
With the new scheme we also need to reinstate the obj->dma_buf link at
import time (in case the only reference userspace has held in-between
was through the dma-buf fd and not through any native gem handle). For
simplicity we don't check whether it's a native object but
unconditionally set up that link - with the new scheme of removing the
obj->dma_buf reference when the last handle disappears we can do that.
To make it clear that this is not just for exported buffers anymore
als rename it from export_dma_buf to dma_buf.
To make sure that now one can race a fd_to_handle or handle_to_fd with
gem_close we use the same tricks as in flink of extending the
dev->object_name_locking critical section. With this change we finally
have a guaranteed 1:1 relationship (at least for native objects)
between gem objects and dma-bufs, even accounting for races (which can
happen since the dma-buf itself holds a reference while in-flight).
This prevent igt/prime_self_import/export-vs-gem_close-race from
Oopsing the kernel. There is still a leak though since the per-file
priv dma-buf/handle cache handling is racy. That will be fixed in a
later patch.
v2: Remove the bogus dma_buf_put from the export_and_register_object
failure path if we've raced with the handle count dropping to 0.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
if (!ret) implies that ret == 0, so no need to clear it again. And
explicitly check for ret == 0 to indicate that we're checking an errno
integer.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
When exporting a gem object as a dma-buf the critical section for the
per-fd prime lock is just the adding (and in case of errors, removing)
of the handle to the per-fd lookup cache.
So restrict the critical section to just that part of the function.
This simplifies later reordering.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Part of the function uses the properly-typed dmabuf variable, the
other an untyped void *buf. Kill the later.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
handle_unreference only clears up the obj->name and the reference,
but would leave a dangling handle in the idr. The right thing
to do is to call handle_delete.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
I've checked both implementations (radeon/nouveau) and they both grab
the page array from ttm simply by dereferencing it and then wrapping
it up with drm_prime_pages_to_sg in the callback and map it with
dma_map_sg (in the helper).
Only the grabbing of the underlying page array is anything we need to
be concerned about, and either those pages are pinned independently,
or we're screwed no matter what.
And indeed, nouveau/radeon pin the backing storage in their
attach/detach functions.
Since I've created this patch cma prime support for dma_buf was added.
drm_gem_cma_prime_get_sg_table only calls kzalloc and the creates&maps
the sg table with dma_get_sgtable. It doesn't touch any gem object
state otherwise. So the cma helpers also look safe.
The only thing we might claim it does is prevent concurrent mapping of
dma_buf attachments. But a) that's not allowed and b) the current code
is racy already since it checks whether the sg mapping exists _before_
grabbing the lock.
So the dev->struct_mutex locking here does absolutely nothing useful,
but only distracts. Remove it.
This should also help Maarten's work to eventually pin the backing
storage more dynamically by preventing locking inversions around
dev->struct_mutex.
v2: Add analysis for recently added cma helper prime code.
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Note that this is slightly tricky since both drivers store their
native objects in dma_buf->priv. But both also embed the base
drm_gem_object at the first position, so the implicit cast is ok.
To use the release helper we need to export it, too.
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
This adds to call low-level mmap() from prime helpers.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The drm_gem_map_detach() can be called with sgt is NULL.
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
In
commit da34242e5e
Author: YoungJun Cho <yj44.cho@samsung.com>
Date: Wed Jun 26 10:21:42 2013 +0900
drm/prime: add return check for dma_buf_fd
the failure case handling was fixed up. But in the case when we
already had the buffer exported it changed the return value:
Previously we've return 0 on success, now we return the fd.
This ABI change has been caught by i-g-t/prime_self_import/with_one_bo.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66436
Cc: YoungJun Cho <yj44.cho@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Tested-by: lu hua <huax.lu@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The dma_buf_fd() can return error when it fails to prepare fd,
so the dma_buf needs to be put.
Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
When drm_prime_add_buf_handle() returns failure for an exported
dma_buf, the dma_buf was already allocated and its refcount was
increased, so it needs to be put.
Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The drm prime also can support it like GEM CMA supports to cache
mapping. It doesn't allow multiple mappings for one attachment.
[airlied: rebased on top of other prime changes]
Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Instead of NULL, error value is casted with ERR_PTR() for
drm_prime_pages_to_sg() and IS_ERR_OR_NULL() macro is replaced
with IS_ERR() macro for drm_gem_map_dma_buf().
Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
The dma_map_sg(), in map_dma_buf callback operation of prime helper,
can return 0 when it fails to map, so it needs to release related
resources.
Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (GNU/Linux)
iQEbBAABAgAGBQJRxf9cAAoJEHm+PkMAQRiGMWkH911xM4gRmFgE7SqVW4F4AWBm
ngcqMqNy9IdqKfibORUUDvVfEa5gjD5ai2quIKpfQiaukbpQJ696H90ijuAkajLn
DQBrN243s0pzhhc/quWINnWxsFQ613JjdUMUMaD7e9A1aKjYzWrPGt/tSjrFXGCP
tArTupVzc/iOmnEQDKiROI/Nokq44QJ36aTGPM7n08xMtpKmkCXM+9/UosBteB0O
HVI33dmjwz7i55fI53XAWyuZCE+gSEnA4z8spJ9LfXso2W14V+roc+GuL6OyeeTI
pCn/+4niVPb4B0ROZlpyVmdZjbPPcMMEK5o+BSJI68SH6LHZTQh2iVuqYfpSyA==
=uUH5
-----END PGP SIGNATURE-----
Merge tag 'v3.10-rc7' into drm-next
Linux 3.10-rc7
The sdvo lvds fix in this -fixes pull
commit c3456fb3e4
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date: Mon Jun 10 09:47:58 2013 +0200
drm/i915: prefer VBT modes for SVDO-LVDS over EDID
has a silent functional conflict with
commit 990256aec2
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date: Fri May 31 12:17:07 2013 +0000
drm: Add probed modes in probe order
in drm-next. W simply need to add the vbt modes before edid modes, i.e. the
other way round than now.
Conflicts:
drivers/gpu/drm/drm_prime.c
drivers/gpu/drm/i915/intel_sdvo.c
The DRM PRIME API passes file flags to the driver for the exported
buffer. Honor them instead of hardcoding 0600.
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>
This allows importing bo's to own device to work without requiring that the buffer is pinned in GART.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Prevents buffers from being pinned forever.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
drm_gem_release should release all handles connected to the drm file and
so should also release the prime lookup entries of these handles. So
just WARN if this isn't the case.
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
In commit be8a42ae60 we inroduced a refcount problem, where on the
drm_gem_prime_fd_to_handle() error path we'll call dma_buf_put() for
self imported dma buffers.
Fix this by taking a reference on the dma buffer in the .gem_import
hook instead of assuming the caller had taken one. Besides fixing the
bug this is also more logical.
Signed-off-by: Imre Deak <imre.deak@intel.com>
Cc: stable@vger.kernel.org
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Currently we have a problem with this:
1. i915: create gem object
2. i915: export gem object to prime
3. radeon: import gem object
4. close prime fd
5. radeon: unref object
6. i915: unref object
i915 has an imported object reference in its file priv, that isn't
cleaned up properly until fd close. The reference gets added at step 2,
but at step 6 we don't have enough info to clean it up.
The solution is to take a reference on the dma-buf when we export it,
and drop the reference when the gem handle goes away.
So when we export a dma_buf from a gem object, we keep track of it
with the handle, we take a reference to the dma_buf. When we close
the handle (i.e. userspace is finished with the buffer), we drop
the reference to the dma_buf, and it gets collected.
This patch isn't meant to fix any other problem or bikesheds, and it doesn't
fix any races with other scenarios.
v1.1: move export symbol line back up.
v2: okay I had to do a bit more, as the first patch showed a leak
on one of my tests, that I found using the dma-buf debugfs support,
the problem case is exporting a buffer twice with the same handle,
we'd add another export handle for it unnecessarily, however
we now fail if we try to export the same object with a different gem handle,
however I'm not sure if that is a case I want to support, and I've
gotten the code to WARN_ON if we hit something like that.
v2.1: rebase this patch, write better commit msg.
v3: cleanup error handling, track import vs export in linked list,
these two patches were separate previously, but seem to work better
like this.
v4: danvet is correct, this code is no longer useful, since the buffer
better exist, so remove it.
v5: always take a reference to the dma buf object, import or export.
(Imre Deak contributed this originally)
v6: square the circle, remove import vs export tracking now
that there is no difference
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
Signed-off-by: Dave Airlie <airlied@redhat.com>
It fixes the issue arises due to passing 'nr_pages' in place of 'nents' to
sg_alloc_table. When ARM_HAS_SG_CHAIN is disabled, it is causing failure in
creating SG table for the buffers having more than 204 physical pages i.e.
equal to SG_MAX_SINGLE_ALLOC.
When using sg_alloc_table_from_pages interface, in place of sg_alloc_table,
page list will be passes to get each contiguous section which is represented
by a single entry in the table. For a Contiguous Buffer, number of entries
should be equal to 1.
Following check is causing the failure which is not applicable for Non-Contig
buffers:
if (WARN_ON_ONCE(nents > max_ents))
return -EINVAL;
Above patch is well tested for EXYNOS4 and EXYNOS5 for with/wihtout IOMMU
supprot. NOUVEAU and RADEON platforms also depends on drm_prime_pages_to_sg
helper function.
This set is base on "exynos-drm-fixes" branch at
http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git
Signed-off-by: Rahul Sharma <rahul.sharma@samsung.com>
Acked-by: Dave Airlie <airlied@gmail.com>
Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
Tested-by: Aaron Plattner <aplattner@nvidia.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Instead of reimplementing all of the dma_buf functionality in every driver,
create helpers drm_prime_import and drm_prime_export that implement them in
terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT
gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export
gem_prime_import_sg_table: convert an sg_table into a drm_gem_object
gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and
drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of
struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces implemented
it. Having it here was a leftover from when I was trying to include i915 in
this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers
did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table.
Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call vunmap on
the pointer avoid having to store the pointer in their GEM private structures.
- Move documentation into a /** DOC */ comment in drm_prime.c and include it in
drm.tmpl with a !P line. I tried to use !F lines to include documentation of
the individual functions from drmP.h, but the docproc / kernel-doc scripts
barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Convert #include "..." to #include <path/...> in drivers/gpu/.
Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: Dave Airlie <airlied@redhat.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Acked-by: Dave Jones <davej@redhat.com>
If userspace attempts to import a buffer it exported on the same device,
we need to return the same GEM handle for it, not a new handle pointing
at the same GEM object.
v2: move removals into a single fn, no need to set to NULL. (Chris Wilson)
Signed-off-by: Dave Airlie <airlied@redhat.com>
the ttm drivers need this currently, in order to get fault handling
working and efficient.
It also allows addrs to be NULL for devices like udl.
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
This adds the basic drm dma-buf interface layer, called PRIME. This
commit doesn't add any driver support, it is simply and agreed upon starting
point so we can work towards merging driver support for the next merge window.
Current drivers with work done are nouveau, i915, udl, exynos and omap.
The main APIs exposed to userspace allow translating a 32-bit object handle
to a file descriptor, and a file descriptor to a 32-bit object handle.
The flags value is currently limited to O_CLOEXEC.
Acknowledgements:
Daniel Vetter: lots of review
Rob Clark: cleaned up lots of the internals and did lifetime review.
v2: rename some functions after Chris preferred a green shed
fix IS_ERR_OR_NULL -> IS_ERR
v3: Fix Ville pointed out using buffer + kmalloc
v4: add locking as per ickle review
v5: allow re-exporting the original dma-buf (Daniel)
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Rob Clark <rob.clark@linaro.org>
Reviewed-by: Sumit Semwal <sumit.semwal@linaro.org>
Reviewed-by: Inki Dae <inki.dae@samsung.com>
Acked-by: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
