Linus On GEM Patches: UNTESTED CRAP

[phoronix]

Phoronix: Linus On GEM Patches: UNTESTED CRAP

Yesterday we shared that the patches for Intel's GEM (the Graphics Execution Manager) were submitted for inclusion into Linux 2.6.28. Those patches that added in GEM along with a few other Direct Rendering Manager improvements have landed into the mainline Linux git tree, but not without commentary from Linus Torvalds. Linus became outraged over new DRM warnings and what Linus describes as horribly bad code...

http://www.phoronix.com/vr.php?view=Njc5Mg

[chithanh]

drivers/gpu/drm/drm_proc.c:525: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’
The assumption that sizeof(int) == sizeof(size_t) makes one wonder whether the code was intended to run on 64 bit platforms at all.

[KDesk]

Is this not wrong?
The Torvalds answer was for Dave Airlie, not for yesterday's David Airlie patch (http://www.phoronix.com/scan.php?page=news_item&px=Njc4OQ)

http://news.gmane.org/gmane.comp.video.dri.devel

[SheeEttin]

Is it just me, or is there a certain image of Linus being painted here?

[smitty3268]

Is it just me, or is there a certain image of Linus being painted here?

The message ended with:


I really wish people were more careful, and took more pride in trying to
write readable code, with small modular functions instead. And move those
variables down to the block they are needed in.

Anyway, I pulled the thing, but _please_ test this cleanup and send it
back to me if it passes your testing. Ok?

Which isn't exactly what I think most people would expect after reading the story here.

[ethana2]

If Ballmer and Gates had as much sense as Linus maybe Windows wouldn't suck as bad.

...but at this point I'm afraid, it's too ****ing late.

[DanL]

I don't always agree with Linus' views, but can you blame him for blowing his lid when he has to repeatedly remind certain people of certain policies?

[hubick]

Let me say thanks to Linus! As one of the many users running x86_64 Linux who has to live with crappy developers not writing 64bit clean code, thank you Linus for yelling at the Intel guys for not having a single person on their team using x86_64!

[Tillin9]

I also think the news blurb is a little misleading, since Linus' response is generally level headed, even more so considering his usual demeanor. I agree with Linus, that code is crap. There is absolutely no excuse for that kind of sloppyness, especially since these don't seem like merging bugs (i.e. the GEM devs were working with slightly different code), but general API, kernel, and C programming bugs. I work on a much less important open source project than the Linux kernel and if any developer tried to commit code to trunk that gave plain old gcc warnings, they would likely have their privileges revoked.

[deanjo]

Unfortunately, some developers are satisfied with code that is "good enough" and don't take pride in their work.

[energyman]

some people might think that Linus' style is abusive - but sometimes you just need to call the crap by its name. And he never abuses noobs - only devs who really should know better.

[deanjo]

some people might think that Linus' style is abusive

I see it more as "Tough Love".:p

[wswartzendruber]

I think Intel's counting on the community to test and clean this up.

[Ex-Cyber]

I thought this was far more troubling than anything Linus wrote:

I don't generally trawl the Fedora kernel build logs, and the amount of warnings we have there, new ones just get lost in the mists.

[energyman]

makes one rething the choice of distro, huh?

[barbro]

Do these patches contain KMS? or do we have to wait until .29

[Dragonlord]

Linus is right, no matter if he said it in a moderate or harsh tone. Many developers nowadays churn out code which makes a real hacker cry and vomit... especially when proposed for inclusion into one of the most vital areas of an operating system: the kernel! Go on Linus... we need clean hacker style code not ATI driver blob crash festival code or intel we-are-the-greatest mess code ( looking at VARIOUS drivers from them being the biggest crap since a long time ).

[etymxris]

I wish my project managers were more like Linus and actually cared about code quality and cleanliness. It's really not that hard to write good code, and it ends up saving you time in the long run.

In any case, if I got a reply like that to my code submissions I'd just laugh, and then try to put on my serious face before I responded.

[airlied]

makes one rething the choice of distro, huh?

Do you think any kernel distro maintainers trawl the warnings in their build logs?

You must live in a different world than everyone else.

Dave.

[energyman]

some distros patch less than others.

[wswartzendruber]

Is it just me, or is there a certain image of Linus being painted here?
Pretty much, yeah.

[KDesk]

Driver developers could compile their own Kernel! Without any other outside patch and occasionally look into the logs.

[highlandsun]

Do you think any kernel distro maintainers trawl the warnings in their build logs?

You must live in a different world than everyone else.

Dave.

Hmm... When I write new code, I watch how it compiles, no matter how big the backdrop it's living in.

Linus is right, people need to take more pride in their work and actually care about their results. Cavalier attitudes like that just give fodder to managers who believe that developers aren't important and are freely interchangeable.

[energyman]

Pretty much, yeah.

the image of a benevolent dicator who does not accept crap from his leutnants.

[TechMage89]

If the compiler spits out tons of warnings when the kernel compiles it says one of two things:

1) There is a problem with the compiler.
2) There is a problem with the code.

Therefore, for stuff intended to be put in mainline, warnings really ought to be looked into. Yes, it is a pain, and it slows things down, and perhaps some warnings involve a solution where the potential problem is recognized and dealt with, but it shouldn't get to the point where devs are just ignoring all the warnings. That kind of defeats the purpose of warnings, doesn't it?

[energyman]

3) the code might be fishy in usual scenarios but in that special case the best/only possible solution.
4) the compiler can not know everything the programmer knows.

[wswartzendruber]

Here's what I got when compiling 2.6.27-git8:

CC drivers/gpu/drm/drm_auth.o
CC drivers/gpu/drm/drm_bufs.o
CC drivers/gpu/drm/drm_cache.o
CC drivers/gpu/drm/drm_context.o
CC drivers/gpu/drm/drm_dma.o
CC drivers/gpu/drm/drm_drawable.o
CC drivers/gpu/drm/drm_drv.o
CC drivers/gpu/drm/drm_fops.o
CC drivers/gpu/drm/drm_gem.o
CC drivers/gpu/drm/drm_ioctl.o
CC drivers/gpu/drm/drm_irq.o
CC drivers/gpu/drm/drm_lock.o
CC drivers/gpu/drm/drm_memory.o
CC drivers/gpu/drm/drm_proc.o
drivers/gpu/drm/drm_proc.c: In function 'drm_gem_one_name_info':
drivers/gpu/drm/drm_proc.c:525: warning: format '%d' expects type 'int', but argument 3 has type 'size_t'
drivers/gpu/drm/drm_proc.c:533: warning: format '%9d' expects type 'int', but argument 4 has type 'size_t'
CC drivers/gpu/drm/drm_stub.o
CC drivers/gpu/drm/drm_vm.o
CC drivers/gpu/drm/drm_agpsupport.o
CC drivers/gpu/drm/drm_scatter.o
CC drivers/gpu/drm/ati_pcigart.o
CC drivers/gpu/drm/drm_pci.o
CC drivers/gpu/drm/drm_sysfs.o
CC drivers/gpu/drm/drm_hashtab.o
CC drivers/gpu/drm/drm_sman.o
CC drivers/gpu/drm/drm_mm.o
CC drivers/gpu/drm/drm_ioc32.o
LD drivers/gpu/drm/drm.o
CC drivers/gpu/drm/i915/i915_drv.o
CC drivers/gpu/drm/i915/i915_dma.o
CC drivers/gpu/drm/i915/i915_irq.o
CC drivers/gpu/drm/i915/i915_mem.o
CC drivers/gpu/drm/i915/i915_opregion.o
CC drivers/gpu/drm/i915/i915_suspend.o
CC drivers/gpu/drm/i915/i915_gem.o
drivers/gpu/drm/i915/i915_gem.c: In function 'i915_gem_gtt_pwrite':
drivers/gpu/drm/i915/i915_gem.c:184: warning: unused variable 'vaddr_atomic'
CC drivers/gpu/drm/i915/i915_gem_debug.o
CC drivers/gpu/drm/i915/i915_gem_proc.o
CC drivers/gpu/drm/i915/i915_gem_tiling.o
CC drivers/gpu/drm/i915/i915_ioc32.o
LD drivers/gpu/drm/i915/i915.o
LD drivers/gpu/drm/i915/built-in.o
LD drivers/gpu/drm/built-in.o
LD drivers/gpu/built-in.o

[Ex-Cyber]

Do you think any kernel distro maintainers trawl the warnings in their build logs?

You must live in a different world than everyone else.

Dave.Just to be clear, I didn't mean my post as a jab at you or Fedora/Red Hat, but rather that it bugs me that such a situation is generally considered normal. My inner Dijkstra pipes up at the oddest times, I guess...

[airlied]

Hmm... When I write new code, I watch how it compiles, no matter how big the backdrop it's living in.

Linus is right, people need to take more pride in their work and actually care about their results. Cavalier attitudes like that just give fodder to managers who believe that developers aren't important and are freely interchangeable.

Do you then go and recompile the code on 32-bit/64-bit, powerpc/sparc etc..

The problem was warnings on a platform the code wasn't developed on.

Normally linux-next picks up this but it was on a holiday during this development cycle.

Dave.

[deanjo]

Do you then go and recompile the code on 32-bit/64-bit, powerpc/sparc etc..

If my code is meant to be cross architecture you better believe I do.

[highlandsun]

All of my dev machines are 64 bit now but I keep a couple 32 bit VMs around for compatibility. Those get run at just about every commit. I don't do PowerPC or SPARC builds more than once a week, but I get all the bases covered before proposing anything for release.

[DeepDayze]

Unfortunately, some developers are satisfied with code that is "good enough" and don't take pride in their work.

Sadly that is all too true and its very pervasive within certain circles.

[Vighy]

The problem was warnings on a platform the code wasn't developed on.


I may be wrong but looking at the warnings and not at the code, they seem to be related to the code itself and not to the platform...

[josvazg]

I don't know if Linus says so based upon of inspection of the code delivered, but it is really so crap that you don't even need to go down to the code to see it.

Intel's linux drivers performance has been decreasing from Ubuntu 7.10 (and distros of its same age) to now, even before the GEM phase.

With Ubuntu 7.04's intel drivers, you could easily get 1500fps at glxgears, on Ubuntu 7.10 you got less that 1500fps, 8.04 lowers the mark to 1100fps, 8.10 to 500pfs and in current JAUNTY alphas TO 300fps!!
(Source: https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/252094)

There are also rendering errors apart from the performance loss.

I thought that the main goals of graphics driver software were stability AND performance. For the time being, both Windows closed source intel drivers and Nvidias ones (Win&Linux) KICKs Intel opensource's drivers ASS.

That's a fact that is being proved TRUE since Ubuntu 7.10, that is October 2007, quite a long time if you ask me.

[TechMage89]

It's not clear what's causing the performace problems in Phoronix's test. Ubuntu has been known to package broken versions of stuff in their betas (and even sometimes in the final version of their distro). It's also not clear what the default settings were.

That said, while GEM for intel ships with the 2.6.28 kernel, the drivers have to take advantage of it extensively (the only use so far is UXA which is still experimental and not enabled by default). As you can see by the Phoronix UXA test results, in most cases, UXA gives a substantial boost in performance.

That said, stuff is going to be a bit unstable while the new technology is introduced. People in charge of releases and distros are doing their best to keep things from breaking in mainline versions, but not every possible problem can be discovered or weeded out by the devs prior to release. For the first time in a number of years, the entire graphics system is being redesigned, and there are likely to be some hiccups. However, the new design will ultimately suit modern hardware and software requirements much better, improving performance, and feature-set.

[wswartzendruber]

Here's a question: Is Gallium3D going to make GEM obsolete?

[Mithrandir]

Here's a question: Is Gallium3D going to make GEM obsolete?

No. Gallium3D a replacement for Mesa (3D framework). GEM is a memory manager. So they are orthogonal.

[bridgman]

Yep. Strictly speaking Gallium3D is a replacement for the HW driver subsystem of Mesa rather than replacing Mesa itself, and other acceleration APIs may call Gallium3D directly, but as Mithrandir said it is definitely orthogonal to GEM.

GEM and TTM are the ones which kinda compete -- in theory GEM is making TTM obsolete although in practice it seems that GEM may not be sufficient for discrete GPUs with separate video memory, so the current direction for ATI parts has been to build a GEM API over the current TTM implementation.

[josvazg]

When something is new and unstable it usually is wise to keep the old stuff around so users can choose to fallback to it while the new stuff gets stable and has a good performance.

Why didn't they choose this path?
Why should we suffer poor linux drivers while Windows works as expected?

Couldn't they keep the closed source versions around while the opensource ones mature?

You DON'T close the old road till the new highway is ready.

More examples?
- AMD/ATI is keeping their closed source drivers while the opensource ones are being enhanced.
- The OSS to ALSA path.

It is painfully long, but the alternative is also quite long and in the meantime you have nothing working fine.

[TechMage89]

Well, it is still using the old stuff, and that's really the problem. Most of the changes being made right now are designed to work with the "new stuff," which sometimes means that they don't work as well with the old stuff. The performance regressions are partially, I believe, because the drivers are using the old tech still, while all the software expects them to be using the new stuff.

I believe I read something about Ubuntu udating their intel graphics drivers to fix some issues for Jaunty, so the performance may be significantly better now.

[deanjo]

When something is new and unstable it usually is wise to keep the old stuff around so users can choose to fallback to it while the new stuff gets stable and has a good performance.

Why didn't they choose this path?
Why should we suffer poor linux drivers while Windows works as expected?

Couldn't they keep the closed source versions around while the opensource ones mature?

You DON'T close the old road till the new highway is ready.

More examples?
- AMD/ATI is keeping their closed source drivers while the opensource ones are being enhanced.
- The OSS to ALSA path.

It is painfully long, but the alternative is also quite long and in the meantime you have nothing working fine.


It seems the trend for last little while is to push stuff out into the mainstream before it's really ready. KDE 4 and PulseAudio are a few other examples where it is made a "defacto standard" before they are really ready for prime time. It seems what we all have bitched about game devels pushing out crap before it's ready has now become a development model to follow for the rest of the industry. Release and fix later. It's really too bad, it hurts linux's rep for stability.

[monraaf]

Intel's linux drivers performance has been decreasing from Ubuntu 7.10 (and distros of its same age) to now, even before the GEM phase.

With Ubuntu 7.04's intel drivers, you could easily get 1500fps at glxgears, on Ubuntu 7.10 you got less that 1500fps, 8.04 lowers the mark to 1100fps, 8.10 to 500pfs and in current JAUNTY alphas TO 300fps!!

60fps ought to be enough for anybody ;)

Seriously though I don't think glxgears is really useful for performance measurement. On my 780G IGP I "only" get between 900 and 1000fps with glxgears, but I do get playable frame rates with Half Life 2 Episode Two at 1280x800, and that's what matters. Not some stupid artificial "benchmark".

[Ex-Cyber]

60fps ought to be enough for anybody ;)

Seriously though I don't think glxgears is really useful for performance measurement. On my 780G IGP I "only" get between 900 and 1000fps with glxgears, but I do get playable frame rates with Half Life 2 Episode Two at 1280x800, and that's what matters. Not some stupid artificial "benchmark".Yep. If memory serves, glxgears is (to the extent that it's a benchmark) just as much a benchmark of how fast the back buffer can be erased/flipped as it is a benchmark of how fast a 3D scene can be drawn, and as such is extremely sensitive to any kind of per-frame API overhead. In any case, it doesn't represent any kind of realistic application. Wasn't it necessary in some builds (not sure if it was stock for some period or a distro addition) to add a flag like --i-understand-that-this-program-is-not-a-benchmark in order to enable the fps output?

[bridgman]

That's the big problem with glxgears; because it only exercises the most basic functions, older chips tend to run faster than newer chips (old chips spent their transistors on ROPs, newer chips spend their transistors on shader processors) and heavily optimized drivers sometimes run slower than simple drivers (if the shader compiler does more optimizing it speeds up complicated things but makes simple things run slower 'cause you often end up doing the optimizing even if there are no gains to be had).

On the other hand, sometimes glxgears just runs slow because the new driver is crap :D

[Kano]

Yes, for Xorg 7.1.1 glxgears needed an option, but -printfps did the same as -iacknowledgethatthistoolisnotabenchmark ;)

[deanjo]

Yes, for Xorg 7.1.1 glxgears needed an option, but -printfps did the same as -iacknowledgethatthistoolisnotabenchmark ;)

A flag they should have never gotten rid of.

[coolos]

Seriously though I don't think glxgears is really useful for performance measurement.
On my laptop with i915GM and on my netbook with i945GM i have always seen a strong relation between glxgears output and openarena benchmark.
Maybe there are counterexamples, but i haven't found any.

i get 1450 fps with ubuntu 8.04, and only 800 with ubuntu 8.10, and 3D effects, Google Earth, Openarena show the same gap between the 2 versions.

[Hephasteus]

I don't always agree with Linus' views, but can you blame him for blowing his lid when he has to repeatedly remind certain people of certain policies?

He knows he's being jerked around. If you can't improve windows break linux. I'm sure he's being as polite as he can possibly be but you pretty much have to use an enforcer on Intel and Microsoft.