[PATCH weston v11 03/13] compositor-drm: Add DRM property cache

[Daniel Stone]

Hi Pekka,
I've made a mental note to make as careful passes over the
documentation as the code before I send for review ...

On 19 July 2017 at 15:43, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 18 Jul 2017 14:14:25 +0100
> Daniel Stone <daniels at collabora.com> wrote:
>> +             /* 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?

Not really, it's just a new value. Consider, e.g., the 'scaling mode'
property: if someone wanted to add a new scaling mode, doing so would
instantly break Weston if it was started when that scaling mode is
set.

I'm not really sure if there's a better way to communicate that we've
got an enum we don't properly handle. I think the behaviour is OK for
now, but we can definitely revisit it if we start adding support for
more and varied plane types.

>> +/**
>> + * 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.

When a connector is hotplugged, that can invalidate the enum
properties, so we still need to re-scan.

>> +                     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.

As above, it'll be true when running on, e.g., hotplug.

Cheers,
Daniel