From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7011C07E98 for ; Mon, 5 Jul 2021 08:51:48 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3E647613C2 for ; Mon, 5 Jul 2021 08:51:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3E647613C2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id BA88489262; Mon, 5 Jul 2021 08:51:47 +0000 (UTC) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by gabe.freedesktop.org (Postfix) with ESMTP id F068F89262 for ; Mon, 5 Jul 2021 08:51:46 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 202ECD6E; Mon, 5 Jul 2021 01:51:46 -0700 (PDT) Received: from [192.168.1.179] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A83B93F5A1; Mon, 5 Jul 2021 01:51:44 -0700 (PDT) Subject: Re: [PATCH v3 5/7] drm/panfrost: Add a new ioctl to submit batches To: Boris Brezillon References: <20210702143225.3347980-1-boris.brezillon@collabora.com> <20210702143225.3347980-6-boris.brezillon@collabora.com> <20210702173843.44b3e322@collabora.com> <20210702201112.4c07c2c7@collabora.com> <20210705104319.7b709530@collabora.com> From: Steven Price Message-ID: Date: Mon, 5 Jul 2021 09:51:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <20210705104319.7b709530@collabora.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jason Ekstrand , Tomeu Vizoso , dri-devel@lists.freedesktop.org, Alyssa Rosenzweig , Rob Herring , Alyssa Rosenzweig , Robin Murphy Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 05/07/2021 09:43, Boris Brezillon wrote: > Hi Steven, > > On Mon, 5 Jul 2021 09:22:39 +0100 > Steven Price wrote: > >> On 02/07/2021 19:11, Boris Brezillon wrote: >>> On Fri, 2 Jul 2021 12:49:55 -0400 >>> Alyssa Rosenzweig wrote: >>> >>>>>> ``` >>>>>>> #define PANFROST_BO_REF_EXCLUSIVE 0x1 >>>>>>> +#define PANFROST_BO_REF_NO_IMPLICIT_DEP 0x2 >>>>>> ``` >>>>>> >>>>>> This seems logically backwards. NO_IMPLICIT_DEP makes sense if we're >>>>>> trying to keep backwards compatibility, but here you're crafting a new >>>>>> interface totally from scratch. If anything, isn't BO_REF_IMPLICIT_DEP >>>>>> the flag you'd want? >>>>> >>>>> AFAICT, all other drivers make the no-implicit-dep an opt-in, and I >>>>> didn't want to do things differently in panfrost. But if that's really >>>>> an issue, I can make it an opt-out. >>>> >>>> I don't have strong feelings either way. I was just under the >>>> impressions other drivers did this for b/w compat reasons which don't >>>> apply here. >>> >>> Okay, I think I'll keep it like that unless there's a strong reason to >>> make no-implicit dep the default. It's safer to oversync than the skip >>> the synchronization, so it does feel like something the user should >>> explicitly enable. >> >> I don't have strong feelings - ultimately the number of projects caring >> about the uABI is so limited the "default" is pretty irrelevant (it's >> not as if we are needing to guide random developers who are new to the >> interface). But a conservative default seems sensible. >> >>>> >>>>>> Hmm. I'm not /opposed/ and I know kbase uses strides but it seems like >>>>>> somewhat unwarranted complexity, and there is a combinatoric explosion >>>>>> here (if jobs, bo refs, and syncobj refs use 3 different versions, as >>>>>> this encoding permits... as opposed to just specifying a UABI version or >>>>>> something like that) >>>>> >>>>> Sounds like a good idea. I'll add a version field and map that >>>>> to a tuple. >>>> >>>> Cc Steven, does this make sense? >>> >>> I have this approach working, and I must admit I prefer it to the >>> per-object stride field passed to the submit struct. >>> >> >> There are benefits both ways: >> >> * Version number: easier to think about, and less worries about >> combinatorial explosion of possible options to test. >> >> * Explicit structure sizes/strides: much harder to accidentally forgot >> to change, clients 'naturally' move to newer versions just with recompiling. > > The version I just sent has a PANFROST_SUBMIT_BATCH_VERSION macro > defined in the the uAPI header, so getting right without changing the > code should be fine (same has with the sizeof(struct xx)) trick with > the per-desc stride approach). > >> >> For now I'd be tempted to go for the version number, but I suspect we >> should also ensure there's a generic 'flags' field in there. That would >> allow us to introduce new features/behaviours in a way which can be >> backported more easily if necessary. > > Adding features at the submit level without changing the version number > is already possible (we can extend drm_panfrost_batch_submit without > breaking the uABI), but I'm not sure that's a good idea... Ah, yes I'd forgotten the ioctl itself already had the implicit sizeof(struct) encoding. I guess there's no need for flags (now) because it can be added later if it even becomes useful. > If you mean adding a flags field at the job level, I can add it, but I > wonder what you have in mind when you say some features might be > interesting to backport. I really thought we'd force people to update > their kernel when they want those new features. My concern is if we ever find a security bug which requires new information/behaviour in the submit ABI to properly fix. In this case it would be appropriate to backport a 'feature' (bug fix) which provides a new ABI but it would need to be a small change. A flags field where we can set a "PANFROST_ACTUALLY_BE_SECURE" bit would be useful then - but we wouldn't want to start bumping version numbers in the backport. But at least for now we could just assume we'll expand the ioctl struct if we ever hit that situation, so no need for an explicit flags field. Steve