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

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

On 02/12/2024 16:33, Thomas Hellström wrote:
> On Mon, 2024-12-02 at 16:28 +0000, Matthew Auld wrote:
>> 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?
> 
> Yes that should work, although perhaps use ret >= 0 instead of
> !(ret < 0)
> 
> and ret == size in the second test?

Yup, that looks cleaner.

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