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

From: Mark yao
Date: Thu Jun 02 2016 - 02:25:57 EST

On 2016å06æ02æ 13:57, Tomeu Vizoso wrote:
On 25 May 2016 at 03:33, Mark yao <mark.yao@xxxxxxxxxxxxxx> 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
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

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.



Hi Tomeu

@@ -504,6 +506,9 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
if (!vop->is_enabled)

+ 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

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

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:

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.





-- ïark Yao

dri-devel mailing list

ïark Yao

dri-devel mailing list

ïark Yao