| Message ID | 20180924181537.12092-3-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 Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: > Variable refresh rate algorithms have typically been enabled only > when the display is covered by a single source of content. > > This patch introduces a new default CRTC property that helps > hint to the driver when the CRTC composition is suitable for variable > refresh rate algorithms. Userspace can set this property dynamically > as the composition changes. > > Whether the variable refresh rate algorithms are active will still > depend on the CRTC being suitable and the connector being capable > and enabled by the user for variable refresh rate support. > > It is worth noting that while the property is atomic it isn't filtered > from legacy userspace queries. This allows for Xorg userspace drivers > to implement support in non-atomic setups. > > Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 1 + > drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++ > drivers/gpu/drm/drm_crtc.c | 2 ++ > drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > include/drm/drm_crtc.h | 13 +++++++++++++ > include/drm/drm_mode_config.h | 8 ++++++++ > 6 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 3cf1aa132778..b768d397a811 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, > state->planes_changed = false; > state->connectors_changed = false; > state->color_mgmt_changed = false; > + state->variable_refresh_changed = false; Another bool just to check if one bool changed seems a bit excessive. Is there a reason you can't directly check if the other bool changed? Actually I don't understand why this per-crtc thing is being added at all. You already have the prop on the connector. Why do we want to make life more difficult by requiring the thing to be set on both the crtc and connector? > state->zpos_changed = false; > state->commit = NULL; > state->event = NULL; > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 0bb27a24a55c..32a4cf8a01c3 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > drm_property_blob_put(mode); > return ret; > + } else if (property == config->prop_variable_refresh) { > + if (state->variable_refresh != val) > + state->variable_refresh_changed = true; > + state->variable_refresh = val; > } else if (property == config->degamma_lut_property) { > ret = drm_atomic_replace_property_blob_from_id(dev, > &state->degamma_lut, > @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > *val = state->active; > else if (property == config->prop_mode_id) > *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > + else if (property == config->prop_variable_refresh) > + *val = state->variable_refresh; > else if (property == config->degamma_lut_property) > *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; > else if (property == config->ctm_property) > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 5f488aa80bcd..ca33d6fb90ac 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); > drm_object_attach_property(&crtc->base, > config->prop_out_fence_ptr, 0); > + drm_object_attach_property(&crtc->base, > + config->prop_variable_refresh, 0); > } > > return 0; > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index ee80788f2c40..1287c161d65d 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.prop_mode_id = prop; > > + prop = drm_property_create_bool(dev, 0, > + "VARIABLE_REFRESH"); > + if (!prop) > + return -ENOMEM; > + dev->mode_config.prop_variable_refresh = prop; > + > prop = drm_property_create(dev, > DRM_MODE_PROP_BLOB, > "DEGAMMA_LUT", 0); > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index b21437bc95bf..32b77f18ce6d 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -168,6 +168,12 @@ struct drm_crtc_state { > * drivers to steer the atomic commit control flow. > */ > bool color_mgmt_changed : 1; > + /** > + * @variable_refresh_changed: Variable refresh support has changed > + * on the CRTC. Used by the atomic helpers and drivers to steer the > + * atomic commit control flow. > + */ > + bool variable_refresh_changed : 1; > > /** > * @no_vblank: > @@ -290,6 +296,13 @@ struct drm_crtc_state { > */ > u32 pageflip_flags; > > + /** > + * @variable_refresh: > + * > + * Indicates whether the CRTC is suitable for variable refresh rate. > + */ > + bool variable_refresh; > + > /** > * @event: > * > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index 928e4172a0bb..1290191cd96e 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -639,6 +639,14 @@ struct drm_mode_config { > * connectors must be of and active must be set to disabled, too. > */ > struct drm_property *prop_mode_id; > + /** > + * @prop_variable_refresh: Default atomic CRTC property to indicate > + * whether the CRTC is suitable for variable refresh rate support. > + * > + * This is only an indication of support, not whether variable > + * refresh is active on the CRTC. > + */ > + struct drm_property *prop_variable_refresh; > > /** > * @dvi_i_subconnector_property: Optional DVI-I property to > -- > 2.19.0
On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: >> Variable refresh rate algorithms have typically been enabled only >> when the display is covered by a single source of content. >> >> This patch introduces a new default CRTC property that helps >> hint to the driver when the CRTC composition is suitable for variable >> refresh rate algorithms. Userspace can set this property dynamically >> as the composition changes. >> >> Whether the variable refresh rate algorithms are active will still >> depend on the CRTC being suitable and the connector being capable >> and enabled by the user for variable refresh rate support. >> >> It is worth noting that while the property is atomic it isn't filtered >> from legacy userspace queries. This allows for Xorg userspace drivers >> to implement support in non-atomic setups. >> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 1 + >> drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++ >> drivers/gpu/drm/drm_crtc.c | 2 ++ >> drivers/gpu/drm/drm_mode_config.c | 6 ++++++ >> include/drm/drm_crtc.h | 13 +++++++++++++ >> include/drm/drm_mode_config.h | 8 ++++++++ >> 6 files changed, 36 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 3cf1aa132778..b768d397a811 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, >> state->planes_changed = false; >> state->connectors_changed = false; >> state->color_mgmt_changed = false; >> + state->variable_refresh_changed = false; > > Another bool just to check if one bool changed seems a bit excessive. > Is there a reason you can't directly check if the other bool changed? It's nice for an atomic driver to be able to check if the property has changed to steer control flow. The driver could just check if the old crtc variable refresh value isn't equal to the new one itself, but there's already precedent set to provide flags like these instead. It also lets the driver change it as needed during atomic commits. You'll see many drivers making use of the other flags like connectors_changed, mode_changed, etc. > > Actually I don't understand why this per-crtc thing is being added at > all. You already have the prop on the connector. Why do we want to > make life more difficult by requiring the thing to be set on both the > crtc and connector? It doesn't make much sense without both. The user can globally enable or disable the variable_refresh_enabled on the connector. This is the user's preference. What they don't control is the variable_refresh - the content hint that userspace specifies when the CRTC contents are suitable for enabling variable refresh features (like adaptive sync). This is userspace's preference. When both preferences agree, only then will variable refresh features be enabled. The reasoning for the split is because not all content is suitable for variable refresh. Desktop environments, web browsers, etc only typically flip when needed - which will result in display flickering. The userspace integration as part of these patches demonstrates enabling variable_refresh on the CRTC selectively. > >> state->zpos_changed = false; >> state->commit = NULL; >> state->event = NULL; >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >> index 0bb27a24a55c..32a4cf8a01c3 100644 >> --- a/drivers/gpu/drm/drm_atomic_uapi.c >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, >> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >> drm_property_blob_put(mode); >> return ret; >> + } else if (property == config->prop_variable_refresh) { >> + if (state->variable_refresh != val) >> + state->variable_refresh_changed = true; >> + state->variable_refresh = val; >> } else if (property == config->degamma_lut_property) { >> ret = drm_atomic_replace_property_blob_from_id(dev, >> &state->degamma_lut, >> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, >> *val = state->active; >> else if (property == config->prop_mode_id) >> *val = (state->mode_blob) ? state->mode_blob->base.id : 0; >> + else if (property == config->prop_variable_refresh) >> + *val = state->variable_refresh; >> else if (property == config->degamma_lut_property) >> *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; >> else if (property == config->ctm_property) >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index 5f488aa80bcd..ca33d6fb90ac 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >> drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); >> drm_object_attach_property(&crtc->base, >> config->prop_out_fence_ptr, 0); >> + drm_object_attach_property(&crtc->base, >> + config->prop_variable_refresh, 0); >> } >> >> return 0; >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >> index ee80788f2c40..1287c161d65d 100644 >> --- a/drivers/gpu/drm/drm_mode_config.c >> +++ b/drivers/gpu/drm/drm_mode_config.c >> @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) >> return -ENOMEM; >> dev->mode_config.prop_mode_id = prop; >> >> + prop = drm_property_create_bool(dev, 0, >> + "VARIABLE_REFRESH"); >> + if (!prop) >> + return -ENOMEM; >> + dev->mode_config.prop_variable_refresh = prop; >> + >> prop = drm_property_create(dev, >> DRM_MODE_PROP_BLOB, >> "DEGAMMA_LUT", 0); >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index b21437bc95bf..32b77f18ce6d 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -168,6 +168,12 @@ struct drm_crtc_state { >> * drivers to steer the atomic commit control flow. >> */ >> bool color_mgmt_changed : 1; >> + /** >> + * @variable_refresh_changed: Variable refresh support has changed >> + * on the CRTC. Used by the atomic helpers and drivers to steer the >> + * atomic commit control flow. >> + */ >> + bool variable_refresh_changed : 1; >> >> /** >> * @no_vblank: >> @@ -290,6 +296,13 @@ struct drm_crtc_state { >> */ >> u32 pageflip_flags; >> >> + /** >> + * @variable_refresh: >> + * >> + * Indicates whether the CRTC is suitable for variable refresh rate. >> + */ >> + bool variable_refresh; >> + >> /** >> * @event: >> * >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >> index 928e4172a0bb..1290191cd96e 100644 >> --- a/include/drm/drm_mode_config.h >> +++ b/include/drm/drm_mode_config.h >> @@ -639,6 +639,14 @@ struct drm_mode_config { >> * connectors must be of and active must be set to disabled, too. >> */ >> struct drm_property *prop_mode_id; >> + /** >> + * @prop_variable_refresh: Default atomic CRTC property to indicate >> + * whether the CRTC is suitable for variable refresh rate support. >> + * >> + * This is only an indication of support, not whether variable >> + * refresh is active on the CRTC. >> + */ >> + struct drm_property *prop_variable_refresh; >> >> /** >> * @dvi_i_subconnector_property: Optional DVI-I property to >> -- >> 2.19.0 > Nicholas Kazlauskas
On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > > On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: > >> Variable refresh rate algorithms have typically been enabled only > >> when the display is covered by a single source of content. > >> > >> This patch introduces a new default CRTC property that helps > >> hint to the driver when the CRTC composition is suitable for variable > >> refresh rate algorithms. Userspace can set this property dynamically > >> as the composition changes. > >> > >> Whether the variable refresh rate algorithms are active will still > >> depend on the CRTC being suitable and the connector being capable > >> and enabled by the user for variable refresh rate support. > >> > >> It is worth noting that while the property is atomic it isn't filtered > >> from legacy userspace queries. This allows for Xorg userspace drivers > >> to implement support in non-atomic setups. > >> > >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 1 + > >> drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++ > >> drivers/gpu/drm/drm_crtc.c | 2 ++ > >> drivers/gpu/drm/drm_mode_config.c | 6 ++++++ > >> include/drm/drm_crtc.h | 13 +++++++++++++ > >> include/drm/drm_mode_config.h | 8 ++++++++ > >> 6 files changed, 36 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 3cf1aa132778..b768d397a811 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, > >> state->planes_changed = false; > >> state->connectors_changed = false; > >> state->color_mgmt_changed = false; > >> + state->variable_refresh_changed = false; > > > > Another bool just to check if one bool changed seems a bit excessive. > > Is there a reason you can't directly check if the other bool changed? > > It's nice for an atomic driver to be able to check if the property has > changed to steer control flow. > > The driver could just check if the old crtc variable refresh value isn't > equal to the new one itself, but there's already precedent set to > provide flags like these instead. Generally the changed flags are for more complicated things. Not sure we want to start adding them for every little thing. Though I suppose active_changed blows a hole in that theory. > > It also lets the driver change it as needed during atomic commits. > You'll see many drivers making use of the other flags like > connectors_changed, mode_changed, etc. > > > > > Actually I don't understand why this per-crtc thing is being added at > > all. You already have the prop on the connector. Why do we want to > > make life more difficult by requiring the thing to be set on both the > > crtc and connector? > > It doesn't make much sense without both. > > The user can globally enable or disable the variable_refresh_enabled on > the connector. This is the user's preference. > > What they don't control is the variable_refresh - the content hint that > userspace specifies when the CRTC contents are suitable for enabling > variable refresh features (like adaptive sync). This is userspace's > preference. By userspace I guess you mean the compositor/display server. I don't really see why the kernel has to be involved like this in a userspace policy matter. If the compositor doesn't think vrr is a good idea then it could simply choose to disable the prop on the connector even when requested by its clients. > > When both preferences agree, only then will variable refresh features be > enabled. > > The reasoning for the split is because not all content is suitable for > variable refresh. Desktop environments, web browsers, etc only typically > flip when needed - which will result in display flickering. > > The userspace integration as part of these patches demonstrates enabling > variable_refresh on the CRTC selectively. > > > > >> state->zpos_changed = false; > >> state->commit = NULL; > >> state->event = NULL; > >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > >> index 0bb27a24a55c..32a4cf8a01c3 100644 > >> --- a/drivers/gpu/drm/drm_atomic_uapi.c > >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c > >> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, > >> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); > >> drm_property_blob_put(mode); > >> return ret; > >> + } else if (property == config->prop_variable_refresh) { > >> + if (state->variable_refresh != val) > >> + state->variable_refresh_changed = true; > >> + state->variable_refresh = val; > >> } else if (property == config->degamma_lut_property) { > >> ret = drm_atomic_replace_property_blob_from_id(dev, > >> &state->degamma_lut, > >> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, > >> *val = state->active; > >> else if (property == config->prop_mode_id) > >> *val = (state->mode_blob) ? state->mode_blob->base.id : 0; > >> + else if (property == config->prop_variable_refresh) > >> + *val = state->variable_refresh; > >> else if (property == config->degamma_lut_property) > >> *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; > >> else if (property == config->ctm_property) > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index 5f488aa80bcd..ca33d6fb90ac 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, > >> drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); > >> drm_object_attach_property(&crtc->base, > >> config->prop_out_fence_ptr, 0); > >> + drm_object_attach_property(&crtc->base, > >> + config->prop_variable_refresh, 0); > >> } > >> > >> return 0; > >> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > >> index ee80788f2c40..1287c161d65d 100644 > >> --- a/drivers/gpu/drm/drm_mode_config.c > >> +++ b/drivers/gpu/drm/drm_mode_config.c > >> @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > >> return -ENOMEM; > >> dev->mode_config.prop_mode_id = prop; > >> > >> + prop = drm_property_create_bool(dev, 0, > >> + "VARIABLE_REFRESH"); > >> + if (!prop) > >> + return -ENOMEM; > >> + dev->mode_config.prop_variable_refresh = prop; > >> + > >> prop = drm_property_create(dev, > >> DRM_MODE_PROP_BLOB, > >> "DEGAMMA_LUT", 0); > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> index b21437bc95bf..32b77f18ce6d 100644 > >> --- a/include/drm/drm_crtc.h > >> +++ b/include/drm/drm_crtc.h > >> @@ -168,6 +168,12 @@ struct drm_crtc_state { > >> * drivers to steer the atomic commit control flow. > >> */ > >> bool color_mgmt_changed : 1; > >> + /** > >> + * @variable_refresh_changed: Variable refresh support has changed > >> + * on the CRTC. Used by the atomic helpers and drivers to steer the > >> + * atomic commit control flow. > >> + */ > >> + bool variable_refresh_changed : 1; > >> > >> /** > >> * @no_vblank: > >> @@ -290,6 +296,13 @@ struct drm_crtc_state { > >> */ > >> u32 pageflip_flags; > >> > >> + /** > >> + * @variable_refresh: > >> + * > >> + * Indicates whether the CRTC is suitable for variable refresh rate. > >> + */ > >> + bool variable_refresh; > >> + > >> /** > >> * @event: > >> * > >> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > >> index 928e4172a0bb..1290191cd96e 100644 > >> --- a/include/drm/drm_mode_config.h > >> +++ b/include/drm/drm_mode_config.h > >> @@ -639,6 +639,14 @@ struct drm_mode_config { > >> * connectors must be of and active must be set to disabled, too. > >> */ > >> struct drm_property *prop_mode_id; > >> + /** > >> + * @prop_variable_refresh: Default atomic CRTC property to indicate > >> + * whether the CRTC is suitable for variable refresh rate support. > >> + * > >> + * This is only an indication of support, not whether variable > >> + * refresh is active on the CRTC. > >> + */ > >> + struct drm_property *prop_variable_refresh; > >> > >> /** > >> * @dvi_i_subconnector_property: Optional DVI-I property to > >> -- > >> 2.19.0 > > > > Nicholas Kazlauskas
On 2018-09-24 10:26 p.m., Ville Syrjälä wrote: > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: >>> >>> Actually I don't understand why this per-crtc thing is being added at >>> all. You already have the prop on the connector. Why do we want to >>> make life more difficult by requiring the thing to be set on both the >>> crtc and connector? >> >> It doesn't make much sense without both. >> >> The user can globally enable or disable the variable_refresh_enabled on >> the connector. This is the user's preference. >> >> What they don't control is the variable_refresh - the content hint that >> userspace specifies when the CRTC contents are suitable for enabling >> variable refresh features (like adaptive sync). This is userspace's >> preference. > > By userspace I guess you mean the compositor/display server. Actually rather the application, see the corresponding Mesa and xf86-video-amdgpu patches. > I don't really see why the kernel has to be involved like this in a > userspace policy matter. If the compositor doesn't think vrr is a good > idea then it could simply choose to disable the prop on the connector > even when requested by its clients. Connector properties are exposed directly to X11 clients as RandR output properties. With only the connector property, the user running e.g. xrandr --output <name> --set variable_refresh_enabled 1 would result in variable refresh being enabled regardless of whether a variable refresh compatible client is currently using page flipping, which can result in flickering or getting stuck at the minimum refresh rate.
On 09/24/2018 04:26 PM, Ville Syrjälä wrote: > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: >>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: >>>> Variable refresh rate algorithms have typically been enabled only >>>> when the display is covered by a single source of content. >>>> >>>> This patch introduces a new default CRTC property that helps >>>> hint to the driver when the CRTC composition is suitable for variable >>>> refresh rate algorithms. Userspace can set this property dynamically >>>> as the composition changes. >>>> >>>> Whether the variable refresh rate algorithms are active will still >>>> depend on the CRTC being suitable and the connector being capable >>>> and enabled by the user for variable refresh rate support. >>>> >>>> It is worth noting that while the property is atomic it isn't filtered >>>> from legacy userspace queries. This allows for Xorg userspace drivers >>>> to implement support in non-atomic setups. >>>> >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> >>>> --- >>>> drivers/gpu/drm/drm_atomic_helper.c | 1 + >>>> drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++ >>>> drivers/gpu/drm/drm_crtc.c | 2 ++ >>>> drivers/gpu/drm/drm_mode_config.c | 6 ++++++ >>>> include/drm/drm_crtc.h | 13 +++++++++++++ >>>> include/drm/drm_mode_config.h | 8 ++++++++ >>>> 6 files changed, 36 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>>> index 3cf1aa132778..b768d397a811 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>>> @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, >>>> state->planes_changed = false; >>>> state->connectors_changed = false; >>>> state->color_mgmt_changed = false; >>>> + state->variable_refresh_changed = false; >>> >>> Another bool just to check if one bool changed seems a bit excessive. >>> Is there a reason you can't directly check if the other bool changed? >> >> It's nice for an atomic driver to be able to check if the property has >> changed to steer control flow. >> >> The driver could just check if the old crtc variable refresh value isn't >> equal to the new one itself, but there's already precedent set to >> provide flags like these instead. > > Generally the changed flags are for more complicated things. Not > sure we want to start adding them for every little thing. Though > I suppose active_changed blows a hole in that theory. > >> >> It also lets the driver change it as needed during atomic commits. >> You'll see many drivers making use of the other flags like >> connectors_changed, mode_changed, etc. >> >>> >>> Actually I don't understand why this per-crtc thing is being added at >>> all. You already have the prop on the connector. Why do we want to >>> make life more difficult by requiring the thing to be set on both the >>> crtc and connector? >> >> It doesn't make much sense without both. >> >> The user can globally enable or disable the variable_refresh_enabled on >> the connector. This is the user's preference. >> >> What they don't control is the variable_refresh - the content hint that >> userspace specifies when the CRTC contents are suitable for enabling >> variable refresh features (like adaptive sync). This is userspace's >> preference. > > By userspace I guess you mean the compositor/display server. I don't > really see why the kernel has to be involved like this in a userspace > policy matter. If the compositor doesn't think vrr is a good idea then > it could simply choose to disable the prop on the connector even when > requested by its clients. The properties are both a kernel and userspace API so I think it's important to think about the usecase and API usage for both. In a DRM driver the variable refresh capability will be determined by connector type and the display that's connected. The driver needs a way to track this if it's going to be sending any sort of packet over the connector. The capability property provides a method for the driver to do this while also exposing this information to the userspace for querying. The variable_refresh_enabled property exists because not all users will want this feature enabled. I think it makes sense to place this alongside the capability property on the connector because of their relation and because of the ease of access for the end user in existing userspace stacks - xrandr makes this easy. The variable_refresh CRTC property exists for a few reasons. Userspace needs a method via the atomic API to let a DRM driver know that it wants variable frame timing to be enabled. The connector enable property certainly satisfies this condition - but I think there are other to take into consideration here. Whenever I mentioned variable refresh "features", what I really meant was operating in one of two modes: (1) Letting the driver and hardware adjust refresh automatically based on the flip rate on a CRTC from a single application (2) Setting a fixed frame duration based on the flip rate on a CRTC from a single application In both cases the important thing to note is that they're both dependent on how often a CRTC is flipping. There's also the "requirement" for single application in both cases - if there are multiple applications issuing flips then the hardware can't determine the correct content rate. The resulting user experience is going to be negative as the monitor seemingly adjusts to a random rate. With the existing kernelspace and userspace stacks a DRM driver can't reasonably understand whether a single application is flipping or not and what variable refresh "mode" to operate in - these both depend on what's happening in userspace. These factors are decided in userspace when interfacing with a DRM CRTC. The userspace integration patches I've posted alongside this interface demonstrate this usage - checks are done against a CRTC to see if the application fully covers the surface and the automatic adjustment mode is only enabled for when flips are issued for the CRTC originating from that application. Determining which connectors use the CRTC can certainly be done and the property could be changed there, but I don't think this logically follows from the API usage described above. The reasoning behind this being a default property on the CRTC is that I don't think userspace should need to keep track of what's actually connected using the CRTC. A user can hotplug displays at will or enable/disable variable refresh support via their display's OSD. Capabilities can change and this is only something a driver really needs to concern themselves with - the driver is what sends the control packets to the hardware. I probably could have gone into more detail about some of this in the cover letter itself for these patchsets. These patches were actually a connector only interface originally but evolved after developing the actual implementation. > >> >> When both preferences agree, only then will variable refresh features be >> enabled. >> >> The reasoning for the split is because not all content is suitable for >> variable refresh. Desktop environments, web browsers, etc only typically >> flip when needed - which will result in display flickering. >> >> The userspace integration as part of these patches demonstrates enabling >> variable_refresh on the CRTC selectively. >> >>> >>>> state->zpos_changed = false; >>>> state->commit = NULL; >>>> state->event = NULL; >>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c >>>> index 0bb27a24a55c..32a4cf8a01c3 100644 >>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c >>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c >>>> @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, >>>> ret = drm_atomic_set_mode_prop_for_crtc(state, mode); >>>> drm_property_blob_put(mode); >>>> return ret; >>>> + } else if (property == config->prop_variable_refresh) { >>>> + if (state->variable_refresh != val) >>>> + state->variable_refresh_changed = true; >>>> + state->variable_refresh = val; >>>> } else if (property == config->degamma_lut_property) { >>>> ret = drm_atomic_replace_property_blob_from_id(dev, >>>> &state->degamma_lut, >>>> @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, >>>> *val = state->active; >>>> else if (property == config->prop_mode_id) >>>> *val = (state->mode_blob) ? state->mode_blob->base.id : 0; >>>> + else if (property == config->prop_variable_refresh) >>>> + *val = state->variable_refresh; >>>> else if (property == config->degamma_lut_property) >>>> *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; >>>> else if (property == config->ctm_property) >>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >>>> index 5f488aa80bcd..ca33d6fb90ac 100644 >>>> --- a/drivers/gpu/drm/drm_crtc.c >>>> +++ b/drivers/gpu/drm/drm_crtc.c >>>> @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >>>> drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); >>>> drm_object_attach_property(&crtc->base, >>>> config->prop_out_fence_ptr, 0); >>>> + drm_object_attach_property(&crtc->base, >>>> + config->prop_variable_refresh, 0); >>>> } >>>> >>>> return 0; >>>> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >>>> index ee80788f2c40..1287c161d65d 100644 >>>> --- a/drivers/gpu/drm/drm_mode_config.c >>>> +++ b/drivers/gpu/drm/drm_mode_config.c >>>> @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) >>>> return -ENOMEM; >>>> dev->mode_config.prop_mode_id = prop; >>>> >>>> + prop = drm_property_create_bool(dev, 0, >>>> + "VARIABLE_REFRESH"); >>>> + if (!prop) >>>> + return -ENOMEM; >>>> + dev->mode_config.prop_variable_refresh = prop; >>>> + >>>> prop = drm_property_create(dev, >>>> DRM_MODE_PROP_BLOB, >>>> "DEGAMMA_LUT", 0); >>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >>>> index b21437bc95bf..32b77f18ce6d 100644 >>>> --- a/include/drm/drm_crtc.h >>>> +++ b/include/drm/drm_crtc.h >>>> @@ -168,6 +168,12 @@ struct drm_crtc_state { >>>> * drivers to steer the atomic commit control flow. >>>> */ >>>> bool color_mgmt_changed : 1; >>>> + /** >>>> + * @variable_refresh_changed: Variable refresh support has changed >>>> + * on the CRTC. Used by the atomic helpers and drivers to steer the >>>> + * atomic commit control flow. >>>> + */ >>>> + bool variable_refresh_changed : 1; >>>> >>>> /** >>>> * @no_vblank: >>>> @@ -290,6 +296,13 @@ struct drm_crtc_state { >>>> */ >>>> u32 pageflip_flags; >>>> >>>> + /** >>>> + * @variable_refresh: >>>> + * >>>> + * Indicates whether the CRTC is suitable for variable refresh rate. >>>> + */ >>>> + bool variable_refresh; >>>> + >>>> /** >>>> * @event: >>>> * >>>> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >>>> index 928e4172a0bb..1290191cd96e 100644 >>>> --- a/include/drm/drm_mode_config.h >>>> +++ b/include/drm/drm_mode_config.h >>>> @@ -639,6 +639,14 @@ struct drm_mode_config { >>>> * connectors must be of and active must be set to disabled, too. >>>> */ >>>> struct drm_property *prop_mode_id; >>>> + /** >>>> + * @prop_variable_refresh: Default atomic CRTC property to indicate >>>> + * whether the CRTC is suitable for variable refresh rate support. >>>> + * >>>> + * This is only an indication of support, not whether variable >>>> + * refresh is active on the CRTC. >>>> + */ >>>> + struct drm_property *prop_variable_refresh; >>>> >>>> /** >>>> * @dvi_i_subconnector_property: Optional DVI-I property to >>>> -- >>>> 2.19.0 >>> >> >> Nicholas Kazlauskas > Nicholas Kazlauskas
On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote: > On 2018-09-24 10:26 p.m., Ville Syrjälä wrote: > > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > >>> > >>> Actually I don't understand why this per-crtc thing is being added at > >>> all. You already have the prop on the connector. Why do we want to > >>> make life more difficult by requiring the thing to be set on both the > >>> crtc and connector? > >> > >> It doesn't make much sense without both. > >> > >> The user can globally enable or disable the variable_refresh_enabled on > >> the connector. This is the user's preference. > >> > >> What they don't control is the variable_refresh - the content hint that > >> userspace specifies when the CRTC contents are suitable for enabling > >> variable refresh features (like adaptive sync). This is userspace's > >> preference. > > > > By userspace I guess you mean the compositor/display server. > > Actually rather the application, see the corresponding Mesa and > xf86-video-amdgpu patches. > > > > I don't really see why the kernel has to be involved like this in a > > userspace policy matter. If the compositor doesn't think vrr is a good > > idea then it could simply choose to disable the prop on the connector > > even when requested by its clients. > > Connector properties are exposed directly to X11 clients as RandR output > properties. With only the connector property, the user running e.g. > > xrandr --output <name> --set variable_refresh_enabled 1 > > would result in variable refresh being enabled regardless of whether a > variable refresh compatible client is currently using page flipping, > which can result in flickering or getting stuck at the minimum refresh rate. in ddx: configure_vrr() { kms_vrr = client_vrr && is_vrr_a_good_idea(); kms_setprop(kms_vrr); } set_prop() { if (is_vrr_prop) configure_vrr(); else kms_setprop(); } other_stuff() { /* some stuff */ ... if (vrr_related_stuff_changed) configure_vrr(); } or something along those lines?
On 2018-09-25 4:04 p.m., Ville Syrjälä wrote: > On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote: >> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote: >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: >>>>> >>>>> Actually I don't understand why this per-crtc thing is being added at >>>>> all. You already have the prop on the connector. Why do we want to >>>>> make life more difficult by requiring the thing to be set on both the >>>>> crtc and connector? >>>> >>>> It doesn't make much sense without both. >>>> >>>> The user can globally enable or disable the variable_refresh_enabled on >>>> the connector. This is the user's preference. >>>> >>>> What they don't control is the variable_refresh - the content hint that >>>> userspace specifies when the CRTC contents are suitable for enabling >>>> variable refresh features (like adaptive sync). This is userspace's >>>> preference. >>> >>> By userspace I guess you mean the compositor/display server. >> >> Actually rather the application, see the corresponding Mesa and >> xf86-video-amdgpu patches. >> >> >>> I don't really see why the kernel has to be involved like this in a >>> userspace policy matter. If the compositor doesn't think vrr is a good >>> idea then it could simply choose to disable the prop on the connector >>> even when requested by its clients. >> >> Connector properties are exposed directly to X11 clients as RandR output >> properties. With only the connector property, the user running e.g. >> >> xrandr --output <name> --set variable_refresh_enabled 1 >> >> would result in variable refresh being enabled regardless of whether a >> variable refresh compatible client is currently using page flipping, >> which can result in flickering or getting stuck at the minimum refresh rate. > > in ddx: > > configure_vrr() > { > kms_vrr = client_vrr && is_vrr_a_good_idea(); > kms_setprop(kms_vrr); > } > > set_prop() > { > if (is_vrr_prop) > configure_vrr(); > else > kms_setprop(); > } > > other_stuff() > { > /* some stuff */ > ... > > if (vrr_related_stuff_changed) > configure_vrr(); > } > > or something along those lines? > Sure, that's not the problem, which is that if the Xorg driver doesn't actively hide the property from clients (e.g. because it's a currently released version), we get the issue I described above. Also, if the Xorg driver were to hide the connector property from clients, there's no way for the user to completely disable variable refresh for a display (unless the Xorg driver exposes another fake property for that, which seems a bit silly).
On Tue, Sep 25, 2018 at 04:35:59PM +0200, Michel Dänzer wrote: > On 2018-09-25 4:04 p.m., Ville Syrjälä wrote: > > On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote: > >> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote: > >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > >>>>> > >>>>> Actually I don't understand why this per-crtc thing is being added at > >>>>> all. You already have the prop on the connector. Why do we want to > >>>>> make life more difficult by requiring the thing to be set on both the > >>>>> crtc and connector? > >>>> > >>>> It doesn't make much sense without both. > >>>> > >>>> The user can globally enable or disable the variable_refresh_enabled on > >>>> the connector. This is the user's preference. > >>>> > >>>> What they don't control is the variable_refresh - the content hint that > >>>> userspace specifies when the CRTC contents are suitable for enabling > >>>> variable refresh features (like adaptive sync). This is userspace's > >>>> preference. > >>> > >>> By userspace I guess you mean the compositor/display server. > >> > >> Actually rather the application, see the corresponding Mesa and > >> xf86-video-amdgpu patches. > >> > >> > >>> I don't really see why the kernel has to be involved like this in a > >>> userspace policy matter. If the compositor doesn't think vrr is a good > >>> idea then it could simply choose to disable the prop on the connector > >>> even when requested by its clients. > >> > >> Connector properties are exposed directly to X11 clients as RandR output > >> properties. With only the connector property, the user running e.g. > >> > >> xrandr --output <name> --set variable_refresh_enabled 1 > >> > >> would result in variable refresh being enabled regardless of whether a > >> variable refresh compatible client is currently using page flipping, > >> which can result in flickering or getting stuck at the minimum refresh rate. > > > > in ddx: > > > > configure_vrr() > > { > > kms_vrr = client_vrr && is_vrr_a_good_idea(); > > kms_setprop(kms_vrr); > > } > > > > set_prop() > > { > > if (is_vrr_prop) > > configure_vrr(); > > else > > kms_setprop(); > > } > > > > other_stuff() > > { > > /* some stuff */ > > ... > > > > if (vrr_related_stuff_changed) > > configure_vrr(); > > } > > > > or something along those lines? > > > > Sure, that's not the problem, which is that if the Xorg driver doesn't > actively hide the property from clients (e.g. because it's a currently > released version), we get the issue I described above. That sounds more like what client caps were meant for. Or maybe drop the connector vrr enable prop and just go with the crtc one? > > Also, if the Xorg driver were to hide the connector property from > clients, there's no way for the user to completely disable variable > refresh for a display (unless the Xorg driver exposes another fake > property for that, which seems a bit silly). > > > -- > Earthling Michel Dänzer | http://www.amd.com > Libre software enthusiast | Mesa and X developer
On Tue, Sep 25, 2018 at 06:28:17PM +0300, Ville Syrjälä wrote: > On Tue, Sep 25, 2018 at 04:35:59PM +0200, Michel Dänzer wrote: > > On 2018-09-25 4:04 p.m., Ville Syrjälä wrote: > > > On Tue, Sep 25, 2018 at 03:28:28PM +0200, Michel Dänzer wrote: > > >> On 2018-09-24 10:26 p.m., Ville Syrjälä wrote: > > >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > > >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > > >>>>> > > >>>>> Actually I don't understand why this per-crtc thing is being added at > > >>>>> all. You already have the prop on the connector. Why do we want to > > >>>>> make life more difficult by requiring the thing to be set on both the > > >>>>> crtc and connector? > > >>>> > > >>>> It doesn't make much sense without both. > > >>>> > > >>>> The user can globally enable or disable the variable_refresh_enabled on > > >>>> the connector. This is the user's preference. > > >>>> > > >>>> What they don't control is the variable_refresh - the content hint that > > >>>> userspace specifies when the CRTC contents are suitable for enabling > > >>>> variable refresh features (like adaptive sync). This is userspace's > > >>>> preference. > > >>> > > >>> By userspace I guess you mean the compositor/display server. > > >> > > >> Actually rather the application, see the corresponding Mesa and > > >> xf86-video-amdgpu patches. > > >> > > >> > > >>> I don't really see why the kernel has to be involved like this in a > > >>> userspace policy matter. If the compositor doesn't think vrr is a good > > >>> idea then it could simply choose to disable the prop on the connector > > >>> even when requested by its clients. > > >> > > >> Connector properties are exposed directly to X11 clients as RandR output > > >> properties. With only the connector property, the user running e.g. > > >> > > >> xrandr --output <name> --set variable_refresh_enabled 1 > > >> > > >> would result in variable refresh being enabled regardless of whether a > > >> variable refresh compatible client is currently using page flipping, > > >> which can result in flickering or getting stuck at the minimum refresh rate. > > > > > > in ddx: > > > > > > configure_vrr() > > > { > > > kms_vrr = client_vrr && is_vrr_a_good_idea(); > > > kms_setprop(kms_vrr); > > > } > > > > > > set_prop() > > > { > > > if (is_vrr_prop) > > > configure_vrr(); > > > else > > > kms_setprop(); > > > } > > > > > > other_stuff() > > > { > > > /* some stuff */ > > > ... > > > > > > if (vrr_related_stuff_changed) > > > configure_vrr(); > > > } > > > > > > or something along those lines? > > > > > > > Sure, that's not the problem, which is that if the Xorg driver doesn't > > actively hide the property from clients (e.g. because it's a currently > > released version), we get the issue I described above. > > That sounds more like what client caps were meant for. Or maybe > drop the connector vrr enable prop and just go with the crtc one? > > > Also, if the Xorg driver were to hide the connector property from > > clients, there's no way for the user to completely disable variable > > refresh for a display (unless the Xorg driver exposes another fake > > property for that, which seems a bit silly). Even easier solution: Only add the enable knob on the CRTC. The CRTC is the place where we set the timing information (through the mode), not the connector. Splitting timing information across crtc and connector doesn't make sense to me, especially since you can have (in theory at least) multiple connectors. And once it's at the CRTC, you don't have the forward-compat issue anymore, it's up to the DDX to decide how/where exactly to expose this knob. Simplest (because no one loves to extend xproto) seems indeed to be to add it as an xrandr connector prop. But just because that's the reasonable solution at the xrandr level doesn't mean it's a great at idea at the kms api level. -Daniel
Hi, I have a somewhat of my own view on what would be involved with VRR, and I'd like to hear what you think of it. Comments inline. On Tue, 25 Sep 2018 09:51:37 -0400 "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > On 09/24/2018 04:26 PM, Ville Syrjälä wrote: > > On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > >> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > >>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: > >>>> Variable refresh rate algorithms have typically been enabled only > >>>> when the display is covered by a single source of content. > >>>> > >>>> This patch introduces a new default CRTC property that helps > >>>> hint to the driver when the CRTC composition is suitable for variable > >>>> refresh rate algorithms. Userspace can set this property dynamically > >>>> as the composition changes. > >>>> > >>>> Whether the variable refresh rate algorithms are active will still > >>>> depend on the CRTC being suitable and the connector being capable > >>>> and enabled by the user for variable refresh rate support. > >>>> > >>>> It is worth noting that while the property is atomic it isn't filtered > >>>> from legacy userspace queries. This allows for Xorg userspace drivers > >>>> to implement support in non-atomic setups. > >>>> > >>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> ... > >>> Actually I don't understand why this per-crtc thing is being added at > >>> all. You already have the prop on the connector. Why do we want to > >>> make life more difficult by requiring the thing to be set on both the > >>> crtc and connector? > >> > >> It doesn't make much sense without both. > >> > >> The user can globally enable or disable the variable_refresh_enabled on > >> the connector. This is the user's preference. > >> > >> What they don't control is the variable_refresh - the content hint that > >> userspace specifies when the CRTC contents are suitable for enabling > >> variable refresh features (like adaptive sync). This is userspace's > >> preference. > > > > By userspace I guess you mean the compositor/display server. I don't > > really see why the kernel has to be involved like this in a userspace > > policy matter. If the compositor doesn't think vrr is a good idea then > > it could simply choose to disable the prop on the connector even when > > requested by its clients. > > The properties are both a kernel and userspace API so I think it's > important to think about the usecase and API usage for both. > > In a DRM driver the variable refresh capability will be determined by > connector type and the display that's connected. The driver needs a way > to track this if it's going to be sending any sort of packet over the > connector. The capability property provides a method for the driver to > do this while also exposing this information to the userspace for querying. > > The variable_refresh_enabled property exists because not all users will > want this feature enabled. I think it makes sense to place this > alongside the capability property on the connector because of their > relation and because of the ease of access for the end user in existing > userspace stacks - xrandr makes this easy. I'm not sure I understood your intention here. Do you mean that you expect games (e.g. via Mesa) to toggle the connector property via RandR to tell if they wish for VRR? The RandR interface is for display configuration utilities, like the UIs that desktop environments use to configure monitors. It should not be used by applications like games to modify anything. I believe there are lots of apps out there that enrage users by abusing RandR, but one should not base an interface design on such abuse. If games are not using RandR to communicate the wish for VRR, what do they use? Does the X11 Present extension have something for it? What is the application-facing API to request VRR, is it in GLX, EGL, Vulkan, something else? Btw. I would not expect xrandr to qualify as proper userspace to validate new kernel UABI, like new properties. Did you have something else for the userspace settable connector property? > The variable_refresh CRTC property exists for a few reasons. > > Userspace needs a method via the atomic API to let a DRM driver know > that it wants variable frame timing to be enabled. The connector enable > property certainly satisfies this condition - but I think there are > other to take into consideration here. I agree that this CRTC property makes sense. > Whenever I mentioned variable refresh "features", what I really meant > was operating in one of two modes: > > (1) Letting the driver and hardware adjust refresh automatically based > on the flip rate on a CRTC from a single application > > (2) Setting a fixed frame duration based on the flip rate on a CRTC from > a single application I wonder if that's too much magic in the kernel... what would be wrong with simply flipping ASAP when VRR is active? How will userspace be able to predict coming flip opportunities if the kernel does so much magic? > In both cases the important thing to note is that they're both dependent > on how often a CRTC is flipping. There's also the "requirement" for > single application in both cases - if there are multiple applications > issuing flips then the hardware can't determine the correct content > rate. The resulting user experience is going to be negative as the > monitor seemingly adjusts to a random rate. > > With the existing kernelspace and userspace stacks a DRM driver can't > reasonably understand whether a single application is flipping or not > and what variable refresh "mode" to operate in - these both depend on > what's happening in userspace. > > These factors are decided in userspace when interfacing with a DRM CRTC. > The userspace integration patches I've posted alongside this interface > demonstrate this usage - checks are done against a CRTC to see if the > application fully covers the surface and the automatic adjustment mode > is only enabled for when flips are issued for the CRTC originating from > that application. > > Determining which connectors use the CRTC can certainly be done and the > property could be changed there, but I don't think this logically > follows from the API usage described above. > > The reasoning behind this being a default property on the CRTC is that I > don't think userspace should need to keep track of what's actually > connected using the CRTC. A user can hotplug displays at will or > enable/disable variable refresh support via their display's OSD. > Capabilities can change and this is only something a driver really needs > to concern themselves with - the driver is what sends the control > packets to the hardware. Userspace must already track carefully what is connected and what is driven through which CRTC, otherwise it cannot drive the KMS API correctly. > I probably could have gone into more detail about some of this in the > cover letter itself for these patchsets. These patches were actually a > connector only interface originally but evolved after developing the > actual implementation. > > > > >> > >> When both preferences agree, only then will variable refresh features be > >> enabled. > >> > >> The reasoning for the split is because not all content is suitable for > >> variable refresh. Desktop environments, web browsers, etc only typically > >> flip when needed - which will result in display flickering. Flickering? What do you mean? > >> > >> The userspace integration as part of these patches demonstrates enabling > >> variable_refresh on the CRTC selectively. I believe that VRR on X11 would probably depend on these bits: 1. Hardware capability Whether the monitor, the CRTC hardware, and the kernel driver all agree that VRR is possible. 2. User preference A toggle on the desktop environment's display settings application to allow or disallow VRR. After all, video mode configuration is here too, and ideally applications should not mess with the video mode directly. 3. Application preference Whether the application, e.g. a game, is prepared to deal with VRR and wishes it to be enabled. 4. X server scenegraph Which windows happen to be showing on the VRR-capable output and does that scenegraph allow e.g. flipping straight to client buffers instead of having the X server copy into framebuffers of its own. You mentioned that VRR probably doesn't make sense if there are multiple windows showing in the same output, so point 4 would cover that. Also games etc. would aim to hit the direct scanout path to avoid wasting time in the X server copy, so it seems that requiring flips to client buffers would be reasonable for enabling VRR. Sounds like that is exactly how you implemented it in xf86-video-amdgpu, isn't it? Skimming through your proposal, it seems you have covered 3 out of 4 points. Did you or I miss something? You cannot have points 2 and 3 fight over the control of the same property. How do you envision VRR to interact with X11 compositing managers? I presume compositing managers aim for the direct scanout path in the X server to avoid yet another copy there, right? Does point 4 need to exclude the Composite Overlay Window (COW) from allowed VRR windows? Given all the above, I would like to question the choice of trying to accommodate X11 interfaces directly in the DRM UABI. There seems to be quite many bits controlled by different processes and VRR should be active only when they all agree. Therefore, if you want end user applications to rely on straight connector property pass-through via RandR, you end up with more properties than strictly necessary, complicating the UABI. In other words, I agree with the criticism here by Ville and Daniel. On the other hand, in Wayland architecture, there is only one single userspace process you need to consider in the DRM UABI: the display server: - There is no separate compositing manager process. - There is no separate application to handle user preference (point 2); well, not in the sense that you would need to consider it in the DRM UABI, because it will go through the display server that is part of the desktop rather than bypassing the desktop. - There is no DRM property pass-through to end-user applications, instead there will be dedicated protocol extensions to let the display server make the final call. Therefore, it would be fine to have just two bits: 1. Hardware capability Whether the monitor, the CRTC hardware, and the kernel driver all agree that VRR is possible. 2. - 4. Whether userspace (i.e. the display server) wants to use VRR or not. This would be the CRTC property. I also believe that it would be useful to expose vmin/vmax to userspace as properties, so that display servers and apps can plan ahead on when they will render. I suppose that can be left for later when someone starts working on userspace taking advantage of it, so that the proper userspace requirement for new UABI is fulfilled. Thanks, pq
On 10/05/2018 04:10 AM, Pekka Paalanen wrote: > Hi, > > I have a somewhat of my own view on what would be involved with VRR, > and I'd like to hear what you think of it. Comments inline. > > > On Tue, 25 Sep 2018 09:51:37 -0400 > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >> On 09/24/2018 04:26 PM, Ville Syrjälä wrote: >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: >>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: >>>>>> Variable refresh rate algorithms have typically been enabled only >>>>>> when the display is covered by a single source of content. >>>>>> >>>>>> This patch introduces a new default CRTC property that helps >>>>>> hint to the driver when the CRTC composition is suitable for variable >>>>>> refresh rate algorithms. Userspace can set this property dynamically >>>>>> as the composition changes. >>>>>> >>>>>> Whether the variable refresh rate algorithms are active will still >>>>>> depend on the CRTC being suitable and the connector being capable >>>>>> and enabled by the user for variable refresh rate support. >>>>>> >>>>>> It is worth noting that while the property is atomic it isn't filtered >>>>>> from legacy userspace queries. This allows for Xorg userspace drivers >>>>>> to implement support in non-atomic setups. >>>>>> >>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > > ... > >>>>> Actually I don't understand why this per-crtc thing is being added at >>>>> all. You already have the prop on the connector. Why do we want to >>>>> make life more difficult by requiring the thing to be set on both the >>>>> crtc and connector? >>>> >>>> It doesn't make much sense without both. >>>> >>>> The user can globally enable or disable the variable_refresh_enabled on >>>> the connector. This is the user's preference. >>>> >>>> What they don't control is the variable_refresh - the content hint that >>>> userspace specifies when the CRTC contents are suitable for enabling >>>> variable refresh features (like adaptive sync). This is userspace's >>>> preference. >>> >>> By userspace I guess you mean the compositor/display server. I don't >>> really see why the kernel has to be involved like this in a userspace >>> policy matter. If the compositor doesn't think vrr is a good idea then >>> it could simply choose to disable the prop on the connector even when >>> requested by its clients. >> >> The properties are both a kernel and userspace API so I think it's >> important to think about the usecase and API usage for both. >> >> In a DRM driver the variable refresh capability will be determined by >> connector type and the display that's connected. The driver needs a way >> to track this if it's going to be sending any sort of packet over the >> connector. The capability property provides a method for the driver to >> do this while also exposing this information to the userspace for querying. >> >> The variable_refresh_enabled property exists because not all users will >> want this feature enabled. I think it makes sense to place this >> alongside the capability property on the connector because of their >> relation and because of the ease of access for the end user in existing >> userspace stacks - xrandr makes this easy. > > I'm not sure I understood your intention here. Do you mean that you > expect games (e.g. via Mesa) to toggle the connector property via RandR > to tell if they wish for VRR? > > The RandR interface is for display configuration utilities, like the UIs > that desktop environments use to configure monitors. It should not be > used by applications like games to modify anything. I believe there are > lots of apps out there that enrage users by abusing RandR, but one > should not base an interface design on such abuse. > > If games are not using RandR to communicate the wish for VRR, what do > they use? Does the X11 Present extension have something for it? > > What is the application-facing API to request VRR, is it in GLX, EGL, > Vulkan, something else? > > Btw. I would not expect xrandr to qualify as proper userspace to > validate new kernel UABI, like new properties. Did you have something > else for the userspace settable connector property? The usecase I was considering with variable_refresh_enabled was a graphical UI in a settings app within desktop environment after they've checked variable_refresh_capable. I was thinking of this as a sort of "standard" way of configuring whether the user wants VRR enabled but you're right about xrandr not really demonstrating this. Every desktop environment already has their own way of dealing with configuration and persisting settings so this doesn't bring any benefit in that regard. I'm almost finished writing up v3s that should address the concerns that Ville, Daniel and you have raised regarding this. They drop the "variable_refresh_enabled" property on the connector. > >> The variable_refresh CRTC property exists for a few reasons. >> >> Userspace needs a method via the atomic API to let a DRM driver know >> that it wants variable frame timing to be enabled. The connector enable >> property certainly satisfies this condition - but I think there are >> other to take into consideration here. > > I agree that this CRTC property makes sense. > >> Whenever I mentioned variable refresh "features", what I really meant >> was operating in one of two modes: >> >> (1) Letting the driver and hardware adjust refresh automatically based >> on the flip rate on a CRTC from a single application >> >> (2) Setting a fixed frame duration based on the flip rate on a CRTC from >> a single application > > I wonder if that's too much magic in the kernel... what would be wrong > with simply flipping ASAP when VRR is active? > > How will userspace be able to predict coming flip opportunities if the > kernel does so much magic? The kernel driver doesn't need to do much more than let the hardware know the variable refresh range. The "magic" is performed by hardware. Most games would like to render as fast as possible to deliver a more responsive and smoother image to the user. Many of these are also resource intensive and won't always be able to render at the fixed refresh rate of the panel (especially for higher refresh rates like 144Hz). The user will experience stuttering if the game takes too long to render and misses the vblank window for the flip. Dynamic VRR adjustment can resolve this problem. The hardware can lower the refresh rate and increase the vblank window in response to this so the user doesn't experience stuttering (or latency). Userspace shouldn't predict anything. > >> In both cases the important thing to note is that they're both dependent >> on how often a CRTC is flipping. There's also the "requirement" for >> single application in both cases - if there are multiple applications >> issuing flips then the hardware can't determine the correct content >> rate. The resulting user experience is going to be negative as the >> monitor seemingly adjusts to a random rate. >> >> With the existing kernelspace and userspace stacks a DRM driver can't >> reasonably understand whether a single application is flipping or not >> and what variable refresh "mode" to operate in - these both depend on >> what's happening in userspace. >> >> These factors are decided in userspace when interfacing with a DRM CRTC. >> The userspace integration patches I've posted alongside this interface >> demonstrate this usage - checks are done against a CRTC to see if the >> application fully covers the surface and the automatic adjustment mode >> is only enabled for when flips are issued for the CRTC originating from >> that application. >> >> Determining which connectors use the CRTC can certainly be done and the >> property could be changed there, but I don't think this logically >> follows from the API usage described above. >> >> The reasoning behind this being a default property on the CRTC is that I >> don't think userspace should need to keep track of what's actually >> connected using the CRTC. A user can hotplug displays at will or >> enable/disable variable refresh support via their display's OSD. >> Capabilities can change and this is only something a driver really needs >> to concern themselves with - the driver is what sends the control >> packets to the hardware. > > Userspace must already track carefully what is connected and what is > driven through which CRTC, otherwise it cannot drive the KMS API > correctly. > >> I probably could have gone into more detail about some of this in the >> cover letter itself for these patchsets. These patches were actually a >> connector only interface originally but evolved after developing the >> actual implementation. >> >>> >>>> >>>> When both preferences agree, only then will variable refresh features be >>>> enabled. >>>> >>>> The reasoning for the split is because not all content is suitable for >>>> variable refresh. Desktop environments, web browsers, etc only typically >>>> flip when needed - which will result in display flickering. > > Flickering? What do you mean? This is a property of how panels work. The luminance for a panel will vary based on how long the vrefresh is. Since the vrefresh length is changing as part of VRR you're more likely to notice the difference in luminance the bigger the difference is. The difference will be largest when switching from the min vrefresh to the max vrefresh duration. Large differences can occur for applications that render on demand like a web browser (and why you wouldn't want VRR enabled for those). The hardware would continuously wait for a flip that isn't coming. Then if the user moves their cursor or the page updates it's going to happen "randomly" in that window and the hardware will adjust to that. > >>>> >>>> The userspace integration as part of these patches demonstrates enabling >>>> variable_refresh on the CRTC selectively. > > > I believe that VRR on X11 would probably depend on these bits: > > 1. Hardware capability > Whether the monitor, the CRTC hardware, and the kernel driver > all agree that VRR is possible. > > 2. User preference > A toggle on the desktop environment's display settings > application to allow or disallow VRR. After all, video mode > configuration is here too, and ideally applications should not > mess with the video mode directly. > > 3. Application preference > Whether the application, e.g. a game, is prepared to deal with > VRR and wishes it to be enabled. > > 4. X server scenegraph > Which windows happen to be showing on the VRR-capable output > and does that scenegraph allow e.g. flipping straight to client > buffers instead of having the X server copy into framebuffers > of its own. > > You mentioned that VRR probably doesn't make sense if there are > multiple windows showing in the same output, so point 4 would cover > that. Also games etc. would aim to hit the direct scanout path to avoid > wasting time in the X server copy, so it seems that requiring flips to > client buffers would be reasonable for enabling VRR. Sounds like that > is exactly how you implemented it in xf86-video-amdgpu, isn't it? > > Skimming through your proposal, it seems you have covered 3 out of 4 > points. Did you or I miss something? You cannot have points 2 and 3 > fight over the control of the same property. > > How do you envision VRR to interact with X11 compositing managers? > > I presume compositing managers aim for the direct scanout path in the X > server to avoid yet another copy there, right? Does point 4 need to > exclude the Composite Overlay Window (COW) from allowed VRR windows? > > > Given all the above, I would like to question the choice of trying to > accommodate X11 interfaces directly in the DRM UABI. There seems to be > quite many bits controlled by different processes and VRR should be > active only when they all agree. Therefore, if you want end user > applications to rely on straight connector property pass-through via > RandR, you end up with more properties than strictly necessary, > complicating the UABI. > > In other words, I agree with the criticism here by Ville and Daniel Your interpretation of the X11 stack is mostly the same as mine with minor differences for the 2-4. For the sake of the explanation I'm actually going to discuss what's part of the plan for v3 here since it's a little simpler and should address your concerns. There's a large library of existing games and no modification should be needed for them to support the dynamic VRR adjustment. They all share a common render stack with GL or Vulkan. So mesa can be leveraged to "mark" the applications as suitable for VRR - with a window property in this case. There are less applications that are unsuitable than there are suitable so a blacklist approach is done here - an opt-out approach for application preference. This covers 3. User preference can be handled as part of the DDX driver with something like an X Option. Dropping the variable_refresh_enabled property in favor of this works. This covers 2. For application suitability, the Present extension will be leveraged here since it covers the logical restrictions (single application, CRTC covered) for this problem. If the window is flipping via the extension then it's suitable. This covers 4. The CRTC VRR enable property would then be set in the DDX driver when user preference and application suitability match. Which then leads into 1 - it will only be enabled when the hardware is capable. Most compositors function well under this stack. It will vary depending on compositor support for window unredirection to let the window flip via the Present extension. Mutter handles this without any configuration and kwin can be configured to work with these patches. Compton can be configured to support unredirection as well. These (among others) are covered as part of the blacklist for the mesa patches. They do need to be explicitly excluded. > > > On the other hand, in Wayland architecture, there is only one single > userspace process you need to consider in the DRM UABI: the display > server: > - There is no separate compositing manager process. > - There is no separate application to handle user preference (point 2); > well, not in the sense that you would need to consider it in the DRM > UABI, because it will go through the display server that is part of > the desktop rather than bypassing the desktop. > - There is no DRM property pass-through to end-user applications, > instead there will be dedicated protocol extensions to let the > display server make the final call. > > Therefore, it would be fine to have just two bits: > > 1. Hardware capability > Whether the monitor, the CRTC hardware, and the kernel driver > all agree that VRR is possible. > > 2. - 4. Whether userspace (i.e. the display server) wants to use VRR or > not. This would be the CRTC property. I did some prototyping using the atomic API directly and the way I was utilizing the properties was exactly like this. Even more reason to go with the two properties (connector capable, CRTC enable) I suppose. > > I also believe that it would be useful to expose vmin/vmax to userspace > as properties, so that display servers and apps can plan ahead on when > they will render. I suppose that can be left for later when someone > starts working on userspace taking advantage of it, so that the proper > userspace requirement for new UABI is fulfilled. Knowing the vmin/vmax could potentially be useful for testing but most applications shouldn't be trying to predict or adjust rendering based on these. I agree with the discussion for this coming later. > > > Thanks, > pq > Nicholas Kazlauskas
On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote: > On 10/05/2018 04:10 AM, Pekka Paalanen wrote: >> >> 2. User preference >> A toggle on the desktop environment's display settings >> application to allow or disallow VRR. After all, video mode >> configuration is here too, and ideally applications should not >> mess with the video mode directly. > > [...] > > User preference can be handled as part of the DDX driver with something > like an X Option. Dropping the variable_refresh_enabled property in > favor of this works. This covers 2. The Xorg driver can expose a RandR output property, even if the kernel doesn't expose a corresponding connector property. See e.g. the TearFree output property in xf86-video-amdgpu. >> I also believe that it would be useful to expose vmin/vmax to userspace >> as properties, so that display servers and apps can plan ahead on when >> they will render. I suppose that can be left for later when someone >> starts working on userspace taking advantage of it, so that the proper >> userspace requirement for new UABI is fulfilled. > > Knowing the vmin/vmax could potentially be useful for testing but most > applications shouldn't be trying to predict or adjust rendering based on > these. FWIW, recent research indicates that this is necessary for perfectly smooth animation without micro-stuttering, even with VRR.
On 10/05/2018 12:56 PM, Michel Dänzer wrote: > On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote: >> On 10/05/2018 04:10 AM, Pekka Paalanen wrote: >>> >>> 2. User preference >>> A toggle on the desktop environment's display settings >>> application to allow or disallow VRR. After all, video mode >>> configuration is here too, and ideally applications should not >>> mess with the video mode directly. >> >> [...] >> >> User preference can be handled as part of the DDX driver with something >> like an X Option. Dropping the variable_refresh_enabled property in >> favor of this works. This covers 2. > > The Xorg driver can expose a RandR output property, even if the kernel > doesn't expose a corresponding connector property. See e.g. the TearFree > output property in xf86-video-amdgpu. The new configuration property for the DDX side of things does pretty much that. > > >>> I also believe that it would be useful to expose vmin/vmax to userspace >>> as properties, so that display servers and apps can plan ahead on when >>> they will render. I suppose that can be left for later when someone >>> starts working on userspace taking advantage of it, so that the proper >>> userspace requirement for new UABI is fulfilled. >> >> Knowing the vmin/vmax could potentially be useful for testing but most >> applications shouldn't be trying to predict or adjust rendering based on >> these. > > FWIW, recent research indicates that this is necessary for perfectly > smooth animation without micro-stuttering, even with VRR. > > This was brought up in previous RFCs about this but I'm not really convinced by the research methodology and the results from those threads. Targeting the max vrefresh of the display (which is known without additional properties) should be best for prediction since anything lower would be inherently less smooth by definition. Nicholas Kazlauskas
On 2018-10-05 7:48 p.m., Kazlauskas, Nicholas wrote: > On 10/05/2018 12:56 PM, Michel Dänzer wrote: >> On 2018-10-05 6:21 p.m., Kazlauskas, Nicholas wrote: >>> On 10/05/2018 04:10 AM, Pekka Paalanen wrote: >>>> >>>> I also believe that it would be useful to expose vmin/vmax to userspace >>>> as properties, so that display servers and apps can plan ahead on when >>>> they will render. I suppose that can be left for later when someone >>>> starts working on userspace taking advantage of it, so that the proper >>>> userspace requirement for new UABI is fulfilled. >>> >>> Knowing the vmin/vmax could potentially be useful for testing but most >>> applications shouldn't be trying to predict or adjust rendering based on >>> these. >> >> FWIW, recent research indicates that this is necessary for perfectly >> smooth animation without micro-stuttering, even with VRR. > > This was brought up in previous RFCs about this but I'm not really > convinced by the research methodology and the results from those threads. > > Targeting the max vrefresh of the display (which is known without > additional properties) should be best for prediction since anything > lower would be inherently less smooth by definition. Smooth animation isn't only about frame-rate, it's also about the timing of each frame's presentation and its contents being consistent with each other[0]. To ensure this, the application has to know the constraints for when the next frame can be presented, and it has to be able to control when the frame is presented. [0] One example of this is https://www.youtube.com/watch?v=V4fgYS38aQg . When this video is played back at 60 fps, the upper sphere moves smoothly, but the lower sphere sometimes jumps back and forth slightly (it's pretty subtle, might take a while to notice), because its position isn't always consistent with the frame's presentation timing.
On Fri, 5 Oct 2018 12:21:20 -0400 "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > On 10/05/2018 04:10 AM, Pekka Paalanen wrote: > > Hi, > > > > I have a somewhat of my own view on what would be involved with VRR, > > and I'd like to hear what you think of it. Comments inline. > > > > > > On Tue, 25 Sep 2018 09:51:37 -0400 > > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > > > >> On 09/24/2018 04:26 PM, Ville Syrjälä wrote: > >>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: > >>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: > >>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: > >>>>>> Variable refresh rate algorithms have typically been enabled only > >>>>>> when the display is covered by a single source of content. > >>>>>> > >>>>>> This patch introduces a new default CRTC property that helps > >>>>>> hint to the driver when the CRTC composition is suitable for variable > >>>>>> refresh rate algorithms. Userspace can set this property dynamically > >>>>>> as the composition changes. > >>>>>> > >>>>>> Whether the variable refresh rate algorithms are active will still > >>>>>> depend on the CRTC being suitable and the connector being capable > >>>>>> and enabled by the user for variable refresh rate support. > >>>>>> > >>>>>> It is worth noting that while the property is atomic it isn't filtered > >>>>>> from legacy userspace queries. This allows for Xorg userspace drivers > >>>>>> to implement support in non-atomic setups. > >>>>>> > >>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> ... > >> Whenever I mentioned variable refresh "features", what I really meant > >> was operating in one of two modes: > >> > >> (1) Letting the driver and hardware adjust refresh automatically based > >> on the flip rate on a CRTC from a single application > >> > >> (2) Setting a fixed frame duration based on the flip rate on a CRTC from > >> a single application > > > > I wonder if that's too much magic in the kernel... what would be wrong > > with simply flipping ASAP when VRR is active? > > > > How will userspace be able to predict coming flip opportunities if the > > kernel does so much magic? > > The kernel driver doesn't need to do much more than let the hardware > know the variable refresh range. The "magic" is performed by hardware. > > Most games would like to render as fast as possible to deliver a more > responsive and smoother image to the user. Many of these are also > resource intensive and won't always be able to render at the fixed > refresh rate of the panel (especially for higher refresh rates like > 144Hz). The user will experience stuttering if the game takes too long > to render and misses the vblank window for the flip. > > Dynamic VRR adjustment can resolve this problem. The hardware can lower > the refresh rate and increase the vblank window in response to this so > the user doesn't experience stuttering (or latency). > > Userspace shouldn't predict anything. ... > >>>> The reasoning for the split is because not all content is suitable for > >>>> variable refresh. Desktop environments, web browsers, etc only typically > >>>> flip when needed - which will result in display flickering. > > > > Flickering? What do you mean? > > This is a property of how panels work. > > The luminance for a panel will vary based on how long the vrefresh is. > Since the vrefresh length is changing as part of VRR you're more likely > to notice the difference in luminance the bigger the difference is. > > The difference will be largest when switching from the min vrefresh to > the max vrefresh duration. > > Large differences can occur for applications that render on demand like > a web browser (and why you wouldn't want VRR enabled for those). The > hardware would continuously wait for a flip that isn't coming. Then if > the user moves their cursor or the page updates it's going to happen > "randomly" in that window and the hardware will adjust to that. Hi Nicholas, it seems I have very much mis-guessed what VRR aims to achieve, and the effect on luminance sounds horrible. People have worked for years to make display timings more explicit, giving better control and predictability over them. It sounds like VRR is not an improvement that allows new smarter software to take control of timings even better. Instead, VRR seems to be a step backwards, introducing more uncertainty into the timings. The expectation of a fixed unknown refresh rate must be built into software for the software to work reasonably while VRR is active. From your comments I understood that the VRR hardware still very much depends on a consistent refresh rate, except the hardware (not the software!) can additionally slew the refresh rate over time. Abrupt changes in frame timings must still be prevented, but I wasn't quite sure if you meant the hardware will do that or if the software must do that, since you are worried about on-demand updating applications causing flickering. Hence, VRR looks like a band-aid for old and simple applications that use brute force (more fps, maximize the work load) in an attempt to make things smoother and to reduce latency. There are lots of such applications, so VRR has its place. It is my mistake of assuming it could do more. Yes, I am disappointed to realize this. I believe that future display servers and applications that actually care about display timings will just opt out from VRR to gain better predictability. I also believe that you will need to blacklist applications like video players whose developers have spent a great deal of effort in making a smart scheduling algorithm for fixed rate display. I assume that a smart sheduling algorithm that is based on prediciting the next display time instant will interact poorly with VRR. A video player will need to know if it is getting fixed rate or VRR to choose the appropriate timing algorithm - wherther it needs to adapt to the display rate or will the display rate adapt to it. In summary, VRR is only good for exactly the use cases you listed, and for other uses it can be harmful, just like you said. Given the above, I think we are now very much on the same page about what the KMS UABI should look like. > Most compositors function well under this stack. It will vary > depending on compositor support for window unredirection to let the > window flip via the Present extension. Mutter handles this without > any configuration and kwin can be configured to work with these > patches. Compton can be configured to support unredirection as well. You are thinking about the applications. What about the compositor itself? Is the COW not hitting the Present direct scanout path, triggering VRR, when what is actually showing is the desktop with on-demand randomly updating windows? > These (among others) are covered as part of the blacklist for the > mesa patches. They do need to be explicitly excluded. Right, you blacklist all compositing managers to prevent them from regressing. That feels a bit ugly to me, needing exceptions to not regress things, but maybe that's business as usual in Mesa? Fortunately that problem with compositing managers won't exist with Wayland. In any case, there must also be a way for an application to explicitly say if it supports VRR or not and to know if it gets VRR or not. Are there no EGL or Vulkan extensions for that? Thanks, pq
On 10/10/2018 03:14 AM, Pekka Paalanen wrote: > On Fri, 5 Oct 2018 12:21:20 -0400 > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >> On 10/05/2018 04:10 AM, Pekka Paalanen wrote: >>> Hi, >>> >>> I have a somewhat of my own view on what would be involved with VRR, >>> and I'd like to hear what you think of it. Comments inline. >>> >>> >>> On Tue, 25 Sep 2018 09:51:37 -0400 >>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: >>> >>>> On 09/24/2018 04:26 PM, Ville Syrjälä wrote: >>>>> On Mon, Sep 24, 2018 at 03:06:02PM -0400, Kazlauskas, Nicholas wrote: >>>>>> On 09/24/2018 02:38 PM, Ville Syrjälä wrote: >>>>>>> On Mon, Sep 24, 2018 at 02:15:36PM -0400, Nicholas Kazlauskas wrote: >>>>>>>> Variable refresh rate algorithms have typically been enabled only >>>>>>>> when the display is covered by a single source of content. >>>>>>>> >>>>>>>> This patch introduces a new default CRTC property that helps >>>>>>>> hint to the driver when the CRTC composition is suitable for variable >>>>>>>> refresh rate algorithms. Userspace can set this property dynamically >>>>>>>> as the composition changes. >>>>>>>> >>>>>>>> Whether the variable refresh rate algorithms are active will still >>>>>>>> depend on the CRTC being suitable and the connector being capable >>>>>>>> and enabled by the user for variable refresh rate support. >>>>>>>> >>>>>>>> It is worth noting that while the property is atomic it isn't filtered >>>>>>>> from legacy userspace queries. This allows for Xorg userspace drivers >>>>>>>> to implement support in non-atomic setups. >>>>>>>> >>>>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> > > ... > >>>> Whenever I mentioned variable refresh "features", what I really meant >>>> was operating in one of two modes: >>>> >>>> (1) Letting the driver and hardware adjust refresh automatically based >>>> on the flip rate on a CRTC from a single application >>>> >>>> (2) Setting a fixed frame duration based on the flip rate on a CRTC from >>>> a single application >>> >>> I wonder if that's too much magic in the kernel... what would be wrong >>> with simply flipping ASAP when VRR is active? >>> >>> How will userspace be able to predict coming flip opportunities if the >>> kernel does so much magic? >> >> The kernel driver doesn't need to do much more than let the hardware >> know the variable refresh range. The "magic" is performed by hardware. >> >> Most games would like to render as fast as possible to deliver a more >> responsive and smoother image to the user. Many of these are also >> resource intensive and won't always be able to render at the fixed >> refresh rate of the panel (especially for higher refresh rates like >> 144Hz). The user will experience stuttering if the game takes too long >> to render and misses the vblank window for the flip. >> >> Dynamic VRR adjustment can resolve this problem. The hardware can lower >> the refresh rate and increase the vblank window in response to this so >> the user doesn't experience stuttering (or latency). >> >> Userspace shouldn't predict anything. > > ... > >>>>>> The reasoning for the split is because not all content is suitable for >>>>>> variable refresh. Desktop environments, web browsers, etc only typically >>>>>> flip when needed - which will result in display flickering. >>> >>> Flickering? What do you mean? >> >> This is a property of how panels work. >> >> The luminance for a panel will vary based on how long the vrefresh is. >> Since the vrefresh length is changing as part of VRR you're more likely >> to notice the difference in luminance the bigger the difference is. >> >> The difference will be largest when switching from the min vrefresh to >> the max vrefresh duration. >> >> Large differences can occur for applications that render on demand like >> a web browser (and why you wouldn't want VRR enabled for those). The >> hardware would continuously wait for a flip that isn't coming. Then if >> the user moves their cursor or the page updates it's going to happen >> "randomly" in that window and the hardware will adjust to that. > > Hi Nicholas, > > it seems I have very much mis-guessed what VRR aims to achieve, and the > effect on luminance sounds horrible. It really depends on the panel characteristics - like the VRR range and panel brightness. Some panels can be particularly unpleasant but others are fine. > > People have worked for years to make display timings more explicit, > giving better control and predictability over them. It sounds like VRR > is not an improvement that allows new smarter software to take control > of timings even better. Instead, VRR seems to be a step backwards, > introducing more uncertainty into the timings. The expectation of a > fixed unknown refresh rate must be built into software for the software > to work reasonably while VRR is active. > > From your comments I understood that the VRR hardware still very much > depends on a consistent refresh rate, except the hardware (not the > software!) can additionally slew the refresh rate over time. Abrupt > changes in frame timings must still be prevented, but I wasn't quite > sure if you meant the hardware will do that or if the software must do > that, since you are worried about on-demand updating applications > causing flickering. > > Hence, VRR looks like a band-aid for old and simple applications that > use brute force (more fps, maximize the work load) in an attempt to > make things smoother and to reduce latency. There are lots of such > applications, so VRR has its place. It is my mistake of assuming it > could do more. Yes, I am disappointed to realize this. > > I believe that future display servers and applications that actually > care about display timings will just opt out from VRR to gain better > predictability. > > I also believe that you will need to blacklist applications like video > players whose developers have spent a great deal of effort in making a > smart scheduling algorithm for fixed rate display. I assume that a > smart sheduling algorithm that is based on prediciting the next display > time instant will interact poorly with VRR. A video player will need to > know if it is getting fixed rate or VRR to choose the appropriate > timing algorithm - wherther it needs to adapt to the display rate or > will the display rate adapt to it. > > In summary, VRR is only good for exactly the use cases you listed, and > for other uses it can be harmful, just like you said. > > Given the above, I think we are now very much on the same page about > what the KMS UABI should look like. The use-case for the dynamic adjustment mode is largely games, yes. But I don't think that's something to be entirely dismissive of. The old and simple applications are the ones that would benefit the least from this because they probably run fairly well on new hardware. Modern games benefit the most from this since they have high performance requirements. There's also a fair amount of these titles coming out in the last few years largely because of the cross-platform deployment capabilities in popular game engines and frameworks. While I don't have actual numbers, I think that most modern games that come out on Linux are scalable when it comes to refresh rate. VRR panels with a "decent" range will see nearly all stuttering gone in practice with no negative impact to the experience at all. This also benefits games that render lower than the monitor's refresh all the time, like 45Hz on a VRR monitor with a 40-60Hz range. The dynamic adjustment also works as expected with implicit vsync on/off via GLX swap control. Lightweight games don't need to burn a ton of power if they're capable of rendering above the refresh rate of the monitor. Vsync on will still cap you at the mode's refresh as expected (like 60Hz). The patches I've put out target this use case mostly because of the benefit for a relatively simple interface. VRR can and has been used in more ways that this, however. An example usecase that differs from this would actually be video playback. The monitor's refresh rate likely doesn't align with the video content rate. An API that exposes direct control over VRR (like the range, refresh duration, presentation timestamp) could allow the application to specify the content rate directly to deliver a smoother playback experience. For example, if you had a 24fps video and the VRR range was 40-60Hz you could target 48Hz via some API here. > > >> Most compositors function well under this stack. It will vary >> depending on compositor support for window unredirection to let the >> window flip via the Present extension. Mutter handles this without >> any configuration and kwin can be configured to work with these >> patches. Compton can be configured to support unredirection as well. > > You are thinking about the applications. What about the compositor > itself? Is the COW not hitting the Present direct scanout path, > triggering VRR, when what is actually showing is the desktop with > on-demand randomly updating windows? > >> These (among others) are covered as part of the blacklist for the >> mesa patches. They do need to be explicitly excluded. > > Right, you blacklist all compositing managers to prevent them from > regressing. That feels a bit ugly to me, needing exceptions to not > regress things, but maybe that's business as usual in Mesa? > > Fortunately that problem with compositing managers won't exist with > Wayland. Compositors do bring out the worst when it comes to flickering for VRR with the dynamic mode. A more explicit API may help here in the future. I think the blacklist approach is still the best when compared with the alternatives. At least when it comes to supporting the vast majority of existing applications without any explicit configuration from the user or developers. I would imagine you're right about a Wayland implementation being cleaner when dealing with some of these issues. > > In any case, there must also be a way for an application to explicitly > say if it supports VRR or not and to know if it gets VRR or not. Are > there no EGL or Vulkan extensions for that? I think this is an interesting idea given what's already out there in terms of swap and present control. There was some discussion about this and the blacklist on the mesa mailing list. For the current implementation the easiest way to override the blacklist is via including an explicit driconf file for the application. A semi-recent change in mesa lets applications do this without overriding the global or user conf. > > > Thanks, > pq > Nicholas Kazlauskas
On Wed, 10 Oct 2018 09:35:50 -0400 "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > The patches I've put out target this use case mostly because of the > benefit for a relatively simple interface. VRR can and has been used in > more ways that this, however. > > An example usecase that differs from this would actually be video > playback. The monitor's refresh rate likely doesn't align with the video > content rate. An API that exposes direct control over VRR (like the > range, refresh duration, presentation timestamp) could allow the > application to specify the content rate directly to deliver a smoother > playback experience. For example, if you had a 24fps video and the VRR > range was 40-60Hz you could target 48Hz via some API here. The way that has been discussed to be implemented is that DRM page flips would carry a target timestamp, which the driver will then meet as good as it can. It is better to define an absolute target timestamp than a frequency, because a timestamp can be used to synchronize with audio and more. Mario Kleiner can tell you all about scientific use cases that require accurate display timing, not just frequency. A timestamp will naturally lend itself to arbitrary on-demand screen updates as well. However, userspace still needs to know if the target timestamp it is contemplating on could realistically be met, so that e.g. a video player can choose between showing video frames as is vs. needing to interpolate for the presentation times it actually can achieve. I suppose we agree on the above. Btw. would a video player even need explicit controls if it knows the display will adapt to the player's refresh rate? It could just use the original video rate and from what I understood, the display will soon end up refreshing at exactly that rate. The player can still use the realized page flip timestamps to synchronize with audio, since it can assume the refresh rate is stable and can predict the next few flips. But, this would only work if the video player knows that VRR will adapt to its rate. While we are talking about predictability of page flips, Weston is already relying on a prediction to reduce the desktop latency to screen. However, it should be possible to modify Weston to implement the kind of VRR support for fullscreen exclusive windows like you are proposing just fine. Just like the X11 window property you mentioned for marking windows as eligible for VRR in Xorg, Wayland will need a similar protocol extension. I'm happy to see work being done for VRR. Thanks, pq
Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: > On Wed, 10 Oct 2018 09:35:50 -0400 > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >> The patches I've put out target this use case mostly because of the >> benefit for a relatively simple interface. VRR can and has been used in >> more ways that this, however. >> >> An example usecase that differs from this would actually be video >> playback. The monitor's refresh rate likely doesn't align with the video >> content rate. An API that exposes direct control over VRR (like the >> range, refresh duration, presentation timestamp) could allow the >> application to specify the content rate directly to deliver a smoother >> playback experience. For example, if you had a 24fps video and the VRR >> range was 40-60Hz you could target 48Hz via some API here. > The way that has been discussed to be implemented is that DRM page flips > would carry a target timestamp, which the driver will then meet as good > as it can. It is better to define an absolute target timestamp than a > frequency, because a timestamp can be used to synchronize with audio > and more. Mario Kleiner can tell you all about scientific use cases > that require accurate display timing, not just frequency. To summarize what information should be provided by the driver stack to make applications happy: 1. The minimum time a frame can be displayed, in other words the maximum frame rate. 2. The maximum time a frame can be displayed, in other words the minimum frame rate. 3. How much change of frame timing is allowed between frames to avoid luminescence flickering. Number 1 and 2 can also be used to signal the availability of VRR to applications, e.g. if they are identical we don't support VRR at all. Regards, Christian. > > A timestamp will naturally lend itself to arbitrary on-demand screen > updates as well. > > However, userspace still needs to know if the target timestamp it is > contemplating on could realistically be met, so that e.g. a video > player can choose between showing video frames as is vs. needing to > interpolate for the presentation times it actually can achieve. > > I suppose we agree on the above. > > Btw. would a video player even need explicit controls if it knows the > display will adapt to the player's refresh rate? It could just use the > original video rate and from what I understood, the display will soon > end up refreshing at exactly that rate. The player can still use the > realized page flip timestamps to synchronize with audio, since it can > assume the refresh rate is stable and can predict the next few flips. > But, this would only work if the video player knows that VRR will adapt > to its rate. > > While we are talking about predictability of page flips, Weston is > already relying on a prediction to reduce the desktop latency to > screen. However, it should be possible to modify Weston to implement > the kind of VRR support for fullscreen exclusive windows like you are > proposing just fine. > > Just like the X11 window property you mentioned for marking windows as > eligible for VRR in Xorg, Wayland will need a similar protocol > extension. > > I'm happy to see work being done for VRR. > > > Thanks, > pq
On Fri, 12 Oct 2018 07:35:57 +0000 "Koenig, Christian" <Christian.Koenig@amd.com> wrote: > Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: > > On Wed, 10 Oct 2018 09:35:50 -0400 > > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > > > >> The patches I've put out target this use case mostly because of the > >> benefit for a relatively simple interface. VRR can and has been used in > >> more ways that this, however. > >> > >> An example usecase that differs from this would actually be video > >> playback. The monitor's refresh rate likely doesn't align with the video > >> content rate. An API that exposes direct control over VRR (like the > >> range, refresh duration, presentation timestamp) could allow the > >> application to specify the content rate directly to deliver a smoother > >> playback experience. For example, if you had a 24fps video and the VRR > >> range was 40-60Hz you could target 48Hz via some API here. > > The way that has been discussed to be implemented is that DRM page flips > > would carry a target timestamp, which the driver will then meet as good > > as it can. It is better to define an absolute target timestamp than a > > frequency, because a timestamp can be used to synchronize with audio > > and more. Mario Kleiner can tell you all about scientific use cases > > that require accurate display timing, not just frequency. > > To summarize what information should be provided by the driver stack to > make applications happy: > > 1. The minimum time a frame can be displayed, in other words the maximum > frame rate. > 2. The maximum time a frame can be displayed, in other words the minimum > frame rate. > 3. How much change of frame timing is allowed between frames to avoid > luminescence flickering. > > Number 1 and 2 can also be used to signal the availability of VRR to > applications, e.g. if they are identical we don't support VRR at all. Hi Christian, "the maximum time a frame can be displayed" is perhaps not an unambiguous definition. A frame can be shown indefinitely in any case. The CRTC will simply start scanning out the same frame again if there is no new one. I understand what you want to say, but perhaps some different words will be in order. I do wonder if applications really want to know the maximum acceptable slew rate in timings... maybe that should be left for the drivers to apply. What I'm thinking is that we have the page flip timestamp with the page flip events to tell when the new FB became active. That information could be extended with a time range on when the very next flip could take place. Applications are already computing that prediction from the flip timestamp and fixed refresh rate, but it might be nice to give them the driver's opinion explicitly. Maybe the tolerable slew rate is not a constant. Other than that, yes, it sounds fine to me. Thanks, pq
Am 12.10.2018 um 11:21 schrieb Pekka Paalanen: > On Fri, 12 Oct 2018 07:35:57 +0000 > "Koenig, Christian" <Christian.Koenig@amd.com> wrote: > >> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: >>> On Wed, 10 Oct 2018 09:35:50 -0400 >>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: >>> >>>> The patches I've put out target this use case mostly because of the >>>> benefit for a relatively simple interface. VRR can and has been used in >>>> more ways that this, however. >>>> >>>> An example usecase that differs from this would actually be video >>>> playback. The monitor's refresh rate likely doesn't align with the video >>>> content rate. An API that exposes direct control over VRR (like the >>>> range, refresh duration, presentation timestamp) could allow the >>>> application to specify the content rate directly to deliver a smoother >>>> playback experience. For example, if you had a 24fps video and the VRR >>>> range was 40-60Hz you could target 48Hz via some API here. >>> The way that has been discussed to be implemented is that DRM page flips >>> would carry a target timestamp, which the driver will then meet as good >>> as it can. It is better to define an absolute target timestamp than a >>> frequency, because a timestamp can be used to synchronize with audio >>> and more. Mario Kleiner can tell you all about scientific use cases >>> that require accurate display timing, not just frequency. >> To summarize what information should be provided by the driver stack to >> make applications happy: >> >> 1. The minimum time a frame can be displayed, in other words the maximum >> frame rate. >> 2. The maximum time a frame can be displayed, in other words the minimum >> frame rate. >> 3. How much change of frame timing is allowed between frames to avoid >> luminescence flickering. >> >> Number 1 and 2 can also be used to signal the availability of VRR to >> applications, e.g. if they are identical we don't support VRR at all. > Hi Christian, > > "the maximum time a frame can be displayed" is perhaps not an > unambiguous definition. A frame can be shown indefinitely in any case. Good point. Please also note that I'm not an expert on the display stuff in general. My background comes more from implementing the VDPAU mediaplayer interface in mesa. So just throwing some ideas in here from the point of view of an userspace developer which wants to write a media player :) > The CRTC will simply start scanning out the same frame again if there > is no new one. I understand what you want to say, but perhaps some > different words will be in order. > > I do wonder if applications really want to know the maximum acceptable > slew rate in timings... maybe that should be left for the drivers to > apply. What I'm thinking is that we have the page flip timestamp with > the page flip events to tell when the new FB became active. That > information could be extended with a time range on when the very next > flip could take place. Applications are already computing that > prediction from the flip timestamp and fixed refresh rate, but it might > be nice to give them the driver's opinion explicitly. Maybe the > tolerable slew rate is not a constant. Well it depends. I agree that the kernel should probably enforce the slew rate to avoid flickering for the user. But it might be beneficial for the application to know things like this. What the application definitely needs to know is when a frame was actually displayed. E.g. application says I want to display this at point X and at some point later the kernel returns saying the frame was displayed at point N where in the ideal case N=X. Additional to that let's please use 64bit values and nanoseconds for every value we use here, and NOT fps, line numbers or similar :) Regards, Christian. > > Other than that, yes, it sounds fine to me. > > > Thanks, > pq
On 10/12/2018 07:20 AM, Koenig, Christian wrote: > Am 12.10.2018 um 11:21 schrieb Pekka Paalanen: >> On Fri, 12 Oct 2018 07:35:57 +0000 >> "Koenig, Christian" <Christian.Koenig@amd.com> wrote: >> >>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: >>>> On Wed, 10 Oct 2018 09:35:50 -0400 >>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: >>>> >>>>> The patches I've put out target this use case mostly because of the >>>>> benefit for a relatively simple interface. VRR can and has been used in >>>>> more ways that this, however. >>>>> >>>>> An example usecase that differs from this would actually be video >>>>> playback. The monitor's refresh rate likely doesn't align with the video >>>>> content rate. An API that exposes direct control over VRR (like the >>>>> range, refresh duration, presentation timestamp) could allow the >>>>> application to specify the content rate directly to deliver a smoother >>>>> playback experience. For example, if you had a 24fps video and the VRR >>>>> range was 40-60Hz you could target 48Hz via some API here. >>>> The way that has been discussed to be implemented is that DRM page flips >>>> would carry a target timestamp, which the driver will then meet as good >>>> as it can. It is better to define an absolute target timestamp than a >>>> frequency, because a timestamp can be used to synchronize with audio >>>> and more. Mario Kleiner can tell you all about scientific use cases >>>> that require accurate display timing, not just frequency. >>> To summarize what information should be provided by the driver stack to >>> make applications happy: >>> >>> 1. The minimum time a frame can be displayed, in other words the maximum >>> frame rate. >>> 2. The maximum time a frame can be displayed, in other words the minimum >>> frame rate. >>> 3. How much change of frame timing is allowed between frames to avoid >>> luminescence flickering. >>> >>> Number 1 and 2 can also be used to signal the availability of VRR to >>> applications, e.g. if they are identical we don't support VRR at all. >> Hi Christian, >> >> "the maximum time a frame can be displayed" is perhaps not an >> unambiguous definition. A frame can be shown indefinitely in any case. > > Good point. Please also note that I'm not an expert on the display stuff > in general. > > My background comes more from implementing the VDPAU mediaplayer > interface in mesa. > > So just throwing some ideas in here from the point of view of an > userspace developer which wants to write a media player :) > >> The CRTC will simply start scanning out the same frame again if there >> is no new one. I understand what you want to say, but perhaps some >> different words will be in order. >> >> I do wonder if applications really want to know the maximum acceptable >> slew rate in timings... maybe that should be left for the drivers to >> apply. What I'm thinking is that we have the page flip timestamp with >> the page flip events to tell when the new FB became active. That >> information could be extended with a time range on when the very next >> flip could take place. Applications are already computing that >> prediction from the flip timestamp and fixed refresh rate, but it might >> be nice to give them the driver's opinion explicitly. Maybe the >> tolerable slew rate is not a constant. > > Well it depends. I agree that the kernel should probably enforce the > slew rate to avoid flickering for the user. > > But it might be beneficial for the application to know things like this. > > What the application definitely needs to know is when a frame was > actually displayed. E.g. application says I want to display this at > point X and at some point later the kernel returns saying the frame was > displayed at point N where in the ideal case N=X. > > Additional to that let's please use 64bit values and nanoseconds for > every value we use here, and NOT fps, line numbers or similar :) > > Regards, > Christian. Flickering will really depend on the panel itself. A wider ranger is more likely to exhibit the issue, but factors like topology, pixel density and other display technology can affect this. It can be hard for a driver to guess at all of this. For many panels it'd be difficult to notice unless it's consistently jumping between the min and max range. Opening up an API that restricts how much the driver can change the refresh rate in a VRR scenario seems a bit extreme, but there's probably some applications that could benefit from this. I'd certainly want to see the actual use case first, though. This still feels more like a driver policy to me. I agree with the nanosecond based timestamp API making the most logical sense here from an API perspective. This does overlap a little bit with the target vblank property that's already on the CRTC, perhaps. The target vblank could be determined based on the timestamp. The driver is likely to exceed the target presentation timestamp by a fair bit if it was to just do this, however. VRR could be used in this case to get closer to the timestamp. A naive implementation could iterate over every rate in the range and take the one with the minimum difference, for example. > >> >> Other than that, yes, it sounds fine to me. >> >> >> Thanks, >> pq > Nicholas Kazlauskas
On Fri, 12 Oct 2018 08:58:23 -0400 "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > On 10/12/2018 07:20 AM, Koenig, Christian wrote: > > Am 12.10.2018 um 11:21 schrieb Pekka Paalanen: > >> On Fri, 12 Oct 2018 07:35:57 +0000 > >> "Koenig, Christian" <Christian.Koenig@amd.com> wrote: > >> > >>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: > >>>> On Wed, 10 Oct 2018 09:35:50 -0400 > >>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >>>> > >>>>> The patches I've put out target this use case mostly because of the > >>>>> benefit for a relatively simple interface. VRR can and has been used in > >>>>> more ways that this, however. > >>>>> > >>>>> An example usecase that differs from this would actually be video > >>>>> playback. The monitor's refresh rate likely doesn't align with the video > >>>>> content rate. An API that exposes direct control over VRR (like the > >>>>> range, refresh duration, presentation timestamp) could allow the > >>>>> application to specify the content rate directly to deliver a smoother > >>>>> playback experience. For example, if you had a 24fps video and the VRR > >>>>> range was 40-60Hz you could target 48Hz via some API here. > >>>> The way that has been discussed to be implemented is that DRM page flips > >>>> would carry a target timestamp, which the driver will then meet as good > >>>> as it can. It is better to define an absolute target timestamp than a > >>>> frequency, because a timestamp can be used to synchronize with audio > >>>> and more. Mario Kleiner can tell you all about scientific use cases > >>>> that require accurate display timing, not just frequency. > >>> To summarize what information should be provided by the driver stack to > >>> make applications happy: > >>> > >>> 1. The minimum time a frame can be displayed, in other words the maximum > >>> frame rate. > >>> 2. The maximum time a frame can be displayed, in other words the minimum > >>> frame rate. > >>> 3. How much change of frame timing is allowed between frames to avoid > >>> luminescence flickering. > >>> > >>> Number 1 and 2 can also be used to signal the availability of VRR to > >>> applications, e.g. if they are identical we don't support VRR at all. > >> Hi Christian, > >> > >> "the maximum time a frame can be displayed" is perhaps not an > >> unambiguous definition. A frame can be shown indefinitely in any case. > > > > Good point. Please also note that I'm not an expert on the display stuff > > in general. > > > > My background comes more from implementing the VDPAU mediaplayer > > interface in mesa. > > > > So just throwing some ideas in here from the point of view of an > > userspace developer which wants to write a media player :) Excellent! > >> The CRTC will simply start scanning out the same frame again if there > >> is no new one. I understand what you want to say, but perhaps some > >> different words will be in order. > >> > >> I do wonder if applications really want to know the maximum acceptable > >> slew rate in timings... maybe that should be left for the drivers to > >> apply. What I'm thinking is that we have the page flip timestamp with > >> the page flip events to tell when the new FB became active. That > >> information could be extended with a time range on when the very next > >> flip could take place. Applications are already computing that > >> prediction from the flip timestamp and fixed refresh rate, but it might > >> be nice to give them the driver's opinion explicitly. Maybe the > >> tolerable slew rate is not a constant. > > > > Well it depends. I agree that the kernel should probably enforce the > > slew rate to avoid flickering for the user. That's what I was hoping if the monitor hardware does not do that already, but now it sounds like it's not possible. Another failed assumption from my side. > > But it might be beneficial for the application to know things like this. Applications should know when they could likely flip, my question is how to tell them. Is the acceptable slew rate a constant for a video mode, or does it depend on the previous refresh interval. > > > > What the application definitely needs to know is when a frame was > > actually displayed. E.g. application says I want to display this at > > point X and at some point later the kernel returns saying the frame was > > displayed at point N where in the ideal case N=X. You already get this timestamp with the DRM page flip events. We also have Wayland and X11 extensions to get it to apps. > > > > Additional to that let's please use 64bit values and nanoseconds for > > every value we use here, and NOT fps, line numbers or similar :) Seconded! I'd really favour absolute timestamps even. > > > > Regards, > > Christian. > > Flickering will really depend on the panel itself. A wider ranger is > more likely to exhibit the issue, but factors like topology, pixel > density and other display technology can affect this. > > It can be hard for a driver to guess at all of this. For many panels > it'd be difficult to notice unless it's consistently jumping between the > min and max range. Do you mean that there is no way to know and get that information from the monitor itself? Nothing in EDID that could even be used as a default and quirk the monitors that got it wrong? > Opening up an API that restricts how much the driver can change the > refresh rate in a VRR scenario seems a bit extreme, but there's probably > some applications that could benefit from this. I'd certainly want to > see the actual use case first, though. This still feels more like a > driver policy to me. I don't think anyone suggested that, certainly I did not. The driver should get that information from the monitor hardware so that it can drive it correctly. If the hardware driver doesn't know, then how could the DRM core or userspace know any better? My whole premise was that the driver knows. Sounds like VRR hardware was designed not only for a consistent refresh rate but also temporary glitches being ok. I suppose this will result in choosing a very pessimistic allowed slew rate in the driver that covers 95% of VRR monitors and handle the rest with quirks. That could still work. > I agree with the nanosecond based timestamp API making the most logical > sense here from an API perspective. This does overlap a little bit with > the target vblank property that's already on the CRTC, perhaps. KMS UABI already has a target vblank property defined, or are you talking about your CRTC hardware? Target vblank counter value makes even less sense with VRR than it ever did with a fixed refresh rate. :-) > The target vblank could be determined based on the timestamp. The driver > is likely to exceed the target presentation timestamp by a fair bit if > it was to just do this, however. VRR could be used in this case to get > closer to the timestamp. A naive implementation could iterate over every > rate in the range and take the one with the minimum difference, for > example. Could you elaborate on that, who could be doing what exactly to achieve what? Thanks, pq
On 10/15/2018 09:57 AM, Pekka Paalanen wrote: > On Fri, 12 Oct 2018 08:58:23 -0400 > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >> On 10/12/2018 07:20 AM, Koenig, Christian wrote: >>> Am 12.10.2018 um 11:21 schrieb Pekka Paalanen: >>>> On Fri, 12 Oct 2018 07:35:57 +0000 >>>> "Koenig, Christian" <Christian.Koenig@amd.com> wrote: >>>> >>>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: >>>>>> On Wed, 10 Oct 2018 09:35:50 -0400 >>>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: >>>>>> >>>>>>> The patches I've put out target this use case mostly because of the >>>>>>> benefit for a relatively simple interface. VRR can and has been used in >>>>>>> more ways that this, however. >>>>>>> >>>>>>> An example usecase that differs from this would actually be video >>>>>>> playback. The monitor's refresh rate likely doesn't align with the video >>>>>>> content rate. An API that exposes direct control over VRR (like the >>>>>>> range, refresh duration, presentation timestamp) could allow the >>>>>>> application to specify the content rate directly to deliver a smoother >>>>>>> playback experience. For example, if you had a 24fps video and the VRR >>>>>>> range was 40-60Hz you could target 48Hz via some API here. >>>>>> The way that has been discussed to be implemented is that DRM page flips >>>>>> would carry a target timestamp, which the driver will then meet as good >>>>>> as it can. It is better to define an absolute target timestamp than a >>>>>> frequency, because a timestamp can be used to synchronize with audio >>>>>> and more. Mario Kleiner can tell you all about scientific use cases >>>>>> that require accurate display timing, not just frequency. >>>>> To summarize what information should be provided by the driver stack to >>>>> make applications happy: >>>>> >>>>> 1. The minimum time a frame can be displayed, in other words the maximum >>>>> frame rate. >>>>> 2. The maximum time a frame can be displayed, in other words the minimum >>>>> frame rate. >>>>> 3. How much change of frame timing is allowed between frames to avoid >>>>> luminescence flickering. >>>>> >>>>> Number 1 and 2 can also be used to signal the availability of VRR to >>>>> applications, e.g. if they are identical we don't support VRR at all. >>>> Hi Christian, >>>> >>>> "the maximum time a frame can be displayed" is perhaps not an >>>> unambiguous definition. A frame can be shown indefinitely in any case. >>> >>> Good point. Please also note that I'm not an expert on the display stuff >>> in general. >>> >>> My background comes more from implementing the VDPAU mediaplayer >>> interface in mesa. >>> >>> So just throwing some ideas in here from the point of view of an >>> userspace developer which wants to write a media player :) > > Excellent! > >>>> The CRTC will simply start scanning out the same frame again if there >>>> is no new one. I understand what you want to say, but perhaps some >>>> different words will be in order. >>>> >>>> I do wonder if applications really want to know the maximum acceptable >>>> slew rate in timings... maybe that should be left for the drivers to >>>> apply. What I'm thinking is that we have the page flip timestamp with >>>> the page flip events to tell when the new FB became active. That >>>> information could be extended with a time range on when the very next >>>> flip could take place. Applications are already computing that >>>> prediction from the flip timestamp and fixed refresh rate, but it might >>>> be nice to give them the driver's opinion explicitly. Maybe the >>>> tolerable slew rate is not a constant. >>> >>> Well it depends. I agree that the kernel should probably enforce the >>> slew rate to avoid flickering for the user. > > That's what I was hoping if the monitor hardware does not do that > already, but now it sounds like it's not possible. Another failed > assumption from my side. > >>> But it might be beneficial for the application to know things like this. > > Applications should know when they could likely flip, my question is > how to tell them. Is the acceptable slew rate a constant for a video > mode, or does it depend on the previous refresh interval. > >>> >>> What the application definitely needs to know is when a frame was >>> actually displayed. E.g. application says I want to display this at >>> point X and at some point later the kernel returns saying the frame was >>> displayed at point N where in the ideal case N=X. > > You already get this timestamp with the DRM page flip events. We also > have Wayland and X11 extensions to get it to apps. > >>> >>> Additional to that let's please use 64bit values and nanoseconds for >>> every value we use here, and NOT fps, line numbers or similar :) > > Seconded! > > I'd really favour absolute timestamps even. > >>> >>> Regards, >>> Christian. >> >> Flickering will really depend on the panel itself. A wider ranger is >> more likely to exhibit the issue, but factors like topology, pixel >> density and other display technology can affect this. >> >> It can be hard for a driver to guess at all of this. For many panels >> it'd be difficult to notice unless it's consistently jumping between the >> min and max range. > > Do you mean that there is no way to know and get that information from > the monitor itself? Nothing in EDID that could even be used as a > default and quirk the monitors that got it wrong? The variable refresh range is exposed, but that's about it. There's certainly no simple algorithm you can feed monitor information into to determine how bad the luminance flickering is going to look to the user. > >> Opening up an API that restricts how much the driver can change the >> refresh rate in a VRR scenario seems a bit extreme, but there's probably >> some applications that could benefit from this. I'd certainly want to >> see the actual use case first, though. This still feels more like a >> driver policy to me. > > I don't think anyone suggested that, certainly I did not. The driver > should get that information from the monitor hardware so that it can > drive it correctly. If the hardware driver doesn't know, then how could > the DRM core or userspace know any better? My whole premise was that the > driver knows. This was referring to the "How much change of frame timing is allowed between frames to avoid luminescence flickering" comment. I assumed that this implied restriction of the min/max VRR ranges from the driver. > > Sounds like VRR hardware was designed not only for a consistent refresh > rate but also temporary glitches being ok. > > I suppose this will result in choosing a very pessimistic allowed slew > rate in the driver that covers 95% of VRR monitors and handle the rest > with quirks. That could still work. > >> I agree with the nanosecond based timestamp API making the most logical >> sense here from an API perspective. This does overlap a little bit with >> the target vblank property that's already on the CRTC, perhaps. > > KMS UABI already has a target vblank property defined, or are you > talking about your CRTC hardware? > > Target vblank counter value makes even less sense with VRR than it ever > did with a fixed refresh rate. :-) > >> The target vblank could be determined based on the timestamp. The driver >> is likely to exceed the target presentation timestamp by a fair bit if >> it was to just do this, however. VRR could be used in this case to get >> closer to the timestamp. A naive implementation could iterate over every >> rate in the range and take the one with the minimum difference, for >> example. > > Could you elaborate on that, who could be doing what exactly to achieve > what? > > > Thanks, > pq > These comments were mostly discussing about how a kernel driver might approach a target presentation timestamp. A target presentation timestamp isn't related to VRR directly. Without VRR this could still be implemented by a kernel driver, but it would effectively be the client asking the driver to pick the right target vblank. There's already some support for this in DRM with the target_vblank CRTC property, but it isn't the easiest thing to use. The driver can do much better at getting closer to the client's target presentation timestamp by adjusting the refresh rate accordingly on a VRR capable monitor. Nicholas Kazlauskas
On Mon, 15 Oct 2018 12:02:14 -0400 "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > On 10/15/2018 09:57 AM, Pekka Paalanen wrote: > > On Fri, 12 Oct 2018 08:58:23 -0400 > > "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > > > >> On 10/12/2018 07:20 AM, Koenig, Christian wrote: > >>> Am 12.10.2018 um 11:21 schrieb Pekka Paalanen: > >>>> On Fri, 12 Oct 2018 07:35:57 +0000 > >>>> "Koenig, Christian" <Christian.Koenig@amd.com> wrote: > >>>> > >>>>> Am 12.10.2018 um 09:23 schrieb Pekka Paalanen: > >>>>>> On Wed, 10 Oct 2018 09:35:50 -0400 > >>>>>> "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com> wrote: > >>>>>> > >> Flickering will really depend on the panel itself. A wider ranger is > >> more likely to exhibit the issue, but factors like topology, pixel > >> density and other display technology can affect this. > >> > >> It can be hard for a driver to guess at all of this. For many panels > >> it'd be difficult to notice unless it's consistently jumping between the > >> min and max range. > > > > Do you mean that there is no way to know and get that information from > > the monitor itself? Nothing in EDID that could even be used as a > > default and quirk the monitors that got it wrong? > > The variable refresh range is exposed, but that's about it. There's > certainly no simple algorithm you can feed monitor information into to > determine how bad the luminance flickering is going to look to the user. > > > > >> Opening up an API that restricts how much the driver can change the > >> refresh rate in a VRR scenario seems a bit extreme, but there's probably > >> some applications that could benefit from this. I'd certainly want to > >> see the actual use case first, though. This still feels more like a > >> driver policy to me. > > > > I don't think anyone suggested that, certainly I did not. The driver > > should get that information from the monitor hardware so that it can > > drive it correctly. If the hardware driver doesn't know, then how could > > the DRM core or userspace know any better? My whole premise was that the > > driver knows. > > This was referring to the "How much change of frame timing is allowed > between frames to avoid luminescence flickering" comment. I assumed that > this implied restriction of the min/max VRR ranges from the driver. Yes, exactly. The limits on the change of frame timings should be enforced by the driver if the hardware does not do it already, so that regardless of when userspace is scheduling flips, the monitor will never flicker. IOW, even with on-demand rendering apps with totally random frame timings, the kernel driver or the monitor hardware must ensure the monitor will never luminance-flicker visibly. Making good image quality (in this case not flickering) depend on userspace doing something specific correctly without even a possibility to know what "correct" is, is just silly in my opinion. Too bad that ship sailed years ago, so we will need to have very pessimistic kernel driver expectations on what slew rates are acceptable. I do believe the kernel driver must enforce limits on slew rate to prevent screen flickering if the hardware does not prevent it already. Besides, the kernel driver needs to know when to re-scan out the current framebuffer in case userspace decides to take a pause, e.g. a temporary stall in a game - compiling shaders or whatever. That should not result in any flickering either, right? How will that be handled? So, maybe it's not a bad idea to think of an UABI for changing the slew rate limits. It is similar to color calibration: the defaults should be ok to watch, but if one wants more correct color reproduction, one needs to calibrate the monitor and load up color correction factors through the driver. The luminance flickering should never be disturbing by default, but maybe some people want to optimize further. > > > > Sounds like VRR hardware was designed not only for a consistent refresh > > rate but also temporary glitches being ok. > > > > I suppose this will result in choosing a very pessimistic allowed slew > > rate in the driver that covers 95% of VRR monitors and handle the rest > > with quirks. That could still work. > > > >> I agree with the nanosecond based timestamp API making the most logical > >> sense here from an API perspective. This does overlap a little bit with > >> the target vblank property that's already on the CRTC, perhaps. > > > > KMS UABI already has a target vblank property defined, or are you > > talking about your CRTC hardware? > > > > Target vblank counter value makes even less sense with VRR than it ever > > did with a fixed refresh rate. :-) > > >> The target vblank could be determined based on the timestamp. The driver > >> is likely to exceed the target presentation timestamp by a fair bit if > >> it was to just do this, however. VRR could be used in this case to get > >> closer to the timestamp. A naive implementation could iterate over every > >> rate in the range and take the one with the minimum difference, for > >> example. > > > > Could you elaborate on that, who could be doing what exactly to achieve > > what? > > These comments were mostly discussing about how a kernel driver might > approach a target presentation timestamp. > > A target presentation timestamp isn't related to VRR directly. Without > VRR this could still be implemented by a kernel driver, but it would > effectively be the client asking the driver to pick the right target > vblank. There's already some support for this in DRM with the > target_vblank CRTC property, but it isn't the easiest thing to use. > > The driver can do much better at getting closer to the client's target > presentation timestamp by adjusting the refresh rate accordingly on a > VRR capable monitor. Right. An absolute timestamp should be easier to use than a target vblank for userspace. On the kernel side it would avoid depending on estimating vblank counts from a clock when vblank interrupts have been opportunistically turned off, put to lower rate, or across modesets that might mess up vblank counters. Also good for self-refreshing panels, virtual outputs that get forwarded as video streams, etc. We digress, though. Target timestamps or vblanks are not necessary for the VRR feature, flip ASAP is what we have right now. The kernel driver should adjust when ASAP is to avoid too high slew rate and luminance flickering. Thanks, pq
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 3cf1aa132778..b768d397a811 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3473,6 +3473,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, state->planes_changed = false; state->connectors_changed = false; state->color_mgmt_changed = false; + state->variable_refresh_changed = false; state->zpos_changed = false; state->commit = NULL; state->event = NULL; diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 0bb27a24a55c..32a4cf8a01c3 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -433,6 +433,10 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc, ret = drm_atomic_set_mode_prop_for_crtc(state, mode); drm_property_blob_put(mode); return ret; + } else if (property == config->prop_variable_refresh) { + if (state->variable_refresh != val) + state->variable_refresh_changed = true; + state->variable_refresh = val; } else if (property == config->degamma_lut_property) { ret = drm_atomic_replace_property_blob_from_id(dev, &state->degamma_lut, @@ -491,6 +495,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc, *val = state->active; else if (property == config->prop_mode_id) *val = (state->mode_blob) ? state->mode_blob->base.id : 0; + else if (property == config->prop_variable_refresh) + *val = state->variable_refresh; else if (property == config->degamma_lut_property) *val = (state->degamma_lut) ? state->degamma_lut->base.id : 0; else if (property == config->ctm_property) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 5f488aa80bcd..ca33d6fb90ac 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -340,6 +340,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); drm_object_attach_property(&crtc->base, config->prop_out_fence_ptr, 0); + drm_object_attach_property(&crtc->base, + config->prop_variable_refresh, 0); } return 0; diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index ee80788f2c40..1287c161d65d 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -310,6 +310,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_mode_id = prop; + prop = drm_property_create_bool(dev, 0, + "VARIABLE_REFRESH"); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_variable_refresh = prop; + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB, "DEGAMMA_LUT", 0); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index b21437bc95bf..32b77f18ce6d 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -168,6 +168,12 @@ struct drm_crtc_state { * drivers to steer the atomic commit control flow. */ bool color_mgmt_changed : 1; + /** + * @variable_refresh_changed: Variable refresh support has changed + * on the CRTC. Used by the atomic helpers and drivers to steer the + * atomic commit control flow. + */ + bool variable_refresh_changed : 1; /** * @no_vblank: @@ -290,6 +296,13 @@ struct drm_crtc_state { */ u32 pageflip_flags; + /** + * @variable_refresh: + * + * Indicates whether the CRTC is suitable for variable refresh rate. + */ + bool variable_refresh; + /** * @event: * diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 928e4172a0bb..1290191cd96e 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -639,6 +639,14 @@ struct drm_mode_config { * connectors must be of and active must be set to disabled, too. */ struct drm_property *prop_mode_id; + /** + * @prop_variable_refresh: Default atomic CRTC property to indicate + * whether the CRTC is suitable for variable refresh rate support. + * + * This is only an indication of support, not whether variable + * refresh is active on the CRTC. + */ + struct drm_property *prop_variable_refresh; /** * @dvi_i_subconnector_property: Optional DVI-I property to
Variable refresh rate algorithms have typically been enabled only when the display is covered by a single source of content. This patch introduces a new default CRTC property that helps hint to the driver when the CRTC composition is suitable for variable refresh rate algorithms. Userspace can set this property dynamically as the composition changes. Whether the variable refresh rate algorithms are active will still depend on the CRTC being suitable and the connector being capable and enabled by the user for variable refresh rate support. It is worth noting that while the property is atomic it isn't filtered from legacy userspace queries. This allows for Xorg userspace drivers to implement support in non-atomic setups. Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> --- drivers/gpu/drm/drm_atomic_helper.c | 1 + drivers/gpu/drm/drm_atomic_uapi.c | 6 ++++++ drivers/gpu/drm/drm_crtc.c | 2 ++ drivers/gpu/drm/drm_mode_config.c | 6 ++++++ include/drm/drm_crtc.h | 13 +++++++++++++ include/drm/drm_mode_config.h | 8 ++++++++ 6 files changed, 36 insertions(+)