Re: [PATCH 1/8] drm/ttm: remove TTM_MEMTYPE_FLAG_CMA

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 16.07.20 um 15:05 schrieb Thomas Zimmermann:
> Hi,
>
> this patchset could have benefited from a cover letter.

Yeah, I didn't thought it would result in such an extensive patch set.

> Am 16.07.20 um 14:50 schrieb Christian König:
> > The original intention was to avoid CPU page table unmaps
> > when BOs move between the GTT and SYSTEM domain.
> >
> > The problem is that this never correctly handled changes
> > in the caching attributes or backing pages.
> >
> > Just drop this for now and simply unmap the CPU page
> > tables in all cases.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c    |  4 +--
> >   drivers/gpu/drm/nouveau/nouveau_bo.c       |  3 +-
> >   drivers/gpu/drm/radeon/radeon_ttm.c        |  2 +-
> >   drivers/gpu/drm/ttm/ttm_bo.c               | 34 ++++------------------
> >   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c |  2 +-
> >   include/drm/ttm/ttm_bo_driver.h            |  1 -
> >   6 files changed, 11 insertions(+), 35 deletions(-)
> Why's CMA cleaned up in one patch and MAPPABLE in the other 7?

There is a chance that the CMA removal fixes some bugs and needs to be 
back-ported. But the other 7 are pure cleanup.

> Wouldn't it have been better to kill both the flags from ttm in 2
> patches, then have one patch per driver and finally remove both flags
> from the ttm header?

That's what I did initially. Basically I found that the 
TTM_MEMTYPE_FLAG_CMA flag might be the root of some problems because of 
the problematic implementation.

While removing it I've found that the TTM_MEMTYPE_FLAG_MAPPABLE is 
completely unused in TTM as well, so I removed both at the same time.

But that turned out to break drivers because they use the 
TTM_MEMTYPE_FLAG_MAPPABLE flag for no good reason at all :)

Anyway, I want to keep the order now since it might be a good idea to 
back port the CMA flag removal (but I'm not 100% sure of that).

Thanks,
Christian.

> Best regards
> Thomas
>
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > index 9c0f12f74af9..44fa8bc49d18 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> > @@ -93,7 +93,7 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >   		man->func = &amdgpu_gtt_mgr_func;
> >   		man->available_caching = TTM_PL_MASK_CACHING;
> >   		man->default_caching = TTM_PL_FLAG_CACHED;
> > -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
> > +		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >   		break;
> >   	case TTM_PL_VRAM:
> >   		/* "On-card" video ram */
> > @@ -108,7 +108,7 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >   	case AMDGPU_PL_OA:
> >   		/* On-chip GDS memory*/
> >   		man->func = &ttm_bo_manager_func;
> > -		man->flags = TTM_MEMTYPE_FLAG_FIXED | TTM_MEMTYPE_FLAG_CMA;
> > +		man->flags = TTM_MEMTYPE_FLAG_FIXED;
> >   		man->available_caching = TTM_PL_FLAG_UNCACHED;
> >   		man->default_caching = TTM_PL_FLAG_UNCACHED;
> >   		break;
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > index a1037478fa3f..7883341f8c83 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > @@ -695,8 +695,7 @@ nouveau_bo_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >   				TTM_PL_FLAG_WC;
> >   			man->default_caching = TTM_PL_FLAG_WC;
> >   		} else {
> > -			man->flags = TTM_MEMTYPE_FLAG_MAPPABLE |
> > -				     TTM_MEMTYPE_FLAG_CMA;
> > +			man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >   			man->available_caching = TTM_PL_MASK_CACHING;
> >   			man->default_caching = TTM_PL_FLAG_CACHED;
> >   		}
> > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
> > index 73085523fad7..54af06df865b 100644
> > --- a/drivers/gpu/drm/radeon/radeon_ttm.c
> > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c
> > @@ -84,7 +84,7 @@ static int radeon_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >   		man->func = &ttm_bo_manager_func;
> >   		man->available_caching = TTM_PL_MASK_CACHING;
> >   		man->default_caching = TTM_PL_FLAG_CACHED;
> > -		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
> > +		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >   #if IS_ENABLED(CONFIG_AGP)
> >   		if (rdev->flags & RADEON_IS_AGP) {
> >   			if (!rdev->ddev->agp) {
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 8b9e7f62bea7..0768a054a916 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -272,20 +272,15 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
> >   				  struct ttm_operation_ctx *ctx)
> >   {
> >   	struct ttm_bo_device *bdev = bo->bdev;
> > -	bool old_is_pci = ttm_mem_reg_is_pci(bdev, &bo->mem);
> > -	bool new_is_pci = ttm_mem_reg_is_pci(bdev, mem);
> >   	struct ttm_mem_type_manager *old_man = &bdev->man[bo->mem.mem_type];
> >   	struct ttm_mem_type_manager *new_man = &bdev->man[mem->mem_type];
> > -	int ret = 0;
> > +	int ret;
> >   
> > -	if (old_is_pci || new_is_pci ||
> > -	    ((mem->placement & bo->mem.placement & TTM_PL_MASK_CACHING) == 0)) {
> > -		ret = ttm_mem_io_lock(old_man, true);
> > -		if (unlikely(ret != 0))
> > -			goto out_err;
> > -		ttm_bo_unmap_virtual_locked(bo);
> > -		ttm_mem_io_unlock(old_man);
> > -	}
> > +	ret = ttm_mem_io_lock(old_man, true);
> > +	if (unlikely(ret != 0))
> > +		goto out_err;
> > +	ttm_bo_unmap_virtual_locked(bo);
> > +	ttm_mem_io_unlock(old_man);
> >   
> >   	/*
> >   	 * Create and bind a ttm if required.
> > @@ -1698,23 +1693,6 @@ EXPORT_SYMBOL(ttm_bo_device_init);
> >    * buffer object vm functions.
> >    */
> >   
> > -bool ttm_mem_reg_is_pci(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> > -{
> > -	struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
> > -
> > -	if (!(man->flags & TTM_MEMTYPE_FLAG_FIXED)) {
> > -		if (mem->mem_type == TTM_PL_SYSTEM)
> > -			return false;
> > -
> > -		if (man->flags & TTM_MEMTYPE_FLAG_CMA)
> > -			return false;
> > -
> > -		if (mem->placement & TTM_PL_FLAG_CACHED)
> > -			return false;
> > -	}
> > -	return true;
> > -}
> > -
> >   void ttm_bo_unmap_virtual_locked(struct ttm_buffer_object *bo)
> >   {
> >   	struct ttm_bo_device *bdev = bo->bdev;
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> > index bfd0c54ec30a..6bea7548aee0 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c
> > @@ -762,7 +762,7 @@ static int vmw_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
> >   		 *  slots as well as the bo size.
> >   		 */
> >   		man->func = &vmw_gmrid_manager_func;
> > -		man->flags = TTM_MEMTYPE_FLAG_CMA | TTM_MEMTYPE_FLAG_MAPPABLE;
> > +		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
> >   		man->available_caching = TTM_PL_FLAG_CACHED;
> >   		man->default_caching = TTM_PL_FLAG_CACHED;
> >   		break;
> > diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> > index 45522e4fbd6b..71b195e78c7c 100644
> > --- a/include/drm/ttm/ttm_bo_driver.h
> > +++ b/include/drm/ttm/ttm_bo_driver.h
> > @@ -47,7 +47,6 @@
> >   
> >   #define TTM_MEMTYPE_FLAG_FIXED         (1 << 0)	/* Fixed (on-card) PCI memory */
> >   #define TTM_MEMTYPE_FLAG_MAPPABLE      (1 << 1)	/* Memory mappable */
> > -#define TTM_MEMTYPE_FLAG_CMA           (1 << 3)	/* Can't map aperture */
> >   
> >   struct ttm_mem_type_manager;
> >   

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel