From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753162AbcEWGdM (ORCPT ); Mon, 23 May 2016 02:33:12 -0400 Received: from regular1.263xmail.com ([211.150.99.139]:51470 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752975AbcEWGdL (ORCPT ); Mon, 23 May 2016 02:33:11 -0400 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 X-ADDR-CHECKED: 0 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: dri-devel@lists.freedesktop.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <3e9fdfc0771963593533940f7a170986> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH 1/2] drm/rockchip: vop: Do check if an update is pending during disable To: Tomeu Vizoso References: <1459937686-9142-1-git-send-email-tomeu.vizoso@collabora.com> <5707043D.6060706@rock-chips.com> <570AFAC2.8080907@rock-chips.com> Cc: "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" From: Mark yao Message-ID: <5742A41B.8030308@rock-chips.com> Date: Mon, 23 May 2016 14:32:59 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016年05月05日 17:34, Tomeu Vizoso wrote: > On 20 April 2016 at 16:23, Tomeu Vizoso wrote: >> On 11 April 2016 at 03:15, Mark yao wrote: >>> On 2016年04月08日 18:54, Tomeu Vizoso wrote: >>>> On 8 April 2016 at 03:07, Mark yao 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 >>>>> --- >>>>> 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@lists.freedesktop.org >>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >>>>> >>>> >>> >>> -- >>> Ｍark Yao >>> >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- Ｍark Yao