[PATCH 0/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[PATCH 0/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Drivers currently treat the IOCTL data for DRM_IOCTL_MODE_CREATE_DUMB
inconsistently. While the documentation clearly states that the handle,
pitch and size fields are outputs, some drivers use their values as a
minimum hint from userspace, presumably to allow userspace to over-
allocate buffers on purpose (for example after negotiating pitch
alignment requirements with other devices).

Most of userspace is well-behaved in that in zeros out the output fields
which works well with drivers that use them as minima. However, some
userspace (like Mesa's GBM DRI backend) only initializes the inputs, so
that the driver will end up using uninitialized data from the stack in
the computations. This can cause the driver to attempt very large
allocations, resulting in failure, or silently use excessively  large
buffers.

This series tries to fix this by sanitizing the IOCTL data (setting the
output fields to 0 in the kernel) so that drivers can no longer use
uninitialized data. While at it, it also fixes existing drivers to not
treat output fields as input/output for consistency.

Note that this is technically breaking ABI since overallocating will no
longer be possible after this series is applied. However, the public DRM
headers have always documented the fields as being outputs, so any
application relying on them being inputs could be considered buggy. The
hope is that no such applications exist in the wild.

Discussion on IRC lead to the conclusion that new IOCTLs should have
input validation and require userspace to zero out output parameters to
avoid this kind of mess in the future. In order to help avoid this kind
of ambiguity it would be a good idea to start documenting IOCTLs more
officially (e.g. in the DRM DocBook).

I'm adding a bunch of people on Cc to get feedback about what userspace
people know might be relying on the current behaviour. Drivers that are
impacted are OMAP, R-Car and any that use GEM CMA helpers.

This series also contains a couple of preparatory patches that add the
GEM CMA helpers to our DocBook.

Thierry

Thierry Reding (7):
  drm/gem: Fix a few kerneldoc typos
  drm/doc: mm: Fix indentation
  drm/doc: Add GEM/CMA helpers to kerneldoc
  drm/cma: Introduce drm_gem_cma_dumb_create_internal()
  drm/omap: gem: dumb: pitch is an output
  drm/rcar: gem: dumb: pitch is an output
  drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

 Documentation/DocBook/drm.tmpl        | 274 +++++++++++++++++-----------------
 drivers/gpu/drm/drm_crtc.c            |   8 +
 drivers/gpu/drm/drm_gem.c             |  11 +-
 drivers/gpu/drm/drm_gem_cma_helper.c  | 182 +++++++++++++++++-----
 drivers/gpu/drm/omapdrm/omap_gem.c    |   2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |   4 +-
 include/drm/drm_gem_cma_helper.h      |  30 +++-
 7 files changed, 319 insertions(+), 192 deletions(-)

-- 
2.1.3

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

[PATCH 1/7] drm/gem: Fix a few kerneldoc typos

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

While at it, adjust the drm_gem_handle_create() function declaration to
be more consistent with other functions in the file.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_gem.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index f6ca51259fa3..597318ae307e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -309,7 +309,7 @@ EXPORT_SYMBOL(drm_gem_dumb_destroy);
  * drm_gem_handle_create_tail - internal functions to create a handle
  * @file_priv: drm file-private structure to register the handle for
  * @obj: object to register
- * @handlep: pionter to return the created handle to the caller
+ * @handlep: pointer to return the created handle to the caller
  * 
  * This expects the dev->object_name_lock to be held already and will drop it
  * before returning. Used to avoid races in establishing new handles when
@@ -362,7 +362,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 }
 
 /**
- * gem_handle_create - create a gem handle for an object
+ * drm_gem_handle_create - create a gem handle for an object
  * @file_priv: drm file-private structure to register the handle for
  * @obj: object to register
  * @handlep: pionter to return the created handle to the caller
@@ -371,10 +371,9 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
  * to the object, which includes a regular reference count. Callers
  * will likely want to dereference the object afterwards.
  */
-int
-drm_gem_handle_create(struct drm_file *file_priv,
-		       struct drm_gem_object *obj,
-		       u32 *handlep)
+int drm_gem_handle_create(struct drm_file *file_priv,
+			  struct drm_gem_object *obj,
+			  u32 *handlep)
 {
 	mutex_lock(&obj->dev->object_name_lock);
 
-- 
2.1.3

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

[PATCH 2/7] drm/doc: mm: Fix indentation

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use spaces consistently for indentation in the memory-management
section.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/DocBook/drm.tmpl | 268 ++++++++++++++++++++---------------------
 1 file changed, 134 insertions(+), 134 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index f6a9d7b21380..18025496a736 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -492,10 +492,10 @@ char *date;</synopsis>
     <sect2>
       <title>The Translation Table Manager (TTM)</title>
       <para>
-	TTM design background and information belongs here.
+        TTM design background and information belongs here.
       </para>
       <sect3>
-	<title>TTM initialization</title>
+        <title>TTM initialization</title>
         <warning><para>This section is outdated.</para></warning>
         <para>
           Drivers wishing to support TTM must fill out a drm_bo_driver
@@ -503,42 +503,42 @@ char *date;</synopsis>
           pointers for initializing the TTM, allocating and freeing memory,
           waiting for command completion and fence synchronization, and memory
           migration. See the radeon_ttm.c file for an example of usage.
-	</para>
-	<para>
-	  The ttm_global_reference structure is made up of several fields:
-	</para>
-	<programlisting>
-	  struct ttm_global_reference {
-	  	enum ttm_global_types global_type;
-	  	size_t size;
-	  	void *object;
-	  	int (*init) (struct ttm_global_reference *);
-	  	void (*release) (struct ttm_global_reference *);
-	  };
-	</programlisting>
-	<para>
-	  There should be one global reference structure for your memory
-	  manager as a whole, and there will be others for each object
-	  created by the memory manager at runtime.  Your global TTM should
-	  have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
-	  object should be sizeof(struct ttm_mem_global), and the init and
-	  release hooks should point at your driver-specific init and
-	  release routines, which probably eventually call
-	  ttm_mem_global_init and ttm_mem_global_release, respectively.
-	</para>
-	<para>
-	  Once your global TTM accounting structure is set up and initialized
-	  by calling ttm_global_item_ref() on it,
-	  you need to create a buffer object TTM to
-	  provide a pool for buffer object allocation by clients and the
-	  kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
-	  and its size should be sizeof(struct ttm_bo_global).  Again,
-	  driver-specific init and release functions may be provided,
-	  likely eventually calling ttm_bo_global_init() and
-	  ttm_bo_global_release(), respectively.  Also, like the previous
-	  object, ttm_global_item_ref() is used to create an initial reference
-	  count for the TTM, which will call your initialization function.
-	</para>
+        </para>
+        <para>
+          The ttm_global_reference structure is made up of several fields:
+        </para>
+        <programlisting>
+          struct ttm_global_reference {
+                  enum ttm_global_types global_type;
+                  size_t size;
+                  void *object;
+                  int (*init) (struct ttm_global_reference *);
+                  void (*release) (struct ttm_global_reference *);
+          };
+        </programlisting>
+        <para>
+          There should be one global reference structure for your memory
+          manager as a whole, and there will be others for each object
+          created by the memory manager at runtime.  Your global TTM should
+          have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
+          object should be sizeof(struct ttm_mem_global), and the init and
+          release hooks should point at your driver-specific init and
+          release routines, which probably eventually call
+          ttm_mem_global_init and ttm_mem_global_release, respectively.
+        </para>
+        <para>
+          Once your global TTM accounting structure is set up and initialized
+          by calling ttm_global_item_ref() on it,
+          you need to create a buffer object TTM to
+          provide a pool for buffer object allocation by clients and the
+          kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
+          and its size should be sizeof(struct ttm_bo_global).  Again,
+          driver-specific init and release functions may be provided,
+          likely eventually calling ttm_bo_global_init() and
+          ttm_bo_global_release(), respectively.  Also, like the previous
+          object, ttm_global_item_ref() is used to create an initial reference
+          count for the TTM, which will call your initialization function.
+        </para>
       </sect3>
     </sect2>
     <sect2 id="drm-gem">
@@ -566,19 +566,19 @@ char *date;</synopsis>
         using driver-specific ioctls.
       </para>
       <para>
-	On a fundamental level, GEM involves several operations:
-	<itemizedlist>
-	  <listitem>Memory allocation and freeing</listitem>
-	  <listitem>Command execution</listitem>
-	  <listitem>Aperture management at command execution time</listitem>
-	</itemizedlist>
-	Buffer object allocation is relatively straightforward and largely
+        On a fundamental level, GEM involves several operations:
+        <itemizedlist>
+          <listitem>Memory allocation and freeing</listitem>
+          <listitem>Command execution</listitem>
+          <listitem>Aperture management at command execution time</listitem>
+        </itemizedlist>
+        Buffer object allocation is relatively straightforward and largely
         provided by Linux's shmem layer, which provides memory to back each
         object.
       </para>
       <para>
         Device-specific operations, such as command execution, pinning, buffer
-	read &amp; write, mapping, and domain ownership transfers are left to
+        read &amp; write, mapping, and domain ownership transfers are left to
         driver-specific ioctls.
       </para>
       <sect3>
@@ -738,16 +738,16 @@ char *date;</synopsis>
           respectively. The conversion is handled by the DRM core without any
           driver-specific support.
         </para>
-	<para>
-	  GEM also supports buffer sharing with dma-buf file descriptors through
-	  PRIME. GEM-based drivers must use the provided helpers functions to
-	  implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
-	  Since sharing file descriptors is inherently more secure than the
-	  easily guessable and global GEM names it is the preferred buffer
-	  sharing mechanism. Sharing buffers through GEM names is only supported
-	  for legacy userspace. Furthermore PRIME also allows cross-device
-	  buffer sharing since it is based on dma-bufs.
-	</para>
+        <para>
+          GEM also supports buffer sharing with dma-buf file descriptors through
+          PRIME. GEM-based drivers must use the provided helpers functions to
+          implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
+          Since sharing file descriptors is inherently more secure than the
+          easily guessable and global GEM names it is the preferred buffer
+          sharing mechanism. Sharing buffers through GEM names is only supported
+          for legacy userspace. Furthermore PRIME also allows cross-device
+          buffer sharing since it is based on dma-bufs.
+        </para>
       </sect3>
       <sect3 id="drm-gem-objects-mapping">
         <title>GEM Objects Mapping</title>
@@ -852,7 +852,7 @@ char *date;</synopsis>
       <sect3>
         <title>Command Execution</title>
         <para>
-	  Perhaps the most important GEM function for GPU devices is providing a
+          Perhaps the most important GEM function for GPU devices is providing a
           command execution interface to clients. Client programs construct
           command buffers containing references to previously allocated memory
           objects, and then submit them to GEM. At that point, GEM takes care to
@@ -874,95 +874,95 @@ char *date;</synopsis>
         <title>GEM Function Reference</title>
 !Edrivers/gpu/drm/drm_gem.c
       </sect3>
-      </sect2>
-      <sect2>
-	<title>VMA Offset Manager</title>
+    </sect2>
+    <sect2>
+      <title>VMA Offset Manager</title>
 !Pdrivers/gpu/drm/drm_vma_manager.c vma offset manager
 !Edrivers/gpu/drm/drm_vma_manager.c
 !Iinclude/drm/drm_vma_manager.h
-      </sect2>
-      <sect2 id="drm-prime-support">
-	<title>PRIME Buffer Sharing</title>
-	<para>
-	  PRIME is the cross device buffer sharing framework in drm, originally
-	  created for the OPTIMUS range of multi-gpu platforms. To userspace
-	  PRIME buffers are dma-buf based file descriptors.
-	</para>
-	<sect3>
-	  <title>Overview and Driver Interface</title>
-	  <para>
-	    Similar to GEM global names, PRIME file descriptors are
-	    also used to share buffer objects across processes. They offer
-	    additional security: as file descriptors must be explicitly sent over
-	    UNIX domain sockets to be shared between applications, they can't be
-	    guessed like the globally unique GEM names.
-	  </para>
-	  <para>
-	    Drivers that support the PRIME
-	    API must set the DRIVER_PRIME bit in the struct
-	    <structname>drm_driver</structname>
-	    <structfield>driver_features</structfield> field, and implement the
-	    <methodname>prime_handle_to_fd</methodname> and
-	    <methodname>prime_fd_to_handle</methodname> operations.
-	  </para>
-	  <para>
-	    <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
-			  struct drm_file *file_priv, uint32_t handle,
-			  uint32_t flags, int *prime_fd);
+    </sect2>
+    <sect2 id="drm-prime-support">
+      <title>PRIME Buffer Sharing</title>
+      <para>
+        PRIME is the cross device buffer sharing framework in drm, originally
+        created for the OPTIMUS range of multi-gpu platforms. To userspace
+        PRIME buffers are dma-buf based file descriptors.
+      </para>
+      <sect3>
+        <title>Overview and Driver Interface</title>
+        <para>
+          Similar to GEM global names, PRIME file descriptors are
+          also used to share buffer objects across processes. They offer
+          additional security: as file descriptors must be explicitly sent over
+          UNIX domain sockets to be shared between applications, they can't be
+          guessed like the globally unique GEM names.
+        </para>
+        <para>
+          Drivers that support the PRIME
+          API must set the DRIVER_PRIME bit in the struct
+          <structname>drm_driver</structname>
+          <structfield>driver_features</structfield> field, and implement the
+          <methodname>prime_handle_to_fd</methodname> and
+          <methodname>prime_fd_to_handle</methodname> operations.
+        </para>
+        <para>
+          <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
+                          struct drm_file *file_priv, uint32_t handle,
+                          uint32_t flags, int *prime_fd);
 int (*prime_fd_to_handle)(struct drm_device *dev,
-			  struct drm_file *file_priv, int prime_fd,
-			  uint32_t *handle);</synopsis>
-	    Those two operations convert a handle to a PRIME file descriptor and
-	    vice versa. Drivers must use the kernel dma-buf buffer sharing framework
-	    to manage the PRIME file descriptors. Similar to the mode setting
-	    API PRIME is agnostic to the underlying buffer object manager, as
-	    long as handles are 32bit unsigned integers.
-	  </para>
-	  <para>
-	    While non-GEM drivers must implement the operations themselves, GEM
-	    drivers must use the <function>drm_gem_prime_handle_to_fd</function>
-	    and <function>drm_gem_prime_fd_to_handle</function> helper functions.
-	    Those helpers rely on the driver
-	    <methodname>gem_prime_export</methodname> and
-	    <methodname>gem_prime_import</methodname> operations to create a dma-buf
-	    instance from a GEM object (dma-buf exporter role) and to create a GEM
-	    object from a dma-buf instance (dma-buf importer role).
-	  </para>
-	  <para>
-	    <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
-				     struct drm_gem_object *obj,
-				     int flags);
+                          struct drm_file *file_priv, int prime_fd,
+                          uint32_t *handle);</synopsis>
+            Those two operations convert a handle to a PRIME file descriptor and
+            vice versa. Drivers must use the kernel dma-buf buffer sharing framework
+            to manage the PRIME file descriptors. Similar to the mode setting
+            API PRIME is agnostic to the underlying buffer object manager, as
+            long as handles are 32bit unsigned integers.
+          </para>
+          <para>
+            While non-GEM drivers must implement the operations themselves, GEM
+            drivers must use the <function>drm_gem_prime_handle_to_fd</function>
+            and <function>drm_gem_prime_fd_to_handle</function> helper functions.
+            Those helpers rely on the driver
+            <methodname>gem_prime_export</methodname> and
+            <methodname>gem_prime_import</methodname> operations to create a dma-buf
+            instance from a GEM object (dma-buf exporter role) and to create a GEM
+            object from a dma-buf instance (dma-buf importer role).
+          </para>
+          <para>
+            <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
+                             struct drm_gem_object *obj,
+                             int flags);
 struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
-					    struct dma_buf *dma_buf);</synopsis>
-	    These two operations are mandatory for GEM drivers that support
-	    PRIME.
-	  </para>
-	</sect3>
-        <sect3>
-          <title>PRIME Helper Functions</title>
-!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+                                            struct dma_buf *dma_buf);</synopsis>
+            These two operations are mandatory for GEM drivers that support
+            PRIME.
+          </para>
         </sect3>
-      </sect2>
-      <sect2>
-	<title>PRIME Function References</title>
+      <sect3>
+        <title>PRIME Helper Functions</title>
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+      </sect3>
+    </sect2>
+    <sect2>
+      <title>PRIME Function References</title>
 !Edrivers/gpu/drm/drm_prime.c
-      </sect2>
-      <sect2>
-	<title>DRM MM Range Allocator</title>
-	<sect3>
-	  <title>Overview</title>
+    </sect2>
+    <sect2>
+      <title>DRM MM Range Allocator</title>
+      <sect3>
+        <title>Overview</title>
 !Pdrivers/gpu/drm/drm_mm.c Overview
-	</sect3>
-	<sect3>
-	  <title>LRU Scan/Eviction Support</title>
+      </sect3>
+      <sect3>
+        <title>LRU Scan/Eviction Support</title>
 !Pdrivers/gpu/drm/drm_mm.c lru scan roaster
-	</sect3>
+      </sect3>
       </sect2>
-      <sect2>
-	<title>DRM MM Range Allocator Function References</title>
+    <sect2>
+      <title>DRM MM Range Allocator Function References</title>
 !Edrivers/gpu/drm/drm_mm.c
 !Iinclude/drm/drm_mm.h
-      </sect2>
+    </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
-- 
2.1.3

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

[PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Most of the functions already have the beginnings of kerneldoc comments
but are using the wrong opening marker. Use the correct opening marker
and flesh out the comments so that they can be integrated with the DRM
DocBook document.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/DocBook/drm.tmpl       |   6 ++
 drivers/gpu/drm/drm_gem_cma_helper.c | 155 ++++++++++++++++++++++++++---------
 include/drm/drm_gem_cma_helper.h     |  25 ++++--
 3 files changed, 139 insertions(+), 47 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 18025496a736..666a210af121 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 !Edrivers/gpu/drm/drm_mm.c
 !Iinclude/drm/drm_mm.h
     </sect2>
+    <sect2>
+      <title>CMA Helper Functions Reference</title>
+!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
+!Edrivers/gpu/drm/drm_gem_cma_helper.c
+!Iinclude/drm/drm_gem_cma_helper.h
+    </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 0316310e2cc4..55306be1f8f7 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -29,18 +29,30 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_vma_manager.h>
 
-/*
+/**
+ * DOC: cma helpers
+ *
+ * The Contiguous Memory Allocator reserves a pool of memory at early boot
+ * that is used to service requests for large blocks of contiguous memory.
+ *
+ * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
+ * objects that are physically contiguous in memory. This is useful for
+ * display drivers that are unable to map scattered buffers via an IOMMU.
+ */
+
+/**
  * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
- * @drm: The drm device
- * @size: The GEM object size
+ * @drm: DRM device
+ * @size: size of the object to allocate
  *
- * This function creates and initializes a GEM CMA object of the given size, but
- * doesn't allocate any memory to back the object.
+ * This function creates and initializes a GEM CMA object of the given size,
+ * but doesn't allocate any memory to back the object.
  *
- * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
+ * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
+ * negative error code on failure.
  */
 static struct drm_gem_cma_object *
-__drm_gem_cma_create(struct drm_device *drm, unsigned int size)
+__drm_gem_cma_create(struct drm_device *drm, size_t size)
 {
 	struct drm_gem_cma_object *cma_obj;
 	struct drm_gem_object *gem_obj;
@@ -69,14 +81,16 @@ error:
 	return ERR_PTR(ret);
 }
 
-/*
+/**
  * drm_gem_cma_create - allocate an object with the given size
+ * @drm: DRM device
+ * @size: size of the object to allocate
  *
- * returns a struct drm_gem_cma_object* on success or ERR_PTR values
- * on failure.
+ * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
+ * negative error code on failure.
  */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
-		unsigned int size)
+					      size_t size)
 {
 	struct drm_gem_cma_object *cma_obj;
 	int ret;
@@ -104,17 +118,21 @@ error:
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_create);
 
-/*
- * drm_gem_cma_create_with_handle - allocate an object with the given
- * size and create a gem handle on it
+/**
+ * drm_gem_cma_create_with_handle - allocate an object with the given size and
+ *     return a GEM handle to it
+ * @file_priv: DRM file-private structure to register the handle for
+ * @drm: DRM device
+ * @size: size of the object to allocate
+ * @handle: return location for the GEM handle
  *
- * returns a struct drm_gem_cma_object* on success or ERR_PTR values
- * on failure.
+ * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
+ * negative error code on failure.
  */
-static struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
-		struct drm_file *file_priv,
-		struct drm_device *drm, unsigned int size,
-		unsigned int *handle)
+static struct drm_gem_cma_object *
+drm_gem_cma_create_with_handle(struct drm_file *file_priv,
+			       struct drm_device *drm, size_t size,
+			       uint32_t *handle)
 {
 	struct drm_gem_cma_object *cma_obj;
 	struct drm_gem_object *gem_obj;
@@ -145,9 +163,9 @@ err_handle_create:
 	return ERR_PTR(ret);
 }
 
-/*
- * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
- * function
+/**
+ * drm_gem_cma_free_object - free resources associated with a CMA GEM object
+ * @gem_obj: GEM object to free
  */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 {
@@ -170,15 +188,20 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
 
-/*
- * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
- * function
+/**
+ * drm_gem_cma_dumb_create - create a dumb buffer object
+ * @file_priv: DRM file-private structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
  *
- * This aligns the pitch and size arguments to the minimum required. wrap
+ * This aligns the pitch and size arguments to the minimum required. Wrap
  * this into your own function if you need bigger alignment.
+ *
+ * Return: 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-		struct drm_device *dev, struct drm_mode_create_dumb *args)
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args)
 {
 	struct drm_gem_cma_object *cma_obj;
 	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
@@ -189,18 +212,25 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 	if (args->size < args->pitch * args->height)
 		args->size = args->pitch * args->height;
 
-	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
-			args->size, &args->handle);
+	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
+						 &args->handle);
 	return PTR_ERR_OR_ZERO(cma_obj);
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
 
-/*
- * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
- * function
+/**
+ * drm_gem_cma_dumb_map_offset - return the fake mmap offset for a CMA GEM
+ *     object
+ * @file_priv: DRM file-private structure containing the GEM object
+ * @drm: DRM device
+ * @handle: GEM object handle
+ * @offset: return location for the fake mmap offset
+ *
+ * Return: 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
-		struct drm_device *drm, uint32_t handle, uint64_t *offset)
+				struct drm_device *drm, u32 handle,
+				u64 *offset)
 {
 	struct drm_gem_object *gem_obj;
 
@@ -208,7 +238,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
 
 	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
 	if (!gem_obj) {
-		dev_err(drm->dev, "failed to lookup gem object\n");
+		dev_err(drm->dev, "failed to lookup GEM object\n");
 		mutex_unlock(&drm->struct_mutex);
 		return -EINVAL;
 	}
@@ -251,8 +281,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
 	return ret;
 }
 
-/*
- * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
+/**
+ * drm_gem_cma_mmap - memory-map a CMA GEM object
+ * @filp: file object
+ * @vma: VMA for the area to be mapped
+ *
+ * Return: 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
 {
@@ -272,7 +306,13 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
 EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
 
 #ifdef CONFIG_DEBUG_FS
-void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m)
+/**
+ * drm_gem_cma_describe - describe a CMA GEM object for debugfs
+ * @cma_obj: CMA GEM object
+ * @m: debugfs file handle
+ */
+void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj,
+			  struct seq_file *m)
 {
 	struct drm_gem_object *obj = &cma_obj->base;
 	struct drm_device *dev = obj->dev;
@@ -291,7 +331,14 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
 EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
 #endif
 
-/* low-level interface prime helpers */
+/**
+ * drm_gem_cma_prime_get_sg_table - provide a scatter/gather table of pinned
+ *     pages for a CMA GEM object
+ * @obj: GEM object
+ *
+ * Return: A pointer to the scatter/gather table of pinned pages or NULL on
+ * failure.
+ */
 struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
 {
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
@@ -315,6 +362,16 @@ out:
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_get_sg_table);
 
+/**
+ * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another
+ *     driver's scatter/gather table of pinned pages
+ * @dev: device to import into
+ * @attach: DMA-BUF attachment
+ * @sgt: scatter/gather table of pinned pages
+ *
+ * Return: A pointer to a newly created GEM object or an ERR_PTR-encoded
+ * negative error code on failure.
+ */
 struct drm_gem_object *
 drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
 				  struct dma_buf_attachment *attach,
@@ -339,6 +396,13 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
 
+/**
+ * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object
+ * @obj: GEM object
+ * @vma: VMA for the area to be mapped
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
 int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 			   struct vm_area_struct *vma)
 {
@@ -357,6 +421,13 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
 
+/**
+ * drm_gem_cma_prime_vmap - map a CMA GEM object into the kernel's virtual
+ *     address space
+ * @obj: GEM object
+ *
+ * Return: The kernel virtual address of the CMA GEM object's backing store.
+ */
 void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
 {
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
@@ -365,6 +436,12 @@ void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
 
+/**
+ * drm_gem_cma_prime_vunmap - unmap a CMA GEM object from the kernel's virtual
+ *     address space
+ * @obj: GEM object
+ * @vaddr: kernel virtual address where the CMA GEM object was mapped
+ */
 void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
 {
 	/* Nothing to do */
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 2ff35f3de9c5..873d4eb7f125 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -4,6 +4,13 @@
 #include <drm/drmP.h>
 #include <drm/drm_gem.h>
 
+/**
+ * struct drm_gem_cma_object - GEM object backed by CMA memory allocations
+ * @base: base GEM object
+ * @paddr: physical address of the backing memory
+ * @sgt: scatter/gather table for imported PRIME buffers
+ * @vaddr: kernel virtual address of the backing memory
+ */
 struct drm_gem_cma_object {
 	struct drm_gem_object base;
 	dma_addr_t paddr;
@@ -19,23 +26,25 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
 	return container_of(gem_obj, struct drm_gem_cma_object, base);
 }
 
-/* free gem object. */
+/* free GEM object */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
 
-/* create memory region for drm framebuffer. */
+/* create memory region for DRM framebuffer */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-		struct drm_device *drm, struct drm_mode_create_dumb *args);
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args);
 
-/* map memory region for drm framebuffer to user space. */
+/* map memory region for DRM framebuffer to user space */
 int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
-		struct drm_device *drm, uint32_t handle, uint64_t *offset);
+				struct drm_device *drm, u32 handle,
+				u64 *offset);
 
-/* set vm_flags and we can change the vm attribute to other one at here. */
+/* set vm_flags and we can change the VM attribute to other one at here */
 int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
 
-/* allocate physical memory. */
+/* allocate physical memory */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
-		unsigned int size);
+					      size_t size);
 
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
-- 
2.1.3

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

[PATCH 4/7] drm/cma: Introduce drm_gem_cma_dumb_create_internal()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function is similar to drm_gem_cma_dumb_create() but targetted at
kernel internal users so that they can override the pitch and size
requirements of the dumb buffer.

It is important to make this difference because the IOCTL says that the
pitch and size fields are to be considered outputs and therefore should
not be used in computations of the framebuffer size. Internal users may
still want to use this code to avoid duplication and at the same time
pass on additional, driver-specific restrictions on the pitch and size.

While at it, convert the R-Car DU driver, the single user that overrides
the pitch, to use the new internal helper.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_gem_cma_helper.c  | 33 ++++++++++++++++++++++++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 +-
 include/drm/drm_gem_cma_helper.h      |  5 +++++
 3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 55306be1f8f7..c55986566f0c 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -189,7 +189,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
 
 /**
- * drm_gem_cma_dumb_create - create a dumb buffer object
+ * drm_gem_cma_dumb_create_internal - create a dumb buffer object
  * @file_priv: DRM file-private structure to create the dumb buffer for
  * @drm: DRM device
  * @args: IOCTL data
@@ -199,12 +199,12 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-			    struct drm_device *drm,
-			    struct drm_mode_create_dumb *args)
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+				     struct drm_device *drm,
+				     struct drm_mode_create_dumb *args)
 {
+	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
 	struct drm_gem_cma_object *cma_obj;
-	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
 
 	if (args->pitch < min_pitch)
 		args->pitch = min_pitch;
@@ -216,6 +216,29 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 						 &args->handle);
 	return PTR_ERR_OR_ZERO(cma_obj);
 }
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
+
+/**
+ * drm_gem_cma_dumb_create - create a dumb buffer object
+ * @file_priv: DRM file-private structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args)
+{
+	struct drm_gem_cma_object *cma_obj;
+
+	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	args->size = args->pitch * args->height;
+
+	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
+						 &args->handle);
+	return PTR_ERR_OR_ZERO(cma_obj);
+}
 EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
 
 /**
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 6c24ad7d03ef..5329491e32c3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -128,7 +128,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 
 	args->pitch = roundup(max(args->pitch, min_pitch), align);
 
-	return drm_gem_cma_dumb_create(file, dev, args);
+	return drm_gem_cma_dumb_create_internal(file, dev, args);
 }
 
 static struct drm_framebuffer *
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 873d4eb7f125..acd6af8a8e67 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -30,6 +30,11 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
 
 /* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+				     struct drm_device *drm,
+				     struct drm_mode_create_dumb *args);
+
+/* create memory region for DRM framebuffer */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 			    struct drm_device *drm,
 			    struct drm_mode_create_dumb *args);
-- 
2.1.3

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

[PATCH 5/7] drm/omap: gem: dumb: pitch is an output

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
IOCTL, only the width, height, bpp and flags fields are inputs. The
caller is not guaranteed to zero out or set handle, pitch and size.
Drivers must not treat these values as possible inputs, otherwise they
may use uninitialized memory during the computation of the framebuffer
size.

The OMAP driver uses the pitch field passed in by userspace as a minimum
and only override it if the driver-computed pitch is larger than what
userspace provided. To prevent this from causing overallocation, fix the
minimum pitch to 0 to enforce the driver-computed pitch.

Signed-off-by: Thierry Reding <treding@nvidia.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 e4849413ee80..bff60b73995b 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -613,7 +613,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	union omap_gem_size gsize;
 
 	/* in case someone tries to feed us a completely bogus stride: */
-	args->pitch = align_pitch(args->pitch, args->width, args->bpp);
+	args->pitch = align_pitch(0, args->width, args->bpp);
 	args->size = PAGE_ALIGN(args->pitch * args->height);
 
 	gsize = (union omap_gem_size){
-- 
2.1.3

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

[PATCH 6/7] drm/rcar: gem: dumb: pitch is an output

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
IOCTL, only the width, height, bpp and flags fields are inputs. The
caller is not guaranteed to zero out or set handle, pitch and size.
Drivers must not treat these values as possible inputs, otherwise they
may use uninitialized memory during the computation of the framebuffer
size.

The R-Car DU driver treats the pitch passed in from userspace as minimum
and will only overwrite it when the driver-computed pitch is larger,
allowing userspace to, intentionally or not, overallocate framebuffers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 5329491e32c3..6289e3797bc5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 	else
 		align = 16 * args->bpp / 8;
 
-	args->pitch = roundup(max(args->pitch, min_pitch), align);
+	args->pitch = roundup(min_pitch, align);
 
 	return drm_gem_cma_dumb_create_internal(file, dev, args);
 }
-- 
2.1.3

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

[PATCH 7/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Some drivers treat the pitch and size fields as inputs and will use them
as minima provided by userspace so that they are only overwritten if the
minimal requirements of the driver exceed them.

This can cause strange behaviour when applications don't zero out these
fields, causing whatever was on the stack to be passed to the IOCTL. In
a typical case this would become visible as a failed allocation if the
pitch or size were unusually high. But this could also cause more subtle
bugs like overallocating dumb framebuffers.

To prevent drivers from misusing these values, make the DRM core zero
out the pitch and size fields before passing the structure to the driver
implementation.

While at it, also set the output handle field to zero for good measure,
even though it's less likely to be abused.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_crtc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0f3c24c0981b..6aceb689ccea 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4755,6 +4755,14 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 	if (PAGE_ALIGN(size) == 0)
 		return -EINVAL;
 
+	/*
+	 * handle, pitch and size are output parameters. Zero them out to
+	 * prevent drivers from accidentally using uninitialized data.
+	 */
+	args->handle = 0;
+	args->pitch = 0;
+	args->size = 0;
+
 	return dev->driver->dumb_create(file_priv, dev, args);
 }
 
-- 
2.1.3

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

Re: [PATCH 1/7] drm/gem: Fix a few kerneldoc typos

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:13PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> While at it, adjust the drm_gem_handle_create() function declaration to
> be more consistent with other functions in the file.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index f6ca51259fa3..597318ae307e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -309,7 +309,7 @@ EXPORT_SYMBOL(drm_gem_dumb_destroy);
>   * drm_gem_handle_create_tail - internal functions to create a handle
>   * @file_priv: drm file-private structure to register the handle for
>   * @obj: object to register
> - * @handlep: pionter to return the created handle to the caller
> + * @handlep: pointer to return the created handle to the caller
>   * 
>   * This expects the dev->object_name_lock to be held already and will drop it
>   * before returning. Used to avoid races in establishing new handles when
> @@ -362,7 +362,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  }
>  
>  /**
> - * gem_handle_create - create a gem handle for an object
> + * drm_gem_handle_create - create a gem handle for an object
>   * @file_priv: drm file-private structure to register the handle for
>   * @obj: object to register
>   * @handlep: pionter to return the created handle to the caller
> @@ -371,10 +371,9 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>   * to the object, which includes a regular reference count. Callers
>   * will likely want to dereference the object afterwards.
>   */
> -int
> -drm_gem_handle_create(struct drm_file *file_priv,
> -		       struct drm_gem_object *obj,
> -		       u32 *handlep)
> +int drm_gem_handle_create(struct drm_file *file_priv,
> +			  struct drm_gem_object *obj,
> +			  u32 *handlep)
>  {
>  	mutex_lock(&obj->dev->object_name_lock);
>  
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/7] drm/doc: mm: Fix indentation

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:14PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Use spaces consistently for indentation in the memory-management
> section.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 268 ++++++++++++++++++++---------------------
>  1 file changed, 134 insertions(+), 134 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index f6a9d7b21380..18025496a736 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -492,10 +492,10 @@ char *date;</synopsis>
>      <sect2>
>        <title>The Translation Table Manager (TTM)</title>
>        <para>
> -	TTM design background and information belongs here.
> +        TTM design background and information belongs here.
>        </para>
>        <sect3>
> -	<title>TTM initialization</title>
> +        <title>TTM initialization</title>
>          <warning><para>This section is outdated.</para></warning>
>          <para>
>            Drivers wishing to support TTM must fill out a drm_bo_driver
> @@ -503,42 +503,42 @@ char *date;</synopsis>
>            pointers for initializing the TTM, allocating and freeing memory,
>            waiting for command completion and fence synchronization, and memory
>            migration. See the radeon_ttm.c file for an example of usage.
> -	</para>
> -	<para>
> -	  The ttm_global_reference structure is made up of several fields:
> -	</para>
> -	<programlisting>
> -	  struct ttm_global_reference {
> -	  	enum ttm_global_types global_type;
> -	  	size_t size;
> -	  	void *object;
> -	  	int (*init) (struct ttm_global_reference *);
> -	  	void (*release) (struct ttm_global_reference *);
> -	  };
> -	</programlisting>
> -	<para>
> -	  There should be one global reference structure for your memory
> -	  manager as a whole, and there will be others for each object
> -	  created by the memory manager at runtime.  Your global TTM should
> -	  have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
> -	  object should be sizeof(struct ttm_mem_global), and the init and
> -	  release hooks should point at your driver-specific init and
> -	  release routines, which probably eventually call
> -	  ttm_mem_global_init and ttm_mem_global_release, respectively.
> -	</para>
> -	<para>
> -	  Once your global TTM accounting structure is set up and initialized
> -	  by calling ttm_global_item_ref() on it,
> -	  you need to create a buffer object TTM to
> -	  provide a pool for buffer object allocation by clients and the
> -	  kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
> -	  and its size should be sizeof(struct ttm_bo_global).  Again,
> -	  driver-specific init and release functions may be provided,
> -	  likely eventually calling ttm_bo_global_init() and
> -	  ttm_bo_global_release(), respectively.  Also, like the previous
> -	  object, ttm_global_item_ref() is used to create an initial reference
> -	  count for the TTM, which will call your initialization function.
> -	</para>
> +        </para>
> +        <para>
> +          The ttm_global_reference structure is made up of several fields:
> +        </para>
> +        <programlisting>
> +          struct ttm_global_reference {
> +                  enum ttm_global_types global_type;
> +                  size_t size;
> +                  void *object;
> +                  int (*init) (struct ttm_global_reference *);
> +                  void (*release) (struct ttm_global_reference *);
> +          };
> +        </programlisting>
> +        <para>
> +          There should be one global reference structure for your memory
> +          manager as a whole, and there will be others for each object
> +          created by the memory manager at runtime.  Your global TTM should
> +          have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
> +          object should be sizeof(struct ttm_mem_global), and the init and
> +          release hooks should point at your driver-specific init and
> +          release routines, which probably eventually call
> +          ttm_mem_global_init and ttm_mem_global_release, respectively.
> +        </para>
> +        <para>
> +          Once your global TTM accounting structure is set up and initialized
> +          by calling ttm_global_item_ref() on it,
> +          you need to create a buffer object TTM to
> +          provide a pool for buffer object allocation by clients and the
> +          kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
> +          and its size should be sizeof(struct ttm_bo_global).  Again,
> +          driver-specific init and release functions may be provided,
> +          likely eventually calling ttm_bo_global_init() and
> +          ttm_bo_global_release(), respectively.  Also, like the previous
> +          object, ttm_global_item_ref() is used to create an initial reference
> +          count for the TTM, which will call your initialization function.
> +        </para>
>        </sect3>
>      </sect2>
>      <sect2 id="drm-gem">
> @@ -566,19 +566,19 @@ char *date;</synopsis>
>          using driver-specific ioctls.
>        </para>
>        <para>
> -	On a fundamental level, GEM involves several operations:
> -	<itemizedlist>
> -	  <listitem>Memory allocation and freeing</listitem>
> -	  <listitem>Command execution</listitem>
> -	  <listitem>Aperture management at command execution time</listitem>
> -	</itemizedlist>
> -	Buffer object allocation is relatively straightforward and largely
> +        On a fundamental level, GEM involves several operations:
> +        <itemizedlist>
> +          <listitem>Memory allocation and freeing</listitem>
> +          <listitem>Command execution</listitem>
> +          <listitem>Aperture management at command execution time</listitem>
> +        </itemizedlist>
> +        Buffer object allocation is relatively straightforward and largely
>          provided by Linux's shmem layer, which provides memory to back each
>          object.
>        </para>
>        <para>
>          Device-specific operations, such as command execution, pinning, buffer
> -	read &amp; write, mapping, and domain ownership transfers are left to
> +        read &amp; write, mapping, and domain ownership transfers are left to
>          driver-specific ioctls.
>        </para>
>        <sect3>
> @@ -738,16 +738,16 @@ char *date;</synopsis>
>            respectively. The conversion is handled by the DRM core without any
>            driver-specific support.
>          </para>
> -	<para>
> -	  GEM also supports buffer sharing with dma-buf file descriptors through
> -	  PRIME. GEM-based drivers must use the provided helpers functions to
> -	  implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
> -	  Since sharing file descriptors is inherently more secure than the
> -	  easily guessable and global GEM names it is the preferred buffer
> -	  sharing mechanism. Sharing buffers through GEM names is only supported
> -	  for legacy userspace. Furthermore PRIME also allows cross-device
> -	  buffer sharing since it is based on dma-bufs.
> -	</para>
> +        <para>
> +          GEM also supports buffer sharing with dma-buf file descriptors through
> +          PRIME. GEM-based drivers must use the provided helpers functions to
> +          implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
> +          Since sharing file descriptors is inherently more secure than the
> +          easily guessable and global GEM names it is the preferred buffer
> +          sharing mechanism. Sharing buffers through GEM names is only supported
> +          for legacy userspace. Furthermore PRIME also allows cross-device
> +          buffer sharing since it is based on dma-bufs.
> +        </para>
>        </sect3>
>        <sect3 id="drm-gem-objects-mapping">
>          <title>GEM Objects Mapping</title>
> @@ -852,7 +852,7 @@ char *date;</synopsis>
>        <sect3>
>          <title>Command Execution</title>
>          <para>
> -	  Perhaps the most important GEM function for GPU devices is providing a
> +          Perhaps the most important GEM function for GPU devices is providing a
>            command execution interface to clients. Client programs construct
>            command buffers containing references to previously allocated memory
>            objects, and then submit them to GEM. At that point, GEM takes care to
> @@ -874,95 +874,95 @@ char *date;</synopsis>
>          <title>GEM Function Reference</title>
>  !Edrivers/gpu/drm/drm_gem.c
>        </sect3>
> -      </sect2>
> -      <sect2>
> -	<title>VMA Offset Manager</title>
> +    </sect2>
> +    <sect2>
> +      <title>VMA Offset Manager</title>
>  !Pdrivers/gpu/drm/drm_vma_manager.c vma offset manager
>  !Edrivers/gpu/drm/drm_vma_manager.c
>  !Iinclude/drm/drm_vma_manager.h
> -      </sect2>
> -      <sect2 id="drm-prime-support">
> -	<title>PRIME Buffer Sharing</title>
> -	<para>
> -	  PRIME is the cross device buffer sharing framework in drm, originally
> -	  created for the OPTIMUS range of multi-gpu platforms. To userspace
> -	  PRIME buffers are dma-buf based file descriptors.
> -	</para>
> -	<sect3>
> -	  <title>Overview and Driver Interface</title>
> -	  <para>
> -	    Similar to GEM global names, PRIME file descriptors are
> -	    also used to share buffer objects across processes. They offer
> -	    additional security: as file descriptors must be explicitly sent over
> -	    UNIX domain sockets to be shared between applications, they can't be
> -	    guessed like the globally unique GEM names.
> -	  </para>
> -	  <para>
> -	    Drivers that support the PRIME
> -	    API must set the DRIVER_PRIME bit in the struct
> -	    <structname>drm_driver</structname>
> -	    <structfield>driver_features</structfield> field, and implement the
> -	    <methodname>prime_handle_to_fd</methodname> and
> -	    <methodname>prime_fd_to_handle</methodname> operations.
> -	  </para>
> -	  <para>
> -	    <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
> -			  struct drm_file *file_priv, uint32_t handle,
> -			  uint32_t flags, int *prime_fd);
> +    </sect2>
> +    <sect2 id="drm-prime-support">
> +      <title>PRIME Buffer Sharing</title>
> +      <para>
> +        PRIME is the cross device buffer sharing framework in drm, originally
> +        created for the OPTIMUS range of multi-gpu platforms. To userspace
> +        PRIME buffers are dma-buf based file descriptors.
> +      </para>
> +      <sect3>
> +        <title>Overview and Driver Interface</title>
> +        <para>
> +          Similar to GEM global names, PRIME file descriptors are
> +          also used to share buffer objects across processes. They offer
> +          additional security: as file descriptors must be explicitly sent over
> +          UNIX domain sockets to be shared between applications, they can't be
> +          guessed like the globally unique GEM names.
> +        </para>
> +        <para>
> +          Drivers that support the PRIME
> +          API must set the DRIVER_PRIME bit in the struct
> +          <structname>drm_driver</structname>
> +          <structfield>driver_features</structfield> field, and implement the
> +          <methodname>prime_handle_to_fd</methodname> and
> +          <methodname>prime_fd_to_handle</methodname> operations.
> +        </para>
> +        <para>
> +          <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
> +                          struct drm_file *file_priv, uint32_t handle,
> +                          uint32_t flags, int *prime_fd);
>  int (*prime_fd_to_handle)(struct drm_device *dev,
> -			  struct drm_file *file_priv, int prime_fd,
> -			  uint32_t *handle);</synopsis>
> -	    Those two operations convert a handle to a PRIME file descriptor and
> -	    vice versa. Drivers must use the kernel dma-buf buffer sharing framework
> -	    to manage the PRIME file descriptors. Similar to the mode setting
> -	    API PRIME is agnostic to the underlying buffer object manager, as
> -	    long as handles are 32bit unsigned integers.
> -	  </para>
> -	  <para>
> -	    While non-GEM drivers must implement the operations themselves, GEM
> -	    drivers must use the <function>drm_gem_prime_handle_to_fd</function>
> -	    and <function>drm_gem_prime_fd_to_handle</function> helper functions.
> -	    Those helpers rely on the driver
> -	    <methodname>gem_prime_export</methodname> and
> -	    <methodname>gem_prime_import</methodname> operations to create a dma-buf
> -	    instance from a GEM object (dma-buf exporter role) and to create a GEM
> -	    object from a dma-buf instance (dma-buf importer role).
> -	  </para>
> -	  <para>
> -	    <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
> -				     struct drm_gem_object *obj,
> -				     int flags);
> +                          struct drm_file *file_priv, int prime_fd,
> +                          uint32_t *handle);</synopsis>
> +            Those two operations convert a handle to a PRIME file descriptor and
> +            vice versa. Drivers must use the kernel dma-buf buffer sharing framework
> +            to manage the PRIME file descriptors. Similar to the mode setting
> +            API PRIME is agnostic to the underlying buffer object manager, as
> +            long as handles are 32bit unsigned integers.
> +          </para>
> +          <para>
> +            While non-GEM drivers must implement the operations themselves, GEM
> +            drivers must use the <function>drm_gem_prime_handle_to_fd</function>
> +            and <function>drm_gem_prime_fd_to_handle</function> helper functions.
> +            Those helpers rely on the driver
> +            <methodname>gem_prime_export</methodname> and
> +            <methodname>gem_prime_import</methodname> operations to create a dma-buf
> +            instance from a GEM object (dma-buf exporter role) and to create a GEM
> +            object from a dma-buf instance (dma-buf importer role).
> +          </para>
> +          <para>
> +            <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
> +                             struct drm_gem_object *obj,
> +                             int flags);
>  struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> -					    struct dma_buf *dma_buf);</synopsis>
> -	    These two operations are mandatory for GEM drivers that support
> -	    PRIME.
> -	  </para>
> -	</sect3>
> -        <sect3>
> -          <title>PRIME Helper Functions</title>
> -!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
> +                                            struct dma_buf *dma_buf);</synopsis>
> +            These two operations are mandatory for GEM drivers that support
> +            PRIME.
> +          </para>
>          </sect3>
> -      </sect2>
> -      <sect2>
> -	<title>PRIME Function References</title>
> +      <sect3>
> +        <title>PRIME Helper Functions</title>
> +!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
> +      </sect3>
> +    </sect2>
> +    <sect2>
> +      <title>PRIME Function References</title>
>  !Edrivers/gpu/drm/drm_prime.c
> -      </sect2>
> -      <sect2>
> -	<title>DRM MM Range Allocator</title>
> -	<sect3>
> -	  <title>Overview</title>
> +    </sect2>
> +    <sect2>
> +      <title>DRM MM Range Allocator</title>
> +      <sect3>
> +        <title>Overview</title>
>  !Pdrivers/gpu/drm/drm_mm.c Overview
> -	</sect3>
> -	<sect3>
> -	  <title>LRU Scan/Eviction Support</title>
> +      </sect3>
> +      <sect3>
> +        <title>LRU Scan/Eviction Support</title>
>  !Pdrivers/gpu/drm/drm_mm.c lru scan roaster
> -	</sect3>
> +      </sect3>
>        </sect2>
> -      <sect2>
> -	<title>DRM MM Range Allocator Function References</title>
> +    <sect2>
> +      <title>DRM MM Range Allocator Function References</title>
>  !Edrivers/gpu/drm/drm_mm.c
>  !Iinclude/drm/drm_mm.h
> -      </sect2>
> +    </sect2>
>    </sect1>
>  
>    <!-- Internals: mode setting -->
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Russell King - ARM Linux]

On Wed, Nov 05, 2014 at 02:25:12PM +0100, Thierry Reding wrote:
> Discussion on IRC lead to the conclusion that new IOCTLs should have
> input validation and require userspace to zero out output parameters to
> avoid this kind of mess in the future. In order to help avoid this kind
> of ambiguity it would be a good idea to start documenting IOCTLs more
> officially (e.g. in the DRM DocBook).

That is a good point, however not everyone can build the documentation.
I tried a while back and the whole documentation subsystem is /soo/
fragile its untrue.  You have to have the exact right versions installed
and hope that the exact right versions have configured themselves to
work together correctly, otherwise you get cryptic unsolvable error
messages.  Tex is notorious for these problems.

When I tried to generate the docbook stuff, after spending a significant
amount of time trying to debug it, I just gave up completely with any
ideas about ever adding to the documentation.  I've since decided that
I am /never/ going to do anything past adding docbook comments to files.

If someone cares about linking them into the rest of the docbook system,
and they have the capability to generate the docbook output, then that's
fine and dandy, but I believe that requiring everyone to have that
capability is asking far too much.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Most of the functions already have the beginnings of kerneldoc comments
> but are using the wrong opening marker. Use the correct opening marker
> and flesh out the comments so that they can be integrated with the DRM
> DocBook document.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Bunch of comments inline below.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl       |   6 ++
>  drivers/gpu/drm/drm_gem_cma_helper.c | 155 ++++++++++++++++++++++++++---------
>  include/drm/drm_gem_cma_helper.h     |  25 ++++--
>  3 files changed, 139 insertions(+), 47 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 18025496a736..666a210af121 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>  !Edrivers/gpu/drm/drm_mm.c
>  !Iinclude/drm/drm_mm.h
>      </sect2>
> +    <sect2>
> +      <title>CMA Helper Functions Reference</title>
> +!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
> +!Edrivers/gpu/drm/drm_gem_cma_helper.c
> +!Iinclude/drm/drm_gem_cma_helper.h
> +    </sect2>
>    </sect1>
>  
>    <!-- Internals: mode setting -->
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 0316310e2cc4..55306be1f8f7 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -29,18 +29,30 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_vma_manager.h>
>  
> -/*
> +/**
> + * DOC: cma helpers
> + *
> + * The Contiguous Memory Allocator reserves a pool of memory at early boot
> + * that is used to service requests for large blocks of contiguous memory.
> + *
> + * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
> + * objects that are physically contiguous in memory. This is useful for
> + * display drivers that are unable to map scattered buffers via an IOMMU.
> + */
> +
> +/**
>   * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
> - * @drm: The drm device
> - * @size: The GEM object size
> + * @drm: DRM device
> + * @size: size of the object to allocate
>   *
> - * This function creates and initializes a GEM CMA object of the given size, but
> - * doesn't allocate any memory to back the object.
> + * This function creates and initializes a GEM CMA object of the given size,
> + * but doesn't allocate any memory to back the object.
>   *
> - * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded

Same bikeshed about "Returns:\n" as with the panel kerneldoc patch.

> + * negative error code on failure.
>   */
>  static struct drm_gem_cma_object *
> -__drm_gem_cma_create(struct drm_device *drm, unsigned int size)
> +__drm_gem_cma_create(struct drm_device *drm, size_t size)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
> @@ -69,14 +81,16 @@ error:
>  	return ERR_PTR(ret);
>  }
>  
> -/*
> +/**
>   * drm_gem_cma_create - allocate an object with the given size
> + * @drm: DRM device
> + * @size: size of the object to allocate
>   *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> + * negative error code on failure.
>   */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -		unsigned int size)
> +					      size_t size)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	int ret;
> @@ -104,17 +118,21 @@ error:
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>  
> -/*
> - * drm_gem_cma_create_with_handle - allocate an object with the given
> - * size and create a gem handle on it
> +/**
> + * drm_gem_cma_create_with_handle - allocate an object with the given size and
> + *     return a GEM handle to it
> + * @file_priv: DRM file-private structure to register the handle for
> + * @drm: DRM device
> + * @size: size of the object to allocate
> + * @handle: return location for the GEM handle
>   *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> + * negative error code on failure.
>   */
> -static struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
> -		struct drm_file *file_priv,
> -		struct drm_device *drm, unsigned int size,
> -		unsigned int *handle)
> +static struct drm_gem_cma_object *
> +drm_gem_cma_create_with_handle(struct drm_file *file_priv,
> +			       struct drm_device *drm, size_t size,
> +			       uint32_t *handle)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
> @@ -145,9 +163,9 @@ err_handle_create:
>  	return ERR_PTR(ret);
>  }
>  
> -/*
> - * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
> - * function
> +/**
> + * drm_gem_cma_free_object - free resources associated with a CMA GEM object
> + * @gem_obj: GEM object to free
>   */
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  {
> @@ -170,15 +188,20 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>  
> -/*
> - * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> - * function
> +/**
> + * drm_gem_cma_dumb_create - create a dumb buffer object
> + * @file_priv: DRM file-private structure to create the dumb buffer for
> + * @drm: DRM device
> + * @args: IOCTL data
>   *
> - * This aligns the pitch and size arguments to the minimum required. wrap
> + * This aligns the pitch and size arguments to the minimum required. Wrap
>   * this into your own function if you need bigger alignment.

I think we should augment this slightly with something like

"Drivers without special needs can directly use this as their
->dumb_create callback."

Similar for dumb_mmap_offset below.

> + *
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -		struct drm_device *dev, struct drm_mode_create_dumb *args)
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> @@ -189,18 +212,25 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  	if (args->size < args->pitch * args->height)
>  		args->size = args->pitch * args->height;
>  
> -	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
> -			args->size, &args->handle);
> +	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> +						 &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>  
> -/*
> - * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
> - * function
> +/**
> + * drm_gem_cma_dumb_map_offset - return the fake mmap offset for a CMA GEM
> + *     object
> + * @file_priv: DRM file-private structure containing the GEM object
> + * @drm: DRM device
> + * @handle: GEM object handle
> + * @offset: return location for the fake mmap offset
> + *
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> -		struct drm_device *drm, uint32_t handle, uint64_t *offset)
> +				struct drm_device *drm, u32 handle,
> +				u64 *offset)
>  {
>  	struct drm_gem_object *gem_obj;
>  
> @@ -208,7 +238,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>  
>  	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
>  	if (!gem_obj) {
> -		dev_err(drm->dev, "failed to lookup gem object\n");
> +		dev_err(drm->dev, "failed to lookup GEM object\n");
>  		mutex_unlock(&drm->struct_mutex);
>  		return -EINVAL;
>  	}
> @@ -251,8 +281,12 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
>  	return ret;
>  }
>  
> -/*
> - * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> +/**
> + * drm_gem_cma_mmap - memory-map a CMA GEM object
> + * @filp: file object
> + * @vma: VMA for the area to be mapped

Imo the real value of kerneldoc is the blabla here that describes what
this should be used for and when. E.g. here

"This function implements an augmented version of the gem DRM file mmap
operation for cma objects: In addition to the usual gem vma setup it
immediately faults in the entire object instead of using on-demand
faulting. Drivers which employ the cma helpers should use this function as
their ->mmap handler for the DRM device file's file_operation structure."

So requires grepping each function and hunting for the usual pattern (and
noticing tons of crazy stuff, usually).

Same for all the functions below.

> + *
> + * Return: 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
> @@ -272,7 +306,13 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
>  
>  #ifdef CONFIG_DEBUG_FS
> -void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m)
> +/**
> + * drm_gem_cma_describe - describe a CMA GEM object for debugfs
> + * @cma_obj: CMA GEM object
> + * @m: debugfs file handle
> + */
> +void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj,
> +			  struct seq_file *m)
>  {
>  	struct drm_gem_object *obj = &cma_obj->base;
>  	struct drm_device *dev = obj->dev;
> @@ -291,7 +331,14 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
>  EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
>  #endif
>  
> -/* low-level interface prime helpers */
> +/**
> + * drm_gem_cma_prime_get_sg_table - provide a scatter/gather table of pinned
> + *     pages for a CMA GEM object
> + * @obj: GEM object
> + *
> + * Return: A pointer to the scatter/gather table of pinned pages or NULL on
> + * failure.
> + */
>  struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -315,6 +362,16 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_get_sg_table);
>  
> +/**
> + * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another
> + *     driver's scatter/gather table of pinned pages
> + * @dev: device to import into
> + * @attach: DMA-BUF attachment
> + * @sgt: scatter/gather table of pinned pages
> + *
> + * Return: A pointer to a newly created GEM object or an ERR_PTR-encoded
> + * negative error code on failure.
> + */
>  struct drm_gem_object *
>  drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  				  struct dma_buf_attachment *attach,
> @@ -339,6 +396,13 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
>  
> +/**
> + * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object
> + * @obj: GEM object
> + * @vma: VMA for the area to be mapped
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
>  int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
>  			   struct vm_area_struct *vma)
>  {
> @@ -357,6 +421,13 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
>  
> +/**
> + * drm_gem_cma_prime_vmap - map a CMA GEM object into the kernel's virtual
> + *     address space
> + * @obj: GEM object
> + *
> + * Return: The kernel virtual address of the CMA GEM object's backing store.
> + */
>  void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -365,6 +436,12 @@ void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
>  
> +/**
> + * drm_gem_cma_prime_vunmap - unmap a CMA GEM object from the kernel's virtual
> + *     address space
> + * @obj: GEM object
> + * @vaddr: kernel virtual address where the CMA GEM object was mapped
> + */
>  void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>  {
>  	/* Nothing to do */
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 2ff35f3de9c5..873d4eb7f125 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -4,6 +4,13 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_gem.h>
>  
> +/**
> + * struct drm_gem_cma_object - GEM object backed by CMA memory allocations
> + * @base: base GEM object
> + * @paddr: physical address of the backing memory
> + * @sgt: scatter/gather table for imported PRIME buffers
> + * @vaddr: kernel virtual address of the backing memory
> + */
>  struct drm_gem_cma_object {
>  	struct drm_gem_object base;
>  	dma_addr_t paddr;
> @@ -19,23 +26,25 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
>  	return container_of(gem_obj, struct drm_gem_cma_object, base);
>  }
>  
> -/* free gem object. */
> +/* free GEM object */
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>  
> -/* create memory region for drm framebuffer. */
> +/* create memory region for DRM framebuffer */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -		struct drm_device *drm, struct drm_mode_create_dumb *args);
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args);
>  
> -/* map memory region for drm framebuffer to user space. */
> +/* map memory region for DRM framebuffer to user space */
>  int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> -		struct drm_device *drm, uint32_t handle, uint64_t *offset);
> +				struct drm_device *drm, u32 handle,
> +				u64 *offset);
>  
> -/* set vm_flags and we can change the vm attribute to other one at here. */
> +/* set vm_flags and we can change the VM attribute to other one at here */
>  int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
>  
> -/* allocate physical memory. */
> +/* allocate physical memory */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -		unsigned int size);
> +					      size_t size);
>  
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/7] drm/cma: Introduce drm_gem_cma_dumb_create_internal()

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:16PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> This function is similar to drm_gem_cma_dumb_create() but targetted at
> kernel internal users so that they can override the pitch and size
> requirements of the dumb buffer.
> 
> It is important to make this difference because the IOCTL says that the
> pitch and size fields are to be considered outputs and therefore should
> not be used in computations of the framebuffer size. Internal users may
> still want to use this code to avoid duplication and at the same time
> pass on additional, driver-specific restrictions on the pitch and size.
> 
> While at it, convert the R-Car DU driver, the single user that overrides
> the pitch, to use the new internal helper.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Some comments about the kerneldoc, with that address (needs the alignment
with the previous patch essentially) this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c  | 33 ++++++++++++++++++++++++++++-----
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 +-
>  include/drm/drm_gem_cma_helper.h      |  5 +++++
>  3 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 55306be1f8f7..c55986566f0c 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -189,7 +189,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>  
>  /**
> - * drm_gem_cma_dumb_create - create a dumb buffer object
> + * drm_gem_cma_dumb_create_internal - create a dumb buffer object
>   * @file_priv: DRM file-private structure to create the dumb buffer for
>   * @drm: DRM device
>   * @args: IOCTL data
> @@ -199,12 +199,12 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);

Kerneldoc should be update to mention what exactly this is useful for, and
how it differs from the drm_gem_cma_dumb_create().
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -			    struct drm_device *drm,
> -			    struct drm_mode_create_dumb *args)
> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> +				     struct drm_device *drm,
> +				     struct drm_mode_create_dumb *args)
>  {
> +	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  	struct drm_gem_cma_object *cma_obj;
> -	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>  
>  	if (args->pitch < min_pitch)
>  		args->pitch = min_pitch;
> @@ -216,6 +216,29 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  						 &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
> +EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
> +
> +/**
> + * drm_gem_cma_dumb_create - create a dumb buffer object
> + * @file_priv: DRM file-private structure to create the dumb buffer for
> + * @drm: DRM device
> + * @args: IOCTL data

Same here (and raised in the previous kerneldoc patch already), this needs
some comments on what it's useful for and what's the difference with the
_internal variant.

> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args)
> +{
> +	struct drm_gem_cma_object *cma_obj;
> +
> +	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +	args->size = args->pitch * args->height;
> +
> +	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> +						 &args->handle);
> +	return PTR_ERR_OR_ZERO(cma_obj);
> +}
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>  
>  /**
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 6c24ad7d03ef..5329491e32c3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -128,7 +128,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  
>  	args->pitch = roundup(max(args->pitch, min_pitch), align);
>  
> -	return drm_gem_cma_dumb_create(file, dev, args);
> +	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
>  
>  static struct drm_framebuffer *
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 873d4eb7f125..acd6af8a8e67 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -30,6 +30,11 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>  
>  /* create memory region for DRM framebuffer */
> +int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
> +				     struct drm_device *drm,
> +				     struct drm_mode_create_dumb *args);
> +
> +/* create memory region for DRM framebuffer */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  			    struct drm_device *drm,
>  			    struct drm_mode_create_dumb *args);
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/7] drm/omap: gem: dumb: pitch is an output

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:17PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags fields are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size.
> Drivers must not treat these values as possible inputs, otherwise they
> may use uninitialized memory during the computation of the framebuffer
> size.
> 
> The OMAP driver uses the pitch field passed in by userspace as a minimum
> and only override it if the driver-computed pitch is larger than what
> userspace provided. To prevent this from causing overallocation, fix the
> minimum pitch to 0 to enforce the driver-computed pitch.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  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 e4849413ee80..bff60b73995b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -613,7 +613,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	union omap_gem_size gsize;
>  
>  	/* in case someone tries to feed us a completely bogus stride: */
> -	args->pitch = align_pitch(args->pitch, args->width, args->bpp);
> +	args->pitch = align_pitch(0, args->width, args->bpp);
>  	args->size = PAGE_ALIGN(args->pitch * args->height);
>  
>  	gsize = (union omap_gem_size){
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/7] drm/rcar: gem: dumb: pitch is an output

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:18PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags fields are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size.
> Drivers must not treat these values as possible inputs, otherwise they
> may use uninitialized memory during the computation of the framebuffer
> size.
> 
> The R-Car DU driver treats the pitch passed in from userspace as minimum
> and will only overwrite it when the driver-computed pitch is larger,
> allowing userspace to, intentionally or not, overallocate framebuffers.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside: I tend to put the maintainer Cc: into the patch so that they're on
the permanent record. That will also make sure that when this patch gets
resend (e.g. stable backport) maintainers are still included properly.
-Daniel

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 5329491e32c3..6289e3797bc5 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	else
>  		align = 16 * args->bpp / 8;
>  
> -	args->pitch = roundup(max(args->pitch, min_pitch), align);
> +	args->pitch = roundup(min_pitch, align);
>  
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }
> -- 
> 2.1.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:25:19PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Some drivers treat the pitch and size fields as inputs and will use them
> as minima provided by userspace so that they are only overwritten if the
> minimal requirements of the driver exceed them.
> 
> This can cause strange behaviour when applications don't zero out these
> fields, causing whatever was on the stack to be passed to the IOCTL. In
> a typical case this would become visible as a failed allocation if the
> pitch or size were unusually high. But this could also cause more subtle
> bugs like overallocating dumb framebuffers.
> 
> To prevent drivers from misusing these values, make the DRM core zero
> out the pitch and size fields before passing the structure to the driver
> implementation.
> 
> While at it, also set the output handle field to zero for good measure,
> even though it's less likely to be abused.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 0f3c24c0981b..6aceb689ccea 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4755,6 +4755,14 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
>  	if (PAGE_ALIGN(size) == 0)
>  		return -EINVAL;
>  
> +	/*
> +	 * handle, pitch and size are output parameters. Zero them out to
> +	 * prevent drivers from accidentally using uninitialized data.

Maybe add: Unfortunately we can't reject ioctls with garbage in them since
existing userspace is not clearing these fields properly.

With that comment: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

That way it's clear that we can never reuse these fields for flags or
anything at all. Also a good reminder for folks that they really should
have if (args->foo) return -EINVAL for any reserved, unused or output-only
fields.
-Daniel

> +	 */
> +	args->handle = 0;
> +	args->pitch = 0;
> +	args->size = 0;
> +
>  	return dev->driver->dumb_create(file_priv, dev, args);
>  }
>  
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Thierry Reding]

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

On Wed, Nov 05, 2014 at 02:24:42PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 05, 2014 at 02:25:12PM +0100, Thierry Reding wrote:
> > Discussion on IRC lead to the conclusion that new IOCTLs should have
> > input validation and require userspace to zero out output parameters to
> > avoid this kind of mess in the future. In order to help avoid this kind
> > of ambiguity it would be a good idea to start documenting IOCTLs more
> > officially (e.g. in the DRM DocBook).
> 
> That is a good point, however not everyone can build the documentation.
> I tried a while back and the whole documentation subsystem is /soo/
> fragile its untrue.  You have to have the exact right versions installed
> and hope that the exact right versions have configured themselves to
> work together correctly, otherwise you get cryptic unsolvable error
> messages.  Tex is notorious for these problems.

I usually only build the HTML documentation which tends to be less prone
to the issues that you're describing.

> When I tried to generate the docbook stuff, after spending a significant
> amount of time trying to debug it, I just gave up completely with any
> ideas about ever adding to the documentation.  I've since decided that
> I am /never/ going to do anything past adding docbook comments to files.
> 
> If someone cares about linking them into the rest of the docbook system,
> and they have the capability to generate the docbook output, then that's
> fine and dandy, but I believe that requiring everyone to have that
> capability is asking far too much.

It looks as if perhaps running the xmldocs target would at least run
kerneldoc on the files even if it doesn't produce anything very human
readable. If that works for you it'd at least give you a means to check
that the kerneldoc comments you've added are consistent.

Thierry

[-- Attachment #1.2: 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 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Thierry Reding]

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

On Wed, Nov 05, 2014 at 03:34:40PM +0100, Daniel Vetter wrote:
> On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Most of the functions already have the beginnings of kerneldoc comments
> > but are using the wrong opening marker. Use the correct opening marker
> > and flesh out the comments so that they can be integrated with the DRM
> > DocBook document.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> 
> Bunch of comments inline below.
> -Daniel
> 
> > ---
> >  Documentation/DocBook/drm.tmpl       |   6 ++
> >  drivers/gpu/drm/drm_gem_cma_helper.c | 155 ++++++++++++++++++++++++++---------
> >  include/drm/drm_gem_cma_helper.h     |  25 ++++--
> >  3 files changed, 139 insertions(+), 47 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 18025496a736..666a210af121 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> >  !Edrivers/gpu/drm/drm_mm.c
> >  !Iinclude/drm/drm_mm.h
> >      </sect2>
> > +    <sect2>
> > +      <title>CMA Helper Functions Reference</title>
> > +!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
> > +!Edrivers/gpu/drm/drm_gem_cma_helper.c
> > +!Iinclude/drm/drm_gem_cma_helper.h
> > +    </sect2>
> >    </sect1>
> >  
> >    <!-- Internals: mode setting -->
> > diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> > index 0316310e2cc4..55306be1f8f7 100644
> > --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> > @@ -29,18 +29,30 @@
> >  #include <drm/drm_gem_cma_helper.h>
> >  #include <drm/drm_vma_manager.h>
> >  
> > -/*
> > +/**
> > + * DOC: cma helpers
> > + *
> > + * The Contiguous Memory Allocator reserves a pool of memory at early boot
> > + * that is used to service requests for large blocks of contiguous memory.
> > + *
> > + * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
> > + * objects that are physically contiguous in memory. This is useful for
> > + * display drivers that are unable to map scattered buffers via an IOMMU.
> > + */
> > +
> > +/**
> >   * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
> > - * @drm: The drm device
> > - * @size: The GEM object size
> > + * @drm: DRM device
> > + * @size: size of the object to allocate
> >   *
> > - * This function creates and initializes a GEM CMA object of the given size, but
> > - * doesn't allocate any memory to back the object.
> > + * This function creates and initializes a GEM CMA object of the given size,
> > + * but doesn't allocate any memory to back the object.
> >   *
> > - * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
> > + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> 
> Same bikeshed about "Returns:\n" as with the panel kerneldoc patch.

I've been following the style described in the kernel-doc nano-HOWTO,
which says that:

	The return value, if any, should be described in a dedicated
	section named "Return".

There are other things in that document that we don't follow in DRM, so
I wonder if we should just consider it as guidelines rather than actual
rules (they aren't enforced anyway) or perhaps make a pass over existing
kerneldoc and convert it to the rules described in that document.

That document is the only reference for the kerneldoc syntax (that I
know of), so I had always thought that we should be following it.

> > -/*
> > - * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> > - * function
> > +/**
> > + * drm_gem_cma_dumb_create - create a dumb buffer object
> > + * @file_priv: DRM file-private structure to create the dumb buffer for
> > + * @drm: DRM device
> > + * @args: IOCTL data
> >   *
> > - * This aligns the pitch and size arguments to the minimum required. wrap
> > + * This aligns the pitch and size arguments to the minimum required. Wrap
> >   * this into your own function if you need bigger alignment.
> 
> I think we should augment this slightly with something like
> 
> "Drivers without special needs can directly use this as their
> ->dumb_create callback."

Good idea.

> Similar for dumb_mmap_offset below.

I don't see any way how drm_gem_cma_dumb_map_offset() could be
parameterized. It certainly looks generic enough not to need any
device-specific quirks.

> > -/*
> > - * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> > +/**
> > + * drm_gem_cma_mmap - memory-map a CMA GEM object
> > + * @filp: file object
> > + * @vma: VMA for the area to be mapped
> 
> Imo the real value of kerneldoc is the blabla here that describes what
> this should be used for and when. E.g. here
> 
> "This function implements an augmented version of the gem DRM file mmap
> operation for cma objects: In addition to the usual gem vma setup it
> immediately faults in the entire object instead of using on-demand
> faulting. Drivers which employ the cma helpers should use this function as
> their ->mmap handler for the DRM device file's file_operation structure."

I've taken the easy route and simply copied your example.

> So requires grepping each function and hunting for the usual pattern (and
> noticing tons of crazy stuff, usually).
> 
> Same for all the functions below.

Okay, I'll see what I can come up with.

Thierry

[-- Attachment #1.2: 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 0/7] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Daniel Vetter]

On Wed, Nov 05, 2014 at 02:24:42PM +0000, Russell King - ARM Linux wrote:
> On Wed, Nov 05, 2014 at 02:25:12PM +0100, Thierry Reding wrote:
> > Discussion on IRC lead to the conclusion that new IOCTLs should have
> > input validation and require userspace to zero out output parameters to
> > avoid this kind of mess in the future. In order to help avoid this kind
> > of ambiguity it would be a good idea to start documenting IOCTLs more
> > officially (e.g. in the DRM DocBook).
> 
> That is a good point, however not everyone can build the documentation.
> I tried a while back and the whole documentation subsystem is /soo/
> fragile its untrue.  You have to have the exact right versions installed
> and hope that the exact right versions have configured themselves to
> work together correctly, otherwise you get cryptic unsolvable error
> messages.  Tex is notorious for these problems.
> 
> When I tried to generate the docbook stuff, after spending a significant
> amount of time trying to debug it, I just gave up completely with any
> ideas about ever adding to the documentation.  I've since decided that
> I am /never/ going to do anything past adding docbook comments to files.
> 
> If someone cares about linking them into the rest of the docbook system,
> and they have the capability to generate the docbook output, then that's
> fine and dandy, but I believe that requiring everyone to have that
> capability is asking far too much.

Ime all the kerneldoc and tex stuff is hopeless. The only thing I use is

$ make htmldocs

That works somewhat after a bit of wrestling. But note that it doesn't
handle file renames gracefully and make clean won't get you out of that
fuzz. So my build-docs script also has a

$ git clean -dfx Documentation/DocBook

in there. Not pretty I agree. Add to that that kerneldoc is a feature poor
markup language (no lists, hyperlinks, chapters or anything really).

I'm trying really hard to get someone at intel (or paid by intel) to fix
this mess up though.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Daniel Vetter]

On Wed, Nov 05, 2014 at 04:01:26PM +0100, Thierry Reding wrote:
> On Wed, Nov 05, 2014 at 03:34:40PM +0100, Daniel Vetter wrote:
> > On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> > > + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> > 
> > Same bikeshed about "Returns:\n" as with the panel kerneldoc patch.
> 
> I've been following the style described in the kernel-doc nano-HOWTO,
> which says that:
> 
> 	The return value, if any, should be described in a dedicated
> 	section named "Return".
> 
> There are other things in that document that we don't follow in DRM, so
> I wonder if we should just consider it as guidelines rather than actual
> rules (they aren't enforced anyway) or perhaps make a pass over existing
> kerneldoc and convert it to the rules described in that document.
> 
> That document is the only reference for the kerneldoc syntax (that I
> know of), so I had always thought that we should be following it.

We've started out with all-uppercase RETURNS from userspace libdrm iirc. I
don't care really, but iirc the Returns: is the common one we use. Imo it
also reads better in English, but not native speaker here ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/7] drm/doc: Add GEM/CMA helpers to kerneldoc

[Thierry Reding]

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

On Wed, Nov 05, 2014 at 04:04:55PM +0100, Daniel Vetter wrote:
> On Wed, Nov 05, 2014 at 04:01:26PM +0100, Thierry Reding wrote:
> > On Wed, Nov 05, 2014 at 03:34:40PM +0100, Daniel Vetter wrote:
> > > On Wed, Nov 05, 2014 at 02:25:15PM +0100, Thierry Reding wrote:
> > > > + * Return: A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded
> > > 
> > > Same bikeshed about "Returns:\n" as with the panel kerneldoc patch.
> > 
> > I've been following the style described in the kernel-doc nano-HOWTO,
> > which says that:
> > 
> > 	The return value, if any, should be described in a dedicated
> > 	section named "Return".
> > 
> > There are other things in that document that we don't follow in DRM, so
> > I wonder if we should just consider it as guidelines rather than actual
> > rules (they aren't enforced anyway) or perhaps make a pass over existing
> > kerneldoc and convert it to the rules described in that document.
> > 
> > That document is the only reference for the kerneldoc syntax (that I
> > know of), so I had always thought that we should be following it.
> 
> We've started out with all-uppercase RETURNS from userspace libdrm iirc. I
> don't care really, but iirc the Returns: is the common one we use. Imo it
> also reads better in English, but not native speaker here ;-)

Alright, I'll go with the variant that you proposed for the sake of
consistency.

Thierry

[-- Attachment #1.2: 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 6/7] drm/rcar: gem: dumb: pitch is an output

[Laurent Pinchart]

Hi Thierry,

Thank you for the patch.

On Wednesday 05 November 2014 14:25:18 Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags fields are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size.
> Drivers must not treat these values as possible inputs, otherwise they
> may use uninitialized memory during the computation of the framebuffer
> size.
> 
> The R-Car DU driver treats the pitch passed in from userspace as minimum
> and will only overwrite it when the driver-computed pitch is larger,
> allowing userspace to, intentionally or not, overallocate framebuffers.

As discussed on IRC, my concern with this is that some userspace applications 
might be relying on this behaviour. I'm not personally aware of any though, so 
I'm not opposed to this patch, but I can't vouch it won't cause any userspace 
breakage.

(On a side note I believe treating the pitch and size arguments as inputs 
could be a worthwhile extension to the API, but given that we haven't rejected 
incorrect values in the past we're pretty much stuck).

> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 5329491e32c3..6289e3797bc5
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct
> drm_device *dev, else
>  		align = 16 * args->bpp / 8;
> 
> -	args->pitch = roundup(max(args->pitch, min_pitch), align);
> +	args->pitch = roundup(min_pitch, align);
> 
>  	return drm_gem_cma_dumb_create_internal(file, dev, args);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH 6/7] drm/rcar: gem: dumb: pitch is an output

[Russell King - ARM Linux]

On Wed, Nov 05, 2014 at 08:47:07PM +0200, Laurent Pinchart wrote:
> (On a side note I believe treating the pitch and size arguments as inputs 
> could be a worthwhile extension to the API, but given that we haven't
> rejected incorrect values in the past we're pretty much stuck).

The bigger picture here is that it is a generic interface to DRM
drivers, and having different behaviour from different drivers is
itself impractical.

If nothing breaks through better enforcement of the API, then we
should go with what the API is currently defined to be, rather than
trying to overload it with new features - and then end up in the
situation where we have something trying to use those new features
and no way to know if the driver implements this "bug" or not.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 0/8] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This second version addresses review comments from before. A new patch
is added that removes a redundant call to drm_gem_free_mmap_offset() in
the CMA GEM helpers.

Below is the cover letter from the first version for more background in
case anybody missed it the last time:

Drivers currently treat the IOCTL data for DRM_IOCTL_MODE_CREATE_DUMB
inconsistently. While the documentation clearly states that the handle,
pitch and size fields are outputs, some drivers use their values as a
minimum hint from userspace, presumably to allow userspace to over-
allocate buffers on purpose (for example after negotiating pitch
alignment requirements with other devices).

Most of userspace is well-behaved in that in zeros out the output fields
which works well with drivers that use them as minima. However, some
userspace (like Mesa's GBM DRI backend) only initializes the inputs, so
that the driver will end up using uninitialized data from the stack in
the computations. This can cause the driver to attempt very large
allocations, resulting in failure, or silently use excessively  large
buffers.

This series tries to fix this by sanitizing the IOCTL data (setting the
output fields to 0 in the kernel) so that drivers can no longer use
uninitialized data. While at it, it also fixes existing drivers to not
treat output fields as input/output for consistency.

Note that this is technically breaking ABI since overallocating will no
longer be possible after this series is applied. However, the public DRM
headers have always documented the fields as being outputs, so any
application relying on them being inputs could be considered buggy. The
hope is that no such applications exist in the wild.

Discussion on IRC lead to the conclusion that new IOCTLs should have
input validation and require userspace to zero out output parameters to
avoid this kind of mess in the future. In order to help avoid this kind
of ambiguity it would be a good idea to start documenting IOCTLs more
officially (e.g. in the DRM DocBook).

I'm adding a bunch of people on Cc to get feedback about what userspace
people know might be relying on the current behaviour. Drivers that are
impacted are OMAP, R-Car and any that use GEM CMA helpers.

This series also contains a couple of preparatory patches that add the
GEM CMA helpers to our DocBook.

Thierry

Thierry Reding (8):
  drm/gem: Fix a few kerneldoc typos
  drm/doc: mm: Fix indentation
  drm/doc: Add GEM/CMA helpers to kerneldoc
  drm/cma: Introduce drm_gem_cma_dumb_create_internal()
  drm/omap: gem: dumb: pitch is an output
  drm/rcar: gem: dumb: pitch is an output
  drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input
  drm/cma: Remove call to drm_gem_free_mmap_offset()

 Documentation/DocBook/drm.tmpl        | 274 +++++++++++++++++-----------------
 drivers/gpu/drm/drm_crtc.c            |  10 ++
 drivers/gpu/drm/drm_gem.c             |  11 +-
 drivers/gpu/drm/drm_gem_cma_helper.c  | 259 ++++++++++++++++++++++++++------
 drivers/gpu/drm/omapdrm/omap_gem.c    |   2 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |   4 +-
 include/drm/drm_gem_cma_helper.h      |  30 +++-
 7 files changed, 395 insertions(+), 195 deletions(-)

-- 
2.1.3

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

[PATCH v2 1/8] drm/gem: Fix a few kerneldoc typos

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

While at it, adjust the drm_gem_handle_create() function declaration to
be more consistent with other functions in the file.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_gem.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index f6ca51259fa3..597318ae307e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -309,7 +309,7 @@ EXPORT_SYMBOL(drm_gem_dumb_destroy);
  * drm_gem_handle_create_tail - internal functions to create a handle
  * @file_priv: drm file-private structure to register the handle for
  * @obj: object to register
- * @handlep: pionter to return the created handle to the caller
+ * @handlep: pointer to return the created handle to the caller
  * 
  * This expects the dev->object_name_lock to be held already and will drop it
  * before returning. Used to avoid races in establishing new handles when
@@ -362,7 +362,7 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 }
 
 /**
- * gem_handle_create - create a gem handle for an object
+ * drm_gem_handle_create - create a gem handle for an object
  * @file_priv: drm file-private structure to register the handle for
  * @obj: object to register
  * @handlep: pionter to return the created handle to the caller
@@ -371,10 +371,9 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
  * to the object, which includes a regular reference count. Callers
  * will likely want to dereference the object afterwards.
  */
-int
-drm_gem_handle_create(struct drm_file *file_priv,
-		       struct drm_gem_object *obj,
-		       u32 *handlep)
+int drm_gem_handle_create(struct drm_file *file_priv,
+			  struct drm_gem_object *obj,
+			  u32 *handlep)
 {
 	mutex_lock(&obj->dev->object_name_lock);
 
-- 
2.1.3

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

[PATCH v2 2/8] drm/doc: mm: Fix indentation

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Use spaces consistently for indentation in the memory-management
section.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 Documentation/DocBook/drm.tmpl | 268 ++++++++++++++++++++---------------------
 1 file changed, 134 insertions(+), 134 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 01b8ca5f1a3d..6e32ef9c75fd 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -492,10 +492,10 @@ char *date;</synopsis>
     <sect2>
       <title>The Translation Table Manager (TTM)</title>
       <para>
-	TTM design background and information belongs here.
+        TTM design background and information belongs here.
       </para>
       <sect3>
-	<title>TTM initialization</title>
+        <title>TTM initialization</title>
         <warning><para>This section is outdated.</para></warning>
         <para>
           Drivers wishing to support TTM must fill out a drm_bo_driver
@@ -503,42 +503,42 @@ char *date;</synopsis>
           pointers for initializing the TTM, allocating and freeing memory,
           waiting for command completion and fence synchronization, and memory
           migration. See the radeon_ttm.c file for an example of usage.
-	</para>
-	<para>
-	  The ttm_global_reference structure is made up of several fields:
-	</para>
-	<programlisting>
-	  struct ttm_global_reference {
-	  	enum ttm_global_types global_type;
-	  	size_t size;
-	  	void *object;
-	  	int (*init) (struct ttm_global_reference *);
-	  	void (*release) (struct ttm_global_reference *);
-	  };
-	</programlisting>
-	<para>
-	  There should be one global reference structure for your memory
-	  manager as a whole, and there will be others for each object
-	  created by the memory manager at runtime.  Your global TTM should
-	  have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
-	  object should be sizeof(struct ttm_mem_global), and the init and
-	  release hooks should point at your driver-specific init and
-	  release routines, which probably eventually call
-	  ttm_mem_global_init and ttm_mem_global_release, respectively.
-	</para>
-	<para>
-	  Once your global TTM accounting structure is set up and initialized
-	  by calling ttm_global_item_ref() on it,
-	  you need to create a buffer object TTM to
-	  provide a pool for buffer object allocation by clients and the
-	  kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
-	  and its size should be sizeof(struct ttm_bo_global).  Again,
-	  driver-specific init and release functions may be provided,
-	  likely eventually calling ttm_bo_global_init() and
-	  ttm_bo_global_release(), respectively.  Also, like the previous
-	  object, ttm_global_item_ref() is used to create an initial reference
-	  count for the TTM, which will call your initialization function.
-	</para>
+        </para>
+        <para>
+          The ttm_global_reference structure is made up of several fields:
+        </para>
+        <programlisting>
+          struct ttm_global_reference {
+                  enum ttm_global_types global_type;
+                  size_t size;
+                  void *object;
+                  int (*init) (struct ttm_global_reference *);
+                  void (*release) (struct ttm_global_reference *);
+          };
+        </programlisting>
+        <para>
+          There should be one global reference structure for your memory
+          manager as a whole, and there will be others for each object
+          created by the memory manager at runtime.  Your global TTM should
+          have a type of TTM_GLOBAL_TTM_MEM.  The size field for the global
+          object should be sizeof(struct ttm_mem_global), and the init and
+          release hooks should point at your driver-specific init and
+          release routines, which probably eventually call
+          ttm_mem_global_init and ttm_mem_global_release, respectively.
+        </para>
+        <para>
+          Once your global TTM accounting structure is set up and initialized
+          by calling ttm_global_item_ref() on it,
+          you need to create a buffer object TTM to
+          provide a pool for buffer object allocation by clients and the
+          kernel itself.  The type of this object should be TTM_GLOBAL_TTM_BO,
+          and its size should be sizeof(struct ttm_bo_global).  Again,
+          driver-specific init and release functions may be provided,
+          likely eventually calling ttm_bo_global_init() and
+          ttm_bo_global_release(), respectively.  Also, like the previous
+          object, ttm_global_item_ref() is used to create an initial reference
+          count for the TTM, which will call your initialization function.
+        </para>
       </sect3>
     </sect2>
     <sect2 id="drm-gem">
@@ -566,19 +566,19 @@ char *date;</synopsis>
         using driver-specific ioctls.
       </para>
       <para>
-	On a fundamental level, GEM involves several operations:
-	<itemizedlist>
-	  <listitem>Memory allocation and freeing</listitem>
-	  <listitem>Command execution</listitem>
-	  <listitem>Aperture management at command execution time</listitem>
-	</itemizedlist>
-	Buffer object allocation is relatively straightforward and largely
+        On a fundamental level, GEM involves several operations:
+        <itemizedlist>
+          <listitem>Memory allocation and freeing</listitem>
+          <listitem>Command execution</listitem>
+          <listitem>Aperture management at command execution time</listitem>
+        </itemizedlist>
+        Buffer object allocation is relatively straightforward and largely
         provided by Linux's shmem layer, which provides memory to back each
         object.
       </para>
       <para>
         Device-specific operations, such as command execution, pinning, buffer
-	read &amp; write, mapping, and domain ownership transfers are left to
+        read &amp; write, mapping, and domain ownership transfers are left to
         driver-specific ioctls.
       </para>
       <sect3>
@@ -738,16 +738,16 @@ char *date;</synopsis>
           respectively. The conversion is handled by the DRM core without any
           driver-specific support.
         </para>
-	<para>
-	  GEM also supports buffer sharing with dma-buf file descriptors through
-	  PRIME. GEM-based drivers must use the provided helpers functions to
-	  implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
-	  Since sharing file descriptors is inherently more secure than the
-	  easily guessable and global GEM names it is the preferred buffer
-	  sharing mechanism. Sharing buffers through GEM names is only supported
-	  for legacy userspace. Furthermore PRIME also allows cross-device
-	  buffer sharing since it is based on dma-bufs.
-	</para>
+        <para>
+          GEM also supports buffer sharing with dma-buf file descriptors through
+          PRIME. GEM-based drivers must use the provided helpers functions to
+          implement the exporting and importing correctly. See <xref linkend="drm-prime-support" />.
+          Since sharing file descriptors is inherently more secure than the
+          easily guessable and global GEM names it is the preferred buffer
+          sharing mechanism. Sharing buffers through GEM names is only supported
+          for legacy userspace. Furthermore PRIME also allows cross-device
+          buffer sharing since it is based on dma-bufs.
+        </para>
       </sect3>
       <sect3 id="drm-gem-objects-mapping">
         <title>GEM Objects Mapping</title>
@@ -852,7 +852,7 @@ char *date;</synopsis>
       <sect3>
         <title>Command Execution</title>
         <para>
-	  Perhaps the most important GEM function for GPU devices is providing a
+          Perhaps the most important GEM function for GPU devices is providing a
           command execution interface to clients. Client programs construct
           command buffers containing references to previously allocated memory
           objects, and then submit them to GEM. At that point, GEM takes care to
@@ -874,95 +874,95 @@ char *date;</synopsis>
         <title>GEM Function Reference</title>
 !Edrivers/gpu/drm/drm_gem.c
       </sect3>
-      </sect2>
-      <sect2>
-	<title>VMA Offset Manager</title>
+    </sect2>
+    <sect2>
+      <title>VMA Offset Manager</title>
 !Pdrivers/gpu/drm/drm_vma_manager.c vma offset manager
 !Edrivers/gpu/drm/drm_vma_manager.c
 !Iinclude/drm/drm_vma_manager.h
-      </sect2>
-      <sect2 id="drm-prime-support">
-	<title>PRIME Buffer Sharing</title>
-	<para>
-	  PRIME is the cross device buffer sharing framework in drm, originally
-	  created for the OPTIMUS range of multi-gpu platforms. To userspace
-	  PRIME buffers are dma-buf based file descriptors.
-	</para>
-	<sect3>
-	  <title>Overview and Driver Interface</title>
-	  <para>
-	    Similar to GEM global names, PRIME file descriptors are
-	    also used to share buffer objects across processes. They offer
-	    additional security: as file descriptors must be explicitly sent over
-	    UNIX domain sockets to be shared between applications, they can't be
-	    guessed like the globally unique GEM names.
-	  </para>
-	  <para>
-	    Drivers that support the PRIME
-	    API must set the DRIVER_PRIME bit in the struct
-	    <structname>drm_driver</structname>
-	    <structfield>driver_features</structfield> field, and implement the
-	    <methodname>prime_handle_to_fd</methodname> and
-	    <methodname>prime_fd_to_handle</methodname> operations.
-	  </para>
-	  <para>
-	    <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
-			  struct drm_file *file_priv, uint32_t handle,
-			  uint32_t flags, int *prime_fd);
+    </sect2>
+    <sect2 id="drm-prime-support">
+      <title>PRIME Buffer Sharing</title>
+      <para>
+        PRIME is the cross device buffer sharing framework in drm, originally
+        created for the OPTIMUS range of multi-gpu platforms. To userspace
+        PRIME buffers are dma-buf based file descriptors.
+      </para>
+      <sect3>
+        <title>Overview and Driver Interface</title>
+        <para>
+          Similar to GEM global names, PRIME file descriptors are
+          also used to share buffer objects across processes. They offer
+          additional security: as file descriptors must be explicitly sent over
+          UNIX domain sockets to be shared between applications, they can't be
+          guessed like the globally unique GEM names.
+        </para>
+        <para>
+          Drivers that support the PRIME
+          API must set the DRIVER_PRIME bit in the struct
+          <structname>drm_driver</structname>
+          <structfield>driver_features</structfield> field, and implement the
+          <methodname>prime_handle_to_fd</methodname> and
+          <methodname>prime_fd_to_handle</methodname> operations.
+        </para>
+        <para>
+          <synopsis>int (*prime_handle_to_fd)(struct drm_device *dev,
+                          struct drm_file *file_priv, uint32_t handle,
+                          uint32_t flags, int *prime_fd);
 int (*prime_fd_to_handle)(struct drm_device *dev,
-			  struct drm_file *file_priv, int prime_fd,
-			  uint32_t *handle);</synopsis>
-	    Those two operations convert a handle to a PRIME file descriptor and
-	    vice versa. Drivers must use the kernel dma-buf buffer sharing framework
-	    to manage the PRIME file descriptors. Similar to the mode setting
-	    API PRIME is agnostic to the underlying buffer object manager, as
-	    long as handles are 32bit unsigned integers.
-	  </para>
-	  <para>
-	    While non-GEM drivers must implement the operations themselves, GEM
-	    drivers must use the <function>drm_gem_prime_handle_to_fd</function>
-	    and <function>drm_gem_prime_fd_to_handle</function> helper functions.
-	    Those helpers rely on the driver
-	    <methodname>gem_prime_export</methodname> and
-	    <methodname>gem_prime_import</methodname> operations to create a dma-buf
-	    instance from a GEM object (dma-buf exporter role) and to create a GEM
-	    object from a dma-buf instance (dma-buf importer role).
-	  </para>
-	  <para>
-	    <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
-				     struct drm_gem_object *obj,
-				     int flags);
+                          struct drm_file *file_priv, int prime_fd,
+                          uint32_t *handle);</synopsis>
+            Those two operations convert a handle to a PRIME file descriptor and
+            vice versa. Drivers must use the kernel dma-buf buffer sharing framework
+            to manage the PRIME file descriptors. Similar to the mode setting
+            API PRIME is agnostic to the underlying buffer object manager, as
+            long as handles are 32bit unsigned integers.
+          </para>
+          <para>
+            While non-GEM drivers must implement the operations themselves, GEM
+            drivers must use the <function>drm_gem_prime_handle_to_fd</function>
+            and <function>drm_gem_prime_fd_to_handle</function> helper functions.
+            Those helpers rely on the driver
+            <methodname>gem_prime_export</methodname> and
+            <methodname>gem_prime_import</methodname> operations to create a dma-buf
+            instance from a GEM object (dma-buf exporter role) and to create a GEM
+            object from a dma-buf instance (dma-buf importer role).
+          </para>
+          <para>
+            <synopsis>struct dma_buf * (*gem_prime_export)(struct drm_device *dev,
+                             struct drm_gem_object *obj,
+                             int flags);
 struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
-					    struct dma_buf *dma_buf);</synopsis>
-	    These two operations are mandatory for GEM drivers that support
-	    PRIME.
-	  </para>
-	</sect3>
-        <sect3>
-          <title>PRIME Helper Functions</title>
-!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+                                            struct dma_buf *dma_buf);</synopsis>
+            These two operations are mandatory for GEM drivers that support
+            PRIME.
+          </para>
         </sect3>
-      </sect2>
-      <sect2>
-	<title>PRIME Function References</title>
+      <sect3>
+        <title>PRIME Helper Functions</title>
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+      </sect3>
+    </sect2>
+    <sect2>
+      <title>PRIME Function References</title>
 !Edrivers/gpu/drm/drm_prime.c
-      </sect2>
-      <sect2>
-	<title>DRM MM Range Allocator</title>
-	<sect3>
-	  <title>Overview</title>
+    </sect2>
+    <sect2>
+      <title>DRM MM Range Allocator</title>
+      <sect3>
+        <title>Overview</title>
 !Pdrivers/gpu/drm/drm_mm.c Overview
-	</sect3>
-	<sect3>
-	  <title>LRU Scan/Eviction Support</title>
+      </sect3>
+      <sect3>
+        <title>LRU Scan/Eviction Support</title>
 !Pdrivers/gpu/drm/drm_mm.c lru scan roaster
-	</sect3>
+      </sect3>
       </sect2>
-      <sect2>
-	<title>DRM MM Range Allocator Function References</title>
+    <sect2>
+      <title>DRM MM Range Allocator Function References</title>
 !Edrivers/gpu/drm/drm_mm.c
 !Iinclude/drm/drm_mm.h
-      </sect2>
+    </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
-- 
2.1.3

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

[PATCH v2 3/8] drm/doc: Add GEM/CMA helpers to kerneldoc

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Most of the functions already have the beginnings of kerneldoc comments
but are using the wrong opening marker. Use the correct opening marker
and flesh out the comments so that they can be integrated with the DRM
DocBook document.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- document return value consistently with other source files
- provide better documentation and use-cases for helpers
---
 Documentation/DocBook/drm.tmpl       |   6 +
 drivers/gpu/drm/drm_gem_cma_helper.c | 220 ++++++++++++++++++++++++++++-------
 include/drm/drm_gem_cma_helper.h     |  25 ++--
 3 files changed, 203 insertions(+), 48 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 6e32ef9c75fd..6a14eab4a3d8 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 !Edrivers/gpu/drm/drm_mm.c
 !Iinclude/drm/drm_mm.h
     </sect2>
+    <sect2>
+      <title>CMA Helper Functions Reference</title>
+!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
+!Edrivers/gpu/drm/drm_gem_cma_helper.c
+!Iinclude/drm/drm_gem_cma_helper.h
+    </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 0316310e2cc4..7f986d7b8e22 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -29,18 +29,31 @@
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_vma_manager.h>
 
-/*
+/**
+ * DOC: cma helpers
+ *
+ * The Contiguous Memory Allocator reserves a pool of memory at early boot
+ * that is used to service requests for large blocks of contiguous memory.
+ *
+ * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
+ * objects that are physically contiguous in memory. This is useful for
+ * display drivers that are unable to map scattered buffers via an IOMMU.
+ */
+
+/**
  * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
- * @drm: The drm device
- * @size: The GEM object size
+ * @drm: DRM device
+ * @size: size of the object to allocate
  *
- * This function creates and initializes a GEM CMA object of the given size, but
- * doesn't allocate any memory to back the object.
+ * This function creates and initializes a GEM CMA object of the given size,
+ * but doesn't allocate any memory to back the object.
  *
- * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
+ * Returns:
+ * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
  */
 static struct drm_gem_cma_object *
-__drm_gem_cma_create(struct drm_device *drm, unsigned int size)
+__drm_gem_cma_create(struct drm_device *drm, size_t size)
 {
 	struct drm_gem_cma_object *cma_obj;
 	struct drm_gem_object *gem_obj;
@@ -69,14 +82,21 @@ error:
 	return ERR_PTR(ret);
 }
 
-/*
+/**
  * drm_gem_cma_create - allocate an object with the given size
+ * @drm: DRM device
+ * @size: size of the object to allocate
+ *
+ * This function creates a CMA GEM object and allocates a contiguous chunk of
+ * memory as backing store. The backing memory has the writecombine attribute
+ * set.
  *
- * returns a struct drm_gem_cma_object* on success or ERR_PTR values
- * on failure.
+ * Returns:
+ * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
  */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
-		unsigned int size)
+					      size_t size)
 {
 	struct drm_gem_cma_object *cma_obj;
 	int ret;
@@ -104,17 +124,26 @@ error:
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_create);
 
-/*
- * drm_gem_cma_create_with_handle - allocate an object with the given
- * size and create a gem handle on it
+/**
+ * drm_gem_cma_create_with_handle - allocate an object with the given size and
+ *     return a GEM handle to it
+ * @file_priv: DRM file-private structure to register the handle for
+ * @drm: DRM device
+ * @size: size of the object to allocate
+ * @handle: return location for the GEM handle
+ *
+ * This function creates a CMA GEM object, allocating a physically contiguous
+ * chunk of memory as backing store. The GEM object is then added to the list
+ * of object associated with the given file and a handle to it is returned.
  *
- * returns a struct drm_gem_cma_object* on success or ERR_PTR values
- * on failure.
+ * Returns:
+ * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
  */
-static struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
-		struct drm_file *file_priv,
-		struct drm_device *drm, unsigned int size,
-		unsigned int *handle)
+static struct drm_gem_cma_object *
+drm_gem_cma_create_with_handle(struct drm_file *file_priv,
+			       struct drm_device *drm, size_t size,
+			       uint32_t *handle)
 {
 	struct drm_gem_cma_object *cma_obj;
 	struct drm_gem_object *gem_obj;
@@ -145,9 +174,14 @@ err_handle_create:
 	return ERR_PTR(ret);
 }
 
-/*
- * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
- * function
+/**
+ * drm_gem_cma_free_object - free resources associated with a CMA GEM object
+ * @gem_obj: GEM object to free
+ *
+ * This function frees the backing memory of the CMA GEM object, cleans up the
+ * GEM object state and frees the memory used to store the object itself.
+ * Drivers using the CMA helpers should set this as their DRM driver's
+ * ->gem_free_object() callback.
  */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 {
@@ -170,15 +204,23 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
 
-/*
- * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
- * function
+/**
+ * drm_gem_cma_dumb_create - create a dumb buffer object
+ * @file_priv: DRM file-private structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
+ *
+ * This function computes the pitch of the dumb buffer and rounds it up to an
+ * integer number of bytes per pixel. Drivers for hardware that doesn't have
+ * any additional restrictions on the pitch can directly use this function as
+ * their ->dumb_create() callback.
  *
- * This aligns the pitch and size arguments to the minimum required. wrap
- * this into your own function if you need bigger alignment.
+ * Returns:
+ * 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-		struct drm_device *dev, struct drm_mode_create_dumb *args)
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args)
 {
 	struct drm_gem_cma_object *cma_obj;
 	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
@@ -189,18 +231,30 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 	if (args->size < args->pitch * args->height)
 		args->size = args->pitch * args->height;
 
-	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
-			args->size, &args->handle);
+	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
+						 &args->handle);
 	return PTR_ERR_OR_ZERO(cma_obj);
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
 
-/*
- * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
- * function
+/**
+ * drm_gem_cma_dumb_map_offset - return the fake mmap offset for a CMA GEM
+ *     object
+ * @file_priv: DRM file-private structure containing the GEM object
+ * @drm: DRM device
+ * @handle: GEM object handle
+ * @offset: return location for the fake mmap offset
+ *
+ * This function look up an object by its handle and returns the fake mmap
+ * offset associated with it. Drivers using the CMA helpers should set this
+ * as their DRM driver's ->dumb_map_offset() callback.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
-		struct drm_device *drm, uint32_t handle, uint64_t *offset)
+				struct drm_device *drm, u32 handle,
+				u64 *offset)
 {
 	struct drm_gem_object *gem_obj;
 
@@ -208,7 +262,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
 
 	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
 	if (!gem_obj) {
-		dev_err(drm->dev, "failed to lookup gem object\n");
+		dev_err(drm->dev, "failed to lookup GEM object\n");
 		mutex_unlock(&drm->struct_mutex);
 		return -EINVAL;
 	}
@@ -251,8 +305,20 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
 	return ret;
 }
 
-/*
- * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
+/**
+ * drm_gem_cma_mmap - memory-map a CMA GEM object
+ * @filp: file object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function implements an augmented version of the GEM DRM file mmap
+ * operation for CMA objects: In addition to the usual GEM VMA setup it
+ * immediately faults in the entire object instead of using on-demaind
+ * faulting. Drivers which employ the CMA helpers should use this function
+ * as their ->mmap() handler in the DRM device file's file_operations
+ * structure.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
  */
 int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
 {
@@ -272,7 +338,16 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
 EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
 
 #ifdef CONFIG_DEBUG_FS
-void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m)
+/**
+ * drm_gem_cma_describe - describe a CMA GEM object for debugfs
+ * @cma_obj: CMA GEM object
+ * @m: debugfs file handle
+ *
+ * This function can be used to dump a human-readable representation of the
+ * CMA GEM object into a synthetic file.
+ */
+void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj,
+			  struct seq_file *m)
 {
 	struct drm_gem_object *obj = &cma_obj->base;
 	struct drm_device *dev = obj->dev;
@@ -291,7 +366,18 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
 EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
 #endif
 
-/* low-level interface prime helpers */
+/**
+ * drm_gem_cma_prime_get_sg_table - provide a scatter/gather table of pinned
+ *     pages for a CMA GEM object
+ * @obj: GEM object
+ *
+ * This function exports a scatter/gather table suitable for PRIME usage by
+ * calling the standard DMA mapping API. Drivers using the CMA helpers should
+ * set this as their DRM driver's ->gem_prime_get_sg_table() callback.
+ *
+ * Returns:
+ * A pointer to the scatter/gather table of pinned pages or NULL on failure.
+ */
 struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
 {
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
@@ -315,6 +401,23 @@ out:
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_get_sg_table);
 
+/**
+ * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another
+ *     driver's scatter/gather table of pinned pages
+ * @dev: device to import into
+ * @attach: DMA-BUF attachment
+ * @sgt: scatter/gather table of pinned pages
+ *
+ * This function imports a scatter/gather table exported via DMA-BUF by
+ * another driver. Imported buffers must be physically contiguous in memory
+ * (i.e. the scatter/gather table must contain a single entry). Drivers that
+ * use the CMA helpers should set this as their DRM driver's
+ * ->gem_prime_import_sg_table() callback.
+ *
+ * Returns:
+ * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
+ * error code on failure.
+ */
 struct drm_gem_object *
 drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
 				  struct dma_buf_attachment *attach,
@@ -339,6 +442,18 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
 
+/**
+ * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object
+ * @obj: GEM object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function maps a buffer imported via DRM PRIME into a userspace
+ * process's address space. Drivers that use the CMA helpers should set this
+ * as their DRM driver's ->gem_prime_mmap() callback.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
 int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 			   struct vm_area_struct *vma)
 {
@@ -357,6 +472,20 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
 
+/**
+ * drm_gem_cma_prime_vmap - map a CMA GEM object into the kernel's virtual
+ *     address space
+ * @obj: GEM object
+ *
+ * This function maps a buffer exported via DRM PRIME into the kernel's
+ * virtual address space. Since the CMA buffers are already mapped into the
+ * kernel virtual address space this simply returns the cached virtual
+ * address. Drivers using the CMA helpers should set this as their DRM
+ * driver's ->gem_prime_vmap() callback.
+ *
+ * Returns:
+ * The kernel virtual address of the CMA GEM object's backing store.
+ */
 void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
 {
 	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
@@ -365,6 +494,17 @@ void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
 }
 EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
 
+/**
+ * drm_gem_cma_prime_vunmap - unmap a CMA GEM object from the kernel's virtual
+ *     address space
+ * @obj: GEM object
+ * @vaddr: kernel virtual address where the CMA GEM object was mapped
+ *
+ * This function removes a buffer exported via DRM PRIME from the kernel's
+ * virtual address space. This is a no-op because CMA buffers cannot be
+ * unmapped from kernel space. Drivers using the CMA helpers should set this
+ * as their DRM driver's ->gem_prime_vunmap() callback.
+ */
 void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
 {
 	/* Nothing to do */
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 2ff35f3de9c5..873d4eb7f125 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -4,6 +4,13 @@
 #include <drm/drmP.h>
 #include <drm/drm_gem.h>
 
+/**
+ * struct drm_gem_cma_object - GEM object backed by CMA memory allocations
+ * @base: base GEM object
+ * @paddr: physical address of the backing memory
+ * @sgt: scatter/gather table for imported PRIME buffers
+ * @vaddr: kernel virtual address of the backing memory
+ */
 struct drm_gem_cma_object {
 	struct drm_gem_object base;
 	dma_addr_t paddr;
@@ -19,23 +26,25 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
 	return container_of(gem_obj, struct drm_gem_cma_object, base);
 }
 
-/* free gem object. */
+/* free GEM object */
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
 
-/* create memory region for drm framebuffer. */
+/* create memory region for DRM framebuffer */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
-		struct drm_device *drm, struct drm_mode_create_dumb *args);
+			    struct drm_device *drm,
+			    struct drm_mode_create_dumb *args);
 
-/* map memory region for drm framebuffer to user space. */
+/* map memory region for DRM framebuffer to user space */
 int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
-		struct drm_device *drm, uint32_t handle, uint64_t *offset);
+				struct drm_device *drm, u32 handle,
+				u64 *offset);
 
-/* set vm_flags and we can change the vm attribute to other one at here. */
+/* set vm_flags and we can change the VM attribute to other one at here */
 int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
 
-/* allocate physical memory. */
+/* allocate physical memory */
 struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
-		unsigned int size);
+					      size_t size);
 
 extern const struct vm_operations_struct drm_gem_cma_vm_ops;
 
-- 
2.1.3

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

[PATCH v2 4/8] drm/cma: Introduce drm_gem_cma_dumb_create_internal()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function is similar to drm_gem_cma_dumb_create() but targetted at
kernel internal users so that they can override the pitch and size
requirements of the dumb buffer.

It is important to make this difference because the IOCTL says that the
pitch and size fields are to be considered outputs and therefore should
not be used in computations of the framebuffer size. Internal users may
still want to use this code to avoid duplication and at the same time
pass on additional, driver-specific restrictions on the pitch and size.

While at it, convert the R-Car DU driver, the single user that overrides
the pitch, to use the new internal helper.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- provide better documentation and use-cases for the helper
---
 drivers/gpu/drm/drm_gem_cma_helper.c  | 45 ++++++++++++++++++++++++++++++-----
 drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 +-
 include/drm/drm_gem_cma_helper.h      |  5 ++++
 3 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 7f986d7b8e22..864b0863d042 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -205,6 +205,39 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
 
 /**
+ * drm_gem_cma_dumb_create_internal - create a dumb buffer object
+ * @file_priv: DRM file-private structure to create the dumb buffer for
+ * @drm: DRM device
+ * @args: IOCTL data
+ *
+ * This aligns the pitch and size arguments to the minimum required. This is
+ * an internal helper that can be wrapped by a driver to account for hardware
+ * with more specific alignment requirements. It should not be used directly
+ * as the ->dumb_create() callback in a DRM driver.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+				     struct drm_device *drm,
+				     struct drm_mode_create_dumb *args)
+{
+	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	struct drm_gem_cma_object *cma_obj;
+
+	if (args->pitch < min_pitch)
+		args->pitch = min_pitch;
+
+	if (args->size < args->pitch * args->height)
+		args->size = args->pitch * args->height;
+
+	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
+						 &args->handle);
+	return PTR_ERR_OR_ZERO(cma_obj);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create_internal);
+
+/**
  * drm_gem_cma_dumb_create - create a dumb buffer object
  * @file_priv: DRM file-private structure to create the dumb buffer for
  * @drm: DRM device
@@ -215,6 +248,10 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
  * any additional restrictions on the pitch can directly use this function as
  * their ->dumb_create() callback.
  *
+ * For hardware with additional restrictions, drivers can adjust the fields
+ * set up by userspace and pass the IOCTL data along to the
+ * drm_gem_cma_dumb_create_internal() function.
+ *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
@@ -223,13 +260,9 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 			    struct drm_mode_create_dumb *args)
 {
 	struct drm_gem_cma_object *cma_obj;
-	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
 
-	if (args->pitch < min_pitch)
-		args->pitch = min_pitch;
-
-	if (args->size < args->pitch * args->height)
-		args->size = args->pitch * args->height;
+	args->pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+	args->size = args->pitch * args->height;
 
 	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
 						 &args->handle);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 6c24ad7d03ef..5329491e32c3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -128,7 +128,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 
 	args->pitch = roundup(max(args->pitch, min_pitch), align);
 
-	return drm_gem_cma_dumb_create(file, dev, args);
+	return drm_gem_cma_dumb_create_internal(file, dev, args);
 }
 
 static struct drm_framebuffer *
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index 873d4eb7f125..acd6af8a8e67 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -30,6 +30,11 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
 void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
 
 /* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+				     struct drm_device *drm,
+				     struct drm_mode_create_dumb *args);
+
+/* create memory region for DRM framebuffer */
 int drm_gem_cma_dumb_create(struct drm_file *file_priv,
 			    struct drm_device *drm,
 			    struct drm_mode_create_dumb *args);
-- 
2.1.3

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

[PATCH v2 5/8] drm/omap: gem: dumb: pitch is an output

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
IOCTL, only the width, height, bpp and flags fields are inputs. The
caller is not guaranteed to zero out or set handle, pitch and size.
Drivers must not treat these values as possible inputs, otherwise they
may use uninitialized memory during the computation of the framebuffer
size.

The OMAP driver uses the pitch field passed in by userspace as a minimum
and only override it if the driver-computed pitch is larger than what
userspace provided. To prevent this from causing overallocation, fix the
minimum pitch to 0 to enforce the driver-computed pitch.

Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.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 e4849413ee80..bff60b73995b 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -613,7 +613,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
 	union omap_gem_size gsize;
 
 	/* in case someone tries to feed us a completely bogus stride: */
-	args->pitch = align_pitch(args->pitch, args->width, args->bpp);
+	args->pitch = align_pitch(0, args->width, args->bpp);
 	args->size = PAGE_ALIGN(args->pitch * args->height);
 
 	gsize = (union omap_gem_size){
-- 
2.1.3

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

[PATCH v2 6/8] drm/rcar: gem: dumb: pitch is an output

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
IOCTL, only the width, height, bpp and flags fields are inputs. The
caller is not guaranteed to zero out or set handle, pitch and size.
Drivers must not treat these values as possible inputs, otherwise they
may use uninitialized memory during the computation of the framebuffer
size.

The R-Car DU driver treats the pitch passed in from userspace as minimum
and will only overwrite it when the driver-computed pitch is larger,
allowing userspace to, intentionally or not, overallocate framebuffers.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 5329491e32c3..6289e3797bc5 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -126,7 +126,7 @@ int rcar_du_dumb_create(struct drm_file *file, struct drm_device *dev,
 	else
 		align = 16 * args->bpp / 8;
 
-	args->pitch = roundup(max(args->pitch, min_pitch), align);
+	args->pitch = roundup(min_pitch, align);
 
 	return drm_gem_cma_dumb_create_internal(file, dev, args);
 }
-- 
2.1.3

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

[PATCH v2 7/8] drm: Sanitize DRM_IOCTL_MODE_CREATE_DUMB input

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Some drivers treat the pitch and size fields as inputs and will use them
as minima provided by userspace so that they are only overwritten if the
minimal requirements of the driver exceed them.

This can cause strange behaviour when applications don't zero out these
fields, causing whatever was on the stack to be passed to the IOCTL. In
a typical case this would become visible as a failed allocation if the
pitch or size were unusually high. But this could also cause more subtle
bugs like overallocating dumb framebuffers.

To prevent drivers from misusing these values, make the DRM core zero
out the pitch and size fields before passing the structure to the driver
implementation.

While at it, also set the output handle field to zero for good measure,
even though it's less likely to be abused.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v2:
- add a comment about why we can't simply reject non-zero output fields
---
 drivers/gpu/drm/drm_crtc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 0f3c24c0981b..4d7a7699772d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4755,6 +4755,16 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,
 	if (PAGE_ALIGN(size) == 0)
 		return -EINVAL;
 
+	/*
+	 * handle, pitch and size are output parameters. Zero them out to
+	 * prevent drivers from accidentally using uninitialized data. Since
+	 * not all existing userspace is clearing these fields properly we
+	 * cannot reject IOCTL with garbage in them.
+	 */
+	args->handle = 0;
+	args->pitch = 0;
+	args->size = 0;
+
 	return dev->driver->dumb_create(file_priv, dev, args);
 }
 
-- 
2.1.3

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

[PATCH v2 8/8] drm/cma: Remove call to drm_gem_free_mmap_offset()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

drm_gem_object_release() called later in the drm_gem_cma_free_object()
function already calls this, so there's no need to do this explicitly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_gem_cma_helper.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
index 864b0863d042..e419eedf751d 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -187,8 +187,6 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
 {
 	struct drm_gem_cma_object *cma_obj;
 
-	drm_gem_free_mmap_offset(gem_obj);
-
 	cma_obj = to_drm_gem_cma_obj(gem_obj);
 
 	if (cma_obj->vaddr) {
-- 
2.1.3

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

Re: [PATCH 6/7] drm/rcar: gem: dumb: pitch is an output

[Laurent Pinchart]

Hi Russell,

On Thursday 06 November 2014 00:54:51 Russell King - ARM Linux wrote:
> On Wed, Nov 05, 2014 at 08:47:07PM +0200, Laurent Pinchart wrote:
> > (On a side note I believe treating the pitch and size arguments as inputs
> > could be a worthwhile extension to the API, but given that we haven't
> > rejected incorrect values in the past we're pretty much stuck).
> 
> The bigger picture here is that it is a generic interface to DRM
> drivers, and having different behaviour from different drivers is
> itself impractical.

I totally agree with you, I just believe it would have been nice to be able to 
extend the API that way, but that's no big deal. Consistency is definitely 
important.

> If nothing breaks through better enforcement of the API, then we
> should go with what the API is currently defined to be, rather than
> trying to overload it with new features - and then end up in the
> situation where we have something trying to use those new features
> and no way to know if the driver implements this "bug" or not.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 3/8] drm/doc: Add GEM/CMA helpers to kerneldoc

[Daniel Vetter]

On Thu, Nov 06, 2014 at 04:49:16PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Most of the functions already have the beginnings of kerneldoc comments
> but are using the wrong opening marker. Use the correct opening marker
> and flesh out the comments so that they can be integrated with the DRM
> DocBook document.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Looks really good. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> Changes in v2:
> - document return value consistently with other source files
> - provide better documentation and use-cases for helpers
> ---
>  Documentation/DocBook/drm.tmpl       |   6 +
>  drivers/gpu/drm/drm_gem_cma_helper.c | 220 ++++++++++++++++++++++++++++-------
>  include/drm/drm_gem_cma_helper.h     |  25 ++--
>  3 files changed, 203 insertions(+), 48 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 6e32ef9c75fd..6a14eab4a3d8 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -963,6 +963,12 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>  !Edrivers/gpu/drm/drm_mm.c
>  !Iinclude/drm/drm_mm.h
>      </sect2>
> +    <sect2>
> +      <title>CMA Helper Functions Reference</title>
> +!Pdrivers/gpu/drm/drm_gem_cma_helper.c cma helpers
> +!Edrivers/gpu/drm/drm_gem_cma_helper.c
> +!Iinclude/drm/drm_gem_cma_helper.h
> +    </sect2>
>    </sect1>
>  
>    <!-- Internals: mode setting -->
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 0316310e2cc4..7f986d7b8e22 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -29,18 +29,31 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_vma_manager.h>
>  
> -/*
> +/**
> + * DOC: cma helpers
> + *
> + * The Contiguous Memory Allocator reserves a pool of memory at early boot
> + * that is used to service requests for large blocks of contiguous memory.
> + *
> + * The DRM GEM/CMA helpers use this allocator as a means to provide buffer
> + * objects that are physically contiguous in memory. This is useful for
> + * display drivers that are unable to map scattered buffers via an IOMMU.
> + */
> +
> +/**
>   * __drm_gem_cma_create - Create a GEM CMA object without allocating memory
> - * @drm: The drm device
> - * @size: The GEM object size
> + * @drm: DRM device
> + * @size: size of the object to allocate
>   *
> - * This function creates and initializes a GEM CMA object of the given size, but
> - * doesn't allocate any memory to back the object.
> + * This function creates and initializes a GEM CMA object of the given size,
> + * but doesn't allocate any memory to back the object.
>   *
> - * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
> + * Returns:
> + * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> + * error code on failure.
>   */
>  static struct drm_gem_cma_object *
> -__drm_gem_cma_create(struct drm_device *drm, unsigned int size)
> +__drm_gem_cma_create(struct drm_device *drm, size_t size)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
> @@ -69,14 +82,21 @@ error:
>  	return ERR_PTR(ret);
>  }
>  
> -/*
> +/**
>   * drm_gem_cma_create - allocate an object with the given size
> + * @drm: DRM device
> + * @size: size of the object to allocate
> + *
> + * This function creates a CMA GEM object and allocates a contiguous chunk of
> + * memory as backing store. The backing memory has the writecombine attribute
> + * set.
>   *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Returns:
> + * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> + * error code on failure.
>   */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -		unsigned int size)
> +					      size_t size)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	int ret;
> @@ -104,17 +124,26 @@ error:
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_create);
>  
> -/*
> - * drm_gem_cma_create_with_handle - allocate an object with the given
> - * size and create a gem handle on it
> +/**
> + * drm_gem_cma_create_with_handle - allocate an object with the given size and
> + *     return a GEM handle to it
> + * @file_priv: DRM file-private structure to register the handle for
> + * @drm: DRM device
> + * @size: size of the object to allocate
> + * @handle: return location for the GEM handle
> + *
> + * This function creates a CMA GEM object, allocating a physically contiguous
> + * chunk of memory as backing store. The GEM object is then added to the list
> + * of object associated with the given file and a handle to it is returned.
>   *
> - * returns a struct drm_gem_cma_object* on success or ERR_PTR values
> - * on failure.
> + * Returns:
> + * A struct drm_gem_cma_object * on success or an ERR_PTR()-encoded negative
> + * error code on failure.
>   */
> -static struct drm_gem_cma_object *drm_gem_cma_create_with_handle(
> -		struct drm_file *file_priv,
> -		struct drm_device *drm, unsigned int size,
> -		unsigned int *handle)
> +static struct drm_gem_cma_object *
> +drm_gem_cma_create_with_handle(struct drm_file *file_priv,
> +			       struct drm_device *drm, size_t size,
> +			       uint32_t *handle)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	struct drm_gem_object *gem_obj;
> @@ -145,9 +174,14 @@ err_handle_create:
>  	return ERR_PTR(ret);
>  }
>  
> -/*
> - * drm_gem_cma_free_object - (struct drm_driver)->gem_free_object callback
> - * function
> +/**
> + * drm_gem_cma_free_object - free resources associated with a CMA GEM object
> + * @gem_obj: GEM object to free
> + *
> + * This function frees the backing memory of the CMA GEM object, cleans up the
> + * GEM object state and frees the memory used to store the object itself.
> + * Drivers using the CMA helpers should set this as their DRM driver's
> + * ->gem_free_object() callback.
>   */
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  {
> @@ -170,15 +204,23 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
>  
> -/*
> - * drm_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> - * function
> +/**
> + * drm_gem_cma_dumb_create - create a dumb buffer object
> + * @file_priv: DRM file-private structure to create the dumb buffer for
> + * @drm: DRM device
> + * @args: IOCTL data
> + *
> + * This function computes the pitch of the dumb buffer and rounds it up to an
> + * integer number of bytes per pixel. Drivers for hardware that doesn't have
> + * any additional restrictions on the pitch can directly use this function as
> + * their ->dumb_create() callback.
>   *
> - * This aligns the pitch and size arguments to the minimum required. wrap
> - * this into your own function if you need bigger alignment.
> + * Returns:
> + * 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -		struct drm_device *dev, struct drm_mode_create_dumb *args)
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  	int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> @@ -189,18 +231,30 @@ int drm_gem_cma_dumb_create(struct drm_file *file_priv,
>  	if (args->size < args->pitch * args->height)
>  		args->size = args->pitch * args->height;
>  
> -	cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
> -			args->size, &args->handle);
> +	cma_obj = drm_gem_cma_create_with_handle(file_priv, drm, args->size,
> +						 &args->handle);
>  	return PTR_ERR_OR_ZERO(cma_obj);
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
>  
> -/*
> - * drm_gem_cma_dumb_map_offset - (struct drm_driver)->dumb_map_offset callback
> - * function
> +/**
> + * drm_gem_cma_dumb_map_offset - return the fake mmap offset for a CMA GEM
> + *     object
> + * @file_priv: DRM file-private structure containing the GEM object
> + * @drm: DRM device
> + * @handle: GEM object handle
> + * @offset: return location for the fake mmap offset
> + *
> + * This function look up an object by its handle and returns the fake mmap
> + * offset associated with it. Drivers using the CMA helpers should set this
> + * as their DRM driver's ->dumb_map_offset() callback.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> -		struct drm_device *drm, uint32_t handle, uint64_t *offset)
> +				struct drm_device *drm, u32 handle,
> +				u64 *offset)
>  {
>  	struct drm_gem_object *gem_obj;
>  
> @@ -208,7 +262,7 @@ int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
>  
>  	gem_obj = drm_gem_object_lookup(drm, file_priv, handle);
>  	if (!gem_obj) {
> -		dev_err(drm->dev, "failed to lookup gem object\n");
> +		dev_err(drm->dev, "failed to lookup GEM object\n");
>  		mutex_unlock(&drm->struct_mutex);
>  		return -EINVAL;
>  	}
> @@ -251,8 +305,20 @@ static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
>  	return ret;
>  }
>  
> -/*
> - * drm_gem_cma_mmap - (struct file_operation)->mmap callback function
> +/**
> + * drm_gem_cma_mmap - memory-map a CMA GEM object
> + * @filp: file object
> + * @vma: VMA for the area to be mapped
> + *
> + * This function implements an augmented version of the GEM DRM file mmap
> + * operation for CMA objects: In addition to the usual GEM VMA setup it
> + * immediately faults in the entire object instead of using on-demaind
> + * faulting. Drivers which employ the CMA helpers should use this function
> + * as their ->mmap() handler in the DRM device file's file_operations
> + * structure.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
>   */
>  int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
> @@ -272,7 +338,16 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
>  EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
>  
>  #ifdef CONFIG_DEBUG_FS
> -void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m)
> +/**
> + * drm_gem_cma_describe - describe a CMA GEM object for debugfs
> + * @cma_obj: CMA GEM object
> + * @m: debugfs file handle
> + *
> + * This function can be used to dump a human-readable representation of the
> + * CMA GEM object into a synthetic file.
> + */
> +void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj,
> +			  struct seq_file *m)
>  {
>  	struct drm_gem_object *obj = &cma_obj->base;
>  	struct drm_device *dev = obj->dev;
> @@ -291,7 +366,18 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m
>  EXPORT_SYMBOL_GPL(drm_gem_cma_describe);
>  #endif
>  
> -/* low-level interface prime helpers */
> +/**
> + * drm_gem_cma_prime_get_sg_table - provide a scatter/gather table of pinned
> + *     pages for a CMA GEM object
> + * @obj: GEM object
> + *
> + * This function exports a scatter/gather table suitable for PRIME usage by
> + * calling the standard DMA mapping API. Drivers using the CMA helpers should
> + * set this as their DRM driver's ->gem_prime_get_sg_table() callback.
> + *
> + * Returns:
> + * A pointer to the scatter/gather table of pinned pages or NULL on failure.
> + */
>  struct sg_table *drm_gem_cma_prime_get_sg_table(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -315,6 +401,23 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_get_sg_table);
>  
> +/**
> + * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another
> + *     driver's scatter/gather table of pinned pages
> + * @dev: device to import into
> + * @attach: DMA-BUF attachment
> + * @sgt: scatter/gather table of pinned pages
> + *
> + * This function imports a scatter/gather table exported via DMA-BUF by
> + * another driver. Imported buffers must be physically contiguous in memory
> + * (i.e. the scatter/gather table must contain a single entry). Drivers that
> + * use the CMA helpers should set this as their DRM driver's
> + * ->gem_prime_import_sg_table() callback.
> + *
> + * Returns:
> + * A pointer to a newly created GEM object or an ERR_PTR-encoded negative
> + * error code on failure.
> + */
>  struct drm_gem_object *
>  drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  				  struct dma_buf_attachment *attach,
> @@ -339,6 +442,18 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
>  
> +/**
> + * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object
> + * @obj: GEM object
> + * @vma: VMA for the area to be mapped
> + *
> + * This function maps a buffer imported via DRM PRIME into a userspace
> + * process's address space. Drivers that use the CMA helpers should set this
> + * as their DRM driver's ->gem_prime_mmap() callback.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
>  int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
>  			   struct vm_area_struct *vma)
>  {
> @@ -357,6 +472,20 @@ int drm_gem_cma_prime_mmap(struct drm_gem_object *obj,
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_mmap);
>  
> +/**
> + * drm_gem_cma_prime_vmap - map a CMA GEM object into the kernel's virtual
> + *     address space
> + * @obj: GEM object
> + *
> + * This function maps a buffer exported via DRM PRIME into the kernel's
> + * virtual address space. Since the CMA buffers are already mapped into the
> + * kernel virtual address space this simply returns the cached virtual
> + * address. Drivers using the CMA helpers should set this as their DRM
> + * driver's ->gem_prime_vmap() callback.
> + *
> + * Returns:
> + * The kernel virtual address of the CMA GEM object's backing store.
> + */
>  void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
>  {
>  	struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
> @@ -365,6 +494,17 @@ void *drm_gem_cma_prime_vmap(struct drm_gem_object *obj)
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_cma_prime_vmap);
>  
> +/**
> + * drm_gem_cma_prime_vunmap - unmap a CMA GEM object from the kernel's virtual
> + *     address space
> + * @obj: GEM object
> + * @vaddr: kernel virtual address where the CMA GEM object was mapped
> + *
> + * This function removes a buffer exported via DRM PRIME from the kernel's
> + * virtual address space. This is a no-op because CMA buffers cannot be
> + * unmapped from kernel space. Drivers using the CMA helpers should set this
> + * as their DRM driver's ->gem_prime_vunmap() callback.
> + */
>  void drm_gem_cma_prime_vunmap(struct drm_gem_object *obj, void *vaddr)
>  {
>  	/* Nothing to do */
> diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
> index 2ff35f3de9c5..873d4eb7f125 100644
> --- a/include/drm/drm_gem_cma_helper.h
> +++ b/include/drm/drm_gem_cma_helper.h
> @@ -4,6 +4,13 @@
>  #include <drm/drmP.h>
>  #include <drm/drm_gem.h>
>  
> +/**
> + * struct drm_gem_cma_object - GEM object backed by CMA memory allocations
> + * @base: base GEM object
> + * @paddr: physical address of the backing memory
> + * @sgt: scatter/gather table for imported PRIME buffers
> + * @vaddr: kernel virtual address of the backing memory
> + */
>  struct drm_gem_cma_object {
>  	struct drm_gem_object base;
>  	dma_addr_t paddr;
> @@ -19,23 +26,25 @@ to_drm_gem_cma_obj(struct drm_gem_object *gem_obj)
>  	return container_of(gem_obj, struct drm_gem_cma_object, base);
>  }
>  
> -/* free gem object. */
> +/* free GEM object */
>  void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
>  
> -/* create memory region for drm framebuffer. */
> +/* create memory region for DRM framebuffer */
>  int drm_gem_cma_dumb_create(struct drm_file *file_priv,
> -		struct drm_device *drm, struct drm_mode_create_dumb *args);
> +			    struct drm_device *drm,
> +			    struct drm_mode_create_dumb *args);
>  
> -/* map memory region for drm framebuffer to user space. */
> +/* map memory region for DRM framebuffer to user space */
>  int drm_gem_cma_dumb_map_offset(struct drm_file *file_priv,
> -		struct drm_device *drm, uint32_t handle, uint64_t *offset);
> +				struct drm_device *drm, u32 handle,
> +				u64 *offset);
>  
> -/* set vm_flags and we can change the vm attribute to other one at here. */
> +/* set vm_flags and we can change the VM attribute to other one at here */
>  int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma);
>  
> -/* allocate physical memory. */
> +/* allocate physical memory */
>  struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
> -		unsigned int size);
> +					      size_t size);
>  
>  extern const struct vm_operations_struct drm_gem_cma_vm_ops;
>  
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 8/8] drm/cma: Remove call to drm_gem_free_mmap_offset()

[Daniel Vetter]

On Thu, Nov 06, 2014 at 04:49:21PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> drm_gem_object_release() called later in the drm_gem_cma_free_object()
> function already calls this, so there's no need to do this explicitly.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_gem_cma_helper.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 864b0863d042..e419eedf751d 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -187,8 +187,6 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
>  {
>  	struct drm_gem_cma_object *cma_obj;
>  
> -	drm_gem_free_mmap_offset(gem_obj);
> -
>  	cma_obj = to_drm_gem_cma_obj(gem_obj);
>  
>  	if (cma_obj->vaddr) {
> -- 
> 2.1.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/omap: gem: dumb: pitch is an output

[Rob Clark]

On Thu, Nov 6, 2014 at 10:49 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags fields are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size.
> Drivers must not treat these values as possible inputs, otherwise they
> may use uninitialized memory during the computation of the framebuffer
> size.
>
> The OMAP driver uses the pitch field passed in by userspace as a minimum
> and only override it if the driver-computed pitch is larger than what
> userspace provided. To prevent this from causing overallocation, fix the
> minimum pitch to 0 to enforce the driver-computed pitch.
>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Rob Clark <robdclark@gmail.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 e4849413ee80..bff60b73995b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -613,7 +613,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>         union omap_gem_size gsize;
>
>         /* in case someone tries to feed us a completely bogus stride: */
> -       args->pitch = align_pitch(args->pitch, args->width, args->bpp);
> +       args->pitch = align_pitch(0, args->width, args->bpp);
>         args->size = PAGE_ALIGN(args->pitch * args->height);
>
>         gsize = (union omap_gem_size){
> --
> 2.1.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/8] drm/omap: gem: dumb: pitch is an output

[Tomi Valkeinen]

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

On 06/11/14 17:49, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> When creating a dumb buffer object using the DRM_IOCTL_MODE_CREATE_DUMB
> IOCTL, only the width, height, bpp and flags fields are inputs. The
> caller is not guaranteed to zero out or set handle, pitch and size.
> Drivers must not treat these values as possible inputs, otherwise they
> may use uninitialized memory during the computation of the framebuffer
> size.
> 
> The OMAP driver uses the pitch field passed in by userspace as a minimum
> and only override it if the driver-computed pitch is larger than what
> userspace provided. To prevent this from causing overallocation, fix the
> minimum pitch to 0 to enforce the driver-computed pitch.
> 
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Thierry Reding <treding@nvidia.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 e4849413ee80..bff60b73995b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -613,7 +613,7 @@ int omap_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
>  	union omap_gem_size gsize;
>  
>  	/* in case someone tries to feed us a completely bogus stride: */

This comment could also be removed, as it doesn't make sense after this
change.

> -	args->pitch = align_pitch(args->pitch, args->width, args->bpp);
> +	args->pitch = align_pitch(0, args->width, args->bpp);
>  	args->size = PAGE_ALIGN(args->pitch * args->height);
>  
>  	gsize = (union omap_gem_size){

Acked-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

 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