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

["Kazlauskas, Nicholas" <nicholas.kazlauskas-5C7GfCeVMHo@public.gmane.org>]

On 10/11/2018 04:39 PM, Harry Wentland wrote:
> 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@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.

Oh right, I forgot about those. Will do.

> 
>>   
>>   };
>>   
>> @@ -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.

In a prior revision to this patch I actually did do this on both but 
reduced it to just the flip - the reason being that there's no point in 
enabling this when we're *not* flipping since it's needed for this to 
function properly.

The merge is probably a good idea down the line to cut down on code 
duplication even further.

> 
>>   	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

That was my original plan, but I think I understand the reasoning behind 
the separation of state and property.

I don't mind this approach too much because the value really feels like 
an implementation detail specific to our driver in this case.

It's not really describing whether the connector is actually capable or 
not, but whether it's fine to perform vrr updates later when we're 
updating the planes.

> 
>> +	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;
>>   };
>>   
>>

Nicholas Kazlauskas
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx