Re: [RFC PATCH] drm/ttm: Do page counting after populate callback succeed

[Christian König <christian.koenig@xxxxxxx>]

Am 15.06.21 um 14:11 schrieb Pan, Xinhui:
> > 2021年6月15日 20:01，Christian König <ckoenig.leichtzumerken@xxxxxxxxx> 写道：
> >
> > Am 15.06.21 um 13:57 schrieb xinhui pan:
> > > Amdgpu set SG flag in populate callback. So TTM still count pages in SG
> > > BO.
> > It's probably better to fix this instead. E.g. why does amdgpu modify the SG flag during populate and not during initial creation? That doesn't seem to make sense.
> fair enough. Let me have a try.
> No idea why we set SG flag in populate years ago.

That's pretty recent IIRC. Felix moved the code around a bit to fix 
another problem.

Christian.

> > Christian.
> >
> > > One easy way to fix this is lets count pages after populate callback.
> > >
> > > We hit one issue that amdgpu alloc many SG BOs, but TTM try to do swap
> > > again and again even if swapout does not swap SG BOs at all.
> > >
> > > Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/ttm/ttm_tt.c | 32 +++++++++++++-------------------
> > >   1 file changed, 13 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> > > index a1a25410ec74..4fa0a8cd71c0 100644
> > > --- a/drivers/gpu/drm/ttm/ttm_tt.c
> > > +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> > > @@ -317,13 +317,6 @@ int ttm_tt_populate(struct ttm_device *bdev,
> > >   	if (ttm_tt_is_populated(ttm))
> > >   		return 0;
> > >   -	if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > > -		atomic_long_add(ttm->num_pages, &ttm_pages_allocated);
> > > -		if (bdev->pool.use_dma32)
> > > -			atomic_long_add(ttm->num_pages,
> > > -					&ttm_dma32_pages_allocated);
> > > -	}
> > > -
> > >   	while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit ||
> > >   	       atomic_long_read(&ttm_dma32_pages_allocated) >
> > >   	       ttm_dma32_pages_limit) {
> > > @@ -342,6 +335,13 @@ int ttm_tt_populate(struct ttm_device *bdev,
> > >   	if (ret)
> > >   		goto error;
> > >   +	if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > > +		atomic_long_add(ttm->num_pages, &ttm_pages_allocated);
> > > +		if (bdev->pool.use_dma32)
> > > +			atomic_long_add(ttm->num_pages,
> > > +					&ttm_dma32_pages_allocated);
> > > +	}
> > > +
> > >   	ttm_tt_add_mapping(bdev, ttm);
> > >   	ttm->page_flags |= TTM_PAGE_FLAG_PRIV_POPULATED;
> > >   	if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> > > @@ -355,12 +355,6 @@ int ttm_tt_populate(struct ttm_device *bdev,
> > >   	return 0;
> > >     error:
> > > -	if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > > -		atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> > > -		if (bdev->pool.use_dma32)
> > > -			atomic_long_sub(ttm->num_pages,
> > > -					&ttm_dma32_pages_allocated);
> > > -	}
> > >   	return ret;
> > >   }
> > >   EXPORT_SYMBOL(ttm_tt_populate);
> > > @@ -384,12 +378,6 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm)
> > >   	if (!ttm_tt_is_populated(ttm))
> > >   		return;
> > >   -	ttm_tt_clear_mapping(ttm);
> > > -	if (bdev->funcs->ttm_tt_unpopulate)
> > > -		bdev->funcs->ttm_tt_unpopulate(bdev, ttm);
> > > -	else
> > > -		ttm_pool_free(&bdev->pool, ttm);
> > > -
> > >   	if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > >   		atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> > >   		if (bdev->pool.use_dma32)
> > > @@ -397,6 +385,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm)
> > >   					&ttm_dma32_pages_allocated);
> > >   	}
> > >   +	ttm_tt_clear_mapping(ttm);
> > > +	if (bdev->funcs->ttm_tt_unpopulate)
> > > +		bdev->funcs->ttm_tt_unpopulate(bdev, ttm);
> > > +	else
> > > +		ttm_pool_free(&bdev->pool, ttm);
> > > +
> > >   	ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED;
> > >   }
> > >