On Mon, Feb 06, 2023 at 07:42:32PM -0500, Lyude Paul wrote:
On Wed, 2023-02-01 at 17:04 +0200, Imre Deak wrote:
Yes, this patch should have no functional change, so please check what would apply to other drivers as well.
Could you also check Ville's comment about storing start_slot elsewhere than the atomic state (leaving only time_slots there). I wonder if that would work, at least it would simplify things I think.
Ideally it'd be nice if we could have all this info in the atomic commit, but yeah - you're not the first person to find this a bit confusing. FWIW though, the way we store start_slot right now is intended to follow the same kind of behavior we have with atomic CRTC commit dependencies, I think I also made it that way so all of the state would be in a single place but there's no hard requirement for it.
As I understood the atomic state should be precomputed during atomic check and not changed later during the commit phase. In case of start_slot this would mean knowing in advance the actual (driver specific) disabling / enabling sequence of the payloads, not sure how feasible it is to figure that out. start_slot also changes dynamically as each payload is disabled, so these intermediate versions of the field would need to be tracked somehow separately from the final (precomputed) versions.
So if you guys think it'd be better design-wise to store this something else, I've got no strong feelings either way
For 0-2:
Reviewed-by: Lyude Paul lyude@redhat.com
Thanks.
Cc: Lyude Paul lyude@redhat.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Karol Herbst kherbst@redhat.com Cc: Harry Wentland harry.wentland@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Wayne Lin Wayne.Lin@amd.com Cc: stable@vger.kernel.org # 6.1 Cc: dri-devel@lists.freedesktop.org Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Imre Deak imre.deak@intel.com
.../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 2 +- drivers/gpu/drm/display/drm_dp_mst_topology.c | 26 ++++++++++--------- drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 ++- drivers/gpu/drm/nouveau/dispnv50/disp.c | 2 +- include/drm/display/drm_dp_mst_helper.h | 3 ++- 5 files changed, 21 insertions(+), 16 deletions(-)
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 a50319fc42b11..180d3893b68da 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 @@ -208,7 +208,7 @@ bool dm_helpers_dp_mst_write_payload_allocation_table( if (enable) drm_dp_add_payload_part1(mst_mgr, mst_state, payload); else
drm_dp_remove_payload(mst_mgr, mst_state, payload);
drm_dp_remove_payload(mst_mgr, mst_state, payload, payload);/* mst_mgr->->payloads are VC payload notify MST branch using DPCD or * AUX message. The sequence is slot 1-63 allocated sequence for each diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c index 847c10aa2098c..1990ff5dc7ddd 100644 --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c @@ -3342,7 +3342,8 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1);
- drm_dp_remove_payload() - Remove an MST payload
- @mgr: Manager to use.
- @mst_state: The MST atomic state
- @payload: The payload to write
- @old_payload: The payload with its old state
- @new_payload: The payload to write
- Removes a payload from an MST topology if it was successfully assigned a start slot. Also updates
- the starting time slots of all other payloads which would have been shifted towards the start of
@@ -3350,36 +3351,37 @@ EXPORT_SYMBOL(drm_dp_add_payload_part1); */ void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state,
struct drm_dp_mst_atomic_payload *payload)
const struct drm_dp_mst_atomic_payload *old_payload,struct drm_dp_mst_atomic_payload *new_payload){ struct drm_dp_mst_atomic_payload *pos; bool send_remove = false; /* We failed to make the payload, so nothing to do */
- if (payload->vc_start_slot == -1)
- if (new_payload->vc_start_slot == -1) return;
mutex_lock(&mgr->lock);
- send_remove = drm_dp_mst_port_downstream_of_branch(payload->port, mgr->mst_primary);
- send_remove = drm_dp_mst_port_downstream_of_branch(new_payload->port, mgr->mst_primary); mutex_unlock(&mgr->lock);
if (send_remove)
drm_dp_destroy_payload_step1(mgr, mst_state, payload);
drm_dp_destroy_payload_step1(mgr, mst_state, new_payload);
payload->vcpi);
new_payload->vcpi);list_for_each_entry(pos, &mst_state->payloads, next) {
if (pos != payload && pos->vc_start_slot > payload->vc_start_slot)pos->vc_start_slot -= payload->time_slots;
if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)pos->vc_start_slot -= old_payload->time_slots;
- payload->vc_start_slot = -1;
- new_payload->vc_start_slot = -1;
mgr->payload_count--;
- mgr->next_start_slot -= payload->time_slots;
- mgr->next_start_slot -= old_payload->time_slots;
- if (payload->delete)
drm_dp_mst_put_port_malloc(payload->port);
- if (new_payload->delete)
drm_dp_mst_put_port_malloc(new_payload->port);} EXPORT_SYMBOL(drm_dp_remove_payload); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index f3cb12dcfe0a7..dc4e5ff1dbb31 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -526,6 +526,8 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state, to_intel_connector(old_conn_state->connector); struct drm_dp_mst_topology_state *mst_state = drm_atomic_get_mst_topology_state(&state->base, &intel_dp->mst_mgr);
- struct drm_dp_mst_atomic_payload *payload =
drm_atomic_get_mst_payload_state(mst_state, connector->port);drm_dbg_kms(&i915->drm, "active links %d\n", @@ -534,7 +536,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state, intel_hdcp_disable(intel_mst->connector); drm_dp_remove_payload(&intel_dp->mst_mgr, mst_state,
drm_atomic_get_mst_payload_state(mst_state, connector->port));
payload, payload);intel_audio_codec_disable(encoder, old_crtc_state, old_conn_state); } diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c index edcb2529b4025..ed9d374147b8d 100644 --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c @@ -885,7 +885,7 @@ nv50_msto_prepare(struct drm_atomic_state *state, // TODO: Figure out if we want to do a better job of handling VCPI allocation failures here? if (msto->disabled) {
drm_dp_remove_payload(mgr, mst_state, payload);
drm_dp_remove_payload(mgr, mst_state, payload, payload);nvif_outp_dp_mst_vcpi(&mstm->outp->outp, msto->head->base.index, 0, 0, 0, 0); } else { diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h index 41fd8352ab656..f5eb9aa152b14 100644 --- a/include/drm/display/drm_dp_mst_helper.h +++ b/include/drm/display/drm_dp_mst_helper.h @@ -841,7 +841,8 @@ int drm_dp_add_payload_part2(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_atomic_payload *payload); void drm_dp_remove_payload(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_mst_topology_state *mst_state,
struct drm_dp_mst_atomic_payload *payload);
const struct drm_dp_mst_atomic_payload *old_payload,struct drm_dp_mst_atomic_payload *new_payload);int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
-- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
-- Cheers, Lyude Paul (she/her) Software Engineer at Red Hat