Re: [PATCH v2 4/4] drm/i915/uapi: document behaviour for DG2 64K support

[Robert Beckett]

On 19/01/2022 18:36, Jordan Justen wrote:
> Robert Beckett <bob.beckett@xxxxxxxxxxxxx> writes:
>
> > From: Matthew Auld <matthew.auld@xxxxxxxxx>
> >
> > On discrete platforms like DG2, we need to support a minimum page size
> > of 64K when dealing with device local-memory. This is quite tricky for
> > various reasons, so try to document the new implicit uapi for this.
> >
> > v2: Fixed suggestions on formatting [Daniel]
> >
> > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > Signed-off-by: Ramalingam C <ramalingam.c@xxxxxxxxx>
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
> > cc: Simon Ser <contact@xxxxxxxxxxx>
> > cc: Pekka Paalanen <ppaalanen@xxxxxxxxx>
> > Cc: Jordan Justen <jordan.l.justen@xxxxxxxxx>
> > Cc: Kenneth Graunke <kenneth@xxxxxxxxxxxxx>
> > Cc: mesa-dev@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Tony Ye <tony.ye@xxxxxxxxx>
> > Cc: Slawomir Milczarek <slawomir.milczarek@xxxxxxxxx>
> > ---
> >   include/uapi/drm/i915_drm.h | 44 ++++++++++++++++++++++++++++++++-----
> >   1 file changed, 39 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 5e678917da70..486b7b96291e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1118,10 +1118,16 @@ struct drm_i915_gem_exec_object2 {
> >   	/**
> >   	 * When the EXEC_OBJECT_PINNED flag is specified this is populated by
> >   	 * the user with the GTT offset at which this object will be pinned.
> > +	 *
> >   	 * When the I915_EXEC_NO_RELOC flag is specified this must contain the
> >   	 * presumed_offset of the object.
> > +	 *
> >   	 * During execbuffer2 the kernel populates it with the value of the
> >   	 * current GTT offset of the object, for future presumed_offset writes.
> > +	 *
> > +	 * See struct drm_i915_gem_create_ext for the rules when dealing with
> > +	 * alignment restrictions with I915_MEMORY_CLASS_DEVICE, on devices with
> > +	 * minimum page sizes, like DG2.
> >   	 */
> >   	__u64 offset;
> >   
> > @@ -3145,11 +3151,39 @@ struct drm_i915_gem_create_ext {
> >   	 *
> >   	 * The (page-aligned) allocated size for the object will be returned.
> >   	 *
> > -	 * Note that for some devices we have might have further minimum
> > -	 * page-size restrictions(larger than 4K), like for device local-memory.
> > -	 * However in general the final size here should always reflect any
> > -	 * rounding up, if for example using the I915_GEM_CREATE_EXT_MEMORY_REGIONS
> > -	 * extension to place the object in device local-memory.
> > +	 *
> > +	 * **DG2 64K min page size implications:**
>
> Long term, I'm not sure that the "**" (for emphasis) is needed here or
> below. It's interesting at the moment, but will be just another thing
> baked into the kernel/user code in a month from now. :)

fair point, I'll make it less shouty

> > +	 *
> > +	 * On discrete platforms, starting from DG2, we have to contend with GTT
> > +	 * page size restrictions when dealing with I915_MEMORY_CLASS_DEVICE
> > +	 * objects.  Specifically the hardware only supports 64K or larger GTT
> > +	 * page sizes for such memory. The kernel will already ensure that all
> > +	 * I915_MEMORY_CLASS_DEVICE memory is allocated using 64K or larger page
> > +	 * sizes underneath.
> > +	 *
> > +	 * Note that the returned size here will always reflect any required
> > +	 * rounding up done by the kernel, i.e 4K will now become 64K on devices
> > +	 * such as DG2.
> > +	 *
> > +	 * **Special DG2 GTT address alignment requirement:**
> > +	 *
> > +	 * The GTT alignment will also need be at least 2M for  such objects.
> > +	 *
> > +	 * Note that due to how the hardware implements 64K GTT page support, we
> > +	 * have some further complications:
> > +	 *
> > +	 *   1) The entire PDE(which covers a 2MB virtual address range), must
> > +	 *   contain only 64K PTEs, i.e mixing 4K and 64K PTEs in the same
> > +	 *   PDE is forbidden by the hardware.
> > +	 *
> > +	 *   2) We still need to support 4K PTEs for I915_MEMORY_CLASS_SYSTEM
> > +	 *   objects.
> > +	 *
> > +	 * To keep things simple for userland, we mandate that any GTT mappings
> > +	 * must be aligned to and rounded up to 2MB. As this only wastes virtual
> > +	 * address space and avoids userland having to copy any needlessly
> > +	 * complicated PDE sharing scheme (coloring) and only affects GD2, this
> > +	 * id deemed to be a good compromise.
>
> typos: GD2, id

thanks

> Isn't much of this more relavent to the vma offset at exec time? Is
> there actually any new restriction on the size field during buffer
> creation?

No new restriction on size, just placement, which mesa is already doing.
The request for ack was just to get an ack from mesa folks that they are 
happy with the mandatory 2MB alignment for DG2 vma.

> I see Matthew references these notes from the offset comments, so if the
> kernel devs prefer it here, then you can add my Acked-by on this patch.

thanks!

> -Jordan
>
> >   	 */
> >   	__u64 size;
> >   	/**
> > --
> > 2.25.1