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

Commit Message

Laurent Pinchart Jan. 24, 2014, 11:08 a.m. UTC

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 ?

        <structname>drm_framebuffer_funcs</structname>). Note that this 
function

Comments

Daniel Vetter Jan. 24, 2014, 4:49 p.m. UTC | #1

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
>

Patch

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