[PATCH v7] drm/amd/display: Add MST atomic routines
Kazlauskas, Nicholas
Nicholas.Kazlauskas at amd.com
Wed Oct 30 18:48:16 UTC 2019
On 2019-10-30 2:42 p.m., Lipski, Mikita wrote:
>
> On 30.10.2019 14:19, Kazlauskas, Nicholas wrote:
>> On 2019-10-28 10:31 a.m., 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 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 at amd.com>
>>> 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 | 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!
The current policy is to always default to 8bpc.
Most userspace isn't aware or setup by default to handle greater than
8bpc and you run out of bandwidth on DP when using 1440p at 144Hz panels
that support 10bpc - those displays get limited to 1440p at 120Hz with no
real indication as to why the modes are missing in the list.
To use higher than 8bpc we currently require the user to explicitly
raise the "max bpc" on the connector with the assumption that they
understand that they might exceed connector bandwidth limitations.
Nicholas Kazlauskas
>
>>
>>> + 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)
>>>
More information about the amd-gfx
mailing list