[PATCH -v2 10/14] drm/exynos: atomic phase 1: add atomic_begin()/atomic_flush()

[Inki Dae]

On 2015년 02월 10일 02:07, Gustavo Padovan wrote:
> 2015-02-09 Daniel Stone <daniel at fooishbar.org>:
> 
>> Hi Inki,
>>
>> On 9 February 2015 at 14:53, Inki Dae <inki.dae at samsung.com> wrote:
>>> I'm merging this patch series but I found two issues. One is already
>>> pointed out.
>>
>> Fantastic - thanks a lot for doing this!
>>
>>> On 2015년 02월 07일 04:37, Gustavo Padovan wrote:
>>>> @@ -628,20 +638,6 @@ static void fimd_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
>>>>               return;
>>>>       }
>>>>
>>>> -     /*
>>>> -      * SHADOWCON/PRTCON register is used for enabling timing.
>>>> -      *
>>>> -      * for example, once only width value of a register is set,
>>>> -      * if the dma is started then fimd hardware could malfunction so
>>>> -      * with protect window setting, the register fields with prefix '_F'
>>>> -      * wouldn't be updated at vsync also but updated once unprotect window
>>>> -      * is set.
>>>> -      */
>>>> -
>>>> -     /* protect windows */
>>>> -     fimd_shadow_protect_win(ctx, win, true);
>>>
>>> You removed to protect shadow register updating at vsync so fimd
>>> hardware could malfunction if setcrtc/page flip/setplane are requested
>>> by userspace. Actually, when I run modetest repeatedly, I could see
>>> overlay isn't rarely turned on.
>>>
>>> So how about calling win_protect/unprotect callbacks like below? If you
>>> are ok, I can modify it and merge it.
>>
>> I think you are missing some details about atomic and the helpers.
>>
>> The helpers wrap _all_ legacy codepaths, e.g. SetCrtc, SetPlane, etc.
>> All of these operations are intercepted by the code in
>> drm_atomic_helper.c / drm_plane_helper.c code, and the legacy
>> operations are turned into atomic operations. For the driver - there
>> are no legacy operations.

Yes, SetCrtc does this but not page flip and SetPlane. When I had page
flip and SetPlane test using modetest, win_protect/unprotect callbacks
are never called. In my opinion, they, page flip and SetPlane, also are
required for win_protect/unprotect when updating overlay registers.

In addition, I could find some issues.

With regard to SetPlane relevent codes, it seems considered for
win_protect/unprotect but actually, they are never called because when
atomic_check callback is called, plane->crtc is NULL which would be set
 at exynos_plane_mode_set function. However, exynos_plane_mode_set
function is called later than atomic_check callback.

One more thing, it seems your below patch makes possible_crtc of each
crtc driver have wrong value because this patch calls exynos_plane_init
function before fimd_ctx_initialize and vidi_ctx_initialize functions
are called.

 commit fba6fe129efda336f84cf05d61c152ee45d4d9b5
Author: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
Date:   Fri Feb 6 17:37:46 2015 -0200

    drm/exynos: remove struct *_win_data abstraction on planes

I wanted to merge this patch series this time but it seems that we have
to review more them. Unfortunately, now merge window and we have no much
time to review them. So how about merging this patch series next time
after reviews enough? Your patch series is great and thanks for your
contribution. But as of now, I'm not sure that this patch series is safe
and would work well.

Thanks,
Inki Dae

>>
>> The atomic helpers guarantee that atomic_begin() will be called before
>> the state is committed, and atomic_flush() will be called after the
>> state is committed. Thus this change is completely safe.
> 
> Exactly, when phase 3 is merged you won't see any of these issues. I have
> patches ready for most part of phase 3. Expect them soon in the mailing list.
> 
> 	Gustavo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>