From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id AF64C30B53E for ; Mon, 11 May 2026 15:12:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778512334; cv=none; b=gjsE3jgSvqso2byQMBTMxM1v1BH3iLJEsoMoNmFKmLnEOjUULxlhW4j3f1m2v8XL7zc1TTJsA36TwuQnzaaNtEe9b4v6Kc2eBwDm0SQz6nlizkLECL7Q/GVfNap9ORVbeVpj3fVk5zk7PfEUiz7DRj49KsLDt95xX0sBr5MJjl8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778512334; c=relaxed/simple; bh=MFENZ4IvnRLDiAxhWlbpAStaP0Sjba2UXqKDzZyF6Z0=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=SvBEmN10LXuxSMqkH9m84yuCxgkzObSIAjY5mwoDslV/8FEGjS4Nkr+JR7lEFHwljGVb48dVrKsczNUjaktR4QBi7tsej8w/VVW1aeYUwhzk7IYJ4qUkKiexiH3tUp/v9ZEaD3qs9FOgvL8tM07ecXuk1P4XXvc7G1Z29tpBS0w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=egMCxhEa; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="egMCxhEa" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1778512330; bh=MFENZ4IvnRLDiAxhWlbpAStaP0Sjba2UXqKDzZyF6Z0=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=egMCxhEasf/qfX+Nj/nspopH7DsaQKvjHcJHhCWcWwyHHjKLySy34QhimEBg/GwOh g3VUjs77a9XeQK8RKBfQAXBVVrbe+wXB6nsC3c6NlTATqFJISpaLz0vZRReoV2tNGx pV/VDHILWgXAIJqDaUNLHeGg9/gPWloCoO21gqH30HuogtLAH+40FYkfIzgtvUDclL 4PmxbMMXNVReYIMHcfFIdHFyQA8qwP0O6dYEHK71ODbFHsl/MWj0gStuFhZSIDTP5j 6zEfWHgl4XIq1RdmuIGoM3whcS18B0ubPObWB3iNqNlkMgrlAXAht9ic0+BILi7hGd OPYdflhxWmhcg== Received: from fedora (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 65BAB17E040C; Mon, 11 May 2026 17:12:10 +0200 (CEST) Date: Mon, 11 May 2026 17:12:07 +0200 From: Boris Brezillon To: Nicolas Frattaroli Cc: kernel@collabora.com, Steven Price , Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/panthor: Wrap register accessor helpers for type safety Message-ID: <20260511171207.69723480@fedora> In-Reply-To: References: <20260508-panthor-gpu-read-type-v1-1-733a9d8b3a11@collabora.com> <20260511135641.790f797d@fedora> Organization: Collabora X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 11 May 2026 15:53:31 +0200 Nicolas Frattaroli wrote: > > > > > { > > > return readl_relaxed(iomem + reg); > > > } > > > > > > -static inline void gpu_write64(void __iomem *iomem, u32 reg, u64 data) > > > +/* > > > + * The function signature of gpu_read/gpu_write/gpu_read_relaxed/... used to > > > + * take a &struct panthor_device* as the first parameter. During the split of > > > + * iomem ranges into individual sub-components, this was changed to take a > > > + * void __iomem* instead. These wrappers exists Tto avoid situations wherein > > > + * pre-refactor patches are applied in error, as they'd compile fine. That's > > > + * because the old calling convention's first parameter implicitly casts to a > > > + * void pointer. > > > + */ > > > + > > > +#define gpu_write(iomem, reg, data) ({ \ > > > + static_assert(!__same_type((iomem), struct panthor_device *)); \ > > > > Hm, this only covers ptdev being passed as an iomem pointer. I know it's > > the only case we had so far, but if we're going to add type enforcement, > > I think I'd prefer if we were covered for more than just ptdev. > > > > One way of doing that would be to wrap the `void __iomem *iomem` in an > > explicit type like: > > > > struct panthor_reg_bank { > > void __iomem *iomem; > > }; > > > > which then gets passed to gpu_{read,write} helpers (see the diff below). > > Hm, okay, the diff below is smaller than I feared. Though it doesn't get > us type checking for someone, say, trying to read GPU_STATUS with the > iomem of panthor_fw. Yep, that's annoying, though solving that would require connecting reg definitions (in panthor_xxx_regs.h) to a specific reg_bank, which is only doable if we provide per-component accessors like: #define mmu_reg(_mmu, _name) ((_mmu)->iomem + MMU_ ##) #define mmu_read(_mmu, _name) gpu_read_iomem(mmu_reg(_mmu, _name)) #define mmu_write(_mmu, _name, _val) \ gpu_write_iomem(mmu_reg(_mmu, _name),_ val) > But neither does my proposal below. > > > > > The other way would be to pass the component, and have the macro > > do the ->iomem deref, but there's a few places where reg banks > > are accessed outside of the components that own them (panthor_hw.c). > > Yeah, I prototyped going down something along that route by having > the register accessors be generics that are implemented by each > component, and it's a bit messy. Either you expose the struct > definitions of individual components so that this header has visibility > into them (not great), or you add boilerplate "do this accessor > operation for this component" helpers for every component, which is both > verbose and possibly causes the inlining to no longer work, though I have > yet to verify that. > > If we do want to go down this route (though I'm not sure, since your > reg bank solution seems to get us the same guarantees but without bringing > generics into this), then the following may be an okay idea: > > I think having just the iomem deref genericised may be a good middle > ground. If instead of making it a deref, we make it return the pointer > to the member into the component that it can then deref, then the > component-specific part can be pure (since offset of the iomem member > is constant so for a particular pointer to a component, the pointer to > the iomem member only depends on the passed-in pointer to component.) > > This should make sure that when the compiler gets > > panthor_gpu_write(ptdev->gpu, foo, bar); > val = panthor_gpu_read(ptdev->gpu, baz); > > it can optimise the expanded > > iomem = *panthor_get_iomem_ptr(ptdev->gpu); > panthor_actual_write(iomem, foo, bar); > iomem = *panthor_get_iomem_ptr(ptdev->gpu); > val = panthor_actual_read(iomem, baz); > > to the simplified > > iomem = *panthor_get_iomem_ptr(ptdev->gpu); > panthor_actual_write(iomem, foo, bar); > val = panthor_actual_read(iomem, baz); > > because panthor_get_iomem_ptr will be known to return the same value > when called with the same input param. Right, as long as the compiler sees the definition of panthor_get__iomem_ptr() (which should be the case any time a read/write happens inside the panthor_.c compilation unit), it hopefully inlines the whole thing and you get the iomem pointer from a direct deref rather than a function call. LTO might even give us link time optim for the bits in panthor_hw.c where the compiler can't see through struct panthor_{gpu,pwr}. This being said, there's still no guarantee that one would mix regs and banks randomly, like gpu_read(ptdev->mmu, GPU_ID); > > Anway, I think it's probably best if I abandon this and you just send > your patch to the list with a real base. I only have one comment on it, > which I've included inline. Let's wait for Liviu's and Steve's feedback before taking any action, cause that's still quite a lot of changes, and it's not clear it will help much once we've got all the pending patchset rebased on misc-next (that's a mistake you do once at rebase time, once you've got bitten, you tend to be more careful ;-)).