On 2018-10-11 04:56 PM, Kazlauskas, Nicholas wrote:
> 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@xxxxxxx>
>>> ---
>>> .../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.
>
Sounds good.
Harry
>>
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
