linux-kernel - Re: [PATCH 10/10] drm/tidss: Fix atomic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 2 Nov 2023 10:23:58 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Aradhya Bhatia <a-bhatia1@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jyri Sarha <jyri.sarha@....fi>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/10] drm/tidss: Fix atomic_flush check

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@...asonboard.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