[PATCH v5] drm/amd/display: Add MST atomic routines

[PATCH v5] drm/amd/display: Add MST atomic routines

[mikita.lipski-5C7GfCeVMHo]

From: Mikita Lipski <mikita.lipski@amd.com>

- Adding encoder atomic check to find vcpi slots for a connector
- Using DRM helper functions to calculate PBN
- Adding connector atomic check to release vcpi slots if connector
loses CRTC
- Calculate  PBN and VCPI slots only once during atomic
check and store them on crtc_state to eliminate
redundant calculation
- Call drm_dp_mst_atomic_check to verify validity of MST topology
during state atomic check

v2: squashed previous 3 separate patches, removed DSC PBN calculation,
and added PBN and VCPI slots properties to amdgpu connector

v3:
- moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
- updates stream's vcpi_slots and pbn on commit
- separated patch from the DSC MST series

v4:
- set vcpi_slots and pbn properties to dm_connector_state
- copy porperties from connector state on to crtc state

v5:
- keep the pbn and vcpi values only on connnector state
- added a void pointer to the stream state instead on two ints,
because dc_stream_state is OS agnostic. Pointer points to the
current dm_connector_state.

Cc: Jun Lei <Jun.Lei@amd.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
 5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 	}
 
 	stream->dm_stream_context = aconnector;
+	stream->dm_stream_state = dm_state;
 
 	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
 		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
@@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
 		state->underscan_hborder = 0;
 		state->underscan_vborder = 0;
 		state->base.max_requested_bpc = 8;
-
+		state->vcpi_slots = 0;
+		state->pbn = 0;
 		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
 			state->abm_level = amdgpu_dm_abm_level;
 
@@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
 	new_state->underscan_enable = state->underscan_enable;
 	new_state->underscan_hborder = state->underscan_hborder;
 	new_state->underscan_vborder = state->underscan_vborder;
-
+	new_state->vcpi_slots = state->vcpi_slots;
+	new_state->pbn = state->pbn;
 	return &new_state->base;
 }
 
@@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 					  struct drm_crtc_state *crtc_state,
 					  struct drm_connector_state *conn_state)
 {
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_connector *connector = conn_state->connector;
+	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
+	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	struct drm_dp_mst_topology_mgr *mst_mgr;
+	struct drm_dp_mst_port *mst_port;
+	int clock, bpp = 0;
+
+	if (!aconnector->port || !aconnector->dc_sink)
+		return 0;
+
+	mst_port = aconnector->port;
+	mst_mgr = &aconnector->mst_port->mst_mgr;
+
+	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
+		return 0;
+
+	if (!state->duplicated) {
+		bpp = (uint8_t)connector->display_info.bpc * 3;
+		clock = adjusted_mode->clock;
+		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
+	}
+	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
+							       mst_mgr,
+							       mst_port,
+							       dm_new_connector_state->pbn);
+	if (dm_new_connector_state->vcpi_slots < 0) {
+		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
+		return dm_new_connector_state->vcpi_slots;
+	}
 	return 0;
 }
 
@@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		hdr_changed =
 			is_hdr_metadata_different(old_con_state, new_con_state);
 
+		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
+
 		if (!scaling_changed && !abm_changed && !hdr_changed)
 			continue;
 
@@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			stream_update.hdr_static_metadata = &hdr_packet;
 		}
 
+
 		status = dc_stream_get_status(dm_new_crtc_state->stream);
 		WARN_ON(!status);
 		WARN_ON(!status->plane_count);
@@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	/* Perform validation of MST topology in the state*/
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		goto fail;
+
 	if (state->legacy_cursor_update) {
 		/*
 		 * This is a fast cursor update coming from the plane update
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 c6fdebee7189..910c8598faf9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -360,6 +360,8 @@ struct dm_connector_state {
 	bool freesync_enable;
 	bool freesync_capable;
 	uint8_t abm_level;
+	uint64_t vcpi_slots;
+	uint64_t pbn;
 };
 
 #define to_dm_connector_state(x)\
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 11e5784aa62a..821d61e060b2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 		bool enable)
 {
 	struct amdgpu_dm_connector *aconnector;
+	struct dm_connector_state *dm_conn_state;
 	struct drm_dp_mst_topology_mgr *mst_mgr;
 	struct drm_dp_mst_port *mst_port;
-	int slots = 0;
 	bool ret;
-	int clock;
-	int bpp = 0;
-	int pbn = 0;
 
 	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
+	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
 
 	if (!aconnector || !aconnector->mst_port)
 		return false;
@@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 	mst_port = aconnector->port;
 
 	if (enable) {
-		clock = stream->timing.pix_clk_100hz / 10;
-
-		switch (stream->timing.display_color_depth) {
-
-		case COLOR_DEPTH_666:
-			bpp = 6;
-			break;
-		case COLOR_DEPTH_888:
-			bpp = 8;
-			break;
-		case COLOR_DEPTH_101010:
-			bpp = 10;
-			break;
-		case COLOR_DEPTH_121212:
-			bpp = 12;
-			break;
-		case COLOR_DEPTH_141414:
-			bpp = 14;
-			break;
-		case COLOR_DEPTH_161616:
-			bpp = 16;
-			break;
-		default:
-			ASSERT(bpp != 0);
-			break;
-		}
-
-		bpp = bpp * 3;
-
-		/* TODO need to know link rate */
-
-		pbn = drm_dp_calc_pbn_mode(clock, bpp);
-
-		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
-		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
 
+		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
+					       dm_conn_state->pbn,
+					       dm_conn_state->vcpi_slots);
 		if (!ret)
 			return false;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 779d0b60cac9..1a17ea1b42e0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
 	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
 }
 
+static int dm_dp_mst_atomic_check(struct drm_connector *connector,
+				struct drm_atomic_state *state)
+{
+	struct drm_connector_state *new_conn_state =
+			drm_atomic_get_new_connector_state(state, connector);
+	struct drm_connector_state *old_conn_state =
+			drm_atomic_get_old_connector_state(state, connector);
+	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_dp_mst_topology_mgr *mst_mgr;
+	struct drm_dp_mst_port *mst_port;
+
+	mst_port = aconnector->port;
+	mst_mgr = &aconnector->mst_port->mst_mgr;
+
+	if (!old_conn_state->crtc)
+		return 0;
+
+	if (new_conn_state->crtc) {
+		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
+		if (!new_crtc_state ||
+		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
+		    new_crtc_state->enable)
+			return 0;
+		}
+
+	return drm_dp_atomic_release_vcpi_slots(state,
+						mst_mgr,
+						mst_port);
+}
+
 static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
 	.get_modes = dm_dp_mst_get_modes,
 	.mode_valid = amdgpu_dm_connector_mode_valid,
 	.atomic_best_encoder = dm_mst_atomic_best_encoder,
+	.atomic_check = dm_dp_mst_atomic_check,
 };
 
 static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index fdb6adc37857..e129717572d3 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -193,6 +193,7 @@ struct dc_stream_state {
 	bool dpms_off;
 
 	void *dm_stream_context;
+	void *dm_stream_state;
 
 	struct dc_cursor_attributes cursor_attributes;
 	struct dc_cursor_position cursor_position;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v5] drm/amd/display: Add MST atomic routines

[mikita.lipski]

From: Mikita Lipski <mikita.lipski@amd.com>

- Adding encoder atomic check to find vcpi slots for a connector
- Using DRM helper functions to calculate PBN
- Adding connector atomic check to release vcpi slots if connector
loses CRTC
- Calculate  PBN and VCPI slots only once during atomic
check and store them on crtc_state to eliminate
redundant calculation
- Call drm_dp_mst_atomic_check to verify validity of MST topology
during state atomic check

v2: squashed previous 3 separate patches, removed DSC PBN calculation,
and added PBN and VCPI slots properties to amdgpu connector

v3:
- moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
- updates stream's vcpi_slots and pbn on commit
- separated patch from the DSC MST series

v4:
- set vcpi_slots and pbn properties to dm_connector_state
- copy porperties from connector state on to crtc state

v5:
- keep the pbn and vcpi values only on connnector state
- added a void pointer to the stream state instead on two ints,
because dc_stream_state is OS agnostic. Pointer points to the
current dm_connector_state.

Cc: Jun Lei <Jun.Lei@amd.com>
Cc: Jerry Zuo <Jerry.Zuo@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Cc: Lyude Paul <lyude@redhat.com>
Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
 drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
 5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
 	}
 
 	stream->dm_stream_context = aconnector;
+	stream->dm_stream_state = dm_state;
 
 	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
 		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
@@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
 		state->underscan_hborder = 0;
 		state->underscan_vborder = 0;
 		state->base.max_requested_bpc = 8;
-
+		state->vcpi_slots = 0;
+		state->pbn = 0;
 		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
 			state->abm_level = amdgpu_dm_abm_level;
 
@@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
 	new_state->underscan_enable = state->underscan_enable;
 	new_state->underscan_hborder = state->underscan_hborder;
 	new_state->underscan_vborder = state->underscan_vborder;
-
+	new_state->vcpi_slots = state->vcpi_slots;
+	new_state->pbn = state->pbn;
 	return &new_state->base;
 }
 
@@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
 					  struct drm_crtc_state *crtc_state,
 					  struct drm_connector_state *conn_state)
 {
+	struct drm_atomic_state *state = crtc_state->state;
+	struct drm_connector *connector = conn_state->connector;
+	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
+	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+	struct drm_dp_mst_topology_mgr *mst_mgr;
+	struct drm_dp_mst_port *mst_port;
+	int clock, bpp = 0;
+
+	if (!aconnector->port || !aconnector->dc_sink)
+		return 0;
+
+	mst_port = aconnector->port;
+	mst_mgr = &aconnector->mst_port->mst_mgr;
+
+	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
+		return 0;
+
+	if (!state->duplicated) {
+		bpp = (uint8_t)connector->display_info.bpc * 3;
+		clock = adjusted_mode->clock;
+		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
+	}
+	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
+							       mst_mgr,
+							       mst_port,
+							       dm_new_connector_state->pbn);
+	if (dm_new_connector_state->vcpi_slots < 0) {
+		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
+		return dm_new_connector_state->vcpi_slots;
+	}
 	return 0;
 }
 
@@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 		hdr_changed =
 			is_hdr_metadata_different(old_con_state, new_con_state);
 
+		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
+
 		if (!scaling_changed && !abm_changed && !hdr_changed)
 			continue;
 
@@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
 			stream_update.hdr_static_metadata = &hdr_packet;
 		}
 
+
 		status = dc_stream_get_status(dm_new_crtc_state->stream);
 		WARN_ON(!status);
 		WARN_ON(!status->plane_count);
@@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	/* Perform validation of MST topology in the state*/
+	ret = drm_dp_mst_atomic_check(state);
+	if (ret)
+		goto fail;
+
 	if (state->legacy_cursor_update) {
 		/*
 		 * This is a fast cursor update coming from the plane update
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 c6fdebee7189..910c8598faf9 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -360,6 +360,8 @@ struct dm_connector_state {
 	bool freesync_enable;
 	bool freesync_capable;
 	uint8_t abm_level;
+	uint64_t vcpi_slots;
+	uint64_t pbn;
 };
 
 #define to_dm_connector_state(x)\
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
index 11e5784aa62a..821d61e060b2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
@@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 		bool enable)
 {
 	struct amdgpu_dm_connector *aconnector;
+	struct dm_connector_state *dm_conn_state;
 	struct drm_dp_mst_topology_mgr *mst_mgr;
 	struct drm_dp_mst_port *mst_port;
-	int slots = 0;
 	bool ret;
-	int clock;
-	int bpp = 0;
-	int pbn = 0;
 
 	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
+	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
 
 	if (!aconnector || !aconnector->mst_port)
 		return false;
@@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
 	mst_port = aconnector->port;
 
 	if (enable) {
-		clock = stream->timing.pix_clk_100hz / 10;
-
-		switch (stream->timing.display_color_depth) {
-
-		case COLOR_DEPTH_666:
-			bpp = 6;
-			break;
-		case COLOR_DEPTH_888:
-			bpp = 8;
-			break;
-		case COLOR_DEPTH_101010:
-			bpp = 10;
-			break;
-		case COLOR_DEPTH_121212:
-			bpp = 12;
-			break;
-		case COLOR_DEPTH_141414:
-			bpp = 14;
-			break;
-		case COLOR_DEPTH_161616:
-			bpp = 16;
-			break;
-		default:
-			ASSERT(bpp != 0);
-			break;
-		}
-
-		bpp = bpp * 3;
-
-		/* TODO need to know link rate */
-
-		pbn = drm_dp_calc_pbn_mode(clock, bpp);
-
-		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
-		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
 
+		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
+					       dm_conn_state->pbn,
+					       dm_conn_state->vcpi_slots);
 		if (!ret)
 			return false;
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
index 779d0b60cac9..1a17ea1b42e0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
@@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
 	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
 }
 
+static int dm_dp_mst_atomic_check(struct drm_connector *connector,
+				struct drm_atomic_state *state)
+{
+	struct drm_connector_state *new_conn_state =
+			drm_atomic_get_new_connector_state(state, connector);
+	struct drm_connector_state *old_conn_state =
+			drm_atomic_get_old_connector_state(state, connector);
+	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
+	struct drm_crtc_state *new_crtc_state;
+	struct drm_dp_mst_topology_mgr *mst_mgr;
+	struct drm_dp_mst_port *mst_port;
+
+	mst_port = aconnector->port;
+	mst_mgr = &aconnector->mst_port->mst_mgr;
+
+	if (!old_conn_state->crtc)
+		return 0;
+
+	if (new_conn_state->crtc) {
+		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
+		if (!new_crtc_state ||
+		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
+		    new_crtc_state->enable)
+			return 0;
+		}
+
+	return drm_dp_atomic_release_vcpi_slots(state,
+						mst_mgr,
+						mst_port);
+}
+
 static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
 	.get_modes = dm_dp_mst_get_modes,
 	.mode_valid = amdgpu_dm_connector_mode_valid,
 	.atomic_best_encoder = dm_mst_atomic_best_encoder,
+	.atomic_check = dm_dp_mst_atomic_check,
 };
 
 static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
index fdb6adc37857..e129717572d3 100644
--- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
+++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
@@ -193,6 +193,7 @@ struct dc_stream_state {
 	bool dpms_off;
 
 	void *dm_stream_context;
+	void *dm_stream_state;
 
 	struct dc_cursor_attributes cursor_attributes;
 	struct dc_cursor_position cursor_position;
-- 
2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5] drm/amd/display: Add MST atomic routines

[Kazlauskas, Nicholas]

On 2019-10-24 5:06 p.m., mikita.lipski@amd.com wrote:
> From: Mikita Lipski <mikita.lipski@amd.com>
> 
> - Adding encoder atomic check to find vcpi slots for a connector
> - Using DRM helper functions to calculate PBN
> - Adding connector atomic check to release vcpi slots if connector
> loses CRTC
> - Calculate  PBN and VCPI slots only once during atomic
> check and store them on crtc_state to eliminate
> redundant calculation
> - Call drm_dp_mst_atomic_check to verify validity of MST topology
> during state atomic check
> 
> v2: squashed previous 3 separate patches, removed DSC PBN calculation,
> and added PBN and VCPI slots properties to amdgpu connector
> 
> v3:
> - moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
> - updates stream's vcpi_slots and pbn on commit
> - separated patch from the DSC MST series
> 
> v4:
> - set vcpi_slots and pbn properties to dm_connector_state
> - copy porperties from connector state on to crtc state
> 
> v5:
> - keep the pbn and vcpi values only on connnector state
> - added a void pointer to the stream state instead on two ints,
> because dc_stream_state is OS agnostic. Pointer points to the
> current dm_connector_state.
> 
> Cc: Jun Lei <Jun.Lei@amd.com>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>

Few comments below, mostly about how you're storing the DRM state in the 
DC stream.


> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
>   drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
>   5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>   	}
>   
>   	stream->dm_stream_context = aconnector;
> +	stream->dm_stream_state = dm_state;
>   
>   	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
>   		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
> @@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>   		state->underscan_hborder = 0;
>   		state->underscan_vborder = 0;
>   		state->base.max_requested_bpc = 8;
> -
> +		state->vcpi_slots = 0;
> +		state->pbn = 0;
>   		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>   			state->abm_level = amdgpu_dm_abm_level;
>   
> @@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>   	new_state->underscan_enable = state->underscan_enable;
>   	new_state->underscan_hborder = state->underscan_hborder;
>   	new_state->underscan_vborder = state->underscan_vborder;
> -
> +	new_state->vcpi_slots = state->vcpi_slots;
> +	new_state->pbn = state->pbn;
>   	return &new_state->base;
>   }
>   
> @@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>   					  struct drm_crtc_state *crtc_state,
>   					  struct drm_connector_state *conn_state)
>   {
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_connector *connector = conn_state->connector;
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +	int clock, bpp = 0;
> +
> +	if (!aconnector->port || !aconnector->dc_sink)
> +		return 0;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> +		return 0;
> +
> +	if (!state->duplicated) {
> +		bpp = (uint8_t)connector->display_info.bpc * 3;
> +		clock = adjusted_mode->clock;
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +	}
> +	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
> +							       mst_mgr,
> +							       mst_port,
> +							       dm_new_connector_state->pbn);
> +	if (dm_new_connector_state->vcpi_slots < 0) {
> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
> +		return dm_new_connector_state->vcpi_slots;
> +	}
>   	return 0;
>   }
>   
> @@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   		hdr_changed =
>   			is_hdr_metadata_different(old_con_state, new_con_state);
>   
> +		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
> +
>   		if (!scaling_changed && !abm_changed && !hdr_changed)
>   			continue;
>   
> @@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			stream_update.hdr_static_metadata = &hdr_packet;
>   		}
>   
> +
>   		status = dc_stream_get_status(dm_new_crtc_state->stream);
>   		WARN_ON(!status);
>   		WARN_ON(!status->plane_count);
> @@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	if (ret)
>   		goto fail;
>   
> +	/* Perform validation of MST topology in the state*/
> +	ret = drm_dp_mst_atomic_check(state);
> +	if (ret)
> +		goto fail;
> +
>   	if (state->legacy_cursor_update) {
>   		/*
>   		 * This is a fast cursor update coming from the plane update
> 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 c6fdebee7189..910c8598faf9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -360,6 +360,8 @@ struct dm_connector_state {
>   	bool freesync_enable;
>   	bool freesync_capable;
>   	uint8_t abm_level;
> +	uint64_t vcpi_slots;
> +	uint64_t pbn;
>   };
>   
>   #define to_dm_connector_state(x)\
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 11e5784aa62a..821d61e060b2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   		bool enable)
>   {
>   	struct amdgpu_dm_connector *aconnector;
> +	struct dm_connector_state *dm_conn_state;
>   	struct drm_dp_mst_topology_mgr *mst_mgr;
>   	struct drm_dp_mst_port *mst_port;
> -	int slots = 0;
>   	bool ret;
> -	int clock;
> -	int bpp = 0;
> -	int pbn = 0;
>   
>   	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
> +	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
>   
>   	if (!aconnector || !aconnector->mst_port)
>   		return false;
> @@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   	mst_port = aconnector->port;
>   
>   	if (enable) {
> -		clock = stream->timing.pix_clk_100hz / 10;
> -
> -		switch (stream->timing.display_color_depth) {
> -
> -		case COLOR_DEPTH_666:
> -			bpp = 6;
> -			break;
> -		case COLOR_DEPTH_888:
> -			bpp = 8;
> -			break;
> -		case COLOR_DEPTH_101010:
> -			bpp = 10;
> -			break;
> -		case COLOR_DEPTH_121212:
> -			bpp = 12;
> -			break;
> -		case COLOR_DEPTH_141414:
> -			bpp = 14;
> -			break;
> -		case COLOR_DEPTH_161616:
> -			bpp = 16;
> -			break;
> -		default:
> -			ASSERT(bpp != 0);
> -			break;
> -		}
> -
> -		bpp = bpp * 3;
> -
> -		/* TODO need to know link rate */
> -
> -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
> -
> -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> -		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>   
> +		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
> +					       dm_conn_state->pbn,
> +					       dm_conn_state->vcpi_slots);
>   		if (!ret)
>   			return false;
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 779d0b60cac9..1a17ea1b42e0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
>   	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
>   }
>   
> +static int dm_dp_mst_atomic_check(struct drm_connector *connector,
> +				struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *new_conn_state =
> +			drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_connector_state *old_conn_state =
> +			drm_atomic_get_old_connector_state(state, connector);
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!old_conn_state->crtc)
> +		return 0;
> +
> +	if (new_conn_state->crtc) {
> +		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
> +		if (!new_crtc_state ||
> +		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
> +		    new_crtc_state->enable)
> +			return 0;
> +		}
> +
> +	return drm_dp_atomic_release_vcpi_slots(state,
> +						mst_mgr,
> +						mst_port);
> +}
> +
>   static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
>   	.get_modes = dm_dp_mst_get_modes,
>   	.mode_valid = amdgpu_dm_connector_mode_valid,
>   	.atomic_best_encoder = dm_mst_atomic_best_encoder,
> +	.atomic_check = dm_dp_mst_atomic_check,
>   };
>   
>   static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> index fdb6adc37857..e129717572d3 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> @@ -193,6 +193,7 @@ struct dc_stream_state {
>   	bool dpms_off;
>   
>   	void *dm_stream_context;
> +	void *dm_stream_state;

I don't think you need to be adding this separate field here. The only 
thing stream->dm_stream_context is used for is storing the aconnector 
right now, which already gives you the DRM state.

I don't think it's a really good idea in general to store DRM connector 
state references here directly in a DC object since we're not holding 
any references to the DRM state it comes from (nor should we want to).

The only place I can see where you really use this new field is during 
HPD and you're already holding appropriate locks there, so it should be 
safe to just access the aconnector->base.state directly.

To verify your assumptions you can add an assertion for holding the lock 
in the MST payload allocation helper though.

Nicholas Kazlauskas

>   
>   	struct dc_cursor_attributes cursor_attributes;
>   	struct dc_cursor_position cursor_position;
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5] drm/amd/display: Add MST atomic routines

[Kazlauskas, Nicholas]

On 2019-10-24 5:06 p.m., mikita.lipski@amd.com wrote:
> From: Mikita Lipski <mikita.lipski@amd.com>
> 
> - Adding encoder atomic check to find vcpi slots for a connector
> - Using DRM helper functions to calculate PBN
> - Adding connector atomic check to release vcpi slots if connector
> loses CRTC
> - Calculate  PBN and VCPI slots only once during atomic
> check and store them on crtc_state to eliminate
> redundant calculation
> - Call drm_dp_mst_atomic_check to verify validity of MST topology
> during state atomic check
> 
> v2: squashed previous 3 separate patches, removed DSC PBN calculation,
> and added PBN and VCPI slots properties to amdgpu connector
> 
> v3:
> - moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
> - updates stream's vcpi_slots and pbn on commit
> - separated patch from the DSC MST series
> 
> v4:
> - set vcpi_slots and pbn properties to dm_connector_state
> - copy porperties from connector state on to crtc state
> 
> v5:
> - keep the pbn and vcpi values only on connnector state
> - added a void pointer to the stream state instead on two ints,
> because dc_stream_state is OS agnostic. Pointer points to the
> current dm_connector_state.
> 
> Cc: Jun Lei <Jun.Lei@amd.com>
> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>

Few comments below, mostly about how you're storing the DRM state in the 
DC stream.


> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
>   drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
>   5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>   	}
>   
>   	stream->dm_stream_context = aconnector;
> +	stream->dm_stream_state = dm_state;
>   
>   	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
>   		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
> @@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>   		state->underscan_hborder = 0;
>   		state->underscan_vborder = 0;
>   		state->base.max_requested_bpc = 8;
> -
> +		state->vcpi_slots = 0;
> +		state->pbn = 0;
>   		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>   			state->abm_level = amdgpu_dm_abm_level;
>   
> @@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>   	new_state->underscan_enable = state->underscan_enable;
>   	new_state->underscan_hborder = state->underscan_hborder;
>   	new_state->underscan_vborder = state->underscan_vborder;
> -
> +	new_state->vcpi_slots = state->vcpi_slots;
> +	new_state->pbn = state->pbn;
>   	return &new_state->base;
>   }
>   
> @@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>   					  struct drm_crtc_state *crtc_state,
>   					  struct drm_connector_state *conn_state)
>   {
> +	struct drm_atomic_state *state = crtc_state->state;
> +	struct drm_connector *connector = conn_state->connector;
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +	int clock, bpp = 0;
> +
> +	if (!aconnector->port || !aconnector->dc_sink)
> +		return 0;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
> +		return 0;
> +
> +	if (!state->duplicated) {
> +		bpp = (uint8_t)connector->display_info.bpc * 3;
> +		clock = adjusted_mode->clock;
> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
> +	}
> +	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
> +							       mst_mgr,
> +							       mst_port,
> +							       dm_new_connector_state->pbn);
> +	if (dm_new_connector_state->vcpi_slots < 0) {
> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
> +		return dm_new_connector_state->vcpi_slots;
> +	}
>   	return 0;
>   }
>   
> @@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   		hdr_changed =
>   			is_hdr_metadata_different(old_con_state, new_con_state);
>   
> +		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
> +
>   		if (!scaling_changed && !abm_changed && !hdr_changed)
>   			continue;
>   
> @@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			stream_update.hdr_static_metadata = &hdr_packet;
>   		}
>   
> +
>   		status = dc_stream_get_status(dm_new_crtc_state->stream);
>   		WARN_ON(!status);
>   		WARN_ON(!status->plane_count);
> @@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	if (ret)
>   		goto fail;
>   
> +	/* Perform validation of MST topology in the state*/
> +	ret = drm_dp_mst_atomic_check(state);
> +	if (ret)
> +		goto fail;
> +
>   	if (state->legacy_cursor_update) {
>   		/*
>   		 * This is a fast cursor update coming from the plane update
> 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 c6fdebee7189..910c8598faf9 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -360,6 +360,8 @@ struct dm_connector_state {
>   	bool freesync_enable;
>   	bool freesync_capable;
>   	uint8_t abm_level;
> +	uint64_t vcpi_slots;
> +	uint64_t pbn;
>   };
>   
>   #define to_dm_connector_state(x)\
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 11e5784aa62a..821d61e060b2 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   		bool enable)
>   {
>   	struct amdgpu_dm_connector *aconnector;
> +	struct dm_connector_state *dm_conn_state;
>   	struct drm_dp_mst_topology_mgr *mst_mgr;
>   	struct drm_dp_mst_port *mst_port;
> -	int slots = 0;
>   	bool ret;
> -	int clock;
> -	int bpp = 0;
> -	int pbn = 0;
>   
>   	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
> +	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
>   
>   	if (!aconnector || !aconnector->mst_port)
>   		return false;
> @@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>   	mst_port = aconnector->port;
>   
>   	if (enable) {
> -		clock = stream->timing.pix_clk_100hz / 10;
> -
> -		switch (stream->timing.display_color_depth) {
> -
> -		case COLOR_DEPTH_666:
> -			bpp = 6;
> -			break;
> -		case COLOR_DEPTH_888:
> -			bpp = 8;
> -			break;
> -		case COLOR_DEPTH_101010:
> -			bpp = 10;
> -			break;
> -		case COLOR_DEPTH_121212:
> -			bpp = 12;
> -			break;
> -		case COLOR_DEPTH_141414:
> -			bpp = 14;
> -			break;
> -		case COLOR_DEPTH_161616:
> -			bpp = 16;
> -			break;
> -		default:
> -			ASSERT(bpp != 0);
> -			break;
> -		}
> -
> -		bpp = bpp * 3;
> -
> -		/* TODO need to know link rate */
> -
> -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
> -
> -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
> -		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>   
> +		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
> +					       dm_conn_state->pbn,
> +					       dm_conn_state->vcpi_slots);
>   		if (!ret)
>   			return false;
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 779d0b60cac9..1a17ea1b42e0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
>   	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
>   }
>   
> +static int dm_dp_mst_atomic_check(struct drm_connector *connector,
> +				struct drm_atomic_state *state)
> +{
> +	struct drm_connector_state *new_conn_state =
> +			drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_connector_state *old_conn_state =
> +			drm_atomic_get_old_connector_state(state, connector);
> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> +	struct drm_crtc_state *new_crtc_state;
> +	struct drm_dp_mst_topology_mgr *mst_mgr;
> +	struct drm_dp_mst_port *mst_port;
> +
> +	mst_port = aconnector->port;
> +	mst_mgr = &aconnector->mst_port->mst_mgr;
> +
> +	if (!old_conn_state->crtc)
> +		return 0;
> +
> +	if (new_conn_state->crtc) {
> +		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
> +		if (!new_crtc_state ||
> +		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
> +		    new_crtc_state->enable)
> +			return 0;
> +		}
> +
> +	return drm_dp_atomic_release_vcpi_slots(state,
> +						mst_mgr,
> +						mst_port);
> +}
> +
>   static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
>   	.get_modes = dm_dp_mst_get_modes,
>   	.mode_valid = amdgpu_dm_connector_mode_valid,
>   	.atomic_best_encoder = dm_mst_atomic_best_encoder,
> +	.atomic_check = dm_dp_mst_atomic_check,
>   };
>   
>   static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> index fdb6adc37857..e129717572d3 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
> @@ -193,6 +193,7 @@ struct dc_stream_state {
>   	bool dpms_off;
>   
>   	void *dm_stream_context;
> +	void *dm_stream_state;

I don't think you need to be adding this separate field here. The only 
thing stream->dm_stream_context is used for is storing the aconnector 
right now, which already gives you the DRM state.

I don't think it's a really good idea in general to store DRM connector 
state references here directly in a DC object since we're not holding 
any references to the DRM state it comes from (nor should we want to).

The only place I can see where you really use this new field is during 
HPD and you're already holding appropriate locks there, so it should be 
safe to just access the aconnector->base.state directly.

To verify your assumptions you can add an assertion for holding the lock 
in the MST payload allocation helper though.

Nicholas Kazlauskas

>   
>   	struct dc_cursor_attributes cursor_attributes;
>   	struct dc_cursor_position cursor_position;
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v5] drm/amd/display: Add MST atomic routines

[Mikita Lipski]

On 25.10.2019 8:06, Kazlauskas, Nicholas wrote:
> On 2019-10-24 5:06 p.m., mikita.lipski@amd.com wrote:
>> From: Mikita Lipski <mikita.lipski@amd.com>
>>
>> - Adding encoder atomic check to find vcpi slots for a connector
>> - Using DRM helper functions to calculate PBN
>> - Adding connector atomic check to release vcpi slots if connector
>> loses CRTC
>> - Calculate  PBN and VCPI slots only once during atomic
>> check and store them on crtc_state to eliminate
>> redundant calculation
>> - Call drm_dp_mst_atomic_check to verify validity of MST topology
>> during state atomic check
>>
>> v2: squashed previous 3 separate patches, removed DSC PBN calculation,
>> and added PBN and VCPI slots properties to amdgpu connector
>>
>> v3:
>> - moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
>> - updates stream's vcpi_slots and pbn on commit
>> - separated patch from the DSC MST series
>>
>> v4:
>> - set vcpi_slots and pbn properties to dm_connector_state
>> - copy porperties from connector state on to crtc state
>>
>> v5:
>> - keep the pbn and vcpi values only on connnector state
>> - added a void pointer to the stream state instead on two ints,
>> because dc_stream_state is OS agnostic. Pointer points to the
>> current dm_connector_state.
>>
>> Cc: Jun Lei <Jun.Lei@amd.com>
>> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
> Few comments below, mostly about how you're storing the DRM state in the
> DC stream.

Hi Nick,

Thanks for pointing that out.

It is definitely better not to introduce a new state pointer to the stream.

I'll apply your comments for the next version.

>
>> ---
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>>    .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
>>    .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
>>    drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
>>    5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>    	}
>>    
>>    	stream->dm_stream_context = aconnector;
>> +	stream->dm_stream_state = dm_state;
>>    
>>    	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
>>    		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
>> @@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>>    		state->underscan_hborder = 0;
>>    		state->underscan_vborder = 0;
>>    		state->base.max_requested_bpc = 8;
>> -
>> +		state->vcpi_slots = 0;
>> +		state->pbn = 0;
>>    		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>>    			state->abm_level = amdgpu_dm_abm_level;
>>    
>> @@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>>    	new_state->underscan_enable = state->underscan_enable;
>>    	new_state->underscan_hborder = state->underscan_hborder;
>>    	new_state->underscan_vborder = state->underscan_vborder;
>> -
>> +	new_state->vcpi_slots = state->vcpi_slots;
>> +	new_state->pbn = state->pbn;
>>    	return &new_state->base;
>>    }
>>    
>> @@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>>    					  struct drm_crtc_state *crtc_state,
>>    					  struct drm_connector_state *conn_state)
>>    {
>> +	struct drm_atomic_state *state = crtc_state->state;
>> +	struct drm_connector *connector = conn_state->connector;
>> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> +	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>> +	struct drm_dp_mst_topology_mgr *mst_mgr;
>> +	struct drm_dp_mst_port *mst_port;
>> +	int clock, bpp = 0;
>> +
>> +	if (!aconnector->port || !aconnector->dc_sink)
>> +		return 0;
>> +
>> +	mst_port = aconnector->port;
>> +	mst_mgr = &aconnector->mst_port->mst_mgr;
>> +
>> +	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
>> +		return 0;
>> +
>> +	if (!state->duplicated) {
>> +		bpp = (uint8_t)connector->display_info.bpc * 3;
>> +		clock = adjusted_mode->clock;
>> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> +	}
>> +	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
>> +							       mst_mgr,
>> +							       mst_port,
>> +							       dm_new_connector_state->pbn);
>> +	if (dm_new_connector_state->vcpi_slots < 0) {
>> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
>> +		return dm_new_connector_state->vcpi_slots;
>> +	}
>>    	return 0;
>>    }
>>    
>> @@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>    		hdr_changed =
>>    			is_hdr_metadata_different(old_con_state, new_con_state);
>>    
>> +		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
>> +
>>    		if (!scaling_changed && !abm_changed && !hdr_changed)
>>    			continue;
>>    
>> @@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>    			stream_update.hdr_static_metadata = &hdr_packet;
>>    		}
>>    
>> +
>>    		status = dc_stream_get_status(dm_new_crtc_state->stream);
>>    		WARN_ON(!status);
>>    		WARN_ON(!status->plane_count);
>> @@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>    	if (ret)
>>    		goto fail;
>>    
>> +	/* Perform validation of MST topology in the state*/
>> +	ret = drm_dp_mst_atomic_check(state);
>> +	if (ret)
>> +		goto fail;
>> +
>>    	if (state->legacy_cursor_update) {
>>    		/*
>>    		 * This is a fast cursor update coming from the plane update
>> 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 c6fdebee7189..910c8598faf9 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -360,6 +360,8 @@ struct dm_connector_state {
>>    	bool freesync_enable;
>>    	bool freesync_capable;
>>    	uint8_t abm_level;
>> +	uint64_t vcpi_slots;
>> +	uint64_t pbn;
>>    };
>>    
>>    #define to_dm_connector_state(x)\
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 11e5784aa62a..821d61e060b2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>    		bool enable)
>>    {
>>    	struct amdgpu_dm_connector *aconnector;
>> +	struct dm_connector_state *dm_conn_state;
>>    	struct drm_dp_mst_topology_mgr *mst_mgr;
>>    	struct drm_dp_mst_port *mst_port;
>> -	int slots = 0;
>>    	bool ret;
>> -	int clock;
>> -	int bpp = 0;
>> -	int pbn = 0;
>>    
>>    	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>> +	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
>>    
>>    	if (!aconnector || !aconnector->mst_port)
>>    		return false;
>> @@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>    	mst_port = aconnector->port;
>>    
>>    	if (enable) {
>> -		clock = stream->timing.pix_clk_100hz / 10;
>> -
>> -		switch (stream->timing.display_color_depth) {
>> -
>> -		case COLOR_DEPTH_666:
>> -			bpp = 6;
>> -			break;
>> -		case COLOR_DEPTH_888:
>> -			bpp = 8;
>> -			break;
>> -		case COLOR_DEPTH_101010:
>> -			bpp = 10;
>> -			break;
>> -		case COLOR_DEPTH_121212:
>> -			bpp = 12;
>> -			break;
>> -		case COLOR_DEPTH_141414:
>> -			bpp = 14;
>> -			break;
>> -		case COLOR_DEPTH_161616:
>> -			bpp = 16;
>> -			break;
>> -		default:
>> -			ASSERT(bpp != 0);
>> -			break;
>> -		}
>> -
>> -		bpp = bpp * 3;
>> -
>> -		/* TODO need to know link rate */
>> -
>> -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> -
>> -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
>> -		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>>    
>> +		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
>> +					       dm_conn_state->pbn,
>> +					       dm_conn_state->vcpi_slots);
>>    		if (!ret)
>>    			return false;
>>    
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 779d0b60cac9..1a17ea1b42e0 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
>>    	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
>>    }
>>    
>> +static int dm_dp_mst_atomic_check(struct drm_connector *connector,
>> +				struct drm_atomic_state *state)
>> +{
>> +	struct drm_connector_state *new_conn_state =
>> +			drm_atomic_get_new_connector_state(state, connector);
>> +	struct drm_connector_state *old_conn_state =
>> +			drm_atomic_get_old_connector_state(state, connector);
>> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> +	struct drm_crtc_state *new_crtc_state;
>> +	struct drm_dp_mst_topology_mgr *mst_mgr;
>> +	struct drm_dp_mst_port *mst_port;
>> +
>> +	mst_port = aconnector->port;
>> +	mst_mgr = &aconnector->mst_port->mst_mgr;
>> +
>> +	if (!old_conn_state->crtc)
>> +		return 0;
>> +
>> +	if (new_conn_state->crtc) {
>> +		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
>> +		if (!new_crtc_state ||
>> +		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
>> +		    new_crtc_state->enable)
>> +			return 0;
>> +		}
>> +
>> +	return drm_dp_atomic_release_vcpi_slots(state,
>> +						mst_mgr,
>> +						mst_port);
>> +}
>> +
>>    static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
>>    	.get_modes = dm_dp_mst_get_modes,
>>    	.mode_valid = amdgpu_dm_connector_mode_valid,
>>    	.atomic_best_encoder = dm_mst_atomic_best_encoder,
>> +	.atomic_check = dm_dp_mst_atomic_check,
>>    };
>>    
>>    static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> index fdb6adc37857..e129717572d3 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> @@ -193,6 +193,7 @@ struct dc_stream_state {
>>    	bool dpms_off;
>>    
>>    	void *dm_stream_context;
>> +	void *dm_stream_state;
> I don't think you need to be adding this separate field here. The only
> thing stream->dm_stream_context is used for is storing the aconnector
> right now, which already gives you the DRM state.
>
> I don't think it's a really good idea in general to store DRM connector
> state references here directly in a DC object since we're not holding
> any references to the DRM state it comes from (nor should we want to).
>
> The only place I can see where you really use this new field is during
> HPD and you're already holding appropriate locks there, so it should be
> safe to just access the aconnector->base.state directly.
>
> To verify your assumptions you can add an assertion for holding the lock
> in the MST payload allocation helper though.
>
> Nicholas Kazlauskas
>
>>    
>>    	struct dc_cursor_attributes cursor_attributes;
>>    	struct dc_cursor_position cursor_position;
>>
-- 
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

Re: [PATCH v5] drm/amd/display: Add MST atomic routines

[Mikita Lipski]

On 25.10.2019 8:06, Kazlauskas, Nicholas wrote:
> On 2019-10-24 5:06 p.m., mikita.lipski@amd.com wrote:
>> From: Mikita Lipski <mikita.lipski@amd.com>
>>
>> - Adding encoder atomic check to find vcpi slots for a connector
>> - Using DRM helper functions to calculate PBN
>> - Adding connector atomic check to release vcpi slots if connector
>> loses CRTC
>> - Calculate  PBN and VCPI slots only once during atomic
>> check and store them on crtc_state to eliminate
>> redundant calculation
>> - Call drm_dp_mst_atomic_check to verify validity of MST topology
>> during state atomic check
>>
>> v2: squashed previous 3 separate patches, removed DSC PBN calculation,
>> and added PBN and VCPI slots properties to amdgpu connector
>>
>> v3:
>> - moved vcpi_slots and pbn properties to dm_crtc_state and dc_stream_state
>> - updates stream's vcpi_slots and pbn on commit
>> - separated patch from the DSC MST series
>>
>> v4:
>> - set vcpi_slots and pbn properties to dm_connector_state
>> - copy porperties from connector state on to crtc state
>>
>> v5:
>> - keep the pbn and vcpi values only on connnector state
>> - added a void pointer to the stream state instead on two ints,
>> because dc_stream_state is OS agnostic. Pointer points to the
>> current dm_connector_state.
>>
>> Cc: Jun Lei <Jun.Lei@amd.com>
>> Cc: Jerry Zuo <Jerry.Zuo@amd.com>
>> Cc: Harry Wentland <harry.wentland@amd.com>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>> Cc: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Mikita Lipski <mikita.lipski@amd.com>
> Few comments below, mostly about how you're storing the DRM state in the
> DC stream.

Hi Nick,

Thanks for pointing that out.

It is definitely better not to introduce a new state pointer to the stream.

I'll apply your comments for the next version.

>
>> ---
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 46 ++++++++++++++++++-
>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  2 +
>>    .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 44 ++----------------
>>    .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 +++++++++++++
>>    drivers/gpu/drm/amd/display/dc/dc_stream.h    |  1 +
>>    5 files changed, 84 insertions(+), 41 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 48f5b43e2698..1d8d7aaba197 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3747,6 +3747,7 @@ create_stream_for_sink(struct amdgpu_dm_connector *aconnector,
>>    	}
>>    
>>    	stream->dm_stream_context = aconnector;
>> +	stream->dm_stream_state = dm_state;
>>    
>>    	stream->timing.flags.LTE_340MCSC_SCRAMBLE =
>>    		drm_connector->display_info.hdmi.scdc.scrambling.low_rates;
>> @@ -4180,7 +4181,8 @@ void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>>    		state->underscan_hborder = 0;
>>    		state->underscan_vborder = 0;
>>    		state->base.max_requested_bpc = 8;
>> -
>> +		state->vcpi_slots = 0;
>> +		state->pbn = 0;
>>    		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP)
>>    			state->abm_level = amdgpu_dm_abm_level;
>>    
>> @@ -4209,7 +4211,8 @@ amdgpu_dm_connector_atomic_duplicate_state(struct drm_connector *connector)
>>    	new_state->underscan_enable = state->underscan_enable;
>>    	new_state->underscan_hborder = state->underscan_hborder;
>>    	new_state->underscan_vborder = state->underscan_vborder;
>> -
>> +	new_state->vcpi_slots = state->vcpi_slots;
>> +	new_state->pbn = state->pbn;
>>    	return &new_state->base;
>>    }
>>    
>> @@ -4610,6 +4613,37 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
>>    					  struct drm_crtc_state *crtc_state,
>>    					  struct drm_connector_state *conn_state)
>>    {
>> +	struct drm_atomic_state *state = crtc_state->state;
>> +	struct drm_connector *connector = conn_state->connector;
>> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> +	struct dm_connector_state *dm_new_connector_state = to_dm_connector_state(conn_state);
>> +	const struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>> +	struct drm_dp_mst_topology_mgr *mst_mgr;
>> +	struct drm_dp_mst_port *mst_port;
>> +	int clock, bpp = 0;
>> +
>> +	if (!aconnector->port || !aconnector->dc_sink)
>> +		return 0;
>> +
>> +	mst_port = aconnector->port;
>> +	mst_mgr = &aconnector->mst_port->mst_mgr;
>> +
>> +	if (!crtc_state->connectors_changed && !crtc_state->mode_changed)
>> +		return 0;
>> +
>> +	if (!state->duplicated) {
>> +		bpp = (uint8_t)connector->display_info.bpc * 3;
>> +		clock = adjusted_mode->clock;
>> +		dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> +	}
>> +	dm_new_connector_state->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
>> +							       mst_mgr,
>> +							       mst_port,
>> +							       dm_new_connector_state->pbn);
>> +	if (dm_new_connector_state->vcpi_slots < 0) {
>> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", dm_new_connector_state->vcpi_slots);
>> +		return dm_new_connector_state->vcpi_slots;
>> +	}
>>    	return 0;
>>    }
>>    
>> @@ -6598,6 +6632,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>    		hdr_changed =
>>    			is_hdr_metadata_different(old_con_state, new_con_state);
>>    
>> +		dm_new_crtc_state->stream->dm_stream_state = new_con_state;
>> +
>>    		if (!scaling_changed && !abm_changed && !hdr_changed)
>>    			continue;
>>    
>> @@ -6621,6 +6657,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>>    			stream_update.hdr_static_metadata = &hdr_packet;
>>    		}
>>    
>> +
>>    		status = dc_stream_get_status(dm_new_crtc_state->stream);
>>    		WARN_ON(!status);
>>    		WARN_ON(!status->plane_count);
>> @@ -7651,6 +7688,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>    	if (ret)
>>    		goto fail;
>>    
>> +	/* Perform validation of MST topology in the state*/
>> +	ret = drm_dp_mst_atomic_check(state);
>> +	if (ret)
>> +		goto fail;
>> +
>>    	if (state->legacy_cursor_update) {
>>    		/*
>>    		 * This is a fast cursor update coming from the plane update
>> 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 c6fdebee7189..910c8598faf9 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -360,6 +360,8 @@ struct dm_connector_state {
>>    	bool freesync_enable;
>>    	bool freesync_capable;
>>    	uint8_t abm_level;
>> +	uint64_t vcpi_slots;
>> +	uint64_t pbn;
>>    };
>>    
>>    #define to_dm_connector_state(x)\
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> index 11e5784aa62a..821d61e060b2 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
>> @@ -182,15 +182,13 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>    		bool enable)
>>    {
>>    	struct amdgpu_dm_connector *aconnector;
>> +	struct dm_connector_state *dm_conn_state;
>>    	struct drm_dp_mst_topology_mgr *mst_mgr;
>>    	struct drm_dp_mst_port *mst_port;
>> -	int slots = 0;
>>    	bool ret;
>> -	int clock;
>> -	int bpp = 0;
>> -	int pbn = 0;
>>    
>>    	aconnector = (struct amdgpu_dm_connector *)stream->dm_stream_context;
>> +	dm_conn_state = (struct dm_connector_state *)stream->dm_stream_state;
>>    
>>    	if (!aconnector || !aconnector->mst_port)
>>    		return false;
>> @@ -203,42 +201,10 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>>    	mst_port = aconnector->port;
>>    
>>    	if (enable) {
>> -		clock = stream->timing.pix_clk_100hz / 10;
>> -
>> -		switch (stream->timing.display_color_depth) {
>> -
>> -		case COLOR_DEPTH_666:
>> -			bpp = 6;
>> -			break;
>> -		case COLOR_DEPTH_888:
>> -			bpp = 8;
>> -			break;
>> -		case COLOR_DEPTH_101010:
>> -			bpp = 10;
>> -			break;
>> -		case COLOR_DEPTH_121212:
>> -			bpp = 12;
>> -			break;
>> -		case COLOR_DEPTH_141414:
>> -			bpp = 14;
>> -			break;
>> -		case COLOR_DEPTH_161616:
>> -			bpp = 16;
>> -			break;
>> -		default:
>> -			ASSERT(bpp != 0);
>> -			break;
>> -		}
>> -
>> -		bpp = bpp * 3;
>> -
>> -		/* TODO need to know link rate */
>> -
>> -		pbn = drm_dp_calc_pbn_mode(clock, bpp);
>> -
>> -		slots = drm_dp_find_vcpi_slots(mst_mgr, pbn);
>> -		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port, pbn, slots);
>>    
>> +		ret = drm_dp_mst_allocate_vcpi(mst_mgr, mst_port,
>> +					       dm_conn_state->pbn,
>> +					       dm_conn_state->vcpi_slots);
>>    		if (!ret)
>>    			return false;
>>    
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 779d0b60cac9..1a17ea1b42e0 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -251,10 +251,42 @@ dm_mst_atomic_best_encoder(struct drm_connector *connector,
>>    	return &to_amdgpu_dm_connector(connector)->mst_encoder->base;
>>    }
>>    
>> +static int dm_dp_mst_atomic_check(struct drm_connector *connector,
>> +				struct drm_atomic_state *state)
>> +{
>> +	struct drm_connector_state *new_conn_state =
>> +			drm_atomic_get_new_connector_state(state, connector);
>> +	struct drm_connector_state *old_conn_state =
>> +			drm_atomic_get_old_connector_state(state, connector);
>> +	struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> +	struct drm_crtc_state *new_crtc_state;
>> +	struct drm_dp_mst_topology_mgr *mst_mgr;
>> +	struct drm_dp_mst_port *mst_port;
>> +
>> +	mst_port = aconnector->port;
>> +	mst_mgr = &aconnector->mst_port->mst_mgr;
>> +
>> +	if (!old_conn_state->crtc)
>> +		return 0;
>> +
>> +	if (new_conn_state->crtc) {
>> +		new_crtc_state = drm_atomic_get_old_crtc_state(state, new_conn_state->crtc);
>> +		if (!new_crtc_state ||
>> +		    !drm_atomic_crtc_needs_modeset(new_crtc_state) ||
>> +		    new_crtc_state->enable)
>> +			return 0;
>> +		}
>> +
>> +	return drm_dp_atomic_release_vcpi_slots(state,
>> +						mst_mgr,
>> +						mst_port);
>> +}
>> +
>>    static const struct drm_connector_helper_funcs dm_dp_mst_connector_helper_funcs = {
>>    	.get_modes = dm_dp_mst_get_modes,
>>    	.mode_valid = amdgpu_dm_connector_mode_valid,
>>    	.atomic_best_encoder = dm_mst_atomic_best_encoder,
>> +	.atomic_check = dm_dp_mst_atomic_check,
>>    };
>>    
>>    static void amdgpu_dm_encoder_destroy(struct drm_encoder *encoder)
>> diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> index fdb6adc37857..e129717572d3 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h
>> @@ -193,6 +193,7 @@ struct dc_stream_state {
>>    	bool dpms_off;
>>    
>>    	void *dm_stream_context;
>> +	void *dm_stream_state;
> I don't think you need to be adding this separate field here. The only
> thing stream->dm_stream_context is used for is storing the aconnector
> right now, which already gives you the DRM state.
>
> I don't think it's a really good idea in general to store DRM connector
> state references here directly in a DC object since we're not holding
> any references to the DRM state it comes from (nor should we want to).
>
> The only place I can see where you really use this new field is during
> HPD and you're already holding appropriate locks there, so it should be
> safe to just access the aconnector->base.state directly.
>
> To verify your assumptions you can add an assertion for holding the lock
> in the MST payload allocation helper though.
>
> Nicholas Kazlauskas
>
>>    
>>    	struct dc_cursor_attributes cursor_attributes;
>>    	struct dc_cursor_position cursor_position;
>>
-- 
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