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

[Boris Brezillon <boris.brezillon@collabora.com>]

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.

Well, I can tell you some people are using 32-bit kernels ;-).

> 
> I can't really think of anything better than letting user space set the
> MMIO range. Having an ioctl which returned a special fd just for MMIO
> would be one option (which would preserve the full 44 bit GPU VA) but
> seems somewhat overkill.

Yeah, I don't think I like the separate-fd approach. Just feels like it
goes against the DRM-way of doing things. And, with 32-bit userspace,
we'd be limited by the CPU VA range anyway. Of course it's orthogonal
to the max file offset, and just because we can't map all buffers at
once, doesn't mean we don't want to be able to address more than 4G of
memory. But with 43-bit left (I think I'd prefer if we enforce a log2
value for the mmio offset/size, meaning that the max MMIO range would be
1ull << 43), that means we're still able to address 8TB of memory. I
guess that's more than enough for 32-bit users...

> Hiding the mmap within an ioctl would of course
> be bad as it breaks tools like Valgrind.

Don't like this idea either.

> 
> Oh and please do make it a range - user space submission will be adding
> to the MMIO range ;)

Yeah, that was the plan (I keep usermode submission in the back of my
mind ;-)).