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

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.

};

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

};