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;elsebreak;- }
- 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 &&