RE: [PATCH 3/3] drm/mst: adjust the function drm_dp_remove_payload_part2()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[Public]

> -----Original Message-----
> From: Imre Deak <imre.deak@xxxxxxxxx>
> Sent: Tuesday, September 12, 2023 7:19 PM
> To: Lin, Wayne <Wayne.Lin@xxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
> lyude@xxxxxxxxxx; jani.nikula@xxxxxxxxx; ville.syrjala@xxxxxxxxxxxxxxx;
> Wentland, Harry <Harry.Wentland@xxxxxxx>; Zuo, Jerry
> <Jerry.Zuo@xxxxxxx>
> Subject: Re: [PATCH 3/3] drm/mst: adjust the function
> drm_dp_remove_payload_part2()
>
> 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.
>

I see. Thanks, Imre. So it's about the idea to have another patch to extract things
like vc_start_slot out of mst 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:

The change looks good to me. I'll update the patch. Thanks for the help.

>
> 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.
--
Regards,
Wayne





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux