Re: [PATCH v2 1/2] drm: Introduce 'power saving policy' drm property

[Leo Li <sunpeng.li@amd.com>]

On 2024-05-22 18:05, Mario Limonciello wrote:
> The `power saving policy` DRM property is an optional property that
> can be added to a connector by a driver.
> 
> This property is for compositors to indicate intent of policy of
> whether a driver can use power saving features that may compromise
> the experience intended by the compositor.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Acked-by: Leo Li <sunpeng.li@amd.com>

Thanks!
> ---
>   drivers/gpu/drm/drm_connector.c | 46 +++++++++++++++++++++++++++++++++
>   include/drm/drm_connector.h     |  2 ++
>   include/drm/drm_mode_config.h   |  5 ++++
>   include/uapi/drm/drm_mode.h     |  7 +++++
>   4 files changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4d2df7f64dc5..088a5874c7a4 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -961,6 +961,11 @@ static const struct drm_prop_enum_list drm_scaling_mode_enum_list[] = {
>   	{ DRM_MODE_SCALE_ASPECT, "Full aspect" },
>   };
>   
> +static const struct drm_prop_enum_list drm_power_saving_policy_enum_list[] = {
> +	{ __builtin_ffs(DRM_MODE_REQUIRE_COLOR_ACCURACY) - 1, "Require color accuracy" },
> +	{ __builtin_ffs(DRM_MODE_REQUIRE_LOW_LATENCY) - 1, "Require low latency" },
> +};
> +
>   static const struct drm_prop_enum_list drm_aspect_ratio_enum_list[] = {
>   	{ DRM_MODE_PICTURE_ASPECT_NONE, "Automatic" },
>   	{ DRM_MODE_PICTURE_ASPECT_4_3, "4:3" },
> @@ -1499,6 +1504,16 @@ static const u32 dp_colorspaces =
>    *
>    *	Drivers can set up these properties by calling
>    *	drm_mode_create_tv_margin_properties().
> + * power saving policy:
> + *	This property is used to set the power saving policy for the connector.
> + *	This property is populated with a bitmask of optional requirements set
> + *	by the drm master for the drm driver to respect:
> + *	- "Require color accuracy": Disable power saving features that will
> + *	  affect color fidelity.
> + *	  For example: Hardware assisted backlight modulation.
> + *	- "Require low latency": Disable power saving features that will
> + *	  affect latency.
> + *	  For example: Panel self refresh (PSR)
>    */
>   
>   int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1963,6 +1978,37 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>   }
>   EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>   
> +/**
> + * drm_mode_create_power_saving_policy_property - create power saving policy property
> + * @dev: DRM device
> + * @supported_policies: bitmask of supported power saving policies
> + *
> + * Called by a driver the first time it's needed, must be attached to desired
> + * connectors.
> + *
> + * Returns: %0
> + */
> +int drm_mode_create_power_saving_policy_property(struct drm_device *dev,
> +						 uint64_t supported_policies)
> +{
> +	struct drm_property *power_saving;
> +
> +	if (dev->mode_config.power_saving_policy)
> +		return 0;
> +	WARN_ON((supported_policies & DRM_MODE_POWER_SAVING_POLICY_ALL) == 0);
> +
> +	power_saving =
> +		drm_property_create_bitmask(dev, 0, "power saving policy",
> +					    drm_power_saving_policy_enum_list,
> +					    ARRAY_SIZE(drm_power_saving_policy_enum_list),
> +					    supported_policies);
> +
> +	dev->mode_config.power_saving_policy = power_saving;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_power_saving_policy_property);
> +
>   /**
>    * DOC: Variable refresh properties
>    *
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fe88d7fc6b8f..b0ec2ec48de7 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -2025,6 +2025,8 @@ int drm_mode_create_dp_colorspace_property(struct drm_connector *connector,
>   					   u32 supported_colorspaces);
>   int drm_mode_create_content_type_property(struct drm_device *dev);
>   int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
> +int drm_mode_create_power_saving_policy_property(struct drm_device *dev,
> +						 uint64_t supported_policies);
>   
>   int drm_connector_set_path_property(struct drm_connector *connector,
>   				    const char *path);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 973119a9176b..32077e701e2f 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -954,6 +954,11 @@ struct drm_mode_config {
>   	 */
>   	struct drm_atomic_state *suspend_state;
>   
> +	/**
> +	 * @power_saving_policy: bitmask for power saving policy requests.
> +	 */
> +	struct drm_property *power_saving_policy;
> +
>   	const struct drm_mode_config_helper_funcs *helper_private;
>   };
>   
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 7040e7ea80c7..c2c86623552c 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -152,6 +152,13 @@ extern "C" {
>   #define DRM_MODE_SCALE_CENTER		2 /* Centered, no scaling */
>   #define DRM_MODE_SCALE_ASPECT		3 /* Full screen, preserve aspect */
>   
> +/* power saving policy options */
> +#define DRM_MODE_REQUIRE_COLOR_ACCURACY	BIT(0)	/* Compositor requires color accuracy */
> +#define DRM_MODE_REQUIRE_LOW_LATENCY	BIT(1)	/* Compositor requires low latency */
> +
> +#define DRM_MODE_POWER_SAVING_POLICY_ALL	(DRM_MODE_REQUIRE_COLOR_ACCURACY |\
> +						 DRM_MODE_REQUIRE_LOW_LATENCY)
> +
>   /* Dithering mode options */
>   #define DRM_MODE_DITHERING_OFF	0
>   #define DRM_MODE_DITHERING_ON	1