[PATCH 1/2] drm/ttm: add a way to bo_wait for either the last read or last write

[Thomas Hellstrom]

On 10/07/2011 03:38 PM, Jerome Glisse wrote:
> On Fri, Oct 7, 2011 at 4:00 AM, Thomas Hellstrom<thomas at shipmail.org>  wrote:
>    
>> On 10/07/2011 12:42 AM, Marek Olšák wrote:
>>      
>>> On Wed, Oct 5, 2011 at 7:54 AM, Thomas Hellstrom<thomas at shipmail.org>
>>>   wrote:
>>>
>>>        
>>>> 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.
>>>>
>>>>          
>>> I don't think we use it like that. To my knowledge, the purpose of the
>>> sync obj (to Radeon Gallium drivers at least) is to be able to wait
>>> for the last use of a buffer. Whether the contents can or cannot be
>>> available to the CPU is totally irrelevant.
>>>
>>> Currently (and it's a very important performance optimization),
>>> buffers stay mapped and available for CPU read/write during their
>>> first map_buffer call. Unmap_buffer is a no-op. The unmapping happens
>>> on buffer destruction. We only call bo_wait when we want to wait for
>>> the GPU until it's done with the buffer (we don't always want that,
>>> sometimes we want to use the unsychronized flag). Otherwise the
>>> contents of buffers are available at *any time*.
>>>
>>> We could probably implement bo_wait privately in the kernel driver and
>>> not use ttm_bo_wait. I preferred code sharing though.
>>>
>>> Textures (especially the tiled ones) are never mapped directly and a
>>> temporary staging resource is used instead, so we don't actually
>>> pollute address space that much. (in case you would have such a
>>> remark) We will use staging resources for buffers too, but it's really
>>> the last resort to avoid waiting when direct access can't be used.
>>>
>>>
>>>
>>>        
>>>>>> 2) 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?
>>>>
>>>>          
>>> There are lots of read-write use cases which don't need any barriers
>>> or flushing. The obvious ones are color blending and depth-stencil
>>> buffering. The OpenGL application is also allowed to use a subrange of
>>> a buffer as a vertex buffer (read-only) and another disjoint subrange
>>> of the same buffer for transform feedback (write-only), which kinda
>>> makes me think about whether we should track subranges instead of
>>> treating a whole buffer as "busy". It gets even more funky with
>>> ARB_shader_image_load_store, which supports atomic read-modify-write
>>> operations on textures, not to mention atomic memory operations in
>>> compute shaders (wait, isn't that also exposed in GL as
>>> GL_ARB_shader_atomic_counters?).
>>>
>>> I was thinking whether the two sync objs should be "read" and
>>> "readwrite", or "read" and "write". I chose the latter, because it's
>>> more fine-grained and we have to keep at least two of them around
>>> anyway.
>>>
>>> So now that you know what we use sync objs for, what are your ideas on
>>> re-implementing that patch in a way that is okay with you? Besides
>>> removing the third sync objs of course.
>>>
>>> Marek
>>>
>>>        
>> OK. First I think we need to make a distinction: bo sync objects vs driver
>> fences. The bo sync obj api is there to strictly provide functionality that
>> the ttm bo subsystem is using, and that follows a simple set of rules:
>>
>> 1) the bo subsystem does never assume sync objects are ordered. That means
>> the bo subsystem needs to wait on a sync object before removing it from a
>> buffer. Any other assumption is buggy and must be fixed. BUT, if that
>> assumption takes place in the driver unknowingly from the ttm bo subsystem
>> (which is usually the case), it's OK.
>>
>> 2) When the sync object(s) attached to the bo are signaled the ttm bo
>> subsystem is free to copy the bo contents and to unbind the bo.
>>
>> 3) The ttm bo system allows sync objects to be signaled in different ways
>> opaque to the subsystem using sync_obj_arg. The driver is responsible for
>> setting up that argument.
>>
>> 4) Driver fences may be used for or expose other functionality or adaptions
>> to APIs as long as the sync obj api exported to the bo sybsystem follows the
>> above rules.
>>
>> This means the following w r t the patch.
>>
>> A) it violates 1). This is a bug that must be fixed. Assumptions that if one
>> sync object is singnaled, another sync object is also signaled must be done
>> in the driver and not in the bo subsystem. Hence we need to explicitly wait
>> for a fence to remove it from the bo.
>>
>> B) the sync_obj_arg carries *per-sync-obj* information on how it should be
>> signaled. If we need to attach multiple sync objects to a buffer object, we
>> also need multiple sync_obj_args. This is a bug and needs to be fixed.
>>
>> C) There is really only one reason that the ttm bo subsystem should care
>> about multiple sync objects, and that is because the driver can't order them
>> efficiently. A such example would be hardware with multiple pipes reading
>> simultaneously from the same texture buffer. Currently we don't support this
>> so only the *last* sync object needs to be know by the bo subsystem. Keeping
>> track of multiple fences generates a lot of completely unnecessary code in
>> the ttm_bo_util file, the ttm_bo_vm file, and will be a nightmare if / when
>> we truly support pipelined moves.
>>
>> As I understand it from your patches, you want to keep multiple fences
>> around only to track rendering history. If we want to do that generically, i
>> suggest doing it in the execbuf util code in the following way:
>>
>> struct ttm_eu_rendering_history {
>>     void *last_read_sync_obj;
>>     void *last_read_sync_obj_arg;
>>     void *last_write_sync_obj;
>>     void *last_write_sync_obj_arg;
>> }
>>
>> Embed this structure in the radeon_bo, and build a small api around it,
>> including *optionally* passing it to the existing execbuf utilities, and you
>> should be done. The bo_util code and bo_vm code doesn't care about the
>> rendering history. Only that the bo is completely idle.
>>
>> Note also that when an accelerated bo move is scheduled, the driver needs to
>> update this struct
>>
>> /Thomas
>>      
> I should have look at the patch long ago ... anyway i think a better
> approach would be to expose fence id as 64bits unsigned to each
> userspace client. I was thinking of mapping a page readonly (same page
> as the one gpu write back) at somespecific offset in drm file (bit
> like sarea but readonly so no lock). Each time userspace submit a
> command stream it would get the fence id associated with the command
> stream. It would then be up to userspace to track btw read or write
> and do appropriate things. The kernel code would be simple (biggest
> issue is finding an offset we can use for that), it would be fast as
> no round trip to kernel to know the last fence.
>
> Each fence seq would be valid only for a specific ring (only future
> gpu impacted here, maybe cayman).
>
> So no change to ttm, just change to radeon to return fence seq and to
> move to an unsigned 64. Issue would be when gpu write back is
> disabled, then we would either need userspace to call somethings like
> bo wait or to other ioctl to get the kernel to update the copy, copy
> would be updated in the irq handler too so at least it get updated
> anytime something enable irq.
>
> Cheers,
> Jerome
>    

That also sounds like a feasible / good way.
Note that the driver determines the mapping offset of TTM address space. 
You can
just adjust that somewhat upwards and you'll have free space for an ro page.

/Thomas