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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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?

/Thomas



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