Re: [PATCH] nouveau: fix OpenFirmware support

[Laurent Vivier]

Le 11/10/2015 01:49, Ilia Mirkin a Ãcrit :
> On Sat, Oct 10, 2015 at 7:45 PM, Laurent Vivier <laurent@xxxxxxxxx> wrote:
>>
>>
>> Le 10/10/2015 21:56, Ilia Mirkin a Ãcrit :
>>> On Sat, Oct 10, 2015 at 3:29 PM, Laurent Vivier <laurent@xxxxxxxxx> wrote:
>>>>
>>>>
>>>> Le 10/10/2015 20:41, Ilia Mirkin a Ãcrit :
>>>>> Hi Laurent,
>>>>>
>>>>> On Sat, Oct 10, 2015 at 9:27 AM, Laurent Vivier <laurent@xxxxxxxxx> wrote:
>>>>>> On PowerMac G5 (and I think on all OpenFirmware platforms), nvbios_pcirTp()
>>>>>> returns NULL. But in fact the OpenFirmware has given us the size
>>>>>> we can store in image->size.
>>>>>>
>>>>>> This size is stored in bios->size by of_init() as there is no way
>>>>>> to retrieve it otherwise. And as we know the size, copy all data
>>>>>> to bios->data.
>>>>>>
>>>>>> Tested on PowerMac G5 with 64bit kernel and a NV43 card (GeForce 6600 LE).
>>>>>
>>>>> Can you give this patch a shot instead?
>>>>>
>>>>> http://cgit.freedesktop.org/~darktama/nouveau/commit/?id=794a63cc75eada9ad6b2a0275c1c8c4d3522864f
>>>>
>>>> Well, I think mine is also a good solution and it is much more simple.
>>>> ;)
>>>>
>>>> ... because it is useless to add a size() function if we can directly
>>>> copy the content/size of the bios in bios->data and bios->size.
>>>> We can do that because we have the size of the property, which is not
>>>> the case when we discover the BIOS directly from a PCI ROM or from ACPI
>>>> (this is why we need a shadow, I think).
>>>
>>> I'll let Ben rule on this.
>>>
>>>>
>>>> For pcir part, I think we can just ignore the result and take the size
>>>> from bios->size, as in the case of non openfirmware bios->size will be 4
>>>> (we have only shadowed the first word to read the id, 0xaa55) and then
>>>> the checksum and others ID searches will fail. So I think the checksum
>>>> should not be ignored.
>>>
>>> Non-OF will still end up with a NVDA,BMP file? That seems surprising.
>>> My understanding is that if OF has it, it should be used. The problem
>>> is that e.g. on my GPU I have a perfectly valid PCI ROM, whose
>>> checksum matches and everything, but it contains who-knows-what apple
>>> happened to leave in there. So I still want OF. Ignoring checksum
>>> failures allows nouveau to always select the OF vbios.
>>>
>>>>
>>>> I've tried to restore behavior before commit:
>>>>
>>>> 7af4dec drm/nouveau/bios: use size/type from pci data structure
>>>>
>>>> and commit:
>>>>
>>>> ad4a362 drm/nouveau/bios: split out shadow methods
>>>>
>>>> Originally, openfirmware content was copied directly into bios->data:
>>>
>>> Yeah, but then the whole interface was redone.
>>>
>>>>
>>>> 77145f1 drm/nouveau: port remainder of drm code, and rip out compat layer
>>>>
>>>>> This resolved my issues on a PPC G5 + NV34. I think mine ran into a
>>>>> few additional problems that you didn't see -- no PCIR header and
>>>>> invalid checksum.
>>>>
>>>> I have no PCIR header too.
>>>
>>> Er right. I realized that shortly after I sent the email. However my
>>> bios isn't even 0x1000 in size, so the read would fail due to not
>>> enough length. (It's also an odd number in size, and your patch chops
>>> off the last few bytes.) The read could, of course, be reduced in
>>> size, but that whole logic is to deal with multiple parts in a vbios,
>>> which on GM20x contain some necessary blobs. I wasn't sure where the
>>> 0x1000 came from or whether it was significant.
>>>
>>>>
>>>> Could you send me the content of the file "NVDA,BMP" you can find
>>>> somewhere under /proc/device-tree/ ?
>>>
>>> http://filebin.ca/2Ib4SdDOAQqC/nv34-vbios.rom
>>>
>>> Note that it's a 2404 byte file as uploaded, but that was from an
>>> attempt to do something silly -- in reality it's 2403 bytes.
>>>
>>>>
>>>> Could you try my patch on your system, please ?
>>>
>>> My G5 is off for now, and the time I do spend with it goes towards
>>> working out mesa issues (it should kinda-sorta work with Mesa 11.0.3
>>> again btw). If I have time, I'll try it out.
>>
>> I've checked on my second PowerMac G5 which seems to be the same as
>> yours (PowerMac7,3).
>>
>> It doesn't work but not because of the checksum:
>>
>> [  140.410535] nouveau 0000:f0:10.0: NVIDIA NV34 (034100a2)
>> [  140.476781] nouveau 0000:f0:10.0: bios: version 04.34.20.19.00
>> [  140.476993] nouveau 0000:f0:10.0: bios: DCB table not found
>> [  140.477186] nouveau 0000:f0:10.0: bios: DCB table not found
>> [  140.477283] nouveau 0000:f0:10.0: bios: DCB table not found
>> [  140.477289] nouveau 0000:f0:10.0: bios: DCB table not found
>> [  140.477664] nouveau 0000:f0:10.0: bios: DCB table not found
>> [  140.480949] nouveau 0000:f0:10.0: devinit: 0x1a08[ ]: unknown opcode 0x10
>> [  140.480962] nouveau 0000:f0:10.0: preinit failed with -22
>> [  140.480978] nouveau: DRM:dddddddd:00000080: init failed with -22
>> [  140.487980] nouveau: probe of 0000:f0:10.0 failed with error -22
>>
> 
> Because the checksum fails on OF but passes on the PCI ROM. But the
> PCI ROM contains silliness (in fact I'm fairly sure we should just
> remove that method, it never provides anything useful) which passes
> the checksum test.
> 
> Try it with my patch from Ben's tree.

On NV34, yours works fine.

Mine fails because:
1- we try to fetch 0x1000 bytes whereas BIOS is smaller,
2- we compute checksum whereas it is no valid

I'm going to send a v2 of my patch which fixes that, not because I think
it will be better, just because I like to finish what I start (and to share)

We can fix (1) by reducing the size of the shadow_fetch from 0x1000 to
0x200 (there is no reason to keep 0x1000 if we know an existing smaller
size), we can fix (2) by setting the image->type from nvbios_of to
disable the checksum.

Thank you for your help,
Laurent
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/