Re: [PATCH v7 1/2] drm: content-type property for HDMI connector

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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.lisovskiy@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel