Hello,
This patch series implements support for dma_buf import in the omapdrm driver. The patches are based on top of the latest drm-next branch and can be found in my git tree at
git://linuxtv.org/pinchartl/fbdev.git omapdrm/dmabuf-import
The first two patches are unrelated fixes and enhancements, but I've included them in the series to avoid merge conflicts.
The next 13 patches are miscellaneous fixes, cleanups and refactoring to prepare for patch 16/16 that implements dma_buf import.
The code has been successfully tested with the vivid driver as an exporter, using a hacked version that uses uncached CPU mappings in vivid when filling the buffers. vivid is a test driver that generates a test pattern using the CPU with cached mappings by default, resulting in corruption on the screen due to missing cache handling. As the problem doesn't occur when sharing buffers not touched by the CPU or touched through uncached mappings only, it will be addressed separately.
Changes since v1:
- Added patch 11/16 ("drm: omapdrm: gem: Fix GEM object destroy in error path") - Drop the dma-buf reexport check, reexport is handled by the DRM core - Make the OMAP_BO_USER_MASK definition private
Laurent Pinchart (16): drm: omapdrm: Fix plane state free in plane reset handler drm: omapdrm: Make fbdev emulation optional drm: omapdrm: gem: Remove unused function prototypes drm: omapdrm: gem: Remove forward declarations drm: omapdrm: gem: Group functions by purpose drm: omapdrm: gem: Move global usergart variable to omap_drm_private drm: omapdrm: gem: Remove omap_drm_private has_dmm field drm: omapdrm: gem: Mask out private flags passed from userspace drm: omapdrm: gem: Clean up GEM objects memory flags drm: omapdrm: gem: Free the correct memory object drm: omapdrm: gem: Fix GEM object destroy in error path drm: omapdrm: gem: Don't free mmap offset twice drm: omapdrm: gem: Simplify error handling when creating GEM object drm: omapdrm: gem: Remove check for impossible condition drm: omapdrm: gem: Refactor GEM object allocation drm: omapdrm: gem: Implement dma_buf import
drivers/gpu/drm/omapdrm/Makefile | 3 +- drivers/gpu/drm/omapdrm/omap_debugfs.c | 4 + drivers/gpu/drm/omapdrm/omap_drv.c | 17 +- drivers/gpu/drm/omapdrm/omap_drv.h | 16 +- drivers/gpu/drm/omapdrm/omap_fbdev.c | 6 +- drivers/gpu/drm/omapdrm/omap_gem.c | 501 +++++++++++++++++++----------- drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 53 +++- drivers/gpu/drm/omapdrm/omap_plane.c | 55 ++-- 8 files changed, 421 insertions(+), 234 deletions(-)
The plane reset handler frees the plane state and allocates a new default state, but when doing so attempt to free the plane state using the base plane state pointer instead of casting it to the driver-specific state object that has been allocated. Fix it by using the omap_plane_atomic_destroy_state() function to destroy the plane state instead of duplicating the code.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_plane.c | 53 ++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 3054bda72688..11d406b160e1 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -188,33 +188,6 @@ static const struct drm_plane_helper_funcs omap_plane_helper_funcs = { .atomic_disable = omap_plane_atomic_disable, };
-static void omap_plane_reset(struct drm_plane *plane) -{ - struct omap_plane *omap_plane = to_omap_plane(plane); - struct omap_plane_state *omap_state; - - if (plane->state && plane->state->fb) - drm_framebuffer_unreference(plane->state->fb); - - kfree(plane->state); - plane->state = NULL; - - omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL); - if (omap_state == NULL) - return; - - /* - * Set defaults depending on whether we are a primary or overlay - * plane. - */ - omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY - ? 0 : omap_plane->id; - omap_state->base.rotation = BIT(DRM_ROTATE_0); - - plane->state = &omap_state->base; - plane->state->plane = plane; -} - static void omap_plane_destroy(struct drm_plane *plane) { struct omap_plane *omap_plane = to_omap_plane(plane); @@ -270,6 +243,32 @@ static void omap_plane_atomic_destroy_state(struct drm_plane *plane, kfree(to_omap_plane_state(state)); }
+static void omap_plane_reset(struct drm_plane *plane) +{ + struct omap_plane *omap_plane = to_omap_plane(plane); + struct omap_plane_state *omap_state; + + if (plane->state) { + omap_plane_atomic_destroy_state(plane, plane->state); + plane->state = NULL; + } + + omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL); + if (omap_state == NULL) + return; + + /* + * Set defaults depending on whether we are a primary or overlay + * plane. + */ + omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY + ? 0 : omap_plane->id; + omap_state->base.rotation = BIT(DRM_ROTATE_0); + + plane->state = &omap_state->base; + plane->state->plane = plane; +} + static int omap_plane_atomic_set_property(struct drm_plane *plane, struct drm_plane_state *state, struct drm_property *property,
Don't compile the fbdev emulation code when fbdev emulation support is disabled.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/Makefile | 3 ++- drivers/gpu/drm/omapdrm/omap_debugfs.c | 4 ++++ drivers/gpu/drm/omapdrm/omap_drv.c | 4 ---- drivers/gpu/drm/omapdrm/omap_drv.h | 10 ++++++++++ drivers/gpu/drm/omapdrm/omap_fbdev.c | 4 ++++ drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++++ 6 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/Makefile b/drivers/gpu/drm/omapdrm/Makefile index 778372b062ad..368c1ec6805a 100644 --- a/drivers/gpu/drm/omapdrm/Makefile +++ b/drivers/gpu/drm/omapdrm/Makefile @@ -12,10 +12,11 @@ omapdrm-y := omap_drv.o \ omap_encoder.o \ omap_connector.o \ omap_fb.o \ - omap_fbdev.o \ omap_gem.o \ omap_gem_dmabuf.o \ omap_dmm_tiler.o \ tcm-sita.o
+omapdrm-$(CONFIG_DRM_FBDEV_EMULATION) += omap_fbdev.o + obj-$(CONFIG_DRM_OMAP) += omapdrm.o diff --git a/drivers/gpu/drm/omapdrm/omap_debugfs.c b/drivers/gpu/drm/omapdrm/omap_debugfs.c index ee91a25127f9..6f5fc14fc015 100644 --- a/drivers/gpu/drm/omapdrm/omap_debugfs.c +++ b/drivers/gpu/drm/omapdrm/omap_debugfs.c @@ -51,6 +51,7 @@ static int mm_show(struct seq_file *m, void *arg) return drm_mm_dump_table(m, &dev->vma_offset_manager->vm_addr_space_mm); }
+#ifdef CONFIG_DRM_FBDEV_EMULATION static int fb_show(struct seq_file *m, void *arg) { struct drm_info_node *node = (struct drm_info_node *) m->private; @@ -73,12 +74,15 @@ static int fb_show(struct seq_file *m, void *arg)
return 0; } +#endif
/* list of debufs files that are applicable to all devices */ static struct drm_info_list omap_debugfs_list[] = { {"gem", gem_show, 0}, {"mm", mm_show, 0}, +#ifdef CONFIG_DRM_FBDEV_EMULATION {"fb", fb_show, 0}, +#endif };
/* list of debugfs files that are specific to devices with dmm/tiler */ diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 5c6609cbb6a2..69be482e8d47 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -692,10 +692,6 @@ static int dev_load(struct drm_device *dev, unsigned long flags) drm_crtc_vblank_off(priv->crtcs[i]);
priv->fbdev = omap_fbdev_init(dev); - if (!priv->fbdev) { - dev_warn(dev->dev, "omap_fbdev_init failed\n"); - /* well, limp along without an fbdev.. maybe X11 will work? */ - }
/* store off drm_device for use in pm ops */ dev_set_drvdata(dev->dev, dev); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 130fca70bfd7..289d9b0984e2 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -138,8 +138,18 @@ void omap_irq_unregister(struct drm_device *dev, struct omap_drm_irq *irq); void omap_drm_irq_uninstall(struct drm_device *dev); int omap_drm_irq_install(struct drm_device *dev);
+#ifdef CONFIG_DRM_FBDEV_EMULATION struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev); void omap_fbdev_free(struct drm_device *dev); +#else +static inline struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) +{ + return NULL; +} +static inline void omap_fbdev_free(struct drm_device *dev) +{ +} +#endif
struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc); enum omap_channel omap_crtc_channel(struct drm_crtc *crtc); diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index b8e4cdec28c3..db4aa35accfc 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -298,6 +298,10 @@ fini: drm_fb_helper_fini(helper); fail: kfree(fbdev); + + dev_warn(dev->dev, "omap_fbdev_init failed\n"); + /* well, limp along without an fbdev.. maybe X11 will work? */ + return NULL; }
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 7ed08fdc4c42..374ce2adc811 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -653,6 +653,7 @@ fail: return ret; }
+#ifdef CONFIG_DRM_FBDEV_EMULATION /* Set scrolling position. This allows us to implement fast scrolling * for console. * @@ -689,6 +690,7 @@ fail:
return ret; } +#endif
/* Sync the buffer for CPU access.. note pages should already be * attached, ie. omap_gem_get_pages() @@ -924,6 +926,7 @@ int omap_gem_put_pages(struct drm_gem_object *obj) return 0; }
+#ifdef CONFIG_DRM_FBDEV_EMULATION /* Get kernel virtual address for CPU access.. this more or less only * exists for omap_fbdev. This should be called with struct_mutex * held. @@ -942,6 +945,7 @@ void *omap_gem_vaddr(struct drm_gem_object *obj) } return omap_obj->vaddr; } +#endif
#ifdef CONFIG_PM /* re-pin objects in DMM in resume path: */
Several DRM core function prototypes refer to functions that don't exist anymore and are thus obviously never called. Remove them.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 374ce2adc811..29a7ac6eb040 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -25,12 +25,6 @@ #include "omap_drv.h" #include "omap_dmm_tiler.h"
-/* remove these once drm core helpers are merged */ -struct page **_drm_gem_get_pages(struct drm_gem_object *obj, gfp_t gfpmask); -void _drm_gem_put_pages(struct drm_gem_object *obj, struct page **pages, - bool dirty, bool accessed); -int _drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size); - /* * GEM buffer object implementation. */
Reorder functions to get rid of forward declarations
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 90 +++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 29a7ac6eb040..a953a967b7db 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -113,8 +113,6 @@ struct omap_gem_object { } *sync; };
-static int get_pages(struct drm_gem_object *obj, struct page ***pages); -static uint64_t mmap_offset(struct drm_gem_object *obj);
/* To deal with userspace mmap'ings of 2d tiled buffers, which (a) are * not necessarily pinned in TILER all the time, and (b) when they are @@ -144,6 +142,30 @@ static struct { int last; /* index of last used entry */ } *usergart;
+/* ----------------------------------------------------------------------------- + * Helpers + */ + +/** get mmap offset */ +static uint64_t mmap_offset(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + int ret; + size_t size; + + WARN_ON(!mutex_is_locked(&dev->struct_mutex)); + + /* Make it mmapable */ + size = omap_gem_mmap_size(obj); + ret = drm_gem_create_mmap_offset_size(obj, size); + if (ret) { + dev_err(dev->dev, "could not allocate mmap offset\n"); + return 0; + } + + return drm_vma_node_offset_addr(&obj->vma_node); +} + static void evict_entry(struct drm_gem_object *obj, enum tiler_fmt fmt, struct usergart_entry *entry) { @@ -266,6 +288,28 @@ free_pages: return ret; }
+/* acquire pages when needed (for example, for DMA where physically + * contiguous buffer is not required + */ +static int get_pages(struct drm_gem_object *obj, struct page ***pages) +{ + struct omap_gem_object *omap_obj = to_omap_bo(obj); + int ret = 0; + + if (is_shmem(obj) && !omap_obj->pages) { + ret = omap_gem_attach_pages(obj); + if (ret) { + dev_err(obj->dev->dev, "could not attach pages\n"); + return ret; + } + } + + /* TODO: even phys-contig.. we should have a list of pages? */ + *pages = omap_obj->pages; + + return 0; +} + /** release backing pages */ static void omap_gem_detach_pages(struct drm_gem_object *obj) { @@ -295,26 +339,6 @@ uint32_t omap_gem_flags(struct drm_gem_object *obj) return to_omap_bo(obj)->flags; }
-/** get mmap offset */ -static uint64_t mmap_offset(struct drm_gem_object *obj) -{ - struct drm_device *dev = obj->dev; - int ret; - size_t size; - - WARN_ON(!mutex_is_locked(&dev->struct_mutex)); - - /* Make it mmapable */ - size = omap_gem_mmap_size(obj); - ret = drm_gem_create_mmap_offset_size(obj, size); - if (ret) { - dev_err(dev->dev, "could not allocate mmap offset\n"); - return 0; - } - - return drm_vma_node_offset_addr(&obj->vma_node); -} - uint64_t omap_gem_mmap_offset(struct drm_gem_object *obj) { uint64_t offset; @@ -861,28 +885,6 @@ int omap_gem_tiled_stride(struct drm_gem_object *obj, uint32_t orient) return ret; }
-/* acquire pages when needed (for example, for DMA where physically - * contiguous buffer is not required - */ -static int get_pages(struct drm_gem_object *obj, struct page ***pages) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret = 0; - - if (is_shmem(obj) && !omap_obj->pages) { - ret = omap_gem_attach_pages(obj); - if (ret) { - dev_err(obj->dev->dev, "could not attach pages\n"); - return ret; - } - } - - /* TODO: even phys-contig.. we should have a list of pages? */ - *pages = omap_obj->pages; - - return 0; -} - /* if !remap, and we don't have pages backing, then fail, rather than * increasing the pin count (which we don't really do yet anyways, * because we don't support swapping pages back out). And 'remap'
Divide the GEM implementation in groups of functions to improve readability.
No code change is performed by this commit.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 140 +++++++++++++++++++++++-------------- 1 file changed, 87 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index a953a967b7db..b6dffdbbc0c1 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -29,14 +29,11 @@ * GEM buffer object implementation. */
-#define to_omap_bo(x) container_of(x, struct omap_gem_object, base) - /* note: we use upper 8 bits of flags for driver-internal flags: */ -#define OMAP_BO_DMA 0x01000000 /* actually is physically contiguous */ +#define OMAP_BO_DMA 0x01000000 /* actually is physically contiguous */ #define OMAP_BO_EXT_SYNC 0x02000000 /* externally allocated sync object */ #define OMAP_BO_EXT_MEM 0x04000000 /* externally allocated memory */
- struct omap_gem_object { struct drm_gem_object base;
@@ -113,6 +110,7 @@ struct omap_gem_object { } *sync; };
+#define to_omap_bo(x) container_of(x, struct omap_gem_object, base)
/* To deal with userspace mmap'ings of 2d tiled buffers, which (a) are * not necessarily pinned in TILER all the time, and (b) when they are @@ -166,6 +164,22 @@ static uint64_t mmap_offset(struct drm_gem_object *obj) return drm_vma_node_offset_addr(&obj->vma_node); }
+/* GEM objects can either be allocated from contiguous memory (in which + * case obj->filp==NULL), or w/ shmem backing (obj->filp!=NULL). But non + * contiguous buffers can be remapped in TILER/DMM if they need to be + * contiguous... but we don't do this all the time to reduce pressure + * on TILER/DMM space when we know at allocation time that the buffer + * will need to be scanned out. + */ +static inline bool is_shmem(struct drm_gem_object *obj) +{ + return obj->filp != NULL; +} + +/* ----------------------------------------------------------------------------- + * Eviction + */ + static void evict_entry(struct drm_gem_object *obj, enum tiler_fmt fmt, struct usergart_entry *entry) { @@ -212,30 +226,9 @@ static void evict(struct drm_gem_object *obj) } }
-/* GEM objects can either be allocated from contiguous memory (in which - * case obj->filp==NULL), or w/ shmem backing (obj->filp!=NULL). But non - * contiguous buffers can be remapped in TILER/DMM if they need to be - * contiguous... but we don't do this all the time to reduce pressure - * on TILER/DMM space when we know at allocation time that the buffer - * will need to be scanned out. - */ -static inline bool is_shmem(struct drm_gem_object *obj) -{ - return obj->filp != NULL; -} - -/** - * shmem buffers that are mapped cached can simulate coherency via using - * page faulting to keep track of dirty pages +/* ----------------------------------------------------------------------------- + * Page Management */ -static inline bool is_cached_coherent(struct drm_gem_object *obj) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - return is_shmem(obj) && - ((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED); -} - -static DEFINE_SPINLOCK(sync_lock);
/** ensure backing pages are allocated */ static int omap_gem_attach_pages(struct drm_gem_object *obj) @@ -380,6 +373,10 @@ int omap_gem_tiled_size(struct drm_gem_object *obj, uint16_t *w, uint16_t *h) return -EINVAL; }
+/* ----------------------------------------------------------------------------- + * Fault Handling + */ + /* Normal handling for the case of faulting in non-tiled buffers */ static int fault_1d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) @@ -614,6 +611,9 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, return 0; }
+/* ----------------------------------------------------------------------------- + * Dumb Buffers + */
/** * omap_gem_dumb_create - create a dumb buffer @@ -710,6 +710,21 @@ fail: } #endif
+/* ----------------------------------------------------------------------------- + * Memory Management & DMA Sync + */ + +/** + * shmem buffers that are mapped cached can simulate coherency via using + * page faulting to keep track of dirty pages + */ +static inline bool is_cached_coherent(struct drm_gem_object *obj) +{ + struct omap_gem_object *omap_obj = to_omap_bo(obj); + return is_shmem(obj) && + ((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED); +} + /* Sync the buffer for CPU access.. note pages should already be * attached, ie. omap_gem_get_pages() */ @@ -943,6 +958,10 @@ void *omap_gem_vaddr(struct drm_gem_object *obj) } #endif
+/* ----------------------------------------------------------------------------- + * Power Management + */ + #ifdef CONFIG_PM /* re-pin objects in DMM in resume path: */ int omap_gem_resume(struct device *dev) @@ -971,6 +990,10 @@ int omap_gem_resume(struct device *dev) } #endif
+/* ----------------------------------------------------------------------------- + * DebugFS + */ + #ifdef CONFIG_DEBUG_FS void omap_gem_describe(struct drm_gem_object *obj, struct seq_file *m) { @@ -1017,9 +1040,12 @@ void omap_gem_describe_objects(struct list_head *list, struct seq_file *m) } #endif
-/* Buffer Synchronization: +/* ----------------------------------------------------------------------------- + * Buffer Synchronization */
+static DEFINE_SPINLOCK(sync_lock); + struct omap_gem_sync_waiter { struct list_head list; struct omap_gem_object *omap_obj; @@ -1265,6 +1291,10 @@ unlock: return ret; }
+/* ----------------------------------------------------------------------------- + * Constructor & Destructor + */ + /* don't call directly.. called from GEM core when it is time to actually * free the object.. */ @@ -1311,30 +1341,6 @@ void omap_gem_free_object(struct drm_gem_object *obj) kfree(obj); }
-/* convenience method to construct a GEM buffer object, and userspace handle */ -int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file, - union omap_gem_size gsize, uint32_t flags, uint32_t *handle) -{ - struct drm_gem_object *obj; - int ret; - - obj = omap_gem_new(dev, gsize, flags); - if (!obj) - return -ENOMEM; - - ret = drm_gem_handle_create(file, obj, handle); - if (ret) { - drm_gem_object_release(obj); - kfree(obj); /* TODO isn't there a dtor to call? just copying i915 */ - return ret; - } - - /* drop reference from allocate - handle holds it now */ - drm_gem_object_unreference_unlocked(obj); - - return 0; -} - /* GEM buffer object constructor */ struct drm_gem_object *omap_gem_new(struct drm_device *dev, union omap_gem_size gsize, uint32_t flags) @@ -1426,7 +1432,35 @@ fail: return NULL; }
-/* init/cleanup.. if DMM is used, we need to set some stuff up.. */ +/* convenience method to construct a GEM buffer object, and userspace handle */ +int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file, + union omap_gem_size gsize, uint32_t flags, uint32_t *handle) +{ + struct drm_gem_object *obj; + int ret; + + obj = omap_gem_new(dev, gsize, flags); + if (!obj) + return -ENOMEM; + + ret = drm_gem_handle_create(file, obj, handle); + if (ret) { + drm_gem_object_release(obj); + kfree(obj); /* TODO isn't there a dtor to call? just copying i915 */ + return ret; + } + + /* drop reference from allocate - handle holds it now */ + drm_gem_object_unreference_unlocked(obj); + + return 0; +} + +/* ----------------------------------------------------------------------------- + * Init & Cleanup + */ + +/* If DMM is used, we need to set some stuff up.. */ void omap_gem_init(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private;
The structure contains data related to a device instance, it shouldn't be a global variable.
While at it rename the usergart structures with an omap_drm_ prefix.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.h | 3 +++ drivers/gpu/drm/omapdrm/omap_gem.c | 54 +++++++++++++++++++++++--------------- 2 files changed, 36 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 289d9b0984e2..718f032aa052 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -36,6 +36,8 @@
#define MODULE_NAME "omapdrm"
+struct omap_drm_usergart; + /* max # of mapper-id's that can be assigned.. todo, come up with a better * (but still inexpensive) way to store/access per-buffer mapper private * data.. @@ -97,6 +99,7 @@ struct omap_drm_private { /* list of GEM objects: */ struct list_head obj_list;
+ struct omap_drm_usergart *usergart; bool has_dmm;
/* properties: */ diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index b6dffdbbc0c1..391bc7378f9f 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -124,21 +124,22 @@ struct omap_gem_object { * for later.. */ #define NUM_USERGART_ENTRIES 2 -struct usergart_entry { +struct omap_drm_usergart_entry { struct tiler_block *block; /* the reserved tiler block */ dma_addr_t paddr; struct drm_gem_object *obj; /* the current pinned obj */ pgoff_t obj_pgoff; /* page offset of obj currently mapped in */ }; -static struct { - struct usergart_entry entry[NUM_USERGART_ENTRIES]; + +struct omap_drm_usergart { + struct omap_drm_usergart_entry entry[NUM_USERGART_ENTRIES]; int height; /* height in rows */ int height_shift; /* ilog2(height in rows) */ int slot_shift; /* ilog2(width per slot) */ int stride_pfn; /* stride in pages */ int last; /* index of last used entry */ -} *usergart; +};
/* ----------------------------------------------------------------------------- * Helpers @@ -181,10 +182,11 @@ static inline bool is_shmem(struct drm_gem_object *obj) */
static void evict_entry(struct drm_gem_object *obj, - enum tiler_fmt fmt, struct usergart_entry *entry) + enum tiler_fmt fmt, struct omap_drm_usergart_entry *entry) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - int n = usergart[fmt].height; + struct omap_drm_private *priv = obj->dev->dev_private; + int n = priv->usergart[fmt].height; size_t size = PAGE_SIZE * n; loff_t off = mmap_offset(obj) + (entry->obj_pgoff << PAGE_SHIFT); @@ -210,16 +212,19 @@ static void evict_entry(struct drm_gem_object *obj, static void evict(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); + struct omap_drm_private *priv = obj->dev->dev_private;
if (omap_obj->flags & OMAP_BO_TILED) { enum tiler_fmt fmt = gem2fmt(omap_obj->flags); int i;
- if (!usergart) + if (!priv->usergart) return;
for (i = 0; i < NUM_USERGART_ENTRIES; i++) { - struct usergart_entry *entry = &usergart[fmt].entry[i]; + struct omap_drm_usergart_entry *entry = + &priv->usergart[fmt].entry[i]; + if (entry->obj == obj) evict_entry(obj, fmt, entry); } @@ -408,7 +413,8 @@ static int fault_2d(struct drm_gem_object *obj, struct vm_area_struct *vma, struct vm_fault *vmf) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - struct usergart_entry *entry; + struct omap_drm_private *priv = obj->dev->dev_private; + struct omap_drm_usergart_entry *entry; enum tiler_fmt fmt = gem2fmt(omap_obj->flags); struct page *pages[64]; /* XXX is this too much to have on stack? */ unsigned long pfn; @@ -421,8 +427,8 @@ static int fault_2d(struct drm_gem_object *obj, * that need to be mapped in to fill 4kb wide CPU page. If the slot * height is 64, then 64 pages fill a 4kb wide by 64 row region. */ - const int n = usergart[fmt].height; - const int n_shift = usergart[fmt].height_shift; + const int n = priv->usergart[fmt].height; + const int n_shift = priv->usergart[fmt].height_shift;
/* * If buffer width in bytes > PAGE_SIZE then the virtual stride is @@ -443,11 +449,11 @@ static int fault_2d(struct drm_gem_object *obj, base_pgoff = round_down(pgoff, m << n_shift);
/* figure out buffer width in slots */ - slots = omap_obj->width >> usergart[fmt].slot_shift; + slots = omap_obj->width >> priv->usergart[fmt].slot_shift;
vaddr = vmf->virtual_address - ((pgoff - base_pgoff) << PAGE_SHIFT);
- entry = &usergart[fmt].entry[usergart[fmt].last]; + entry = &priv->usergart[fmt].entry[priv->usergart[fmt].last];
/* evict previous buffer using this usergart entry, if any: */ if (entry->obj) @@ -494,12 +500,13 @@ static int fault_2d(struct drm_gem_object *obj,
for (i = n; i > 0; i--) { vm_insert_mixed(vma, (unsigned long)vaddr, pfn); - pfn += usergart[fmt].stride_pfn; + pfn += priv->usergart[fmt].stride_pfn; vaddr += PAGE_SIZE * m; }
/* simple round-robin: */ - usergart[fmt].last = (usergart[fmt].last + 1) % NUM_USERGART_ENTRIES; + priv->usergart[fmt].last = (priv->usergart[fmt].last + 1) + % NUM_USERGART_ENTRIES;
return 0; } @@ -1353,7 +1360,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, int ret;
if (flags & OMAP_BO_TILED) { - if (!usergart) { + if (!priv->usergart) { dev_err(dev->dev, "Tiled buffers require DMM\n"); goto fail; } @@ -1464,6 +1471,7 @@ int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file, void omap_gem_init(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private; + struct omap_drm_usergart *usergart; const enum tiler_fmt fmts[] = { TILFMT_8BIT, TILFMT_16BIT, TILFMT_32BIT }; @@ -1492,10 +1500,11 @@ void omap_gem_init(struct drm_device *dev) usergart[i].stride_pfn = tiler_stride(fmts[i], 0) >> PAGE_SHIFT; usergart[i].slot_shift = ilog2((PAGE_SIZE / h) >> i); for (j = 0; j < NUM_USERGART_ENTRIES; j++) { - struct usergart_entry *entry = &usergart[i].entry[j]; - struct tiler_block *block = - tiler_reserve_2d(fmts[i], w, h, - PAGE_SIZE); + struct omap_drm_usergart_entry *entry; + struct tiler_block *block; + + entry = &usergart[i].entry[j]; + block = tiler_reserve_2d(fmts[i], w, h, PAGE_SIZE); if (IS_ERR(block)) { dev_err(dev->dev, "reserve failed: %d, %d, %ld\n", @@ -1511,13 +1520,16 @@ void omap_gem_init(struct drm_device *dev) } }
+ priv->usergart = usergart; priv->has_dmm = true; }
void omap_gem_deinit(struct drm_device *dev) { + struct omap_drm_private *priv = dev->dev_private; + /* I believe we can rely on there being no more outstanding GEM * objects which could depend on usergart/dmm at this point. */ - kfree(usergart); + kfree(priv->usergart); }
The field is set to true iff the usergart field is not NULL. Test usergart instead.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 2 +- drivers/gpu/drm/omapdrm/omap_drv.h | 1 - drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 +- drivers/gpu/drm/omapdrm/omap_gem.c | 5 ++--- drivers/gpu/drm/omapdrm/omap_plane.c | 2 +- 5 files changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 69be482e8d47..83d63d71062c 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -303,7 +303,7 @@ static int omap_modeset_init_properties(struct drm_device *dev) { struct omap_drm_private *priv = dev->dev_private;
- if (priv->has_dmm) { + if (priv->usergart) { dev->mode_config.rotation_property = drm_mode_create_rotation_property(dev, BIT(DRM_ROTATE_0) | BIT(DRM_ROTATE_90) | diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 718f032aa052..97fcb892fdd7 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -100,7 +100,6 @@ struct omap_drm_private { struct list_head obj_list;
struct omap_drm_usergart *usergart; - bool has_dmm;
/* properties: */ struct drm_property *zorder_prop; diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index db4aa35accfc..9fb15de1207f 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -132,7 +132,7 @@ static int omap_fbdev_create(struct drm_fb_helper *helper, mode_cmd.width * ((sizes->surface_bpp + 7) / 8), mode_cmd.width, sizes->surface_bpp);
- fbdev->ywrap_enabled = priv->has_dmm && ywrap_enabled; + fbdev->ywrap_enabled = priv->usergart && ywrap_enabled; if (fbdev->ywrap_enabled) { /* need to align pitch to page size if using DMM scrolling */ mode_cmd.pitches[0] = PAGE_ALIGN(mode_cmd.pitches[0]); diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 391bc7378f9f..8e3b415aa43b 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -787,7 +787,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
mutex_lock(&obj->dev->struct_mutex);
- if (remap && is_shmem(obj) && priv->has_dmm) { + if (remap && is_shmem(obj) && priv->usergart) { if (omap_obj->paddr_cnt == 0) { struct page **pages; uint32_t npages = obj->size >> PAGE_SHIFT; @@ -1393,7 +1393,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
obj = &omap_obj->base;
- if ((flags & OMAP_BO_SCANOUT) && !priv->has_dmm) { + if ((flags & OMAP_BO_SCANOUT) && !priv->usergart) { /* attempt to allocate contiguous memory if we don't * have DMM for remappign discontiguous buffers */ @@ -1521,7 +1521,6 @@ void omap_gem_init(struct drm_device *dev) }
priv->usergart = usergart; - priv->has_dmm = true; }
void omap_gem_deinit(struct drm_device *dev) diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 11d406b160e1..1c6ec7080f16 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -208,7 +208,7 @@ void omap_plane_install_properties(struct drm_plane *plane, struct drm_device *dev = plane->dev; struct omap_drm_private *priv = dev->dev_private;
- if (priv->has_dmm) { + if (priv->usergart) { struct drm_property *prop = dev->mode_config.rotation_property;
drm_object_attach_property(obj, prop, 0);
On 14/12/15 22:39, Laurent Pinchart wrote:
The field is set to true iff the usergart field is not NULL. Test usergart instead.
That's true, but I don't like the change.
'has_dmm' means we have DMM (well, TILER, really). TILER is used for 1D and 2D modes. 'usergart' is only for TILER 2D CPU access (if I'm not mistaken).
I'm not sure what it would buy us or if it's worth the effort, but maybe we want to make the 2D mode optional in the future.
And even if we do have 2D mode enabled, due to HW issues a 2D mapped buffer can't really be accessed by the CPU, as it's just too slow. So maybe we can have 2D mode supported, but without usergart.
I don't know. But I want to keep these things separated.
That said, maybe some of the 'if's would be cleaner if they checked for usergart instead of has_dmm, if usergart is really what they depend on.
Tomi
The 8 high order bits of the buffer flags are reserved for internal use. Mask them out from the flags passed by userspace.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
Changes since v1:
- Make the OMAP_BO_USER_MASK definition private --- drivers/gpu/drm/omapdrm/omap_drv.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 83d63d71062c..2f571eb2d1c9 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -547,14 +547,19 @@ static int ioctl_set_param(struct drm_device *dev, void *data, return 0; }
+#define OMAP_BO_USER_MASK 0x00ffffff /* flags settable by userspace */ + static int ioctl_gem_new(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_omap_gem_new *args = data; + u32 flags = args->flags & OMAP_BO_USER_MASK; + VERB("%p:%p: size=0x%08x, flags=%08x", dev, file_priv, - args->size.bytes, args->flags); - return omap_gem_new_handle(dev, file_priv, args->size, - args->flags, &args->handle); + args->size.bytes, flags); + + return omap_gem_new_handle(dev, file_priv, args->size, flags, + &args->handle); }
static int ioctl_gem_cpu_prep(struct drm_device *dev, void *data,
The driver assumes that only objects backed by shmem need to be mapped through DMM. While this is true with the current code, the assumption won't hold with dma_buf import support.
Condition the mapping based on whether the buffer has been allocated using the DMA mapping API instead and clean up the flags to avoid having to check both flags and GEM object filp field to decide how to process buffers. Flags are not the authoritative source of information regarding where the buffer memory comes from, and are renamed to make that clearer.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 57 +++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 8e3b415aa43b..644dff8ab516 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -30,9 +30,10 @@ */
/* note: we use upper 8 bits of flags for driver-internal flags: */ -#define OMAP_BO_DMA 0x01000000 /* actually is physically contiguous */ -#define OMAP_BO_EXT_SYNC 0x02000000 /* externally allocated sync object */ -#define OMAP_BO_EXT_MEM 0x04000000 /* externally allocated memory */ +#define OMAP_BO_MEM_DMA_API 0x01000000 /* memory allocated with the dma_alloc_* API */ +#define OMAP_BO_MEM_SHMEM 0x02000000 /* memory allocated through shmem backing */ +#define OMAP_BO_MEM_EXT 0x04000000 /* memory allocated externally */ +#define OMAP_BO_EXT_SYNC 0x10000000 /* externally allocated sync object */
struct omap_gem_object { struct drm_gem_object base; @@ -48,16 +49,16 @@ struct omap_gem_object { uint32_t roll;
/** - * If buffer is allocated physically contiguous, the OMAP_BO_DMA flag - * is set and the paddr is valid. Also if the buffer is remapped in - * TILER and paddr_cnt > 0, then paddr is valid. But if you are using - * the physical address and OMAP_BO_DMA is not set, then you should - * be going thru omap_gem_{get,put}_paddr() to ensure the mapping is - * not removed from under your feet. + * If buffer is allocated physically contiguous, the OMAP_BO_MEM_DMA_API + * flag is set and the paddr is valid. Also if the buffer is remapped + * in TILER and paddr_cnt > 0, then paddr is valid. But if you are using + * the physical address and OMAP_BO_MEM_DMA_API is not set, then you + * should be going thru omap_gem_{get,put}_paddr() to ensure the mapping + * is not removed from under your feet. * * Note that OMAP_BO_SCANOUT is a hint from userspace that DMA capable - * buffer is requested, but doesn't mean that it is. Use the - * OMAP_BO_DMA flag to determine if the buffer has a DMA capable + * buffer is requested, but doesn't mean that it is. Use the + * OMAP_BO_MEM_DMA_API flag to determine if the buffer has a DMA capable * physical address. */ dma_addr_t paddr; @@ -165,18 +166,6 @@ static uint64_t mmap_offset(struct drm_gem_object *obj) return drm_vma_node_offset_addr(&obj->vma_node); }
-/* GEM objects can either be allocated from contiguous memory (in which - * case obj->filp==NULL), or w/ shmem backing (obj->filp!=NULL). But non - * contiguous buffers can be remapped in TILER/DMM if they need to be - * contiguous... but we don't do this all the time to reduce pressure - * on TILER/DMM space when we know at allocation time that the buffer - * will need to be scanned out. - */ -static inline bool is_shmem(struct drm_gem_object *obj) -{ - return obj->filp != NULL; -} - /* ----------------------------------------------------------------------------- * Eviction */ @@ -294,7 +283,7 @@ static int get_pages(struct drm_gem_object *obj, struct page ***pages) struct omap_gem_object *omap_obj = to_omap_bo(obj); int ret = 0;
- if (is_shmem(obj) && !omap_obj->pages) { + if ((omap_obj->flags & OMAP_BO_MEM_SHMEM) && !omap_obj->pages) { ret = omap_gem_attach_pages(obj); if (ret) { dev_err(obj->dev->dev, "could not attach pages\n"); @@ -398,7 +387,7 @@ static int fault_1d(struct drm_gem_object *obj, omap_gem_cpu_sync(obj, pgoff); pfn = page_to_pfn(omap_obj->pages[pgoff]); } else { - BUG_ON(!(omap_obj->flags & OMAP_BO_DMA)); + BUG_ON(!(omap_obj->flags & OMAP_BO_MEM_DMA_API)); pfn = (omap_obj->paddr >> PAGE_SHIFT) + pgoff; }
@@ -728,7 +717,8 @@ fail: static inline bool is_cached_coherent(struct drm_gem_object *obj) { struct omap_gem_object *omap_obj = to_omap_bo(obj); - return is_shmem(obj) && + + return (omap_obj->flags & OMAP_BO_MEM_SHMEM) && ((omap_obj->flags & OMAP_BO_CACHE_MASK) == OMAP_BO_CACHED); }
@@ -787,7 +777,8 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
mutex_lock(&obj->dev->struct_mutex);
- if (remap && is_shmem(obj) && priv->usergart) { + if (!(omap_obj->flags & OMAP_BO_MEM_DMA_API) && + remap && priv->usergart) { if (omap_obj->paddr_cnt == 0) { struct page **pages; uint32_t npages = obj->size >> PAGE_SHIFT; @@ -834,7 +825,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, omap_obj->paddr_cnt++;
*paddr = omap_obj->paddr; - } else if (omap_obj->flags & OMAP_BO_DMA) { + } else if (omap_obj->flags & OMAP_BO_MEM_DMA_API) { *paddr = omap_obj->paddr; } else { ret = -EINVAL; @@ -1327,11 +1318,11 @@ void omap_gem_free_object(struct drm_gem_object *obj) WARN_ON(omap_obj->paddr_cnt > 0);
/* don't free externally allocated backing memory */ - if (!(omap_obj->flags & OMAP_BO_EXT_MEM)) { + if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) { if (omap_obj->pages) omap_gem_detach_pages(obj);
- if (!is_shmem(obj)) { + if (omap_obj->flags & OMAP_BO_MEM_DMA_API) { dma_free_writecombine(dev->dev, obj->size, omap_obj->vaddr, omap_obj->paddr); } else if (omap_obj->vaddr) { @@ -1405,7 +1396,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, return NULL; }
- flags |= OMAP_BO_DMA; + flags |= OMAP_BO_MEM_DMA_API; }
spin_lock(&priv->list_lock); @@ -1419,7 +1410,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, omap_obj->height = gsize.tiled.height; }
- if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM)) { + if (flags & (OMAP_BO_MEM_DMA_API | OMAP_BO_MEM_EXT)) { drm_gem_private_object_init(dev, obj, size); } else { ret = drm_gem_object_init(dev, obj, size); @@ -1428,6 +1419,8 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
mapping = file_inode(obj->filp)->i_mapping; mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32); + + omap_obj->flags |= OMAP_BO_MEM_SHMEM; }
return obj;
The GEM object free handler frees memory allocated by the driver using the pointer to the drm_gem_object instead of the pointer to the omap_gem_object that embeds it. This doesn't cause any issue in practice as the drm_gem_object is the first field of omap_gem_object, but would cause memory corruption if the structure layout changes. Fix it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 644dff8ab516..f24bb71d9946 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1336,7 +1336,7 @@ void omap_gem_free_object(struct drm_gem_object *obj)
drm_gem_object_release(obj);
- kfree(obj); + kfree(omap_obj); }
/* GEM buffer object constructor */
Use the omap_gem_free_object() function to destroy the GEM object in the omap_gem_new_handle() error path instead of doing it manually (and incorrectly).
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index f24bb71d9946..ca84943dbfbc 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1445,8 +1445,7 @@ int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file,
ret = drm_gem_handle_create(file, obj, handle); if (ret) { - drm_gem_object_release(obj); - kfree(obj); /* TODO isn't there a dtor to call? just copying i915 */ + omap_gem_free_object(obj); return ret; }
The drm_gem_free_mmap_offset() call in omap_gem_free_object() is redundant as the same function is called from drm_gem_object_release(). Remove it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index ca84943dbfbc..08ffcce1f62c 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1310,8 +1310,6 @@ void omap_gem_free_object(struct drm_gem_object *obj) list_del(&omap_obj->mm_list); spin_unlock(&priv->list_lock);
- drm_gem_free_mmap_offset(obj); - /* this means the object is still pinned.. which really should * not happen. I think.. */
The goto error statement end up just returning NULL without performing any cleanup, replace it with a direct return.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 08ffcce1f62c..aa7a8484a6d0 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1343,7 +1343,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, { struct omap_drm_private *priv = dev->dev_private; struct omap_gem_object *omap_obj; - struct drm_gem_object *obj = NULL; + struct drm_gem_object *obj; struct address_space *mapping; size_t size; int ret; @@ -1351,7 +1351,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, if (flags & OMAP_BO_TILED) { if (!priv->usergart) { dev_err(dev->dev, "Tiled buffers require DMM\n"); - goto fail; + return NULL; }
/* tiled buffers are always shmem paged backed.. when they are @@ -1424,9 +1424,7 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, return obj;
fail: - if (obj) - omap_gem_free_object(obj); - + omap_gem_free_object(obj); return NULL; }
The GEM object can't be tiled without a usergart as that condition is checked and considered as an error when creating the GEM object.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index aa7a8484a6d0..ba16d792a33e 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -207,9 +207,6 @@ static void evict(struct drm_gem_object *obj) enum tiler_fmt fmt = gem2fmt(omap_obj->flags); int i;
- if (!priv->usergart) - return; - for (i = 0; i < NUM_USERGART_ENTRIES; i++) { struct omap_drm_usergart_entry *entry = &priv->usergart[fmt].entry[i];
Split the individual steps of GEM object allocation and initialization clearly. This improves readability and prepares for dma_buf import support.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 75 ++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index ba16d792a33e..51580eaa37af 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1345,67 +1345,80 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, size_t size; int ret;
+ /* Validate the flags and compute the memory and cache flags. */ if (flags & OMAP_BO_TILED) { if (!priv->usergart) { dev_err(dev->dev, "Tiled buffers require DMM\n"); return NULL; }
- /* tiled buffers are always shmem paged backed.. when they are - * scanned out, they are remapped into DMM/TILER + /* + * Tiled buffers are always shmem paged backed. When they are + * scanned out, they are remapped into DMM/TILER. */ flags &= ~OMAP_BO_SCANOUT; + flags |= OMAP_BO_MEM_SHMEM;
- /* currently don't allow cached buffers.. there is some caching - * stuff that needs to be handled better + /* + * Currently don't allow cached buffers. There is some caching + * stuff that needs to be handled better. */ flags &= ~(OMAP_BO_CACHED|OMAP_BO_WC|OMAP_BO_UNCACHED); flags |= tiler_get_cpu_cache_flags(); - - /* align dimensions to slot boundaries... */ - tiler_align(gem2fmt(flags), - &gsize.tiled.width, &gsize.tiled.height); - - /* ...and calculate size based on aligned dimensions */ - size = tiler_size(gem2fmt(flags), - gsize.tiled.width, gsize.tiled.height); - } else { - size = PAGE_ALIGN(gsize.bytes); + } else if ((flags & OMAP_BO_SCANOUT) && !priv->usergart) { + /* + * Use contiguous memory if we don't have DMM to remap + * discontiguous buffers. + */ + flags |= OMAP_BO_MEM_DMA_API; + } else if (!(flags & OMAP_BO_MEM_EXT)) { + /* + * All other buffers not backed with external memory are + * shmem-backed. + */ + flags |= OMAP_BO_MEM_SHMEM; }
+ /* Allocate the initialize the OMAP GEM object. */ omap_obj = kzalloc(sizeof(*omap_obj), GFP_KERNEL); if (!omap_obj) return NULL;
obj = &omap_obj->base; + omap_obj->flags = flags;
- if ((flags & OMAP_BO_SCANOUT) && !priv->usergart) { - /* attempt to allocate contiguous memory if we don't - * have DMM for remappign discontiguous buffers + if (flags & OMAP_BO_TILED) { + /* + * For tiled buffers align dimensions to slot boundaries and + * calculate size based on aligned dimensions. */ - omap_obj->vaddr = dma_alloc_writecombine(dev->dev, size, - &omap_obj->paddr, GFP_KERNEL); - if (!omap_obj->vaddr) { - kfree(omap_obj); + tiler_align(gem2fmt(flags), &gsize.tiled.width, + &gsize.tiled.height);
- return NULL; - } + size = tiler_size(gem2fmt(flags), gsize.tiled.width, + gsize.tiled.height);
- flags |= OMAP_BO_MEM_DMA_API; + omap_obj->width = gsize.tiled.width; + omap_obj->height = gsize.tiled.height; + } else { + size = PAGE_ALIGN(gsize.bytes); }
spin_lock(&priv->list_lock); list_add(&omap_obj->mm_list, &priv->obj_list); spin_unlock(&priv->list_lock);
- omap_obj->flags = flags; - - if (flags & OMAP_BO_TILED) { - omap_obj->width = gsize.tiled.width; - omap_obj->height = gsize.tiled.height; + /* Allocate memory if needed. */ + if (flags & OMAP_BO_MEM_DMA_API) { + omap_obj->vaddr = dma_alloc_writecombine(dev->dev, size, + &omap_obj->paddr, + GFP_KERNEL); + if (!omap_obj->vaddr) + goto fail; }
- if (flags & (OMAP_BO_MEM_DMA_API | OMAP_BO_MEM_EXT)) { + /* Initialize the GEM object. */ + if (!(flags & OMAP_BO_MEM_SHMEM)) { drm_gem_private_object_init(dev, obj, size); } else { ret = drm_gem_object_init(dev, obj, size); @@ -1414,8 +1427,6 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
mapping = file_inode(obj->filp)->i_mapping; mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32); - - omap_obj->flags |= OMAP_BO_MEM_SHMEM; }
return obj;
OMAP GEM objects backed by dma_buf reuse the current OMAP GEM object support as much as possible. If the imported buffer is physically contiguous its physical address will be used directly, reusing the OMAP_BO_MEM_DMA_API code paths. Otherwise it will be mapped through the TILER using a pages list created from the scatterlist instead of the shmem backing storage.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
Changes since v1:
- Drop the dma-buf reexport check, reexport is handled by the DRM core --- drivers/gpu/drm/omapdrm/omap_drv.h | 2 + drivers/gpu/drm/omapdrm/omap_gem.c | 138 ++++++++++++++++++++++++------ drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 53 +++++++++--- 3 files changed, 159 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 97fcb892fdd7..6615b7d51ee3 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -200,6 +200,8 @@ void omap_gem_deinit(struct drm_device *dev);
struct drm_gem_object *omap_gem_new(struct drm_device *dev, union omap_gem_size gsize, uint32_t flags); +struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, + struct sg_table *sgt); int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file, union omap_gem_size gsize, uint32_t flags, uint32_t *handle); void omap_gem_free_object(struct drm_gem_object *obj); diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 51580eaa37af..2beb78632279 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -33,6 +33,7 @@ #define OMAP_BO_MEM_DMA_API 0x01000000 /* memory allocated with the dma_alloc_* API */ #define OMAP_BO_MEM_SHMEM 0x02000000 /* memory allocated through shmem backing */ #define OMAP_BO_MEM_EXT 0x04000000 /* memory allocated externally */ +#define OMAP_BO_MEM_DMABUF 0x08000000 /* memory imported from a dmabuf */ #define OMAP_BO_EXT_SYNC 0x10000000 /* externally allocated sync object */
struct omap_gem_object { @@ -49,17 +50,25 @@ struct omap_gem_object { uint32_t roll;
/** - * If buffer is allocated physically contiguous, the OMAP_BO_MEM_DMA_API - * flag is set and the paddr is valid. Also if the buffer is remapped - * in TILER and paddr_cnt > 0, then paddr is valid. But if you are using - * the physical address and OMAP_BO_MEM_DMA_API is not set, then you - * should be going thru omap_gem_{get,put}_paddr() to ensure the mapping - * is not removed from under your feet. + * paddr contains the buffer DMA address. It is valid for * - * Note that OMAP_BO_SCANOUT is a hint from userspace that DMA capable - * buffer is requested, but doesn't mean that it is. Use the - * OMAP_BO_MEM_DMA_API flag to determine if the buffer has a DMA capable - * physical address. + * - buffers allocated through the DMA mapping API (with the + * OMAP_BO_MEM_DMA_API flag set) + * + * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set) + * if they are physically contiguous (when sgt->orig_nents == 1) + * + * - buffers mapped through the TILER when paddr_cnt is not zero, in + * which case the DMA address points to the TILER aperture + * + * Physically contiguous buffers have their DMA address equal to the + * physical address as we don't remap those buffers through the TILER. + * + * Buffers mapped to the TILER have their DMA address pointing to the + * TILER aperture. As TILER mappings are refcounted (through paddr_cnt) + * the DMA address must be accessed through omap_get_get_paddr() to + * ensure that the mapping won't disappear unexpectedly. References must + * be released with omap_gem_put_paddr(). */ dma_addr_t paddr;
@@ -69,6 +78,12 @@ struct omap_gem_object { uint32_t paddr_cnt;
/** + * If the buffer has been imported from a dmabuf the OMAP_DB_DMABUF flag + * is set and the sgt field is valid. + */ + struct sg_table *sgt; + + /** * tiler block used when buffer is remapped in DMM/TILER. */ struct tiler_block *block; @@ -166,6 +181,17 @@ static uint64_t mmap_offset(struct drm_gem_object *obj) return drm_vma_node_offset_addr(&obj->vma_node); }
+static bool is_contiguous(struct omap_gem_object *omap_obj) +{ + if (omap_obj->flags & OMAP_BO_MEM_DMA_API) + return true; + + if ((omap_obj->flags & OMAP_BO_MEM_DMABUF) && omap_obj->sgt->nents == 1) + return true; + + return false; +} + /* ----------------------------------------------------------------------------- * Eviction */ @@ -384,7 +410,7 @@ static int fault_1d(struct drm_gem_object *obj, omap_gem_cpu_sync(obj, pgoff); pfn = page_to_pfn(omap_obj->pages[pgoff]); } else { - BUG_ON(!(omap_obj->flags & OMAP_BO_MEM_DMA_API)); + BUG_ON(!is_contiguous(omap_obj)); pfn = (omap_obj->paddr >> PAGE_SHIFT) + pgoff; }
@@ -774,8 +800,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
mutex_lock(&obj->dev->struct_mutex);
- if (!(omap_obj->flags & OMAP_BO_MEM_DMA_API) && - remap && priv->usergart) { + if (!is_contiguous(omap_obj) && remap && priv->usergart) { if (omap_obj->paddr_cnt == 0) { struct page **pages; uint32_t npages = obj->size >> PAGE_SHIFT; @@ -822,7 +847,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj, omap_obj->paddr_cnt++;
*paddr = omap_obj->paddr; - } else if (omap_obj->flags & OMAP_BO_MEM_DMA_API) { + } else if (is_contiguous(omap_obj)) { *paddr = omap_obj->paddr; } else { ret = -EINVAL; @@ -1290,9 +1315,6 @@ unlock: * Constructor & Destructor */
-/* don't call directly.. called from GEM core when it is time to actually - * free the object.. - */ void omap_gem_free_object(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; @@ -1314,14 +1336,20 @@ void omap_gem_free_object(struct drm_gem_object *obj)
/* don't free externally allocated backing memory */ if (!(omap_obj->flags & OMAP_BO_MEM_EXT)) { - if (omap_obj->pages) - omap_gem_detach_pages(obj); + if (omap_obj->pages) { + if (omap_obj->flags & OMAP_BO_MEM_DMABUF) + kfree(omap_obj->pages); + else + omap_gem_detach_pages(obj); + }
if (omap_obj->flags & OMAP_BO_MEM_DMA_API) { dma_free_writecombine(dev->dev, obj->size, omap_obj->vaddr, omap_obj->paddr); } else if (omap_obj->vaddr) { vunmap(omap_obj->vaddr); + } else if (obj->import_attach) { + drm_prime_gem_destroy(obj, omap_obj->sgt); } }
@@ -1367,14 +1395,15 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev, flags |= tiler_get_cpu_cache_flags(); } else if ((flags & OMAP_BO_SCANOUT) && !priv->usergart) { /* - * Use contiguous memory if we don't have DMM to remap - * discontiguous buffers. + * OMAP_BO_SCANOUT hints that the buffer doesn't need to be + * tiled. However, to lower the pressure on memory allocation, + * use contiguous memory only if no TILER is available. */ flags |= OMAP_BO_MEM_DMA_API; - } else if (!(flags & OMAP_BO_MEM_EXT)) { + } else if (!(flags & (OMAP_BO_MEM_EXT | OMAP_BO_MEM_DMABUF))) { /* - * All other buffers not backed with external memory are - * shmem-backed. + * All other buffers not backed by external memory or dma_buf + * are shmem-backed. */ flags |= OMAP_BO_MEM_SHMEM; } @@ -1436,6 +1465,67 @@ fail: return NULL; }
+struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, + struct sg_table *sgt) +{ + struct omap_drm_private *priv = dev->dev_private; + struct omap_gem_object *omap_obj; + struct drm_gem_object *obj; + union omap_gem_size gsize; + + /* Without a DMM only physically contiguous buffers can be supported. */ + if (sgt->orig_nents != 1 && !priv->usergart) + return ERR_PTR(-EINVAL); + + mutex_lock(&dev->struct_mutex); + + gsize.bytes = PAGE_ALIGN(size); + obj = omap_gem_new(dev, gsize, OMAP_BO_MEM_DMABUF | OMAP_BO_WC); + if (!obj) { + obj = ERR_PTR(-ENOMEM); + goto done; + } + + omap_obj = to_omap_bo(obj); + omap_obj->sgt = sgt; + + if (sgt->orig_nents == 1) { + omap_obj->paddr = sg_dma_address(sgt->sgl); + } else { + /* Create pages list from sgt */ + struct sg_page_iter iter; + struct page **pages; + unsigned int npages; + unsigned int i = 0; + + npages = DIV_ROUND_UP(size, PAGE_SIZE); + pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL); + if (!pages) { + omap_gem_free_object(obj); + obj = ERR_PTR(-ENOMEM); + goto done; + } + + omap_obj->pages = pages; + + for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) { + pages[i++] = sg_page_iter_page(&iter); + if (i > npages) + break; + } + + if (WARN_ON(i != npages)) { + omap_gem_free_object(obj); + obj = ERR_PTR(-ENOMEM); + goto done; + } + } + +done: + mutex_unlock(&dev->struct_mutex); + return obj; +} + /* convenience method to construct a GEM buffer object, and userspace handle */ int omap_gem_new_handle(struct drm_device *dev, struct drm_file *file, union omap_gem_size gsize, uint32_t flags, uint32_t *handle) diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c index 27c297672076..19e426b698d3 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c @@ -21,6 +21,10 @@
#include "omap_drv.h"
+/* ----------------------------------------------------------------------------- + * DMABUF Export + */ + static struct sg_table *omap_gem_map_dma_buf( struct dma_buf_attachment *attachment, enum dma_data_direction dir) @@ -178,15 +182,20 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev, return dma_buf_export(&exp_info); }
+/* ----------------------------------------------------------------------------- + * DMABUF Import + */ + struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev, - struct dma_buf *buffer) + struct dma_buf *dma_buf) { + struct dma_buf_attachment *attach; struct drm_gem_object *obj; + struct sg_table *sgt; + int ret;
- /* is this one of own objects? */ - if (buffer->ops == &omap_dmabuf_ops) { - obj = buffer->priv; - /* is it from our device? */ + if (dma_buf->ops == &omap_dmabuf_ops) { + obj = dma_buf->priv; if (obj->dev == dev) { /* * Importing dmabuf exported from out own gem increases @@ -197,9 +206,33 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev, } }
- /* - * TODO add support for importing buffers from other devices.. - * for now we don't need this but would be nice to add eventually - */ - return ERR_PTR(-EINVAL); + attach = dma_buf_attach(dma_buf, dev->dev); + if (IS_ERR(attach)) + return ERR_CAST(attach); + + get_dma_buf(dma_buf); + + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR(sgt)) { + ret = PTR_ERR(sgt); + goto fail_detach; + } + + obj = omap_gem_new_dmabuf(dev, dma_buf->size, sgt); + if (IS_ERR(obj)) { + ret = PTR_ERR(obj); + goto fail_unmap; + } + + obj->import_attach = attach; + + return obj; + +fail_unmap: + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); +fail_detach: + dma_buf_detach(dma_buf, attach); + dma_buf_put(dma_buf); + + return ERR_PTR(ret); }
Hi Laurent,
On 14/12/15 22:39, Laurent Pinchart wrote:
Hello,
This patch series implements support for dma_buf import in the omapdrm driver. The patches are based on top of the latest drm-next branch and can be found in my git tree at
git://linuxtv.org/pinchartl/fbdev.git omapdrm/dmabuf-importThe first two patches are unrelated fixes and enhancements, but I've included them in the series to avoid merge conflicts.
The next 13 patches are miscellaneous fixes, cleanups and refactoring to prepare for patch 16/16 that implements dma_buf import.
The code has been successfully tested with the vivid driver as an exporter, using a hacked version that uses uncached CPU mappings in vivid when filling the buffers. vivid is a test driver that generates a test pattern using the CPU with cached mappings by default, resulting in corruption on the screen due to missing cache handling. As the problem doesn't occur when sharing buffers not touched by the CPU or touched through uncached mappings only, it will be addressed separately.
I've picked up these patches to my omapdrm-4.5 branch:
drm: omapdrm: move omap_plane_reset() drm: omapdrm: Fix plane state free in plane reset handler drm: omapdrm: Make fbdev emulation optional drm: omapdrm: gem: Remove unused function prototypes drm: omapdrm: gem: Remove forward declarations drm: omapdrm: gem: Group functions by purpose drm: omapdrm: gem: Move global usergart variable to omap_drm_private drm: omapdrm: gem: Mask out private flags passed from userspace drm: omapdrm: gem: Free the correct memory object drm: omapdrm: gem: Fix GEM object destroy in error path drm: omapdrm: gem: Don't free mmap offset twice drm: omapdrm: gem: Simplify error handling when creating GEM object drm: omapdrm: gem: Remove check for impossible condition
I did split the "Fix plane state free in plane reset handler" into two, moving the function first. And as I dropped few patches from between the others, they're not exactly as they were in your series.
The remaining patches are:
drm: omapdrm: gem: Remove omap_drm_private has_dmm field drm: omapdrm: gem: Clean up GEM objects memory flags drm: omapdrm: gem: Refactor GEM object allocation drm: omapdrm: gem: Implement dma_buf import
Those are more complex, and I haven't fully reviewed them yet. I wanted to get the bulk of the patches already, so that I can ensure we get at least those to 4.5.
Tomi
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Tomi Valkeinen