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

[Thomas Hellström]

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