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

[Tomeu Vizoso <tomeu.vizoso@collabora.com>]

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
>