Re: [PATCH v2 2/3] drm: Add variable refresh property to DRM CRTC

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx