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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Thanks.

-- Mark Yao

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Mark Yao
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux