Re: [PATCH 1/9] drm: Add variable refresh rate properties to DRM connector

[Manasi Navare <manasi.d.navare@intel.com>]

Thanks for the patch, I have meant to send these patches out sitting in
my internal tree but never got to it.
Find my comments below:


On Tue, Sep 11, 2018 at 12:13:25PM -0400, Nicholas Kazlauskas wrote:
> Modern monitor hardware is capable of supporting variable refresh rates
> and adaptive sync technologies. The properties for querying and
> controlling these features should be exposed on the DRM connector.
> 
> This patch introduces two new properties for variable refresh rate
> support:
> 
> - variable_refresh_capable
> - variable_refresh_enabled
> 
> These are optional properties that can be added to a DRM connector
> dynamically by using drm_connector_attach_variable_refresh_properties.
> 
> DRM drivers should set variable_refresh_capable as applicable for
> their hardware. The property variable_refresh_enabled as a userspace
> controlled option.
> 
> Change-Id: I5f60f8b57534e1d3dacda4c64c6c9106b42f4439
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
>  drivers/gpu/drm/drm_atomic.c    |  9 +++++++++
>  drivers/gpu/drm/drm_connector.c | 35 +++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     | 27 +++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d0478abc01bd..2f89ab0fac87 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1403,6 +1403,11 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
>  		state->content_type = val;
>  	} else if (property == connector->scaling_mode_property) {
>  		state->scaling_mode = val;
> +	} else if (property == connector->variable_refresh_capable_property) {
> +		DRM_DEBUG_KMS("only drivers can set variable_refresh_capable\n");

No need for this debug message here if its defined as an immutable prop

> +		return -EINVAL;
> +	} else if (property == connector->variable_refresh_enabled_property) {
> +		state->variable_refresh_enabled = val;
>  	} else if (property == connector->content_protection_property) {
>  		if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) {
>  			DRM_DEBUG_KMS("only drivers can set CP Enabled\n");
> @@ -1508,6 +1513,10 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
>  		*val = state->content_type;
>  	} else if (property == connector->scaling_mode_property) {
>  		*val = state->scaling_mode;
> +	} else if (property == connector->variable_refresh_capable_property) {
> +		*val = state->variable_refresh_capable;
> +	} else if (property == connector->variable_refresh_enabled_property) {
> +		*val = state->variable_refresh_enabled;
>  	} else if (property == connector->content_protection_property) {
>  		*val = state->content_protection;
>  	} else if (property == config->writeback_fb_id_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 6011d769d50b..37fb33fa77b9 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1250,6 +1250,41 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>  
> +/**
> + * drm_connector_attach_variable_refresh_properties - creates and attaches
> + * properties for connectors that support adaptive refresh
> + * @connector: connector to create adaptive refresh properties on
> + */
> +int drm_connector_attach_variable_refresh_properties(
> +	struct drm_connector *connector)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *prop;
> +
> +	if (!connector->variable_refresh_capable_property) {
> +		prop = drm_property_create_bool(dev, 0,

You can create this property as a DRM_MODE_PROP_IMMUTABLE property here
which would indicate that it cannot be changed by the userspace and
avoid the check later.

Manasi

> +			"variable_refresh_capable");
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->variable_refresh_capable_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +
> +	if (!connector->variable_refresh_enabled_property) {
> +		prop = drm_property_create_bool(dev, 0,
> +			"variable_refresh_enabled");
> +		if (!prop)
> +			return -ENOMEM;
> +
> +		connector->variable_refresh_enabled_property = prop;
> +		drm_object_attach_property(&connector->base, prop, 0);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_variable_refresh_properties);
> +
>  /**
>   * drm_connector_attach_scaling_mode_property - attach atomic scaling mode property
>   * @connector: connector to attach scaling mode property on.
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 97ea41dc678f..105a127e9191 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -448,6 +448,18 @@ struct drm_connector_state {
>  	 */
>  	unsigned int content_protection;
>  
> +	/**
> +	 * @variable_refresh_enabled: Connector property used to check
> +	 * if variable refresh is supported on the device.
> +	 */
> +	bool variable_refresh_capable;
> +
> +	/**
> +	 * @variable_refresh_enabled: Connector property used to check
> +	 * if variable refresh is enabled.
> +	 */
> +	bool variable_refresh_enabled;
> +
>  	/**
>  	 * @writeback_job: Writeback job for writeback connectors
>  	 *
> @@ -909,6 +921,19 @@ struct drm_connector {
>  	 */
>  	struct drm_property *scaling_mode_property;
>  
> +	/**
> +	 * @variable_refresh_capable_property: Optional property for
> +	 * querying hardware support for variable refresh.
> +	 */
> +	struct drm_property *variable_refresh_capable_property;
> +
> +	/**
> +	 * @variable_refresh_enabled_property: Optional property for
> +	 * enabling or disabling support for variable refresh
> +	 * on the connector.
> +	 */
> +	struct drm_property *variable_refresh_enabled_property;
> +
>  	/**
>  	 * @content_protection_property: DRM ENUM property for content
>  	 * protection. See drm_connector_attach_content_protection_property().
> @@ -1182,6 +1207,8 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  int drm_connector_attach_content_type_property(struct drm_connector *dev);
>  int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
>  					       u32 scaling_mode_mask);
> +int drm_connector_attach_variable_refresh_properties(
> +	struct drm_connector *connector);
>  int drm_connector_attach_content_protection_property(
>  		struct drm_connector *connector);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel