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

[Mark yao <mark.yao@rock-chips.com>]

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
>>
>
>


-- 
Ｍark Yao