Re: [PATCH v5 2/9] gpu: nova-core: gsp: add mechanism to wait for space on command queue

["Danilo Krummrich" <dakr@kernel.org>]

On Fri Mar 6, 2026 at 1:48 AM CET, Alexandre Courbot wrote:
> On Thu Mar 5, 2026 at 8:16 PM JST, Danilo Krummrich wrote:
>> @Alex: It also seems that this is based on broken code, i.e. I noticed how the
>> DMA read is done in this case in e.g. gsp_read_ptr().
>>
>> 	fn cpu_read_ptr(&self) -> u32 {
>> 	    let gsp_mem = self.0.start_ptr();
>> 	
>> 	    // SAFETY:
>> 	    //  - The ['CoherentAllocation'] contains at least one object.
>> 	    //  - By the invariants of CoherentAllocation the pointer is valid.
>> 	    (unsafe { (*gsp_mem).cpuq.rx.read_ptr() } % MSGQ_NUM_PAGES)
>> 	}
>>
>> Why isn't this using dma_read!()? I think creating this reference is UB.
>
> We can't - technically we would have to have the `dma_read` in `cmdq.rs`
> so it can access the `CoherentAllocation` (and do an unwrap in the
> process):
>
>     dma_read!(self.0, 0, .gspq.rx.0.readPtr).unwrap()
>
> ... but that cannot be done as `MsgqRxHeader` is part of the bindings
> (i.e. in `fw.rs`) and thus its internal fields are not visible to
> `cmdq.rs`, as per our policy of making the bindigns opaque.

We can have a helpers for doing such dma_read!() calls in gsp/fw.rs instead, and
just forward from the actual methods.

	fn cpu_read_ptr(&self) -> u32 {
	    fw:gsp_mem::cpu_rx_ptr(self) % MSGQ_NUM_PAGES
	}

> This can probably be done better with I/O projections, but for now we have to
> do the read_volatile by ourselves.

Not necessarily, see above.

> What makes this reference UB btw?

Gary explained this in another reply already; I think fixing this with Opaque or
passing raw pointers instead involves even more unsafe. Whereas the simple
indirection from above is fully safe and can easily replaced with I/O
projections once we have them.