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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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