Skip to contentSkip to conversation

drm/tilcdc: Cleanup tilcdc init sequence

PATCH In review Jyri Sarha 3 participants 10 messages Nov 2, 2016 – Nov 21, 2016

Subsystems (3)
DRM drivers DRM misc DRM drivers for TI lcdc
lore
Share & tools
lore

Jyri Sarha

author
#0 lore raw

v3 cover drm/tilcdc: Cleanup tilcdc init sequence

Since v2 version of this series:
- Rebased on top of latest drm-next
- Add "drm/tilcdc: Fix race from forced shutdown of crtc in unload"

Since first version of this series:
- Dropped "drm/i2c: tda998x: Remove obsolete drm_connector_register() call"
- an earlier instance of the same patch already exists
- Got rid of unload callback and deprecated drm_put_dev() calls

This series depends on tda998x dropping the drm_connector_register(). Please keep me in loop so that I know when it gets merged so that I know when it is safe send a pull request for these.

A patch[1] that crashed all drm drivers using load() hook was briefly part of linux-next, so became aware that tilcdc should stop using that too.

The two first patches should be merged before the third can be merged. So the merge order should be agreed with tda998x before going forward.

The last patch is just a cleanup, but depends on earlier ones.

Best regards,
Jyri

[1] https://patchwork.kernel.org/patch/9322771/

Jyri Sarha (4):
drm/tilcdc: Remove obsolete drm_connector_register() calls drm/tilcdc: Stop using struct drm_driver load() callback drm/tilcdc: Use unload to handle initialization failures drm/tilcdc: Fix race from forced shutdown of crtc in unload

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c   |  35 +++++--
 drivers/gpu/drm/tilcdc/tilcdc_drv.c    | 185 +++++++++++++++------------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h    |   4 +-
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  |   2 -
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |   2 -
 5 files changed, 115 insertions(+), 113 deletions(-)
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw

PATCH v3 1/4 drm/tilcdc: Remove obsolete drm_connector_register() calls

Remove obsolete drm_connector_register() calls from tilcdc_panel.c and tilcdc_tfp410.c. All connectors are registered when drm_dev_register() is called.

Signed-off-by Jyri Sarha <[email protected]>
Reviewed-by Laurent Pinchart <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 2 --
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 2 --
 2 files changed, 4 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_panel.c +0 -2
index 2134bb20..ad7a0e8 100644
@@ -240,8 +240,6 @@ static struct drm_connector *panel_connector_create(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
-	drm_connector_register(connector);
-
 	return connector;
 
 fail:
drivers/gpu/drm/tilcdc/tilcdc_tfp410.c +0 -2
index c32c1ef..eea82b4 100644
@@ -251,8 +251,6 @@ static struct drm_connector *tfp410_connector_create(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
-	drm_connector_register(connector);
-
 	return connector;
 
 fail:
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#2 lore raw

PATCH v3 2/4 drm/tilcdc: Stop using struct drm_driver load() callback

In reply to Jyri Sarha

Stop using struct drm_driver load() and unload() callbacks. The callbacks should not be used anymore. Instead of using load the drm_device is allocated with drm_dev_alloc() and registered with drm_dev_register() only after the driver is completely initialized. The deinitialization is done directly either in component unbind callback or in platform driver demove callback.

Signed-off-by Jyri Sarha <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 124 ++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 54 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_drv.c +70 -54
index 5cf534c..11f3262 100644
@@ -194,18 +194,22 @@ static int cpufreq_transition(struct notifier_block *nb,
  * DRM operations:
  */
 
-static int tilcdc_unload(struct drm_device *dev)
+static void tilcdc_fini(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	tilcdc_remove_external_encoders(dev);
+	drm_modeset_lock_crtc(priv->crtc, NULL);
+	tilcdc_crtc_disable(priv->crtc);
+	drm_modeset_unlock_crtc(priv->crtc);
+
+	drm_dev_unregister(dev);
 
-	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
+	drm_fbdev_cma_fini(priv->fbdev);
+	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
 
-	drm_irq_uninstall(dev);
+	tilcdc_remove_external_encoders(dev);
 
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,

@@ -225,28 +229,34 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	return 0;
+	drm_dev_unref(dev);
 }
 
-static int tilcdc_load(struct drm_device *dev, unsigned long flags)
+static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 {
-	struct platform_device *pdev = dev->platformdev;
-	struct device_node *node = pdev->dev.of_node;
+	struct drm_device *ddev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node = dev->of_node;
 	struct tilcdc_drm_private *priv;
 	struct resource *res;
 	u32 bpp = 0;
 	int ret;
 
-	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		dev_err(dev, "failed to allocate private data\n");
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev = drm_dev_alloc(ddrv, dev);
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	ddev->platformdev = pdev;
+	ddev->dev_private = priv;
 
 	priv->is_componentized =
-		tilcdc_get_external_components(dev->dev, NULL) > 0;
+		tilcdc_get_external_components(dev, NULL) > 0;
 
 	priv->wq = alloc_ordered_workqueue("tilcdc", 0);
 	if (!priv->wq) {

@@ -256,21 +266,21 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-		dev_err(dev->dev, "failed to get memory resource\n");
+		dev_err(dev, "failed to get memory resource\n");
 		ret = -EINVAL;
 		goto fail_free_wq;
 	}
 
 	priv->mmio = ioremap_nocache(res->start, resource_size(res));
 	if (!priv->mmio) {
-		dev_err(dev->dev, "failed to ioremap\n");
+		dev_err(dev, "failed to ioremap\n");
 		ret = -ENOMEM;
 		goto fail_free_wq;
 	}
 
-	priv->clk = clk_get(dev->dev, "fck");
+	priv->clk = clk_get(dev, "fck");
 	if (IS_ERR(priv->clk)) {
-		dev_err(dev->dev, "failed to get functional clock\n");
+		dev_err(dev, "failed to get functional clock\n");
 		ret = -ENODEV;
 		goto fail_iounmap;
 	}

@@ -280,7 +290,7 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	ret = cpufreq_register_notifier(&priv->freq_transition,
 			CPUFREQ_TRANSITION_NOTIFIER);
 	if (ret) {
-		dev_err(dev->dev, "failed to register cpufreq notifier\n");
+		dev_err(dev, "failed to register cpufreq notifier\n");
 		goto fail_put_clk;
 	}
 #endif

@@ -301,11 +311,11 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 
 	DBG("Maximum Pixel Clock Value %dKHz", priv->max_pixelclock);
 
-	pm_runtime_enable(dev->dev);
+	pm_runtime_enable(dev);
 
 	/* Determine LCD IP Version */
-	pm_runtime_get_sync(dev->dev);
-	switch (tilcdc_read(dev, LCDC_PID_REG)) {
+	pm_runtime_get_sync(dev);
+	switch (tilcdc_read(ddev, LCDC_PID_REG)) {
 	case 0x4c100102:
 		priv->rev = 1;
 		break;

@@ -314,14 +324,14 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		priv->rev = 2;
 		break;
 	default:
-		dev_warn(dev->dev, "Unknown PID Reg value 0x%08x, "
-				"defaulting to LCD revision 1\n",
-				tilcdc_read(dev, LCDC_PID_REG));
+		dev_warn(dev, "Unknown PID Reg value 0x%08x, "
+			"defaulting to LCD revision 1\n",
+			tilcdc_read(ddev, LCDC_PID_REG));
 		priv->rev = 1;
 		break;
 	}
 
-	pm_runtime_put_sync(dev->dev);
+	pm_runtime_put_sync(dev);
 
 	if (priv->rev == 1) {
 		DBG("Revision 1 LCDC supports only RGB565 format");

@@ -354,74 +364,82 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 		}
 	}
 
-	ret = modeset_init(dev);
+	ret = modeset_init(ddev);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize mode setting\n");
+		dev_err(dev, "failed to initialize mode setting\n");
 		goto fail_cpufreq_unregister;
 	}
 
-	platform_set_drvdata(pdev, dev);
+	platform_set_drvdata(pdev, ddev);
 
 	if (priv->is_componentized) {
-		ret = component_bind_all(dev->dev, dev);
+		ret = component_bind_all(dev, ddev);
 		if (ret < 0)
 			goto fail_mode_config_cleanup;
 
-		ret = tilcdc_add_external_encoders(dev);
+		ret = tilcdc_add_external_encoders(ddev);
 		if (ret < 0)
 			goto fail_component_cleanup;
 	}
 
 	if ((priv->num_encoders == 0) || (priv->num_connectors == 0)) {
-		dev_err(dev->dev, "no encoders/connectors found\n");
+		dev_err(dev, "no encoders/connectors found\n");
 		ret = -ENXIO;
 		goto fail_external_cleanup;
 	}
 
-	ret = drm_vblank_init(dev, 1);
+	ret = drm_vblank_init(ddev, 1);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to initialize vblank\n");
+		dev_err(dev, "failed to initialize vblank\n");
 		goto fail_external_cleanup;
 	}
 
-	ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0));
+	ret = drm_irq_install(ddev, platform_get_irq(pdev, 0));
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to install IRQ handler\n");
+		dev_err(dev, "failed to install IRQ handler\n");
 		goto fail_vblank_cleanup;
 	}
 
-	drm_mode_config_reset(dev);
+	drm_mode_config_reset(ddev);
 
-	priv->fbdev = drm_fbdev_cma_init(dev, bpp,
-			dev->mode_config.num_crtc,
-			dev->mode_config.num_connector);
+	priv->fbdev = drm_fbdev_cma_init(ddev, bpp,
+			ddev->mode_config.num_crtc,
+			ddev->mode_config.num_connector);
 	if (IS_ERR(priv->fbdev)) {
 		ret = PTR_ERR(priv->fbdev);
 		goto fail_irq_uninstall;
 	}
 
-	drm_kms_helper_poll_init(dev);
+	drm_kms_helper_poll_init(ddev);
+
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto fail_platform_init;
 
 	return 0;
 
+fail_platform_init:
+	drm_kms_helper_poll_fini(ddev);
+	drm_fbdev_cma_fini(priv->fbdev);
+
 fail_irq_uninstall:
-	drm_irq_uninstall(dev);
+	drm_irq_uninstall(ddev);
 
 fail_vblank_cleanup:
-	drm_vblank_cleanup(dev);
+	drm_vblank_cleanup(ddev);
 
 fail_component_cleanup:
 	if (priv->is_componentized)
-		component_unbind_all(dev->dev, dev);
+		component_unbind_all(dev, dev);
 
 fail_mode_config_cleanup:
-	drm_mode_config_cleanup(dev);
+	drm_mode_config_cleanup(ddev);
 
 fail_external_cleanup:
-	tilcdc_remove_external_encoders(dev);
+	tilcdc_remove_external_encoders(ddev);
 
 fail_cpufreq_unregister:
-	pm_runtime_disable(dev->dev);
+	pm_runtime_disable(dev);
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,
 			CPUFREQ_TRANSITION_NOTIFIER);

@@ -438,7 +456,8 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	destroy_workqueue(priv->wq);
 
 fail_unset_priv:
-	dev->dev_private = NULL;
+	ddev->dev_private = NULL;
+	drm_dev_unref(ddev);
 
 	return ret;
 }

@@ -585,8 +604,6 @@ static void tilcdc_debugfs_cleanup(struct drm_minor *minor)
 static struct drm_driver tilcdc_driver = {
 	.driver_features    = (DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET |
 			       DRIVER_PRIME | DRIVER_ATOMIC),
-	.load               = tilcdc_load,
-	.unload             = tilcdc_unload,
 	.lastclose          = tilcdc_lastclose,
 	.irq_handler        = tilcdc_irq,
 	.get_vblank_counter = drm_vblank_no_hw_counter,

@@ -660,10 +677,9 @@ static int tilcdc_pm_resume(struct device *dev)
 /*
  * Platform driver:
  */
-
 static int tilcdc_bind(struct device *dev)
 {
-	return drm_platform_init(&tilcdc_driver, to_platform_device(dev));
+	return tilcdc_init(&tilcdc_driver, dev);
 }
 
 static void tilcdc_unbind(struct device *dev)

@@ -674,7 +690,7 @@ static void tilcdc_unbind(struct device *dev)
 	if (!ddev->dev_private)
 		return;
 
-	drm_put_dev(dev_get_drvdata(dev));
+	tilcdc_fini(dev_get_drvdata(dev));
 }
 
 static const struct component_master_ops tilcdc_comp_ops = {

@@ -697,7 +713,7 @@ static int tilcdc_pdev_probe(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 	else if (ret == 0)
-		return drm_platform_init(&tilcdc_driver, pdev);
+		return tilcdc_init(&tilcdc_driver, &pdev->dev);
 	else
 		return component_master_add_with_match(&pdev->dev,
 						       &tilcdc_comp_ops,

@@ -712,7 +728,7 @@ static int tilcdc_pdev_remove(struct platform_device *pdev)
 	if (ret < 0)
 		return ret;
 	else if (ret == 0)
-		drm_put_dev(platform_get_drvdata(pdev));
+		tilcdc_fini(platform_get_drvdata(pdev));
 	else
 		component_master_del(&pdev->dev, &tilcdc_comp_ops);
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Reviewed-by on v3 2/4
#3 lore raw

Re: [PATCHv3 2/4] drm/tilcdc: Stop using struct drm_driver load() callback

Hi Jyri,

Thank you for the patch.

Quoted 25 lines · On Wednesday 02 Nov 2016 17:57:50 Jyri Sarha wrote:

Stop using struct drm_driver load() and unload() callbacks. The callbacks should not be used anymore. Instead of using load the drm_device is allocated with drm_dev_alloc() and registered with drm_dev_register() only after the driver is completely initialized. The deinitialization is done directly either in component unbind callback or in platform driver demove callback.

Signed-off-by Jyri Sarha <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 124 ++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 54 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 5cf534c..11f3262 100644

drivers/gpu/drm/tilcdc/tilcdc_drv.c +1 -1
@@ -194,18 +194,22 @@ static int cpufreq_transition(struct notifier_block
 *nb, * DRM operations:
  */
 
-static int tilcdc_unload(struct drm_device *dev)
+static void tilcdc_fini(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int cpufreq_transition(struct notifier_block · line 208

Quoted 10 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +6 -2
-	tilcdc_remove_external_encoders(dev);
+	drm_modeset_lock_crtc(priv->crtc, NULL);
+	tilcdc_crtc_disable(priv->crtc);
+	drm_modeset_unlock_crtc(priv->crtc);
+
+	drm_dev_unregister(dev);
 
-	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
+	drm_fbdev_cma_fini(priv->fbdev);

Is there a need to reorder these ?

Quoted 30 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +7 -6
+	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
 
-	drm_irq_uninstall(dev);
+	tilcdc_remove_external_encoders(dev);
 
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,

@@ -225,28 +229,34 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	return 0;
+	drm_dev_unref(dev);
 }
 
-static int tilcdc_load(struct drm_device *dev, unsigned long flags)
+static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 {
-	struct platform_device *pdev = dev->platformdev;
-	struct device_node *node = pdev->dev.of_node;
+	struct drm_device *ddev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node = dev->of_node;
 	struct tilcdc_drm_private *priv;
 	struct resource *res;
 	u32 bpp = 0;
 	int ret;

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int tilcdc_unload(struct drm_device *dev) · line 251

Quoted 10 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -3
-	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		dev_err(dev, "failed to allocate private data\n");
 		return -ENOMEM;
 	}
 
-	dev->dev_private = priv;
+	ddev = drm_dev_alloc(ddrv, dev);

As a follow-up patch you might want to move tilcdc_driver above this function and use it directly to remove the ddrv argument.

Apart from that,

Reviewed-by Laurent Pinchart <[email protected]>
Quoted 5 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +5 -0
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	ddev->platformdev = pdev;
+	ddev->dev_private = priv;

[snip]

Signature · 8 lines
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#4 lore raw

Re: [PATCHv3 2/4] drm/tilcdc: Stop using struct drm_driver load() callback

Quoted 37 lines · Laurent Pinchart

Hi Jyri,

Thank you for the patch.

Quoted 32 lines · On Wednesday 02 Nov 2016 17:57:50 Jyri Sarha wrote:

Stop using struct drm_driver load() and unload() callbacks. The callbacks should not be used anymore. Instead of using load the drm_device is allocated with drm_dev_alloc() and registered with drm_dev_register() only after the driver is completely initialized. The deinitialization is done directly either in component unbind callback or in platform driver demove callback.

Signed-off-by Jyri Sarha <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 124 ++++++++++++++++++--------------
 1 file changed, 70 insertions(+), 54 deletions(-)
(quoted diff) +0 -0

b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 5cf534c..11f3262 100644

drivers/gpu/drm/tilcdc/tilcdc_drv.c +6 -2
@@ -194,18 +194,22 @@ static int cpufreq_transition(struct notifier_block
 *nb, * DRM operations:
  */
 
-static int tilcdc_unload(struct drm_device *dev)
+static void tilcdc_fini(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	tilcdc_remove_external_encoders(dev);
+	drm_modeset_lock_crtc(priv->crtc, NULL);
+	tilcdc_crtc_disable(priv->crtc);
+	drm_modeset_unlock_crtc(priv->crtc);
+
+	drm_dev_unregister(dev);
Quoted 5 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int cpufreq_transition(struct notifier_block *nb, · line 208

Quoted 3 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +1 -1
-	drm_fbdev_cma_fini(priv->fbdev);
 	drm_kms_helper_poll_fini(dev);
+	drm_fbdev_cma_fini(priv->fbdev);

Is there a need to reorder these ?

I am not sure actually. When the things did not work properly in the beginning I just ordered unloading to happen strictly in the reverse order compared to loading. I did not go back to check what changes were actually needed when I got things working.

Quoted 39 lines
Quoted 39 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +9 -8
+	drm_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
-	drm_vblank_cleanup(dev);
 
-	drm_irq_uninstall(dev);
+	tilcdc_remove_external_encoders(dev);
 
 #ifdef CONFIG_CPU_FREQ
 	cpufreq_unregister_notifier(&priv->freq_transition,

@@ -225,28 +229,34 @@ static int tilcdc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 
-	return 0;
+	drm_dev_unref(dev);
 }
 
-static int tilcdc_load(struct drm_device *dev, unsigned long flags)
+static int tilcdc_init(struct drm_driver *ddrv, struct device *dev)
 {
-	struct platform_device *pdev = dev->platformdev;
-	struct device_node *node = pdev->dev.of_node;
+	struct drm_device *ddev;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct device_node *node = dev->of_node;
 	struct tilcdc_drm_private *priv;
 	struct resource *res;
 	u32 bpp = 0;
 	int ret;
 
-	priv = devm_kzalloc(dev->dev, sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
-		dev_err(dev->dev, "failed to allocate private data\n");
+		dev_err(dev, "failed to allocate private data\n");
 		return -ENOMEM;
 	}
Quoted 6 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int tilcdc_unload(struct drm_device *dev) · line 251

Quoted 2 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +1 -1
-	dev->dev_private = priv;
+	ddev = drm_dev_alloc(ddrv, dev);

As a follow-up patch you might want to move tilcdc_driver above this function and use it directly to remove the ddrv argument.

Ok, thanks.

Quoted 4 lines

Apart from that,

Reviewed-by Laurent Pinchart <[email protected]>
Quoted 8 lines
Quoted 5 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +5 -0
+	if (IS_ERR(ddev))
+		return PTR_ERR(ddev);
+
+	ddev->platformdev = pdev;
+	ddev->dev_private = priv;

[snip]

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#5 lore raw

PATCH v3 3/4 drm/tilcdc: Use unload to handle initialization failures

In reply to Jyri Sarha

Use unload to handle initialization failures instead of complex goto label mess. To do this the initialization sequence needed slight reordering and some unload functions needed to become conditional.

Signed-off-by Jyri Sarha <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c |  10 ++--
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 101 ++++++++++++-----------------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |   3 +-
 3 files changed, 43 insertions(+), 71 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +7 -3
index ea79e09..6277363 100644
@@ -177,14 +177,12 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	tilcdc_crtc->enabled = true;
 }
 
-void tilcdc_crtc_disable(struct drm_crtc *crtc)
+void tilcdc_crtc_off(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-
 	if (!tilcdc_crtc->enabled)
 		return;
 

@@ -228,6 +226,12 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc)
 	tilcdc_crtc->enabled = false;
 }
 
+static void tilcdc_crtc_disable(struct drm_crtc *crtc)
+{
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+	tilcdc_crtc_off(crtc);
+}
+
 static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
 {
 	return crtc->state && crtc->state->enable && crtc->state->active;
drivers/gpu/drm/tilcdc/tilcdc_drv.c +34 -67

207 lines not on this page · raw message

drivers/gpu/drm/tilcdc/tilcdc_drv.h +2 -1
index 9780c37..7db23f2 100644
@@ -89,6 +89,7 @@ struct tilcdc_drm_private {
 	struct drm_connector *connectors[8];
 	const struct drm_connector_helper_funcs *connector_funcs[8];
 
+	bool is_registered;
 	bool is_componentized;
 };
 

@@ -172,7 +173,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
 					bool simulate_vesa_sync);
 int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
-void tilcdc_crtc_disable(struct drm_crtc *crtc);
+void tilcdc_crtc_off(struct drm_crtc *crtc);
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event);
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#6 lore raw

Re: [PATCHv3 3/4] drm/tilcdc: Use unload to handle initialization failures

2016-11-02 16:57 GMT+01:00 Jyri Sarha <[email protected]>:

Quoted 6 lines

Use unload to handle initialization failures instead of complex goto label mess. To do this the initialization sequence needed slight reordering and some unload functions needed to become conditional.

Signed-off-by Jyri Sarha <[email protected]>

---

I'm not sure yet of the exact error path, but with this patch tilcdc_crtc_destroy() fails with a NULL-pointer dereference at dmam_free_coherent() due to crtc->dev being NULL if there are no panels registered.

Thanks,
Bartosz Golaszewski
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#7 lore raw

Re: [PATCHv3 3/4] drm/tilcdc: Use unload to handle initialization failures

Quoted 5 lines · Bartosz Golaszewski

2016-11-02 16:57 GMT+01:00 Jyri Sarha <[email protected]>:

Quoted 4 lines

Use unload to handle initialization failures instead of complex goto label mess. To do this the initialization sequence needed slight reordering and some unload functions needed to become conditional.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Jyri Sarha <[email protected]>

---

I'm not sure yet of the exact error path, but with this patch tilcdc_crtc_destroy() fails with a NULL-pointer dereference at dmam_free_coherent() due to crtc->dev being NULL if there are no panels registered.

Argh, should have read the dmam_alloc_coherent() function documentation. I just wondered what the extra m in function prefix was for and did not realize that it was a devres version of the function (I would have expected such a function to be called devm_dma_alloc_coherent()).

Anyway, I'll drop the "drm/tilcdc: Free palette dma memory in tilcdc_crtc_destroy()" patch.

Thanks,
Jyri

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#8 lore raw

Re: [PATCHv3 3/4] drm/tilcdc: Use unload to handle initialization failures

2016-11-21 11:24 GMT+01:00 Jyri Sarha <[email protected]>:

Quoted 15 lines · Bartosz Golaszewski
Quoted 5 lines

2016-11-02 16:57 GMT+01:00 Jyri Sarha <[email protected]>:

Quoted 4 lines

Use unload to handle initialization failures instead of complex goto label mess. To do this the initialization sequence needed slight reordering and some unload functions needed to become conditional.

Quoted 8 lines
Quoted 2 lines
Signed-off-by Jyri Sarha <[email protected]>

---

I'm not sure yet of the exact error path, but with this patch tilcdc_crtc_destroy() fails with a NULL-pointer dereference at dmam_free_coherent() due to crtc->dev being NULL if there are no panels registered.

Quoted 5 lines

Argh, should have read the dmam_alloc_coherent() function documentation. I just wondered what the extra m in function prefix was for and did not realize that it was a devres version of the function (I would have expected such a function to be called devm_dma_alloc_coherent()).

I don't get it either - the original commit introducing devres (9ac7849e35f7: "devres: device resource management") already had different prefixes for different managed interfaces.

Maybe we should propose renaming them unless there's a good reason to keep the dmam prefix?

Thanks,
Bartosz
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#9 lore raw

PATCH v3 4/4 drm/tilcdc: Fix race from forced shutdown of crtc in unload

In reply to Jyri Sarha

Fix race from forced shutdown of crtc in unload by adding internal locking and a boolean telling if device is going to be shutdown.

Signed-off-by Jyri Sarha <[email protected]>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 29 +++++++++++++++++++++++------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  2 +-
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  3 ++-
 3 files changed, 26 insertions(+), 8 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +23 -6
index 6277363..0d09acc 100644
@@ -33,7 +33,9 @@ struct tilcdc_crtc {
 	struct drm_plane primary;
 	const struct tilcdc_panel_info *info;
 	struct drm_pending_vblank_event *event;
+	struct mutex enable_lock;
 	bool enabled;
+	bool shutdown;
 	wait_queue_head_t frame_done_wq;
 	bool frame_done;
 	spinlock_t irq_lock;

@@ -158,9 +160,11 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-
-	if (tilcdc_crtc->enabled)
+	mutex_lock(&tilcdc_crtc->enable_lock);
+	if (tilcdc_crtc->enabled || tilcdc_crtc->shutdown) {
+		mutex_unlock(&tilcdc_crtc->enable_lock);
 		return;
+	}
 
 	pm_runtime_get_sync(dev->dev);
 

@@ -175,17 +179,22 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	drm_crtc_vblank_on(crtc);
 
 	tilcdc_crtc->enabled = true;
+	mutex_unlock(&tilcdc_crtc->enable_lock);
 }
 
-void tilcdc_crtc_off(struct drm_crtc *crtc)
+static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	if (!tilcdc_crtc->enabled)
+	mutex_lock(&tilcdc_crtc->enable_lock);
+	if (shutdown)
+		tilcdc_crtc->shutdown = true;
+	if (!tilcdc_crtc->enabled) {
+		mutex_unlock(&tilcdc_crtc->enable_lock);
 		return;
-
+	}
 	tilcdc_crtc->frame_done = false;
 	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
 

@@ -224,12 +233,18 @@ void tilcdc_crtc_off(struct drm_crtc *crtc)
 	tilcdc_crtc->last_vblank = ktime_set(0, 0);
 
 	tilcdc_crtc->enabled = false;
+	mutex_unlock(&tilcdc_crtc->enable_lock);
 }
 
 static void tilcdc_crtc_disable(struct drm_crtc *crtc)
 {
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
-	tilcdc_crtc_off(crtc);
+	tilcdc_crtc_off(crtc, false);
+}
+
+void tilcdc_crtc_shutdown(struct drm_crtc *crtc)
+{
+	tilcdc_crtc_off(crtc, true);
 }
 
 static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)

@@ -857,6 +872,8 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 	if (ret < 0)
 		goto fail;
 
+	mutex_init(&tilcdc_crtc->enable_lock);
+
 	init_waitqueue_head(&tilcdc_crtc->frame_done_wq);
 
 	drm_flip_work_init(&tilcdc_crtc->unref_work,
drivers/gpu/drm/tilcdc/tilcdc_drv.c +1 -1
index 14896b6..dcc9621 100644
@@ -197,7 +197,7 @@ static void tilcdc_fini(struct drm_device *dev)
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
 	if (priv->crtc)
-		tilcdc_crtc_off(priv->crtc);
+		tilcdc_crtc_shutdown(priv->crtc);
 
 	if (priv->is_registered)
 		drm_dev_unregister(dev);
drivers/gpu/drm/tilcdc/tilcdc_drv.h +2 -1
index 7db23f2..d31fe5d 100644
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_bridge.h>
 
 /* Defaulting to pixel clock defined on AM335x */
 #define TILCDC_DEFAULT_MAX_PIXELCLOCK  126000

@@ -173,7 +174,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc,
 					bool simulate_vesa_sync);
 int tilcdc_crtc_mode_valid(struct drm_crtc *crtc, struct drm_display_mode *mode);
 int tilcdc_crtc_max_width(struct drm_crtc *crtc);
-void tilcdc_crtc_off(struct drm_crtc *crtc);
+void tilcdc_crtc_shutdown(struct drm_crtc *crtc);
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 		struct drm_framebuffer *fb,
 		struct drm_pending_vblank_event *event);
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel