Re: [PATCH v3 07/17] drm/imagination: Add GPU ID parsing and firmware loading

[Marek Vasut <marek.vasut@mailbox.org>]

On 7/5/23 15:13, Frank Binns wrote:
> On Mon, 2023-06-26 at 10:38 -0500, Adam Ford wrote:
>> On Mon, Jun 26, 2023 at 8:22 AM Frank Binns <Frank.Binns@imgtec.com> wrote:
>>> Hi Adam,
>>>
>>> On Sat, 2023-06-17 at 07:48 -0500, Adam Ford wrote:
>>>> On Tue, Jun 13, 2023 at 10:20 AM Sarah Walker <sarah.walker@imgtec.com> wrote:
>>>>> Read the GPU ID register at probe time and select the correct
>>>>> features/quirks/enhancements. Use the GPU ID to form the firmware
>>>>> file name and load the firmware.
>>>>
>>>> I have a Rogue 6250 variant, but the BVNC is returning a slightly
>>>> different revision than the firmware that's currently available, and
>>>> the older firmware for the vendor driver doesn't work with this new
>>>> code.
>>>>
>>>> Linux responds with Unsupported BVNC: 4.45.2.58.  From what I can
>>>> tell, the closest available firmware is 4.40.2.51.
>>>>
>>>> Will there be more firmware variants in the future or will there be
>>>> some options to build the firmware somehow?
>>>
>>> We don't plan to support the SoC vendor provided firmware binaries as this will
>>> mean having to deal with many different versions of the firmware and its
>>> interface. Specifically, the interface can change in backwards incompatible ways
>>> between releases, it changes based on the hardware feature set & bugs and it's
>>> split across userspace & the kernel. This makes these SoC provided firmware
>>> binaries very difficult to support in this new driver.
>>
>> Thanks for the response.
>>
>> That makes sense.  I would hope that various SoC vendors would jump on
>> the bandwagon to work with your group to get their hardware supported.
>>> As an alternative, we'll be releasing firmware binaries as we add support for
>>> more Rogue GPUs. We'll also release binaries upon request, in case others in the
>>> community want to work on support in the meantime - we're just getting things
>>> set up for this at the moment.
>>
>> The Mesa side of things appears to be missing some documentation, and
>> the power VR stuff still appears listed as experimental.  Is there
>> some documentation somewhere that would explain to someone how to go
>> about porting the Rogue 6250 to a different hardware variant of the
>> 6250?  I don't really know the difference between BVNC of 4.45.2.58
>> and 4.40.2.51, but I can't imagine they are drastically different.
> 
> One thing I forgot to mention is that, alongside the firmware binaries, we'll
> also provide the corresponding device info, e.g. for Mesa:
> https://gitlab.freedesktop.org/mesa/mesa/-/blob/e714b35301a33145399f8939ca864ffd14b49de9/src/imagination/common/pvr_device_info.c#L32-125
> 
> We don't have any specific porting documentation, but we did just send out a
> Mesa MR adding some initial (basic) documentation:
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23992
> 
> In terms of differences between the two GX6250 variants, it doesn't seem that
> there's anything feature-wise that will require any driver changes that are
> specific to the 4.45.2.58 variant (the different firmware should in theory be
> sufficient). There are still some driver changes required, however.
> 
> Assuming the SoC you're interested in is already well supported upstream and the
> clocks, power controllers, etc needed by the GPU are also already supported then
> the following changes will be required at a minimum:
> 
> 1. A GPU node will need adding to the device tree source file for your specific
>     board
> 2. The compatible string for the GPU node will need adding to the list of
>     supported devices in the kernel driver (grep for "dt_match" in the driver
>     code)
> 3. The device info we provide alongside the firmware binary will need adding to
>     the kernel driver and Mesa
> 4. The compatible string for the GPU and display controller device tree nodes
>     will need adding to the Vulkan driver (grep for "pvr_drm_configs" in the Mesa
>     code to see existing examples)
>     
> Hopefully that covers everything, but no doubt I missed something!
> 
> With respect to the experimental status of the driver, I think there are three
> things that need to happen before we can drop this tag. Firstly, the kernel
> driver needs to be merged to the kernel. Secondly, we need to pass Khronos
> conformance on at least one of the devices we support (our current focus is on
> the AXE-1-16M). Finally, we need to upstream all our Mesa changes. This is
> something that we've been chipping away at, but we do have a big backlog in our
> public branch [1]. I expect it's going to be quite some time until all of this
> work is complete.
> 
> While so much code is sitting in downstream branches I think it's going to be
> somewhat painful for people to meaningfully contribute to the driver itself.
> Effort is probably best spent on getting the other drivers, which the GPU driver
> depends on, upstream for the platform(s) you're interested in.
> 
> Just to say that I'm by no means trying to put you off from contributing, but
> simply trying to warn you that until the driver is out of its experimental
> state, a lot of things are going to be in flux and the development process is
> currently a lot more complicated.
> 
> It's also worth highlighting that we're a small team tackling a very large job!
> We're doing our best to do things in the proper way and to lay good foundations
> for the future. We're also learning along the way, so please bear with us :-)

Last year I spent considerable amount of time trying to bring this up on 
R-Car M3-W, but that all failed due to unavailable firmware for this GPU 
revision. The GPU is also GX6250 , so supporting it should be basically 
trivial, considering how the R-Car Gen3 upstream support is all there 
and the driver already supports GX6250 .

Would it be possible for imgtec to provide suitable firmware for this 
GX6250 revision, so the bring up effort could be resumed ?

I think that would also yield a couple more reviewers of this driver, 
since without a suitable firmware, the kernel driver and user space 
stack can not be tested, so there is no point in reviewing or even 
merging such untestable code.