[PATCH weston v10 19/61] compositor-drm: Add DRM property cache
Pekka Paalanen
ppaalanen at gmail.com
Mon Apr 10 14:46:59 UTC 2017
On Tue, 4 Apr 2017 17:54:37 +0100
Daniel Stone <daniels at 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. In particular, the enum map has been lifted into the property
> item itself, and changed from per-object to per-object-type, which
> necessitated removing the value cache.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Co-authored-with: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
> libweston/compositor-drm.c | 418 +++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 363 insertions(+), 55 deletions(-)
Hi,
https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#kms-properties says:
"Properties are defined by their symbolic name, userspace must
keep a per-object mapping from those names to the property ID
used in the atomic IOCTL and in the get/set property IOCTL."
It says per-object, not per-object-type.
Furthermore it says:
"This structure represent a modeset object property. It
combines both the name of the property with the set of
permissible values. This means that when a driver wants to use
a property with the same name on different objects, but with
different value ranges, then it must create property for each
one. An example would be rotation of drm_plane, when e.g. the
primary plane cannot be rotated. But if both the name and the
value range match, then the same property structure can be
instantiated multiple times for the same object. Userspace must
be able to cope with this and cannot assume that the same
symbolic property will have the same modeset object ID on all
modeset objects."
Therefore AFAIU, I think this patch is not right, because it uses a
global property id cache (for connectors) instead of a per object
(connector) one.
Other details below.
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 8a31abb1..a32339e4 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;
> @@ -116,6 +154,9 @@ struct drm_backend {
>
> struct udev_input input;
>
> + /* Holds the properties for connectors */
> + struct drm_property_info props_conn[WDRM_CONNECTOR__COUNT];
This is shared between all connectors in the system. Yet, the KMS spec
seems to say that such may not be the case: different connectors may
have a different property id for the same-named property.
It is unlikely that EDID would have different property id, don't know
about DPMS. There are lots of connector properties defined though, so
for future's sake this should be in weston_output.
> +
> int32_t cursor_width;
> int32_t cursor_height;
>
> @@ -215,7 +256,6 @@ struct drm_output {
> uint32_t connector_id;
> drmModeCrtcPtr original_crtc;
> struct drm_edid edid;
> - drmModePropertyPtr dpms_prop;
>
> enum dpms_enum dpms;
> struct backlight *backlight;
> @@ -311,6 +351,262 @@ 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;
> + }
> + }
> +
> + return def;
> +}
Looks good.
> +
> +/**
> + * Cache DRM property values
> + *
> + * Update a per-object-type array of drm_property_info structures, given the
> + * current property values for a particular object.
> + *
> + * 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 don't understand the last bit. The function seems to be skipping
everything that has been seen once before, so updating invalidated
properties is not possible.
> + *
> + * This updates the property IDs and enum values within the drm_property_info
> + * array.
> + *
> + * Property lookup is not mandatory and may fail; users should carefully
> + * check the return value, which is a bitmask populated with the indices
> + * of properties which were successfully looked up.
Nothing uses the return value, not even after the complete atomic
series. It should be removed.
> + *
> + * 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 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.
> + *
> + * @param b DRM backend object
> + * @param info DRM property info array
> + * @param num_infos Number of entries in DRM property info array
> + * @param props DRM object properties for a given object
> + *
> + * @returns Bitmask of populated entries in map
> + */
> +static uint32_t
> +drm_property_info_update(struct drm_backend *b,
> + struct drm_property_info *info,
> + unsigned int num_infos,
> + drmModeObjectProperties *props)
> +{
> + drmModePropertyRes *prop;
> + uint32_t valid_mask = 0;
> + unsigned i, j;
> +
> + assert(num_infos <= 32 && "update return type");
Remove assert() here and valid_mask below when removing the return
value.
> +
> + for (i = 0; i < props->count_props; i++) {
> + bool props_incomplete = false;
> + unsigned int k;
> +
> + for (j = 0; j < num_infos; j++) {
> + if (info[j].prop_id == props->props[i])
> + break;
> + if (!info[j].prop_id)
> + props_incomplete = true;
> + }
> +
> + /* We've already discovered this property. */
> + if (j != num_infos)
> + continue;
> +
> + /* We haven't found this property ID, but as we've already
> + * found all known properties, we don't need to look any
> + * further. */
> + if (!props_incomplete)
> + break;
> +
> + 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];
> + valid_mask |= 1U << j;
> +
> + if (info[j].num_enum_values == 0) {
> + 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);
This does not seems to be ignoring, because the assignment to
info[j].prop_id was already done.
> + continue;
> + }
> +
> + for (k = 0; k < info[j].num_enum_values; k++) {
> + int l;
> +
> + if (info[j].enum_values[k].valid)
> + 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
> +
> + return valid_mask;
> +}
> +
> +/**
> + * Copy DRM property information
> + *
> + * Copies an array of drm_property_info entries.
> + *
> + * @param dst_out Pointer to (already-allocated) new array
> + * @param src Property info array to copy from
> + * @param num_props Number of properties to copy from src
> + */
> +static bool
> +drm_property_info_copy(struct drm_property_info *dst,
> + const struct drm_property_info *src,
> + unsigned int num_props)
> +{
> + unsigned int i;
> +
> + memcpy(dst, src, num_props * sizeof(*dst));
> +
> + for (i = 0; i < num_props; i++) {
> + unsigned int j;
> +
> + dst[i].prop_id = 0;
> +
> + if (src[i].num_enum_values == 0)
> + continue;
> +
> + dst[i].enum_values =
> + malloc(src[i].num_enum_values *
> + sizeof(*dst[i].enum_values));
> + if (!dst[i].enum_values)
> + goto err;
> +
> + memcpy(dst[i].enum_values, src[i].enum_values,
> + src[i].num_enum_values * sizeof(*dst[i].enum_values));
> +
> + for (j = 0; j < dst[i].num_enum_values; j++)
> + dst[i].enum_values[j].valid = false;
> + }
First it looks like it copies everything, but in fact it only copies
the property and enum-item names and counts, and sets all ids to zero
and enum-items not valid. How about not using memcpy() and just writing
out what it's actually doing, for better readability?
The doc also forgets to explain that it essentially copies only the
names, not ids. The caller must use drm_property_info_update() to fill
in the ids.
> +
> + return true;
> +
> +err:
> + while (i--)
> + free(dst[i].enum_values);
> + free(dst);
Shouldn't free(dst) be left for the caller who allocated it?
> + return false;
> +}
> +
> +/**
> + * Free DRM property information
> + *
> + * Frees a DRM property information array and all associated memory
Except it does not free the array itself.
> + *
> + * @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);
>
The rest of the code did not raise any comments on this reading.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170410/8e491381/attachment.sig>
More information about the wayland-devel
mailing list