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

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

 



On Wed, Jun 21, 2017 at 04:04:13PM +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
> 
> 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>

Two comments on this:

- The kerneldoc for this new property is missing, see
  https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-properties for
  what that should look like. I think a new section for HDMI properties
  might be good. For the text itself just take your commit message and
  make sure it formats correctly when building the kernel documentation.

- Putting a HDMI-specific property into the set of standard properties
  feels rather wrong, we already have functions to e.g. create tv or dvi-i
  properties. I think it'd be much better to maybe have a function to
  create all the HDMI properties. I'd would be really lovely if we could
  document the other HDMI properties like broadcast mode while at it too.

- The property values should be limited to what the driver can support, I
  guess that would mean limiting the available ycbcr modes? Or does all
  our hw support all the modes, including 420 (on the sink side)?

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_atomic.c        |  4 ++++
>  drivers/gpu/drm/drm_atomic_helper.c |  4 ++++
>  drivers/gpu/drm/drm_connector.c     | 32 ++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h         | 18 ++++++++++++++++++
>  include/drm/drm_mode_config.h       |  5 +++++
>  5 files changed, 63 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 77dcef0..ea90f8e 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 86d3093..1356b3f 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 5cd61af..f3c5928 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
> @@ -789,6 +814,13 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.link_status_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;
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 1305fe9..5ba1f32 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -321,6 +321,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
> @@ -365,6 +376,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;
>  };
>  
>  /**
> @@ -993,6 +1010,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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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