| Message ID | 20211107230821.13511-2-digetx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/2] drm/dp: Add drm_dp_aux_register_ddc/chardev() helpers | expand |
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. > > 1. 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 > > 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.
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. >> >> 1. 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. 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. >>> >>> 1. 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. > > 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; 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); > 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 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.
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) {
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(-)