On Tue, 18 Jul 2017 14:14:25 +0100 Daniel Stone <dani...@collabora.com> wrote:
> Add a cache for DRM property IDs and values, and use it for the two
> connector properties we currently update: DPMS and EDID.
>
> As DRM property ID values are not stable, we need to do a name -> ID
> lookup each run in order to discover the property IDs and enum values to
> use for those properties. Rather than open-coding this, add a property
> cache which we can use across multiple different object types.
>
> This patch takes substantial work from the universal planes support
> originally authored by Pekka Paalanen, though it has been heavily
> reworked.
>
> Signed-off-by: Daniel Stone <dani...@collabora.com>
> Co-authored-with: Pekka Paalanen <pekka.paala...@collabora.co.uk>
> ---
> libweston/compositor-drm.c | 341
> ++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 289 insertions(+), 52 deletions(-)
Hi,
strictly speaking, the code below is correct. I pointed out some issues
with documentation, and made some questions/suggestion to make even
more robust, but all in all, this looks good.
You are free to slap my
Reviewed-by: Pekka Paalanen <pekka.paala...@collabora.co.uk>
after dealing with my comments as you see fit.
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 54b50d9d..e7e8e821 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -79,6 +79,44 @@
> #define GBM_BO_USE_CURSOR GBM_BO_USE_CURSOR_64X64
> #endif
>
> +/**
> + * List of properties attached to a DRM connector
> + */
> +enum wdrm_connector_property {
> + WDRM_CONNECTOR_EDID = 0,
> + WDRM_CONNECTOR_DPMS,
> + WDRM_CONNECTOR__COUNT
> +};
> +
> +/**
> + * Represents the values of an enum-type KMS property
> + */
> +struct drm_property_enum_info {
> + const char *name; /**< name as string (static, not freed) */
> + bool valid; /**< true if value is supported; ignore if false */
> + uint64_t value; /**< raw value */
> +};
> +
> +/**
> + * Holds information on a DRM property, including its ID and the enum
> + * values it holds.
> + *
> + * DRM properties are allocated dynamically, and maintained as DRM objects
> + * within the normal object ID space; they thus do not have a stable ID
> + * to refer to. This includes enum values, which must be referred to by
> + * integer values, but these are not stable.
> + *
> + * drm_property_info allows a cache to be maintained where Weston can use
> + * enum values internally to refer to properties, with the mapping to DRM
> + * ID values being maintained internally.
> + */
> +struct drm_property_info {
> + const char *name; /**< name as string (static, not freed) */
> + uint32_t prop_id; /**< KMS property object ID */
> + unsigned int num_enum_values; /**< number of enum values */
> + struct drm_property_enum_info *enum_values; /**< array of enum values */
> +};
> +
> struct drm_backend {
> struct weston_backend base;
> struct weston_compositor *compositor;
> @@ -215,7 +253,9 @@ struct drm_output {
> uint32_t connector_id;
> drmModeCrtcPtr original_crtc;
> struct drm_edid edid;
> - drmModePropertyPtr dpms_prop;
> +
> + /* Holds the properties for the connector */
> + struct drm_property_info props_conn[WDRM_CONNECTOR__COUNT];
>
> enum dpms_enum dpms;
> struct backlight *backlight;
> @@ -311,6 +351,212 @@ drm_output_pageflip_timer_create(struct drm_output
> *output)
> return 0;
> }
>
> +/**
> + * Get the current value of a KMS property
> + *
> + * Given a drmModeObjectGetProperties return, as well as the
> drm_property_info
> + * for the target property, return the current value of that property,
> + * with an optional default. If the property is a KMS enum type, the return
> + * value will be translated into the appropriate internal enum.
> + *
> + * If the property is not present, the default value will be returned.
> + *
> + * @param info Internal structure for property to look up
> + * @param props Raw KMS properties for the target object
> + * @param def Value to return if property is not found
> + */
> +static uint64_t
> +drm_property_get_value(struct drm_property_info *info,
> + drmModeObjectPropertiesPtr props,
> + uint64_t def)
> +{
> + unsigned int i;
> +
> + if (info->prop_id == 0)
> + return def;
> +
> + for (i = 0; i < props->count_props; i++) {
> + unsigned int j;
> +
> + if (props->props[i] != info->prop_id)
> + continue;
> +
> + /* Simple (non-enum) types can return the value directly */
> + if (info->num_enum_values == 0)
> + return props->prop_values[i];
> +
> + /* Map from raw value to enum value */
> + for (j = 0; j < info->num_enum_values; j++) {
> + if (!info->enum_values[j].valid)
> + continue;
> + if (info->enum_values[j].value != props->prop_values[i])
> + continue;
> +
> + return j;
> + }
> +
> + /* We don't have a mapping for this enum; return default. */
> + break;
When we have an enum property whose value we cannot interpret, would
this be a place for an assert(0) or an scream in the Weston log? It is
either a bug in Weston, libdrm, or in the kernel, right?
> + }
> +
> + return def;
> +}
> +
> +/**
> + * Cache DRM property values
> + *
> + * Update a per-object-type array of drm_property_info structures, given the
> + * current property values for a particular object.
I think that should instead say:
* Cache DRM property IDs
*
* Update a per-object array of drm_property_info structures, given the
* DRM properties of the object.
The original implementation I had actually cached the current values as
well rather than only property IDs and enum value IDs, but we do not do
that anymore.
> + *
> + * Call this every time an object newly appears (note that only connectors
> + * can be hotplugged), the first time it is seen, or when its status changes
> + * in a way which invalidates the potential property values (currently, the
> + * only case for this is connector hotplug).
I believe this should shorten to: "Call this for every DRM object
individually once."
The language about status changes is probably a remnant of the value
caching days.
> + *
> + * This updates the property IDs and enum values within the drm_property_info
> + * array.
> + *
> + * DRM property enum values are dynamic at runtime; the user must query the
> + * property to find out the desired runtime value for a requested string
> + * name. Using the 'type' field on planes as an example, there is no single
> + * hardcoded constant for primary plane types; instead, the property must be
> + * queried at runtime to find the value associated with the string "Primary".
> + *
> + * This helper queries and caches the enum values, to allow us to use a set
> + * of compile-time-constant enums portably across various implementations.
The above is good, but the below...
> + * The values given in enum_names are searched for, and stored in the
> + * same-indexed field of the map array.
> + *
> + * For example, if the DRM driver exposes 'Foo' as value 7 and 'Bar' as value
> + * 19, passing in enum_names containing 'Foo' and 'Bar' in that order, will
> + * populate map with 7 and 19, in that order.
> + *
> + * This should always be used with static C enums to represent each value,
> e.g.
> + * WDRM_PLANE_MISC_FOO, WDRM_PLANE_MISC_BAR.
Not sure this hunk is too clear or even matching today's code in more
than spirit. Remove or rewrite? I'd be fine with just removing.
> + *
> + * @param b DRM backend object
> + * @param src DRM property info array to source from
> + * @param dst DRM property info array to copy into
'dst' is actually 'info' in the code, rename?
> + * @param num_infos Number of entries in DRM property info array
and in the src array.
> + * @param props DRM object properties for the object
> + *
> + * @returns Bitmask of populated entries in map
Returns void.
> + */
> +static void
> +drm_property_info_update(struct drm_backend *b,
> + const struct drm_property_info *src,
> + struct drm_property_info *info,
> + unsigned int num_infos,
> + drmModeObjectProperties *props)
> +{
> + drmModePropertyRes *prop;
> + unsigned i, j;
> +
> + for (i = 0; i < num_infos; i++) {
> + unsigned int j;
> +
> + info[i].name = src[i].name;
> + info[i].prop_id = 0;
> + info[i].num_enum_values = src[i].num_enum_values;
> +
> + if (src[i].num_enum_values == 0)
> + continue;
> +
> + info[i].enum_values =
> + malloc(src[i].num_enum_values *
> + sizeof(*info[i].enum_values));
> + assert(info[i].enum_values);
> + for (j = 0; j < info[i].num_enum_values; j++) {
> + info[i].enum_values[j].name =
> src[i].enum_values[j].name;
> + info[i].enum_values[j].valid = false;
> + }
> + }
> +
> + for (i = 0; i < props->count_props; i++) {
> + unsigned int k;
> +
> + prop = drmModeGetProperty(b->drm.fd, props->props[i]);
> + if (!prop)
> + continue;
> +
> + for (j = 0; j < num_infos; j++) {
> + if (!strcmp(prop->name, info[j].name))
> + break;
> + }
> +
> + /* We don't know/care about this property. */
> + if (j == num_infos) {
> +#ifdef DEBUG
> + weston_log("DRM debug: unrecognized property %u '%s'\n",
> + prop->prop_id, prop->name);
> +#endif
> + drmModeFreeProperty(prop);
> + continue;
> + }
> +
> + info[j].prop_id = props->props[i];
> +
> + if (info[j].num_enum_values == 0) {
Would we want to ensure here that the property is not an enum?
> + drmModeFreeProperty(prop);
> + continue;
> + }
> +
> + if (!(prop->flags & DRM_MODE_PROP_ENUM)) {
> + weston_log("DRM: expected property %s to be an enum,"
> + " but it is not; ignoring\n", prop->name);
> + drmModeFreeProperty(prop);
> + info[j].prop_id = 0;
> + continue;
> + }
> +
> + for (k = 0; k < info[j].num_enum_values; k++) {
> + int l;
> +
> + if (info[j].enum_values[k].valid)
This should never be true, should it?
If it was, it would seem odd to skip updating it, when we happily
update everything else.
> + continue;
> +
> + for (l = 0; l < prop->count_enums; l++) {
> + if (!strcmp(prop->enums[l].name,
> + info[j].enum_values[k].name))
> + break;
> + }
> +
> + if (l == prop->count_enums)
> + continue;
> +
> + info[j].enum_values[k].valid = true;
> + info[j].enum_values[k].value = prop->enums[l].value;
> + }
> +
> + drmModeFreeProperty(prop);
> + }
> +
> +#ifdef DEBUG
> + for (i = 0; i < num_infos; i++) {
> + if (info[i].prop_id == 0)
> + weston_log("DRM warning: property '%s' missing\n",
> + info[i].name);
> + }
> +#endif
> +}
Anyway, drm_property_info_update() code looks correct. Is the name
accurate though? How about drm_property_info_populate()? We do not
expect to call it more than once for each DRM object.
> +
> +/**
> + * Free DRM property information
> + *
> + * Frees all memory associated with a DRM property info array.
> + *
> + * @param info DRM property info array
> + * @param num_props Number of entries in array to free
> + */
> +static void
> +drm_property_info_free(struct drm_property_info *info, int num_props)
> +{
> + int i;
> +
> + for (i = 0; i < num_props; i++)
> + free(info[i].enum_values);
> +}
> +
> static void
> drm_output_set_cursor(struct drm_output *output);
>
> @@ -2036,39 +2282,21 @@ drm_set_backlight(struct weston_output *output_base,
> uint32_t value)
> backlight_set_brightness(output->backlight, new_brightness);
> }
>
> -static drmModePropertyPtr
> -drm_get_prop(int fd, drmModeConnectorPtr connector, const char *name)
> -{
> - drmModePropertyPtr props;
> - int i;
> -
> - for (i = 0; i < connector->count_props; i++) {
> - props = drmModeGetProperty(fd, connector->props[i]);
> - if (!props)
> - continue;
> -
> - if (!strcmp(props->name, name))
> - return props;
> -
> - drmModeFreeProperty(props);
> - }
> -
> - return NULL;
> -}
> -
> static void
> drm_set_dpms(struct weston_output *output_base, enum dpms_enum level)
> {
> struct drm_output *output = to_drm_output(output_base);
> struct weston_compositor *ec = output_base->compositor;
> struct drm_backend *b = to_drm_backend(ec);
> + struct drm_property_info *prop =
> + &output->props_conn[WDRM_CONNECTOR_DPMS];
> int ret;
>
> - if (!output->dpms_prop)
> + if (!prop->prop_id)
> return;
>
> ret = drmModeConnectorSetProperty(b->drm.fd, output->connector_id,
> - output->dpms_prop->prop_id, level);
> + prop->prop_id, level);
> if (ret) {
> weston_log("DRM: DPMS: failed property set for %s\n",
> output->base.name);
> @@ -2423,26 +2651,20 @@ edid_parse(struct drm_edid *edid, const uint8_t
> *data, size_t length)
> }
>
> static void
> -find_and_parse_output_edid(struct drm_backend *b,
> - struct drm_output *output,
> - drmModeConnector *connector)
> +find_and_parse_output_edid(struct drm_backend *b, struct drm_output *output,
> + drmModeObjectPropertiesPtr props)
> {
> drmModePropertyBlobPtr edid_blob = NULL;
> - drmModePropertyPtr property;
> - int i;
> + uint32_t blob_id;
> int rc;
>
> - for (i = 0; i < connector->count_props && !edid_blob; i++) {
> - property = drmModeGetProperty(b->drm.fd, connector->props[i]);
> - if (!property)
> - continue;
> - if ((property->flags & DRM_MODE_PROP_BLOB) &&
> - !strcmp(property->name, "EDID")) {
> - edid_blob = drmModeGetPropertyBlob(b->drm.fd,
> -
> connector->prop_values[i]);
> - }
> - drmModeFreeProperty(property);
> - }
> + blob_id =
> + drm_property_get_value(&output->props_conn[WDRM_CONNECTOR_EDID],
> + props, 0);
> + if (!blob_id)
> + return;
> +
> + edid_blob = drmModeGetPropertyBlob(b->drm.fd, blob_id);
> if (!edid_blob)
> return;
>
> @@ -2733,19 +2955,17 @@ drm_output_enable(struct weston_output *base)
> struct drm_backend *b = to_drm_backend(base->compositor);
> struct weston_mode *m;
>
> - output->dpms_prop = drm_get_prop(b->drm.fd, output->connector, "DPMS");
> -
> if (b->pageflip_timeout)
> drm_output_pageflip_timer_create(output);
>
> if (b->use_pixman) {
> if (drm_output_init_pixman(output, b) < 0) {
> weston_log("Failed to init output pixman state\n");
> - goto err_free;
> + goto err;
> }
> } else if (drm_output_init_egl(output, b) < 0) {
> weston_log("Failed to init output gl state\n");
> - goto err_free;
> + goto err;
> }
>
> if (output->backlight) {
> @@ -2768,7 +2988,6 @@ drm_output_enable(struct weston_output *base)
>
> output->base.subpixel =
> drm_subpixel_to_wayland(output->connector->subpixel);
>
> - find_and_parse_output_edid(b, output, output->connector);
> if (output->connector->connector_type == DRM_MODE_CONNECTOR_LVDS ||
> output->connector->connector_type == DRM_MODE_CONNECTOR_eDP)
> output->base.connection_internal = 1;
> @@ -2795,9 +3014,7 @@ drm_output_enable(struct weston_output *base)
>
> return 0;
>
> -err_free:
> - drmModeFreeProperty(output->dpms_prop);
> -
> +err:
> return -1;
> }
>
> @@ -2822,8 +3039,6 @@ drm_output_deinit(struct weston_output *base)
> weston_plane_release(&output->scanout_plane);
> weston_plane_release(&output->cursor_plane);
>
> - drmModeFreeProperty(output->dpms_prop);
> -
> /* Turn off hardware cursor */
> drmModeSetCursor(b->drm.fd, output->crtc_id, 0, 0, 0);
> }
> @@ -2864,6 +3079,8 @@ drm_output_destroy(struct weston_output *base)
>
> weston_output_destroy(&output->base);
>
> + drm_property_info_free(output->props_conn, WDRM_CONNECTOR__COUNT);
> +
> drmModeFreeConnector(output->connector);
>
> if (output->backlight)
> @@ -2915,9 +3132,15 @@ create_output_for_connector(struct drm_backend *b,
> struct udev_device *drm_device)
> {
> struct drm_output *output;
> + drmModeObjectPropertiesPtr props;
> struct drm_mode *drm_mode;
> int i;
>
> + static const struct drm_property_info connector_props[] = {
> + [WDRM_CONNECTOR_EDID] = { .name = "EDID" },
> + [WDRM_CONNECTOR_DPMS] = { .name = "DPMS" },
> + };
> +
> i = find_crtc_for_connector(b, resources, connector);
> if (i < 0) {
> weston_log("No usable crtc/encoder pair for connector.\n");
> @@ -2946,6 +3169,17 @@ create_output_for_connector(struct drm_backend *b,
> output->destroy_pending = 0;
> output->disable_pending = 0;
>
> + props = drmModeObjectGetProperties(b->drm.fd, connector->connector_id,
> + DRM_MODE_OBJECT_CONNECTOR);
> + if (!props) {
> + weston_log("failed to get connector properties\n");
> + goto err;
> + }
> + drm_property_info_update(b, connector_props, output->props_conn,
> + WDRM_CONNECTOR__COUNT, props);
> + find_and_parse_output_edid(b, output, props);
> + drmModeFreeObjectProperties(props);
> +
> weston_output_init(&output->base, b->compositor);
>
> wl_list_init(&output->base.mode_list);
> @@ -2987,6 +3221,8 @@ create_outputs(struct drm_backend *b, struct
> udev_device *drm_device)
> b->max_height = resources->max_height;
>
> for (i = 0; i < resources->count_connectors; i++) {
> + int ret;
> +
> connector = drmModeGetConnector(b->drm.fd,
> resources->connectors[i]);
> if (connector == NULL)
> @@ -2995,9 +3231,10 @@ create_outputs(struct drm_backend *b, struct
> udev_device *drm_device)
> if (connector->connection == DRM_MODE_CONNECTED &&
> (b->connector == 0 ||
> connector->connector_id == b->connector)) {
> - if (create_output_for_connector(b, resources,
> - connector, drm_device)
> < 0)
> - continue;
> + ret = create_output_for_connector(b, resources,
> + connector,
> drm_device);
> + if (ret < 0)
> + weston_log("failed to create new connector\n");
This change seems to be extra, but doesn't hurt.
> } else {
> drmModeFreeConnector(connector);
> }
Thanks,
pq
pgpx55Jd9IXzS.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
