Three comments about the design are inline:
> +void drm_crtc_attach_property(struct drm_crtc *crtc,
> + struct drm_property *property, uint64_t init_val)
> +{
> + int i;
> +
> + for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) {
> + if (crtc->property_ids[i] == 0) {
> + crtc->property_ids[i] = property->base.id;
> + crtc->property_values[i] = init_val;
> + return;
> + }
> + }
> + BUG_ON(1);
I looked at drm_connector_attach_property and saw that instead of
BUG_ON(1), it tries to return -EINVAL. The problem is that only zero
callers check for the return value of drm_connector_attach_property. I
can provide a patch for drm_connector_attach_property changing the
-EINVAL for BUG_ON(1) if no one objects. Or I can also add -EINVAL to
drm_crtc_attach_property and, to be consistent, not check for it :)
>
> +int drm_mode_crtc_get_properties_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
... (same function) ...
> +
> + for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++)
> + if (crtc->property_ids[i] != 0)
> + props_count++;
... (same function) ...
> + for (i = 0; i < props_count; i++) {
> + if (put_user(crtc->property_ids[i],
> + props_ptr + copied)) {
> + ret = -EFAULT;
> + goto out;
> + }
If you look at the connector properties (and now crtc properties),
you'll see that they're stored in "property_ids" arrays, and invalid
IDs are "0". There is code to add properties, but I didn't find code
to remove them. But if you look at the connector code, you'll that it
is implemented in a way that if the array is something like: [
valid_id, 0, valid_id, 0, 0, 0 ... ], the code will work (even with a
zero in the middle of two valid ids). While this makes the code more
resistant to mistakes, it also makes some loops longer (you don't need
to stop at the first invalid id, you need to keep looking). And, as
far as I found, we don't ever reach the [ valid, 0, valid ] case
because we never remove properties. Even if we start removing
properties, we can try to avoid the [valid, 0, valid] case. In my
patch, this condition is not valid for the crtc properties, so we can
make loops shorter (and I see I could still improve one of the loops).
Which way do we prefer?
> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_GETPROPERTIES, drm_mode_crtc_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CRTC_SETPROPERTY, drm_mode_crtc_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED)
I'm not sure about how we want these flags. I just copied from
connector properties ioctls.
--
Paulo Zanoni
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
