From: Leif D. <lde...@re...> - 2001-10-27 17:55:39
|
Well, I just got my box to hang hard (like with the vt switching) when running tuxracer and switching modes with Ctrl-Alt-+ (I have 3 modes defined in my config and the hang happened when looping back to the original mode, i.e. the third switch), so I think the answer is yes, it needs locking. I really should use a journalling filesystem, all this fsck-ing is getting a bit tedious. ;) --Leif -- Leif Delgass |
From: Manuel T. <man...@so...> - 2001-10-27 19:09:12
|
El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > Well, I just got my box to hang hard (like with the vt switching) when > running tuxracer and switching modes with Ctrl-Alt-+ (I have 3 modes > defined in my config and the hang happened when looping back to the > original mode, i.e. the third switch), so I think the > answer is yes, it needs locking. I really should use a journalling > filesystem, all this fsck-ing is getting a bit tedious. ;) > OK. I have added the DRILock/Unlock to the AtiModeSet function in the atimode.c file. I've added another condition (also to the locks in the aticonsole.c file for vt changing) in this way: #ifdef XF86DRI if (pATI->Chip >=3D ATI_CHIP_264GT && pATI->directRenderingEnabled) { DRILock(pScreen,0); } #endif ... #ifdef XF86DRI if (pATI->Chip >=3D ATI_CHIP_264GT && pATI->directRenderingEnabled) { DRIUnlock(pScreen); } #endif I think this is a better way because the code in aticonsole.c and atimod= e.c is generic for all the ATI adapters, so we are restricting the checking = to the Mach64 chips greater than the GT that is the first one (if I'm not mistaken) supporting 3D. I'm also changing the related macros for locking the 2D accelerated func= tions. > --Leif |
From: Leif D. <lde...@re...> - 2001-10-27 19:47:01
Attachments:
m64-locking-macro.patch.gz
|
On Sat, 27 Oct 2001, Manuel Teira wrote: > El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > > Well, I just got my box to hang hard (like with the vt switching) whe= n > > running tuxracer and switching modes with Ctrl-Alt-+ (I have 3 modes > > defined in my config and the hang happened when looping back to the > > original mode, i.e. the third switch), so I think the > > answer is yes, it needs locking. I really should use a journalling > > filesystem, all this fsck-ing is getting a bit tedious. ;) > > >=20 > OK. I have added the DRILock/Unlock to the AtiModeSet function in the=20 > atimode.c file. I've added another condition (also to the locks in the=20 > aticonsole.c file for vt changing) in this way: It looks like ATIEnter/LeaveVT calls ATIEnter/LeaveGraphics, which in tur= n calls ATIModeSet. Won't this lead to trying to obtain a lock when the lock is already secured? It might be better to put the lock in aticonsole.c in the ATIModeSwitch function. =20 > #ifdef XF86DRI > if (pATI->Chip >=3D ATI_CHIP_264GT && > pATI->directRenderingEnabled) > { > DRILock(pScreen,0); > } > #endif > ... > #ifdef XF86DRI > if (pATI->Chip >=3D ATI_CHIP_264GT && > pATI->directRenderingEnabled) > { > DRIUnlock(pScreen); > } > #endif >=20 >=20 > I think this is a better way because the code in aticonsole.c and atimo= de.c=20 > is generic for all the ATI adapters, so we are restricting the checking= to=20 > the Mach64 chips greater than the GT that is the first one (if I'm not=20 > mistaken) supporting 3D. Yep, that looks better. > I'm also changing the related macros for locking the 2D accelerated fun= ctions. OK. I'm attaching a small patch that defines and uses a convenience macr= o used by the other drm drivers (LOCK_TEST_WITH_RETURN). We can use this when we add more ioctls. The patch also includes the FIFO size defines, but you can just say no to that and delete the .rej file. Another thing I noticed is that all the other 2D/dri drivers notify the=20 xf86 module loader about DRI/DRM symbols using [xf86]LoaderRefSymLists, i= n=20 addition to other symbols (see R128PreInit in r128_driver.c for an=20 example). I can't find anywhere that the ati/mach64 driver does this, bu= t=20 it doesn't seem to be causing any problems. On a totally unrelated note, do you know how to get etags to index K&R=20 style functions like the ones in aticonsole.c? --=20 Leif Delgass=20 |
From: Manuel T. <man...@so...> - 2001-10-27 20:42:58
|
El S=E1b 27 Oct 2001 21:40, Leif Delgass escribi=F3: > On Sat, 27 Oct 2001, Manuel Teira wrote: > > El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > > > Well, I just got my box to hang hard (like with the vt switching) = when > > > running tuxracer and switching modes with Ctrl-Alt-+ (I have 3 mod= es > > > defined in my config and the hang happened when looping back to th= e > > > original mode, i.e. the third switch), so I think the > > > answer is yes, it needs locking. I really should use a journallin= g > > > filesystem, all this fsck-ing is getting a bit tedious. ;) > > > > OK. I have added the DRILock/Unlock to the AtiModeSet function in th= e > > atimode.c file. I've added another condition (also to the locks in t= he > > aticonsole.c file for vt changing) in this way: > > It looks like ATIEnter/LeaveVT calls ATIEnter/LeaveGraphics, which in = turn > calls ATIModeSet. Won't this lead to trying to obtain a lock when the > lock is already secured? It might be better to put the lock in > aticonsole.c in the ATIModeSwitch function. You are right, I was deadlocking the server. Anyway, the idea is that ATIModeSet has to Lock and Unlock, but ATIEnterVT(LeaveVT) has only to Unlock(Lock) the DRM. We could Lock/Unlock in ATISwitchMode, but I thi= nk that we are locking more that needed, because the ATIModeCalculate that = is also called from ATISwitchMode doesn't need to be locked. I think that the better way to do this is: ATIEnterVT: Unlock at the end of the Function (to avoid interlocks) ATILeaveVT: Lock at the end of the Function (to avoid interlocks) ATIModeSet: Lock at the beginning and Unlock at the end. What do you thing about this? > OK. I'm attaching a small patch that defines and uses a convenience m= acro > used by the other drm drivers (LOCK_TEST_WITH_RETURN). We can use thi= s > when we add more ioctls. The patch also includes the FIFO size define= s, > but you can just say no to that and delete the .rej file. All right. I've commited the changes to the CVS branch yet, but I have t= o fix the deadlock you've found, so, I'll also send the changes you've sent me= . > > On a totally unrelated note, do you know how to get etags to index K&R > style functions like the ones in aticonsole.c? No. Sorry. |
From: Leif D. <lde...@re...> - 2001-10-27 22:17:05
|
On Sat, 27 Oct 2001, Manuel Teira wrote: > El S=E1b 27 Oct 2001 21:40, Leif Delgass escribi=F3: > > On Sat, 27 Oct 2001, Manuel Teira wrote: > > > El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > > > > Well, I just got my box to hang hard (like with the vt switching)= when > > > > running tuxracer and switching modes with Ctrl-Alt-+ (I have 3 mo= des > > > > defined in my config and the hang happened when looping back to t= he > > > > original mode, i.e. the third switch), so I think the > > > > answer is yes, it needs locking. I really should use a journalli= ng > > > > filesystem, all this fsck-ing is getting a bit tedious. ;) > > > > > > OK. I have added the DRILock/Unlock to the AtiModeSet function in t= he > > > atimode.c file. I've added another condition (also to the locks in = the > > > aticonsole.c file for vt changing) in this way: > > > > It looks like ATIEnter/LeaveVT calls ATIEnter/LeaveGraphics, which in= turn > > calls ATIModeSet. Won't this lead to trying to obtain a lock when th= e > > lock is already secured? It might be better to put the lock in > > aticonsole.c in the ATIModeSwitch function. >=20 > You are right, I was deadlocking the server. Anyway, the idea is that=20 > ATIModeSet has to Lock and Unlock, but ATIEnterVT(LeaveVT) has only to=20 > Unlock(Lock) the DRM. We could Lock/Unlock in ATISwitchMode, but I th= ink=20 > that we are locking more that needed, because the ATIModeCalculate that= is=20 > also called from ATISwitchMode doesn't need to be locked. >=20 > I think that the better way to do this is: >=20 > ATIEnterVT: Unlock at the end of the Function (to avoid interlocks) > ATILeaveVT: Lock at the end of the Function (to avoid interlocks) > ATIModeSet: Lock at the beginning and Unlock at the end. >=20 > What do you thing about this? Well, let's look at the sequence assuming what you suggest. This is as=20 much for me to get it straight in my head as anything else... Xserver is running. We switch to text mode: 1. ATILeaveVT is called, which calls ATILeaveGraphics 2. ATIModeSave 3. ATIModeSet (DRILock,DRIUnlock) 4. ATILock, return from ATILeaveGraphics 5. DRILock I'm not sure it's worth unlocking just to lock again here. Text mode is running. We switch to graphics mode: 1. ATIEnterVT is called, which calls ATIEnterGraphics 2. ATIUnlock 3. ATIModeCalculate 4. ATIModeSave 5. ATIModeSet (DRILock,DRIUnlock), return from ATIEnterGraphics Oops!, at this point we still hold the DRILock, so again we'll be trying=20 to lock twice, right? 6. DRIUnlock Here we've already unlocked. Well, we could unlock at the _beginning_ of ATIEnterVT, but here's what I= =20 would suggest... Don't do locking/unlocking in ATIModeSet, but do the locking in the three= =20 functions in aticonsole.c: ATILeaveVT() 1. DRILock, then call ATILeaveGraphics 2. ATIModeSave 3. ATIModeSet (ok, we've got the lock already) 4. ATILock, return from ATILeaveGraphics 5. return ATIEnterVT() 1. call ATIEnterGraphics 2. ATIUnlock 3. ATIModeCalculate 4. ATIModeSave 5. ATIModeSet (we still hold the DRILock), return 6. DRIUnlock, return =20 ATISwitchMode() 1. ATIModeCalculate 2. DRILock 3. ATIModeSet 4. DRIUnlock, return Here you can still avoid locking until after the mode calculate. Also ATIModeSet is called by ATIClockPreInit, and I don't think DRI locking is necessary at that point (in fact, I think it happens before the DRI initialization?). Of course, this is my high-level view (I haven't followed all the code in the sequence in detail) and I could be missing something. I'm also not sure exactly _where_ the lockup happens and so I'm not sure how fine grained the locking can get. What do you think? --Leif --=20 Leif Delgass=20 |
From: Manuel T. <man...@so...> - 2001-10-28 10:22:29
|
El Dom 28 Oct 2001 00:10, Leif Delgass escribi=F3: > On Sat, 27 Oct 2001, Manuel Teira wrote: > > El S=E1b 27 Oct 2001 21:40, Leif Delgass escribi=F3: > > > On Sat, 27 Oct 2001, Manuel Teira wrote: > > > > El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > > > > > Well, I just got my box to hang hard (like with the vt switchi= ng) > > > > > when running tuxracer and switching modes with Ctrl-Alt-+ (I h= ave 3 > > > > > modes defined in my config and the hang happened when looping = back > > > > > to the original mode, i.e. the third switch), so I think the > > > > > answer is yes, it needs locking. I really should use a journa= lling > > > > > filesystem, all this fsck-ing is getting a bit tedious. ;) > > > > > > > > OK. I have added the DRILock/Unlock to the AtiModeSet function i= n the > > > > atimode.c file. I've added another condition (also to the locks = in > > > > the aticonsole.c file for vt changing) in this way: > > > > > > It looks like ATIEnter/LeaveVT calls ATIEnter/LeaveGraphics, which= in > > > turn calls ATIModeSet. Won't this lead to trying to obtain a lock= when > > > the lock is already secured? It might be better to put the lock i= n > > > aticonsole.c in the ATIModeSwitch function. > > > > You are right, I was deadlocking the server. Anyway, the idea is tha= t > > ATIModeSet has to Lock and Unlock, but ATIEnterVT(LeaveVT) has only = to > > Unlock(Lock) the DRM. We could Lock/Unlock in ATISwitchMode, but I > > think that we are locking more that needed, because the ATIModeCalcu= late > > that is also called from ATISwitchMode doesn't need to be locked. > > > > I think that the better way to do this is: > > > > ATIEnterVT: Unlock at the end of the Function (to avoid interlocks) Well, here I wanted to say 'at the beginning of the Function'. Sorry. > > ATILeaveVT: Lock at the end of the Function (to avoid interlocks) > > ATIModeSet: Lock at the beginning and Unlock at the end. > > > > What do you thing about this? > > Well, let's look at the sequence assuming what you suggest. This is a= s > much for me to get it straight in my head as anything else... > > Xserver is running. We switch to text mode: > > 1. ATILeaveVT is called, which calls ATILeaveGraphics > 2. =09ATIModeSave > 3. =09ATIModeSet (DRILock,DRIUnlock) > 4. =09ATILock, return from ATILeaveGraphics > 5. DRILock > > I'm not sure it's worth unlocking just to lock again here. > > Text mode is running. We switch to graphics mode: > > 1. ATIEnterVT is called, which calls ATIEnterGraphics > 2. =09ATIUnlock > 3. =09ATIModeCalculate > 4. =09ATIModeSave > 5. =09ATIModeSet (DRILock,DRIUnlock), return from ATIEnterGraphics > Oops!, at this point we still hold the DRILock, so again we'll be tryi= ng > to lock twice, right? > 6. DRIUnlock > Here we've already unlocked. Yes, you're right. Too late to think yesterday. > > Well, we could unlock at the _beginning_ of ATIEnterVT, but here's wha= t I > would suggest... > > Don't do locking/unlocking in ATIModeSet, but do the locking in the th= ree > functions in aticonsole.c: > > ATILeaveVT() > 1. DRILock, then call ATILeaveGraphics > 2. ATIModeSave > 3. ATIModeSet (ok, we've got the lock already) > 4. ATILock, return from ATILeaveGraphics > 5. return > > ATIEnterVT() > 1. call ATIEnterGraphics > 2. ATIUnlock > 3. ATIModeCalculate > 4. ATIModeSave > 5. ATIModeSet (we still hold the DRILock), return > 6. DRIUnlock, return > > ATISwitchMode() > 1. ATIModeCalculate > 2. DRILock > 3. ATIModeSet > 4. DRIUnlock, return > At a first glance, it looks that this aproximation is the best. The idea= is use for the locking the upper functions to avoid interlock problems. We = do not want to use our locks from 2 levels of functions. In this way, I thi= nk there is no problem. > Here you can still avoid locking until after the mode calculate. Also > ATIModeSet is called by ATIClockPreInit, and I don't think DRI locking= is > necessary at that point (in fact, I think it happens before the DRI > initialization?). Of course, this is my high-level view (I haven't > followed all the code in the sequence in detail) and I could be missin= g > something. I'm also not sure exactly _where_ the lockup happens and s= o > I'm not sure how fine grained the locking can get. What do you think? My idea is that we don't need more detail. We just need to lock when a m= ode change is made (both for going to text mode or changing the screen mode)= and that changes are made with the API defined in aticonsole.c. <snip src=3D"from your other mail"> OK, I actually looked at the definition of DRILock/DRIUnlock (programs/Xserver/GL/dri/dri.c), and it does reference counting, so locking twice might not be an issue, it just increments the reference count. What I haven't found yet is where the DRM_LOCK/DRM_UNLOCK macros used in DRILock/DRIUnlock are defined, and I'm not sure how contention i= s handled either. So I'm going to try and get a better idea of how the locking is actually implemented, and hopefully I know what I'm talking about next time. </snip> I still think that we should avoid more than one consecutive locks. It h= as nonsense from my point of view, at least in this case. The DRM_LOCK/DRM_UNLOCK macros are defined in: xc/programs/Xserver/hw/xfree86/os-support/xf86drm.h and a lot of other things I don't feel strong enough to look at now. ;-= ) And to finish, I think that your last aproximation is the good one. I wi= ll look again at it this night, touch the code in that way and commit these= and your other changes to the CVS. BTW, your yesterday question: "do you know how to get etags to index K&R style functions like the ones= in aticonsole.c" I thought that K&R style was this way: int foo(a,b) int a; int b; { /* Function body */ } Or something so, isn't it? But the functions in aticonsole.c are ANSI-C defined, arent't they, but = with a little unusual indenting: int foo( int a, int b ) |
From: Leif D. <lde...@re...> - 2001-10-28 17:24:03
|
On Sun, 28 Oct 2001, Manuel Teira wrote: [snip] > I still think that we should avoid more than one consecutive locks. It > has nonsense from my point of view, at least in this case. The > DRM_LOCK/DRM_UNLOCK macros are defined in: > xc/programs/Xserver/hw/xfree86/os-support/xf86drm.h and a lot of other > things I don't feel strong enough to look at now. ;-) Yes, if we can avoid multiple locks, it's better. I did finally track down the macros and re-read the locking doc on the PI site. The LOCK/UNLOCK_HARDWARE macro in the mesa code looks like it is using the drm locking scheme. I have a way to go yet to understand all the mesa stuff, but it looks like there's a good deal of work to do there. There are a lot of disabled bits of code and everything is being flushed with mach64EmitHwStateLocked. We'll need to get the AGP stuff done (with fallback for PCI) and finish the drm ioctls before we can get mesa using DMA buffers, though. BTW, I noticed one other place where the LOCK_TEST_WITH_RETURN can be used in mach64_dma.c in mach64_dma_idle(). > And to finish, I think that your last aproximation is the good one. I will > look again at it this night, touch the code in that way and commit these and > your other changes to the CVS. OK. I did test it out doing it in the way I suggested, but I'm still getting a lockup. I was thinking of compiling with the debugging macros turned on to get a better idea of where the problem is. Since the problem doesn't seem to happen with gears, I wonder if there's something in the mesa code that isn't locking properly. > BTW, your yesterday question: > "do you know how to get etags to index K&R style functions like the ones in > aticonsole.c" > I thought that K&R style was this way: > int foo(a,b) > int a; > int b; > { > /* Function body */ > } > > Or something so, isn't it? > > But the functions in aticonsole.c are ANSI-C defined, arent't they, but with > a little unusual indenting: > int > foo( > int a, > int b > ) Yes, you're right. It turns out that etags indexes these fine, but I'm not able to expand the function list in the emacs speedbar, so it may be that the speedbar code is using a regex that's looking for the function names and args on the same line. I'll have to learn a little more lisp, I think. ;) --Leif -- Leif Delgass |
From: Manuel T. <man...@so...> - 2001-10-28 20:50:42
|
El Dom 28 Oct 2001 19:17, Leif Delgass escribi=F3: <snip> > OK. I did test it out doing it in the way I suggested, but I'm still > getting a lockup. I was thinking of compiling with the debugging macr= os > turned on to get a better idea of where the problem is. Since the pro= blem > doesn't seem to happen with gears, I wonder if there's something in th= e > mesa code that isn't locking properly. Have in mind that the Mesa layer is now writing directly to the register= s. Are this direct writes locked in all the Mesa lib? For example, and at a= first glance, the function: mach64UploadHwStateLocked @ mach64_ioctl.c is making WRITE access to the card without calling LOCK_HARDWARE. Perhap= s this funcion is called from another place were the lock takes place, I d= on't know. Anyway, when the DMA work takes shape on the Mesa layer, the place= s where the lock is needed will be less than now (I think), only when the = DMA buffers are flushed via a BusMaster operation. So, I think that we must not worry about the interlock while the DMA wor= k is not finished. Frank, how is it going? Any news? Well, I've just commited the changes to the CVS. The Sunday is finishing= and a new week of hard work is beginning :-( . At least here in Spain the we= ek is shorter (it's only 4 work days). ;-) Best regards. -- Manuel Teira |
From: Leif D. <lde...@re...> - 2001-10-28 21:30:57
|
On Sun, 28 Oct 2001, Manuel Teira wrote: > El Dom 28 Oct 2001 19:17, Leif Delgass escribi=F3: >=20 > <snip> >=20 > > OK. I did test it out doing it in the way I suggested, but I'm still > > getting a lockup. I was thinking of compiling with the debugging mac= ros > > turned on to get a better idea of where the problem is. Since the pr= oblem > > doesn't seem to happen with gears, I wonder if there's something in t= he > > mesa code that isn't locking properly. >=20 > Have in mind that the Mesa layer is now writing directly to the registe= rs.=20 > Are this direct writes locked in all the Mesa lib? For example, and at = a=20 > first glance, the function: > =20 > mach64UploadHwStateLocked @ mach64_ioctl.c >=20 > is making WRITE access to the card without calling LOCK_HARDWARE. Perha= ps=20 > this funcion is called from another place were the lock takes place, I = don't=20 > know.=20 In theory, it should only be called after locking, which is why Locked is in the function name, I think (and from what I can tell from a quick scan of the code, it is being called after locking). I tried testing the changes again and remembered to look at the X log when I rebooted before starting X again. I got a series (19) of "DRIUnlock called when not locked" messages. I'm going to try to trace all the DRILock/Unlock calls and see if I can track this down. > Anyway, when the DMA work takes shape on the Mesa layer, the places=20 > where the lock is needed will be less than now (I think), only when the= DMA=20 > buffers are flushed via a BusMaster operation.=20 > So, I think that we must not worry about the interlock while the DMA wo= rk is=20 > not finished.=20 >=20 > Frank, how is it going? Any news? >=20 > Well, I've just commited the changes to the CVS. The Sunday is finishin= g and=20 > a new week of hard work is beginning :-( . At least here in Spain the w= eek is=20 > shorter (it's only 4 work days). ;-) Nice. Of course, I'm currently not working so I have lots of spare time=20 to waste on this. ;) --=20 Leif Delgass=20 |
From: Frank C. E. <fe...@ai...> - 2001-10-29 15:21:08
|
On Sunday 28 October 2001 03:33 pm, Manuel Teira wrote: > is making WRITE access to the card without calling LOCK_HARDWARE. Perhaps > this funcion is called from another place were the lock takes place, I > don't know. Anyway, when the DMA work takes shape on the Mesa layer, the > places where the lock is needed will be less than now (I think), only when > the DMA buffers are flushed via a BusMaster operation. > So, I think that we must not worry about the interlock while the DMA work > is not finished. That would be my take on things. > Frank, how is it going? Any news? I have started the work. It's taking me longer than I'd originally planned because I'm having to play "find another job quick" as my employer hasn't laid me off and I'm a month and a half past due for paychecks as of Wednesday. I should have the start of something sometime this week, thankfully. -- Frank Earl |
From: Manuel T. <man...@so...> - 2001-10-28 10:22:31
|
El Dom 28 Oct 2001 00:10, Leif Delgass escribi=F3: > On Sat, 27 Oct 2001, Manuel Teira wrote: > > El S=E1b 27 Oct 2001 21:40, Leif Delgass escribi=F3: > > > On Sat, 27 Oct 2001, Manuel Teira wrote: > > > > El S=E1b 27 Oct 2001 19:49, Leif Delgass escribi=F3: > > > > > Well, I just got my box to hang hard (like with the vt switchi= ng) > > > > > when running tuxracer and switching modes with Ctrl-Alt-+ (I h= ave 3 > > > > > modes defined in my config and the hang happened when looping = back > > > > > to the original mode, i.e. the third switch), so I think the > > > > > answer is yes, it needs locking. I really should use a journa= lling > > > > > filesystem, all this fsck-ing is getting a bit tedious. ;) > > > > > > > > OK. I have added the DRILock/Unlock to the AtiModeSet function i= n the > > > > atimode.c file. I've added another condition (also to the locks = in > > > > the aticonsole.c file for vt changing) in this way: > > > > > > It looks like ATIEnter/LeaveVT calls ATIEnter/LeaveGraphics, which= in > > > turn calls ATIModeSet. Won't this lead to trying to obtain a lock= when > > > the lock is already secured? It might be better to put the lock i= n > > > aticonsole.c in the ATIModeSwitch function. > > > > You are right, I was deadlocking the server. Anyway, the idea is tha= t > > ATIModeSet has to Lock and Unlock, but ATIEnterVT(LeaveVT) has only = to > > Unlock(Lock) the DRM. We could Lock/Unlock in ATISwitchMode, but I > > think that we are locking more that needed, because the ATIModeCalcu= late > > that is also called from ATISwitchMode doesn't need to be locked. > > > > I think that the better way to do this is: > > > > ATIEnterVT: Unlock at the end of the Function (to avoid interlocks) Well, here I wanted to say 'at the beginning of the Function'. Sorry. > > ATILeaveVT: Lock at the end of the Function (to avoid interlocks) > > ATIModeSet: Lock at the beginning and Unlock at the end. > > > > What do you thing about this? > > Well, let's look at the sequence assuming what you suggest. This is a= s > much for me to get it straight in my head as anything else... > > Xserver is running. We switch to text mode: > > 1. ATILeaveVT is called, which calls ATILeaveGraphics > 2. =09ATIModeSave > 3. =09ATIModeSet (DRILock,DRIUnlock) > 4. =09ATILock, return from ATILeaveGraphics > 5. DRILock > > I'm not sure it's worth unlocking just to lock again here. > > Text mode is running. We switch to graphics mode: > > 1. ATIEnterVT is called, which calls ATIEnterGraphics > 2. =09ATIUnlock > 3. =09ATIModeCalculate > 4. =09ATIModeSave > 5. =09ATIModeSet (DRILock,DRIUnlock), return from ATIEnterGraphics > Oops!, at this point we still hold the DRILock, so again we'll be tryi= ng > to lock twice, right? > 6. DRIUnlock > Here we've already unlocked. Yes, you're right. Too late to think yesterday. > > Well, we could unlock at the _beginning_ of ATIEnterVT, but here's wha= t I > would suggest... > > Don't do locking/unlocking in ATIModeSet, but do the locking in the th= ree > functions in aticonsole.c: > > ATILeaveVT() > 1. DRILock, then call ATILeaveGraphics > 2. ATIModeSave > 3. ATIModeSet (ok, we've got the lock already) > 4. ATILock, return from ATILeaveGraphics > 5. return > > ATIEnterVT() > 1. call ATIEnterGraphics > 2. ATIUnlock > 3. ATIModeCalculate > 4. ATIModeSave > 5. ATIModeSet (we still hold the DRILock), return > 6. DRIUnlock, return > > ATISwitchMode() > 1. ATIModeCalculate > 2. DRILock > 3. ATIModeSet > 4. DRIUnlock, return > At a first glance, it looks that this aproximation is the best. The idea= is use for the locking the upper functions to avoid interlock problems. We = do not want to use our locks from 2 levels of functions. In this way, I thi= nk there is no problem. > Here you can still avoid locking until after the mode calculate. Also > ATIModeSet is called by ATIClockPreInit, and I don't think DRI locking= is > necessary at that point (in fact, I think it happens before the DRI > initialization?). Of course, this is my high-level view (I haven't > followed all the code in the sequence in detail) and I could be missin= g > something. I'm also not sure exactly _where_ the lockup happens and s= o > I'm not sure how fine grained the locking can get. What do you think? My idea is that we don't need more detail. We just need to lock when a m= ode change is made (both for going to text mode or changing the screen mode)= and that changes are made with the API defined in aticonsole.c. <snip src=3D"from your other mail"> OK, I actually looked at the definition of DRILock/DRIUnlock (programs/Xserver/GL/dri/dri.c), and it does reference counting, so locking twice might not be an issue, it just increments the reference count. What I haven't found yet is where the DRM_LOCK/DRM_UNLOCK macros used in DRILock/DRIUnlock are defined, and I'm not sure how contention i= s handled either. So I'm going to try and get a better idea of how the locking is actually implemented, and hopefully I know what I'm talking about next time. </snip> I still think that we should avoid more than one consecutive locks. It h= as nonsense from my point of view, at least in this case. The DRM_LOCK/DRM_UNLOCK macros are defined in: xc/programs/Xserver/hw/xfree86/os-support/xf86drm.h and a lot of other things I don't feel strong enough to look at now. ;-= ) And to finish, I think that your last aproximation is the good one. I wi= ll look again at it this night, touch the code in that way and commit these= and your other changes to the CVS. BTW, your yesterday question: "do you know how to get etags to index K&R style functions like the ones= in aticonsole.c" I thought that K&R style was this way: int foo(a,b) int a; int b; { /* Function body */ } Or something so, isn't it? But the functions in aticonsole.c are ANSI-C defined, arent't they, but = with a little unusual indenting: int foo( int a, int b ) { /* Function body */ } Is this correct? Anyway, I've never used etags/ctags. How do you use them? With emacs? Ar= e they really useful? Could you put me a little example of how to use this= , please? Best regards -- M. Teira |