Re: [RFC PATCH] drm/pancsf: Add a new driver for Mali CSF-based GPUs

[Steven Price <steven.price@arm.com>]

On 03/02/2023 16:29, Alyssa Rosenzweig wrote:
>>> Mali v10 (second Valhal iteration) and later GPUs replaced the Job
>>> Manager block by a command stream based interface called CSF (for
>>> Command Stream Frontend). This interface is not only turning the job
>>> chain based submission model into a command stream based one, but also
>>> introducing FW-assisted scheduling of command stream queues. This is a
>>> fundamental shift in both how userspace is supposed to submit jobs, but
>>> also how the driver is architectured. We initially tried to retrofit the
>>> CSF model into panfrost, but this ended up introducing unneeded
>>> complexity to the existing driver, which we all know is a potential
>>> source of regression.
>>
>> While I agree there's some big differences which effectively mandate
>> splitting the driver I do think there are some parts which make a lot of
>> sense to share.
>>
>> For example pancsf_regs.h and panfrost_regs.h are really quite similar
>> and I think could easily be combined. The clock/regulator code is pretty
>> much a direct copy/paste (just adding support for more clocks), etc.
>>
>> What would be ideal is factoring out 'generic' parts from panfrost and
>> then being able to use them from pancsf.
>>
>> I had a go at starting that:
>>
>> https://gitlab.arm.com/linux-arm/linux-sp/-/tree/pancsf-refactor
>>
>> (lightly tested for Panfrost, only build tested for pancsf).
>>
>> That saves around 200 lines overall and avoids needing to maintain two
>> lots of clock/regulator code. There's definite scope for sharing (most)
>> register definitions between the drivers and quite possibly some of the
>> MMU/memory code (although there's diminishing returns there).
> 
> 200 lines saved in a 5kloc+ driver doesn't seem worth much, especially
> against the added testing combinatorics, TBH. The main reason I can see
> to unify is if we want VM_BIND (and related goodies) on JM hardware too.
> That's only really for Vulkan and I really don't see the case for Vulkan
> on anything older than Valhall at this point. So it comes down to
> whether we want to start Vulkan at v9 or skip to v10. The separate
> panfrost/pancsf drivers approach strongly favours the latter.

While I agree 200 lines isn't much in the grand scheme what I really
don't want is to have to maintain two (almost) identical versions of the
same code. I agree with the concept entirely of having a separate .ko
and not trying to keep it all "one driver". Just, for the bits where
it's clearly copy/pasted from the existing Panfrost, lets move that code
into a common file and build it into both drivers.

Ultimately the 200 line saving was just a couple of hours this morning -
indeed I was 'reviewing' by comparing against Panfrost and thinking "if
it works in Panfrost it must be correct" - the review would be even
easier if it wasn't new code ;)

And as far as I'm aware the changes I'm proposing don't make any
difference to testing - I'm not sure I understand that statement.

The MMU/memory code I'm undecided on. There's clearly copied code there
but quite a few differences. If we can unify and get extra goodies for
Panfrost then it's worth doing, if the unification is going to be hard
or risk regressions then perhaps not - especially if Mesa isn't going to
get the features (which depends whether anyone working on Mesa wants to
work on Vulkan for Bifrost).

Anyway, just to be clear I don't want to stand in the way of getting
this merged. If necessary the refactor can be done on top afterwards
(indeed that's what I've got in my repo).

Thanks,

Steve