Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

[Carsten Haitzler]

On 7/23/24 5:06 PM, Boris Brezillon wrote:
> Hi Steve,
>
> On Mon, 15 Jul 2024 10:12:16 +0100
> Steven Price <steven.price@xxxxxxx> wrote:
>
> > I note it also shows that the "panthor_regs.rs" would ideally be shared.
> > For arm64 we have been moving to generating system register descriptions
> > from a text source (see arch/arm64/tools/sysreg) - I'm wondering whether
> > something similar is needed for Panthor to generate both C and Rust
> > headers? Although perhaps that's overkill, sysregs are certainly
> > somewhat more complex.
>
> Just had a long discussion with Daniel regarding this panthor_regs.rs
> auto-generation, and, while I agree this is something we'd rather do if
> we intend to maintain the C and rust code base forever, I'm not
> entirely convinced this is super useful here because:
>
> 1. the C code base is meant to be entirely replaced by a rust driver.
> Of course, that's not going to happen overnight, so maybe it'd be worth
> having this autogen script but...
>
> 2. the set of register and register fields seems to be pretty stable.
> We might have a few things to update to support v11, v12, etc, but it
> doesn't look like the layout will suddenly become completely different.
>
> 3. the number of registers and fields is somewhat reasonable, which
> means we should be able to catch mistakes during review. And in case
> one slip through, it's not the end of the world either because this
> stays internal to the kernel driver. We'll either figure it out when
> rust-ifying panthor components, or that simply means the register is
> not used and the mistake is harmless until the register starts being
> used
>
> 4. we're still unclear on how GPU registers should be exposed in rust,
> so any script we develop is likely to require heavy changes every time
> we change our mind

You have a good point. A script sounds nice, but given the restricted 
domain size, it maybe better to be manually maintained. Given that I 
also think the right way to access registers is to do it as safely as 
possible.

So a gpu_write() or gpu_read() are "unsafe" in that you can write 
invalid values to a just about anything in C. If we're trying to harden 
drivers like panthor and make it "impossible" to do the wrong thing, 
then IMHO for example MCU_CONTROL should be abstracted so I can ONLY 
write MCU_CONTROL_* values that are for that register and nothing else 
in Rust. This should fail at compile time if I ever write something 
invalid to a register, and I can't write to anything but a known/exposed 
register.

Interestingly the C code could also abstract the same way and at least 
produce warnings too and become safer. It may be useful to mimic the 
design pattern there to keep panthor.rs and panthor.c in sync more easily?

So my opinion would be to try get the maximum value from Rust and have 
things like proper register abstractions that are definitely safe.

> For all these reasons, I think I'd prefer to have Daniel focus on a
> proper rust abstraction to expose GPU registers and fields the rust-way,
> rather than have him spend days/weeks on a script that is likely to be
> used a couple times (if not less) before the driver is entirely
> rewritten in rust. I guess the only interesting aspect remaining after
> the conversion is done is conciseness of register definitions if we
> were using some sort of descriptive format that gets converted to rust
> code, but it comes at the cost of maintaining this script. I'd probably
> have a completely different opinion if the Mali register layout was a
> moving target, but it doesn't seem to be the case.
>
> FYI, Daniel has a python script parsing panthor_regs.h and generating
> panthor_regs.rs out of it which he can share if you're interested.
>
> Regards,
>
> Boris