[PATCH 01/14] drm/amd/display: Add MST atomic routines
Lyude Paul
lyude at redhat.com
Mon Oct 7 21:00:15 UTC 2019
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 at amd.com wrote:
> From: Mikita Lipski <mikita.lipski at 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 at amd.com>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Signed-off-by: Mikita Lipski <mikita.lipski at 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)
--
Cheers,
Lyude Paul
More information about the amd-gfx
mailing list