[PATCH 0/3] drm/exynos: modify usage of wait for vblank
Inki Dae
inki.dae at samsung.com
Tue Dec 4 23:16:52 PST 2012
2012/12/5 Inki Dae <inki.dae at samsung.com>
>
>
> 2012/12/5 Prathyush K <prathyush at chromium.org>
>
>>
>>
>>
>> On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki.dae at samsung.com> wrote:
>>
>>>
>>>
>>> 2012/12/5 Inki Dae <inki.dae at samsung.com>
>>>
>>>>
>>>>
>>>> 2012/12/5 Prathyush K <prathyush.k at 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
> 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 at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>
>>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20121205/8bd0d812/attachment-0001.html>
More information about the dri-devel
mailing list