W dniu 18 grudnia 2009 15:17 użytkownik Jerome Glisse <gli...@freedesktop.org> napisał: > On Fri, Dec 18, 2009 at 02:27:57PM +0100, Rafał Miłecki wrote: >> W dniu 18 grudnia 2009 13:19 użytkownik Jerome Glisse >> <gli...@freedesktop.org> napisał: >> > On Thu, Dec 17, 2009 at 12:04:02AM +0100, Rafał Miłecki wrote: >> >> In future we will execute AtomBIOS commands from contexts (like power >> >> management) so we need to make sure we won't execute two commands at >> >> same time to prevent locking up GPU. >> >> >> >> With this patch applied Sedat Dilek (RV515) was able to finally test >> >> my PM patch. Also tested on my RV620. >> >> >> >> -- >> >> Rafał >> > >> > I am in favor of a different approach, using a r/w lock and >> > taking the lock in read in all path, and write in powermanager >> > & init path. Doc state that we need exclusive access to GPU >> > while doing PM stuff. Thus r/w lock sounds like what we want >> > to do. Should be hard to add that to ioctl & modesetting callback. >> > >> > Never the less this patch might be usefull, as for instance >> > we might be doing modesetting at the same time on 2 different >> > head of an avivo gpu and this might lead to // execution of >> > atombios and it's my understanding that atombios code is not >> > safe in this regards. >> >> Sorry, I'm confused. So finally: do you think we should use read/write >> locking? >> >> I guess we could make atom_execute_table using read-lock and add new >> function like atom_execute_table_write_lock using write-lock. Then >> convert sensitive places (like power management) to use _write_lock >> instead standard atom_execute_table. >> >> Not sure about that modesetting at same time. If we don't want to do 2 >> modesetting operations at same time, is there really sense to >> introduce read/write locking? When will we use read lock then? Just >> for connectors and powerplay_info reading? Sounds like wasting time >> for that. >> >> Also could you explain: >> > Should be hard to add that to ioctl & modesetting callback. >> please? >> >> -- >> Rafał >> > > To summup, this patch is good because it will exclude // > execution of atom stuff which i think is good. > > Yet i don't think it's enough for power management. The > idea of r/w lock is that we can have any number of reader > so the lock won't block anyone in any path. But once you > take the lock in write mode you are the only one messing > with the hw which is what we want for PM (according to > doc). To be effective reader lock must be taken in all > ioctl & modesetting callback, and write lock in pm path. > > I think this r/w lock needs discussion, i am not fan of > adding another lock to the driver.
But I'm not sure if we have much more time if we want to still include PM in 2.6.33 :| Is that patch OK for now? And eventually to be improved later? Not sure what do you need to discuss, but I think proposed solution would match your description. By adding read_lock in current atom.c::atom_execute_table we would make all current code (including ioctl and modesetting) use read_lock. Then adding proposed atom.c::atom_execute_table_write_lock and using it in radeon_pm.c would give as excluding access to hardware for powermanagement operations. Is there anything lacking in that? Not sure why you like or not locking, but I don't think we can solve this problem without locks :) -- Rafał ------------------------------------------------------------------------------ This SF.Net email is sponsored by the Verizon Developer Community Take advantage of Verizon's best-in-class app development support A streamlined, 14 day to market process makes app distribution fast and easy Join now and get one step closer to millions of Verizon customers http://p.sf.net/sfu/verizon-dev2dev -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
