Re: [PATCH v2 02/15] drm/panthor: Add uAPI

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Steven Price <steven.price@arm.com>
Cc: "Neil Armstrong" <neil.armstrong@linaro.org>,
	"Nicolas Boichat" <drinkcat@chromium.org>,
	"Daniel Stone" <daniels@collabora.com>,
	"Liviu Dudau" <Liviu.Dudau@arm.com>,
	dri-devel@lists.freedesktop.org,
	"Clément Péron" <peron.clem@gmail.com>,
	"Marty E . Plummer" <hanetzer@startmail.com>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Faith Ekstrand" <faith.ekstrand@collabora.com>
Subject: Re: [PATCH v2 02/15] drm/panthor: Add uAPI
Date: Mon, 4 Sep 2023 18:16:34 +0200	[thread overview]
Message-ID: <20230904181634.5871dfa3@collabora.com> (raw)
In-Reply-To: <6f49ff37-0c39-35b3-f78b-02b69a3dcbd0@arm.com>

On Mon, 4 Sep 2023 16:22:19 +0100
Steven Price <steven.price@arm.com> wrote:

> On 04/09/2023 10:26, Boris Brezillon wrote:
> > On Mon, 4 Sep 2023 08:42:08 +0100
> > Steven Price <steven.price@arm.com> wrote:
> >   
> >> On 01/09/2023 17:10, Boris Brezillon wrote:  
> >>> On Wed,  9 Aug 2023 18:53:15 +0200
> >>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>>     
> >>>> +/**
> >>>> + * DOC: MMIO regions exposed to userspace.
> >>>> + *
> >>>> + * .. c:macro:: DRM_PANTHOR_USER_MMIO_OFFSET
> >>>> + *
> >>>> + * File offset for all MMIO regions being exposed to userspace. Don't use
> >>>> + * this value directly, use DRM_PANTHOR_USER_<name>_OFFSET values instead.
> >>>> + *
> >>>> + * .. c:macro:: DRM_PANTHOR_USER_FLUSH_ID_MMIO_OFFSET
> >>>> + *
> >>>> + * File offset for the LATEST_FLUSH_ID register. The Userspace driver controls
> >>>> + * GPU cache flushling through CS instructions, but the flush reduction
> >>>> + * mechanism requires a flush_id. This flush_id could be queried with an
> >>>> + * ioctl, but Arm provides a well-isolated register page containing only this
> >>>> + * read-only register, so let's expose this page through a static mmap offset
> >>>> + * and allow direct mapping of this MMIO region so we can avoid the
> >>>> + * user <-> kernel round-trip.
> >>>> + */
> >>>> +#define DRM_PANTHOR_USER_MMIO_OFFSET		(0x1ull << 56)    
> >>>
> >>> I'm playing with a 32-bit kernel/userspace, and this is problematic,
> >>> because vm_pgoff is limited to 32-bit there, meaning we can only map up
> >>> to (1ull << (PAGE_SHIFT + 32)) - 1. Should we add a DEV_QUERY to let
> >>> userspace set the mmio range?    
> >>
> >> Hmm, I was rather hoping we could ignore 32 bit these days ;) But while
> >> I can't see why anyone would be running a 32 bit kernel, I guess 32 bit
> >> user space is likely to still be needed.  
> > 
> > Uh, I just hit a new problem with 32-bit kernels: the io-pgtable
> > interface (io_pgtable_ops) passes device VAs as unsigned longs, meaning
> > the GPU VA space is limited to 4G on a 32-bit build :-(. Robin, any
> > chance you could advise me on what to do here?
> > 
> > 1. assume this limitation is here for a good reason, and limit the GPU
> > VA space to 32-bits on 32-bit kernels
> > 
> > or
> > 
> > 2. update the interface to make iova an u64  
> 
> I'm not sure I can answer the question from a technical perspective,
> hopefully Robin will be able to.

Had a quick chat with Robin, and he's recommending going for #1 too.

> 
> But why do we care about 32-bit kernels on a platform which is new
> enough to have a CSF-GPU (and by extension a recent 64-bit CPU)?

Apparently the memory you save by switching to a 32-bit kernel matters
to some people. To clarify, the CPU is aarch64, but they want to use it
in 32-bit mode.

> 
> Given the other limitations present in a 32-bit kernel I'd be tempted to
> say '1' just for simplicity. Especially since apparently we've lived
> with this for panfrost which presumably has the same limitation (even
> though all Bifrost/Midgard GPUs have at least 33 bits of VA space).

Well, Panfrost is simpler in that you don't have this kernel VA range,
and, IIRC, we are using the old format that naturally limits the GPU VA
space to 4G.