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

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

[Prathyush K]

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.

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(-)

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 2661 bytes --]

2012/12/5 Prathyush K <prathyush.k@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.

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
>

[-- Attachment #1.2: Type: text/html, Size: 3709 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 3712 bytes --]

2012/12/5 Inki Dae <inki.dae@samsung.com>

>
>
> 2012/12/5 Prathyush K <prathyush.k@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.

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.

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
>>
>
>

[-- Attachment #1.2: Type: text/html, Size: 5259 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Prathyush K]

[-- Attachment #1.1: Type: text/plain, Size: 4703 bytes --]

On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki.dae@samsung.com> wrote:

>
>
> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>
>>
>>
>> 2012/12/5 Prathyush K <prathyush.k@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.

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
>
>

[-- Attachment #1.2: Type: text/html, Size: 7581 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 5862 bytes --]

2012/12/5 Prathyush K <prathyush@chromium.org>

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

[-- Attachment #1.2: Type: text/html, Size: 9351 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 6237 bytes --]

2012/12/5 Inki Dae <inki.dae@samsung.com>

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

[-- Attachment #1.2: Type: text/html, Size: 10046 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 6790 bytes --]

2012/12/5 Inki Dae <inki.dae@samsung.com>

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

[-- Attachment #1.2: Type: text/html, Size: 10940 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eunchul Kim]

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

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

[Prathyush K]

[-- Attachment #1.1: Type: text/plain, Size: 8797 bytes --]

On Wed, Dec 5, 2012 at 5:33 PM, Eunchul Kim <chulspro.kim@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@samsung.com>
>>
>>
>>>
>>> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>>>
>>>
>>>>
>>>> 2012/12/5 Prathyush K <prathyush@chromium.org>
>>>>
>>>>
>>>>>
>>>>>
>>>>> On Wed, Dec 5, 2012 at 11:40 AM, Inki Dae <inki.dae@samsung.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>>>
>>>>>> 2012/12/5 Inki Dae <inki.dae@samsung.com>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> 2012/12/5 Prathyush K <prathyush.k@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@lists.freedesktop.**org <dri-devel@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@lists.freedesktop.**org <dri-devel@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@lists.freedesktop.**org <dri-devel@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@lists.freedesktop.**org <dri-devel@lists.freedesktop.org>
>> http://lists.freedesktop.org/**mailman/listinfo/dri-devel<http://lists.freedesktop.org/mailman/listinfo/dri-devel>
>>
>>
>

[-- Attachment #1.2: Type: text/html, Size: 11786 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel