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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/doc: Clarify the dumb object interfaces
Date: Fri, 24 Jan 2014 12:13:11 +0100	[thread overview]
Message-ID: <8133797.MaFuyckv2B@avalon> (raw)
In-Reply-To: <1390485025-16840-1-git-send-email-daniel.vetter@ffwll.ch>

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