Received: by 2002:a05:6a10:af89:0:0:0:0 with SMTP id iu9csp6356786pxb; Thu, 27 Jan 2022 12:01:34 -0800 (PST) X-Google-Smtp-Source: ABdhPJzLDq+y5NatwofPIPb6JJX+Xk9/RHpYWGHDmtAcnLojiezNwWLQKZ2wMVZtLdbi6NwZqTf6 X-Received: by 2002:a17:90a:f682:: with SMTP id cl2mr15575149pjb.59.1643313694438; Thu, 27 Jan 2022 12:01:34 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1643313694; cv=none; d=google.com; s=arc-20160816; b=yoQIie6RKwUKScfgTXoEg146e6pGmHlM+1qs8oRRQB+Nhhrp00GAsRBp1Nbke2caFe P+Pb78srBKiMwLYz2XediXR61WVE2upX+Ff8uiO65xVkbKdxONKzZFDte69Q7du5MJwe yUwv+nYrkbdPcIljqzSxefJB2s5IYH0R2PnAnFDZa8OaOPkxlJ6xmld7vfVBQCWsBaDD VuFxbpN9Pw4oWujHj0hghg7AwQOeLBZdCHsX6UnlG74xAV3FKWvE3bsr0rZ8VW5R6+b+ RSyFtgr1VDoV+C0GZZRdvd3/nV2n8nhswS5HOaDgXe5mgaiFU5X0cAaKKakqodHyb6HH To0w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:mail-followup-to :message-id:subject:cc:to:from:date:dkim-signature; bh=EpsL82QGawXA+MspDkzQXzazfa90OBJ0RXteaUVBe0g=; b=GBDRQG3roVxiLipUkMwqM0i+ZUPW4U9KI6r8IO4yyezOXLcE5x8V4GFu3lnu0jD920 tgXXfqKet3sb3DOJb5+ssTccG0L6hCia27ab55PuuL+e17puIRdlCZ7X/VtedvD8WrI2 3DmOQ4o/zjMb6d7o4uojyKYooaV+DmYQ3pds3DqvRWYjPMD+bIwnc64/uPuxxIKzWy5r uRhx0/pQBcO8wHaW/kSFSaovrf2LX30ypA6ZlkEpzpCa8xPXz545f7RNK75fjKAlS0RC USwGnJVcB9pgfa1NB7XdkwFZQzcgJVNf/mcQ6tZxP7c4+sd6k2mh0AEIjbKTy6TM4+xE KgxQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b="efvV/Uzu"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id u3si3224128plg.68.2022.01.27.12.00.54; Thu, 27 Jan 2022 12:01:34 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@ffwll.ch header.s=google header.b="efvV/Uzu"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231374AbiA0LQj (ORCPT + 99 others); Thu, 27 Jan 2022 06:16:39 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60382 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231251AbiA0LQf (ORCPT ); Thu, 27 Jan 2022 06:16:35 -0500 Received: from mail-wr1-x433.google.com (mail-wr1-x433.google.com [IPv6:2a00:1450:4864:20::433]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D09AC06173B for ; Thu, 27 Jan 2022 03:16:34 -0800 (PST) Received: by mail-wr1-x433.google.com with SMTP id s9so4102487wrb.6 for ; Thu, 27 Jan 2022 03:16:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=EpsL82QGawXA+MspDkzQXzazfa90OBJ0RXteaUVBe0g=; b=efvV/UzuKzY52GovxKYPQeIOwEF/HuM9kP0DcVzxRxeiZWWO+6HIKDEdqb6uIKG6Hm l8DsuWLlYAVexgP9y9XfJ1bcXo4X/6C1qRV8zBrOUtJf8hHvNkHqpwPsr9Z7t5KH6+56 uCtiV6ZgXKYtYH2nIjOXLj5xVMvWiKhtejfQI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :content-transfer-encoding:in-reply-to; bh=EpsL82QGawXA+MspDkzQXzazfa90OBJ0RXteaUVBe0g=; b=pidTNm6n5bYGJp5/9tu8KVL5oHtlN/TskhOfPJSz7c2jGe6SVOqhpttWBqDu4YuQRO B919dOTR7P4649aZnGJxrzJG3pDWODg4+isCHORtCtVMN5Qri9iEutHtUbH+UEAaA1Vd NNA+R7/MAJsG0MMw1wsZ1NjZ3fgiMwVRpPrLjzj3VCGSilHSgZbbR/d2YzralwDlfyLh NMCTFZtbnvdCM3giAxF6NnWm1ywNyQ7KBTMRTCQVpTDSv7Lt+tiE6/VIV0JMXLGQqO9f Iu5nVrMtCEjfkXwjMG+cwDKT2ER7Kb6gVMRL72DYCgFE7rmjqtbKCEPdDZmq4Hl2boKA vUPQ== X-Gm-Message-State: AOAM533Di4ymKVmHHkdLzpf+YCebS5SZGo3KWIAZJG5YVlH2txmpADtX Eoa8svc9suLVbeI4oCtrL44Qig== X-Received: by 2002:a5d:6543:: with SMTP id z3mr2671000wrv.619.1643282192772; Thu, 27 Jan 2022 03:16:32 -0800 (PST) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id f6sm1813759wrj.26.2022.01.27.03.16.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jan 2022 03:16:31 -0800 (PST) Date: Thu, 27 Jan 2022 12:16:30 +0100 From: Daniel Vetter To: Christian =?iso-8859-1?Q?K=F6nig?= Cc: Lucas De Marchi , linaro-mm-sig@lists.linaro.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 02/19] dma-buf-map: Add helper to initialize second map Message-ID: Mail-Followup-To: Christian =?iso-8859-1?Q?K=F6nig?= , Lucas De Marchi , linaro-mm-sig@lists.linaro.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org References: <20220126203702.1784589-1-lucas.demarchi@intel.com> <20220126203702.1784589-3-lucas.demarchi@intel.com> <20220127075728.ygwgorhnrwaocdqv@ldmartin-desk2> <3066c6a7-fc73-d34d-d209-a3ff6818dfb6@amd.com> <20220127093332.wnkd2qy4tvwg5i5l@ldmartin-desk2> <27aed6b1-b465-6a52-2b0a-d748c9798414@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <27aed6b1-b465-6a52-2b0a-d748c9798414@amd.com> X-Operating-System: Linux phenom 5.10.0-8-amd64 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 27, 2022 at 11:21:20AM +0100, Christian K?nig wrote: > Am 27.01.22 um 11:00 schrieb Daniel Vetter: > > On Thu, Jan 27, 2022 at 01:33:32AM -0800, Lucas De Marchi wrote: > > > On Thu, Jan 27, 2022 at 09:57:25AM +0100, Daniel Vetter wrote: > > > > On Thu, Jan 27, 2022 at 09:02:54AM +0100, Christian K?nig wrote: > > > > > Am 27.01.22 um 08:57 schrieb Lucas De Marchi: > > > > > > On Thu, Jan 27, 2022 at 08:27:11AM +0100, Christian K?nig wrote: > > > > > > > Am 26.01.22 um 21:36 schrieb Lucas De Marchi: > > > > > > > > When dma_buf_map struct is passed around, it's useful to be able to > > > > > > > > initialize a second map that takes care of reading/writing to an offset > > > > > > > > of the original map. > > > > > > > > > > > > > > > > Add a helper that copies the struct and add the offset to the proper > > > > > > > > address. > > > > > > > Well what you propose here can lead to all kind of problems and is > > > > > > > rather bad design as far as I can see. > > > > > > > > > > > > > > The struct dma_buf_map is only to be filled in by the exporter and > > > > > > > should not be modified in this way by the importer. > > > > > > humn... not sure if I was? clear. There is no importer and exporter here. > > > > > Yeah, and exactly that's what I'm pointing out as problem here. > > > > > > > > > > You are using the inter driver framework for something internal to the > > > > > driver. That is an absolutely clear NAK! > > > > > > > > > > We could discuss that, but you guys are just sending around patches to do > > > > > this without any consensus that this is a good idea. > > > > Uh I suggested this, also we're already using dma_buf_map all over the > > > > place as a convenient abstraction. So imo that's all fine, it should allow > > > > drivers to simplify some code where on igpu it's in normal kernel memory > > > > and on dgpu it's behind some pci bar. > > > > > > > > Maybe we should have a better name for that struct (and maybe also a > > > > better place), but way back when we discussed that bikeshed I didn't come > > > > up with anything better really. > > > I suggest iosys_map since it abstracts access to IO and system memory. > > > > > > > > > There is a role delegation on filling out and reading a buffer when > > > > > > that buffer represents a struct layout. > > > > > > > > > > > > struct bla { > > > > > > ????int a; > > > > > > ????int b; > > > > > > ????int c; > > > > > > ????struct foo foo; > > > > > > ????struct bar bar; > > > > > > ????int d; > > > > > > } > > > > > > > > > > > > > > > > > > This implementation allows you to have: > > > > > > > > > > > > ????fill_foo(struct dma_buf_map *bla_map) { ... } > > > > > > ????fill_bar(struct dma_buf_map *bla_map) { ... } > > > > > > > > > > > > and the first thing these do is to make sure the map it's pointing to > > > > > > is relative to the struct it's supposed to write/read. Otherwise you're > > > > > > suggesting everything to be relative to struct bla, or to do the same > > > > > > I'm doing it, but IMO more prone to error: > > > > > > > > > > > > ????struct dma_buf_map map = *bla_map; > > > > > > ????dma_buf_map_incr(map, offsetof(...)); > > > > Wrt the issue at hand I think the above is perfectly fine code. The idea > > > > with dma_buf_map is really that it's just a special pointer, so writing > > > > the code exactly as pointer code feels best. Unfortunately you cannot make > > > > them typesafe (because of C), so the code sometimes looks a bit ugly. > > > > Otherwise we could do stuff like container_of and all that with > > > > typechecking in the macros. > > > I had exactly this code above, but after writting quite a few patches > > > using it, particularly with functions that have to write to 2 maps (see > > > patch 6 for example), it felt much better to have something to > > > initialize correctly from the start > > > > > > struct dma_buf_map other_map = *bla_map; > > > /* poor Lucas forgetting dma_buf_map_incr(map, offsetof(...)); */ > > > > > > is error prone and hard to debug since you will be reading/writting > > > from/to another location rather than exploding > > > > > > While with the construct below > > > > > > other_map; > > > ... > > > other_map = INITIALIZER() > > > > > > I can rely on the compiler complaining about uninitialized var. And > > > in most of the cases I can just have this single line in the beggining of the > > > function when the offset is constant: > > > > > > struct dma_buf_map other_map = INITIALIZER(bla_map, offsetof(..)); > > Hm yeah that's a good point that this allows us to rely on the compiler to > > check for uninitialized variables. > > > > Maybe include the above (with editing, but keeping the examples) in the > > kerneldoc to explain why/how to use this? With that the concept at least > > has my > > > > Acked-by: Daniel Vetter > > > > I'll leave it up to you & Christian to find a prettier color choice for > > the naming bikeshed. > > There is one major issue remaining with this and that is dma_buf_vunmap(): > > void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map *map); > > Here we expect the original pointer as returned by dma_buf_map(), otherwise > we vunmap() the wrong area! > > For all TTM based driver this doesn't matter since we keep the vmap base > separately in the BO anyway (IIRC), but we had at least one case where this > made boom last year. Yeah but isn't that the same if it's just a void *? If you pass the wrong pointer to an unmap function and not exactly what you go from the map function, then things go boom. This is like complaining that the following code wont work u32 *stuff stuff = kmap_local(some_page); *stuff++ = 0; *stuff = 1; kunmap_locak(stuff); It's just ... don't do that :-) Also since we pass dma_buf_map by value and not by pointer anywhere, the risk of this happening is pretty low since you tend to work on a copy. Same with void * pointers really. Now if people start to pass around struct dma_buf_map * as pointers for anything else than out parameters, then we're screwed. But that's like passing around void ** for lolz, which is just wrong (except when it's an out parameter or actually an array of pointers ofc). Or I really don't get your concern and you mean something else? -Daniel > Christian. > > > -Daniel > > > > > Lucas De Marchi > > > > > > > -Daniel > > > > > > > > > > IMO this construct is worse because at a point in time in the function > > > > > > the map was pointing to the wrong thing the function was supposed to > > > > > > read/write. > > > > > > > > > > > > It's also useful when the function has double duty, updating a global > > > > > > part of the struct and a table inside it (see example in patch 6) > > > > > > > > > > > > thanks > > > > > > Lucas De Marchi > > > > -- > > > > Daniel Vetter > > > > Software Engineer, Intel Corporation > > > > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fblog.ffwll.ch%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C0654a16ea3444271d7c308d9e17bd35d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637788744226808874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Q6soluBglaZLhLszdapaWuUVsqMq5qvJOKiJjO%2B9BTg%3D&reserved=0 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch