[Intel-xe] [RFC PATCH] drm/xe/uAPi: Clarify the meaning of min_page_size and MIN_ALIGNMENT

[Thomas Hellström]

On 10/16/23 13:30, Francois Dugast wrote:
> On Mon, Oct 16, 2023 at 01:03:07PM +0200, Thomas Hellström wrote:
>> Hi,
>>
>> On 10/13/23 22:16, Rodrigo Vivi wrote:
>>> On Fri, Oct 13, 2023 at 12:22:26PM +0200, Thomas Hellström wrote:
>>>> In particular, don't *enforce* alignment of buffer object sizes to
>>>> MIN_ALIGNMENT, as enforcing that for imported dma-bufs would not
>>>> really make sense.
>>>>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Cc: Matthew Brost <matthew.brost at intel.com>
>>>> Cc: Francois Dugast <francois.dugast at intel.com>
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>>> ---
>>>>    include/uapi/drm/xe_drm.h | 17 ++++++++++++-----
>>>>    1 file changed, 12 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>>> index d48d8e3c898c..5895c95879d1 100644
>>>> --- a/include/uapi/drm/xe_drm.h
>>>> +++ b/include/uapi/drm/xe_drm.h
>>>> @@ -167,11 +167,10 @@ struct drm_xe_query_mem_region {
>>>>    	 *
>>>>    	 * When the kernel allocates memory for this region, the
>>>>    	 * underlying pages will be at least @min_page_size in size.
>>>> -	 *
>>>> -	 * Important note: When userspace allocates a GTT address which
>>>> -	 * can point to memory allocated from this region, it must also
>>>> -	 * respect this minimum alignment. This is enforced by the
>>>> -	 * kernel.
>>> This old statement seems bold on the kernel is enforcing it. If we are
>>> but intend to not enforce anymore we should also change the code in
>>> the same patch, no?!
>>> Also by the commit message I had the impression that this was removing
>>> the enforcement, and not just the documentation about it.
>> This RFC was more intended to trigger a discussion on how to handle this.
>> I'll actually bring back the above somewhat reworded and also rework the
>> commit message a bit to reflect that this is an RFC for an uAPI change under
>> discussion.
>>
>>>> +	 * Buffer objects with an allowable placement in this region may
>>>> +	 * be created with a smaller size, but for increased performance,
>>>> +	 * user-space drivers should align sizes of such buffer objects
>>>> +	 * to this value whenever possible.
>>>>    	 */
>>>>    	__u32 min_page_size;
>>>>    	/**
>>>> @@ -252,6 +251,14 @@ struct drm_xe_query_config {
>>>>    #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
>>>>    #define XE_QUERY_CONFIG_FLAGS			1
>>>>    	#define XE_QUERY_CONFIG_FLAGS_HAS_VRAM		(0x1 << 0)
>>>> +/*
>>>> + * XE_QUERY_CONFIG_MIN_ALIGNMENT - GPU VM bind address - and size alignment.
>>>> + * GPU virtual address mappings of buffer objects must have their VM addr and
>>>> + * range aligned to this value, with the exception of buffer objects with
>>>> + * allowable placement in system memory only. If this alignment results in
>>>> + * a mapping beyond the end of the buffer object, accesses beyond the
>>>> + * end of the buffer object have undefined reslut.
>>> s/reslut/result.
>>>
>>> Should we make this into a kernel doc?
>> It looks like all other defines of constants in this file doesn't use
>> kerneldoc. If we want to do that, It looks like we need to break this out to
>> a top-level DOC: section, similar to what's done for
>> XE_RESET_FAILED_UEVENT.
>>
>> /Thomas
>>
>>
> Hi Thomas,
>
> A proposal to make constants visible in kernel doc is part of the
> uAPI alignment series take 2:
> https://patchwork.freedesktop.org/patch/562078/?series=124977&rev=1
>
> Francois

Ah, thanks Francois, I'll take a look.

/Thomas