[01/14] drm/amd/display: Add MST atomic routines

Submitted by mikita.lipski@amd.com on Oct. 1, 2019, 4:17 p.m.

Details

Message ID 20191001161721.13793-2-mikita.lipski@amd.com
State New
Headers show

Not browsing as part of any series.

Commit Message

mikita.lipski@amd.com Oct. 1, 2019, 4:17 p.m.
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 amdgpu connector 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

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 | 42 +++++++++++++++++++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 ++
 .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 42 ++-----------------
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 ++++++++++++++
 4 files changed, 81 insertions(+), 39 deletions(-)

Patch hide | download patch | download mbox

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 239b1ae86007..3fc1afccbb33 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4573,6 +4573,41 @@  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_crtc_state *dm_new_crtc_state = to_dm_crtc_state(crtc_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 (!dm_new_crtc_state)
+		return 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;
+		aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp);
+	}
+	aconnector->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
+							       mst_mgr,
+							       mst_port,
+							       aconnector->pbn);
+
+	if (aconnector->vcpi_slots < 0) {
+		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n", aconnector->vcpi_slots);
+		return aconnector->vcpi_slots;
+	}
 	return 0;
 }
 
@@ -5197,6 +5232,8 @@  void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
 	aconnector->base.dpms = DRM_MODE_DPMS_OFF;
 	aconnector->hpd.hpd = AMDGPU_HPD_NONE; /* not used */
 	aconnector->audio_inst = -1;
+	aconnector->vcpi_slots = 0;
+	aconnector->pbn = 0;
 	mutex_init(&aconnector->hpd_lock);
 
 	/*
@@ -7592,6 +7629,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..3ce104324096 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -280,6 +280,10 @@  struct amdgpu_dm_connector {
 	struct amdgpu_dm_connector *mst_port;
 	struct amdgpu_encoder *mst_encoder;
 
+	/* MST specific */
+	uint32_t vcpi_slots;
+	uint32_t pbn;
+
 	/* TODO see if we can merge with ddc_bus or make a dm_connector */
 	struct amdgpu_i2c_adapter *i2c;
 
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..5256abe32e92 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
@@ -184,11 +184,7 @@  bool dm_helpers_dp_mst_write_payload_allocation_table(
 	struct amdgpu_dm_connector *aconnector;
 	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;
 
@@ -203,42 +199,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,
+					       aconnector->pbn,
+					       aconnector->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 3af2b429ff1b..7f3ce29bd14c 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
@@ -250,10 +250,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)

Comments

Sorry this took me a little while to get to, I've been at XDC.

This is closer then, but still a couple more issues below (also-thank you for
including the changelog!)

On Tue, 2019-10-01 at 12:17 -0400, 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 amdgpu connector 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
> 
> 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 | 42 +++++++++++++++++++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 ++
>  .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 42 ++-----------------
>  .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 32 ++++++++++++++
>  4 files changed, 81 insertions(+), 39 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 239b1ae86007..3fc1afccbb33 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4573,6 +4573,41 @@ 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_crtc_state *dm_new_crtc_state =
> to_dm_crtc_state(crtc_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 (!dm_new_crtc_state)
> +		return 0;

You can remove this, crtc_state will never be NULL
> +
> +	if (!aconnector->port || !aconnector->dc_sink)
> +		return 0;
> +

This makes me think that this should probably just be moved into
amdgpu_dm_mst_types.c since we're not using this encoder check for anything
else, but I'll leave that decision up to you.

> +	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;
> +		aconnector->pbn = drm_dp_calc_pbn_mode(clock, bpp);

We can't do this...
> +	}
> +	aconnector->vcpi_slots = drm_dp_atomic_find_vcpi_slots(state,
> +							       mst_mgr,
> +							       mst_port,
> +							       aconnector-
> >pbn);

...and we can't do this: you're not allowed to modify anything during the
atomic check other than the atomic states that were passed in (e.g. crtc_state
along with anything in it's respective struct drm_atomic_state). Remember
we're trying to check if a configuration is valid here -before- we've
committed anything to hardware. So, the pbn and vcpi values need to be stored
in the connector's atomic state.

> +
> +	if (aconnector->vcpi_slots < 0) {
> +		DRM_DEBUG_ATOMIC("failed finding vcpi slots: %d\n",
> aconnector->vcpi_slots);
> +		return aconnector->vcpi_slots;
> +	}
>  	return 0;
>  }
>  
> @@ -5197,6 +5232,8 @@ void amdgpu_dm_connector_init_helper(struct
> amdgpu_display_manager *dm,
>  	aconnector->base.dpms = DRM_MODE_DPMS_OFF;
>  	aconnector->hpd.hpd = AMDGPU_HPD_NONE; /* not used */
>  	aconnector->audio_inst = -1;
> +	aconnector->vcpi_slots = 0;
> +	aconnector->pbn = 0;
>  	mutex_init(&aconnector->hpd_lock);
>  
>  	/*
> @@ -7592,6 +7629,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..3ce104324096 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -280,6 +280,10 @@ struct amdgpu_dm_connector {
>  	struct amdgpu_dm_connector *mst_port;
>  	struct amdgpu_encoder *mst_encoder;
>  
> +	/* MST specific */
> +	uint32_t vcpi_slots;
> +	uint32_t pbn;
> +
>  	/* TODO see if we can merge with ddc_bus or make a dm_connector */
>  	struct amdgpu_i2c_adapter *i2c;
>  
> 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..5256abe32e92 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
> @@ -184,11 +184,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>  	struct amdgpu_dm_connector *aconnector;
>  	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;
>  
> @@ -203,42 +199,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,
> +					       aconnector->pbn,
> +					       aconnector->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 3af2b429ff1b..7f3ce29bd14c 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
> @@ -250,10 +250,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;
> +		}
Need to fix the indenting here. The rest looks good so far though, keep going!
:)

> +
> +	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)