Re: [PATCH v1 2/2] drm/tegra: Use drm_dp_aux_register_ddc/chardev() helpers

[Dmitry Osipenko <digetx@gmail.com>]

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);
>