| Message ID | 20240722165503.2084999-4-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/14] drm/dp_mst: Factor out function to queue a topology probe work | expand |
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 >
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 --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);
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(-)