Re: [PATCH v2 05/11] drm: create hdmi output property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, May 30, 2017 at 10:18:19PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 5/30/2017 10:06 PM, Ville Syrjälä wrote:
> > On Tue, May 30, 2017 at 05:43:44PM +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, using which, a userspace
> > I think we should add the 4:2:0 only mode support first since that
> > doesn't require new uapi. The uapi stuff probably needs more careful
> > thought as we might want to consider exposing more of the
> > colorimetry stuff supported by the HDMI infoframes, and DP MSA MISC
> > and VSC SDP.
> I was coming from the opposite school of thought. I was thinking 
> 420_only modes should
> be handled carefully, whereas 420_also doesnt need any uapi (This patch 
> series contains
> 420_also and doesnt modify the UAPI) due to following reasons:
> 
> assume there is a mode 3840x2160@60 appears to be in 2 different EDIDs, 
> in first EDID as a 420_only mode
> and in other as 420_also mode.
> - If we add a 420_also mode in the mode_list, userspace might pick it 
> for the modeset as favorite, as most of the
>   420 modes are 4k modes.
> - Now, if userspace doesn't set the hdmi_output property to YCBCR420, 
> and sends a modeset on this mode:
>       - the modeset will be successful in case of a 420_also mode, as it 
> can be supported in RGB/YUV444 also.
>       - the modeset will fail in case of 420_only mode, as this mode 
> cant be supported in any other hdmi output format.
> 
> In this case, addition of 420_only mode, in the connectors->modes list 
> should be done, only when the driver is ready to
> handle the YCBCR420 output, or we have to inform userspace about these 
> modes which need the hdmi_ouput property to
> be set with the modeset, which might in turn need an uabi.
> 
> Does it make a case ?

What I think we want is to automagically enable 4:2:0 output if
userspace picks a 4:2:0 only mode, regardless of the property value.
And in fact this way we don't even need the property to enable the
use of 4:2:0 only modes. Which is great because it means current
userspace can start to use those modes without any code changes.

> - Shashank
> >> 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.
> >>
> >> V2: Added description for the new variable to address build warning
> >>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Jose Abreu <joabreu@synopsys.com>
> >> Cc: Daniel Vetter <daniel.vetter@intel.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_atomic.c        |  2 ++
> >>   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, 61 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index e163701..8c4e040 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);
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 636e561..86b1780 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -567,6 +567,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 49cc38c..f77f283 100644
> >> --- a/include/drm/drm_connector.h
> >> +++ b/include/drm/drm_connector.h
> >> @@ -313,6 +313,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
> >> @@ -357,6 +368,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;
> >>   };
> >>   
> >>   /**
> >> @@ -976,6 +993,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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel