[PATCH 0/3] Update DC commit sequence

[PATCH 0/3] Update DC commit sequence

[Rodrigo Siqueira]

This patchset introduces the basic infrastructure to change our display
commit sequence to make it more robust and flexible for handling some
display features. Current, amdgpu_dm tries to commit a validation state
directly into update_planes and commit_streams. However, we want to
avoid committing the state directly into DC; instead, we only want to
pass streams/plane pointers into DC and let it manage the commit state
entirely. This behavior is more future-proof because it keeps the state
manipulation inside DC, which can benefit some specific features that
might want to play with the context state.

Cc: Daniel Wheeler <daniel.wheeler@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>

Aurabindo Pillai (1):
  drm/amd/display: Move dc_state copy in commit_tail after
    dc_commit_state

Rodrigo Siqueira (2):
  drm/amd/display: Add functions for new DC commit sequence
  drm/amd/display: Enable new commit sequence only for DCN32x

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  17 +-
 drivers/gpu/drm/amd/display/dc/core/dc.c      | 108 ++++++++++
 .../gpu/drm/amd/display/dc/core/dc_resource.c | 204 +++++++++++++++++-
 drivers/gpu/drm/amd/display/dc/dc.h           |   6 +
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  11 +-
 5 files changed, 333 insertions(+), 13 deletions(-)

-- 
2.35.1

[PATCH 1/3] drm/amd/display: Add functions for new DC commit sequence

[Rodrigo Siqueira]

Since DCN32, our DC commit become more complex and requires more
adjustments to work well with some features. This is a preparation work
for replacing dc_commit_state and dc_commit_updates_for_stream with
dc_update_planes_and_stream and dc_update_planes_and_stream
respectively.

Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Co-developed-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c      |  92 ++++++++
 .../gpu/drm/amd/display/dc/core/dc_resource.c | 204 +++++++++++++++++-
 drivers/gpu/drm/amd/display/dc/dc.h           |   6 +
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  11 +-
 4 files changed, 308 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 660316a536f7..ad3aafb73cba 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -59,6 +59,7 @@
 
 #include "dc_link_dp.h"
 #include "dc_dmub_srv.h"
+#include "dc_stream.h"
 
 #include "dsc.h"
 
@@ -1505,6 +1506,25 @@ static bool context_changed(
 	return false;
 }
 
+static bool streams_changed(struct dc *dc,
+			    struct dc_stream_state *streams[],
+			    uint8_t stream_count)
+{
+	uint8_t i;
+
+	if (stream_count != dc->current_state->stream_count)
+		return true;
+
+	for (i = 0; i < dc->current_state->stream_count; i++) {
+		if (dc->current_state->streams[i] != streams[i])
+			return true;
+		if (!streams[i]->link->link_state_valid)
+			return true;
+	}
+
+	return false;
+}
+
 bool dc_validate_boot_timing(const struct dc *dc,
 				const struct dc_sink *sink,
 				struct dc_crtc_timing *crtc_timing)
@@ -1923,6 +1943,78 @@ bool dc_commit_state(struct dc *dc, struct dc_state *context)
 	return (result == DC_OK);
 }
 
+enum dc_status dc_commit_streams(struct dc *dc,
+				 struct dc_stream_state *streams[],
+				 uint8_t stream_count)
+{
+	int i, j;
+	struct dc_state *context;
+	enum dc_status res = DC_OK;
+	struct dc_validation_set set[MAX_STREAMS] = {0};
+
+	if (dc->ctx->dce_environment == DCE_ENV_VIRTUAL_HW)
+		return res;
+
+	if (!streams_changed(dc, streams, stream_count))
+		return res;
+
+	DC_LOG_DC("%s: %d streams\n", __func__, stream_count);
+
+	for (i = 0; i < stream_count; i++) {
+		struct dc_stream_state *stream = streams[i];
+		struct dc_stream_status *status = dc_stream_get_status(stream);
+
+		dc_stream_log(dc, stream);
+
+		set[i].stream = stream;
+
+		if (status) {
+			set[i].plane_count = status->plane_count;
+			for (j = 0; j < status->plane_count; j++)
+				set[i].plane_states[j] = status->plane_states[j];
+		}
+	}
+
+	context = dc_create_state(dc);
+	if (!context)
+		goto context_alloc_fail;
+
+	dc_resource_state_copy_construct_current(dc, context);
+
+	res = dc_validate_with_context(dc, set, stream_count, context, false);
+	if (res != DC_OK) {
+		BREAK_TO_DEBUGGER();
+		goto fail;
+	}
+
+	res = dc_commit_state_no_check(dc, context);
+
+	for (i = 0; i < stream_count; i++) {
+		for (j = 0; j < context->stream_count; j++) {
+			if (streams[i]->stream_id == context->streams[j]->stream_id)
+				streams[i]->out.otg_offset = context->stream_status[j].primary_otg_inst;
+
+			if (dc_is_embedded_signal(streams[i]->signal)) {
+				struct dc_stream_status *status = dc_stream_get_status_from_state(context, streams[i]);
+
+				if (dc->hwss.is_abm_supported)
+					status->is_abm_supported = dc->hwss.is_abm_supported(dc, context, streams[i]);
+				else
+					status->is_abm_supported = true;
+			}
+		}
+	}
+
+fail:
+	dc_release_state(context);
+
+context_alloc_fail:
+
+	DC_LOG_DC("%s Finished.\n", __func__);
+
+	return res;
+}
+
 bool dc_acquire_release_mpc_3dlut(
 		struct dc *dc, bool acquire,
 		struct dc_stream_state *stream,
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
index fd8db482e56f..0c58a19e9115 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
@@ -2593,6 +2593,208 @@ bool dc_resource_is_dsc_encoding_supported(const struct dc *dc)
 	return dc->res_pool->res_cap->num_dsc > 0;
 }
 
+static bool planes_changed_for_existing_stream(struct dc_state *context,
+					       struct dc_stream_state *stream,
+					       const struct dc_validation_set set[],
+					       int set_count)
+{
+	int i, j;
+	struct dc_stream_status *stream_status = NULL;
+
+	for (i = 0; i < context->stream_count; i++)
+		if (context->streams[i] == stream) {
+			stream_status = &context->stream_status[i];
+			break;
+		}
+
+	if (!stream_status)
+		ASSERT(0);
+
+	for (i = 0; i < set_count; i++)
+		if (set[i].stream == stream)
+			break;
+
+	if (i == set_count)
+		ASSERT(0);
+
+	if (set[i].plane_count != stream_status->plane_count)
+		return true;
+
+	for (j = 0; j < set[i].plane_count; j++)
+		if (set[i].plane_states[j] != stream_status->plane_states[j])
+			return true;
+
+	return false;
+}
+
+enum dc_status dc_validate_with_context(struct dc *dc,
+					const struct dc_validation_set set[],
+					int set_count,
+					struct dc_state *context,
+					bool fast_validate)
+{
+	struct dc_stream_state *unchanged_streams[MAX_PIPES] = { 0 };
+	struct dc_stream_state *del_streams[MAX_PIPES] = { 0 };
+	struct dc_stream_state *add_streams[MAX_PIPES] = { 0 };
+	int old_stream_count = context->stream_count;
+	enum dc_status res = DC_ERROR_UNEXPECTED;
+	int unchanged_streams_count = 0;
+	int del_streams_count = 0;
+	int add_streams_count = 0;
+	bool found = false;
+	int i, j, k;
+
+	DC_LOGGER_INIT(dc->ctx->logger);
+
+	/* First remove from context all deleted streams */
+	for (i = 0; i < old_stream_count; i++) {
+		struct dc_stream_state *stream = context->streams[i];
+
+		for (j = 0; j < set_count; j++) {
+			if (stream == set[j].stream) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+			del_streams[del_streams_count++] = stream;
+
+		found = false;
+	}
+
+	/* Now add new streams */
+	for (i = 0; i < set_count; i++) {
+		struct dc_stream_state *stream = set[i].stream;
+
+		for (j = 0; j < old_stream_count; j++) {
+			if (stream == context->streams[j]) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+			add_streams[add_streams_count++] = stream;
+
+		found = false;
+	}
+
+	/*
+	 * Build array of unchanged  streams, it is needed to handle
+	 * planes only changes (added/removed/updated)
+	 */
+	for (i = 0; i < set_count; i++) {
+		for (j = 0; j < del_streams_count; j++) {
+			if (set[i].stream == del_streams[j]) {
+				found = true;
+				break;
+			}
+		}
+
+		if (!found) {
+			for (j = 0; j < add_streams_count; j++) {
+				if (set[i].stream == add_streams[j]) {
+					found = true;
+					break;
+				}
+			}
+		}
+
+		if (!found)
+			unchanged_streams[unchanged_streams_count++] = set[i].stream;
+
+		found = false;
+	}
+
+	/* Remove all planes for unchanged streams if planes changed */
+	for (i = 0; i < unchanged_streams_count; i++) {
+		if (planes_changed_for_existing_stream(context,
+						       unchanged_streams[i],
+						       set,
+						       set_count)) {
+			if (!dc_rem_all_planes_for_stream(dc,
+							  unchanged_streams[i],
+							  context)) {
+				res = DC_FAIL_DETACH_SURFACES;
+				goto fail;
+			}
+		}
+	}
+
+	/* Remove all planes for removed streams and then remove the streams */
+	for (i = 0; i < del_streams_count; i++) {
+		/* Need to cpy the dwb data from the old stream in order to efc to work */
+		if (del_streams[i]->num_wb_info > 0) {
+			for (j = 0; j < add_streams_count; j++) {
+				if (del_streams[i]->sink == add_streams[j]->sink) {
+					add_streams[j]->num_wb_info = del_streams[i]->num_wb_info;
+					for (k = 0; k < del_streams[i]->num_wb_info; k++)
+						add_streams[j]->writeback_info[k] = del_streams[i]->writeback_info[k];
+				}
+			}
+		}
+
+		if (!dc_rem_all_planes_for_stream(dc, del_streams[i], context)) {
+			res = DC_FAIL_DETACH_SURFACES;
+			goto fail;
+		}
+
+		res = dc_remove_stream_from_ctx(dc, context, del_streams[i]);
+		if (res != DC_OK)
+			goto fail;
+	}
+
+	/* Swap seamless boot stream to pipe 0 (if needed) to ensure pipe_ctx matches.
+	 * This may change in the future if seamless_boot_stream can be multiple.
+	 */
+	for (i = 0; i < add_streams_count; i++) {
+		mark_seamless_boot_stream(dc, add_streams[i]);
+		if (add_streams[i]->apply_seamless_boot_optimization && i != 0) {
+			struct dc_stream_state *temp = add_streams[0];
+
+			add_streams[0] = add_streams[i];
+			add_streams[i] = temp;
+			break;
+		}
+	}
+
+	/* Add new streams and then add all planes for the new stream */
+	for (i = 0; i < add_streams_count; i++) {
+		calculate_phy_pix_clks(add_streams[i]);
+		res = dc_add_stream_to_ctx(dc, context, add_streams[i]);
+		if (res != DC_OK)
+			goto fail;
+
+		if (!add_all_planes_for_stream(dc, add_streams[i], set, set_count, context)) {
+			res = DC_FAIL_ATTACH_SURFACES;
+			goto fail;
+		}
+	}
+
+	/* Add all planes for unchanged streams if planes changed */
+	for (i = 0; i < unchanged_streams_count; i++) {
+		if (planes_changed_for_existing_stream(context,
+						       unchanged_streams[i],
+						       set,
+						       set_count)) {
+			if (!add_all_planes_for_stream(dc, unchanged_streams[i], set, set_count, context)) {
+				res = DC_FAIL_ATTACH_SURFACES;
+				goto fail;
+			}
+		}
+	}
+
+	res = dc_validate_global_state(dc, context, fast_validate);
+
+fail:
+	if (res != DC_OK) {
+		DC_LOG_WARNING("%s:resource validation failed, dc_status:%d\n",
+			       __func__,
+			       res);
+	}
+	return res;
+}
 
 /**
  * dc_validate_global_state() - Determine if HW can support a given state
@@ -3734,4 +3936,4 @@ bool dc_resource_acquire_secondary_pipe_for_mpc_odm(
 	}
 
 	return true;
-}
\ No newline at end of file
+}
diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
index bfc5474c0f4c..2753d9eabd16 100644
--- a/drivers/gpu/drm/amd/display/dc/dc.h
+++ b/drivers/gpu/drm/amd/display/dc/dc.h
@@ -1286,6 +1286,12 @@ enum dc_status dc_validate_plane(struct dc *dc, const struct dc_plane_state *pla
 
 void get_clock_requirements_for_state(struct dc_state *state, struct AsicStateEx *info);
 
+enum dc_status dc_validate_with_context(struct dc *dc,
+					const struct dc_validation_set set[],
+					int set_count,
+					struct dc_state *context,
+					bool fast_validate);
+
 bool dc_set_generic_gpio_for_stereo(bool enable,
 		struct gpio_service *gpio_service);
 
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index 9e6025c98db9..d5b3c80635ea 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -378,10 +378,9 @@ bool dc_stream_get_scanoutpos(const struct dc_stream_state *stream,
 				  uint32_t *h_position,
 				  uint32_t *v_position);
 
-enum dc_status dc_add_stream_to_ctx(
-			struct dc *dc,
-		struct dc_state *new_ctx,
-		struct dc_stream_state *stream);
+enum dc_status dc_add_stream_to_ctx(struct dc *dc,
+				    struct dc_state *new_ctx,
+				    struct dc_stream_state *stream);
 
 enum dc_status dc_remove_stream_from_ctx(
 		struct dc *dc,
@@ -445,6 +444,10 @@ enum dc_status dc_validate_stream(struct dc *dc, struct dc_stream_state *stream)
  *   Phy, Encoder, Timing Generator are programmed and enabled.
  *   New streams are enabled with blank stream; no memory read.
  */
+enum dc_status dc_commit_streams(struct dc *dc,
+				 struct dc_stream_state *streams[],
+				 uint8_t stream_count);
+
 /*
  * Enable stereo when commit_streams is not required,
  * for example, frame alternate.
-- 
2.35.1

[PATCH 2/3] drm/amd/display: Enable new commit sequence only for DCN32x

[Rodrigo Siqueira]

Change commit sequence might impact multiple ASICs; for this reason, it
is better to run it only on DCN32x for the first set of validation.
After we get more confident about this change, we should expand it to
all ASICs.

Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Co-developed-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
index ad3aafb73cba..21d20900cfe3 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -1913,6 +1913,13 @@ bool dc_commit_state(struct dc *dc, struct dc_state *context)
 	enum dc_status result = DC_ERROR_UNEXPECTED;
 	int i;
 
+	/* TODO: Enable the new commit sequence for all ASICs */
+	if (dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_0 ||
+	    dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_1) {
+		result = dc_commit_streams(dc, context->streams, context->stream_count);
+		return result == DC_OK;
+	}
+
 	if (!context_changed(dc, context))
 		return DC_OK;
 
@@ -3898,6 +3905,15 @@ void dc_commit_updates_for_stream(struct dc *dc,
 	struct dc_context *dc_ctx = dc->ctx;
 	int i, j;
 
+	/* TODO: Enable the new commit sequence for all ASICs */
+	if (dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_0 ||
+	    dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_1) {
+		dc_update_planes_and_stream(dc, srf_updates,
+					    surface_count, stream,
+					    stream_update);
+		return;
+	}
+
 	stream_status = dc_stream_get_status(stream);
 	context = dc->current_state;
 
-- 
2.35.1

[PATCH 3/3] drm/amd/display: Move dc_state copy in commit_tail after dc_commit_state

[Rodrigo Siqueira]

From: Aurabindo Pillai <aurabindo.pillai@amd.com>

[Why&How]
With the new commit sequence, we do not want the state to be copied before
the call to dc_commit_state() since this leaks the phantom streams into
new state.

Fix this by doing the dc state copy right after the dc_commit_state()
call.

Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 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 63f076a46260..17a9108f8186 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7999,15 +7999,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
 
 	dm_state = dm_atomic_get_new_state(state);
-	if (dm_state && dm_state->context) {
+	if (dm_state && dm_state->context)
 		dc_state = dm_state->context;
-	} else {
-		/* No state changes, retain current state. */
-		dc_state_temp = dc_create_state(dm->dc);
-		ASSERT(dc_state_temp);
-		dc_state = dc_state_temp;
-		dc_resource_state_copy_construct_current(dm->dc, dc_state);
-	}
 
 	for_each_oldnew_crtc_in_state (state, crtc, old_crtc_state,
 				       new_crtc_state, i) {
@@ -8127,6 +8120,14 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		mutex_unlock(&dm->dc_lock);
 	}
 
+	if (dc_state == NULL) {
+		/* No state changes, retain current state. */
+		dc_state_temp = dc_create_state(dm->dc);
+		ASSERT(dc_state_temp);
+		dc_state = dc_state_temp;
+		dc_resource_state_copy_construct_current(dm->dc, dc_state);
+	}
+
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-- 
2.35.1

Re: [PATCH 1/3] drm/amd/display: Add functions for new DC commit sequence

[Harry Wentland]

On 2022-10-11 14:11, Rodrigo Siqueira wrote:
> Since DCN32, our DC commit become more complex and requires more
> adjustments to work well with some features. This is a preparation work
> for replacing dc_commit_state and dc_commit_updates_for_stream with
> dc_update_planes_and_stream and dc_update_planes_and_stream

I think you mean
"dc_commit_streams and dc_update_planes_and_stream"

Harry

> respectively.
> 
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Co-developed-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc.c      |  92 ++++++++
>  .../gpu/drm/amd/display/dc/core/dc_resource.c | 204 +++++++++++++++++-
>  drivers/gpu/drm/amd/display/dc/dc.h           |   6 +
>  drivers/gpu/drm/amd/display/dc/dc_stream.h    |  11 +-
>  4 files changed, 308 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index 660316a536f7..ad3aafb73cba 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -59,6 +59,7 @@
>  
>  #include "dc_link_dp.h"
>  #include "dc_dmub_srv.h"
> +#include "dc_stream.h"
>  
>  #include "dsc.h"
>  
> @@ -1505,6 +1506,25 @@ static bool context_changed(
>  	return false;
>  }
>  
> +static bool streams_changed(struct dc *dc,
> +			    struct dc_stream_state *streams[],
> +			    uint8_t stream_count)
> +{
> +	uint8_t i;
> +
> +	if (stream_count != dc->current_state->stream_count)
> +		return true;
> +
> +	for (i = 0; i < dc->current_state->stream_count; i++) {
> +		if (dc->current_state->streams[i] != streams[i])
> +			return true;
> +		if (!streams[i]->link->link_state_valid)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  bool dc_validate_boot_timing(const struct dc *dc,
>  				const struct dc_sink *sink,
>  				struct dc_crtc_timing *crtc_timing)
> @@ -1923,6 +1943,78 @@ bool dc_commit_state(struct dc *dc, struct dc_state *context)
>  	return (result == DC_OK);
>  }
>  
> +enum dc_status dc_commit_streams(struct dc *dc,
> +				 struct dc_stream_state *streams[],
> +				 uint8_t stream_count)
> +{
> +	int i, j;
> +	struct dc_state *context;
> +	enum dc_status res = DC_OK;
> +	struct dc_validation_set set[MAX_STREAMS] = {0};
> +
> +	if (dc->ctx->dce_environment == DCE_ENV_VIRTUAL_HW)
> +		return res;
> +
> +	if (!streams_changed(dc, streams, stream_count))
> +		return res;
> +
> +	DC_LOG_DC("%s: %d streams\n", __func__, stream_count);
> +
> +	for (i = 0; i < stream_count; i++) {
> +		struct dc_stream_state *stream = streams[i];
> +		struct dc_stream_status *status = dc_stream_get_status(stream);
> +
> +		dc_stream_log(dc, stream);
> +
> +		set[i].stream = stream;
> +
> +		if (status) {
> +			set[i].plane_count = status->plane_count;
> +			for (j = 0; j < status->plane_count; j++)
> +				set[i].plane_states[j] = status->plane_states[j];
> +		}
> +	}
> +
> +	context = dc_create_state(dc);
> +	if (!context)
> +		goto context_alloc_fail;
> +
> +	dc_resource_state_copy_construct_current(dc, context);
> +
> +	res = dc_validate_with_context(dc, set, stream_count, context, false);
> +	if (res != DC_OK) {
> +		BREAK_TO_DEBUGGER();
> +		goto fail;
> +	}
> +
> +	res = dc_commit_state_no_check(dc, context);
> +
> +	for (i = 0; i < stream_count; i++) {
> +		for (j = 0; j < context->stream_count; j++) {
> +			if (streams[i]->stream_id == context->streams[j]->stream_id)
> +				streams[i]->out.otg_offset = context->stream_status[j].primary_otg_inst;
> +
> +			if (dc_is_embedded_signal(streams[i]->signal)) {
> +				struct dc_stream_status *status = dc_stream_get_status_from_state(context, streams[i]);
> +
> +				if (dc->hwss.is_abm_supported)
> +					status->is_abm_supported = dc->hwss.is_abm_supported(dc, context, streams[i]);
> +				else
> +					status->is_abm_supported = true;
> +			}
> +		}
> +	}
> +
> +fail:
> +	dc_release_state(context);
> +
> +context_alloc_fail:
> +
> +	DC_LOG_DC("%s Finished.\n", __func__);
> +
> +	return res;
> +}
> +
>  bool dc_acquire_release_mpc_3dlut(
>  		struct dc *dc, bool acquire,
>  		struct dc_stream_state *stream,
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> index fd8db482e56f..0c58a19e9115 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_resource.c
> @@ -2593,6 +2593,208 @@ bool dc_resource_is_dsc_encoding_supported(const struct dc *dc)
>  	return dc->res_pool->res_cap->num_dsc > 0;
>  }
>  
> +static bool planes_changed_for_existing_stream(struct dc_state *context,
> +					       struct dc_stream_state *stream,
> +					       const struct dc_validation_set set[],
> +					       int set_count)
> +{
> +	int i, j;
> +	struct dc_stream_status *stream_status = NULL;
> +
> +	for (i = 0; i < context->stream_count; i++)
> +		if (context->streams[i] == stream) {
> +			stream_status = &context->stream_status[i];
> +			break;
> +		}
> +
> +	if (!stream_status)
> +		ASSERT(0);
> +
> +	for (i = 0; i < set_count; i++)
> +		if (set[i].stream == stream)
> +			break;
> +
> +	if (i == set_count)
> +		ASSERT(0);
> +
> +	if (set[i].plane_count != stream_status->plane_count)
> +		return true;
> +
> +	for (j = 0; j < set[i].plane_count; j++)
> +		if (set[i].plane_states[j] != stream_status->plane_states[j])
> +			return true;
> +
> +	return false;
> +}
> +
> +enum dc_status dc_validate_with_context(struct dc *dc,
> +					const struct dc_validation_set set[],
> +					int set_count,
> +					struct dc_state *context,
> +					bool fast_validate)
> +{
> +	struct dc_stream_state *unchanged_streams[MAX_PIPES] = { 0 };
> +	struct dc_stream_state *del_streams[MAX_PIPES] = { 0 };
> +	struct dc_stream_state *add_streams[MAX_PIPES] = { 0 };
> +	int old_stream_count = context->stream_count;
> +	enum dc_status res = DC_ERROR_UNEXPECTED;
> +	int unchanged_streams_count = 0;
> +	int del_streams_count = 0;
> +	int add_streams_count = 0;
> +	bool found = false;
> +	int i, j, k;
> +
> +	DC_LOGGER_INIT(dc->ctx->logger);
> +
> +	/* First remove from context all deleted streams */
> +	for (i = 0; i < old_stream_count; i++) {
> +		struct dc_stream_state *stream = context->streams[i];
> +
> +		for (j = 0; j < set_count; j++) {
> +			if (stream == set[j].stream) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found)
> +			del_streams[del_streams_count++] = stream;
> +
> +		found = false;
> +	}
> +
> +	/* Now add new streams */
> +	for (i = 0; i < set_count; i++) {
> +		struct dc_stream_state *stream = set[i].stream;
> +
> +		for (j = 0; j < old_stream_count; j++) {
> +			if (stream == context->streams[j]) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found)
> +			add_streams[add_streams_count++] = stream;
> +
> +		found = false;
> +	}
> +
> +	/*
> +	 * Build array of unchanged  streams, it is needed to handle
> +	 * planes only changes (added/removed/updated)
> +	 */
> +	for (i = 0; i < set_count; i++) {
> +		for (j = 0; j < del_streams_count; j++) {
> +			if (set[i].stream == del_streams[j]) {
> +				found = true;
> +				break;
> +			}
> +		}
> +
> +		if (!found) {
> +			for (j = 0; j < add_streams_count; j++) {
> +				if (set[i].stream == add_streams[j]) {
> +					found = true;
> +					break;
> +				}
> +			}
> +		}
> +
> +		if (!found)
> +			unchanged_streams[unchanged_streams_count++] = set[i].stream;
> +
> +		found = false;
> +	}
> +
> +	/* Remove all planes for unchanged streams if planes changed */
> +	for (i = 0; i < unchanged_streams_count; i++) {
> +		if (planes_changed_for_existing_stream(context,
> +						       unchanged_streams[i],
> +						       set,
> +						       set_count)) {
> +			if (!dc_rem_all_planes_for_stream(dc,
> +							  unchanged_streams[i],
> +							  context)) {
> +				res = DC_FAIL_DETACH_SURFACES;
> +				goto fail;
> +			}
> +		}
> +	}
> +
> +	/* Remove all planes for removed streams and then remove the streams */
> +	for (i = 0; i < del_streams_count; i++) {
> +		/* Need to cpy the dwb data from the old stream in order to efc to work */
> +		if (del_streams[i]->num_wb_info > 0) {
> +			for (j = 0; j < add_streams_count; j++) {
> +				if (del_streams[i]->sink == add_streams[j]->sink) {
> +					add_streams[j]->num_wb_info = del_streams[i]->num_wb_info;
> +					for (k = 0; k < del_streams[i]->num_wb_info; k++)
> +						add_streams[j]->writeback_info[k] = del_streams[i]->writeback_info[k];
> +				}
> +			}
> +		}
> +
> +		if (!dc_rem_all_planes_for_stream(dc, del_streams[i], context)) {
> +			res = DC_FAIL_DETACH_SURFACES;
> +			goto fail;
> +		}
> +
> +		res = dc_remove_stream_from_ctx(dc, context, del_streams[i]);
> +		if (res != DC_OK)
> +			goto fail;
> +	}
> +
> +	/* Swap seamless boot stream to pipe 0 (if needed) to ensure pipe_ctx matches.
> +	 * This may change in the future if seamless_boot_stream can be multiple.
> +	 */
> +	for (i = 0; i < add_streams_count; i++) {
> +		mark_seamless_boot_stream(dc, add_streams[i]);
> +		if (add_streams[i]->apply_seamless_boot_optimization && i != 0) {
> +			struct dc_stream_state *temp = add_streams[0];
> +
> +			add_streams[0] = add_streams[i];
> +			add_streams[i] = temp;
> +			break;
> +		}
> +	}
> +
> +	/* Add new streams and then add all planes for the new stream */
> +	for (i = 0; i < add_streams_count; i++) {
> +		calculate_phy_pix_clks(add_streams[i]);
> +		res = dc_add_stream_to_ctx(dc, context, add_streams[i]);
> +		if (res != DC_OK)
> +			goto fail;
> +
> +		if (!add_all_planes_for_stream(dc, add_streams[i], set, set_count, context)) {
> +			res = DC_FAIL_ATTACH_SURFACES;
> +			goto fail;
> +		}
> +	}
> +
> +	/* Add all planes for unchanged streams if planes changed */
> +	for (i = 0; i < unchanged_streams_count; i++) {
> +		if (planes_changed_for_existing_stream(context,
> +						       unchanged_streams[i],
> +						       set,
> +						       set_count)) {
> +			if (!add_all_planes_for_stream(dc, unchanged_streams[i], set, set_count, context)) {
> +				res = DC_FAIL_ATTACH_SURFACES;
> +				goto fail;
> +			}
> +		}
> +	}
> +
> +	res = dc_validate_global_state(dc, context, fast_validate);
> +
> +fail:
> +	if (res != DC_OK) {
> +		DC_LOG_WARNING("%s:resource validation failed, dc_status:%d\n",
> +			       __func__,
> +			       res);
> +	}
> +	return res;
> +}
>  
>  /**
>   * dc_validate_global_state() - Determine if HW can support a given state
> @@ -3734,4 +3936,4 @@ bool dc_resource_acquire_secondary_pipe_for_mpc_odm(
>  	}
>  
>  	return true;
> -}
> \ No newline at end of file
> +}
> diff --git a/drivers/gpu/drm/amd/display/dc/dc.h b/drivers/gpu/drm/amd/display/dc/dc.h
> index bfc5474c0f4c..2753d9eabd16 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc.h
> @@ -1286,6 +1286,12 @@ enum dc_status dc_validate_plane(struct dc *dc, const struct dc_plane_state *pla
>  
>  void get_clock_requirements_for_state(struct dc_state *state, struct AsicStateEx *info);
>  
> +enum dc_status dc_validate_with_context(struct dc *dc,
> +					const struct dc_validation_set set[],
> +					int set_count,
> +					struct dc_state *context,
> +					bool fast_validate);
> +
>  bool dc_set_generic_gpio_for_stereo(bool enable,
>  		struct gpio_service *gpio_service);
>  
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> index 9e6025c98db9..d5b3c80635ea 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> @@ -378,10 +378,9 @@ bool dc_stream_get_scanoutpos(const struct dc_stream_state *stream,
>  				  uint32_t *h_position,
>  				  uint32_t *v_position);
>  
> -enum dc_status dc_add_stream_to_ctx(
> -			struct dc *dc,
> -		struct dc_state *new_ctx,
> -		struct dc_stream_state *stream);
> +enum dc_status dc_add_stream_to_ctx(struct dc *dc,
> +				    struct dc_state *new_ctx,
> +				    struct dc_stream_state *stream);
>  
>  enum dc_status dc_remove_stream_from_ctx(
>  		struct dc *dc,
> @@ -445,6 +444,10 @@ enum dc_status dc_validate_stream(struct dc *dc, struct dc_stream_state *stream)
>   *   Phy, Encoder, Timing Generator are programmed and enabled.
>   *   New streams are enabled with blank stream; no memory read.
>   */
> +enum dc_status dc_commit_streams(struct dc *dc,
> +				 struct dc_stream_state *streams[],
> +				 uint8_t stream_count);
> +
>  /*
>   * Enable stereo when commit_streams is not required,
>   * for example, frame alternate.

RE: [PATCH 0/3] Update DC commit sequence

[Wheeler, Daniel]

[Public]

Hi all,

I tested these patches as well, they were added to the most recent patchset.


Tested-by: Daniel Wheeler <daniel.wheeler@amd.com>

Thank you,

Dan Wheeler
Sr. Technologist  |  AMD
SW Display
------------------------------------------------------------------------------------------------------------------
1 Commerce Valley Dr E, Thornhill, ON L3T 7X6
Facebook |  Twitter |  amd.com  


-----Original Message-----
From: Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com> 
Sent: October 11, 2022 2:12 PM
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Wheeler, Daniel <Daniel.Wheeler@amd.com>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>
Subject: [PATCH 0/3] Update DC commit sequence

This patchset introduces the basic infrastructure to change our display commit sequence to make it more robust and flexible for handling some display features. Current, amdgpu_dm tries to commit a validation state directly into update_planes and commit_streams. However, we want to avoid committing the state directly into DC; instead, we only want to pass streams/plane pointers into DC and let it manage the commit state entirely. This behavior is more future-proof because it keeps the state manipulation inside DC, which can benefit some specific features that might want to play with the context state.

Cc: Daniel Wheeler <daniel.wheeler@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>

Aurabindo Pillai (1):
  drm/amd/display: Move dc_state copy in commit_tail after
    dc_commit_state

Rodrigo Siqueira (2):
  drm/amd/display: Add functions for new DC commit sequence
  drm/amd/display: Enable new commit sequence only for DCN32x

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  17 +-
 drivers/gpu/drm/amd/display/dc/core/dc.c      | 108 ++++++++++
 .../gpu/drm/amd/display/dc/core/dc_resource.c | 204 +++++++++++++++++-
 drivers/gpu/drm/amd/display/dc/dc.h           |   6 +
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  11 +-
 5 files changed, 333 insertions(+), 13 deletions(-)

--
2.35.1

Re: [PATCH 3/3] drm/amd/display: Move dc_state copy in commit_tail after dc_commit_state

[Harry Wentland]

On 2022-10-11 14:11, Rodrigo Siqueira wrote:
> From: Aurabindo Pillai <aurabindo.pillai@amd.com>
> 
> [Why&How]
> With the new commit sequence, we do not want the state to be copied before
> the call to dc_commit_state() since this leaks the phantom streams into
> new state.
> 

What are phantom streams? Why are they needed? Please elaborate.

> Fix this by doing the dc state copy right after the dc_commit_state()
> call.
> 
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 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 63f076a46260..17a9108f8186 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7999,15 +7999,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  	drm_atomic_helper_update_legacy_modeset_state(dev, state);
>  
>  	dm_state = dm_atomic_get_new_state(state);
> -	if (dm_state && dm_state->context) {
> +	if (dm_state && dm_state->context)
>  		dc_state = dm_state->context;
> -	} else {
> -		/* No state changes, retain current state. */
> -		dc_state_temp = dc_create_state(dm->dc);
> -		ASSERT(dc_state_temp);
> -		dc_state = dc_state_temp;
> -		dc_resource_state_copy_construct_current(dm->dc, dc_state);
> -	}

If I understand this right this change means that when we do a
"fast" update we'll simply modify the existing dc_state directly,
instead of operating on a copy. Is this safe? Keep in mind that
calls in amdgpu_dm can be multi-threaded, unlike calls on other
OSes that use our driver.

In particular, it looks like dm_enable_per_frame_crtc_master_sync
is called on dc_state before the dc_lock is taken. Could this
lead to issues similar to the ones outlined by Nick in his commit
when he wrote this code: 
eb3dc8978596 ("drm/amd/display: Use private obj helpers for dm_atomic_state")

>  
>  	for_each_oldnew_crtc_in_state (state, crtc, old_crtc_state,
>  				       new_crtc_state, i) {
> @@ -8127,6 +8120,14 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>  		mutex_unlock(&dm->dc_lock);
>  	}
>  
> +	if (dc_state == NULL) {
> +		/* No state changes, retain current state. */
> +		dc_state_temp = dc_create_state(dm->dc);
> +		ASSERT(dc_state_temp);
> +		dc_state = dc_state_temp;
> +		dc_resource_state_copy_construct_current(dm->dc, dc_state);
> +	}
> +

Does moving this down serve a purpose other than the one to preserve
this code?

Moving this means that dc_commit_state (now delegating to
dc_commit_streams on some ASICs) is operating on dc->current_state
whereas dc_commit_updates_for_stream (now delegating to
dc_update_planes_and_stream on some ASICs) is operating on
the state copy, i.e. the local dc_state.

Is this really the intention? Will this make our lives easier or
harder? This code is already hard to parse. I fail to see how this
will improve that situation.

Harry

>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  

Re: [PATCH 2/3] drm/amd/display: Enable new commit sequence only for DCN32x

[Harry Wentland]

On 2022-10-11 14:11, Rodrigo Siqueira wrote:
> Change commit sequence might impact multiple ASICs; for this reason, it
> is better to run it only on DCN32x for the first set of validation.
> After we get more confident about this change, we should expand it to
> all ASICs.
> 
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> Co-developed-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c
> index ad3aafb73cba..21d20900cfe3 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> @@ -1913,6 +1913,13 @@ bool dc_commit_state(struct dc *dc, struct dc_state *context)
>  	enum dc_status result = DC_ERROR_UNEXPECTED;
>  	int i;
>  
> +	/* TODO: Enable the new commit sequence for all ASICs */
> +	if (dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_0 ||
> +	    dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_1) {

Why not check that dc_ctx->dce_version == DCN_VERSION_3_2?

I don't like using the family ID in DC unless we have a good
reason to.

> +		result = dc_commit_streams(dc, context->streams, context->stream_count);

Wasn't this function initially intended as a crutch for other OSes
that couldn't use dc_commit_state directly for some reasons?

I'm not sure this change is moving into the right direction, at least
not without a much more detailed explanation of why dc_commit_streams
and dc_update_planes_and_streams are the better solution.

Harry

> +		return result == DC_OK;
> +	}
> +
>  	if (!context_changed(dc, context))
>  		return DC_OK;
>  
> @@ -3898,6 +3905,15 @@ void dc_commit_updates_for_stream(struct dc *dc,
>  	struct dc_context *dc_ctx = dc->ctx;
>  	int i, j;
>  
> +	/* TODO: Enable the new commit sequence for all ASICs */
> +	if (dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_0 ||
> +	    dc->ctx->asic_id.chip_family == AMDGPU_FAMILY_GC_11_0_1) {
> +		dc_update_planes_and_stream(dc, srf_updates,
> +					    surface_count, stream,
> +					    stream_update);
> +		return;
> +	}
> +
>  	stream_status = dc_stream_get_status(stream);
>  	context = dc->current_state;
>