[RFC weston 14/14] compositor-drm: Atomic modesetting support
Pekka Paalanen
ppaalanen at gmail.com
Thu May 21 08:31:15 PDT 2015
On Thu, 21 May 2015 08:29:11 +0100
Daniel Stone <daniels at collabora.com> wrote:
> Building on the drm_plane work from earlier, use the new atomic
> modesetting interface where available, including enabling overlay planes
> by default.
>
> It is still highly experimental, but manages to at least prove the
> overall atomic modesetting and blob property interfaces.
>
> This patch was jointly authored by Pekka Paalanen, Louis-Francis
> Ratte-Boulianne, and myself.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Signed-off-by: Louis-Francis Ratte-Boulianne <lfrb at collabora.com>
> Co-authored-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> Co-authored-by: Louis-Francis Ratte-Boulianne <lfrb at collabora.com>
> ---
> src/compositor-drm.c | 735 +++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 710 insertions(+), 25 deletions(-)
Hi,
naturally this patch should bump the libdrm requirement in
configure.ac, but since there is not a libdrm release with these bits
yet...
But we still could merge this early with suitable #ifdefs.
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index bbc65ce..3922b54 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -52,6 +52,10 @@
> #include "vaapi-recorder.h"
> #include "presentation_timing-server-protocol.h"
>
> +#ifndef static_assert
> +#define static_assert(cond, msg) assert((cond) && msg)
> +#endif
> +
> #ifndef DRM_CAP_TIMESTAMP_MONOTONIC
> #define DRM_CAP_TIMESTAMP_MONOTONIC 0x6
> #endif
> @@ -60,6 +64,14 @@
> #define DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
> #endif
>
> +#ifndef DRM_CLIENT_CAP_ATOMIC
> +#define DRM_CLIENT_CAP_ATOMIC 0xa
> +#endif
> +
> +#ifndef DRM_CLIENT_CAP_ATOMIC
> +#define DRM_CLIENT_CAP_ATOMIC 3
> +#endif
Hmm, so which one is the right one? :-)
> +
> #ifndef DRM_CAP_CURSOR_WIDTH
> #define DRM_CAP_CURSOR_WIDTH 0x8
> #endif
> @@ -72,6 +84,9 @@
> #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
> #endif
>
> +/* XXX: For now, you probably want this enabled. */
> +#define ATOMIC_DEBUG 1
> +
> static int option_current_mode = 0;
>
> enum output_config {
> @@ -88,6 +103,16 @@ enum output_config {
> */
> enum wdrm_plane_property {
> WDRM_PLANE_TYPE = 0,
> + WDRM_PLANE_SRC_X,
> + WDRM_PLANE_SRC_Y,
> + WDRM_PLANE_SRC_W,
> + WDRM_PLANE_SRC_H,
> + WDRM_PLANE_CRTC_X,
> + WDRM_PLANE_CRTC_Y,
> + WDRM_PLANE_CRTC_W,
> + WDRM_PLANE_CRTC_H,
> + WDRM_PLANE_FB_ID,
> + WDRM_PLANE_CRTC_ID,
> WDRM_PLANE__COUNT
> };
>
> @@ -102,6 +127,23 @@ enum wdrm_plane_type {
> };
>
> /**
> + * List of properties attached to DRM CRTCs
> + */
> +enum wdrm_crtc_property {
> + WDRM_CRTC_MODE_ID = 0,
> + WDRM_CRTC_ACTIVE,
> + WDRM_CRTC__COUNT
> +};
> +
> +/**
> + * List of properties attached to DRM connectors
> + */
> +enum wdrm_connector_property {
> + WDRM_CONNECTOR_CRTC_ID = 0,
> + WDRM_CONNECTOR__COUNT
> +};
> +
> +/**
> * One property attached to a DRM mode object (plane, CRTC, connector).
> */
> struct property_item {
> @@ -117,6 +159,25 @@ struct plane_properties {
> struct property_item item[WDRM_PLANE__COUNT];
> uint64_t typemap[WDRM_PLANE_TYPE__COUNT]; /**< map for type enum */
> uint32_t value_valid_mask;
> + uint32_t value_pend_mask;
> +};
> +
> +/**
> + * Holding structure for CRTC properties.
> + */
> +struct crtc_properties {
> + struct property_item item[WDRM_CRTC__COUNT];
> + uint32_t value_valid_mask;
> + uint32_t value_pend_mask;
> +};
> +
> +/**
> + * Holding structure for CRTC properties.
ITYM connector properties.
> + */
> +struct connector_properties {
> + struct property_item item[WDRM_CONNECTOR__COUNT];
> + uint32_t value_valid_mask;
> + uint32_t value_pend_mask;
> };
>
> struct drm_compositor {
> @@ -156,6 +217,7 @@ struct drm_compositor {
> int cursors_are_broken;
>
> bool universal_planes;
> + bool atomic_modeset;
>
> int use_pixman;
>
> @@ -170,6 +232,7 @@ struct drm_compositor {
> struct drm_mode {
> struct weston_mode base;
> drmModeModeInfo mode_info;
> + uint32_t blob_id;
> };
>
> struct drm_output;
> @@ -232,6 +295,8 @@ struct drm_plane {
> uint32_t dest_x, dest_y;
> uint32_t dest_w, dest_h;
>
> + struct wl_list flip_link; /* drm_output::plane_flip_list */
> +
> uint32_t formats[];
> };
>
> @@ -246,6 +311,9 @@ struct drm_output {
> drmModePropertyPtr dpms_prop;
> uint32_t format;
>
> + struct crtc_properties props_crtc;
> + struct connector_properties props_conn;
> +
> int vblank_pending;
> int page_flip_pending;
> int destroy_pending;
> @@ -258,7 +326,6 @@ struct drm_output {
> struct drm_plane *primary_plane;
> struct weston_view *cursor_view;
> int current_cursor;
> - struct drm_fb *current, *next;
> struct backlight *backlight;
>
> struct drm_fb *dumb[2];
> @@ -268,6 +335,8 @@ struct drm_output {
>
> struct vaapi_recorder *recorder;
> struct wl_listener recorder_frame_listener;
> +
> + struct wl_list plane_flip_list; /* drm_plane::flip_link */
> };
>
> struct drm_parameters {
> @@ -509,6 +578,20 @@ drm_plane_property_get(struct drm_plane *plane, enum wdrm_plane_property prop)
> return plane->props.item[prop].value;
> }
>
> +static enum wdrm_plane_type
> +drm_plane_get_type(struct drm_plane *plane)
> +{
> + uint64_t drm_type = drm_plane_property_get(plane, WDRM_PLANE_TYPE);
> + int i;
> +
> + for (i = 0; i < WDRM_PLANE_TYPE__COUNT; i++) {
> + if (plane->props.typemap[i] == drm_type)
> + return i;
> + }
> +
Would probably want to yell about unknown types?
> + return WDRM_PLANE_TYPE_OVERLAY;
> +}
> +
> /**
> * Initialise DRM properties for planes
> *
> @@ -522,11 +605,21 @@ plane_properties_init(struct drm_plane *plane)
> {
> static const char * const plane_property_names[] = {
> [WDRM_PLANE_TYPE] = "type",
> + [WDRM_PLANE_SRC_X] = "SRC_X",
> + [WDRM_PLANE_SRC_Y] = "SRC_Y",
> + [WDRM_PLANE_SRC_W] = "SRC_W",
> + [WDRM_PLANE_SRC_H] = "SRC_H",
> + [WDRM_PLANE_CRTC_X] = "CRTC_X",
> + [WDRM_PLANE_CRTC_Y] = "CRTC_Y",
> + [WDRM_PLANE_CRTC_W] = "CRTC_W",
> + [WDRM_PLANE_CRTC_H] = "CRTC_H",
> + [WDRM_PLANE_FB_ID] = "FB_ID",
> + [WDRM_PLANE_CRTC_ID] = "CRTC_ID",
> };
> static const char * const plane_type_names[] = {
> [WDRM_PLANE_TYPE_PRIMARY] = "Primary",
> - [WDRM_PLANE_TYPE_OVERLAY] = "Overlay",
> [WDRM_PLANE_TYPE_CURSOR] = "Cursor",
> + [WDRM_PLANE_TYPE_OVERLAY] = "Overlay",
> };
> uint32_t type_mask;
> uint32_t required_mask;
> @@ -549,6 +642,13 @@ plane_properties_init(struct drm_plane *plane)
> required_mask = 0;
> if (plane->compositor->universal_planes)
> required_mask |= 1 << WDRM_PLANE_TYPE;
> + if (plane->compositor->atomic_modeset)
> + required_mask |=
> + (1 << WDRM_PLANE_SRC_X) | (1 << WDRM_PLANE_SRC_Y) | \
> + (1 << WDRM_PLANE_SRC_W) | (1 << WDRM_PLANE_SRC_H) | \
> + (1 << WDRM_PLANE_CRTC_X) | (1 << WDRM_PLANE_CRTC_Y) | \
> + (1 << WDRM_PLANE_CRTC_W) | (1 << WDRM_PLANE_CRTC_H) | \
> + (1 << WDRM_PLANE_FB_ID) | (1 << WDRM_PLANE_CRTC_ID);
>
> if (plane->props.value_valid_mask != required_mask) {
> weston_log("DRM error: failed to look up all plane properties "
> @@ -594,6 +694,274 @@ static void plane_properties_release(struct drm_plane *plane)
> plane->props.value_valid_mask = 0;
> }
>
> +/**
> + * Initialise DRM properties for CRTC and connector
> + *
> + * Set up the holding structures to track DRM object properties set on the CRTC
> + * and connector associated with an output.
> + * Free the memory allocated here with output_properties_release.
> + *
> + * @param output Output to configure
> + */
> +static bool output_properties_init(struct drm_output *output)
> +{
> + struct drm_compositor *ec =
> + (struct drm_compositor *) output->base.compositor;
> + static const char * const crtc_property_names[] = {
> + [WDRM_CRTC_MODE_ID] = "MODE_ID",
> + [WDRM_CRTC_ACTIVE] = "ACTIVE",
> + };
> + static const char * const connector_property_names[] = {
> + [WDRM_CONNECTOR_CRTC_ID] = "CRTC_ID",
> + };
> + uint32_t required_mask;
> +
> + static_assert(ARRAY_LENGTH(crtc_property_names) == WDRM_CRTC__COUNT,
> + "crtc_property_names mismatch with the enum");
> + static_assert(WDRM_CRTC__COUNT <= 32,
> + "need more bits for crtc item_valid_mask");
> +
> + static_assert(ARRAY_LENGTH(connector_property_names) == WDRM_CONNECTOR__COUNT,
> + "connector_property_names mismatch with the enum");
> + static_assert(WDRM_CONNECTOR__COUNT <= 32,
> + "need more bits for connector item_valid_mask");
Should do a s/item_valid_mask/value_valid_mask and value_pend_mask/
everwhere...
> +
> + output->props_crtc.value_valid_mask =
> + drm_properties_get_from_obj(ec,
> + output->props_crtc.item,
> + crtc_property_names,
> + WDRM_CRTC__COUNT,
> + output->crtc_id,
> + DRM_MODE_OBJECT_CRTC);
> +
> + required_mask = 0;
> + if (ec->atomic_modeset)
> + required_mask |= (1 << WDRM_CRTC_MODE_ID) | \
> + (1 << WDRM_CRTC_ACTIVE);
> + if (output->props_crtc.value_valid_mask != required_mask) {
Should we accept extra properties we might detect in the future?
> + weston_log("DRM error: failed to look up all CRTC properties "
> + "(wanted 0x%x got 0x%x) on ID %d\n",
> + required_mask, output->props_crtc.value_valid_mask,
> + output->crtc_id);
> + return false;
> + }
> +
> + output->props_conn.value_valid_mask =
> + drm_properties_get_from_obj(ec,
> + output->props_conn.item,
> + connector_property_names,
> + WDRM_CONNECTOR__COUNT,
> + output->connector_id,
> + DRM_MODE_OBJECT_CONNECTOR);
> + required_mask = 0;
> + if (ec->atomic_modeset)
> + required_mask |= (1 << WDRM_CONNECTOR_CRTC_ID);
> + if (output->props_conn.value_valid_mask != required_mask) {
> + weston_log("DRM error: failed to look up all connector properties "
> + "(wanted 0x%x got 0x%x) on ID %d\n",
> + required_mask, output->props_conn.value_valid_mask,
> + output->connector_id);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +/**
> + * Free DRM CRTC and connector properties
> + *
> + * The counterpart to output_properties_init.
> + *
> + * @param output Output to release properties for
> + */
> +static void output_properties_release(struct drm_output *output)
> +{
> + property_item_array_release(output->props_crtc.item, WDRM_CRTC__COUNT);
> + output->props_crtc.value_valid_mask = 0;
> + property_item_array_release(output->props_conn.item,
> + WDRM_CONNECTOR__COUNT);
> + output->props_conn.value_valid_mask = 0;
> +}
> +
> +static void
> +property_debug(const char *klass, uint32_t obj_id,
> + struct property_item *item, unsigned len,
> + unsigned en, uint64_t value)
> +{
> +#ifdef ATOMIC_DEBUG
> + uint32_t prop_id = len;
> + const char *prop_name = "<illegal enum>";
> +
> + if (en < len) {
> + prop_id = item[en].id;
> + if (item[en].drm_prop)
> + prop_name = item[en].drm_prop->name;
> + else
> + prop_name = "<no property>";
> + }
> +
> + weston_log("DRM req %s id %u: prop[%u] %u '%s' = %" PRIu64 "\n",
> + klass, obj_id, en, prop_id, prop_name, value);
> +#endif
> +}
> +
> +static void
> +print_drm_mode_info(const drmModeModeInfo *m)
> +{
> +#ifdef ATOMIC_DEBUG
> + weston_log_continue("mode '%s', px %u MHz, refresh %u\n", m->name,
> + m->clock, m->vrefresh);
> + weston_log_continue("horz: %u %u %u %u %u\n", m->hdisplay,
> + m->hsync_start, m->hsync_end, m->htotal, m->hskew);
> + weston_log_continue("vert: %u %u %u %u %u\n", m->vdisplay,
> + m->vsync_start, m->vsync_end, m->vtotal, m->vscan);
> + weston_log_continue("flags %#08x, type %#08x\n", m->flags, m->type);
> +#endif /* ATOMIC_DEBUG */
> +}
> +
> +static void
> +plane_property_debug(struct drm_plane *plane,
> + enum wdrm_plane_property prop, uint64_t value)
> +{
> + property_debug("plane", plane->plane_id, plane->props.item,
> + WDRM_PLANE__COUNT, prop, value);
> +}
> +
> +static void
> +connector_property_debug(struct drm_output *output,
> + enum wdrm_connector_property prop, uint64_t value)
> +{
> + property_debug("connector", output->connector_id,
> + output->props_conn.item, WDRM_CONNECTOR__COUNT,
> + prop, value);
> +}
> +
> +static void
> +crtc_property_debug(struct drm_output *output,
> + enum wdrm_crtc_property prop, uint64_t value)
> +{
> + property_debug("CRTC", output->crtc_id, output->props_crtc.item,
> + WDRM_CRTC__COUNT, prop, value);
> +}
> +
> +static void
> +drm_plane_update_begin(struct drm_plane *plane)
> +{
> + plane->props.value_pend_mask = 0;
> +}
> +
> +static void
> +drm_plane_update_success(struct drm_plane *plane)
> +{
> + plane->props.value_valid_mask |= plane->props.value_pend_mask;
> +}
> +
> +static void
> +drm_crtc_update_begin(struct drm_output *output)
> +{
> + output->props_crtc.value_pend_mask = 0;
> +}
> +
> +static void
> +drm_crtc_update_success(struct drm_output *output)
> +{
> + output->props_crtc.value_valid_mask |= output->props_crtc.value_pend_mask;
> +}
> +
> +static void
> +drm_connector_update_begin(struct drm_output *output)
> +{
> + output->props_conn.value_pend_mask = 0;
> +}
> +
> +static void
> +drm_connector_update_success(struct drm_output *output)
> +{
> + output->props_conn.value_valid_mask |= output->props_conn.value_pend_mask;
> +}
> +
> +static int
> +atomic_plane_add(drmModeAtomicReq *req, struct drm_plane *plane,
> + enum wdrm_plane_property prop, uint64_t value)
> +{
> + struct property_item *item = &plane->props.item[prop];
> + uint32_t mask = 1U << prop;
> +
> + if (!item->id)
> + return -1;
> +
> + if ((plane->props.value_valid_mask |
> + plane->props.value_pend_mask) & mask &&
> + item->value == value)
> + return 0;
> +
> + plane_property_debug(plane, prop, value);
> +
> + if (drmModeAtomicAddProperty(req, plane->plane_id, item->id, value) < 0)
> + return -1;
> +
> + plane->props.value_valid_mask &= ~mask;
> + item->value = value;
> + plane->props.value_pend_mask |= mask;
> +
> + return 0;
> +}
> +
> +static int
> +atomic_connector_add(drmModeAtomicReq *req, struct drm_output *output,
> + enum wdrm_connector_property prop, uint64_t value)
> +{
> + struct property_item *item = &output->props_conn.item[prop];
> + uint32_t mask = 1U << prop;
> +
> + if (!item->id)
> + return -1;
> +
> + if ((output->props_conn.value_valid_mask |
> + output->props_conn.value_pend_mask) & mask &&
> + item->value == value)
> + return 0;
> +
> + connector_property_debug(output, prop, value);
> +
> + if (drmModeAtomicAddProperty(req, output->connector_id, item->id,
> + value) < 0)
> + return -1;
> +
> + output->props_conn.value_valid_mask &= ~mask;
> + item->value = value;
> + output->props_conn.value_pend_mask |= mask;
> +
> + return 0;
> +}
> +
> +static int
> +atomic_crtc_add(drmModeAtomicReq *req, struct drm_output *output,
> + enum wdrm_crtc_property prop, uint64_t value)
> +{
> + struct property_item *item = &output->props_crtc.item[prop];
> + uint32_t mask = 1U << prop;
> +
> + if (!item->id)
> + return -1;
> +
> + if ((output->props_crtc.value_valid_mask |
> + output->props_crtc.value_pend_mask) & mask &&
> + item->value == value)
> + return 0;
> +
> + crtc_property_debug(output, prop, value);
> +
> + if (drmModeAtomicAddProperty(req, output->crtc_id, item->id, value) < 0)
> + return -1;
> +
> + output->props_crtc.value_valid_mask &= ~mask;
> + item->value = value;
> + output->props_crtc.value_pend_mask |= mask;
> +
> + return 0;
> +}
> +
> static void
> drm_output_set_cursor(struct drm_output *output);
>
> @@ -891,6 +1259,14 @@ drm_output_render_gl(struct drm_output *output, pixman_region32_t *damage)
> (struct drm_compositor *) output->base.compositor;
> struct gbm_bo *bo;
>
> + if (!pixman_region32_not_empty(damage) &&
> + output->primary_plane->current) {
> + if (!output->primary_plane->next)
> + output->primary_plane->next =
> + output->primary_plane->current;
> + return;
> + }
> +
> c->base.renderer->repaint_output(&output->base, damage);
This hunk should be a separate patch. Also, for a more complete
solution, see
https://git.collabora.com/cgit/user/pq/weston.git/commit/?h=next&id=73457d6d9b7ae63d7144deef98b34ddc163a2521
>
> bo = gbm_surface_lock_front_buffer(output->surface);
> @@ -969,6 +1345,189 @@ drm_output_set_gamma(struct weston_output *output_base,
> weston_log("set gamma failed: %m\n");
> }
>
> +/**
> + * Populates an existing atomic request with the properties required for a
> + * full modeset.
> + *
> + * drm_output_populate_atomic_pageflip() must be called for at least the
> + * primary plane after this, and before committing, if the output is enabled.
> + *
> + * @param output Output to calculate modeset parameters for
> + * @param req Pre-allocated atomic request to fill with values
> + * @param enable If true, enable the output; if true, switch it off
ITYM false? :-)
> + */
> +static int
> +drm_output_populate_atomic_modeset(struct drm_output *output,
> + drmModeAtomicReq *req,
> + bool enable)
> +{
> + struct drm_mode *mode = NULL;
> + int ret = 0;
> +
> + drm_crtc_update_begin(output);
> + drm_connector_update_begin(output);
> +
> + if (enable) {
> + mode = container_of(output->base.current_mode,
> + struct drm_mode, base);
> + assert(mode->blob_id);
> + print_drm_mode_info(&mode->mode_info);
> + output->primary_plane->src_w = mode->base.width;
> + output->primary_plane->src_h = mode->base.height;
> + output->primary_plane->dest_w = mode->base.width;
> + output->primary_plane->dest_h = mode->base.height;
> + }
> + else {
> + output->primary_plane->src_w = 0;
> + output->primary_plane->src_h = 0;
> + output->primary_plane->dest_w = 0;
> + output->primary_plane->dest_h = 0;
> + }
> +
> + ret |= atomic_connector_add(req, output, WDRM_CONNECTOR_CRTC_ID,
> + enable ? output->crtc_id : 0);
> + ret |= atomic_crtc_add(req, output, WDRM_CRTC_MODE_ID,
> + enable ? mode->blob_id : 0);
> + ret |= atomic_crtc_add(req, output, WDRM_CRTC_ACTIVE, enable);
> +
> + return ret;
> +}
> +
> +/**
> + * Populates an existing atomic request with the properties required to change
> + * the visible framebuffer.
> + */
> +static int
> +drm_output_populate_atomic_plane(struct drm_output *output, struct drm_plane *p,
> + drmModeAtomicReq *req)
> +{
> + struct drm_compositor *compositor =
> + (struct drm_compositor *) output->base.compositor;
> + uint32_t fb_id = 0;
> + int ret = 0;
> +
> + assert(p->output == output || p->output == NULL);
Does this not mean that a sprite cannot migrate from one output to
another without being disabled for a frame in between? Is that
intentional? What guarantees we don't attempt it and hit the assert?
> +
> + if (p->next && !compositor->sprites_hidden)
> + fb_id = p->next->fb_id;
> +
> + if (fb_id == 0)
> + output = NULL;
> +
> + drm_plane_update_begin(p);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_CRTC_ID,
> + output ? output->crtc_id : 0);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_FB_ID, fb_id);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_SRC_X, p->src_x << 16);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_SRC_Y, p->src_y << 16);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_SRC_W, p->src_w << 16);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_SRC_H, p->src_h << 16);
Shouldn't the << 16 be done where the src values are set instead of
here? If we do it here, we can never get fractional values.
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_CRTC_X, p->dest_x);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_CRTC_Y, p->dest_y);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_CRTC_W, p->dest_w);
> + ret |= atomic_plane_add(req, p, WDRM_PLANE_CRTC_H, p->dest_h);
> +
> + if (ret)
> + return ret;
> +
> + p->output = output;
> +
> + if (output)
> + wl_list_insert(&output->plane_flip_list, &p->flip_link);
> +
> + return 0;
> +}
> +
> +static int
> +drm_output_repaint_atomic(struct weston_output *output_base,
> + pixman_region32_t *damage)
> +{
> + struct drm_output *output = (struct drm_output *) output_base;
> + struct drm_compositor *compositor =
> + (struct drm_compositor *) output->base.compositor;
> + struct drm_plane *plane, *plane_tmp;
> + drmModeAtomicReq *req;
> + uint32_t flags = DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_PAGE_FLIP_EVENT;
> + int errno_save;
> + int ret = 0;
> +
> + if (output->destroy_pending)
> + return -1;
> +
> + assert(wl_list_empty(&output->plane_flip_list));
> +
> + if (!output->primary_plane->next)
> + drm_output_render(output, damage);
> +
> + if (!output->primary_plane->next) {
> + weston_log("DRM: output render failed\n");
> + return -1;
> + }
> +
> + req = drmModeAtomicAlloc();
> + if (!req) {
> + weston_log("DRM: couldn't allocate atomic request\n");
> + goto err_render;
> + }
> +
> + /* If there is no current framebuffer, then we need to do a modeset. */
> + if (!output->primary_plane->current) {
Can atomic handle stride change without a modeset?
Should we do something with DPMS?
> + ret = drm_output_populate_atomic_modeset(output, req, true);
> + if (ret)
> + goto err_populate;
> + flags |= DRM_MODE_ATOMIC_ALLOW_MODESET;
> + }
> +
> + wl_list_for_each(plane, &compositor->sprite_list, link) {
> + if (plane->output != output)
> + continue;
Should we skip if (!s->current && !s->next), do we rely on the property
state caching, of the output tracking in a drm_plane? I suppose
plane->output is somehow guaranteed NULL so we're good here?
> +
> + ret = drm_output_populate_atomic_plane(output, plane, req);
> + if (ret)
> + goto err_populate;
> + }
> +
> + errno_save = 0;
ITYM errno = 0;?
> + ret = drmModeAtomicCommit(compositor->drm.fd, req, flags, output);
> + errno_save = errno;
> + drmModeAtomicFree(req);
Move this freeing further down below, otherwise the goto err_pageflip
below will hit double-free.
> + if (ret) {
> + weston_log("DRM error: atomic commit failed for output "
> + "%s (%dx%d): %s",
> + output->base.name,
> + output->base.current_mode->width,
> + output->base.current_mode->height,
> + strerror(errno_save));
> + goto err_pageflip;
> + }
> +
> + if (!output->primary_plane->current) {
> + drm_crtc_update_success(output);
> + drm_connector_update_success(output);
> + }
> +
> + wl_list_for_each(plane, &output->plane_flip_list, link)
> + drm_plane_update_success(plane);
Should set page_flip_pending=1? Oh, but with atomic, the
plane_flip_list does the job. Ok.
> +
> + return 0;
> +
> +err_pageflip:
> + wl_list_for_each_safe(plane, plane_tmp, &output->plane_flip_list,
> + flip_link)
> + wl_list_remove(&plane->flip_link); /* XXX: release next? */
Uhhh... brain capacity exceeded for today.
> +
> + output->cursor_view = NULL;
> +
> +err_populate:
> + drmModeAtomicFree(req);
> +
> +err_render:
> + drm_output_release_fb(output, output->primary_plane->next);
> + output->primary_plane->next = NULL;
> +
> + return -1;
> +}
> +
> static int
> drm_output_repaint(struct weston_output *output_base,
> pixman_region32_t *damage)
> @@ -1083,6 +1642,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> output_base->compositor;
> uint32_t fb_id;
> struct timespec ts;
> + int ret;
>
> if (output->destroy_pending)
> return;
> @@ -1094,8 +1654,32 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
>
> fb_id = output->primary_plane->current->fb_id;
>
> - if (drmModePageFlip(compositor->drm.fd, output->crtc_id, fb_id,
> - DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> + if (compositor->atomic_modeset) {
> + drmModeAtomicReq *req = drmModeAtomicAlloc();
> +
> + output->primary_plane->next = output->primary_plane->current;
> +
> + ret = drm_output_populate_atomic_plane(output, output->primary_plane,
> + req);
> + if (ret) {
> + weston_log("DRM: start repaint atomic failed\n");
> + goto finish_frame;
> + }
> +
> + ret = drmModeAtomicCommit(compositor->drm.fd, req,
> + (DRM_MODE_PAGE_FLIP_EVENT | \
> + DRM_MODE_ATOMIC_NONBLOCK),
> + output);
> + drmModeAtomicFree(req);
> + if (ret) {
> + weston_log("DRM error: start repaint atomic commit\n");
> + wl_list_remove(&output->primary_plane->flip_link);
> + goto finish_frame;
> + }
> +
> + drm_plane_update_success(output->primary_plane);
> + } else if (drmModePageFlip(compositor->drm.fd, output->crtc_id, fb_id,
> + DRM_MODE_PAGE_FLIP_EVENT, output) < 0) {
> weston_log("queueing pageflip failed: %m\n");
> goto finish_frame;
> }
> @@ -1103,6 +1687,7 @@ drm_output_start_repaint_loop(struct weston_output *output_base)
> return;
>
> finish_frame:
> + weston_log("DRM: cannot pageflip when starting repaint loop\n");
> /* if we cannot page-flip, immediately finish frame */
> weston_compositor_read_presentation_clock(&compositor->base, &ts);
> weston_output_finish_frame(output_base, &ts,
> @@ -1152,6 +1737,9 @@ page_flip_handler(int fd, unsigned int frame,
> unsigned int sec, unsigned int usec, void *data)
> {
> struct drm_output *output = (struct drm_output *) data;
> + struct drm_compositor *compositor =
> + (struct drm_compositor *) output->base.compositor;
> + struct drm_plane *plane, *plane_tmp;
> struct timespec ts;
> uint32_t flags = PRESENTATION_FEEDBACK_KIND_VSYNC |
> PRESENTATION_FEEDBACK_KIND_HW_COMPLETION |
> @@ -1159,11 +1747,24 @@ page_flip_handler(int fd, unsigned int frame,
>
> drm_output_update_msc(output, frame);
>
> + if (compositor->atomic_modeset) {
> + wl_list_for_each_safe(plane, plane_tmp,
> + &output->plane_flip_list, flip_link) {
> + if (plane->current != plane->next)
> + drm_output_release_fb(output, plane->current);
> + plane->current = plane->next;
> + plane->next = NULL;
> + wl_list_remove(&plane->flip_link);
> + }
> + }
> +
> /* We don't set page_flip_pending on start_repaint_loop, in that case
> * we just want to page flip to the current buffer to get an accurate
> * timestamp */
> - if (output->page_flip_pending) {
> - drm_output_release_fb(output, output->primary_plane->current);
> + if (!compositor->atomic_modeset && output->page_flip_pending) {
> + if (output->primary_plane->current != output->primary_plane->next)
> + drm_output_release_fb(output,
> + output->primary_plane->current);
> output->primary_plane->current = output->primary_plane->next;
> output->primary_plane->next = NULL;
> }
> @@ -1591,6 +2192,7 @@ drm_output_destroy(struct weston_output *output_base)
> backlight_destroy(output->backlight);
>
> drmModeFreeProperty(output->dpms_prop);
> + output_properties_release(output);
>
> /* Turn off hardware cursor */
> drmModeSetCursor(c->drm.fd, output->crtc_id, 0, 0, 0);
> @@ -1685,7 +2287,8 @@ drm_output_switch_mode(struct weston_output *output_base, struct weston_mode *mo
>
> /* reset rendering stuff. */
> drm_output_release_fb(output, output->primary_plane->current);
> - drm_output_release_fb(output, output->primary_plane->next);
> + if (output->primary_plane->current != output->primary_plane->next)
> + drm_output_release_fb(output, output->primary_plane->next);
> output->primary_plane->current = output->primary_plane->next = NULL;
>
> if (ec->use_pixman) {
> @@ -1782,6 +2385,20 @@ init_drm(struct drm_compositor *ec, struct udev_device *device)
> weston_log("DRM: %s universal planes\n",
> ec->universal_planes ? "supports" : "does not support");
>
> + ret = drmSetClientCap(fd, DRM_CLIENT_CAP_ATOMIC, 1);
> + ec->atomic_modeset = (ret == 0);
> + weston_log("DRM: %susing atomic modesetting\n",
> + ec->atomic_modeset ? "" : "not ");
So this is enough, no need to do an empty test set?
> +
> + /*
> + * KMS support for hardware planes cannot properly synchronize
> + * without nuclear page flip. Without nuclear/atomic, hw plane
> + * plane updates would either tear or cause extra waits for vblanks
> + * which means dropping the compositor framerate to a fraction.
> + */
> + if (!ec->atomic_modeset)
> + ec->sprites_are_broken = 1;
> +
> return 0;
> }
>
> @@ -1878,6 +2495,8 @@ init_pixman(struct drm_compositor *ec)
> static struct drm_mode *
> drm_output_add_mode(struct drm_output *output, const drmModeModeInfo *info)
> {
> + struct drm_compositor *ec =
> + (struct drm_compositor *) output->base.compositor;
> struct drm_mode *mode;
> uint64_t refresh;
>
> @@ -1906,6 +2525,15 @@ drm_output_add_mode(struct drm_output *output, const drmModeModeInfo *info)
> if (info->type & DRM_MODE_TYPE_PREFERRED)
> mode->base.flags |= WL_OUTPUT_MODE_PREFERRED;
>
> + if (ec->atomic_modeset &&
> + drmModeCreatePropertyBlob(ec->drm.fd, info, sizeof(*info),
> + &mode->blob_id) != 0) {
> + weston_log("Failed to create blob for mode %s\n",
> + info->name);
> + free(mode);
> + return NULL;
> + }
> +
> wl_list_insert(output->base.mode_list.prev, &mode->base.link);
>
> return mode;
> @@ -1997,8 +2625,29 @@ drm_set_dpms(struct weston_output *output_base, enum dpms_enum level)
> if (!output->dpms_prop)
> return;
>
> - drmModeConnectorSetProperty(c->drm.fd, output->connector_id,
> - output->dpms_prop->prop_id, level);
> + if (c->atomic_modeset) {
> + drmModeAtomicReq *req = drmModeAtomicAlloc();
> + int ret;
> +
> + ret = atomic_crtc_add(req, output, WDRM_CRTC_ACTIVE,
> + (level == WESTON_DPMS_ON));
> + if (ret) {
> + weston_log("DRM: DPMS: failed to add ACTIVE prop\n");
> + return;
> + }
> +
> + ret = drmModeAtomicCommit(c->drm.fd, req, 0, output);
> + drmModeAtomicFree(req);
> + if (ret) {
> + weston_log("DRM: DPMS: failed atomic commit for %s\n",
> + output->base.name);
> + return;
> + }
> + }
> + else {
> + drmModeConnectorSetProperty(c->drm.fd, output->connector_id,
> + output->dpms_prop->prop_id, level);
> + }
> }
>
> static const char * const connector_type_names[] = {
> @@ -2517,6 +3166,8 @@ drm_output_init_primary_plane(struct drm_output *output)
> continue;
> if (plane->output)
> continue;
> + if (output->primary_plane)
> + continue;
> if (!drm_plane_crtc_supported(output,
> plane->possible_crtcs))
> continue;
> @@ -2571,6 +3222,8 @@ drm_output_init_cursor(struct drm_output *output)
> continue;
> if (plane->output)
> continue;
> + if (output->cursor_plane)
> + continue;
> if (!drm_plane_crtc_supported(output,
> plane->possible_crtcs))
> continue;
> @@ -2637,12 +3290,14 @@ create_output_for_connector(struct drm_compositor *ec,
> if (output == NULL)
> return -1;
>
> + output->base.compositor = &ec->base;
> output->base.subpixel = drm_subpixel_to_wayland(connector->subpixel);
> output->base.name = make_connector_name(connector);
> output->base.make = "unknown";
> output->base.model = "unknown";
> output->base.serial_number = "unknown";
> wl_list_init(&output->base.mode_list);
> + wl_list_init(&output->plane_flip_list);
>
> section = weston_config_get_section(ec->base.config, "output", "name",
> output->base.name);
> @@ -2701,8 +3356,18 @@ create_output_for_connector(struct drm_compositor *ec,
>
> if (config == OUTPUT_CONFIG_OFF) {
> weston_log("Disabling output %s\n", output->base.name);
> - drmModeSetCrtc(ec->drm.fd, output->crtc_id,
> - 0, 0, 0, 0, 0, NULL);
> + if (ec->atomic_modeset) {
> + drmModeAtomicReq *req = drmModeAtomicAlloc();
> + drm_output_populate_atomic_modeset(output, req, false);
> + drmModeAtomicCommit(ec->drm.fd, req,
> + DRM_MODE_ATOMIC_ALLOW_MODESET,
> + output);
> + drmModeAtomicFree(req);
> + }
> + else {
> + drmModeSetCrtc(ec->drm.fd, output->crtc_id,
> + 0, 0, 0, 0, 0, NULL);
> + }
> goto err_free;
> }
>
> @@ -2726,6 +3391,9 @@ create_output_for_connector(struct drm_compositor *ec,
> }
> drm_output_init_cursor(output);
>
> + if (!output_properties_init(output))
> + goto err_output;
> +
> if (ec->use_pixman) {
> if (drm_output_init_pixman(output, ec) < 0) {
> weston_log("Failed to init output pixman state\n");
> @@ -2754,7 +3422,10 @@ create_output_for_connector(struct drm_compositor *ec,
> output->base.connection_internal = 1;
>
> output->base.start_repaint_loop = drm_output_start_repaint_loop;
> - output->base.repaint = drm_output_repaint;
> + if (ec->atomic_modeset)
> + output->base.repaint = drm_output_repaint_atomic;
> + else
> + output->base.repaint = drm_output_repaint;
> output->base.destroy = drm_output_destroy;
> output->base.assign_planes = drm_assign_planes;
> output->base.set_dpms = drm_set_dpms;
> @@ -2786,6 +3457,10 @@ err_output:
> err_free:
> wl_list_for_each_safe(drm_mode, next, &output->base.mode_list,
> base.link) {
> + /* XXX: Needs proper destroy function. */
> + if (ec->atomic_modeset)
> + drmModeDestroyPropertyBlob(ec->drm.fd,
> + drm_mode->blob_id);
> wl_list_remove(&drm_mode->base.link);
> free(drm_mode);
> }
> @@ -2841,7 +3516,7 @@ drm_plane_create(struct drm_compositor *ec, const drmModePlane *kplane)
> }
>
> if (ec->universal_planes)
> - plane->type = drm_plane_property_get(plane, WDRM_PLANE_TYPE);
> + plane->type = drm_plane_get_type(plane);
Heh, I didn't notice this before. The old code uses the wrong
"namespace" for type, drm_plane_property_get() returns the raw DRM
value, not a WDRM value.
> else
> plane->type = WDRM_PLANE_TYPE_OVERLAY;
>
> @@ -3133,8 +3808,17 @@ drm_compositor_set_modes(struct drm_compositor *compositor)
> {
> struct drm_output *output;
> struct drm_mode *drm_mode;
> + drmModeAtomicReq *req = NULL;
> int ret;
>
> + if (compositor->atomic_modeset) {
> + req = drmModeAtomicAlloc();
> + if (!req) {
> + weston_log("DRM: failed to allocate atomic req\n");
> + return;
> + }
> + }
> +
> wl_list_for_each(output, &compositor->base.output_list, base.link) {
> if (!output->primary_plane->current) {
> /* If something that would cause the output to
> @@ -3146,6 +3830,11 @@ drm_compositor_set_modes(struct drm_compositor *compositor)
> continue;
> }
>
> + if (compositor->atomic_modeset) {
> + drm_output_populate_atomic_modeset(output, req, true);
> + continue;
> + }
> +
> drm_mode = (struct drm_mode *) output->base.current_mode;
> ret = drmModeSetCrtc(compositor->drm.fd, output->crtc_id,
> output->primary_plane->current->fb_id, 0, 0,
> @@ -3158,6 +3847,14 @@ drm_compositor_set_modes(struct drm_compositor *compositor)
> output->base.x, output->base.y);
> }
> }
> +
> + if (compositor->atomic_modeset) {
> + ret = drmModeAtomicCommit(compositor->drm.fd, req,
> + DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> + drmModeAtomicFree(req);
> + if (ret)
> + weston_log("DRM: atomic modeset failed\n");
> + }
> }
>
> static void
> @@ -3469,18 +4166,6 @@ drm_compositor_create(struct wl_display *display,
> if (ec == NULL)
> return NULL;
>
> - /*
> - * KMS support for hardware planes cannot properly synchronize
> - * without nuclear page flip. Without nuclear/atomic, hw plane
> - * and cursor plane updates would either tear or cause extra
> - * waits for vblanks which means dropping the compositor framerate
> - * to a fraction.
> - *
> - * These can be enabled again when nuclear/atomic support lands.
> - */
> - ec->sprites_are_broken = 1;
> - ec->cursors_are_broken = 1;
> -
> section = weston_config_get_section(config, "core", NULL, NULL);
> if (get_gbm_format_from_section(section,
> GBM_FORMAT_XRGB8888,
Yes! \o/
Btw. this patch also reverts the cursors_are_broken default, so maybe
we should do that separately.
Thanks,
pq
More information about the wayland-devel
mailing list