Re: [PATCH] drm/ttm: ttm_fault callback to allow driver to handle bo placement V3

[Thomas Hellstrom <thomas@shipmail.org>]

Jerome Glisse wrote:
> On fault the driver is given the opportunity to perform any operation
> it sees fit in order to place the buffer into a CPU visible area of
> memory. This patch doesn't break TTM users, nouveau, vmwgfx and radeon
> should keep working properly. Future patch will take advantage of this
> infrastructure and remove the old path from TTM once driver are
> converted.
>
> V2 return VM_FAULT_NOPAGE if callback return -EBUSY or -ERESTARTSYS
> V3 balance io_mem_reserve and io_mem_free call, fault_reserve_notify
>    is responsible to perform any necessary task for mapping to succeed
>
> Signed-off-by: Jerome Glisse <jglisse@redhat.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c      |    7 ++-
>  drivers/gpu/drm/ttm/ttm_bo_util.c |   95 ++++++++++++++++++-------------------
>  drivers/gpu/drm/ttm/ttm_bo_vm.c   |   46 ++++++++----------
>  include/drm/ttm/ttm_bo_api.h      |   21 ++++++++
>  include/drm/ttm/ttm_bo_driver.h   |   16 ++++++-
>  5 files changed, 108 insertions(+), 77 deletions(-)
>
> @@ -1588,8 +1591,8 @@ void ttm_bo_unmap_virtual(struct ttm_buffer_object *bo)
>  
>  	if (!bdev->dev_mapping)
>  		return;
> -
>   

Still a lot of these. Please remove.
>  
> +int ttm_mem_io_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> +{
> +	int ret;
> +
> +	if (bdev->driver->io_mem_reserve) {
> +		if (!atomic_xchg(&mem->bus.use_count, 1)) {
> +			ret = bdev->driver->io_mem_reserve(bdev, mem);
> +			if (unlikely(ret != 0))
> +				return ret;
> +		}
> +	} else {
> +		ret = ttm_bo_pci_offset(bdev, mem, &mem->bus.base, &mem->bus.offset, &mem->bus.size);
> +		if (unlikely(ret != 0))
> +			return ret;
> +		mem->bus.is_iomem = (mem->bus.size > 0) ? 1 : 0;
> +	}
> +	return 0;
> +}
> +
> +void ttm_mem_io_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem)
> +{
> +	if (bdev->driver->io_mem_reserve) {
> +		atomic_set(&mem->bus.use_count, 0);
>   

Shouldn't there be a test for zero before calling this?
> +		bdev->driver->io_mem_free(bdev, mem);
> +	}
> +}
> +
>   

Hmm. I don't get the logic of the above refcounting. First, the kernel 
can preempt between refcounting and driver calls:

Thread a sets use_count to 1 and preempts.
Thread b sees use_count 1 and calls ttm_bo_pci_offset(), but we haven't 
yet done an io_mem_reserve??

Otoh, from the last section it seems we always will hold bo::reserved 
around these calls, so instead we could make use_count a non-atomic 
variable.

Then, for the rest please consider the following use cases:

1) We want to temporarily map a bo within the kernel. We do:
reserve_bo().
make_mappable().
kmap().
kunmap().
free_mappable_resources().  // This is just a hint. When the bo is 
unreserved, the manager is free to evict the mappable region.
unreserve_bo().

2) We want to permanently map a bo within the kernel (kernel map for fbdev).
We do (bo is not reserved).
pin_mappable().
kmap().
access
kunmap().
unpin_mappable().

3) Fault.
reserve_bo().
make_mappable().
set_up user_space_map().
unreserve_bo().

/// Here the manager is free to evict the mappable range by reserving 
and then calling ttm_bo_unmap_virtual().

4) Unmap Virtual. (Called reserved).
unmap_user_space_mappings().
free_mappable_resources().

It looks to me like you've implemented make_mappable() = 
ttm_mem_io_reserve() and free_mappable_resources() = ttm_mem_io_free(), 
and from the above use cases we can see that they always will be called 
when the bo is reserved. Hence no need for an atomic variable and we can 
ignore the race scenarios above.

but what about pin_mappable() and unpin_mappable()? A general mapping 
manager must be able to perform these operations. Perhaps



Finally, consider a very simple mapping manager that uses the two ttm 
regions VRAM and VRAM_MAPPABLE. We'd implement the driver operations as 
follows, assuming we add io_mem_pin and io_mem_unpin:

io_mem_reserve:
ttm_bo_validate(VRAM_MAPPABLE); (Evicting as needed).

io_mem_unreserve:
noop().

io_mem_pin:
ttm_bo_reserve()
if (pin_count++ == 0)
   ttm_bo_validate(VRAM_MAPPABLE | NO_EVICT);
ttm_bo_unreserve()

io_mem_unpin:
ttm_bo_reserve()
if (--pin_count == 0)
   ttm_bo_validate(VRAM_MAPPABLE);
ttm_bo_unreserve()

This simple mapping manager would need a struct ttm_buffer_object as an 
argument. Not just the mem argument.

/Thomas




------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
--