Re: [PATCH v7] drm/amd/display: Add MST atomic routines
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.lip...@amd.com wrote:
>>> From: Mikita Lipski
>>>
>>> - 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
>>> Cc: Jerry Zuo
>>> Cc: Harry Wentland
>>> Cc: Nicholas Kazlauskas
>>> Cc: Lyude Paul
>>> Signed-off-by: Mikita Lipski
>>> ---
>>> .../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 bandwidt
Re: [PATCH v7] drm/amd/display: Add MST atomic routines
On 30.10.2019 14:19, Kazlauskas, Nicholas wrote:
> On 2019-10-28 10:31 a.m., mikita.lip...@amd.com wrote:
>> From: Mikita Lipski
>>
>> - 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
>> Cc: Jerry Zuo
>> Cc: Harry Wentland
>> Cc: Nicholas Kazlauskas
>> Cc: Lyude Paul
>> Signed-off-by: Mikita Lipski
>> ---
>>.../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,
>> +
Re: [PATCH v7] drm/amd/display: Add MST atomic routines
On 2019-10-28 10:31 a.m., mikita.lip...@amd.com wrote:
> From: Mikita Lipski
>
> - 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
> Cc: Jerry Zuo
> Cc: Harry Wentland
> Cc: Nicholas Kazlauskas
> Cc: Lyude Paul
> Signed-off-by: Mikita Lipski
> ---
> .../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?
> + 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
[PATCH v7] drm/amd/display: Add MST atomic routines
From: Mikita Lipski
- 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
Cc: Jerry Zuo
Cc: Harry Wentland
Cc: Nicholas Kazlauskas
Cc: Lyude Paul
Signed-off-by: Mikita Lipski
---
.../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;
+ 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
