W dniu 18 lutego 2010 21:47 użytkownik Alex Deucher <[email protected]> napisał: > 2010/2/18 Rafał Miłecki <[email protected]>: >> W dniu 18 lutego 2010 20:29 użytkownik Alex Deucher >> <[email protected]> napisał: >>> 2010/2/17 Rafał Miłecki <[email protected]>: >>>> We kept requested and current modes in many places, depending on current >>>> state. >>>> That was useless, one place for holding that is enough. >>>> >>>> Signed-off-by: Rafał Miłecki <[email protected]> >>>> --- >>>> Tested on my RV620, no problems. Alex: can you review this patch? It's your >>>> code I modify/remove in it. >>> >>> NACK. Why are you replacing pointers with copies of of the power >>> state structs? The idea is to keep one array of power states and >>> pointers to the current one, default one, and requested one. Then >>> comparing power states is just comparing pointers and when you change >>> the power state, you just update the pointer rather then memcpying the >>> entire struct. >> >> Then first of all we would need to reduce modes pointers. Keeping mode >> pointer in every state struct is/was useless. > > What do you mean? There is not a pointer to every state. There are > only 3 (1. current, 2. requested 3. default) and they are not useless. > > You have power states which defines the global state of a particular > power state and then each power state has a set of 1 or more clock > modes. So within a power state you can switch between clock modes. > You need a set of clock pointers to track the current clock mode and > the requested clock mode. You also need to know the current and > requested power state so you need pointers there as well. We could > get rid of the default pointers, but that's mostly there for > convenience to avoid having to look up the default state when we need > it. > > For example, say you had the following power tables: > > 1. 3d power state: > a. low clock mode > b. medium clock mode > c. high clock mode > > 2. battery power state > a. low clock mode > b. medium clock mode > c. high clock mode > > 3. default power state > a. low clock mode > b. medium clock mode > c. high clock mode > > > By default state 3 would be selected so the current state would be '3' > and the current clock mode would be a say 'a'. The current state > pointer would point to '3' and the current clock mode pointer would > point to 'a'. If you then went on battery, you'd want power state > '2'. At that point, the requested state pointer points to '2' and one > of the clock modes gets selected, lets say 'c', so the requested clock > mode pointer points to 'c'. When we change the power state, we > compare the current and requested power state pointers, if they are > the same, no need to change anything. if they are different, we make > the changes, then set the current power state pointer to the requested > power state pointer and the current clock mode pointer to the > requested clock mode pointer. Now say we are in power state '2' and > clock mode 'c' (the high clock for power state '2') and the system > goes idle; at the point we will want to stay in power state '2', but > select clock mode 'a' (the low clock for power state '2'). So we > compare the current and requested clock mode pointers and if they are > not equal, we change the clock mode, at which time we update the > current clock mode pointers to point to the requested clock.
Really, you didn't need to explain all of that. I understand current idea and implementation :) The thing is that in your implementation we have 8 (eight!) pointers to current clock mode and 8 pointers to requested clock mode. This part is useless IMO. Let me prepare another patch to show reduction without introducing stuct & memcpy. >> About introduced solution (keeping struct and memcpy to it) I >> introduced that to make hacking requested mode possible. Info from >> AtomBIOS about PCIE lanes seem to be useless (I've never seen anything >> else than 16) so I want to hack found mode for DPMS OFF to use 1 PCIE >> lane. Without keeping whole struct it would be impossible without >> overwriting original entry in array and loosing original info. > > That's fine for a local hack, but we don't want that upstream. If you > are planning to hack all the power tables, then why even use them? > I'm not sure we really change the lanes that often if at all on r6xx+. > In my experience, a lot of cards lock up when change the lanes or > fail to change properly which results in getting stuck at lower > numbers of lanes. I don't want to hack all power tables, just consider hacking clocks/etc for minimal (dpms off). Don't want to touch other. -- Rafał ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev -- _______________________________________________ Dri-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/dri-devel
