LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
@ 2021-06-29 11:58 Raphael GALLAIS-POU - foss
  2021-06-30  0:35 ` Marek Vasut
  2021-07-06 15:21 ` yannick Fertre
  0 siblings, 2 replies; 7+ messages in thread
From: Raphael GALLAIS-POU - foss @ 2021-06-29 11:58 UTC (permalink / raw)
  To: Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel
  Cc: Yannick FERTRE, Philippe CORNU, Marek Vasut,
	Raphael GALLAIS-POU - foss, Raphael GALLAIS-POU

Bugzilla ticket: https://intbugzilla.st.com/show_bug.cgi?id=60620
Gerrit patch: https://gerrit.st.com/c/mpu/oe/st/linux-stm32/+/208093/

In the LTDC driver, pm_runtime_get_sync was wrongly used and caused the
LTDC pixel clock to be systematically enabled in the clock summary.

After one simple use of the LTDC by activating and deactivating,
the clock summary results as below:

~# cat /sys/kernel/debug/clk/clk_summary | grep ltdc
        ltdc_px               1        1        0    29700000          0     0  50000         N
              ltdc            0        0        0   133250000          0     0  50000         N

By doing so, pm_runtime_get_sync only increments the clock counter when
the driver was in not active, displaying the right information when the
LTDC is not in use, resulting of the below clock summary after deactivation
of the LTDC.

~# cat /sys/kernel/debug/clk/clk_summary | grep ltdc
        ltdc_px               0        0        0    29700000          0     0  50000         N
              ltdc            0        0        0   133250000          0     0  50000         N

The clocks are activated either by the crtc_set_nofb function or
by the crtc_atomic_enable function. A check of pm_runtime activity must
be done before set clocks on. This check must also be done for others
functions which access registers.

Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 08b71248044d..bf9d18023698 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ 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);
+		if (ret) {
+			DRM_ERROR("Failed to set mode, cannot get sync\n");
+			return;
+		}
+	}
 
 	/* Sets the background color value */
 	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
@@ -783,6 +790,7 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state,
 									  plane);
 	struct drm_framebuffer *fb = newstate->fb;
+	struct drm_device *ddev = plane->dev;
 	u32 lofs = plane->index * LAY_OFS;
 	u32 x0 = newstate->crtc_x;
 	u32 x1 = newstate->crtc_x + newstate->crtc_w - 1;
@@ -792,6 +800,11 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
 	u32 val, pitch_in_bytes, line_length, paddr, ahbp, avbp, bpcr;
 	enum ltdc_pix_fmt pf;
 
+	if (!pm_runtime_active(ddev->dev)) {
+		DRM_DEBUG_DRIVER("crtc not activated");
+		return;
+	}
+
 	if (!newstate->crtc || !fb) {
 		DRM_DEBUG_DRIVER("fb or crtc NULL");
 		return;
@@ -897,8 +910,14 @@ static void ltdc_plane_atomic_disable(struct drm_plane *plane,
 	struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state,
 									  plane);
 	struct ltdc_device *ldev = plane_to_ltdc(plane);
+	struct drm_device *ddev = plane->dev;
 	u32 lofs = plane->index * LAY_OFS;
 
+	if (!pm_runtime_active(ddev->dev)) {
+		DRM_DEBUG_DRIVER("crtc already deactivated");
+		return;
+	}
+
 	/* disable layer */
 	reg_clear(ldev->regs, LTDC_L1CR + lofs, LXCR_LEN);
 
-- 
2.24.3

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-06-29 11:58 [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Raphael GALLAIS-POU - foss
@ 2021-06-30  0:35 ` Marek Vasut
  2021-07-02  9:23   ` Raphael Gallais-Pou
  2021-07-06 15:21 ` yannick Fertre
  1 sibling, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2021-06-30  0:35 UTC (permalink / raw)
  To: Raphael GALLAIS-POU - foss
  Cc: Yannick FERTRE, Philippe CORNU, Raphael GALLAIS-POU,
	Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel, Stephen Boyd

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 ?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-06-30  0:35 ` Marek Vasut
@ 2021-07-02  9:23   ` Raphael Gallais-Pou
  2021-07-02 18:07     ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: Raphael Gallais-Pou @ 2021-07-02  9:23 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Yannick FERTRE, Philippe CORNU, Raphael GALLAIS-POU,
	Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel, Stephen Boyd

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-07-02  9:23   ` Raphael Gallais-Pou
@ 2021-07-02 18:07     ` Marek Vasut
  2021-08-17  9:43       ` Raphael Gallais-Pou
  0 siblings, 1 reply; 7+ messages in thread
From: Marek Vasut @ 2021-07-02 18:07 UTC (permalink / raw)
  To: Raphael Gallais-Pou
  Cc: Yannick FERTRE, Philippe CORNU, Raphael GALLAIS-POU,
	Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel, Stephen Boyd

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 ?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-06-29 11:58 [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Raphael GALLAIS-POU - foss
  2021-06-30  0:35 ` Marek Vasut
@ 2021-07-06 15:21 ` yannick Fertre
  1 sibling, 0 replies; 7+ messages in thread
From: yannick Fertre @ 2021-07-06 15:21 UTC (permalink / raw)
  To: Raphael GALLAIS-POU - foss, Philippe CORNU - foss,
	Benjamin Gaignard, David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel
  Cc: Yannick FERTRE, Philippe CORNU, Marek Vasut, Raphael GALLAIS-POU

Hi Raphaël,

thanks for the patch.

Tested-by: Yannick Fertre <yannick.fertre@foss.st.com>



On 6/29/21 1:58 PM, Raphael GALLAIS-POU - foss wrote:
> Bugzilla ticket: https://intbugzilla.st.com/show_bug.cgi?id=60620
> Gerrit patch: https://gerrit.st.com/c/mpu/oe/st/linux-stm32/+/208093/
> 
> In the LTDC driver, pm_runtime_get_sync was wrongly used and caused the
> LTDC pixel clock to be systematically enabled in the clock summary.
> 
> After one simple use of the LTDC by activating and deactivating,
> the clock summary results as below:
> 
> ~# cat /sys/kernel/debug/clk/clk_summary | grep ltdc
>          ltdc_px               1        1        0    29700000          0     0  50000         N
>                ltdc            0        0        0   133250000          0     0  50000         N
> 
> By doing so, pm_runtime_get_sync only increments the clock counter when
> the driver was in not active, displaying the right information when the
> LTDC is not in use, resulting of the below clock summary after deactivation
> of the LTDC.
> 
> ~# cat /sys/kernel/debug/clk/clk_summary | grep ltdc
>          ltdc_px               0        0        0    29700000          0     0  50000         N
>                ltdc            0        0        0   133250000          0     0  50000         N
> 
> The clocks are activated either by the crtc_set_nofb function or
> by the crtc_atomic_enable function. A check of pm_runtime activity must
> be done before set clocks on. This check must also be done for others
> functions which access registers.
> 
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
>   drivers/gpu/drm/stm/ltdc.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 08b71248044d..bf9d18023698 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ 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);
> +		if (ret) {
> +			DRM_ERROR("Failed to set mode, cannot get sync\n");
> +			return;
> +		}
> +	}
>   
>   	/* Sets the background color value */
>   	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
> @@ -783,6 +790,7 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
>   	struct drm_plane_state *newstate = drm_atomic_get_new_plane_state(state,
>   									  plane);
>   	struct drm_framebuffer *fb = newstate->fb;
> +	struct drm_device *ddev = plane->dev;
>   	u32 lofs = plane->index * LAY_OFS;
>   	u32 x0 = newstate->crtc_x;
>   	u32 x1 = newstate->crtc_x + newstate->crtc_w - 1;
> @@ -792,6 +800,11 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
>   	u32 val, pitch_in_bytes, line_length, paddr, ahbp, avbp, bpcr;
>   	enum ltdc_pix_fmt pf;
>   
> +	if (!pm_runtime_active(ddev->dev)) {
> +		DRM_DEBUG_DRIVER("crtc not activated");
> +		return;
> +	}
> +
>   	if (!newstate->crtc || !fb) {
>   		DRM_DEBUG_DRIVER("fb or crtc NULL");
>   		return;
> @@ -897,8 +910,14 @@ static void ltdc_plane_atomic_disable(struct drm_plane *plane,
>   	struct drm_plane_state *oldstate = drm_atomic_get_old_plane_state(state,
>   									  plane);
>   	struct ltdc_device *ldev = plane_to_ltdc(plane);
> +	struct drm_device *ddev = plane->dev;
>   	u32 lofs = plane->index * LAY_OFS;
>   
> +	if (!pm_runtime_active(ddev->dev)) {
> +		DRM_DEBUG_DRIVER("crtc already deactivated");
> +		return;
> +	}
> +
>   	/* disable layer */
>   	reg_clear(ldev->regs, LTDC_L1CR + lofs, LXCR_LEN);
>   
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-07-02 18:07     ` Marek Vasut
@ 2021-08-17  9:43       ` Raphael Gallais-Pou
  2021-08-19 22:22         ` Marek Vasut
  0 siblings, 1 reply; 7+ messages in thread
From: Raphael Gallais-Pou @ 2021-08-17  9:43 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Yannick FERTRE, Philippe CORNU, Raphael GALLAIS-POU,
	Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel, Stephen Boyd


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,


I've run a deeper analysis over this implementation.

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.


Regards,

Raphaël


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks
  2021-08-17  9:43       ` Raphael Gallais-Pou
@ 2021-08-19 22:22         ` Marek Vasut
  0 siblings, 0 replies; 7+ messages in thread
From: Marek Vasut @ 2021-08-19 22:22 UTC (permalink / raw)
  To: Raphael Gallais-Pou
  Cc: Yannick FERTRE, Philippe CORNU, Raphael GALLAIS-POU,
	Yannick FERTRE - foss, Philippe CORNU - foss, Benjamin Gaignard,
	David Airlie, Daniel Vetter, Maxime Coquelin,
	Alexandre TORGUE - foss, dri-devel, linux-stm32,
	linux-arm-kernel, linux-kernel, Stephen Boyd

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) ?

[...]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-08-19 22:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 11:58 [PATCH] drm/stm: ltdc: improve pm_runtime to stop clocks Raphael GALLAIS-POU - foss
2021-06-30  0:35 ` Marek Vasut
2021-07-02  9:23   ` Raphael Gallais-Pou
2021-07-02 18:07     ` Marek Vasut
2021-08-17  9:43       ` Raphael Gallais-Pou
2021-08-19 22:22         ` Marek Vasut
2021-07-06 15:21 ` yannick Fertre

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).