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

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