On Wed, Dec 5, 2012 at 5:33 PM, Eunchul Kim <chulspro.kim at samsung.com>wrote:
> 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)
>
Hi Eunchul,
Yes, I think this is better. I'll modify this in the next patch version and
post tomorrow.
Thanks for reviewing.
Regards,
Prathyush
> + 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 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 <dri-devel at
>>>>>>>> lists.freedesktop.org>
>>>>>>>> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<http://lists.freedesktop.org/mailman/listinfo/dri-devel>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> ______________________________**_________________
>>>>>> dri-devel mailing list
>>>>>> dri-devel at lists.freedesktop.**org <dri-devel at lists.freedesktop.org>
>>>>>> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<http://lists.freedesktop.org/mailman/listinfo/dri-devel>
>>>>>>
>>>>>>
>>>>>>
>>>>> ______________________________**_________________
>>>>> dri-devel mailing list
>>>>> dri-devel at lists.freedesktop.**org <dri-devel at lists.freedesktop.org>
>>>>> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<http://lists.freedesktop.org/mailman/listinfo/dri-devel>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>>
>> ______________________________**_________________
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.**org <dri-devel at lists.freedesktop.org>
>> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<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/8761e778/attachment-0001.html>
