Re: [PATCH v2 03/12] drm/exynos/decon5433: fix vblank event handling

From: Andrzej Hajda <a.hajda@samsung.com>
To: "Michel Dänzer" <michel@daenzer.net>, "Inki Dae" <inki.dae@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	dri-devel@lists.freedesktop.org,
	Javier Martinez Canillas <javier@osg.samsung.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Marek Szyprowski <m.szyprowski@samsung.com>
Subject: Re: [PATCH v2 03/12] drm/exynos/decon5433: fix vblank event handling
Date: Thu, 09 Mar 2017 07:54:44 +0100	[thread overview]
Message-ID: <288be3ac-8274-b8e4-ff9f-03101af6bdec@samsung.com> (raw)
In-Reply-To: <50f9ade8-4bb2-6071-6c2b-89ea8f525613@daenzer.net>

On 09.03.2017 04:54, Michel Dänzer wrote:
> On 08/03/17 11:58 PM, Andrzej Hajda wrote:
>> Current implementation of event handling assumes that vblank interrupt is
>> always called at the right time. It is not true, it can be delayed due to
>> various reasons. As a result different races can happen. The patch fixes
>> the issue by using hardware frame counter present in DECON to serialize
>> vblank and commit completion events.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> v2:
>>   - added internal decon_get_frame_count function,
>>   - updated frame counter on flush,
>>   - misc style fixes (thank Inki)
> [...]
>
>> @@ -579,6 +636,23 @@ static const struct component_ops decon_component_ops = {
>>  	.unbind = decon_unbind,
>>  };
>>  
>> +static void decon_handle_vblank(struct decon_context *ctx)
>> +{
>> +	u32 frm;
>> +
>> +	spin_lock(&ctx->vblank_lock);
>> +
>> +	frm = decon_get_frame_count(ctx, true);
>> +
>> +	if (frm != ctx->frame_id) {
>> +		if (frm > ctx->frame_id)
>> +			drm_crtc_handle_vblank(&ctx->crtc->base);
> This comparison isn't safe WRT the counter value returned by
> decon_get_frame_count wrapping around.

And knowing that max framerate is 60fps it will happen after:

0xffffffff / 60fps / 60sec / 60min / 24h / 365days = 2.3 years

So after 2.3 years of uninterrupted work of panel/TV we will lose one or
two vblanks :) Highly improbable and even then low damage.

> If it goes all the way up to
> 0xffffffff before wrapping around to zero, it can be handled e.g. by
>
> 		if ((s32)(frm - ctx->frame_id) > 0)
>
> otherwise it gets a bit more complicated.
>
>
But the fix looks simple so I think it is worth to fix it. Thanks.

Regards

Andrzej

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel