From patchwork Fri Jan 24 11:08:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 3533861 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id DD4AEC02DC for ; Fri, 24 Jan 2014 11:08:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 01BDF20173 for ; Fri, 24 Jan 2014 11:08:00 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id CF2A620155 for ; Fri, 24 Jan 2014 11:07:54 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 05CB3FB5D9; Fri, 24 Jan 2014 03:07:52 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 6C554FB5D9 for ; Fri, 24 Jan 2014 03:07:49 -0800 (PST) Received: from avalon.localnet (unknown [91.177.147.208]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id A532435A47; Fri, 24 Jan 2014 12:06:53 +0100 (CET) From: Laurent Pinchart To: Daniel Vetter Subject: Re: [PATCH 01/19] drm/doc: Clarify the dumb object interfaces Date: Fri, 24 Jan 2014 12:08:36 +0100 Message-ID: <1750990.T5fEHcnzYt@avalon> User-Agent: KMail/4.11.5 (Linux/3.10.25-gentoo; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20140123134640.GX9772@phenom.ffwll.local> References: <1390467164-951-1-git-send-email-daniel.vetter@ffwll.ch> <1805347.h4MtSGFZv1@avalon> <20140123134640.GX9772@phenom.ffwll.local> MIME-Version: 1.0 Cc: Daniel Vetter , DRI Development X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development 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 X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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: > > > > > > > > > > > > > > > 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. > > 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. > > > > > + > > > > > > > > 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 ? drm_framebuffer_funcs). Note that this function 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; - The initailization of the new framebuffer instance is finalized with a + The initialization of the new framebuffer instance is finalized with a call to drm_framebuffer_init which takes a pointer to DRM frame buffer operations (struct