From: Ionela Voinescu <ionela.voinescu@arm.com> To: Lukasz Luba <lukasz.luba@arm.com> Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, dri-devel@lists.freedesktop.org, amit.kucheria@verdurent.com, airlied@linux.ie, daniel.lezcano@linaro.org, steven.price@arm.com, alyssa.rosenzweig@collabora.com, rui.zhang@intel.com, orjan.eide@arm.com Subject: Re: [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status Date: Tue, 1 Dec 2020 14:55:36 +0000 [thread overview] Message-ID: <20201201145536.GB7206@arm.com> (raw) In-Reply-To: <2fc2031d-e38e-2a17-8667-f2fc8d4f724b@arm.com> On Tuesday 01 Dec 2020 at 12:19:18 (+0000), Lukasz Luba wrote: > > > On 12/1/20 10:36 AM, Ionela Voinescu wrote: > > Hi, > > > > Sorry for the delay and for the noise on this older version. I first > > want to understand the code better. > > > > On Thursday 22 Oct 2020 at 11:55:28 (+0100), Lukasz Luba wrote: > > [..] > > > > > > > > > > > > +{ > > > > > + /* Make some space if needed */ > > > > > + if (status->busy_time > 0xffff) { > > > > > + status->busy_time >>= 10; > > > > > + status->total_time >>= 10; > > > > > + } > > > > > > > > How about removing the above code and adding here: > > > > > > > > status->busy_time = status->busy_time ? : 1; > > > > > > It's not equivalent. The code operates on raw device values, which > > > might be big (e.g. read from counters). If it's lager than the 0xffff, > > > it is going to be shifted to get smaller. > > > > > > > Yes, the big values are handled below through the division and by making > > total_time = 1024. These two initial checks are only to cover the > > possibility for busy_time and total_time being 0, or busy_time > > > total_time. > > > > > > > > > > > + > > > > > + if (status->busy_time > status->total_time) > > > > > > > > This check would then cover the possibility that total_time is 0. > > > > > > > > > + status->busy_time = status->total_time; > > > > > > > > But a reversal is needed here: > > > > status->total_time = status->busy_time; > > > > > > No, I want to clamp the busy_time, which should not be bigger that > > > total time. It could happen when we deal with 'raw' values from device > > > counters. > > > > > > > Yes, I understand. But isn't making total_time = busy_time accomplishing > > the same thing? > > > > > > > > > > > + > > > > > + status->busy_time *= 100; > > > > > + status->busy_time /= status->total_time ? : 1; > > > > > + > > > > > + /* Avoid division by 0 */ > > > > > + status->busy_time = status->busy_time ? : 1; > > > > > + status->total_time = 100; > > > > > > > > Then all of this code can be replaced by: > > > > > > > > status->busy_time = (unsigned long)div64_u64((u64)status->busy_time << 10, > > > > status->total_time); > > > > status->total_time = 1 << 10; > > > > > > No, the total_time closed to 'unsigned long' would overflow. > > > > > > > I'm not sure I understand. total_time gets a value of 1024, it's not > > itself shifted by 10. > > > > > > > > > > This way you gain some resolution to busy_time and the divisions in the > > > > callers would just become shifts by 10. > > > > > > > > > I don't want to gain more resolution here. I want to be prepare for raw > > > (not processed yet) big values coming from driver. > > > > > > > Agreed! The higher resolution is an extra benefit. The more important > > benefit is that, through my suggestion, you'd be replacing all future > > divisions by shifts. > > You have probably missed some bits. > I don't see benefits, you have div64_u64() which is heavy on 32bit CPUs. > > Then, what is the range of these values: > busy_time [0, 1024], total_time 1024 in your case. > These values are used for estimating power in two cases: > 1. in devfreq_cooling_get_requested_power() > est_power = power * busy_time / total_time > 2. in devfreq_cooling_power2state(): > est_power = power * total_time / busy_time > > As you can see above, the est_power values could overflow if total_time, > busy_time are raw values (like in old implementation). So normalize them > into 'some' scale. That was the motivation ('scale' motivation below). > Agreed! I do think scaling is necessary, but in my mind the [0, 1024] scale made more sense. > In your case you cannot avoid division in 2. use case, because busy_time > can be any value in range [0, 1024]. > We could avoid the division in 1. use case, but load in cpufreq cooling > is also in range of [0, 100], so this devfreq cooling is aligned. I > would like to avoid situation when someone is parsing the traces > and these two devices present different load scale. > Got it! Looking through the code I did overlook that 2 was reversed. > I will think about better 'devfreq utilization' (as also Daniel > suggested)in future, but first this EM must be in mainline and cpufreq > cooling changes made by Viresh also there. > But it would be more then just scale change to [0, 1024]... > Okay, looking forward to this. It would be nice to align all of these utilization metrics in the future for all kinds of devices. Thanks, Ionela.
WARNING: multiple messages have this Message-ID (diff)
From: Ionela Voinescu <ionela.voinescu@arm.com> To: Lukasz Luba <lukasz.luba@arm.com> Cc: amit.kucheria@verdurent.com, linux-pm@vger.kernel.org, airlied@linux.ie, daniel.lezcano@linaro.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, steven.price@arm.com, alyssa.rosenzweig@collabora.com, rui.zhang@intel.com, orjan.eide@arm.com Subject: Re: [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status Date: Tue, 1 Dec 2020 14:55:36 +0000 [thread overview] Message-ID: <20201201145536.GB7206@arm.com> (raw) In-Reply-To: <2fc2031d-e38e-2a17-8667-f2fc8d4f724b@arm.com> On Tuesday 01 Dec 2020 at 12:19:18 (+0000), Lukasz Luba wrote: > > > On 12/1/20 10:36 AM, Ionela Voinescu wrote: > > Hi, > > > > Sorry for the delay and for the noise on this older version. I first > > want to understand the code better. > > > > On Thursday 22 Oct 2020 at 11:55:28 (+0100), Lukasz Luba wrote: > > [..] > > > > > > > > > > > > +{ > > > > > + /* Make some space if needed */ > > > > > + if (status->busy_time > 0xffff) { > > > > > + status->busy_time >>= 10; > > > > > + status->total_time >>= 10; > > > > > + } > > > > > > > > How about removing the above code and adding here: > > > > > > > > status->busy_time = status->busy_time ? : 1; > > > > > > It's not equivalent. The code operates on raw device values, which > > > might be big (e.g. read from counters). If it's lager than the 0xffff, > > > it is going to be shifted to get smaller. > > > > > > > Yes, the big values are handled below through the division and by making > > total_time = 1024. These two initial checks are only to cover the > > possibility for busy_time and total_time being 0, or busy_time > > > total_time. > > > > > > > > > > > + > > > > > + if (status->busy_time > status->total_time) > > > > > > > > This check would then cover the possibility that total_time is 0. > > > > > > > > > + status->busy_time = status->total_time; > > > > > > > > But a reversal is needed here: > > > > status->total_time = status->busy_time; > > > > > > No, I want to clamp the busy_time, which should not be bigger that > > > total time. It could happen when we deal with 'raw' values from device > > > counters. > > > > > > > Yes, I understand. But isn't making total_time = busy_time accomplishing > > the same thing? > > > > > > > > > > > + > > > > > + status->busy_time *= 100; > > > > > + status->busy_time /= status->total_time ? : 1; > > > > > + > > > > > + /* Avoid division by 0 */ > > > > > + status->busy_time = status->busy_time ? : 1; > > > > > + status->total_time = 100; > > > > > > > > Then all of this code can be replaced by: > > > > > > > > status->busy_time = (unsigned long)div64_u64((u64)status->busy_time << 10, > > > > status->total_time); > > > > status->total_time = 1 << 10; > > > > > > No, the total_time closed to 'unsigned long' would overflow. > > > > > > > I'm not sure I understand. total_time gets a value of 1024, it's not > > itself shifted by 10. > > > > > > > > > > This way you gain some resolution to busy_time and the divisions in the > > > > callers would just become shifts by 10. > > > > > > > > > I don't want to gain more resolution here. I want to be prepare for raw > > > (not processed yet) big values coming from driver. > > > > > > > Agreed! The higher resolution is an extra benefit. The more important > > benefit is that, through my suggestion, you'd be replacing all future > > divisions by shifts. > > You have probably missed some bits. > I don't see benefits, you have div64_u64() which is heavy on 32bit CPUs. > > Then, what is the range of these values: > busy_time [0, 1024], total_time 1024 in your case. > These values are used for estimating power in two cases: > 1. in devfreq_cooling_get_requested_power() > est_power = power * busy_time / total_time > 2. in devfreq_cooling_power2state(): > est_power = power * total_time / busy_time > > As you can see above, the est_power values could overflow if total_time, > busy_time are raw values (like in old implementation). So normalize them > into 'some' scale. That was the motivation ('scale' motivation below). > Agreed! I do think scaling is necessary, but in my mind the [0, 1024] scale made more sense. > In your case you cannot avoid division in 2. use case, because busy_time > can be any value in range [0, 1024]. > We could avoid the division in 1. use case, but load in cpufreq cooling > is also in range of [0, 100], so this devfreq cooling is aligned. I > would like to avoid situation when someone is parsing the traces > and these two devices present different load scale. > Got it! Looking through the code I did overlook that 2 was reversed. > I will think about better 'devfreq utilization' (as also Daniel > suggested)in future, but first this EM must be in mainline and cpufreq > cooling changes made by Viresh also there. > But it would be more then just scale change to [0, 1024]... > Okay, looking forward to this. It would be nice to align all of these utilization metrics in the future for all kinds of devices. Thanks, Ionela. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-12-01 14:56 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-09-21 12:20 [PATCH 0/5] Thermal devfreq cooling improvements with Energy Model Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba 2020-09-21 12:20 ` [PATCH 1/5] thermal: devfreq_cooling: change tracing function and arguments Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba 2020-09-21 12:20 ` [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba 2020-10-07 16:11 ` Ionela Voinescu 2020-10-07 16:11 ` Ionela Voinescu 2020-10-22 10:55 ` Lukasz Luba 2020-10-22 10:55 ` Lukasz Luba 2020-12-01 10:36 ` Ionela Voinescu 2020-12-01 10:36 ` Ionela Voinescu 2020-12-01 12:19 ` Lukasz Luba 2020-12-01 12:19 ` Lukasz Luba 2020-12-01 14:55 ` Ionela Voinescu [this message] 2020-12-01 14:55 ` Ionela Voinescu 2020-10-14 14:34 ` Daniel Lezcano 2020-10-14 14:34 ` Daniel Lezcano 2020-10-22 11:45 ` Lukasz Luba 2020-10-22 11:45 ` Lukasz Luba 2020-09-21 12:20 ` [PATCH 3/5] thermal: devfreq_cooling: add new registration functions with Energy Model Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba 2020-10-07 12:07 ` Ionela Voinescu 2020-10-07 12:07 ` Ionela Voinescu 2020-10-22 11:17 ` Lukasz Luba 2020-10-22 11:17 ` Lukasz Luba 2020-12-01 14:05 ` Ionela Voinescu 2020-12-01 14:05 ` Ionela Voinescu 2020-12-01 14:37 ` Lukasz Luba 2020-12-01 14:37 ` Lukasz Luba 2020-12-01 15:02 ` Ionela Voinescu 2020-12-01 15:02 ` Ionela Voinescu 2020-09-21 12:20 ` [PATCH 4/5] thermal: devfreq_cooling: remove old power model and use EM Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba 2020-10-07 15:12 ` Ionela Voinescu 2020-10-07 15:12 ` Ionela Voinescu 2020-10-22 11:26 ` Lukasz Luba 2020-10-22 11:26 ` Lukasz Luba 2020-09-21 12:20 ` [PATCH 5/5] drm/panfrost: Register devfreq cooling and attempt to add Energy Model Lukasz Luba 2020-09-21 12:20 ` Lukasz Luba
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201201145536.GB7206@arm.com \ --to=ionela.voinescu@arm.com \ --cc=airlied@linux.ie \ --cc=alyssa.rosenzweig@collabora.com \ --cc=amit.kucheria@verdurent.com \ --cc=daniel.lezcano@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=lukasz.luba@arm.com \ --cc=orjan.eide@arm.com \ --cc=rui.zhang@intel.com \ --cc=steven.price@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.