2012/12/5 Prathyush K <prathy...@chromium.org>
>
>
>
> On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki....@samsung.com> wrote:
>
>>
>>
>> 2012/12/5 Inki Dae <inki....@samsung.com>
>>
>>>
>>>
>>> 2012/12/5 Prathyush K <prathyus...@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@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
