[PATCH 00/10] drm/tidss: Probe related fixes and cleanups

[PATCH 00/10] drm/tidss: Probe related fixes and cleanups

[Tomi Valkeinen]

While working on the TI BSP kernel, adding bootload splash screen
support, I noticed some issues with the driver and opportunities for
cleanups and improvements.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Tomi Valkeinen (10):
      drm/tidss: Use pm_runtime_resume_and_get()
      drm/tidss: Use PM autosuspend
      drm/tidss: Drop useless variable init
      drm/tidss: Move reset to the end of dispc_init()
      drm/tidss: Return error value from from softreset
      drm/tidss: Check for K2G in in dispc_softreset()
      drm/tidss: Fix dss reset
      drm/tidss: Add dispc_is_idle()
      drm/tidss: IRQ code cleanup
      drm/tidss: Fix atomic_flush check

 drivers/gpu/drm/tidss/tidss_crtc.c  |  9 ++---
 drivers/gpu/drm/tidss/tidss_dispc.c | 76 +++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/tidss/tidss_drv.c   | 14 +++++--
 drivers/gpu/drm/tidss/tidss_irq.c   | 54 ++++----------------------
 4 files changed, 92 insertions(+), 61 deletions(-)
---
base-commit: 9d7c8c066916f231ca0ed4e4fce6c4b58ca3e451
change-id: 20231030-tidss-probe-854b1098c3af

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH 01/10] drm/tidss: Use pm_runtime_resume_and_get()

[Tomi Valkeinen]

Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(), which
will handle error situations better. Also fix the return, as there
should be no reason for the current complex return.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 4d063eb9cd0b..f403db11b846 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -32,9 +32,9 @@ int tidss_runtime_get(struct tidss_device *tidss)
 
 	dev_dbg(tidss->dev, "%s\n", __func__);
 
-	r = pm_runtime_get_sync(tidss->dev);
+	r = pm_runtime_resume_and_get(tidss->dev);
 	WARN_ON(r < 0);
-	return r < 0 ? r : 0;
+	return r;
 }
 
 void tidss_runtime_put(struct tidss_device *tidss)

-- 
2.34.1

Re: [PATCH 01/10] drm/tidss: Use pm_runtime_resume_and_get()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:38AM +0200, Tomi Valkeinen wrote:
> Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync(), which
> will handle error situations better. Also fix the return, as there
> should be no reason for the current complex return.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

But I think we would be better off dropping the two small wrappers.

> ---
>  drivers/gpu/drm/tidss/tidss_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 4d063eb9cd0b..f403db11b846 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -32,9 +32,9 @@ int tidss_runtime_get(struct tidss_device *tidss)
>  
>  	dev_dbg(tidss->dev, "%s\n", __func__);
>  
> -	r = pm_runtime_get_sync(tidss->dev);
> +	r = pm_runtime_resume_and_get(tidss->dev);
>  	WARN_ON(r < 0);
> -	return r < 0 ? r : 0;
> +	return r;
>  }
>  
>  void tidss_runtime_put(struct tidss_device *tidss)
> 

-- 
Regards,

Laurent Pinchart

[PATCH 02/10] drm/tidss: Use PM autosuspend

[Tomi Valkeinen]

Use runtime PM autosuspend feature, with 1s timeout, to avoid
unnecessary suspend-resume cycles when, e.g. the userspace temporarily
turns off the crtcs when configuring the outputs.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index f403db11b846..64914331715a 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
 
 	dev_dbg(tidss->dev, "%s\n", __func__);
 
-	r = pm_runtime_put_sync(tidss->dev);
+	pm_runtime_mark_last_busy(tidss->dev);
+
+	r = pm_runtime_put_autosuspend(tidss->dev);
 	WARN_ON(r < 0);
 }
 
@@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+
 #ifndef CONFIG_PM
 	/* If we don't have PM, we need to call resume manually */
 	dispc_runtime_resume(tidss->dispc);
@@ -215,6 +220,7 @@ static void tidss_remove(struct platform_device *pdev)
 	/* If we don't have PM, we need to call suspend manually */
 	dispc_runtime_suspend(tidss->dispc);
 #endif
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
 	/* devm allocated dispc goes away with the dev so mark it NULL */

-- 
2.34.1

Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote:
> Use runtime PM autosuspend feature, with 1s timeout, to avoid
> unnecessary suspend-resume cycles when, e.g. the userspace temporarily
> turns off the crtcs when configuring the outputs.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index f403db11b846..64914331715a 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
>  
>  	dev_dbg(tidss->dev, "%s\n", __func__);
>  
> -	r = pm_runtime_put_sync(tidss->dev);
> +	pm_runtime_mark_last_busy(tidss->dev);
> +
> +	r = pm_runtime_put_autosuspend(tidss->dev);
>  	WARN_ON(r < 0);
>  }
>  
> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(dev);
>  
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +
>  #ifndef CONFIG_PM
>  	/* If we don't have PM, we need to call resume manually */
>  	dispc_runtime_resume(tidss->dispc);

By the way, there's a way to handle this without any ifdef:

	dispc_runtime_resume(tidss->dispc);

	pm_runtime_set_active(dev);
	pm_runtime_get_noresume(dev);
	pm_runtime_enable(dev);
	pm_runtime_set_autosuspend_delay(dev, 1000);
	pm_runtime_use_autosuspend(dev);

Then, in the error path,

	pm_runtime_dont_use_autosuspend(dev);
	pm_runtime_disable(dev);
	pm_runtime_put_noidle(dev);

	dispc_runtime_suspend(tidss->dispc);

And in remove:

	pm_runtime_dont_use_autosuspend(dev);
	pm_runtime_disable(dev);
	if (!pm_runtime_status_suspended(dev))
		dispc_runtime_suspend(tidss->dispc);
	pm_runtime_set_suspended(dev);

And yes, runtime PM is a horrible API.

> @@ -215,6 +220,7 @@ static void tidss_remove(struct platform_device *pdev)
>  	/* If we don't have PM, we need to call suspend manually */
>  	dispc_runtime_suspend(tidss->dispc);
>  #endif
> +	pm_runtime_dont_use_autosuspend(dev);

This also needs to be done in the probe error path.

>  	pm_runtime_disable(dev);
>  
>  	/* devm allocated dispc goes away with the dev so mark it NULL */
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

[Tomi Valkeinen]

On 01/11/2023 15:54, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote:
>> Use runtime PM autosuspend feature, with 1s timeout, to avoid
>> unnecessary suspend-resume cycles when, e.g. the userspace temporarily
>> turns off the crtcs when configuring the outputs.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>> index f403db11b846..64914331715a 100644
>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
>>   
>>   	dev_dbg(tidss->dev, "%s\n", __func__);
>>   
>> -	r = pm_runtime_put_sync(tidss->dev);
>> +	pm_runtime_mark_last_busy(tidss->dev);
>> +
>> +	r = pm_runtime_put_autosuspend(tidss->dev);
>>   	WARN_ON(r < 0);
>>   }
>>   
>> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
>>   
>>   	pm_runtime_enable(dev);
>>   
>> +	pm_runtime_set_autosuspend_delay(dev, 1000);
>> +	pm_runtime_use_autosuspend(dev);
>> +
>>   #ifndef CONFIG_PM
>>   	/* If we don't have PM, we need to call resume manually */
>>   	dispc_runtime_resume(tidss->dispc);
> 
> By the way, there's a way to handle this without any ifdef:
> 
> 	dispc_runtime_resume(tidss->dispc);
> 
> 	pm_runtime_set_active(dev);
> 	pm_runtime_get_noresume(dev);
> 	pm_runtime_enable(dev);
> 	pm_runtime_set_autosuspend_delay(dev, 1000);
> 	pm_runtime_use_autosuspend(dev);

I'm not sure I follow what you are trying to do here. The call to 
dispc_runtime_resume() would crash if we have PM, as the HW would not be 
enabled at that point. And even if it wouldn't, we don't want to call 
dispc_runtime_resume() in probe when we have PM.

> Then, in the error path,
> 
> 	pm_runtime_dont_use_autosuspend(dev);
> 	pm_runtime_disable(dev);
> 	pm_runtime_put_noidle(dev);
> 
> 	dispc_runtime_suspend(tidss->dispc);
> 
> And in remove:
> 
> 	pm_runtime_dont_use_autosuspend(dev);
> 	pm_runtime_disable(dev);
> 	if (!pm_runtime_status_suspended(dev))
> 		dispc_runtime_suspend(tidss->dispc);
> 	pm_runtime_set_suspended(dev);
> 
> And yes, runtime PM is a horrible API.
> 
>> @@ -215,6 +220,7 @@ static void tidss_remove(struct platform_device *pdev)
>>   	/* If we don't have PM, we need to call suspend manually */
>>   	dispc_runtime_suspend(tidss->dispc);
>>   #endif
>> +	pm_runtime_dont_use_autosuspend(dev);
> 
> This also needs to be done in the probe error path.

Oops. Right, I'll add that.

  Tomi

Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

[Laurent Pinchart]

Hi Tomi,

CC'ing Sakari for his expertise on runtime PM (I think he will soon
start wishing he would be ignorant in this area).

On Thu, Nov 02, 2023 at 08:34:45AM +0200, Tomi Valkeinen wrote:
> On 01/11/2023 15:54, Laurent Pinchart wrote:
> > On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote:
> >> Use runtime PM autosuspend feature, with 1s timeout, to avoid
> >> unnecessary suspend-resume cycles when, e.g. the userspace temporarily
> >> turns off the crtcs when configuring the outputs.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
> >>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> >> index f403db11b846..64914331715a 100644
> >> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> >> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> >> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
> >>   
> >>   	dev_dbg(tidss->dev, "%s\n", __func__);
> >>   
> >> -	r = pm_runtime_put_sync(tidss->dev);
> >> +	pm_runtime_mark_last_busy(tidss->dev);
> >> +
> >> +	r = pm_runtime_put_autosuspend(tidss->dev);
> >>   	WARN_ON(r < 0);
> >>   }
> >>   
> >> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
> >>   
> >>   	pm_runtime_enable(dev);
> >>   
> >> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> >> +	pm_runtime_use_autosuspend(dev);
> >> +
> >>   #ifndef CONFIG_PM
> >>   	/* If we don't have PM, we need to call resume manually */
> >>   	dispc_runtime_resume(tidss->dispc);
> > 
> > By the way, there's a way to handle this without any ifdef:
> > 
> > 	dispc_runtime_resume(tidss->dispc);
> > 
> > 	pm_runtime_set_active(dev);
> > 	pm_runtime_get_noresume(dev);
> > 	pm_runtime_enable(dev);
> > 	pm_runtime_set_autosuspend_delay(dev, 1000);
> > 	pm_runtime_use_autosuspend(dev);
> 
> I'm not sure I follow what you are trying to do here. The call to 
> dispc_runtime_resume() would crash if we have PM, as the HW would not be 
> enabled at that point.

Isn't dispc_runtime_resume() meant to enable the hardware ?

The idea is to enable the hardware, then enable runtime PM, and tell the
runtime PM framework that the device is enabled. If CONFIG_PM is not
set, the RPM calls will be no-ops, and the device will stay enable. If
CONFIG_PM is set, the device will be enabled, and will get disabled at
end of probe by a call to pm_runtime_put_autosuspend().

> And even if it wouldn't, we don't want to call dispc_runtime_resume()
> in probe when we have PM.

Don't you need to enable the device at probe time in order to reset it,
as done in later patches in the series ?

> > Then, in the error path,
> > 
> > 	pm_runtime_dont_use_autosuspend(dev);
> > 	pm_runtime_disable(dev);
> > 	pm_runtime_put_noidle(dev);
> > 
> > 	dispc_runtime_suspend(tidss->dispc);
> > 
> > And in remove:
> > 
> > 	pm_runtime_dont_use_autosuspend(dev);
> > 	pm_runtime_disable(dev);
> > 	if (!pm_runtime_status_suspended(dev))
> > 		dispc_runtime_suspend(tidss->dispc);
> > 	pm_runtime_set_suspended(dev);
> > 
> > And yes, runtime PM is a horrible API.
> > 
> >> @@ -215,6 +220,7 @@ static void tidss_remove(struct platform_device *pdev)
> >>   	/* If we don't have PM, we need to call suspend manually */
> >>   	dispc_runtime_suspend(tidss->dispc);
> >>   #endif
> >> +	pm_runtime_dont_use_autosuspend(dev);
> > 
> > This also needs to be done in the probe error path.
> 
> Oops. Right, I'll add that.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 02/10] drm/tidss: Use PM autosuspend

[Tomi Valkeinen]

On 06/11/2023 00:53, Laurent Pinchart wrote:
> Hi Tomi,
> 
> CC'ing Sakari for his expertise on runtime PM (I think he will soon
> start wishing he would be ignorant in this area).
> 
> On Thu, Nov 02, 2023 at 08:34:45AM +0200, Tomi Valkeinen wrote:
>> On 01/11/2023 15:54, Laurent Pinchart wrote:
>>> On Wed, Nov 01, 2023 at 11:17:39AM +0200, Tomi Valkeinen wrote:
>>>> Use runtime PM autosuspend feature, with 1s timeout, to avoid
>>>> unnecessary suspend-resume cycles when, e.g. the userspace temporarily
>>>> turns off the crtcs when configuring the outputs.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    drivers/gpu/drm/tidss/tidss_drv.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
>>>> index f403db11b846..64914331715a 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_drv.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
>>>> @@ -43,7 +43,9 @@ void tidss_runtime_put(struct tidss_device *tidss)
>>>>    
>>>>    	dev_dbg(tidss->dev, "%s\n", __func__);
>>>>    
>>>> -	r = pm_runtime_put_sync(tidss->dev);
>>>> +	pm_runtime_mark_last_busy(tidss->dev);
>>>> +
>>>> +	r = pm_runtime_put_autosuspend(tidss->dev);
>>>>    	WARN_ON(r < 0);
>>>>    }
>>>>    
>>>> @@ -144,6 +146,9 @@ static int tidss_probe(struct platform_device *pdev)
>>>>    
>>>>    	pm_runtime_enable(dev);
>>>>    
>>>> +	pm_runtime_set_autosuspend_delay(dev, 1000);
>>>> +	pm_runtime_use_autosuspend(dev);
>>>> +
>>>>    #ifndef CONFIG_PM
>>>>    	/* If we don't have PM, we need to call resume manually */
>>>>    	dispc_runtime_resume(tidss->dispc);
>>>
>>> By the way, there's a way to handle this without any ifdef:
>>>
>>> 	dispc_runtime_resume(tidss->dispc);
>>>
>>> 	pm_runtime_set_active(dev);
>>> 	pm_runtime_get_noresume(dev);
>>> 	pm_runtime_enable(dev);
>>> 	pm_runtime_set_autosuspend_delay(dev, 1000);
>>> 	pm_runtime_use_autosuspend(dev);
>>
>> I'm not sure I follow what you are trying to do here. The call to
>> dispc_runtime_resume() would crash if we have PM, as the HW would not be
>> enabled at that point.
> 
> Isn't dispc_runtime_resume() meant to enable the hardware ?
> 
> The idea is to enable the hardware, then enable runtime PM, and tell the
> runtime PM framework that the device is enabled. If CONFIG_PM is not
> set, the RPM calls will be no-ops, and the device will stay enable. If
> CONFIG_PM is set, the device will be enabled, and will get disabled at
> end of probe by a call to pm_runtime_put_autosuspend().

(The text below is more about the end result of this series, rather than 
this specific patch):

Hmm, no, I don't think that's how it works. My understanding is this:

There are multiple parts "enabling the hardware", and I think they 
usually need to be done in this order: 1) enabling the parent devices, 
2) system level HW module enable (this is possibly really part of the 
1), 3) clk/regulator/register setup.

3) is handled by the driver, but 1) and 2) are handled via the runtime 
PM framework. Calling dispc_runtime_resume() as the first thing could 
mean that DSS's parents are not enabled or that the DSS HW module is not 
enabled at the system control level.

That's why I first call pm_runtime_set_active(), which should handle 1) 
and 2).

The only thing dispc_runtime_resume() does wrt. enabling the hardware is 
enabling the fclk. It does a lot more, but all the rest is just 
configuring the hardware to settings that we always want to use (e.g. 
fifo management).

Now, if the bootloader had enabled the display, and the driver did:

- pm_runtime_enable()
- pm_runtime_get()
- dispc_reset()

it would cause dispc_runtime_resume() to be called before the reset. 
This would mean that the dispc_runtime_resume() would be changing 
settings that must not be changed while streaming is enabled.

We could do a DSS reset always as the first thing in 
dispc_runtime_resume() (after enabling the fclk), but that feels a bit 
pointless as after the first reset the DSS is in a known state.

Also, if we don't do a reset at probe time, there are things we need to 
take care of: at least we need to mask the IRQs (presuming we register 
the DSS interrupt at probe time). But generally speaking, I feel a bit 
uncomfortable leaving an IP possibly running in an unknown state after 
probe. I'd much rather just reset it at probe.

  Tomi

[PATCH 03/10] drm/tidss: Drop useless variable init

[Tomi Valkeinen]

No need to initialize the ret to 0 in dispc_softreset().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 9d9dee7abaef..ad7999434299 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2705,7 +2705,7 @@ static void dispc_init_errata(struct dispc_device *dispc)
 static void dispc_softreset(struct dispc_device *dispc)
 {
 	u32 val;
-	int ret = 0;
+	int ret;
 
 	/* Soft reset */
 	REG_FLD_MOD(dispc, DSS_SYSCONFIG, 1, 1, 1);

-- 
2.34.1

Re: [PATCH 03/10] drm/tidss: Drop useless variable init

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:40AM +0200, Tomi Valkeinen wrote:
> No need to initialize the ret to 0 in dispc_softreset().
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 9d9dee7abaef..ad7999434299 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2705,7 +2705,7 @@ static void dispc_init_errata(struct dispc_device *dispc)
>  static void dispc_softreset(struct dispc_device *dispc)
>  {
>  	u32 val;
> -	int ret = 0;
> +	int ret;
>  
>  	/* Soft reset */
>  	REG_FLD_MOD(dispc, DSS_SYSCONFIG, 1, 1, 1);
> 

-- 
Regards,

Laurent Pinchart

[PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Tomi Valkeinen]

We do a DSS reset in the middle of the dispc_init(). While that happens
to work now, we should really make sure that e..g the fclk, which is
acquired only later in the function, is enabled when doing a reset. This
will be handled in a later patch, but for now, let's move the
dispc_softreset() call to the end of dispc_init(), which is a sensible
place for it anyway.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index ad7999434299..9430625e2d62 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2777,10 +2777,6 @@ int dispc_init(struct tidss_device *tidss)
 			return r;
 	}
 
-	/* K2G display controller does not support soft reset */
-	if (feat->subrev != DISPC_K2G)
-		dispc_softreset(dispc);
-
 	for (i = 0; i < dispc->feat->num_vps; i++) {
 		u32 gamma_size = dispc->feat->vp_feat.color.gamma_size;
 		u32 *gamma_table;
@@ -2831,5 +2827,9 @@ int dispc_init(struct tidss_device *tidss)
 
 	tidss->dispc = dispc;
 
+	/* K2G display controller does not support soft reset */
+	if (feat->subrev != DISPC_K2G)
+		dispc_softreset(dispc);
+
 	return 0;
 }

-- 
2.34.1

Re: [PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:41AM +0200, Tomi Valkeinen wrote:
> We do a DSS reset in the middle of the dispc_init(). While that happens
> to work now, we should really make sure that e..g the fclk, which is
> acquired only later in the function, is enabled when doing a reset. This
> will be handled in a later patch, but for now, let's move the
> dispc_softreset() call to the end of dispc_init(), which is a sensible
> place for it anyway.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

But do I understand correctly that the device isn't powered up at this
point ? That seems problematic.

I'm also not sure why we need to reset the device at probe time.

> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index ad7999434299..9430625e2d62 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2777,10 +2777,6 @@ int dispc_init(struct tidss_device *tidss)
>  			return r;
>  	}
>  
> -	/* K2G display controller does not support soft reset */
> -	if (feat->subrev != DISPC_K2G)
> -		dispc_softreset(dispc);
> -
>  	for (i = 0; i < dispc->feat->num_vps; i++) {
>  		u32 gamma_size = dispc->feat->vp_feat.color.gamma_size;
>  		u32 *gamma_table;
> @@ -2831,5 +2827,9 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	tidss->dispc = dispc;
>  
> +	/* K2G display controller does not support soft reset */
> +	if (feat->subrev != DISPC_K2G)
> +		dispc_softreset(dispc);
> +
>  	return 0;
>  }
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Tomi Valkeinen]

On 01/11/2023 15:57, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:41AM +0200, Tomi Valkeinen wrote:
>> We do a DSS reset in the middle of the dispc_init(). While that happens
>> to work now, we should really make sure that e..g the fclk, which is
>> acquired only later in the function, is enabled when doing a reset. This
>> will be handled in a later patch, but for now, let's move the
>> dispc_softreset() call to the end of dispc_init(), which is a sensible
>> place for it anyway.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> But do I understand correctly that the device isn't powered up at this
> point ? That seems problematic.

Indeed. It's fixed later in this series.

> I'm also not sure why we need to reset the device at probe time.

That's the usual place to do a reset, to make sure the HW is in a known 
state, is it not? Where would you place it?

  Tomi

Re: [PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Laurent Pinchart]

On Thu, Nov 02, 2023 at 08:40:10AM +0200, Tomi Valkeinen wrote:
> On 01/11/2023 15:57, Laurent Pinchart wrote:
> > On Wed, Nov 01, 2023 at 11:17:41AM +0200, Tomi Valkeinen wrote:
> >> We do a DSS reset in the middle of the dispc_init(). While that happens
> >> to work now, we should really make sure that e..g the fclk, which is
> >> acquired only later in the function, is enabled when doing a reset. This
> >> will be handled in a later patch, but for now, let's move the
> >> dispc_softreset() call to the end of dispc_init(), which is a sensible
> >> place for it anyway.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > But do I understand correctly that the device isn't powered up at this
> > point ? That seems problematic.
> 
> Indeed. It's fixed later in this series.
> 
> > I'm also not sure why we need to reset the device at probe time.
> 
> That's the usual place to do a reset, to make sure the HW is in a known 
> state, is it not? Where would you place it?

The first time the device is used, or possibly every time it is resumed
? It seems that you're resuming it at probe time for the only reason
that you want to then reset it. Resuming it at probe could get entirely
skipped.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 04/10] drm/tidss: Move reset to the end of dispc_init()

[Tomi Valkeinen]

On 06/11/2023 00:54, Laurent Pinchart wrote:
> On Thu, Nov 02, 2023 at 08:40:10AM +0200, Tomi Valkeinen wrote:
>> On 01/11/2023 15:57, Laurent Pinchart wrote:
>>> On Wed, Nov 01, 2023 at 11:17:41AM +0200, Tomi Valkeinen wrote:
>>>> We do a DSS reset in the middle of the dispc_init(). While that happens
>>>> to work now, we should really make sure that e..g the fclk, which is
>>>> acquired only later in the function, is enabled when doing a reset. This
>>>> will be handled in a later patch, but for now, let's move the
>>>> dispc_softreset() call to the end of dispc_init(), which is a sensible
>>>> place for it anyway.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> But do I understand correctly that the device isn't powered up at this
>>> point ? That seems problematic.
>>
>> Indeed. It's fixed later in this series.
>>
>>> I'm also not sure why we need to reset the device at probe time.
>>
>> That's the usual place to do a reset, to make sure the HW is in a known
>> state, is it not? Where would you place it?
> 
> The first time the device is used, or possibly every time it is resumed
> ? It seems that you're resuming it at probe time for the only reason
> that you want to then reset it. Resuming it at probe could get entirely
> skipped.

As I mentioned in the earlier reply, I feel better resetting as early as 
possible. Otherwise the DSS may be running, in an unknown state, doing 
things.

It's hard to say if that would cause problems or not. The DSS IP would 
be using clocks that have not been set up or enabled by the driver, 
which might mean that the clock rates get changed underneath, caused by 
some other driver configuring its clocks. That might lead to DSS IP 
misbehaving. Whether that would cause any issues, hard to say.

Still, I'd much rather have the DSS IP in a known state.

We could reset it every time the DSS is resumed, and that would be safe, 
but it also feels pointless, as the DSS is in a known state already 
(presuming it was reset in probe).

  Tomi

[PATCH 05/10] drm/tidss: Return error value from from softreset

[Tomi Valkeinen]

Return an error value from dispc_softreset() so that the caller can
handle the errors.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 9430625e2d62..cdbb88289082 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2702,7 +2702,7 @@ static void dispc_init_errata(struct dispc_device *dispc)
 	}
 }
 
-static void dispc_softreset(struct dispc_device *dispc)
+static int dispc_softreset(struct dispc_device *dispc)
 {
 	u32 val;
 	int ret;
@@ -2712,8 +2712,12 @@ static void dispc_softreset(struct dispc_device *dispc)
 	/* Wait for reset to complete */
 	ret = readl_poll_timeout(dispc->base_common + DSS_SYSSTATUS,
 				 val, val & 1, 100, 5000);
-	if (ret)
-		dev_warn(dispc->dev, "failed to reset dispc\n");
+	if (ret) {
+		dev_err(dispc->dev, "failed to reset dispc\n");
+		return ret;
+	}
+
+	return 0;
 }
 
 int dispc_init(struct tidss_device *tidss)
@@ -2828,8 +2832,11 @@ int dispc_init(struct tidss_device *tidss)
 	tidss->dispc = dispc;
 
 	/* K2G display controller does not support soft reset */
-	if (feat->subrev != DISPC_K2G)
-		dispc_softreset(dispc);
+	if (feat->subrev != DISPC_K2G) {
+		r = dispc_softreset(dispc);
+		if (r)
+			return r;
+	}
 
 	return 0;
 }

-- 
2.34.1

Re: [PATCH 05/10] drm/tidss: Return error value from from softreset

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:42AM +0200, Tomi Valkeinen wrote:
> Return an error value from dispc_softreset() so that the caller can
> handle the errors.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 9430625e2d62..cdbb88289082 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2702,7 +2702,7 @@ static void dispc_init_errata(struct dispc_device *dispc)
>  	}
>  }
>  
> -static void dispc_softreset(struct dispc_device *dispc)
> +static int dispc_softreset(struct dispc_device *dispc)
>  {
>  	u32 val;
>  	int ret;
> @@ -2712,8 +2712,12 @@ static void dispc_softreset(struct dispc_device *dispc)
>  	/* Wait for reset to complete */
>  	ret = readl_poll_timeout(dispc->base_common + DSS_SYSSTATUS,
>  				 val, val & 1, 100, 5000);
> -	if (ret)
> -		dev_warn(dispc->dev, "failed to reset dispc\n");
> +	if (ret) {
> +		dev_err(dispc->dev, "failed to reset dispc\n");
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  int dispc_init(struct tidss_device *tidss)
> @@ -2828,8 +2832,11 @@ int dispc_init(struct tidss_device *tidss)
>  	tidss->dispc = dispc;
>  
>  	/* K2G display controller does not support soft reset */
> -	if (feat->subrev != DISPC_K2G)
> -		dispc_softreset(dispc);
> +	if (feat->subrev != DISPC_K2G) {
> +		r = dispc_softreset(dispc);
> +		if (r)
> +			return r;

tidss->dispc will be set in this case. Could that cause problems ?

> +	}
>  
>  	return 0;
>  }
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/10] drm/tidss: Return error value from from softreset

[Tomi Valkeinen]

On 01/11/2023 15:59, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:42AM +0200, Tomi Valkeinen wrote:
>> Return an error value from dispc_softreset() so that the caller can
>> handle the errors.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_dispc.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index 9430625e2d62..cdbb88289082 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -2702,7 +2702,7 @@ static void dispc_init_errata(struct dispc_device *dispc)
>>   	}
>>   }
>>   
>> -static void dispc_softreset(struct dispc_device *dispc)
>> +static int dispc_softreset(struct dispc_device *dispc)
>>   {
>>   	u32 val;
>>   	int ret;
>> @@ -2712,8 +2712,12 @@ static void dispc_softreset(struct dispc_device *dispc)
>>   	/* Wait for reset to complete */
>>   	ret = readl_poll_timeout(dispc->base_common + DSS_SYSSTATUS,
>>   				 val, val & 1, 100, 5000);
>> -	if (ret)
>> -		dev_warn(dispc->dev, "failed to reset dispc\n");
>> +	if (ret) {
>> +		dev_err(dispc->dev, "failed to reset dispc\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   int dispc_init(struct tidss_device *tidss)
>> @@ -2828,8 +2832,11 @@ int dispc_init(struct tidss_device *tidss)
>>   	tidss->dispc = dispc;
>>   
>>   	/* K2G display controller does not support soft reset */
>> -	if (feat->subrev != DISPC_K2G)
>> -		dispc_softreset(dispc);
>> +	if (feat->subrev != DISPC_K2G) {
>> +		r = dispc_softreset(dispc);
>> +		if (r)
>> +			return r;
> 
> tidss->dispc will be set in this case. Could that cause problems ?

Not at the moment, but better be safe. I'll move the reset a few lines 
higher.

  Tomi

[PATCH 06/10] drm/tidss: Check for K2G in in dispc_softreset()

[Tomi Valkeinen]

K2G doesn't have softreset feature. Instead of having every caller of
dispc_softreset() check for K2G, move the check into dispc_softreset(),
and make dispc_softreset() return 0 in case of K2G.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index cdbb88289082..f204a0701e9f 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2707,6 +2707,10 @@ static int dispc_softreset(struct dispc_device *dispc)
 	u32 val;
 	int ret;
 
+	/* K2G display controller does not support soft reset */
+	if (dispc->feat->subrev == DISPC_K2G)
+		return 0;
+
 	/* Soft reset */
 	REG_FLD_MOD(dispc, DSS_SYSCONFIG, 1, 1, 1);
 	/* Wait for reset to complete */
@@ -2831,12 +2835,9 @@ int dispc_init(struct tidss_device *tidss)
 
 	tidss->dispc = dispc;
 
-	/* K2G display controller does not support soft reset */
-	if (feat->subrev != DISPC_K2G) {
-		r = dispc_softreset(dispc);
-		if (r)
-			return r;
-	}
+	r = dispc_softreset(dispc);
+	if (r)
+		return r;
 
 	return 0;
 }

-- 
2.34.1

Re: [PATCH 06/10] drm/tidss: Check for K2G in in dispc_softreset()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:43AM +0200, Tomi Valkeinen wrote:
> K2G doesn't have softreset feature. Instead of having every caller of
> dispc_softreset() check for K2G, move the check into dispc_softreset(),
> and make dispc_softreset() return 0 in case of K2G.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index cdbb88289082..f204a0701e9f 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2707,6 +2707,10 @@ static int dispc_softreset(struct dispc_device *dispc)
>  	u32 val;
>  	int ret;
>  
> +	/* K2G display controller does not support soft reset */
> +	if (dispc->feat->subrev == DISPC_K2G)
> +		return 0;
> +
>  	/* Soft reset */
>  	REG_FLD_MOD(dispc, DSS_SYSCONFIG, 1, 1, 1);
>  	/* Wait for reset to complete */
> @@ -2831,12 +2835,9 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	tidss->dispc = dispc;
>  
> -	/* K2G display controller does not support soft reset */
> -	if (feat->subrev != DISPC_K2G) {
> -		r = dispc_softreset(dispc);
> -		if (r)
> -			return r;
> -	}
> +	r = dispc_softreset(dispc);
> +	if (r)
> +		return r;
>  
>  	return 0;
>  }
> 

-- 
Regards,

Laurent Pinchart

[PATCH 07/10] drm/tidss: Fix dss reset

[Tomi Valkeinen]

The probe function calls dispc_softreset() before runtime PM is enabled
and without enabling any of the DSS clocks. This happens to work by
luck, and we need to make sure the DSS HW is active and the fclk is
enabled.

To fix the above, add a new function, dispc_init_hw(), which does:

- pm_runtime_set_active()
- clk_prepare_enable(fclk)
- dispc_softreset().

This ensures that the reset can be successfully accomplished.

Note that we use pm_runtime_set_active(), not the normal
pm_runtime_get(). The reason for this is that at this point we haven't
enabled the runtime PM yet and also we don't want the normal resume
callback to be called: the dispc resume callback does some initial HW
setup, and it expects that the HW was off (no video ports are
streaming). If the bootloader has enabled the DSS and has set up a
boot time splash-screen, the DSS would be enabled and streaming which
might lead to issues with the normal resume callback.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index f204a0701e9f..13db062892e3 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc)
 	return 0;
 }
 
+static int dispc_init_hw(struct dispc_device *dispc)
+{
+	struct device *dev = dispc->dev;
+	int ret;
+
+	ret = pm_runtime_set_active(dev);
+	if (ret) {
+		dev_err(dev, "Failed to set DSS PM to active\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(dispc->fclk);
+	if (ret) {
+		dev_err(dev, "Failed to enable DSS fclk\n");
+		goto err_runtime_suspend;
+	}
+
+	ret = dispc_softreset(dispc);
+	if (ret)
+		goto err_clk_disable;
+
+	clk_disable_unprepare(dispc->fclk);
+	ret = pm_runtime_set_suspended(dev);
+	if (ret) {
+		dev_err(dev, "Failed to set DSS PM to suspended\n");
+		return ret;
+	}
+
+	return 0;
+
+err_clk_disable:
+	clk_disable_unprepare(dispc->fclk);
+
+err_runtime_suspend:
+	ret = pm_runtime_set_suspended(dev);
+	if (ret) {
+		dev_err(dev, "Failed to set DSS PM to suspended\n");
+		return ret;
+	}
+
+	return ret;
+}
+
 int dispc_init(struct tidss_device *tidss)
 {
 	struct device *dev = tidss->dev;
@@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss)
 
 	tidss->dispc = dispc;
 
-	r = dispc_softreset(dispc);
+	r = dispc_init_hw(dispc);
 	if (r)
 		return r;
 

-- 
2.34.1

Re: [PATCH 07/10] drm/tidss: Fix dss reset

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
> The probe function calls dispc_softreset() before runtime PM is enabled
> and without enabling any of the DSS clocks. This happens to work by
> luck, and we need to make sure the DSS HW is active and the fclk is
> enabled.
> 
> To fix the above, add a new function, dispc_init_hw(), which does:
> 
> - pm_runtime_set_active()
> - clk_prepare_enable(fclk)
> - dispc_softreset().
> 
> This ensures that the reset can be successfully accomplished.
> 
> Note that we use pm_runtime_set_active(), not the normal
> pm_runtime_get(). The reason for this is that at this point we haven't
> enabled the runtime PM yet and also we don't want the normal resume
> callback to be called: the dispc resume callback does some initial HW
> setup, and it expects that the HW was off (no video ports are
> streaming). If the bootloader has enabled the DSS and has set up a
> boot time splash-screen, the DSS would be enabled and streaming which
> might lead to issues with the normal resume callback.

I think the right way to do this would be, in probe(), to

- power on the device
- enable runtime PM, masking the device as active
- at end of probe, calling pm_runtime_put_autosuspend()

Please also see my review of patch 02/10.

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 45 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index f204a0701e9f..13db062892e3 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2724,6 +2724,49 @@ static int dispc_softreset(struct dispc_device *dispc)
>  	return 0;
>  }
>  
> +static int dispc_init_hw(struct dispc_device *dispc)
> +{
> +	struct device *dev = dispc->dev;
> +	int ret;
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set DSS PM to active\n");
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(dispc->fclk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable DSS fclk\n");
> +		goto err_runtime_suspend;
> +	}
> +
> +	ret = dispc_softreset(dispc);
> +	if (ret)
> +		goto err_clk_disable;
> +
> +	clk_disable_unprepare(dispc->fclk);
> +	ret = pm_runtime_set_suspended(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set DSS PM to suspended\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +
> +err_clk_disable:
> +	clk_disable_unprepare(dispc->fclk);
> +
> +err_runtime_suspend:
> +	ret = pm_runtime_set_suspended(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set DSS PM to suspended\n");
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
>  int dispc_init(struct tidss_device *tidss)
>  {
>  	struct device *dev = tidss->dev;
> @@ -2835,7 +2878,7 @@ int dispc_init(struct tidss_device *tidss)
>  
>  	tidss->dispc = dispc;
>  
> -	r = dispc_softreset(dispc);
> +	r = dispc_init_hw(dispc);
>  	if (r)
>  		return r;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 07/10] drm/tidss: Fix dss reset

[Tomi Valkeinen]

On 01/11/2023 16:30, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
>> The probe function calls dispc_softreset() before runtime PM is enabled
>> and without enabling any of the DSS clocks. This happens to work by
>> luck, and we need to make sure the DSS HW is active and the fclk is
>> enabled.
>>
>> To fix the above, add a new function, dispc_init_hw(), which does:
>>
>> - pm_runtime_set_active()
>> - clk_prepare_enable(fclk)
>> - dispc_softreset().
>>
>> This ensures that the reset can be successfully accomplished.
>>
>> Note that we use pm_runtime_set_active(), not the normal
>> pm_runtime_get(). The reason for this is that at this point we haven't
>> enabled the runtime PM yet and also we don't want the normal resume
>> callback to be called: the dispc resume callback does some initial HW
>> setup, and it expects that the HW was off (no video ports are
>> streaming). If the bootloader has enabled the DSS and has set up a
>> boot time splash-screen, the DSS would be enabled and streaming which
>> might lead to issues with the normal resume callback.
> 
> I think the right way to do this would be, in probe(), to
> 
> - power on the device
> - enable runtime PM, masking the device as active
> - at end of probe, calling pm_runtime_put_autosuspend()

Can you explain what that would accomplish, or why the code in this 
patch is wrong?

If I understand it right, you're suggesting a more "normal" power up at 
the probe time, and then leaving the DSS enabled, but with autosuspend. 
That would require powering up, doing a reset, and calling 
dispc_runtime_resume. Which can be done, but I'm not sure why it's 
better, as we're not interested in "normal" power up at probe time.

But I can see that my approach looks perhaps a bit odd just by looking 
at these patches. This work was related to keeping the bootloader's 
splash screen on the screen for a longer time, i.e. delaying reset.

For that, I wanted an early function (dispc_init_hw) which would, 
instead of always resetting the DSS as it does in this version, peek at 
the DSS hardware, and see if the DSS is already streaming. If no, do a 
reset and proceed normally. If yes, skip the reset, leave the clocks 
enabled, and keep DSS PM active.

Later, when we'd be doing the first modeset, the driver would do the 
initial reset.

So, that's why I wanted an independent function for the HW probing/init, 
which is called before runtime PM is enabled, and I did not want normal 
runtime resume to be called as dispc_runtime_resume() would break the 
display.

I think a better solution would be to set up the fb of tidss's fbdev to 
use the reserved memory, used for the boot splash screen. But I didn't 
figure out a way to do this. But even there we'd like to delay the reset 
until the first modeset (when the fbdev display is getting enabled).

  Tomi

Re: [PATCH 07/10] drm/tidss: Fix dss reset

[Francesco Dolcini]

On Wed, Nov 01, 2023 at 11:17:44AM +0200, Tomi Valkeinen wrote:
> The probe function calls dispc_softreset() before runtime PM is enabled
> and without enabling any of the DSS clocks. This happens to work by
> luck, and we need to make sure the DSS HW is active and the fclk is
> enabled.

Not sure on the exact implication here, however from the description
this seems a candidate for stable and would need a Fixes tag.

Francesco

[PATCH 08/10] drm/tidss: Add dispc_is_idle()

[Tomi Valkeinen]

Add a helper function, dispc_is_idle(), which returns whether the DSS is
idle (i.e. is any video port enabled).

For now we add a call to it in the suspend and resume callbacks, and
print a warning if in either place the DSS is not idle. In the future
this can be used to detect if the bootloader had enabled the DSS, and
the driver can act on that.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
index 13db062892e3..a527c28c8833 100644
--- a/drivers/gpu/drm/tidss/tidss_dispc.c
+++ b/drivers/gpu/drm/tidss/tidss_dispc.c
@@ -2603,10 +2603,18 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
 	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
 }
 
+static bool dispc_is_idle(struct dispc_device *dispc)
+{
+	return REG_GET(dispc, DSS_SYSSTATUS, 9, 9);
+}
+
 int dispc_runtime_suspend(struct dispc_device *dispc)
 {
 	dev_dbg(dispc->dev, "suspend\n");
 
+	if (!dispc_is_idle(dispc))
+		dev_warn(dispc->dev, "Bad HW state: DSS not idle when suspending");
+
 	dispc->is_enabled = false;
 
 	clk_disable_unprepare(dispc->fclk);
@@ -2620,6 +2628,9 @@ int dispc_runtime_resume(struct dispc_device *dispc)
 
 	clk_prepare_enable(dispc->fclk);
 
+	if (!dispc_is_idle(dispc))
+		dev_warn(dispc->dev, "Bad HW state: DSS not idle when resuming");
+
 	if (REG_GET(dispc, DSS_SYSSTATUS, 0, 0) == 0)
 		dev_warn(dispc->dev, "DSS FUNC RESET not done!\n");
 

-- 
2.34.1

Re: [PATCH 08/10] drm/tidss: Add dispc_is_idle()

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:45AM +0200, Tomi Valkeinen wrote:
> Add a helper function, dispc_is_idle(), which returns whether the DSS is
> idle (i.e. is any video port enabled).
> 
> For now we add a call to it in the suspend and resume callbacks, and
> print a warning if in either place the DSS is not idle.

Could you please explain here why these checks are needed/useful ? Why
would the dispc not be idle ?

> In the future
> this can be used to detect if the bootloader had enabled the DSS, and
> the driver can act on that.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_dispc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 13db062892e3..a527c28c8833 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -2603,10 +2603,18 @@ void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
>  	dispc_vp_set_color_mgmt(dispc, hw_videoport, state, newmodeset);
>  }
>  
> +static bool dispc_is_idle(struct dispc_device *dispc)
> +{
> +	return REG_GET(dispc, DSS_SYSSTATUS, 9, 9);
> +}
> +
>  int dispc_runtime_suspend(struct dispc_device *dispc)
>  {
>  	dev_dbg(dispc->dev, "suspend\n");
>  
> +	if (!dispc_is_idle(dispc))
> +		dev_warn(dispc->dev, "Bad HW state: DSS not idle when suspending");
> +
>  	dispc->is_enabled = false;
>  
>  	clk_disable_unprepare(dispc->fclk);
> @@ -2620,6 +2628,9 @@ int dispc_runtime_resume(struct dispc_device *dispc)
>  
>  	clk_prepare_enable(dispc->fclk);
>  
> +	if (!dispc_is_idle(dispc))
> +		dev_warn(dispc->dev, "Bad HW state: DSS not idle when resuming");
> +
>  	if (REG_GET(dispc, DSS_SYSSTATUS, 0, 0) == 0)
>  		dev_warn(dispc->dev, "DSS FUNC RESET not done!\n");
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 08/10] drm/tidss: Add dispc_is_idle()

[Tomi Valkeinen]

On 01/11/2023 16:32, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:45AM +0200, Tomi Valkeinen wrote:
>> Add a helper function, dispc_is_idle(), which returns whether the DSS is
>> idle (i.e. is any video port enabled).
>>
>> For now we add a call to it in the suspend and resume callbacks, and
>> print a warning if in either place the DSS is not idle.
> 
> Could you please explain here why these checks are needed/useful ? Why
> would the dispc not be idle ?

I'll drop this. This was mostly a debugging aid for myself when testing 
runtime PM.

  Tomi

[PATCH 09/10] drm/tidss: IRQ code cleanup

[Tomi Valkeinen]

The IRQ setup code is overly complex. All we really need to do is
initialize the related fields in struct tidss_device, and request the
IRQ.

We can drop all the HW accesses, as they are pointless: the driver will
set the IRQs correctly when it needs any of the IRQs, and at probe time
we have done a reset, so we know that all the IRQs are masked by default
in the hardware.

Thus we can combine the tidss_irq_preinstall() and
tidss_irq_postinstall() into the tidss_irq_install() function, drop the
HW accesses, and drop the use of spinlock, as this is done at init time
and there can be no races.

We can also drop the HW access from the tidss_irq_uninstall(), as the
driver will anyway disable and suspend the hardware at remove time.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_drv.c |  2 ++
 drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
 2 files changed, 10 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
index 64914331715a..37693f30d98b 100644
--- a/drivers/gpu/drm/tidss/tidss_drv.c
+++ b/drivers/gpu/drm/tidss/tidss_drv.c
@@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, tidss);
 
+	spin_lock_init(&tidss->wait_lock);
+
 	ret = dispc_init(tidss);
 	if (ret) {
 		dev_err(dev, "failed to initialize dispc: %d\n", ret);
diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
index 0c681c7600bc..604334ef526a 100644
--- a/drivers/gpu/drm/tidss/tidss_irq.c
+++ b/drivers/gpu/drm/tidss/tidss_irq.c
@@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
 	spin_unlock_irqrestore(&tidss->wait_lock, flags);
 }
 
-static void tidss_irq_preinstall(struct drm_device *ddev)
-{
-	struct tidss_device *tidss = to_tidss(ddev);
-
-	spin_lock_init(&tidss->wait_lock);
-
-	tidss_runtime_get(tidss);
-
-	dispc_set_irqenable(tidss->dispc, 0);
-	dispc_read_and_clear_irqstatus(tidss->dispc);
-
-	tidss_runtime_put(tidss);
-}
-
-static void tidss_irq_postinstall(struct drm_device *ddev)
+int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
 {
 	struct tidss_device *tidss = to_tidss(ddev);
-	unsigned long flags;
-	unsigned int i;
+	int ret;
 
-	tidss_runtime_get(tidss);
+	if (irq == IRQ_NOTCONNECTED)
+		return -ENOTCONN;
 
-	spin_lock_irqsave(&tidss->wait_lock, flags);
+	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
+	if (ret)
+		return ret;
 
 	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
 
-	for (i = 0; i < tidss->num_crtcs; ++i) {
+	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
 		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
 
 		tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
@@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
 		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
 	}
 
-	tidss_irq_update(tidss);
-
-	spin_unlock_irqrestore(&tidss->wait_lock, flags);
-
-	tidss_runtime_put(tidss);
-}
-
-int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
-{
-	int ret;
-
-	if (irq == IRQ_NOTCONNECTED)
-		return -ENOTCONN;
-
-	tidss_irq_preinstall(ddev);
-
-	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
-	if (ret)
-		return ret;
-
-	tidss_irq_postinstall(ddev);
-
 	return 0;
 }
 
@@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
 {
 	struct tidss_device *tidss = to_tidss(ddev);
 
-	tidss_runtime_get(tidss);
-	dispc_set_irqenable(tidss->dispc, 0);
-	tidss_runtime_put(tidss);
-
 	free_irq(tidss->irq, ddev);
 }

-- 
2.34.1

Re: [PATCH 09/10] drm/tidss: IRQ code cleanup

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
> The IRQ setup code is overly complex. All we really need to do is
> initialize the related fields in struct tidss_device, and request the
> IRQ.
> 
> We can drop all the HW accesses, as they are pointless: the driver will
> set the IRQs correctly when it needs any of the IRQs, and at probe time
> we have done a reset, so we know that all the IRQs are masked by default
> in the hardware.

Even for K2G ?

> Thus we can combine the tidss_irq_preinstall() and
> tidss_irq_postinstall() into the tidss_irq_install() function, drop the
> HW accesses, and drop the use of spinlock, as this is done at init time
> and there can be no races.
> 
> We can also drop the HW access from the tidss_irq_uninstall(), as the
> driver will anyway disable and suspend the hardware at remove time.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c |  2 ++
>  drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
>  2 files changed, 10 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 64914331715a..37693f30d98b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, tidss);
>  
> +	spin_lock_init(&tidss->wait_lock);
> +
>  	ret = dispc_init(tidss);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize dispc: %d\n", ret);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 0c681c7600bc..604334ef526a 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
>  	spin_unlock_irqrestore(&tidss->wait_lock, flags);
>  }
>  
> -static void tidss_irq_preinstall(struct drm_device *ddev)
> -{
> -	struct tidss_device *tidss = to_tidss(ddev);
> -
> -	spin_lock_init(&tidss->wait_lock);
> -
> -	tidss_runtime_get(tidss);
> -
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	dispc_read_and_clear_irqstatus(tidss->dispc);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -static void tidss_irq_postinstall(struct drm_device *ddev)
> +int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
> -	unsigned long flags;
> -	unsigned int i;
> +	int ret;
>  
> -	tidss_runtime_get(tidss);
> +	if (irq == IRQ_NOTCONNECTED)
> +		return -ENOTCONN;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> +	if (ret)
> +		return ret;
>  
>  	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
>  
> -	for (i = 0; i < tidss->num_crtcs; ++i) {
> +	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
>  		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
>  
>  		tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
> @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
>  		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
>  	}
>  
> -	tidss_irq_update(tidss);
> -
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> -{
> -	int ret;
> -
> -	if (irq == IRQ_NOTCONNECTED)
> -		return -ENOTCONN;
> -
> -	tidss_irq_preinstall(ddev);
> -
> -	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> -	if (ret)
> -		return ret;
> -
> -	tidss_irq_postinstall(ddev);
> -
>  	return 0;
>  }
>  
> @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
>  
> -	tidss_runtime_get(tidss);
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	tidss_runtime_put(tidss);
> -
>  	free_irq(tidss->irq, ddev);
>  }
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 09/10] drm/tidss: IRQ code cleanup

[Tomi Valkeinen]

On 01/11/2023 16:52, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
>> The IRQ setup code is overly complex. All we really need to do is
>> initialize the related fields in struct tidss_device, and request the
>> IRQ.
>>
>> We can drop all the HW accesses, as they are pointless: the driver will
>> set the IRQs correctly when it needs any of the IRQs, and at probe time
>> we have done a reset, so we know that all the IRQs are masked by default
>> in the hardware.
> 
> Even for K2G ?

Good point. I'll add a simple manual reset for k2g, masking the IRQs and 
disabling the VPs.

  Tomi

[PATCH 10/10] drm/tidss: Fix atomic_flush check

[Tomi Valkeinen]

tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
returns immediately as there's no reason to do any register changes.

However, the code checks for 'crtc->state->enable', which does not
reflect the actual HW state. We should instead look at the
'crtc->state->active' flag.

This causes the tidss_crtc_atomic_flush() to proceed with the flush even
if the active state is false, which then causes us to hit the
WARN_ON(!crtc->state->event) check.

Fix this by checking the active flag, and while at it, fix the related
debug print which had "active" and "needs modeset" wrong way.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 5e5e466f35d1..4c7009a5d643 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
 	struct tidss_device *tidss = to_tidss(ddev);
 	unsigned long flags;
 
-	dev_dbg(ddev->dev,
-		"%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
-		crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
-		crtc->state->enable, crtc->state->event);
+	dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
+		__func__, crtc->name, crtc->state->active,
+		drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
 
 	/* There is nothing to do if CRTC is not going to be enabled. */
-	if (!crtc->state->enable)
+	if (!crtc->state->active)
 		return;
 
 	/*

-- 
2.34.1

Re: [PATCH 10/10] drm/tidss: Fix atomic_flush check

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
> returns immediately as there's no reason to do any register changes.
> 
> However, the code checks for 'crtc->state->enable', which does not
> reflect the actual HW state. We should instead look at the
> 'crtc->state->active' flag.
> 
> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
> if the active state is false, which then causes us to hit the
> WARN_ON(!crtc->state->event) check.
> 
> Fix this by checking the active flag, and while at it, fix the related
> debug print which had "active" and "needs modeset" wrong way.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 5e5e466f35d1..4c7009a5d643 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>  	struct tidss_device *tidss = to_tidss(ddev);
>  	unsigned long flags;
>  
> -	dev_dbg(ddev->dev,
> -		"%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
> -		crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
> -		crtc->state->enable, crtc->state->event);
> +	dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
> +		__func__, crtc->name, crtc->state->active,
> +		drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);

While at it, how about this ?

	dev_dbg(ddev->dev, "%s: %s is %sactive, %s modeset, event %p\n",
		__func__, crtc->name, crtc->state->active ? "" : "not ",
		drm_atomic_crtc_needs_modeset(crtc->state) ? "needs", "doesn't need",
		crtc->state->event);

>  
>  	/* There is nothing to do if CRTC is not going to be enabled. */
> -	if (!crtc->state->enable)
> +	if (!crtc->state->active)

I think the drm_atomic_helper_commit_planes() helper will handle this if
you pass it the DRM_PLANE_COMMIT_ACTIVE_ONLY flag.

>  		return;
>  
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH 10/10] drm/tidss: Fix atomic_flush check

[Tomi Valkeinen]

On 01/11/2023 16:56, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
>> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
>> returns immediately as there's no reason to do any register changes.
>>
>> However, the code checks for 'crtc->state->enable', which does not
>> reflect the actual HW state. We should instead look at the
>> 'crtc->state->active' flag.
>>
>> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
>> if the active state is false, which then causes us to hit the
>> WARN_ON(!crtc->state->event) check.
>>
>> Fix this by checking the active flag, and while at it, fix the related
>> debug print which had "active" and "needs modeset" wrong way.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/tidss/tidss_crtc.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 5e5e466f35d1..4c7009a5d643 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -169,13 +169,12 @@ static void tidss_crtc_atomic_flush(struct drm_crtc *crtc,
>>   	struct tidss_device *tidss = to_tidss(ddev);
>>   	unsigned long flags;
>>   
>> -	dev_dbg(ddev->dev,
>> -		"%s: %s enabled %d, needs modeset %d, event %p\n", __func__,
>> -		crtc->name, drm_atomic_crtc_needs_modeset(crtc->state),
>> -		crtc->state->enable, crtc->state->event);
>> +	dev_dbg(ddev->dev, "%s: %s active %d, needs modeset %d, event %p\n",
>> +		__func__, crtc->name, crtc->state->active,
>> +		drm_atomic_crtc_needs_modeset(crtc->state), crtc->state->event);
> 
> While at it, how about this ?

Why not. The active part won't be needed if we use 
DRM_PLANE_COMMIT_ACTIVE_ONLY, though.

> 	dev_dbg(ddev->dev, "%s: %s is %sactive, %s modeset, event %p\n",
> 		__func__, crtc->name, crtc->state->active ? "" : "not ",
> 		drm_atomic_crtc_needs_modeset(crtc->state) ? "needs", "doesn't need",
> 		crtc->state->event);
> 
>>   
>>   	/* There is nothing to do if CRTC is not going to be enabled. */
>> -	if (!crtc->state->enable)
>> +	if (!crtc->state->active)
> 
> I think the drm_atomic_helper_commit_planes() helper will handle this if
> you pass it the DRM_PLANE_COMMIT_ACTIVE_ONLY flag.

I considered it, but it does a bit more, as it also affects the plane 
updates. We specifically did not use DRM_PLANE_COMMIT_ACTIVE_ONLY as it 
didn't work with DSS.

That said, I can't figure out what was the issue. It's possible the 
issue was only on the older DSS HW, with omapdrm (tidss code was 
originally somewhat based on omapdrm). I'm pretty sure the issue was 
related to multi-display systems and the plane updates there. But I 
don't have any multi-display board which uses tidss.

So, I'll keep this patch, but add another on top, which uses 
DRM_PLANE_COMMIT_ACTIVE_ONLY. Then it'll be easy to revert the 
DRM_PLANE_COMMIT_ACTIVE_ONLY one if needed, while still keeping this fix.

  Tomi

Re: [PATCH 10/10] drm/tidss: Fix atomic_flush check

[Francesco Dolcini]

On Wed, Nov 01, 2023 at 11:17:47AM +0200, Tomi Valkeinen wrote:
> tidss_crtc_atomic_flush() checks if the crtc is enabled, and if not,
> returns immediately as there's no reason to do any register changes.
> 
> However, the code checks for 'crtc->state->enable', which does not
> reflect the actual HW state. We should instead look at the
> 'crtc->state->active' flag.
> 
> This causes the tidss_crtc_atomic_flush() to proceed with the flush even
> if the active state is false, which then causes us to hit the
> WARN_ON(!crtc->state->event) check.
> 
> Fix this by checking the active flag, and while at it, fix the related
> debug print which had "active" and "needs modeset" wrong way.

Candidate for stable? Add a Fixes tag?

Francesco