The CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN logic in src/devices/pci_device.c:pci_dev_init is messed up.
First of all, pci_dev_init should only do anything when the pci rom should be run. Secondly, vga_inited should only be set when the rom has been run, and never otherwise as this should be done by the relevant init function of possible (future) VGA setup drivers.
Signed-off-by: Luc Verhaegen libv@skynet.be
Hi Luc, welcome to the List!
On Wednesday 02 January 2008, Luc Verhaegen wrote:
The CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN logic in src/devices/pci_device.c:pci_dev_init is messed up.
Well, it's not as clear as it could be, but I see no flaw so far.
Keep in mind the logic: CONFIG_PCI_ROM_RUN means run all PCI ROMs. CONFIG_CONSOLE_VGA means console might be on VGA, run "the" VGA ROM _regardless_ of the setting of CONFIG_PCI_ROM_RUN.
First of all, pci_dev_init should only do anything when the pci rom should be run.
Yes? As far as I can see pci_rom_probe, pci_rom_load and run_bios are always run in sequence, unless there is an error.
Secondly, vga_inited should only be set when the rom has been run, and never otherwise as this should be done by the relevant init function of possible (future) VGA setup drivers.
Besides the fact that in case of multiple VGAs it is not possible to specify which one is the console, vga_inited is only set iff "the" VGA's ROM has been run.
Your patch admittedly improves readability, but breaks the logic above.
Torsten
On Wednesday 02 January 2008, I wrote:
Your patch admittedly improves readability, but breaks the logic above.
I'd like to apply the beautifying part. What do you think? [ "trivial" by Russ' definition, BTW ;-]
Torsten
you need a signed-off by ...
Acked-by: Ronald G. Minnich rminnich@gmail.com
beautifying is always a good idea.
ron
On Jan 2, 2008 3:04 PM, Torsten Duwe duwe@lst.de wrote:
On Wednesday 02 January 2008, I wrote:
Your patch admittedly improves readability, but breaks the logic above.
I'd like to apply the beautifying part. What do you think? [ "trivial" by Russ' definition, BTW ;-]
Torsten-- linuxbios mailing list linuxbios@linuxbios.org http://www.linuxbios.org/mailman/listinfo/linuxbios
On Wed, Jan 02, 2008 at 03:46:15PM -0800, ron minnich wrote:
you need a signed-off by ...
Acked-by: Ronald G. Minnich rminnich@gmail.com
beautifying is always a good idea.
ron
How bloody insulting.
Luc Verhaegen.
On Jan 2, 2008 5:02 PM, Luc Verhaegen libv@skynet.be wrote:
On Wed, Jan 02, 2008 at 03:46:15PM -0800, ron minnich wrote:
you need a signed-off by ...
Acked-by: Ronald G. Minnich rminnich@gmail.com
beautifying is always a good idea.
ron
How bloody insulting.
Well, no insult was intended.
ron
On Jan 2, 2008 1:16 AM, Luc Verhaegen libv@skynet.be wrote:
The CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN logic in src/devices/pci_device.c:pci_dev_init is messed up.
It may be hard to read, but I am pretty sure it is doing the right thing. Can you provide an instance in which it does the wrong thing?
Thanks
ron
On Wed, Jan 02, 2008 at 01:26:10PM -0800, ron minnich wrote:
On Jan 2, 2008 1:16 AM, Luc Verhaegen libv@skynet.be wrote:
The CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN logic in src/devices/pci_device.c:pci_dev_init is messed up.
It may be hard to read, but I am pretty sure it is doing the right thing. Can you provide an instance in which it does the wrong thing?
pci_dev_init is the default pci device initialisation routine.
It is a dumb pci device initialisation routine, lacking all device specific knowledge.
It can only try to achieve anything when there is a pci rom present which can do the initialisation for us.
When we choose to not run pci roms, we do not initialise anything, and we certainly shouldn't go claiming we did either.
When we choose to not run pci roms, why do we still run pci roms of devices if they just happen to be VGA devices?
So why bother setting vga_inited outside of PCI_ROM_RUN?
If you still do not believe logic, then please set CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN and then build your rom. The build will fail due to missing symbols.
This is how i came across this issue in the first place.
What i am trying to do is implement native VGA bring-up for unichrome. I have had userspace code doing this on top of a clean linuxbios for more than 8 months. This is not just my X driver, i can also set up standard vga text mode, so it is not a pipe dream.
In that light it is perfectly valid to claim CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN, because: * i have no need for a vga rom, as i provide the necessary code for a full vga text mode bringup myself. * i have no need to run any other rom either. * i do want vga console logging.
So when i initialise the unichrome, i set vga_inited myself, like any device driver should when it initialises a vga compatible device to a vga compatible state and expects the console code to make use of it as a vga compatible console.
Since the lunacy that is the northbridge/via/vt8623/vgabios code never bothered to set up vga_inited, i am not breaking this either. Because how does one break what is already broken, and why should i bother fixing what isn't worth fixing? It will eventually be tossed anyway, as it will be done properly.
Since I am trailblazing native vga bringup, it is perfectly understandable that some bumps are encountered along the way. What i do not understand is how something so perfectly logical is rejected without proper consideration, tossed aside as if it were a mindless cleanup.
I do not do mindless cleanups.
Feel free to read over my tiny and trivial patch again.
Thank you for making my day, and a happy 2008 to you too.
Luc Verhaegen. A now very agitated SUSE/Novell X Driver Developer.
On Jan 2, 2008 5:03 PM, Luc Verhaegen libv@skynet.be wrote:
pci_dev_init is the default pci device initialisation routine.
It is a dumb pci device initialisation routine, lacking all device specific knowledge.
Right. By design.
It can only try to achieve anything when there is a pci rom present which can do the initialisation for us.
When we choose to not run pci roms, we do not initialise anything, and we certainly shouldn't go claiming we did either.
OK. Where precisely are we making that claim? What is the path by which we will claim to initialise something when we did not. Sorry, I am looking at the code and not seeing it.
When we choose to not run pci roms, why do we still run pci roms of devices if they just happen to be VGA devices?
Because that was what the user community asked for. The VGA ROM stuff came first. Later, people wanted all option roms to be run. Then, it turned out there is a need for some platforms to run VGA option ROMS only. Finally, there can be a need for ALL option roms to be run but NO VGA consoles to be set up. The logic in this function is twisted because it grew by accretion. I make no claims that it is clean -- it is not.
But I have not yet seen the case that it is wrong.
So why bother setting vga_inited outside of PCI_ROM_RUN?
Because, once you reach that line, you've run the vga bios. I don't understand the question.
If you still do not believe logic, then please set CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN and then build your rom. The build will fail due to missing symbols.
That's a prolem. Which symbols? Which mainboards? This is an error but it has been a long enough time since I did this that I can easily believe it will happen.
What i am trying to do is implement native VGA bring-up for unichrome. I have had userspace code doing this on top of a clean linuxbios for more than 8 months. This is not just my X driver, i can also set up standard vga text mode, so it is not a pipe dream.
That's great to hear. Be aware that SiS also did native vga text mode in 2000, by Ollie Lo. So, we know that it is possible to have this work. We have done a fair amount of native and emulated VGA bringup over the years.
In that light it is perfectly valid to claim CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN, because:
- i have no need for a vga rom, as i provide the necessary code for a full vga text mode bringup myself.
- i have no need to run any other rom either.
- i do want vga console logging.
Ah. Now I see what you are trying to do. I misunderstood from your first message. No argument. We've been doing this for years. And if you can't do it, then something is broken for sure. So I'm happy to see it fixed.
At the same time, let's suppose for the sake of argument that you set BOTH CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN AND you have the native hardware bringup. How does that hurt your native device? There's no option rom for it --> it won't be run. I'd like to understand this. We have had cases where people had native bringup and installed a vga card, and both were needed. In that case we would want roms to run.
So when i initialise the unichrome, i set vga_inited myself, like any device driver should when it initialises a vga compatible device to a vga compatible state and expects the console code to make use of it as a vga compatible console.
OK. It's fine to set it there I suppose.
Since I am trailblazing native vga bringup, it is perfectly understandable that some bumps are encountered along the way. What i do not understand is how something so perfectly logical is rejected without proper consideration, tossed aside as if it were a mindless cleanup.
It was considered. I think there is a bigger picture than you might have realised. Torsten also pointed out a secondary issue.
I read your patch. I am still unsure of the case where there are vga class devices but we don't want a vga console (hint: GPU computing). It seems there is no way in your code to allow us to run the option ROMs for a "vga" device and not enable a vga console.
This is a cooperative project and some patience from all sides is important. So please bear with us as we try to make sure we understand your needs and what you are doing. Very trivial changes can turn into problems in ways we do not expect, and VGA and ROM issues have been a headache.
thanks
ron
ron minnich wrote:
On Jan 2, 2008 5:03 PM, Luc Verhaegen libv@skynet.be wrote:
If you still do not believe logic, then please set CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN and then build your rom. The build will fail due to missing symbols.
That's a prolem. Which symbols? Which mainboards? This is an error but it has been a long enough time since I did this that I can easily believe it will happen.
On any board. Just an example:
gcc -m32 -nostdlib -nostartfiles -static -o linuxbios_ram -T /home/corey/linuxbios/msi_945g/LinuxBIOSv2/src/config/linuxbios_ram.ld linuxbios_ram.o linuxbios_ram.o: In function `pci_dev_init': (.text+0x33f2): undefined reference to `pci_rom_probe' linuxbios_ram.o: In function `pci_dev_init': (.text+0x3402): undefined reference to `pci_rom_load' linuxbios_ram.o: In function `pci_dev_init': (.text+0x3412): undefined reference to `run_bios' collect2: ld returned 1 exit status make[1]: *** [linuxbios_ram] Error 1 make[1]: Leaving directory `/home/corey/linuxbios/msi_945g/LinuxBIOSv2/targets/gigabyte/m57sli/m57sli/normal' make: *** [normal/linuxbios.rom] Error 1
This patch fixes the problem. It's not elegant but v2 is on the path to obsolescence and I would rather not do something more complex.
Comments welcome. Luc, thanks for raising this issue.
ron
On Wed, Jan 02, 2008 at 09:41:18PM -0800, ron minnich wrote:
This patch fixes the problem. It's not elegant but v2 is on the path to obsolescence and I would rather not do something more complex.
Comments welcome. Luc, thanks for raising this issue.
ron
Grep the tree. Find out where CONFIG_PCI_ROM_RUN is used. Find out where CONFIG_CONSOLE_VGA is used. Find out where vga_inited is used. Find out that my patch was actually rather trivial, and that there really is nothing to be afraid of.
Needlessly including the pci_rom and emulator code is rather bad. Especially when i am working hard to not include a 64kB vga bios and instead limit the size for equal functionality to several kB.
As for complexity, i am looking at a +4400 -1000 diff for native unichrome already.
How useful is linuxbiosv3 at this point? Can it be used to boot something as well-known as the epia-m already?
I really do not see what you continue to keep having against this trivial and rather sane fix.
Luc Verhaegen.
On Wed, Jan 02, 2008 at 05:31:27PM -0800, ron minnich wrote:
On Jan 2, 2008 5:03 PM, Luc Verhaegen libv@skynet.be wrote:
What i am trying to do is implement native VGA bring-up for unichrome. I have had userspace code doing this on top of a clean linuxbios for more than 8 months. This is not just my X driver, i can also set up standard vga text mode, so it is not a pipe dream.
That's great to hear. Be aware that SiS also did native vga text mode in 2000, by Ollie Lo. So, we know that it is possible to have this work. We have done a fair amount of native and emulated VGA bringup over the years.
Where did this code go?
In that light it is perfectly valid to claim CONFIG_CONSOLE_VGA without setting CONFIG_PCI_ROM_RUN, because:
- i have no need for a vga rom, as i provide the necessary code for a full vga text mode bringup myself.
- i have no need to run any other rom either.
- i do want vga console logging.
Ah. Now I see what you are trying to do. I misunderstood from your first message. No argument. We've been doing this for years. And if you can't do it, then something is broken for sure. So I'm happy to see it fixed.
At the same time, let's suppose for the sake of argument that you set BOTH CONFIG_CONSOLE_VGA and CONFIG_PCI_ROM_RUN AND you have the native hardware bringup. How does that hurt your native device?
It doesn't, except that it makes this whole exercise rather useless. What point is there in a fully free bios when one still uselessly includes emulator code?
There's no option rom for it --> it won't be run. I'd like to understand this. We have had cases where people had native bringup and installed a vga card, and both were needed. In that case we would want roms to run.
What about providing both CONFIG_PCI_ROM_RUN and CONFIG_CONSOLE_VGA, at the same time?
In the case a standalone card is used, the target Config.lb should have CONFIG_PCI_ROM_RUN. But it should never be forced by the motherboard or a device Config.lb when not needed.
So when i initialise the unichrome, i set vga_inited myself, like any device driver should when it initialises a vga compatible device to a vga compatible state and expects the console code to make use of it as a vga compatible console.
OK. It's fine to set it there I suppose.
Why "I suppose"?
vga_inited is from the console code. We are currently only setting it, as an extern, in the device code. What could stop anyone from setting it in any other code?
It was considered. I think there is a bigger picture than you might have realised. Torsten also pointed out a secondary issue.
CONFIG_CONSOLE_VGA is not the right solution in that case. Maybe CONFIG_VGA_ROM_RUN should be trivially introduced.
I read your patch. I am still unsure of the case where there are vga class devices but we don't want a vga console (hint: GPU computing).
As one of the main radeonhd driver developers, i am rather acutely aware of gpgpu. But i fail to see the relevance of this here.
It seems there is no way in your code to allow us to run the option ROMs for a "vga" device and not enable a vga console.
My patch allows this in the exact same manner as before. Please look at the resulting code.
The real question is: What happens the unerring linuxbios user only specificies only CONFIG_CONSOLE_VGA, and still expects it to work with a vga rom. But this is a purely an issue of bad former practice and bad documentation.
Both options should be specified if both are needed.
This is a cooperative project and some patience from all sides is important. So please bear with us as we try to make sure we understand your needs and what you are doing. Very trivial changes can turn into problems in ways we do not expect, and VGA and ROM issues have been a headache.
Heh.
Luc Verhaegen.
The real question is: What happens the unerring linuxbios user only specificies only CONFIG_CONSOLE_VGA, and still expects it to work with a vga rom. But this is a purely an issue of bad former practice and bad documentation.
Both options should be specified if both are needed.
I think it is pretty clearly stated here:
http://linuxbios.org/VGA_support
Also this might help some:
http://www.linuxbios.org/data/vgabios/
Thanks - Joe
On Thursday 03 January 2008, Luc Verhaegen wrote:
It was considered. I think there is a bigger picture than you might have realised. Torsten also pointed out a secondary issue.
CONFIG_CONSOLE_VGA is not the right solution in that case. Maybe CONFIG_VGA_ROM_RUN should be trivially introduced.
To not increase the obvious confusion further, I would call the option something negative, like ...SKIP_NON_VGA, so nobody assumes it enables any ROM execution. Maybe introduce this for v3.
I read your patch. I am still unsure of the case where there are vga class devices but we don't want a vga console (hint: GPU computing).
As one of the main radeonhd driver developers, i am rather acutely aware of gpgpu. But i fail to see the relevance of this here.
Indeed. PCI_ROM_RUN will do, regardless of the console.
It seems there is no way in your code to allow us to run the option ROMs for a "vga" device and not enable a vga console.
My patch allows this in the exact same manner as before. Please look at the resulting code.
The real question is: What happens the unerring linuxbios user only specificies only CONFIG_CONSOLE_VGA, and still expects it to work with a vga rom. But this is a purely an issue of bad former practice and bad documentation.
Nope, it's a *bug* noone has noticed until you tried. We can now fix the code or redefine the semantics. Your patch implicitly does the latter, and without further explanation I assumed you didn't understand them. Ron already mentioned this is v2, so I'd say let's go for it; original patch ACKed.
If you had mentioned that CONSOLE_VGA=1 PCI_ROM_RUN=0 does not compile and you are working on a free standing driver, that would have saved us from the irrelevant part of this discussion.
So if nobody objects I'll commit the original patch, after the west coast has had a chance to comment ;-)
Torsten
On Jan 3, 2008 3:05 AM, Torsten Duwe duwe@lst.de wrote:
If you had mentioned that CONSOLE_VGA=1 PCI_ROM_RUN=0 does not compile and you are working on a free standing driver, that would have saved us from the irrelevant part of this discussion.
Exactly. Simple, clear bug reports are always useful. Took me a few minutes to fix.
So if nobody objects I'll commit the original patch, after the west coast has had a chance to comment ;-)
Luc's patch is unnecessary as submitted; it will eliminate an option that, in the past, was deemed desirable by some users (they may no longer care, it may no longer matter). You can just use the patch I sent, which fixes the problem and preserves behavior which, at one point, somebody wanted. When the code got broken I am not sure. That said, if the feeling is that we should go with Luc's patch, I have no objection.
I should comment on the "who wants an emulator in a free BIOS" argument; that discussion was resolved years ago, in favor or "lots of people need it, whether they want it or not" -- I don't like it either, but that's the way it goes. I was one of the strongest opponents of using the option ROMs. I no longer see any way out of using the option ROMs in some cases. It's the same reason we have binary blobs in the linux kernel (e.g. microcode patches); sometimes there is no choice.
So, even if we have a totally free driver for graphics, we will probably see continuing demand for the emulator, to support cards with option ROMs.
ron
On Thu, Jan 03, 2008 at 10:37:45AM -0800, ron minnich wrote:
On Jan 3, 2008 3:05 AM, Torsten Duwe duwe@lst.de wrote:
If you had mentioned that CONSOLE_VGA=1 PCI_ROM_RUN=0 does not compile and you are working on a free standing driver, that would have saved us from the irrelevant part of this discussion.
Exactly. Simple, clear bug reports are always useful. Took me a few minutes to fix.
How long do you think it took me to fix in the first place?
The difference is that one fix uses CONSOLE_VGA as the name obviously suggests, and the other continues non-obvious side effects, which might be desired for 95% of the cases, but which completely rules out building without those non-obvious side-effects.
So if nobody objects I'll commit the original patch, after the west coast has had a chance to comment ;-)
Luc's patch is unnecessary as submitted; it will eliminate an option that, in the past, was deemed desirable by some users (they may no longer care, it may no longer matter).
Those users can either use both PCI_ROM_RUN _and_ CONSOLE_VGA, or a now often suggested third option (VGA_ROM_RUN or similar, maybe VGA_PCI_ROM_RUN, or the inverse logic?) can be introduced for the other case.
I am specifically not removing an option, i am simply sanitising the usage of an option, to match its linguistical and logical meaning, as there are cases where the non-obvious side-effects are not needed.
You can just use the patch I sent, which fixes the problem and preserves behavior which, at one point, somebody wanted.
It exacerbates the problem.
When the code got broken I am not sure.
I do not care when it got broken or who did so. I do know that the code got broken because the side-effect was non-obvious. And i do want to see it become obvious.
That said, if the feeling is that we should go with Luc's patch, I have no objection.
I should comment on the "who wants an emulator in a free BIOS" argument; that discussion was resolved years ago, in favor or "lots of people need it, whether they want it or not" -- I don't like it either, but that's the way it goes. I was one of the strongest opponents of using the option ROMs. I no longer see any way out of using the option ROMs in some cases. It's the same reason we have binary blobs in the linux kernel (e.g. microcode patches); sometimes there is no choice.
I never said i do not want emulators in free BIOSes. I said that, when a board doesn't require the emulator, and when the linuxbios user doesn't add the requirement later on through the target config.lb, then there is no real need to force inclusion of the emulator.
So, even if we have a totally free driver for graphics, we will probably see continuing demand for the emulator, to support cards with option ROMs.
Of course. But does this mean that we should always impose the emulator on everybody all the time? Or do we leave this up to: * whichever chip driver that might require it. * whichever board that might require it. * whichever user that might see a point in it when building his target image.
For the EPIA-M, the hardware present doesn't require pci roms. And the typical use case, with a single pci slot, does not involve pci roms. So why impose the emulator?
Tonight or tomorrow, whenever i will have the time, i will go over _all_ instances of CONSOLE_VGA and prepend this with either PCI_ROM_RUN (when this isn't already done), or something VGA specific, whatever the consensus will be. So, to those interested, please come to some consensus on this.
For the EPIA-M, I will add a commented out PCI_ROM_RUN option to the Config.lb with a comment beforehand explaining that, when desired, this should be enabled. It should not be enabled per default, as those building an image should at least read and alter the target Config.lb file.
I do hope that this will finally satisfy you so that i can finally move on and fix the remaining issues with my native vga textmode bringup.
Luc Verhaegen. SUSE/Novell X Driver Developer.
-
Corey Osgood -
joe＠smittys.pointclark.net -
Luc Verhaegen -
ron minnich -
Torsten Duwe