Message ID | 20190718014329.8107-7-lyude@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | DP MST Refactors + debugging tools + suspend/resume reprobing | expand |
On Wed, Jul 17, 2019 at 09:42:29PM -0400, Lyude Paul wrote: > This will allow us to add some locking for port PDTs, which can't be > done from drm_dp_destroy_port() since we don't know what locks the > caller might be holding. Also, this gets rid of a good bit of unneeded > code. > > Cc: Juston Li <juston.li@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Harry Wentland <hwentlan@amd.com> > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++---------------- > 1 file changed, 17 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c > index defc5e09fb9a..0295e007c836 100644 > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > @@ -1509,31 +1509,22 @@ static void drm_dp_destroy_port(struct kref *kref) > container_of(kref, struct drm_dp_mst_port, topology_kref); > struct drm_dp_mst_topology_mgr *mgr = port->mgr; > > - if (!port->input) { > - kfree(port->cached_edid); > - > - /* > - * The only time we don't have a connector > - * on an output port is if the connector init > - * fails. > - */ > - if (port->connector) { > - /* we can't destroy the connector here, as > - * we might be holding the mode_config.mutex > - * from an EDID retrieval */ > - > - mutex_lock(&mgr->destroy_connector_lock); > - list_add(&port->next, &mgr->destroy_connector_list); > - mutex_unlock(&mgr->destroy_connector_lock); > - schedule_work(&mgr->destroy_connector_work); > - return; > - } > - /* no need to clean up vcpi > - * as if we have no connector we never setup a vcpi */ > - drm_dp_port_teardown_pdt(port, port->pdt); > - port->pdt = DP_PEER_DEVICE_NONE; > + /* There's nothing that needs locking to destroy an input port yet */ > + if (port->input) { > + drm_dp_mst_put_port_malloc(port); > + return; > } > - drm_dp_mst_put_port_malloc(port); > + > + kfree(port->cached_edid); > + > + /* > + * we can't destroy the connector here, as we might be holding the > + * mode_config.mutex from an EDID retrieval > + */ > + mutex_lock(&mgr->destroy_connector_lock); > + list_add(&port->next, &mgr->destroy_connector_list); > + mutex_unlock(&mgr->destroy_connector_lock); > + schedule_work(&mgr->destroy_connector_work); So if I'm not completely blind this just flattens the above code flow (by inverting the if (port->input)). > } > > /** > @@ -3881,7 +3872,8 @@ drm_dp_finish_destroy_port(struct drm_dp_mst_port *port) > { > INIT_LIST_HEAD(&port->next); > > - port->mgr->cbs->destroy_connector(port->mgr, port->connector); > + if (port->connector) And this here I can't connect with the commit message. I'm confused, did something go wrong with some rebase here, and this patch should have a different title/summary? -Daniel > + port->mgr->cbs->destroy_connector(port->mgr, port->connector); > > drm_dp_port_teardown_pdt(port, port->pdt); > port->pdt = DP_PEER_DEVICE_NONE; > -- > 2.21.0 >
On Tue, 2019-08-13 at 16:52 +0200, Daniel Vetter wrote: > On Wed, Jul 17, 2019 at 09:42:29PM -0400, Lyude Paul wrote: > > This will allow us to add some locking for port PDTs, which can't be > > done from drm_dp_destroy_port() since we don't know what locks the > > caller might be holding. Also, this gets rid of a good bit of unneeded > > code. > > > > Cc: Juston Li <juston.li@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Harry Wentland <hwentlan@amd.com> > > Signed-off-by: Lyude Paul <lyude@redhat.com> > > --- > > drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++---------------- > > 1 file changed, 17 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c > > b/drivers/gpu/drm/drm_dp_mst_topology.c > > index defc5e09fb9a..0295e007c836 100644 > > --- a/drivers/gpu/drm/drm_dp_mst_topology.c > > +++ b/drivers/gpu/drm/drm_dp_mst_topology.c > > @@ -1509,31 +1509,22 @@ static void drm_dp_destroy_port(struct kref *kref) > > container_of(kref, struct drm_dp_mst_port, topology_kref); > > struct drm_dp_mst_topology_mgr *mgr = port->mgr; > > > > - if (!port->input) { > > - kfree(port->cached_edid); > > - > > - /* > > - * The only time we don't have a connector > > - * on an output port is if the connector init > > - * fails. > > - */ > > - if (port->connector) { > > - /* we can't destroy the connector here, as > > - * we might be holding the mode_config.mutex > > - * from an EDID retrieval */ > > - > > - mutex_lock(&mgr->destroy_connector_lock); > > - list_add(&port->next, &mgr->destroy_connector_list); > > - mutex_unlock(&mgr->destroy_connector_lock); > > - schedule_work(&mgr->destroy_connector_work); > > - return; > > - } > > - /* no need to clean up vcpi > > - * as if we have no connector we never setup a vcpi */ > > - drm_dp_port_teardown_pdt(port, port->pdt); > > - port->pdt = DP_PEER_DEVICE_NONE; > > + /* There's nothing that needs locking to destroy an input port yet */ > > + if (port->input) { > > + drm_dp_mst_put_port_malloc(port); > > + return; > > } > > - drm_dp_mst_put_port_malloc(port); > > + > > + kfree(port->cached_edid); > > + > > + /* > > + * we can't destroy the connector here, as we might be holding the > > + * mode_config.mutex from an EDID retrieval > > + */ > > + mutex_lock(&mgr->destroy_connector_lock); > > + list_add(&port->next, &mgr->destroy_connector_list); > > + mutex_unlock(&mgr->destroy_connector_lock); > > + schedule_work(&mgr->destroy_connector_work); > > So if I'm not completely blind this just flattens the above code flow (by > inverting the if (port->input)). Now I'm really remembering why I refactored this. The control flow on the previous version of this is pretty misleading. To summarize so it's a bit more obvious: if (port->input) { drm_dp_mst_put_port_malloc(port); return; } else if (port->connector) { add_connector_to_destroy_list(); return; /* ^ now, this is where PDT teardown happens */ } else { drm_dp_port_teardown_pdt(port, port->pdt); } /* free edid etc etc */ So, I suppose the title of this patch would be more accurate if it was "drm/dp_mst: Remove PDT teardown in destroy_port() and refactor" I'll go ahead and change that > > > } > > > > /** > > @@ -3881,7 +3872,8 @@ drm_dp_finish_destroy_port(struct drm_dp_mst_port > > *port) > > { > > INIT_LIST_HEAD(&port->next); > > > > - port->mgr->cbs->destroy_connector(port->mgr, port->connector); > > + if (port->connector) > > And this here I can't connect with the commit message. I'm confused, did > something go wrong with some rebase here, and this patch should have a > different title/summary? > -Daniel No, this is correct. In the previous drm_dp_destroy_port() function we only added a port to the delayed destroy work if it had a connector on it. Now that we add ports unconditionally, we have to check port->connector before trying to call port->mgr->cbs->destroy_connector() since port->connector is no longer guaranteed to be != NULL. > > > + port->mgr->cbs->destroy_connector(port->mgr, port->connector); > > > > drm_dp_port_teardown_pdt(port, port->pdt); > > port->pdt = DP_PEER_DEVICE_NONE; > > -- > > 2.21.0 > >
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c index defc5e09fb9a..0295e007c836 100644 --- a/drivers/gpu/drm/drm_dp_mst_topology.c +++ b/drivers/gpu/drm/drm_dp_mst_topology.c @@ -1509,31 +1509,22 @@ static void drm_dp_destroy_port(struct kref *kref) container_of(kref, struct drm_dp_mst_port, topology_kref); struct drm_dp_mst_topology_mgr *mgr = port->mgr; - if (!port->input) { - kfree(port->cached_edid); - - /* - * The only time we don't have a connector - * on an output port is if the connector init - * fails. - */ - if (port->connector) { - /* we can't destroy the connector here, as - * we might be holding the mode_config.mutex - * from an EDID retrieval */ - - mutex_lock(&mgr->destroy_connector_lock); - list_add(&port->next, &mgr->destroy_connector_list); - mutex_unlock(&mgr->destroy_connector_lock); - schedule_work(&mgr->destroy_connector_work); - return; - } - /* no need to clean up vcpi - * as if we have no connector we never setup a vcpi */ - drm_dp_port_teardown_pdt(port, port->pdt); - port->pdt = DP_PEER_DEVICE_NONE; + /* There's nothing that needs locking to destroy an input port yet */ + if (port->input) { + drm_dp_mst_put_port_malloc(port); + return; } - drm_dp_mst_put_port_malloc(port); + + kfree(port->cached_edid); + + /* + * we can't destroy the connector here, as we might be holding the + * mode_config.mutex from an EDID retrieval + */ + mutex_lock(&mgr->destroy_connector_lock); + list_add(&port->next, &mgr->destroy_connector_list); + mutex_unlock(&mgr->destroy_connector_lock); + schedule_work(&mgr->destroy_connector_work); } /** @@ -3881,7 +3872,8 @@ drm_dp_finish_destroy_port(struct drm_dp_mst_port *port) { INIT_LIST_HEAD(&port->next); - port->mgr->cbs->destroy_connector(port->mgr, port->connector); + if (port->connector) + port->mgr->cbs->destroy_connector(port->mgr, port->connector); drm_dp_port_teardown_pdt(port, port->pdt); port->pdt = DP_PEER_DEVICE_NONE;
This will allow us to add some locking for port PDTs, which can't be done from drm_dp_destroy_port() since we don't know what locks the caller might be holding. Also, this gets rid of a good bit of unneeded code. Cc: Juston Li <juston.li@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Harry Wentland <hwentlan@amd.com> Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_mst_topology.c | 42 +++++++++++---------------- 1 file changed, 17 insertions(+), 25 deletions(-)