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.kazlaus...@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
--
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
