On Wed, May 28, 2014 at 07:57:18PM -0400, Rob Clark wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
[...]
> +struct drm_property *drm_property_create_object(struct drm_device *dev,
> + int flags, const char *name, uint32_t type)
Indentation here is inconsistent with other functions in this file.
> @@ -3294,14 +3316,16 @@ int drm_property_add_enum(struct drm_property *property, int index,
> {
> struct drm_property_enum *prop_enum;
>
> - if (!(property->flags & (DRM_MODE_PROP_ENUM | DRM_MODE_PROP_BITMASK)))
> + if (!(drm_property_type_is(property, DRM_MODE_PROP_ENUM) ||
> + drm_property_type_is(property, DRM_MODE_PROP_BITMASK)))
> return -EINVAL;
The bulk of this patch seems to be this pattern of substituting explicit
flag checks with drm_property_type_is() and that kind of hides the real
purpose of this patch. Splitting that into a separate patch would make
the addition of drm_property_create_object() more concise.
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
[...]
> @@ -964,6 +982,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev,
> struct drm_property *drm_property_create_range(struct drm_device *dev, int flags,
> const char *name,
> uint64_t min, uint64_t max);
> +struct drm_property *drm_property_create_object(struct drm_device *dev,
> + int flags, const char *name, uint32_t type);
> extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property);
> extern int drm_property_add_enum(struct drm_property *property, int index,
> uint64_t value, const char *name);
> @@ -980,6 +1000,13 @@ extern int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> int gamma_size);
> extern struct drm_mode_object *drm_mode_object_find(struct drm_device *dev,
> uint32_t id, uint32_t type);
> +
> +static inline struct drm_mode_object *
> +drm_property_get_obj(struct drm_property *property, uint64_t value)
Perhaps for consistent naming with drm_property_create_object() this
would be better called drm_property_get_object()?
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
[...]
> +/* non-extended types: legacy bitmask, one bit per type: */
> +#define DRM_MODE_PROP_LEGACY_TYPE ( \
> + DRM_MODE_PROP_RANGE | \
> + DRM_MODE_PROP_ENUM | \
> + DRM_MODE_PROP_BLOB | \
> + DRM_MODE_PROP_BITMASK)
> +
> +/* extended-types: rather than continue to consume a bit per type,
> + * grab a chunk of the bits to use as integer type id.
> + */
> +#define DRM_MODE_PROP_EXTENDED_TYPE 0x0000ffc0
> +#define DRM_MODE_PROP_TYPE(n) ((n) << 6)
> +#define DRM_MODE_PROP_OBJECT DRM_MODE_PROP_TYPE(1)
Ugh, that's an unfortunate bit of userspace ABI...
Could this perhaps be done in a more unified, yet backwards-compatible
way? For example if we define legacy properties like this:
#define DRM_MODE_PROP_TYPE(n) ((n) << 0)
#define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE( 2)
#define DRM_MODE_PROP_ENUM DRM_MODE_PROP_TYPE( 8)
#define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(16)
#define DRM_MODE_PROP_RANGE DRM_MODE_PROP_TYPE(32)
And define the type mask to be:
#define DRM_MODE_PROP_TYPE 0x0000fffa
Leaving out only the two real flags (PENDING and IMMUTABLE). This should
allow things to be done without constantly having to special case legacy
vs. extended types. It leaves a bunch of wholes in the number space and
we need to be careful to introduce new types only in the upper range,
but it has the advantage of not requiring special handling.
Thierry
Attachment:
pgp2w4Y795M3x.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
