Re: [PATCH v5 2/4] drm: Add vrr_enabled property to drm CRTC

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 12.10.2018 um 18:44 schrieb Nicholas Kazlauskas:
> This patch introduces the 'vrr_enabled' CRTC property to allow
> dynamic control over variable refresh rate support for a CRTC.
>
> This property should be treated like a content hint to the driver -
> if the hardware or driver is not capable of driving variable refresh
> timings then this is not considered an error.
>
> Capability for variable refresh rate support should be determined
> by querying the vrr_capable drm connector property.
>
> It is worth noting that while the property is intended for atomic use
> it isn't filtered from legacy userspace queries. This allows for Xorg
> userspace drivers to implement support.

I'm honestly still not convinced that a per CRTC property is actually 
the right approach.

What we should rather do instead is to implement a target timestamp for 
the page flip.

Regards,
Christian.

>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> ---
>   drivers/gpu/drm/drm_atomic_uapi.c | 4 ++++
>   drivers/gpu/drm/drm_crtc.c        | 2 ++
>   drivers/gpu/drm/drm_mode_config.c | 6 ++++++
>   include/drm/drm_crtc.h            | 9 +++++++++
>   include/drm/drm_mode_config.h     | 5 +++++
>   5 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f315098c..eec396a57b88 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -433,6 +433,8 @@ 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_vrr_enabled) {
> +		state->vrr_enabled = val;
>   	} else if (property == config->degamma_lut_property) {
>   		ret = drm_atomic_replace_property_blob_from_id(dev,
>   					&state->degamma_lut,
> @@ -491,6 +493,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_vrr_enabled)
> +		*val = state->vrr_enabled;
>   	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..e4eb2c897ff4 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_vrr_enabled, 0);
>   	}
>   
>   	return 0;
> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> index ee80788f2c40..5670c67f28d4 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,
> +			"VRR_ENABLED");
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_vrr_enabled = 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..39c3900aab3c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -290,6 +290,15 @@ struct drm_crtc_state {
>   	 */
>   	u32 pageflip_flags;
>   
> +	/**
> +	 * @vrr_enabled:
> +	 *
> +	 * Indicates if variable refresh rate should be enabled for the CRTC.
> +	 * Support for the requested vrr state will depend on driver and
> +	 * hardware capabiltiy - lacking support is not treated as failure.
> +	 */
> +	bool vrr_enabled;
> +
>   	/**
>   	 * @event:
>   	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 928e4172a0bb..49f2fcfdb5fc 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -639,6 +639,11 @@ struct drm_mode_config {
>   	 * connectors must be of and active must be set to disabled, too.
>   	 */
>   	struct drm_property *prop_mode_id;
> +	/**
> +	 * @prop_vrr_enabled: Default atomic CRTC property to indicate
> +	 * whether variable refresh rate should be enabled on the CRTC.
> +	 */
> +	struct drm_property *prop_vrr_enabled;
>   
>   	/**
>   	 * @dvi_i_subconnector_property: Optional DVI-I property to

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel