From: Keith W. <ke...@tu...> - 2003-04-25 22:07:48
|
Leif Delgass wrote: > On Fri, 25 Apr 2003, Keith Whitwell wrote: > > >>Linus Torvalds wrote: >> >>>On Fri, 25 Apr 2003, Dave Jones wrote: >>> >>> >>>>>I wonder what I do differently that I can see this, and apparently Keith >>>>>and others cannot. >>>> >>>>What version of DRI are you using? cvs HEAD ? >>> >>> >>>Current 2.5.x DRI, which is pretty close to CVS head. But this has been >>>on-going at least for a few weeks, definitely over the last DRI->kernel >>>merge. >> >>I've tried again & still can't reproduce it. I'm using basically rh7.3 and >>dri head. My agpgart module isn't standard, being left over from our internal >>development for the i830 -- maybe that's a source of the differences? >> >>Keith > > > One thing that looks odd to me is that both the i810 and i830 DDX call the > DRM dma init before DRIFinishScreenInit, which means the lock isn't held > during the dma init. Actually, I'm wondering why none of the drivers seem > to have the lock test macro in the dma,cp,cce, etc. init/cleanup ioctl. > Other DDX drivers have comments about DRIFinishScreenInit needing to be > called before the kernel init so that the lock is held. > > The bug in 4.3.0 with the i810/i830 DDX not removing the IRQ handler (only > affecting 'older' kernel modules that used IRQs), is a good example of why > we should try not to make assumptions that userspace will call ioctls in a > certain order, or call the cleanup ioctls at all. We shouldn't oops > because userspace didn't do what we expected. Obviously -- I don't think there's a deliberate policy of doing this - at worst a selective blindness to the workings of code on the other sides of various lines in the sand. I know I don't pay a lot of attention to the code in the DDX once it appears to be working. > This is a bit offtopic, but I think all the drivers that can use IRQs > should probably call DRM(uninstall_irq) in the cleanup function if it > hasn't been done already (i.e. dev->irq != 0), and should make sure it's > cleanup function has been called on takedown -- the radeon driver does > this in DRIVER_PRETAKEDOWN(). That sounds reasonable - feel free to make these sort of cleanups as you feel motivated to do so... Keith |