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 <ppaala...@gmail.com> wrote:
> On Tue, 18 Jul 2017 14:14:25 +0100
> Daniel Stone <dani...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
