Re: [PATCH v5 09/17] drm: create hdmi output property

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

 



On Wed, Jul 05, 2017 at 11:39:30AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 7/4/2017 9:06 PM, Daniel Vetter wrote:
> > On Tue, Jul 04, 2017 at 07:41:56PM +0530, Shashank Sharma wrote:
> > > HDMI displays can support various output types, based on
> > > the color space and subsampling type. The possible
> > > outputs from a HDMI 2.0 monitor could be:
> > >   - RGB
> > >   - YCBCR 444
> > >   - YCBCR 422
> > >   - YCBCR 420
> > > 
> > > This patch adds a drm property "hdmi_output_format", using which,
> > > a user can specify its preference, for the HDMI output type. The
> > > output type enums are similar to the mentioned outputs above. To
> > > handle various subsampling of YCBCR output types, this property
> > > allows two special cases:
> > >   - DRM_HDMI_OUTPUT_YCBCR_HQ
> > >     This indicates preferred output should be YCBCR output, with highest
> > >     subsampling rate by the source/sink, which can be typically:
> > >   	- ycbcr444
> > >   	- ycbcr422
> > >   	- ycbcr420
> > >   - DRM_HDMI_OUTPUT_YCBCR_LQ
> > >     This indicates preferred output should be YCBCR output, with lowest
> > >     subsampling rate supported by source/sink, which can be:
> > >   	- ycbcr420
> > >   	- ycbcr422
> > >   	- ycbcr444
> > > 
> > > Default value of the property is set to 0 = RGB, so no changes if you
> > > dont set the property.
> > > 
> > > PS: While doing modeset for YCBCR 420 only modes, this property is
> > >      ignored, as those timings can be supported only in YCBCR 420
> > >      output mode.
> > > 
> > > V2: Added description for the new variable to address build warning
> > > V3: Rebase
> > > V4: Rebase
> > > V5: Added get_property counterpart to fix IGT BAT failures (BAT/CI)
> > >      Danvet:
> > >      - Add documentation for the new property
> > >      - Create a sub-section for HDMI properties, and add documentation for
> > >        few more HDMI propeties. Added documentation for:
> > > 	- Broadcast RGB
> > > 	- aspect ratio
> > > 
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Jose Abreu <joabreu@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > Bunch more documentation nitpicks below. I'd personally also split up the
> > patch into documenting the existing props and adding the new format one.
> In fact thats what I would do now. I dont want HDMI 2.0 patch series to get
> blocked due to documentation, as with all the comments
> it seems like some work. Lets do it in this way:
>  - This patch series will add the documentation for hdmi_output_property
> - I will send a separate patch which will collectively add documentation for
> all standard HDMI properties.
> > Thanks, Daniel
> > 
> > > ---
> > >   Documentation/gpu/drm-kms.rst       | 12 +++++++
> > >   drivers/gpu/drm/drm_atomic.c        |  4 +++
> > >   drivers/gpu/drm/drm_atomic_helper.c |  4 +++
> > >   drivers/gpu/drm/drm_connector.c     | 69 ++++++++++++++++++++++++++++++++++++-
> > >   drivers/gpu/drm/drm_crtc_internal.h |  1 +
> > >   drivers/gpu/drm/drm_mode_config.c   |  4 +++
> > >   drivers/gpu/drm/i915/intel_modes.c  | 13 +++++++
> > >   include/drm/drm_connector.h         | 18 ++++++++++
> > >   include/drm/drm_mode_config.h       |  5 +++
> > >   9 files changed, 129 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> > > index 3072841..dcdd6ff 100644
> > > --- a/Documentation/gpu/drm-kms.rst
> > > +++ b/Documentation/gpu/drm-kms.rst
> > > @@ -508,6 +508,18 @@ Standard Connector Properties
> > >   .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > >      :doc: standard connector properties
> > > +Standard HDMI Properties
> > > +-----------------------------
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > +   :doc: hdmi_output_format
> > > +
> > > +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> > > +   :doc: aspect ratio property
> > I'd have just created 1 DOC: section titled "standard HDMI properties" and
> > listed all of them in 1 comment block. People often forget to add the
> > include stanza in the .rst files, having bigger comments helps with that.
> > 
> > But feel free to go either way.
> Yes, this would be easier for me too, but this means I will have to add all
> the documentation in one place, in one file, whether the respective
> property is created at that place or not. I am not sure if everyone would be
> ok with that during review. But if you think we should go ahead,
> thanks for easing this up for me :-).

It might result in a minor merge conflict, but nothing bad should happen
if we put all the docs into one section.

> > > +.. kernel-doc:: drivers/gpu/drm/i915/intel_modes.c
> > > +   :doc: Broadcast RGB property
> > Please no generic properties documented in driver code.
> Broadcast RGB property is created here, and also, while I was adding
> documentation for it, I realized its kept in dev_priv, which is specific to
> I915 driver, So its not generic too. I was not sure if that will be OK, if I
> add an Intel specific property's description in DRM layer ?

It should be generic, but oh well it isn't. Either document it in the
core, or we'll leave this to the future when the create helper gets
extracted to the core.

> > > +
> > >   Plane Composition Properties
> > >   ----------------------------
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 09ca662..adcb89d 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1192,6 +1192,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
> > >   		state->picture_aspect_ratio = val;
> > >   	} else if (property == connector->scaling_mode_property) {
> > >   		state->scaling_mode = val;
> > > +	} else if (property == config->hdmi_output_property) {
> > > +		state->hdmi_output = val;
> > >   	} else if (connector->funcs->atomic_set_property) {
> > >   		return connector->funcs->atomic_set_property(connector,
> > >   				state, property, val);
> > > @@ -1272,6 +1274,8 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> > >   		*val = state->picture_aspect_ratio;
> > >   	} else if (property == connector->scaling_mode_property) {
> > >   		*val = state->scaling_mode;
> > > +	} else if (property == config->hdmi_output_property) {
> > > +		*val = state->hdmi_output;
> > >   	} else if (connector->funcs->atomic_get_property) {
> > >   		return connector->funcs->atomic_get_property(connector,
> > >   				state, property, val);
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 23e4661..2e7459f 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -637,6 +637,10 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> > >   			if (old_connector_state->link_status !=
> > >   			    new_connector_state->link_status)
> > >   				new_crtc_state->connectors_changed = true;
> > > +
> > > +			if (old_connector_state->hdmi_output !=
> > > +			    new_connector_state->hdmi_output)
> > > +				new_crtc_state->connectors_changed = true;
> > >   		}
> > >   		if (funcs->atomic_check)
> > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > > index 8072e6e..8357918 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -227,6 +227,11 @@ int drm_connector_init(struct drm_device *dev,
> > >   					      config->edid_property,
> > >   					      0);
> > > +	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
> > > +		drm_object_attach_property(&connector->base,
> > > +					   config->hdmi_output_property,
> > > +					   0);
> > > +
> > >   	drm_object_attach_property(&connector->base,
> > >   				      config->dpms_property, 0);
> > > @@ -617,6 +622,26 @@ static const struct drm_prop_enum_list drm_link_status_enum_list[] = {
> > >   };
> > >   DRM_ENUM_NAME_FN(drm_get_link_status_name, drm_link_status_enum_list)
> > > +static const struct drm_prop_enum_list drm_hdmi_output_enum_list[] = {
> > > +	{ DRM_HDMI_OUTPUT_DEFAULT_RGB, "output_rgb" },
> > > +	{ DRM_HDMI_OUTPUT_YCBCR444, "output_ycbcr444" },
> > > +	{ DRM_HDMI_OUTPUT_YCBCR422, "output_ycbcr422" },
> > > +	{ DRM_HDMI_OUTPUT_YCBCR420, "output_ycbcr420" },
> > > +	{ DRM_HDMI_OUTPUT_YCBCR_HQ, "output_ycbcr_high_subsampling" },
> > > +	{ DRM_HDMI_OUTPUT_YCBCR_LQ, "output_ycbcr_low_subsampling" },
> > > +	{ DRM_HDMI_OUTPUT_INVALID, "invalid_output" },
> > > +};
> > > +
> > > +/**
> > > + * drm_get_hdmi_output_name - return a string for a given hdmi output enum
> > > + * @type: enum of output type
> > > + */
> > > +const char *drm_get_hdmi_output_name(enum drm_hdmi_output_type type)
> > > +{
> > > +	return drm_hdmi_output_enum_list[type].name;
> > > +}
> > > +EXPORT_SYMBOL(drm_get_hdmi_output_name);
> > > +
> > >   /**
> > >    * drm_display_info_set_bus_formats - set the supported bus formats
> > >    * @info: display info to store bus formats in
> > > @@ -697,7 +722,36 @@ static const struct drm_prop_enum_list drm_tv_subconnector_enum_list[] = {
> > >   	{ DRM_MODE_SUBCONNECTOR_SCART,     "SCART"     }, /* TV-out */
> > >   };
> > >   DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> > > -		 drm_tv_subconnector_enum_list)
> > > +		 drm_tv_subconnector_enum_list);
> > > +
> > > +/**
> > > + * DOC: hdmi_output_format
> > > + *
> > > + * hdmi_output_format:
> > > + *	Enum property which allows a userspace to provide its preference for a
> > > + *	HDMI output format. Standard HDMI 1.4b outputs can support RGB/YCBCR444
> > > + *	YCBCR422 output formats, whereas HDMI 2.0 outputs can additionally
> > > + *	support YCBCR420 output. Default value of the property is HDMI output
> > > + *	RGB.
> > > + *
> > > + *	A driver can attach to this property, and then handle the HDMI output
> > > + *	based on its capabilities and sink support. There are few helper
> > > + *	functions available in drm_modes.c which can help in finding the best
> > > + *	suitable output considering a sink/source/mode combination. Refer to
> > > + *	drm_modes.c:drm_display_info_hdmi_output_type()
> > > + */
> > > +int drm_connector_create_hdmi_properties(struct drm_device *dev)
> > > +{
> > > +	struct drm_property *prop;
> > > +
> > > +	prop = drm_property_create_enum(dev, 0, "hdmi_output_format",
> > > +					drm_hdmi_output_enum_list,
> > > +					ARRAY_SIZE(drm_hdmi_output_enum_list));
> > > +	if (!prop)
> > > +		return -ENOMEM;
> > > +	dev->mode_config.hdmi_output_property = prop;
> > > +	return 0;
> > > +}
> > >   /**
> > >    * DOC: standard connector properties
> > > @@ -1024,6 +1078,19 @@ int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> > >   }
> > >   EXPORT_SYMBOL(drm_connector_attach_scaling_mode_property);
> > > +
> > > +/**
> > > + * DOC: aspect ratio property
> > > + *
> > > + * aspect ratio:
> > > + *	Enum property to override the HDMI output's aspect ratio.
> > > + *	When this property is set, the aspect ratio of the frame in
> > > + *	AVI infoframes is set as per the property value. For example
> > > + *	if userspace sets the property value to DRM_MODE_PICTURE_ASPECT_4_3
> > > + *	the output aspect ratio is set to 4:3 (regardless of the PAR of mode)
> > Would be good to document all possible values. Same for the other
> > properties, plus insert a link to the function that creates the property
> > (where we have that).
> This is the part, which makes me feel that I should create this patch
> separately, as this seems like slightly bigger cleanup than
> I thought, so I dont want this to be a part of HDMI 2.0 series.

Writing good docs is hard work, but it's part of creating a new uapi :-)
But yeah you can split out the overall cleanup if you think that's easier
(it'll probably result in some fun small conflicts).

> > 
> > > + *
> > > + */
> > > +
> > >   /**
> > >    * drm_mode_create_aspect_ratio_property - create aspect ratio property
> > >    * @dev: DRM device
> > > diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h
> > > index d077c54..e6c4adc 100644
> > > --- a/drivers/gpu/drm/drm_crtc_internal.h
> > > +++ b/drivers/gpu/drm/drm_crtc_internal.h
> > > @@ -140,6 +140,7 @@ int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
> > >   				    struct drm_property *property,
> > >   				    uint64_t value);
> > >   int drm_connector_create_standard_properties(struct drm_device *dev);
> > > +int drm_connector_create_hdmi_properties(struct drm_device *dev);
> > >   const char *drm_get_connector_force_name(enum drm_connector_force force);
> > >   /* IOCTL */
> > > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> > > index d986225..3ba9262 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -210,6 +210,10 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
> > >   	if (ret)
> > >   		return ret;
> > > +	ret = drm_connector_create_hdmi_properties(dev);
> > > +	if (ret)
> > > +		return ret;
> > We still create all the property values instead of just the ones that the
> > source hw supports. E.g. aspect ratio property is only created if needed,
> > not unconditionally on all connectors.
> > 
> > Note that users see properties through xrandr and the gui screen
> > configuration tools, adding properties that never do anything is
> > confusing, more for properties where you can select invalid values.
> If I understood this properly, you want me to create the property in I915
> layer, just before attaching it to object (similar to aspect ratio and
> others)
> something like:
> intel_attach_hdmi_output_property()
> {
>       if (!mode_config->hdmi_output_property)
>                    mode_config->hdmi_output_property = create_prop();
>         now_attach_property();
> }

Yes.

> I was assuming that once other drivers move to HDMI 2.0, they might all need
> this for YCBCR420 outputs, so it would be ok to create it generically.
> But at the same time, this also seems like a good idea.

gen8 isn't going to magically support hdmi2.0. We shouldn't advertise it
to users either. Same holds for other drivers and other features. Having a
generic/shared function to create it is good, but automically creating
something that on many drivers/platforms does nothing is not good, that
only confuses users (and userspace programmers).

Cheers, Daniel

> 
> - Shashank
> > > +
> > >   	prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
> > >   					"type", drm_plane_type_enum_list,
> > >   					ARRAY_SIZE(drm_plane_type_enum_list));
> > > diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> > > index 951e834..d07de45 100644
> > > --- a/drivers/gpu/drm/i915/intel_modes.c
> > > +++ b/drivers/gpu/drm/i915/intel_modes.c
> > > @@ -104,6 +104,19 @@ static const struct drm_prop_enum_list broadcast_rgb_names[] = {
> > >   	{ INTEL_BROADCAST_RGB_LIMITED, "Limited 16:235" },
> > >   };
> > > +/**
> > > + * DOC: Broadcast RGB property
> > > + *
> > > + * Broadcast RGB:
> > > + *	Enum property to indicate RGB color range of a HDMI output.
> > > + *	For limited range RGB outputs, a remapping of pixel values from 0-255
> > > + *	should be remaped to a range of 16-235. When this property is set to
> > > + *	Limited 16:235 and CTM is set, the hardware will be programmed with the
> > > + *	result of the multiplication of CTM by the limited  range matrix, to
> > > + *	ensure the pixels normaly in the range 0..1.0 are remapped to the range
> > > + *	16/255..235/255.
> > > + *
> > > + */
> > >   void
> > >   intel_attach_broadcast_rgb_property(struct drm_connector *connector)
> > >   {
> > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > > index 4bc0882..3773600 100644
> > > --- a/include/drm/drm_connector.h
> > > +++ b/include/drm/drm_connector.h
> > > @@ -319,6 +319,17 @@ struct drm_tv_connector_state {
> > >   	unsigned int hue;
> > >   };
> > > +/* HDMI output pixel format */
> > > +enum drm_hdmi_output_type {
> > > +	DRM_HDMI_OUTPUT_DEFAULT_RGB, /* default RGB */
> > > +	DRM_HDMI_OUTPUT_YCBCR444, /* YCBCR 444 */
> > > +	DRM_HDMI_OUTPUT_YCBCR422, /* YCBCR 422 */
> > > +	DRM_HDMI_OUTPUT_YCBCR420, /* YCBCR 420 */
> > > +	DRM_HDMI_OUTPUT_YCBCR_HQ, /* Highest subsampled YUV */
> > > +	DRM_HDMI_OUTPUT_YCBCR_LQ, /* Lowest subsampled YUV */
> > > +	DRM_HDMI_OUTPUT_INVALID, /* Guess what ? */
> > > +};
> > > +
> > >   /**
> > >    * struct drm_connector_state - mutable connector state
> > >    * @connector: backpointer to the connector
> > > @@ -363,6 +374,12 @@ struct drm_connector_state {
> > >   	 * upscaling, mostly used for built-in panels.
> > >   	 */
> > >   	unsigned int scaling_mode;
> > > +
> > > +	/**
> > > +	 * @hdmi_output: Connector property to control the
> > > +	 * HDMI output mode (RGB/YCBCR444/422/420).
> > > +	 */
> > > +	enum drm_hdmi_output_type hdmi_output;
> > >   };
> > >   /**
> > > @@ -991,6 +1008,7 @@ static inline void drm_connector_unreference(struct drm_connector *connector)
> > >   const char *drm_get_connector_status_name(enum drm_connector_status status);
> > >   const char *drm_get_subpixel_order_name(enum subpixel_order order);
> > > +const char *drm_get_hdmi_output_name(enum drm_hdmi_output_type type);
> > >   const char *drm_get_dpms_name(int val);
> > >   const char *drm_get_dvi_i_subconnector_name(int val);
> > >   const char *drm_get_dvi_i_select_name(int val);
> > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> > > index 4298171..1887261 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -740,6 +740,11 @@ struct drm_mode_config {
> > >   	 * the position of the output on the host's screen.
> > >   	 */
> > >   	struct drm_property *suggested_y_property;
> > > +	/**
> > > +	 * @hdmi_output_property: output pixel format from HDMI display
> > > +	 * Default is set for RGB
> > > +	 */
> > > +	struct drm_property *hdmi_output_property;
> > >   	/* dumb ioctl parameters */
> > >   	uint32_t preferred_depth, prefer_shadow;
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux