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

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

On Mon, 2024-12-02 at 17:01 +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().
> 
> v2 (Thomas)
>  - Make sure we check for the partial copy case. Also since this api
> is
>    easy to get wrong, wrap the whole thing in a new helper to hide
> the
>    details and then convert the existing users over.
> 
> 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>

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

> ---
>  drivers/gpu/drm/xe/display/intel_bo.c |  2 +-
>  drivers/gpu/drm/xe/xe_bo.c            | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_bo.h            |  1 +
>  drivers/gpu/drm/xe/xe_vm.c            |  8 ++------
>  4 files changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/display/intel_bo.c
> b/drivers/gpu/drm/xe/display/intel_bo.c
> index 43141964f6f2..b463f5bd4eed 100644
> --- a/drivers/gpu/drm/xe/display/intel_bo.c
> +++ b/drivers/gpu/drm/xe/display/intel_bo.c
> @@ -41,7 +41,7 @@ int intel_bo_read_from_page(struct drm_gem_object
> *obj, u64 offset, void *dst, i
>  {
>  	struct xe_bo *bo = gem_to_xe_bo(obj);
>  
> -	return ttm_bo_access(&bo->ttm, offset, dst, size, 0);
> +	return xe_bo_read(bo, offset, dst, size);
>  }
>  
>  struct intel_frontbuffer *intel_bo_get_frontbuffer(struct
> drm_gem_object *obj)
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index f51d86511cb9..af85e5b8895f 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1320,6 +1320,30 @@ static int xe_bo_vm_access(struct
> vm_area_struct *vma, unsigned long addr,
>  	return ret;
>  }
>  
> +/**
> + * xe_bo_read() - Read from an xe_bo
> + * @bo: The buffer object to read from.
> + * @offset: The byte offset to start reading from.
> + * @dst: Location to store the read.
> + * @size: Size in bytes for the read.
> +
> + * Read @size bytes from the @bo, starting from @offset, storing
> into @dst.
> + *
> + * Return: Zero on success, or negative error.
> + */
> +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 == size)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
>  static const struct vm_operations_struct xe_gem_vm_ops = {
>  	.fault = xe_gem_fault,
>  	.open = ttm_bo_vm_open,
> diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
> index d0dce44317c7..d9386ab03140 100644
> --- a/drivers/gpu/drm/xe/xe_bo.h
> +++ b/drivers/gpu/drm/xe/xe_bo.h
> @@ -228,6 +228,7 @@ xe_bo_ggtt_addr(struct xe_bo *bo)
>  
>  int xe_bo_vmap(struct xe_bo *bo);
>  void xe_bo_vunmap(struct xe_bo *bo);
> +int xe_bo_read(struct xe_bo *bo, u64 offset, void *dst, int size);
>  
>  bool mem_type_is_vram(u32 mem_type);
>  bool xe_bo_is_vram(struct xe_bo *bo);
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 2492750505d6..7788680da4e5 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -3321,12 +3321,8 @@ void xe_vm_snapshot_capture_delayed(struct
> xe_vm_snapshot *snap)
>  		}
>  
>  		if (bo) {
> -			err = ttm_bo_access(&bo->ttm, snap-
> >snap[i].bo_ofs,
> -					    snap->snap[i].data,
> snap->snap[i].len, 0);
> -			if (!(err < 0) && err != snap->snap[i].len)
> -				err = -EIO;
> -			else if (!(err < 0))
> -				err = 0;
> +			err = xe_bo_read(bo, snap->snap[i].bo_ofs,
> +					 snap->snap[i].data, snap-
> >snap[i].len);
>  		} else {
>  			void __user *userptr = (void __user
> *)(size_t)snap->snap[i].bo_ofs;
>