From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 1/8] drm: add object property type Date: Fri, 30 May 2014 09:57:55 +0200 Message-ID: <20140530075754.GB24748@ulmo> References: <1401321445-31906-1-git-send-email-robdclark@gmail.com> <1401321445-31906-2-git-send-email-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0737717678==" Return-path: Received: from mail-wi0-f174.google.com (mail-wi0-f174.google.com [209.85.212.174]) by gabe.freedesktop.org (Postfix) with ESMTP id 764B26EA65 for ; Fri, 30 May 2014 01:00:31 -0700 (PDT) Received: by mail-wi0-f174.google.com with SMTP id r20so630945wiv.1 for ; Fri, 30 May 2014 01:00:28 -0700 (PDT) In-Reply-To: <1401321445-31906-2-git-send-email-robdclark@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Clark Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0737717678== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tsOsTdHNUZQcU9Ye" Content-Disposition: inline --tsOsTdHNUZQcU9Ye Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 *pr= operty, int index, > { > struct drm_property_enum *prop_enum; > =20 > - 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(stru= ct drm_device *dev, > struct drm_property *drm_property_create_range(struct drm_device *dev, i= nt 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_prop= erty *property); > extern int drm_property_add_enum(struct drm_property *property, int inde= x, > 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 *d= ev, > 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 --tsOsTdHNUZQcU9Ye Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTiDoCAAoJEN0jrNd/PrOh6WsP/RTaU6+1x54rpoBUYdr2lhku /m2BA39nY04a/RwEI+l1BGStyD096nw2MhqI/NuGCO2YB5blawUgEaT1qK4l6iPj fnoJ6KlADDNG+v/L5hOaUjbbtm8NPeYQc3MviydKGuFgfeqiRCnqJKK66aRTKiVM 1AyiAmQhVZmkSxIYFlZaqHMabZGZiVH6CZZcYtdpjJLdow0Dzmz4zi4sY0ragN9Y 1J/i8ArZnOCuPuLl+vvQIAuuh/sFWok4fgo+NW7UEDxunG5iMM2s329+mbReqbeP MjxtPRkxtSu2prZ30EfGupBdw7oZk31CVGF14ami2yKK+h58e8kdFB2cziWHNW2I EMsUILkDBZuCTEXcvkFxpFW9pKT/0Z1Jx+xkiBFlHYC/mkQ4UWZNXrH4E8IISQ77 3tr7m4uwOV5fCWbtBvlV/elaXXXVcRH934rTZ3fD6jroOZ+dK6+fW/kX/Wtv4DZw Hm12TlaIBoD+M+TFhE2odU18ulKPnBJvhiEvmAGi0B/Kny+AxEVqRsakArA7hEDS zZX1VFvZoJFonKXBZC59kyUMT59r/Zgg/qU1M07lB/l5eigX/OZ4vIkVOA4Sqa44 yylGc/T1mzXX943B/k7w6yvpS4UVEcb4rMpwdTEY8Yy4NjyHUZyDEK5chUVDAvzp /LT06A5XvV4Iueylre/n =7fI7 -----END PGP SIGNATURE----- --tsOsTdHNUZQcU9Ye-- --===============0737717678== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0737717678==--