On 30/09/2021 23:12, Alyssa Rosenzweig wrote: >>>> + /* Executable implies readable */ >>>> + if ((args->flags & PANFROST_BO_NOREAD) && >>>> + !(args->flags & PANFROST_BO_NOEXEC)) >>>> + return -EINVAL; >>> >>> Generally, executable also implies not-writeable. Should we check that? >> >> We were allowing it until now, so doing that would break the backward >> compat, unfortunately. > > Not a problem if you only enforce this starting with the appropriate > UABI version, but... > >> Steve also mentioned that the DDK might use shaders modifying other >> shaders here [1] > > What? I believe it, but what? *might* ;) I've not looked in detail and a quick look through the code just now I can't actually find anything which does. > For the case of pilot shaders, that shouldn't require self-modifying > code. As I understand, the DDK binds the push uniform (FAU / RMU) buffer > as global shader memory (SSBO) and uses regular STORE instructions on > it. That requires writability on that BO but that should be fine. Yeah it looks like you're correct here - I've never looked closely into exactly how pilot shaders work. It would appear that the DDK checks (in user space) for the GPU-executable + GPU-writable condition and moans. So this obviously isn't used by the DDK as it stands. For the actual patch: Reviewed-by: Steven Price <steven.price@xxxxxxx> Although if you can figure out how to add the "private mapping" flag at the same time we can avoid bumping the version number too many times. And today I'm actually in the office so (perversely) I haven't got the hardware to actually test it at the moment - I really need to get remote access set up! Thanks, Steve
