On Wed, 20 Dec 2017 12:26:34 +0000 Daniel Stone <dani...@collabora.com> wrote:
> Add support for using the atomic-modesetting API to apply output state.
> Unlike previous series, this commit does not unflip sprites_are_broken,
> until further work has been done with assign_planes to make it reliable.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> Co-authored-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> Co-authored-by: Louis-Francis Ratté-Boulianne
> <louis-francis.ratte-boulia...@collabora.com>
> Co-authored-by: Derek Foreman <derek.fore...@collabora.co.uk>
> ---
> configure.ac | 2 +-
> libweston/compositor-drm.c | 504
> ++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 431 insertions(+), 75 deletions(-)
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index cb8f00e95..d5955c9ca 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> +static int
> +drm_output_apply_state_atomic(struct drm_output_state *state,
> + drmModeAtomicReq *req,
> + uint32_t *flags)
> +{
> + struct drm_output *output = state->output;
> + struct drm_backend *backend = to_drm_backend(output->base.compositor);
> + struct drm_plane_state *plane_state;
> + struct drm_mode *current_mode = to_drm_mode(output->base.current_mode);
> + int ret = 0;
> +
> + if (state->dpms != output->state_cur->dpms)
> + *flags |= DRM_MODE_ATOMIC_ALLOW_MODESET;
> +
> + if (state->dpms == WESTON_DPMS_ON) {
> + ret = drm_mode_ensure_blob(backend, current_mode);
> + if (ret != 0)
> + goto err;
> +
> + ret |= crtc_add_prop(req, output, WDRM_CRTC_MODE_ID,
> + current_mode->blob_id);
> + ret |= crtc_add_prop(req, output, WDRM_CRTC_ACTIVE, 1);
> + ret |= connector_add_prop(req, output, WDRM_CONNECTOR_CRTC_ID,
> + output->crtc_id);
> + } else {
> + ret |= crtc_add_prop(req, output, WDRM_CRTC_MODE_ID, 0);
> + ret |= crtc_add_prop(req, output, WDRM_CRTC_ACTIVE, 0);
> + ret |= connector_add_prop(req, output, WDRM_CONNECTOR_CRTC_ID,
> + 0);
> + }
> +
> + if (ret != 0) {
> + weston_log("couldn't set atomic CRTC/connector state\n");
> + goto err;
> + }
> +
> + wl_list_for_each(plane_state, &state->plane_list, link) {
> + struct drm_plane *plane = plane_state->plane;
> +
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_FB_ID,
> + plane_state->fb ? plane_state->fb->fb_id
> : 0);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_ID,
> + plane_state->fb ? output->crtc_id : 0);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_SRC_X,
> + plane_state->src_x);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_SRC_Y,
> + plane_state->src_y);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_SRC_W,
> + plane_state->src_w);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_SRC_H,
> + plane_state->src_h);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_X,
> + plane_state->dest_x);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_Y,
> + plane_state->dest_y);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_W,
> + plane_state->dest_w);
> + ret |= plane_add_prop(req, plane, WDRM_PLANE_CRTC_H,
> + plane_state->dest_h);
> +
> + if (ret != 0) {
> + weston_log("couldn't set plane state\n");
> + goto err;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + drm_output_state_free(state);
> + return -1;
> +}
> +
> +static int
> +drm_pending_state_apply_atomic(struct drm_pending_state *pending_state,
> + enum drm_state_apply_mode mode)
> +{
> + struct drm_backend *b = pending_state->backend;
> + struct drm_output_state *output_state, *tmp;
> + struct drm_plane *plane;
> + drmModeAtomicReq *req = drmModeAtomicAlloc();
> + uint32_t flags = 0;
> + int ret = 0;
> +
> + if (!req)
> + return -1;
> +
> + if (b->state_invalid) {
> + uint32_t *unused;
> + int err;
> +
> + /* If we need to reset all our state (e.g. because we've
> + * just started, or just been VT-switched in), explicitly
> + * disable all the CRTCs and connectors we aren't using. */
> + wl_array_for_each(unused, &b->unused_connectors) {
> + }
> +
> + wl_array_for_each(unused, &b->unused_crtcs) {
> + }
> +
> + /* Disable all the planes; planes which are being used will
> + * override this state in the output-state application. */
> + wl_list_for_each(plane, &b->plane_list, link) {
> + plane_add_prop(req, plane, WDRM_PLANE_CRTC_ID, 0);
> + plane_add_prop(req, plane, WDRM_PLANE_FB_ID, 0);
> + }
> +
> + flags |= DRM_MODE_ATOMIC_ALLOW_MODESET;
> + }
> +
> + wl_list_for_each(output_state, &pending_state->output_list, link) {
Hi,
this should probably be wl_list_for_each_safe(), because the error path
of drm_output_apply_state_atomic() will drm_output_state_free().
> + if (mode == DRM_STATE_APPLY_SYNC)
> + assert(output_state->dpms == WESTON_DPMS_OFF);
> + ret |= drm_output_apply_state_atomic(output_state, req, &flags);
> + }
> +
> + if (ret != 0) {
> + weston_log("atomic: couldn't compile atomic state\n");
> + goto out;
> + }
>
> +#ifdef HAVE_DRM_ATOMIC
> +static void
> +atomic_flip_handler(int fd, unsigned int frame, unsigned int sec,
> + unsigned int usec, unsigned int crtc_id, void *data)
> +{
> + struct drm_backend *b = data;
> + struct drm_output *output = drm_output_find_by_crtc(b, crtc_id);
> + uint32_t flags = WP_PRESENTATION_FEEDBACK_KIND_VSYNC |
> + WP_PRESENTATION_FEEDBACK_KIND_HW_COMPLETION |
> + WP_PRESENTATION_FEEDBACK_KIND_HW_CLOCK;
> +
> + /* During the initial modeset, we can disable CRTCs which we don't
> + * actually handle during normal operation; this will give us events
> + * for unknown outputs. Ignore them. */
> + if (!output)
> + return;
Did you consider that drm_output_find_by_crtc() will search also the
pending_output_list? Which means outputs we don't drive.
Maybe the check should be (!output || !output->base.enabled)?
> +
> + drm_output_update_msc(output, frame);
> +
> + assert(b->atomic_modeset);
> + assert(output->atomic_complete_pending);
I think the above would explode here at least, and maybe confuse also
elsewhere.
> + output->atomic_complete_pending = 0;
> +
> + drm_output_update_complete(output, flags, sec, usec);
> +}
> +#endif
> +
> static uint32_t
> drm_output_check_plane_format(struct drm_plane *p,
> struct weston_view *ev, struct gbm_bo *bo)
> @@ -2868,11 +3250,19 @@ drm_output_switch_mode(struct weston_output
> *output_base, struct weston_mode *mo
> static int
> on_drm_input(int fd, uint32_t mask, void *data)
> {
> +#ifdef HAVE_DRM_ATOMIC
> + struct drm_backend *b = data;
> +#endif
> drmEventContext evctx;
>
> memset(&evctx, 0, sizeof evctx);
> - evctx.version = 2;
> - evctx.page_flip_handler = page_flip_handler;
> + evctx.version = 3;
I believe this is effectively missing something like:
#ifdef HAVE_DRM_ATOMIC
evctx.version = 3;
#else
evctx.version = 2;
#endif
Otherwise, if Weston is built against libdrm without HAVE_DRM_ATOMIC,
then drmEventContext may not contain the field page_flip_handler2. If
libdrm is then upgraded without recompiling Weston, evctx.version=3
means libdrm will try to inspect page_flip_handler2 which has no memory
allocated and will get arbitrary stack data instead.
> +#ifdef HAVE_DRM_ATOMIC
> + if (b->atomic_modeset)
> + evctx.page_flip_handler2 = atomic_flip_handler;
> + else
> +#endif
> + evctx.page_flip_handler = page_flip_handler;
> evctx.vblank_handler = vblank_handler;
> drmHandleEvent(fd, &evctx);
>
Aside from the above and Philipp's comments, the patch looks ok.
Thanks,
pq
pgpnb3nD9_c_e.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
