Re: [PATCH v19 05/10] compositor-drm: Add test-only mode to state application
Hi, On Wed, 11 Jul 2018 at 11:20, Pekka Paalanen wrote: > On Tue, 10 Jul 2018 18:58:44 +0100 Daniel Stone 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 ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v19 05/10] compositor-drm: Add test-only mode to state application
On Tue, 10 Jul 2018 18:58:44 +0100
Daniel Stone wrote:
> The atomic API can allow us to test state before we apply it, to see if
> it will be valid. Use this when we construct a plane configuration, to
> see if it has a chance of ever working. If not, we can fail
> assign_planes early.
>
> This will be used in later patches to incrementally build state by
> proposing and testing potential configurations one at a time.
>
> Signed-off-by: Daniel Stone
> Tested-by: Emre Ucan
> ---
> libweston/compositor-drm.c | 37 +
> 1 file changed, 37 insertions(+)
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 28a580cd2..f7a364837 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -260,6 +260,7 @@ enum drm_output_state_duplicate_mode {
> enum drm_state_apply_mode {
> DRM_STATE_APPLY_SYNC, /**< state fully processed */
> DRM_STATE_APPLY_ASYNC, /**< state pending event delivery */
> + DRM_STATE_TEST_ONLY, /**< test if the state can be applied */
> };
>
> struct drm_backend {
> @@ -1822,6 +1823,7 @@ drm_pending_state_get_output(struct drm_pending_state
> *pending_state,
> }
>
> static int drm_pending_state_apply_sync(struct drm_pending_state *state);
> +static int drm_pending_state_test(struct drm_pending_state *state);
>
> /**
> * Mark a drm_output_state (the output's last state) as complete. This
> handles
> @@ -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'.
> +
> if (ret != 0) {
> weston_log("atomic: couldn't commit new state: %m\n");
> goto out;
> @@ -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.
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.
Thanks,
pq
> +static int
> +drm_pending_state_test(struct drm_pending_state *pending_state)
> +{
> +#ifdef HAVE_DRM_ATOMIC
> + struct drm_backend *b = pending_state->backend;
> +
> + if (b->atomic_modeset)
> + return drm_pending_state_apply_atomic(pending_state,
> + DRM_STATE_TEST_ONLY);
> +#endif
> +
> + /* We have no way to test state before application on the legacy
> + * modesetting API, so just claim it succeeded. */
> + return 0;
> +}
> +
> static int
> drm_pending_state_apply(struct drm_pending_state *pending_state)
> {
> @@ -3271,6 +3296,7 @@ drm_output_propose_state(struct weston_output
> *output_base,
> struct weston_view *ev;
> pixman_region32_t surface_overlap, renderer_region, occluded_region;
> bool planes_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_RENDERER_ONLY);
> + int ret;
>
> assert(!output->state_last);
> state = drm_output_state_duplicate(output->state_cur,
> @@ -3388,7 +3414,18 @@ drm_output_propose_state(struct weston_output
> *output_base,
> pixman_region32_fini(&renderer_region);
> pixman_region32_fini(&occluded_region);
>
> + /* Check to see if this state will actually work. */
> + ret = drm_pending_state_test(state->pending_state);
> + if (ret != 0)
> + goto err;
> +
> return state;
> +
> +err:
> + pixman_region32_fini(&renderer_region);
> + pixman_region32_fini(&occluded_region);
> + drm_output_state_free(state);
> + return NULL;
> }
>
> static void
pgpgIPnHQRlkT.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH v19 05/10] compositor-drm: Add test-only mode to state application
On Tue, 10 Jul 2018 at 18:59, Daniel Stone wrote: > @@ -3388,7 +3414,18 @@ drm_output_propose_state(struct weston_output > *output_base, > pixman_region32_fini(&renderer_region); > pixman_region32_fini(&occluded_region); > > + /* Check to see if this state will actually work. */ > + ret = drm_pending_state_test(state->pending_state); > + if (ret != 0) > + goto err; > + > return state; > + > +err: > + pixman_region32_fini(&renderer_region); > + pixman_region32_fini(&occluded_region); > + drm_output_state_free(state); > + return NULL; > } Looking at this fresh, this err jump should jump to freeing the state and returning NULL, not finishing the regions. The region free is only needed by 'Add planes-only mode to state proposal', which can bail out inside the per-view loop. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH v19 05/10] compositor-drm: Add test-only mode to state application
The atomic API can allow us to test state before we apply it, to see if
it will be valid. Use this when we construct a plane configuration, to
see if it has a chance of ever working. If not, we can fail
assign_planes early.
This will be used in later patches to incrementally build state by
proposing and testing potential configurations one at a time.
Signed-off-by: Daniel Stone
Tested-by: Emre Ucan
---
libweston/compositor-drm.c | 37 +
1 file changed, 37 insertions(+)
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 28a580cd2..f7a364837 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -260,6 +260,7 @@ enum drm_output_state_duplicate_mode {
enum drm_state_apply_mode {
DRM_STATE_APPLY_SYNC, /**< state fully processed */
DRM_STATE_APPLY_ASYNC, /**< state pending event delivery */
+ DRM_STATE_TEST_ONLY, /**< test if the state can be applied */
};
struct drm_backend {
@@ -1822,6 +1823,7 @@ drm_pending_state_get_output(struct drm_pending_state
*pending_state,
}
static int drm_pending_state_apply_sync(struct drm_pending_state *state);
+static int drm_pending_state_test(struct drm_pending_state *state);
/**
* Mark a drm_output_state (the output's last state) as complete. This handles
@@ -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;
+
if (ret != 0) {
weston_log("atomic: couldn't commit new state: %m\n");
goto out;
@@ -2640,6 +2649,22 @@ out:
*
* Unconditionally takes ownership of pending_state, and clears state_invalid.
*/
+static int
+drm_pending_state_test(struct drm_pending_state *pending_state)
+{
+#ifdef HAVE_DRM_ATOMIC
+ struct drm_backend *b = pending_state->backend;
+
+ if (b->atomic_modeset)
+ return drm_pending_state_apply_atomic(pending_state,
+ DRM_STATE_TEST_ONLY);
+#endif
+
+ /* We have no way to test state before application on the legacy
+* modesetting API, so just claim it succeeded. */
+ return 0;
+}
+
static int
drm_pending_state_apply(struct drm_pending_state *pending_state)
{
@@ -3271,6 +3296,7 @@ drm_output_propose_state(struct weston_output
*output_base,
struct weston_view *ev;
pixman_region32_t surface_overlap, renderer_region, occluded_region;
bool planes_ok = (mode != DRM_OUTPUT_PROPOSE_STATE_RENDERER_ONLY);
+ int ret;
assert(!output->state_last);
state = drm_output_state_duplicate(output->state_cur,
@@ -3388,7 +3414,18 @@ drm_output_propose_state(struct weston_output
*output_base,
pixman_region32_fini(&renderer_region);
pixman_region32_fini(&occluded_region);
+ /* Check to see if this state will actually work. */
+ ret = drm_pending_state_test(state->pending_state);
+ if (ret != 0)
+ goto err;
+
return state;
+
+err:
+ pixman_region32_fini(&renderer_region);
+ pixman_region32_fini(&occluded_region);
+ drm_output_state_free(state);
+ return NULL;
}
static void
--
2.17.1
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
