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

[Inki Dae]

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?

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/5ffcd1dd/attachment.html>