From: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
To: Marek Vasut <marex@denx.de>
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, 2 Jul 2021 11:23:21 +0200 [thread overview]
Message-ID: <3bb823e4-4724-7072-fe9f-7b8a355c8e50@foss.st.com> (raw)
In-Reply-To: <420e243d-7541-a07e-177b-d2db11c26aef@denx.de>
Hello Marek,
Sorry for the late answer.
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.
Regards,
Raphaël G-P
WARNING: multiple messages have this Message-ID (diff)
From: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
To: Marek Vasut <marex@denx.de>
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, 2 Jul 2021 11:23:21 +0200 [thread overview]
Message-ID: <3bb823e4-4724-7072-fe9f-7b8a355c8e50@foss.st.com> (raw)
In-Reply-To: <420e243d-7541-a07e-177b-d2db11c26aef@denx.de>
Hello Marek,
Sorry for the late answer.
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.
Regards,
Raphaël G-P
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
To: Marek Vasut <marex@denx.de>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>,
Raphael GALLAIS-POU <raphael.gallais-pou@st.com>,
David Airlie <airlied@linux.ie>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Yannick FERTRE - foss <yannick.fertre@foss.st.com>,
Philippe CORNU <philippe.cornu@st.com>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
Alexandre TORGUE - foss <alexandre.torgue@foss.st.com>,
Yannick FERTRE <yannick.fertre@st.com>,
Philippe CORNU - foss <philippe.cornu@foss.st.com>,
Benjamin Gaignard <benjamin.gaignard@linaro.org>,
Stephen Boyd <swboyd@chromium.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>
Subject: Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
Date: Fri, 2 Jul 2021 11:23:21 +0200 [thread overview]
Message-ID: <3bb823e4-4724-7072-fe9f-7b8a355c8e50@foss.st.com> (raw)
In-Reply-To: <420e243d-7541-a07e-177b-d2db11c26aef@denx.de>
Hello Marek,
Sorry for the late answer.
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.
Regards,
Raphaël G-P
next prev parent reply other threads:[~2021-07-02 9:23 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 [this message]
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
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=3bb823e4-4724-7072-fe9f-7b8a355c8e50@foss.st.com \
--to=raphael.gallais-pou@foss.st.com \
--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=marex@denx.de \
--cc=mcoquelin.stm32@gmail.com \
--cc=philippe.cornu@foss.st.com \
--cc=philippe.cornu@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: link
Be 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.