When a plane is being disabled but it's still enabled, do check if the
previous update has been completed by reading yrgb_mst back.
Otherwise, pending pageflips would remain pending after a CRTC is
disabled.
Signed-off-by: Tomeu Vizoso <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a9b1e8b5ac85..f46b1fd1887b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct vop_win *vop_win)
struct vop_plane_state *state = to_vop_plane_state(plane->state);
dma_addr_t yrgb_mst;
- if (!state->enable)
- return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
+ if (!state->enable &&
+ VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0)
+ return true;
yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
--
2.5.5
When a VOP is disabled, it will stop raising interrupts. If we had a
pending pageflip when the VOP is disabled, userspace won't get that
event until the corresponding CRTC is enabled again.
So let's wait for any pending events that may be right before disabling
a CRTC.
Signed-off-by: Tomeu Vizoso <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index f46b1fd1887b..6dc87fa96f29 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -142,6 +142,8 @@ struct vop {
struct vop_win win[];
};
+static void vop_crtc_wait_for_update(struct drm_crtc *crtc);
+
static inline void vop_writel(struct vop *vop, uint32_t offset, uint32_t v)
{
writel(v, vop->regs + offset);
@@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
if (!vop->is_enabled)
return;
+ if (crtc->state->event || vop->event)
+ vop_crtc_wait_for_update(crtc);
+
/*
* We need to make sure that all windows are disabled before we
* disable that crtc. Otherwise we might try to scan from a destroyed
--
2.5.5
On 8 April 2016 at 03:07, Mark yao <[email protected]> wrote:
> On 2016年04月06日 18:14, Tomeu Vizoso wrote:
>
> When a plane is being disabled but it's still enabled, do check if the
> previous update has been completed by reading yrgb_mst back.
>
> Otherwise, pending pageflips would remain pending after a CRTC is
> disabled.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a9b1e8b5ac85..f46b1fd1887b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct vop_win
> *vop_win)
> struct vop_plane_state *state = to_vop_plane_state(plane->state);
> dma_addr_t yrgb_mst;
>
> - if (!state->enable)
> - return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
> + if (!state->enable &&
> + VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0)
> + return true;
>
>
> It is wrong, the patch would cause a bug.
>
> when state->enable is 0, check yrgb_mst == state->yrgb_mst always be true,
> because state->yrgb_mst not update on plane disabled funtion, that would
> cause iommu crash.
Sorry, but I don't understand where's the bug and what could cause
that crash. What the existing code was doing is saying that a pageflip
event is still pending if we have told the plane to disable but for
some reason it hasn't yet.
With this modification, if we read back that it's already disabled, we
return true as before. But if we read back that it isn't disabled yet,
then we still check the fb pointers and compare them.
The iommu mapping is removed when the _CRTC_ is disabled, and what
this series does is to wait for the pending pageflip to finish before
conitnuing with CRTC disablement.
> About pending pageflips would remain pending, can you describe more info
> about it? I think those pending pageflips should be ignore when CRTC is
> disabled.
Well, right now in rockchip-drm pending pageflips won't be ignored
when a CRTC is disabled, but will be delivered when it's re-enabled.
If they would be to be ignored (understanding that as dropped), that
would require modifications to clients so they keep track of which fbs
were used in a particular crtc and destroy them when the crtc is
disabled, but that would be incorrect when using the i915 DRM driver
(I also assume others do the same). Given that the pageflip ioctl
isn't driver-specific, I think there cannot be such a difference in
behavior between drivers.
With the current behavior (pending pageflip events being delayed until
the CRTC is enabled again), compositors and other clients will be
holding on to the fb in the pending pageflip until an arbitrary point
in the future that may not ever come. To me that sounds like a serious
modification of the assumptions on fb lifecycle that might not be
warranted.
So in summary, even if I haven't found any explicit documentation on
this, I think the ABI is that any pending pageflips are to be
delivered when that CRTC is being disabled and not later.
Thanks,
Tomeu
> Thanks.
>
>
> yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
>
>
>
> --
> Ｍark Yao
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
On 2016年04月08日 18:54, Tomeu Vizoso wrote:
> On 8 April 2016 at 03:07, Mark yao <[email protected]> wrote:
>> On 2016年04月06日 18:14, Tomeu Vizoso wrote:
>>
>> When a plane is being disabled but it's still enabled, do check if the
>> previous update has been completed by reading yrgb_mst back.
>>
>> Otherwise, pending pageflips would remain pending after a CRTC is
>> disabled.
>>
>> Signed-off-by: Tomeu Vizoso <[email protected]>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index a9b1e8b5ac85..f46b1fd1887b 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct vop_win
>> *vop_win)
>> struct vop_plane_state *state = to_vop_plane_state(plane->state);
>> dma_addr_t yrgb_mst;
>>
>> - if (!state->enable)
>> - return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
>> + if (!state->enable &&
>> + VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0)
>> + return true;
>>
>>
>> It is wrong, the patch would cause a bug.
>>
>> when state->enable is 0, check yrgb_mst == state->yrgb_mst always be true,
>> because state->yrgb_mst not update on plane disabled funtion, that would
>> cause iommu crash.
> Sorry, but I don't understand where's the bug and what could cause
> that crash. What the existing code was doing is saying that a pageflip
> event is still pending if we have told the plane to disable but for
> some reason it hasn't yet.
>
> With this modification, if we read back that it's already disabled, we
> return true as before. But if we read back that it isn't disabled yet,
> then we still check the fb pointers and compare them.
>
> The iommu mapping is removed when the _CRTC_ is disabled, and what
> this series does is to wait for the pending pageflip to finish before
> conitnuing with CRTC disablement.
the iommu mapping will unmap after plane disabled, we need sure that the
plane really disabled before unmap, if not, the unmap may call before
plane really disable, vop may access unmap address, then would get iommu
page fault.
>> About pending pageflips would remain pending, can you describe more info
>> about it? I think those pending pageflips should be ignore when CRTC is
>> disabled.
> Well, right now in rockchip-drm pending pageflips won't be ignored
> when a CRTC is disabled, but will be delivered when it's re-enabled.
>
> If they would be to be ignored (understanding that as dropped), that
> would require modifications to clients so they keep track of which fbs
> were used in a particular crtc and destroy them when the crtc is
> disabled, but that would be incorrect when using the i915 DRM driver
> (I also assume others do the same). Given that the pageflip ioctl
> isn't driver-specific, I think there cannot be such a difference in
> behavior between drivers.
>
> With the current behavior (pending pageflip events being delayed until
> the CRTC is enabled again), compositors and other clients will be
> holding on to the fb in the pending pageflip until an arbitrary point
> in the future that may not ever come. To me that sounds like a serious
> modification of the assumptions on fb lifecycle that might not be
> warranted.
>
> So in summary, even if I haven't found any explicit documentation on
> this, I think the ABI is that any pending pageflips are to be
> delivered when that CRTC is being disabled and not later.
on drivers/gpu/drm/rockchip/rockchip_drm_fb.c
drm_atomic_helper_commit_planes(dev, state, true);
rockchip_atomic_wait_for_complete(dev, state);
We set active_only = true, I think planes can only update when crtc is
active. and rockchip_atomic_wait_for_complete only wait when crtc is active.
Thanks.
> Thanks,
>
> Tomeu
>
>> Thanks.
>>
>>
>> yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
>>
>>
>>
>> --
>> Ｍark Yao
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
>
--
Ｍark Yao
On 11 April 2016 at 03:15, Mark yao <[email protected]> wrote:
> On 2016年04月08日 18:54, Tomeu Vizoso wrote:
>>
>> On 8 April 2016 at 03:07, Mark yao <[email protected]> wrote:
>>>
>>> On 2016年04月06日 18:14, Tomeu Vizoso wrote:
>>>
>>> When a plane is being disabled but it's still enabled, do check if the
>>> previous update has been completed by reading yrgb_mst back.
>>>
>>> Otherwise, pending pageflips would remain pending after a CRTC is
>>> disabled.
>>>
>>> Signed-off-by: Tomeu Vizoso <[email protected]>
>>> ---
>>> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index a9b1e8b5ac85..f46b1fd1887b 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -1064,8 +1064,9 @@ static bool vop_win_pending_is_complete(struct
>>> vop_win
>>> *vop_win)
>>> struct vop_plane_state *state = to_vop_plane_state(plane->state);
>>> dma_addr_t yrgb_mst;
>>>
>>> - if (!state->enable)
>>> - return VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0;
>>> + if (!state->enable &&
>>> + VOP_WIN_GET(vop_win->vop, vop_win->data, enable) == 0)
>>> + return true;
>>>
>>>
>>> It is wrong, the patch would cause a bug.
>>>
>>> when state->enable is 0, check yrgb_mst == state->yrgb_mst always be
>>> true,
>>> because state->yrgb_mst not update on plane disabled funtion, that would
>>> cause iommu crash.
>>
>> Sorry, but I don't understand where's the bug and what could cause
>> that crash. What the existing code was doing is saying that a pageflip
>> event is still pending if we have told the plane to disable but for
>> some reason it hasn't yet.
>>
>> With this modification, if we read back that it's already disabled, we
>> return true as before. But if we read back that it isn't disabled yet,
>> then we still check the fb pointers and compare them.
>>
>> The iommu mapping is removed when the _CRTC_ is disabled, and what
>> this series does is to wait for the pending pageflip to finish before
>> conitnuing with CRTC disablement.
>
>
> the iommu mapping will unmap after plane disabled, we need sure that the
> plane really disabled before unmap, if not, the unmap may call before plane
> really disable, vop may access unmap address, then would get iommu page
> fault.
Sorry, but I still don't see the error condition that you are
describing. AFAICS, the unmap will happen after the last pageflip has
completed.
>>> About pending pageflips would remain pending, can you describe more info
>>> about it? I think those pending pageflips should be ignore when CRTC is
>>> disabled.
>>
>> Well, right now in rockchip-drm pending pageflips won't be ignored
>> when a CRTC is disabled, but will be delivered when it's re-enabled.
>>
>> If they would be to be ignored (understanding that as dropped), that
>> would require modifications to clients so they keep track of which fbs
>> were used in a particular crtc and destroy them when the crtc is
>> disabled, but that would be incorrect when using the i915 DRM driver
>> (I also assume others do the same). Given that the pageflip ioctl
>> isn't driver-specific, I think there cannot be such a difference in
>> behavior between drivers.
>>
>> With the current behavior (pending pageflip events being delayed until
>> the CRTC is enabled again), compositors and other clients will be
>> holding on to the fb in the pending pageflip until an arbitrary point
>> in the future that may not ever come. To me that sounds like a serious
>> modification of the assumptions on fb lifecycle that might not be
>> warranted.
>>
>> So in summary, even if I haven't found any explicit documentation on
>> this, I think the ABI is that any pending pageflips are to be
>> delivered when that CRTC is being disabled and not later.
>
>
> on drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>
> drm_atomic_helper_commit_planes(dev, state, true);
> rockchip_atomic_wait_for_complete(dev, state);
>
> We set active_only = true, I think planes can only update when crtc is
> active. and rockchip_atomic_wait_for_complete only wait when crtc is active.
That's fine, but if a pageflip is pending when the client requests the
CRTC to be disabled, we need to wait for the event to be emitted
before we actually disable the HW.
Regards,
Tomeu
> Thanks.
>
>> Thanks,
>>
>> Tomeu
>>
>>> Thanks.
>>>
>>>
>>> yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
>>>
>>>
>>>
>>> --
>>> Ｍark Yao
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> [email protected]
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>>
>
>
> --
> Ｍark Yao
>
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel