[PATCH 0/3] drm/exynos: modify usage of wait for vblank
Inki Dae
inki.dae at samsung.com
Tue Dec 4 23:27:40 PST 2012
2012/12/5 Inki Dae <inki.dae at samsung.com>
>
>
> 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
>
>
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 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/64bd8d16/attachment.html>
More information about the dri-devel
mailing list