[PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly

[Lin, Wayne]

[AMD Official Use Only]

________________________________________
> From: Wentland, Harry <Harry.Wentland at amd.com>
> Sent: Thursday, June 17, 2021 03:53
> To: Lin, Wayne; dri-devel at lists.freedesktop.org
> Cc: lyude at redhat.com; Kazlauskas, Nicholas; Zuo, Jerry; Pillai, Aurabindo; Maarten Lankhorst; Maxime Ripard; Thomas Zimmermann; stable at vger.kernel.org
> Subject: Re: [PATCH v2 1/2] drm/dp_mst: Do not set proposed vcpi directly
>
>
>
> On 2021-06-15 11:55 p.m., Wayne Lin wrote:
> > [Why]
> > When we receive CSN message to notify one port is disconnected, we will
> > implicitly set its corresponding num_slots to 0. Later on, we will
> > eventually call drm_dp_update_payload_part1() to arrange down streams.
> >
> > In drm_dp_update_payload_part1(), we iterate over all proposed_vcpis[]
> > to do the update. Not specific to a target sink only. For example, if we
> > light up 2 monitors, Monitor_A and Monitor_B, and then we unplug
> > Monitor_B. Later on, when we call drm_dp_update_payload_part1() to try
> > to update payload for Monitor_A, we'll also implicitly clean payload for
> > Monitor_B at the same time. And finally, when we try to call
> > drm_dp_update_payload_part1() to clean payload for Monitor_B, we will do
> > nothing at this time since payload for Monitor_B has been cleaned up
> > previously.
> >
> > For StarTech 1to3 DP hub, it seems like if we didn't update DPCD payload
> > ID table then polling for "ACT Handled"(BIT_1 of DPCD 002C0h) will fail
> > and this polling will last for 3 seconds.
> >
> > Therefore, guess the best way is we don't set the proposed_vcpi[]
> > diretly. Let user of these herlper functions to set the proposed_vcpi
> > directly.
> >
> > [How]
> > 1. Revert commit 7617e9621bf2 ("drm/dp_mst: clear time slots for ports
> > invalid")
> > 2. Tackle the issue in previous commit by skipping those trasient
> > proposed VCPIs. These stale VCPIs shoulde be explicitly cleared by
> > user later on.
> >
> > Changes since v1:
> > * Change debug macro to use drm_dbg_kms() instead
> > * Amend the commit message to add Fixed & Cc tags
> >
> > Signed-off-by: Wayne Lin <Wayne.Lin at amd.com>
> > Fixes: 7617e9621bf2 ("drm/dp_mst: clear time slots for ports invalid")
> > Cc: Lyude Paul <lyude at redhat.com>
> > Cc: Wayne Lin <Wayne.Lin at amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Maxime Ripard <mripard at kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: <stable at vger.kernel.org> # v5.5+
> > ---
> >  drivers/gpu/drm/drm_dp_mst_topology.c | 36 ++++++++-------------------
> >  1 file changed, 10 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> > index 32b7f8983b94..b41b837db66d 100644
> > --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> > @@ -2501,7 +2501,7 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> >  {
> >       struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> >       struct drm_dp_mst_port *port;
> > -     int old_ddps, old_input, ret, i;
> > +     int old_ddps, ret;
> >       u8 new_pdt;
> >       bool new_mcs;
> >       bool dowork = false, create_connector = false;
> > @@ -2533,7 +2533,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> >       }
> >
> >       old_ddps = port->ddps;
> > -     old_input = port->input;
> >       port->input = conn_stat->input_port;
> >       port->ldps = conn_stat->legacy_device_plug_status;
> >       port->ddps = conn_stat->displayport_device_plug_status;
> > @@ -2555,28 +2554,6 @@ drm_dp_mst_handle_conn_stat(struct drm_dp_mst_branch *mstb,
> >               dowork = false;
> >       }
> >
> > -     if (!old_input && old_ddps != port->ddps && !port->ddps) {
> > -             for (i = 0; i < mgr->max_payloads; i++) {
> > -                     struct drm_dp_vcpi *vcpi = mgr->proposed_vcpis[i];
> > -                     struct drm_dp_mst_port *port_validated;
> > -
> > -                     if (!vcpi)
> > -                             continue;
> > -
> > -                     port_validated =
> > -                             container_of(vcpi, struct drm_dp_mst_port, vcpi);
> > -                     port_validated =
> > -                             drm_dp_mst_topology_get_port_validated(mgr, port_validated);
> > -                     if (!port_validated) {
> > -                             mutex_lock(&mgr->payload_lock);
> > -                             vcpi->num_slots = 0;
> > -                             mutex_unlock(&mgr->payload_lock);
> > -                     } else {
> > -                             drm_dp_mst_topology_put_port(port_validated);
> > -                     }
> > -             }
> > -     }
> > -
> >       if (port->connector)
> >               drm_modeset_unlock(&mgr->base.lock);
> >       else if (create_connector)
> > @@ -3410,8 +3387,15 @@ int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr)
> >                               port = drm_dp_mst_topology_get_port_validated(
> >                                   mgr, port);
> >                               if (!port) {
> > -                                     mutex_unlock(&mgr->payload_lock);
> > -                                     return -EINVAL;
> > +                                     if (vcpi->num_slots == payload->num_slots) {
> > +                                             cur_slots += vcpi->num_slots;
> > +                                             payload->start_slot = req_payload.start_slot;
> > +                                             continue;
> > +                                     } else {
> > +                                             drm_dbg_kms("Fail:set payload to invalid sink");
>
> drm_dbg_kms takes a drm_device as first parameter.
>
> Harry

Thanks Harry. Should be addressed by another patch provided by José.
>
> > +                                             mutex_unlock(&mgr->payload_lock);
> > +                                             return -EINVAL;
> > +                                     }
> >                               }
> >                               put_port = true;
> >                       }
> >
--
Regards,
Wayne