| Message ID | 20191001161721.13793-2-mikita.lipski@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DSC MST support for AMDGPU | expand |
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)
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)