| Message ID | 1459937686-9142-1-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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. > 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. 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 >
On 2016?04?08? 18:54, Tomeu Vizoso wrote: > On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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. >> 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. 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 >> > >
On 11 April 2016 at 03:15, Mark yao <mark.yao@rock-chips.com> wrote: > On 2016?04?08? 18:54, Tomeu Vizoso wrote: >> >> On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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. 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
On 20 April 2016 at 16:23, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > On 11 April 2016 at 03:15, Mark yao <mark.yao@rock-chips.com> wrote: >> On 2016?04?08? 18:54, Tomeu Vizoso wrote: >>> >>> On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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? 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
On 2016年05月05日 17:34, Tomeu Vizoso wrote: > On 20 April 2016 at 16:23, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> On 11 April 2016 at 03:15, Mark yao <mark.yao@rock-chips.com> wrote: >>> On 2016年04月08日 18:54, Tomeu Vizoso wrote: >>>> On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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@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 > >
On 23 May 2016 at 08:32, Mark yao <mark.yao@rock-chips.com> wrote: > On 2016年05月05日 17:34, Tomeu Vizoso wrote: >> >> On 20 April 2016 at 16:23, Tomeu Vizoso <tomeu.vizoso@collabora.com> >> wrote: >>> >>> On 11 April 2016 at 03:15, Mark yao <mark.yao@rock-chips.com> wrote: >>>> >>>> On 2016年04月08日 18:54, Tomeu Vizoso wrote: >>>>> >>>>> On 8 April 2016 at 03:07, Mark yao <mark.yao@rock-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@collabora.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. Ok, maybe I need to ask you first what the original block of code intended to achieve. The TRM I have is very terse and I don't find any explanation there. The battery of tests I have pass just fine without it. > Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for > pending events when disabling a CRTC" Yes, this function is currently returning false when the pageflip has been completed but the plan has been already disabled. If you have any different idea of how to fix this bug, please share. Thanks, Tomeu
On 25 May 2016 at 03:33, Mark yao <mark.yao@rock-chips.com> wrote: > On 2016年05月25日 09:06, Mark yao wrote: > > On 2016年05月24日 18:11, Tomeu Vizoso wrote: > > 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. > > Ok, maybe I need to ask you first what the original block of code > intended to achieve. The TRM I have is very terse and I don't find any > explanation there. The battery of tests I have pass just fine without > it. > >> Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for >> pending events when disabling a CRTC" > > Yes, this function is currently returning false when the pageflip has > been completed but the plan has been already disabled. > > If you have any different idea of how to fix this bug, please share. > > Thanks, > > Tomeu > > > > Hi Tomeu > > @@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc) > if (!vop->is_enabled) > return; > > + if (crtc->state->event || vop->event) > + vop_crtc_wait_for_update(crtc); > + > > I think above change has some problem, > > the function stack: > ->drm swap state > ->vop_crtc_disable > ->vop_atomic_begin > ->vop_atomic_flush > > on vop_crtc_disable, crtc->state is new state, the crtc->state->event not > yet update to vop, wait for crtc->state->event here is wrong. > > So I think the patch should be: > + if (vop->event) > + vop_crtc_wait_for_update(crtc); > + > > > call vop_crtc_wait_for_update(crtc) here also is unsafe, it will reinit the > vop->wait_update_complete. > > I doubt that, since use the serialize outstanding nonblocking commits, only > one process can run into the update stack, old vop->event should be free on > last time, if we get vop->event here, that should be a bug. > > > Then the patch "drm/rockchip: vop: Do check if an update is pending during > disable" should be no needed. Hi Mark, with Daniel's series linked below this and the other issues I found in the Rockchip driver are fixed: http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/91023/focus=91053 Thanks, Tomeu > Thanks. > > -- Ｍ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 >
On 2016年06月02日 13:57, Tomeu Vizoso wrote: > On 25 May 2016 at 03:33, Mark yao <mark.yao@rock-chips.com> wrote: >> On 2016年05月25日 09:06, Mark yao wrote: >> >> On 2016年05月24日 18:11, Tomeu Vizoso wrote: >> >> 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. >> Ok, maybe I need to ask you first what the original block of code >> intended to achieve. The TRM I have is very terse and I don't find any >> explanation there. The battery of tests I have pass just fine without >> it. >> >>> Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for >>> pending events when disabling a CRTC" >> Yes, this function is currently returning false when the pageflip has >> been completed but the plan has been already disabled. >> >> If you have any different idea of how to fix this bug, please share. >> >> Thanks, >> >> Tomeu >> >> >> >> Hi Tomeu >> >> @@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc) >> if (!vop->is_enabled) >> return; >> >> + if (crtc->state->event || vop->event) >> + vop_crtc_wait_for_update(crtc); >> + >> >> I think above change has some problem, >> >> the function stack: >> ->drm swap state >> ->vop_crtc_disable >> ->vop_atomic_begin >> ->vop_atomic_flush >> >> on vop_crtc_disable, crtc->state is new state, the crtc->state->event not >> yet update to vop, wait for crtc->state->event here is wrong. >> >> So I think the patch should be: >> + if (vop->event) >> + vop_crtc_wait_for_update(crtc); >> + >> >> >> call vop_crtc_wait_for_update(crtc) here also is unsafe, it will reinit the >> vop->wait_update_complete. >> >> I doubt that, since use the serialize outstanding nonblocking commits, only >> one process can run into the update stack, old vop->event should be free on >> last time, if we get vop->event here, that should be a bug. >> >> >> Then the patch "drm/rockchip: vop: Do check if an update is pending during >> disable" should be no needed. > Hi Mark, > > with Daniel's series linked below this and the other issues I found in > the Rockchip driver are fixed: > > http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/91023/focus=91053 Good news, I also see the Daniel's series patches, wonderful update. You can add a Tested-by for Daniel's rockchip patches, and I add a Acked-by for those rockchip patches. Thanks > Thanks, > > Tomeu > >> Thanks. >> >> -- Ｍ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 >> > >
On 2 June 2016 at 08:25, Mark yao <mark.yao@rock-chips.com> wrote: > On 2016年06月02日 13:57, Tomeu Vizoso wrote: >> >> On 25 May 2016 at 03:33, Mark yao <mark.yao@rock-chips.com> wrote: >>> >>> On 2016年05月25日 09:06, Mark yao wrote: >>> >>> On 2016年05月24日 18:11, Tomeu Vizoso wrote: >>> >>> 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. >>> >>> Ok, maybe I need to ask you first what the original block of code >>> intended to achieve. The TRM I have is very terse and I don't find any >>> explanation there. The battery of tests I have pass just fine without >>> it. >>> >>>> Does this patch is needed for "[PATCH 2/2] drm/rockchip: vop: Wait for >>>> pending events when disabling a CRTC" >>> >>> Yes, this function is currently returning false when the pageflip has >>> been completed but the plan has been already disabled. >>> >>> If you have any different idea of how to fix this bug, please share. >>> >>> Thanks, >>> >>> Tomeu >>> >>> >>> >>> Hi Tomeu >>> >>> @@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc) >>> if (!vop->is_enabled) >>> return; >>> >>> + if (crtc->state->event || vop->event) >>> + vop_crtc_wait_for_update(crtc); >>> + >>> >>> I think above change has some problem, >>> >>> the function stack: >>> ->drm swap state >>> ->vop_crtc_disable >>> ->vop_atomic_begin >>> ->vop_atomic_flush >>> >>> on vop_crtc_disable, crtc->state is new state, the crtc->state->event not >>> yet update to vop, wait for crtc->state->event here is wrong. >>> >>> So I think the patch should be: >>> + if (vop->event) >>> + vop_crtc_wait_for_update(crtc); >>> + >>> >>> >>> call vop_crtc_wait_for_update(crtc) here also is unsafe, it will reinit >>> the >>> vop->wait_update_complete. >>> >>> I doubt that, since use the serialize outstanding nonblocking commits, >>> only >>> one process can run into the update stack, old vop->event should be free >>> on >>> last time, if we get vop->event here, that should be a bug. >>> >>> >>> Then the patch "drm/rockchip: vop: Do check if an update is pending >>> during >>> disable" should be no needed. >> >> Hi Mark, >> >> with Daniel's series linked below this and the other issues I found in >> the Rockchip driver are fixed: >> >> >> http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/91023/focus=91053 > > > Good news, I also see the Daniel's series patches, wonderful update. > > You can add a Tested-by for Daniel's rockchip patches, and I add a Acked-by > for those rockchip patches. Yup, should be already there. Regards, Tomeu > Thanks > > >> Thanks, >> >> Tomeu >> >>> Thanks. >>> >>> -- Ｍ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 > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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; yrgb_mst = VOP_WIN_GET_YRGBADDR(vop_win->vop, vop_win->data);
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@collabora.com> --- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)