diff mbox series

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

Message ID 20231101-tidss-probe-v1-10-45149e0f9415@ideasonboard.com
State New
Headers
Series drm/tidss: Probe related fixes and cleanups |

Commit Message

Tomi Valkeinen Nov. 1, 2023, 9:17 a.m. UTC 
  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(-)

Comments

Laurent Pinchart Nov. 1, 2023, 2:56 p.m. UTC | #1 
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;
>  
>  	/*
Tomi Valkeinen Nov. 2, 2023, 8:23 a.m. UTC | #2 
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
Francesco Dolcini Nov. 2, 2023, 2:55 p.m. UTC | #3 
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
diff mbox series

Patch

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;
 
 	/*