On Thu Mar 5, 2026 at 8:37 AM CET, Eliot Courtney wrote:
> On Wed Mar 4, 2026 at 8:39 PM JST, Danilo Krummrich wrote:
>> On Wed Mar 4, 2026 at 2:42 AM CET, Eliot Courtney wrote:
>>> + fn allocate_command(&mut self, size: usize, timeout: Delta) ->
>>> Result<GspCommand<'_>> {
>>> + read_poll_timeout(
>>> + || Ok(self.driver_write_area_size()),
>>> + |available_bytes| *available_bytes >=
>>> size_of::<GspMsgElement>() + size,
>>> + Delta::ZERO,
>>
>> Isn't this either creating unneccessary thrashing of the memory controller or
>> unnecessary contention at the cache-coherency level?
>>
>> I think we should probably add at least a small delay of something around
>> 1us.
>
> This is what nouveau does (specifically `usleep_range(1, 2)`). OTOH,
> openrm just does a busy wait, which is what I replicated here for now.
> GSP command queue not having space IIUC is meant to be very exceptional.
> I am not sure which is best, maybe Alex has an opinion, but also happy
> to change it because that reasoning makes sense to me and I don't know
> enough about the distribution of how often it would actually need
> to wait to know if 0 delay is justified.
Well, what this code says is "let's hammer the cache / memory controller as fast
as we can for up to one second".
This really should come with some justification why it is actually needed for
proper operation of the driver.
@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.
