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

[Tomeu Vizoso]

On 20 April 2016 at 16:23, Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx> wrote:
> 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.

Hi Mark,

could you tell me if you agree that there's a bug that needs to be
solved, and if so, what do you think we should do about it?

Thanks,

Tomeu

> 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