On 10/05/2011 04:08 AM, Marek Olšák wrote:
On Tue, Oct 4, 2011 at 1:48 PM, Thomas Hellstromthomas@shipmail.org wrote:
Bah, I totally missed this patch and thus never reviewed it :( Probably on vacation.
There are a couple of things I'd like to point out.
- The bo subsystem may never assume that fence objects are ordered, so that
when we unref bo::sync_obj, we may never assume that previously attached fence objects are signaled and can be unref'd Think for example fence objects submitted to different command streams. This is a bug and must be fixed.
If what you say is true, then even the original sync_obj can't be trusted. What if I overwrite sync_obj with a new one and the new one is signalled sooner than the old one?
The driver validation code will in effect overwrite the old with a new one, because the driver validation code knows what sync objects are ordered. If, during validation of a buffer object, the driver validation code detects that the buffer is already fenced with a sync object that will signal out-of-order, the driver validation code needs to *wait* for that sync object to signal before proceeding, or insert a sync object barrier in the command stream.
The TTM bo code doesn't know how to order fences, and never assumes that they are ordered.
We can detach fence objects from buffers in the driver validation code, because that code knows whether fences are implicitly ordered, or can order them either by inserting a barrier (semaphore in NV languange) or waiting
I am not sure I follow you here. ttm_bo_wait needs the fences... unless we want to move the fences out of TTM into drivers.
Please see the above explanation.
for the fence to expire. (For example if the new validation is READ and the fence currently attached is WRITE, we might need to schedule a gpu cache flush before detaching the write fence).
I am not sure what fences have to do with flushing. Write caches should be flushed automatically when resources are unbound. When a resource is used for write and read at the same time, it's not our problem: the user is responsible for flushing (e.g. through memory and texture barriers in OpenGL), not the driver.
How flushing is done is up to the driver writer, (fences is an excellent tool to do it in an efficient way), but barriers like the write-read barrier example above may need to be inserted for various reasons. Let's say you use render-to-texture, unbind the texture from the fbo, and then want to texture from it. At some point you *need* to flush if you have a write cache, and that flush needs to happen when you remove the write fence from the buffer, in order to replace it with a read fence, since after that the information that the buffer has been written to is gone. IIRC nouveau uses barriers like this to order fences from different command streams, Unichrome used it to order fences from different hardware engines.
In any case, I'm not saying fences is the best way to flush but since the bo code assumes that signaling a sync object means "make the buffer contents available for CPU read / write", it's usually a good way to do it; there's even a sync_obj_flush() method that gets called when a potential flush needs to happen.
- Can't we say that a write_sync_obj is simply a sync_obj? What's the
difference between those two? I think we should remove the write_sync_obj bo member.
Okay, but I think we should remove sync_obj instead, and keep write and read sync objs. In the case of READWRITE usage, read_sync_obj would be equal to write_sync_obj.
Sure, I'm fine with that.
One other thing, though, that makes me a little puzzled:
Let's assume you don't allow readers and writers at the same time, which is my perception of how read- and write fences should work; you either have a list of read fences or a single write fence (in the same way a read-write lock works).
Now, if you only allow a single read fence, like in this patch. That implies that you can only have either a single read fence or a single write fence at any one time. We'd only need a single fence pointer on the bo, and sync_obj_arg would tell us whether to signal the fence for read or for write (assuming that sync_obj_arg was set up to indicate read / write at validation time), then the patch really isn't necessary at all, as it only allows a single read fence?
Or is it that you want to allow read- and write fences co-existing? In that case, what's the use case?
Thanks, Thomas
Marek