Add drm_dp_aux_register_ddc/chardev() helpers that allow DP drivers to register I2C DDC adapter and character device separately.
Cc: stable@vger.kernel.org # 5.13+ Reported-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Tested-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Signed-off-by: Dmitry Osipenko digetx@gmail.com --- drivers/gpu/drm/drm_dp_helper.c | 112 +++++++++++++++++++++++++++----- include/drm/drm_dp_helper.h | 4 ++ 2 files changed, 99 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4d0d1e8e51fa..56e3e57e6dc7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -1775,7 +1775,7 @@ EXPORT_SYMBOL(drm_dp_remote_aux_init); * drm_dp_aux_init() - minimally initialise an aux channel * @aux: DisplayPort AUX channel * - * If you need to use the drm_dp_aux's i2c adapter prior to registering it with + * If you need to use the drm_dp_aux handle prior to registering it with * the outside world, call drm_dp_aux_init() first. For drivers which are * grandparents to their AUX adapters (e.g. the AUX adapter is parented by a * &drm_connector), you must still call drm_dp_aux_register() once the connector @@ -1790,6 +1790,9 @@ EXPORT_SYMBOL(drm_dp_remote_aux_init); */ void drm_dp_aux_init(struct drm_dp_aux *aux) { + if (aux->ddc.algo) + return; + mutex_init(&aux->hw_mutex); mutex_init(&aux->cec.lock); INIT_WORK(&aux->crc_work, drm_dp_aux_crc_work); @@ -1827,31 +1830,23 @@ EXPORT_SYMBOL(drm_dp_aux_init); * mentioned above need to call drm_dp_aux_init() in order to use the AUX * channel before registration. * + * Don't mix drm_dp_aux_register() with drm_dp_aux_register_chardev/ddc(). + * * Returns 0 on success or a negative error code on failure. */ int drm_dp_aux_register(struct drm_dp_aux *aux) { int ret;
- WARN_ON_ONCE(!aux->drm_dev); - - 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; + drm_dp_aux_init(aux);
- strlcpy(aux->ddc.name, aux->name ? aux->name : dev_name(aux->dev), - sizeof(aux->ddc.name)); - - ret = drm_dp_aux_register_devnode(aux); + ret = drm_dp_aux_register_ddc(aux); if (ret) return ret;
- ret = i2c_add_adapter(&aux->ddc); + ret = drm_dp_aux_register_chardev(aux); if (ret) { - drm_dp_aux_unregister_devnode(aux); + drm_dp_aux_unregister_ddc(aux); return ret; }
@@ -1865,11 +1860,94 @@ EXPORT_SYMBOL(drm_dp_aux_register); */ void drm_dp_aux_unregister(struct drm_dp_aux *aux) { - drm_dp_aux_unregister_devnode(aux); - i2c_del_adapter(&aux->ddc); + drm_dp_aux_unregister_chardev(aux); + drm_dp_aux_unregister_ddc(aux); } EXPORT_SYMBOL(drm_dp_aux_unregister);
+/** + * drm_dp_aux_register_ddc() - initialise and register I2C DDC part of AUX channel + * @aux: DisplayPort AUX channel + * + * Automatically calls drm_dp_aux_init() if this hasn't been done yet. + * If you need to use the drm_dp_aux's I2C adapter prior to registering it with + * the outside world, call drm_dp_aux_register_ddc() first. For drivers which + * are grandparents to their AUX adapters (e.g. the AUX adapter is parented by a + * &drm_connector), you must still call drm_dp_aux_register_chardev() once the + * connector has been registered to allow userspace access to the auxiliary DP + * channel. + * + * For devices which use a separate platform device for their AUX adapters, this + * may be called as early as required by the driver. + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux) +{ + drm_dp_aux_init(aux); + + if (WARN_ON(aux->ddc.class == I2C_CLASS_DDC)) + return -EBUSY; + + 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)); + + return i2c_add_adapter(&aux->ddc); +} +EXPORT_SYMBOL(drm_dp_aux_register_ddc); + +/** + * drm_dp_aux_unregister_ddc() - unregister I2C DDC part of AUX channel + * @aux: DisplayPort AUX channel + */ +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux) +{ + i2c_del_adapter(&aux->ddc); + + aux->ddc.class = 0; +} +EXPORT_SYMBOL(drm_dp_aux_unregister_ddc); + +/** + * drm_dp_aux_register_chardev() - initialise and register userspace part of AUX channel + * @aux: DisplayPort AUX channel + * + * Automatically calls drm_dp_aux_init() if this hasn't been done yet. This + * should only be called once the parent of @aux, &drm_dp_aux.dev, is + * initialized. For devices which are grandparents of their AUX channels, + * &drm_dp_aux.dev will typically be the &drm_connector &device which + * corresponds to @aux. For these devices, it's advised to call + * drm_dp_aux_register_chardev() in &drm_connector_funcs.late_register, and + * likewise to call drm_dp_aux_unregister_chardev() in + * &drm_connector_funcs.early_unregister. Functions which don't follow this + * will likely Oops when %CONFIG_DRM_DP_AUX_CHARDEV is enabled. + * + * Returns 0 on success or a negative error code on failure. + */ +int drm_dp_aux_register_chardev(struct drm_dp_aux *aux) +{ + WARN_ON_ONCE(!aux->drm_dev); + + drm_dp_aux_init(aux); + + return drm_dp_aux_register_devnode(aux); +} +EXPORT_SYMBOL(drm_dp_aux_register_chardev); + +/** + * drm_dp_aux_unregister() - unregister userspace part of AUX channel + * @aux: DisplayPort AUX channel + */ +void drm_dp_aux_unregister_chardev(struct drm_dp_aux *aux) +{ + drm_dp_aux_unregister_devnode(aux); +} +EXPORT_SYMBOL(drm_dp_aux_unregister_chardev); + #define PSR_SETUP_TIME(x) [DP_PSR_SETUP_TIME_ ## x >> DP_PSR_SETUP_TIME_SHIFT] = (x)
/** diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index b52df4db3e8f..80130458188d 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -2130,6 +2130,10 @@ void drm_dp_remote_aux_init(struct drm_dp_aux *aux); void drm_dp_aux_init(struct drm_dp_aux *aux); int drm_dp_aux_register(struct drm_dp_aux *aux); void drm_dp_aux_unregister(struct drm_dp_aux *aux); +int drm_dp_aux_register_ddc(struct drm_dp_aux *aux); +void drm_dp_aux_unregister_ddc(struct drm_dp_aux *aux); +int drm_dp_aux_register_chardev(struct drm_dp_aux *aux); +void drm_dp_aux_unregister_chardev(struct drm_dp_aux *aux);
int drm_dp_start_crc(struct drm_dp_aux *aux, struct drm_crtc *crtc); int drm_dp_stop_crc(struct drm_dp_aux *aux);
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
Cc: stable@vger.kernel.org # 5.13+ Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors") Reported-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Tested-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Signed-off-by: Dmitry Osipenko digetx@gmail.com --- drivers/gpu/drm/tegra/dpaux.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index 1f96e416fa08..e0d675c7c2e5 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -532,7 +532,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_register_ddc(&dpaux->aux); + if (err < 0) + return err;
/* * Assume that by default the DPAUX/I2C pads will be used for HDMI, @@ -585,6 +587,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev);
+ drm_dp_aux_unregister_ddc(&dpaux->aux); + mutex_lock(&dpaux_lock); list_del(&dpaux->list); mutex_unlock(&dpaux_lock); @@ -718,7 +722,7 @@ int drm_dp_aux_attach(struct drm_dp_aux *aux, struct tegra_output *output) int err;
aux->drm_dev = output->connector.dev; - err = drm_dp_aux_register(aux); + err = drm_dp_aux_register_chardev(aux); if (err < 0) return err;
@@ -759,7 +763,7 @@ int drm_dp_aux_detach(struct drm_dp_aux *aux) unsigned long timeout; int err;
- drm_dp_aux_unregister(aux); + drm_dp_aux_unregister_chardev(aux); disable_irq(dpaux->irq);
if (dpaux->output->panel) {
On Mon, Nov 08, 2021 at 02:08:21AM +0300, Dmitry Osipenko wrote:
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
This feels a bit backward, I think the clean solution would be to untangle the SOR loading from the panel driver loading, and then only block registering the overall drm_device on both drivers having loaded.
This here at least feels like a game of whack-a-mole, if like every driver needs its own careful staging of everything. -Daniel
Cc: stable@vger.kernel.org # 5.13+ Fixes: 39c17ae60ea9 ("drm/tegra: Don't register DP AUX channels before connectors") Reported-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Tested-by: Thomas Graichen thomas.graichen@gmail.com # T124 Nyan Big Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/tegra/dpaux.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index 1f96e416fa08..e0d675c7c2e5 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -532,7 +532,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_register_ddc(&dpaux->aux);
if (err < 0)
return err;/*
- Assume that by default the DPAUX/I2C pads will be used for HDMI,
@@ -585,6 +587,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev);
- drm_dp_aux_unregister_ddc(&dpaux->aux);
- mutex_lock(&dpaux_lock); list_del(&dpaux->list); mutex_unlock(&dpaux_lock);
@@ -718,7 +722,7 @@ int drm_dp_aux_attach(struct drm_dp_aux *aux, struct tegra_output *output) int err;
aux->drm_dev = output->connector.dev;
- err = drm_dp_aux_register(aux);
- err = drm_dp_aux_register_chardev(aux); if (err < 0) return err;
@@ -759,7 +763,7 @@ int drm_dp_aux_detach(struct drm_dp_aux *aux) unsigned long timeout; int err;
- drm_dp_aux_unregister(aux);
drm_dp_aux_unregister_chardev(aux); disable_irq(dpaux->irq);
if (dpaux->output->panel) {
-- 2.33.1
08.11.2021 18:17, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 02:08:21AM +0300, Dmitry Osipenko wrote:
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
This feels a bit backward, I think the clean solution would be to untangle the SOR loading from the panel driver loading, and then only block registering the overall drm_device on both drivers having loaded.
Sounds impossible.
1. DRM device can be created only when all components are ready, panel is one of the components.
2. SOR driver is controlling panel and programs h/w based on panel presence.
3. Panel can't become ready until DP AUX DDC is created.
4. DP AUX DDC can't be created until DRM device is created.
5. Go to 1.
Even if there is an option to somehow rewrite Tegra DRM driver to accommodate it to the desired driver model, it won't be something portable to stable kernels.
This here at least feels like a game of whack-a-mole, if like every driver needs its own careful staging of everything.
That is inevitable because each hardware design is individual.
On Mon, Nov 08, 2021 at 09:16:07PM +0300, Dmitry Osipenko wrote:
08.11.2021 18:17, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 02:08:21AM +0300, Dmitry Osipenko wrote:
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
This feels a bit backward, I think the clean solution would be to untangle the SOR loading from the panel driver loading, and then only block registering the overall drm_device on both drivers having loaded.
Sounds impossible.
- DRM device can be created only when all components are ready, panel
is one of the components.
Nope. drm_device can be instantiated whenever you feel like. drm_dev_register can only be called when it's all fully set up. Absolutely nothing would work if drm_device wouldn't support this two-stage setup.
So sequence:
1. drm_dev_init
2. bind sor driver
3. create dp aux ddc
4. bind panel
5. yay we have everything, drm_dev_register
This should work, and it's designed to work like this actually. You couldn't write big complex drivers otherwise. -Daniel
SOR driver is controlling panel and programs h/w based on panel presence.
Panel can't become ready until DP AUX DDC is created.
DP AUX DDC can't be created until DRM device is created.
Go to 1.
Even if there is an option to somehow rewrite Tegra DRM driver to accommodate it to the desired driver model, it won't be something portable to stable kernels.
This here at least feels like a game of whack-a-mole, if like every driver needs its own careful staging of everything.
That is inevitable because each hardware design is individual.
09.11.2021 12:19, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 09:16:07PM +0300, Dmitry Osipenko wrote:
08.11.2021 18:17, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 02:08:21AM +0300, Dmitry Osipenko wrote:
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
This feels a bit backward, I think the clean solution would be to untangle the SOR loading from the panel driver loading, and then only block registering the overall drm_device on both drivers having loaded.
Sounds impossible.
- DRM device can be created only when all components are ready, panel
is one of the components.
Nope. drm_device can be instantiated whenever you feel like. drm_dev_register can only be called when it's all fully set up. Absolutely nothing would work if drm_device wouldn't support this two-stage setup.
So sequence:
drm_dev_init
bind sor driver
create dp aux ddc
bind panel
yay we have everything, drm_dev_register
This should work, and it's designed to work like this actually. You couldn't write big complex drivers otherwise.
All Tegra SoCs have graphics bus named Host1x, that is where components comprising DRM driver are sitting on.
The Tegra driver model works like this:
1. Once Host1x driver is loaded, it populates children sub-nodes of Host1x device-tree node, this creates Tegra DRM platform sub-devices.
2. Tegra DRM driver module-init call registers main "Host1x DRM" driver and platform sub-drivers. Now Host1x bus knows what devices comprise Tegra DRM because Host1x driver descriptor contains that info.
3. Tegra DRM platform sub-drivers are bound to the sub-devices.
4. Once Host1x bus sees that all Tegra DRM sub-drivers are bound, it creates Host1x DRM device.
5. The Host1x DRM device is bound to the Host1x DRM driver and host1x_drm_probe(host1x_dev) is invoked, which does the following:
drm_dev_alloc(driver, host1x_dev) host1x_device_init(host1x_dev) drm_dev_register(drm).
6. The host1x_device_init() invokes second stage initialization of Tegra DRM sub-drivers, that is init() callback of host1x_client_ops registered by DRM platform sub-drivers during theirs probe.
The DP AUX DDC is currently created in step 6, while it should be created in step 3, otherwise SOR driver can't be bound.
It's possible to add early-init stage to the Host1x driver model where DRM device can be created before DRM platform sub-drivers are registered and probed. This is ad-hoc solution, but it should work okay in practice.
I can make v2 if you and Thierry are okay with that solution, see it below.
--- 8< ---
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index 1f96e416fa08..9e17a75a1383 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -530,9 +530,12 @@ static int tegra_dpaux_probe(struct platform_device *pdev) disable_irq(dpaux->irq);
dpaux->aux.transfer = tegra_dpaux_transfer; + dpaux->aux.drm_dev = tegra_drm_device(); dpaux->aux.dev = &pdev->dev;
- drm_dp_aux_init(&dpaux->aux); + err = drm_dp_aux_register(&dpaux->aux); + if (err < 0) + return err;
/* * Assume that by default the DPAUX/I2C pads will be used for HDMI, @@ -585,6 +588,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev);
+ drm_dp_aux_unregister(&dpaux->aux); + mutex_lock(&dpaux_lock); list_del(&dpaux->list); mutex_unlock(&dpaux_lock); @@ -717,11 +722,6 @@ int drm_dp_aux_attach(struct drm_dp_aux *aux, struct tegra_output *output) unsigned long timeout; int err;
- aux->drm_dev = output->connector.dev; - err = drm_dp_aux_register(aux); - if (err < 0) - return err; - output->connector.polled = DRM_CONNECTOR_POLL_HPD; dpaux->output = output;
@@ -759,7 +759,6 @@ int drm_dp_aux_detach(struct drm_dp_aux *aux) unsigned long timeout; int err;
- drm_dp_aux_unregister(aux); disable_irq(dpaux->irq);
if (dpaux->output->panel) { diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index b2ebba802107..d95f9dea6b86 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1124,21 +1124,42 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) return domain != NULL; }
-static int host1x_drm_probe(struct host1x_device *dev) +static struct drm_device *terga_drm_dev; + +struct drm_device *tegra_drm_device(void) { - struct tegra_drm *tegra; - struct drm_device *drm; - int err; + return terga_drm_dev; +}
- drm = drm_dev_alloc(&tegra_drm_driver, &dev->dev); +static int host1x_drm_dev_init(struct host1x_device *dev) +{ + struct drm_device *drm = drm_dev_alloc(&tegra_drm_driver, &dev->dev); if (IS_ERR(drm)) return PTR_ERR(drm);
+ dev_set_drvdata(&dev->dev, drm); + terga_drm_dev = drm; + + return 0; +} + +static void host1x_drm_dev_deinit(struct host1x_device *dev) +{ + struct drm_device *drm = dev_get_drvdata(&dev->dev); + + terga_drm_dev = NULL; + drm_dev_put(drm); +} + +static int host1x_drm_probe(struct host1x_device *dev) +{ + struct drm_device *drm = dev_get_drvdata(&dev->dev); + struct tegra_drm *tegra; + int err; + tegra = kzalloc(sizeof(*tegra), GFP_KERNEL); - if (!tegra) { - err = -ENOMEM; - goto put; - } + if (!tegra) + return -ENOMEM;
if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) { tegra->domain = iommu_domain_alloc(&platform_bus_type); @@ -1155,7 +1176,6 @@ static int host1x_drm_probe(struct host1x_device *dev) mutex_init(&tegra->clients_lock); INIT_LIST_HEAD(&tegra->clients);
- dev_set_drvdata(&dev->dev, drm); drm->dev_private = tegra; tegra->drm = drm;
@@ -1276,8 +1296,7 @@ static int host1x_drm_probe(struct host1x_device *dev) iommu_domain_free(tegra->domain); free: kfree(tegra); -put: - drm_dev_put(drm); + return err; }
@@ -1310,7 +1329,6 @@ static int host1x_drm_remove(struct host1x_device *dev) }
kfree(tegra); - drm_dev_put(drm);
return 0; } @@ -1382,6 +1400,8 @@ static struct host1x_driver host1x_drm_driver = { .probe = host1x_drm_probe, .remove = host1x_drm_remove, .subdevs = host1x_drm_subdevs, + .init = host1x_drm_dev_init, + .deinit = host1x_drm_dev_deinit, };
static struct platform_driver * const drivers[] = { diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index fc0a19554eac..4bfe79b5c7ce 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -64,6 +64,8 @@ struct tegra_drm { struct tegra_display_hub *hub; };
+struct drm_device *tegra_drm_device(void); + static inline struct host1x *tegra_drm_to_host1x(struct tegra_drm *tegra) { return dev_get_drvdata(tegra->drm->dev->parent); diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c index 0d81eede1217..25d688e5c742 100644 --- a/drivers/gpu/host1x/bus.c +++ b/drivers/gpu/host1x/bus.c @@ -479,8 +479,18 @@ static int host1x_device_add(struct host1x *host1x, device->dev.dma_parms = &device->dma_parms; dma_set_max_seg_size(&device->dev, UINT_MAX);
+ if (driver->init) { + err = driver->init(device); + if (err < 0) { + kfree(device); + return err; + } + } + err = host1x_device_parse_dt(device, driver); if (err < 0) { + if (driver->deinit) + driver->deinit(device); kfree(device); return err; } @@ -512,11 +522,16 @@ static int host1x_device_add(struct host1x *host1x, static void host1x_device_del(struct host1x *host1x, struct host1x_device *device) { + struct host1x_driver *driver = device->driver; + if (device->registered) { device->registered = false; device_del(&device->dev); }
+ if (driver->deinit) + driver->deinit(device); + put_device(&device->dev); }
diff --git a/include/linux/host1x.h b/include/linux/host1x.h index e8dc5bc41f79..5e5ba33af4ae 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -346,6 +346,8 @@ struct host1x_device; * @driver: core driver * @subdevs: table of OF device IDs matching subdevices for this driver * @list: list node for the driver + * @init: called when the host1x logical driver is registered + * @deinit: called when the host1x logical driver is unregistered * @probe: called when the host1x logical device is probed * @remove: called when the host1x logical device is removed * @shutdown: called when the host1x logical device is shut down @@ -356,6 +358,8 @@ struct host1x_driver { const struct of_device_id *subdevs; struct list_head list;
+ int (*init)(struct host1x_device *device); + void (*deinit)(struct host1x_device *device); int (*probe)(struct host1x_device *device); int (*remove)(struct host1x_device *device); void (*shutdown)(struct host1x_device *device);
09.11.2021 16:52, Dmitry Osipenko пишет:
09.11.2021 12:19, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 09:16:07PM +0300, Dmitry Osipenko wrote:
08.11.2021 18:17, Daniel Vetter пишет:
On Mon, Nov 08, 2021 at 02:08:21AM +0300, Dmitry Osipenko wrote:
Use drm_dp_aux_register_ddc/chardev() helpers that allow to register I2C adapter separately from the character device. This fixes broken display panel driver of Acer Chromebook CB5-311 that fails to probe starting with v5.13 kernel when DP AUX registration order was changed. Tegra SOR driver is never probed now using the new registration order because tegra-output always fails with -EPROBE_DEFER due to missing display panel that requires DP AUX DDC to be registered first. The offending commit made DDC to be registered after SOR's output, which can't ever happen. Use new helpers to restore the registration order and revive display panel.
This feels a bit backward, I think the clean solution would be to untangle the SOR loading from the panel driver loading, and then only block registering the overall drm_device on both drivers having loaded.
Sounds impossible.
- DRM device can be created only when all components are ready, panel
is one of the components.
Nope. drm_device can be instantiated whenever you feel like. drm_dev_register can only be called when it's all fully set up. Absolutely nothing would work if drm_device wouldn't support this two-stage setup.
So sequence:
drm_dev_init
bind sor driver
create dp aux ddc
bind panel
yay we have everything, drm_dev_register
This should work, and it's designed to work like this actually. You couldn't write big complex drivers otherwise.
All Tegra SoCs have graphics bus named Host1x, that is where components comprising DRM driver are sitting on.
The Tegra driver model works like this:
- Once Host1x driver is loaded, it populates children sub-nodes of
Host1x device-tree node, this creates Tegra DRM platform sub-devices.
- Tegra DRM driver module-init call registers main "Host1x DRM" driver
and platform sub-drivers. Now Host1x bus knows what devices comprise Tegra DRM because Host1x driver descriptor contains that info.
Tegra DRM platform sub-drivers are bound to the sub-devices.
Once Host1x bus sees that all Tegra DRM sub-drivers are bound, it
creates Host1x DRM device.
- The Host1x DRM device is bound to the Host1x DRM driver and
host1x_drm_probe(host1x_dev) is invoked, which does the following:
drm_dev_alloc(driver, host1x_dev) host1x_device_init(host1x_dev) drm_dev_register(drm).
- The host1x_device_init() invokes second stage initialization of Tegra
DRM sub-drivers, that is init() callback of host1x_client_ops registered by DRM platform sub-drivers during theirs probe.
The DP AUX DDC is currently created in step 6, while it should be created in step 3, otherwise SOR driver can't be bound.
It's possible to add early-init stage to the Host1x driver model where DRM device can be created before DRM platform sub-drivers are registered and probed. This is ad-hoc solution, but it should work okay in practice.
I can make v2 if you and Thierry are okay with that solution, see it below.
--- 8< ---
diff --git a/drivers/gpu/drm/tegra/dpaux.c b/drivers/gpu/drm/tegra/dpaux.c index 1f96e416fa08..9e17a75a1383 100644 --- a/drivers/gpu/drm/tegra/dpaux.c +++ b/drivers/gpu/drm/tegra/dpaux.c @@ -530,9 +530,12 @@ static int tegra_dpaux_probe(struct platform_device *pdev) disable_irq(dpaux->irq);
dpaux->aux.transfer = tegra_dpaux_transfer;
- dpaux->aux.drm_dev = tegra_drm_device(); dpaux->aux.dev = &pdev->dev;
- drm_dp_aux_init(&dpaux->aux);
err = drm_dp_aux_register(&dpaux->aux);
if (err < 0)
return err;/*
- Assume that by default the DPAUX/I2C pads will be used for HDMI,
@@ -585,6 +588,8 @@ static int tegra_dpaux_remove(struct platform_device *pdev) pm_runtime_put_sync(&pdev->dev); pm_runtime_disable(&pdev->dev);
- drm_dp_aux_unregister(&dpaux->aux);
- mutex_lock(&dpaux_lock); list_del(&dpaux->list); mutex_unlock(&dpaux_lock);
@@ -717,11 +722,6 @@ int drm_dp_aux_attach(struct drm_dp_aux *aux, struct tegra_output *output) unsigned long timeout; int err;
- aux->drm_dev = output->connector.dev;
- err = drm_dp_aux_register(aux);
- if (err < 0)
return err;- output->connector.polled = DRM_CONNECTOR_POLL_HPD; dpaux->output = output;
@@ -759,7 +759,6 @@ int drm_dp_aux_detach(struct drm_dp_aux *aux) unsigned long timeout; int err;
drm_dp_aux_unregister(aux); disable_irq(dpaux->irq);
if (dpaux->output->panel) {
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index b2ebba802107..d95f9dea6b86 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1124,21 +1124,42 @@ static bool host1x_drm_wants_iommu(struct host1x_device *dev) return domain != NULL; }
-static int host1x_drm_probe(struct host1x_device *dev) +static struct drm_device *terga_drm_dev;
+struct drm_device *tegra_drm_device(void) {
- struct tegra_drm *tegra;
- struct drm_device *drm;
- int err;
- return terga_drm_dev;
+}
- drm = drm_dev_alloc(&tegra_drm_driver, &dev->dev);
+static int host1x_drm_dev_init(struct host1x_device *dev) +{
struct drm_device *drm = drm_dev_alloc(&tegra_drm_driver, &dev->dev); if (IS_ERR(drm)) return PTR_ERR(drm);
dev_set_drvdata(&dev->dev, drm);
terga_drm_dev = drm;
Although, platform_register_drivers() should be moved here out from host1x_drm_init(), otherwise DRM drivers may get probed before main host1x driver is registered and then terga_drm_dev will be NULL. But you should get the idea anyways.
- return 0;
+}
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
- terga_drm_dev = NULL;
- drm_dev_put(drm);
+}
+static int host1x_drm_probe(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
- struct tegra_drm *tegra;
- int err;
- tegra = kzalloc(sizeof(*tegra), GFP_KERNEL);
- if (!tegra) {
err = -ENOMEM;goto put;- }
if (!tegra)
return -ENOMEM;if (host1x_drm_wants_iommu(dev) && iommu_present(&platform_bus_type)) { tegra->domain = iommu_domain_alloc(&platform_bus_type);
@@ -1155,7 +1176,6 @@ static int host1x_drm_probe(struct host1x_device *dev) mutex_init(&tegra->clients_lock); INIT_LIST_HEAD(&tegra->clients);
- dev_set_drvdata(&dev->dev, drm); drm->dev_private = tegra; tegra->drm = drm;
@@ -1276,8 +1296,7 @@ static int host1x_drm_probe(struct host1x_device *dev) iommu_domain_free(tegra->domain); free: kfree(tegra); -put:
- drm_dev_put(drm);
- return err;
}
@@ -1310,7 +1329,6 @@ static int host1x_drm_remove(struct host1x_device *dev) }
kfree(tegra);
drm_dev_put(drm);
return 0;
} @@ -1382,6 +1400,8 @@ static struct host1x_driver host1x_drm_driver = { .probe = host1x_drm_probe, .remove = host1x_drm_remove, .subdevs = host1x_drm_subdevs,
- .init = host1x_drm_dev_init,
- .deinit = host1x_drm_dev_deinit,
};
static struct platform_driver * const drivers[] = { diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index fc0a19554eac..4bfe79b5c7ce 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -64,6 +64,8 @@ struct tegra_drm { struct tegra_display_hub *hub; };
+struct drm_device *tegra_drm_device(void);
static inline struct host1x *tegra_drm_to_host1x(struct tegra_drm *tegra) { return dev_get_drvdata(tegra->drm->dev->parent); diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c index 0d81eede1217..25d688e5c742 100644 --- a/drivers/gpu/host1x/bus.c +++ b/drivers/gpu/host1x/bus.c @@ -479,8 +479,18 @@ static int host1x_device_add(struct host1x *host1x, device->dev.dma_parms = &device->dma_parms; dma_set_max_seg_size(&device->dev, UINT_MAX);
- if (driver->init) {
err = driver->init(device);if (err < 0) {kfree(device);return err;}- }
- err = host1x_device_parse_dt(device, driver); if (err < 0) {
if (driver->deinit)driver->deinit(device);@@ -512,11 +522,16 @@ static int host1x_device_add(struct host1x *host1x, static void host1x_device_del(struct host1x *host1x, struct host1x_device *device) {
struct host1x_driver *driver = device->driver;
if (device->registered) { device->registered = false; device_del(&device->dev); }
if (driver->deinit)
driver->deinit(device);put_device(&device->dev);
}
diff --git a/include/linux/host1x.h b/include/linux/host1x.h index e8dc5bc41f79..5e5ba33af4ae 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -346,6 +346,8 @@ struct host1x_device;
- @driver: core driver
- @subdevs: table of OF device IDs matching subdevices for this driver
- @list: list node for the driver
- @init: called when the host1x logical driver is registered
- @deinit: called when the host1x logical driver is unregistered
- @probe: called when the host1x logical device is probed
- @remove: called when the host1x logical device is removed
- @shutdown: called when the host1x logical device is shut down
@@ -356,6 +358,8 @@ struct host1x_driver { const struct of_device_id *subdevs; struct list_head list;
- int (*init)(struct host1x_device *device);
- void (*deinit)(struct host1x_device *device); int (*probe)(struct host1x_device *device); int (*remove)(struct host1x_device *device); void (*shutdown)(struct host1x_device *device);
09.11.2021 17:08, Dmitry Osipenko пишет:
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
Nah, that should cause deadlock. This ad-hoc is too lame.
Another solution is to defer probing of DP AUX driver while tegra_drm_device() returns NULL, but it's icky.
Reverting the original DP AUX DDC registration order is the best option so far, IMO.
09.11.2021 17:17, Dmitry Osipenko пишет:
09.11.2021 17:08, Dmitry Osipenko пишет:
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
Nah, that should cause deadlock. This ad-hoc is too lame.
Actually, there is no problem here as I see now. The host1x driver populates DT nodes after host1x_register() [1], meaning that Host1x DRM will be always inited first.
[1] https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
Still I'm not a fan of the ad-hoc solution.
Another solution is to defer probing of DP AUX driver while tegra_drm_device() returns NULL, but it's icky.
Reverting the original DP AUX DDC registration order is the best option so far, IMO.
On Tue, Nov 09, 2021 at 05:39:02PM +0300, Dmitry Osipenko wrote:
09.11.2021 17:17, Dmitry Osipenko пишет:
09.11.2021 17:08, Dmitry Osipenko пишет:
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
Nah, that should cause deadlock. This ad-hoc is too lame.
Actually, there is no problem here as I see now. The host1x driver populates DT nodes after host1x_register() [1], meaning that Host1x DRM will be always inited first.
[1] https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
Still I'm not a fan of the ad-hoc solution.
Could we not fix this by making the panel "hot-pluggable"? I don't think there's anything inherent to the driver that would prevent doing so. The original reason for why things are as intertwined as they are now is because back at the time deferred framebuffer creation didn't exist. In fact I added deferred framebuffer support with Daniel's help precisely to fix a similar issue for things like HDMI and DP.
With HDMI and DP it's slightly less critical, because a lack of mode support would've created a 1024x768 framebuffer, which most HDMI/DP monitors support. However, with panels we need to ensure that the exact mode from the panel is used to create the framebuffer, so it can only be created when the panel is available.
But, given that deferred framebuffer creation works now (which allows the framebuffer console to show up at the preferred resolution for HDMI and DP), even if a monitor is attached only after the driver has probed already, we should be able to make something like that work with panels as well.
Thierry
Another solution is to defer probing of DP AUX driver while tegra_drm_device() returns NULL, but it's icky.
Reverting the original DP AUX DDC registration order is the best option so far, IMO.
12.11.2021 13:52, Thierry Reding пишет:
On Tue, Nov 09, 2021 at 05:39:02PM +0300, Dmitry Osipenko wrote:
09.11.2021 17:17, Dmitry Osipenko пишет:
09.11.2021 17:08, Dmitry Osipenko пишет:
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{
- struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
Nah, that should cause deadlock. This ad-hoc is too lame.
Actually, there is no problem here as I see now. The host1x driver populates DT nodes after host1x_register() [1], meaning that Host1x DRM will be always inited first.
[1] https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
Still I'm not a fan of the ad-hoc solution.
Could we not fix this by making the panel "hot-pluggable"? I don't think there's anything inherent to the driver that would prevent doing so. The original reason for why things are as intertwined as they are now is because back at the time deferred framebuffer creation didn't exist. In fact I added deferred framebuffer support with Daniel's help precisely to fix a similar issue for things like HDMI and DP.
I don't understand what do you mean by "hot-pluggable", panel is static. Please either clarify more or type the patch.
Keep in mind that fix should be simple and portable because stable kernels are wrecked.
With HDMI and DP it's slightly less critical, because a lack of mode support would've created a 1024x768 framebuffer, which most HDMI/DP monitors support. However, with panels we need to ensure that the exact mode from the panel is used to create the framebuffer, so it can only be created when the panel is available.
But, given that deferred framebuffer creation works now (which allows the framebuffer console to show up at the preferred resolution for HDMI and DP), even if a monitor is attached only after the driver has probed already, we should be able to make something like that work with panels as well.
BTW, I see now that DPAUX I2C transfer helper may access aux->drm_device. Hence v1 patch isn't correct anyways.
For now I'll try to test more the ad-hoc variant with Thomas and send it as v2 if we won't have a better solution.
On Fri, 2021-11-12 at 17:32 +0300, Dmitry Osipenko wrote:
12.11.2021 13:52, Thierry Reding пишет:
On Tue, Nov 09, 2021 at 05:39:02PM +0300, Dmitry Osipenko wrote:
09.11.2021 17:17, Dmitry Osipenko пишет:
09.11.2021 17:08, Dmitry Osipenko пишет:
+static void host1x_drm_dev_deinit(struct host1x_device *dev) +{ + struct drm_device *drm = dev_get_drvdata(&dev->dev);
And platform_unregister_drivers() should be moved here.
Nah, that should cause deadlock. This ad-hoc is too lame.
Actually, there is no problem here as I see now. The host1x driver populates DT nodes after host1x_register() [1], meaning that Host1x DRM will be always inited first.
[1] https://elixir.bootlin.com/linux/v5.15/source/drivers/gpu/host1x/dev.c#L475
Still I'm not a fan of the ad-hoc solution.
Could we not fix this by making the panel "hot-pluggable"? I don't think there's anything inherent to the driver that would prevent doing so. The original reason for why things are as intertwined as they are now is because back at the time deferred framebuffer creation didn't exist. In fact I added deferred framebuffer support with Daniel's help precisely to fix a similar issue for things like HDMI and DP.
I don't understand what do you mean by "hot-pluggable", panel is static. Please either clarify more or type the patch.
Keep in mind that fix should be simple and portable because stable kernels are wrecked.
With HDMI and DP it's slightly less critical, because a lack of mode support would've created a 1024x768 framebuffer, which most HDMI/DP monitors support. However, with panels we need to ensure that the exact mode from the panel is used to create the framebuffer, so it can only be created when the panel is available.
But, given that deferred framebuffer creation works now (which allows the framebuffer console to show up at the preferred resolution for HDMI and DP), even if a monitor is attached only after the driver has probed already, we should be able to make something like that work with panels as well.
BTW, I see now that DPAUX I2C transfer helper may access aux->drm_device. Hence v1 patch isn't correct anyways.
JFYI - unless I'm misunderstanding you, the aux->drm_dev accesses in the DPAUX i2c transfer functions are just from the various drm_{dbg,err,etc.} calls, which means that they all should be able to handle aux->drm_dev being NULL. If you can set aux->drm_dev before i2c transfers start that's more ideal, since otherwise you'll see the AUX device name as "(null)" in the kernel log, but any point before device registration should work.
For now I'll try to test more the ad-hoc variant with Thomas and send it as v2 if we won't have a better solution.
12.11.2021 23:26, Lyude Paul пишет:
BTW, I see now that DPAUX I2C transfer helper may access aux->drm_device. Hence v1 patch isn't correct anyways.
JFYI - unless I'm misunderstanding you, the aux->drm_dev accesses in the DPAUX i2c transfer functions are just from the various drm_{dbg,err,etc.} calls, which means that they all should be able to handle aux->drm_dev being NULL. If you can set aux->drm_dev before i2c transfers start that's more ideal, since otherwise you'll see the AUX device name as "(null)" in the kernel log, but any point before device registration should work.
Thanks, I realized that have seen DRM log with a such debug messages just a day ago.
drm drm: [drm:drm_dp_i2c_do_msg] (null): transaction timed out
So yes, it's indeed not critical.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dmitry Osipenko -
Lyude Paul -
Thierry Reding