>> Whenever a connector on an MST network is attached, detached, or
>> undergoes a modeset, the DSC configs for each stream on that
>> topology will be recalculated. This can change their required
>> bandwidth, requiring a full reprogramming, as though a modeset
>> was performed, even if that stream did not change timing.
>>
>> Therefore, whenever a crtc has drm_atomic_crtc_needs_modeset,
>> for each crtc that shares a MST topology with that stream and
>> supports DSC, add that crtc (and all affected connectors and
>> planes) to the atomic state and set mode_changed on its state
>>
>> Cc: Leo Li <sunpeng.li@xxxxxxx>
>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>> Signed-off-by: David Francis <David.Francis@xxxxxxx>
>> ---
>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 +++++++++++++++++++
>> 1 file changed, 80 insertions(+)
>>
>> 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 145fd73025dc..8d5357aec5e8 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -6475,7 +6475,78 @@ static int do_aquire_global_lock(struct drm_device *dev,
>>
>> return ret < 0 ? ret : 0;
>> }
>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>> +static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm_crtc *crtc)
>> +{
>> + struct drm_connector *connector;
>> + struct drm_connector_state *conn_state;
>> + struct drm_connector_list_iter conn_iter;
>> + struct drm_crtc_state *new_crtc_state;
>> + struct amdgpu_dm_connector *aconnector = NULL, *aconnector_to_add;
>> + int i, j, ret;
>> + struct drm_crtc *crtcs_affected[MAX_PIPES] = { 0 }; > +
>> + for_each_new_connector_in_state(state, connector, conn_state, i) {
>> + if (conn_state->crtc != crtc)
>> + continue;
>> +
>> + aconnector = to_amdgpu_dm_connector(connector);
>> + if (!aconnector->port)
>> + aconnector = NULL;
>> + else
>> + break;
>> + }
>> +
>> + if (!aconnector)
>> + return 0;
>> +
>> + i = 0;
>> + drm_connector_list_iter_begin(state->dev, &conn_iter);
>
>I don't like that we're grabbing the global connector lock every single
>time any CRTC undergoes a modeset even for ASICs that don't support DSC.
>
>We do lock everything below in atomic check anyway for FULL updates but
>I'd like to avoid adding more code that does this if possible. Maybe a
>check at the start that only does this if the ASIC has DSC support would
>be OK.
>
Will do
>> + drm_for_each_connector_iter(connector, &conn_iter) {
>> + if (!connector->state || !connector->state->crtc)
>> + continue;
>> +
>> + aconnector_to_add = to_amdgpu_dm_connector(connector);
>> + if (!aconnector_to_add->port)
>> + continue;
>> +
>> + if (aconnector_to_add->port->mgr != aconnector->port->mgr)
>> + continue;
>> +
>> + if (!aconnector_to_add->dc_sink)
>> + continue;
>> +
>> + if (!aconnector_to_add->dc_sink->sink_dsc_caps.dsc_dec_caps.is_dsc_supported)
>> + continue;
>>
>> + if (i >= MAX_PIPES)
>> + continue;
>> +
>> + crtcs_affected[i] = connector->state->crtc;
>
>Drop this crtcs_affected array and just perform the logic below right
>here. We don't really need two loops here, redundant calls to
>drm_atomic_get_crtc_state and the other helpers are fine.
>
Unfortunately, calling drm_atomic_get_crtc_state inside
drm_for_each_connector_iter causes a lockdep warning
>> + i++;
>> + }
>> + drm_connector_list_iter_end(&conn_iter);
>> +
>> + for (j = 0; j < i; j++) {
>> + new_crtc_state = drm_atomic_get_crtc_state(state, crtcs_affected[j]);
>> + if (IS_ERR(new_crtc_state))
>> + return PTR_ERR(new_crtc_state);
>> +
>> + new_crtc_state->mode_changed = true;
>> +
>> + ret = drm_atomic_add_affected_connectors(state, crtcs_affected[j]);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_atomic_add_affected_planes(state, crtcs_affected[j]);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + return 0;
>> +
>> +}
>> +#endif
>> static void get_freesync_config_for_crtc(
>> struct dm_crtc_state *new_crtc_state,
>> struct dm_connector_state *new_con_state)
>> @@ -7178,6 +7249,15 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>> goto fail;
>> }
>>
>> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
>> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>> + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>> + ret = add_affected_mst_dsc_crtcs(state, crtc);
>> + if (ret)
>> + goto fail;
>> + }
>> + }
>> +#endif
>> /*
>> * Add all primary and overlay planes on the CRTC to the state
>> * whenever a plane is enabled to maintain correct z-ordering
________________________________________
From: Kazlauskas, Nicholas <Nicholas.Kazlauskas@xxxxxxx>
Sent: August 19, 2019 3:34 PM
To: Francis, David; dri-devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Li, Sun peng (Leo)
Subject: Re: [PATCH 14/14] drm/amd/display: Trigger modesets on MST DSC connectors
On 8/19/19 11:50 AM, David Francis wrote:
> Whenever a connector on an MST network is attached, detached, or
> undergoes a modeset, the DSC configs for each stream on that
> topology will be recalculated. This can change their required
> bandwidth, requiring a full reprogramming, as though a modeset
> was performed, even if that stream did not change timing.
>
> Therefore, whenever a crtc has drm_atomic_crtc_needs_modeset,
> for each crtc that shares a MST topology with that stream and
> supports DSC, add that crtc (and all affected connectors and
> planes) to the atomic state and set mode_changed on its state
>
> Cc: Leo Li <sunpeng.li@xxxxxxx>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
> Signed-off-by: David Francis <David.Francis@xxxxxxx>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 +++++++++++++++++++
> 1 file changed, 80 insertions(+)
>
> 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 145fd73025dc..8d5357aec5e8 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6475,7 +6475,78 @@ static int do_aquire_global_lock(struct drm_device *dev,
>
> return ret < 0 ? ret : 0;
> }
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> +static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm_crtc *crtc)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> + struct drm_connector_list_iter conn_iter;
> + struct drm_crtc_state *new_crtc_state;
> + struct amdgpu_dm_connector *aconnector = NULL, *aconnector_to_add;
> + int i, j, ret;
> + struct drm_crtc *crtcs_affected[MAX_PIPES] = { 0 }; > +
> + for_each_new_connector_in_state(state, connector, conn_state, i) {
> + if (conn_state->crtc != crtc)
> + continue;
> +
> + aconnector = to_amdgpu_dm_connector(connector);
> + if (!aconnector->port)
> + aconnector = NULL;
> + else
> + break;
> + }
> +
> + if (!aconnector)
> + return 0;
> +
> + i = 0;
> + drm_connector_list_iter_begin(state->dev, &conn_iter);
I don't like that we're grabbing the global connector lock every single
time any CRTC undergoes a modeset even for ASICs that don't support DSC.
We do lock everything below in atomic check anyway for FULL updates but
I'd like to avoid adding more code that does this if possible. Maybe a
check at the start that only does this if the ASIC has DSC support would
be OK.
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + if (!connector->state || !connector->state->crtc)
> + continue;
> +
> + aconnector_to_add = to_amdgpu_dm_connector(connector);
> + if (!aconnector_to_add->port)
> + continue;
> +
> + if (aconnector_to_add->port->mgr != aconnector->port->mgr)
> + continue;
> +
> + if (!aconnector_to_add->dc_sink)
> + continue;
> +
> + if (!aconnector_to_add->dc_sink->sink_dsc_caps.dsc_dec_caps.is_dsc_supported)
> + continue;
>
> + if (i >= MAX_PIPES)
> + continue;
> +
> + crtcs_affected[i] = connector->state->crtc;
Drop this crtcs_affected array and just perform the logic below right
here. We don't really need two loops here, redundant calls to
drm_atomic_get_crtc_state and the other helpers are fine.
> + i++;
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +
> + for (j = 0; j < i; j++) {
> + new_crtc_state = drm_atomic_get_crtc_state(state, crtcs_affected[j]);
> + if (IS_ERR(new_crtc_state))
> + return PTR_ERR(new_crtc_state);
> +
> + new_crtc_state->mode_changed = true;
> +
> + ret = drm_atomic_add_affected_connectors(state, crtcs_affected[j]);
> + if (ret)
> + return ret;
> +
> + ret = drm_atomic_add_affected_planes(state, crtcs_affected[j]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +
> +}
> +#endif
> static void get_freesync_config_for_crtc(
> struct dm_crtc_state *new_crtc_state,
> struct dm_connector_state *new_con_state)
> @@ -7178,6 +7249,15 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
> goto fail;
> }
>
> +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> + if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> + ret = add_affected_mst_dsc_crtcs(state, crtc);
> + if (ret)
> + goto fail;
> + }
> + }
> +#endif
> /*
> * Add all primary and overlay planes on the CRTC to the state
> * whenever a plane is enabled to maintain correct z-ordering
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
