Message-ID: <CAL_Jsq+Dr5zO5MKEGq0dW9SuTuawaJMhHziFd73Ef_S1zbOkXw@mail.gmail.com>
Date: Wed, 24 Jul 2024 08:15:38 -0500
From: Rob Herring <robh@...nel.org>
To: Steven Price <steven.price@....com>, Boris Brezillon <boris.brezillon@...labora.com>, 
	Daniel Almeida <daniel.almeida@...labora.com>
Cc: Wedson Almeida Filho <wedsonaf@...il.com>, ojeda@...nel.org, 
	Danilo Krummrich <dakr@...hat.com>, lyude@...hat.com, lina@...hilina.net, mcanal@...lia.com, 
	airlied@...il.com, rust-for-linux@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

On Wed, Jul 24, 2024 at 3:59 AM Steven Price <steven.price@....com> wrote:
>
> Hi Boris,
>
> On 23/07/2024 17:06, Boris Brezillon wrote:
> > Hi Steve,
> >
> > On Mon, 15 Jul 2024 10:12:16 +0100
> > Steven Price <steven.price@....com> 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:
>
> So I think we need some more alignment on how the 'Rustification'
> (oxidation?) of the driver is going to happen.
>
> My understanding was that the intention was to effectively start a
> completely separate driver (I call it "Rustthor" here) with the view
> that it would eventually replace (the C) Panthor. Rustthor would be
> written by taking the C driver and incrementally converting parts to
> Rust, but as a separate code base so that 'global' refactoring can be
> done when necessary without risking the stability of Panthor. Then once
> Rustthor is feature complete the Panthor driver can be dropped.
> Obviously we'd keep the UABI the same to avoid user space having to care.

We did discuss this, but I've come to the conclusion that's the wrong
approach. Converting is going to need to track kernel closely as there
are lots of dependencies with the various rust abstractions needed. If
we just copy over the C driver, that's an invitation to diverge and
accumulate technical debt. The advice to upstreaming things is never
go work on a fork for a couple of years and come back with a huge pile
of code to upstream. I don't think this situation is any different. If
there's a path to do it in small pieces, we should take it.

What parts of the current driver are optional that we could leave out?
Perhaps devfreq and any power mgt. That's not much, so I think the
rust implementation (complete or partial) will always be feature
complete.

> I may have got the wrong impression - and I'm certainly not saying the
> above is how we have to do it. But I think we need to go into it with
> open-eyes if we're proposing a creeping Rust implementation upstream of
> the main Mali driver. That approach will make ensuring stability harder
> and will make the bar for implementing large refactors higher (we'd need
> significantly more review and testing for each change to ensure there
> are no regressions).

This sounds to me like the old argument for products running ancient
kernels. Don't change anything so it is 'stable' and doesn't regress.
I think it's a question of when, not if we're going to upstream the
partially converted driver. Pretty much the only reason I see to wait
(ignoring dependencies) is not technical, but the concerns with
markets/environments that can't/won't adopt Rust yet. That's probably
the biggest issue with this patch. If converting the main driver first
is a requirement (as discussed elsewhere in this thread), I think all
the dependencies are going to take some time to upstream, so it's not
something we have to decide anytime soon.

Speaking of converting the main driver, here's what I've got so far
doing that[1]. It's a top down conversion with the driver model and
DRM registration in Rust. All the ioctls are rust wrappers calling
into driver C code. It's compiling without the top commit.

> > 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...
>
> Just to put my cards on the table. I'm not completely convinced a Rust
> driver is necessarily an improvement, and I saw this as more of an
> experiment - let's see what a Rust driver looks like and then we can
> decide which is preferable. I'd like to be "proved wrong" and be shown a
> Rust driver which is much cleaner and easier to work with, but I still
> need convincing ;)

Unless your Rust is as good as your C, that's never going to happen.

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=rust/panthor-6.10

Powered by blists - more mailing lists