Re: [PATCH v12 06/10] drm/ttm/tests: Add tests with mock resource managers

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

On Mon, 2024-06-03 at 10:28 +0200, Karolina Stolarek wrote:
> > > > > diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> > > > > b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> > > > > index 2f590bae53f8..2a2585b37118 100644
> > > > > --- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> > > > > +++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
> > > > > @@ -27,8 +27,42 @@ static int mock_move(struct
> > > > > ttm_buffer_object
> > > > > *bo,
> > > > > bool evict,
> > > > >    		     struct ttm_resource *new_mem,
> > > > >    		     struct ttm_place *hop)
> > > > >    {
> (...)
> > > > > +
> > > > > +		if (ret)
> > > > > +			return ret;
> > > > > +
> > > > > +		ttm_resource_free(bo, &bo->resource);
> > > > > +		ttm_bo_assign_mem(bo, new_mem);
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	return ttm_bo_move_memcpy(bo, ctx, new_mem);
> > > > 
> > > > Do we hit this move_memcpy()? Since the mock manager doesn't
> > > > actually
> > > > reserve any memory to manager, I'd expect this to run into
> > > > problems?
> > > 
> > > We do. The mock manager has use_tt=true, so on move, we'd use
> > > ttm_kmap_iter_tt_init() for src and dest and copy the pages. I'm
> > > not
> > > sure if that's the right approach, but it enables me to test if
> > > ttm_operation_ctx's bytes_moved is correctly updated.
> > 
> > Ah, ok. It's probably not a common use-case since with both src and
> > dst
> > having use_tt, IIRC ttm should keep the pages and their content
> > mostly
> > intact across a move. So you would memcpy the source on itself?
> > 
> > But it would give some coverage of the copy code though.
> 
> I dug around and it looks like, in the current scenario, 
> ttm_bo_move_memcpy() is just ttm_bo_move_sync_cleanup() 
> (ttm_resource_free + ttm_bo_assign_mem). That means I should revisit
> the 
> definitions of move and mock manager... I'll try to simplify them.
> 
> Do I understand correctly that we'd prefer to have a mock manager
> with 
> user_tt=false?

Yes, but then you need to allocate a chunk of contigous memory for the
mock manager to manage. And instead of using drm_buddy you'd have to
use drm_mm to manage it, since the ttm_kmap_iter default iterators can
only handle either
a) Contigous memory regions as returned from the drm_mm manager.
b) Fragmented memory regions as returned from the drm_buddy manager,
but in that case, they currently only handle pci io memory.

So I'd suggest to go with the current code and mark as a TODO: to
implement a) above.

/Thomas


> 
> All the best,
> Karolina
> 
> > 
> > /Thomas