[PATCH 0/2] omapdrm: Remove .load/.unload midlayer

[PATCH 0/2] omapdrm: Remove .load/.unload midlayer

[Laurent Pinchart]

Hello,

This small patch series removes the .load and .unload operations from the
omapdrm driver and moves the code to the probe/remove handlers. The patches
are based on top of my pending omapdrm stack, and have been pushed for
convenience to

	git://linuxtv.org/pinchartl/media.git omapdrm/devel

Laurent Pinchart (2):
  drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures
  drm: omapdrm: Perform initialization/cleanup at probe/remove time

 drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c |   2 +-
 drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c |   3 +-
 drivers/gpu/drm/omapdrm/omap_connector.c        |   6 +-
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c        |   4 +-
 drivers/gpu/drm/omapdrm/omap_drv.c              | 229 ++++++++++++------------
 drivers/gpu/drm/omapdrm/omap_drv.h              |   1 +
 drivers/gpu/drm/omapdrm/omap_encoder.c          |   2 +-
 drivers/gpu/drm/omapdrm/omap_gem.c              |   3 +-
 8 files changed, 125 insertions(+), 125 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures

[Laurent Pinchart]

By linking the sizeof to a variable type the code will be less prone to
bugs due to future type changes of variables.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 2 +-
 drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c | 3 +--
 drivers/gpu/drm/omapdrm/omap_connector.c        | 4 ++--
 drivers/gpu/drm/omapdrm/omap_dmm_tiler.c        | 4 ++--
 drivers/gpu/drm/omapdrm/omap_encoder.c          | 2 +-
 5 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
index dc026a843712..a2bb855a2851 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
@@ -1253,7 +1253,7 @@ static int dsicm_probe(struct platform_device *pdev)
 	dsicm_hw_reset(ddata);
 
 	if (ddata->use_dsi_backlight) {
-		memset(&props, 0, sizeof(struct backlight_properties));
+		memset(&props, 0, sizeof(props));
 		props.max_brightness = 255;
 
 		props.type = BACKLIGHT_RAW;
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c b/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
index 136d30484d02..bf626acae271 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
@@ -119,8 +119,7 @@ static void __init omapdss_omapify_node(struct device_node *node)
 
 static void __init omapdss_add_to_list(struct device_node *node, bool root)
 {
-	struct dss_conv_node *n = kmalloc(sizeof(struct dss_conv_node),
-		GFP_KERNEL);
+	struct dss_conv_node *n = kmalloc(sizeof(*n), GFP_KERNEL);
 	if (n) {
 		n->node = node;
 		n->root = root;
diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
index 2580e8673908..691cffebb76e 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -162,7 +162,7 @@ static int omap_connector_mode_valid(struct drm_connector *connector,
 
 		dssdrv->get_timings(dssdev, &t);
 
-		if (memcmp(&vm, &t, sizeof(struct videomode)))
+		if (memcmp(&vm, &t, sizeof(vm)))
 			r = -EINVAL;
 		else
 			r = 0;
@@ -217,7 +217,7 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
 
 	omap_dss_get_device(dssdev);
 
-	omap_connector = kzalloc(sizeof(struct omap_connector), GFP_KERNEL);
+	omap_connector = kzalloc(sizeof(*omap_connector), GFP_KERNEL);
 	if (!omap_connector)
 		goto fail;
 
diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
index 4ceed7a9762f..3cab06661a08 100644
--- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
+++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
@@ -224,7 +224,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct pat_area *area,
 	int rows = (1 + area->y1 - area->y0);
 	int i = columns*rows;
 
-	pat = alloc_dma(txn, sizeof(struct pat), &pat_pa);
+	pat = alloc_dma(txn, sizeof(*pat), &pat_pa);
 
 	if (txn->last_pat)
 		txn->last_pat->next_pa = (uint32_t)pat_pa;
@@ -735,7 +735,7 @@ static int omap_dmm_probe(struct platform_device *dev)
 
 	/* alloc engines */
 	omap_dmm->engines = kcalloc(omap_dmm->num_engines,
-				    sizeof(struct refill_engine), GFP_KERNEL);
+				    sizeof(*omap_dmm->engines), GFP_KERNEL);
 	if (!omap_dmm->engines) {
 		ret = -ENOMEM;
 		goto fail;
diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
index a20f30039aee..86c977b7189a 100644
--- a/drivers/gpu/drm/omapdrm/omap_encoder.c
+++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
@@ -117,7 +117,7 @@ static int omap_encoder_update(struct drm_encoder *encoder,
 
 		dssdrv->get_timings(dssdev, &t);
 
-		if (memcmp(vm, &t, sizeof(struct videomode)))
+		if (memcmp(vm, &t, sizeof(*vm)))
 			ret = -EINVAL;
 		else
 			ret = 0;
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm: omapdrm: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

The drm driver .load() operation is prone to race conditions as it
initializes the driver after registering the device nodes. Its usage is
deprecated, inline it in the probe function and call drm_dev_alloc() and
drm_dev_register() explicitly.

For consistency inline the .unload() handler in the remove function as
well.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/omap_connector.c |   2 -
 drivers/gpu/drm/omapdrm/omap_drv.c       | 229 ++++++++++++++++---------------
 drivers/gpu/drm/omapdrm/omap_drv.h       |   1 +
 drivers/gpu/drm/omapdrm/omap_gem.c       |   3 +-
 4 files changed, 118 insertions(+), 117 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
index 691cffebb76e..f90e2d22c5ec 100644
--- a/drivers/gpu/drm/omapdrm/omap_connector.c
+++ b/drivers/gpu/drm/omapdrm/omap_connector.c
@@ -240,8 +240,6 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
 	connector->interlace_allowed = 1;
 	connector->doublescan_allowed = 0;
 
-	drm_connector_register(connector);
-
 	return connector;
 
 fail:
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 0a2d461d62cf..dc282e023118 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -493,11 +493,6 @@ static int omap_modeset_init(struct drm_device *dev)
 	return 0;
 }
 
-static void omap_modeset_free(struct drm_device *dev)
-{
-	drm_mode_config_cleanup(dev);
-}
-
 /*
  * drm ioctl funcs
  */
@@ -633,95 +628,6 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] =
  * drm driver funcs
  */
 
-/**
- * load - setup chip and create an initial config
- * @dev: DRM device
- * @flags: startup flags
- *
- * The driver load routine has to do several things:
- *   - initialize the memory manager
- *   - allocate initial config memory
- *   - setup the DRM framebuffer with the allocated memory
- */
-static int dev_load(struct drm_device *dev, unsigned long flags)
-{
-	struct omap_drm_platform_data *pdata = dev->dev->platform_data;
-	struct omap_drm_private *priv;
-	unsigned int i;
-	int ret;
-
-	DBG("load: dev=%p", dev);
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	priv->omaprev = pdata->omaprev;
-
-	dev->dev_private = priv;
-
-	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
-	init_waitqueue_head(&priv->commit.wait);
-	spin_lock_init(&priv->commit.lock);
-
-	spin_lock_init(&priv->list_lock);
-	INIT_LIST_HEAD(&priv->obj_list);
-
-	omap_gem_init(dev);
-
-	ret = omap_modeset_init(dev);
-	if (ret) {
-		dev_err(dev->dev, "omap_modeset_init failed: ret=%d\n", ret);
-		dev->dev_private = NULL;
-		kfree(priv);
-		return ret;
-	}
-
-	/* Initialize vblank handling, start with all CRTCs disabled. */
-	ret = drm_vblank_init(dev, priv->num_crtcs);
-	if (ret)
-		dev_warn(dev->dev, "could not init vblank\n");
-
-	for (i = 0; i < priv->num_crtcs; i++)
-		drm_crtc_vblank_off(priv->crtcs[i]);
-
-	priv->fbdev = omap_fbdev_init(dev);
-
-	/* store off drm_device for use in pm ops */
-	dev_set_drvdata(dev->dev, dev);
-
-	drm_kms_helper_poll_init(dev);
-
-	return 0;
-}
-
-static int dev_unload(struct drm_device *dev)
-{
-	struct omap_drm_private *priv = dev->dev_private;
-
-	DBG("unload: dev=%p", dev);
-
-	drm_kms_helper_poll_fini(dev);
-
-	if (priv->fbdev)
-		omap_fbdev_free(dev);
-
-	omap_modeset_free(dev);
-	omap_gem_deinit(dev);
-
-	destroy_workqueue(priv->wq);
-
-	drm_vblank_cleanup(dev);
-	omap_drm_irq_uninstall(dev);
-
-	kfree(dev->dev_private);
-	dev->dev_private = NULL;
-
-	dev_set_drvdata(dev->dev, NULL);
-
-	return 0;
-}
-
 static int dev_open(struct drm_device *dev, struct drm_file *file)
 {
 	file->driver_priv = NULL;
@@ -806,8 +712,6 @@ static const struct file_operations omapdriver_fops = {
 static struct drm_driver omap_drm_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM  | DRIVER_PRIME |
 		DRIVER_ATOMIC,
-	.load = dev_load,
-	.unload = dev_unload,
 	.open = dev_open,
 	.lastclose = dev_lastclose,
 	.get_vblank_counter = drm_vblank_no_hw_counter,
@@ -837,30 +741,129 @@ static struct drm_driver omap_drm_driver = {
 	.patchlevel = DRIVER_PATCHLEVEL,
 };
 
-static int pdev_probe(struct platform_device *device)
+static int pdev_probe(struct platform_device *pdev)
 {
-	int r;
+	struct omap_drm_platform_data *pdata = pdev->dev.platform_data;
+	struct omap_drm_private *priv;
+	struct drm_device *ddev;
+	unsigned int i;
+	int ret;
+
+	DBG("%s", pdev->name);
 
 	if (omapdss_is_initialized() == false)
 		return -EPROBE_DEFER;
 
 	omap_crtc_pre_init();
 
-	r = omap_connect_dssdevs();
-	if (r) {
+	ret = omap_connect_dssdevs();
+	if (ret) {
 		omap_crtc_pre_uninit();
-		return r;
+		goto err_crtc_uninit;
+	}
+
+	/* Allocate and initialize the driver private structure. */
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		ret = -ENOMEM;
+		goto err_disconnect_dssdevs;
+	}
+
+	priv->omaprev = pdata->omaprev;
+	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
+
+	init_waitqueue_head(&priv->commit.wait);
+	spin_lock_init(&priv->commit.lock);
+	spin_lock_init(&priv->list_lock);
+	INIT_LIST_HEAD(&priv->obj_list);
+
+	platform_set_drvdata(pdev, priv);
+
+	/* Allocate and initialize the DRM device. */
+	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
+	if (IS_ERR(ddev)) {
+		ret = PTR_ERR(ddev);
+		goto err_free_priv;
+	}
+
+	priv->drm_dev = ddev;
+	ddev->dev_private = priv;
+
+	omap_gem_init(ddev);
+
+	ret = omap_modeset_init(ddev);
+	if (ret) {
+		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
+		goto err_free_drm_dev;
 	}
 
-	DBG("%s", device->name);
-	return drm_platform_init(&omap_drm_driver, device);
+	/* Initialize vblank handling, start with all CRTCs disabled. */
+	ret = drm_vblank_init(ddev, priv->num_crtcs);
+	if (ret) {
+		dev_err(&pdev->dev, "could not init vblank\n");
+		goto err_cleanup_modeset;
+	}
+
+	for (i = 0; i < priv->num_crtcs; i++)
+		drm_crtc_vblank_off(priv->crtcs[i]);
+
+	priv->fbdev = omap_fbdev_init(ddev);
+
+	drm_kms_helper_poll_init(ddev);
+
+	/*
+	 * Register the DRM device with the core and the connectors with
+	 * sysfs.
+	 */
+	ret = drm_dev_register(ddev, 0);
+	if (ret)
+		goto err_cleanup_helpers;
+
+	return 0;
+
+err_cleanup_helpers:
+	drm_kms_helper_poll_fini(ddev);
+	if (priv->fbdev)
+		omap_fbdev_free(ddev);
+	drm_vblank_cleanup(ddev);
+err_cleanup_modeset:
+	drm_mode_config_cleanup(ddev);
+	omap_drm_irq_uninstall(ddev);
+err_free_drm_dev:
+	omap_gem_deinit(ddev);
+	drm_dev_unref(ddev);
+err_free_priv:
+	destroy_workqueue(priv->wq);
+	kfree(priv);
+err_disconnect_dssdevs:
+	omap_disconnect_dssdevs();
+err_crtc_uninit:
+	omap_crtc_pre_uninit();
+	return ret;
 }
 
-static int pdev_remove(struct platform_device *device)
+static int pdev_remove(struct platform_device *pdev)
 {
+	struct omap_drm_private *priv = platform_get_drvdata(pdev);
+	struct drm_device *ddev = priv->drm_dev;
+
 	DBG("");
 
-	drm_put_dev(platform_get_drvdata(device));
+	drm_dev_unregister(ddev);
+
+	if (priv->fbdev)
+		omap_fbdev_free(ddev);
+
+	drm_kms_helper_poll_fini(ddev);
+	drm_mode_config_cleanup(ddev);
+
+	omap_drm_irq_uninstall(ddev);
+	omap_gem_deinit(ddev);
+
+	drm_dev_unref(ddev);
+
+	destroy_workqueue(priv->wq);
+	kfree(priv);
 
 	omap_disconnect_dssdevs();
 	omap_crtc_pre_uninit();
@@ -907,26 +910,26 @@ static int omap_drm_resume_all_displays(void)
 
 static int omap_drm_suspend(struct device *dev)
 {
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct omap_drm_private *priv = dev_get_drvdata(dev);
 
-	drm_kms_helper_poll_disable(drm_dev);
+	drm_kms_helper_poll_disable(priv->drm_dev);
 
-	drm_modeset_lock_all(drm_dev);
+	drm_modeset_lock_all(priv->drm_dev);
 	omap_drm_suspend_all_displays();
-	drm_modeset_unlock_all(drm_dev);
+	drm_modeset_unlock_all(priv->drm_dev);
 
 	return 0;
 }
 
 static int omap_drm_resume(struct device *dev)
 {
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
+	struct omap_drm_private *priv = dev_get_drvdata(dev);
 
-	drm_modeset_lock_all(drm_dev);
+	drm_modeset_lock_all(priv->drm_dev);
 	omap_drm_resume_all_displays();
-	drm_modeset_unlock_all(drm_dev);
+	drm_modeset_unlock_all(priv->drm_dev);
 
-	drm_kms_helper_poll_enable(drm_dev);
+	drm_kms_helper_poll_enable(priv->drm_dev);
 
 	return omap_gem_resume(dev);
 }
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
index b20377efd01b..c3dcd311f82b 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -58,6 +58,7 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
 
 struct omap_drm_private {
 	uint32_t omaprev;
+	struct drm_device *drm_dev;
 
 	unsigned int num_crtcs;
 	struct drm_crtc *crtcs[8];
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index d4e1e11466f8..09a161736a48 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -1000,8 +1000,7 @@ void *omap_gem_vaddr(struct drm_gem_object *obj)
 /* re-pin objects in DMM in resume path: */
 int omap_gem_resume(struct device *dev)
 {
-	struct drm_device *drm_dev = dev_get_drvdata(dev);
-	struct omap_drm_private *priv = drm_dev->dev_private;
+	struct omap_drm_private *priv = dev_get_drvdata(dev);
 	struct omap_gem_object *omap_obj;
 	int ret = 0;
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures

[Daniel Vetter]

On Tue, Dec 13, 2016 at 05:21:42PM +0200, Laurent Pinchart wrote:
> By linking the sizeof to a variable type the code will be less prone to
> bugs due to future type changes of variables.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Isn't there some cocci magic that does this for you?
-Daniel

> ---
>  drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 2 +-
>  drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c | 3 +--
>  drivers/gpu/drm/omapdrm/omap_connector.c        | 4 ++--
>  drivers/gpu/drm/omapdrm/omap_dmm_tiler.c        | 4 ++--
>  drivers/gpu/drm/omapdrm/omap_encoder.c          | 2 +-
>  5 files changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index dc026a843712..a2bb855a2851 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -1253,7 +1253,7 @@ static int dsicm_probe(struct platform_device *pdev)
>  	dsicm_hw_reset(ddata);
>  
>  	if (ddata->use_dsi_backlight) {
> -		memset(&props, 0, sizeof(struct backlight_properties));
> +		memset(&props, 0, sizeof(props));
>  		props.max_brightness = 255;
>  
>  		props.type = BACKLIGHT_RAW;
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c b/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
> index 136d30484d02..bf626acae271 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c
> @@ -119,8 +119,7 @@ static void __init omapdss_omapify_node(struct device_node *node)
>  
>  static void __init omapdss_add_to_list(struct device_node *node, bool root)
>  {
> -	struct dss_conv_node *n = kmalloc(sizeof(struct dss_conv_node),
> -		GFP_KERNEL);
> +	struct dss_conv_node *n = kmalloc(sizeof(*n), GFP_KERNEL);
>  	if (n) {
>  		n->node = node;
>  		n->root = root;
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 2580e8673908..691cffebb76e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -162,7 +162,7 @@ static int omap_connector_mode_valid(struct drm_connector *connector,
>  
>  		dssdrv->get_timings(dssdev, &t);
>  
> -		if (memcmp(&vm, &t, sizeof(struct videomode)))
> +		if (memcmp(&vm, &t, sizeof(vm)))
>  			r = -EINVAL;
>  		else
>  			r = 0;
> @@ -217,7 +217,7 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
>  
>  	omap_dss_get_device(dssdev);
>  
> -	omap_connector = kzalloc(sizeof(struct omap_connector), GFP_KERNEL);
> +	omap_connector = kzalloc(sizeof(*omap_connector), GFP_KERNEL);
>  	if (!omap_connector)
>  		goto fail;
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> index 4ceed7a9762f..3cab06661a08 100644
> --- a/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> +++ b/drivers/gpu/drm/omapdrm/omap_dmm_tiler.c
> @@ -224,7 +224,7 @@ static void dmm_txn_append(struct dmm_txn *txn, struct pat_area *area,
>  	int rows = (1 + area->y1 - area->y0);
>  	int i = columns*rows;
>  
> -	pat = alloc_dma(txn, sizeof(struct pat), &pat_pa);
> +	pat = alloc_dma(txn, sizeof(*pat), &pat_pa);
>  
>  	if (txn->last_pat)
>  		txn->last_pat->next_pa = (uint32_t)pat_pa;
> @@ -735,7 +735,7 @@ static int omap_dmm_probe(struct platform_device *dev)
>  
>  	/* alloc engines */
>  	omap_dmm->engines = kcalloc(omap_dmm->num_engines,
> -				    sizeof(struct refill_engine), GFP_KERNEL);
> +				    sizeof(*omap_dmm->engines), GFP_KERNEL);
>  	if (!omap_dmm->engines) {
>  		ret = -ENOMEM;
>  		goto fail;
> diff --git a/drivers/gpu/drm/omapdrm/omap_encoder.c b/drivers/gpu/drm/omapdrm/omap_encoder.c
> index a20f30039aee..86c977b7189a 100644
> --- a/drivers/gpu/drm/omapdrm/omap_encoder.c
> +++ b/drivers/gpu/drm/omapdrm/omap_encoder.c
> @@ -117,7 +117,7 @@ static int omap_encoder_update(struct drm_encoder *encoder,
>  
>  		dssdrv->get_timings(dssdev, &t);
>  
> -		if (memcmp(vm, &t, sizeof(struct videomode)))
> +		if (memcmp(vm, &t, sizeof(*vm)))
>  			ret = -EINVAL;
>  		else
>  			ret = 0;
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: omapdrm: Perform initialization/cleanup at probe/remove time

[Daniel Vetter]

On Tue, Dec 13, 2016 at 05:21:43PM +0200, Laurent Pinchart wrote:
> The drm driver .load() operation is prone to race conditions as it
> initializes the driver after registering the device nodes. Its usage is
> deprecated, inline it in the probe function and call drm_dev_alloc() and
> drm_dev_register() explicitly.
> 
> For consistency inline the .unload() handler in the remove function as
> well.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_connector.c |   2 -
>  drivers/gpu/drm/omapdrm/omap_drv.c       | 229 ++++++++++++++++---------------
>  drivers/gpu/drm/omapdrm/omap_drv.h       |   1 +
>  drivers/gpu/drm/omapdrm/omap_gem.c       |   3 +-
>  4 files changed, 118 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_connector.c b/drivers/gpu/drm/omapdrm/omap_connector.c
> index 691cffebb76e..f90e2d22c5ec 100644
> --- a/drivers/gpu/drm/omapdrm/omap_connector.c
> +++ b/drivers/gpu/drm/omapdrm/omap_connector.c
> @@ -240,8 +240,6 @@ struct drm_connector *omap_connector_init(struct drm_device *dev,
>  	connector->interlace_allowed = 1;
>  	connector->doublescan_allowed = 0;
>  
> -	drm_connector_register(connector);
> -
>  	return connector;
>  
>  fail:
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> index 0a2d461d62cf..dc282e023118 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> @@ -493,11 +493,6 @@ static int omap_modeset_init(struct drm_device *dev)
>  	return 0;
>  }
>  
> -static void omap_modeset_free(struct drm_device *dev)
> -{
> -	drm_mode_config_cleanup(dev);
> -}
> -
>  /*
>   * drm ioctl funcs
>   */
> @@ -633,95 +628,6 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] =
>   * drm driver funcs
>   */
>  
> -/**
> - * load - setup chip and create an initial config
> - * @dev: DRM device
> - * @flags: startup flags
> - *
> - * The driver load routine has to do several things:
> - *   - initialize the memory manager
> - *   - allocate initial config memory
> - *   - setup the DRM framebuffer with the allocated memory
> - */
> -static int dev_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct omap_drm_platform_data *pdata = dev->dev->platform_data;
> -	struct omap_drm_private *priv;
> -	unsigned int i;
> -	int ret;
> -
> -	DBG("load: dev=%p", dev);
> -
> -	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -
> -	priv->omaprev = pdata->omaprev;
> -
> -	dev->dev_private = priv;
> -
> -	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> -	init_waitqueue_head(&priv->commit.wait);
> -	spin_lock_init(&priv->commit.lock);
> -
> -	spin_lock_init(&priv->list_lock);
> -	INIT_LIST_HEAD(&priv->obj_list);
> -
> -	omap_gem_init(dev);
> -
> -	ret = omap_modeset_init(dev);
> -	if (ret) {
> -		dev_err(dev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> -		dev->dev_private = NULL;
> -		kfree(priv);
> -		return ret;
> -	}
> -
> -	/* Initialize vblank handling, start with all CRTCs disabled. */
> -	ret = drm_vblank_init(dev, priv->num_crtcs);
> -	if (ret)
> -		dev_warn(dev->dev, "could not init vblank\n");
> -
> -	for (i = 0; i < priv->num_crtcs; i++)
> -		drm_crtc_vblank_off(priv->crtcs[i]);
> -
> -	priv->fbdev = omap_fbdev_init(dev);
> -
> -	/* store off drm_device for use in pm ops */
> -	dev_set_drvdata(dev->dev, dev);
> -
> -	drm_kms_helper_poll_init(dev);
> -
> -	return 0;
> -}
> -
> -static int dev_unload(struct drm_device *dev)
> -{
> -	struct omap_drm_private *priv = dev->dev_private;
> -
> -	DBG("unload: dev=%p", dev);
> -
> -	drm_kms_helper_poll_fini(dev);
> -
> -	if (priv->fbdev)
> -		omap_fbdev_free(dev);
> -
> -	omap_modeset_free(dev);
> -	omap_gem_deinit(dev);
> -
> -	destroy_workqueue(priv->wq);
> -
> -	drm_vblank_cleanup(dev);
> -	omap_drm_irq_uninstall(dev);
> -
> -	kfree(dev->dev_private);
> -	dev->dev_private = NULL;
> -
> -	dev_set_drvdata(dev->dev, NULL);
> -
> -	return 0;
> -}
> -
>  static int dev_open(struct drm_device *dev, struct drm_file *file)
>  {
>  	file->driver_priv = NULL;
> @@ -806,8 +712,6 @@ static const struct file_operations omapdriver_fops = {
>  static struct drm_driver omap_drm_driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM  | DRIVER_PRIME |
>  		DRIVER_ATOMIC,
> -	.load = dev_load,
> -	.unload = dev_unload,
>  	.open = dev_open,
>  	.lastclose = dev_lastclose,
>  	.get_vblank_counter = drm_vblank_no_hw_counter,
> @@ -837,30 +741,129 @@ static struct drm_driver omap_drm_driver = {
>  	.patchlevel = DRIVER_PATCHLEVEL,
>  };
>  
> -static int pdev_probe(struct platform_device *device)
> +static int pdev_probe(struct platform_device *pdev)
>  {
> -	int r;
> +	struct omap_drm_platform_data *pdata = pdev->dev.platform_data;
> +	struct omap_drm_private *priv;
> +	struct drm_device *ddev;
> +	unsigned int i;
> +	int ret;
> +
> +	DBG("%s", pdev->name);
>  
>  	if (omapdss_is_initialized() == false)
>  		return -EPROBE_DEFER;
>  
>  	omap_crtc_pre_init();
>  
> -	r = omap_connect_dssdevs();
> -	if (r) {
> +	ret = omap_connect_dssdevs();
> +	if (ret) {
>  		omap_crtc_pre_uninit();
> -		return r;
> +		goto err_crtc_uninit;
> +	}
> +
> +	/* Allocate and initialize the driver private structure. */
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv) {
> +		ret = -ENOMEM;
> +		goto err_disconnect_dssdevs;
> +	}
> +
> +	priv->omaprev = pdata->omaprev;
> +	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> +
> +	init_waitqueue_head(&priv->commit.wait);
> +	spin_lock_init(&priv->commit.lock);
> +	spin_lock_init(&priv->list_lock);
> +	INIT_LIST_HEAD(&priv->obj_list);
> +
> +	platform_set_drvdata(pdev, priv);

So either I'm blind, or the old code stored the drm_device in here, but
the new one the omap private stuff. Too lazy to audit the full driver, and
either way this should be a separate patch if you want to change it.

> +
> +	/* Allocate and initialize the DRM device. */
> +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);

Since this is a driver you work on, embedding is recommended, see
drm_dev_init(). Looks soo much better.

> +	if (IS_ERR(ddev)) {
> +		ret = PTR_ERR(ddev);
> +		goto err_free_priv;
> +	}
> +
> +	priv->drm_dev = ddev;
> +	ddev->dev_private = priv;
> +
> +	omap_gem_init(ddev);
> +
> +	ret = omap_modeset_init(ddev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", ret);
> +		goto err_free_drm_dev;
>  	}
>  
> -	DBG("%s", device->name);
> -	return drm_platform_init(&omap_drm_driver, device);
> +	/* Initialize vblank handling, start with all CRTCs disabled. */
> +	ret = drm_vblank_init(ddev, priv->num_crtcs);
> +	if (ret) {
> +		dev_err(&pdev->dev, "could not init vblank\n");
> +		goto err_cleanup_modeset;
> +	}
> +
> +	for (i = 0; i < priv->num_crtcs; i++)
> +		drm_crtc_vblank_off(priv->crtcs[i]);
> +
> +	priv->fbdev = omap_fbdev_init(ddev);
> +
> +	drm_kms_helper_poll_init(ddev);
> +
> +	/*
> +	 * Register the DRM device with the core and the connectors with
> +	 * sysfs.
> +	 */
> +	ret = drm_dev_register(ddev, 0);
> +	if (ret)
> +		goto err_cleanup_helpers;
> +
> +	return 0;
> +
> +err_cleanup_helpers:
> +	drm_kms_helper_poll_fini(ddev);
> +	if (priv->fbdev)
> +		omap_fbdev_free(ddev);
> +	drm_vblank_cleanup(ddev);
> +err_cleanup_modeset:
> +	drm_mode_config_cleanup(ddev);
> +	omap_drm_irq_uninstall(ddev);
> +err_free_drm_dev:
> +	omap_gem_deinit(ddev);
> +	drm_dev_unref(ddev);
> +err_free_priv:
> +	destroy_workqueue(priv->wq);
> +	kfree(priv);
> +err_disconnect_dssdevs:
> +	omap_disconnect_dssdevs();
> +err_crtc_uninit:
> +	omap_crtc_pre_uninit();
> +	return ret;
>  }
>  
> -static int pdev_remove(struct platform_device *device)
> +static int pdev_remove(struct platform_device *pdev)
>  {
> +	struct omap_drm_private *priv = platform_get_drvdata(pdev);
> +	struct drm_device *ddev = priv->drm_dev;
> +
>  	DBG("");
>  
> -	drm_put_dev(platform_get_drvdata(device));
> +	drm_dev_unregister(ddev);
> +
> +	if (priv->fbdev)
> +		omap_fbdev_free(ddev);
> +
> +	drm_kms_helper_poll_fini(ddev);

Probably want to keep poll_fini before you tear up the fbdev, because the
poll worker can call into the fbdev stuff. Not sure why you flipped those.

> +	drm_mode_config_cleanup(ddev);
> +


	drm_vblank_cleanup(dev);
^^ This one seems lost.

And I assume the driver core reset the drvdata to NULL, but too lazy to
double-check ;-)

With the above addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But the semi-random slight re-ordering of the init/teardown sequence on
this made it a bit tricky to review ...
-Daniel

> +	omap_drm_irq_uninstall(ddev);
> +	omap_gem_deinit(ddev);
> +
> +	drm_dev_unref(ddev);
> +
> +	destroy_workqueue(priv->wq);
> +	kfree(priv);
>  
>  	omap_disconnect_dssdevs();
>  	omap_crtc_pre_uninit();
> @@ -907,26 +910,26 @@ static int omap_drm_resume_all_displays(void)
>  
>  static int omap_drm_suspend(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  
> -	drm_kms_helper_poll_disable(drm_dev);
> +	drm_kms_helper_poll_disable(priv->drm_dev);
>  
> -	drm_modeset_lock_all(drm_dev);
> +	drm_modeset_lock_all(priv->drm_dev);
>  	omap_drm_suspend_all_displays();
> -	drm_modeset_unlock_all(drm_dev);
> +	drm_modeset_unlock_all(priv->drm_dev);
>  
>  	return 0;
>  }
>  
>  static int omap_drm_resume(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  
> -	drm_modeset_lock_all(drm_dev);
> +	drm_modeset_lock_all(priv->drm_dev);
>  	omap_drm_resume_all_displays();
> -	drm_modeset_unlock_all(drm_dev);
> +	drm_modeset_unlock_all(priv->drm_dev);
>  
> -	drm_kms_helper_poll_enable(drm_dev);
> +	drm_kms_helper_poll_enable(priv->drm_dev);
>  
>  	return omap_gem_resume(dev);
>  }
> diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h
> index b20377efd01b..c3dcd311f82b 100644
> --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> @@ -58,6 +58,7 @@ int omap_irq_wait(struct drm_device *dev, struct omap_irq_wait *wait,
>  
>  struct omap_drm_private {
>  	uint32_t omaprev;
> +	struct drm_device *drm_dev;
>  
>  	unsigned int num_crtcs;
>  	struct drm_crtc *crtcs[8];
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index d4e1e11466f8..09a161736a48 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -1000,8 +1000,7 @@ void *omap_gem_vaddr(struct drm_gem_object *obj)
>  /* re-pin objects in DMM in resume path: */
>  int omap_gem_resume(struct device *dev)
>  {
> -	struct drm_device *drm_dev = dev_get_drvdata(dev);
> -	struct omap_drm_private *priv = drm_dev->dev_private;
> +	struct omap_drm_private *priv = dev_get_drvdata(dev);
>  	struct omap_gem_object *omap_obj;
>  	int ret = 0;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures

[Laurent Pinchart]

On Tuesday 13 Dec 2016 22:12:12 Daniel Vetter wrote:
> On Tue, Dec 13, 2016 at 05:21:42PM +0200, Laurent Pinchart wrote:
> > By linking the sizeof to a variable type the code will be less prone to
> > bugs due to future type changes of variables.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Isn't there some cocci magic that does this for you?

There should be, but as it was only a handful of calls (well, for anyone who 
has a hand with 7 fingers) I was done before I thought I could use coccinelle.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: omapdrm: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 13 Dec 2016 22:21:10 Daniel Vetter wrote:
> On Tue, Dec 13, 2016 at 05:21:43PM +0200, Laurent Pinchart wrote:
> > The drm driver .load() operation is prone to race conditions as it
> > initializes the driver after registering the device nodes. Its usage is
> > deprecated, inline it in the probe function and call drm_dev_alloc() and
> > drm_dev_register() explicitly.
> > 
> > For consistency inline the .unload() handler in the remove function as
> > well.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_connector.c |   2 -
> >  drivers/gpu/drm/omapdrm/omap_drv.c       | 229 +++++++++++++-------------
> >  drivers/gpu/drm/omapdrm/omap_drv.h       |   1 +
> >  drivers/gpu/drm/omapdrm/omap_gem.c       |   3 +-
> >  4 files changed, 118 insertions(+), 117 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
> > b/drivers/gpu/drm/omapdrm/omap_drv.c index 0a2d461d62cf..dc282e023118
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c

[snip]

> > @@ -837,30 +741,129 @@ static struct drm_driver omap_drm_driver = {
> >  	.patchlevel = DRIVER_PATCHLEVEL,
> >  };
> > 
> > -static int pdev_probe(struct platform_device *device)
> > +static int pdev_probe(struct platform_device *pdev)
> >  {
> > -	int r;
> > +	struct omap_drm_platform_data *pdata = pdev->dev.platform_data;
> > +	struct omap_drm_private *priv;
> > +	struct drm_device *ddev;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	DBG("%s", pdev->name);
> > 
> >  	if (omapdss_is_initialized() == false)
> >  		return -EPROBE_DEFER;
> >  	
> >  	omap_crtc_pre_init();
> > 
> > -	r = omap_connect_dssdevs();
> > -	if (r) {
> > +	ret = omap_connect_dssdevs();
> > +	if (ret) {
> >  		omap_crtc_pre_uninit();
> > -		return r;
> > +		goto err_crtc_uninit;
> > +	}
> > +
> > +	/* Allocate and initialize the driver private structure. */
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv) {
> > +		ret = -ENOMEM;
> > +		goto err_disconnect_dssdevs;
> > +	}
> > +
> > +	priv->omaprev = pdata->omaprev;
> > +	priv->wq = alloc_ordered_workqueue("omapdrm", 0);
> > +
> > +	init_waitqueue_head(&priv->commit.wait);
> > +	spin_lock_init(&priv->commit.lock);
> > +	spin_lock_init(&priv->list_lock);
> > +	INIT_LIST_HEAD(&priv->obj_list);
> > +
> > +	platform_set_drvdata(pdev, priv);
> 
> So either I'm blind, or the old code stored the drm_device in here, but
> the new one the omap private stuff. Too lazy to audit the full driver, and
> either way this should be a separate patch if you want to change it.

OK, I'll split it to a separate patch.

> > +
> > +	/* Allocate and initialize the DRM device. */
> > +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> 
> Since this is a driver you work on, embedding is recommended, see
> drm_dev_init(). Looks soo much better.

How does that work with drm_dev_unref() then ? When the last reference goes 
away drm_dev_release() will kfree() the drm_device instance, which is only 
valid if I allocate it dynamically.

> > +	if (IS_ERR(ddev)) {
> > +		ret = PTR_ERR(ddev);
> > +		goto err_free_priv;
> > +	}
> > +
> > +	priv->drm_dev = ddev;
> > +	ddev->dev_private = priv;
> > +
> > +	omap_gem_init(ddev);
> > +
> > +	ret = omap_modeset_init(ddev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "omap_modeset_init failed: ret=%d\n", 
ret);
> > +		goto err_free_drm_dev;
> > 
> >  	}
> > 
> > -	DBG("%s", device->name);
> > -	return drm_platform_init(&omap_drm_driver, device);
> > +	/* Initialize vblank handling, start with all CRTCs disabled. */
> > +	ret = drm_vblank_init(ddev, priv->num_crtcs);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "could not init vblank\n");
> > +		goto err_cleanup_modeset;
> > +	}
> > +
> > +	for (i = 0; i < priv->num_crtcs; i++)
> > +		drm_crtc_vblank_off(priv->crtcs[i]);
> > +
> > +	priv->fbdev = omap_fbdev_init(ddev);
> > +
> > +	drm_kms_helper_poll_init(ddev);
> > +
> > +	/*
> > +	 * Register the DRM device with the core and the connectors with
> > +	 * sysfs.
> > +	 */
> > +	ret = drm_dev_register(ddev, 0);
> > +	if (ret)
> > +		goto err_cleanup_helpers;
> > +
> > +	return 0;
> > +
> > +err_cleanup_helpers:
> > +	drm_kms_helper_poll_fini(ddev);
> > +	if (priv->fbdev)
> > +		omap_fbdev_free(ddev);
> > +	drm_vblank_cleanup(ddev);
> > +err_cleanup_modeset:
> > +	drm_mode_config_cleanup(ddev);
> > +	omap_drm_irq_uninstall(ddev);
> > +err_free_drm_dev:
> > +	omap_gem_deinit(ddev);
> > +	drm_dev_unref(ddev);
> > +err_free_priv:
> > +	destroy_workqueue(priv->wq);
> > +	kfree(priv);
> > +err_disconnect_dssdevs:
> > +	omap_disconnect_dssdevs();
> > +err_crtc_uninit:
> > +	omap_crtc_pre_uninit();
> > +	return ret;
> >  }
> > 
> > -static int pdev_remove(struct platform_device *device)
> > +static int pdev_remove(struct platform_device *pdev)
> >  {
> > +	struct omap_drm_private *priv = platform_get_drvdata(pdev);
> > +	struct drm_device *ddev = priv->drm_dev;
> > +
> >  	DBG("");
> > 
> > -	drm_put_dev(platform_get_drvdata(device));
> > +	drm_dev_unregister(ddev);
> > +
> > +	if (priv->fbdev)
> > +		omap_fbdev_free(ddev);
> > +
> > +	drm_kms_helper_poll_fini(ddev);
> 
> Probably want to keep poll_fini before you tear up the fbdev, because the
> poll worker can call into the fbdev stuff. Not sure why you flipped those.

Good question. I'll fix that.

> > +	drm_mode_config_cleanup(ddev);
> > +
> 
> 	drm_vblank_cleanup(dev);
> ^^ This one seems lost.

drm_dev_unregister() calls drm_vblank_cleanup().

> And I assume the driver core reset the drvdata to NULL, but too lazy to
> double-check ;-)

Yes it does.

> With the above addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But the semi-random slight re-ordering of the init/teardown sequence on
> this made it a bit tricky to review ...
> -Daniel
> 
> > +	omap_drm_irq_uninstall(ddev);
> > +	omap_gem_deinit(ddev);
> > +
> > +	drm_dev_unref(ddev);
> > +
> > +	destroy_workqueue(priv->wq);
> > +	kfree(priv);
> > 
> >  	omap_disconnect_dssdevs();
> >  	omap_crtc_pre_uninit();

[snip]

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: omapdrm: Perform initialization/cleanup at probe/remove time

[Daniel Vetter]

On Tue, Dec 13, 2016 at 11:41:09PM +0200, Laurent Pinchart wrote:
> On Tuesday 13 Dec 2016 22:21:10 Daniel Vetter wrote:
> > On Tue, Dec 13, 2016 at 05:21:43PM +0200, Laurent Pinchart wrote:
> > > +
> > > +	/* Allocate and initialize the DRM device. */
> > > +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> > 
> > Since this is a driver you work on, embedding is recommended, see
> > drm_dev_init(). Looks soo much better.
> 
> How does that work with drm_dev_unref() then ? When the last reference goes 
> away drm_dev_release() will kfree() the drm_device instance, which is only 
> valid if I allocate it dynamically.

This is drm, we only solve shit half-assed until we realize how silly
we've been. Meanwhile just place the drm_device as the first thing in your
driver struct. The kerneldoc for drm_dev_init even bothers to explain this
all!
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: omapdrm: Perform initialization/cleanup at probe/remove time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 13 Dec 2016 22:48:50 Daniel Vetter wrote:
> On Tue, Dec 13, 2016 at 11:41:09PM +0200, Laurent Pinchart wrote:
> > On Tuesday 13 Dec 2016 22:21:10 Daniel Vetter wrote:
> >> On Tue, Dec 13, 2016 at 05:21:43PM +0200, Laurent Pinchart wrote:
> >>> +
> >>> +	/* Allocate and initialize the DRM device. */
> >>> +	ddev = drm_dev_alloc(&omap_drm_driver, &pdev->dev);
> >> 
> >> Since this is a driver you work on, embedding is recommended, see
> >> drm_dev_init(). Looks soo much better.
> > 
> > How does that work with drm_dev_unref() then ? When the last reference
> > goes away drm_dev_release() will kfree() the drm_device instance, which is
> > only valid if I allocate it dynamically.
> 
> This is drm, we only solve shit half-assed until we realize how silly
> we've been. Meanwhile just place the drm_device as the first thing in your
> driver struct.

I think I'll skip it for now, and revisit that when the drm_device ->release() 
operation lands.

> The kerneldoc for drm_dev_init even bothers to explain this all!

Not in Dave's drm-next, only in an obscure tree that hardly anyone knows about 
;-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel