op 12-11-13 22:49, Thomas Hellstrom schreef:
On 11/12/2013 07:26 PM, Maarten Lankhorst wrote:
Most drivers have refcounting done in gem, so lets get rid of another refcounting layer. ;) It has been confusing to keep track of 2 refcounts, so lets just let the driver worry about refcounting, and keep it hidden from ttm entirely. The core doesn't need to know about the refcounting anywhere.
Vmwgfx has 1 bo that doesn't use vmw_dma_buffer, but instead ttm_bo_create. Converting this call makes every bo use vmw_dma_buffer as base, which means I can simply add refcount to vmw_dma_buffer.
Mostly meant as a RFC, so I only took effort into converting vmwgfx, radeon, nouveau. Thoughts?
Hmm. I don't really see the purpose of this?
With the patches that embed a GEM object I think it's stupid to keep another refcount. When I was playing with TTM in nouveau it was very unclear for me when an object is truly dead due to the various not-dead stages. I want to get rid of this confusion and either have a ttm object in a alive state, with the gem object still alive too, or in a dying state when ttm_bo_release is called and delayed destroy is used.
First the ttm bo reference is used by the vm system, so you need to duplicate a lot of vm stuff across all drivers, which is bad because if something needs to change here, we need to change it in all drivers. Seems you've forgotten qxl, cirrus, mgag200 and ast mmap() here?
"Mostly meant as a RFC, so I only took effort into converting vmwgfx, radeon, nouveau."
I inlined ttm_bo_mmap because extending the callbacks with an ops structure parameter was more complicated. Now that everything is done with the drm_vma_offset helpers I don't expect big changes here.
The fault handler is still in core ttm, and radeon was already wrapping it poorly. Implementing it in the driver means core ttm does not need to worry about refcounting any more, any refcounting is now entirely up to the drivers.
Second, the vmwgfx driver relies so much on ttm refcounting that you needed to re-add it for this driver, and actually will rely even more on bare ttm objects in our upcoming hardware revision where they are used as page table bos.
Where have I heard this before? :P I don't believe page table bo's would need refcounting anyway, so just call ttm_bo_release. If you really do need refcounting on them just create a vmw_dma_buffer bo again.
Finally, it looks to me like the gain in the gem drivers can be accomplished by just implementing ttm_bo_release() on top of ttm_bo_unref(), and leave the vm system alone. Sure, you'll add an extra atomic operation on object destruction, but that's not a high price to pay...
And extra memory usage, and keeping the confusion of when to use a ttm or a gem refcount.