diff mbox series

[03/14] drm/dp_mst: Simplify the condition when to enumerate path resources

Message ID 20240722165503.2084999-4-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/dp_mst: Enable LT fallback for UHBR<->non-UHBR rates | expand

Commit Message

Imre Deak July 22, 2024, 4:54 p.m. UTC
In the
	if (old_ddps != port->ddps || !created)
		if (port->ddps && !port->input)
			ret = drm_dp_send_enum_path_resources();

sequence the first if's condition is true if the port exists already
(!created) or the port was created anew (hence old_ddps==0) and it was
in the plugged state (port->ddps==1). The second if's condition is true
for output ports in the plugged state. So the function is called for an
output port in the plugged state, regardless if it already existed or
not and regardless of the old plugged state. In all other cases
port->full_pbn can be zeroed as the port is either an input for which
full_pbn is never set, or an output in the unplugged state for which
full_pbn was already zeroed previously or the port was just created
(with port->full_pbn==0).

Simplify the condition, making it clear that the path resources are
always enumerated for an output port in the plugged state.

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

Comments

Manasi Navare Aug. 4, 2024, 1:45 p.m. UTC | #1
On Mon, Jul 22, 2024 at 9:55 AM Imre Deak <imre.deak@intel.com> wrote:
>
> In the
>         if (old_ddps != port->ddps || !created)
>                 if (port->ddps && !port->input)
>                         ret = drm_dp_send_enum_path_resources();
>
> sequence the first if's condition is true if the port exists already
> (!created) or the port was created anew (hence old_ddps==0) and it was
> in the plugged state (port->ddps==1). The second if's condition is true
> for output ports in the plugged state. So the function is called for an
> output port in the plugged state, regardless if it already existed or
> not and regardless of the old plugged state. In all other cases
> port->full_pbn can be zeroed as the port is either an input for which
> full_pbn is never set, or an output in the unplugged state for which
> full_pbn was already zeroed previously or the port was just created
> (with port->full_pbn==0).
>
> Simplify the condition, making it clear that the path resources are
> always enumerated for an output port in the plugged state.

Would this take care of the cases where a branch device is present
between source and the sink and
its properly allocating the resources and advertising UHBR capability
from branch to sink. This was a bug earlier
with UHBR on branch device/ MST hub

Manasi

>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_mst_topology.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 70e4bfc3532e0..bcc5bbed9bd04 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2339,7 +2339,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>  {
>         struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
>         struct drm_dp_mst_port *port;
> -       int old_ddps = 0, ret;
> +       int ret;
>         u8 new_pdt = DP_PEER_DEVICE_NONE;
>         bool new_mcs = 0;
>         bool created = false, send_link_addr = false, changed = false;
> @@ -2372,7 +2372,6 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>                  */
>                 drm_modeset_lock(&mgr->base.lock, NULL);
>
> -               old_ddps = port->ddps;
>                 changed = port->ddps != port_msg->ddps ||
>                         (port->ddps &&
>                          (port->ldps != port_msg->legacy_device_plug_status ||
> @@ -2407,15 +2406,13 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
>          * Reprobe PBN caps on both hotplug, and when re-probing the link
>          * for our parent mstb
>          */
> -       if (old_ddps != port->ddps || !created) {
> -               if (port->ddps && !port->input) {
> -                       ret = drm_dp_send_enum_path_resources(mgr, mstb,
> -                                                             port);
> -                       if (ret == 1)
> -                               changed = true;
> -               } else {
> -                       port->full_pbn = 0;
> -               }
> +       if (port->ddps && !port->input) {
> +               ret = drm_dp_send_enum_path_resources(mgr, mstb,
> +                                                     port);
> +               if (ret == 1)
> +                       changed = true;
> +       } else {
> +               port->full_pbn = 0;
>         }
>
>         ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> --
> 2.44.2
>
Imre Deak Aug. 5, 2024, 1:47 p.m. UTC | #2
On Sun, Aug 04, 2024 at 06:45:43AM -0700, Manasi Navare wrote:
> On Mon, Jul 22, 2024 at 9:55 AM Imre Deak <imre.deak@intel.com> wrote:
> >
> > In the
> >         if (old_ddps != port->ddps || !created)
> >                 if (port->ddps && !port->input)
> >                         ret = drm_dp_send_enum_path_resources();
> >
> > sequence the first if's condition is true if the port exists already
> > (!created) or the port was created anew (hence old_ddps==0) and it was
> > in the plugged state (port->ddps==1). The second if's condition is true
> > for output ports in the plugged state. So the function is called for an
> > output port in the plugged state, regardless if it already existed or
> > not and regardless of the old plugged state. In all other cases
> > port->full_pbn can be zeroed as the port is either an input for which
> > full_pbn is never set, or an output in the unplugged state for which
> > full_pbn was already zeroed previously or the port was just created
> > (with port->full_pbn==0).
> >
> > Simplify the condition, making it clear that the path resources are
> > always enumerated for an output port in the plugged state.
> 
> Would this take care of the cases where a branch device is present
> between source and the sink and its properly allocating the resources
> and advertising UHBR capability from branch to sink. This was a bug
> earlier with UHBR on branch device/ MST hub

I suppose you refer to [1].

The patchset as a whole should ensure that the BW reported by branch
devices via the ENUM_PATH_RESOURCES message is correct even across
changing between UHBR <-> non-UHBR link rates between the source and the
first downstream branch devices. More on this are detailed at [2] and
[3]. It looks like this would also address the issue described in [1],
but I couldn't test this yet, as I don't have any hub/dock supporting
UHBR rates.

--Imre

[1] https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10970
[2] https://patchwork.freedesktop.org/patch/605424/?series=136347&rev=2
[3] https://patchwork.freedesktop.org/patch/605419/?series=136347&rev=2



> 
> Manasi
> 
> >
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 70e4bfc3532e0..bcc5bbed9bd04 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -2339,7 +2339,7 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> >  {
> >         struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
> >         struct drm_dp_mst_port *port;
> > -       int old_ddps = 0, ret;
> > +       int ret;
> >         u8 new_pdt = DP_PEER_DEVICE_NONE;
> >         bool new_mcs = 0;
> >         bool created = false, send_link_addr = false, changed = false;
> > @@ -2372,7 +2372,6 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> >                  */
> >                 drm_modeset_lock(&mgr->base.lock, NULL);
> >
> > -               old_ddps = port->ddps;
> >                 changed = port->ddps != port_msg->ddps ||
> >                         (port->ddps &&
> >                          (port->ldps != port_msg->legacy_device_plug_status ||
> > @@ -2407,15 +2406,13 @@ drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
> >          * Reprobe PBN caps on both hotplug, and when re-probing the link
> >          * for our parent mstb
> >          */
> > -       if (old_ddps != port->ddps || !created) {
> > -               if (port->ddps && !port->input) {
> > -                       ret = drm_dp_send_enum_path_resources(mgr, mstb,
> > -                                                             port);
> > -                       if (ret == 1)
> > -                               changed = true;
> > -               } else {
> > -                       port->full_pbn = 0;
> > -               }
> > +       if (port->ddps && !port->input) {
> > +               ret = drm_dp_send_enum_path_resources(mgr, mstb,
> > +                                                     port);
> > +               if (ret == 1)
> > +                       changed = true;
> > +       } else {
> > +               port->full_pbn = 0;
> >         }
> >
> >         ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);
> > --
> > 2.44.2
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 70e4bfc3532e0..bcc5bbed9bd04 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2339,7 +2339,7 @@  drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 {
 	struct drm_dp_mst_topology_mgr *mgr = mstb->mgr;
 	struct drm_dp_mst_port *port;
-	int old_ddps = 0, ret;
+	int ret;
 	u8 new_pdt = DP_PEER_DEVICE_NONE;
 	bool new_mcs = 0;
 	bool created = false, send_link_addr = false, changed = false;
@@ -2372,7 +2372,6 @@  drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 		 */
 		drm_modeset_lock(&mgr->base.lock, NULL);
 
-		old_ddps = port->ddps;
 		changed = port->ddps != port_msg->ddps ||
 			(port->ddps &&
 			 (port->ldps != port_msg->legacy_device_plug_status ||
@@ -2407,15 +2406,13 @@  drm_dp_mst_handle_link_address_port(struct drm_dp_mst_branch *mstb,
 	 * Reprobe PBN caps on both hotplug, and when re-probing the link
 	 * for our parent mstb
 	 */
-	if (old_ddps != port->ddps || !created) {
-		if (port->ddps && !port->input) {
-			ret = drm_dp_send_enum_path_resources(mgr, mstb,
-							      port);
-			if (ret == 1)
-				changed = true;
-		} else {
-			port->full_pbn = 0;
-		}
+	if (port->ddps && !port->input) {
+		ret = drm_dp_send_enum_path_resources(mgr, mstb,
+						      port);
+		if (ret == 1)
+			changed = true;
+	} else {
+		port->full_pbn = 0;
 	}
 
 	ret = drm_dp_port_set_pdt(port, new_pdt, new_mcs);