On Wed, Mar 24, 2010 at 09:08:08PM +0100, Thomas Hellstrom wrote:
> Jerome Glisse wrote:
> >On Wed, Mar 24, 2010 at 07:27:57PM +0100, Thomas Hellstrom wrote:
> >>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 <jgli...@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.
> >>
> >
> >use_count could become bool io_reserved it's atomic for patch historical
> >reason, will respawn a patch without atomic and blank line removal.
>
> Great. Please also consider the test for 0 on unreserve.
>
> >>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
> >
> >Idea is that buffer that will be mapped the whole time will also be
> >set with no evict so unmap virtual is never call on them (at least
> >that is my feeling from the code). So iomeme_reserve works also for
> >pinned buffer and i don't separate the pined/not pinned case from
> >the driver io manager (if driver has any).
>
> Yes, That's the case for simple io managers, where the mappable
> range is simply a (sub)TTM region. Then TTM NO_EVICT is equivalent
> to io manager NO_EVICT. However, if the IO manager is not a TTM
> region manager but something the driver implements with its own LRU
> list, the IO manager must be informed about this. Admitted, we have
> no driver like this yet, and the common code won't be using that
> API, so I'm OK with leaving it for now. I might add a comment about
> this close to the io manager hooks later on.
>
> >>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
> >>
> >
> >I didn't go the splitted way to avoid having a frontier btw
> >mappable & unmappable vram, i think it's better to avoid this
>
> Agreed. This was just a simple case to demonstrate. However, even
> with io_mem_reseve() we'd need to pass a bo. What's the reason for
> passing a mem here?
>
> >Hope being that not much buffer in vram need to be mapped
> >(best case being buffer never mapped ;))
> >
> >Cheers,
> >Jerome
>
> Thanks,
> Thomas
>
Reason for passing ttm_mem is ttm_bo_move_memcpy which might need
to map 2 different io mem associated to the same bo. Otherwise i
would have use bo as argument, thought maybe i can still pass bo
& mem so driver manager can retrieve bo information like NO_EVICT
and take approriate choice for its io management.
Thomas thanks a lot for reviewing, commenting & feedback.
Cheers,
Jerome
------------------------------------------------------------------------------
Download Intel® 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
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
