[PATCH v19 05/10] compositor-drm: Add test-only mode to state application

[Daniel Stone]

Hi,
On Wed, 11 Jul 2018 at 11:20, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 10 Jul 2018 18:58:44 +0100 Daniel Stone <daniels at collabora.com> wrote:
> > @@ -2610,9 +2612,16 @@ drm_pending_state_apply_atomic(struct drm_pending_state *pending_state,
> >       case DRM_STATE_APPLY_ASYNC:
> >               flags |= DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_ATOMIC_NONBLOCK;
> >               break;
> > +     case DRM_STATE_TEST_ONLY:
> > +             flags |= DRM_MODE_ATOMIC_TEST_ONLY;
> > +             break;
> >       }
> >
> >       ret = drmModeAtomicCommit(b->drm.fd, req, flags, b);
> > +
> > +     if (mode == DRM_STATE_TEST_ONLY)
> > +             return ret;
>
> This leaks at least 'req'.

Right, I found that before re-sending for v20 and fixed locally.

> > @@ -2640,6 +2649,22 @@ out:
> >   *
> >   * Unconditionally takes ownership of pending_state, and clears state_invalid.
> >   */
>
> The documentation above is misplaced now. drm_pending_state_test()
> needs new docs, particularly about what it is supposed to do with
> pending_state.

Done.

> An idea for the future: maybe 'req' could be cached with the
> pending_state if it passed the test? To avoid having to build the same
> working 'req' twice every frame.

I did think about it but decided it probably wasn't particularly
valuable. drmModeAtomicReq is just a dumb container:
drmModeAtomicAddProperty adds (obj,prop,value) to a list, increments
the serial, and returns. The 'expensive' work is done in
drmModeAtomicCommit, which collates that list into a set of final
values before submission to the kernel. This is done so that an atomic
request can have a list of properties which potentially overwrite each
other, but can then be 'rewound' with drmModeAtomicSetCursor in order
to do non-destructive testing. It also complicates our tracking
somewhat, since we'd need to track the dirty status of a pending_state
everywhere we modified its child states.

Cheers,
Daniel