On 31.10.2019 9:16, Kazlauskas, Nicholas wrote:
> On 2019-10-30 3:24 p.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:
>> - Fix error with using max capable bpc
>>
>> 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>
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>
> You might want to verify that this still works as you expect when
> changing "max bpc" on an MST display.
>
> My understanding is that it'd still force a new modeset before encoder
> atomic check is called so you *should* have the correct bpc value during
> MST calculations.
Thanks.
It does still works with MST even if you change the mode to the lower
resolution.
The encoder atomic check is called during drm_atomic_helper_check_modeset
so new modeset is already forced then.
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 65 ++++++++++++++++++-
>> .../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, 109 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..d75726013436 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;
>> }
>>
>> @@ -4606,10 +4608,64 @@ static void dm_encoder_helper_disable(struct drm_encoder *encoder)
>>
>> }
>>
>> +static int convert_dc_color_depth_into_bpc (enum dc_color_depth display_color_depth)
>> +{
>> + switch (display_color_depth) {
>> + case COLOR_DEPTH_666:
>> + return 6;
>> + case COLOR_DEPTH_888:
>> + return 8;
>> + case COLOR_DEPTH_101010:
>> + return 10;
>> + case COLOR_DEPTH_121212:
>> + return 12;
>> + case COLOR_DEPTH_141414:
>> + return 14;
>> + case COLOR_DEPTH_161616:
>> + return 16;
>> + default:
>> + break;
>> + }
>> + return 0;
>> +}
>> +
>> 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;
>> + enum dc_color_depth color_depth;
>> + 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) {
>> + color_depth = convert_color_depth_from_display_info(connector, conn_state);
>> + bpp = convert_dc_color_depth_into_bpc(color_depth) * 3;
> Minor nitpick here - this is converting from a numerical bpc to a DC
> enum and then from the DC enum back to a numerical bpc. Almost seems
> like we should just have a helper to get the numerical bpc.
>
> Logic seems fine though.
It does seem redundant, but by calling
convert_color_depth_from_display_info
we are ensuring to choose the min between max display bpc and max
requested bpc.
Plus it does some other checks to get the right bpc.
>
>> + 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", (int)dm_new_connector_state->vcpi_slots);
>> + return dm_new_connector_state->vcpi_slots;
>> + }
>> return 0;
>> }
>>
>> @@ -7651,6 +7707,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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
