Re: [PATCH 1/8] drm: add object property type

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3630 bytes --]

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 #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel