[PATCH 00/15] omapdrm: Implement dma_buf import

[PATCH 00/15] omapdrm: Implement dma_buf import

[Laurent Pinchart]

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 12 patches are miscellaneous fixes, cleanups and refactoring to
prepare for patch 15/15 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.

Laurent Pinchart (15):
  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: 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        |  15 +-
 drivers/gpu/drm/omapdrm/omap_drv.h        |  16 +-
 drivers/gpu/drm/omapdrm/omap_fbdev.c      |   6 +-
 drivers/gpu/drm/omapdrm/omap_gem.c        | 504 +++++++++++++++++++-----------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  57 +++-
 drivers/gpu/drm/omapdrm/omap_plane.c      |  55 ++--
 include/uapi/drm/omap_drm.h               |   1 +
 9 files changed, 426 insertions(+), 235 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/15] drm: omapdrm: Fix plane state free in plane reset handler

[Laurent Pinchart]

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,
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/15] drm: omapdrm: Make fbdev emulation optional

[Laurent Pinchart]

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: */
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/15] drm: omapdrm: gem: Remove unused function prototypes

[Laurent Pinchart]

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.
  */
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/15] drm: omapdrm: gem: Remove forward declarations

[Laurent Pinchart]

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'
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/15] drm: omapdrm: gem: Group functions by purpose

[Laurent Pinchart]

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;
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/15] drm: omapdrm: gem: Move global usergart variable to omap_drm_private

[Laurent Pinchart]

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);
 }
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/15] drm: omapdrm: gem: Remove omap_drm_private has_dmm field

[Laurent Pinchart]

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);
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Laurent Pinchart]

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>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
 include/uapi/drm/omap_drm.h        | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 83d63d71062c..e405ab23d7a6 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -551,10 +551,13 @@ 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,
diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
index 1d0b1172664e..6852c26e8f78 100644
--- a/include/uapi/drm/omap_drm.h
+++ b/include/uapi/drm/omap_drm.h
@@ -36,6 +36,7 @@ struct drm_omap_param {
 #define OMAP_BO_SCANOUT		0x00000001	/* scanout capable (phys contiguous) */
 #define OMAP_BO_CACHE_MASK	0x00000006	/* cache type mask, see cache modes */
 #define OMAP_BO_TILED_MASK	0x00000f00	/* tiled mapping mask, see tiled modes */
+#define OMAP_BO_USER_MASK	0x00ffffff	/* flags settable by userspace */
 
 /* cache modes */
 #define OMAP_BO_CACHED		0x00000000	/* default */
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/15] drm: omapdrm: gem: Clean up GEM objects memory flags

[Laurent Pinchart]

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;
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/15] drm: omapdrm: gem: Free the correct memory object

[Laurent Pinchart]

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 */
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/15] drm: omapdrm: gem: Don't free mmap offset twice

[Laurent Pinchart]

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 f24bb71d9946..5ba447d717c6 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..
 	 */
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 12/15] drm: omapdrm: gem: Simplify error handling when creating GEM object

[Laurent Pinchart]

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 5ba447d717c6..c2b0cdfd7798 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;
 }
 
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 13/15] drm: omapdrm: gem: Remove check for impossible condition

[Laurent Pinchart]

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 c2b0cdfd7798..a95b6486fbf6 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];
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 14/15] drm: omapdrm: gem: Refactor GEM object allocation

[Laurent Pinchart]

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 a95b6486fbf6..11df4f78d8a7 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;
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 15/15] drm: omapdrm: gem: Implement dma_buf import

[Laurent Pinchart]

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.

Disallow exporting imported buffers for now as those code paths haven't
been verified. Use cases of such a feature are suspicious anyway.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.h        |   2 +
 drivers/gpu/drm/omapdrm/omap_gem.c        | 138 ++++++++++++++++++++++++------
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  57 +++++++++---
 3 files changed, 163 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 11df4f78d8a7..8a54d333a47b 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..3f7d25a8baf1 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)
@@ -170,6 +174,10 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
 {
 	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
 
+	/* Don't allow exporting buffers that have been imported. */
+	if (obj->import_attach)
+		return ERR_PTR(-EINVAL);
+
 	exp_info.ops = &omap_dmabuf_ops;
 	exp_info.size = obj->size;
 	exp_info.flags = flags;
@@ -178,15 +186,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 +210,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);
 }
-- 
2.4.10

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/15] drm: omapdrm: gem: Implement dma_buf import

[Daniel Vetter]

On Sat, Dec 05, 2015 at 12:27:19AM +0200, Laurent Pinchart wrote:
> 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.
> 
> Disallow exporting imported buffers for now as those code paths haven't
> been verified. Use cases of such a feature are suspicious anyway.

If you export a buffer that's been imported the drm_prime.c helpers should
dig out the original dma-buf again. You should never end up with a dma-buf
-> omap gem bo -> dma-buf chain.

If that doesn't work then there's a bug somewhere ...
-Daniel

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.h        |   2 +
>  drivers/gpu/drm/omapdrm/omap_gem.c        | 138 ++++++++++++++++++++++++------
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  57 +++++++++---
>  3 files changed, 163 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 11df4f78d8a7..8a54d333a47b 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..3f7d25a8baf1 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)
> @@ -170,6 +174,10 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
>  {
>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>  
> +	/* Don't allow exporting buffers that have been imported. */
> +	if (obj->import_attach)
> +		return ERR_PTR(-EINVAL);
> +
>  	exp_info.ops = &omap_dmabuf_ops;
>  	exp_info.size = obj->size;
>  	exp_info.flags = flags;
> @@ -178,15 +186,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 +210,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);
>  }
> -- 
> 2.4.10
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 15/15] drm: omapdrm: gem: Implement dma_buf import

[Laurent Pinchart]

Hi Daniel,

On Saturday 05 December 2015 16:40:14 Daniel Vetter wrote:
> On Sat, Dec 05, 2015 at 12:27:19AM +0200, Laurent Pinchart wrote:
> > 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.
> > 
> > Disallow exporting imported buffers for now as those code paths haven't
> > been verified. Use cases of such a feature are suspicious anyway.
> 
> If you export a buffer that's been imported the drm_prime.c helpers should
> dig out the original dma-buf again. You should never end up with a dma-buf
> -> omap gem bo -> dma-buf chain.
> 
> If that doesn't work then there's a bug somewhere ...

Very good point. I'll drop the check. Thank you for the review.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h        |   2 +
> >  drivers/gpu/drm/omapdrm/omap_gem.c        | 138 +++++++++++++++++++------
> >  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c |  57 +++++++++---
> >  3 files changed, 163 insertions(+), 34 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Emil Velikov]

On 4 December 2015 at 22:27, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> The 8 high order bits of the buffer flags are reserved for internal use.
> Mask them out from the flags passed by userspace.
>
Ouch... I know that the Intel guys are pretty rigorous about input
validation, although I wonder how many drivers are (partially or not)
missing these.

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
>  include/uapi/drm/omap_drm.h        | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 83d63d71062c..e405ab23d7a6 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -551,10 +551,13 @@ 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,
> diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
> index 1d0b1172664e..6852c26e8f78 100644
> --- a/include/uapi/drm/omap_drm.h
> +++ b/include/uapi/drm/omap_drm.h
> @@ -36,6 +36,7 @@ struct drm_omap_param {
>  #define OMAP_BO_SCANOUT                0x00000001      /* scanout capable (phys contiguous) */
>  #define OMAP_BO_CACHE_MASK     0x00000006      /* cache type mask, see cache modes */
>  #define OMAP_BO_TILED_MASK     0x00000f00      /* tiled mapping mask, see tiled modes */
> +#define OMAP_BO_USER_MASK      0x00ffffff      /* flags settable by userspace */
>
Fwiw I'm not too sure that adding the mask here  (UAPI) is a good
idea. If you like to keep it, I'd suggest that it encapsulates only
what's currently available. Might even want to move the individual
masks in the respective sections/hunks. Speaking of which  -
OMAP_BO_TILED_MASK should be 0x00000300, shouldn't it ?

Regards,
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] drm: omapdrm: Fix plane state free in plane reset handler

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 3087 bytes --]



On 05/12/15 00:27, Laurent Pinchart wrote:
> 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,
> 

This one moves the function, so it's pretty hard to see what actually
was changed.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] drm: omapdrm: Fix plane state free in plane reset handler

[Laurent Pinchart]

Hi Tomi,

On Wednesday 09 December 2015 14:43:19 Tomi Valkeinen wrote:
> On 05/12/15 00:27, Laurent Pinchart wrote:
> > 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,
> 
> This one moves the function, so it's pretty hard to see what actually
> was changed.

It's unfortunately needed if we want to avoid forward declarations. I could 
split the patch in two, but given the size of the function and the extent of 
the change I thought it wouldn't be worth it.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/15] drm: omapdrm: Fix plane state free in plane reset handler

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 636 bytes --]


On 13/12/15 22:39, Laurent Pinchart wrote:

>> This one moves the function, so it's pretty hard to see what actually
>> was changed.
> 
> It's unfortunately needed if we want to avoid forward declarations. I could 
> split the patch in two, but given the size of the function and the extent of 
> the change I thought it wouldn't be worth it.

Maybe not, if everything goes perfectly and the patch is never applied
to any other kernel tree.

But if there is a single change to the function, or to the surrounding
lines, having the fix and the move in the same patch makes resolving the
conflict much harder.

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/15] drm: omapdrm: gem: Free the correct memory object

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1092 bytes --]



On 05/12/15 00:27, Laurent Pinchart wrote:
> 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 */
> 

There's another kfree(obj) in omap_gem_new_handle().

 Tomi


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/15] drm: omapdrm: gem: Free the correct memory object

[Laurent Pinchart]

Hi Tomi,

On Monday 14 December 2015 13:45:44 Tomi Valkeinen wrote:
> On 05/12/15 00:27, Laurent Pinchart wrote:
> > 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 */
> 
> There's another kfree(obj) in omap_gem_new_handle().

Good point. I'll add a patch to fix that and submit v2.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Laurent Pinchart]

Hi Emil,

On Monday 07 December 2015 14:13:43 Emil Velikov wrote:
> On 4 December 2015 at 22:27, Laurent Pinchart wrote:
> > The 8 high order bits of the buffer flags are reserved for internal use.
> > Mask them out from the flags passed by userspace.
> 
> Ouch... I know that the Intel guys are pretty rigorous about input
> validation, although I wonder how many drivers are (partially or not)
> missing these.
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
> >  include/uapi/drm/omap_drm.h        | 1 +
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 83d63d71062c..e405ab23d7a6
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > @@ -551,10 +551,13 @@ 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,
> > diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
> > index 1d0b1172664e..6852c26e8f78 100644
> > --- a/include/uapi/drm/omap_drm.h
> > +++ b/include/uapi/drm/omap_drm.h
> > @@ -36,6 +36,7 @@ struct drm_omap_param {
> > 
> >  #define OMAP_BO_SCANOUT        0x00000001      /* scanout capable (phys
> >  contiguous) */
> >  #define OMAP_BO_CACHE_MASK     0x00000006      /* cache type mask, see
> >  cache modes */
> >  #define OMAP_BO_TILED_MASK     0x00000f00      /* tiled mapping mask, see
> >  tiled modes */
> > +#define OMAP_BO_USER_MASK      0x00ffffff      /* flags settable by
> > userspace */
>
> Fwiw I'm not too sure that adding the mask here  (UAPI) is a good
> idea.

Good point, I'll make it private.

> If you like to keep it, I'd suggest that it encapsulates only what's
> currently available. Might even want to move the individual masks in the
> respective sections/hunks. Speaking of which - OMAP_BO_TILED_MASK should be
> 0x00000300, shouldn't it ?

I suppose 0x00000f00 was chosen to reserve a couple of bits for additional 
tiled modes.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Emil Velikov]

Hi Laurent,

On 14 December 2015 at 20:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Emil,
>
> On Monday 07 December 2015 14:13:43 Emil Velikov wrote:
>> On 4 December 2015 at 22:27, Laurent Pinchart wrote:
>> > The 8 high order bits of the buffer flags are reserved for internal use.
>> > Mask them out from the flags passed by userspace.
>>
>> Ouch... I know that the Intel guys are pretty rigorous about input
>> validation, although I wonder how many drivers are (partially or not)
>> missing these.
>>
>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > ---
>> >
>> >  drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
>> >  include/uapi/drm/omap_drm.h        | 1 +
>> >  2 files changed, 7 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 83d63d71062c..e405ab23d7a6
>> > 100644
>> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>> > @@ -551,10 +551,13 @@ 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,
>> > diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
>> > index 1d0b1172664e..6852c26e8f78 100644
>> > --- a/include/uapi/drm/omap_drm.h
>> > +++ b/include/uapi/drm/omap_drm.h
>> > @@ -36,6 +36,7 @@ struct drm_omap_param {
>> >
>> >  #define OMAP_BO_SCANOUT        0x00000001      /* scanout capable (phys
>> >  contiguous) */
>> >  #define OMAP_BO_CACHE_MASK     0x00000006      /* cache type mask, see
>> >  cache modes */
>> >  #define OMAP_BO_TILED_MASK     0x00000f00      /* tiled mapping mask, see
>> >  tiled modes */
>> > +#define OMAP_BO_USER_MASK      0x00ffffff      /* flags settable by
>> > userspace */
>>
>> Fwiw I'm not too sure that adding the mask here  (UAPI) is a good
>> idea.
>
> Good point, I'll make it private.
>
Thanks for the update (hiding the extra define from UAPI). Shame we
cannot do that for OMAP_BO_TILED_MASK :'-(

>> If you like to keep it, I'd suggest that it encapsulates only what's
>> currently available. Might even want to move the individual masks in the
>> respective sections/hunks. Speaking of which - OMAP_BO_TILED_MASK should be
>> 0x00000300, shouldn't it ?
>
> I suppose 0x00000f00 was chosen to reserve a couple of bits for additional
> tiled modes.
>
Perhaps a silly question - what is the reason to have larger mask than
the actual supported flags ? Looking from other drm modules (and vague
memories of other subsystems), drivers do not try to be forward
compatible and reject (should reject that is) unsupported flags.

Am I missing something ?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/15] drm: omapdrm: gem: Mask out private flags passed from userspace

[Rob Clark]

On Wed, Dec 16, 2015 at 12:33 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> Hi Laurent,
>
> On 14 December 2015 at 20:33, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Emil,
>>
>> On Monday 07 December 2015 14:13:43 Emil Velikov wrote:
>>> On 4 December 2015 at 22:27, Laurent Pinchart wrote:
>>> > The 8 high order bits of the buffer flags are reserved for internal use.
>>> > Mask them out from the flags passed by userspace.
>>>
>>> Ouch... I know that the Intel guys are pretty rigorous about input
>>> validation, although I wonder how many drivers are (partially or not)
>>> missing these.
>>>
>>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> > ---
>>> >
>>> >  drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++++++---
>>> >  include/uapi/drm/omap_drm.h        | 1 +
>>> >  2 files changed, 7 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 83d63d71062c..e405ab23d7a6
>>> > 100644
>>> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
>>> > @@ -551,10 +551,13 @@ 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,
>>> > diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h
>>> > index 1d0b1172664e..6852c26e8f78 100644
>>> > --- a/include/uapi/drm/omap_drm.h
>>> > +++ b/include/uapi/drm/omap_drm.h
>>> > @@ -36,6 +36,7 @@ struct drm_omap_param {
>>> >
>>> >  #define OMAP_BO_SCANOUT        0x00000001      /* scanout capable (phys
>>> >  contiguous) */
>>> >  #define OMAP_BO_CACHE_MASK     0x00000006      /* cache type mask, see
>>> >  cache modes */
>>> >  #define OMAP_BO_TILED_MASK     0x00000f00      /* tiled mapping mask, see
>>> >  tiled modes */
>>> > +#define OMAP_BO_USER_MASK      0x00ffffff      /* flags settable by
>>> > userspace */
>>>
>>> Fwiw I'm not too sure that adding the mask here  (UAPI) is a good
>>> idea.
>>
>> Good point, I'll make it private.
>>
> Thanks for the update (hiding the extra define from UAPI). Shame we
> cannot do that for OMAP_BO_TILED_MASK :'-(
>
>>> If you like to keep it, I'd suggest that it encapsulates only what's
>>> currently available. Might even want to move the individual masks in the
>>> respective sections/hunks. Speaking of which - OMAP_BO_TILED_MASK should be
>>> 0x00000300, shouldn't it ?
>>
>> I suppose 0x00000f00 was chosen to reserve a couple of bits for additional
>> tiled modes.
>>
> Perhaps a silly question - what is the reason to have larger mask than
> the actual supported flags ? Looking from other drm modules (and vague
> memories of other subsystems), drivers do not try to be forward
> compatible and reject (should reject that is) unsupported flags.

yeah, it was chosen to reserve some extra bits if needed in the future..

but yeah, current kernel should probably reject unknown tile modes..
which I think should be easy since iirc there are only 3 (which is a
nice maskable number)

BR,
-R

> Am I missing something ?
>
> Thanks
> Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel