On 09/11/2018 12:31 PM, Ville Syrjälä wrote:
On Tue, Sep 11, 2018 at 07:22:43PM +0300, Ville Syrjälä wrote:
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.kazlaus...@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");
+ return -EINVAL;
Why is that needed? Why not just avoid exposing the vrr_enabled prop
when it's not supported?
Ah, I guess you want to change the value after plugging in a new
display. Yeah, if we don't want userspace to have to parse the EDID I
guess we need this. But in that case it should be marked immutable,
and then you don't need any of this atomic state related code for it.
Yeah, it's for hotplugging that it works this way. The reason it's not
marked as immutable is mostly convention - the existing driver
write-only properties (with userspace queries) do the same thing.
The immutable property flag is only used for non-connector properties.
During testing I also noticed that something likes to cache the initial
default value for these immutable flagged properties for userspace
queries - so when you call xrandr you only end up seeing the default
value and not the current.
+ } else if (property == connector->variable_refresh_enabled_property) {
+ state->variable_refresh_enabled = val;
So one thing I already asked long ago is whether there would a point in
exposing some kind of min/max refresh rate range control. Never really
got an answer. Was somehting like that considered and rejected?
For the initial patchset it seemed best to keep things simple and target
the dynamic adaptation usecase with on/off toggle.
The driver having access to the full range can enable additional
functionality that can improve the user experience - a good example is
low framerate compensation.
Adding the properties to support setting the min/max could be done on
top of this, however. Driver behavior would likely need to be defined
for atomic check. The driver needs to verify with the monitor that the
ranges are valid at some point - whether it silently fails or returns an
error is an example of something to consider here.
Do you have a specific usecase in mind that needs min/max exposed to
userspace?
} 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,
+ "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
--
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
