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

[Pekka Paalanen]

On Mon, 24 Jul 2017 18:13:39 +0100
Daniel Stone <daniel at fooishbar.org> 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 <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.

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

-------------- 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/20170725/709f9910/attachment-0001.sig>