Re: [PATCH 0/3] drm/exynos: modify usage of wait for vblank

[Eunchul Kim <chulspro.kim@samsung.com>]

Dear Prathyush

Thank's your clarification.

I have one opinion about your [PATCH 1/3]
How about to add atomic_read() condition in each irq_handler ?
like this.

e.g)
+	if (atomic_read(&ctx->wait_vsync_event)) {
+		/* set wait vsync event to zero and wake up queue. */
+		atomic_set(&ctx->wait_vsync_event, 0);
+		DRM_WAKEUP(&ctx->wait_vsync_queue);
+	}

I lost your PATCH mail. so, I send my comment using this body mail.

Thank's
BR

Eunchul Kim.

On 12/05/2012 04:27 PM, Inki Dae wrote:
> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>
>>
>>
>> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>>
>>>
>>>
>>> 2012/12/5 Prathyush K <prathyush@chromium.org>
>>>
>>>>
>>>>
>>>>
>>>> On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>>>
>>>>>
>>>>>
>>>>> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>>>>>
>>>>>>
>>>>>>
>>>>>> 2012/12/5 Prathyush K <prathyush.k@samsung.com>
>>>>>>
>>>>>>> This patchset fixes a few issues with use of wait for vblank in
>>>>>>> exynos drm.
>>>>>>>
>>>>>>> Please apply these three patches without "drm/exynos: release fb
>>>>>>> pended by page flip"
>>>>>>> patch.
>>>>>>>
>>>>>>> Patch 1: modify wait for vsync functions to use wait queues
>>>>>>> This modifies the wait_for_vblank functions to use wait queues
>>>>>>> thus ensuring that the current task goes to sleep without taking
>>>>>>> up CPU while waiting.
>>>>>>>
>>>>>>> Patch 2: move wait_for_vblank to manager_ops
>>>>>>> Currently, we do not call wait for vblank if encoder is in DPMS_OFF
>>>>>>> state inside exynos_drm_encoder_complete_scanout function. This is
>>>>>>> because wait for vblank is treated as an overlay op.
>>>>>>
>>>>>> This can be
>>>>>>> modified to a manager_op thus removing the above check for DPMS_OFF.
>>>>>>> This fixes a crash while removing a fb.
>>>>>>> While removing the current fb (crtc->fb == fb) the encoder
>>>>>>> dpms off is called and then the framebuffer is removed. So in
>>>>>>> this case, the wait for vblank is not called thus leading to a crash
>>>>>>> (since fb might get removed before dma is finished)
>>>>>>>
>>>>>>> Patch 3: do not disable crtc if already off
>>>>>>> We should not disable the overlay if the crtc is in DPMS_OFF state.
>>>>>>> Also, the disable function should not call DPMS_OFF of the crtc.
>>>>>>> This is required to ensure we are able to wait for vblank
>>>>>>> before freeing any framebuffers after disabling the crtc.
>>>>>>>
>>>>>>> With these three patches and without "drm/exynos: release fb pended
>>>>>>> by page flip"
>>>>>>> I could not find any crash/page_fault in drm with fimd/hdmi during
>>>>>>> hotplug
>>>>>>> and page flips.
>>>>>>>
>>>>>>>
>>>>>> It seems better than old one and also including some exception codes,
>>>>>> Patch 3 we did't consider. Ok, we will test these patches and merge them
>>>>>> instead of old one if no problem.
>>>>>>
>>>>>>
>>>>>
>>>>> Tested and worked fine. But with this patch and without "drm/exynos:
>>>>> release fb pended by page flip", we would still have potential issue to
>>>>> page fault like below,
>>>>> 1. dma is accessing no current fb
>>>>> 2. the fb to be released is not current fb so the fb would be released
>>>>> without disabling crtc when drm is released.
>>>>> 3. but the memory region to no current fb is being accessed by the dma
>>>>> so the page fault would be induced. This is a rare case but possible issue.
>>>>>
>>>>> Hi Mr. Dae,
>>>>
>>>> But we would not release the fb till we get a vblank (i.e. wait for
>>>> vblank is called before removing every framebuffer).
>>>>
>>>> Taking your example itself:
>>>>
>>>> fb0 is current fb i.e. (crtc->fb = fb0).
>>>>
>>>> But the dma is accessing from fb1.
>>>>
>>>> We call remove fb1.
>>>>
>>>> In this case, as you said, crtc will not be disabled as fb1 is not
>>>> current fb.
>>>>
>>>> But we wait for vblank before removing fb1. Thus we ensure that dma from
>>>> fb1 is complete and dma from fb0 has started.
>>>> So it is safe to remove fb1.
>>>>
>>>
>>> Please, see the below codes,
>>>
>>> drm_release()
>>>          drm_fb_release()
>>>                  drm_framebuffer_remove()
>>>                          /* assume that current fb is fb1 and dma is
>>> accessing fb0 but trying to release fb0 at here */
>>>                          /* this situation could be reproduced with
>>> pageflip test. */
>>>                          fb0 is not crtc->fb so the crtc isn't disabled
>>>                          ...
>>>                          drm_framebuffer_unreference(fb0);  <- fb0 will be
>>> released without wait_for_vblank().
>>>
>>> The wait_for_vblank() is called by exynos_drm_encoder_complete_scanout()
>>> when fb->func->destroy() is called so the wait_for_vblank() will be called
>>> after fb0 is released.
>>> Is there another place that wait_for_vblank() is called?
>>>
>>>
>> Ah, sorry~. exynos_drm_encoder_completescanout() is called prior to gem
>> releasing. Right, no problem. :)
>>
>> Thanks,
>> Inki Dae
>>
>>
>
> In addition,
>
> I had added exynos_drm_encoder_complete_scanout() function to resolve this
> issue. but it seems like that previous patch didn't perform wait_for_vblank
> correctly. Anyway this is just exynos specific code. so I hope maybe
> Daniel's fixup can resolve this issue in drm core.
>
> Thanks,
> Inki Dae
>
>
>>
>>> Thanks,
>>> Inki Dae
>>>
>>>
>>>> Hope I correctly understood the issue you mentioned.
>>>>
>>>>
>>>>
>>>>> Anyway I removed the patch, "drm/exynos: release fb pended by page
>>>>> flip". And I hope this issue can be resolved by Daniel's fixup later.
>>>>>
>>>>> And I think it's better to separate your patch set into 4 patches like
>>>>> below,
>>>>> 1. the patch including only exynos drm framework part.
>>>>> 2. the patch including only fimd driver part.
>>>>> 3. the patch including only hdmi driver part.
>>>>> 4. the patch including exception code.(previous Patch 3)
>>>>>
>>>>> Please, re-send them.
>>>>>
>>>>> Sure. I'll post them again as requested.
>>>>
>>>> Regards,
>>>> Prathyush
>>>>
>>>>
>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>   Thanks,
>>>>>> Inki Dae
>>>>>>
>>>>>>
>>>>>>> Prathyush K (3)
>>>>>>>    drm/exynos: modify wait for vsync functions to use wait queues
>>>>>>>    drm/exynos: move wait_for_vblank to manager_ops
>>>>>>>    drm/exynos: do not disable crtc if already off
>>>>>>>
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_crtc.c    |    6 +++-
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_drv.h     |   20 ++------------
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_encoder.c |   15 +++--------
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_fimd.c    |   37
>>>>>>> ++++++++++++++++++--------
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_hdmi.c    |   22 ++++++++--------
>>>>>>>   drivers/gpu/drm/exynos/exynos_drm_hdmi.h    |    2 +-
>>>>>>>   drivers/gpu/drm/exynos/exynos_mixer.c       |   37
>>>>>>> +++++++++++++++++---------
>>>>>>>   7 files changed, 73 insertions(+), 66 deletions(-)
>>>>>>>
>>>>>>> _______________________________________________
>>>>>>> dri-devel mailing list
>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> dri-devel mailing list
>>>>> dri-devel@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>>
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>>>
>>>
>>
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>