From: Marek Vasut <marex@denx.de> To: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> Cc: Yannick FERTRE <yannick.fertre@st.com>, Philippe CORNU <philippe.cornu@st.com>, Raphael GALLAIS-POU <raphael.gallais-pou@st.com>, Yannick FERTRE - foss <yannick.fertre@foss.st.com>, Philippe CORNU - foss <philippe.cornu@foss.st.com>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Alexandre TORGUE - foss <alexandre.torgue@foss.st.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org> Subject: Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Date: Fri, 20 Aug 2021 00:22:28 +0200 [thread overview] Message-ID: <a051825c-1795-b7c4-6fdd-64595e52a6ca@denx.de> (raw) In-Reply-To: <629b6666-74b3-91f0-5c6c-e26d55ea62d8@foss.st.com> On 8/17/21 11:43 AM, Raphael Gallais-Pou wrote: > > On 7/2/21 8:07 PM, Marek Vasut wrote: >> On 7/2/21 11:23 AM, Raphael Gallais-Pou wrote: >>> Hello Marek, >> >> Hi, >> >>> Sorry for the late answer. >> >> No worries, take your time >> >>> On 6/30/21 2:35 AM, Marek Vasut wrote: >>>> On 6/29/21 1:58 PM, Raphael GALLAIS-POU - foss wrote: >>>> >>>> [...] >>>> >>>>> +++ b/drivers/gpu/drm/stm/ltdc.c >>>>> @@ -425,10 +425,17 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, >>>>> { >>>>> struct ltdc_device *ldev = crtc_to_ltdc(crtc); >>>>> struct drm_device *ddev = crtc->dev; >>>>> + int ret; >>>>> DRM_DEBUG_DRIVER("\n"); >>>>> - pm_runtime_get_sync(ddev->dev); >>>>> + if (!pm_runtime_active(ddev->dev)) { >>>>> + ret = pm_runtime_get_sync(ddev->dev); >>>> >>>> All these if (!pm_runtime_active()) then pm_runtime_get_sync() calls look like workaround for some larger issue. Shouldn't the pm_runtime do some refcounting on its own , so this shouldn't be needed ? >>> >>> >>> This problem purely comes from the driver internals, so I don't think it is a workaround. >>> >>> Because of the "ltdc_crtc_mode_set_nofb" function which does not have any "symmetrical" call, such as enable/disable functions, there was two calls to pm_runtime_get_sync against one call to pm_runtime_put_sync. >>> >>> This instability resulted in the LTDC clocks being always enabled, even when the peripheral was disabled. This could be seen in the clk_summary as explained in the patch summary among other things. >>> >>> By doing so, we first check if the clocks are not already activated, and in that case we call pm_runtime_get_sync. >> >> I just have to wonder, how come other drivers don't need these if (!pm_runtime_active()) pm_runtime_get_sync() conditions. I think they just get/put the runtime PM within a call itself, not across function calls. Maybe that could be the right fix here too ? > > > Hello Marek, Hi, > I've run a deeper analysis over this implementation. Thank you > If I may take rockchip's "rockchip_drm_vop.c" driver, there is an boolean "is_enabled" set to true when crtc_atomic_enable is called. > > The above implementation could save us from adding such field in the ltdc_dev structure. > > Another solution could be in order to simply call pm_runtime_get_sync() in ltdc_crtc_mode_set_nofb() and by removing this condition in ltdc_atomic_crtc_disable() the driver behaves just like the first version of this patch. > > In this way, it avoids such conditions and seems more to get along with the current implementation. Let me maybe ask a different question -- can ltdc_crtc_mode_set_nofb() ever be called with the LTDC suspended (so you would have to call pm_runtime_get_sync() in that function to power the block up and to get access to its registers) ? [...]
WARNING: multiple messages have this Message-ID (diff)
From: Marek Vasut <marex@denx.de> To: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> Cc: Yannick FERTRE <yannick.fertre@st.com>, Philippe CORNU <philippe.cornu@st.com>, Raphael GALLAIS-POU <raphael.gallais-pou@st.com>, Yannick FERTRE - foss <yannick.fertre@foss.st.com>, Philippe CORNU - foss <philippe.cornu@foss.st.com>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Maxime Coquelin <mcoquelin.stm32@gmail.com>, Alexandre TORGUE - foss <alexandre.torgue@foss.st.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "linux-stm32@st-md-mailman.stormreply.com" <linux-stm32@st-md-mailman.stormreply.com>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Stephen Boyd <swboyd@chromium.org> Subject: Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Date: Fri, 20 Aug 2021 00:22:28 +0200 [thread overview] Message-ID: <a051825c-1795-b7c4-6fdd-64595e52a6ca@denx.de> (raw) In-Reply-To: <629b6666-74b3-91f0-5c6c-e26d55ea62d8@foss.st.com> On 8/17/21 11:43 AM, Raphael Gallais-Pou wrote: > > On 7/2/21 8:07 PM, Marek Vasut wrote: >> On 7/2/21 11:23 AM, Raphael Gallais-Pou wrote: >>> Hello Marek, >> >> Hi, >> >>> Sorry for the late answer. >> >> No worries, take your time >> >>> On 6/30/21 2:35 AM, Marek Vasut wrote: >>>> On 6/29/21 1:58 PM, Raphael GALLAIS-POU - foss wrote: >>>> >>>> [...] >>>> >>>>> +++ b/drivers/gpu/drm/stm/ltdc.c >>>>> @@ -425,10 +425,17 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc, >>>>> { >>>>> struct ltdc_device *ldev = crtc_to_ltdc(crtc); >>>>> struct drm_device *ddev = crtc->dev; >>>>> + int ret; >>>>> DRM_DEBUG_DRIVER("\n"); >>>>> - pm_runtime_get_sync(ddev->dev); >>>>> + if (!pm_runtime_active(ddev->dev)) { >>>>> + ret = pm_runtime_get_sync(ddev->dev); >>>> >>>> All these if (!pm_runtime_active()) then pm_runtime_get_sync() calls look like workaround for some larger issue. Shouldn't the pm_runtime do some refcounting on its own , so this shouldn't be needed ? >>> >>> >>> This problem purely comes from the driver internals, so I don't think it is a workaround. >>> >>> Because of the "ltdc_crtc_mode_set_nofb" function which does not have any "symmetrical" call, such as enable/disable functions, there was two calls to pm_runtime_get_sync against one call to pm_runtime_put_sync. >>> >>> This instability resulted in the LTDC clocks being always enabled, even when the peripheral was disabled. This could be seen in the clk_summary as explained in the patch summary among other things. >>> >>> By doing so, we first check if the clocks are not already activated, and in that case we call pm_runtime_get_sync. >> >> I just have to wonder, how come other drivers don't need these if (!pm_runtime_active()) pm_runtime_get_sync() conditions. I think they just get/put the runtime PM within a call itself, not across function calls. Maybe that could be the right fix here too ? > > > Hello Marek, Hi, > I've run a deeper analysis over this implementation. Thank you > If I may take rockchip's "rockchip_drm_vop.c" driver, there is an boolean "is_enabled" set to true when crtc_atomic_enable is called. > > The above implementation could save us from adding such field in the ltdc_dev structure. > > Another solution could be in order to simply call pm_runtime_get_sync() in ltdc_crtc_mode_set_nofb() and by removing this condition in ltdc_atomic_crtc_disable() the driver behaves just like the first version of this patch. > > In this way, it avoids such conditions and seems more to get along with the current implementation. Let me maybe ask a different question -- can ltdc_crtc_mode_set_nofb() ever be called with the LTDC suspended (so you would have to call pm_runtime_get_sync() in that function to power the block up and to get access to its registers) ? [...] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-08-19 22:22 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-06-29 11:58 [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Raphael GALLAIS-POU - foss 2021-06-29 11:58 ` Raphael GALLAIS-POU - foss 2021-06-29 11:58 ` Raphael GALLAIS-POU - foss 2021-06-30 0:35 ` Marek Vasut 2021-06-30 0:35 ` Marek Vasut 2021-06-30 0:35 ` Marek Vasut 2021-07-02 9:23 ` Raphael Gallais-Pou 2021-07-02 9:23 ` Raphael Gallais-Pou 2021-07-02 9:23 ` Raphael Gallais-Pou 2021-07-02 18:07 ` Marek Vasut 2021-07-02 18:07 ` Marek Vasut 2021-07-02 18:07 ` Marek Vasut 2021-08-17 9:43 ` Raphael Gallais-Pou 2021-08-17 9:43 ` Raphael Gallais-Pou 2021-08-17 9:43 ` Raphael Gallais-Pou 2021-08-19 22:22 ` Marek Vasut [this message] 2021-08-19 22:22 ` Marek Vasut 2021-08-19 22:22 ` Marek Vasut 2021-07-06 15:21 ` yannick Fertre 2021-07-06 15:21 ` yannick Fertre 2021-07-06 15:21 ` yannick Fertre
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=a051825c-1795-b7c4-6fdd-64595e52a6ca@denx.de \ --to=marex@denx.de \ --cc=airlied@linux.ie \ --cc=alexandre.torgue@foss.st.com \ --cc=benjamin.gaignard@linaro.org \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=philippe.cornu@foss.st.com \ --cc=philippe.cornu@st.com \ --cc=raphael.gallais-pou@foss.st.com \ --cc=raphael.gallais-pou@st.com \ --cc=swboyd@chromium.org \ --cc=yannick.fertre@foss.st.com \ --cc=yannick.fertre@st.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.