13:04 <bbrezillon> robmur01, stepri01: just had to debug a problem where a compute job was corrupting an executable BO, and finding the source of the corruption would have been easier if we had a way to mark executable BOs non-writable (I hacked the driver to mark all executable BOs non-writable, but I guess we should add an extra flag for that)

13:09 <robmur01> bbrezillon: can we know the intended purpose of a BO at the point it's mapped in panfrost_gem_open()?

13:11 <robmur01> hmm, actually I see we already have bo->noexec... can we just infer no_exec == RW and !no_exec == RX?

13:12 <robmur01> fiddling with the prot flags in panfrost_mmu_map() will be trivial, but changing permissions once something's already mapped much less so

13:12 <stepri01> I'd be a bit worried about breaking something by simply removing write permissions from !no_exec. But I can't see any issues with having an extra flag to specify that the GPU should never write

13:13 <stepri01> readonly no_exec regions could be useful to catch other bugs in the future

13:14 <robmur01> yeah, I was wondering if RWX might still be needed for some of the tricksy self-modifying stuff with write value jobs

13:16 <stepri01> AFAIK the self-modifying stuff with write value jobs is with job descriptors getting modified, so that doesn't need executable. But I do think there might have been something in the past using a full shader to modify the next job

13:17 <stepri01> The DDK also has "pilot shaders" which precompute 'almost constants' and I've no idea whether that sort of trick requires RWX

13:21 <robmur01> if we're looking at BO flags, might it also be worth thinking about whether we could also identify GPU-private mappings which may benefit from reduced shareability?

13:28 <daniels> stepri01: oh you do pilot shaders as well

13:29 <daniels> (qcom also has this)

13:31 <stepri01> daniels: I suspect it's a fairly common optimisation

13:31 <stepri01> certainly there seems to be enough content out there which has these almost constant expressions in fragment shaders

13:41 <bbrezillon> robmur01, stepri01: yep, I feel like changing the 'exec is RW' behavior is risky (kind of breaking the ABI if someone relies on it, FWIW, mesa doesn't)

13:48 <bbrezillon> robmur01: I guess we can pass that info as well

14:33 <robclark> bbrezillon: on drm/msm side, we have a MSM_BO_GPU_READONLY that can be specified at allocation time.. we use that for things like cmdstream and other things that GPU has no business writing.. it's a thing I'd recommend

14:36 <bbrezillon> robclark: yep, makes perfect senes

14:37 <bbrezillon> well, we do have jobs that are modifying cmdstream descs

14:38 <bbrezillon> but we should be able to figure it out at allocation time, and allocate those from a RW pool