On 30.10.2019 14:19, Kazlauskas, Nicholas wrote:
> On 2019-10-28 10:31 a.m., mikita.lipski@xxxxxxx wrote:
>> From: Mikita Lipski <mikita.lipski@xxxxxxx>
>>
>> - 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.
>>
>> v6:
>> - Remove new param from stream
>>
>> v7:
>> - Cleanup
>> - Remove state pointer from stream
>>
>> Cc: Jun Lei <Jun.Lei@xxxxxxx>
>> Cc: Jerry Zuo <Jerry.Zuo@xxxxxxx>
>> Cc: Harry Wentland <harry.wentland@xxxxxxx>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>> Cc: Lyude Paul <lyude@xxxxxxxxxx>
>> Signed-off-by: Mikita Lipski <mikita.lipski@xxxxxxx>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 ++++++++++++++-
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 2 +
>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 51 +++++--------------
>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 32 ++++++++++++
>> 4 files changed, 86 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..28f6b93ab371 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4180,7 +4180,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 +4210,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 +4612,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;
> Is this correct? Do we always want to calculate this based on the
> display capable bpc value instead of the one actually in use?
Wouldn't it be the same thing since we always try to use highest bpc?
Unless, we reduce the colour depth and display capable bpc will stay the
same, which would cause PBN be higher than needed.
Ok yeah that was a false assumption. I'll update it, thanks!
>
>> + 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;
>> }
>>
>> @@ -7651,6 +7684,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..1b2cc85b4815 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,20 @@ bool dm_helpers_dp_mst_write_payload_allocation_table(
>> bool enable)
>> {
>> struct amdgpu_dm_connector *aconnector;
>> + struct drm_connector *connector;
>> + 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;
>> + /* Accessing the connector state is required for vcpi_slots allocation
>> + * and directly relies on behaviour in commit check
>> + * that blocks before commit guaranteeing that the state
>> + * is not gonna be swapped while still in use in commit tail */
>> +
>> + dm_conn_state = to_dm_connector_state(aconnector->base.state);
>> +
>>
>> if (!aconnector || !aconnector->mst_port)
>> return false;
>> @@ -203,42 +208,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)
>>
--
Thanks,
Mikita Lipski
Software Engineer 2, AMD
mikita.lipski@xxxxxxx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
