On Fri, May 30, 2014 at 3:57 AM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> 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()?
hmm, I thought I'd dropped the drm_property_get_obj() fxn and used
_object_find() directly in the one call site. We don't actually want
drm_property_get_obj() because of fb's. What that is supposed to be
is _object_find() directly in drm_property_change_is_valid() and
drm_mode_object_find() elsewhere.
so apparently I lost some patch which was supposed to be squashed back
into this one. :-/
>> 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:
I mostly went for the keep-bitmask-comparision-for-old-types out of
backwards compat paranoia.. at least if someone somehow managed to
create an enum-blob-range property, it would follow the same code
paths it did before ;-)
quite possibly overkill. But it's hidden behind helper fxns/macros
(and I was planning on doing the same on the userspace side). So meh.
BR,
-R
> #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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
