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

[Gustavo Padovan <gustavo@padovan.org>]

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