On 05/19/2010 10:13 PM, Daniel Vetter wrote: > On Wed, May 19, 2010 at 09:51:33PM +0200, Thomas Hellstr?m wrote: > >> Daniel, >> TTM releases the spinlock protecting the drm_mm manager in between >> evictions to be able to wait (without holding locks) for bo idle. >> That means that the lru list may have changed between the first >> eviction and the next. In practice, drivers either don't allow >> simultaneous bo validation or should disallow it if thrashing is >> likely to occur, so the likelyhood of the lru list changing in >> between evictions should be small but it needs to be taken into >> account. >> > Well, in my opinion ttm locking optimize for the wrong case: Usually there > should be a little bit of room free to fully pipeline everything. And if > it there is no room free anymore, performance is gonna dip anyway, so we > might as well block.
Yes, the driver is free to block in such cases if it wants to. It's a matter of taking a command submission rwsem in either read- or write mode. However, a validation sequence of one DRI client shouldn't unnecessarily block other renderers whose render targets / sources are already in memory, but that's only a special case. TTM tries to make sure and encourage driver writers make sure no CPU locks are held while waiting for a GPU, which may turn out to be optimizing for the wrong case in a single-client-single-gpu environment, but turn out to be a good choice in other situations. In any case, the code must take into account that the lru may be modified when the spinlock is released, which I believe you have addressed below. > I think the linux vm with the split between > asynchronous background writeback and synchronous writeback in case of low > amounts of free memory could be used as inspiration. > > Whatever, I think ttm could use this fair eviction stuff even with the > current locking scheme: Do all the accounting under the spinlock, i.e. > - scanning the lru > - building up the eviction list > - mark the memory blocks as free (with drm_mm_put_block) > - reserve the complete free hole (needs a new function in drm_mm, but > range-restricted allocations are not yet implemented yet, anyway) with a > temporary object. > > Then do the effective eviction outside the spinlock. iirc ttm already uses > such shadow (dunno what they're really called) objects for buffer moves. > > >> Nevertheless, the drm_mm.c cleanup is >> Acked-by: Thomas Hellstrom<thellstrom at vmwgfx.com> >> > Does that include the drm_mm_node->private pointer removal? Jerome naked > that one (but I've objected). Just to clarify. > > IIRC, Jerome's range validation patches was using that member at some stage. I think if Jerome needs the pointer it should stay. Thanks, Thomas >> I'll leave it to the Intel guys to comment on the fair eviction stuff. >> >> /Thomas >> > Thanks, Daniel >
