On Wed, 2018-05-02 at 13:08 +0200, Daniel Vetter wrote: > On Wed, May 02, 2018 at 09:08:11AM +0000, Lisovskiy, Stanislav wrote: > > On Wed, 2018-05-02 at 10:15 +0200, Daniel Vetter wrote: > > > > > > > > > > On Wed, May 02, 2018 at 08:09:24AM +0000, Lisovskiy, Stanislav > > > wrote: > > > > On Mon, 2018-04-30 at 17:00 +0200, Daniel Vetter wrote: > > > > > On Fri, Apr 27, 2018 at 10:40:00PM +0300, Ville Syrjälä > > > > > wrote: > > > > > > On Mon, Apr 23, 2018 at 10:34:41AM +0300, StanLis wrote: > > > > > > > From: Stanislav Lisovskiy <stanislav.lisovs...@intel.com> > > > > > > > > > > > > > > + > > > > > > > +/** > > > > > > > + * DOC: HDMI connector properties > > > > > > > + * > > > > > > > + * content type (HDMI specific): > > > > > > > + * Indicates content type setting to be used in > > > > > > > HDMI > > > > > > > infoframes to indicate > > > > > > > + * content type for the external device, so that > > > > > > > it > > > > > > > adjusts it's display > > > > > > > + * settings accordingly. > > > > > > > + * > > > > > > > + * The value of this property can be one of the > > > > > > > following: > > > > > > > + * > > > > > > > + * - DRM_MODE_CONTENT_TYPE_NO_DATA = 0 > > > > > > > + * Content type is unknown, it > > > > > > > content(itc) > > > > > > > bit > > > > > > > is cleared. > > > > > > > + * - DRM_MODE_CONTENT_TYPE_GRAPHICS = 4 > > > > > > > + * Content type is graphics, it > > > > > > > content(itc) > > > > > > > bit > > > > > > > is set. > > > > > > > + * - DRM_MODE_CONTENT_TYPE_PHOTO = 5 > > > > > > > + * Content type is photo, itc bit is set. > > > > > > > + * - DRM_MODE_CONTENT_TYPE_CINEMA = 6 > > > > > > > + * Content type is cinema, itc bit is > > > > > > > set. > > > > > > > + * - DRM_MODE_CONTENT_TYPE_GAME = 7 > > > > > > > + * Content type is game, itc bit is set. > > > > > > > > > > > > I wonder if we're trying to document the uabi or the > > > > > > internals > > > > > > here. > > > > > > If we are interested in the uabi, then we should document > > > > > > the > > > > > > enum > > > > > > value strings here. If on the other hand we're trying to > > > > > > document > > > > > > the > > > > > > internal details then we should keep the DRM_CONTENT_TYPE_* > > > > > > stuff. > > > > > > Maybe we want both? The raw numbers I think we should just > > > > > > throw > > > > > > out. > > > > > > While they do have some specific meaning in the case > > > > > > (matching > > > > > > the > > > > > > bits > > > > > > in the infoframe), I'm not sure that's important enough to > > > > > > include > > > > > > in > > > > > > the docs. We could add a comment next to the > > > > > > DRM_MODE_CONTENT_TYPE_* > > > > > > definitions. > > > > > > > > > > > > Looks like the content protection prop is documenting the > > > > > > internals > > > > > > only > > > > > > as well. Hmm. Actually it looks like those things are > > > > > > defined > > > > > > in > > > > > > the uapi > > > > > > header. Oh and the scaling mode prop also does that. This > > > > > > is > > > > > > all > > > > > > setting > > > > > > a rather bad example for userspace. Or maybe we've given up > > > > > > on > > > > > > the > > > > > > "the > > > > > > string is the uabi" notion entirely? > > > > > > > > > > Wrt documenting uapi: That should imo also be in there, but I > > > > > realize > > > > > it > > > > > makes it a bit a mess. The kerneldoc should definitely align > > > > > with > > > > > other > > > > > property docs to make sure it all looks consistent (i.e. > > > > > enumeration > > > > > list > > > > > with the same indentation as all the other properties). > > > > > > > > > > I guess it'd be good if we can aim for "the string is the > > > > > uabi", > > > > > but > > > > > in > > > > > practice people will hardcode. For cases where this is likely > > > > > I > > > > > think > > > > > having the defines in the uapi header is probably better. > > > > > -Daniel > > > > > > > > So how should we proceed then? In fact those > > > > definitions(DRM_MODE_CONTENT_TYPE_*) are already in > > > > the uapi header(I've added them in the first patch), done > > > > same way as aspect ratio: > > > > > > > > * Picture aspect ratio options */ > > > > #define DRM_MODE_PICTURE_ASPECT_NONE 0 > > > > #define DRM_MODE_PICTURE_ASPECT_4_3 1 > > > > #define DRM_MODE_PICTURE_ASPECT_16_9 2 > > > > > > This aren't for a property, but for flags in the > > > drm_mode_modeinfo > > > structure. That means it's required to have them as #defines. > > > > Ok, honestly I thought aspect ratio is also a property, because it > > is > > created along with other drm connector properties in > > drm_connector.c: > > > > static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] > > = { > > { DRM_MODE_PICTURE_ASPECT_NONE, "Automatic" }, > > { DRM_MODE_PICTURE_ASPECT_4_3, "4:3" }, > > { DRM_MODE_PICTURE_ASPECT_16_9, "16:9" }, > > }; > > > > /** > > * drm_mode_create_aspect_ratio_property - create aspect ratio > > property > > * @dev: DRM device > > * > > * Called by a driver the first time it's needed, must be attached > > to > > desired > > * connectors. > > * > > * Returns: > > * Zero on success, negative errno on failure. > > */ > > int drm_mode_create_aspect_ratio_property(struct drm_device *dev) > > { > > if (dev->mode_config.aspect_ratio_property) > > return 0; > > > > dev->mode_config.aspect_ratio_property = > > drm_property_create_enum(dev, 0, "aspect ratio", > > drm_aspect_ratio_enum_list, > > ARRAY_SIZE(drm_aspect_ratio_enum_lis > > t)); > > > > if (dev->mode_config.aspect_ratio_property == NULL) > > return -ENOMEM; > > > > return 0; > > } > > > > And basically as I see drm_atomic_connector_set_property can set it > > either. > > Oh, oops, I totally forgot that we have this. I guess we've been bad > at > this a few times already. Other properties with hardcoded values are > DPMS, > scaling and probably a few more. > > So not sure what to do here. I guess you're current code is ok. > -Daniel
Ok, but anyway good to know how it should be at least. I guess anyway I need to throw away numbers and move to strings instead of enum definitions. -- Best Regards, Lisovskiy Stanislav _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx