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

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

 



Thanks for the review, Daniel.

My comments inline.

Regards
Shashank
On 6/22/2017 12:44 PM, Daniel Vetter wrote:
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.
Thanks for pointing this out, I will add the doc.
- 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.
Sure, let see see what can I do here.
- 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)?
This property is targeted at DRM layer, so naturally its for all the HWs along with Intel HW, so it serves a big range. All our HDMI 1.4 sources support RGB444, and after this series, can support YCBCR444. All HDMI 2.0 sources should support YCBCR420, but they can declare this using a bool variable which I added in patch 3 (ycbcr420_allowed) As we are targeting both HDMI 1.4 as well as HDMI 2.0 (Src and Sink), as a whole we are covering all options.

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

_______________________________________________
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