Re: [PATCH v2 1/2] gpu: nova-core: add initial driver stub

[Alexandre Courbot]

Hi Danilo,

On Thu Feb 6, 2025 at 11:49 PM JST, Danilo Krummrich wrote:
>> > +impl Spec {
>> > +    fn new(bar: &Devres<Bar0>) -> Result<Spec> {
>> > +        let bar = bar.try_access().ok_or(ENXIO)?;
>> > +        let boot0 = regs::Boot0::read(&bar);
>> > +
>> > +        let Some(chipset) = Chipset::from_u32(boot0.chipset()) else {
>> > +            return Err(ENODEV);
>> > +        };
>> > +
>> > +        let Some(arch) = Architecture::from_u32(boot0.arch()) else {
>> > +            return Err(ENODEV);
>> > +        };
>>
>> Technically the Architecture is already known if the Chipset has been
>> built successfully, so there should be no need to build it again (and
>> test for a failure that cannot happen at this point).
>>
>> Since the architecture information is already embedded in Chipset, maybe
>> we can have an arch() method there?
>>
>> Something like:
>>
>>     impl Chipset {
>>         pub(crate) fn arch(self) -> Architecture {
>>             match self as u32 & !0xf {
>>                 0x160 => Architecture::Turing,
>>                 0x170 => Architecture::Ampere,
>>                 0x190 => Architecture::Ada,
>>                 _ => unreachable!(),
>>             }
>>         }
>>     }
>
> I thought about this, which is also why the comment above says: "consider to
> store within Chipset, if arbitrary_enum_discriminant becomes stable".
>
> I did not go with what you suggest because it leaves us with either
> Chipset::arch() returning a Result, which is annoying, or with Chipset::arch()
> being able to panic the kernel, which I'd dislike even more.
>
> There's also a third option, which would be to have some kind of unknown
> architecture, which we could catch later on, but that's just a worse variation
> of returning a Result.
>
> Another reason was that I did not want to encode register specific masks into
> the Chipset type.

Agreed, none of these solutions are completely satisfying. From the
point that we have successfully built a Chipset, we should be able to
get its architecture without any runtime error or failure path. So I
guess this leaves matching all the variants. It's a bit verbose, but
hopefully later we can gather all the static information about chipsets
in a single place, and generate these implementations (including a
Display implementation - ideally the debug one would also print the hex
representation of the chipset to be more useful for troubleshooting)
using macros.

Cheers,
Alex.