From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces Date: Thu, 23 Jan 2014 13:56:51 +0100 Message-ID: <1805347.h4MtSGFZv1@avalon> References: <1390467164-951-1-git-send-email-daniel.vetter@ffwll.ch> <1755306.LJ2EgIi7Xi@avalon> <20140123124731.GV9772@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 53F20FBEC6 for ; Thu, 23 Jan 2014 04:56:04 -0800 (PST) In-Reply-To: <20140123124731.GV9772@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Daniel Vetter Cc: Daniel Vetter , DRI Development List-Id: dri-devel@lists.freedesktop.org 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 > > > --- > > > > > > 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; > > > handle (or a list of memory handles for multi-planar formats) > > > through the drm_mode_fb_cmd2 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. > > > > > > Drivers must first validate the requested frame buffer > > > parameters passed > > > @@ -1052,6 +998,71 @@ int max_width, max_height; > > > drm_framebuffer_unregister_private. > > > > > > > > > + Dumb GEM Objects > > > + > > > + 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. > > > + > > > > 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