| Message ID | 20180911161333.5334-2-nicholas.kazlauskas@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A DRM API for adaptive sync and variable refresh rate support | expand |
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"); > + return -EINVAL; Why is that needed? Why not just avoid exposing the vrr_enabled prop when it's not supported? > + } 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? > } 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
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.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"); > > + 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. > > > + } 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? > > > } 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
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
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.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"); > > + return -EINVAL; > > Why is that needed? Why not just avoid exposing the vrr_enabled prop > when it's not supported? > > > + } 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? > Yes I think we will need the min/max refresh rate to be exposed from the driver. Since the driver will parse the edid and monitor range descriptor information and will know the min/max variable refresh values and it needs to expose that to the userspace along with the variable_refresh_capable property. Manasi > > } 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
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.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"); >>> + 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 >
On Tue, Sep 11, 2018 at 01:36:01PM -0400, Kazlauskas, Nicholas wrote: > 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.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"); > >>> + 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. There are plenty of immutable connector properties: EDID, TILE, non-desktop, etc. I'm pretty sure there are more immutable connector properties than there are immutable properties on other types of objects. And considering immutable connector properties existed before other types of objects even had any properties I'd say it's well established by now :) > 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. 'xrandr --current' is supposed to not probe hard for new stuff. Without the --current it may or may not result in a full probe depending on the implementation in the ddx and or the kms driver. A full probe should definitely happen after a hotplug. If it's not happening then there is a bug somewhere. > > > > >> > >>> + } 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? Not really. Some vague ideas about video playback and such, and maybe just for power saving. It was just an idea that occurred to me when thinking about this stuff. Although I suppose the compositor could always throttle the flip rate using other means. And in tne end it comes down to a policy question of how the compositor would decide what the allowed range is. > > >> > >>> } 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 > >
On Tue, Sep 11, 2018 at 01:36:01PM -0400, Kazlauskas, Nicholas wrote: > 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.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"); > > > > + 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. Immutable is used for the edid property. So exactly what you want here - static data that only depends upon the plugged in monitor, which userspace isn't allowed to change. Yeah immutable is mislabelled, it just means "userspace can't change it". It's not actually for immutable stuff. The docs should explain this somewhere, if not, pls type a patch to fix this. -Daniel > > > > > > > > > > + } 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 > > >
On 09/11/2018 01:56 PM, Ville Syrjälä wrote: > On Tue, Sep 11, 2018 at 01:36:01PM -0400, Kazlauskas, Nicholas wrote: >> 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.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"); >>>>> + 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. > > There are plenty of immutable connector properties: EDID, TILE, > non-desktop, etc. I'm pretty sure there are more immutable connector > properties than there are immutable properties on other types of > objects. And considering immutable connector properties existed > before other types of objects even had any properties I'd say > it's well established by now :) I went back and checked and I guess you were right and most connector properties have this on them. Not sure what part of the code I was thinking back on. > >> 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 > > 'xrandr --current' is supposed to not probe hard for new stuff. Without > the --current it may or may not result in a full probe depending on the > implementation in the ddx and or the kms driver. A full probe should > definitely happen after a hotplug. If it's not happening then there is > a bug somewhere. This might be a bug then since I don't use the --current flag (and it exhibits the same issue with it). I just tested this again with the DRM_MODE_PROP_IMMUTABLE flag set and it shows up as 0 for monitors that support it (even though it's set in kernelspace to 1). > >> >>> >>>> >>>>> + } 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? > > Not really. Some vague ideas about video playback and such, and maybe > just for power saving. It was just an idea that occurred to me when > thinking about this stuff. Although I suppose the compositor could > always throttle the flip rate using other means. And in tne end it > comes down to a policy question of how the compositor would decide > what the allowed range is. > >> >>>> >>>>> } 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 >>> >
On 2018-09-11 01:56 PM, Ville Syrjälä wrote: > On Tue, Sep 11, 2018 at 01:36:01PM -0400, Kazlauskas, Nicholas wrote: >> 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.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"); >>>>> + 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. > > There are plenty of immutable connector properties: EDID, TILE, > non-desktop, etc. I'm pretty sure there are more immutable connector > properties than there are immutable properties on other types of > objects. And considering immutable connector properties existed > before other types of objects even had any properties I'd say > it's well established by now :) > >> 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. > > 'xrandr --current' is supposed to not probe hard for new stuff. Without > the --current it may or may not result in a full probe depending on the > implementation in the ddx and or the kms driver. A full probe should > definitely happen after a hotplug. If it's not happening then there is > a bug somewhere. > >> >>> >>>> >>>>> + } 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? > > Not really. Some vague ideas about video playback and such, and maybe > just for power saving. It was just an idea that occurred to me when > thinking about this stuff. Although I suppose the compositor could > always throttle the flip rate using other means. And in tne end it > comes down to a policy question of how the compositor would decide > what the allowed range is. > Responding here to both your and Manasi's questions. What good would exposing vmin/vmax do? The gaming case is very well served by the properties Nicholas is introducing and the video playback case would be best served with something like a target_present_time or target_frame_duration. Exposing vmin/vmax might unnecessarily tie userland to DP/HDMI spec details and complicate an already complex problem. Harry >> >>>> >>>>> } 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 >>> >
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; + } 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, + "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);
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(+)