Re: [PATCH] drm/xe/display: fix ttm_bo_access() usage

[Matthew Auld <matthew.auld@intel.com>]

On 02/12/2024 15:56, Thomas Hellström wrote:
> On Mon, 2024-12-02 at 15:41 +0000, Matthew Auld wrote:
>> ttm_bo_access() returns the size on success. Account for that
>> otherwise
>> the caller incorrectly thinks this is an error in
>> intel_atomic_prepare_plane_clear_colors().
>>
>> Fixes: b6308aaa24a7 ("drm/xe/display: Update intel_bo_read_from_page
>> to use ttm_bo_access")
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3661
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> ---
>>   drivers/gpu/drm/xe/display/intel_bo.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/display/intel_bo.c
>> b/drivers/gpu/drm/xe/display/intel_bo.c
>> index 43141964f6f2..b2a8d06cc78a 100644
>> --- a/drivers/gpu/drm/xe/display/intel_bo.c
>> +++ b/drivers/gpu/drm/xe/display/intel_bo.c
>> @@ -40,8 +40,13 @@ int intel_bo_fb_mmap(struct drm_gem_object *obj,
>> struct vm_area_struct *vma)
>>   int intel_bo_read_from_page(struct drm_gem_object *obj, u64 offset,
>> void *dst, int size)
>>   {
>>   	struct xe_bo *bo = gem_to_xe_bo(obj);
>> +	int ret;
>>   
>> -	return ttm_bo_access(&bo->ttm, offset, dst, size, 0);
>> +	ret = ttm_bo_access(&bo->ttm, offset, dst, size, 0);
>> +	if (ret == size)
>> +		ret = 0;
>> +
>> +	return ret;
>>   }
>>   
> 
> Looking at how callers are using struct vm_operations_struct::access()
> it looks possible that 0 <= ret < size, indicating the number of bytes
> actually processed.
> 
> Now since ttm_bo_access() is actually a helper to implement the above,
> we strictly don't have to follow that for its interface, but I think it
> makes sense to assert that ret is either == size or something negative.
> Ideas?

hmm, what about if we add something like:

+int xe_bo_read(struct xe_bo *bo, u64 offset, void *dst, int size)
+{
+       int ret;
+
+       ret = ttm_bo_access(&bo->ttm, offset, dst, size, 0);
+       if (!(ret < 0) && ret != size)
+               ret = -EIO;
+       else if (!(ret < 0))
+               ret = 0;
+
+       return ret;
+}

And then both callers use that?

> 
> /Thomas
> 
> 
> 
>>   struct intel_frontbuffer *intel_bo_get_frontbuffer(struct
>> drm_gem_object *obj)
>