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

[Lyude Paul <lyude@redhat.com>]

This still needs to be moved into an atomic helper so it can be reused by
other drivers
...
...

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?

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 &&
-- 
Cheers,
	Lyude Paul

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel