From: manafm@codeaurora.org To: Akhil P Oommen <akhilpo@codeaurora.org> Cc: Matthias Kaehlcke <mka@chromium.org>, Doug Anderson <dianders@chromium.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, dri-devel@freedesktop.org, freedreno <freedreno@lists.freedesktop.org> Subject: Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Add gpu cooling support Date: Thu, 15 Oct 2020 00:07:01 +0530 [thread overview] Message-ID: <a4be2cf9e51e4f40aae3f9a56989a42f@codeaurora.org> (raw) In-Reply-To: <fc490021-b046-68c5-7ceb-9c63d3ff5650@codeaurora.org> On 2020-10-14 18:59, Akhil P Oommen wrote: > On 10/9/2020 10:27 PM, Matthias Kaehlcke wrote: >> On Fri, Oct 09, 2020 at 08:05:10AM -0700, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Oct 8, 2020 at 10:10 AM Akhil P Oommen >>> <akhilpo@codeaurora.org> wrote: >>>> >>>> Add cooling-cells property and the cooling maps for the gpu tzones >>>> to support GPU cooling. >>>> >>>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>>> --- >>>> arch/arm64/boot/dts/qcom/sc7180.dtsi | 29 >>>> ++++++++++++++++++++++------- >>>> 1 file changed, 22 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> index d46b383..40d6a28 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> @@ -2,7 +2,7 @@ >>>> /* >>>> * SC7180 SoC device tree source >>>> * >>>> - * Copyright (c) 2019, The Linux Foundation. All rights reserved. >>>> + * Copyright (c) 2019-20, The Linux Foundation. All rights >>>> reserved. >>>> */ >>>> >>>> #include <dt-bindings/clock/qcom,dispcc-sc7180.h> >>>> @@ -1885,6 +1885,7 @@ >>>> iommus = <&adreno_smmu 0>; >>>> operating-points-v2 = <&gpu_opp_table>; >>>> qcom,gmu = <&gmu>; >>>> + #cooling-cells = <2>; >>> >>> Presumably we should add this to the devicetree bindings, too? > Yes, thanks for catching this. Will update in the next patch. > >>> >>> >>>> interconnects = <&gem_noc MASTER_GFX3D >>>> &mc_virt SLAVE_EBI1>; >>>> interconnect-names = "gfx-mem"; >>>> @@ -3825,16 +3826,16 @@ >>>> }; >>>> >>>> gpuss0-thermal { >>>> - polling-delay-passive = <0>; >>>> + polling-delay-passive = <100>; >>> >>> Why did you make this change? I'm pretty sure that we _don't_ want >>> this since we're using interrupts for the thermal sensor. See commit >>> 22337b91022d ("arm64: dts: qcom: sc7180: Changed polling mode in >>> Thermal-zones node"). >> >> I was going to ask the same, this shouldn't be needed. As per our understanding unlike "polling-delay", this delay property is intended to activate polling thread on post trip threshold violation and it is irrespective of sensor is capable for trip interrupt or not. This polling is more of governor related. Below are the few references from Documentation/code which tells polling-delay-passive is needed for IPA for better IPA performance. As per Power allocator documentations 1. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/driver-api/thermal/power_allocator.rst?h=v5.4.71#n264 "The power allocator governor's PID controller works best if there is a periodic tick. If you have a driver that calls `thermal_zone_device_update()` (or anything that ends up calling the governor's `throttle()` function) repetitively, the governor response won't be very good. Note that this is not particular to this governor, step-wise will also misbehave if you call its throttle() faster than the normal thermal framework tick (due to interrupts for example) as it will overreact" 2. In Power allocator code, when switch_on/control trip temp violation, it is enabling passive counter to activate passive polling @ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/power_allocator.c?h=v5.4.71#n634 3. while calculating derivative term, it is using passive_delay @ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/power_allocator.c?h=v5.4.71#n243 4. Sensor interrupt will work if temperature is fluctuating between trip_temp and hysteresis. But say a case where we are not enabling polling-delay-passive. In this case if current temperature > control_temp trip(2nd passive trip) and temperature trend is still raising, then sensor high trip will be disabled (OR configured for critical trip threshold). No more trip interrupt from sensor until it reaches critical trip or falls below control_temp hysteresis. How the governor re-evaluate its next mitigation without passive polling thread here ? I think the same is required for CPU thermal zone as well. >> >>>> polling-delay = <0>; >>>> >>>> thermal-sensors = <&tsens0 13>; >>>> >>>> trips { >>>> gpuss0_alert0: trip-point0 { >>>> - temperature = <90000>; >>>> + temperature = <95000>; >>>> hysteresis = <2000>; >>>> - type = "hot"; >>>> + type = "passive"; >>> >>> Matthias probably knows better, but I wonder if we should be making >>> two passive trip levels like we do with CPU. IIRC this is important >>> if someone wants to be able to use this with IPA. >> >> Yes, please introduce a second trip point and make both of them >> 'passive'. >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > Adding Manaf here. > > -Akhil.
WARNING: multiple messages have this Message-ID (diff)
From: manafm@codeaurora.org To: Akhil P Oommen <akhilpo@codeaurora.org> Cc: linux-arm-msm <linux-arm-msm@vger.kernel.org>, Doug Anderson <dianders@chromium.org>, LKML <linux-kernel@vger.kernel.org>, Matthias Kaehlcke <mka@chromium.org>, dri-devel@freedesktop.org, freedreno <freedreno@lists.freedesktop.org> Subject: Re: [PATCH 1/2] arm64: dts: qcom: sc7180: Add gpu cooling support Date: Thu, 15 Oct 2020 00:07:01 +0530 [thread overview] Message-ID: <a4be2cf9e51e4f40aae3f9a56989a42f@codeaurora.org> (raw) In-Reply-To: <fc490021-b046-68c5-7ceb-9c63d3ff5650@codeaurora.org> On 2020-10-14 18:59, Akhil P Oommen wrote: > On 10/9/2020 10:27 PM, Matthias Kaehlcke wrote: >> On Fri, Oct 09, 2020 at 08:05:10AM -0700, Doug Anderson wrote: >>> Hi, >>> >>> On Thu, Oct 8, 2020 at 10:10 AM Akhil P Oommen >>> <akhilpo@codeaurora.org> wrote: >>>> >>>> Add cooling-cells property and the cooling maps for the gpu tzones >>>> to support GPU cooling. >>>> >>>> Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org> >>>> --- >>>> arch/arm64/boot/dts/qcom/sc7180.dtsi | 29 >>>> ++++++++++++++++++++++------- >>>> 1 file changed, 22 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> index d46b383..40d6a28 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi >>>> @@ -2,7 +2,7 @@ >>>> /* >>>> * SC7180 SoC device tree source >>>> * >>>> - * Copyright (c) 2019, The Linux Foundation. All rights reserved. >>>> + * Copyright (c) 2019-20, The Linux Foundation. All rights >>>> reserved. >>>> */ >>>> >>>> #include <dt-bindings/clock/qcom,dispcc-sc7180.h> >>>> @@ -1885,6 +1885,7 @@ >>>> iommus = <&adreno_smmu 0>; >>>> operating-points-v2 = <&gpu_opp_table>; >>>> qcom,gmu = <&gmu>; >>>> + #cooling-cells = <2>; >>> >>> Presumably we should add this to the devicetree bindings, too? > Yes, thanks for catching this. Will update in the next patch. > >>> >>> >>>> interconnects = <&gem_noc MASTER_GFX3D >>>> &mc_virt SLAVE_EBI1>; >>>> interconnect-names = "gfx-mem"; >>>> @@ -3825,16 +3826,16 @@ >>>> }; >>>> >>>> gpuss0-thermal { >>>> - polling-delay-passive = <0>; >>>> + polling-delay-passive = <100>; >>> >>> Why did you make this change? I'm pretty sure that we _don't_ want >>> this since we're using interrupts for the thermal sensor. See commit >>> 22337b91022d ("arm64: dts: qcom: sc7180: Changed polling mode in >>> Thermal-zones node"). >> >> I was going to ask the same, this shouldn't be needed. As per our understanding unlike "polling-delay", this delay property is intended to activate polling thread on post trip threshold violation and it is irrespective of sensor is capable for trip interrupt or not. This polling is more of governor related. Below are the few references from Documentation/code which tells polling-delay-passive is needed for IPA for better IPA performance. As per Power allocator documentations 1. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/driver-api/thermal/power_allocator.rst?h=v5.4.71#n264 "The power allocator governor's PID controller works best if there is a periodic tick. If you have a driver that calls `thermal_zone_device_update()` (or anything that ends up calling the governor's `throttle()` function) repetitively, the governor response won't be very good. Note that this is not particular to this governor, step-wise will also misbehave if you call its throttle() faster than the normal thermal framework tick (due to interrupts for example) as it will overreact" 2. In Power allocator code, when switch_on/control trip temp violation, it is enabling passive counter to activate passive polling @ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/power_allocator.c?h=v5.4.71#n634 3. while calculating derivative term, it is using passive_delay @ https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/power_allocator.c?h=v5.4.71#n243 4. Sensor interrupt will work if temperature is fluctuating between trip_temp and hysteresis. But say a case where we are not enabling polling-delay-passive. In this case if current temperature > control_temp trip(2nd passive trip) and temperature trend is still raising, then sensor high trip will be disabled (OR configured for critical trip threshold). No more trip interrupt from sensor until it reaches critical trip or falls below control_temp hysteresis. How the governor re-evaluate its next mitigation without passive polling thread here ? I think the same is required for CPU thermal zone as well. >> >>>> polling-delay = <0>; >>>> >>>> thermal-sensors = <&tsens0 13>; >>>> >>>> trips { >>>> gpuss0_alert0: trip-point0 { >>>> - temperature = <90000>; >>>> + temperature = <95000>; >>>> hysteresis = <2000>; >>>> - type = "hot"; >>>> + type = "passive"; >>> >>> Matthias probably knows better, but I wonder if we should be making >>> two passive trip levels like we do with CPU. IIRC this is important >>> if someone wants to be able to use this with IPA. >> >> Yes, please introduce a second trip point and make both of them >> 'passive'. >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > Adding Manaf here. > > -Akhil. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-10-14 18:37 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-08 17:09 [PATCH 1/2] arm64: dts: qcom: sc7180: Add gpu cooling support Akhil P Oommen 2020-10-08 17:09 ` Akhil P Oommen 2020-10-08 17:09 ` [PATCH 2/2] drm/msm: Add support for GPU cooling Akhil P Oommen 2020-10-08 17:09 ` Akhil P Oommen 2020-10-09 18:36 ` [2/2] " mka 2020-10-09 18:36 ` mka 2020-10-12 13:33 ` Akhil P Oommen 2020-10-12 13:33 ` Akhil P Oommen 2020-10-12 17:40 ` mka 2020-10-12 17:40 ` mka 2020-10-13 13:53 ` Akhil P Oommen 2020-10-13 13:53 ` Akhil P Oommen 2020-10-13 17:40 ` mka 2020-10-13 17:40 ` mka 2020-10-13 19:21 ` Akhil P Oommen 2020-10-13 19:21 ` Akhil P Oommen 2020-10-14 1:09 ` mka 2020-10-14 1:09 ` mka 2020-10-09 15:05 ` [PATCH 1/2] arm64: dts: qcom: sc7180: Add gpu cooling support Doug Anderson 2020-10-09 15:05 ` Doug Anderson 2020-10-09 16:57 ` Matthias Kaehlcke 2020-10-09 16:57 ` Matthias Kaehlcke 2020-10-14 13:29 ` Akhil P Oommen 2020-10-14 13:29 ` Akhil P Oommen 2020-10-14 18:37 ` manafm [this message] 2020-10-14 18:37 ` manafm 2020-10-15 22:19 ` Matthias Kaehlcke 2020-10-15 22:19 ` Matthias Kaehlcke 2020-10-16 13:46 ` Akhil P Oommen 2020-10-16 13:46 ` Akhil P Oommen
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=a4be2cf9e51e4f40aae3f9a56989a42f@codeaurora.org \ --to=manafm@codeaurora.org \ --cc=akhilpo@codeaurora.org \ --cc=dianders@chromium.org \ --cc=dri-devel@freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mka@chromium.org \ /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.