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