Re: [PATCH 15/15] drm/amd/display: Trigger modesets on MST DSC connectors

[Mikita Lipski <mlipski-5C7GfCeVMHo@public.gmane.org>]

On 19.09.2019 20:08, Lyude Paul wrote:
> This still needs to be moved into an atomic helper so it can be reused by
> other drivers

It would be a future to move the implementation to a contained atomic 
helper.

> ...
> ...
> 
> however, I've had this patch series on my mind for a while and something
> occurred to me that would be a lot easier. Why exactly are we not just
> enabling DSC wherever it's supported, regardless of whether or not it's
> actually needed to fit into bandwidth constraints? The reason I mention this
> is while only enabling DSC when needed makes perfect sense for SST, since I'd
> assume non-DSC consumes less power, it doesn't quite make sense for MST.
> 
> See: currently with MST (at least this is the case with i915, but I plan on
> making it the case with nouveau as well), we always default to link training
> at the maximum link rate/lane count. i915 does the opposite with SST for
> optimization, but we maximize for MST because maximizing the amount of
> bandwidth we have to work with means that we can enable new displays and
> disable displays without having to potentially update the rest of the topology
> with new PBN/VCPI allocations (unless the bandwidth restrictions get lowered
> later, of course).
> 
> An abstract example: Let's say a user has a hub setup with two displays,
> without DSC enabled. The user plugs in a third display, but this display won't
> fit into the available bandwidth without enabling DSC. But assuming enabling
> DSC causes us to need to recalculate the bandwidth for the other two displays,
> we end up having to do a modeset on all three displays.
> 
> Alternatively: the user has two displays again. This time though we started
> off by using DSC from the start, so adding a new display can be done without
> performing a modeset on the other two displays.
> 
> Taking this into consideration, wouldn't we be able to just get rid of this
> whole function by enabling DSC by default instead of as-needed? And get a
> nicer result?
The idea of enabling DSC from the beginning does sound appealing, but 
DSC is a lossy compression and we think it is better to enable it only 
when we need it, not to degrade picture quality. The greedy algorithm 
developed by David Francis in his patch "MST DSC compute fair share" 
tries to minimize compression and enable maximum number of streams 
without it, so they won't lose the picture quality.

That being said,  I agree, the implementation of DSC code would be much 
simpler if we would just enable DSC from the beginning, but again we do 
not want to compromise the picture quality.

> 
> On Wed, 2019-09-18 at 16:26 -0400, mikita.lipski@amd.com wrote:
>> From: David Francis <David.Francis@amd.com>
>>
>> Whenever a connector on an MST network is attached, detached, or
>> undergoes a modeset, the DSC configs for each stream on that
>> topology will be recalculated. This can change their required
>> bandwidth, requiring a full reprogramming, as though a modeset
>> was performed, even if that stream did not change timing.
>>
>> Therefore, whenever a crtc has drm_atomic_crtc_needs_modeset,
>> for each crtc that shares a MST topology with that stream and
>> supports DSC, add that crtc (and all affected connectors and
>> planes) to the atomic state and set mode_changed on its state
>>
>> v2: Do this check only on Navi and before adding connectors
>> and planes on modesetting crtcs
>>
>> Cc: Leo Li <sunpeng.li@amd.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: David Francis <David.Francis@amd.com>
>> ---
>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 79 +++++++++++++++++++
>>   1 file changed, 79 insertions(+)
>>
>> 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 ba017e6bf0b4..f65326e85b86 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6704,6 +6704,74 @@ static int do_aquire_global_lock(struct drm_device
>> *dev,
>>   	return ret < 0 ? ret : 0;
>>   }
>>   
>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>> +/*
>> + * TODO: This logic should at some point be moved into DRM
>> + */
>> +static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state,
>> struct drm_crtc *crtc)
>> +{
>> +	struct drm_connector *connector;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_connector_list_iter conn_iter;
>> +	struct drm_crtc_state *new_crtc_state;
>> +	struct amdgpu_dm_connector *aconnector = NULL, *aconnector_to_add;
>> +	int i, j;
>> +	struct drm_crtc *crtcs_affected[AMDGPU_MAX_CRTCS] = { 0 };
>> +
>> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
>> +		if (conn_state->crtc != crtc)
>> +			continue;
>> +
>> +		aconnector = to_amdgpu_dm_connector(connector);
>> +		if (!aconnector->port)
>> +			aconnector = NULL;
>> +		else
>> +			break;
>> +	}
>> +
>> +	if (!aconnector)
>> +		return 0;
>> +
>> +	i = 0;
>> +	drm_connector_list_iter_begin(state->dev, &conn_iter);
>> +	drm_for_each_connector_iter(connector, &conn_iter) {
>> +		if (!connector->state || !connector->state->crtc)
>> +			continue;
>> +
>> +		aconnector_to_add = to_amdgpu_dm_connector(connector);
>> +		if (!aconnector_to_add->port)
>> +			continue;
>> +
>> +		if (aconnector_to_add->port->mgr != aconnector->port->mgr)
>> +			continue;
>> +
>> +		if (!aconnector_to_add->dc_sink)
>> +			continue;
>> +
>> +		if (!aconnector_to_add->dc_sink-
>>> sink_dsc_caps.dsc_dec_caps.is_dsc_supported)
>> +			continue;
>> +
>> +		if (i >= AMDGPU_MAX_CRTCS)
>> +			continue;
>> +
>> +		crtcs_affected[i] = connector->state->crtc;
>> +		i++;
>> +	}
>> +	drm_connector_list_iter_end(&conn_iter);
>> +
>> +	for (j = 0; j < i; j++) {
>> +		new_crtc_state = drm_atomic_get_crtc_state(state,
>> crtcs_affected[j]);
>> +		if (IS_ERR(new_crtc_state))
>> +			return PTR_ERR(new_crtc_state);
>> +
>> +		new_crtc_state->mode_changed = true;
>> +	}
>> +
>> +	return 0;
>> +
>> +}
>> +#endif
>> +
>>   static void get_freesync_config_for_crtc(
>>   	struct dm_crtc_state *new_crtc_state,
>>   	struct dm_connector_state *new_con_state)
>> @@ -7388,6 +7456,17 @@ static int amdgpu_dm_atomic_check(struct drm_device
>> *dev,
>>   	if (ret)
>>   		goto fail;
>>   
>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>> +	if (adev->asic_type >= CHIP_NAVI10) {
>> +		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>> new_crtc_state, i) {
>> +			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>> +				ret = add_affected_mst_dsc_crtcs(state, crtc);
>> +				if (ret)
>> +					goto fail;
>> +			}
>> +		}
>> +	}
>> +#endif
>>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>> new_crtc_state, i) {
>>   		if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
>>   		    !new_crtc_state->color_mgmt_changed &&

-- 
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@amd.com
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx