2015-02-10 Inki Dae <inki.dae at samsung.com>:
> 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.
Once atomic phase 3 all operations will call win_protect/win_unprotect. I'll
send the next version of this patchset with the atomic phase 3 patches
included.
>
> 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.
Sure, I can fix this. Thanks for your review.
Gustavo
