[PATCH v1 2/2] drm/tegra: Use drm_dp_aux_register_ddc/chardev() helpers
Dmitry Osipenko
digetx at gmail.com
Tue Nov 9 14:08:04 UTC 2021
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);
>
More information about the dri-devel
mailing list