On Mon, 24 Jul 2017 18:13:39 +0100 Daniel Stone <[email protected]> wrote:
> 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 <[email protected]> wrote: > > On Tue, 18 Jul 2017 14:14:25 +0100 > > Daniel Stone <[email protected]> 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. Hi, oh yes, that makes sense. > >> +/** > >> + * 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. Does that not mean that a new DRM connector object appears? Or that an existing DRM connector object magically becomes rewritten in which case what do we use for triggering the re-scan? Do we need to re-scan all DRM connector objects every time we get a hotplug event? I mean, now that you said that, and I have read the doc comment a dozen times more, I'm getting a feeling of what it's trying to say in a very very compressed form. Expand a bit, please. :-) Thanks, pq > >> + 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
pgpkMh4YH39SP.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
