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

[Daniel Stone]

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.

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.

> This warning would mean a fb object leak and while I have a review, I
> couldn't find why it causes the leak.

I'm not really surprised; the fb refcounting in Exynos has long had
quite a few issues. The refcounting is incredibly difficult to get
right, and whilst atomic changes the order of operations a little, it
also makes the semantics of fb/vblank refcounting much more
consistent, so it should be much easier to fix than usual.

> However, I'd like to merge this patch series this time and fix this
> issue at RC for phase 3.

Great, thankyou! Really very happy about this. :)

Cheers,
Daniel