Re: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending during disable

[Tomeu Vizoso]

On 11 April 2016 at 03:15, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:
> On 2016å04æ08æ 18:54, Tomeu Vizoso wrote:
>>
>> On 8 April 2016 at 03:07, Mark yao <mark.yao@xxxxxxxxxxxxxx> 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 <tomeu.vizoso@xxxxxxxxxxxxx>
>>> ---
>>>   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
>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>>
>
>
> --
> ïark Yao
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel