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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux