| Message ID | 20210419225523.184856-4-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Use new DRM printk funcs (like drm_dbg_*()) in DP helpers | expand |
On Mon, Apr 19, 2021 at 06:55:05PM -0400, Lyude Paul wrote: > When moving around drm_dp_aux_register() calls, it turned out we > accidentally managed to cause issues with the Tegra driver due to the fact > the Tegra driver would attempt to retrieve a reference to the AUX channel's > i2c adapter - which wouldn't be initialized until drm_dp_aux_register() is > called. > > This doesn't actually make a whole ton of sense, as it's not unexpected for > a driver to need to be able to use an AUX adapter before it's been > registered. Likewise-it's not unexpected for a driver to try using the i2c > adapter for said AUX channel before it's been registered as well. In fact, > the current documentation for drm_dp_aux_init() even seems to imply that > drm_dp_aux_init() is supposed to be handling i2c adapter creation for this > precise reason - not drm_dp_aux_register(). > > Since the i2c adapter doesn't need to be linked to the DRM device in any > way, we can just fix this problem by moving i2c adapter creation out of > drm_dp_aux_register() and into drm_dp_aux_init(). Additionally, since this > means that drm_dp_aux_init() can fail we go ahead and add a __must_check > attribute to it so that drivers don't ignore its return status on failures. > And finally, we add a drm_dp_aux_fini() and hook it up in all DRM drivers > across the kernel to take care of cleaning up the i2c adapter once it's no > longer needed. > > This should also fix the regressions noted in the Tegra driver. The init vs. register split is intentional. Registering the thing and allowing userspace access to it before the rest of the driver is ready isn't particularly great. For a while now we've tried to move towards an architecture where the driver is fully initialzied before anything gets exposed to userspace.
On Tue, 2021-04-20 at 02:16 +0300, Ville Syrjälä wrote: > > The init vs. register split is intentional. Registering the thing > and allowing userspace access to it before the rest of the driver > is ready isn't particularly great. For a while now we've tried to > move towards an architecture where the driver is fully initialzied > before anything gets exposed to userspace. Yeah-thank you for pointing this out. Thierry - do you think there's an alternate solution we could go with in Tegra to fix the get_device() issue that wouldn't require us trying to expose the i2c adapter early? >
OK - talked with Ville a bit on this and did some of my own research, I actually think that moving i2c to drm_dp_aux_init() is the right decision for the time being. The reasoning behind this being that as shown by my previous work of fixing drivers that call drm_dp_aux_register() too early - it seems like there's already been drivers that have been working just fine with setting up the i2c device before DRM registration. In the future, it'd probably be better if we can split up i2c_add_adapter() into an init and register function - but we'll have to talk with the i2c maintainers to see if this is acceptable w/ them On Thu, 2021-04-22 at 13:18 -0400, Lyude Paul wrote: > On Tue, 2021-04-20 at 02:16 +0300, Ville Syrjälä wrote: > > > > The init vs. register split is intentional. Registering the thing > > and allowing userspace access to it before the rest of the driver > > is ready isn't particularly great. For a while now we've tried to > > move towards an architecture where the driver is fully initialzied > > before anything gets exposed to userspace. > > Yeah-thank you for pointing this out. Thierry - do you think there's an > alternate solution we could go with in Tegra to fix the get_device() issue > that wouldn't require us trying to expose the i2c adapter early? > > > >
On Thu, 2021-04-22 at 18:33 -0400, Lyude Paul wrote: > OK - talked with Ville a bit on this and did some of my own research, I > actually think that moving i2c to drm_dp_aux_init() is the right decision > for > the time being. The reasoning behind this being that as shown by my previous > work of fixing drivers that call drm_dp_aux_register() too early - it seems > like there's already been drivers that have been working just fine with > setting up the i2c device before DRM registration. > > In the future, it'd probably be better if we can split up i2c_add_adapter() > into an init and register function - but we'll have to talk with the i2c > maintainers to see if this is acceptable w/ them Actually - I think adding the ability to refcount dp aux adapters might be a better solution so I'm going to try that! > > On Thu, 2021-04-22 at 13:18 -0400, Lyude Paul wrote: > > On Tue, 2021-04-20 at 02:16 +0300, Ville Syrjälä wrote: > > > > > > The init vs. register split is intentional. Registering the thing > > > and allowing userspace access to it before the rest of the driver > > > is ready isn't particularly great. For a while now we've tried to > > > move towards an architecture where the driver is fully initialzied > > > before anything gets exposed to userspace. > > > > Yeah-thank you for pointing this out. Thierry - do you think there's an > > alternate solution we could go with in Tegra to fix the get_device() issue > > that wouldn't require us trying to expose the i2c adapter early? > > > > > > > >
On Thu, Apr 22, 2021 at 01:18:09PM -0400, Lyude Paul wrote: > On Tue, 2021-04-20 at 02:16 +0300, Ville Syrjälä wrote: > > > > The init vs. register split is intentional. Registering the thing > > and allowing userspace access to it before the rest of the driver > > is ready isn't particularly great. For a while now we've tried to > > move towards an architecture where the driver is fully initialzied > > before anything gets exposed to userspace. > > Yeah-thank you for pointing this out. Thierry - do you think there's an > alternate solution we could go with in Tegra to fix the get_device() issue > that wouldn't require us trying to expose the i2c adapter early? I suppose we could do it in a hackish way that grabs a reference to the I2C adapter only upon registration. We can't do that for the regular I2C DDC case where the I2C controller is an external one because by the time we get to registration it could've gone again. This would make both code paths asymmetric, so I'd prefer not to do it. Perhaps it could serve as an stop-gap solution until something better is in place, though. Thierry
On Thu, Apr 22, 2021 at 06:33:44PM -0400, Lyude Paul wrote: > OK - talked with Ville a bit on this and did some of my own research, I > actually think that moving i2c to drm_dp_aux_init() is the right decision for > the time being. The reasoning behind this being that as shown by my previous > work of fixing drivers that call drm_dp_aux_register() too early - it seems > like there's already been drivers that have been working just fine with > setting up the i2c device before DRM registration. > > In the future, it'd probably be better if we can split up i2c_add_adapter() > into an init and register function - but we'll have to talk with the i2c > maintainers to see if this is acceptable w/ them Yeah, that sounds like a better long-term solution. We could leave i2c_add_adapter() in place, since it's already half-way split up into some initialization code and i2c_register_adapter(), so it shouldn't be all that difficult to split out an i2c_init_adapter() so that outside users can do the split setup. Thierry
On Fri, Apr 23, 2021 at 12:11:06AM -0400, Lyude Paul wrote: > On Thu, 2021-04-22 at 18:33 -0400, Lyude Paul wrote: > > OK - talked with Ville a bit on this and did some of my own research, I > > actually think that moving i2c to drm_dp_aux_init() is the right decision > > for > > the time being. The reasoning behind this being that as shown by my previous > > work of fixing drivers that call drm_dp_aux_register() too early - it seems > > like there's already been drivers that have been working just fine with > > setting up the i2c device before DRM registration. > > > > In the future, it'd probably be better if we can split up i2c_add_adapter() > > into an init and register function - but we'll have to talk with the i2c > > maintainers to see if this is acceptable w/ them > > Actually - I think adding the ability to refcount dp aux adapters might be a > better solution so I'm going to try that! I'm curious: how is a DP AUX adapter reference count going to solve the issue of potentially registering devices too early (i.e. before the DRM is registered)? Is it because registering too early could cause a reference count problem if somebody get a hold of the DP AUX adapter before the parent DRM device is around? Thierry
On Fri, 2021-04-23 at 14:39 +0200, Thierry Reding wrote: > > I'm curious: how is a DP AUX adapter reference count going to solve the > issue of potentially registering devices too early (i.e. before the DRM > is registered)? > > Is it because registering too early could cause a reference count > problem if somebody get a hold of the DP AUX adapter before the parent > DRM device is around? Well currently the problem is that we kind of want to avoid setting up the i2c adapter before the DRM driver is registered with userspace, but it's not really possible to do that if we need the core device struct for the ddc adapter initialized so that tegra can call get_device() on it in drivers/gpu/drm/tegra/sor.c. So my thought is instead of calling get_device() on the ddc adapter that the AUX channel provides, why not just call it on the actual platform device that implements the AUX channel instead? I think this should work pretty nicely while still preventing the platform device for the AUX channel from disappearing before the SOR has disappeared. > > Thierry
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c index b9c11c2b2885..23b2134a651b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c @@ -1963,8 +1963,11 @@ amdgpu_connector_add(struct amdgpu_device *adev, connector->display_info.subpixel_order = subpixel_order; - if (has_aux) - amdgpu_atombios_dp_aux_init(amdgpu_connector); + if (has_aux) { + int ret = amdgpu_atombios_dp_aux_init(amdgpu_connector); + if (ret) + goto failed; + } if (connector_type == DRM_MODE_CONNECTOR_DisplayPort || connector_type == DRM_MODE_CONNECTOR_eDP) { diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c index a3ba9ca11e98..54c209ab8c9f 100644 --- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c +++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c @@ -184,12 +184,18 @@ amdgpu_atombios_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *m return ret; } -void amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector) +int amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector) { + int ret; + amdgpu_connector->ddc_bus->rec.hpd = amdgpu_connector->hpd.hpd; amdgpu_connector->ddc_bus->aux.transfer = amdgpu_atombios_dp_aux_transfer; - drm_dp_aux_init(&amdgpu_connector->ddc_bus->aux); + ret = drm_dp_aux_init(&amdgpu_connector->ddc_bus->aux); + if (ret) + return ret; + amdgpu_connector->ddc_bus->has_aux = true; + return ret; } /***** general DP utility functions *****/ diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.h b/drivers/gpu/drm/amd/amdgpu/atombios_dp.h index f59d85eaddf0..6b65cbf009fd 100644 --- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.h +++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.h @@ -24,7 +24,7 @@ #ifndef __ATOMBIOS_DP_H__ #define __ATOMBIOS_DP_H__ -void amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector); +int amdgpu_atombios_dp_aux_init(struct amdgpu_connector *amdgpu_connector); u8 amdgpu_atombios_dp_get_sinktype(struct amdgpu_connector *amdgpu_connector); int amdgpu_atombios_dp_get_dpcd(struct amdgpu_connector *amdgpu_connector); int amdgpu_atombios_dp_get_panel_mode(struct drm_encoder *encoder, diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c index 3dee9cce9c9e..51fbbf3ef59b 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c @@ -439,7 +439,10 @@ int amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm, dm_aux->aux.transfer = dm_dp_aux_transfer; dm_aux->ddc_service = aconnector->dc_link->ddc; - drm_dp_aux_init(&dm_aux->aux); + ret = drm_dp_aux_init(&dm_aux->aux); + if (ret) + goto free_name; + drm_dp_cec_register_connector(&dm_aux->aux, &aconnector->base); if (aconnector->base.connector_type == DRM_MODE_CONNECTOR_eDP) @@ -456,6 +459,7 @@ int amdgpu_dm_initialize_dp_connector(struct amdgpu_display_manager *dm, return 0; unreg_cec: drm_dp_cec_unregister_connector(&dm_aux->aux); +free_name: kfree(dm_aux->aux.name); return ret; } diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c index aa6cda458eb9..c105dcb79c37 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx6345.c @@ -794,6 +794,8 @@ static int anx6345_i2c_remove(struct i2c_client *client) unregister_i2c_dummy_clients(anx6345); + drm_dp_aux_fini(&anx6345->aux); + kfree(anx6345->edid); mutex_destroy(&anx6345->lock); diff --git a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c index f20558618220..0778458e81be 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c +++ b/drivers/gpu/drm/bridge/analogix/analogix-anx78xx.c @@ -1364,6 +1364,8 @@ static int anx78xx_i2c_remove(struct i2c_client *client) unregister_i2c_dummy_clients(anx78xx); + drm_dp_aux_fini(&anx78xx->aux); + kfree(anx78xx->edid); return 0; diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index f115233b1cb9..f82e193d32ae 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1806,6 +1806,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_unbind); void analogix_dp_remove(struct analogix_dp_device *dp) { clk_disable_unprepare(dp->clock); + drm_dp_aux_fini(&dp->aux); } EXPORT_SYMBOL_GPL(analogix_dp_remove); diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index 01e95466502a..c5e2bc75b226 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -2436,14 +2436,17 @@ static int cdns_mhdp_probe(struct platform_device *pdev) mutex_init(&mhdp->link_mutex); spin_lock_init(&mhdp->start_lock); - drm_dp_aux_init(&mhdp->aux); mhdp->aux.dev = dev; mhdp->aux.transfer = cdns_mhdp_transfer; + ret = drm_dp_aux_init(&mhdp->aux); + if (ret) + return ret; mhdp->regs = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(mhdp->regs)) { dev_err(dev, "Failed to get memory resource\n"); - return PTR_ERR(mhdp->regs); + ret = PTR_ERR(mhdp->regs); + goto aux_fini; } mhdp->sapb_regs = devm_platform_ioremap_resource_byname(pdev, "mhdptx-sapb"); @@ -2458,7 +2461,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev) mhdp->phy = devm_of_phy_get_by_index(dev, pdev->dev.of_node, 0); if (IS_ERR(mhdp->phy)) { dev_err(dev, "no PHY configured\n"); - return PTR_ERR(mhdp->phy); + ret = PTR_ERR(mhdp->phy); + goto aux_fini; } platform_set_drvdata(pdev, mhdp); @@ -2555,6 +2559,8 @@ static int cdns_mhdp_probe(struct platform_device *pdev) pm_runtime_disable(dev); clk_disable: clk_disable_unprepare(mhdp->clk); +aux_fini: + drm_dp_aux_fini(&mhdp->aux); return ret; } @@ -2597,6 +2603,8 @@ static int cdns_mhdp_remove(struct platform_device *pdev) clk_disable_unprepare(mhdp->clk); + drm_dp_aux_fini(&mhdp->aux); + return ret; } diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index da89922721ed..6a56acf36a9a 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1693,7 +1693,9 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) tc->aux.name = "TC358767 AUX i2c adapter"; tc->aux.dev = tc->dev; tc->aux.transfer = tc_aux_transfer; - drm_dp_aux_init(&tc->aux); + ret = drm_dp_aux_init(&tc->aux); + if (ret) + return ret; tc->bridge.funcs = &tc_bridge_funcs; if (tc->hpd_pin >= 0) @@ -1713,6 +1715,7 @@ static int tc_remove(struct i2c_client *client) struct tc_data *tc = i2c_get_clientdata(client); drm_bridge_remove(&tc->bridge); + drm_dp_aux_fini(&tc->aux); return 0; } diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 88df4dd0f39d..6190451c9195 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -1291,17 +1291,16 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pm_runtime_enable(pdata->dev); ret = ti_sn_setup_gpio_controller(pdata); - if (ret) { - pm_runtime_disable(pdata->dev); - return ret; - } - + if (ret) + goto fail_pm; i2c_set_clientdata(client, pdata); pdata->aux.name = "ti-sn65dsi86-aux"; pdata->aux.dev = pdata->dev; pdata->aux.transfer = ti_sn_aux_transfer; - drm_dp_aux_init(&pdata->aux); + ret = drm_dp_aux_init(&pdata->aux); + if (ret) + goto fail_pm; pdata->bridge.funcs = &ti_sn_bridge_funcs; pdata->bridge.of_node = client->dev.of_node; @@ -1311,6 +1310,9 @@ static int ti_sn_bridge_probe(struct i2c_client *client, ti_sn_debugfs_init(pdata); return 0; +fail_pm: + pm_runtime_disable(pdata->dev); + return ret; } static int ti_sn_bridge_remove(struct i2c_client *client) @@ -1332,6 +1334,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client) mipi_dsi_device_unregister(pdata->dsi); } + drm_dp_aux_fini(&pdata->aux); + drm_bridge_remove(&pdata->bridge); return 0; diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index cb2f53e56685..6c9e552233fd 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1731,10 +1731,18 @@ EXPORT_SYMBOL(drm_dp_remote_aux_init); * If you need to use the drm_dp_aux's i2c adapter prior to registering it * with the outside world, call drm_dp_aux_init() first. You must still * call drm_dp_aux_register() once the connector has been registered to - * allow userspace access to the auxiliary DP channel. + * allow userspace access to the auxiliary DP channel. Once the AUX channel is + * no longer being used and has been unregistered with + * drm_dp_aux_unregister(), the driver must clean up any resources it's + * allocated with drm_dp_aux_fini(). + * + * Returns: + * %0 on success, negative error code on failure */ -void drm_dp_aux_init(struct drm_dp_aux *aux) +int drm_dp_aux_init(struct drm_dp_aux *aux) { + int ret; + mutex_init(&aux->hw_mutex); mutex_init(&aux->cec.lock); INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work); @@ -1744,16 +1752,55 @@ void drm_dp_aux_init(struct drm_dp_aux *aux) aux->ddc.retries = 3; aux->ddc.lock_ops = &drm_dp_i2c_lock_ops; + + aux->ddc.class = I2C_CLASS_DDC; + aux->ddc.owner = THIS_MODULE; + aux->ddc.dev.parent = aux->dev; + + strlcpy(aux->ddc.name, aux->name ?: dev_name(aux->dev), sizeof(aux->ddc.name)); + + ret = i2c_add_adapter(&aux->ddc); + if (ret) { + aux->ddc.algo = NULL; + mutex_destroy(&aux->hw_mutex); + mutex_destroy(&aux->cec.lock); + } + + return ret; } EXPORT_SYMBOL(drm_dp_aux_init); /** - * drm_dp_aux_register() - initialise and register aux channel + * drm_dp_aux_fini() - release resources from an aux channel * @aux: DisplayPort AUX channel * - * Automatically calls drm_dp_aux_init() if this hasn't been done yet. + * Cleans up any resources associated with a DP AUX channel, along with + * removing it's associated i2c adapter. Must always be called once an AUX + * channel is being removed. Note that if drm_dp_aux_init() was not called on + * @aux, this function is a no-op. + */ +void drm_dp_aux_fini(struct drm_dp_aux *aux) +{ + if (!aux->ddc.algo) + return; + + i2c_del_adapter(&aux->ddc); + mutex_destroy(&aux->hw_mutex); + mutex_destroy(&aux->cec.lock); +} +EXPORT_SYMBOL(drm_dp_aux_fini); + +/** + * drm_dp_aux_register() - register aux channel + * @aux: DisplayPort AUX channel + * + * Automatically calls drm_dp_aux_init() if this hasn't been done yet. The + * driver must make sure to call drm_dp_aux_unregister() to unregister the + * device, and drm_dp_aux_fini() to cleanup the device after it's been + * unregistered. + * * This should only be called when the underlying &struct drm_connector is - * initialiazed already. Therefore the best place to call this is from + * initialized already. Therefore the best place to call this is from * &drm_connector_funcs.late_register. Not that drivers which don't follow this * will Oops when CONFIG_DRM_DP_AUX_CHARDEV is enabled. * @@ -1766,39 +1813,32 @@ EXPORT_SYMBOL(drm_dp_aux_init); int drm_dp_aux_register(struct drm_dp_aux *aux) { int ret; + const bool init_aux = !aux->ddc.algo; - if (!aux->ddc.algo) - drm_dp_aux_init(aux); - - aux->ddc.class = I2C_CLASS_DDC; - aux->ddc.owner = THIS_MODULE; - aux->ddc.dev.parent = aux->dev; - - strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), - sizeof(aux->ddc.name)); + if (init_aux) { + ret = drm_dp_aux_init(aux); + if (ret) + return ret; + } ret = drm_dp_aux_register_devnode(aux); - if (ret) - return ret; + if (ret && init_aux) + drm_dp_aux_fini(aux); - ret = i2c_add_adapter(&aux->ddc); - if (ret) { - drm_dp_aux_unregister_devnode(aux); - return ret; - } - - return 0; + return ret; } EXPORT_SYMBOL(drm_dp_aux_register); /** * drm_dp_aux_unregister() - unregister an AUX adapter * @aux: DisplayPort AUX channel + * + * Note that this function does not take care of calling drm_dp_aux_fini(), + * the driver must handle this part itself. */ void drm_dp_aux_unregister(struct drm_dp_aux *aux) { drm_dp_aux_unregister_devnode(aux); - i2c_del_adapter(&aux->ddc); } EXPORT_SYMBOL(drm_dp_aux_unregister); diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c b/drivers/gpu/drm/i915/display/intel_dp_aux.c index 7e83bc2cc34a..a79c6e781638 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c @@ -644,15 +644,17 @@ void intel_dp_aux_fini(struct intel_dp *intel_dp) if (cpu_latency_qos_request_active(&intel_dp->pm_qos)) cpu_latency_qos_remove_request(&intel_dp->pm_qos); + drm_dp_aux_fini(&intel_dp->aux); kfree(intel_dp->aux.name); } -void intel_dp_aux_init(struct intel_dp *intel_dp) +int intel_dp_aux_init(struct intel_dp *intel_dp) { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct intel_encoder *encoder = &dig_port->base; enum aux_ch aux_ch = dig_port->aux_ch; + int ret; if (DISPLAY_VER(dev_priv) >= 12) { intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg; @@ -682,7 +684,9 @@ void intel_dp_aux_init(struct intel_dp *intel_dp) else intel_dp->get_aux_send_ctl = g4x_get_aux_send_ctl; - drm_dp_aux_init(&intel_dp->aux); + ret = drm_dp_aux_init(&intel_dp->aux); + if (ret) + return ret; /* Failure to allocate our preferred name is not critical */ if (DISPLAY_VER(dev_priv) >= 12 && aux_ch >= AUX_CH_USBC1) @@ -696,4 +700,6 @@ void intel_dp_aux_init(struct intel_dp *intel_dp) intel_dp->aux.transfer = intel_dp_aux_transfer; cpu_latency_qos_add_request(&intel_dp->pm_qos, PM_QOS_DEFAULT_VALUE); + + return ret; } diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.h b/drivers/gpu/drm/i915/display/intel_dp_aux.h index 4afbe76217b9..321bab28eb07 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_aux.h +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.h @@ -13,6 +13,6 @@ struct intel_dp; u32 intel_dp_pack_aux(const u8 *src, int src_bytes); void intel_dp_aux_fini(struct intel_dp *intel_dp); -void intel_dp_aux_init(struct intel_dp *intel_dp); +int intel_dp_aux_init(struct intel_dp *intel_dp); #endif /* __INTEL_DP_AUX_H__ */ diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c index 7c22bfe0fc7d..2ef42ca1fda5 100644 --- a/drivers/gpu/drm/msm/dp/dp_aux.c +++ b/drivers/gpu/drm/msm/dp/dp_aux.c @@ -494,6 +494,7 @@ int dp_aux_register(struct drm_dp_aux *dp_aux) void dp_aux_unregister(struct drm_dp_aux *dp_aux) { drm_dp_aux_unregister(dp_aux); + drm_dp_aux_fini(dp_aux); } struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog) diff --git a/drivers/gpu/drm/msm/edp/edp_aux.c b/drivers/gpu/drm/msm/edp/edp_aux.c index df10a0196d94..ced3a38781ed 100644 --- a/drivers/gpu/drm/msm/edp/edp_aux.c +++ b/drivers/gpu/drm/msm/edp/edp_aux.c @@ -218,6 +218,7 @@ void msm_edp_aux_destroy(struct device *dev, struct edp_aux *aux) { if (aux) { drm_dp_aux_unregister(&aux->drm_aux); + drm_dp_aux_fini(&aux->drm_aux); mutex_destroy(&aux->msg_mutex); } } diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c index 61e6d7412505..e5a93fab856e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_connector.c +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c @@ -402,6 +402,7 @@ nouveau_connector_destroy(struct drm_connector *connector) if (nv_connector->aux.transfer) { drm_dp_cec_unregister_connector(&nv_connector->aux); drm_dp_aux_unregister(&nv_connector->aux); + drm_dp_aux_fini(&nv_connector->aux); kfree(nv_connector->aux.name); } kfree(connector); diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c index 607ad5620bd9..f185d3e96bf8 100644 --- a/drivers/gpu/drm/radeon/radeon_connectors.c +++ b/drivers/gpu/drm/radeon/radeon_connectors.c @@ -921,6 +921,7 @@ static void radeon_connector_unregister(struct drm_connector *connector) if (radeon_connector->ddc_bus && radeon_connector->ddc_bus->has_aux) { drm_dp_aux_unregister(&radeon_connector->ddc_bus->aux); + drm_dp_aux_fini(&radeon_connector->ddc_bus->aux); radeon_connector->ddc_bus->has_aux = false; } } diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index ea56c6ec25e4..6379f2f32d4d 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -534,7 +534,9 @@ static int tegra_dpaux_probe(struct platform_device *pdev) dpaux->aux.transfer = tegra_dpaux_transfer; dpaux->aux.dev = &pdev->dev; - drm_dp_aux_init(&dpaux->aux); + err = drm_dp_aux_init(&dpaux->aux); + if (err) + return err; /* * Assume that by default the DPAUX/I2C pads will be used for HDMI, @@ -546,7 +548,7 @@ static int tegra_dpaux_probe(struct platform_device *pdev) */ err = tegra_dpaux_pad_config(dpaux, DPAUX_PADCTL_FUNC_I2C); if (err < 0) - return err; + goto aux_fini; #ifdef CONFIG_GENERIC_PINCONF dpaux->desc.name = dev_name(&pdev->dev); @@ -559,7 +561,8 @@ static int tegra_dpaux_probe(struct platform_device *pdev) dpaux->pinctrl = devm_pinctrl_register(&pdev->dev, &dpaux->desc, dpaux); if (IS_ERR(dpaux->pinctrl)) { dev_err(&pdev->dev, "failed to register pincontrol\n"); - return PTR_ERR(dpaux->pinctrl); + err = PTR_ERR(dpaux->pinctrl); + goto aux_fini; } #endif /* enable and clear all interrupts */ @@ -573,6 +576,9 @@ static int tegra_dpaux_probe(struct platform_device *pdev) mutex_unlock(&dpaux_lock); return 0; +aux_fini: + drm_dp_aux_fini(&dpaux->aux); + return err; } static int tegra_dpaux_remove(struct platform_device *pdev) @@ -584,6 +590,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) /* make sure pads are powered down when not in use */ tegra_dpaux_pad_power_down(dpaux); + drm_dp_aux_fini(&dpaux->aux); + pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev); diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c index 59d1fb017da0..5370b99aabdf 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_dp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c @@ -1083,6 +1083,7 @@ static int zynqmp_dp_aux_init(struct zynqmp_dp *dp) static void zynqmp_dp_aux_cleanup(struct zynqmp_dp *dp) { drm_dp_aux_unregister(&dp->aux); + drm_dp_aux_fini(&dp->aux); } /* ----------------------------------------------------------------------------- diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index e44b0ee7b85e..54d13a4d819f 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -2009,7 +2009,8 @@ bool drm_dp_lttpr_voltage_swing_level_3_supported(const u8 caps[DP_LTTPR_PHY_CAP bool drm_dp_lttpr_pre_emphasis_level_3_supported(const u8 caps[DP_LTTPR_PHY_CAP_SIZE]); void drm_dp_remote_aux_init(struct drm_dp_aux *aux); -void drm_dp_aux_init(struct drm_dp_aux *aux); +__must_check int drm_dp_aux_init(struct drm_dp_aux *aux); +void drm_dp_aux_fini(struct drm_dp_aux *aux); __must_check int drm_dp_aux_register(struct drm_dp_aux *aux); void drm_dp_aux_unregister(struct drm_dp_aux *aux);
When moving around drm_dp_aux_register() calls, it turned out we accidentally managed to cause issues with the Tegra driver due to the fact the Tegra driver would attempt to retrieve a reference to the AUX channel's i2c adapter - which wouldn't be initialized until drm_dp_aux_register() is called. This doesn't actually make a whole ton of sense, as it's not unexpected for a driver to need to be able to use an AUX adapter before it's been registered. Likewise-it's not unexpected for a driver to try using the i2c adapter for said AUX channel before it's been registered as well. In fact, the current documentation for drm_dp_aux_init() even seems to imply that drm_dp_aux_init() is supposed to be handling i2c adapter creation for this precise reason - not drm_dp_aux_register(). Since the i2c adapter doesn't need to be linked to the DRM device in any way, we can just fix this problem by moving i2c adapter creation out of drm_dp_aux_register() and into drm_dp_aux_init(). Additionally, since this means that drm_dp_aux_init() can fail we go ahead and add a __must_check attribute to it so that drivers don't ignore its return status on failures. And finally, we add a drm_dp_aux_fini() and hook it up in all DRM drivers across the kernel to take care of cleaning up the i2c adapter once it's no longer needed. This should also fix the regressions noted in the Tegra driver. Signed-off-by: Lyude Paul <lyude@redhat.com> Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors") Cc: Thierry Reding <treding@nvidia.com> Cc: Thierry Reding <thierry.reding@gmail.com> --- .../gpu/drm/amd/amdgpu/amdgpu_connectors.c | 7 +- drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 10 ++- drivers/gpu/drm/amd/amdgpu/atombios_dp.h | 2 +- .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 6 +- .../drm/bridge/analogix/analogix-anx6345.c | 2 + .../drm/bridge/analogix/analogix-anx78xx.c | 2 + .../drm/bridge/analogix/analogix_dp_core.c | 1 + .../drm/bridge/cadence/cdns-mhdp8546-core.c | 14 ++- drivers/gpu/drm/bridge/tc358767.c | 5 +- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++-- drivers/gpu/drm/drm_dp_helper.c | 88 ++++++++++++++----- drivers/gpu/drm/i915/display/intel_dp_aux.c | 10 ++- drivers/gpu/drm/i915/display/intel_dp_aux.h | 2 +- drivers/gpu/drm/msm/dp/dp_aux.c | 1 + drivers/gpu/drm/msm/edp/edp_aux.c | 1 + drivers/gpu/drm/nouveau/nouveau_connector.c | 1 + drivers/gpu/drm/radeon/radeon_connectors.c | 1 + drivers/gpu/drm/tegra/dpaux.c | 14 ++- drivers/gpu/drm/xlnx/zynqmp_dp.c | 1 + include/drm/drm_dp_helper.h | 3 +- 20 files changed, 140 insertions(+), 47 deletions(-)