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

[Tomeu Vizoso]

On 2 June 2016 at 08:25, Mark yao <mark.yao@xxxxxxxxxxxxxx> wrote:
> 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
>>>> 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@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>>
>>> --
>>> ïark Yao
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>
>>
>
>
> --
> ïark Yao
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel