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

[Daniel Vetter <daniel@ffwll.ch>]

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