Message-ID: <5742A41B.8030308@rock-chips.com>
Date:	Mon, 23 May 2016 14:32:59 +0800
From:	Mark yao <mark.yao@...k-chips.com>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending
 during disable

On 2016年05月05日 17:34, Tomeu Vizoso wrote:
> On 20 April 2016 at 16:23, Tomeu Vizoso <tomeu.vizoso@...labora.com> wrote:
>> On 11 April 2016 at 03:15, Mark yao <mark.yao@...k-chips.com> wrote:
>>> On 2016年04月08日 18:54, Tomeu Vizoso wrote:
>>>> On 8 April 2016 at 03:07, Mark yao <mark.yao@...k-chips.com> 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@...labora.com>
>>>>> ---
>>>>>    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?
Hi Tomeu

Sorry for reply late.
I don't agree the changes:

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

This changes actually would lead a bug.
when we disable a plane, the vop_win_pending_is_complete would always 
return true, That is not allowed, would cause fb free too early.

Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for 
pending events when disabling a CRTC"

Thanks.

> 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@...ts.freedesktop.org
>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>
>>>>
>>>
>>> --
>>> Ｍark Yao
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@...ts.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>


-- 
Ｍark Yao

Powered by blists - more mailing lists