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

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)