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

[Gustavo Padovan <gustavo@padovan.org>]

2015-02-10 Inki Dae <inki.dae@samsung.com>:

> On 2015년 02월 10일 02:07, Gustavo Padovan wrote:
> > 2015-02-09 Daniel Stone <daniel@fooishbar.org>:
> > 
> >> Hi Inki,
> >>
> >> On 9 February 2015 at 14:53, Inki Dae <inki.dae@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel