Re: [PATCH 1/3] drm: add connector info/property for non-std displays

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 16 Oct 2017, Dave Airlie <airlied@gmail.com> wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> This adds the infrastructure needed to quirk displays
> using edid and to mark them a non-standard.
>
> A non-standard display is one which doesn't work like
> a normal rectangular monitor or requires some transformation
> of the output by the rendering process to make sense.
>
> This is meant to cover head mounted devices like HTC Vive.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 13 +++++++++++++
>  drivers/gpu/drm/drm_edid.c      |  8 ++++++--
>  include/drm/drm_connector.h     |  5 +++++
>  include/drm/drm_mode_config.h   |  7 +++++++
>  4 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 704fc8934616..50a176483619 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -234,6 +234,10 @@ int drm_connector_init(struct drm_device *dev,
>  				   config->link_status_property,
>  				   0);
>  
> +	drm_object_attach_property(&connector->base,
> +				   config->non_std_display_property,
> +				   0);
> +
>  	if (drm_core_check_feature(dev, DRIVER_ATOMIC)) {
>  		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
>  	}
> @@ -811,6 +815,11 @@ int drm_connector_create_standard_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.link_status_property = prop;
>  
> +	prop = drm_property_create_bool(dev, DRM_MODE_PROP_IMMUTABLE, "non_standard");

I'm completely lost about the rules for naming properties, and whether
we should aim to have some uniformity in the naming.

We have properties with lower case, upper case, and capitalized case. We
have properties with spaces, underscores, and hyphens. We have enum
property values with a similar mix, and the style in values doesn't even
always match the style in the property!

I share the concerns about the property name with Thierry and Daniel,
but if we decided to go with non-standard, the property could be named
"non-standard", "non standard", "non_standard", "Non-Standard",
"NON_STANDARD", and a few more combinations, and still I wouldn't be
able to say whether it's right or wrong.

Of the above, I guess I'd go for "non-standard" because it matches
English usage.

> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.non_std_display_property = prop;
> +
>  	return 0;
>  }
>  
> @@ -1194,6 +1203,10 @@ int drm_mode_connector_update_edid_property(struct drm_connector *connector,
>  	if (edid)
>  		size = EDID_LENGTH * (1 + edid->extensions);
>  
> +	drm_object_property_set_value(&connector->base,
> +				      dev->mode_config.non_std_display_property,
> +				      connector->display_info.non_std);
> +
>  	ret = drm_property_replace_global_blob(dev,
>  					       &connector->edid_blob_ptr,
>  	                                       size,
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 00ddabfbf980..4225052def37 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -82,6 +82,8 @@
>  #define EDID_QUIRK_FORCE_6BPC			(1 << 10)
>  /* Force 10bpc */
>  #define EDID_QUIRK_FORCE_10BPC			(1 << 11)
> +/* Non standard display device (i.e. HMD) */
> +#define EDID_QUIRK_NON_STD_DEVICE               (1 << 12)
>  
>  struct detailed_mode_closure {
>  	struct drm_connector *connector;
> @@ -4393,7 +4395,7 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>  }
>  
>  static void drm_add_display_info(struct drm_connector *connector,
> -				 struct edid *edid)
> +				 struct edid *edid, u32 quirks)
>  {
>  	struct drm_display_info *info = &connector->display_info;
>  
> @@ -4407,6 +4409,8 @@ static void drm_add_display_info(struct drm_connector *connector,
>  	info->max_tmds_clock = 0;
>  	info->dvi_dual = false;
>  
> +	info->non_std = !!(quirks & EDID_QUIRK_NON_STD_DEVICE);

Nitpick, the !! are unnecessary for std bools.

BR,
Jani.

> +
>  	if (edid->revision < 3)
>  		return;
>  
> @@ -4627,7 +4631,7 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
>  	 * To avoid multiple parsing of same block, lets parse that map
>  	 * from sink info, before parsing CEA modes.
>  	 */
> -	drm_add_display_info(connector, edid);
> +	drm_add_display_info(connector, edid, quirks);
>  
>  	/*
>  	 * EDID spec says modes should be preferred in this order:
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b4285c40e1e4..5c3a1667458b 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -284,6 +284,11 @@ struct drm_display_info {
>  	 * @hdmi: advance features of a HDMI sink.
>  	 */
>  	struct drm_hdmi_info hdmi;
> +
> +	/**
> +	 * @non_std: Non standard display device (HMD).
> +	 */
> +	bool non_std;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 0b4ac2ebc610..a5a44aeccff4 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -728,6 +728,13 @@ struct drm_mode_config {
>  	 */
>  	struct drm_property *suggested_y_property;
>  
> +	/**
> +	 * @non_std_display_property: Optional connector property with a hint
> +	 * that device isn't a standard display, and the console/desktop,
> +	 * should not be displayed on it.
> +	 */
> +	struct drm_property *non_std_display_property;
> +
>  	/* dumb ioctl parameters */
>  	uint32_t preferred_depth, prefer_shadow;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel