[PATCH v4 4/4] drm/amd/display: Set FreeSync state using drm VRR properties

Harry Wentland harry.wentland at amd.com
Thu Oct 11 20:39:12 UTC 2018


On 2018-10-11 12:39 PM, Nicholas Kazlauskas wrote:
> Support for AMDGPU specific FreeSync properties and ioctls are dropped
> from amdgpu_dm in favor of supporting drm variable refresh rate
> properties.
> 
> The drm vrr_capable property is now attached to any DP/HDMI connector.
> Its value is updated accordingly to the connector's FreeSync capabiltiy.
> 
> The freesync_enable logic and ioctl control has has been dropped in
> favor of utilizing the vrr_enabled on the drm CRTC. This allows for more
> fine grained atomic control over which CRTCs should support variable
> refresh rate.
> 
> To handle state changes for vrr_enabled it was easiest to drop the
> forced modeset on freesync_enabled change. This patch now performs the
> required stream updates when planes are flipped.
> 
> This is done for a few reasons:
> 
> (1) VRR stream updates can be done in the fast update path
> 
> (2) amdgpu_dm_atomic_check would need to be hacked apart to check
>     desired variable refresh state and capability before the CRTC
>     disable pass.
> 
> (3) Performing VRR stream updates on-flip is needed for enabling BTR
>     support.
> 
> VRR packets and timing adjustments are now tracked and compared to
> previous values sent to the hardware.
> 
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 255 +++++++++---------
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   7 +-
>  2 files changed, 138 insertions(+), 124 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 6a2342d72742..d5de4b91e144 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1802,72 +1802,6 @@ static void dm_bandwidth_update(struct amdgpu_device *adev)
>  	/* TODO: implement later */
>  }
>  
> -static int amdgpu_notify_freesync(struct drm_device *dev, void *data,
> -				struct drm_file *filp)
> -{
> -	struct drm_atomic_state *state;
> -	struct drm_modeset_acquire_ctx ctx;
> -	struct drm_crtc *crtc;
> -	struct drm_connector *connector;
> -	struct drm_connector_state *old_con_state, *new_con_state;
> -	int ret = 0;
> -	uint8_t i;
> -	bool enable = false;
> -
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -	state = drm_atomic_state_alloc(dev);
> -	if (!state) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	state->acquire_ctx = &ctx;
> -
> -retry:
> -	drm_for_each_crtc(crtc, dev) {
> -		ret = drm_atomic_add_affected_connectors(state, crtc);
> -		if (ret)
> -			goto fail;
> -
> -		/* TODO rework amdgpu_dm_commit_planes so we don't need this */
> -		ret = drm_atomic_add_affected_planes(state, crtc);
> -		if (ret)
> -			goto fail;
> -	}
> -
> -	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
> -		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
> -		struct drm_crtc_state *new_crtc_state;
> -		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
> -		struct dm_crtc_state *dm_new_crtc_state;
> -
> -		if (!acrtc) {
> -			ASSERT(0);
> -			continue;
> -		}
> -
> -		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
> -		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -
> -		dm_new_crtc_state->freesync_enabled = enable;
> -	}
> -
> -	ret = drm_atomic_commit(state);
> -
> -fail:
> -	if (ret == -EDEADLK) {
> -		drm_atomic_state_clear(state);
> -		drm_modeset_backoff(&ctx);
> -		goto retry;
> -	}
> -
> -	drm_atomic_state_put(state);
> -
> -out:
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -	return ret;
> -}
>  
>  static const struct amdgpu_display_funcs dm_display_funcs = {
>  	.bandwidth_update = dm_bandwidth_update, /* called unconditionally */
> @@ -1881,7 +1815,6 @@ static const struct amdgpu_display_funcs dm_display_funcs = {
>  		dm_crtc_get_scanoutpos,/* called unconditionally */
>  	.add_encoder = NULL, /* VBIOS parsing. DAL does it. */
>  	.add_connector = NULL, /* VBIOS parsing. DAL does it. */
> -	.notify_freesync = amdgpu_notify_freesync,

Please also drop from amdgpu_display_funcs definition in amdgpu_mode.h. Might want to drop the set_freesync_property from there as well.

>  
>  };
>  
> @@ -2834,7 +2767,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>  	state->adjust = cur->adjust;
>  	state->vrr_infopacket = cur->vrr_infopacket;
> -	state->freesync_enabled = cur->freesync_enabled;
> +	state->vrr_supported = cur->vrr_supported;
> +	state->freesync_config = cur->freesync_config;
>  
>  	/* TODO Duplicate dc_stream after objects are stream object is flattened */
>  
> @@ -3053,8 +2987,6 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>  	__drm_atomic_helper_connector_duplicate_state(connector, &new_state->base);
>  
>  	new_state->freesync_capable = state->freesync_capable;
> -	new_state->freesync_enable = state->freesync_enable;
> -
>  	return &new_state->base;
>  }
>  
> @@ -3800,6 +3732,11 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>  				adev->mode_info.underscan_vborder_property,
>  				0);
>  
> +	if (connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> +	    connector_type == DRM_MODE_CONNECTOR_DisplayPort) {
> +		drm_connector_attach_vrr_capable_property(
> +			&aconnector->base);
> +	}
>  }
>  
>  static int amdgpu_dm_i2c_xfer(struct i2c_adapter *i2c_adap,
> @@ -4176,6 +4113,77 @@ static void prepare_flip_isr(struct amdgpu_crtc *acrtc)
>  						 acrtc->crtc_id);
>  }
>  
> +static void update_freesync_state_on_stream(
> +	struct amdgpu_display_manager *dm,
> +	struct dm_crtc_state *new_crtc_state,
> +	struct dc_stream_state *new_stream)
> +{
> +	struct mod_vrr_params vrr = {0};
> +	struct dc_info_packet vrr_infopacket = {0};
> +	struct mod_freesync_config config = new_crtc_state->freesync_config;
> +
> +	if (!new_stream)
> +		return;
> +
> +	/*
> +	 * TODO: Determine why min/max totals and vrefresh can be 0 here.
> +	 * For now it's sufficient to just guard against these conditions.
> +	 */
> +
> +	if (!new_stream->timing.h_total || !new_stream->timing.v_total)
> +		return;
> +
> +	if (new_crtc_state->vrr_supported &&
> +	    config.min_refresh_in_uhz &&
> +	    config.max_refresh_in_uhz) {
> +		config.state = new_crtc_state->base.vrr_enabled ?
> +			VRR_STATE_ACTIVE_VARIABLE :
> +			VRR_STATE_INACTIVE;
> +	} else {
> +		config.state = VRR_STATE_UNSUPPORTED;
> +	}
> +
> +	mod_freesync_build_vrr_params(dm->freesync_module,
> +				      new_stream,
> +				      &config, &vrr);
> +
> +	mod_freesync_build_vrr_infopacket(
> +		dm->freesync_module,
> +		new_stream,
> +		&vrr,
> +		packet_type_vrr,
> +		transfer_func_unknown,
> +		&vrr_infopacket);
> +
> +	new_crtc_state->freesync_timing_changed =
> +		(memcmp(&new_crtc_state->adjust,
> +			&vrr.adjust,
> +			sizeof(vrr.adjust)) != 0);
> +
> +	new_crtc_state->freesync_vrr_info_changed =
> +		(memcmp(&new_crtc_state->vrr_infopacket,
> +			&vrr_infopacket,
> +			sizeof(vrr_infopacket)) != 0);
> +
> +	new_crtc_state->adjust = vrr.adjust;
> +	new_crtc_state->vrr_infopacket = vrr_infopacket;
> +
> +	new_stream->adjust = new_crtc_state->adjust;
> +	new_stream->vrr_infopacket = vrr_infopacket;
> +
> +	if (new_crtc_state->freesync_vrr_info_changed)
> +		DRM_DEBUG_KMS("VRR packet update: crtc=%u enabled=%d state=%d",
> +			      new_crtc_state->base.crtc->base.id,
> +			      (int)new_crtc_state->base.vrr_enabled,
> +			      (int)vrr.state);
> +
> +	if (new_crtc_state->freesync_timing_changed)
> +		DRM_DEBUG_KMS("VRR timing update: crtc=%u min=%u max=%u\n",
> +			      new_crtc_state->base.crtc->base.id,
> +			      vrr.adjust.v_total_min,
> +			      vrr.adjust.v_total_max);
> +}
> +
>  /*
>   * Executes flip
>   *
> @@ -4197,6 +4205,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>  	struct dc_flip_addrs addr = { {0} };
>  	/* TODO eliminate or rename surface_update */
>  	struct dc_surface_update surface_updates[1] = { {0} };
> +	struct dc_stream_update stream_update = {0};
>  	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(crtc->state);
>  	struct dc_stream_status *stream_status;
>  
> @@ -4269,11 +4278,26 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc,
>  	}
>  	surface_updates->flip_addr = &addr;
>  
> +	if (acrtc_state->stream) {
> +		update_freesync_state_on_stream(
> +			&adev->dm,
> +			acrtc_state,
> +			acrtc_state->stream);
> +
> +		if (acrtc_state->freesync_timing_changed)
> +			stream_update.adjust =
> +				&acrtc_state->stream->adjust;
> +
> +		if (acrtc_state->freesync_vrr_info_changed)
> +			stream_update.vrr_infopacket =
> +				&acrtc_state->stream->vrr_infopacket;
> +	}
> +

For consistency we might also want to do this in commit_planes_to_stream before the call to dc_commit_updates_for_stream.

We should really merge the two codepaths for dc_commit_updates_for_stream one of these days.

>  	dc_commit_updates_for_stream(adev->dm.dc,
>  					     surface_updates,
>  					     1,
>  					     acrtc_state->stream,
> -					     NULL,
> +					     &stream_update,
>  					     &surface_updates->surface,
>  					     state);
>  
> @@ -4333,11 +4357,6 @@ static bool commit_planes_to_stream(
>  	stream_update->dst = dc_stream->dst;
>  	stream_update->out_transfer_func = dc_stream->out_transfer_func;
>  
> -	if (dm_new_crtc_state->freesync_enabled != dm_old_crtc_state->freesync_enabled) {
> -		stream_update->vrr_infopacket = &dc_stream->vrr_infopacket;
> -		stream_update->adjust = &dc_stream->adjust;
> -	}
> -
>  	for (i = 0; i < new_plane_count; i++) {
>  		updates[i].surface = plane_states[i];
>  		updates[i].gamma =
> @@ -4473,9 +4492,6 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  			spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
>  		}
>  
> -		dc_stream_attach->adjust = acrtc_state->adjust;
> -		dc_stream_attach->vrr_infopacket = acrtc_state->vrr_infopacket;
> -
>  		if (false == commit_planes_to_stream(dm->dc,
>  							plane_states_constructed,
>  							planes_count,
> @@ -4679,9 +4695,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  		WARN_ON(!status);
>  		WARN_ON(!status->plane_count);
>  
> -		dm_new_crtc_state->stream->adjust = dm_new_crtc_state->adjust;
> -		dm_new_crtc_state->stream->vrr_infopacket = dm_new_crtc_state->vrr_infopacket;
> -
>  		/*TODO How it works with MPO ?*/
>  		if (!commit_planes_to_stream(
>  				dm->dc,
> @@ -4899,20 +4912,18 @@ static int do_aquire_global_lock(struct drm_device *dev,
>  	return ret < 0 ? ret : 0;
>  }
>  
> -void set_freesync_on_stream(struct amdgpu_display_manager *dm,
> -			    struct dm_crtc_state *new_crtc_state,
> -			    struct dm_connector_state *new_con_state,
> -			    struct dc_stream_state *new_stream)
> +static void get_freesync_config_for_crtc(
> +	struct dm_crtc_state *new_crtc_state,
> +	struct dm_connector_state *new_con_state)
>  {
>  	struct mod_freesync_config config = {0};
> -	struct mod_vrr_params vrr = {0};
> -	struct dc_info_packet vrr_infopacket = {0};
>  	struct amdgpu_dm_connector *aconnector =
>  			to_amdgpu_dm_connector(new_con_state->base.connector);
>  
> -	if (new_con_state->freesync_capable &&
> -	    new_con_state->freesync_enable) {
> -		config.state = new_crtc_state->freesync_enabled ?
> +	new_crtc_state->vrr_supported = new_con_state->freesync_capable;
> +
> +	if (new_con_state->freesync_capable) {
> +		config.state = new_crtc_state->base.vrr_enabled ?
>  				VRR_STATE_ACTIVE_VARIABLE :
>  				VRR_STATE_INACTIVE;
>  		config.min_refresh_in_uhz =
> @@ -4922,19 +4933,18 @@ void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>  		config.vsif_supported = true;
>  	}
>  
> -	mod_freesync_build_vrr_params(dm->freesync_module,
> -				      new_stream,
> -				      &config, &vrr);
> +	new_crtc_state->freesync_config = config;
> +}
>  
> -	mod_freesync_build_vrr_infopacket(dm->freesync_module,
> -					  new_stream,
> -					  &vrr,
> -					  packet_type_fs1,
> -					  NULL,
> -					  &vrr_infopacket);
> +static void reset_freesync_config_for_crtc(
> +	struct dm_crtc_state *new_crtc_state)
> +{
> +	new_crtc_state->vrr_supported = false;
>  
> -	new_crtc_state->adjust = vrr.adjust;
> -	new_crtc_state->vrr_infopacket = vrr_infopacket;
> +	memset(&new_crtc_state->adjust, 0,
> +	       sizeof(new_crtc_state->adjust));
> +	memset(&new_crtc_state->vrr_infopacket, 0,
> +	       sizeof(new_crtc_state->vrr_infopacket));
>  }
>  
>  static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
> @@ -5009,9 +5019,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  				break;
>  			}
>  
> -			set_freesync_on_stream(dm, dm_new_crtc_state,
> -					       dm_new_conn_state, new_stream);
> -
>  			if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
>  			    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
>  				new_crtc_state->mode_changed = false;
> @@ -5020,9 +5027,6 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  			}
>  		}
>  
> -		if (dm_old_crtc_state->freesync_enabled != dm_new_crtc_state->freesync_enabled)
> -			new_crtc_state->mode_changed = true;
> -
>  		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
>  			goto next_crtc;
>  
> @@ -5059,6 +5063,8 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  			dc_stream_release(dm_old_crtc_state->stream);
>  			dm_new_crtc_state->stream = NULL;
>  
> +			reset_freesync_config_for_crtc(dm_new_crtc_state);
> +
>  			*lock_and_validation_needed = true;
>  
>  		} else {/* Add stream for any updated/enabled CRTC */
> @@ -5136,7 +5142,9 @@ static int dm_update_crtcs_state(struct amdgpu_display_manager *dm,
>  			amdgpu_dm_set_ctm(dm_new_crtc_state);
>  		}
>  
> -
> +		/* Update Freesync settings. */
> +		get_freesync_config_for_crtc(dm_new_crtc_state,
> +					     dm_new_conn_state);
>  	}
>  
>  	return ret;
> @@ -5401,12 +5409,9 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>  		goto fail;
>  
>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
> -		struct dm_crtc_state *dm_old_crtc_state  = to_dm_crtc_state(old_crtc_state);
> -
>  		if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
>  		    !new_crtc_state->color_mgmt_changed &&
> -		    (dm_old_crtc_state->freesync_enabled == dm_new_crtc_state->freesync_enabled))
> +		    !new_crtc_state->vrr_enabled)
>  			continue;
>  
>  		if (!new_crtc_state->enable)
> @@ -5558,14 +5563,15 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	struct detailed_data_monitor_range *range;
>  	struct amdgpu_dm_connector *amdgpu_dm_connector =
>  			to_amdgpu_dm_connector(connector);
> -	struct dm_connector_state *dm_con_state;
> +	struct dm_connector_state *dm_con_state = NULL;
>  
>  	struct drm_device *dev = connector->dev;
>  	struct amdgpu_device *adev = dev->dev_private;
> +	bool freesync_capable = false;
>  
>  	if (!connector->state) {
>  		DRM_ERROR("%s - Connector has no state", __func__);
> -		return;
> +		goto update;
>  	}
>  
>  	if (!edid) {
> @@ -5575,9 +5581,7 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  		amdgpu_dm_connector->max_vfreq = 0;
>  		amdgpu_dm_connector->pixel_clock_mhz = 0;
>  
> -		dm_con_state->freesync_capable = false;
> -		dm_con_state->freesync_enable = false;
> -		return;
> +		goto update;
>  	}
>  
>  	dm_con_state = to_dm_connector_state(connector->state);
> @@ -5585,10 +5589,10 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  	edid_check_required = false;
>  	if (!amdgpu_dm_connector->dc_sink) {
>  		DRM_ERROR("dc_sink NULL, could not add free_sync module.\n");
> -		return;
> +		goto update;
>  	}
>  	if (!adev->dm.freesync_module)
> -		return;
> +		goto update;
>  	/*
>  	 * if edid non zero restrict freesync only for dp and edp
>  	 */
> @@ -5600,7 +5604,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  						amdgpu_dm_connector);
>  		}
>  	}
> -	dm_con_state->freesync_capable = false;
>  	if (edid_check_required == true && (edid->version > 1 ||
>  	   (edid->version == 1 && edid->revision > 1))) {
>  		for (i = 0; i < 4; i++) {
> @@ -5632,8 +5635,16 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
>  		if (amdgpu_dm_connector->max_vfreq -
>  		    amdgpu_dm_connector->min_vfreq > 10) {
>  
> -			dm_con_state->freesync_capable = true;
> +			freesync_capable = true;
>  		}
>  	}
> +
> +update:
> +	if (dm_con_state)
> +		dm_con_state->freesync_capable = freesync_capable;
> +

Interesting. Looks like we could just grab this from the drm_connector property as this isn't something that changes with the state but DRM forces us to track it on the connector state as drm_object_property_get_value() throws a WARN_ON for atomic drivers. Not sure I like it but it's what we got, I guess.

Harry

> +	if (connector->vrr_capable_property)
> +		drm_connector_set_vrr_capable_property(connector,
> +						       freesync_capable);
>  }
>  
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 978b34a5011c..a5aaf8b08968 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -185,7 +185,11 @@ struct dm_crtc_state {
>  	int crc_skip_count;
>  	bool crc_enabled;
>  
> -	bool freesync_enabled;
> +	bool freesync_timing_changed;
> +	bool freesync_vrr_info_changed;
> +
> +	bool vrr_supported;
> +	struct mod_freesync_config freesync_config;
>  	struct dc_crtc_timing_adjust adjust;
>  	struct dc_info_packet vrr_infopacket;
>  };
> @@ -207,7 +211,6 @@ struct dm_connector_state {
>  	uint8_t underscan_vborder;
>  	uint8_t underscan_hborder;
>  	bool underscan_enable;
> -	bool freesync_enable;
>  	bool freesync_capable;
>  };
>  
> 


More information about the amd-gfx mailing list