Re: [PATCH 11/11] video/aperture: Only remove sysfb on the default vga pci device
On 1/11/23 8:58 AM, Javier Martinez Canillas wrote:
Hello Daniel,
On 1/11/23 16:41, Daniel Vetter wrote:
This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
sysfb device registration when removing conflicting FBs"), where we
remove the sysfb when loading a driver for an unrelated pci device,
resulting in the user loosing their efifb console or similar.
Note that in practice this only is a problem with the nvidia blob,
because that's the only gpu driver people might install which does not
come with an fbdev driver of it's own. For everyone else the real gpu
driver will restor a working console.
restore
Also note that in the referenced bug there's confusion that this same
bug also happens on amdgpu. But that was just another amdgpu specific
regression, which just happened to happen at roughly the same time and
with the same user-observable symptons. That bug is fixed now, see
symptoms
https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
For the above reasons the cc: stable is just notionally, this patch
will need a backport and that's up to nvidia if they care enough.
Maybe adding a Fixes: ee7a69aa38d8 tag here too ?
References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
Signed-off-by: Daniel Vetter
Cc: Aaron Plattner
Cc: Javier Martinez Canillas
Cc: Thomas Zimmermann
Cc: Helge Deller
Cc: Sam Ravnborg
Cc: Alex Deucher
Cc: # v5.19+ (if someone else does the backport)
---
drivers/video/aperture.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index ba565515480d..a1821d369bb1 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct
pci_dev *pdev, const char *na
primary = pdev == vga_default_device();
+ if (primary)
+ sysfb_disable();
+
for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
continue;
base = pci_resource_start(pdev, bar);
size = pci_resource_len(pdev, bar);
- ret = aperture_remove_conflicting_devices(base, size, name);
- if (ret)
- return ret;
+ aperture_detach_devices(base, size);
Maybe mention in the commit message that you are doing this change, something
like:
"Instead of calling aperture_remove_conflicting_devices() to remove the
conflicting
devices, just call to aperture_detach_devices() to detach the device that
matches
the same PCI BAR / aperture range. Since the former is just a wrapper of the
latter
plus a sysfb_disable() call, and now that's done in this function but only for
the
primary devices"
Patch looks good to me:
Reviewed-by: Javier Martinez Canillas
Thanks Daniel and Javier!
I wasn't able to reproduce the original problem on my hybrid laptop
since it refuses to boot with the console on an external display, but I
was able to reproduce it by switching the configuration around: booting
with i915.modeset=0 and with an experimental version of nvidia-drm that
registers a framebuffer console. I verified that loading nvidia-drm
breaks the efi-firmware framebuffer on Intel on Arch's
linux-6.1.4-arch1-1 kernel and that applying this patch series fixes it. So
Tested-by: Aaron Plattner
FWIW, the bug ought to be reproducible with i915.modeset=0 + any other
drm driver that registers a framebuffer.
-- Aaron
Re: [PATCH 7/8] video/aperture: Only remove sysfb on the default vga pci device
On 4/4/23 1:18 PM, Daniel Vetter wrote:
Instead of calling aperture_remove_conflicting_devices() to remove the
conflicting devices, just call to aperture_detach_devices() to detach
the device that matches the same PCI BAR / aperture range. Since the
former is just a wrapper of the latter plus a sysfb_disable() call,
and now that's done in this function but only for the primary devices.
This fixes a regression introduced by ee7a69aa38d8 ("fbdev: Disable
sysfb device registration when removing conflicting FBs"), where we
remove the sysfb when loading a driver for an unrelated pci device,
resulting in the user loosing their efifb console or similar.
Note that in practice this only is a problem with the nvidia blob,
because that's the only gpu driver people might install which does not
come with an fbdev driver of it's own. For everyone else the real gpu
driver will restore a working console.
It might be worth noting that this also affects devices that have no
driver installed, or where the driver failed to initialize or was
configured not to set a mode. E.g. I reproduced this problem on a laptop
with i915.modeset=0 and an NVIDIA driver that calls
drm_fbdev_generic_setup. It would also reproduce on a system that sets
modeset=0 (or has a GPU that's too new for its corresponding kernel
driver) and that passes an NVIDIA GPU through to a VM using vfio-pci
since that also calls aperture_remove_conflicting_pci_devices.
I agree that in practice this will mostly affect people with our driver
until I get my changes to add drm_fbdev_generic_setup checked in. But
these other cases don't seem all that unlikely to me.
-- Aaron
Also note that in the referenced bug there's confusion that this same
bug also happens on amdgpu. But that was just another amdgpu specific
regression, which just happened to happen at roughly the same time and
with the same user-observable symptoms. That bug is fixed now, see
https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
Note that we should not have any such issues on non-pci multi-gpu
issues, because I could only find two such cases:
- SoC with some external panel over spi or similar. These panel
drivers do not use drm_aperture_remove_conflicting_framebuffers(),
so no problem.
- vga+mga, which is a direct console driver and entirely bypasses all
this.
For the above reasons the cc: stable is just notionally, this patch
will need a backport and that's up to nvidia if they care enough.
v2:
- Explain a bit better why other multi-gpu that aren't pci shouldn't
have any issues with making all this fully pci specific.
v3
- polish commit message (Javier)
Fixes: ee7a69aa38d8 ("fbdev: Disable sysfb device registration when removing
conflicting FBs")
Tested-by: Aaron Plattner
Reviewed-by: Javier Martinez Canillas
References: https://bugzilla.kernel.org/show_bug.cgi?id=216303#c28
Signed-off-by: Daniel Vetter
Cc: Aaron Plattner
Cc: Javier Martinez Canillas
Cc: Thomas Zimmermann
Cc: Helge Deller
Cc: Sam Ravnborg
Cc: Alex Deucher
Cc: # v5.19+ (if someone else does the backport)
---
drivers/video/aperture.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/video/aperture.c b/drivers/video/aperture.c
index 8f1437339e49..2394c2d310f8 100644
--- a/drivers/video/aperture.c
+++ b/drivers/video/aperture.c
@@ -321,15 +321,16 @@ int aperture_remove_conflicting_pci_devices(struct
pci_dev *pdev, const char *na
primary = pdev == vga_default_device();
+ if (primary)
+ sysfb_disable();
+
for (bar = 0; bar < PCI_STD_NUM_BARS; ++bar) {
if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
continue;
base = pci_resource_start(pdev, bar);
size = pci_resource_len(pdev, bar);
- ret = aperture_remove_conflicting_devices(base, size, name);
- if (ret)
- return ret;
+ aperture_detach_devices(base, size);
}
if (primary) {
role of crtcs in modesetting interfaces and possible abstraction away from userspace
On 09/08/2014 11:37 PM, Keith Packard wrote: > With atomic mode setting in the kernel, I think you're probably right in > proposing to eliminate explicit CRTC allocation from that. I do think > you'll want to indicate the number of available CRTCs in the display > engine, and the number of CRTCs each monitor consumes. Do you know if > there are some of these monitors that can display lower resolution modes > with only a single CRTC? Or is the hardware so separate that you end up > always using multiple CRTCs to drive them? The one I tried definitely can; indeed it has to because the VBIOS doesn't know how to set up MST and drives the port in DP 1.1 mode. I'm sure someone will build a monitor that only turns on half of the display if you do that, but I'd be kind of surprised if someone made one that just doesn't work if you use a DP 1.1-only GPU. -- Aaron
Stupid NVIDIA 3D vgaarb.c patch
On 09/23/2014 01:29 PM, Benjamin Herrenschmidt wrote: > On Mon, 2014-09-22 at 13:43 -0700, Linus Torvalds wrote: >> Adding proper people and mailing lists.. >> >> The PCI_CLASS_DISPLAY_VGA test goes back to the very beginning by >> BenH, and I have no idea if adding PCI_CLASS_DISPLAY_3D is >> appropriate, but hopefully somebody does. The fact that it makes >> things work certainly argues fairly convincingly for it, but I want >> some backup here. >> >> Dave, BenH? >> >> Christopher(?), can you please also specify which laptop etc. And the >> patch itself seems to have come from somebody else, unless you're >> Lekensteyn. So we'd want to get the provenance of that too. > > Hrm, that sucks. "3D" could mean anything really, we might need an > explicit vendor ID check as well and maybe even device ID ... If my understanding is correct, the board designers explicitly mark them as "3D controller" when they don't have any outputs connected, specifically so the SBIOS won't choose them as the boot VGA device. Depending on the design, some GPUs on these 3D controller boards don't have a display engine at all, while others still have it in the silicon but leave it disabled at runtime. In either case, VGA should not be routed to them and I don't think they should need to participate in VGA arbitration. -- Aaron > Ben. > >> Linus >> >> On Mon, Sep 22, 2014 at 1:28 PM, C Bergstr?m >> wrote: >>> Hi Linus, >>> >>> I don't know who the original author is and I can have one of the distro >>> graphics friends review it, but I need this patch below to get NVIDIA >>> drivers to work (without using any Intel graphics) on my laptop >>> http://pastebin.com/wpmFi38k >>> >>> Sorry - I know this is not the proper way to submit a patch, but it's >>> trivial and I'm not a kernel dev. >>> >>> Thanks >>> >>> ./C
Abstracting buffer sharing mechanism from the drm drivers
On 11/16/2012 01:14 PM, Jerome Glisse wrote: > On Fri, Nov 16, 2012 at 12:05:40PM -0800, Aaron Plattner wrote: >> At the suggestion of a few drm developers, I'm looking at abstracting the >> buffer >> sharing mechanism away from the individual drm drivers and treating it as a >> low-level interface that kernel subsystems use to communicate, rather than as >> something drivers should be accessing directly. This would also mean that >> they >> wouldn't have to each implement their own set of dma_buf_ops, and the logic >> for >> things like detecting that you're importing your own dma_buf would be written >> once, in drm_prime.c and not in every driver's gem_prime_import function. >> >> Of course, it's slightly difficult because each driver's implementation >> seems to >> be subtly different. >> >> * i915 uses its own special locking function, >> i915_mutex_lock_interruptible. >> >> * nouveau and radeon pin the pages when the dma_buf is created, while i915 >> pins >> them at map time. >> >> * the vmap functions are different between i915 and radeon/nouveau, but it >> looks like all they use the dma_buf object for is to find the GEM object. >> >> Does it make sense to try to abstract the dma_buf parts of this? For >> example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets >> i915 >> do its i915_gem_wait_for_error thing, takes the lock, calls a new >> gem_get_pages >> driver hook, does the dma_map_sg call, and handles the unlocking? I'll come >> up >> with a more detailed proposal or patches if this sounds like a good idea. > > I don't think it's a good idea, i understand why people like sharing code, > but we > many example that shows that at one point trying to come with a common > infrastructure just hurt you. There will always be little tweak and little > things > that a specific driver of a specific gpu will want to do at one point. For > instance > you can look at intel mode display code rework, it turn out that the crtc > helper > is getting in there way and i have too similar code in wip for radeon. I think the patch "drm/prime: drop reference on imported dma-buf come from gem" [1] is a perfect example of why I think this code should be abstracted. It applies an identical change to each of five identical chunks of code. It's really easy to mess up merges of changes like this with later copying and pasting, e.g. the introduction of a new driver like tegradrm. > When it comes to dma memory sharing i think the right level of helper are the > one > that allow to retrive array of page, to pin/unpin those page, and possibly > something > that can map those page into a vma at a given offset but wouldn't do any > synchronization. ie nothing that impose a common logic on how things happen > and when > are the point of synchronization. > > Yes it means there is some code duplication but i am always in the side that > trying > to over factorize code hurt more than code duplication. The main issue here is that drm drivers have to call dma_buf functions directly when dma_buf should be a drm_prime implementation detail. -- Aaron [1] http://lists.freedesktop.org/archives/dri-devel/2012-November/030417.html
A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
On 08/31/2012 08:00 PM, Dave Airlie wrote: >> object interface, so that Optimus-based laptops can use our driver to drive >> the discrete GPU and display on the integrated GPU. The good news is that >> I've got a proof of concept working. > > Don't suppose you'll be interested in adding the other method at some point as > well? since saving power is probably important to a lot of people That's milestone 2. I'm focusing on display offload to start because it's easier to implement and lays the groundwork for the kernel pieces. I have to emphasize that I'm just doing a feasibility study right now and I can't promise that we're going to officially support this stuff. >> During a review of the current code, we came up with a few concerns: >> >> 1. The output source is responsible for allocating the shared memory >> >> Right now, the X server calls CreatePixmap on the output source screen and >> then expects the output sink screen to be able to display from whatever >> memory >> the source allocates. Right now, the source has no mechanism for asking the >> sink what its requirements are for the surface. I'm using our own internal >> pitch alignment requirements and that seems to be good enough for the Intel >> device to scan out, but that could be pure luck. > > Well in theory it might be nice but it would have been premature since so far > the only interactions for prime are combination of intel, nvidia and AMD, and > I think everyone has fairly similar pitch alignment requirements, I'd be > interested in adding such an interface but I don't think its some I personally > would be working on. Okay. Hopefully that won't be too painful to add if we ever need it in the future. >> other, or is it sufficient to just define a lowest common denominator format >> and if your hardware can't deal with that format, you just don't get to share >> buffers? > > At the moment I'm happy to just go with linear, minimum pitch alignment 64 or 256, for us. > something as a base standard, but yeah I'm happy for it to work either way, > just don't have enough evidence it's worth it yet. I've not looked at ARM > stuff, so patches welcome if people consider they need to use this stuff for > SoC devices. We can always hack it to whatever is necessary if we see that the sink side driver is Tegra, but I was hoping for something more general. >> 2. There's no fallback mechanism if sharing can't be negotiated >> >> If RandR fails to share a pixmap with the output sink screen, the whole >> modeset fails. This means you'll end up not seeing anything on the screen >> and >> you'll probably think your computer locked up. Should there be some sort of >> software copy fallback to ensure that something at least shows up on the >> display? > > Uggh, it would be fairly slow and unuseable, I'd rather they saw nothing, but > again open to suggestions on how to make this work, since it might fail for > other reasons and in that case there is still nothing a sw copy can do. What > happens if the slave intel device just fails to allocate a pixmap, but yeah > I'm willing to think about it a bit more when we have some reference > implementations. Just rolling back the modeset operation to whatever was working before would be a good start. It's worse than that on my current laptop, though, since our driver sees a phantom CRT output and we happily start driving pixels to it that end up going nowhere. I'll need to think about what the right behavior is there since I don't know if we want to rely on an X client to make that configuration work. >> 3. How should the memory be allocated? >> >> In the prototype I threw together, I'm allocating the shared memory using >> shm_open and then exporting that as a dma-buf file descriptor using an ioctl >> I >> added to the kernel, and then importing that memory back into our driver >> through dma_buf_attach & dma_buf_map_attachment. Does it make sense for >> user-space programs to be able to export shmfs files like that? Should that >> interface go in DRM / GEM / PRIME instead? Something else? I'm pretty >> unfamiliar with this kernel code so any suggestions would be appreciated. > > Your kernel driver should in theory be doing it all, if you allocate shared > pixmaps in GTT accessible memory, then you need an ioctl to tell your kernel > driver to export the dma buf to the fd handle. (assuming we get rid of the > _GPL, which people have mentioned they are open to doing). We have handle->fd > and fd->handle interfaces on DRM, you'd need something similiar on the nvidia > kernel driver interface. Okay, I can do that. We already have a mechanism for importing buffers allocated elsewhere so reusing that for shmfs and/or dma-buf seemed like a natural extension. I don't think adding a separate ioctl for exporting our own allocations will add too much extra code. > Yes for 4 some sort of fencing is being worked on by Maarten for other stuff > but would be a pre-req for doing this, and also some d
Abstracting buffer sharing mechanism from the drm drivers
At the suggestion of a few drm developers, I'm looking at abstracting the buffer sharing mechanism away from the individual drm drivers and treating it as a low-level interface that kernel subsystems use to communicate, rather than as something drivers should be accessing directly. This would also mean that they wouldn't have to each implement their own set of dma_buf_ops, and the logic for things like detecting that you're importing your own dma_buf would be written once, in drm_prime.c and not in every driver's gem_prime_import function. Of course, it's slightly difficult because each driver's implementation seems to be subtly different. * i915 uses its own special locking function, i915_mutex_lock_interruptible. * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins them at map time. * the vmap functions are different between i915 and radeon/nouveau, but it looks like all they use the dma_buf object for is to find the GEM object. Does it make sense to try to abstract the dma_buf parts of this? For example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915 do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages driver hook, does the dma_map_sg call, and handles the unlocking? I'll come up with a more detailed proposal or patches if this sounds like a good idea. -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Abstracting buffer sharing mechanism from the drm drivers
On 11/16/2012 01:14 PM, Jerome Glisse wrote: On Fri, Nov 16, 2012 at 12:05:40PM -0800, Aaron Plattner wrote: At the suggestion of a few drm developers, I'm looking at abstracting the buffer sharing mechanism away from the individual drm drivers and treating it as a low-level interface that kernel subsystems use to communicate, rather than as something drivers should be accessing directly. This would also mean that they wouldn't have to each implement their own set of dma_buf_ops, and the logic for things like detecting that you're importing your own dma_buf would be written once, in drm_prime.c and not in every driver's gem_prime_import function. Of course, it's slightly difficult because each driver's implementation seems to be subtly different. * i915 uses its own special locking function, i915_mutex_lock_interruptible. * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins them at map time. * the vmap functions are different between i915 and radeon/nouveau, but it looks like all they use the dma_buf object for is to find the GEM object. Does it make sense to try to abstract the dma_buf parts of this? For example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915 do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages driver hook, does the dma_map_sg call, and handles the unlocking? I'll come up with a more detailed proposal or patches if this sounds like a good idea. I don't think it's a good idea, i understand why people like sharing code, but we many example that shows that at one point trying to come with a common infrastructure just hurt you. There will always be little tweak and little things that a specific driver of a specific gpu will want to do at one point. For instance you can look at intel mode display code rework, it turn out that the crtc helper is getting in there way and i have too similar code in wip for radeon. I think the patch "drm/prime: drop reference on imported dma-buf come from gem" [1] is a perfect example of why I think this code should be abstracted. It applies an identical change to each of five identical chunks of code. It's really easy to mess up merges of changes like this with later copying and pasting, e.g. the introduction of a new driver like tegradrm. When it comes to dma memory sharing i think the right level of helper are the one that allow to retrive array of page, to pin/unpin those page, and possibly something that can map those page into a vma at a given offset but wouldn't do any synchronization. ie nothing that impose a common logic on how things happen and when are the point of synchronization. Yes it means there is some code duplication but i am always in the side that trying to over factorize code hurt more than code duplication. The main issue here is that drm drivers have to call dma_buf functions directly when dma_buf should be a drm_prime implementation detail. -- Aaron [1] http://lists.freedesktop.org/archives/dri-devel/2012-November/030417.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/4] Prime helpers
This series adds helper functions that abstract the core parts of .gem_prime_import and .gem_prime_export so that drivers don't have to worry about the low-level details. These helpers are optional. A driver can use them by plugging in drm_gem_prime_import and drm_gem_prime_export into the drm_driver structure, or it can bypass them by plugging in its own functions. The first patch adds these helpers, and the later patches switch three drivers over to using them. Jerome, I tried to address your concerns about code sharing by making the helpers available without making them required. I haven't yet compile-tested the Exynos change since I don't have a toolchain set up for that, but I'll try to do that today. Aaron Plattner (4): drm: add prime helpers drm/nouveau: use prime helpers drm/radeon: use prime helpers drm/exynos: use prime helpers drivers/gpu/drm/drm_prime.c| 190 - drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 151 ++- drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 +- drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 + drivers/gpu/drm/nouveau/nouveau_bo.h | 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 11 +- drivers/gpu/drm/nouveau/nouveau_prime.c| 172 -- drivers/gpu/drm/radeon/radeon_drv.c| 17 +-- drivers/gpu/drm/radeon/radeon_prime.c | 169 - include/drm/drmP.h | 15 +++ 12 files changed, 288 insertions(+), 464 deletions(-) -- 1.8.0.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/4] drm: add prime helpers
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_pages: convert a drm_gem_object to an sg_table for export
gem_prime_import_sg: 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.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/drm_prime.c | 190 +++-
include/drm/drmP.h | 15
2 files changed, 204 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..566c2ac 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -53,7 +53,8 @@
* Self-importing: if userspace is using PRIME as a replacement for flink
* then it will get a fd->handle request for a GEM object that it created.
* Drivers should detect this situation and return back the gem object
- * from the dma-buf private.
+ * from the dma-buf private. Prime will do this automatically for drivers that
+ * use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,146 @@ struct drm_prime_member {
uint32_t handle;
};
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *st;
+
+ mutex_lock_interruptible(&obj->dev->struct_mutex);
+
+ st = obj->dev->driver->gem_prime_get_pages(obj);
+
+ if (!IS_ERR_OR_NULL(st))
+ dma_map_sg(attach->dev, st->sgl, st->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
+ return st;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sg, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sg->sgl, sg->nents, dir);
+ sg_free_table(sg);
+ kfree(sg);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+ void *virtual = ERR_PTR(-EINVAL);
+
+ if (!dev->driver->gem_prime_vmap)
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&dev->struct_mutex);
+ if (obj->vmapping_count) {
+ obj->vmapping_count++;
+ virtual = obj->vmapping_ptr;
+ goto out_unlock;
+ }
+
+ virtual = dev->driver->gem_prime_vmap(obj);
+ if (IS_ERR(virtual)) {
+ mutex_unlock(&dev->struct_mutex);
+ return virtual;
+ }
+ obj->vmapping_count = 1;
+ obj->vmapping_ptr = virtual;
+out_unlock:
+ mutex_unlock(&dev->struct_mutex);
+ return obj->vmapping_ptr;
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ mutex_lock(&dev->struct_mutex);
+ obj->vmapping_count--;
+ if (obj->vmapping_count == 0) {
+ dev->driver->gem_prime_vunmap(obj);
+ obj->vmapping_ptr = NULL;
+ }
+ mutex_unlock(&dev->struct_mutex);
+}
+
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
+
+static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num, void *addr)
+{
+
+}
+static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
+
+static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
+ unsigned long page_num, void *addr)
+{
+
+}
+
+static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
+ struct vm_area_struct *vma)
+{
+ return -EINVAL;
+}
+
+static int drm_gem_begin_cpu_access(struct dma_buf *dma_buf,
+ size_t start, size_t length, enum dma_data_direction direction)
+{
+ return -EINVAL;
+}
+
+static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
+ .map_dma_buf = drm_gem_map_
[PATCH 3/4] drm/radeon: use prime helpers
Simplify the Radeon prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/radeon/radeon_drv.c | 17 ++--
drivers/gpu/drm/radeon/radeon_prime.c | 169 +-
2 files changed, 31 insertions(+), 155 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon/radeon_drv.c
index 8c1a83c..a724c3c 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -113,11 +113,11 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
int radeon_mode_dumb_destroy(struct drm_file *file_priv,
struct drm_device *dev,
uint32_t handle);
-struct dma_buf *radeon_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj,
- int flags);
-struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+struct sg_table *radeon_gem_prime_get_pages(struct drm_gem_object *obj);
+struct drm_gem_object *radeon_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg);
+int radeon_gem_prime_pin(struct drm_gem_object *obj);
#if defined(CONFIG_DEBUG_FS)
int radeon_debugfs_init(struct drm_minor *minor);
@@ -392,8 +392,11 @@ static struct drm_driver kms_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = radeon_gem_prime_export,
- .gem_prime_import = radeon_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = radeon_gem_prime_pin,
+ .gem_prime_get_pages = radeon_gem_prime_get_pages,
+ .gem_prime_import_sg = radeon_gem_prime_import_sg,
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c
b/drivers/gpu/drm/radeon/radeon_prime.c
index e095218..3a047c7 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -28,198 +28,71 @@
#include "radeon.h"
#include
-#include
-
-static struct sg_table *radeon_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *radeon_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct radeon_bo *bo = attachment->dmabuf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int npages = bo->tbo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void radeon_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
-struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
-
- if (bo->gem_base.export_dma_buf == dma_buf) {
- DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base);
- bo->gem_base.export_dma_buf = NULL;
- drm_gem_object_unreference_unlocked(&bo->gem_base);
- }
-}
-
-static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_kunmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num, void *addr)
-{
-
-}
-static void *radeon_gem_kmap(struct dma_buf *dma_buf, unsigned long page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_kunmap(struct dma_buf *dma_buf, unsigned long page_num,
void *addr)
-{
+ return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
}
-static int radeon_gem_prime_mmap(struct dma_buf *dma_buf, struct
vm_area_struct *vma)
+void *radeon_gem_prime_vmap(struct drm_gem_object *obj)
{
- return -EINVAL;
-}
-
-static void *radeon_gem_prime_vmap(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int ret;
- mutex_lock(&dev->struct_mutex);
- if (bo->vmapping_count) {
- bo->vmapping_count++;
- goto out_unlock;
-
[PATCH 2/4] drm/nouveau: use prime helpers
Simplify the Nouveau prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/nouveau/nouveau_bo.h| 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 -
drivers/gpu/drm/nouveau/nouveau_gem.h | 11 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 172
5 files changed, 35 insertions(+), 160 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h
b/drivers/gpu/drm/nouveau/nouveau_bo.h
index dec51b1..1793631 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -31,7 +31,6 @@ struct nouveau_bo {
int pin_refcnt;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
static inline struct nouveau_bo *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index a7529b3..7bbe1c4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -644,8 +644,13 @@ driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = nouveau_gem_prime_export,
- .gem_prime_import = nouveau_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = nouveau_gem_prime_pin,
+ .gem_prime_get_pages = nouveau_gem_prime_get_pages,
+ .gem_prime_import_sg = nouveau_gem_prime_import_sg,
+ .gem_prime_vmap = nouveau_gem_prime_vmap,
+ .gem_prime_vunmap = nouveau_gem_prime_vunmap,
.gem_init_object = nouveau_gem_object_new,
.gem_free_object = nouveau_gem_object_del,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 5e2f521..9c01f7c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
*
*/
-#include
-
#include
#include "nouveau_drm.h"
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h
b/drivers/gpu/drm/nouveau/nouveau_gem.h
index 5c10492..195e23c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -35,9 +35,12 @@ extern int nouveau_gem_ioctl_cpu_fini(struct drm_device *,
void *,
extern int nouveau_gem_ioctl_info(struct drm_device *, void *,
struct drm_file *);
-extern struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj, int flags);
-extern struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+extern int nouveau_gem_prime_pin(struct drm_gem_object *);
+extern struct sg_table *nouveau_gem_prime_get_pages(struct drm_gem_object *);
+extern struct drm_gem_object *nouveau_gem_prime_import_sg(struct drm_device *,
+ size_t size,
+ struct sg_table *);
+extern void *nouveau_gem_prime_vmap(struct drm_gem_object *);
+extern void nouveau_gem_prime_vunmap(struct drm_gem_object *);
#endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c
b/drivers/gpu/drm/nouveau/nouveau_prime.c
index 3543fec..c3683f0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -22,126 +22,42 @@
* Authors: Dave Airlie
*/
-#include
-
#include
#include "nouveau_drm.h"
#include "nouveau_gem.h"
-static struct sg_table *nouveau_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *nouveau_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct nouveau_bo *nvbo = attachment->dmabuf->priv;
- struct drm_device *dev = nvbo->gem->dev;
+ struct nouveau_bo *nvbo = nouveau_gem_object(obj);
int npages = nvbo->bo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void nouveau_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
- struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct nouveau_bo *nvbo = dma_buf->priv;
-
- if (nvbo->gem->export_dma_buf == dma_buf) {
- nvb
[PATCH 4/4] drm/exynos: use prime helpers
Simplify the Exynos prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
I still need to find a system to test this.
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 151 ++---
drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 +
3 files changed, 18 insertions(+), 148 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 539da9f..7f6610d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -28,8 +28,6 @@
#include "exynos_drm_drv.h"
#include "exynos_drm_gem.h"
-#include
-
static struct sg_table *exynos_get_sgt(struct drm_device *drm_dev,
struct exynos_drm_gem_buf *buf)
{
@@ -56,15 +54,12 @@ out:
return NULL;
}
-static struct sg_table *
- exynos_gem_map_dma_buf(struct dma_buf_attachment *attach,
- enum dma_data_direction dir)
+struct sg_table *exynos_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv;
+ struct exynos_drm_gem_obj *gem_obj = to_exynos_gem_obj(obj);
struct drm_device *dev = gem_obj->base.dev;
struct exynos_drm_gem_buf *buf;
struct sg_table *sgt = NULL;
- int nents;
DRM_DEBUG_PRIME("%s\n", __FILE__);
@@ -74,119 +69,20 @@ static struct sg_table *
return sgt;
}
- mutex_lock(&dev->struct_mutex);
-
sgt = exynos_get_sgt(dev, buf);
if (!sgt)
- goto err_unlock;
-
- nents = dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- if (!nents) {
- DRM_ERROR("failed to map sgl with iommu.\n");
- sgt = NULL;
- goto err_unlock;
- }
+ goto err;
DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);
-err_unlock:
- mutex_unlock(&dev->struct_mutex);
+err:
return sgt;
}
-static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
- struct sg_table *sgt,
- enum dma_data_direction dir)
-{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
-
- sg_free_table(sgt);
- kfree(sgt);
- sgt = NULL;
-}
-
-static void exynos_dmabuf_release(struct dma_buf *dmabuf)
+struct drm_gem_object *exynos_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg)
{
- struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv;
-
- DRM_DEBUG_PRIME("%s\n", __FILE__);
-
- /*
-* exynos_dmabuf_release() call means that file object's
-* f_count is 0 and it calls drm_gem_object_handle_unreference()
-* to drop the references that these values had been increased
-* at drm_prime_handle_to_fd()
-*/
- if (exynos_gem_obj->base.export_dma_buf == dmabuf) {
- exynos_gem_obj->base.export_dma_buf = NULL;
-
- /*
-* drop this gem object refcount to release allocated buffer
-* and resources.
-*/
- drm_gem_object_unreference_unlocked(&exynos_gem_obj->base);
- }
-}
-
-static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num,
- void *addr)
-{
- /* TODO */
-}
-
-static void *exynos_gem_dmabuf_kmap(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
- unsigned long page_num, void *addr)
-{
- /* TODO */
-}
-
-static int exynos_gem_dmabuf_mmap(struct dma_buf *dma_buf,
- struct vm_area_struct *vma)
-{
- return -ENOTTY;
-}
-
-static struct dma_buf_ops exynos_dmabuf_ops = {
- .map_dma_buf= exynos_gem_map_dma_buf,
- .unmap_dma_buf = exynos_gem_unmap_dma_buf,
- .kmap = exynos_gem_dmabuf_kmap,
- .kmap_atomic= exynos_gem_dmabuf_kmap_atomic,
- .kunmap = exynos_gem_dmabuf_kunmap,
- .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic,
- .mm
[PATCH v2] drm/exynos: use prime helpers
Simplify the Exynos prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
v2: fix dumb mistakes now that I have a toolchain that can compile-test this
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 156 ++---
drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 +
3 files changed, 20 insertions(+), 151 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 539da9f..71b40f4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -28,8 +28,6 @@
#include "exynos_drm_drv.h"
#include "exynos_drm_gem.h"
-#include
-
static struct sg_table *exynos_get_sgt(struct drm_device *drm_dev,
struct exynos_drm_gem_buf *buf)
{
@@ -56,15 +54,12 @@ out:
return NULL;
}
-static struct sg_table *
- exynos_gem_map_dma_buf(struct dma_buf_attachment *attach,
- enum dma_data_direction dir)
+struct sg_table *exynos_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv;
+ struct exynos_drm_gem_obj *gem_obj = to_exynos_gem_obj(obj);
struct drm_device *dev = gem_obj->base.dev;
struct exynos_drm_gem_buf *buf;
struct sg_table *sgt = NULL;
- int nents;
DRM_DEBUG_PRIME("%s\n", __FILE__);
@@ -74,120 +69,20 @@ static struct sg_table *
return sgt;
}
- mutex_lock(&dev->struct_mutex);
-
sgt = exynos_get_sgt(dev, buf);
if (!sgt)
- goto err_unlock;
-
- nents = dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- if (!nents) {
- DRM_ERROR("failed to map sgl with iommu.\n");
- sgt = NULL;
- goto err_unlock;
- }
+ goto err;
DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);
-err_unlock:
- mutex_unlock(&dev->struct_mutex);
+err:
return sgt;
}
-static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
- struct sg_table *sgt,
- enum dma_data_direction dir)
-{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
-
- sg_free_table(sgt);
- kfree(sgt);
- sgt = NULL;
-}
-
-static void exynos_dmabuf_release(struct dma_buf *dmabuf)
+struct drm_gem_object *exynos_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sgt)
{
- struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv;
-
- DRM_DEBUG_PRIME("%s\n", __FILE__);
-
- /*
-* exynos_dmabuf_release() call means that file object's
-* f_count is 0 and it calls drm_gem_object_handle_unreference()
-* to drop the references that these values had been increased
-* at drm_prime_handle_to_fd()
-*/
- if (exynos_gem_obj->base.export_dma_buf == dmabuf) {
- exynos_gem_obj->base.export_dma_buf = NULL;
-
- /*
-* drop this gem object refcount to release allocated buffer
-* and resources.
-*/
- drm_gem_object_unreference_unlocked(&exynos_gem_obj->base);
- }
-}
-
-static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num,
- void *addr)
-{
- /* TODO */
-}
-
-static void *exynos_gem_dmabuf_kmap(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
- unsigned long page_num, void *addr)
-{
- /* TODO */
-}
-
-static int exynos_gem_dmabuf_mmap(struct dma_buf *dma_buf,
- struct vm_area_struct *vma)
-{
- return -ENOTTY;
-}
-
-static struct dma_buf_ops exynos_dmabuf_ops = {
- .map_dma_buf= exynos_gem_map_dma_buf,
- .unmap_dma_buf = exynos_gem_unmap_dma_buf,
- .kmap = exynos_gem_dmabuf_kmap,
- .kmap_atomic= exynos_gem_dmabuf_kmap_atomic,
- .kunmap = exynos_gem_dmabuf_kunmap,
- .kunmap_atomic = exynos_gem_dmabuf_kun
Re: [PATCH v2] drm/exynos: use prime helpers
On 12/06/2012 10:36 PM, Inki Dae wrote: Hi, CCing media guys. I agree with you but we should consider one issue released to v4l2. As you may know, V4L2-based driver uses vb2 as buffer manager and the vb2 includes dmabuf feature>(import and export) And v4l2 uses streaming concept>(qbuf and dqbuf) With dmabuf and iommu, generally qbuf imports a fd into its own buffer and maps it with its own iommu table calling dma_buf_map_attachment(). And dqbuf calls dma_buf_unmap_attachment() to unmap that buffer from its own iommu table. But now vb2's unmap_dma_buf callback is nothing to do. I think that the reason is the below issue, If qbuf maps buffer with iomm table and dqbuf unmaps it from iommu table then it has performance deterioration because qbuf and dqbuf are called repeatedly. And this means map/unmap are repeated also. So I think media guys moved dma_unmap_sg call from its own unmap_dma_buf callback to detach callback instead. For this, you can refer to vb2_dc_dmabuf_ops_unmap and vb2_dc_dmabuf_ops_detach function. So I added the below patch to avoid that performance deterioration and am testing it now.(this patch is derived from videobuf2-dma-contig.c) http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git;a=commit;h=576b1c3de8b90cf1570b8418b60afd1edaae4e30 Thus, I'm not sure that your common set could cover all the cases including other frameworks. Please give me any opinions. It seems like this adjustment would make perfect sense to add to the helper layer I suggested. E.g., instead of having an exynos_attach structure that caches the sgt, there'd be a struct drm_gem_prime_attach that would do the same thing, and save the sgt it gets from driver->gem_prime_get_sg. Then it would benefit nouveau and radeon, too. Alternatively, patch #4 could be dropped and Exynos can continue to reimplement all of this core functionality, since the helpers are optional, but I don't see anything about this change that should make it Exynos-specific, unless I'm missing something. -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm: add prime helpers
On 12/06/2012 01:46 PM, Daniel Vetter wrote: On Thu, Dec 06, 2012 at 10:07:48AM -0800, Aaron Plattner wrote: 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_pages: convert a drm_gem_object to an sg_table for export gem_prime_import_sg: 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. Signed-off-by: Aaron Plattner A few comments: - Can you please add kerneldoc entries for these new helpers? Laurent Pinchart started a nice drm kerneldoc as an overview. And since then we've at least integrated the kerneldoc api reference at a nice place there and brushed it up a bit when touching drm core or helpers in a bigger patch series. See e.g. my recent dp helper series for what I'm looking for. I think we should have kerneldoc at least for the exported functions. Good call, I'll look into that. - Just an idea for all the essential noop cpu helpers: Maybe just call them dma_buf*noop and shovel them as convenience helpers into the dma-buf.c code in the core, for drivers that don't want/can't/won't bother to implement the cpu access support. Related: Exporting the functions in general so that drivers could pick&choose Seems like a good idea as a follow-on change. Do you think it would make more sense to have noop helpers, or allow them to be NULL in dma-buf.c? - s/gem_prime_get_pages/gem_prime_get_sg/ drm/i915 now uses sg_tables everywhere to manage backing storage, and we're starting to use the stolen memory, too. Stolen memory is not struct page backed, so better make this clear in the function calls. I know that the current prime/dma_buf code from Dave doesn't really work too well (*cough*) if the backing storage isn't struct page backed, at least on the ttm import side. This also links in with my 2nd comment above - dma_buf requires that exporter map the sg into the importers device space to support fake subdevices which share the exporters iommu/gart, hence such incetious exporter might want to overwrite some of the functions. Will do in a v2 set. - To make it a first-class helper and remove any and all midlayer stints from this (I truly loath midlayers ...) maybe shovel this into a drm_prime_helper.c file. Also, adding functions to the core vtable for pure helpers is usually not too neat, since it makes it way too damn easy to mix things up and transform the helper into a midlayer by accident. E.g. the crtc helper functions all just use a generic void * pointer for their vtables, other helpers either subclass an existing struct or use their completely independent structs (which the driver can both embed into it's own object struct and then do the right upclassing in the callbacks). tbh haven't looked to closely at the series to asses what would make sense, but we have way too much gunk in the drm vtable already ... I'm not sure I fully understand what you mean by this. Are you suggesting creating a separate struct for these functions and having a void* pointer to that in struct drm_driver? Dunno whether this aligns with what you want to achieve here. But on a quick hunch this code feels rather unflexible and just refactors the identical copypasta code back into one copy. Anyway, just figured I'll drop my 2 cents here, feel free to ignore (maybe safe for the last one). I think uncopypasta-ing the current code is beneficial on its own. Do you think doing this now would make it harder to do the further refactoring you suggest later? -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm: add prime helpers
On 12/07/2012 10:48 AM, Daniel Vetter wrote:
On Fri, Dec 07, 2012 at 09:58:38AM -0800, Aaron Plattner wrote:
On 12/06/2012 01:46 PM, Daniel Vetter wrote:
- To make it a first-class helper and remove any and all midlayer stints
from this (I truly loath midlayers ...) maybe shovel this into a
drm_prime_helper.c file. Also, adding functions to the core vtable for
pure helpers is usually not too neat, since it makes it way too damn
easy to mix things up and transform the helper into a midlayer by
accident. E.g. the crtc helper functions all just use a generic void *
pointer for their vtables, other helpers either subclass an existing
struct or use their completely independent structs (which the driver can
both embed into it's own object struct and then do the right upclassing
in the callbacks). tbh haven't looked to closely at the series to asses
what would make sense, but we have way too much gunk in the drm vtable
already ...
I'm not sure I fully understand what you mean by this. Are you
suggesting creating a separate struct for these functions and having a
void* pointer to that in struct drm_driver?
Create a new struct like
struct drm_prime_helper_funcs {
int (*gem_prime_pin)(struct drm_gem_object *obj);
struct sg_table *(*gem_prime_get_pages)(struct drm_gem_object *obj);
struct drm_gem_object *(*gem_prime_import_sg)(struct drm_device *dev,
size_t size, struct sg_table *sg);
void *(*gem_prime_vmap)(struct drm_gem_object *obj);
void (*gem_prime_vunmap)(struct drm_gem_object *obj);
}
struct drm_prime_helper_obj {
/* vmap information */
int vmapping_count;
void *vmapping_ptr;
/* any other data the helpers need ... */
struct drm_prime_helper_funcs *funcs;
}
Ah, I see what you mean. This would also need either a pointer to the
drv directly so the helpers can lock drv->struct_mutex, or a helper
function to convert from a drm_prime_helper_obj* to a gem_buffer_object*
in a driver-specific way since the helper doesn't know the layout of the
driver's bo structure.
So it would be something like
struct drm_prime_helper_obj *helper_obj = dma_buf->priv;
struct drm_prime_helper_funcs *funcs = helper_obj->funcs;
struct drm_gem_object *obj = funcs->get_gem_obj(helper_obj);
mutex_lock(&obj->dev->struct_mutex);
funcs->whatever(obj);
mutex_unlock&obj->dev->struct_mutex);
and then for example, radeon_drm_prime_get_gem_obj would be
struct radeon_bo *bo = container_of(helper_obj, struct radeon_bo,
prime_helper);
return &bo->gem_base;
?
That seems a little over-engineered to me, but I can code it up.
--
Aaron
Drivers then fill out a func vtable and embedded the helper obj into their
own gpu bo struct, i.e.
struct radeon_bo {
struct ttm_bo ttm_bo;
struct gem_buffer_object gem_bo;
struct drm_prime_helper_obj prime_bo;
/* other stuff */
}
In the prime helper callbacks in the driver then upcast the prime_bo to
the radeon_bo instead of the gem_bo to the radeon bo.
Upsides: Guaranteed to not interfere with drivers not using it, with an
imo higher chance that the helper is cleanly separate from core stuff (no
guarantee ofc) and so easier to refactor in the future. And drm is full
with legacy stuff used by very few drivers, but highly intermingled with
drm core used by other drivers (my little holy war here is to slowly
refactor those things out and shovel them into drivers), hence why I
prefer to have an as clean separation of optional stuff as possible ...
Also, this way allows different drivers to use completely different helper
libraries for the same task, without those two helper libraries ever
interfering.
Downside: Adds a pointer to each bo struct for drivers using the helpers.
Given how big those tend to be, not too onerous. Given And maybe a bit of
confusion in driver callbacks, but since the linux container_of is
typechecked by the compiler, not too onerous either.
Dunno whether this aligns with what you want to achieve here. But on a
quick hunch this code feels rather unflexible and just refactors the
identical copypasta code back into one copy. Anyway, just figured I'll
drop my 2 cents here, feel free to ignore (maybe safe for the last one).
I think uncopypasta-ing the current code is beneficial on its own.
Do you think doing this now would make it harder to do the further
refactoring you suggest later?
If we later on go with something like the above, it'll require going over
all drivers again, doing similar mechanic changes. If no one yet managed
to intermingle the helpers with the core in the meantime, that is ;-)
Cheers, Daniel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm: add prime helpers
On 12/07/2012 01:38 PM, Daniel Vetter wrote: On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner wrote: Ah, I see what you mean. This would also need either a pointer to the drv directly so the helpers can lock drv->struct_mutex, or a helper function to convert from a drm_prime_helper_obj* to a gem_buffer_object* in a driver-specific way since the helper doesn't know the layout of the driver's bo structure. Ah, locking, and it involves dev->struct_mutex. Another pet peeve of mine ;-) Tbh I haven't looked at locking issues when stitching together my quick proposal. The problem with the dev->struct_mutex lock is that it's everywhere and hence it's way to easy to create deadlocks with it. Especially with prime, where a simple rule like "take this lock first, always" are suddenly really more complicated since gem drivers can assume both the importer and exporter role ... I haven't done a full locking review, but I expect current prime to deadlock (through driver calls) when userspace starts doing funny things. So imo it's best to move dev->struct_mutex as far away as possible from helper functions and think really hard whether we really need it. I've noticed three functions: drm_gem_map_dma_buf: We can remove the locking here imo, if drivers need dev->struct_mutex for internal consistency, they can take it in their callbacks. The dma_buf_attachment is very much like a fancy sg list + backing storage pointer. If multiple callers concurrently interact with the same attachment, havoc might ensue. Importers should add their own locking if concurrent access is possible (e.g. similar to how filling in the same sg table concurrently and then calling dma_map_sg concurrently would lead to chaos). Otoh creating/destroying attachments with attach/detach is protected by the dma_buf->lock. I've checked dma-buf docs, and that this is not specced in the docs, but was very much the intention. So I think we can solve this with a simple doc update ;-) drm_gem_dmabuf_v(un)map: Beside that drivers might need to lock for internal consistency the lock only protects the vmapping_counter and pointer and avoids that we race concurrent vmap/vunmap calls to the driver. Now vmap/vunmap is very much like an attachment, just that we don't have an explicit struct for each client since there's only one cpu address space. So pretty much every driver is forced to reinvent vmap refcounting, which feels like an interface bug. What about moving this code to the dma-buf.c interface shim and protecting it with dma_buf->lock? Atm we only take that lock for attach/detach, and drivers using vmap place the vmap call at the same spot hw drivers would place the attach call, so this shouldn't introduce any nefarious locking issues. So I think this would be the right way to move forward. And with that we've weaned the prime helpers off their dependency of dev->struct_mutex, which should make them a notch more flexible and so useful (since fighting locking inversions is a royal pain best avoided). Comments? This all sounds good, but it's getting well outside the realm of what I'm comfortable doing in the short term as a kernel noob. Would you object to going with a version of these changes that leaves the new hooks where they are now and then applying these suggestions later? I don't think the risk of the helpers turning into a required mid-layer is very high given that i915 won't use them and it's one of the most-tested drivers. -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [RFC] dma-buf: implement vmap refcounting in the interface logic
On 12/17/2012 03:58 PM, Daniel Vetter wrote:
All drivers which implement this need to have some sort of refcount to
allow concurrent vmap usage. Hence implement this in the dma-buf core.
To protect against concurrent calls we need a lock, which potentially
causes new funny locking inversions. But this shouldn't be a problem
for exporters with statically allocated backing storage, and more
dynamic drivers have decent issues already anyway.
Inspired by some refactoring patches from Aaron Plattner, who
implemented the same idea, but only for drm/prime drivers.
v2: Check in dma_buf_release that no dangling vmaps are left.
Suggested by Aaron Plattner. We might want to do similar checks for
attachments, but that's for another patch. Also fix up ERR_PTR return
for vmap.
v3: Check whether the passed-in vmap address matches with the cached
one for vunmap. Eventually we might want to remove that parameter -
compared to the kmap functions there's no need for the vaddr for
unmapping. Suggested by Chris Wilson.
Cc: Aaron Plattner
Signed-off-by: Daniel Vetter
---
Compile-tested only - Aaron has been bugging me too a bit too often
about this on irc.
Cheers, Daniel
---
Documentation/dma-buf-sharing.txt | 6 +-
drivers/base/dma-buf.c| 43 ++-
include/linux/dma-buf.h | 4 +++-
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/Documentation/dma-buf-sharing.txt
b/Documentation/dma-buf-sharing.txt
index 0188903..4966b1b 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -302,7 +302,11 @@ Access to a dma_buf from the kernel context involves three
steps:
void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
The vmap call can fail if there is no vmap support in the exporter, or if
it
- runs out of vmalloc space. Fallback to kmap should be implemented.
+ runs out of vmalloc space. Fallback to kmap should be implemented. Note that
+ the dma-buf layer keeps a reference count for all vmap access and calls down
+ into the exporter's vmap function only when no vmapping exists, and only
+ unmaps it once. Protection against concurrent vmap/vunmap calls is provided
+ by taking the dma_buf->lock mutex.
3. Finish access
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index a3f79c4..67d3cd5 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -39,6 +39,8 @@ static int dma_buf_release(struct inode *inode, struct file
*file)
dmabuf = file->private_data;
+ BUG_ON(dmabuf->vmapping_counter);
+
dmabuf->ops->release(dmabuf);
kfree(dmabuf);
return 0;
@@ -482,12 +484,34 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
*/
void *dma_buf_vmap(struct dma_buf *dmabuf)
{
+ void *ptr;
+
if (WARN_ON(!dmabuf))
return NULL;
- if (dmabuf->ops->vmap)
- return dmabuf->ops->vmap(dmabuf);
- return NULL;
+ if (!dmabuf->ops->vmap)
+ return NULL;
+
+ mutex_lock(&dmabuf->lock);
+ if (dmabuf->vmapping_counter) {
+ dmabuf->vmapping_counter++;
+ BUG_ON(!dmabuf->vmap_ptr);
+ ptr = dmabuf->vmap_ptr;
+ goto out_unlock;
+ }
+
+ BUG_ON(dmabuf->vmap_ptr);
+
+ ptr = dmabuf->ops->vmap(dmabuf);
+ if (IS_ERR_OR_NULL(ptr))
+ goto out_unlock;
+
+ dmabuf->vmap_ptr = ptr;
+ dmabuf->vmapping_counter = 1;
+
+out_unlock:
+ mutex_unlock(&dmabuf->lock);
+ return ptr;
}
EXPORT_SYMBOL_GPL(dma_buf_vmap);
@@ -501,7 +525,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
if (WARN_ON(!dmabuf))
return;
- if (dmabuf->ops->vunmap)
- dmabuf->ops->vunmap(dmabuf, vaddr);
+ BUG_ON(!dmabuf->vmap_ptr);
+ BUG_ON(dmabuf->vmapping_counter > 0);
This should be BUG_ON(vmapping_counter == 0);
Otherwise, this seems to work fine.
--
Aaron
+ BUG_ON(dmabuf->vmap_ptr != vaddr);
+
+ mutex_lock(&dmabuf->lock);
+ if (--dmabuf->vmapping_counter == 0) {
+ if (dmabuf->ops->vunmap)
+ dmabuf->ops->vunmap(dmabuf, vaddr);
+ dmabuf->vmap_ptr = NULL;
+ }
+ mutex_unlock(&dmabuf->lock);
}
EXPORT_SYMBOL_GPL(dma_buf_vunmap);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index bd2e52c..e3bf2f6 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -119,8 +119,10 @@ struct dma_buf {
struct file *file;
struct list_head attachments;
const struct dma_buf_ops *ops;
- /* mutex to serialize list manipulation and attach/detach */
+ /* mutex to serialize list manipulation, attach/detach and vmap/unmap */
struct mutex lock;
+
Re: [PATCH] [RFC] dma-buf: implement vmap refcounting in the interface logic
On 12/20/2012 05:14 AM, Daniel Vetter wrote:
All drivers which implement this need to have some sort of refcount to
allow concurrent vmap usage. Hence implement this in the dma-buf core.
To protect against concurrent calls we need a lock, which potentially
causes new funny locking inversions. But this shouldn't be a problem
for exporters with statically allocated backing storage, and more
dynamic drivers have decent issues already anyway.
Inspired by some refactoring patches from Aaron Plattner, who
implemented the same idea, but only for drm/prime drivers.
v2: Check in dma_buf_release that no dangling vmaps are left.
Suggested by Aaron Plattner. We might want to do similar checks for
attachments, but that's for another patch. Also fix up ERR_PTR return
for vmap.
v3: Check whether the passed-in vmap address matches with the cached
one for vunmap. Eventually we might want to remove that parameter -
compared to the kmap functions there's no need for the vaddr for
unmapping. Suggested by Chris Wilson.
v4: Fix a brown-paper-bag bug spotted by Aaron Plattner.
The kernel spotted it when I tried to vunmap a buffer. :)
Cc: Aaron Plattner
Signed-off-by: Daniel Vetter
Reviewed-by: Aaron Plattner
Tested-by: Aaron Plattner
--
Aaron
---
Documentation/dma-buf-sharing.txt | 6 +-
drivers/base/dma-buf.c| 43 ++-
include/linux/dma-buf.h | 4 +++-
3 files changed, 46 insertions(+), 7 deletions(-)
diff --git a/Documentation/dma-buf-sharing.txt
b/Documentation/dma-buf-sharing.txt
index 0188903..4966b1b 100644
--- a/Documentation/dma-buf-sharing.txt
+++ b/Documentation/dma-buf-sharing.txt
@@ -302,7 +302,11 @@ Access to a dma_buf from the kernel context involves three
steps:
void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
The vmap call can fail if there is no vmap support in the exporter, or if
it
- runs out of vmalloc space. Fallback to kmap should be implemented.
+ runs out of vmalloc space. Fallback to kmap should be implemented. Note that
+ the dma-buf layer keeps a reference count for all vmap access and calls down
+ into the exporter's vmap function only when no vmapping exists, and only
+ unmaps it once. Protection against concurrent vmap/vunmap calls is provided
+ by taking the dma_buf->lock mutex.
3. Finish access
diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index a3f79c4..26b68de 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -39,6 +39,8 @@ static int dma_buf_release(struct inode *inode, struct file
*file)
dmabuf = file->private_data;
+ BUG_ON(dmabuf->vmapping_counter);
+
dmabuf->ops->release(dmabuf);
kfree(dmabuf);
return 0;
@@ -482,12 +484,34 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
*/
void *dma_buf_vmap(struct dma_buf *dmabuf)
{
+ void *ptr;
+
if (WARN_ON(!dmabuf))
return NULL;
- if (dmabuf->ops->vmap)
- return dmabuf->ops->vmap(dmabuf);
- return NULL;
+ if (!dmabuf->ops->vmap)
+ return NULL;
+
+ mutex_lock(&dmabuf->lock);
+ if (dmabuf->vmapping_counter) {
+ dmabuf->vmapping_counter++;
+ BUG_ON(!dmabuf->vmap_ptr);
+ ptr = dmabuf->vmap_ptr;
+ goto out_unlock;
+ }
+
+ BUG_ON(dmabuf->vmap_ptr);
+
+ ptr = dmabuf->ops->vmap(dmabuf);
+ if (IS_ERR_OR_NULL(ptr))
+ goto out_unlock;
+
+ dmabuf->vmap_ptr = ptr;
+ dmabuf->vmapping_counter = 1;
+
+out_unlock:
+ mutex_unlock(&dmabuf->lock);
+ return ptr;
}
EXPORT_SYMBOL_GPL(dma_buf_vmap);
@@ -501,7 +525,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
if (WARN_ON(!dmabuf))
return;
- if (dmabuf->ops->vunmap)
- dmabuf->ops->vunmap(dmabuf, vaddr);
+ BUG_ON(!dmabuf->vmap_ptr);
+ BUG_ON(dmabuf->vmapping_counter == 0);
+ BUG_ON(dmabuf->vmap_ptr != vaddr);
+
+ mutex_lock(&dmabuf->lock);
+ if (--dmabuf->vmapping_counter == 0) {
+ if (dmabuf->ops->vunmap)
+ dmabuf->ops->vunmap(dmabuf, vaddr);
+ dmabuf->vmap_ptr = NULL;
+ }
+ mutex_unlock(&dmabuf->lock);
}
EXPORT_SYMBOL_GPL(dma_buf_vunmap);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index bd2e52c..e3bf2f6 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -119,8 +119,10 @@ struct dma_buf {
struct file *file;
struct list_head attachments;
const struct dma_buf_ops *ops;
- /* mutex to serialize list manipulation and attach/detach */
+ /* mutex to serialize list manipulation, attach/detach and vmap/unmap */
struct mutex lock;
+ unsigne
[PATCH v3 0/3] Prime helpers
This series adds helper functions that abstract the core parts of .gem_prime_import and .gem_prime_export so that drivers don't have to worry about the low-level details. These helpers are optional. A driver can use them by plugging in drm_gem_prime_import and drm_gem_prime_export into the drm_driver structure, or it can bypass them by plugging in its own functions. The first patch adds these helpers, and the later patches switch three drivers over to using them. This version of the series addresses concerns raised by Daniel Vetter, and to leave the vmap locking and refcounting to the dma-buf core code. It also drops Exynos from the series since its driver diverged between when I first sent out this series and now. This series depends on commit 90b6e90cb03352a352015ca213ac9f4fab3308f3 of the for-next branch of git://git.linaro.org/people/sumitsemwal/linux-dma-buf Aaron Plattner (3): drm: add prime helpers drm/nouveau: use prime helpers drm/radeon: use prime helpers Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++- drivers/gpu/drm/nouveau/nouveau_bo.h| 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 173 - drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 21 ++-- drivers/gpu/drm/radeon/radeon_prime.c | 170 - include/drm/drmP.h | 12 +++ 11 files changed, 270 insertions(+), 319 deletions(-) -- 1.8.1.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/3] drm: add prime helpers
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
Documentation/DocBook/drm.tmpl | 4 +
drivers/gpu/drm/drm_prime.c| 186 -
include/drm/drmP.h | 12 +++
3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 4ee2304..ed40576 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -743,6 +743,10 @@ char *date;
These two operations are mandatory for GEM drivers that support DRM
PRIME.
+
+ DRM PRIME Helper Functions Reference
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+
GEM Objects Mapping
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..366910d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -53,7 +53,8 @@
* Self-importing: if userspace is using PRIME as a replacement for flink
* then it will get a fd->handle request for a GEM object that it created.
* Drivers should detect this situation and return back the gem object
- * from the dma-buf private.
+ * from the dma-buf private. Prime will do this automatically for drivers that
+ * use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,137 @@ struct drm_prime_member {
uint32_t handle;
};
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *sgt;
+
+ mutex_lock(&obj->dev->struct_mutex);
+
+ sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+
+ if (!IS_ERR_OR_NULL(sgt))
+ dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
+ return sgt;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ sg_free_table(sgt);
+ kfree(sgt);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ return dev->driver->gem_prime_vmap(obj);
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ dev->driver->gem_prime_vunmap(obj, vaddr);
+}
+
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
[PATCH 2/3] drm/nouveau: use prime helpers
Simplify the Nouveau prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
v2: Rename functions to nouveau_gem_prime_get_sg_table and
nouveau_gem_prime_import_sg_table.
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
drivers/gpu/drm/nouveau/nouveau_bo.h| 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 -
drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 173
5 files changed, 34 insertions(+), 161 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h
b/drivers/gpu/drm/nouveau/nouveau_bo.h
index 25ca379..836677a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -31,7 +31,6 @@ struct nouveau_bo {
int pin_refcnt;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
static inline struct nouveau_bo *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 180a45e..5de1f9a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -645,8 +645,13 @@ driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = nouveau_gem_prime_export,
- .gem_prime_import = nouveau_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = nouveau_gem_prime_pin,
+ .gem_prime_get_sg_table = nouveau_gem_prime_get_sg_table,
+ .gem_prime_import_sg_table = nouveau_gem_prime_import_sg_table,
+ .gem_prime_vmap = nouveau_gem_prime_vmap,
+ .gem_prime_vunmap = nouveau_gem_prime_vunmap,
.gem_init_object = nouveau_gem_object_new,
.gem_free_object = nouveau_gem_object_del,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 8bf695c..24e0aab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
*
*/
-#include
-
#include
#include "nouveau_drm.h"
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h
b/drivers/gpu/drm/nouveau/nouveau_gem.h
index 5c10492..8d7a3f0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -35,9 +35,11 @@ extern int nouveau_gem_ioctl_cpu_fini(struct drm_device *,
void *,
extern int nouveau_gem_ioctl_info(struct drm_device *, void *,
struct drm_file *);
-extern struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj, int flags);
-extern struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+extern int nouveau_gem_prime_pin(struct drm_gem_object *);
+extern struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object
*);
+extern struct drm_gem_object *nouveau_gem_prime_import_sg_table(
+ struct drm_device *, size_t size, struct sg_table *);
+extern void *nouveau_gem_prime_vmap(struct drm_gem_object *);
+extern void nouveau_gem_prime_vunmap(struct drm_gem_object *, void *);
#endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c
b/drivers/gpu/drm/nouveau/nouveau_prime.c
index b8e05ae..f53e108 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -22,126 +22,42 @@
* Authors: Dave Airlie
*/
-#include
-
#include
#include "nouveau_drm.h"
#include "nouveau_gem.h"
-static struct sg_table *nouveau_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
- struct nouveau_bo *nvbo = attachment->dmabuf->priv;
- struct drm_device *dev = nvbo->gem->dev;
+ struct nouveau_bo *nvbo = nouveau_gem_object(obj);
int npages = nvbo->bo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void nouveau_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
- struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct nouveau_bo *nvbo = dma_buf->priv;
-
- if (nv
[PATCH 3/3] drm/radeon: use prime helpers
Simplify the Radeon prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
v2:
- Rename functions to radeon_gem_prime_get_sg_table and
radeon_gem_prime_import_sg_table.
- Delete the now-unused vmapping_count variable.
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
drivers/gpu/drm/radeon/radeon.h | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 21 +++--
drivers/gpu/drm/radeon/radeon_prime.c | 170 +-
3 files changed, 35 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 34e5230..48bb80e 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -342,7 +342,6 @@ struct radeon_bo {
struct drm_gem_object gem_base;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon/radeon_drv.c
index dff6cf7..7a63817 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -117,11 +117,13 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
int radeon_mode_dumb_destroy(struct drm_file *file_priv,
struct drm_device *dev,
uint32_t handle);
-struct dma_buf *radeon_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj,
- int flags);
-struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj);
+struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg);
+int radeon_gem_prime_pin(struct drm_gem_object *obj);
+void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
+void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
#if defined(CONFIG_DEBUG_FS)
int radeon_debugfs_init(struct drm_minor *minor);
@@ -396,8 +398,13 @@ static struct drm_driver kms_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = radeon_gem_prime_export,
- .gem_prime_import = radeon_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = radeon_gem_prime_pin,
+ .gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
+ .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
+ .gem_prime_vmap = radeon_gem_prime_vmap,
+ .gem_prime_vunmap = radeon_gem_prime_vunmap,
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c
b/drivers/gpu/drm/radeon/radeon_prime.c
index 26c23bb..4940af7 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -28,199 +28,71 @@
#include "radeon.h"
#include
-#include
-
-static struct sg_table *radeon_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
- struct radeon_bo *bo = attachment->dmabuf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int npages = bo->tbo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void radeon_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
-struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
-
- if (bo->gem_base.export_dma_buf == dma_buf) {
- DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base);
- bo->gem_base.export_dma_buf = NULL;
- drm_gem_object_unreference_unlocked(&bo->gem_base);
- }
-}
-
-static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_
Re: [PATCH v3 1/3] drm: add prime helpers
On 01/16/2013 01:50 AM, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
Two minor things, but since you're not touching drm/i915 I don't care that
much ...
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *sgt;
+
+ mutex_lock(&obj->dev->struct_mutex);
+
+ sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+
+ if (!IS_ERR_OR_NULL(sgt))
+ dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
The locking here looks superflous and not required to protect anything
drm_gem_map_dma_buf does. If drivers need this (and ttm based ones
shouldn't really), they can grab it in their ->gem_prime_get_sg_table
callback. So I think a follow-up patch to ditch this would be good.
If you look at the later patches, this was just moved from the drivers.
I agree that evaluating whether or not it's actually necessary should
be a separate follow-up.
+ return sgt;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ sg_free_table(sgt);
+ kfree(sgt);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ return dev->driver->gem_prime_vmap(obj);
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ dev->driver->gem_prime_vunmap(obj, vaddr);
+}
Imo these two don't add value, simpler to add a typechecking
drm_dmabuf_to_gem_obj static inline somewhere. But then you'd need to pass
in the dmabuf fops struct from drivers and export the helpers. More
flexible, but also a bit work to redo. Like I've said, I don't care too
much ...
I considered that, but it seems like the wrong abstraction. The
advantage here is that drivers don't have to care even a little bit
about the buffer sharing mechanism, which simplifies the drivers.
I.e., from patch #2,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 8bf695c..24e0aab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
*
*/
-#include
Can I consider this a Reviewed-by?
--
Aaron
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: modify pages_to_sg prime helper to create optimized SG table
On 01/28/2013 05:38 AM, Rahul Sharma wrote:
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
Reviewed-by: Aaron Plattner
I also verified that this reduces my 2025-entry sg_table to 6 entries, so
Tested-by: Aaron Plattner
--
Aaron
---
drivers/gpu/drm/drm_prime.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..072ee08 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -217,21 +217,17 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev,
void *data,
struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
{
struct sg_table *sg = NULL;
- struct scatterlist *iter;
- int i;
int ret;
sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
if (!sg)
goto out;
- ret = sg_alloc_table(sg, nr_pages, GFP_KERNEL);
+ ret = sg_alloc_table_from_pages(sg, pages, nr_pages, 0,
+ nr_pages << PAGE_SHIFT, GFP_KERNEL);
if (ret)
goto out;
- for_each_sg(sg->sgl, iter, nr_pages, i)
- sg_set_page(iter, pages[i], PAGE_SIZE, 0);
-
return sg;
out:
kfree(sg);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH libdrm] tests/dristat: add -C to pretty-print device capabilities
Signed-off-by: Aaron Plattner
---
Example output of dristat -C:
/dev/dri/card0
Device capabilities:
Dumb framebuffer: yes
VBlank high crtc: yes
Preferred depth: 24
Prefer shadow: yes
Prime: import export
tests/dristat.c | 69 -
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/tests/dristat.c b/tests/dristat.c
index 900a3e6..d36c3de 100644
--- a/tests/dristat.c
+++ b/tests/dristat.c
@@ -24,9 +24,11 @@
* DEALINGS IN THE SOFTWARE.
*
* Authors: Rickard E. (Rik) Faith
+ * Authors: Aaron Plattner
*
*/
+#include
#include
#include
#include
@@ -35,11 +37,14 @@
#include "xf86drmHash.c"
#include "xf86drm.c"
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
#define DRM_VERSION 0x0001
#define DRM_MEMORY 0x0002
#define DRM_CLIENTS 0x0004
#define DRM_STATS 0x0008
#define DRM_BUSID 0x0010
+#define DRM_CAPS0x0020
static void getversion(int fd)
{
@@ -228,6 +233,65 @@ static void getstats(int fd, int i)
}
+enum cap_type {
+CAP_BOOL,
+CAP_UINT,
+CAP_PRIME
+};
+
+static void printcap(enum cap_type type, uint64_t value)
+{
+switch (type) {
+case CAP_BOOL:
+ if (value) printf("yes");
+ else printf("no");
+ break;
+case CAP_UINT:
+ printf("%" PRIu64, value);
+ break;
+case CAP_PRIME:
+ if (value == 0) printf("none");
+ else {
+ if (value & DRM_PRIME_CAP_IMPORT) printf("import ");
+ if (value & DRM_PRIME_CAP_EXPORT) printf("export");
+ }
+ break;
+}
+}
+
+static void getcaps(int fd)
+{
+const struct {
+ uint64_t capability;
+ enum cap_type type;
+ const char *name;
+} caps[] = {
+ { DRM_CAP_DUMB_BUFFER, CAP_BOOL, "Dumb framebuffer" },
+ { DRM_CAP_VBLANK_HIGH_CRTC, CAP_BOOL, "VBlank high crtc" },
+ { DRM_CAP_DUMB_PREFERRED_DEPTH, CAP_UINT, "Preferred depth" },
+ { DRM_CAP_DUMB_PREFER_SHADOW, CAP_BOOL, "Prefer shadow" },
+ { DRM_CAP_PRIME,CAP_PRIME, "Prime" },
+};
+int i;
+
+printf(" Device capabilities:\n");
+
+for (i = 0; i < ARRAY_SIZE(caps); i++) {
+ uint64_t value;
+ int ret = drmGetCap(fd, caps[i].capability, &value);
+
+ printf("%s: ", caps[i].name);
+
+ if (ret) {
+ printf("\n");
+ continue;
+ }
+
+ printcap(caps[i].type, value);
+ printf("\n");
+}
+}
+
int main(int argc, char **argv)
{
int c;
@@ -238,7 +302,7 @@ int main(int argc, char **argv)
char buf[64];
int i;
-while ((c = getopt(argc, argv, "avmcsbM:i:")) != EOF)
+while ((c = getopt(argc, argv, "avmcCsbM:i:")) != EOF)
switch (c) {
case 'a': mask = ~0; break;
case 'v': mask |= DRM_VERSION;break;
@@ -246,6 +310,7 @@ int main(int argc, char **argv)
case 'c': mask |= DRM_CLIENTS;break;
case 's': mask |= DRM_STATS; break;
case 'b': mask |= DRM_BUSID; break;
+ case 'C': mask |= DRM_CAPS; break;
case 'i': interval = strtol(optarg, NULL, 0); break;
case 'M': minor = strtol(optarg, NULL, 0);break;
default:
@@ -254,6 +319,7 @@ int main(int argc, char **argv)
fprintf( stderr, " -aShow all available information\n"
);
fprintf( stderr, " -bShow DRM bus ID's\n" );
fprintf( stderr, " -cDisplay information about DRM
clients\n" );
+ fprintf( stderr, " -CDisplay DRM device
capabilities\n" );
fprintf( stderr, " -i [interval] Continuously display statistics
every [interval] seconds\n" );
fprintf( stderr, " -vDisplay DRM module and card
version information\n" );
fprintf( stderr, " -mDisplay memory use information\n"
);
@@ -272,6 +338,7 @@ int main(int argc, char **argv)
if (mask & DRM_MEMORY) getvm(fd);
if (mask & DRM_CLIENTS) getclients(fd);
if (mask & DRM_STATS) getstats(fd, interval);
+ if (mask & DRM_CAPS)getcaps(fd);
close(fd);
}
}
--
1.8.1.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
So I've been experimenting with support for Dave Airlie's new RandR 1.4 provider object interface, so that Optimus-based laptops can use our driver to drive the discrete GPU and display on the integrated GPU. The good news is that I've got a proof of concept working. During a review of the current code, we came up with a few concerns: 1. The output source is responsible for allocating the shared memory Right now, the X server calls CreatePixmap on the output source screen and then expects the output sink screen to be able to display from whatever memory the source allocates. Right now, the source has no mechanism for asking the sink what its requirements are for the surface. I'm using our own internal pitch alignment requirements and that seems to be good enough for the Intel device to scan out, but that could be pure luck. Does it make sense to add a mechanism for drivers to negotiate this with each other, or is it sufficient to just define a lowest common denominator format and if your hardware can't deal with that format, you just don't get to share buffers? One of my coworkers brought to my attention the fact that Tegra requires a specific pitch alignment, and cannot accommodate larger pitches. If other SoC designs have similar restrictions, we might need to add a handshake mechanism. 2. There's no fallback mechanism if sharing can't be negotiated If RandR fails to share a pixmap with the output sink screen, the whole modeset fails. This means you'll end up not seeing anything on the screen and you'll probably think your computer locked up. Should there be some sort of software copy fallback to ensure that something at least shows up on the display? 3. How should the memory be allocated? In the prototype I threw together, I'm allocating the shared memory using shm_open and then exporting that as a dma-buf file descriptor using an ioctl I added to the kernel, and then importing that memory back into our driver through dma_buf_attach & dma_buf_map_attachment. Does it make sense for user-space programs to be able to export shmfs files like that? Should that interface go in DRM / GEM / PRIME instead? Something else? I'm pretty unfamiliar with this kernel code so any suggestions would be appreciated. -- Aaron P.S. for those unfamiliar with PRIME: Dave Airlie added new support to the X Resize and Rotate extension version 1.4 to support offloading display and rendering to different drivers. PRIME is the DRM implementation in the kernel, layered on top of DMA-BUF, that implements the actual sharing of buffers between drivers. http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt?id=randrproto-1.4.0#n122 http://airlied.livejournal.com/7.html - update on hotplug server http://airlied.livejournal.com/76078.html - randr 1.5 demo videos --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Linaro-mm-sig] A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
On 08/30/2012 10:31 AM, Aaron Plattner wrote: So I've been experimenting with support for Dave Airlie's new RandR 1.4 provider object interface, so that Optimus-based laptops can use our driver to drive the discrete GPU and display on the integrated GPU. The good news is that I've got a proof of concept working. During a review of the current code, we came up with a few concerns: 1. The output source is responsible for allocating the shared memory Right now, the X server calls CreatePixmap on the output source screen and then expects the output sink screen to be able to display from whatever memory the source allocates. Right now, the source has no mechanism for asking the sink what its requirements are for the surface. I'm using our own internal pitch alignment requirements and that seems to be good enough for the Intel device to scan out, but that could be pure luck. Does it make sense to add a mechanism for drivers to negotiate this with each other, or is it sufficient to just define a lowest common denominator format and if your hardware can't deal with that format, you just don't get to share buffers? One of my coworkers brought to my attention the fact that Tegra requires a specific pitch alignment, and cannot accommodate larger pitches. If other SoC designs have similar restrictions, we might need to add a handshake mechanism. 2. There's no fallback mechanism if sharing can't be negotiated If RandR fails to share a pixmap with the output sink screen, the whole modeset fails. This means you'll end up not seeing anything on the screen and you'll probably think your computer locked up. Should there be some sort of software copy fallback to ensure that something at least shows up on the display? 3. How should the memory be allocated? In the prototype I threw together, I'm allocating the shared memory using shm_open and then exporting that as a dma-buf file descriptor using an ioctl I added to the kernel, and then importing that memory back into our driver through dma_buf_attach & dma_buf_map_attachment. Does it make sense for user-space programs to be able to export shmfs files like that? Should that interface go in DRM / GEM / PRIME instead? Something else? I'm pretty unfamiliar with this kernel code so any suggestions would be appreciated. There's also a #4 that didn't seem relevant to cross-post to linaro-mm-sig: 4. There's no mechanism for double buffering the output sink RandR allocates one pixmap on the output source screen and sets up tracking so the source driver can copy the screen into the shared pixmap. However, the sink driver scans out from the shared pixmap directly. There's no mechanism to prevent tearing on the sink side of the pipeline. It seems like it would be nice if the source could trigger the sink device to flip between front and back buffers when the copy is finished, and get back a fence to indicate when the flip has occurred and the source can start the next copy. -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
On 08/31/2012 08:00 PM, Dave Airlie wrote: object interface, so that Optimus-based laptops can use our driver to drive the discrete GPU and display on the integrated GPU. The good news is that I've got a proof of concept working. Don't suppose you'll be interested in adding the other method at some point as well? since saving power is probably important to a lot of people That's milestone 2. I'm focusing on display offload to start because it's easier to implement and lays the groundwork for the kernel pieces. I have to emphasize that I'm just doing a feasibility study right now and I can't promise that we're going to officially support this stuff. During a review of the current code, we came up with a few concerns: 1. The output source is responsible for allocating the shared memory Right now, the X server calls CreatePixmap on the output source screen and then expects the output sink screen to be able to display from whatever memory the source allocates. Right now, the source has no mechanism for asking the sink what its requirements are for the surface. I'm using our own internal pitch alignment requirements and that seems to be good enough for the Intel device to scan out, but that could be pure luck. Well in theory it might be nice but it would have been premature since so far the only interactions for prime are combination of intel, nvidia and AMD, and I think everyone has fairly similar pitch alignment requirements, I'd be interested in adding such an interface but I don't think its some I personally would be working on. Okay. Hopefully that won't be too painful to add if we ever need it in the future. other, or is it sufficient to just define a lowest common denominator format and if your hardware can't deal with that format, you just don't get to share buffers? At the moment I'm happy to just go with linear, minimum pitch alignment 64 or 256, for us. something as a base standard, but yeah I'm happy for it to work either way, just don't have enough evidence it's worth it yet. I've not looked at ARM stuff, so patches welcome if people consider they need to use this stuff for SoC devices. We can always hack it to whatever is necessary if we see that the sink side driver is Tegra, but I was hoping for something more general. 2. There's no fallback mechanism if sharing can't be negotiated If RandR fails to share a pixmap with the output sink screen, the whole modeset fails. This means you'll end up not seeing anything on the screen and you'll probably think your computer locked up. Should there be some sort of software copy fallback to ensure that something at least shows up on the display? Uggh, it would be fairly slow and unuseable, I'd rather they saw nothing, but again open to suggestions on how to make this work, since it might fail for other reasons and in that case there is still nothing a sw copy can do. What happens if the slave intel device just fails to allocate a pixmap, but yeah I'm willing to think about it a bit more when we have some reference implementations. Just rolling back the modeset operation to whatever was working before would be a good start. It's worse than that on my current laptop, though, since our driver sees a phantom CRT output and we happily start driving pixels to it that end up going nowhere. I'll need to think about what the right behavior is there since I don't know if we want to rely on an X client to make that configuration work. 3. How should the memory be allocated? In the prototype I threw together, I'm allocating the shared memory using shm_open and then exporting that as a dma-buf file descriptor using an ioctl I added to the kernel, and then importing that memory back into our driver through dma_buf_attach & dma_buf_map_attachment. Does it make sense for user-space programs to be able to export shmfs files like that? Should that interface go in DRM / GEM / PRIME instead? Something else? I'm pretty unfamiliar with this kernel code so any suggestions would be appreciated. Your kernel driver should in theory be doing it all, if you allocate shared pixmaps in GTT accessible memory, then you need an ioctl to tell your kernel driver to export the dma buf to the fd handle. (assuming we get rid of the _GPL, which people have mentioned they are open to doing). We have handle->fd and fd->handle interfaces on DRM, you'd need something similiar on the nvidia kernel driver interface. Okay, I can do that. We already have a mechanism for importing buffers allocated elsewhere so reusing that for shmfs and/or dma-buf seemed like a natural extension. I don't think adding a separate ioctl for exporting our own allocations will add too much extra code. Yes for 4 some sort of fencing is being worked on by Maarten for other stuff but would be a pre-req for doing this, and also some devices don't want fullscreen updates, like USB, so doing flipped updates would have to be optional or negoitated. It makes sense for us as wel
Re: [PATCH v3 1/3] drm: add prime helpers
On 04/12/13 07:58, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
Documentation/DocBook/drm.tmpl | 4 +
drivers/gpu/drm/drm_prime.c| 186 -
include/drm/drmP.h | 12 +++
3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 4ee2304..ed40576 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -743,6 +743,10 @@ char *date;
These two operations are mandatory for GEM drivers that support DRM
PRIME.
+
+ DRM PRIME Helper Functions Reference
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+
GEM Objects Mapping
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..366910d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -53,7 +53,8 @@
* Self-importing: if userspace is using PRIME as a replacement for flink
* then it will get a fd->handle request for a GEM object that it created.
* Drivers should detect this situation and return back the gem object
- * from the dma-buf private.
+ * from the dma-buf private. Prime will do this automatically for drivers that
+ * use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,137 @@ struct drm_prime_member {
uint32_t handle;
};
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *sgt;
+
+ mutex_lock(&obj->dev->struct_mutex);
+
+ sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+
+ if (!IS_ERR_OR_NULL(sgt))
+ dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
+ return sgt;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ sg_free_table(sgt);
+ kfree(sgt);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ return dev->driver->gem_prime_vmap(obj);
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ dev->driver->gem_prime_vunmap(obj, vaddr);
+}
+
+static void *drm_gem_dmabuf_
Re: [i915] Backlight brighter since 3.9.0
On 05/20/2013 02:55 PM, Daniel Vetter wrote: On Sat, May 18, 2013 at 12:39:14AM +0200, Jan Hinnerk Stosch wrote: Hallo, I hope this is the right place to ask, because I actually don't know whether it is a bug or a feature that I'm experiencing since linux 3.9: When I boot my system the backlight gets extremely bright compared to older kernel versions. It is most obvious when I leave X (more a yellow than a black background), but I have the impression, that the colors in X are brighter than usual, too. I used my spare time this afternoon to do a kernel bisect and learned that the first "bad" commit is 55bc60db5988c8366751d3d04dd690698a53412c. As I don't have insight or understanding of the code: Is this behaviour intended and how could I change it to the old state or is it a bug and should I report it somewhere? My system is as follows: Intel i5-3570k with Intel HD 4000 my monitor is connected via HDMI. If you need any more information just tell me. Yeah, this is a feature. HDMI has (for oddball backwards compat with analog TV signals) a special mode which reduces the useable RGB value range by chopping off about 10% at the bottom and top end. This results in light colors getting brighter and dark colors getting darker. The above mentioned commit tries (to the best of our knowledge) to auto-set the option which most likely fits what the hdmi sink will do with the color data. You can either fix this up in the hdmi sink with the on-screen menu or by manually setting the "RBG Broadcast" property for the relevant hdmi connector to the setting you want. This property seems like it's generally useful for all GPUs that support range compression. Has anyone started the process of adding it to randrproto.txt as an official property? http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt#n1723 -- Aaron --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. --- ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [i915] Backlight brighter since 3.9.0
On 06/03/2013 12:36 PM, Daniel Vetter wrote: On Mon, Jun 03, 2013 at 09:13:18AM -0700, Aaron Plattner wrote: On 05/20/2013 02:55 PM, Daniel Vetter wrote: On Sat, May 18, 2013 at 12:39:14AM +0200, Jan Hinnerk Stosch wrote: Hallo, I hope this is the right place to ask, because I actually don't know whether it is a bug or a feature that I'm experiencing since linux 3.9: When I boot my system the backlight gets extremely bright compared to older kernel versions. It is most obvious when I leave X (more a yellow than a black background), but I have the impression, that the colors in X are brighter than usual, too. I used my spare time this afternoon to do a kernel bisect and learned that the first "bad" commit is 55bc60db5988c8366751d3d04dd690698a53412c. As I don't have insight or understanding of the code: Is this behaviour intended and how could I change it to the old state or is it a bug and should I report it somewhere? My system is as follows: Intel i5-3570k with Intel HD 4000 my monitor is connected via HDMI. If you need any more information just tell me. Yeah, this is a feature. HDMI has (for oddball backwards compat with analog TV signals) a special mode which reduces the useable RGB value range by chopping off about 10% at the bottom and top end. This results in light colors getting brighter and dark colors getting darker. The above mentioned commit tries (to the best of our knowledge) to auto-set the option which most likely fits what the hdmi sink will do with the color data. You can either fix this up in the hdmi sink with the on-screen menu or by manually setting the "RBG Broadcast" property for the relevant hdmi connector to the setting you want. This property seems like it's generally useful for all GPUs that support range compression. Has anyone started the process of adding it to randrproto.txt as an official property? http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt#n1723 Oops, I didn't know that we have some properties standardized there, especially since the existing pile of drm/kms drivers seem to only lously follow them. Should we move this into the kernel since that's essentially the place that defines them? Maybe? I think I'm the only one who even tries to follow those, so "SHOULD" and "MUST" don't really mean a whole lot right now. One option would be to just abandon the idea of standardizing properties, but I do think standardization is good. Where that standard should live, though, is a another question. The kernel doesn't seem like the right place since RandR properties are useful on lots of platforms other than Linux. -Daniel -- Aaron ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm: add mmap function to prime helpers
On 06/12/2013 06:16 AM, Joonyoung Shim wrote:
This adds to call low-level mmap() from prime helpers.
Signed-off-by: Joonyoung Shim
---
drivers/gpu/drm/drm_prime.c | 5 -
include/drm/drmP.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index d92853e..3a008b2 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -165,7 +165,10 @@ static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
struct vm_area_struct *vma)
{
- return -EINVAL;
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ return dev->driver->gem_prime_mmap(obj, vma);
Won't this crash if the driver doesn't fill in the new field and
userspace tries to map it?
}
static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 82670ac..12083dc 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -937,6 +937,8 @@ struct drm_driver {
struct sg_table *sgt);
void *(*gem_prime_vmap)(struct drm_gem_object *obj);
void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
+ int (*gem_prime_mmap)(struct drm_gem_object *obj,
+ struct vm_area_struct *vma);
/* vga arb irq handler */
void (*vgaarb_irq)(struct drm_device *dev, bool state);
--
Aaron
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/3] drm: add prime helpers
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
Documentation/DocBook/drm.tmpl | 4 +
drivers/gpu/drm/drm_prime.c| 186 +-
include/drm/drmP.h | 12 +++
3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..366910d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
+ * DOC: PRIME Helpers
+ *
+ * Drivers can implement @gem_prime_export and @gem_prime_import in terms
of + * simpler APIs by using the helper functions @drm_gem_prime_export and
+ * @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
+ *
+ * Export callbacks:
+ *
+ * - @gem_prime_pin (optional): prepare a GEM object for exporting
+ *
+ * - @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
+ * - @gem_prime_vmap: vmap a buffer exported by your driver
+ *
+ * - @gem_prime_vunmap: vunmap a buffer exported by your driver
+ *
+ * Import callback:
+ *
+ * - @gem_prime_import_sg_table (import): produce a GEM object from
another + *driver's scatter/gather table
+ */
+
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
+struct drm_gem_object *obj, int flags)
+{
+ if (dev->driver->gem_prime_pin) {
+ int ret = dev->driver->gem_prime_pin(obj);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+ return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
+ 0600);
Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit
5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you
pointed it out just now.
+}
+EXPORT_SYMBOL(drm_gem_prime_export);
--
Aaron
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/3] drm: add prime helpers
On 06/18/2013 04:37 PM, Laurent Pinchart wrote:
Hi Aaron,
On Tuesday 18 June 2013 16:28:15 Aaron Plattner wrote:
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
Documentation/DocBook/drm.tmpl | 4 +
drivers/gpu/drm/drm_prime.c| 186
+-
include/drm/drmP.h | 12 +++
3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..366910d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
+ * DOC: PRIME Helpers
+ *
+ * Drivers can implement @gem_prime_export and @gem_prime_import in
terms
of + * simpler APIs by using the helper functions @drm_gem_prime_export
and
+ * @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
+ *
+ * Export callbacks:
+ *
+ * - @gem_prime_pin (optional): prepare a GEM object for exporting
+ *
+ * - @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
+ * - @gem_prime_vmap: vmap a buffer exported by your driver
+ *
+ * - @gem_prime_vunmap: vunmap a buffer exported by your driver
+ *
+ * Import callback:
+ *
+ * - @gem_prime_import_sg_table (import): produce a GEM object from
another + *driver's scatter/gather table
+ */
+
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
+struct drm_gem_object *obj, int flags)
+{
+ if (dev->driver->gem_prime_pin) {
+ int ret = dev->driver->gem_prime_pin(obj);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+ return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
+ 0600);
Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit
5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you
pointed it out just now.
That's a pretty valid reason I guess :-) Should I submit a patch or are you
already working on one ?
:) Sorry!
I'm not working on this right now, but I can put it on my queue if you
like. You'll probably be able to fix and test it sooner than I will,
though.
+}
+EXPORT_SYMBOL(drm_gem_prime_export);
--
Aaron
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
So I've been experimenting with support for Dave Airlie's new RandR 1.4 provider object interface, so that Optimus-based laptops can use our driver to drive the discrete GPU and display on the integrated GPU. The good news is that I've got a proof of concept working. During a review of the current code, we came up with a few concerns: 1. The output source is responsible for allocating the shared memory Right now, the X server calls CreatePixmap on the output source screen and then expects the output sink screen to be able to display from whatever memory the source allocates. Right now, the source has no mechanism for asking the sink what its requirements are for the surface. I'm using our own internal pitch alignment requirements and that seems to be good enough for the Intel device to scan out, but that could be pure luck. Does it make sense to add a mechanism for drivers to negotiate this with each other, or is it sufficient to just define a lowest common denominator format and if your hardware can't deal with that format, you just don't get to share buffers? One of my coworkers brought to my attention the fact that Tegra requires a specific pitch alignment, and cannot accommodate larger pitches. If other SoC designs have similar restrictions, we might need to add a handshake mechanism. 2. There's no fallback mechanism if sharing can't be negotiated If RandR fails to share a pixmap with the output sink screen, the whole modeset fails. This means you'll end up not seeing anything on the screen and you'll probably think your computer locked up. Should there be some sort of software copy fallback to ensure that something at least shows up on the display? 3. How should the memory be allocated? In the prototype I threw together, I'm allocating the shared memory using shm_open and then exporting that as a dma-buf file descriptor using an ioctl I added to the kernel, and then importing that memory back into our driver through dma_buf_attach & dma_buf_map_attachment. Does it make sense for user-space programs to be able to export shmfs files like that? Should that interface go in DRM / GEM / PRIME instead? Something else? I'm pretty unfamiliar with this kernel code so any suggestions would be appreciated. -- Aaron P.S. for those unfamiliar with PRIME: Dave Airlie added new support to the X Resize and Rotate extension version 1.4 to support offloading display and rendering to different drivers. PRIME is the DRM implementation in the kernel, layered on top of DMA-BUF, that implements the actual sharing of buffers between drivers. http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt?id=randrproto-1.4.0#n122 http://airlied.livejournal.com/7.html - update on hotplug server http://airlied.livejournal.com/76078.html - randr 1.5 demo videos --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ---
[Linaro-mm-sig] A few questions about the best way to implement RandR 1.4 / PRIME buffer sharing
On 08/30/2012 10:31 AM, Aaron Plattner wrote: > So I've been experimenting with support for Dave Airlie's new RandR 1.4 > provider > object interface, so that Optimus-based laptops can use our driver to drive > the > discrete GPU and display on the integrated GPU. The good news is that I've > got > a proof of concept working. > > During a review of the current code, we came up with a few concerns: > > 1. The output source is responsible for allocating the shared memory > > Right now, the X server calls CreatePixmap on the output source screen and > then > expects the output sink screen to be able to display from whatever memory the > source allocates. Right now, the source has no mechanism for asking the sink > what its requirements are for the surface. I'm using our own internal pitch > alignment requirements and that seems to be good enough for the Intel device > to > scan out, but that could be pure luck. > > Does it make sense to add a mechanism for drivers to negotiate this with each > other, or is it sufficient to just define a lowest common denominator format > and > if your hardware can't deal with that format, you just don't get to share > buffers? > > One of my coworkers brought to my attention the fact that Tegra requires a > specific pitch alignment, and cannot accommodate larger pitches. If other SoC > designs have similar restrictions, we might need to add a handshake mechanism. > > 2. There's no fallback mechanism if sharing can't be negotiated > > If RandR fails to share a pixmap with the output sink screen, the whole > modeset > fails. This means you'll end up not seeing anything on the screen and you'll > probably think your computer locked up. Should there be some sort of software > copy fallback to ensure that something at least shows up on the display? > > 3. How should the memory be allocated? > > In the prototype I threw together, I'm allocating the shared memory using > shm_open and then exporting that as a dma-buf file descriptor using an ioctl I > added to the kernel, and then importing that memory back into our driver > through > dma_buf_attach & dma_buf_map_attachment. Does it make sense for user-space > programs to be able to export shmfs files like that? Should that interface go > in DRM / GEM / PRIME instead? Something else? I'm pretty unfamiliar with > this > kernel code so any suggestions would be appreciated. There's also a #4 that didn't seem relevant to cross-post to linaro-mm-sig: 4. There's no mechanism for double buffering the output sink RandR allocates one pixmap on the output source screen and sets up tracking so the source driver can copy the screen into the shared pixmap. However, the sink driver scans out from the shared pixmap directly. There's no mechanism to prevent tearing on the sink side of the pipeline. It seems like it would be nice if the source could trigger the sink device to flip between front and back buffers when the copy is finished, and get back a fence to indicate when the flip has occurred and the source can start the next copy. -- Aaron
[PATCH 0/4] Prime helpers
This series adds helper functions that abstract the core parts of .gem_prime_import and .gem_prime_export so that drivers don't have to worry about the low-level details. These helpers are optional. A driver can use them by plugging in drm_gem_prime_import and drm_gem_prime_export into the drm_driver structure, or it can bypass them by plugging in its own functions. The first patch adds these helpers, and the later patches switch three drivers over to using them. Jerome, I tried to address your concerns about code sharing by making the helpers available without making them required. I haven't yet compile-tested the Exynos change since I don't have a toolchain set up for that, but I'll try to do that today. Aaron Plattner (4): drm: add prime helpers drm/nouveau: use prime helpers drm/radeon: use prime helpers drm/exynos: use prime helpers drivers/gpu/drm/drm_prime.c| 190 - drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 151 ++- drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 +- drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 + drivers/gpu/drm/nouveau/nouveau_bo.h | 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 11 +- drivers/gpu/drm/nouveau/nouveau_prime.c| 172 -- drivers/gpu/drm/radeon/radeon_drv.c| 17 +-- drivers/gpu/drm/radeon/radeon_prime.c | 169 - include/drm/drmP.h | 15 +++ 12 files changed, 288 insertions(+), 464 deletions(-) -- 1.8.0.1
[PATCH 1/4] drm: add prime helpers
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_pages: convert a drm_gem_object to an sg_table for export
gem_prime_import_sg: 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.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/drm_prime.c | 190 +++-
include/drm/drmP.h | 15
2 files changed, 204 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..566c2ac 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -53,7 +53,8 @@
* Self-importing: if userspace is using PRIME as a replacement for flink
* then it will get a fd->handle request for a GEM object that it created.
* Drivers should detect this situation and return back the gem object
- * from the dma-buf private.
+ * from the dma-buf private. Prime will do this automatically for drivers that
+ * use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,146 @@ struct drm_prime_member {
uint32_t handle;
};
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *st;
+
+ mutex_lock_interruptible(&obj->dev->struct_mutex);
+
+ st = obj->dev->driver->gem_prime_get_pages(obj);
+
+ if (!IS_ERR_OR_NULL(st))
+ dma_map_sg(attach->dev, st->sgl, st->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
+ return st;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sg, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sg->sgl, sg->nents, dir);
+ sg_free_table(sg);
+ kfree(sg);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+ void *virtual = ERR_PTR(-EINVAL);
+
+ if (!dev->driver->gem_prime_vmap)
+ return ERR_PTR(-EINVAL);
+
+ mutex_lock(&dev->struct_mutex);
+ if (obj->vmapping_count) {
+ obj->vmapping_count++;
+ virtual = obj->vmapping_ptr;
+ goto out_unlock;
+ }
+
+ virtual = dev->driver->gem_prime_vmap(obj);
+ if (IS_ERR(virtual)) {
+ mutex_unlock(&dev->struct_mutex);
+ return virtual;
+ }
+ obj->vmapping_count = 1;
+ obj->vmapping_ptr = virtual;
+out_unlock:
+ mutex_unlock(&dev->struct_mutex);
+ return obj->vmapping_ptr;
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ mutex_lock(&dev->struct_mutex);
+ obj->vmapping_count--;
+ if (obj->vmapping_count == 0) {
+ dev->driver->gem_prime_vunmap(obj);
+ obj->vmapping_ptr = NULL;
+ }
+ mutex_unlock(&dev->struct_mutex);
+}
+
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
+
+static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num, void *addr)
+{
+
+}
+static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
+
+static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
+ unsigned long page_num, void *addr)
+{
+
+}
+
+static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
+ struct vm_area_struct *vma)
+{
+ return -EINVAL;
+}
+
+static int drm_gem_begin_cpu_access(struct dma_buf *dma_buf,
+ size_t start, size_t length, enum dma_data_direction direction)
+{
+ return -EINVAL;
+}
+
+static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
+ .map_dma_buf = drm_gem_map_
[PATCH 3/4] drm/radeon: use prime helpers
Simplify the Radeon prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/radeon/radeon_drv.c | 17 ++--
drivers/gpu/drm/radeon/radeon_prime.c | 169 +-
2 files changed, 31 insertions(+), 155 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon/radeon_drv.c
index 8c1a83c..a724c3c 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -113,11 +113,11 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
int radeon_mode_dumb_destroy(struct drm_file *file_priv,
struct drm_device *dev,
uint32_t handle);
-struct dma_buf *radeon_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj,
- int flags);
-struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+struct sg_table *radeon_gem_prime_get_pages(struct drm_gem_object *obj);
+struct drm_gem_object *radeon_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg);
+int radeon_gem_prime_pin(struct drm_gem_object *obj);
#if defined(CONFIG_DEBUG_FS)
int radeon_debugfs_init(struct drm_minor *minor);
@@ -392,8 +392,11 @@ static struct drm_driver kms_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = radeon_gem_prime_export,
- .gem_prime_import = radeon_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = radeon_gem_prime_pin,
+ .gem_prime_get_pages = radeon_gem_prime_get_pages,
+ .gem_prime_import_sg = radeon_gem_prime_import_sg,
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c
b/drivers/gpu/drm/radeon/radeon_prime.c
index e095218..3a047c7 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -28,198 +28,71 @@
#include "radeon.h"
#include
-#include
-
-static struct sg_table *radeon_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *radeon_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct radeon_bo *bo = attachment->dmabuf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int npages = bo->tbo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void radeon_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
-struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
-
- if (bo->gem_base.export_dma_buf == dma_buf) {
- DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base);
- bo->gem_base.export_dma_buf = NULL;
- drm_gem_object_unreference_unlocked(&bo->gem_base);
- }
-}
-
-static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_kunmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num, void *addr)
-{
-
-}
-static void *radeon_gem_kmap(struct dma_buf *dma_buf, unsigned long page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_kunmap(struct dma_buf *dma_buf, unsigned long page_num,
void *addr)
-{
+ return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
}
-static int radeon_gem_prime_mmap(struct dma_buf *dma_buf, struct
vm_area_struct *vma)
+void *radeon_gem_prime_vmap(struct drm_gem_object *obj)
{
- return -EINVAL;
-}
-
-static void *radeon_gem_prime_vmap(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int ret;
- mutex_lock(&dev->struct_mutex);
- if (bo->vmapping_count) {
- bo->vmapping_count++;
- goto out_unlock;
-
[PATCH 2/4] drm/nouveau: use prime helpers
Simplify the Nouveau prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
drivers/gpu/drm/nouveau/nouveau_bo.h| 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 -
drivers/gpu/drm/nouveau/nouveau_gem.h | 11 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 172
5 files changed, 35 insertions(+), 160 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h
b/drivers/gpu/drm/nouveau/nouveau_bo.h
index dec51b1..1793631 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -31,7 +31,6 @@ struct nouveau_bo {
int pin_refcnt;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
static inline struct nouveau_bo *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index a7529b3..7bbe1c4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -644,8 +644,13 @@ driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = nouveau_gem_prime_export,
- .gem_prime_import = nouveau_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = nouveau_gem_prime_pin,
+ .gem_prime_get_pages = nouveau_gem_prime_get_pages,
+ .gem_prime_import_sg = nouveau_gem_prime_import_sg,
+ .gem_prime_vmap = nouveau_gem_prime_vmap,
+ .gem_prime_vunmap = nouveau_gem_prime_vunmap,
.gem_init_object = nouveau_gem_object_new,
.gem_free_object = nouveau_gem_object_del,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 5e2f521..9c01f7c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
*
*/
-#include
-
#include
#include "nouveau_drm.h"
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h
b/drivers/gpu/drm/nouveau/nouveau_gem.h
index 5c10492..195e23c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -35,9 +35,12 @@ extern int nouveau_gem_ioctl_cpu_fini(struct drm_device *,
void *,
extern int nouveau_gem_ioctl_info(struct drm_device *, void *,
struct drm_file *);
-extern struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj, int flags);
-extern struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+extern int nouveau_gem_prime_pin(struct drm_gem_object *);
+extern struct sg_table *nouveau_gem_prime_get_pages(struct drm_gem_object *);
+extern struct drm_gem_object *nouveau_gem_prime_import_sg(struct drm_device *,
+ size_t size,
+ struct sg_table *);
+extern void *nouveau_gem_prime_vmap(struct drm_gem_object *);
+extern void nouveau_gem_prime_vunmap(struct drm_gem_object *);
#endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c
b/drivers/gpu/drm/nouveau/nouveau_prime.c
index 3543fec..c3683f0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -22,126 +22,42 @@
* Authors: Dave Airlie
*/
-#include
-
#include
#include "nouveau_drm.h"
#include "nouveau_gem.h"
-static struct sg_table *nouveau_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *nouveau_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct nouveau_bo *nvbo = attachment->dmabuf->priv;
- struct drm_device *dev = nvbo->gem->dev;
+ struct nouveau_bo *nvbo = nouveau_gem_object(obj);
int npages = nvbo->bo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void nouveau_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
- struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct nouveau_bo *nvbo = dma_buf->priv;
-
- if (nvbo->gem->export_dma_buf == dma_buf) {
- nvbo->gem->export_dma_buf =
[PATCH 4/4] drm/exynos: use prime helpers
Simplify the Exynos prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
I still need to find a system to test this.
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 151 ++---
drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 +
3 files changed, 18 insertions(+), 148 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 539da9f..7f6610d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -28,8 +28,6 @@
#include "exynos_drm_drv.h"
#include "exynos_drm_gem.h"
-#include
-
static struct sg_table *exynos_get_sgt(struct drm_device *drm_dev,
struct exynos_drm_gem_buf *buf)
{
@@ -56,15 +54,12 @@ out:
return NULL;
}
-static struct sg_table *
- exynos_gem_map_dma_buf(struct dma_buf_attachment *attach,
- enum dma_data_direction dir)
+struct sg_table *exynos_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv;
+ struct exynos_drm_gem_obj *gem_obj = to_exynos_gem_obj(obj);
struct drm_device *dev = gem_obj->base.dev;
struct exynos_drm_gem_buf *buf;
struct sg_table *sgt = NULL;
- int nents;
DRM_DEBUG_PRIME("%s\n", __FILE__);
@@ -74,119 +69,20 @@ static struct sg_table *
return sgt;
}
- mutex_lock(&dev->struct_mutex);
-
sgt = exynos_get_sgt(dev, buf);
if (!sgt)
- goto err_unlock;
-
- nents = dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- if (!nents) {
- DRM_ERROR("failed to map sgl with iommu.\n");
- sgt = NULL;
- goto err_unlock;
- }
+ goto err;
DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);
-err_unlock:
- mutex_unlock(&dev->struct_mutex);
+err:
return sgt;
}
-static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
- struct sg_table *sgt,
- enum dma_data_direction dir)
-{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
-
- sg_free_table(sgt);
- kfree(sgt);
- sgt = NULL;
-}
-
-static void exynos_dmabuf_release(struct dma_buf *dmabuf)
+struct drm_gem_object *exynos_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg)
{
- struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv;
-
- DRM_DEBUG_PRIME("%s\n", __FILE__);
-
- /*
-* exynos_dmabuf_release() call means that file object's
-* f_count is 0 and it calls drm_gem_object_handle_unreference()
-* to drop the references that these values had been increased
-* at drm_prime_handle_to_fd()
-*/
- if (exynos_gem_obj->base.export_dma_buf == dmabuf) {
- exynos_gem_obj->base.export_dma_buf = NULL;
-
- /*
-* drop this gem object refcount to release allocated buffer
-* and resources.
-*/
- drm_gem_object_unreference_unlocked(&exynos_gem_obj->base);
- }
-}
-
-static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num,
- void *addr)
-{
- /* TODO */
-}
-
-static void *exynos_gem_dmabuf_kmap(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
- unsigned long page_num, void *addr)
-{
- /* TODO */
-}
-
-static int exynos_gem_dmabuf_mmap(struct dma_buf *dma_buf,
- struct vm_area_struct *vma)
-{
- return -ENOTTY;
-}
-
-static struct dma_buf_ops exynos_dmabuf_ops = {
- .map_dma_buf= exynos_gem_map_dma_buf,
- .unmap_dma_buf = exynos_gem_unmap_dma_buf,
- .kmap = exynos_gem_dmabuf_kmap,
- .kmap_atomic= exynos_gem_dmabuf_kmap_atomic,
- .kunmap = exynos_gem_dmabuf_kunmap,
- .kunmap_atomic = exynos_gem_dmabuf_kunmap_atomic,
- .mm
[PATCH v2] drm/exynos: use prime helpers
Simplify the Exynos prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
Signed-off-by: Aaron Plattner
---
v2: fix dumb mistakes now that I have a toolchain that can compile-test this
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 156 ++---
drivers/gpu/drm/exynos/exynos_drm_dmabuf.h | 13 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.c| 2 +
3 files changed, 20 insertions(+), 151 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
index 539da9f..71b40f4 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
@@ -28,8 +28,6 @@
#include "exynos_drm_drv.h"
#include "exynos_drm_gem.h"
-#include
-
static struct sg_table *exynos_get_sgt(struct drm_device *drm_dev,
struct exynos_drm_gem_buf *buf)
{
@@ -56,15 +54,12 @@ out:
return NULL;
}
-static struct sg_table *
- exynos_gem_map_dma_buf(struct dma_buf_attachment *attach,
- enum dma_data_direction dir)
+struct sg_table *exynos_gem_prime_get_pages(struct drm_gem_object *obj)
{
- struct exynos_drm_gem_obj *gem_obj = attach->dmabuf->priv;
+ struct exynos_drm_gem_obj *gem_obj = to_exynos_gem_obj(obj);
struct drm_device *dev = gem_obj->base.dev;
struct exynos_drm_gem_buf *buf;
struct sg_table *sgt = NULL;
- int nents;
DRM_DEBUG_PRIME("%s\n", __FILE__);
@@ -74,120 +69,20 @@ static struct sg_table *
return sgt;
}
- mutex_lock(&dev->struct_mutex);
-
sgt = exynos_get_sgt(dev, buf);
if (!sgt)
- goto err_unlock;
-
- nents = dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- if (!nents) {
- DRM_ERROR("failed to map sgl with iommu.\n");
- sgt = NULL;
- goto err_unlock;
- }
+ goto err;
DRM_DEBUG_PRIME("buffer size = 0x%lx\n", buf->size);
-err_unlock:
- mutex_unlock(&dev->struct_mutex);
+err:
return sgt;
}
-static void exynos_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
- struct sg_table *sgt,
- enum dma_data_direction dir)
-{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
-
- sg_free_table(sgt);
- kfree(sgt);
- sgt = NULL;
-}
-
-static void exynos_dmabuf_release(struct dma_buf *dmabuf)
+struct drm_gem_object *exynos_gem_prime_import_sg(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sgt)
{
- struct exynos_drm_gem_obj *exynos_gem_obj = dmabuf->priv;
-
- DRM_DEBUG_PRIME("%s\n", __FILE__);
-
- /*
-* exynos_dmabuf_release() call means that file object's
-* f_count is 0 and it calls drm_gem_object_handle_unreference()
-* to drop the references that these values had been increased
-* at drm_prime_handle_to_fd()
-*/
- if (exynos_gem_obj->base.export_dma_buf == dmabuf) {
- exynos_gem_obj->base.export_dma_buf = NULL;
-
- /*
-* drop this gem object refcount to release allocated buffer
-* and resources.
-*/
- drm_gem_object_unreference_unlocked(&exynos_gem_obj->base);
- }
-}
-
-static void *exynos_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
- unsigned long page_num,
- void *addr)
-{
- /* TODO */
-}
-
-static void *exynos_gem_dmabuf_kmap(struct dma_buf *dma_buf,
- unsigned long page_num)
-{
- /* TODO */
-
- return NULL;
-}
-
-static void exynos_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
- unsigned long page_num, void *addr)
-{
- /* TODO */
-}
-
-static int exynos_gem_dmabuf_mmap(struct dma_buf *dma_buf,
- struct vm_area_struct *vma)
-{
- return -ENOTTY;
-}
-
-static struct dma_buf_ops exynos_dmabuf_ops = {
- .map_dma_buf= exynos_gem_map_dma_buf,
- .unmap_dma_buf = exynos_gem_unmap_dma_buf,
- .kmap = exynos_gem_dmabuf_kmap,
- .kmap_atomic= exynos_gem_dmabuf_kmap_atomic,
- .kunmap = exynos_gem_dmabuf_kunmap,
- .kunmap_atomic = exynos_gem_dmabuf_kun
[PATCH v2] drm/exynos: use prime helpers
On 12/06/2012 10:36 PM, Inki Dae wrote: > > Hi, > > CCing media guys. > > I agree with you but we should consider one issue released to v4l2. > > As you may know, V4L2-based driver uses vb2 as buffer manager and the > vb2 includes dmabuf feature>(import and export) And v4l2 uses streaming > concept>(qbuf and dqbuf) > With dmabuf and iommu, generally qbuf imports a fd into its own buffer > and maps it with its own iommu table calling dma_buf_map_attachment(). > And dqbuf calls dma_buf_unmap_attachment() to unmap that buffer from its > own iommu table. > But now vb2's unmap_dma_buf callback is nothing to do. I think that the > reason is the below issue, > > If qbuf maps buffer with iomm table and dqbuf unmaps it from iommu table > then it has performance deterioration because qbuf and dqbuf are called > repeatedly. > And this means map/unmap are repeated also. So I think media guys moved > dma_unmap_sg call from its own unmap_dma_buf callback to detach callback > instead. > For this, you can refer to vb2_dc_dmabuf_ops_unmap and > vb2_dc_dmabuf_ops_detach function. > > So I added the below patch to avoid that performance deterioration and > am testing it now.(this patch is derived from videobuf2-dma-contig.c) > http://git.kernel.org/?p=linux/kernel/git/daeinki/drm-exynos.git;a=commit;h=576b1c3de8b90cf1570b8418b60afd1edaae4e30 > > Thus, I'm not sure that your common set could cover all the cases > including other frameworks. Please give me any opinions. It seems like this adjustment would make perfect sense to add to the helper layer I suggested. E.g., instead of having an exynos_attach structure that caches the sgt, there'd be a struct drm_gem_prime_attach that would do the same thing, and save the sgt it gets from driver->gem_prime_get_sg. Then it would benefit nouveau and radeon, too. Alternatively, patch #4 could be dropped and Exynos can continue to reimplement all of this core functionality, since the helpers are optional, but I don't see anything about this change that should make it Exynos-specific, unless I'm missing something. -- Aaron
[PATCH 1/4] drm: add prime helpers
On 12/06/2012 01:46 PM, Daniel Vetter wrote: > On Thu, Dec 06, 2012 at 10:07:48AM -0800, Aaron Plattner wrote: >> 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_pages: convert a drm_gem_object to an sg_table for export >>gem_prime_import_sg: 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. >> >> Signed-off-by: Aaron Plattner > > A few comments: > > - Can you please add kerneldoc entries for these new helpers? Laurent >Pinchart started a nice drm kerneldoc as an overview. And since then >we've at least integrated the kerneldoc api reference at a nice place >there and brushed it up a bit when touching drm core or helpers in a >bigger patch series. See e.g. my recent dp helper series for what I'm >looking for. I think we should have kerneldoc at least for the exported >functions. Good call, I'll look into that. > - Just an idea for all the essential noop cpu helpers: Maybe just call >them dma_buf*noop and shovel them as convenience helpers into the >dma-buf.c code in the core, for drivers that don't want/can't/won't >bother to implement the cpu access support. Related: Exporting the >functions in general so that drivers could pick&choose Seems like a good idea as a follow-on change. Do you think it would make more sense to have noop helpers, or allow them to be NULL in dma-buf.c? > - s/gem_prime_get_pages/gem_prime_get_sg/ drm/i915 now uses sg_tables >everywhere to manage backing storage, and we're starting to use the >stolen memory, too. Stolen memory is not struct page backed, so better >make this clear in the function calls. I know that the current >prime/dma_buf code from Dave doesn't really work too well (*cough*) if >the backing storage isn't struct page backed, at least on the ttm import >side. This also links in with my 2nd comment above - dma_buf requires >that exporter map the sg into the importers device space to support fake >subdevices which share the exporters iommu/gart, hence such incetious >exporter might want to overwrite some of the functions. Will do in a v2 set. > - To make it a first-class helper and remove any and all midlayer stints >from this (I truly loath midlayers ...) maybe shovel this into a >drm_prime_helper.c file. Also, adding functions to the core vtable for >pure helpers is usually not too neat, since it makes it way too damn >easy to mix things up and transform the helper into a midlayer by >accident. E.g. the crtc helper functions all just use a generic void * >pointer for their vtables, other helpers either subclass an existing >struct or use their completely independent structs (which the driver can >both embed into it's own object struct and then do the right upclassing >in the callbacks). tbh haven't looked to closely at the series to asses >what would make sense, but we have way too much gunk in the drm vtable >already ... I'm not sure I fully understand what you mean by this. Are you suggesting creating a separate struct for these functions and having a void* pointer to that in struct drm_driver? > Dunno whether this aligns with what you want to achieve here. But on a > quick hunch this code feels rather unflexible and just refactors the > identical copypasta code back into one copy. Anyway, just figured I'll > drop my 2 cents here, feel free to ignore (maybe safe for the last one). I think uncopypasta-ing the current code is beneficial on its own. Do you think doing this now would make it harder to do the further refactoring you suggest later? -- Aaron
[PATCH 1/4] drm: add prime helpers
On 12/07/2012 10:48 AM, Daniel Vetter wrote:
> On Fri, Dec 07, 2012 at 09:58:38AM -0800, Aaron Plattner wrote:
>> On 12/06/2012 01:46 PM, Daniel Vetter wrote:
>>> - To make it a first-class helper and remove any and all midlayer stints
>>>from this (I truly loath midlayers ...) maybe shovel this into a
>>>drm_prime_helper.c file. Also, adding functions to the core vtable for
>>>pure helpers is usually not too neat, since it makes it way too damn
>>>easy to mix things up and transform the helper into a midlayer by
>>>accident. E.g. the crtc helper functions all just use a generic void *
>>>pointer for their vtables, other helpers either subclass an existing
>>>struct or use their completely independent structs (which the driver can
>>>both embed into it's own object struct and then do the right upclassing
>>>in the callbacks). tbh haven't looked to closely at the series to asses
>>>what would make sense, but we have way too much gunk in the drm vtable
>>>already ...
>>
>> I'm not sure I fully understand what you mean by this. Are you
>> suggesting creating a separate struct for these functions and having a
>> void* pointer to that in struct drm_driver?
>
> Create a new struct like
>
> struct drm_prime_helper_funcs {
> int (*gem_prime_pin)(struct drm_gem_object *obj);
> struct sg_table *(*gem_prime_get_pages)(struct drm_gem_object *obj);
> struct drm_gem_object *(*gem_prime_import_sg)(struct drm_device *dev,
> size_t size, struct sg_table *sg);
> void *(*gem_prime_vmap)(struct drm_gem_object *obj);
> void (*gem_prime_vunmap)(struct drm_gem_object *obj);
> }
>
> struct drm_prime_helper_obj {
> /* vmap information */
> int vmapping_count;
> void *vmapping_ptr;
>
> /* any other data the helpers need ... */
> struct drm_prime_helper_funcs *funcs;
> }
Ah, I see what you mean. This would also need either a pointer to the
drv directly so the helpers can lock drv->struct_mutex, or a helper
function to convert from a drm_prime_helper_obj* to a gem_buffer_object*
in a driver-specific way since the helper doesn't know the layout of the
driver's bo structure.
So it would be something like
struct drm_prime_helper_obj *helper_obj = dma_buf->priv;
struct drm_prime_helper_funcs *funcs = helper_obj->funcs;
struct drm_gem_object *obj = funcs->get_gem_obj(helper_obj);
mutex_lock(&obj->dev->struct_mutex);
funcs->whatever(obj);
mutex_unlock&obj->dev->struct_mutex);
and then for example, radeon_drm_prime_get_gem_obj would be
struct radeon_bo *bo = container_of(helper_obj, struct radeon_bo,
prime_helper);
return &bo->gem_base;
?
That seems a little over-engineered to me, but I can code it up.
--
Aaron
> Drivers then fill out a func vtable and embedded the helper obj into their
> own gpu bo struct, i.e.
>
> struct radeon_bo {
> struct ttm_bo ttm_bo;
> struct gem_buffer_object gem_bo;
> struct drm_prime_helper_obj prime_bo;
>
> /* other stuff */
> }
>
> In the prime helper callbacks in the driver then upcast the prime_bo to
> the radeon_bo instead of the gem_bo to the radeon bo.
>
> Upsides: Guaranteed to not interfere with drivers not using it, with an
> imo higher chance that the helper is cleanly separate from core stuff (no
> guarantee ofc) and so easier to refactor in the future. And drm is full
> with legacy stuff used by very few drivers, but highly intermingled with
> drm core used by other drivers (my little holy war here is to slowly
> refactor those things out and shovel them into drivers), hence why I
> prefer to have an as clean separation of optional stuff as possible ...
>
> Also, this way allows different drivers to use completely different helper
> libraries for the same task, without those two helper libraries ever
> interfering.
>
> Downside: Adds a pointer to each bo struct for drivers using the helpers.
> Given how big those tend to be, not too onerous. Given And maybe a bit of
> confusion in driver callbacks, but since the linux container_of is
> typechecked by the compiler, not too onerous either.
>
>>> Dunno whether this aligns with what you want to achieve here. But on a
>>> quick hunch this code feels rather unflexible and just refactors the
>>> identical copypasta code back into one copy. Anyway, just figured I'll
>>> drop my 2 cents here, feel free to ignore (maybe safe for the last one).
>>
>> I think uncopypasta-ing the current code is beneficial on its own.
>> Do you think doing this now would make it harder to do the further
>> refactoring you suggest later?
>
> If we later on go with something like the above, it'll require going over
> all drivers again, doing similar mechanic changes. If no one yet managed
> to intermingle the helpers with the core in the meantime, that is ;-)
>
> Cheers, Daniel
>
[PATCH 1/4] drm: add prime helpers
On 12/07/2012 01:38 PM, Daniel Vetter wrote: > On Fri, Dec 7, 2012 at 9:33 PM, Aaron Plattner > wrote: >> Ah, I see what you mean. This would also need either a pointer to the drv >> directly so the helpers can lock drv->struct_mutex, or a helper function to >> convert from a drm_prime_helper_obj* to a gem_buffer_object* in a >> driver-specific way since the helper doesn't know the layout of the driver's >> bo structure. > > Ah, locking, and it involves dev->struct_mutex. Another pet peeve of > mine ;-) Tbh I haven't looked at locking issues when stitching > together my quick proposal. > > The problem with the dev->struct_mutex lock is that it's everywhere > and hence it's way to easy to create deadlocks with it. Especially > with prime, where a simple rule like "take this lock first, always" > are suddenly really more complicated since gem drivers can assume both > the importer and exporter role ... I haven't done a full locking > review, but I expect current prime to deadlock (through driver calls) > when userspace starts doing funny things. > > So imo it's best to move dev->struct_mutex as far away as possible > from helper functions and think really hard whether we really need it. > I've noticed three functions: > > drm_gem_map_dma_buf: We can remove the locking here imo, if drivers > need dev->struct_mutex for internal consistency, they can take it in > their callbacks. The dma_buf_attachment is very much like a fancy sg > list + backing storage pointer. If multiple callers concurrently > interact with the same attachment, havoc might ensue. Importers should > add their own locking if concurrent access is possible (e.g. similar > to how filling in the same sg table concurrently and then calling > dma_map_sg concurrently would lead to chaos). Otoh creating/destroying > attachments with attach/detach is protected by the dma_buf->lock. > > I've checked dma-buf docs, and that this is not specced in the docs, > but was very much the intention. So I think we can solve this with a > simple doc update ;-) > > drm_gem_dmabuf_v(un)map: Beside that drivers might need to lock for > internal consistency the lock only protects the vmapping_counter and > pointer and avoids that we race concurrent vmap/vunmap calls to the > driver. Now vmap/vunmap is very much like an attachment, just that we > don't have an explicit struct for each client since there's only one > cpu address space. > > So pretty much every driver is forced to reinvent vmap refcounting, > which feels like an interface bug. What about moving this code to the > dma-buf.c interface shim and protecting it with dma_buf->lock? Atm we > only take that lock for attach/detach, and drivers using vmap place > the vmap call at the same spot hw drivers would place the attach call, > so this shouldn't introduce any nefarious locking issues. So I think > this would be the right way to move forward. > > And with that we've weaned the prime helpers off their dependency of > dev->struct_mutex, which should make them a notch more flexible and so > useful (since fighting locking inversions is a royal pain best > avoided). > > Comments? This all sounds good, but it's getting well outside the realm of what I'm comfortable doing in the short term as a kernel noob. Would you object to going with a version of these changes that leaves the new hooks where they are now and then applying these suggestions later? I don't think the risk of the helpers turning into a required mid-layer is very high given that i915 won't use them and it's one of the most-tested drivers. -- Aaron
[PATCH] [RFC] dma-buf: implement vmap refcounting in the interface logic
On 12/17/2012 03:58 PM, Daniel Vetter wrote:
> All drivers which implement this need to have some sort of refcount to
> allow concurrent vmap usage. Hence implement this in the dma-buf core.
>
> To protect against concurrent calls we need a lock, which potentially
> causes new funny locking inversions. But this shouldn't be a problem
> for exporters with statically allocated backing storage, and more
> dynamic drivers have decent issues already anyway.
>
> Inspired by some refactoring patches from Aaron Plattner, who
> implemented the same idea, but only for drm/prime drivers.
>
> v2: Check in dma_buf_release that no dangling vmaps are left.
> Suggested by Aaron Plattner. We might want to do similar checks for
> attachments, but that's for another patch. Also fix up ERR_PTR return
> for vmap.
>
> v3: Check whether the passed-in vmap address matches with the cached
> one for vunmap. Eventually we might want to remove that parameter -
> compared to the kmap functions there's no need for the vaddr for
> unmapping. Suggested by Chris Wilson.
>
> Cc: Aaron Plattner
> Signed-off-by: Daniel Vetter
> ---
> Compile-tested only - Aaron has been bugging me too a bit too often
> about this on irc.
>
> Cheers, Daniel
> ---
> Documentation/dma-buf-sharing.txt | 6 +-
> drivers/base/dma-buf.c| 43
> ++-
> include/linux/dma-buf.h | 4 +++-
> 3 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt
> b/Documentation/dma-buf-sharing.txt
> index 0188903..4966b1b 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -302,7 +302,11 @@ Access to a dma_buf from the kernel context involves
> three steps:
> void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
>
> The vmap call can fail if there is no vmap support in the exporter, or
> if it
> - runs out of vmalloc space. Fallback to kmap should be implemented.
> + runs out of vmalloc space. Fallback to kmap should be implemented. Note
> that
> + the dma-buf layer keeps a reference count for all vmap access and calls
> down
> + into the exporter's vmap function only when no vmapping exists, and only
> + unmaps it once. Protection against concurrent vmap/vunmap calls is
> provided
> + by taking the dma_buf->lock mutex.
>
> 3. Finish access
>
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index a3f79c4..67d3cd5 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -39,6 +39,8 @@ static int dma_buf_release(struct inode *inode, struct file
> *file)
>
> dmabuf = file->private_data;
>
> + BUG_ON(dmabuf->vmapping_counter);
> +
> dmabuf->ops->release(dmabuf);
> kfree(dmabuf);
> return 0;
> @@ -482,12 +484,34 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
>*/
> void *dma_buf_vmap(struct dma_buf *dmabuf)
> {
> + void *ptr;
> +
> if (WARN_ON(!dmabuf))
> return NULL;
>
> - if (dmabuf->ops->vmap)
> - return dmabuf->ops->vmap(dmabuf);
> - return NULL;
> + if (!dmabuf->ops->vmap)
> + return NULL;
> +
> + mutex_lock(&dmabuf->lock);
> + if (dmabuf->vmapping_counter) {
> + dmabuf->vmapping_counter++;
> + BUG_ON(!dmabuf->vmap_ptr);
> + ptr = dmabuf->vmap_ptr;
> + goto out_unlock;
> + }
> +
> + BUG_ON(dmabuf->vmap_ptr);
> +
> + ptr = dmabuf->ops->vmap(dmabuf);
> + if (IS_ERR_OR_NULL(ptr))
> + goto out_unlock;
> +
> + dmabuf->vmap_ptr = ptr;
> + dmabuf->vmapping_counter = 1;
> +
> +out_unlock:
> + mutex_unlock(&dmabuf->lock);
> + return ptr;
> }
> EXPORT_SYMBOL_GPL(dma_buf_vmap);
>
> @@ -501,7 +525,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
> if (WARN_ON(!dmabuf))
> return;
>
> - if (dmabuf->ops->vunmap)
> - dmabuf->ops->vunmap(dmabuf, vaddr);
> + BUG_ON(!dmabuf->vmap_ptr);
> + BUG_ON(dmabuf->vmapping_counter > 0);
This should be BUG_ON(vmapping_counter == 0);
Otherwise, this seems to work fine.
--
Aaron
> + BUG_ON(dmabuf->vmap_ptr != vaddr);
> +
> + mutex_lock(&dmabuf->lock);
> + if (--dmabuf->vmapping_counter == 0) {
> + if (dmabuf->ops->vunmap)
> + dmabuf->ops->vunmap(dmabuf, vaddr);
> + dmabuf->vmap_ptr = NULL;
> + }
> + m
[PATCH] [RFC] dma-buf: implement vmap refcounting in the interface logic
On 12/20/2012 05:14 AM, Daniel Vetter wrote:
> All drivers which implement this need to have some sort of refcount to
> allow concurrent vmap usage. Hence implement this in the dma-buf core.
>
> To protect against concurrent calls we need a lock, which potentially
> causes new funny locking inversions. But this shouldn't be a problem
> for exporters with statically allocated backing storage, and more
> dynamic drivers have decent issues already anyway.
>
> Inspired by some refactoring patches from Aaron Plattner, who
> implemented the same idea, but only for drm/prime drivers.
>
> v2: Check in dma_buf_release that no dangling vmaps are left.
> Suggested by Aaron Plattner. We might want to do similar checks for
> attachments, but that's for another patch. Also fix up ERR_PTR return
> for vmap.
>
> v3: Check whether the passed-in vmap address matches with the cached
> one for vunmap. Eventually we might want to remove that parameter -
> compared to the kmap functions there's no need for the vaddr for
> unmapping. Suggested by Chris Wilson.
>
> v4: Fix a brown-paper-bag bug spotted by Aaron Plattner.
The kernel spotted it when I tried to vunmap a buffer. :)
> Cc: Aaron Plattner
> Signed-off-by: Daniel Vetter
Reviewed-by: Aaron Plattner
Tested-by: Aaron Plattner
--
Aaron
> ---
> Documentation/dma-buf-sharing.txt | 6 +-
> drivers/base/dma-buf.c| 43
> ++-
> include/linux/dma-buf.h | 4 +++-
> 3 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/dma-buf-sharing.txt
> b/Documentation/dma-buf-sharing.txt
> index 0188903..4966b1b 100644
> --- a/Documentation/dma-buf-sharing.txt
> +++ b/Documentation/dma-buf-sharing.txt
> @@ -302,7 +302,11 @@ Access to a dma_buf from the kernel context involves
> three steps:
> void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
>
> The vmap call can fail if there is no vmap support in the exporter, or
> if it
> - runs out of vmalloc space. Fallback to kmap should be implemented.
> + runs out of vmalloc space. Fallback to kmap should be implemented. Note
> that
> + the dma-buf layer keeps a reference count for all vmap access and calls
> down
> + into the exporter's vmap function only when no vmapping exists, and only
> + unmaps it once. Protection against concurrent vmap/vunmap calls is
> provided
> + by taking the dma_buf->lock mutex.
>
> 3. Finish access
>
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index a3f79c4..26b68de 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -39,6 +39,8 @@ static int dma_buf_release(struct inode *inode, struct file
> *file)
>
> dmabuf = file->private_data;
>
> + BUG_ON(dmabuf->vmapping_counter);
> +
> dmabuf->ops->release(dmabuf);
> kfree(dmabuf);
> return 0;
> @@ -482,12 +484,34 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap);
>*/
> void *dma_buf_vmap(struct dma_buf *dmabuf)
> {
> + void *ptr;
> +
> if (WARN_ON(!dmabuf))
> return NULL;
>
> - if (dmabuf->ops->vmap)
> - return dmabuf->ops->vmap(dmabuf);
> - return NULL;
> + if (!dmabuf->ops->vmap)
> + return NULL;
> +
> + mutex_lock(&dmabuf->lock);
> + if (dmabuf->vmapping_counter) {
> + dmabuf->vmapping_counter++;
> + BUG_ON(!dmabuf->vmap_ptr);
> + ptr = dmabuf->vmap_ptr;
> + goto out_unlock;
> + }
> +
> + BUG_ON(dmabuf->vmap_ptr);
> +
> + ptr = dmabuf->ops->vmap(dmabuf);
> + if (IS_ERR_OR_NULL(ptr))
> + goto out_unlock;
> +
> + dmabuf->vmap_ptr = ptr;
> + dmabuf->vmapping_counter = 1;
> +
> +out_unlock:
> + mutex_unlock(&dmabuf->lock);
> + return ptr;
> }
> EXPORT_SYMBOL_GPL(dma_buf_vmap);
>
> @@ -501,7 +525,16 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, void *vaddr)
> if (WARN_ON(!dmabuf))
> return;
>
> - if (dmabuf->ops->vunmap)
> - dmabuf->ops->vunmap(dmabuf, vaddr);
> + BUG_ON(!dmabuf->vmap_ptr);
> + BUG_ON(dmabuf->vmapping_counter == 0);
> + BUG_ON(dmabuf->vmap_ptr != vaddr);
> +
> + mutex_lock(&dmabuf->lock);
> + if (--dmabuf->vmapping_counter == 0) {
> + if (dmabuf->ops->vunmap)
> + dmabuf->ops->vunmap(dmabuf, vaddr);
> + dmabuf->vmap_ptr = NULL;
> + }
> + mutex_unlock(&dmabuf->lock);
> }
[patch] drm: use after free in drm_pci_exit()
On 01/20/2014 02:31 AM, Dan Carpenter wrote:
> We can't use "dev" after we freed it on the line before.
>
> Fixes: b3f2333de8e8 ('drm: restrict the device list for shadow attached
> drivers')
> Signed-off-by: Dan Carpenter
I just ran into this same problem, and this change fixes it.
Tested-by: Aaron Plattner
Reviewed-by: Aaron Plattner
and since he just sent me an independently-developed identical change,
Reviewed-by: John Hubbard
> diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
> index 5736aaa7e86c..f7af69bcf3f4 100644
> --- a/drivers/gpu/drm/drm_pci.c
> +++ b/drivers/gpu/drm/drm_pci.c
> @@ -468,8 +468,8 @@ void drm_pci_exit(struct drm_driver *driver, struct
> pci_driver *pdriver)
> } else {
> list_for_each_entry_safe(dev, tmp, &driver->legacy_dev_list,
>legacy_dev_list) {
> - drm_put_dev(dev);
> list_del(&dev->legacy_dev_list);
> + drm_put_dev(dev);
> }
> }
> DRM_INFO("Module unloaded\n");
> --
[RFC] drm + i915 DP MST code preview
On 05/03/2014 02:00 AM, Chris Wilson wrote: > On Sat, May 03, 2014 at 07:08:02AM +1000, Dave Airlie wrote: >> On 2 May 2014 18:52, Chris Wilson wrote: >>> On Fri, May 02, 2014 at 02:39:37PM +1000, Dave Airlie wrote: >> the GUID is only on DP 1.2 devices, so you don't get one for ever >> port, also GUIDs are wiped on powerdown on most devices, default GUID >> is 0 except where devices have USB hubs as well, so it probably >> doesn't make much sense to bother exposing them directly. > > Ok. It looks like if we do attempt to maintain persistent naming, we need > to do it in the kernel anyway. That is to make sure that a downstream > device always has the same type-id upon reconnection - at least for the > lifetime of module. Or maybe the output name is irrelevant for > preserving extended desktop configurations? Dunno if it helps, but for roughly similar reasons we ended up naming the outputs based on their topology paths in the NVIDIA driver. So for example a port named DP-3 that has a Dell UP2414Q attached will show up as two outputs named DP-3.1 and DP-3.8 since its internal bridge uses downstream ports 1 and 8. This has worked out fairly well in practice. Here's how I described it in the README: When DisplayPort 1.2 branch devices are present, display devices will be created with type- and connector-based names that are based on how they are connected to the branch device tree. For example, if a connector named DP-2 has a branch device attached and a DisplayPort device is connected to the branch device's first downstream port, a display device named DP-2.1 might be created. If another branch device is connected between the first branch device and the display device, the name might be DP-2.1.1. To avoid cluttering the output list, DisplayPort 1.2 devices can be deleted when they are no longer connected and are not named in any MetaModes. This behavior can be enabled with the DeleteUnusedDP12Displays option. http://us.download.nvidia.com/XFree86/Linux-x86/337.19/README/displaydevicenames.html -- Aaron
[RFC] drm + i915 DP MST code preview
On 05/07/2014 02:00 AM, Dave Airlie wrote: > On 7 May 2014 17:16, Aaron Plattner wrote: >> On 05/03/2014 02:00 AM, Chris Wilson wrote: >>> >>> On Sat, May 03, 2014 at 07:08:02AM +1000, Dave Airlie wrote: >>>> >>>> On 2 May 2014 18:52, Chris Wilson wrote: >>>>> >>>>> On Fri, May 02, 2014 at 02:39:37PM +1000, Dave Airlie wrote: >>>> >>>> the GUID is only on DP 1.2 devices, so you don't get one for ever >>>> port, also GUIDs are wiped on powerdown on most devices, default GUID >>>> is 0 except where devices have USB hubs as well, so it probably >>>> doesn't make much sense to bother exposing them directly. >>> >>> >>> Ok. It looks like if we do attempt to maintain persistent naming, we need >>> to do it in the kernel anyway. That is to make sure that a downstream >>> device always has the same type-id upon reconnection - at least for the >>> lifetime of module. Or maybe the output name is irrelevant for >>> preserving extended desktop configurations? >> >> >> Dunno if it helps, but for roughly similar reasons we ended up naming the >> outputs based on their topology paths in the NVIDIA driver. So for example >> a port named DP-3 that has a Dell UP2414Q attached will show up as two >> outputs named DP-3.1 and DP-3.8 since its internal bridge uses downstream >> ports 1 and 8. This has worked out fairly well in practice. >> >> Here's how I described it in the README: >> >> When DisplayPort 1.2 branch devices are present, display >> devices will be created with type- and connector-based names >> that are based on how they are connected to the branch device >> tree. For example, if a connector named DP-2 has a branch >> device attached and a DisplayPort device is connected to the >> branch device's first downstream port, a display device named >> DP-2.1 might be created. If another branch device is >> connected between the first branch device and the display >> device, the name might be DP-2.1.1. >> >> To avoid cluttering the output list, DisplayPort 1.2 devices >> can be deleted when they are no longer connected and are not >> named in any MetaModes. This behavior can be enabled with the >> DeleteUnusedDP12Displays option. >> >> http://us.download.nvidia.com/XFree86/Linux-x86/337.19/README/displaydevicenames.html > > Is the cleaning up an option because it caused some problems? > I'm seeing some gnome-settings-daemon, gnome-shell crash because the > XIDs are gone away and they get X errors, just wondering if this is > what you were seeing, Yeah, this exactly. RandR's timestamp mechanism doesn't help you avoid BadOutput errors, although we could probably extend the protocol to add that, if we decide it's necessary. Since the topology-based IDs are relatively stable unless you get a whole pile of branch devices and swap them around in weird ways all day, it was easier to just leave them around. They'll get reused if a device with that topology path ever comes back. > Dave.
[RFC] drm + i915 DP MST code preview
On 05/07/2014 01:07 AM, Daniel Vetter wrote: > On Wed, May 07, 2014 at 12:16:37AM -0700, Aaron Plattner wrote: >> On 05/03/2014 02:00 AM, Chris Wilson wrote: >>> On Sat, May 03, 2014 at 07:08:02AM +1000, Dave Airlie wrote: >>>> On 2 May 2014 18:52, Chris Wilson wrote: >>>>> On Fri, May 02, 2014 at 02:39:37PM +1000, Dave Airlie wrote: >>>> the GUID is only on DP 1.2 devices, so you don't get one for ever >>>> port, also GUIDs are wiped on powerdown on most devices, default GUID >>>> is 0 except where devices have USB hubs as well, so it probably >>>> doesn't make much sense to bother exposing them directly. >>> >>> Ok. It looks like if we do attempt to maintain persistent naming, we need >>> to do it in the kernel anyway. That is to make sure that a downstream >>> device always has the same type-id upon reconnection - at least for the >>> lifetime of module. Or maybe the output name is irrelevant for >>> preserving extended desktop configurations? >> >> Dunno if it helps, but for roughly similar reasons we ended up naming the >> outputs based on their topology paths in the NVIDIA driver. So for example >> a port named DP-3 that has a Dell UP2414Q attached will show up as two >> outputs named DP-3.1 and DP-3.8 since its internal bridge uses downstream >> ports 1 and 8. This has worked out fairly well in practice. >> >> Here's how I described it in the README: >> >> When DisplayPort 1.2 branch devices are present, display >> devices will be created with type- and connector-based names >> that are based on how they are connected to the branch device >> tree. For example, if a connector named DP-2 has a branch >> device attached and a DisplayPort device is connected to the >> branch device's first downstream port, a display device named >> DP-2.1 might be created. If another branch device is >> connected between the first branch device and the display >> device, the name might be DP-2.1.1. >> >> To avoid cluttering the output list, DisplayPort 1.2 devices >> can be deleted when they are no longer connected and are not >> named in any MetaModes. This behavior can be enabled with the >> DeleteUnusedDP12Displays option. >> >> http://us.download.nvidia.com/XFree86/Linux-x86/337.19/README/displaydevicenames.html > > I'm unclear how you name non-DP branch devices. Do they show up as > DP-2.1-HDMI or something similar? Or do you just keep the DP-2.1 name but > set the connector type to HDMI? I don't have any branches that support passive adapters on downstream ports, so I'm not sure what happens with those. > In general I think your prosoal of adding branch downstream ports to name > MST connectors sounds really good. The kms object ids will still be > random, but at least if users connect the same topology to the same ports > we should have stable names. Yeah, the RandR output XIDs work the same way. > And of course userspace can still check the EDID serial. > -Daniel
[PATCH v3 1/3] drm: add prime helpers
On 04/12/13 07:58, Daniel Vetter wrote:
> On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
>> 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 be8a42ae60addd8b6092535c11b42d099d6470ec
>>("drm/prime: drop reference on imported dma-buf come from gem")
>>
>> Signed-off-by: Aaron Plattner
>> Cc: Daniel Vetter
>> Cc: David Airlie
>> ---
>> Documentation/DocBook/drm.tmpl | 4 +
>> drivers/gpu/drm/drm_prime.c| 186
>> -
>> include/drm/drmP.h | 12 +++
>> 3 files changed, 201 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> index 4ee2304..ed40576 100644
>> --- a/Documentation/DocBook/drm.tmpl
>> +++ b/Documentation/DocBook/drm.tmpl
>> @@ -743,6 +743,10 @@ char *date;
>> These two operations are mandatory for GEM drivers that support
>> DRM
>> PRIME.
>>
>> +
>> + DRM PRIME Helper Functions Reference
>> +!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
>> +
>>
>>
>> GEM Objects Mapping
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 7f12573..366910d 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -53,7 +53,8 @@
>>* Self-importing: if userspace is using PRIME as a replacement for flink
>>* then it will get a fd->handle request for a GEM object that it created.
>>* Drivers should detect this situation and return back the gem object
>> - * from the dma-buf private.
>> + * from the dma-buf private. Prime will do this automatically for drivers
>> that
>> + * use the drm_gem_prime_{import,export} helpers.
>>*/
>>
>> struct drm_prime_member {
>> @@ -62,6 +63,137 @@ struct drm_prime_member {
>>uint32_t handle;
>> };
>>
>> +static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment
>> *attach,
>> + enum dma_data_direction dir)
>> +{
>> + struct drm_gem_object *obj = attach->dmabuf->priv;
>> + struct sg_table *sgt;
>> +
>> + mutex_lock(&obj->dev->struct_mutex);
>> +
>> + sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> +
>> + if (!IS_ERR_OR_NULL(sgt))
>> + dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +
>> + mutex_unlock(&obj->dev->struct_mutex);
>> + return sgt;
>> +}
>> +
>> +static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> + struct sg_table *sgt, enum dma_data_direction dir)
>>
tile property contents
On 10/13/2014 08:23 PM, Dave Airlie wrote: > Hi, > > So I've been hacking on mutter and the gnome pieces for tiling, and > I've at least fixed mutter locally so maximise windows works and the > heads are in the right order. > > Now I've strung all the pieces together using a single KMS property > that X.org propogates, and mutter picks up and propagates over dbus as > well, > > Currently I've ascii encoded the property into a blob, > > > > I'm thinking of dropping the version field and just exposing TILE2 > property if we need it later to add more values, Nifty. Is there a randrproto.txt spec for this planned? > The other fields: > tileid: a group id assigned by the kernel to all tiles in the same What format does this ID need to be in? It looks like monitors are identified by (vendor id, product id, serial number) tuples in the DisplayID extension block so it would make sense to just concatenate that into one giant number as the tileid. Having to centrally manage these (in the kernel??) seems like overkill. > group - unique per group > flags: bit 0 : single monitor enclosure > maxhtiles: total number of horiz tiles > maxvtiles: total number of vert tiles > h_tile_loc: horiz location of this output in tile group > v_tile_loc: vert location of this output in tile group > tile_w: width of this tile > tile_h: height of this tile. > > Now we extract all of these from the DisplayID v1.3 block, and I'm > wondering if maybe I shouldn't just export the whole DisplayID tiling > info block instead, it however encodes a few other pieces of > information, including bezel info, and some flags specifying behaviour > in some cases. I don't know whether the other flags would be useful, but one important one is the "native resolution" field. At least one monitor I've seen fails to work if you drive the normal EDID "preferred" timings on both tiles. > The former could be more suitable for cases where DisplayID isn't > available (Dual DSI panels?) but I'm worried abuot exposing too little > at this point making TILE useless when the next monitor comes out. > > I'm not sure any part of the stack should be extracting things and > splitting them out, I'd like to just give the same tile property all > the way through. > > Dave. > ___ > xorg-devel at lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel > -- Aaron
tile property contents
On 12/02/2014 07:04 PM, Dave Airlie wrote: > On 3 December 2014 at 10:01, Aaron Plattner wrote: >> On 10/13/2014 08:23 PM, Dave Airlie wrote: >>> >>> Hi, >>> >>> So I've been hacking on mutter and the gnome pieces for tiling, and >>> I've at least fixed mutter locally so maximise windows works and the >>> heads are in the right order. >>> >>> Now I've strung all the pieces together using a single KMS property >>> that X.org propogates, and mutter picks up and propagates over dbus as >>> well, >>> >>> Currently I've ascii encoded the property into a blob, >>> >>> >>> >>> >>> I'm thinking of dropping the version field and just exposing TILE2 >>> property if we need it later to add more values, >> >> >> Nifty. Is there a randrproto.txt spec for this planned? >> > > Well for KMS its a kernel property and is documented there, > I suppose randrproto should also contain the info. >> >> What format does this ID need to be in? It looks like monitors are >> identified by (vendor id, product id, serial number) tuples in the DisplayID >> extension block so it would make sense to just concatenate that into one >> giant number as the tileid. Having to centrally manage these (in the >> kernel??) seems like overkill. > > I don't mind, but central management is what we've done, it wasn't a lot > of work, you could munge the vendor/product/serial, but maybe > DisplayId won't be the only game in town in the future and I'd hate > to tie things to it. Alright. At least for now, we can just centrally manage group IDs in our X driver until we move that stuff to the kernel. >>> group - unique per group >>> flags: bit 0 : single monitor enclosure >>> maxhtiles: total number of horiz tiles >>> maxvtiles: total number of vert tiles >>> h_tile_loc: horiz location of this output in tile group >>> v_tile_loc: vert location of this output in tile group >>> tile_w: width of this tile >>> tile_h: height of this tile. >>> >>> Now we extract all of these from the DisplayID v1.3 block, and I'm >>> wondering if maybe I shouldn't just export the whole DisplayID tiling >>> info block instead, it however encodes a few other pieces of >>> information, including bezel info, and some flags specifying behaviour >>> in some cases. >> >> >> I don't know whether the other flags would be useful, but one important one >> is the "native resolution" field. At least one monitor I've seen fails to >> work if you drive the normal EDID "preferred" timings on both tiles. > > I think the last two fields are copied from that, the tile w/h, I can't see > any mention of a specific native res flag. Oh, got it. I was confused by this because our DisplayID parsing library calls this field 'native_resolution'. -- Aaron
[PATCH libdrm resend] tests/dristat: add -C to pretty-print device capabilities
Signed-off-by: Aaron Plattner
---
Example output of dristat -C:
/dev/dri/card0
Device capabilities:
Dumb framebuffer: yes
VBlank high crtc: yes
Preferred depth: 24
Prefer shadow: yes
Prime: import export
tests/dristat.c | 69 -
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/tests/dristat.c b/tests/dristat.c
index 900a3e6..d36c3de 100644
--- a/tests/dristat.c
+++ b/tests/dristat.c
@@ -24,9 +24,11 @@
* DEALINGS IN THE SOFTWARE.
*
* Authors: Rickard E. (Rik) Faith
+ * Authors: Aaron Plattner
*
*/
+#include
#include
#include
#include
@@ -35,11 +37,14 @@
#include "xf86drmHash.c"
#include "xf86drm.c"
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
#define DRM_VERSION 0x0001
#define DRM_MEMORY 0x0002
#define DRM_CLIENTS 0x0004
#define DRM_STATS 0x0008
#define DRM_BUSID 0x0010
+#define DRM_CAPS0x0020
static void getversion(int fd)
{
@@ -228,6 +233,65 @@ static void getstats(int fd, int i)
}
+enum cap_type {
+CAP_BOOL,
+CAP_UINT,
+CAP_PRIME
+};
+
+static void printcap(enum cap_type type, uint64_t value)
+{
+switch (type) {
+case CAP_BOOL:
+ if (value) printf("yes");
+ else printf("no");
+ break;
+case CAP_UINT:
+ printf("%" PRIu64, value);
+ break;
+case CAP_PRIME:
+ if (value == 0) printf("none");
+ else {
+ if (value & DRM_PRIME_CAP_IMPORT) printf("import ");
+ if (value & DRM_PRIME_CAP_EXPORT) printf("export");
+ }
+ break;
+}
+}
+
+static void getcaps(int fd)
+{
+const struct {
+ uint64_t capability;
+ enum cap_type type;
+ const char *name;
+} caps[] = {
+ { DRM_CAP_DUMB_BUFFER, CAP_BOOL, "Dumb framebuffer" },
+ { DRM_CAP_VBLANK_HIGH_CRTC, CAP_BOOL, "VBlank high crtc" },
+ { DRM_CAP_DUMB_PREFERRED_DEPTH, CAP_UINT, "Preferred depth" },
+ { DRM_CAP_DUMB_PREFER_SHADOW, CAP_BOOL, "Prefer shadow" },
+ { DRM_CAP_PRIME,CAP_PRIME, "Prime" },
+};
+int i;
+
+printf(" Device capabilities:\n");
+
+for (i = 0; i < ARRAY_SIZE(caps); i++) {
+ uint64_t value;
+ int ret = drmGetCap(fd, caps[i].capability, &value);
+
+ printf("%s: ", caps[i].name);
+
+ if (ret) {
+ printf("\n");
+ continue;
+ }
+
+ printcap(caps[i].type, value);
+ printf("\n");
+}
+}
+
int main(int argc, char **argv)
{
int c;
@@ -238,7 +302,7 @@ int main(int argc, char **argv)
char buf[64];
int i;
-while ((c = getopt(argc, argv, "avmcsbM:i:")) != EOF)
+while ((c = getopt(argc, argv, "avmcCsbM:i:")) != EOF)
switch (c) {
case 'a': mask = ~0; break;
case 'v': mask |= DRM_VERSION;break;
@@ -246,6 +310,7 @@ int main(int argc, char **argv)
case 'c': mask |= DRM_CLIENTS;break;
case 's': mask |= DRM_STATS; break;
case 'b': mask |= DRM_BUSID; break;
+ case 'C': mask |= DRM_CAPS; break;
case 'i': interval = strtol(optarg, NULL, 0); break;
case 'M': minor = strtol(optarg, NULL, 0);break;
default:
@@ -254,6 +319,7 @@ int main(int argc, char **argv)
fprintf( stderr, " -aShow all available information\n"
);
fprintf( stderr, " -bShow DRM bus ID's\n" );
fprintf( stderr, " -cDisplay information about DRM
clients\n" );
+ fprintf( stderr, " -CDisplay DRM device
capabilities\n" );
fprintf( stderr, " -i [interval] Continuously display statistics
every [interval] seconds\n" );
fprintf( stderr, " -vDisplay DRM module and card
version information\n" );
fprintf( stderr, " -mDisplay memory use information\n"
);
@@ -272,6 +338,7 @@ int main(int argc, char **argv)
if (mask & DRM_MEMORY) getvm(fd);
if (mask & DRM_CLIENTS) getclients(fd);
if (mask & DRM_STATS) getstats(fd, interval);
+ if (mask & DRM_CAPS)getcaps(fd);
close(fd);
}
}
--
1.9.2
[PATCH v3 0/3] Prime helpers
This series adds helper functions that abstract the core parts of .gem_prime_import and .gem_prime_export so that drivers don't have to worry about the low-level details. These helpers are optional. A driver can use them by plugging in drm_gem_prime_import and drm_gem_prime_export into the drm_driver structure, or it can bypass them by plugging in its own functions. The first patch adds these helpers, and the later patches switch three drivers over to using them. This version of the series addresses concerns raised by Daniel Vetter, and to leave the vmap locking and refcounting to the dma-buf core code. It also drops Exynos from the series since its driver diverged between when I first sent out this series and now. This series depends on commit 90b6e90cb03352a352015ca213ac9f4fab3308f3 of the for-next branch of git://git.linaro.org/people/sumitsemwal/linux-dma-buf Aaron Plattner (3): drm: add prime helpers drm/nouveau: use prime helpers drm/radeon: use prime helpers Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++- drivers/gpu/drm/nouveau/nouveau_bo.h| 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 173 - drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 21 ++-- drivers/gpu/drm/radeon/radeon_prime.c | 170 - include/drm/drmP.h | 12 +++ 11 files changed, 270 insertions(+), 319 deletions(-) -- 1.8.1.1
[PATCH v3 1/3] drm: add prime helpers
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 be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
Documentation/DocBook/drm.tmpl | 4 +
drivers/gpu/drm/drm_prime.c| 186 -
include/drm/drmP.h | 12 +++
3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 4ee2304..ed40576 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -743,6 +743,10 @@ char *date;
These two operations are mandatory for GEM drivers that support DRM
PRIME.
+
+ DRM PRIME Helper Functions Reference
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+
GEM Objects Mapping
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 7f12573..366910d 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -53,7 +53,8 @@
* Self-importing: if userspace is using PRIME as a replacement for flink
* then it will get a fd->handle request for a GEM object that it created.
* Drivers should detect this situation and return back the gem object
- * from the dma-buf private.
+ * from the dma-buf private. Prime will do this automatically for drivers that
+ * use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,137 @@ struct drm_prime_member {
uint32_t handle;
};
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction dir)
+{
+ struct drm_gem_object *obj = attach->dmabuf->priv;
+ struct sg_table *sgt;
+
+ mutex_lock(&obj->dev->struct_mutex);
+
+ sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
+
+ if (!IS_ERR_OR_NULL(sgt))
+ dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+
+ mutex_unlock(&obj->dev->struct_mutex);
+ return sgt;
+}
+
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
+ struct sg_table *sgt, enum dma_data_direction dir)
+{
+ dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
+ sg_free_table(sgt);
+ kfree(sgt);
+}
+
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+
+ if (obj->export_dma_buf == dma_buf) {
+ /* drop the reference on the export fd holds */
+ obj->export_dma_buf = NULL;
+ drm_gem_object_unreference_unlocked(obj);
+ }
+}
+
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ return dev->driver->gem_prime_vmap(obj);
+}
+
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
+{
+ struct drm_gem_object *obj = dma_buf->priv;
+ struct drm_device *dev = obj->dev;
+
+ dev->driver->gem_prime_vunmap(obj, vaddr);
+}
+
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
+ unsigned long page_num)
+{
+ return NULL;
+}
+
+st
[PATCH 2/3] drm/nouveau: use prime helpers
Simplify the Nouveau prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
v2: Rename functions to nouveau_gem_prime_get_sg_table and
nouveau_gem_prime_import_sg_table.
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
drivers/gpu/drm/nouveau/nouveau_bo.h| 1 -
drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +-
drivers/gpu/drm/nouveau/nouveau_gem.c | 2 -
drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +-
drivers/gpu/drm/nouveau/nouveau_prime.c | 173
5 files changed, 34 insertions(+), 161 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h
b/drivers/gpu/drm/nouveau/nouveau_bo.h
index 25ca379..836677a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.h
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.h
@@ -31,7 +31,6 @@ struct nouveau_bo {
int pin_refcnt;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
static inline struct nouveau_bo *
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 180a45e..5de1f9a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -645,8 +645,13 @@ driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = nouveau_gem_prime_export,
- .gem_prime_import = nouveau_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = nouveau_gem_prime_pin,
+ .gem_prime_get_sg_table = nouveau_gem_prime_get_sg_table,
+ .gem_prime_import_sg_table = nouveau_gem_prime_import_sg_table,
+ .gem_prime_vmap = nouveau_gem_prime_vmap,
+ .gem_prime_vunmap = nouveau_gem_prime_vunmap,
.gem_init_object = nouveau_gem_object_new,
.gem_free_object = nouveau_gem_object_del,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c
b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 8bf695c..24e0aab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -24,8 +24,6 @@
*
*/
-#include
-
#include
#include "nouveau_drm.h"
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h
b/drivers/gpu/drm/nouveau/nouveau_gem.h
index 5c10492..8d7a3f0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.h
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
@@ -35,9 +35,11 @@ extern int nouveau_gem_ioctl_cpu_fini(struct drm_device *,
void *,
extern int nouveau_gem_ioctl_info(struct drm_device *, void *,
struct drm_file *);
-extern struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj, int flags);
-extern struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+extern int nouveau_gem_prime_pin(struct drm_gem_object *);
+extern struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object
*);
+extern struct drm_gem_object *nouveau_gem_prime_import_sg_table(
+ struct drm_device *, size_t size, struct sg_table *);
+extern void *nouveau_gem_prime_vmap(struct drm_gem_object *);
+extern void nouveau_gem_prime_vunmap(struct drm_gem_object *, void *);
#endif
diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c
b/drivers/gpu/drm/nouveau/nouveau_prime.c
index b8e05ae..f53e108 100644
--- a/drivers/gpu/drm/nouveau/nouveau_prime.c
+++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
@@ -22,126 +22,42 @@
* Authors: Dave Airlie
*/
-#include
-
#include
#include "nouveau_drm.h"
#include "nouveau_gem.h"
-static struct sg_table *nouveau_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
- struct nouveau_bo *nvbo = attachment->dmabuf->priv;
- struct drm_device *dev = nvbo->gem->dev;
+ struct nouveau_bo *nvbo = nouveau_gem_object(obj);
int npages = nvbo->bo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void nouveau_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
- struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct nouveau_bo *nvbo = dma_buf->priv;
-
- if (nv
[PATCH 3/3] drm/radeon: use prime helpers
Simplify the Radeon prime implementation by using the default behavior provided
by drm_gem_prime_import and drm_gem_prime_export.
v2:
- Rename functions to radeon_gem_prime_get_sg_table and
radeon_gem_prime_import_sg_table.
- Delete the now-unused vmapping_count variable.
Signed-off-by: Aaron Plattner
Cc: Daniel Vetter
Cc: David Airlie
---
drivers/gpu/drm/radeon/radeon.h | 1 -
drivers/gpu/drm/radeon/radeon_drv.c | 21 +++--
drivers/gpu/drm/radeon/radeon_prime.c | 170 +-
3 files changed, 35 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 34e5230..48bb80e 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -342,7 +342,6 @@ struct radeon_bo {
struct drm_gem_object gem_base;
struct ttm_bo_kmap_obj dma_buf_vmap;
- int vmapping_count;
};
#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon/radeon_drv.c
index dff6cf7..7a63817 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -117,11 +117,13 @@ int radeon_mode_dumb_create(struct drm_file *file_priv,
int radeon_mode_dumb_destroy(struct drm_file *file_priv,
struct drm_device *dev,
uint32_t handle);
-struct dma_buf *radeon_gem_prime_export(struct drm_device *dev,
- struct drm_gem_object *obj,
- int flags);
-struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
- struct dma_buf *dma_buf);
+struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj);
+struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev,
+ size_t size,
+ struct sg_table *sg);
+int radeon_gem_prime_pin(struct drm_gem_object *obj);
+void *radeon_gem_prime_vmap(struct drm_gem_object *obj);
+void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
#if defined(CONFIG_DEBUG_FS)
int radeon_debugfs_init(struct drm_minor *minor);
@@ -396,8 +398,13 @@ static struct drm_driver kms_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
- .gem_prime_export = radeon_gem_prime_export,
- .gem_prime_import = radeon_gem_prime_import,
+ .gem_prime_export = drm_gem_prime_export,
+ .gem_prime_import = drm_gem_prime_import,
+ .gem_prime_pin = radeon_gem_prime_pin,
+ .gem_prime_get_sg_table = radeon_gem_prime_get_sg_table,
+ .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table,
+ .gem_prime_vmap = radeon_gem_prime_vmap,
+ .gem_prime_vunmap = radeon_gem_prime_vunmap,
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
diff --git a/drivers/gpu/drm/radeon/radeon_prime.c
b/drivers/gpu/drm/radeon/radeon_prime.c
index 26c23bb..4940af7 100644
--- a/drivers/gpu/drm/radeon/radeon_prime.c
+++ b/drivers/gpu/drm/radeon/radeon_prime.c
@@ -28,199 +28,71 @@
#include "radeon.h"
#include
-#include
-
-static struct sg_table *radeon_gem_map_dma_buf(struct dma_buf_attachment
*attachment,
- enum dma_data_direction dir)
+struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
- struct radeon_bo *bo = attachment->dmabuf->priv;
- struct drm_device *dev = bo->rdev->ddev;
+ struct radeon_bo *bo = gem_to_radeon_bo(obj);
int npages = bo->tbo.num_pages;
- struct sg_table *sg;
- int nents;
-
- mutex_lock(&dev->struct_mutex);
- sg = drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages);
- nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir);
- mutex_unlock(&dev->struct_mutex);
- return sg;
-}
-
-static void radeon_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
-struct sg_table *sg, enum
dma_data_direction dir)
-{
- dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
- sg_free_table(sg);
- kfree(sg);
-}
-
-static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf)
-{
- struct radeon_bo *bo = dma_buf->priv;
-
- if (bo->gem_base.export_dma_buf == dma_buf) {
- DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base);
- bo->gem_base.export_dma_buf = NULL;
- drm_gem_object_unreference_unlocked(&bo->gem_base);
- }
-}
-
-static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long
page_num)
-{
- return NULL;
-}
-
-static void radeon_gem_
[PATCH v3 1/3] drm: add prime helpers
On 01/16/2013 01:50 AM, Daniel Vetter wrote:
> On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
>> 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 be8a42ae60addd8b6092535c11b42d099d6470ec
>>("drm/prime: drop reference on imported dma-buf come from gem")
>>
>> Signed-off-by: Aaron Plattner
>> Cc: Daniel Vetter
>> Cc: David Airlie
>
> Two minor things, but since you're not touching drm/i915 I don't care that
> much ...
>
>> +static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment
>> *attach,
>> + enum dma_data_direction dir)
>> +{
>> + struct drm_gem_object *obj = attach->dmabuf->priv;
>> + struct sg_table *sgt;
>> +
>> + mutex_lock(&obj->dev->struct_mutex);
>> +
>> + sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
>> +
>> + if (!IS_ERR_OR_NULL(sgt))
>> + dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> +
>> + mutex_unlock(&obj->dev->struct_mutex);
>
> The locking here looks superflous and not required to protect anything
> drm_gem_map_dma_buf does. If drivers need this (and ttm based ones
> shouldn't really), they can grab it in their ->gem_prime_get_sg_table
> callback. So I think a follow-up patch to ditch this would be good.
If you look at the later patches, this was just moved from the drivers.
I agree that evaluating whether or not it's actually necessary should
be a separate follow-up.
>> + return sgt;
>> +}
>> +
>> +static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> + struct sg_table *sgt, enum dma_data_direction dir)
>> +{
>> + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
>> + sg_free_table(sgt);
>> + kfree(sgt);
>> +}
>> +
>> +static void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
>> +{
>> + struct drm_gem_object *obj = dma_buf->priv;
>> +
>> + if (obj->export_dma_buf == dma_buf) {
>> + /* drop the reference on the export fd holds */
>> + obj->export_dma_buf = NULL;
>> + drm_gem_object_unreference_unlocked(obj);
>> + }
>> +}
>> +
>> +static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>> +{
>> + struct drm_gem_object *obj = dma_buf->priv;
>> + struct drm_device *dev = obj->dev;
>> +
>> + return dev->driver->gem_prime_vmap(obj);
>> +}
>> +
>> +static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>> +{
>> + struct drm_gem_object *obj = dma_buf->priv;
>> + struct drm_device *dev = obj->dev;
>> +
>> + dev->driver->gem_prime_vunmap(obj, vad
[PATCH] drm: modify pages_to_sg prime helper to create optimized SG table
On 01/28/2013 05:38 AM, Rahul Sharma wrote:
> 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
Reviewed-by: Aaron Plattner
I also verified that this reduces my 2025-entry sg_table to 6 entries, so
Tested-by: Aaron Plattner
--
Aaron
> ---
> drivers/gpu/drm/drm_prime.c | 8 ++--
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 7f12573..072ee08 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -217,21 +217,17 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device
> *dev, void *data,
> struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
> {
> struct sg_table *sg = NULL;
> - struct scatterlist *iter;
> - int i;
> int ret;
>
> sg = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> if (!sg)
> goto out;
>
> - ret = sg_alloc_table(sg, nr_pages, GFP_KERNEL);
> + ret = sg_alloc_table_from_pages(sg, pages, nr_pages, 0,
> + nr_pages << PAGE_SHIFT, GFP_KERNEL);
> if (ret)
> goto out;
>
> - for_each_sg(sg->sgl, iter, nr_pages, i)
> - sg_set_page(iter, pages[i], PAGE_SIZE, 0);
> -
> return sg;
> out:
> kfree(sg);
>
[PATCH libdrm] tests/dristat: add -C to pretty-print device capabilities
Signed-off-by: Aaron Plattner
---
Example output of dristat -C:
/dev/dri/card0
Device capabilities:
Dumb framebuffer: yes
VBlank high crtc: yes
Preferred depth: 24
Prefer shadow: yes
Prime: import export
tests/dristat.c | 69 -
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/tests/dristat.c b/tests/dristat.c
index 900a3e6..d36c3de 100644
--- a/tests/dristat.c
+++ b/tests/dristat.c
@@ -24,9 +24,11 @@
* DEALINGS IN THE SOFTWARE.
*
* Authors: Rickard E. (Rik) Faith
+ * Authors: Aaron Plattner
*
*/
+#include
#include
#include
#include
@@ -35,11 +37,14 @@
#include "xf86drmHash.c"
#include "xf86drm.c"
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+
#define DRM_VERSION 0x0001
#define DRM_MEMORY 0x0002
#define DRM_CLIENTS 0x0004
#define DRM_STATS 0x0008
#define DRM_BUSID 0x0010
+#define DRM_CAPS0x0020
static void getversion(int fd)
{
@@ -228,6 +233,65 @@ static void getstats(int fd, int i)
}
+enum cap_type {
+CAP_BOOL,
+CAP_UINT,
+CAP_PRIME
+};
+
+static void printcap(enum cap_type type, uint64_t value)
+{
+switch (type) {
+case CAP_BOOL:
+ if (value) printf("yes");
+ else printf("no");
+ break;
+case CAP_UINT:
+ printf("%" PRIu64, value);
+ break;
+case CAP_PRIME:
+ if (value == 0) printf("none");
+ else {
+ if (value & DRM_PRIME_CAP_IMPORT) printf("import ");
+ if (value & DRM_PRIME_CAP_EXPORT) printf("export");
+ }
+ break;
+}
+}
+
+static void getcaps(int fd)
+{
+const struct {
+ uint64_t capability;
+ enum cap_type type;
+ const char *name;
+} caps[] = {
+ { DRM_CAP_DUMB_BUFFER, CAP_BOOL, "Dumb framebuffer" },
+ { DRM_CAP_VBLANK_HIGH_CRTC, CAP_BOOL, "VBlank high crtc" },
+ { DRM_CAP_DUMB_PREFERRED_DEPTH, CAP_UINT, "Preferred depth" },
+ { DRM_CAP_DUMB_PREFER_SHADOW, CAP_BOOL, "Prefer shadow" },
+ { DRM_CAP_PRIME,CAP_PRIME, "Prime" },
+};
+int i;
+
+printf(" Device capabilities:\n");
+
+for (i = 0; i < ARRAY_SIZE(caps); i++) {
+ uint64_t value;
+ int ret = drmGetCap(fd, caps[i].capability, &value);
+
+ printf("%s: ", caps[i].name);
+
+ if (ret) {
+ printf("\n");
+ continue;
+ }
+
+ printcap(caps[i].type, value);
+ printf("\n");
+}
+}
+
int main(int argc, char **argv)
{
int c;
@@ -238,7 +302,7 @@ int main(int argc, char **argv)
char buf[64];
int i;
-while ((c = getopt(argc, argv, "avmcsbM:i:")) != EOF)
+while ((c = getopt(argc, argv, "avmcCsbM:i:")) != EOF)
switch (c) {
case 'a': mask = ~0; break;
case 'v': mask |= DRM_VERSION;break;
@@ -246,6 +310,7 @@ int main(int argc, char **argv)
case 'c': mask |= DRM_CLIENTS;break;
case 's': mask |= DRM_STATS; break;
case 'b': mask |= DRM_BUSID; break;
+ case 'C': mask |= DRM_CAPS; break;
case 'i': interval = strtol(optarg, NULL, 0); break;
case 'M': minor = strtol(optarg, NULL, 0);break;
default:
@@ -254,6 +319,7 @@ int main(int argc, char **argv)
fprintf( stderr, " -aShow all available information\n"
);
fprintf( stderr, " -bShow DRM bus ID's\n" );
fprintf( stderr, " -cDisplay information about DRM
clients\n" );
+ fprintf( stderr, " -CDisplay DRM device
capabilities\n" );
fprintf( stderr, " -i [interval] Continuously display statistics
every [interval] seconds\n" );
fprintf( stderr, " -vDisplay DRM module and card
version information\n" );
fprintf( stderr, " -mDisplay memory use information\n"
);
@@ -272,6 +338,7 @@ int main(int argc, char **argv)
if (mask & DRM_MEMORY) getvm(fd);
if (mask & DRM_CLIENTS) getclients(fd);
if (mask & DRM_STATS) getstats(fd, interval);
+ if (mask & DRM_CAPS)getcaps(fd);
close(fd);
}
}
--
1.8.1.2
Abstracting buffer sharing mechanism from the drm drivers
At the suggestion of a few drm developers, I'm looking at abstracting the buffer sharing mechanism away from the individual drm drivers and treating it as a low-level interface that kernel subsystems use to communicate, rather than as something drivers should be accessing directly. This would also mean that they wouldn't have to each implement their own set of dma_buf_ops, and the logic for things like detecting that you're importing your own dma_buf would be written once, in drm_prime.c and not in every driver's gem_prime_import function. Of course, it's slightly difficult because each driver's implementation seems to be subtly different. * i915 uses its own special locking function, i915_mutex_lock_interruptible. * nouveau and radeon pin the pages when the dma_buf is created, while i915 pins them at map time. * the vmap functions are different between i915 and radeon/nouveau, but it looks like all they use the dma_buf object for is to find the GEM object. Does it make sense to try to abstract the dma_buf parts of this? For example, a hypothetical new drm_gem_map_dma_buf would call a hook that lets i915 do its i915_gem_wait_for_error thing, takes the lock, calls a new gem_get_pages driver hook, does the dma_map_sg call, and handles the unlocking? I'll come up with a more detailed proposal or patches if this sounds like a good idea. -- Aaron
[PATCH] drm/udl: add enable/disable_vblank stubs
vblank_disable_and_save calls the driver's disable_vblank hook unconditionally,
which crashes the udl driver since it doesn't implement it. Fix this by adding
stub implementations of these functions identical to the qxl ones.
usb 5-1: USB disconnect, device number 2
BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [< (null)>] (null)
PGD 3bc23d067 PUD 3bc0fc067 PMD 0
Oops: 0010 [#1] PREEMPT SMP
Modules linked in: udlfb fb_sys_fops nvidia(PO) udl drm_kms_helper drm
syscopyarea sysfillrect sysimgblt netconsole cfg80211
joydev mousedev nls_iso8859_1 nls_cp437 vfat fat coretemp
intel_rapl eeepc_wmi asus_wmi sparse_keymap led_class
rfkill hwmon iosf_mbi x86_pkg_temp_thermal intel_powerclamp
iTCO_wdt iTCO_vendor_support evdev kvm crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel aesni_intel mac_hid
tpm_infineon aes_x86_64 lrw gf128mul e1000e glue_helper
ablk_helper cryptd mxm_wmi tpm_tis processor psmouse ptp
serio_raw tpm snd_hda_codec_hdmi i2c_i801 i2c_core lpc_ich
pps_core pcspkr snd_hda_codec_realtek snd_hda_codec_generic
mei_me mei snd_hda_intel snd_hda_controller snd_hda_codec
snd_hwdep snd_pcm snd_timer snd ie31200_edac edac_core
soundcore thermal shpchp battery video wmi button fan
sch_fq_codel nfs lockd grace sunrpc fscache btrfs xor
raid6_pq hid_generic usbhid hid sd_mod atkbd libps2 ahci
libahci crc32c_intel xhci_pci libata ehci_pci xhci_hcd
ehci_hcd scsi_mod usbcore usb_common i8042 serio [last
unloaded: drm]
CPU: 2 PID: 2044 Comm: Xorg.bin Tainted: P O 3.19.0-1-agp #1
Hardware name: System manufacturer System Product Name/P8Z77-V, BIOS 2104
08/13/2013
task: 8803f99d27c0 ti: 8803bc1c4000 task.ti: 8803bc1c4000
RIP: 0010:[<>] [< (null)>] (null)
RSP: 0018:8803bc1c7d00 EFLAGS: 00010046
RAX: a06d0140 RBX: RCX:
RDX: cee6 RSI: RDI: 8804037c3000
RBP: 8803bc1c7d88 R08: 0047b69a R09:
R10: a06de187 R11: 3246 R12: 880403d74540
R13: 0003 R14: 8804037c3000 R15: 8803bc32eac8
FS: 7f7f4753a900() GS:88041ec8() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: CR3: 0003bc24 CR4: 001407e0
Stack:
a06e06da 8803bc1c7d38 0082
8804037c3188 8803bc1c7d40 0282 8803bc1c7d68
0106 cee6 f69756fa 880403d74580
Call Trace:
[] ? vblank_disable_and_save+0x9a/0x200 [drm]
[] drm_vblank_cleanup+0x65/0xb0 [drm]
[] udl_driver_unload+0x18/0x50 [udl]
[] drm_dev_unregister+0x2d/0xb0 [drm]
[] drm_put_dev+0x27/0x70 [drm]
[] drm_release+0x347/0x520 [drm]
[] __fput+0x9f/0x200
[] fput+0xe/0x10
[] task_work_run+0xb7/0xf0
[] do_notify_resume+0x95/0xa0
[] int_signal+0x12/0x17
Code: Bad RIP value.
RIP [< (null)>] (null)
RSP
CR2:
---[ end trace e0b184ca053571af ]---
Reported-by: Thomas Meyer
Link: http://lists.freedesktop.org/archives/dri-devel/2014-December/073652.html
Signed-off-by: Aaron Plattner
---
Daniel, it looks like your change "[5/5] drm/irq: Don't call
->get_vblank_counter directly from irq_uninstall/cleanup" masks the immediate
problem, but it's not clear to me whether that's just because I didn't manage to
trigger any of the new vblank stuff, or whether your change really fixes it. It
does seem like these vblank functions are intended to be called unconditionally.
drivers/gpu/drm/udl/udl_drv.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index d5728ec85254..5bacb556b0f5 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -16,6 +16,20 @@ static int udl_driver_set_busid(struct drm_device *d, struct
drm_master *m)
return 0;
}
+static u32 udl_noop_get_vblank_counter(struct drm_device *dev, int crtc)
+{
+ return dev->vblank[crtc].count.counter;
+}
+
+static int udl_noop_enable_vblank(struct drm_device *dev, int crtc)
+{
+ return 0;
+}
+
+static void udl_noop_disable_vblank(struct drm_device *dev, int crtc)
+{
+}
+
static const struct vm_operations_struct udl_gem_vm_ops = {
.fault = udl_gem_fault,
.open = drm_gem_vm_open,
@@ -42,6 +56,10 @@ static struct drm_dri
[i915] Backlight brighter since 3.9.0
On 05/20/2013 02:55 PM, Daniel Vetter wrote: > On Sat, May 18, 2013 at 12:39:14AM +0200, Jan Hinnerk Stosch wrote: >> Hallo, >> >> I hope this is the right place to ask, because I actually don't know >> whether it is a bug or a feature that I'm experiencing since linux 3.9: >> When I boot my system the backlight gets extremely bright compared to older >> kernel versions. It is most obvious when I leave X (more a yellow than a >> black background), but I have the impression, that the colors in X are >> brighter than usual, too. >> I used my spare time this afternoon to do a kernel bisect and learned that >> the first "bad" commit is 55bc60db5988c8366751d3d04dd690698a53412c. As I >> don't have insight or understanding of the code: Is this behaviour intended >> and how could I change it to the old state or is it a bug and should I >> report it somewhere? >> My system is as follows: >> Intel i5-3570k with Intel HD 4000 >> my monitor is connected via HDMI. >> If you need any more information just tell me. > > Yeah, this is a feature. HDMI has (for oddball backwards compat with > analog TV signals) a special mode which reduces the useable RGB value > range by chopping off about 10% at the bottom and top end. This results in > light colors getting brighter and dark colors getting darker. > > The above mentioned commit tries (to the best of our knowledge) to > auto-set the option which most likely fits what the hdmi sink will do with > the color data. You can either fix this up in the hdmi sink with the > on-screen menu or by manually setting the "RBG Broadcast" property for the > relevant hdmi connector to the setting you want. This property seems like it's generally useful for all GPUs that support range compression. Has anyone started the process of adding it to randrproto.txt as an official property? http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt#n1723 -- Aaron --- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ---
[i915] Backlight brighter since 3.9.0
On 06/03/2013 12:36 PM, Daniel Vetter wrote: > On Mon, Jun 03, 2013 at 09:13:18AM -0700, Aaron Plattner wrote: >> On 05/20/2013 02:55 PM, Daniel Vetter wrote: >>> On Sat, May 18, 2013 at 12:39:14AM +0200, Jan Hinnerk Stosch wrote: >>>> Hallo, >>>> >>>> I hope this is the right place to ask, because I actually don't know >>>> whether it is a bug or a feature that I'm experiencing since linux 3.9: >>>> When I boot my system the backlight gets extremely bright compared to older >>>> kernel versions. It is most obvious when I leave X (more a yellow than a >>>> black background), but I have the impression, that the colors in X are >>>> brighter than usual, too. >>>> I used my spare time this afternoon to do a kernel bisect and learned that >>>> the first "bad" commit is 55bc60db5988c8366751d3d04dd690698a53412c. As I >>>> don't have insight or understanding of the code: Is this behaviour intended >>>> and how could I change it to the old state or is it a bug and should I >>>> report it somewhere? >>>> My system is as follows: >>>> Intel i5-3570k with Intel HD 4000 >>>> my monitor is connected via HDMI. >>>> If you need any more information just tell me. >>> >>> Yeah, this is a feature. HDMI has (for oddball backwards compat with >>> analog TV signals) a special mode which reduces the useable RGB value >>> range by chopping off about 10% at the bottom and top end. This results in >>> light colors getting brighter and dark colors getting darker. >>> >>> The above mentioned commit tries (to the best of our knowledge) to >>> auto-set the option which most likely fits what the hdmi sink will do with >>> the color data. You can either fix this up in the hdmi sink with the >>> on-screen menu or by manually setting the "RBG Broadcast" property for the >>> relevant hdmi connector to the setting you want. >> >> This property seems like it's generally useful for all GPUs that >> support range compression. Has anyone started the process of adding >> it to randrproto.txt as an official property? >> >> http://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt#n1723 > > Oops, I didn't know that we have some properties standardized there, > especially since the existing pile of drm/kms drivers seem to only lously > follow them. Should we move this into the kernel since that's essentially > the place that defines them? Maybe? I think I'm the only one who even tries to follow those, so "SHOULD" and "MUST" don't really mean a whole lot right now. One option would be to just abandon the idea of standardizing properties, but I do think standardization is good. Where that standard should live, though, is a another question. The kernel doesn't seem like the right place since RandR properties are useful on lots of platforms other than Linux. > -Daniel -- Aaron
[PATCH 1/3] drm: add mmap function to prime helpers
On 06/12/2013 06:16 AM, Joonyoung Shim wrote:
> This adds to call low-level mmap() from prime helpers.
>
> Signed-off-by: Joonyoung Shim
> ---
> drivers/gpu/drm/drm_prime.c | 5 -
> include/drm/drmP.h | 2 ++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index d92853e..3a008b2 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -165,7 +165,10 @@ static void drm_gem_dmabuf_kunmap(struct dma_buf
> *dma_buf,
> static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
> struct vm_area_struct *vma)
> {
> - return -EINVAL;
> + struct drm_gem_object *obj = dma_buf->priv;
> + struct drm_device *dev = obj->dev;
> +
> + return dev->driver->gem_prime_mmap(obj, vma);
Won't this crash if the driver doesn't fill in the new field and
userspace tries to map it?
> }
>
> static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 82670ac..12083dc 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -937,6 +937,8 @@ struct drm_driver {
> struct sg_table *sgt);
> void *(*gem_prime_vmap)(struct drm_gem_object *obj);
> void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
> + int (*gem_prime_mmap)(struct drm_gem_object *obj,
> + struct vm_area_struct *vma);
>
> /* vga arb irq handler */
> void (*vgaarb_irq)(struct drm_device *dev, bool state);
>
--
Aaron
[PATCH v3 1/3] drm: add prime helpers
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
> Hi Aaron,
>
> A bit late, but here's a small question.
>
> On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
>> 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 be8a42ae60addd8b6092535c11b42d099d6470ec
>>("drm/prime: drop reference on imported dma-buf come from gem")
>>
>> Signed-off-by: Aaron Plattner
>> Cc: Daniel Vetter
>> Cc: David Airlie
>> ---
>> Documentation/DocBook/drm.tmpl | 4 +
>> drivers/gpu/drm/drm_prime.c| 186 +-
>> include/drm/drmP.h | 12 +++
>> 3 files changed, 201 insertions(+), 1 deletion(-)
>
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index 7f12573..366910d 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>
> [snip]
>
>> +/**
>> + * DOC: PRIME Helpers
>> + *
>> + * Drivers can implement @gem_prime_export and @gem_prime_import in terms
>> of + * simpler APIs by using the helper functions @drm_gem_prime_export and
>> + * @drm_gem_prime_import. These functions implement dma-buf support in
>> terms of + * five lower-level driver callbacks:
>> + *
>> + * Export callbacks:
>> + *
>> + * - @gem_prime_pin (optional): prepare a GEM object for exporting
>> + *
>> + * - @gem_prime_get_sg_table: provide a scatter/gather table of pinned
>> pages + *
>> + * - @gem_prime_vmap: vmap a buffer exported by your driver
>> + *
>> + * - @gem_prime_vunmap: vunmap a buffer exported by your driver
>> + *
>> + * Import callback:
>> + *
>> + * - @gem_prime_import_sg_table (import): produce a GEM object from
>> another + *driver's scatter/gather table
>> + */
>> +
>> +struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>> + struct drm_gem_object *obj, int flags)
>> +{
>> +if (dev->driver->gem_prime_pin) {
>> +int ret = dev->driver->gem_prime_pin(obj);
>> +if (ret)
>> +return ERR_PTR(ret);
>> +}
>> +return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
>> + 0600);
>
> Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit
5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you
pointed it out just now.
>> +}
>> +EXPORT_SYMBOL(drm_gem_prime_export);
--
Aaron
[PATCH v3 1/3] drm: add prime helpers
On 06/18/2013 04:37 PM, Laurent Pinchart wrote:
> Hi Aaron,
>
> On Tuesday 18 June 2013 16:28:15 Aaron Plattner wrote:
>> On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
>>> Hi Aaron,
>>>
>>> A bit late, but here's a small question.
>>>
>>> On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
>>>> 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 be8a42ae60addd8b6092535c11b42d099d6470ec
>>>>
>>>> ("drm/prime: drop reference on imported dma-buf come from gem")
>>>>
>>>> Signed-off-by: Aaron Plattner
>>>> Cc: Daniel Vetter
>>>> Cc: David Airlie
>>>> ---
>>>>
>>>>Documentation/DocBook/drm.tmpl | 4 +
>>>>drivers/gpu/drm/drm_prime.c| 186
>>>>+-
>>>>include/drm/drmP.h | 12 +++
>>>>3 files changed, 201 insertions(+), 1 deletion(-)
>>>
>>> [snip]
>>>
>>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>>> index 7f12573..366910d 100644
>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>
>>> [snip]
>>>
>>>> +/**
>>>> + * DOC: PRIME Helpers
>>>> + *
>>>> + * Drivers can implement @gem_prime_export and @gem_prime_import in
>>>> terms
>>>> of + * simpler APIs by using the helper functions @drm_gem_prime_export
>>>> and
>>>> + * @drm_gem_prime_import. These functions implement dma-buf support in
>>>> terms of + * five lower-level driver callbacks:
>>>> + *
>>>> + * Export callbacks:
>>>> + *
>>>> + * - @gem_prime_pin (optional): prepare a GEM object for exporting
>>>> + *
>>>> + * - @gem_prime_get_sg_table: provide a scatter/gather table of pinned
>>>> pages + *
>>>> + * - @gem_prime_vmap: vmap a buffer exported by your driver
>>>> + *
>>>> + * - @gem_prime_vunmap: vunmap a buffer exported by your driver
>>>> + *
>>>> + * Import callback:
>>>> + *
>>>> + * - @gem_prime_import_sg_table (import): produce a GEM object from
>>>> another + *driver's scatter/gather table
>>>> + */
>>>> +
>>>> +struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>>>> + struct drm_gem_object *obj, int flags)
>>>> +{
>>>> + if (dev->driver->gem_prime_pin) {
>>>> + int ret = dev->driver->gem_prime_pin(obj);
>>>> + if (ret)
>>>> + return ERR_PTR(ret);
>>>> + }
>>>> + return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
>>>> +0600);
>>>
>>> Why do you use 0600 instead of the flags passed by the caller ?
>>
>> Because I copied & pasted it from i915_gem_prime_export prior to commit
>> 5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you
>> pointed it out just now.
>
> That's a pretty valid reason I guess :-) Should I submit a patch or are you
> already working on one ?
:) Sorry!
I'm not working on this right now, but I can put it on my queue if you
like. You'll probably be able to fix and test it sooner than I will,
though.
>>>> +}
>>>> +EXPORT_SYMBOL(drm_gem_prime_export);
--
Aaron
