Re: [PATCH v2 02/18] drm/exynos: use wait_event_timeout() for safety usage

From: YoungJun Cho <yj44.cho@samsung.com>
To: Daniel Kurtz <djkurtz@chromium.org>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org,
	linux-samsung-soc <linux-samsung-soc@vger.kernel.org>,
	Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Seung-Woo Kim <sw0312.kim@samsung.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Kyungmin Park <kyungmin.park@samsung.com>,
	Rob Herring <robh+dt@kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Kumar Gala <galak@codeaurora.org>,
	Kukjin Kim <kgene.kim@samsung.com>,
	s.trumtrar@pengutronix.de
Subject: Re: [PATCH v2 02/18] drm/exynos: use wait_event_timeout() for safety usage
Date: Wed, 21 May 2014 15:28:05 +0900	[thread overview]
Message-ID: <537C4775.1050702@samsung.com> (raw)
In-Reply-To: <CAGS+omCHJppm=9GeFarb_UhwB-mSJi+nxWFw1FC+ew3OOA-srQ@mail.gmail.com>

Hi Daniel

On 05/21/2014 03:01 PM, Daniel Kurtz wrote:
> On Wed, May 21, 2014 at 12:42 PM, YoungJun Cho <yj44.cho@samsung.com> wrote:
>> There could be the case that the page flip operation isn't finished correctly
>> with some abnormal condition such as panel reset. So this patch replaces
>> wait_event() with wait_event_timeout() to avoid waiting for page flip completion
>> infinitely.
>> And clears exynos_crtc->pending_flip in exynos_drm_crtc_page_flip() when
>> exynos_drm_crtc_mode_set_commit() is failed.
>>
>> Signed-off-by: YoungJun Cho <yj44.cho@samsung.com>
>> Acked-by: Inki Dae <inki.dae@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_crtc.c |    7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> index 95c9435..3bf091d 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
>> @@ -69,8 +69,10 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
>>
>>          if (mode > DRM_MODE_DPMS_ON) {
>>                  /* wait for the completion of page flip. */
>> -               wait_event(exynos_crtc->pending_flip_queue,
>> -                               atomic_read(&exynos_crtc->pending_flip) == 0);
>> +               if (!wait_event_timeout(exynos_crtc->pending_flip_queue,
>> +                               !atomic_read(&exynos_crtc->pending_flip),
>> +                               HZ/20))
>> +                       atomic_set(&exynos_crtc->pending_flip, 0);
>
> I meant that changing this to wait_event_timeout() seems to be masking
> the original problem, that pending_flip wasn't being cleared.

Yes, I agree.

The original purpose of this patch is to avoid lock-up during modetest 
(with test_vsync) with command mode panel.
In MIPI DSI command mode interface, the display controller can not 
generate VSYNC signal and uses TE signal instead which is generated by 
panel.
If there is an abnormal power off or reset condition, it is possible 
that the display controller misses the TE signal and it makes lock-up.
So I needed this patch.

> Now that you are now clearing pending_flip in the error path, you
> don't need the timeout, right?
>

There might be my missing point. Would you explain more detail?

Thank you.
Best regards YJ

> -Dan
>
>>                  drm_vblank_off(crtc->dev, exynos_crtc->pipe);
>>          }
>>
>> @@ -259,6 +261,7 @@ static int exynos_drm_crtc_page_flip(struct drm_crtc *crtc,
>>                          spin_lock_irq(&dev->event_lock);
>>                          drm_vblank_put(dev, exynos_crtc->pipe);
>>                          list_del(&event->base.link);
>> +                       atomic_set(&exynos_crtc->pending_flip, 0);
>>                          spin_unlock_irq(&dev->event_lock);
>>
>>                          goto out;
>> --
>> 1.7.9.5
>>
>