On Tue, Sep 12, 2023 at 07:26:29AM +0000, Lin, Wayne wrote:
> [Public]
> [...]
> >
> > I'd like to be sure that the payload is removed with the size it was added with
> > in the previous commit and as I wrote above not depend for this on the new
> > payload state with a mixture of old/current/new states.
> > Based on that I'd be ok for instance with a new
> >
> > int drm_dp_remove_port_payload(mgr, mst_state, port)
> >
> > function which looks up / removes the payload with the time_slots calculated
> > based on the payload table as in your patch and returns the calculated
> > time_slots.
> >
> > The AMD driver could call the above function and the current
> > drm_dp_remove_payload(mgr, mst_state, old_payload) function would be
> >
> > time_slots = drm_dp_remove_port_payload(mgr, mst_state,
> > old_payload->port);
> > WARN_ON(time_slots != old_payload->time_slots);
> >
> > --Imre
>
> Sorry but I might not fully understand what you suggested here. Would like to know
> if you agree on referring to the time slot number of the payload table at the moment
> is better then referring old_payload->time_slots for drm_dp_remove_payload()? If
> you agree on that, my patch actually is just replacing old_payload->time_slots with
> the more appropriate one. Not adding mixture of old/current but replacing the old
> with the current one.
The new_payload state contains a mixture of old/current/new state at the
moment and this patch adds more dependency on that, recalculating the
old payload size from that state. For i915 this recalculation isn't
needed, the size is already available in the old payload state.
> And like what I explained in previous example, when calling
> drm_dp_remove_payload(), the time slot number to be removed shouldn't be
> constrained to the one in previous commit. The number in the payload table when
> we're about to remove the payload might be a better choice. Could you elaborate
> more what's the mixture that this patch is adding on, please?
>
> As for the changing suggestion, are you suggesting to create a new function
> drm_dp_remove_port_payload() to wrap up the calculation in my patch? If so, I think
> that's the consensus to use current time slot number to replace the one in old_payload.
> Therefore, it doesn't have to pass old_payload to drm_dp_remove_port_payload(), and
> "WARN_ON(time_slots != old_payload->time_slots);" is not appropriate as for the
> example that I gave previously.
I meant something like the following:
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 cbef4ff28cd8a..0555433d8050b 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
@@ -343,7 +343,7 @@ bool dm_helpers_dp_mst_send_payload_allocation(
struct amdgpu_dm_connector *aconnector;
struct drm_dp_mst_topology_state *mst_state;
struct drm_dp_mst_topology_mgr *mst_mgr;
- struct drm_dp_mst_atomic_payload *new_payload, *old_payload;
+ struct drm_dp_mst_atomic_payload *new_payload;
enum mst_progress_status set_flag = MST_ALLOCATE_NEW_PAYLOAD;
enum mst_progress_status clr_flag = MST_CLEAR_ALLOCATED_PAYLOAD;
int ret = 0;
@@ -366,9 +366,8 @@ bool dm_helpers_dp_mst_send_payload_allocation(
if (enable) {
ret = drm_dp_add_payload_part2(mst_mgr, mst_state->base.state, new_payload);
} else {
- dm_helpers_construct_old_payload(stream->link, mst_state->pbn_div,
- new_payload, old_payload);
- drm_dp_remove_payload_part2(mst_mgr, mst_state, old_payload, new_payload);
+ drm_dp_remove_current_payload_part2(mst_mgr, mst_state->base.state,
+ aconnector->mst_output_port);
}
if (ret) {
diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index e04f87ff755ac..4d25dba789e91 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3382,37 +3382,70 @@ EXPORT_SYMBOL(drm_dp_remove_payload_part1);
* drm_dp_remove_payload_part2() - Remove an MST payload locally
* @mgr: Manager to use.
* @mst_state: The MST atomic state
- * @old_payload: The payload with its old state
- * @new_payload: The payload with its latest state
+ * @port: MST port
*
* Updates the starting time slots of all other payloads which would have been shifted towards
* the start of the payload ID table as a result of removing a payload. Driver should call this
* function whenever it removes a payload in its HW. It's independent to the result of payload
* allocation/deallocation at branch devices along the virtual channel.
*/
-void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
- struct drm_dp_mst_topology_state *mst_state,
- const struct drm_dp_mst_atomic_payload *old_payload,
- struct drm_dp_mst_atomic_payload *new_payload)
+int drm_dp_remove_current_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_atomic_state *state,
+ struct drm_dp_mst_port *port)
{
struct drm_dp_mst_atomic_payload *pos;
+ struct drm_dp_mst_topology_state *mst_state =
+ drm_atomic_get_new_mst_topology_state(state, mgr);
+ struct drm_dp_mst_atomic_payload *new_payload =
+ drm_atomic_get_mst_payload_state(mst_state, port);
+ int time_slots_to_remove;
+ u8 next_payload_vc_start = mgr->next_start_slot;
+
+ /* Find the current allocated time slot number of the payload */
+ list_for_each_entry(pos, &mst_state->payloads, next) {
+ if (pos != new_payload &&
+ pos->vc_start_slot > new_payload->vc_start_slot &&
+ pos->vc_start_slot < next_payload_vc_start)
+ next_payload_vc_start = pos->vc_start_slot;
+ }
+
+ time_slots_to_remove = next_payload_vc_start - new_payload->vc_start_slot;
/* Remove local payload allocation */
list_for_each_entry(pos, &mst_state->payloads, next) {
if (pos != new_payload && pos->vc_start_slot > new_payload->vc_start_slot)
- pos->vc_start_slot -= old_payload->time_slots;
+ pos->vc_start_slot -= time_slots_to_remove;
}
new_payload->vc_start_slot = -1;
mgr->payload_count--;
- mgr->next_start_slot -= old_payload->time_slots;
+ mgr->next_start_slot -= time_slots_to_remove;
if (new_payload->delete)
drm_dp_mst_put_port_malloc(new_payload->port);
new_payload->payload_allocation_status = DRM_DP_MST_PAYLOAD_ALLOCATION_NONE;
+
+ return time_slots_to_remove;
+}
+EXPORT_SYMBOL(drm_dp_remove_current_payload_part2);
+
+void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_atomic_state *state,
+ struct drm_dp_mst_port *port)
+{
+ struct drm_dp_mst_topology_state *old_mst_state =
+ drm_atomic_get_old_mst_topology_state(state, mgr);
+ struct drm_dp_mst_atomic_payload *old_payload =
+ drm_atomic_get_mst_payload_state(old_mst_state, port);
+ int time_slots;
+
+ time_slots = drm_dp_remove_current_payload_part2(mgr, state, port);
+
+ drm_WARN_ON(mgr->dev, time_slots != old_payload->time_slots);
}
EXPORT_SYMBOL(drm_dp_remove_payload_part2);
+
/**
* drm_dp_add_payload_part2() - Execute payload update part 2
* @mgr: Manager to use.
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 1c7f0b6afe475..3ab491d9c8d27 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -576,14 +576,6 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
struct intel_dp *intel_dp = &dig_port->dp;
struct intel_connector *connector =
to_intel_connector(old_conn_state->connector);
- struct drm_dp_mst_topology_state *old_mst_state =
- drm_atomic_get_old_mst_topology_state(&state->base, &intel_dp->mst_mgr);
- struct drm_dp_mst_topology_state *new_mst_state =
- drm_atomic_get_new_mst_topology_state(&state->base, &intel_dp->mst_mgr);
- const struct drm_dp_mst_atomic_payload *old_payload =
- drm_atomic_get_mst_payload_state(old_mst_state, connector->port);
- struct drm_dp_mst_atomic_payload *new_payload =
- drm_atomic_get_mst_payload_state(new_mst_state, connector->port);
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
bool last_mst_stream;
@@ -604,8 +596,7 @@ static void intel_mst_post_disable_dp(struct intel_atomic_state *state,
wait_for_act_sent(encoder, old_crtc_state);
- drm_dp_remove_payload_part2(&intel_dp->mst_mgr, new_mst_state,
- old_payload, new_payload);
+ drm_dp_remove_payload_part2(&intel_dp->mst_mgr, &state->base, connector->port);
intel_ddi_disable_transcoder_func(old_crtc_state);
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index bba01fa0780c9..1ed724fe11f96 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -889,17 +889,13 @@ nv50_msto_cleanup(struct drm_atomic_state *state,
struct nouveau_drm *drm = nouveau_drm(msto->encoder.dev);
struct drm_dp_mst_atomic_payload *new_payload =
drm_atomic_get_mst_payload_state(new_mst_state, msto->mstc->port);
- struct drm_dp_mst_topology_state *old_mst_state =
- drm_atomic_get_old_mst_topology_state(state, mgr);
- const struct drm_dp_mst_atomic_payload *old_payload =
- drm_atomic_get_mst_payload_state(old_mst_state, msto->mstc->port);
NV_ATOMIC(drm, "%s: msto cleanup\n", msto->encoder.name);
if (msto->disabled) {
msto->mstc = NULL;
msto->disabled = false;
- drm_dp_remove_payload_part2(mgr, new_mst_state, old_payload, new_payload);
+ drm_dp_remove_payload_part2(mgr, state, msto->mstc->port);
} else if (msto->enabled) {
drm_dp_add_payload_part2(mgr, state, new_payload);
msto->enabled = false;
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 4429d3b1745b6..9288501ffe8d2 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -856,9 +856,11 @@ void drm_dp_remove_payload_part1(struct drm_dp_mst_topology_mgr *mgr,
struct drm_dp_mst_topology_state *mst_state,
struct drm_dp_mst_atomic_payload *payload);
void drm_dp_remove_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
- struct drm_dp_mst_topology_state *mst_state,
- const struct drm_dp_mst_atomic_payload *old_payload,
- struct drm_dp_mst_atomic_payload *new_payload);
+ struct drm_atomic_state *state,
+ struct drm_dp_mst_port *port);
+int drm_dp_remove_current_payload_part2(struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_atomic_state *state,
+ struct drm_dp_mst_port *port);
int drm_dp_check_act_status(struct drm_dp_mst_topology_mgr *mgr);
> Thanks for helping me out here.
