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

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

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?

Thanks,
Thomas



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