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 at 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
--
Ville Syrjälä
Intel
