[PATCH 00/19] DRM developer's guide polish, part 1

[PATCH 00/19] DRM developer's guide polish, part 1

[Daniel Vetter]

Hi all,

So I've finally gotten around to honor my promise for some nice drm_mm.c
documentation and got slightly sidetracked. I have more cleanups in my queue
(mostly to better document the modesetting functions and polish the kerneldoc
reference sections a bit) but David Herrmann suggested that reviewing smaller
doc patch piles is easier. So here we go.

The meat of this is documenting drm_mm.c and added kerneldoc for drm_prime.c,
with a few adjacent things I've stumbled over. I'm also trying to clarify the
overall DocBook a bit by emphasising more what's core code, which parts are just
helper functions and how it precisely ties together (e.g. both prime and dumb
buffers only assume that the buffer managers has uint32_t handles, nothing else
really).

Comments, flames and review highly welcome.

Cheers, Daniel

Daniel Vetter (19):
  drm/doc: Clarify the dumb object interfaces
  drm/doc: Fix up kerneldoc in drm_edid.c
  drm/doc: Clean up and integrate kerneldoc for drm_gem.c
  drm/doc: Remove <term> from rendernode docs
  drm/doc: Reorganize driver documentation
  drm/doc: Move the vma offset manager to the right spot
  drm/doc: Remove the "command submissin and fencing" section
  drm/doc: No more drm perf counters
  drm/doc: Document drm_helper_resume_force_mode
  drm/doc: Hide legacy horrors better
  drm/docs: Include hdmi infoframe helper reference
  drm/doc: Clarify PRIME documentation
  drm/doc: Add PRIME function references
  drm/doc: Update copyright
  drm/mm: Remove MM_UNUSED_TARGET
  drm/doc: Overview documentation for drm_mm.c
  drm/doc: Add fucntion reference documentation for drm_mm.c
  drm/kms: rip out drm_mode_connector_detach_encoder
  drm/kms: don't export drm_mode_group_init_legacy_group

 Documentation/DocBook/drm.tmpl             | 484 +++++++++++++++++++----------
 drivers/gpu/drm/drm_crtc.c                 |  16 -
 drivers/gpu/drm/drm_crtc_helper.c          |   9 +
 drivers/gpu/drm/drm_edid.c                 |  26 +-
 drivers/gpu/drm/drm_gem.c                  |  63 +++-
 drivers/gpu/drm/drm_mm.c                   | 211 +++++++++++--
 drivers/gpu/drm/drm_prime.c                | 110 +++++--
 drivers/staging/imx-drm/imx-ldb.c          |   2 -
 drivers/staging/imx-drm/parallel-display.c |   2 -
 include/drm/drm_crtc.h                     |   2 -
 include/drm/drm_mm.h                       | 154 +++++++--
 include/linux/hdmi.h                       |  12 +
 12 files changed, 815 insertions(+), 276 deletions(-)

-- 
1.8.5.2

[PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

- This is _not_ a generic interface to create gem objects, but just an
  interface to make early boot services (like boot splash) with a
  generic KMS userspace driver possible. Hence it's better to move
  the documentation for this from the GEM section to the KMS section,
  next to the creation of framebuffer objects.

- Make it really clear that the returned handle isn't necessarily a
  GEM object (it can also be e.g. a TTM handle when running on top of
  vmwgfx).

- Add a paragraph to make it clear that this is just for unaccelarated
  userspace - gpu drivers need to have their own buffer object
  creation ioctl which is hardware specific.

Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++++-------------------
 1 file changed, 68 insertions(+), 57 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ed1d6d289022..9c3fdd59c995 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -830,62 +830,6 @@ char *date;</synopsis>
         </para>
       </sect3>
       <sect3>
-        <title>Dumb GEM Objects</title>
-        <para>
-          The GEM API doesn't standardize GEM objects creation and leaves it to
-          driver-specific ioctls. While not an issue for full-fledged graphics
-          stacks that include device-specific userspace components (in libdrm for
-          instance), this limit makes DRM-based early boot graphics unnecessarily
-          complex.
-        </para>
-        <para>
-          Dumb GEM objects partly alleviate the problem by providing a standard
-          API to create dumb buffers suitable for scanout, which can then be used
-          to create KMS frame buffers.
-        </para>
-        <para>
-          To support dumb GEM objects drivers must implement the
-          <methodname>dumb_create</methodname>,
-          <methodname>dumb_destroy</methodname> and
-          <methodname>dumb_map_offset</methodname> operations.
-        </para>
-        <itemizedlist>
-          <listitem>
-            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
-                     struct drm_mode_create_dumb *args);</synopsis>
-            <para>
-              The <methodname>dumb_create</methodname> operation creates a GEM
-              object suitable for scanout based on the width, height and depth
-              from the struct <structname>drm_mode_create_dumb</structname>
-              argument. It fills the argument's <structfield>handle</structfield>,
-              <structfield>pitch</structfield> and <structfield>size</structfield>
-              fields with a handle for the newly created GEM object and its line
-              pitch and size in bytes.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
-                      uint32_t handle);</synopsis>
-            <para>
-              The <methodname>dumb_destroy</methodname> operation destroys a dumb
-              GEM object created by <methodname>dumb_create</methodname>.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
-                         uint32_t handle, uint64_t *offset);</synopsis>
-            <para>
-              The <methodname>dumb_map_offset</methodname> operation associates an
-              mmap fake offset with the GEM object given by the handle and returns
-              it. Drivers must use the
-              <function>drm_gem_create_mmap_offset</function> function to
-              associate the fake offset as described in
-              <xref linkend="drm-gem-objects-mapping"/>.
-            </para>
-          </listitem>
-        </itemizedlist>
-      </sect3>
-      <sect3>
         <title>Memory Coherency</title>
         <para>
           When mapped to the device or used in a command buffer, backing pages
@@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
         handle (or a list of memory handles for multi-planar formats) through
         the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
         assumes that the driver uses GEM, those handles thus reference GEM
-        objects.
+        objects. But drivers are free to use their own backing storage object
+	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
+	and so expects TTM handles in the create ioctl and not GEM objects.
       </para>
       <para>
         Drivers must first validate the requested frame buffer parameters passed
@@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
 	<function>drm_framebuffer_unregister_private</function>.
     </sect2>
     <sect2>
+      <title>Dumb GEM Objects</title>
+      <para>
+	The KMS API doesn't standardize backing storage object creation and
+	leaves it to driver-specific ioctls. Furthermore actually creating a
+	buffer object even for GEM-based drivers is done through a
+	driver-specific ioctl - GEM only has a common userspace interface for
+	sharing and destroying objects. While not an issue for full-fledged
+	graphics stacks that include device-specific userspace components (in
+	libdrm for instance), this limit makes DRM-based early boot graphics
+	unnecessarily complex.
+      </para>
+      <para>
+        Dumb objects partly alleviate the problem by providing a standard
+        API to create dumb buffers suitable for scanout, which can then be used
+        to create KMS frame buffers.
+      </para>
+      <para>
+        To support dumb objects drivers must implement the
+        <methodname>dumb_create</methodname>,
+        <methodname>dumb_destroy</methodname> and
+        <methodname>dumb_map_offset</methodname> operations.
+      </para>
+      <itemizedlist>
+        <listitem>
+          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
+                   struct drm_mode_create_dumb *args);</synopsis>
+          <para>
+            The <methodname>dumb_create</methodname> operation creates a driver
+	    object (GEM or TTM handle) object suitable for scanout based on the
+	    width, height and depth from the struct
+	    <structname>drm_mode_create_dumb</structname> argument. It fills the
+	    argument's <structfield>handle</structfield>,
+	    <structfield>pitch</structfield> and <structfield>size</structfield>
+	    fields with a handle for the newly created object and its line
+            pitch and size in bytes.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
+                    uint32_t handle);</synopsis>
+          <para>
+            The <methodname>dumb_destroy</methodname> operation destroys a dumb
+            object created by <methodname>dumb_create</methodname>.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
+                       uint32_t handle, uint64_t *offset);</synopsis>
+          <para>
+            The <methodname>dumb_map_offset</methodname> operation associates an
+            mmap fake offset with the object given by the handle and returns
+            it. Drivers must use the
+            <function>drm_gem_create_mmap_offset</function> function to
+            associate the fake offset as described in
+            <xref linkend="drm-gem-objects-mapping"/>.
+          </para>
+        </listitem>
+      </itemizedlist>
+      <para>
+        Note that dumb objects may not be used for gpu accelaration, as has been
+	attempted on some ARM embedded platforms. Such drivers really must have
+	a hardware-specific ioctl to allocate suitable objects.
+      </para>
+    </sect2>
+    <sect2>
       <title>Output Polling</title>
       <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis>
       <para>
-- 
1.8.5.2

[PATCH 02/19] drm/doc: Fix up kerneldoc in drm_edid.c

[Daniel Vetter]

v2: Also do s/RETURNS/Returns/, less yelling in docs is always good.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_edid.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b924306b8477..2d54e460c95b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1098,10 +1098,14 @@ EXPORT_SYMBOL(drm_edid_is_valid);
 /**
  * Get EDID information via I2C.
  *
- * \param adapter : i2c device adaptor
- * \param buf     : EDID data buffer to be filled
- * \param len     : EDID data buffer length
- * \return 0 on success or -1 on failure.
+ * @adapter : i2c device adaptor
+ * @buf: EDID data buffer to be filled
+ * @block: 128 byte EDID block to start fetching from
+ * @len: EDID data buffer length to fetch
+ *
+ * Returns:
+ *
+ * 0 on success or -1 on failure.
  *
  * Try to fetch EDID information by calling i2c driver function.
  */
@@ -1243,9 +1247,11 @@ out:
 
 /**
  * Probe DDC presence.
+ * @adapter: i2c adapter to probe
+ *
+ * Returns:
  *
- * \param adapter : i2c device adaptor
- * \return 1 on success
+ * 1 on success
  */
 bool
 drm_probe_ddc(struct i2c_adapter *adapter)
@@ -1586,8 +1592,10 @@ bad_std_timing(u8 a, u8 b)
 
 /**
  * drm_mode_std - convert standard mode info (width, height, refresh) into mode
+ * @connector: connector of for the EDID block
+ * @edid: EDID block to scan
  * @t: standard timing params
- * @timing_level: standard timing level
+ * @revision: standard timing level
  *
  * Take the standard timing params (in this case width, aspect, and refresh)
  * and convert them into a real mode using CVT/GTF/DMT.
@@ -2132,6 +2140,7 @@ do_established_modes(struct detailed_timing *timing, void *c)
 
 /**
  * add_established_modes - get est. modes from EDID and add them
+ * @connector: connector of for the EDID block
  * @edid: EDID block to scan
  *
  * Each EDID block contains a bitmap of the supported "established modes" list
@@ -2194,6 +2203,7 @@ do_standard_modes(struct detailed_timing *timing, void *c)
 
 /**
  * add_standard_modes - get std. modes from EDID and add them
+ * @connector: connector of for the EDID block
  * @edid: EDID block to scan
  *
  * Standard modes can be calculated using the appropriate standard (DMT,
@@ -3300,6 +3310,7 @@ EXPORT_SYMBOL(drm_detect_hdmi_monitor);
 
 /**
  * drm_detect_monitor_audio - check monitor audio capability
+ * @edid: EDID block to scan
  *
  * Monitor should have CEA extension block.
  * If monitor has 'basic audio', but no CEA audio blocks, it's 'basic
@@ -3345,6 +3356,7 @@ EXPORT_SYMBOL(drm_detect_monitor_audio);
 
 /**
  * drm_rgb_quant_range_selectable - is RGB quantization range selectable?
+ * @edid: EDID block to scan
  *
  * Check whether the monitor reports the RGB quantization range selection
  * as supported. The AVI infoframe can then be used to inform the monitor
-- 
1.8.5.2

[PATCH 03/19] drm/doc: Clean up and integrate kerneldoc for drm_gem.c

[Daniel Vetter]

Fairly incomplete, but at least a start.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl |  6 +++-
 drivers/gpu/drm/drm_gem.c      | 63 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9c3fdd59c995..0660bae6928f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -868,7 +868,11 @@ char *date;</synopsis>
           abstracted from the client in libdrm.
         </para>
       </sect3>
-    </sect2>
+      <sect2>
+        <title>GEM Function Reference</title>
+!Edrivers/gpu/drm/drm_gem.c
+      </sect2>
+      </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 5bbad873c798..2136052ccee1 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -85,9 +85,9 @@
 #endif
 
 /**
- * Initialize the GEM device fields
+ * drm_gem_init - Initialize the GEM device fields
+ * @dev: drm_devic structure to initialize
  */
-
 int
 drm_gem_init(struct drm_device *dev)
 {
@@ -120,6 +120,11 @@ drm_gem_destroy(struct drm_device *dev)
 }
 
 /**
+ * drm_gem_object_init - initialize an allocated shmem-backed GEM object
+ * @dev: drm_device the object should be initialized for
+ * @obj: drm_gem_object to initialize
+ * @size: object size
+ *
  * Initialize an already allocated GEM object of the specified size with
  * shmfs backing store.
  */
@@ -141,6 +146,11 @@ int drm_gem_object_init(struct drm_device *dev,
 EXPORT_SYMBOL(drm_gem_object_init);
 
 /**
+ * drm_gem_object_init - initialize an allocated private GEM object
+ * @dev: drm_device the object should be initialized for
+ * @obj: drm_gem_object to initialize
+ * @size: object size
+ *
  * Initialize an already allocated GEM object of the specified size with
  * no GEM provided backing store. Instead the caller is responsible for
  * backing the object and handling it.
@@ -176,6 +186,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
 }
 
 /**
+ * drm_gem_object_free - release resources bound to userspace handles
+ * @obj: GEM object to clean up.
+ *
  * Called after the last handle to the object has been closed
  *
  * Removes any name for the object. Note that this must be
@@ -225,7 +238,12 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
 }
 
 /**
- * Removes the mapping from handle to filp for this object.
+ * drm_gem_handle_delete - deletes the given file-private handle
+ * @filp: drm file-private structure to use for the handle look up
+ * @handle: userspace handle to delete
+ *
+ * Removes the GEM handle from the @filp lookup table and if this is the last
+ * handle also cleans up linked resources like GEM names.
  */
 int
 drm_gem_handle_delete(struct drm_file *filp, u32 handle)
@@ -270,6 +288,9 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
 
 /**
  * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
+ * @file: drm file-private structure to remove the dumb handle from
+ * @dev: corresponding drm_device
+ * @handle: the dumb handle to remove
  * 
  * This implements the ->dumb_destroy kms driver callback for drivers which use
  * gem to manage their backing storage.
@@ -284,6 +305,9 @@ 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
  * 
  * 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
@@ -336,6 +360,11 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
 }
 
 /**
+ * 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
+ *
  * Create a handle for this object. This adds a handle reference
  * to the object, which includes a regular reference count. Callers
  * will likely want to dereference the object afterwards.
@@ -536,6 +565,11 @@ drm_gem_object_lookup(struct drm_device *dev, struct drm_file *filp,
 EXPORT_SYMBOL(drm_gem_object_lookup);
 
 /**
+ * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
+ * @dev: drm_device
+ * @data: ioctl data
+ * @file_priv: drm file-private structure
+ *
  * Releases the handle to an mm object.
  */
 int
@@ -554,6 +588,11 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data,
 }
 
 /**
+ * drm_gem_flink_ioctl - implementation of the GEM_FLINK ioctl
+ * @dev: drm_device
+ * @data: ioctl data
+ * @file_priv: drm file-private structure
+ *
  * Create a global name for an object, returning the name.
  *
  * Note that the name does not hold a reference; when the object
@@ -601,6 +640,11 @@ err:
 }
 
 /**
+ * drm_gem_open - implementation of the GEM_OPEN ioctl
+ * @dev: drm_device
+ * @data: ioctl data
+ * @file_priv: drm file-private structure
+ *
  * Open an object using the global name, returning a handle and the size.
  *
  * This handle (of course) holds a reference to the object, so the object
@@ -640,6 +684,10 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
 }
 
 /**
+ * gem_gem_open - initalizes GEM file-private structures at devnode open time
+ * @dev: drm_device which is being opened by userspace
+ * @file_private: drm file-private structure to set up
+ *
  * Called at device open time, sets up the structure for handling refcounting
  * of mm objects.
  */
@@ -650,7 +698,7 @@ drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
 	spin_lock_init(&file_private->table_lock);
 }
 
-/**
+/*
  * Called at device close to release the file's
  * handle references on objects.
  */
@@ -674,6 +722,10 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 }
 
 /**
+ * drm_gem_release - release file-private GEM resources
+ * @dev: drm_device which is being closed by userspace
+ * @file_private: drm file-private structure to clean up
+ *
  * Called at close time when the filp is going away.
  *
  * Releases any remaining references on objects by this filp.
@@ -697,6 +749,9 @@ drm_gem_object_release(struct drm_gem_object *obj)
 EXPORT_SYMBOL(drm_gem_object_release);
 
 /**
+ * drm_gem_object_free - free a GEM object
+ * @kref: kref of the object to free
+ *
  * Called after the last reference to the object has been lost.
  * Must be called holding struct_ mutex
  *
-- 
1.8.5.2

[PATCH 04/19] drm/doc: Remove <term> from rendernode docs

[Daniel Vetter]

The stylesheet doesn't allow this in normal paragraphs.

Cc: David Herrmann dh.herrmann@gmail.com>
Acked-by: David Herrmann dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 0660bae6928f..3dc7ad7ff405 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2673,8 +2673,8 @@ int (*resume) (struct drm_device *);</synopsis>
         DRM core provides multiple character-devices for user-space to use.
         Depending on which device is opened, user-space can perform a different
         set of operations (mainly ioctls). The primary node is always created
-        and called <term>card&lt;num&gt;</term>. Additionally, a currently
-        unused control node, called <term>controlD&lt;num&gt;</term> is also
+        and called card&lt;num&gt;. Additionally, a currently
+        unused control node, called controlD&lt;num&gt; is also
         created. The primary node provides all legacy operations and
         historically was the only interface used by userspace. With KMS, the
         control node was introduced. However, the planned KMS control interface
@@ -2689,21 +2689,21 @@ int (*resume) (struct drm_device *);</synopsis>
         nodes were introduced. Render nodes solely serve render clients, that
         is, no modesetting or privileged ioctls can be issued on render nodes.
         Only non-global rendering commands are allowed. If a driver supports
-        render nodes, it must advertise it via the <term>DRIVER_RENDER</term>
+        render nodes, it must advertise it via the DRIVER_RENDER
         DRM driver capability. If not supported, the primary node must be used
         for render clients together with the legacy drmAuth authentication
         procedure.
       </para>
       <para>
         If a driver advertises render node support, DRM core will create a
-        separate render node called <term>renderD&lt;num&gt;</term>. There will
+        separate render node called renderD&lt;num&gt;. There will
         be one render node per device. No ioctls except  PRIME-related ioctls
-        will be allowed on this node. Especially <term>GEM_OPEN</term> will be
+        will be allowed on this node. Especially GEM_OPEN will be
         explicitly prohibited. Render nodes are designed to avoid the
         buffer-leaks, which occur if clients guess the flink names or mmap
         offsets on the legacy interface. Additionally to this basic interface,
         drivers must mark their driver-dependent render-only ioctls as
-        <term>DRM_RENDER_ALLOW</term> so render clients can use them. Driver
+        DRM_RENDER_ALLOW so render clients can use them. Driver
         authors must be careful not to allow any privileged ioctls on render
         nodes.
       </para>
-- 
1.8.5.2

[PATCH 05/19] drm/doc: Reorganize driver documentation

[Daniel Vetter]

Split up the DocBook into the core drm part and a 2nd part for
driver documentation. As an example add a very (very!) basic
skeleton for i915.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 80 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 3dc7ad7ff405..13330e236a9f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -60,7 +60,15 @@
 
 <toc></toc>
 
-  <!-- Introduction -->
+<part id="drmCore">
+  <title>DRM Core</title>
+  <partintro>
+    <para>
+      This first part of the DRM Developer's Guide documents core DRM code,
+      helper libraries for writting drivers and generic userspace interfaces
+      exposed by DRM drivers.
+    </para>
+  </partintro>
 
   <chapter id="drmIntroduction">
     <title>Introduction</title>
@@ -2764,15 +2772,73 @@ int (*resume) (struct drm_device *);</synopsis>
     </sect1>
 
   </chapter>
+</part>
+<part id="drmDrivers">
+  <title>DRM Drivers</title>
 
-  <!-- API reference -->
+  <partintro>
+    <para>
+      This second part of the DRM Developer's Guide documents driver code,
+      implementation details and also all the driver-specific userspace
+      interfaces. Especially since all hardware-accelaration interfaces to
+      userspace are driver specific for efficiency and other reasons these
+      interfaces can be rather substantial. Hence every driver has its own
+      chapter.
+    </para>
+  </partintro>
 
-  <appendix id="drmDriverApi">
-    <title>DRM Driver API</title>
+  <chapter id="drmI915">
+    <title>drm/i915 Intel GFX Driver</title>
     <para>
-      Include auto-generated API reference here (need to reference it
-      from paragraphs above too).
+      The drm/i915 driver supports all (with the exception of some very early
+      models) integrated GFX chipsets with both Intel display and rendering
+      blocks. This excludes a set of SoC platforms with an SGX rendering unit,
+      those have basic support through the gma500 drm driver.
     </para>
-  </appendix>
+    <sect1>
+      <title>Display Hardware Handling</title>
+      <para>
+        This section covers everything related to the display hardware including
+        the mode setting infrastructure, plane, sprite and cursor handling and
+        display, output probing and related topics.
+      </para>
+      <sect2>
+        <title>Mode Setting Infrastructure</title>
+        <para>
+          The i915 driver is thus far the only DRM driver which doesn't use the
+          common DRM helper code to implement mode setting sequences. Thus it
+          has its own tailor-made infrastructure for executing a display
+          configuration change.
+        </para>
+      </sect2>
+      <sect2>
+        <title>Plane Configuration</title>
+        <para>
+	  This section covers plane configuration and composition with the
+	  primary plane, sprites, cursors and overlays. This includes the
+	  infrastructure to do atomic vsync'ed updates of all this state and
+	  also tightly coupled topics like watermark setup and computation,
+	  framebuffer compression and panel self refresh.
+        </para>
+      </sect2>
+      <sect2>
+        <title>Output Probing</title>
+        <para>
+	  This section covers output probing and related infrastructure like the
+	  hotplug interrupt storm detection and mitigation code. Note that the
+	  i915 driver still uses most of the common DRM helper code for output
+	  probing, so those sections fully apply.
+        </para>
+      </sect2>
+    </sect1>
 
+    <sect1>
+      <title>Memory Management and Command Submission</title>
+      <para>
+	This sections covers all things related to the GEM implementation in the
+	i915 driver.
+      </para>
+    </sect1>
+  </chapter>
+</part>
 </book>
-- 
1.8.5.2

[PATCH 06/19] drm/doc: Move the vma offset manager to the right spot

[Daniel Vetter]

Currently it's sitting in the mode setting helper section, which isn't
quite right. Looks much better in the memory management section next
to TTM and GEM.

Cc: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 13330e236a9f..d5b087dee14d 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -881,6 +881,12 @@ char *date;</synopsis>
 !Edrivers/gpu/drm/drm_gem.c
       </sect2>
       </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>
   </sect1>
 
   <!-- Internals: mode setting -->
@@ -2218,12 +2224,6 @@ void intel_crt_init(struct drm_device *dev)
 !Iinclude/drm/drm_flip_work.h
 !Edrivers/gpu/drm/drm_flip_work.c
     </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>
   </sect1>
 
   <!-- Internals: kms properties -->
-- 
1.8.5.2

[PATCH 07/19] drm/doc: Remove the "command submissin and fencing" section

[Daniel Vetter]

This should be done in the driver chapter instead.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index d5b087dee14d..9625cf7aa96a 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2586,16 +2586,6 @@ int num_ioctls;</synopsis>
   </sect1>
 
   <sect1>
-    <title>Command submission &amp; fencing</title>
-    <para>
-      This should cover a few device-specific command submission
-      implementations.
-    </para>
-  </sect1>
-
-  <!-- Internals: suspend/resume -->
-
-  <sect1>
     <title>Suspend/Resume</title>
     <para>
       The DRM core provides some suspend/resume code, but drivers wanting full
-- 
1.8.5.2

[PATCH 08/19] drm/doc: No more drm perf counters

[Daniel Vetter]

Those all died with

commit 0111be42186fc5461b9e9d579014c70869ab3152
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Fri Oct 4 14:53:41 2013 +0300

    drm: Kill drm perf counter leftovers

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9625cf7aa96a..975a4d0a1d3f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -272,8 +272,8 @@ char *date;</synopsis>
       <para>
         The <methodname>load</methodname> method is the driver and device
         initialization entry point. The method is responsible for allocating and
-        initializing driver private data, specifying supported performance
-        counters, performing resource allocation and mapping (e.g. acquiring
+	initializing driver private data, performing resource allocation and
+	mapping (e.g. acquiring
         clocks, mapping registers or allocating command buffers), initializing
         the memory manager (<xref linkend="drm-memory-management"/>), installing
         the IRQ handler (<xref linkend="drm-irq-registration"/>), setting up
@@ -303,7 +303,7 @@ char *date;</synopsis>
 	their <methodname>load</methodname> method called with flags to 0.
       </para>
       <sect3>
-        <title>Driver Private &amp; Performance Counters</title>
+        <title>Driver Private Data</title>
         <para>
           The driver private hangs off the main
           <structname>drm_device</structname> structure and can be used for
@@ -315,14 +315,6 @@ char *date;</synopsis>
           <structname>drm_device</structname>.<structfield>dev_priv</structfield>
           set to NULL when the driver is unloaded.
         </para>
-        <para>
-          DRM supports several counters which were used for rough performance
-          characterization. This stat counter system is deprecated and should not
-          be used. If performance monitoring is desired, the developer should
-          investigate and potentially enhance the kernel perf and tracing
-          infrastructure to export GPU related performance information for
-          consumption by performance monitoring tools and applications.
-        </para>
       </sect3>
       <sect3 id="drm-irq-registration">
         <title>IRQ Registration</title>
-- 
1.8.5.2

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

[PATCH 09/19] drm/doc: Document drm_helper_resume_force_mode

[Daniel Vetter]

Stumbled over while reviewing all occurences in the DRM doc talking
about suspend/resume.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl    | 7 +++++--
 drivers/gpu/drm/drm_crtc_helper.c | 9 +++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 975a4d0a1d3f..e63a3aae3f14 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -1151,8 +1151,11 @@ int max_width, max_height;</synopsis>
             This operation is called with the mode config lock held.
           </para>
           <note><para>
-            FIXME: How should set_config interact with DPMS? If the CRTC is
-            suspended, should it be resumed?
+	    Note that the drm core has no notion of restoring the mode setting
+	    state after resume, since all resume handling is in the full
+	    responsibility of the driver. The common mode setting helper library
+	    though provides a helper which can be used for this:
+	    <function>drm_helper_resume_force_mode</function>.
           </para></note>
         </sect4>
         <sect4>
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
index 01361aba033b..121ebeaa4cfd 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -972,6 +972,15 @@ int drm_helper_mode_fill_fb_struct(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_helper_mode_fill_fb_struct);
 
+/**
+ * drm_helper_resume_force_mode - force-restore mode setting configuration
+ * @dev: drm_device which should be restored
+ *
+ * Drivers which use the mode setting helpers can use this function to
+ * force-restore the mode setting configuration e.g. on resume or when something
+ * else might have trampled over the hw state (like some overzealous old BIOSen
+ * tended to do).
+ */
 int drm_helper_resume_force_mode(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
-- 
1.8.5.2

[PATCH 10/19] drm/doc: Hide legacy horrors better

[Daniel Vetter]

By consolidating them all into one section at the very end. And to
make double-sure that no one gets confused start with a stern warning
against any use of them. And prefix all subsections with "Legacy".

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 56 +++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index e63a3aae3f14..251b87cfad8c 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2579,32 +2579,44 @@ int num_ioctls;</synopsis>
       </para>
     </sect2>
   </sect1>
-
   <sect1>
-    <title>Suspend/Resume</title>
-    <para>
-      The DRM core provides some suspend/resume code, but drivers wanting full
-      suspend/resume support should provide save() and restore() functions.
-      These are called at suspend, hibernate, or resume time, and should perform
-      any state save or restore required by your device across suspend or
-      hibernate states.
-    </para>
-    <synopsis>int (*suspend) (struct drm_device *, pm_message_t state);
-int (*resume) (struct drm_device *);</synopsis>
+    <title>Legacy Support Code</title>
     <para>
-      Those are legacy suspend and resume methods. New driver should use the
-      power management interface provided by their bus type (usually through
-      the struct <structname>device_driver</structname> dev_pm_ops) and set
-      these methods to NULL.
+      The section very brievely covers some of the old legacy support code which
+      is only used by old DRM drivers which have done a so-called shadow-attach
+      to the underlying device instead of registering as a real driver. This
+      also includes some of the old generic buffer mangement and command
+      submission code. Do not use any of this in new and modern drivers.
     </para>
-  </sect1>
 
-  <sect1>
-    <title>DMA services</title>
-    <para>
-      This should cover how DMA mapping etc. is supported by the core.
-      These functions are deprecated and should not be used.
-    </para>
+    <sect2>
+      <title>Legacy Suspend/Resume</title>
+      <para>
+	The DRM core provides some suspend/resume code, but drivers wanting full
+	suspend/resume support should provide save() and restore() functions.
+	These are called at suspend, hibernate, or resume time, and should perform
+	any state save or restore required by your device across suspend or
+	hibernate states.
+      </para>
+      <synopsis>int (*suspend) (struct drm_device *, pm_message_t state);
+  int (*resume) (struct drm_device *);</synopsis>
+      <para>
+	Those are legacy suspend and resume methods which
+	<emphasis>only</emphasis> work with the legacy shadow-attach driver
+	registration functions. New driver should use the power management
+	interface provided by their bus type (usually through
+	the struct <structname>device_driver</structname> dev_pm_ops) and set
+	these methods to NULL.
+      </para>
+    </sect2>
+
+    <sect2>
+      <title>Legacy DMA Services</title>
+      <para>
+	This should cover how DMA mapping etc. is supported by the core.
+	These functions are deprecated and should not be used.
+      </para>
+    </sect2>
   </sect1>
   </chapter>
 
-- 
1.8.5.2

[PATCH 11/19] drm/docs: Include hdmi infoframe helper reference

[Daniel Vetter]

Thierry created such nice kerneldocs, it's a shame we've left them
lingering!

For the fun of it also add a bit of kerneldoc to the header so that we
can also include that. Just in case someone adds kerneldoc in there.

Cc: Thierry Reding thierry.reding@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 11 +++++++++++
 include/linux/hdmi.h           | 12 ++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 251b87cfad8c..af4d05b0c4fd 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2219,6 +2219,17 @@ void intel_crt_init(struct drm_device *dev)
 !Iinclude/drm/drm_flip_work.h
 !Edrivers/gpu/drm/drm_flip_work.c
     </sect2>
+    <sect2>
+      <title>HDMI Infoframes Helper Reference</title>
+      <para>
+	Strictly speaking this is not a DRM helper library but generally useable
+	by any driver interfacing with HDMI outputs like v4l or alsa drivers.
+	But it nicely fits into the overall topic of mode setting helper
+	libraries and hence is also included here.
+      </para>
+!Iinclude/linux/hdmi.h
+!Edrivers/video/hdmi.c
+    </sect2>
   </sect1>
 
   <!-- Internals: kms properties -->
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 9231be9e90a2..11c0182a153b 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -262,6 +262,18 @@ union hdmi_vendor_any_infoframe {
 	struct hdmi_vendor_infoframe hdmi;
 };
 
+/**
+ * union hdmi_infoframe - overall union of all abstract infoframe representations
+ * @any: generic infoframe
+ * @avi: avi infoframe
+ * @spd: spd infoframe
+ * @vendor: union of all vendor infoframes
+ * @audio: audio infoframe
+ *
+ * This is used by the generic pack function. This works since all infoframes
+ * have the same header which also indicates which type of infoframe should be
+ * packed.
+ */
 union hdmi_infoframe {
 	struct hdmi_any_infoframe any;
 	struct hdmi_avi_infoframe avi;
-- 
1.8.5.2

[PATCH 12/19] drm/doc: Clarify PRIME documentation

[Daniel Vetter]

PRIME fds aren't actually GEM fds but are (like the modeset API)
independent of the underlying buffer manager, as long as that one uses
uint32_t as handles. So move that entire section out of the GEM
section and reword it a bit to clarify which parts of PRIME are
generic, and which are the mandatory pieces for GEM drivers to
correctly implement the GEM lifetime rules. The rewording mostly
consists of not mixing up GEM, PRIME and DRM.

I've considered adding some blurbs to the GEM object lifetime section
about interactions with dma-bufs, but then dropped that. As long as
drivers use the right helpers they should have this all implemented
correctly and hence can be regarded as an implementation detail of the
PRIME/GEM helpers. So no need to confuse driver writers with those
tricky interactions.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++++++-----------------
 1 file changed, 74 insertions(+), 51 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index af4d05b0c4fd..0cc1d85d04de 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -697,55 +697,16 @@ char *date;</synopsis>
           respectively. The conversion is handled by the DRM core without any
           driver-specific support.
         </para>
-        <para>
-          Similar to global names, GEM file descriptors are also used to share GEM
-          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 GEM file descriptors, also known as the DRM 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.
-        </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 DRM
-          PRIME.
-        </para>
-        <sect4>
-          <title>DRM PRIME Helper Functions Reference</title>
-!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
-        </sect4>
+	<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>
@@ -868,10 +829,10 @@ char *date;</synopsis>
           abstracted from the client in libdrm.
         </para>
       </sect3>
-      <sect2>
+      <sect3>
         <title>GEM Function Reference</title>
 !Edrivers/gpu/drm/drm_gem.c
-      </sect2>
+      </sect3>
       </sect2>
       <sect2>
 	<title>VMA Offset Manager</title>
@@ -879,6 +840,68 @@ char *date;</synopsis>
 !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);
+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 unsinged 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 Reference</title>
+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
+        </sect3>
+      </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
-- 
1.8.5.2

[PATCH 13/19] drm/doc: Add PRIME function references

[Daniel Vetter]

For giant hilarity the DocBook reference overview is only generated
when in a level 2 section, not in a level 3 section. So we need to
move this up a bit as a side-by-side section to the main PRIME
documentation.

Whatever.

To have a complete set of references add the missing kerneldoc for all
functions exported to modules with the exception of the file private
init/destry functions - drivers have no business calling those, so
let's just drop the EXPORT_SYMBOL instead.

Also reflow the function parameters to align correctly and break at 80
chars - my OCD couldn't stand them while writing the kerneldoc ;-)

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl |   6 ++-
 drivers/gpu/drm/drm_prime.c    | 110 +++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 22 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 0cc1d85d04de..07abe52b1176 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -898,10 +898,14 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 	  </para>
 	</sect3>
         <sect3>
-          <title>PRIME Helper Functions Reference</title>
+          <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>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 56805c39c906..f1437b6c8dbf 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -68,7 +68,8 @@ struct drm_prime_attachment {
 	enum dma_data_direction dir;
 };
 
-static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
+static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
+				    struct dma_buf *dma_buf, uint32_t handle)
 {
 	struct drm_prime_member *member;
 
@@ -174,7 +175,7 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
 }
 
 static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
-		enum dma_data_direction dir)
+					    enum dma_data_direction dir)
 {
 	struct drm_prime_attachment *prime_attach = attach->priv;
 	struct drm_gem_object *obj = attach->dmabuf->priv;
@@ -211,11 +212,19 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
 }
 
 static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
-		struct sg_table *sgt, enum dma_data_direction dir)
+				  struct sg_table *sgt,
+				  enum dma_data_direction dir)
 {
 	/* nothing to be done here */
 }
 
+/**
+ * drm_gem_dmabuf_release - dma_buf release implementation for GEM
+ * @dma_buf: buffer to be released
+ *
+ * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
+ * must use this in their dma_buf ops structure as the release callback.
+ */
 void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
@@ -242,30 +251,30 @@ static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
 }
 
 static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
-		unsigned long page_num)
+					unsigned long page_num)
 {
 	return NULL;
 }
 
 static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
-		unsigned long page_num, void *addr)
+					 unsigned long page_num, void *addr)
 {
 
 }
 static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
-		unsigned long page_num)
+				 unsigned long page_num)
 {
 	return NULL;
 }
 
 static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
-		unsigned long page_num, void *addr)
+				  unsigned long page_num, void *addr)
 {
 
 }
 
 static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
-		struct vm_area_struct *vma)
+			       struct vm_area_struct *vma)
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 	struct drm_device *dev = obj->dev;
@@ -315,6 +324,15 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
  *    driver's scatter/gather table
  */
 
+/**
+ * drm_gem_prime_export - helper library implemention of the export callback
+ * @dev: drm_device to export from
+ * @obj: GEM object to export
+ * @flags: flags like DRM_CLOEXEC
+ *
+ * This is the implementation of the gem_prime_export functions for GEM drivers
+ * using the PRIME helpers.
+ */
 struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
 				     struct drm_gem_object *obj, int flags)
 {
@@ -355,9 +373,23 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 	return dmabuf;
 }
 
+/**
+ * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
+ * @dev: dev to export the buffer from
+ * @file_priv: drm file-private structure
+ * @handle: buffer handle to export
+ * @flags: flags like DRM_CLOEXEC
+ * @prime_fd: pointer to storage for the fd id of the create dma-buf
+ *
+ * This is the PRIME export function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual exporting from GEM object to a dma-buf is done through the
+ * gem_prime_export driver callback.
+ */
 int drm_gem_prime_handle_to_fd(struct drm_device *dev,
-		struct drm_file *file_priv, uint32_t handle, uint32_t flags,
-		int *prime_fd)
+			       struct drm_file *file_priv, uint32_t handle,
+			       uint32_t flags,
+			       int *prime_fd)
 {
 	struct drm_gem_object *obj;
 	int ret = 0;
@@ -441,6 +473,14 @@ out_unlock:
 }
 EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
 
+/**
+ * drm_gem_prime_import - helper library implemention of the import callback
+ * @dev: drm_device to import into
+ * @dma_buf: dma-buf object to import
+ *
+ * This is the implementation of the gem_prime_import functions for GEM drivers
+ * using the PRIME helpers.
+ */
 struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
 					    struct dma_buf *dma_buf)
 {
@@ -496,8 +536,21 @@ fail_detach:
 }
 EXPORT_SYMBOL(drm_gem_prime_import);
 
+/**
+ * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
+ * @dev: dev to export the buffer from
+ * @file_priv: drm file-private structure
+ * @prime_fd: fd id of the dma-buf which should be imported
+ * @handle: pointer to storage for the handle of the imported buffer object
+ *
+ * This is the PRIME import function which must be used mandatorily by GEM
+ * drivers to ensure correct lifetime management of the underlying GEM object.
+ * The actual importing of GEM object from the dma-buf is done through the
+ * gem_import_export driver callback.
+ */
 int drm_gem_prime_fd_to_handle(struct drm_device *dev,
-		struct drm_file *file_priv, int prime_fd, uint32_t *handle)
+			       struct drm_file *file_priv, int prime_fd,
+			       uint32_t *handle)
 {
 	struct dma_buf *dma_buf;
 	struct drm_gem_object *obj;
@@ -598,12 +651,14 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
 			args->fd, &args->handle);
 }
 
-/*
- * drm_prime_pages_to_sg
+/**
+ * drm_prime_pages_to_sg - converts a page array into an sg list
+ * @pages: pointer to the array of page pointers to convert
+ * @nr_pages: length of the page vector
  *
- * this helper creates an sg table object from a set of pages
+ * This helper creates an sg table object from a set of pages
  * the driver is responsible for mapping the pages into the
- * importers address space
+ * importers address space for use with dma_buf itself.
  */
 struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
 {
@@ -628,9 +683,16 @@ out:
 }
 EXPORT_SYMBOL(drm_prime_pages_to_sg);
 
-/* export an sg table into an array of pages and addresses
-   this is currently required by the TTM driver in order to do correct fault
-   handling */
+/**
+ * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
+ * @sgt: scatter-gather table to convert
+ * @pages: array of page pointers to store the page array in
+ * @addrs: optional array to store the dma bus address of each page
+ * @max_pages: size of both the passed-in arrays
+ *
+ * Exports an sg table into an array of pages and addresses. This is currently
+ * required by the TTM driver in order to do correct fault handling.
+ */
 int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
 				     dma_addr_t *addrs, int max_pages)
 {
@@ -663,7 +725,15 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
 	return 0;
 }
 EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
-/* helper function to cleanup a GEM/prime object */
+
+/**
+ * drm_prime_gem_destroy - helper to clean up a PRIME-imported GEM object
+ * @obj: GEM object which was created from a dma-buf
+ * @sg: the sg-table which was pinned at import time
+ *
+ * This is the cleanup functions which GEM drivers need to call when they use
+ * @drm_gem_prime_import to import dma-bufs.
+ */
 void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
 {
 	struct dma_buf_attachment *attach;
@@ -683,11 +753,9 @@ void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
 	INIT_LIST_HEAD(&prime_fpriv->head);
 	mutex_init(&prime_fpriv->lock);
 }
-EXPORT_SYMBOL(drm_prime_init_file_private);
 
 void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
 {
 	/* by now drm_gem_release should've made sure the list is empty */
 	WARN_ON(!list_empty(&prime_fpriv->head));
 }
-EXPORT_SYMBOL(drm_prime_destroy_file_private);
-- 
1.8.5.2

[PATCH 14/19] drm/doc: Update copyright

[Daniel Vetter]

I've done quite a bit of cleanups, clarifications and mostly
integrating kerneldoc. So I guess I should add myself.

Also split up the copyright notices per holder to make it clear which
year ranges are covered.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 07abe52b1176..ede383af1868 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -29,12 +29,26 @@
 	  </address>
 	</affiliation>
       </author>
+      <author>
+	<firstname>Daniel</firstname>
+	<surname>Vetter</surname>
+	<contrib>Contributions all over the place</contrib>
+	<affiliation>
+	  <orgname>Intel Corporation</orgname>
+	  <address>
+	    <email>daniel.vetter@ffwll.ch</email>
+	  </address>
+	</affiliation>
+      </author>
     </authorgroup>
 
     <copyright>
       <year>2008-2009</year>
-      <year>2012</year>
+      <year>2013-2014</year>
       <holder>Intel Corporation</holder>
+    </copyright>
+    <copyright>
+      <year>2012</year>
       <holder>Laurent Pinchart</holder>
     </copyright>
 
-- 
1.8.5.2

[PATCH 15/19] drm/mm: Remove MM_UNUSED_TARGET

[Daniel Vetter]

This was missed in

commit c700c67bae6698fbc6bd20e2ae5dc62ddd367b3b
Author: David Herrmann <dh.herrmann@gmail.com>
Date:   Sat Jul 27 13:39:28 2013 +0200

    drm/mm: remove unused API

Cc: David Herrmann <dh.herrmann@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_mm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index af93cc55259f..d0a8e8482fe0 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -47,8 +47,6 @@
 #include <linux/seq_file.h>
 #include <linux/export.h>
 
-#define MM_UNUSED_TARGET 4
-
 static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
 						unsigned long size,
 						unsigned alignment,
-- 
1.8.5.2

[PATCH 16/19] drm/doc: Overview documentation for drm_mm.c

[Daniel Vetter]

kerneldoc polish will follow in the next patch.

Hopefully documenting the lru scan support a bit better spurse someone
to give this a shot in the ttm eviction code. At least in i915 it
helped quite a lot with memory thrashing on platforms where eviction
was (we've fixed that too meanwhile) fairly expensive.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 11 +++++++
 drivers/gpu/drm/drm_mm.c       | 67 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ede383af1868..c6f916989b92 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -920,6 +920,17 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 	<title>PRIME Function References</title>
 !Edrivers/gpu/drm/drm_prime.c
       </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>
+!Pdrivers/gpu/drm/drm_mm.c lru scan roaster
+	</sect3>
+      </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index d0a8e8482fe0..276a7a27c166 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -47,6 +47,45 @@
 #include <linux/seq_file.h>
 #include <linux/export.h>
 
+/**
+ * DOC: Overview
+ *
+ * drm_mm provides a simple range allocator. The drivers are free to use the
+ * resource allocator from the linux core if it suits them, the upside of drm_mm
+ * is that it's in the DRM core. Which means that it's easier to extend for
+ * some of the crazier special purpose needs of gpus.
+ *
+ * The main data struct is &drm_mm, allocations are tracked in &drm_mm_node.
+ * Drivers are free to embed either of them into their own suitable
+ * datastructures. drm_mm itself will not do any allocations of its own, so if
+ * drivers choose not to embed nodes they need to still allocate them
+ * themselves.
+ *
+ * The range allocator also supports reservation of preallocated blocks. This is
+ * useful for taking over initial mode setting configurations from the firmware,
+ * where an object needs to be created which exactly matches the firmware's
+ * scanout target. As long as the range is still free it can be inserted anytime
+ * after the allocator is initialized, which helps with avoiding looped
+ * depencies in the driver load sequence.
+ *
+ * drm_mm maintains a stack of most recently freed holes, which of all
+ * simplistic datastructures seems to be a fairly decent approach to clustering
+ * allocations and avoiding too much fragmentation. This means free space
+ * searches are O(num_holes). Given that all the fancy features drm_mm supports
+ * something better would be fairly complex and since gfx thrashing is a fairly
+ * steep cliff not a real concern. Removing a node again is O(1).
+ *
+ * drm_mm supports a few features: Alignment and range restrictions can be
+ * supplied. Further more every &drm_mm_node has a color value (which is just an
+ * opaqua unsigned long) which in conjunction with a driver callback can be used
+ * to implement sophisticated placement restrictions. The i915 DRM driver uses
+ * this to implement guard pages between incompatible caching domains in the
+ * graphics TT.
+ *
+ * Finally iteration helpers to walk all nodes and all holes are provided as are
+ * some basic allocator dumpers for debugging.
+ */
+
 static struct drm_mm_node *drm_mm_search_free_generic(const struct drm_mm *mm,
 						unsigned long size,
 						unsigned alignment,
@@ -400,6 +439,34 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new)
 EXPORT_SYMBOL(drm_mm_replace_node);
 
 /**
+ * DOC: lru scan roaster
+ *
+ * Very often GPUs need to have continuous allocations for a given object. When
+ * evicting objects to make space for a new one it is therefore not most
+ * efficient when we simply start to select all objects from the tail of an LRU
+ * until there's a suitable hole: Especially for big objects or nodes that
+ * otherwise have special allocation constraints there's a good chance we evict
+ * lots of (smaller) objects unecessarily.
+ *
+ * The DRM range allocator supports this use-case through the scanning
+ * interfaces. First a scan operation needs to be initialized with
+ * drm_mm_init_scan() or drm_mm_init_scan_with_range(). The the driver adds
+ * objects to the roaster (probably by walking an LRU list, but this can be
+ * freely implemented) until a suitable hole is found or there's no further
+ * evitable object.
+ *
+ * The the driver must walk through all objects again in exactly the reverse
+ * order to restore the allocator state. Note that while the allocator is used
+ * in the scan mode no other operation is allowed.
+ *
+ * Finally the driver evicts all objects selected in the scan. Adding and
+ * removing an object is O(1), and since freeing a node is also O(1) the overall
+ * complexity is O(scanned_objects). So like the free stack which needs to be
+ * walked before a scan operation even begins this is linear in the number of
+ * objects. It doesn't seem to hurt badly.
+ */
+
+/**
  * Initializa lru scanning.
  *
  * This simply sets up the scanning routines with the parameters for the desired
-- 
1.8.5.2

[PATCH 17/19] drm/doc: Add fucntion reference documentation for drm_mm.c

[Daniel Vetter]

While at it do a tiny bit of interface cleanup and convert boolean
return values to bool. With this patch all exported functions and inline
helpers which are part of the drm_mm public interface are documented.

Also drop superflous extern function modifiers since most of drm_mm.h
doesn't use them - more consistent that way.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl |   5 ++
 drivers/gpu/drm/drm_mm.c       | 144 ++++++++++++++++++++++++++++++++------
 include/drm/drm_mm.h           | 154 +++++++++++++++++++++++++++++++++--------
 3 files changed, 251 insertions(+), 52 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index c6f916989b92..d36e97126f59 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -931,6 +931,11 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
 !Pdrivers/gpu/drm/drm_mm.c lru scan roaster
 	</sect3>
       </sect2>
+      <sect2>
+	<title>DRM MM Range Allocator Function References</title>
+!Edrivers/gpu/drm/drm_mm.c
+!Iinclude/drm/drm_mm.h
+      </sect2>
   </sect1>
 
   <!-- Internals: mode setting -->
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
index 276a7a27c166..a2d45b748f86 100644
--- a/drivers/gpu/drm/drm_mm.c
+++ b/drivers/gpu/drm/drm_mm.c
@@ -144,6 +144,20 @@ static void drm_mm_insert_helper(struct drm_mm_node *hole_node,
 	}
 }
 
+/**
+ * drm_mm_reserve_node - insert an pre-initialized node
+ * @mm: drm_mm allocator to insert @node into
+ * @node: drm_mm_node to insert
+ *
+ * This functions inserts an already set-up drm_mm_node into the allocator,
+ * meaning that start, size and color must be set by the caller. This is useful
+ * to initialize the allocator with preallocated objects which must be set-up
+ * before the range allocator can be set-up, e.g. when taking over a firmware
+ * framebuffer.
+ *
+ * Returns:
+ * 0 on success, -ENOSPC if there's no hole where @node is.
+ */
 int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
 {
 	struct drm_mm_node *hole;
@@ -185,9 +199,18 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
 EXPORT_SYMBOL(drm_mm_reserve_node);
 
 /**
- * Search for free space and insert a preallocated memory node. Returns
- * -ENOSPC if no suitable free area is available. The preallocated memory node
- * must be cleared.
+ * drm_mm_insert_node_generic - search for space and insert @node
+ * @mm: drm_mm to allocate from
+ * @node: preallocate node to insert
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @color: opaque tag value to use for this node
+ * @flags: flags to fine-tune the allocation
+ *
+ * The preallocated node must be cleared to 0.
+ *
+ * Returns:
+ * 0 on success, -ENOSPC if there's no suitable hole.
  */
 int drm_mm_insert_node_generic(struct drm_mm *mm, struct drm_mm_node *node,
 			       unsigned long size, unsigned alignment,
@@ -259,9 +282,20 @@ static void drm_mm_insert_helper_range(struct drm_mm_node *hole_node,
 }
 
 /**
- * Search for free space and insert a preallocated memory node. Returns
- * -ENOSPC if no suitable free area is available. This is for range
- * restricted allocations. The preallocated memory node must be cleared.
+ * drm_mm_insert_node_in_range_generic - ranged search for space and insert @node
+ * @mm: drm_mm to allocate from
+ * @node: preallocate node to insert
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @color: opaque tag value to use for this node
+ * @start: start of the allowed range for this node
+ * @end: end of the allowed range for this node
+ * @flags: flags to fine-tune the allocation
+ *
+ * The preallocated node must be cleared to 0.
+ *
+ * Returns:
+ * 0 on success, -ENOSPC if there's no suitable hole.
  */
 int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, struct drm_mm_node *node,
 					unsigned long size, unsigned alignment, unsigned long color,
@@ -284,7 +318,12 @@ int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, struct drm_mm_node *n
 EXPORT_SYMBOL(drm_mm_insert_node_in_range_generic);
 
 /**
- * Remove a memory node from the allocator.
+ * drm_mm_remove_node - Remove a memory node from the allocator.
+ * @node: drm_mm_node to remove
+ *
+ * This just removes a node from its drm_mm allocator. The node does not need to
+ * be cleared again before it can be re-inserted into this or any other drm_mm
+ * allocator. It is a bug to call this function on a un-allocated node.
  */
 void drm_mm_remove_node(struct drm_mm_node *node)
 {
@@ -421,7 +460,13 @@ static struct drm_mm_node *drm_mm_search_free_in_range_generic(const struct drm_
 }
 
 /**
- * Moves an allocation. To be used with embedded struct drm_mm_node.
+ * drm_mm_replace_node - move an allocation from @old to @new
+ * @old: drm_mm_node to remove from the allocator
+ * @new: drm_mm_node which should inherit @old's allocation
+ *
+ * This is useful for when drivers embed the drm_mm_node structure and hence
+ * can't move allocations by reassigning pointers. It's a combination of remove
+ * and insert with the guarantee that the allocation start will match.
  */
 void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new)
 {
@@ -467,12 +512,18 @@ EXPORT_SYMBOL(drm_mm_replace_node);
  */
 
 /**
- * Initializa lru scanning.
+ * drm_mm_init_scan - initialize lru scanning
+ * @mm: drm_mm to scan
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @color: opaque tag value to use for the allocation
  *
  * This simply sets up the scanning routines with the parameters for the desired
- * hole.
+ * hole. Note that there's no need to specify allocation flags, since they only
+ * change the place a node is allocated from within a suitable hole.
  *
- * Warning: As long as the scan list is non-empty, no other operations than
+ * Warning:
+ * As long as the scan list is non-empty, no other operations than
  * adding/removing nodes to/from the scan list are allowed.
  */
 void drm_mm_init_scan(struct drm_mm *mm,
@@ -492,12 +543,20 @@ void drm_mm_init_scan(struct drm_mm *mm,
 EXPORT_SYMBOL(drm_mm_init_scan);
 
 /**
- * Initializa lru scanning.
+ * drm_mm_init_scan - initialize range-restricted lru scanning
+ * @mm: drm_mm to scan
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @color: opaque tag value to use for the allocation
+ * @start: start of the allowed range for the allocation
+ * @end: end of the allowed range for the allocation
  *
  * This simply sets up the scanning routines with the parameters for the desired
- * hole. This version is for range-restricted scans.
+ * hole. Note that there's no need to specify allocation flags, since they only
+ * change the place a node is allocated from within a suitable hole.
  *
- * Warning: As long as the scan list is non-empty, no other operations than
+ * Warning:
+ * As long as the scan list is non-empty, no other operations than
  * adding/removing nodes to/from the scan list are allowed.
  */
 void drm_mm_init_scan_with_range(struct drm_mm *mm,
@@ -521,12 +580,16 @@ void drm_mm_init_scan_with_range(struct drm_mm *mm,
 EXPORT_SYMBOL(drm_mm_init_scan_with_range);
 
 /**
+ * drm_mm_scan_add_block - add a node to the scan list
+ * @node: drm_mm_node to add
+ *
  * Add a node to the scan list that might be freed to make space for the desired
  * hole.
  *
- * Returns non-zero, if a hole has been found, zero otherwise.
+ * Returns:
+ * True if a hole has been found, false otherwise.
  */
-int drm_mm_scan_add_block(struct drm_mm_node *node)
+bool drm_mm_scan_add_block(struct drm_mm_node *node)
 {
 	struct drm_mm *mm = node->mm;
 	struct drm_mm_node *prev_node;
@@ -566,15 +629,16 @@ int drm_mm_scan_add_block(struct drm_mm_node *node)
 			    mm->scan_size, mm->scan_alignment)) {
 		mm->scan_hit_start = hole_start;
 		mm->scan_hit_end = hole_end;
-		return 1;
+		return true;
 	}
 
-	return 0;
+	return false;
 }
 EXPORT_SYMBOL(drm_mm_scan_add_block);
 
 /**
- * Remove a node from the scan list.
+ * drm_mm_scan_remove_block - remove a node from the scan list
+ * @node: drm_mm_node to remove
  *
  * Nodes _must_ be removed in the exact same order from the scan list as they
  * have been added, otherwise the internal state of the memory manager will be
@@ -584,10 +648,11 @@ EXPORT_SYMBOL(drm_mm_scan_add_block);
  * immediately following drm_mm_search_free with !DRM_MM_SEARCH_BEST will then
  * return the just freed block (because its at the top of the free_stack list).
  *
- * Returns one if this block should be evicted, zero otherwise. Will always
- * return zero when no hole has been found.
+ * Returns:
+ * True if this block should be evicted, false otherwise. Will always
+ * return false when no hole has been found.
  */
-int drm_mm_scan_remove_block(struct drm_mm_node *node)
+bool drm_mm_scan_remove_block(struct drm_mm_node *node)
 {
 	struct drm_mm *mm = node->mm;
 	struct drm_mm_node *prev_node;
@@ -608,7 +673,15 @@ int drm_mm_scan_remove_block(struct drm_mm_node *node)
 }
 EXPORT_SYMBOL(drm_mm_scan_remove_block);
 
-int drm_mm_clean(struct drm_mm * mm)
+/**
+ * drm_mm_clean - checks whether an allocator is clean
+ * @mm: drm_mm allocator to check
+ *
+ * Returns:
+ * True if the allocator is completely free, false if there's still a node
+ * allocated in it.
+ */
+bool drm_mm_clean(struct drm_mm * mm)
 {
 	struct list_head *head = &mm->head_node.node_list;
 
@@ -616,6 +689,14 @@ int drm_mm_clean(struct drm_mm * mm)
 }
 EXPORT_SYMBOL(drm_mm_clean);
 
+/**
+ * drm_mm_init - initialize a drm-mm allocator
+ * @mm: the drm_mm structure to initialize
+ * @start: start of the range managed by @mm
+ * @size: end of the range managed by @mm
+ *
+ * Note that @mm must be cleared to 0 before calling this function.
+ */
 void drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
 {
 	INIT_LIST_HEAD(&mm->hole_stack);
@@ -637,6 +718,13 @@ void drm_mm_init(struct drm_mm * mm, unsigned long start, unsigned long size)
 }
 EXPORT_SYMBOL(drm_mm_init);
 
+/**
+ * drm_mm_takedown - clean up a drm_mm allocator
+ * @mm: drm_mm allocator to clean up
+ *
+ * Note that it is a bug to call this function on an allocator which is not
+ * clean.
+ */
 void drm_mm_takedown(struct drm_mm * mm)
 {
 	WARN(!list_empty(&mm->head_node.node_list),
@@ -662,6 +750,11 @@ static unsigned long drm_mm_debug_hole(struct drm_mm_node *entry,
 	return 0;
 }
 
+/**
+ * drm_mm_debug_table - dump allocator state to dmesg
+ * @mm: drm_mm allocator to dump
+ * @prefix: prefix to use for dumping to dmesg
+ */
 void drm_mm_debug_table(struct drm_mm *mm, const char *prefix)
 {
 	struct drm_mm_node *entry;
@@ -700,6 +793,11 @@ static unsigned long drm_mm_dump_hole(struct seq_file *m, struct drm_mm_node *en
 	return 0;
 }
 
+/**
+ * drm_mm_dump_table - dump allocator state to a seq_file
+ * @m: seq_file to dump to
+ * @mm: drm_mm allocator to dump
+ */
 int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm)
 {
 	struct drm_mm_node *entry;
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index cba67865d18f..8b6981ab3fcf 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -85,11 +85,31 @@ struct drm_mm {
 			     unsigned long *start, unsigned long *end);
 };
 
+/**
+ * drm_mm_node_allocated - checks whether a node is allocated
+ * @node: drm_mm_node to check
+ *
+ * Drivers should use this helpers for proper encapusulation of drm_mm
+ * internals.
+ *
+ * Returns:
+ * True if the @node is allocated.
+ */
 static inline bool drm_mm_node_allocated(struct drm_mm_node *node)
 {
 	return node->allocated;
 }
 
+/**
+ * drm_mm_initialized - checks whether an allocator is initialized
+ * @mm: drm_mm to check
+ *
+ * Drivers should use this helpers for proper encapusulation of drm_mm
+ * internals.
+ *
+ * Returns:
+ * True if the @mm is initialized.
+ */
 static inline bool drm_mm_initialized(struct drm_mm *mm)
 {
 	return mm->hole_stack.next;
@@ -100,6 +120,17 @@ static inline unsigned long __drm_mm_hole_node_start(struct drm_mm_node *hole_no
 	return hole_node->start + hole_node->size;
 }
 
+/**
+ * drm_mm_hole_node_start - computes the start of the hole following @node
+ * @hole_node: drm_mm_node which implicitly tracks the following hole
+ *
+ * This is useful for driver-sepific debug dumpers. Otherwise drivers should not
+ * inspect holes themselves. Drivers must check first whether a hole indeed
+ * follows by looking at node->hole_follows.
+ *
+ * Returns:
+ * Start of the subsequent hole.
+ */
 static inline unsigned long drm_mm_hole_node_start(struct drm_mm_node *hole_node)
 {
 	BUG_ON(!hole_node->hole_follows);
@@ -112,18 +143,49 @@ static inline unsigned long __drm_mm_hole_node_end(struct drm_mm_node *hole_node
 			  struct drm_mm_node, node_list)->start;
 }
 
+/**
+ * drm_mm_hole_node_end - computes the end of the hole following @node
+ * @hole_node: drm_mm_node which implicitly tracks the following hole
+ *
+ * This is useful for driver-sepific debug dumpers. Otherwise drivers should not
+ * inspect holes themselves. Drivers must check first whether a hole indeed
+ * follows by looking at node->hole_follows.
+ *
+ * Returns:
+ * End of the subsequent hole.
+ */
 static inline unsigned long drm_mm_hole_node_end(struct drm_mm_node *hole_node)
 {
 	return __drm_mm_hole_node_end(hole_node);
 }
 
+/**
+ * drm_mm_for_each_node - iterator to walk over all allocated nodes
+ * @entry: drm_mm_node structure to assign to in each iteration step
+ * @mm: drm_mm allocator to walk
+ *
+ * This iterator walks over all nodes in the range allocator. It is implemented
+ * with list_for_each, so not save against removal of elements.
+ */
 #define drm_mm_for_each_node(entry, mm) list_for_each_entry(entry, \
 						&(mm)->head_node.node_list, \
 						node_list)
 
-/* Note that we need to unroll list_for_each_entry in order to inline
- * setting hole_start and hole_end on each iteration and keep the
- * macro sane.
+/**
+ * drm_mm_for_each_hole - iterator to walk over all holes
+ * @entry: drm_mm_node used internally to track progress
+ * @mm: drm_mm allocator to walk
+ * @hole_start: ulong variable to assign the hole start to on each iteration
+ * @hole_end: ulong variable to assign the hole end to on each iteration
+ *
+ * This iterator walks over all holes in the range allocator. It is implemented
+ * with list_for_each, so not save against removal of elements. @entry is used
+ * internally and will not reflect a real drm_mm_node for the very first hole.
+ * Hence users of this iterator may not access it.
+ *
+ * Implementation Note:
+ * We need to inline list_for_each_entry in order to be able to set hole_start
+ * and hole_end on each iteration while keeping the macro sane.
  */
 #define drm_mm_for_each_hole(entry, mm, hole_start, hole_end) \
 	for (entry = list_entry((mm)->hole_stack.next, struct drm_mm_node, hole_stack); \
@@ -136,14 +198,30 @@ static inline unsigned long drm_mm_hole_node_end(struct drm_mm_node *hole_node)
 /*
  * Basic range manager support (drm_mm.c)
  */
-extern int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node);
-
-extern int drm_mm_insert_node_generic(struct drm_mm *mm,
-				      struct drm_mm_node *node,
-				      unsigned long size,
-				      unsigned alignment,
-				      unsigned long color,
-				      enum drm_mm_search_flags flags);
+int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node);
+
+int drm_mm_insert_node_generic(struct drm_mm *mm,
+			       struct drm_mm_node *node,
+			       unsigned long size,
+			       unsigned alignment,
+			       unsigned long color,
+			       enum drm_mm_search_flags flags);
+/**
+ * drm_mm_insert_node - search for space and insert @node
+ * @mm: drm_mm to allocate from
+ * @node: preallocate node to insert
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @flags: flags to fine-tune the allocation
+ *
+ * This is a simplified version of drm_mm_insert_node_generic() with @color set
+ * to 0.
+ *
+ * The preallocated node must be cleared to 0.
+ *
+ * Returns:
+ * 0 on success, -ENOSPC if there's no suitable hole.
+ */
 static inline int drm_mm_insert_node(struct drm_mm *mm,
 				     struct drm_mm_node *node,
 				     unsigned long size,
@@ -153,14 +231,32 @@ static inline int drm_mm_insert_node(struct drm_mm *mm,
 	return drm_mm_insert_node_generic(mm, node, size, alignment, 0, flags);
 }
 
-extern int drm_mm_insert_node_in_range_generic(struct drm_mm *mm,
-				       struct drm_mm_node *node,
-				       unsigned long size,
-				       unsigned alignment,
-				       unsigned long color,
-				       unsigned long start,
-				       unsigned long end,
-				       enum drm_mm_search_flags flags);
+int drm_mm_insert_node_in_range_generic(struct drm_mm *mm,
+					struct drm_mm_node *node,
+					unsigned long size,
+					unsigned alignment,
+					unsigned long color,
+					unsigned long start,
+					unsigned long end,
+					enum drm_mm_search_flags flags);
+/**
+ * drm_mm_insert_node_in_range - ranged search for space and insert @node
+ * @mm: drm_mm to allocate from
+ * @node: preallocate node to insert
+ * @size: size of the allocation
+ * @alignment: alignment of the allocation
+ * @start: start of the allowed range for this node
+ * @end: end of the allowed range for this node
+ * @flags: flags to fine-tune the allocation
+ *
+ * This is a simplified version of drm_mm_insert_node_in_range_generic() with
+ * @color set to 0.
+ *
+ * The preallocated node must be cleared to 0.
+ *
+ * Returns:
+ * 0 on success, -ENOSPC if there's no suitable hole.
+ */
 static inline int drm_mm_insert_node_in_range(struct drm_mm *mm,
 					      struct drm_mm_node *node,
 					      unsigned long size,
@@ -173,13 +269,13 @@ static inline int drm_mm_insert_node_in_range(struct drm_mm *mm,
 						   0, start, end, flags);
 }
 
-extern void drm_mm_remove_node(struct drm_mm_node *node);
-extern void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new);
-extern void drm_mm_init(struct drm_mm *mm,
-			unsigned long start,
-			unsigned long size);
-extern void drm_mm_takedown(struct drm_mm *mm);
-extern int drm_mm_clean(struct drm_mm *mm);
+void drm_mm_remove_node(struct drm_mm_node *node);
+void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new);
+void drm_mm_init(struct drm_mm *mm,
+		 unsigned long start,
+		 unsigned long size);
+void drm_mm_takedown(struct drm_mm *mm);
+bool drm_mm_clean(struct drm_mm *mm);
 
 void drm_mm_init_scan(struct drm_mm *mm,
 		      unsigned long size,
@@ -191,10 +287,10 @@ void drm_mm_init_scan_with_range(struct drm_mm *mm,
 				 unsigned long color,
 				 unsigned long start,
 				 unsigned long end);
-int drm_mm_scan_add_block(struct drm_mm_node *node);
-int drm_mm_scan_remove_block(struct drm_mm_node *node);
+bool drm_mm_scan_add_block(struct drm_mm_node *node);
+bool drm_mm_scan_remove_block(struct drm_mm_node *node);
 
-extern void drm_mm_debug_table(struct drm_mm *mm, const char *prefix);
+void drm_mm_debug_table(struct drm_mm *mm, const char *prefix);
 #ifdef CONFIG_DEBUG_FS
 int drm_mm_dump_table(struct seq_file *m, struct drm_mm *mm);
 #endif
-- 
1.8.5.2

[PATCH 18/19] drm/kms: rip out drm_mode_connector_detach_encoder

[Daniel Vetter]

It's only used by imx, and that one gets it wrong - there's no need
to deteach the encoder before removing it.

And really, neither current drm modesetting code nor all the userspace
we have can handle dynamic changes in the set of possible encoders for
a given connector. So let's just remove this before someone starts
doing something really nasty with it.

As a plus, one less kerneldoc comment to write.

Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c                 | 15 ---------------
 drivers/staging/imx-drm/imx-ldb.c          |  2 --
 drivers/staging/imx-drm/parallel-display.c |  2 --
 include/drm/drm_crtc.h                     |  2 --
 4 files changed, 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 266a01d7f635..cf134540b675 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3460,21 +3460,6 @@ int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_mode_connector_attach_encoder);
 
-void drm_mode_connector_detach_encoder(struct drm_connector *connector,
-				    struct drm_encoder *encoder)
-{
-	int i;
-	for (i = 0; i < DRM_CONNECTOR_MAX_ENCODER; i++) {
-		if (connector->encoder_ids[i] == encoder->base.id) {
-			connector->encoder_ids[i] = 0;
-			if (connector->encoder == encoder)
-				connector->encoder = NULL;
-			break;
-		}
-	}
-}
-EXPORT_SYMBOL(drm_mode_connector_detach_encoder);
-
 int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 				  int gamma_size)
 {
diff --git a/drivers/staging/imx-drm/imx-ldb.c b/drivers/staging/imx-drm/imx-ldb.c
index 654bf03e05ff..16946b7dde96 100644
--- a/drivers/staging/imx-drm/imx-ldb.c
+++ b/drivers/staging/imx-drm/imx-ldb.c
@@ -596,8 +596,6 @@ static int imx_ldb_remove(struct platform_device *pdev)
 		struct drm_connector *connector = &channel->connector;
 		struct drm_encoder *encoder = &channel->encoder;
 
-		drm_mode_connector_detach_encoder(connector, encoder);
-
 		imx_drm_remove_connector(channel->imx_drm_connector);
 		imx_drm_remove_encoder(channel->imx_drm_encoder);
 	}
diff --git a/drivers/staging/imx-drm/parallel-display.c b/drivers/staging/imx-drm/parallel-display.c
index 24aa9beedcfb..e36dd2b24c93 100644
--- a/drivers/staging/imx-drm/parallel-display.c
+++ b/drivers/staging/imx-drm/parallel-display.c
@@ -243,8 +243,6 @@ static int imx_pd_remove(struct platform_device *pdev)
 	struct drm_connector *connector = &imxpd->connector;
 	struct drm_encoder *encoder = &imxpd->encoder;
 
-	drm_mode_connector_detach_encoder(connector, encoder);
-
 	imx_drm_remove_connector(imxpd->imx_drm_connector);
 	imx_drm_remove_encoder(imxpd->imx_drm_encoder);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e51e8975dd6f..68d23acc4ffa 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1052,8 +1052,6 @@ extern const char *drm_get_encoder_name(const struct drm_encoder *encoder);
 
 extern int drm_mode_connector_attach_encoder(struct drm_connector *connector,
 					     struct drm_encoder *encoder);
-extern void drm_mode_connector_detach_encoder(struct drm_connector *connector,
-					   struct drm_encoder *encoder);
 extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
 					 int gamma_size);
 extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
-- 
1.8.5.2

[PATCH 19/19] drm/kms: don't export drm_mode_group_init_legacy_group

[Daniel Vetter]

Driver modules really don't have much business frobbing around with
this: Splitting up modeset resources (if we ever get around to enable
this for real) should be something purely controlled and managed by
the drm core in a driver-agnostic fashion. As usual imx disagrees, but
that's due to the convoluted setup sequence. Russell King is working
on a proper fix for that, so this patch needs to wait for those to
land to avoid breaking imx.

Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_crtc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index cf134540b675..98084413a842 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1264,7 +1264,6 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
 
 	return 0;
 }
-EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
 
 /**
  * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
-- 
1.8.5.2

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
>
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
>
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
>
> Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++++-------------------
>  1 file changed, 68 insertions(+), 57 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..9c3fdd59c995 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -830,62 +830,6 @@ char *date;</synopsis>
>          </para>
>        </sect3>
>        <sect3>
> -        <title>Dumb GEM Objects</title>
> -        <para>
> -          The GEM API doesn't standardize GEM objects creation and leaves it to
> -          driver-specific ioctls. While not an issue for full-fledged graphics
> -          stacks that include device-specific userspace components (in libdrm for
> -          instance), this limit makes DRM-based early boot graphics unnecessarily
> -          complex.
> -        </para>
> -        <para>
> -          Dumb GEM objects partly alleviate the problem by providing a standard
> -          API to create dumb buffers suitable for scanout, which can then be used
> -          to create KMS frame buffers.
> -        </para>
> -        <para>
> -          To support dumb GEM objects drivers must implement the
> -          <methodname>dumb_create</methodname>,
> -          <methodname>dumb_destroy</methodname> and
> -          <methodname>dumb_map_offset</methodname> operations.
> -        </para>
> -        <itemizedlist>
> -          <listitem>
> -            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
> -                     struct drm_mode_create_dumb *args);</synopsis>
> -            <para>
> -              The <methodname>dumb_create</methodname> operation creates a GEM
> -              object suitable for scanout based on the width, height and depth
> -              from the struct <structname>drm_mode_create_dumb</structname>
> -              argument. It fills the argument's <structfield>handle</structfield>,
> -              <structfield>pitch</structfield> and <structfield>size</structfield>
> -              fields with a handle for the newly created GEM object and its line
> -              pitch and size in bytes.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
> -                      uint32_t handle);</synopsis>
> -            <para>
> -              The <methodname>dumb_destroy</methodname> operation destroys a dumb
> -              GEM object created by <methodname>dumb_create</methodname>.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
> -                         uint32_t handle, uint64_t *offset);</synopsis>
> -            <para>
> -              The <methodname>dumb_map_offset</methodname> operation associates an
> -              mmap fake offset with the GEM object given by the handle and returns
> -              it. Drivers must use the
> -              <function>drm_gem_create_mmap_offset</function> function to
> -              associate the fake offset as described in
> -              <xref linkend="drm-gem-objects-mapping"/>.
> -            </para>
> -          </listitem>
> -        </itemizedlist>
> -      </sect3>
> -      <sect3>
>          <title>Memory Coherency</title>
>          <para>
>            When mapped to the device or used in a command buffer, backing pages
> @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
>          handle (or a list of memory handles for multi-planar formats) through
>          the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
>          assumes that the driver uses GEM, those handles thus reference GEM
> -        objects.
> +        objects. But drivers are free to use their own backing storage object
> +       handles, e.g. vmwgfx directly exposes special TTM handles to userspace
> +       and so expects TTM handles in the create ioctl and not GEM objects.

Maybe remove the sentence saying "this document assumes that the
driver uses GEM". I don't see where we explicitly do that. Otherwise
the patch looks fine.

Thanks
David

>        </para>
>        <para>
>          Drivers must first validate the requested frame buffer parameters passed
> @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>         <function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb GEM Objects</title>
> +      <para>
> +       The KMS API doesn't standardize backing storage object creation and
> +       leaves it to driver-specific ioctls. Furthermore actually creating a
> +       buffer object even for GEM-based drivers is done through a
> +       driver-specific ioctl - GEM only has a common userspace interface for
> +       sharing and destroying objects. While not an issue for full-fledged
> +       graphics stacks that include device-specific userspace components (in
> +       libdrm for instance), this limit makes DRM-based early boot graphics
> +       unnecessarily complex.
> +      </para>
> +      <para>
> +        Dumb objects partly alleviate the problem by providing a standard
> +        API to create dumb buffers suitable for scanout, which can then be used
> +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
> +                   struct drm_mode_create_dumb *args);</synopsis>
> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates a driver
> +           object (GEM or TTM handle) object suitable for scanout based on the
> +           width, height and depth from the struct
> +           <structname>drm_mode_create_dumb</structname> argument. It fills the
> +           argument's <structfield>handle</structfield>,
> +           <structfield>pitch</structfield> and <structfield>size</structfield>
> +           fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
> +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation destroys a dumb
> +            object created by <methodname>dumb_create</methodname>.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
> +                       uint32_t handle, uint64_t *offset);</synopsis>
> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation associates an
> +            mmap fake offset with the object given by the handle and returns
> +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration, as has been
> +       attempted on some ARM embedded platforms. Such drivers really must have
> +       a hardware-specific ioctl to allocate suitable objects.
> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis>
>        <para>
> --
> 1.8.5.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/19] drm/doc: Clean up and integrate kerneldoc for drm_gem.c

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Fairly incomplete, but at least a start.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl |  6 +++-
>  drivers/gpu/drm/drm_gem.c      | 63 +++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 64 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 9c3fdd59c995..0660bae6928f 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -868,7 +868,11 @@ char *date;</synopsis>
>            abstracted from the client in libdrm.
>          </para>
>        </sect3>
> -    </sect2>
> +      <sect2>
> +        <title>GEM Function Reference</title>
> +!Edrivers/gpu/drm/drm_gem.c
> +      </sect2>
> +      </sect2>
>    </sect1>
>
>    <!-- Internals: mode setting -->
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 5bbad873c798..2136052ccee1 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -85,9 +85,9 @@
>  #endif
>
>  /**
> - * Initialize the GEM device fields
> + * drm_gem_init - Initialize the GEM device fields
> + * @dev: drm_devic structure to initialize
>   */
> -
>  int
>  drm_gem_init(struct drm_device *dev)
>  {
> @@ -120,6 +120,11 @@ drm_gem_destroy(struct drm_device *dev)
>  }
>
>  /**
> + * drm_gem_object_init - initialize an allocated shmem-backed GEM object
> + * @dev: drm_device the object should be initialized for
> + * @obj: drm_gem_object to initialize
> + * @size: object size
> + *
>   * Initialize an already allocated GEM object of the specified size with
>   * shmfs backing store.
>   */
> @@ -141,6 +146,11 @@ int drm_gem_object_init(struct drm_device *dev,
>  EXPORT_SYMBOL(drm_gem_object_init);
>
>  /**
> + * drm_gem_object_init - initialize an allocated private GEM object
> + * @dev: drm_device the object should be initialized for
> + * @obj: drm_gem_object to initialize
> + * @size: object size
> + *
>   * Initialize an already allocated GEM object of the specified size with
>   * no GEM provided backing store. Instead the caller is responsible for
>   * backing the object and handling it.
> @@ -176,6 +186,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
>  }
>
>  /**
> + * drm_gem_object_free - release resources bound to userspace handles
> + * @obj: GEM object to clean up.
> + *
>   * Called after the last handle to the object has been closed
>   *
>   * Removes any name for the object. Note that this must be
> @@ -225,7 +238,12 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
>  }
>
>  /**
> - * Removes the mapping from handle to filp for this object.
> + * drm_gem_handle_delete - deletes the given file-private handle
> + * @filp: drm file-private structure to use for the handle look up
> + * @handle: userspace handle to delete
> + *
> + * Removes the GEM handle from the @filp lookup table and if this is the last
> + * handle also cleans up linked resources like GEM names.
>   */
>  int
>  drm_gem_handle_delete(struct drm_file *filp, u32 handle)
> @@ -270,6 +288,9 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
>
>  /**
>   * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
> + * @file: drm file-private structure to remove the dumb handle from
> + * @dev: corresponding drm_device
> + * @handle: the dumb handle to remove
>   *
>   * This implements the ->dumb_destroy kms driver callback for drivers which use
>   * gem to manage their backing storage.
> @@ -284,6 +305,9 @@ 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
>   *
>   * 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
> @@ -336,6 +360,11 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>  }
>
>  /**
> + * 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
> + *
>   * Create a handle for this object. This adds a handle reference
>   * to the object, which includes a regular reference count. Callers
>   * will likely want to dereference the object afterwards.
> @@ -536,6 +565,11 @@ drm_gem_object_lookup(struct drm_device *dev, struct drm_file *filp,
>  EXPORT_SYMBOL(drm_gem_object_lookup);
>
>  /**
> + * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
> + * @dev: drm_device
> + * @data: ioctl data
> + * @file_priv: drm file-private structure
> + *
>   * Releases the handle to an mm object.
>   */
>  int
> @@ -554,6 +588,11 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data,
>  }
>
>  /**
> + * drm_gem_flink_ioctl - implementation of the GEM_FLINK ioctl
> + * @dev: drm_device
> + * @data: ioctl data
> + * @file_priv: drm file-private structure
> + *
>   * Create a global name for an object, returning the name.
>   *
>   * Note that the name does not hold a reference; when the object
> @@ -601,6 +640,11 @@ err:
>  }
>
>  /**
> + * drm_gem_open - implementation of the GEM_OPEN ioctl
> + * @dev: drm_device
> + * @data: ioctl data
> + * @file_priv: drm file-private structure
> + *
>   * Open an object using the global name, returning a handle and the size.
>   *
>   * This handle (of course) holds a reference to the object, so the object
> @@ -640,6 +684,10 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
>  }
>
>  /**
> + * gem_gem_open - initalizes GEM file-private structures at devnode open time
> + * @dev: drm_device which is being opened by userspace
> + * @file_private: drm file-private structure to set up
> + *
>   * Called at device open time, sets up the structure for handling refcounting
>   * of mm objects.
>   */
> @@ -650,7 +698,7 @@ drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
>         spin_lock_init(&file_private->table_lock);
>  }
>
> -/**
> +/*
>   * Called at device close to release the file's
>   * handle references on objects.
>   */
> @@ -674,6 +722,10 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
>  }
>
>  /**
> + * drm_gem_release - release file-private GEM resources
> + * @dev: drm_device which is being closed by userspace
> + * @file_private: drm file-private structure to clean up
> + *
>   * Called at close time when the filp is going away.
>   *
>   * Releases any remaining references on objects by this filp.
> @@ -697,6 +749,9 @@ drm_gem_object_release(struct drm_gem_object *obj)
>  EXPORT_SYMBOL(drm_gem_object_release);

drm_gem_object_release() is still missing. Lets at least add a
kernel-doc entry ala:

/**
 * drm_gem_object_release() - destroy GEM object
 * @obj: drm-object to release
 *
 * This is the counter-part to drm_gem_object_init() and
drm_gem_private_object_init() and
 * must be called by a driver in its gem_free_object() callback to
release any gem-internal
 * resources of the GEM-bo.
 */

>
>  /**
> + * drm_gem_object_free - free a GEM object
> + * @kref: kref of the object to free
> + *
>   * Called after the last reference to the object has been lost.
>   * Must be called holding struct_ mutex
>   *

drm_gem_object_free() is internal to drm_gem.c. We only export it
because our object_unreference() helper is "static inline" in the
header. I don't think we should include this in the kernel-doc. No-one
should call this directly (same drm_gem_object_release_handle()).

Thanks
David

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

Re: [PATCH 13/19] drm/doc: Add PRIME function references

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> For giant hilarity the DocBook reference overview is only generated
> when in a level 2 section, not in a level 3 section. So we need to
> move this up a bit as a side-by-side section to the main PRIME
> documentation.
>
> Whatever.

I tried digging through scripts/kernel-doc but.. ugh.. it's perl! No
idea how to fix that. But sect2 seems fine as the whole section is
PRIME-related.

Thanks
David

>
> To have a complete set of references add the missing kerneldoc for all
> functions exported to modules with the exception of the file private
> init/destry functions - drivers have no business calling those, so
> let's just drop the EXPORT_SYMBOL instead.
>
> Also reflow the function parameters to align correctly and break at 80
> chars - my OCD couldn't stand them while writing the kerneldoc ;-)
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl |   6 ++-
>  drivers/gpu/drm/drm_prime.c    | 110 +++++++++++++++++++++++++++++++++--------
>  2 files changed, 94 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 0cc1d85d04de..07abe52b1176 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -898,10 +898,14 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>           </para>
>         </sect3>
>          <sect3>
> -          <title>PRIME Helper Functions Reference</title>
> +          <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>
>    </sect1>
>
>    <!-- Internals: mode setting -->
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 56805c39c906..f1437b6c8dbf 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -68,7 +68,8 @@ struct drm_prime_attachment {
>         enum dma_data_direction dir;
>  };
>
> -static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> +                                   struct dma_buf *dma_buf, uint32_t handle)
>  {
>         struct drm_prime_member *member;
>
> @@ -174,7 +175,7 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
>  }
>
>  static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> -               enum dma_data_direction dir)
> +                                           enum dma_data_direction dir)
>  {
>         struct drm_prime_attachment *prime_attach = attach->priv;
>         struct drm_gem_object *obj = attach->dmabuf->priv;
> @@ -211,11 +212,19 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>  }
>
>  static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> -               struct sg_table *sgt, enum dma_data_direction dir)
> +                                 struct sg_table *sgt,
> +                                 enum dma_data_direction dir)
>  {
>         /* nothing to be done here */
>  }
>
> +/**
> + * drm_gem_dmabuf_release - dma_buf release implementation for GEM
> + * @dma_buf: buffer to be released
> + *
> + * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
> + * must use this in their dma_buf ops structure as the release callback.
> + */
>  void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
>  {
>         struct drm_gem_object *obj = dma_buf->priv;
> @@ -242,30 +251,30 @@ static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>  }
>
>  static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
> -               unsigned long page_num)
> +                                       unsigned long page_num)
>  {
>         return NULL;
>  }
>
>  static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
> -               unsigned long page_num, void *addr)
> +                                        unsigned long page_num, void *addr)
>  {
>
>  }
>  static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
> -               unsigned long page_num)
> +                                unsigned long page_num)
>  {
>         return NULL;
>  }
>
>  static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
> -               unsigned long page_num, void *addr)
> +                                 unsigned long page_num, void *addr)
>  {
>
>  }
>
>  static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
> -               struct vm_area_struct *vma)
> +                              struct vm_area_struct *vma)
>  {
>         struct drm_gem_object *obj = dma_buf->priv;
>         struct drm_device *dev = obj->dev;
> @@ -315,6 +324,15 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>   *    driver's scatter/gather table
>   */
>
> +/**
> + * drm_gem_prime_export - helper library implemention of the export callback
> + * @dev: drm_device to export from
> + * @obj: GEM object to export
> + * @flags: flags like DRM_CLOEXEC
> + *
> + * This is the implementation of the gem_prime_export functions for GEM drivers
> + * using the PRIME helpers.
> + */
>  struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>                                      struct drm_gem_object *obj, int flags)
>  {
> @@ -355,9 +373,23 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>         return dmabuf;
>  }
>
> +/**
> + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> + * @dev: dev to export the buffer from
> + * @file_priv: drm file-private structure
> + * @handle: buffer handle to export
> + * @flags: flags like DRM_CLOEXEC
> + * @prime_fd: pointer to storage for the fd id of the create dma-buf
> + *
> + * This is the PRIME export function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM object.
> + * The actual exporting from GEM object to a dma-buf is done through the
> + * gem_prime_export driver callback.
> + */
>  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> -               struct drm_file *file_priv, uint32_t handle, uint32_t flags,
> -               int *prime_fd)
> +                              struct drm_file *file_priv, uint32_t handle,
> +                              uint32_t flags,
> +                              int *prime_fd)
>  {
>         struct drm_gem_object *obj;
>         int ret = 0;
> @@ -441,6 +473,14 @@ out_unlock:
>  }
>  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>
> +/**
> + * drm_gem_prime_import - helper library implemention of the import callback
> + * @dev: drm_device to import into
> + * @dma_buf: dma-buf object to import
> + *
> + * This is the implementation of the gem_prime_import functions for GEM drivers
> + * using the PRIME helpers.
> + */
>  struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>                                             struct dma_buf *dma_buf)
>  {
> @@ -496,8 +536,21 @@ fail_detach:
>  }
>  EXPORT_SYMBOL(drm_gem_prime_import);
>
> +/**
> + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> + * @dev: dev to export the buffer from
> + * @file_priv: drm file-private structure
> + * @prime_fd: fd id of the dma-buf which should be imported
> + * @handle: pointer to storage for the handle of the imported buffer object
> + *
> + * This is the PRIME import function which must be used mandatorily by GEM
> + * drivers to ensure correct lifetime management of the underlying GEM object.
> + * The actual importing of GEM object from the dma-buf is done through the
> + * gem_import_export driver callback.
> + */
>  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> -               struct drm_file *file_priv, int prime_fd, uint32_t *handle)
> +                              struct drm_file *file_priv, int prime_fd,
> +                              uint32_t *handle)
>  {
>         struct dma_buf *dma_buf;
>         struct drm_gem_object *obj;
> @@ -598,12 +651,14 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>                         args->fd, &args->handle);
>  }
>
> -/*
> - * drm_prime_pages_to_sg
> +/**
> + * drm_prime_pages_to_sg - converts a page array into an sg list
> + * @pages: pointer to the array of page pointers to convert
> + * @nr_pages: length of the page vector
>   *
> - * this helper creates an sg table object from a set of pages
> + * This helper creates an sg table object from a set of pages
>   * the driver is responsible for mapping the pages into the
> - * importers address space
> + * importers address space for use with dma_buf itself.
>   */
>  struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
>  {
> @@ -628,9 +683,16 @@ out:
>  }
>  EXPORT_SYMBOL(drm_prime_pages_to_sg);
>
> -/* export an sg table into an array of pages and addresses
> -   this is currently required by the TTM driver in order to do correct fault
> -   handling */
> +/**
> + * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
> + * @sgt: scatter-gather table to convert
> + * @pages: array of page pointers to store the page array in
> + * @addrs: optional array to store the dma bus address of each page
> + * @max_pages: size of both the passed-in arrays
> + *
> + * Exports an sg table into an array of pages and addresses. This is currently
> + * required by the TTM driver in order to do correct fault handling.
> + */
>  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>                                      dma_addr_t *addrs, int max_pages)
>  {
> @@ -663,7 +725,15 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>         return 0;
>  }
>  EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
> -/* helper function to cleanup a GEM/prime object */
> +
> +/**
> + * drm_prime_gem_destroy - helper to clean up a PRIME-imported GEM object
> + * @obj: GEM object which was created from a dma-buf
> + * @sg: the sg-table which was pinned at import time
> + *
> + * This is the cleanup functions which GEM drivers need to call when they use
> + * @drm_gem_prime_import to import dma-bufs.
> + */
>  void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
>  {
>         struct dma_buf_attachment *attach;
> @@ -683,11 +753,9 @@ void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
>         INIT_LIST_HEAD(&prime_fpriv->head);
>         mutex_init(&prime_fpriv->lock);
>  }
> -EXPORT_SYMBOL(drm_prime_init_file_private);
>
>  void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
>  {
>         /* by now drm_gem_release should've made sure the list is empty */
>         WARN_ON(!list_empty(&prime_fpriv->head));
>  }
> -EXPORT_SYMBOL(drm_prime_destroy_file_private);
> --
> 1.8.5.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/19] drm/doc: Add PRIME function references

[Daniel Vetter]

On Thu, Jan 23, 2014 at 10:28:42AM +0100, David Herrmann wrote:
> Hi
> 
> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > For giant hilarity the DocBook reference overview is only generated
> > when in a level 2 section, not in a level 3 section. So we need to
> > move this up a bit as a side-by-side section to the main PRIME
> > documentation.
> >
> > Whatever.
> 
> I tried digging through scripts/kernel-doc but.. ugh.. it's perl! No
> idea how to fix that. But sect2 seems fine as the whole section is
> PRIME-related.

Well we have two sect2 now: One for the PRIME overview, the other for the
reference documenation. I've done the same split for drm_mm btw. If we
want to fix this I think this is actually in the DocBook stylesheet, not
in the kerneldoc thing. I've checked the intermediate xml and all the
function references are there, they even get generated as html files and
you can xref them from within the docbook. There's just no section topic
for sect3 levels, so you never see a link to that separate page without an
explicit reference.
-Daniel

> 
> Thanks
> David
> 
> >
> > To have a complete set of references add the missing kerneldoc for all
> > functions exported to modules with the exception of the file private
> > init/destry functions - drivers have no business calling those, so
> > let's just drop the EXPORT_SYMBOL instead.
> >
> > Also reflow the function parameters to align correctly and break at 80
> > chars - my OCD couldn't stand them while writing the kerneldoc ;-)
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/DocBook/drm.tmpl |   6 ++-
> >  drivers/gpu/drm/drm_prime.c    | 110 +++++++++++++++++++++++++++++++++--------
> >  2 files changed, 94 insertions(+), 22 deletions(-)
> >
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 0cc1d85d04de..07abe52b1176 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -898,10 +898,14 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> >           </para>
> >         </sect3>
> >          <sect3>
> > -          <title>PRIME Helper Functions Reference</title>
> > +          <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>
> >    </sect1>
> >
> >    <!-- Internals: mode setting -->
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 56805c39c906..f1437b6c8dbf 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -68,7 +68,8 @@ struct drm_prime_attachment {
> >         enum dma_data_direction dir;
> >  };
> >
> > -static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> > +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> > +                                   struct dma_buf *dma_buf, uint32_t handle)
> >  {
> >         struct drm_prime_member *member;
> >
> > @@ -174,7 +175,7 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
> >  }
> >
> >  static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> > -               enum dma_data_direction dir)
> > +                                           enum dma_data_direction dir)
> >  {
> >         struct drm_prime_attachment *prime_attach = attach->priv;
> >         struct drm_gem_object *obj = attach->dmabuf->priv;
> > @@ -211,11 +212,19 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> >  }
> >
> >  static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> > -               struct sg_table *sgt, enum dma_data_direction dir)
> > +                                 struct sg_table *sgt,
> > +                                 enum dma_data_direction dir)
> >  {
> >         /* nothing to be done here */
> >  }
> >
> > +/**
> > + * drm_gem_dmabuf_release - dma_buf release implementation for GEM
> > + * @dma_buf: buffer to be released
> > + *
> > + * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
> > + * must use this in their dma_buf ops structure as the release callback.
> > + */
> >  void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
> >  {
> >         struct drm_gem_object *obj = dma_buf->priv;
> > @@ -242,30 +251,30 @@ static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> >  }
> >
> >  static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
> > -               unsigned long page_num)
> > +                                       unsigned long page_num)
> >  {
> >         return NULL;
> >  }
> >
> >  static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
> > -               unsigned long page_num, void *addr)
> > +                                        unsigned long page_num, void *addr)
> >  {
> >
> >  }
> >  static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
> > -               unsigned long page_num)
> > +                                unsigned long page_num)
> >  {
> >         return NULL;
> >  }
> >
> >  static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
> > -               unsigned long page_num, void *addr)
> > +                                 unsigned long page_num, void *addr)
> >  {
> >
> >  }
> >
> >  static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
> > -               struct vm_area_struct *vma)
> > +                              struct vm_area_struct *vma)
> >  {
> >         struct drm_gem_object *obj = dma_buf->priv;
> >         struct drm_device *dev = obj->dev;
> > @@ -315,6 +324,15 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> >   *    driver's scatter/gather table
> >   */
> >
> > +/**
> > + * drm_gem_prime_export - helper library implemention of the export callback
> > + * @dev: drm_device to export from
> > + * @obj: GEM object to export
> > + * @flags: flags like DRM_CLOEXEC
> > + *
> > + * This is the implementation of the gem_prime_export functions for GEM drivers
> > + * using the PRIME helpers.
> > + */
> >  struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> >                                      struct drm_gem_object *obj, int flags)
> >  {
> > @@ -355,9 +373,23 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> >         return dmabuf;
> >  }
> >
> > +/**
> > + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> > + * @dev: dev to export the buffer from
> > + * @file_priv: drm file-private structure
> > + * @handle: buffer handle to export
> > + * @flags: flags like DRM_CLOEXEC
> > + * @prime_fd: pointer to storage for the fd id of the create dma-buf
> > + *
> > + * This is the PRIME export function which must be used mandatorily by GEM
> > + * drivers to ensure correct lifetime management of the underlying GEM object.
> > + * The actual exporting from GEM object to a dma-buf is done through the
> > + * gem_prime_export driver callback.
> > + */
> >  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> > -               struct drm_file *file_priv, uint32_t handle, uint32_t flags,
> > -               int *prime_fd)
> > +                              struct drm_file *file_priv, uint32_t handle,
> > +                              uint32_t flags,
> > +                              int *prime_fd)
> >  {
> >         struct drm_gem_object *obj;
> >         int ret = 0;
> > @@ -441,6 +473,14 @@ out_unlock:
> >  }
> >  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
> >
> > +/**
> > + * drm_gem_prime_import - helper library implemention of the import callback
> > + * @dev: drm_device to import into
> > + * @dma_buf: dma-buf object to import
> > + *
> > + * This is the implementation of the gem_prime_import functions for GEM drivers
> > + * using the PRIME helpers.
> > + */
> >  struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> >                                             struct dma_buf *dma_buf)
> >  {
> > @@ -496,8 +536,21 @@ fail_detach:
> >  }
> >  EXPORT_SYMBOL(drm_gem_prime_import);
> >
> > +/**
> > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> > + * @dev: dev to export the buffer from
> > + * @file_priv: drm file-private structure
> > + * @prime_fd: fd id of the dma-buf which should be imported
> > + * @handle: pointer to storage for the handle of the imported buffer object
> > + *
> > + * This is the PRIME import function which must be used mandatorily by GEM
> > + * drivers to ensure correct lifetime management of the underlying GEM object.
> > + * The actual importing of GEM object from the dma-buf is done through the
> > + * gem_import_export driver callback.
> > + */
> >  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> > -               struct drm_file *file_priv, int prime_fd, uint32_t *handle)
> > +                              struct drm_file *file_priv, int prime_fd,
> > +                              uint32_t *handle)
> >  {
> >         struct dma_buf *dma_buf;
> >         struct drm_gem_object *obj;
> > @@ -598,12 +651,14 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> >                         args->fd, &args->handle);
> >  }
> >
> > -/*
> > - * drm_prime_pages_to_sg
> > +/**
> > + * drm_prime_pages_to_sg - converts a page array into an sg list
> > + * @pages: pointer to the array of page pointers to convert
> > + * @nr_pages: length of the page vector
> >   *
> > - * this helper creates an sg table object from a set of pages
> > + * This helper creates an sg table object from a set of pages
> >   * the driver is responsible for mapping the pages into the
> > - * importers address space
> > + * importers address space for use with dma_buf itself.
> >   */
> >  struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
> >  {
> > @@ -628,9 +683,16 @@ out:
> >  }
> >  EXPORT_SYMBOL(drm_prime_pages_to_sg);
> >
> > -/* export an sg table into an array of pages and addresses
> > -   this is currently required by the TTM driver in order to do correct fault
> > -   handling */
> > +/**
> > + * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
> > + * @sgt: scatter-gather table to convert
> > + * @pages: array of page pointers to store the page array in
> > + * @addrs: optional array to store the dma bus address of each page
> > + * @max_pages: size of both the passed-in arrays
> > + *
> > + * Exports an sg table into an array of pages and addresses. This is currently
> > + * required by the TTM driver in order to do correct fault handling.
> > + */
> >  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> >                                      dma_addr_t *addrs, int max_pages)
> >  {
> > @@ -663,7 +725,15 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> >         return 0;
> >  }
> >  EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
> > -/* helper function to cleanup a GEM/prime object */
> > +
> > +/**
> > + * drm_prime_gem_destroy - helper to clean up a PRIME-imported GEM object
> > + * @obj: GEM object which was created from a dma-buf
> > + * @sg: the sg-table which was pinned at import time
> > + *
> > + * This is the cleanup functions which GEM drivers need to call when they use
> > + * @drm_gem_prime_import to import dma-bufs.
> > + */
> >  void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
> >  {
> >         struct dma_buf_attachment *attach;
> > @@ -683,11 +753,9 @@ void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
> >         INIT_LIST_HEAD(&prime_fpriv->head);
> >         mutex_init(&prime_fpriv->lock);
> >  }
> > -EXPORT_SYMBOL(drm_prime_init_file_private);
> >
> >  void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
> >  {
> >         /* by now drm_gem_release should've made sure the list is empty */
> >         WARN_ON(!list_empty(&prime_fpriv->head));
> >  }
> > -EXPORT_SYMBOL(drm_prime_destroy_file_private);
> > --
> > 1.8.5.2
> >
> > _______________________________________________
> > 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

Re: [PATCH 03/19] drm/doc: Clean up and integrate kerneldoc for drm_gem.c

[Daniel Vetter]

On Thu, Jan 23, 2014 at 10:21:33AM +0100, David Herrmann wrote:
> Hi
> 
> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Fairly incomplete, but at least a start.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/DocBook/drm.tmpl |  6 +++-
> >  drivers/gpu/drm/drm_gem.c      | 63 +++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 64 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index 9c3fdd59c995..0660bae6928f 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -868,7 +868,11 @@ char *date;</synopsis>
> >            abstracted from the client in libdrm.
> >          </para>
> >        </sect3>
> > -    </sect2>
> > +      <sect2>
> > +        <title>GEM Function Reference</title>
> > +!Edrivers/gpu/drm/drm_gem.c
> > +      </sect2>
> > +      </sect2>
> >    </sect1>
> >
> >    <!-- Internals: mode setting -->
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 5bbad873c798..2136052ccee1 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -85,9 +85,9 @@
> >  #endif
> >
> >  /**
> > - * Initialize the GEM device fields
> > + * drm_gem_init - Initialize the GEM device fields
> > + * @dev: drm_devic structure to initialize
> >   */
> > -
> >  int
> >  drm_gem_init(struct drm_device *dev)
> >  {
> > @@ -120,6 +120,11 @@ drm_gem_destroy(struct drm_device *dev)
> >  }
> >
> >  /**
> > + * drm_gem_object_init - initialize an allocated shmem-backed GEM object
> > + * @dev: drm_device the object should be initialized for
> > + * @obj: drm_gem_object to initialize
> > + * @size: object size
> > + *
> >   * Initialize an already allocated GEM object of the specified size with
> >   * shmfs backing store.
> >   */
> > @@ -141,6 +146,11 @@ int drm_gem_object_init(struct drm_device *dev,
> >  EXPORT_SYMBOL(drm_gem_object_init);
> >
> >  /**
> > + * drm_gem_object_init - initialize an allocated private GEM object
> > + * @dev: drm_device the object should be initialized for
> > + * @obj: drm_gem_object to initialize
> > + * @size: object size
> > + *
> >   * Initialize an already allocated GEM object of the specified size with
> >   * no GEM provided backing store. Instead the caller is responsible for
> >   * backing the object and handling it.
> > @@ -176,6 +186,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp)
> >  }
> >
> >  /**
> > + * drm_gem_object_free - release resources bound to userspace handles
> > + * @obj: GEM object to clean up.
> > + *
> >   * Called after the last handle to the object has been closed
> >   *
> >   * Removes any name for the object. Note that this must be
> > @@ -225,7 +238,12 @@ drm_gem_object_handle_unreference_unlocked(struct drm_gem_object *obj)
> >  }
> >
> >  /**
> > - * Removes the mapping from handle to filp for this object.
> > + * drm_gem_handle_delete - deletes the given file-private handle
> > + * @filp: drm file-private structure to use for the handle look up
> > + * @handle: userspace handle to delete
> > + *
> > + * Removes the GEM handle from the @filp lookup table and if this is the last
> > + * handle also cleans up linked resources like GEM names.
> >   */
> >  int
> >  drm_gem_handle_delete(struct drm_file *filp, u32 handle)
> > @@ -270,6 +288,9 @@ EXPORT_SYMBOL(drm_gem_handle_delete);
> >
> >  /**
> >   * drm_gem_dumb_destroy - dumb fb callback helper for gem based drivers
> > + * @file: drm file-private structure to remove the dumb handle from
> > + * @dev: corresponding drm_device
> > + * @handle: the dumb handle to remove
> >   *
> >   * This implements the ->dumb_destroy kms driver callback for drivers which use
> >   * gem to manage their backing storage.
> > @@ -284,6 +305,9 @@ 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
> >   *
> >   * 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
> > @@ -336,6 +360,11 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> >  }
> >
> >  /**
> > + * 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
> > + *
> >   * Create a handle for this object. This adds a handle reference
> >   * to the object, which includes a regular reference count. Callers
> >   * will likely want to dereference the object afterwards.
> > @@ -536,6 +565,11 @@ drm_gem_object_lookup(struct drm_device *dev, struct drm_file *filp,
> >  EXPORT_SYMBOL(drm_gem_object_lookup);
> >
> >  /**
> > + * drm_gem_close_ioctl - implementation of the GEM_CLOSE ioctl
> > + * @dev: drm_device
> > + * @data: ioctl data
> > + * @file_priv: drm file-private structure
> > + *
> >   * Releases the handle to an mm object.
> >   */
> >  int
> > @@ -554,6 +588,11 @@ drm_gem_close_ioctl(struct drm_device *dev, void *data,
> >  }
> >
> >  /**
> > + * drm_gem_flink_ioctl - implementation of the GEM_FLINK ioctl
> > + * @dev: drm_device
> > + * @data: ioctl data
> > + * @file_priv: drm file-private structure
> > + *
> >   * Create a global name for an object, returning the name.
> >   *
> >   * Note that the name does not hold a reference; when the object
> > @@ -601,6 +640,11 @@ err:
> >  }
> >
> >  /**
> > + * drm_gem_open - implementation of the GEM_OPEN ioctl
> > + * @dev: drm_device
> > + * @data: ioctl data
> > + * @file_priv: drm file-private structure
> > + *
> >   * Open an object using the global name, returning a handle and the size.
> >   *
> >   * This handle (of course) holds a reference to the object, so the object
> > @@ -640,6 +684,10 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data,
> >  }
> >
> >  /**
> > + * gem_gem_open - initalizes GEM file-private structures at devnode open time
> > + * @dev: drm_device which is being opened by userspace
> > + * @file_private: drm file-private structure to set up
> > + *
> >   * Called at device open time, sets up the structure for handling refcounting
> >   * of mm objects.
> >   */
> > @@ -650,7 +698,7 @@ drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
> >         spin_lock_init(&file_private->table_lock);
> >  }
> >
> > -/**
> > +/*
> >   * Called at device close to release the file's
> >   * handle references on objects.
> >   */
> > @@ -674,6 +722,10 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
> >  }
> >
> >  /**
> > + * drm_gem_release - release file-private GEM resources
> > + * @dev: drm_device which is being closed by userspace
> > + * @file_private: drm file-private structure to clean up
> > + *
> >   * Called at close time when the filp is going away.
> >   *
> >   * Releases any remaining references on objects by this filp.
> > @@ -697,6 +749,9 @@ drm_gem_object_release(struct drm_gem_object *obj)
> >  EXPORT_SYMBOL(drm_gem_object_release);
> 
> drm_gem_object_release() is still missing. Lets at least add a
> kernel-doc entry ala:
> 
> /**
>  * drm_gem_object_release() - destroy GEM object
>  * @obj: drm-object to release
>  *
>  * This is the counter-part to drm_gem_object_init() and
> drm_gem_private_object_init() and
>  * must be called by a driver in its gem_free_object() callback to
> release any gem-internal
>  * resources of the GEM-bo.
>  */
> 
> >
> >  /**
> > + * drm_gem_object_free - free a GEM object
> > + * @kref: kref of the object to free
> > + *
> >   * Called after the last reference to the object has been lost.
> >   * Must be called holding struct_ mutex
> >   *
> 
> drm_gem_object_free() is internal to drm_gem.c. We only export it
> because our object_unreference() helper is "static inline" in the
> header. I don't think we should include this in the kernel-doc. No-one
> should call this directly (same drm_gem_object_release_handle()).

I think I'll postpone this to a real patch to clean up drm_gem kerneldoc.
Essentially this patch here just shuts up kerneldoc warnings about
mismatched kerneldoc compared to the actual function definition. But I'll
keep your suggestions in mind.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 19/19] drm/kms: don't export drm_mode_group_init_legacy_group

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Driver modules really don't have much business frobbing around with
> this: Splitting up modeset resources (if we ever get around to enable
> this for real) should be something purely controlled and managed by
> the drm core in a driver-agnostic fashion. As usual imx disagrees, but
> that's due to the convoluted setup sequence. Russell King is working
> on a proper fix for that, so this patch needs to wait for those to
> land to avoid breaking imx.

I tried working on "modeset-object splitting" as part of the
DRM-Master rework, but that's all really racy and we'd break current
user-space. For instance the object-indices (compared to object-IDs)
need to be constant, but with our current object-lists, removing an
object will change indices (thus breaking possible_crtcs/encoders).

There are ways to make that work, but it'd require huge drm_crtc.c
changes. And I also think our locking doesn't provide for that.
Furthermore, resource-retrieval is not atomic (we need multiple
ioctls: GetResources, GetCrtc, ...) so if we change the objects in
between, we may break running apps in a subtle way.

So I'm all for making object-lists immutable after drm_dev_register()
was called. Imo, hardware should wait for all sub-devices to be
present and then call drm_dev_register(). Once the first sub-device is
removed, you call drm_dev_unregister(). And I think that's what
Russel's compound-device infrastructure does by hiding the sub-devices
in a compound device.

If there's ever hardware that truly supports sub-device hotplugging,
we can support that by adding a DRM-ClientCap like 3D modes. But as I
understand, the current hardware is still a single piece of hardware
which just might get probed via different buses and thus is not an
atomic entity. So the device is still hotplugged as a whole, we just
don't get the events through a single path.

Thanks
David

>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_crtc.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index cf134540b675..98084413a842 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1264,7 +1264,6 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
>
>         return 0;
>  }
> -EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
>
>  /**
>   * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
> --
> 1.8.5.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 13/19] drm/doc: Add PRIME function references

[David Herrmann]

Hi

On Thu, Jan 23, 2014 at 10:37 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jan 23, 2014 at 10:28:42AM +0100, David Herrmann wrote:
>> Hi
>>
>> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > For giant hilarity the DocBook reference overview is only generated
>> > when in a level 2 section, not in a level 3 section. So we need to
>> > move this up a bit as a side-by-side section to the main PRIME
>> > documentation.
>> >
>> > Whatever.
>>
>> I tried digging through scripts/kernel-doc but.. ugh.. it's perl! No
>> idea how to fix that. But sect2 seems fine as the whole section is
>> PRIME-related.
>
> Well we have two sect2 now: One for the PRIME overview, the other for the
> reference documenation. I've done the same split for drm_mm btw. If we
> want to fix this I think this is actually in the DocBook stylesheet, not
> in the kerneldoc thing. I've checked the intermediate xml and all the
> function references are there, they even get generated as html files and
> you can xref them from within the docbook. There's just no section topic
> for sect3 levels, so you never see a link to that separate page without an
> explicit reference.

Hm, why not this:

         </sect3>
+       <title>PRIME Function References</title>
+!Edrivers/gpu/drm/drm_prime.c
+      </sect2>

So you just put it at the end of the prime-sect2?

The kernel-doc script at least has "<sect2>" hardcoded, but yeah, no
idea where to fix that. So I think this is fine.

Thanks
David

>
>>
>> Thanks
>> David
>>
>> >
>> > To have a complete set of references add the missing kerneldoc for all
>> > functions exported to modules with the exception of the file private
>> > init/destry functions - drivers have no business calling those, so
>> > let's just drop the EXPORT_SYMBOL instead.
>> >
>> > Also reflow the function parameters to align correctly and break at 80
>> > chars - my OCD couldn't stand them while writing the kerneldoc ;-)
>> >
>> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > ---
>> >  Documentation/DocBook/drm.tmpl |   6 ++-
>> >  drivers/gpu/drm/drm_prime.c    | 110 +++++++++++++++++++++++++++++++++--------
>> >  2 files changed, 94 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
>> > index 0cc1d85d04de..07abe52b1176 100644
>> > --- a/Documentation/DocBook/drm.tmpl
>> > +++ b/Documentation/DocBook/drm.tmpl
>> > @@ -898,10 +898,14 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
>> >           </para>
>> >         </sect3>
>> >          <sect3>
>> > -          <title>PRIME Helper Functions Reference</title>
>> > +          <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>
>> >    </sect1>
>> >
>> >    <!-- Internals: mode setting -->
>> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> > index 56805c39c906..f1437b6c8dbf 100644
>> > --- a/drivers/gpu/drm/drm_prime.c
>> > +++ b/drivers/gpu/drm/drm_prime.c
>> > @@ -68,7 +68,8 @@ struct drm_prime_attachment {
>> >         enum dma_data_direction dir;
>> >  };
>> >
>> > -static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
>> > +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
>> > +                                   struct dma_buf *dma_buf, uint32_t handle)
>> >  {
>> >         struct drm_prime_member *member;
>> >
>> > @@ -174,7 +175,7 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
>> >  }
>> >
>> >  static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>> > -               enum dma_data_direction dir)
>> > +                                           enum dma_data_direction dir)
>> >  {
>> >         struct drm_prime_attachment *prime_attach = attach->priv;
>> >         struct drm_gem_object *obj = attach->dmabuf->priv;
>> > @@ -211,11 +212,19 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
>> >  }
>> >
>> >  static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
>> > -               struct sg_table *sgt, enum dma_data_direction dir)
>> > +                                 struct sg_table *sgt,
>> > +                                 enum dma_data_direction dir)
>> >  {
>> >         /* nothing to be done here */
>> >  }
>> >
>> > +/**
>> > + * drm_gem_dmabuf_release - dma_buf release implementation for GEM
>> > + * @dma_buf: buffer to be released
>> > + *
>> > + * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
>> > + * must use this in their dma_buf ops structure as the release callback.
>> > + */
>> >  void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
>> >  {
>> >         struct drm_gem_object *obj = dma_buf->priv;
>> > @@ -242,30 +251,30 @@ static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>> >  }
>> >
>> >  static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
>> > -               unsigned long page_num)
>> > +                                       unsigned long page_num)
>> >  {
>> >         return NULL;
>> >  }
>> >
>> >  static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
>> > -               unsigned long page_num, void *addr)
>> > +                                        unsigned long page_num, void *addr)
>> >  {
>> >
>> >  }
>> >  static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
>> > -               unsigned long page_num)
>> > +                                unsigned long page_num)
>> >  {
>> >         return NULL;
>> >  }
>> >
>> >  static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
>> > -               unsigned long page_num, void *addr)
>> > +                                 unsigned long page_num, void *addr)
>> >  {
>> >
>> >  }
>> >
>> >  static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
>> > -               struct vm_area_struct *vma)
>> > +                              struct vm_area_struct *vma)
>> >  {
>> >         struct drm_gem_object *obj = dma_buf->priv;
>> >         struct drm_device *dev = obj->dev;
>> > @@ -315,6 +324,15 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
>> >   *    driver's scatter/gather table
>> >   */
>> >
>> > +/**
>> > + * drm_gem_prime_export - helper library implemention of the export callback
>> > + * @dev: drm_device to export from
>> > + * @obj: GEM object to export
>> > + * @flags: flags like DRM_CLOEXEC
>> > + *
>> > + * This is the implementation of the gem_prime_export functions for GEM drivers
>> > + * using the PRIME helpers.
>> > + */
>> >  struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
>> >                                      struct drm_gem_object *obj, int flags)
>> >  {
>> > @@ -355,9 +373,23 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>> >         return dmabuf;
>> >  }
>> >
>> > +/**
>> > + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
>> > + * @dev: dev to export the buffer from
>> > + * @file_priv: drm file-private structure
>> > + * @handle: buffer handle to export
>> > + * @flags: flags like DRM_CLOEXEC
>> > + * @prime_fd: pointer to storage for the fd id of the create dma-buf
>> > + *
>> > + * This is the PRIME export function which must be used mandatorily by GEM
>> > + * drivers to ensure correct lifetime management of the underlying GEM object.
>> > + * The actual exporting from GEM object to a dma-buf is done through the
>> > + * gem_prime_export driver callback.
>> > + */
>> >  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
>> > -               struct drm_file *file_priv, uint32_t handle, uint32_t flags,
>> > -               int *prime_fd)
>> > +                              struct drm_file *file_priv, uint32_t handle,
>> > +                              uint32_t flags,
>> > +                              int *prime_fd)
>> >  {
>> >         struct drm_gem_object *obj;
>> >         int ret = 0;
>> > @@ -441,6 +473,14 @@ out_unlock:
>> >  }
>> >  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
>> >
>> > +/**
>> > + * drm_gem_prime_import - helper library implemention of the import callback
>> > + * @dev: drm_device to import into
>> > + * @dma_buf: dma-buf object to import
>> > + *
>> > + * This is the implementation of the gem_prime_import functions for GEM drivers
>> > + * using the PRIME helpers.
>> > + */
>> >  struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
>> >                                             struct dma_buf *dma_buf)
>> >  {
>> > @@ -496,8 +536,21 @@ fail_detach:
>> >  }
>> >  EXPORT_SYMBOL(drm_gem_prime_import);
>> >
>> > +/**
>> > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
>> > + * @dev: dev to export the buffer from
>> > + * @file_priv: drm file-private structure
>> > + * @prime_fd: fd id of the dma-buf which should be imported
>> > + * @handle: pointer to storage for the handle of the imported buffer object
>> > + *
>> > + * This is the PRIME import function which must be used mandatorily by GEM
>> > + * drivers to ensure correct lifetime management of the underlying GEM object.
>> > + * The actual importing of GEM object from the dma-buf is done through the
>> > + * gem_import_export driver callback.
>> > + */
>> >  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
>> > -               struct drm_file *file_priv, int prime_fd, uint32_t *handle)
>> > +                              struct drm_file *file_priv, int prime_fd,
>> > +                              uint32_t *handle)
>> >  {
>> >         struct dma_buf *dma_buf;
>> >         struct drm_gem_object *obj;
>> > @@ -598,12 +651,14 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
>> >                         args->fd, &args->handle);
>> >  }
>> >
>> > -/*
>> > - * drm_prime_pages_to_sg
>> > +/**
>> > + * drm_prime_pages_to_sg - converts a page array into an sg list
>> > + * @pages: pointer to the array of page pointers to convert
>> > + * @nr_pages: length of the page vector
>> >   *
>> > - * this helper creates an sg table object from a set of pages
>> > + * This helper creates an sg table object from a set of pages
>> >   * the driver is responsible for mapping the pages into the
>> > - * importers address space
>> > + * importers address space for use with dma_buf itself.
>> >   */
>> >  struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
>> >  {
>> > @@ -628,9 +683,16 @@ out:
>> >  }
>> >  EXPORT_SYMBOL(drm_prime_pages_to_sg);
>> >
>> > -/* export an sg table into an array of pages and addresses
>> > -   this is currently required by the TTM driver in order to do correct fault
>> > -   handling */
>> > +/**
>> > + * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
>> > + * @sgt: scatter-gather table to convert
>> > + * @pages: array of page pointers to store the page array in
>> > + * @addrs: optional array to store the dma bus address of each page
>> > + * @max_pages: size of both the passed-in arrays
>> > + *
>> > + * Exports an sg table into an array of pages and addresses. This is currently
>> > + * required by the TTM driver in order to do correct fault handling.
>> > + */
>> >  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>> >                                      dma_addr_t *addrs, int max_pages)
>> >  {
>> > @@ -663,7 +725,15 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>> >         return 0;
>> >  }
>> >  EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
>> > -/* helper function to cleanup a GEM/prime object */
>> > +
>> > +/**
>> > + * drm_prime_gem_destroy - helper to clean up a PRIME-imported GEM object
>> > + * @obj: GEM object which was created from a dma-buf
>> > + * @sg: the sg-table which was pinned at import time
>> > + *
>> > + * This is the cleanup functions which GEM drivers need to call when they use
>> > + * @drm_gem_prime_import to import dma-bufs.
>> > + */
>> >  void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
>> >  {
>> >         struct dma_buf_attachment *attach;
>> > @@ -683,11 +753,9 @@ void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
>> >         INIT_LIST_HEAD(&prime_fpriv->head);
>> >         mutex_init(&prime_fpriv->lock);
>> >  }
>> > -EXPORT_SYMBOL(drm_prime_init_file_private);
>> >
>> >  void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
>> >  {
>> >         /* by now drm_gem_release should've made sure the list is empty */
>> >         WARN_ON(!list_empty(&prime_fpriv->head));
>> >  }
>> > -EXPORT_SYMBOL(drm_prime_destroy_file_private);
>> > --
>> > 1.8.5.2
>> >
>> > _______________________________________________
>> > 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

Re: [PATCH 13/19] drm/doc: Add PRIME function references

[Daniel Vetter]

On Thu, Jan 23, 2014 at 10:45:09AM +0100, David Herrmann wrote:
> Hi
> 
> On Thu, Jan 23, 2014 at 10:37 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jan 23, 2014 at 10:28:42AM +0100, David Herrmann wrote:
> >> Hi
> >>
> >> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> > For giant hilarity the DocBook reference overview is only generated
> >> > when in a level 2 section, not in a level 3 section. So we need to
> >> > move this up a bit as a side-by-side section to the main PRIME
> >> > documentation.
> >> >
> >> > Whatever.
> >>
> >> I tried digging through scripts/kernel-doc but.. ugh.. it's perl! No
> >> idea how to fix that. But sect2 seems fine as the whole section is
> >> PRIME-related.
> >
> > Well we have two sect2 now: One for the PRIME overview, the other for the
> > reference documenation. I've done the same split for drm_mm btw. If we
> > want to fix this I think this is actually in the DocBook stylesheet, not
> > in the kerneldoc thing. I've checked the intermediate xml and all the
> > function references are there, they even get generated as html files and
> > you can xref them from within the docbook. There's just no section topic
> > for sect3 levels, so you never see a link to that separate page without an
> > explicit reference.
> 
> Hm, why not this:
> 
>          </sect3>
> +       <title>PRIME Function References</title>
> +!Edrivers/gpu/drm/drm_prime.c
> +      </sect2>
> 
> So you just put it at the end of the prime-sect2?
> 
> The kernel-doc script at least has "<sect2>" hardcoded, but yeah, no
> idea where to fix that. So I think this is fine.

The only links generated are for the section toc, which is always at the
top of the section. So if we combine it you'll have a pile of function
references + the overview links for the subsections. Loosk rather bad imo.
-Daniel

> 
> Thanks
> David
> 
> >
> >>
> >> Thanks
> >> David
> >>
> >> >
> >> > To have a complete set of references add the missing kerneldoc for all
> >> > functions exported to modules with the exception of the file private
> >> > init/destry functions - drivers have no business calling those, so
> >> > let's just drop the EXPORT_SYMBOL instead.
> >> >
> >> > Also reflow the function parameters to align correctly and break at 80
> >> > chars - my OCD couldn't stand them while writing the kerneldoc ;-)
> >> >
> >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > ---
> >> >  Documentation/DocBook/drm.tmpl |   6 ++-
> >> >  drivers/gpu/drm/drm_prime.c    | 110 +++++++++++++++++++++++++++++++++--------
> >> >  2 files changed, 94 insertions(+), 22 deletions(-)
> >> >
> >> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> >> > index 0cc1d85d04de..07abe52b1176 100644
> >> > --- a/Documentation/DocBook/drm.tmpl
> >> > +++ b/Documentation/DocBook/drm.tmpl
> >> > @@ -898,10 +898,14 @@ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev,
> >> >           </para>
> >> >         </sect3>
> >> >          <sect3>
> >> > -          <title>PRIME Helper Functions Reference</title>
> >> > +          <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>
> >> >    </sect1>
> >> >
> >> >    <!-- Internals: mode setting -->
> >> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> >> > index 56805c39c906..f1437b6c8dbf 100644
> >> > --- a/drivers/gpu/drm/drm_prime.c
> >> > +++ b/drivers/gpu/drm/drm_prime.c
> >> > @@ -68,7 +68,8 @@ struct drm_prime_attachment {
> >> >         enum dma_data_direction dir;
> >> >  };
> >> >
> >> > -static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv, struct dma_buf *dma_buf, uint32_t handle)
> >> > +static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> >> > +                                   struct dma_buf *dma_buf, uint32_t handle)
> >> >  {
> >> >         struct drm_prime_member *member;
> >> >
> >> > @@ -174,7 +175,7 @@ void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpr
> >> >  }
> >> >
> >> >  static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> >> > -               enum dma_data_direction dir)
> >> > +                                           enum dma_data_direction dir)
> >> >  {
> >> >         struct drm_prime_attachment *prime_attach = attach->priv;
> >> >         struct drm_gem_object *obj = attach->dmabuf->priv;
> >> > @@ -211,11 +212,19 @@ static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
> >> >  }
> >> >
> >> >  static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
> >> > -               struct sg_table *sgt, enum dma_data_direction dir)
> >> > +                                 struct sg_table *sgt,
> >> > +                                 enum dma_data_direction dir)
> >> >  {
> >> >         /* nothing to be done here */
> >> >  }
> >> >
> >> > +/**
> >> > + * drm_gem_dmabuf_release - dma_buf release implementation for GEM
> >> > + * @dma_buf: buffer to be released
> >> > + *
> >> > + * Generic release function for dma_bufs exported as PRIME buffers. GEM drivers
> >> > + * must use this in their dma_buf ops structure as the release callback.
> >> > + */
> >> >  void drm_gem_dmabuf_release(struct dma_buf *dma_buf)
> >> >  {
> >> >         struct drm_gem_object *obj = dma_buf->priv;
> >> > @@ -242,30 +251,30 @@ static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> >> >  }
> >> >
> >> >  static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
> >> > -               unsigned long page_num)
> >> > +                                       unsigned long page_num)
> >> >  {
> >> >         return NULL;
> >> >  }
> >> >
> >> >  static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
> >> > -               unsigned long page_num, void *addr)
> >> > +                                        unsigned long page_num, void *addr)
> >> >  {
> >> >
> >> >  }
> >> >  static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
> >> > -               unsigned long page_num)
> >> > +                                unsigned long page_num)
> >> >  {
> >> >         return NULL;
> >> >  }
> >> >
> >> >  static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
> >> > -               unsigned long page_num, void *addr)
> >> > +                                 unsigned long page_num, void *addr)
> >> >  {
> >> >
> >> >  }
> >> >
> >> >  static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
> >> > -               struct vm_area_struct *vma)
> >> > +                              struct vm_area_struct *vma)
> >> >  {
> >> >         struct drm_gem_object *obj = dma_buf->priv;
> >> >         struct drm_device *dev = obj->dev;
> >> > @@ -315,6 +324,15 @@ static const struct dma_buf_ops drm_gem_prime_dmabuf_ops =  {
> >> >   *    driver's scatter/gather table
> >> >   */
> >> >
> >> > +/**
> >> > + * drm_gem_prime_export - helper library implemention of the export callback
> >> > + * @dev: drm_device to export from
> >> > + * @obj: GEM object to export
> >> > + * @flags: flags like DRM_CLOEXEC
> >> > + *
> >> > + * This is the implementation of the gem_prime_export functions for GEM drivers
> >> > + * using the PRIME helpers.
> >> > + */
> >> >  struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
> >> >                                      struct drm_gem_object *obj, int flags)
> >> >  {
> >> > @@ -355,9 +373,23 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
> >> >         return dmabuf;
> >> >  }
> >> >
> >> > +/**
> >> > + * drm_gem_prime_handle_to_fd - PRIME export function for GEM drivers
> >> > + * @dev: dev to export the buffer from
> >> > + * @file_priv: drm file-private structure
> >> > + * @handle: buffer handle to export
> >> > + * @flags: flags like DRM_CLOEXEC
> >> > + * @prime_fd: pointer to storage for the fd id of the create dma-buf
> >> > + *
> >> > + * This is the PRIME export function which must be used mandatorily by GEM
> >> > + * drivers to ensure correct lifetime management of the underlying GEM object.
> >> > + * The actual exporting from GEM object to a dma-buf is done through the
> >> > + * gem_prime_export driver callback.
> >> > + */
> >> >  int drm_gem_prime_handle_to_fd(struct drm_device *dev,
> >> > -               struct drm_file *file_priv, uint32_t handle, uint32_t flags,
> >> > -               int *prime_fd)
> >> > +                              struct drm_file *file_priv, uint32_t handle,
> >> > +                              uint32_t flags,
> >> > +                              int *prime_fd)
> >> >  {
> >> >         struct drm_gem_object *obj;
> >> >         int ret = 0;
> >> > @@ -441,6 +473,14 @@ out_unlock:
> >> >  }
> >> >  EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
> >> >
> >> > +/**
> >> > + * drm_gem_prime_import - helper library implemention of the import callback
> >> > + * @dev: drm_device to import into
> >> > + * @dma_buf: dma-buf object to import
> >> > + *
> >> > + * This is the implementation of the gem_prime_import functions for GEM drivers
> >> > + * using the PRIME helpers.
> >> > + */
> >> >  struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
> >> >                                             struct dma_buf *dma_buf)
> >> >  {
> >> > @@ -496,8 +536,21 @@ fail_detach:
> >> >  }
> >> >  EXPORT_SYMBOL(drm_gem_prime_import);
> >> >
> >> > +/**
> >> > + * drm_gem_prime_fd_to_handle - PRIME import function for GEM drivers
> >> > + * @dev: dev to export the buffer from
> >> > + * @file_priv: drm file-private structure
> >> > + * @prime_fd: fd id of the dma-buf which should be imported
> >> > + * @handle: pointer to storage for the handle of the imported buffer object
> >> > + *
> >> > + * This is the PRIME import function which must be used mandatorily by GEM
> >> > + * drivers to ensure correct lifetime management of the underlying GEM object.
> >> > + * The actual importing of GEM object from the dma-buf is done through the
> >> > + * gem_import_export driver callback.
> >> > + */
> >> >  int drm_gem_prime_fd_to_handle(struct drm_device *dev,
> >> > -               struct drm_file *file_priv, int prime_fd, uint32_t *handle)
> >> > +                              struct drm_file *file_priv, int prime_fd,
> >> > +                              uint32_t *handle)
> >> >  {
> >> >         struct dma_buf *dma_buf;
> >> >         struct drm_gem_object *obj;
> >> > @@ -598,12 +651,14 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data,
> >> >                         args->fd, &args->handle);
> >> >  }
> >> >
> >> > -/*
> >> > - * drm_prime_pages_to_sg
> >> > +/**
> >> > + * drm_prime_pages_to_sg - converts a page array into an sg list
> >> > + * @pages: pointer to the array of page pointers to convert
> >> > + * @nr_pages: length of the page vector
> >> >   *
> >> > - * this helper creates an sg table object from a set of pages
> >> > + * This helper creates an sg table object from a set of pages
> >> >   * the driver is responsible for mapping the pages into the
> >> > - * importers address space
> >> > + * importers address space for use with dma_buf itself.
> >> >   */
> >> >  struct sg_table *drm_prime_pages_to_sg(struct page **pages, int nr_pages)
> >> >  {
> >> > @@ -628,9 +683,16 @@ out:
> >> >  }
> >> >  EXPORT_SYMBOL(drm_prime_pages_to_sg);
> >> >
> >> > -/* export an sg table into an array of pages and addresses
> >> > -   this is currently required by the TTM driver in order to do correct fault
> >> > -   handling */
> >> > +/**
> >> > + * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
> >> > + * @sgt: scatter-gather table to convert
> >> > + * @pages: array of page pointers to store the page array in
> >> > + * @addrs: optional array to store the dma bus address of each page
> >> > + * @max_pages: size of both the passed-in arrays
> >> > + *
> >> > + * Exports an sg table into an array of pages and addresses. This is currently
> >> > + * required by the TTM driver in order to do correct fault handling.
> >> > + */
> >> >  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> >> >                                      dma_addr_t *addrs, int max_pages)
> >> >  {
> >> > @@ -663,7 +725,15 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> >> >         return 0;
> >> >  }
> >> >  EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
> >> > -/* helper function to cleanup a GEM/prime object */
> >> > +
> >> > +/**
> >> > + * drm_prime_gem_destroy - helper to clean up a PRIME-imported GEM object
> >> > + * @obj: GEM object which was created from a dma-buf
> >> > + * @sg: the sg-table which was pinned at import time
> >> > + *
> >> > + * This is the cleanup functions which GEM drivers need to call when they use
> >> > + * @drm_gem_prime_import to import dma-bufs.
> >> > + */
> >> >  void drm_prime_gem_destroy(struct drm_gem_object *obj, struct sg_table *sg)
> >> >  {
> >> >         struct dma_buf_attachment *attach;
> >> > @@ -683,11 +753,9 @@ void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
> >> >         INIT_LIST_HEAD(&prime_fpriv->head);
> >> >         mutex_init(&prime_fpriv->lock);
> >> >  }
> >> > -EXPORT_SYMBOL(drm_prime_init_file_private);
> >> >
> >> >  void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv)
> >> >  {
> >> >         /* by now drm_gem_release should've made sure the list is empty */
> >> >         WARN_ON(!list_empty(&prime_fpriv->head));
> >> >  }
> >> > -EXPORT_SYMBOL(drm_prime_destroy_file_private);
> >> > --
> >> > 1.8.5.2
> >> >
> >> > _______________________________________________
> >> > 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 19/19] drm/kms: don't export drm_mode_group_init_legacy_group

[Daniel Vetter]

On Thu, Jan 23, 2014 at 10:42:02AM +0100, David Herrmann wrote:
> Hi
> 
> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Driver modules really don't have much business frobbing around with
> > this: Splitting up modeset resources (if we ever get around to enable
> > this for real) should be something purely controlled and managed by
> > the drm core in a driver-agnostic fashion. As usual imx disagrees, but
> > that's due to the convoluted setup sequence. Russell King is working
> > on a proper fix for that, so this patch needs to wait for those to
> > land to avoid breaking imx.
> 
> I tried working on "modeset-object splitting" as part of the
> DRM-Master rework, but that's all really racy and we'd break current
> user-space. For instance the object-indices (compared to object-IDs)
> need to be constant, but with our current object-lists, removing an
> object will change indices (thus breaking possible_crtcs/encoders).
> 
> There are ways to make that work, but it'd require huge drm_crtc.c
> changes. And I also think our locking doesn't provide for that.
> Furthermore, resource-retrieval is not atomic (we need multiple
> ioctls: GetResources, GetCrtc, ...) so if we change the objects in
> between, we may break running apps in a subtle way.
> 
> So I'm all for making object-lists immutable after drm_dev_register()
> was called. Imo, hardware should wait for all sub-devices to be
> present and then call drm_dev_register(). Once the first sub-device is
> removed, you call drm_dev_unregister(). And I think that's what
> Russel's compound-device infrastructure does by hiding the sub-devices
> in a compound device.
> 
> If there's ever hardware that truly supports sub-device hotplugging,
> we can support that by adding a DRM-ClientCap like 3D modes. But as I
> understand, the current hardware is still a single piece of hardware
> which just might get probed via different buses and thus is not an
> atomic entity. So the device is still hotplugged as a whole, we just
> don't get the events through a single path.

Yeah, if we ever want to allow hotplugging after driver setup that's
massive work to do it right. Which is why I want to plug this hole here
before someone sneaks in for real ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 19/19] drm/kms: don't export drm_mode_group_init_legacy_group

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 11:00:28AM +0100, Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 10:42:02AM +0100, David Herrmann wrote:
> > If there's ever hardware that truly supports sub-device hotplugging,
> > we can support that by adding a DRM-ClientCap like 3D modes. But as I
> > understand, the current hardware is still a single piece of hardware
> > which just might get probed via different buses and thus is not an
> > atomic entity. So the device is still hotplugged as a whole, we just
> > don't get the events through a single path.
> 
> Yeah, if we ever want to allow hotplugging after driver setup that's
> massive work to do it right. Which is why I want to plug this hole here
> before someone sneaks in for real ;-)

David did say at the kernel summit that he's not interested at all in
hotplugging the components of a DRM system, and his statement appeared
to be very clear in that regard that it was never going to happen.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote:
> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
> 
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
> 
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
> 
> Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++------------------
> 1 file changed, 68 insertions(+), 57 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..9c3fdd59c995 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -830,62 +830,6 @@ char *date;</synopsis>
>          </para>
>        </sect3>
>        <sect3>
> -        <title>Dumb GEM Objects</title>
> -        <para>
> -          The GEM API doesn't standardize GEM objects creation and leaves
> it to -          driver-specific ioctls. While not an issue for
> full-fledged graphics -          stacks that include device-specific
> userspace components (in libdrm for -          instance), this limit makes
> DRM-based early boot graphics unnecessarily -          complex.
> -        </para>
> -        <para>
> -          Dumb GEM objects partly alleviate the problem by providing a
> standard -          API to create dumb buffers suitable for scanout, which
> can then be used -          to create KMS frame buffers.
> -        </para>
> -        <para>
> -          To support dumb GEM objects drivers must implement the
> -          <methodname>dumb_create</methodname>,
> -          <methodname>dumb_destroy</methodname> and
> -          <methodname>dumb_map_offset</methodname> operations.
> -        </para>
> -        <itemizedlist>
> -          <listitem>
> -            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev, -                     struct drm_mode_create_dumb
> *args);</synopsis> -            <para>
> -              The <methodname>dumb_create</methodname> operation creates a
> GEM -              object suitable for scanout based on the width, height
> and depth -              from the struct
> <structname>drm_mode_create_dumb</structname> -              argument. It
> fills the argument's <structfield>handle</structfield>, -             
> <structfield>pitch</structfield> and <structfield>size</structfield> -     
>         fields with a handle for the newly created GEM object and its line
> -              pitch and size in bytes.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv,
> struct drm_device *dev, -                      uint32_t handle);</synopsis>
> -            <para>
> -              The <methodname>dumb_destroy</methodname> operation destroys
> a dumb -              GEM object created by
> <methodname>dumb_create</methodname>. -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev, -                         uint32_t handle, uint64_t
> *offset);</synopsis> -            <para>
> -              The <methodname>dumb_map_offset</methodname> operation
> associates an -              mmap fake offset with the GEM object given by
> the handle and returns -              it. Drivers must use the
> -              <function>drm_gem_create_mmap_offset</function> function to
> -              associate the fake offset as described in
> -              <xref linkend="drm-gem-objects-mapping"/>.
> -            </para>
> -          </listitem>
> -        </itemizedlist>
> -      </sect3>
> -      <sect3>
>          <title>Memory Coherency</title>
>          <para>
>            When mapped to the device or used in a command buffer, backing
> pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
>          handle (or a list of memory handles for multi-planar formats)
> through the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
> assumes that the driver uses GEM, those handles thus reference GEM -       
> objects.
> +        objects. But drivers are free to use their own backing storage
> object
> +	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
> +	and so expects TTM handles in the create ioctl and not GEM objects.

Nitpicking, I would say

"Drivers are however free to use their own backing storage object handles, 
e.g. vmwgfx directly exposes special TTM handles to userspace and so expects 
TTM handles in the create ioctl, not GEM objects."

>        <para>
>          Drivers must first validate the requested frame buffer parameters
> passed @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>  	<function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb GEM Objects</title>
> +      <para>
> +	The KMS API doesn't standardize backing storage object creation and

Strictly speaking isn't it the DRM API that's responsible for memory 
management, not the KMS API ?

> +	leaves it to driver-specific ioctls. Furthermore actually creating a
> +	buffer object even for GEM-based drivers is done through a
> +	driver-specific ioctl - GEM only has a common userspace interface for
> +	sharing and destroying objects. While not an issue for full-fledged
> +	graphics stacks that include device-specific userspace components (in
> +	libdrm for instance), this limit makes DRM-based early boot graphics
> +	unnecessarily complex.
> +      </para>

This feels a bit out of place, can't we leave the section where it was ?

> +      <para>
> +        Dumb objects partly alleviate the problem by providing a standard
> +        API to create dumb buffers suitable for scanout, which can then be
> used +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev, +                   struct drm_mode_create_dumb
> *args);</synopsis> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates a
> driver +	    object (GEM or TTM handle) object suitable for scanout based
> on the +	    width, height and depth from the struct
> +	    <structname>drm_mode_create_dumb</structname> argument. It fills the
> +	    argument's <structfield>handle</structfield>,
> +	    <structfield>pitch</structfield> and <structfield>size</structfield>
> +	    fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct
> drm_device *dev, +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation destroys a
> dumb +            object created by <methodname>dumb_create</methodname>. +
>          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev, +                       uint32_t handle, uint64_t
> *offset);</synopsis> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation
> associates an +            mmap fake offset with the object given by the
> handle and returns +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration, as has
> been +	attempted on some ARM embedded platforms. Such drivers really must
> have +	a hardware-specific ioctl to allocate suitable objects.
> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device
> *dev);</synopsis> <para>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi David,

On Thursday 23 January 2014 10:14:35 David Herrmann wrote:
> On Thu, Jan 23, 2014 at 9:52 AM, Daniel Vetter wrote:
> > - This is _not_ a generic interface to create gem objects, but just an
> >   interface to make early boot services (like boot splash) with a
> >   generic KMS userspace driver possible. Hence it's better to move
> >   the documentation for this from the GEM section to the KMS section,
> >   next to the creation of framebuffer objects.
> > 
> > - Make it really clear that the returned handle isn't necessarily a
> >   GEM object (it can also be e.g. a TTM handle when running on top of
> >   vmwgfx).
> > 
> > - Add a paragraph to make it clear that this is just for unaccelarated
> >   userspace - gpu drivers need to have their own buffer object
> >   creation ioctl which is hardware specific.
> > 
> > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > 
> >  Documentation/DocBook/drm.tmpl | 125 +++++++++++++++++++-----------------
> > 1 file changed, 68 insertions(+), 57 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl
> > b/Documentation/DocBook/drm.tmpl index ed1d6d289022..9c3fdd59c995 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl

[snip]

> > @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
> > 
> >          handle (or a list of memory handles for multi-planar formats)
> >          through the <parameter>drm_mode_fb_cmd2</parameter> argument.
> >          This document assumes that the driver uses GEM, those handles
> >          thus reference GEM 
> > -        objects.
> > +        objects. But drivers are free to use their own backing storage
> > object
> > +       handles, e.g. vmwgfx directly exposes special TTM handles to
> > userspace
> > +       and so expects TTM handles in the create ioctl and not GEM
> > objects.
>
> Maybe remove the sentence saying "this document assumes that the
> driver uses GEM". I don't see where we explicitly do that.

We do in one place, in the documentation of the create_handle operation. We
could apply the following change and replace the above sentence with

"For drivers using GEM those handles reference GEM objects. Drivers are
however free to use their own backing storage object handles, e.g. vmwgfx
directly exposes special TTM handles to userspace and so expects TTM handles
in the create ioctl, not GEM object handles."

@@ -1010,8 +1010,8 @@ int max_width, max_height;</synopsis>
               reference the memory object associated with the first plane.
             </para>
             <para>
-              Drivers call <function>drm_gem_handle_create</function> to create
-              the handle.
+              GEM-aware drivers call <function>drm_gem_handle_create</function> to
+              create the handle.
             </para>
           </listitem>
           <listitem>

> Otherwise the patch looks fine.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

On Thu, Jan 23, 2014 at 12:21:42PM +0100, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote:
> > - This is _not_ a generic interface to create gem objects, but just an
> >   interface to make early boot services (like boot splash) with a
> >   generic KMS userspace driver possible. Hence it's better to move
> >   the documentation for this from the GEM section to the KMS section,
> >   next to the creation of framebuffer objects.
> > 
> > - Make it really clear that the returned handle isn't necessarily a
> >   GEM object (it can also be e.g. a TTM handle when running on top of
> >   vmwgfx).
> > 
> > - Add a paragraph to make it clear that this is just for unaccelarated
> >   userspace - gpu drivers need to have their own buffer object
> >   creation ioctl which is hardware specific.
> > 
> > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++++------------------
> > 1 file changed, 68 insertions(+), 57 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index ed1d6d289022..9c3fdd59c995 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> > @@ -830,62 +830,6 @@ char *date;</synopsis>
> >          </para>
> >        </sect3>
> >        <sect3>
> > -        <title>Dumb GEM Objects</title>
> > -        <para>
> > -          The GEM API doesn't standardize GEM objects creation and leaves
> > it to -          driver-specific ioctls. While not an issue for
> > full-fledged graphics -          stacks that include device-specific
> > userspace components (in libdrm for -          instance), this limit makes
> > DRM-based early boot graphics unnecessarily -          complex.
> > -        </para>
> > -        <para>
> > -          Dumb GEM objects partly alleviate the problem by providing a
> > standard -          API to create dumb buffers suitable for scanout, which
> > can then be used -          to create KMS frame buffers.
> > -        </para>
> > -        <para>
> > -          To support dumb GEM objects drivers must implement the
> > -          <methodname>dumb_create</methodname>,
> > -          <methodname>dumb_destroy</methodname> and
> > -          <methodname>dumb_map_offset</methodname> operations.
> > -        </para>
> > -        <itemizedlist>
> > -          <listitem>
> > -            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> > drm_device *dev, -                     struct drm_mode_create_dumb
> > *args);</synopsis> -            <para>
> > -              The <methodname>dumb_create</methodname> operation creates a
> > GEM -              object suitable for scanout based on the width, height
> > and depth -              from the struct
> > <structname>drm_mode_create_dumb</structname> -              argument. It
> > fills the argument's <structfield>handle</structfield>, -             
> > <structfield>pitch</structfield> and <structfield>size</structfield> -     
> >         fields with a handle for the newly created GEM object and its line
> > -              pitch and size in bytes.
> > -            </para>
> > -          </listitem>
> > -          <listitem>
> > -            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv,
> > struct drm_device *dev, -                      uint32_t handle);</synopsis>
> > -            <para>
> > -              The <methodname>dumb_destroy</methodname> operation destroys
> > a dumb -              GEM object created by
> > <methodname>dumb_create</methodname>. -            </para>
> > -          </listitem>
> > -          <listitem>
> > -            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> > struct drm_device *dev, -                         uint32_t handle, uint64_t
> > *offset);</synopsis> -            <para>
> > -              The <methodname>dumb_map_offset</methodname> operation
> > associates an -              mmap fake offset with the GEM object given by
> > the handle and returns -              it. Drivers must use the
> > -              <function>drm_gem_create_mmap_offset</function> function to
> > -              associate the fake offset as described in
> > -              <xref linkend="drm-gem-objects-mapping"/>.
> > -            </para>
> > -          </listitem>
> > -        </itemizedlist>
> > -      </sect3>
> > -      <sect3>
> >          <title>Memory Coherency</title>
> >          <para>
> >            When mapped to the device or used in a command buffer, backing
> > pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
> >          handle (or a list of memory handles for multi-planar formats)
> > through the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
> > assumes that the driver uses GEM, those handles thus reference GEM -       
> > objects.
> > +        objects. But drivers are free to use their own backing storage
> > object
> > +	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
> > +	and so expects TTM handles in the create ioctl and not GEM objects.
> 
> Nitpicking, I would say
> 
> "Drivers are however free to use their own backing storage object handles, 
> e.g. vmwgfx directly exposes special TTM handles to userspace and so expects 
> TTM handles in the create ioctl, not GEM objects."

I've already adjusted this a bit but haven't yet sent out the new version
of the patch. Slightly different wording now.

> >        <para>
> >          Drivers must first validate the requested frame buffer parameters
> > passed @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> >  	<function>drm_framebuffer_unregister_private</function>.
> >      </sect2>
> >      <sect2>
> > +      <title>Dumb GEM Objects</title>
> > +      <para>
> > +	The KMS API doesn't standardize backing storage object creation and
> 
> Strictly speaking isn't it the DRM API that's responsible for memory 
> management, not the KMS API ?

The driver's private api is responsible for memory management, but the
crucial thing here is that the KMS ioctls don't mandate anything specific
(beyong that it needs to use uint32_t for handles).

> > +	leaves it to driver-specific ioctls. Furthermore actually creating a
> > +	buffer object even for GEM-based drivers is done through a
> > +	driver-specific ioctl - GEM only has a common userspace interface for
> > +	sharing and destroying objects. While not an issue for full-fledged
> > +	graphics stacks that include device-specific userspace components (in
> > +	libdrm for instance), this limit makes DRM-based early boot graphics
> > +	unnecessarily complex.
> > +      </para>
> 
> This feels a bit out of place, can't we leave the section where it was ?

Imo the justification for why we have the dumb ioctls should be here. And
I wanted to mention both that KMS doesn't mandate a particular bo
interface like GEM and that on top GEM wouldn't even provide a common
allocation function anyway.

But besides that I think there's some room for improvement in the GEM
section to clarify what is the actual core interfaces, what is more helper
library in nature and what in GEM is more just a common design pattern for
driver ioctls but not specified in a mandatory way anywhere. E.g. atm all
drivers which implement a GEM interface (radeon, i915, ...) have a mostly
implicitly synchronizing buffer access interface, but there's nothing in
GEM mandating this. Or the usual confusing between TTM directly exposed to
userspace and TTM hidden behind a GEM-based ioctl interface.
-Daniel

> 
> > +      <para>
> > +        Dumb objects partly alleviate the problem by providing a standard
> > +        API to create dumb buffers suitable for scanout, which can then be
> > used +        to create KMS frame buffers.
> > +      </para>
> > +      <para>
> > +        To support dumb objects drivers must implement the
> > +        <methodname>dumb_create</methodname>,
> > +        <methodname>dumb_destroy</methodname> and
> > +        <methodname>dumb_map_offset</methodname> operations.
> > +      </para>
> > +      <itemizedlist>
> > +        <listitem>
> > +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> > drm_device *dev, +                   struct drm_mode_create_dumb
> > *args);</synopsis> +          <para>
> > +            The <methodname>dumb_create</methodname> operation creates a
> > driver +	    object (GEM or TTM handle) object suitable for scanout based
> > on the +	    width, height and depth from the struct
> > +	    <structname>drm_mode_create_dumb</structname> argument. It fills the
> > +	    argument's <structfield>handle</structfield>,
> > +	    <structfield>pitch</structfield> and <structfield>size</structfield>
> > +	    fields with a handle for the newly created object and its line
> > +            pitch and size in bytes.
> > +          </para>
> > +        </listitem>
> > +        <listitem>
> > +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct
> > drm_device *dev, +                    uint32_t handle);</synopsis>
> > +          <para>
> > +            The <methodname>dumb_destroy</methodname> operation destroys a
> > dumb +            object created by <methodname>dumb_create</methodname>. +
> >          </para>
> > +        </listitem>
> > +        <listitem>
> > +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> > struct drm_device *dev, +                       uint32_t handle, uint64_t
> > *offset);</synopsis> +          <para>
> > +            The <methodname>dumb_map_offset</methodname> operation
> > associates an +            mmap fake offset with the object given by the
> > handle and returns +            it. Drivers must use the
> > +            <function>drm_gem_create_mmap_offset</function> function to
> > +            associate the fake offset as described in
> > +            <xref linkend="drm-gem-objects-mapping"/>.
> > +          </para>
> > +        </listitem>
> > +      </itemizedlist>
> > +      <para>
> > +        Note that dumb objects may not be used for gpu accelaration, as has
> > been +	attempted on some ARM embedded platforms. Such drivers really must
> > have +	a hardware-specific ioctl to allocate suitable objects.
> > +      </para>
> > +    </sect2>
> > +    <sect2>
> >        <title>Output Polling</title>
> >        <synopsis>void (*output_poll_changed)(struct drm_device
> > *dev);</synopsis> <para>
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

- This is _not_ a generic interface to create gem objects, but just an
  interface to make early boot services (like boot splash) with a
  generic KMS userspace driver possible. Hence it's better to move
  the documentation for this from the GEM section to the KMS section,
  next to the creation of framebuffer objects.

- Make it really clear that the returned handle isn't necessarily a
  GEM object (it can also be e.g. a TTM handle when running on top of
  vmwgfx).

- Add a paragraph to make it clear that this is just for unaccelarated
  userspace - gpu drivers need to have their own buffer object
  creation ioctl which is hardware specific.

v2: Clarify that the documentation doesn't just apply to GEM-based
drivers only but is now generally valid, as suggested by David.

Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 129 ++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 59 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ed1d6d289022..809bf5f7e1c6 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -830,62 +830,6 @@ char *date;</synopsis>
         </para>
       </sect3>
       <sect3>
-        <title>Dumb GEM Objects</title>
-        <para>
-          The GEM API doesn't standardize GEM objects creation and leaves it to
-          driver-specific ioctls. While not an issue for full-fledged graphics
-          stacks that include device-specific userspace components (in libdrm for
-          instance), this limit makes DRM-based early boot graphics unnecessarily
-          complex.
-        </para>
-        <para>
-          Dumb GEM objects partly alleviate the problem by providing a standard
-          API to create dumb buffers suitable for scanout, which can then be used
-          to create KMS frame buffers.
-        </para>
-        <para>
-          To support dumb GEM objects drivers must implement the
-          <methodname>dumb_create</methodname>,
-          <methodname>dumb_destroy</methodname> and
-          <methodname>dumb_map_offset</methodname> operations.
-        </para>
-        <itemizedlist>
-          <listitem>
-            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
-                     struct drm_mode_create_dumb *args);</synopsis>
-            <para>
-              The <methodname>dumb_create</methodname> operation creates a GEM
-              object suitable for scanout based on the width, height and depth
-              from the struct <structname>drm_mode_create_dumb</structname>
-              argument. It fills the argument's <structfield>handle</structfield>,
-              <structfield>pitch</structfield> and <structfield>size</structfield>
-              fields with a handle for the newly created GEM object and its line
-              pitch and size in bytes.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
-                      uint32_t handle);</synopsis>
-            <para>
-              The <methodname>dumb_destroy</methodname> operation destroys a dumb
-              GEM object created by <methodname>dumb_create</methodname>.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
-                         uint32_t handle, uint64_t *offset);</synopsis>
-            <para>
-              The <methodname>dumb_map_offset</methodname> operation associates an
-              mmap fake offset with the GEM object given by the handle and returns
-              it. Drivers must use the
-              <function>drm_gem_create_mmap_offset</function> function to
-              associate the fake offset as described in
-              <xref linkend="drm-gem-objects-mapping"/>.
-            </para>
-          </listitem>
-        </itemizedlist>
-      </sect3>
-      <sect3>
         <title>Memory Coherency</title>
         <para>
           When mapped to the device or used in a command buffer, backing pages
@@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
         Frame buffers rely on the underneath memory manager for low-level memory
         operations. When creating a frame buffer applications pass a memory
         handle (or a list of memory handles for multi-planar formats) through
-        the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
-        assumes that the driver uses GEM, those handles thus reference GEM
-        objects.
+	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers using
+	GEM as their userspace buffer management interface this would be a GEM
+	handle.  But drivers are free to use their own backing storage object
+	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
+	and so expects TTM handles in the create ioctl and not GEM objects.
       </para>
       <para>
         Drivers must first validate the requested frame buffer parameters passed
@@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
 	<function>drm_framebuffer_unregister_private</function>.
     </sect2>
     <sect2>
+      <title>Dumb GEM Objects</title>
+      <para>
+	The KMS API doesn't standardize backing storage object creation and
+	leaves it to driver-specific ioctls. Furthermore actually creating a
+	buffer object even for GEM-based drivers is done through a
+	driver-specific ioctl - GEM only has a common userspace interface for
+	sharing and destroying objects. While not an issue for full-fledged
+	graphics stacks that include device-specific userspace components (in
+	libdrm for instance), this limit makes DRM-based early boot graphics
+	unnecessarily complex.
+      </para>
+      <para>
+        Dumb objects partly alleviate the problem by providing a standard
+        API to create dumb buffers suitable for scanout, which can then be used
+        to create KMS frame buffers.
+      </para>
+      <para>
+        To support dumb objects drivers must implement the
+        <methodname>dumb_create</methodname>,
+        <methodname>dumb_destroy</methodname> and
+        <methodname>dumb_map_offset</methodname> operations.
+      </para>
+      <itemizedlist>
+        <listitem>
+          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
+                   struct drm_mode_create_dumb *args);</synopsis>
+          <para>
+            The <methodname>dumb_create</methodname> operation creates a driver
+	    object (GEM or TTM handle) object suitable for scanout based on the
+	    width, height and depth from the struct
+	    <structname>drm_mode_create_dumb</structname> argument. It fills the
+	    argument's <structfield>handle</structfield>,
+	    <structfield>pitch</structfield> and <structfield>size</structfield>
+	    fields with a handle for the newly created object and its line
+            pitch and size in bytes.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
+                    uint32_t handle);</synopsis>
+          <para>
+            The <methodname>dumb_destroy</methodname> operation destroys a dumb
+            object created by <methodname>dumb_create</methodname>.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
+                       uint32_t handle, uint64_t *offset);</synopsis>
+          <para>
+            The <methodname>dumb_map_offset</methodname> operation associates an
+            mmap fake offset with the object given by the handle and returns
+            it. Drivers must use the
+            <function>drm_gem_create_mmap_offset</function> function to
+            associate the fake offset as described in
+            <xref linkend="drm-gem-objects-mapping"/>.
+          </para>
+        </listitem>
+      </itemizedlist>
+      <para>
+        Note that dumb objects may not be used for gpu accelaration, as has been
+	attempted on some ARM embedded platforms. Such drivers really must have
+	a hardware-specific ioctl to allocate suitable objects.
+      </para>
+    </sect2>
+    <sect2>
       <title>Output Polling</title>
       <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis>
       <para>
-- 
1.8.5.2

Re: [PATCH 19/19] drm/kms: don't export drm_mode_group_init_legacy_group

[Daniel Vetter]

On Thu, Jan 23, 2014 at 10:05:19AM +0000, Russell King - ARM Linux wrote:
> On Thu, Jan 23, 2014 at 11:00:28AM +0100, Daniel Vetter wrote:
> > On Thu, Jan 23, 2014 at 10:42:02AM +0100, David Herrmann wrote:
> > > If there's ever hardware that truly supports sub-device hotplugging,
> > > we can support that by adding a DRM-ClientCap like 3D modes. But as I
> > > understand, the current hardware is still a single piece of hardware
> > > which just might get probed via different buses and thus is not an
> > > atomic entity. So the device is still hotplugged as a whole, we just
> > > don't get the events through a single path.
> > 
> > Yeah, if we ever want to allow hotplugging after driver setup that's
> > massive work to do it right. Which is why I want to plug this hole here
> > before someone sneaks in for real ;-)
> 
> David did say at the kernel summit that he's not interested at all in
> hotplugging the components of a DRM system, and his statement appeared
> to be very clear in that regard that it was never going to happen.

I think we eventually need to be able to hot-plug connectors (and maybe
encoders) to support fancy dp1.2 output routing and multiple streams over
the same cable. But I agree that hotplugging crtcs is insane and better
done by hotplugging an entire drm device with that crtc. In any case not
something to worry about right now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

On Thursday 23 January 2014 13:47:31 Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 12:21:42PM +0100, Laurent Pinchart wrote:
> > On Thursday 23 January 2014 09:52:26 Daniel Vetter wrote:
> > > - This is _not_ a generic interface to create gem objects, but just an
> > >   interface to make early boot services (like boot splash) with a
> > >   generic KMS userspace driver possible. Hence it's better to move
> > >   the documentation for this from the GEM section to the KMS section,
> > >   next to the creation of framebuffer objects.
> > > 
> > > - Make it really clear that the returned handle isn't necessarily a
> > >   GEM object (it can also be e.g. a TTM handle when running on top of
> > >   vmwgfx).
> > > 
> > > - Add a paragraph to make it clear that this is just for unaccelarated
> > >   userspace - gpu drivers need to have their own buffer object
> > >   creation ioctl which is hardware specific.
> > > 
> > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > > 
> > >  Documentation/DocBook/drm.tmpl | 125 ++++++++++++++++++----------------
> > >  1 file changed, 68 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/Documentation/DocBook/drm.tmpl
> > > b/Documentation/DocBook/drm.tmpl index ed1d6d289022..9c3fdd59c995
> > > 100644
> > > --- a/Documentation/DocBook/drm.tmpl
> > > +++ b/Documentation/DocBook/drm.tmpl

[snip]

> > > pages @@ -970,7 +914,9 @@ int max_width, max_height;</synopsis>
> > >          handle (or a list of memory handles for multi-planar formats)
> > > through the <parameter>drm_mode_fb_cmd2</parameter> argument. This
> > > document assumes that the driver uses GEM, those handles thus reference
> > > GEM
> > > -        objects.
> > > +        objects. But drivers are free to use their own backing storage
> > > object
> > > +	handles, e.g. vmwgfx directly exposes special TTM handles to
> > > userspace
> > > +	and so expects TTM handles in the create ioctl and not GEM objects.
> > 
> > Nitpicking, I would say
> > 
> > "Drivers are however free to use their own backing storage object handles,
> > e.g. vmwgfx directly exposes special TTM handles to userspace and so
> > expects TTM handles in the create ioctl, not GEM objects."
> 
> I've already adjusted this a bit but haven't yet sent out the new version
> of the patch. Slightly different wording now.

OK. Please also see my reply to David.

> > >        <para>
> > >          Drivers must first validate the requested frame buffer
> > >          parameters passed
> > > @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> > >  	<function>drm_framebuffer_unregister_private</function>.
> > >      </sect2>
> > >      <sect2>
> > > +      <title>Dumb GEM Objects</title>
> > > +      <para>
> > > +	The KMS API doesn't standardize backing storage object creation and
> > 
> > Strictly speaking isn't it the DRM API that's responsible for memory
> > management, not the KMS API ?
> 
> The driver's private api is responsible for memory management, but the
> crucial thing here is that the KMS ioctls don't mandate anything specific
> (beyong that it needs to use uint32_t for handles).

Sure, but my point was that I believe memory management is the responsibility 
of DRM, not KMS. It of course needs to be driver-specific.

> > > +	leaves it to driver-specific ioctls. Furthermore actually creating a
> > > +	buffer object even for GEM-based drivers is done through a
> > > +	driver-specific ioctl - GEM only has a common userspace interface for
> > > +	sharing and destroying objects. While not an issue for full-fledged
> > > +	graphics stacks that include device-specific userspace components (in
> > > +	libdrm for instance), this limit makes DRM-based early boot graphics
> > > +	unnecessarily complex.
> > > +      </para>
> > 
> > This feels a bit out of place, can't we leave the section where it was ?
> 
> Imo the justification for why we have the dumb ioctls should be here. And
> I wanted to mention both that KMS doesn't mandate a particular bo
> interface like GEM and that on top GEM wouldn't even provide a common
> allocation function anyway.

I agree that a justification here is a good idea, but I would just add one 
paragraph that references the dumb GEM objects section instead of scattering 
GEM documentation around.

> But besides that I think there's some room for improvement in the GEM
> section to clarify what is the actual core interfaces, what is more helper
> library in nature and what in GEM is more just a common design pattern for
> driver ioctls but not specified in a mandatory way anywhere. E.g. atm all
> drivers which implement a GEM interface (radeon, i915, ...) have a mostly
> implicitly synchronizing buffer access interface, but there's nothing in
> GEM mandating this. Or the usual confusing between TTM directly exposed to
> userspace and TTM hidden behind a GEM-based ioctl interface.

I agree, the GEM section should be improved, and TTM documentation would be 
nice as well. I'm lacking time though (as well as knowledge about TTM).

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Thursday 23 January 2014 13:48:17 Daniel Vetter wrote:
> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
> 
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
> 
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
> 
> v2: Clarify that the documentation doesn't just apply to GEM-based
> drivers only but is now generally valid, as suggested by David.
> 
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 129 ++++++++++++++++++++------------------
> 1 file changed, 70 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..809bf5f7e1c6 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl

[snip]

> @@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
>          Frame buffers rely on the underneath memory manager for low-level
> memory operations. When creating a frame buffer applications pass a memory
> handle (or a list of memory handles for multi-planar formats) through -    
>    the <parameter>drm_mode_fb_cmd2</parameter> argument. This document -   
>     assumes that the driver uses GEM, those handles thus reference GEM -   
>     objects.
> +	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers using
> +	GEM as their userspace buffer management interface this would be a GEM
> +	handle.  But drivers are free to use their own backing storage object
> +	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
> +	and so expects TTM handles in the create ioctl and not GEM objects.

I'm not sure if the rule also applies to English, but I've always been thought 
not to start a sentence by "but" in French, so what about the following 
wording (as proposed in a previous mail that was sent too late for your v2 - 
you're too fast ;-)) ?

"Drivers are however free to use their own backing storage object handles, 
e.g. vmwgfx directly exposes special TTM handles to userspace and so expects 
TTM handles in the create ioctl, not GEM handles."

(I've also replaced "GEM objects" by "GEM handles" in the last sentence)

>        </para>
>        <para>
>          Drivers must first validate the requested frame buffer parameters
> passed @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>  	<function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb GEM Objects</title>
> +      <para>
> +	The KMS API doesn't standardize backing storage object creation and
> +	leaves it to driver-specific ioctls. Furthermore actually creating a
> +	buffer object even for GEM-based drivers is done through a
> +	driver-specific ioctl - GEM only has a common userspace interface for
> +	sharing and destroying objects. While not an issue for full-fledged
> +	graphics stacks that include device-specific userspace components (in
> +	libdrm for instance), this limit makes DRM-based early boot graphics
> +	unnecessarily complex.
> +      </para>

Regarding the location of this section, let's continue the discussion in the 
mail thread of your first patch.

> +      <para>
> +        Dumb objects partly alleviate the problem by providing a standard
> +        API to create dumb buffers suitable for scanout, which can then be
> used +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev, +                   struct drm_mode_create_dumb
> *args);</synopsis> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates a
> driver +	    object (GEM or TTM handle) object suitable for scanout based
> on the +	    width, height and depth from the struct
> +	    <structname>drm_mode_create_dumb</structname> argument. It fills the
> +	    argument's <structfield>handle</structfield>,
> +	    <structfield>pitch</structfield> and <structfield>size</structfield>
> +	    fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct
> drm_device *dev, +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation destroys a
> dumb +            object created by <methodname>dumb_create</methodname>. +
>          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev, +                       uint32_t handle, uint64_t
> *offset);</synopsis> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation
> associates an +            mmap fake offset with the object given by the
> handle and returns +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration, as has
> been +	attempted on some ARM embedded platforms. Such drivers really must
> have +	a hardware-specific ioctl to allocate suitable objects.
> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device
> *dev);</synopsis> <para>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

On Thu, Jan 23, 2014 at 01:56:51PM +0100, Laurent Pinchart wrote:
> > > >        <para>
> > > >          Drivers must first validate the requested frame buffer
> > > >          parameters passed
> > > > @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> > > >  	<function>drm_framebuffer_unregister_private</function>.
> > > >      </sect2>
> > > >      <sect2>
> > > > +      <title>Dumb GEM Objects</title>
> > > > +      <para>
> > > > +	The KMS API doesn't standardize backing storage object creation and
> > > 
> > > Strictly speaking isn't it the DRM API that's responsible for memory
> > > management, not the KMS API ?
> > 
> > The driver's private api is responsible for memory management, but the
> > crucial thing here is that the KMS ioctls don't mandate anything specific
> > (beyong that it needs to use uint32_t for handles).
> 
> Sure, but my point was that I believe memory management is the responsibility 
> of DRM, not KMS. It of course needs to be driver-specific.

Well imo the dumb ioctls are part of kms. DRM itself doesn't have any
memory management interfaces (at least if you ignore all the horror
stories around legacy ums/dri1 drivers). My reasons are:
- If you implement a kms driver you really should implement the dumb
  interfaces. Even when you have almost no memory management like the
  simpledrm driver.
- If you have a driver with memory management and command submission but
  no KMS, there's no reason at all to implement the dumb interfaces.
- ARM people abused dumb buffers for accelaration "because it worked", so
  imo moving it's documentation as far away as possible from the memory
  management section is a feature.

So the dumb stuff is a KMS interface to abstract away the lowest common
denominator between all kms drivers. Actually memory manament needs a real
interface, and is obviously separate.

> > > > +	leaves it to driver-specific ioctls. Furthermore actually creating a
> > > > +	buffer object even for GEM-based drivers is done through a
> > > > +	driver-specific ioctl - GEM only has a common userspace interface for
> > > > +	sharing and destroying objects. While not an issue for full-fledged
> > > > +	graphics stacks that include device-specific userspace components (in
> > > > +	libdrm for instance), this limit makes DRM-based early boot graphics
> > > > +	unnecessarily complex.
> > > > +      </para>
> > > 
> > > This feels a bit out of place, can't we leave the section where it was ?
> > 
> > Imo the justification for why we have the dumb ioctls should be here. And
> > I wanted to mention both that KMS doesn't mandate a particular bo
> > interface like GEM and that on top GEM wouldn't even provide a common
> > allocation function anyway.
> 
> I agree that a justification here is a good idea, but I would just add one 
> paragraph that references the dumb GEM objects section instead of scattering 
> GEM documentation around.

I've pretty much removed all mention of dumb gem objects ;-) There's one
mention of dumb_create left in the GEM/memory management section, but that
one is just an example for the lifetime and reference counting rules. So
not relevant.
> 
> > But besides that I think there's some room for improvement in the GEM
> > section to clarify what is the actual core interfaces, what is more helper
> > library in nature and what in GEM is more just a common design pattern for
> > driver ioctls but not specified in a mandatory way anywhere. E.g. atm all
> > drivers which implement a GEM interface (radeon, i915, ...) have a mostly
> > implicitly synchronizing buffer access interface, but there's nothing in
> > GEM mandating this. Or the usual confusing between TTM directly exposed to
> > userspace and TTM hidden behind a GEM-based ioctl interface.
> 
> I agree, the GEM section should be improved, and TTM documentation would be 
> nice as well. I'm lacking time though (as well as knowledge about TTM).

I have a few ideas, but I think I'll postpone this until I get around to
documenting the i915 GEM code a bit. Having a concrete driver to talk
about should help greatly to separate common concepts from artifacts of
the i915 implementation. I guess that review will also lead to some abi
cleanups to remove i915-ism from core gem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

- This is _not_ a generic interface to create gem objects, but just an
  interface to make early boot services (like boot splash) with a
  generic KMS userspace driver possible. Hence it's better to move
  the documentation for this from the GEM section to the KMS section,
  next to the creation of framebuffer objects.

- Make it really clear that the returned handle isn't necessarily a
  GEM object (it can also be e.g. a TTM handle when running on top of
  vmwgfx).

- Add a paragraph to make it clear that this is just for unaccelarated
  userspace - gpu drivers need to have their own buffer object
  creation ioctl which is hardware specific.

v2: Clarify that the documentation doesn't just apply to GEM-based
drivers only but is now generally valid, as suggested by David.

v3: Polish the intro sentence a bit and one s/objects/handles/ for
clarification, both suggested by Laurent.

Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 129 ++++++++++++++++++++++-------------------
 1 file changed, 70 insertions(+), 59 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ed1d6d289022..767318d5ddb6 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -830,62 +830,6 @@ char *date;</synopsis>
         </para>
       </sect3>
       <sect3>
-        <title>Dumb GEM Objects</title>
-        <para>
-          The GEM API doesn't standardize GEM objects creation and leaves it to
-          driver-specific ioctls. While not an issue for full-fledged graphics
-          stacks that include device-specific userspace components (in libdrm for
-          instance), this limit makes DRM-based early boot graphics unnecessarily
-          complex.
-        </para>
-        <para>
-          Dumb GEM objects partly alleviate the problem by providing a standard
-          API to create dumb buffers suitable for scanout, which can then be used
-          to create KMS frame buffers.
-        </para>
-        <para>
-          To support dumb GEM objects drivers must implement the
-          <methodname>dumb_create</methodname>,
-          <methodname>dumb_destroy</methodname> and
-          <methodname>dumb_map_offset</methodname> operations.
-        </para>
-        <itemizedlist>
-          <listitem>
-            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
-                     struct drm_mode_create_dumb *args);</synopsis>
-            <para>
-              The <methodname>dumb_create</methodname> operation creates a GEM
-              object suitable for scanout based on the width, height and depth
-              from the struct <structname>drm_mode_create_dumb</structname>
-              argument. It fills the argument's <structfield>handle</structfield>,
-              <structfield>pitch</structfield> and <structfield>size</structfield>
-              fields with a handle for the newly created GEM object and its line
-              pitch and size in bytes.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
-                      uint32_t handle);</synopsis>
-            <para>
-              The <methodname>dumb_destroy</methodname> operation destroys a dumb
-              GEM object created by <methodname>dumb_create</methodname>.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
-                         uint32_t handle, uint64_t *offset);</synopsis>
-            <para>
-              The <methodname>dumb_map_offset</methodname> operation associates an
-              mmap fake offset with the GEM object given by the handle and returns
-              it. Drivers must use the
-              <function>drm_gem_create_mmap_offset</function> function to
-              associate the fake offset as described in
-              <xref linkend="drm-gem-objects-mapping"/>.
-            </para>
-          </listitem>
-        </itemizedlist>
-      </sect3>
-      <sect3>
         <title>Memory Coherency</title>
         <para>
           When mapped to the device or used in a command buffer, backing pages
@@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
         Frame buffers rely on the underneath memory manager for low-level memory
         operations. When creating a frame buffer applications pass a memory
         handle (or a list of memory handles for multi-planar formats) through
-        the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
-        assumes that the driver uses GEM, those handles thus reference GEM
-        objects.
+	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers using
+	GEM as their userspace buffer management interface this would be a GEM
+	handle.  Drivers are however free to use their own backing storage object
+	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
+	and so expects TTM handles in the create ioctl and not GEM handles.
       </para>
       <para>
         Drivers must first validate the requested frame buffer parameters passed
@@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
 	<function>drm_framebuffer_unregister_private</function>.
     </sect2>
     <sect2>
+      <title>Dumb GEM Objects</title>
+      <para>
+	The KMS API doesn't standardize backing storage object creation and
+	leaves it to driver-specific ioctls. Furthermore actually creating a
+	buffer object even for GEM-based drivers is done through a
+	driver-specific ioctl - GEM only has a common userspace interface for
+	sharing and destroying objects. While not an issue for full-fledged
+	graphics stacks that include device-specific userspace components (in
+	libdrm for instance), this limit makes DRM-based early boot graphics
+	unnecessarily complex.
+      </para>
+      <para>
+        Dumb objects partly alleviate the problem by providing a standard
+        API to create dumb buffers suitable for scanout, which can then be used
+        to create KMS frame buffers.
+      </para>
+      <para>
+        To support dumb objects drivers must implement the
+        <methodname>dumb_create</methodname>,
+        <methodname>dumb_destroy</methodname> and
+        <methodname>dumb_map_offset</methodname> operations.
+      </para>
+      <itemizedlist>
+        <listitem>
+          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
+                   struct drm_mode_create_dumb *args);</synopsis>
+          <para>
+            The <methodname>dumb_create</methodname> operation creates a driver
+	    object (GEM or TTM handle) object suitable for scanout based on the
+	    width, height and depth from the struct
+	    <structname>drm_mode_create_dumb</structname> argument. It fills the
+	    argument's <structfield>handle</structfield>,
+	    <structfield>pitch</structfield> and <structfield>size</structfield>
+	    fields with a handle for the newly created object and its line
+            pitch and size in bytes.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
+                    uint32_t handle);</synopsis>
+          <para>
+            The <methodname>dumb_destroy</methodname> operation destroys a dumb
+            object created by <methodname>dumb_create</methodname>.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
+                       uint32_t handle, uint64_t *offset);</synopsis>
+          <para>
+            The <methodname>dumb_map_offset</methodname> operation associates an
+            mmap fake offset with the object given by the handle and returns
+            it. Drivers must use the
+            <function>drm_gem_create_mmap_offset</function> function to
+            associate the fake offset as described in
+            <xref linkend="drm-gem-objects-mapping"/>.
+          </para>
+        </listitem>
+      </itemizedlist>
+      <para>
+        Note that dumb objects may not be used for gpu accelaration, as has been
+	attempted on some ARM embedded platforms. Such drivers really must have
+	a hardware-specific ioctl to allocate suitable objects.
+      </para>
+    </sect2>
+    <sect2>
       <title>Output Polling</title>
       <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis>
       <para>
-- 
1.8.5.2

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

On Thursday 23 January 2014 14:46:40 Daniel Vetter wrote:
> On Thu, Jan 23, 2014 at 01:56:51PM +0100, Laurent Pinchart wrote:
> > > > >        <para>
> > > > >        
> > > > >          Drivers must first validate the requested frame buffer
> > > > >          parameters passed
> > > > > 
> > > > > @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> > > > > 
> > > > >  	<function>drm_framebuffer_unregister_private</function>.
> > > > >  	
> > > > >      </sect2>
> > > > >      <sect2>
> > > > > 
> > > > > +      <title>Dumb GEM Objects</title>
> > > > > +      <para>
> > > > > +	The KMS API doesn't standardize backing storage object creation
> > > > > and
> > > > 
> > > > Strictly speaking isn't it the DRM API that's responsible for memory
> > > > management, not the KMS API ?
> > > 
> > > The driver's private api is responsible for memory management, but the
> > > crucial thing here is that the KMS ioctls don't mandate anything
> > > specific (beyong that it needs to use uint32_t for handles).
> > 
> > Sure, but my point was that I believe memory management is the
> > responsibility of DRM, not KMS. It of course needs to be driver-specific.
> 
> Well imo the dumb ioctls are part of kms. DRM itself doesn't have any
> memory management interfaces (at least if you ignore all the horror
> stories around legacy ums/dri1 drivers). My reasons are:
> - If you implement a kms driver you really should implement the dumb
>   interfaces. Even when you have almost no memory management like the
>   simpledrm driver.
> - If you have a driver with memory management and command submission but
>   no KMS, there's no reason at all to implement the dumb interfaces.
> - ARM people abused dumb buffers for accelaration "because it worked", so
>   imo moving it's documentation as far away as possible from the memory
>   management section is a feature.
> 
> So the dumb stuff is a KMS interface to abstract away the lowest common
> denominator between all kms drivers. Actually memory manament needs a real
> interface, and is obviously separate.

OK. Those ioctls are not available on render nodes, which I suppose is another 
sign that they're KMS ioctls.

What about the device-specific memory allocation ioctls though ? Are they 
considered part of DRM or KMS ?

> > > > > +	leaves it to driver-specific ioctls. Furthermore actually
> > > > > creating a
> > > > > +	buffer object even for GEM-based drivers is done through a
> > > > > +	driver-specific ioctl - GEM only has a common userspace
> > > > > interface for
> > > > > +	sharing and destroying objects. While not an issue for
> > > > > full-fledged
> > > > > +	graphics stacks that include device-specific userspace
> > > > > components (in
> > > > > +	libdrm for instance), this limit makes DRM-based early boot
> > > > > graphics
> > > > > +	unnecessarily complex.
> > > > > +      </para>
> > > > 
> > > > This feels a bit out of place, can't we leave the section where it was
> > > > ?
> > > 
> > > Imo the justification for why we have the dumb ioctls should be here.
> > > And I wanted to mention both that KMS doesn't mandate a particular bo
> > > interface like GEM and that on top GEM wouldn't even provide a common
> > > allocation function anyway.
> > 
> > I agree that a justification here is a good idea, but I would just add one
> > paragraph that references the dumb GEM objects section instead of
> > scattering GEM documentation around.
> 
> I've pretty much removed all mention of dumb gem objects ;-) There's one
> mention of dumb_create left in the GEM/memory management section, but that
> one is just an example for the lifetime and reference counting rules. So
> not relevant.

Fair enough. I'm fine with that.

> > > But besides that I think there's some room for improvement in the GEM
> > > section to clarify what is the actual core interfaces, what is more
> > > helper library in nature and what in GEM is more just a common design
> > > pattern for driver ioctls but not specified in a mandatory way anywhere.
> > > E.g. atm all drivers which implement a GEM interface (radeon, i915, ...)
> > > have a mostly implicitly synchronizing buffer access interface, but
> > > there's nothing in GEM mandating this. Or the usual confusing between
> > > TTM directly exposed to userspace and TTM hidden behind a GEM-based
> > > ioctl interface.
> > 
> > I agree, the GEM section should be improved, and TTM documentation would
> > be nice as well. I'm lacking time though (as well as knowledge about TTM).
> 
> I have a few ideas, but I think I'll postpone this until I get around to
> documenting the i915 GEM code a bit. Having a concrete driver to talk
> about should help greatly to separate common concepts from artifacts of
> the i915 implementation. I guess that review will also lead to some abi
> cleanups to remove i915-ism from core gem.

Soudns good to me.

BTW, while you're at it, could you squash this typo fix in ?

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ed1d6d2..1e6579f 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -992,7 +992,7 @@ int max_width, max_height;</synopsis>
       </para>
 
       <para>
-       The initailization of the new framebuffer instance is finalized with a
+       The initialization of the new framebuffer instance is finalized with a
        call to <function>drm_framebuffer_init</function> which takes a 
pointer
        to DRM frame buffer operations (struct
        <structname>drm_framebuffer_funcs</structname>). Note that this 
function

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

One last round of nitpicking (including a typo fix, which gives me an excuse 
for a couple more comments :-)).

On Thursday 23 January 2014 14:50:25 Daniel Vetter wrote:
> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
> 
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
> 
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
> 
> v2: Clarify that the documentation doesn't just apply to GEM-based
> drivers only but is now generally valid, as suggested by David.
> 
> v3: Polish the intro sentence a bit and one s/objects/handles/ for
> clarification, both suggested by Laurent.
> 
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 129 ++++++++++++++++++++------------------
> 1 file changed, 70 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..767318d5ddb6 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl

[snip]

> @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>  	<function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb GEM Objects</title>

What about calling this "Dumb Memory Objects" (or something similar), as 
they're not specific to GEM ?

> +      <para>
> +	The KMS API doesn't standardize backing storage object creation and
> +	leaves it to driver-specific ioctls. Furthermore actually creating a
> +	buffer object even for GEM-based drivers is done through a
> +	driver-specific ioctl - GEM only has a common userspace interface for
> +	sharing and destroying objects. While not an issue for full-fledged
> +	graphics stacks that include device-specific userspace components (in
> +	libdrm for instance), this limit makes DRM-based early boot graphics
> +	unnecessarily complex.
> +      </para>
> +      <para>
> +        Dumb objects partly alleviate the problem by providing a standard
> +        API to create dumb buffers suitable for scanout, which can then be
> used
> +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev,
> +                   struct drm_mode_create_dumb *args);</synopsis>
> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates a
> driver
> +	    object (GEM or TTM handle) object suitable for scanout based on the

s/object suitable/suitable/ ?

> +	    width, height and depth from the struct
> +	    <structname>drm_mode_create_dumb</structname> argument. It fills the
> +	    argument's <structfield>handle</structfield>,
> +	    <structfield>pitch</structfield> and <structfield>size</structfield>
> +	    fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct
> drm_device *dev,
> +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation destroys a
> dumb
> +            object created by <methodname>dumb_create</methodname>.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev,
> +                       uint32_t handle, uint64_t *offset);</synopsis>
> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation
> associates an
> +            mmap fake offset with the object given by the handle and
> returns
> +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration, as has
> been
> +	attempted on some ARM embedded platforms. Such drivers really must have
> +	a hardware-specific ioctl to allocate suitable objects.

What about s/objects/memory objects/ ? "object" alone is rather vague for 
people not too familiar with DRM/KMS.

> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device
> *dev);</synopsis> <para>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

On Fri, Jan 24, 2014 at 12:08:36PM +0100, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thursday 23 January 2014 14:46:40 Daniel Vetter wrote:
> > On Thu, Jan 23, 2014 at 01:56:51PM +0100, Laurent Pinchart wrote:
> > > > > >        <para>
> > > > > >        
> > > > > >          Drivers must first validate the requested frame buffer
> > > > > >          parameters passed
> > > > > > 
> > > > > > @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> > > > > > 
> > > > > >  	<function>drm_framebuffer_unregister_private</function>.
> > > > > >  	
> > > > > >      </sect2>
> > > > > >      <sect2>
> > > > > > 
> > > > > > +      <title>Dumb GEM Objects</title>
> > > > > > +      <para>
> > > > > > +	The KMS API doesn't standardize backing storage object creation
> > > > > > and
> > > > > 
> > > > > Strictly speaking isn't it the DRM API that's responsible for memory
> > > > > management, not the KMS API ?
> > > > 
> > > > The driver's private api is responsible for memory management, but the
> > > > crucial thing here is that the KMS ioctls don't mandate anything
> > > > specific (beyong that it needs to use uint32_t for handles).
> > > 
> > > Sure, but my point was that I believe memory management is the
> > > responsibility of DRM, not KMS. It of course needs to be driver-specific.
> > 
> > Well imo the dumb ioctls are part of kms. DRM itself doesn't have any
> > memory management interfaces (at least if you ignore all the horror
> > stories around legacy ums/dri1 drivers). My reasons are:
> > - If you implement a kms driver you really should implement the dumb
> >   interfaces. Even when you have almost no memory management like the
> >   simpledrm driver.
> > - If you have a driver with memory management and command submission but
> >   no KMS, there's no reason at all to implement the dumb interfaces.
> > - ARM people abused dumb buffers for accelaration "because it worked", so
> >   imo moving it's documentation as far away as possible from the memory
> >   management section is a feature.
> > 
> > So the dumb stuff is a KMS interface to abstract away the lowest common
> > denominator between all kms drivers. Actually memory manament needs a real
> > interface, and is obviously separate.
> 
> OK. Those ioctls are not available on render nodes, which I suppose is another 
> sign that they're KMS ioctls.
> 
> What about the device-specific memory allocation ioctls though ? Are they 
> considered part of DRM or KMS ?

DRM is everything, so I'd add it to the driver-specific GEM section of
the documentation. Like I've said in another mail in this thread, there's
some room in the GEM docs to untangle core interfaces from driver-specific
interfaces (but often done in a similar way as established best practice).
But right now I'm mostly going through the modesetting stuff, also since
that's the part I want to start with for i915.

[snip]

> BTW, while you're at it, could you squash this typo fix in ?

Applied, thanks for spotting this typo.
-Daniel

> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d2..1e6579f 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -992,7 +992,7 @@ int max_width, max_height;</synopsis>
>        </para>
>  
>        <para>
> -       The initailization of the new framebuffer instance is finalized with a
> +       The initialization of the new framebuffer instance is finalized with a
>         call to <function>drm_framebuffer_init</function> which takes a 
> pointer
>         to DRM frame buffer operations (struct
>         <structname>drm_framebuffer_funcs</structname>). Note that this 
> function
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

On Fri, Jan 24, 2014 at 12:13:11PM +0100, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> One last round of nitpicking (including a typo fix, which gives me an excuse 
> for a couple more comments :-)).
> 
> On Thursday 23 January 2014 14:50:25 Daniel Vetter wrote:
> > - This is _not_ a generic interface to create gem objects, but just an
> >   interface to make early boot services (like boot splash) with a
> >   generic KMS userspace driver possible. Hence it's better to move
> >   the documentation for this from the GEM section to the KMS section,
> >   next to the creation of framebuffer objects.
> > 
> > - Make it really clear that the returned handle isn't necessarily a
> >   GEM object (it can also be e.g. a TTM handle when running on top of
> >   vmwgfx).
> > 
> > - Add a paragraph to make it clear that this is just for unaccelarated
> >   userspace - gpu drivers need to have their own buffer object
> >   creation ioctl which is hardware specific.
> > 
> > v2: Clarify that the documentation doesn't just apply to GEM-based
> > drivers only but is now generally valid, as suggested by David.
> > 
> > v3: Polish the intro sentence a bit and one s/objects/handles/ for
> > clarification, both suggested by Laurent.
> > 
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  Documentation/DocBook/drm.tmpl | 129 ++++++++++++++++++++------------------
> > 1 file changed, 70 insertions(+), 59 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> > index ed1d6d289022..767318d5ddb6 100644
> > --- a/Documentation/DocBook/drm.tmpl
> > +++ b/Documentation/DocBook/drm.tmpl
> 
> [snip]
> 
> > @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
> >  	<function>drm_framebuffer_unregister_private</function>.
> >      </sect2>
> >      <sect2>
> > +      <title>Dumb GEM Objects</title>
> 
> What about calling this "Dumb Memory Objects" (or something similar), as 
> they're not specific to GEM ?

I've gone through the entire section to remove all GEM-specific language
and missed the title. That's some good fail right there ;-) I'm going with
buffer objects, which I think is the most established language for gfx
memory management - GL also uses it in specs.

[snip]

> > +	attempted on some ARM embedded platforms. Such drivers really must have
> > +	a hardware-specific ioctl to allocate suitable objects.
> 
> What about s/objects/memory objects/ ? "object" alone is rather vague for 
> people not too familiar with DRM/KMS.

Also opted for buffer object here. Other suggestions applies unchaged.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/doc: Clarify the dumb object interfaces

[Daniel Vetter]

- This is _not_ a generic interface to create gem objects, but just an
  interface to make early boot services (like boot splash) with a
  generic KMS userspace driver possible. Hence it's better to move
  the documentation for this from the GEM section to the KMS section,
  next to the creation of framebuffer objects.

- Make it really clear that the returned handle isn't necessarily a
  GEM object (it can also be e.g. a TTM handle when running on top of
  vmwgfx).

- Add a paragraph to make it clear that this is just for unaccelarated
  userspace - gpu drivers need to have their own buffer object
  creation ioctl which is hardware specific.

v2: Clarify that the documentation doesn't just apply to GEM-based
drivers only but is now generally valid, as suggested by David.

v3: Polish the intro sentence a bit and one s/objects/handles/ for
clarification, both suggested by Laurent.

v4: More text polish from Laurent's review.

Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/DocBook/drm.tmpl | 131 ++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 60 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index ed1d6d289022..67cfe184749c 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -830,62 +830,6 @@ char *date;</synopsis>
         </para>
       </sect3>
       <sect3>
-        <title>Dumb GEM Objects</title>
-        <para>
-          The GEM API doesn't standardize GEM objects creation and leaves it to
-          driver-specific ioctls. While not an issue for full-fledged graphics
-          stacks that include device-specific userspace components (in libdrm for
-          instance), this limit makes DRM-based early boot graphics unnecessarily
-          complex.
-        </para>
-        <para>
-          Dumb GEM objects partly alleviate the problem by providing a standard
-          API to create dumb buffers suitable for scanout, which can then be used
-          to create KMS frame buffers.
-        </para>
-        <para>
-          To support dumb GEM objects drivers must implement the
-          <methodname>dumb_create</methodname>,
-          <methodname>dumb_destroy</methodname> and
-          <methodname>dumb_map_offset</methodname> operations.
-        </para>
-        <itemizedlist>
-          <listitem>
-            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
-                     struct drm_mode_create_dumb *args);</synopsis>
-            <para>
-              The <methodname>dumb_create</methodname> operation creates a GEM
-              object suitable for scanout based on the width, height and depth
-              from the struct <structname>drm_mode_create_dumb</structname>
-              argument. It fills the argument's <structfield>handle</structfield>,
-              <structfield>pitch</structfield> and <structfield>size</structfield>
-              fields with a handle for the newly created GEM object and its line
-              pitch and size in bytes.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
-                      uint32_t handle);</synopsis>
-            <para>
-              The <methodname>dumb_destroy</methodname> operation destroys a dumb
-              GEM object created by <methodname>dumb_create</methodname>.
-            </para>
-          </listitem>
-          <listitem>
-            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
-                         uint32_t handle, uint64_t *offset);</synopsis>
-            <para>
-              The <methodname>dumb_map_offset</methodname> operation associates an
-              mmap fake offset with the GEM object given by the handle and returns
-              it. Drivers must use the
-              <function>drm_gem_create_mmap_offset</function> function to
-              associate the fake offset as described in
-              <xref linkend="drm-gem-objects-mapping"/>.
-            </para>
-          </listitem>
-        </itemizedlist>
-      </sect3>
-      <sect3>
         <title>Memory Coherency</title>
         <para>
           When mapped to the device or used in a command buffer, backing pages
@@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
         Frame buffers rely on the underneath memory manager for low-level memory
         operations. When creating a frame buffer applications pass a memory
         handle (or a list of memory handles for multi-planar formats) through
-        the <parameter>drm_mode_fb_cmd2</parameter> argument. This document
-        assumes that the driver uses GEM, those handles thus reference GEM
-        objects.
+	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers using
+	GEM as their userspace buffer management interface this would be a GEM
+	handle.  Drivers are however free to use their own backing storage object
+	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
+	and so expects TTM handles in the create ioctl and not GEM handles.
       </para>
       <para>
         Drivers must first validate the requested frame buffer parameters passed
@@ -992,7 +938,7 @@ int max_width, max_height;</synopsis>
       </para>
 
       <para>
-	The initailization of the new framebuffer instance is finalized with a
+	The initilization of the new framebuffer instance is finalized with a
 	call to <function>drm_framebuffer_init</function> which takes a pointer
 	to DRM frame buffer operations (struct
 	<structname>drm_framebuffer_funcs</structname>). Note that this function
@@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
 	<function>drm_framebuffer_unregister_private</function>.
     </sect2>
     <sect2>
+      <title>Dumb Buffer Objects</title>
+      <para>
+	The KMS API doesn't standardize backing storage object creation and
+	leaves it to driver-specific ioctls. Furthermore actually creating a
+	buffer object even for GEM-based drivers is done through a
+	driver-specific ioctl - GEM only has a common userspace interface for
+	sharing and destroying objects. While not an issue for full-fledged
+	graphics stacks that include device-specific userspace components (in
+	libdrm for instance), this limit makes DRM-based early boot graphics
+	unnecessarily complex.
+      </para>
+      <para>
+        Dumb objects partly alleviate the problem by providing a standard
+        API to create dumb buffers suitable for scanout, which can then be used
+        to create KMS frame buffers.
+      </para>
+      <para>
+        To support dumb objects drivers must implement the
+        <methodname>dumb_create</methodname>,
+        <methodname>dumb_destroy</methodname> and
+        <methodname>dumb_map_offset</methodname> operations.
+      </para>
+      <itemizedlist>
+        <listitem>
+          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct drm_device *dev,
+                   struct drm_mode_create_dumb *args);</synopsis>
+          <para>
+            The <methodname>dumb_create</methodname> operation creates a driver
+	    object (GEM or TTM handle) suitable for scanout based on the
+	    width, height and depth from the struct
+	    <structname>drm_mode_create_dumb</structname> argument. It fills the
+	    argument's <structfield>handle</structfield>,
+	    <structfield>pitch</structfield> and <structfield>size</structfield>
+	    fields with a handle for the newly created object and its line
+            pitch and size in bytes.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct drm_device *dev,
+                    uint32_t handle);</synopsis>
+          <para>
+            The <methodname>dumb_destroy</methodname> operation destroys a dumb
+            object created by <methodname>dumb_create</methodname>.
+          </para>
+        </listitem>
+        <listitem>
+          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv, struct drm_device *dev,
+                       uint32_t handle, uint64_t *offset);</synopsis>
+          <para>
+            The <methodname>dumb_map_offset</methodname> operation associates an
+            mmap fake offset with the object given by the handle and returns
+            it. Drivers must use the
+            <function>drm_gem_create_mmap_offset</function> function to
+            associate the fake offset as described in
+            <xref linkend="drm-gem-objects-mapping"/>.
+          </para>
+        </listitem>
+      </itemizedlist>
+      <para>
+        Note that dumb objects may not be used for gpu accelaration, as has been
+	attempted on some ARM embedded platforms. Such drivers really must have
+	a hardware-specific ioctl to allocate suitable buffer objects.
+      </para>
+    </sect2>
+    <sect2>
       <title>Output Polling</title>
       <synopsis>void (*output_poll_changed)(struct drm_device *dev);</synopsis>
       <para>
-- 
1.8.5.2

Re: [PATCH] drm/doc: Clarify the dumb object interfaces

[Dieter Nützel]

Am 24.01.2014 17:58, schrieb Daniel Vetter:

Just two typos ('said' my spellchecker...;-)

Regards,
    Dieter

> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
> 
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
> 
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
> 
> v2: Clarify that the documentation doesn't just apply to GEM-based
> drivers only but is now generally valid, as suggested by David.
> 
> v3: Polish the intro sentence a bit and one s/objects/handles/ for
> clarification, both suggested by Laurent.
> 
> v4: More text polish from Laurent's review.
> 
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  Documentation/DocBook/drm.tmpl | 131 
> ++++++++++++++++++++++-------------------
>  1 file changed, 71 insertions(+), 60 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl 
> b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..67cfe184749c 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -830,62 +830,6 @@ char *date;</synopsis>
>          </para>
>        </sect3>
>        <sect3>
> -        <title>Dumb GEM Objects</title>
> -        <para>
> -          The GEM API doesn't standardize GEM objects creation and 
> leaves it to
> -          driver-specific ioctls. While not an issue for full-fledged 
> graphics
> -          stacks that include device-specific userspace components
> (in libdrm for
> -          instance), this limit makes DRM-based early boot graphics
> unnecessarily
> -          complex.
> -        </para>
> -        <para>
> -          Dumb GEM objects partly alleviate the problem by providing a 
> standard
> -          API to create dumb buffers suitable for scanout, which can
> then be used
> -          to create KMS frame buffers.
> -        </para>
> -        <para>
> -          To support dumb GEM objects drivers must implement the
> -          <methodname>dumb_create</methodname>,
> -          <methodname>dumb_destroy</methodname> and
> -          <methodname>dumb_map_offset</methodname> operations.
> -        </para>
> -        <itemizedlist>
> -          <listitem>
> -            <synopsis>int (*dumb_create)(struct drm_file *file_priv,
> struct drm_device *dev,
> -                     struct drm_mode_create_dumb *args);</synopsis>
> -            <para>
> -              The <methodname>dumb_create</methodname> operation 
> creates a GEM
> -              object suitable for scanout based on the width, height 
> and depth
> -              from the struct 
> <structname>drm_mode_create_dumb</structname>
> -              argument. It fills the argument's
> <structfield>handle</structfield>,
> -              <structfield>pitch</structfield> and
> <structfield>size</structfield>
> -              fields with a handle for the newly created GEM object
> and its line
> -              pitch and size in bytes.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv,
> struct drm_device *dev,
> -                      uint32_t handle);</synopsis>
> -            <para>
> -              The <methodname>dumb_destroy</methodname> operation
> destroys a dumb
> -              GEM object created by 
> <methodname>dumb_create</methodname>.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_map_offset)(struct drm_file
> *file_priv, struct drm_device *dev,
> -                         uint32_t handle, uint64_t 
> *offset);</synopsis>
> -            <para>
> -              The <methodname>dumb_map_offset</methodname> operation
> associates an
> -              mmap fake offset with the GEM object given by the
> handle and returns
> -              it. Drivers must use the
> -              <function>drm_gem_create_mmap_offset</function> function 
> to
> -              associate the fake offset as described in
> -              <xref linkend="drm-gem-objects-mapping"/>.
> -            </para>
> -          </listitem>
> -        </itemizedlist>
> -      </sect3>
> -      <sect3>
>          <title>Memory Coherency</title>
>          <para>
>            When mapped to the device or used in a command buffer, 
> backing pages
> @@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
>          Frame buffers rely on the underneath memory manager for
> low-level memory
>          operations. When creating a frame buffer applications pass a 
> memory
>          handle (or a list of memory handles for multi-planar formats) 
> through
> -        the <parameter>drm_mode_fb_cmd2</parameter> argument. This 
> document
> -        assumes that the driver uses GEM, those handles thus reference 
> GEM
> -        objects.
> +	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers 
> using
> +	GEM as their userspace buffer management interface this would be a 
> GEM
> +	handle.  Drivers are however free to use their own backing storage 
> object
> +	handles, e.g. vmwgfx directly exposes special TTM handles to 
> userspace
> +	and so expects TTM handles in the create ioctl and not GEM handles.
>        </para>
>        <para>
>          Drivers must first validate the requested frame buffer
> parameters passed
> @@ -992,7 +938,7 @@ int max_width, max_height;</synopsis>
>        </para>
> 
>        <para>
> -	The initailization of the new framebuffer instance is finalized with 
> a
> +	The initilization of the new framebuffer instance is finalized with a

=> initialization ;-)

>  	call to <function>drm_framebuffer_init</function> which takes a 
> pointer
>  	to DRM frame buffer operations (struct
>  	<structname>drm_framebuffer_funcs</structname>). Note that this 
> function
> @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>  	<function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb Buffer Objects</title>
> +      <para>
> +	The KMS API doesn't standardize backing storage object creation and
> +	leaves it to driver-specific ioctls. Furthermore actually creating a
> +	buffer object even for GEM-based drivers is done through a
> +	driver-specific ioctl - GEM only has a common userspace interface for
> +	sharing and destroying objects. While not an issue for full-fledged
> +	graphics stacks that include device-specific userspace components (in
> +	libdrm for instance), this limit makes DRM-based early boot graphics
> +	unnecessarily complex.
> +      </para>
> +      <para>
> +        Dumb objects partly alleviate the problem by providing a 
> standard
> +        API to create dumb buffers suitable for scanout, which can 
> then be used
> +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv,
> struct drm_device *dev,
> +                   struct drm_mode_create_dumb *args);</synopsis>
> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates 
> a driver
> +	    object (GEM or TTM handle) suitable for scanout based on the
> +	    width, height and depth from the struct
> +	    <structname>drm_mode_create_dumb</structname> argument. It fills 
> the
> +	    argument's <structfield>handle</structfield>,
> +	    <structfield>pitch</structfield> and 
> <structfield>size</structfield>
> +	    fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv,
> struct drm_device *dev,
> +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation 
> destroys a dumb
> +            object created by <methodname>dumb_create</methodname>.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file
> *file_priv, struct drm_device *dev,
> +                       uint32_t handle, uint64_t *offset);</synopsis>
> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation
> associates an
> +            mmap fake offset with the object given by the handle and 
> returns
> +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function 
> to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration,

=> acceleration

> as has been
> +	attempted on some ARM embedded platforms. Such drivers really must 
> have
> +	a hardware-specific ioctl to allocate suitable buffer objects.
> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device 
> *dev);</synopsis>
>        <para>

Re: [PATCH] drm/doc: Clarify the dumb object interfaces

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Friday 24 January 2014 17:58:40 Daniel Vetter wrote:
> - This is _not_ a generic interface to create gem objects, but just an
>   interface to make early boot services (like boot splash) with a
>   generic KMS userspace driver possible. Hence it's better to move
>   the documentation for this from the GEM section to the KMS section,
>   next to the creation of framebuffer objects.
> 
> - Make it really clear that the returned handle isn't necessarily a
>   GEM object (it can also be e.g. a TTM handle when running on top of
>   vmwgfx).
> 
> - Add a paragraph to make it clear that this is just for unaccelarated
>   userspace - gpu drivers need to have their own buffer object
>   creation ioctl which is hardware specific.
> 
> v2: Clarify that the documentation doesn't just apply to GEM-based
> drivers only but is now generally valid, as suggested by David.
> 
> v3: Polish the intro sentence a bit and one s/objects/handles/ for
> clarification, both suggested by Laurent.
> 
> v4: More text polish from Laurent's review.
> 
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/DocBook/drm.tmpl | 131 ++++++++++++++++++++------------------
> 1 file changed, 71 insertions(+), 60 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index ed1d6d289022..67cfe184749c 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -830,62 +830,6 @@ char *date;</synopsis>
>          </para>
>        </sect3>
>        <sect3>
> -        <title>Dumb GEM Objects</title>
> -        <para>
> -          The GEM API doesn't standardize GEM objects creation and leaves
> it to -          driver-specific ioctls. While not an issue for
> full-fledged graphics -          stacks that include device-specific
> userspace components (in libdrm for -          instance), this limit makes
> DRM-based early boot graphics unnecessarily -          complex.
> -        </para>
> -        <para>
> -          Dumb GEM objects partly alleviate the problem by providing a
> standard -          API to create dumb buffers suitable for scanout, which
> can then be used -          to create KMS frame buffers.
> -        </para>
> -        <para>
> -          To support dumb GEM objects drivers must implement the
> -          <methodname>dumb_create</methodname>,
> -          <methodname>dumb_destroy</methodname> and
> -          <methodname>dumb_map_offset</methodname> operations.
> -        </para>
> -        <itemizedlist>
> -          <listitem>
> -            <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev, -                     struct drm_mode_create_dumb
> *args);</synopsis> -            <para>
> -              The <methodname>dumb_create</methodname> operation creates a
> GEM -              object suitable for scanout based on the width, height
> and depth -              from the struct
> <structname>drm_mode_create_dumb</structname> -              argument. It
> fills the argument's <structfield>handle</structfield>, -             
> <structfield>pitch</structfield> and <structfield>size</structfield> -     
>         fields with a handle for the newly created GEM object and its line
> -              pitch and size in bytes.
> -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_destroy)(struct drm_file *file_priv,
> struct drm_device *dev, -                      uint32_t handle);</synopsis>
> -            <para>
> -              The <methodname>dumb_destroy</methodname> operation destroys
> a dumb -              GEM object created by
> <methodname>dumb_create</methodname>. -            </para>
> -          </listitem>
> -          <listitem>
> -            <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev, -                         uint32_t handle, uint64_t
> *offset);</synopsis> -            <para>
> -              The <methodname>dumb_map_offset</methodname> operation
> associates an -              mmap fake offset with the GEM object given by
> the handle and returns -              it. Drivers must use the
> -              <function>drm_gem_create_mmap_offset</function> function to
> -              associate the fake offset as described in
> -              <xref linkend="drm-gem-objects-mapping"/>.
> -            </para>
> -          </listitem>
> -        </itemizedlist>
> -      </sect3>
> -      <sect3>
>          <title>Memory Coherency</title>
>          <para>
>            When mapped to the device or used in a command buffer, backing
> pages @@ -968,9 +912,11 @@ int max_width, max_height;</synopsis>
>          Frame buffers rely on the underneath memory manager for low-level
> memory operations. When creating a frame buffer applications pass a memory
> handle (or a list of memory handles for multi-planar formats) through -    
>    the <parameter>drm_mode_fb_cmd2</parameter> argument. This document -   
>     assumes that the driver uses GEM, those handles thus reference GEM -   
>     objects.
> +	the <parameter>drm_mode_fb_cmd2</parameter> argument. For drivers using
> +	GEM as their userspace buffer management interface this would be a GEM
> +	handle.  Drivers are however free to use their own backing storage object
> +	handles, e.g. vmwgfx directly exposes special TTM handles to userspace
> +	and so expects TTM handles in the create ioctl and not GEM handles.
>        </para>
>        <para>
>          Drivers must first validate the requested frame buffer parameters
> passed @@ -992,7 +938,7 @@ int max_width, max_height;</synopsis>
>        </para>
> 
>        <para>
> -	The initailization of the new framebuffer instance is finalized with a
> +	The initilization of the new framebuffer instance is finalized with a
>  	call to <function>drm_framebuffer_init</function> which takes a pointer
>  	to DRM frame buffer operations (struct
>  	<structname>drm_framebuffer_funcs</structname>). Note that this function
> @@ -1052,6 +998,71 @@ int max_width, max_height;</synopsis>
>  	<function>drm_framebuffer_unregister_private</function>.
>      </sect2>
>      <sect2>
> +      <title>Dumb Buffer Objects</title>
> +      <para>
> +	The KMS API doesn't standardize backing storage object creation and
> +	leaves it to driver-specific ioctls. Furthermore actually creating a
> +	buffer object even for GEM-based drivers is done through a
> +	driver-specific ioctl - GEM only has a common userspace interface for
> +	sharing and destroying objects. While not an issue for full-fledged
> +	graphics stacks that include device-specific userspace components (in
> +	libdrm for instance), this limit makes DRM-based early boot graphics
> +	unnecessarily complex.
> +      </para>
> +      <para>
> +        Dumb objects partly alleviate the problem by providing a standard
> +        API to create dumb buffers suitable for scanout, which can then be
> used +        to create KMS frame buffers.
> +      </para>
> +      <para>
> +        To support dumb objects drivers must implement the
> +        <methodname>dumb_create</methodname>,
> +        <methodname>dumb_destroy</methodname> and
> +        <methodname>dumb_map_offset</methodname> operations.
> +      </para>
> +      <itemizedlist>
> +        <listitem>
> +          <synopsis>int (*dumb_create)(struct drm_file *file_priv, struct
> drm_device *dev, +                   struct drm_mode_create_dumb
> *args);</synopsis> +          <para>
> +            The <methodname>dumb_create</methodname> operation creates a
> driver +	    object (GEM or TTM handle) suitable for scanout based on the
> +	    width, height and depth from the struct
> +	    <structname>drm_mode_create_dumb</structname> argument. It fills the
> +	    argument's <structfield>handle</structfield>,
> +	    <structfield>pitch</structfield> and <structfield>size</structfield>
> +	    fields with a handle for the newly created object and its line
> +            pitch and size in bytes.
> +          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_destroy)(struct drm_file *file_priv, struct
> drm_device *dev, +                    uint32_t handle);</synopsis>
> +          <para>
> +            The <methodname>dumb_destroy</methodname> operation destroys a
> dumb +            object created by <methodname>dumb_create</methodname>. +
>          </para>
> +        </listitem>
> +        <listitem>
> +          <synopsis>int (*dumb_map_offset)(struct drm_file *file_priv,
> struct drm_device *dev, +                       uint32_t handle, uint64_t
> *offset);</synopsis> +          <para>
> +            The <methodname>dumb_map_offset</methodname> operation
> associates an +            mmap fake offset with the object given by the
> handle and returns +            it. Drivers must use the
> +            <function>drm_gem_create_mmap_offset</function> function to
> +            associate the fake offset as described in
> +            <xref linkend="drm-gem-objects-mapping"/>.
> +          </para>
> +        </listitem>
> +      </itemizedlist>
> +      <para>
> +        Note that dumb objects may not be used for gpu accelaration, as has
> been +	attempted on some ARM embedded platforms. Such drivers really must
> have +	a hardware-specific ioctl to allocate suitable buffer objects.
> +      </para>
> +    </sect2>
> +    <sect2>
>        <title>Output Polling</title>
>        <synopsis>void (*output_poll_changed)(struct drm_device
> *dev);</synopsis> <para>

-- 
Regards,

Laurent Pinchart