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