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