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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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