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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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