[Mesa-dev] [PATCH 00/12] RadeonSI: Prevent IB submissions with illegal memory usage

[Nicolai Hähnle]

On 29.07.2016 23:42, Marek Olšák wrote:
> This series ensures that memory usage of gfx IBs is below the kernel-
> exposed memory limits in most cases.
>
> It's not possible to prevent CS ioctl failures completely because it
> depends on whether continuous free space for all buffers is available,
> i.e. it depends on where some system buffers are pinned, rather than
> how much total free space you have.
>
> Piglit max-texture-size and tex3d-maxsize tests easily pass with this.
>
> There are also some cleanups.

Reading through the series, I believe that the individual patches are 
fine (other than some bike-shedding perhaps), but it may be time to 
change the CS submission approach into more of a 
transaction-with-commit/rollback one. This is mostly because the whole 
dance around r600_context_add_resource_size, radeon_add_to_buffer_list 
and radeon_add_to_buffer_list_check_mem just feels very fragile.

Basically, radeon_winsys_cs would have a "committed" pointer into the IB 
in addition to the "current" pointer. Similarly, the buffer list would 
have a "committed" index. Winsys callbacks cs_commit and cs_rollback 
would do the obvious thing of moving the committed pointers to current 
or the other way around. (On second thought, it's probably only the 
buffer list that needs "committed" pointers.)

Then, the buffer handling in the driver code would be simplified as follows:

- As long as the CS already contains some committed draw/DMA call, 
radeon_add_to_buffer_list always checks whether the memory size is 
exceeded. If so, it calls cs_rollback, flushes, and returns a 
corresponding code.

- There is no need to distinguish radeon_add_to_buffer_list and 
radeon_add_to_buffer_list_check_mem anymore, since all additions are 
checked if and only if there really is something to flush.

- There is no more need for r600_context_add_resource_size. Instead, all 
draw/DMA-related buffers (vertex & index buffers, indirect data, DMA 
buffers) are added via radeon_add_to_buffer_list just before the call to 
si_need_cs_space. If radeon_add_to_buffer_list indicates that a flush 
happened, the sequence is repeated.

- After each draw call, cs_commit is called.

Another, tangentially related issue: we really shouldn't add buffers 
tied to descriptors in the *_begin_new_cs and even the set_* functions; 
instead, it should ideally happen at si_upload_descriptors time. 
Otherwise, we may end up adding buffers to a CS that aren't needed anymore.

Cheers,
Nicolai

> Please review.
>
> Marek
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>