On Tue 19 Apr 03:56 PDT 2016, Archit Taneja wrote:

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1) results in kernel warnings. This causes a lot of backtraces when we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja architt@codeaurora.org

drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index 26129bf..ce86117 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on) for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { struct hdmi_gpio_data gpio = config->gpios[i];

• 	if (gpio.output) {
  

• 		int value = gpio.value ? 0 : 1;
  

• 	if (gpio.num != -1) {
  

• 		if (gpio.output) {
  

• 			int value = gpio.value ? 0 : 1;
  
  

• 		gpio_set_value_cansleep(gpio.num, value);
  

• 	}
  

• 			gpio_set_value_cansleep(gpio.num,
  

• 						value);
  

• 		}
  
  

• 	gpio_free(gpio.num);
  

• 		gpio_free(gpio.num);
  

• 	}
  

  };

  DBG("gpio off");

@@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)

return 0; err:

• while (i--)

• gpio_free(config->gpios[i].num);
  

• while (i--) {

• if (config->gpios[i].num != -1)
  

• 	gpio_free(config->gpios[i].num);
  

• }

  return ret;

}

The patch in itself looks good, but the bigger picture does not.

The ddc and hdp should be muxed to the hdmi block, so they should not operated as gpios.

The mux seems more of a gpio so it should be made more explicit - i.e. actually support muxing (if that's needed) rather than just setting hard coded values.

And you should not gpio_request/free the gpio upon every usage, request it during "probe time" and release it during "remove".

Regards, Bjorn

On Tue, Apr 19, 2016 at 04:26:35PM +0530, Archit Taneja wrote:

Move the drm_connector registration from the encoder(HDMI/DSI etc) drivers to the msm platform driver. This will simplify the task of ensuring that the connectors are registered only after the drm_device itself is registered.

The connectors' destroy ops are made to use kzalloc instead of devm_kzalloc to ensure that that the connectors can be successfully unregistered when the msm driver module is removed. The memory for the connectors is unallocated when drm_mode_config_cleanup() is called during either during an error or during driver remove.

Signed-off-by: Archit Taneja architt@codeaurora.org

There's an in-flight patch series to add a common connector_(un)register_all(). Would be good to pick that up and rebase on top.

The patch series is from Alexey Brodkin Alexey.Brodkin@synopsys.com. -Daniel

---

drivers/gpu/drm/msm/dsi/dsi_manager.c | 27 ++++++++-------------- drivers/gpu/drm/msm/edp/edp_connector.c | 20 ++++------------ drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 17 +++----------- drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c | 16 ++----------- drivers/gpu/drm/msm/msm_drv.c | 15 ++++++++++++ 5 files changed, 33 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c index 58ba7ec..c8d1f19 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c @@ -198,9 +198,13 @@ static enum drm_connector_status dsi_mgr_connector_detect(

static void dsi_mgr_connector_destroy(struct drm_connector *connector) {

• struct dsi_connector *dsi_connector = to_dsi_connector(connector);
• DBG("");

• drm_connector_unregister(connector);

• drm_connector_cleanup(connector);
• kfree(dsi_connector);

}

static void dsi_dual_connector_fix_modes(struct drm_connector *connector) @@ -538,12 +542,9 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id) struct dsi_connector *dsi_connector; int ret, i;

• dsi_connector = devm_kzalloc(msm_dsi->dev->dev,

• 		sizeof(*dsi_connector), GFP_KERNEL);
  

• if (!dsi_connector) {

• ret = -ENOMEM;
  

• goto fail;
  

• }

• dsi_connector = kzalloc(sizeof(*dsi_connector), GFP_KERNEL);

• if (!dsi_connector)

• return ERR_PTR(-ENOMEM);
  
  

  dsi_connector->id = id;

@@ -552,7 +553,7 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id) ret = drm_connector_init(msm_dsi->dev, connector, &dsi_mgr_connector_funcs, DRM_MODE_CONNECTOR_DSI); if (ret)

• goto fail;
  

• return ERR_PTR(ret);
  
  

  drm_connector_helper_add(connector, &dsi_mgr_conn_helper_funcs);

@@ -565,21 +566,11 @@ struct drm_connector *msm_dsi_manager_connector_init(u8 id) connector->interlace_allowed = 0; connector->doublescan_allowed = 0;

• ret = drm_connector_register(connector);

• if (ret)

• goto fail;
  

• for (i = 0; i < MSM_DSI_ENCODER_NUM; i++) drm_mode_connector_attach_encoder(connector, msm_dsi->encoders[i]);

  return connector;

-fail:

• if (connector)

• dsi_mgr_connector_destroy(connector);
  

• return ERR_PTR(ret);

}

/* initialize bridge */ diff --git a/drivers/gpu/drm/msm/edp/edp_connector.c b/drivers/gpu/drm/msm/edp/edp_connector.c index b4d1b46..72360cd 100644 --- a/drivers/gpu/drm/msm/edp/edp_connector.c +++ b/drivers/gpu/drm/msm/edp/edp_connector.c @@ -37,7 +37,7 @@ static void edp_connector_destroy(struct drm_connector *connector) struct edp_connector *edp_connector = to_edp_connector(connector);

DBG("");

• drm_connector_unregister(connector);

• drm_connector_cleanup(connector);

  kfree(edp_connector);

@@ -124,10 +124,8 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp) int ret;

edp_connector = kzalloc(sizeof(*edp_connector), GFP_KERNEL);

• if (!edp_connector) {

• ret = -ENOMEM;
  

• goto fail;
  

• }

• if (!edp_connector)

• return ERR_PTR(-ENOMEM);
  
  

  edp_connector->edp = edp;

@@ -136,7 +134,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp) ret = drm_connector_init(edp->dev, connector, &edp_connector_funcs, DRM_MODE_CONNECTOR_eDP); if (ret)

• goto fail;
  

• return ERR_PTR(ret);
  
  

  drm_connector_helper_add(connector, &edp_connector_helper_funcs);

@@ -147,17 +145,7 @@ struct drm_connector *msm_edp_connector_init(struct msm_edp *edp) connector->interlace_allowed = false; connector->doublescan_allowed = false;

• ret = drm_connector_register(connector);

• if (ret)

• goto fail;
  

• drm_mode_connector_attach_encoder(connector, edp->encoder);

  return connector;

-fail:

• if (connector)

• edp_connector_destroy(connector);
  

• return ERR_PTR(ret);

} diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index ce86117..9cc84ce 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -346,7 +346,6 @@ static void hdmi_connector_destroy(struct drm_connector *connector)

hdp_disable(hdmi_connector);

• drm_connector_unregister(connector); drm_connector_cleanup(connector);

  kfree(hdmi_connector);

@@ -438,10 +437,8 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi) int ret;

hdmi_connector = kzalloc(sizeof(*hdmi_connector), GFP_KERNEL);

• if (!hdmi_connector) {

• ret = -ENOMEM;
  

• goto fail;
  

• }

• if (!hdmi_connector)

• return ERR_PTR(-ENOMEM);
  
  

  hdmi_connector->hdmi = hdmi; INIT_WORK(&hdmi_connector->hpd_work, msm_hdmi_hotplug_work);

@@ -458,21 +455,13 @@ struct drm_connector *msm_hdmi_connector_init(struct hdmi *hdmi) connector->interlace_allowed = 0; connector->doublescan_allowed = 0;

• drm_connector_register(connector);
• ret = hpd_enable(hdmi_connector); if (ret) { dev_err(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);

• goto fail;
  

• return ERR_PTR(ret);
  

  }

  drm_mode_connector_attach_encoder(connector, hdmi->encoder);

  return connector;

-fail:

• if (connector)

• hdmi_connector_destroy(connector);
  

• return ERR_PTR(ret);

} diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c index e73e174..2648cd7 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_lvds_connector.c @@ -48,7 +48,6 @@ static void mdp4_lvds_connector_destroy(struct drm_connector *connector) struct mdp4_lvds_connector *mdp4_lvds_connector = to_mdp4_lvds_connector(connector);

• drm_connector_unregister(connector); drm_connector_cleanup(connector);

  kfree(mdp4_lvds_connector);

@@ -121,13 +120,10 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, { struct drm_connector *connector = NULL; struct mdp4_lvds_connector *mdp4_lvds_connector;

• int ret;

  mdp4_lvds_connector = kzalloc(sizeof(*mdp4_lvds_connector), GFP_KERNEL);

• if (!mdp4_lvds_connector) {

• ret = -ENOMEM;
  

• goto fail;
  

• }

• if (!mdp4_lvds_connector)

• return ERR_PTR(-ENOMEM);
  
  

  mdp4_lvds_connector->encoder = encoder; mdp4_lvds_connector->panel_node = panel_node;

@@ -143,15 +139,7 @@ struct drm_connector *mdp4_lvds_connector_init(struct drm_device *dev, connector->interlace_allowed = 0; connector->doublescan_allowed = 0;

• drm_connector_register(connector);

• drm_mode_connector_attach_encoder(connector, encoder);

  return connector;

-fail:

• if (connector)

• mdp4_lvds_connector_destroy(connector);
  

• return ERR_PTR(ret);

} diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index c03b967..47f40d9 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -197,6 +197,8 @@ static int msm_unload(struct drm_device *dev)

drm_kms_helper_poll_fini(dev);

• drm_connector_unregister_all(dev);

#ifdef CONFIG_DRM_FBDEV_EMULATION if (fbdev && priv->fbdev) msm_fbdev_free(dev); @@ -326,6 +328,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags) struct platform_device *pdev = dev->platformdev; struct msm_drm_private *priv; struct msm_kms *kms;

• struct drm_connector *connector; int ret;

  priv = kzalloc(sizeof(*priv), GFP_KERNEL);

@@ -410,6 +413,18 @@ static int msm_load(struct drm_device *dev, unsigned long flags) goto fail; }

• mutex_lock(&dev->mode_config.mutex);
• drm_for_each_connector(connector, dev) {

• ret = drm_connector_register(connector);
  

• if (ret) {
  

• 	mutex_unlock(&dev->mode_config.mutex);
  

• 	goto fail;
  

• }
  

• }
• mutex_unlock(&dev->mode_config.mutex);
• drm_mode_config_reset(dev);

#ifdef CONFIG_DRM_FBDEV_EMULATION

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

---

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

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1) results in kernel warnings. This causes a lot of backtraces when we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index 26129bf..ce86117 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on) for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { struct hdmi_gpio_data gpio = config->gpios[i];

- if (gpio.output) { - int value = gpio.value ? 0 : 1; + if (gpio.num != -1) { + if (gpio.output) { + int value = gpio.value ? 0 : 1;

- gpio_set_value_cansleep(gpio.num, value); - } + gpio_set_value_cansleep(gpio.num, + value); + }

- gpio_free(gpio.num); + gpio_free(gpio.num); + } };

DBG("gpio off"); @@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)

return 0; err: - while (i--) - gpio_free(config->gpios[i].num); + while (i--) { + if (config->gpios[i].num != -1) + gpio_free(config->gpios[i].num); + }

return ret; }

On 04/25/2016 11:18 PM, twp@codeaurora.org wrote:

On 2016-04-25 03:16, Archit Taneja wrote:

...

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1) results in kernel warnings. This causes a lot of backtraces when we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja architt@codeaurora.org

drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index 26129bf..ce86117 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -112,13 +112,16 @@ static int gpio_config(struct hdmi *hdmi, bool on) for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { struct hdmi_gpio_data gpio = config->gpios[i];

•        if (gpio.output) {
  

•            int value = gpio.value ? 0 : 1;
  

•        if (gpio.num != -1) {
  

•            if (gpio.output) {
  

•                int value = gpio.value ? 0 : 1;
  
  

•            gpio_set_value_cansleep(gpio.num, value);
  

•        }
  

•                gpio_set_value_cansleep(gpio.num,
  

•                            value);
  

•            }
  
  

•        gpio_free(gpio.num);
  

•            gpio_free(gpio.num);
  

•        }
  

Can you do something like:

if (gpio.num == -1) continue;

instead? That would avoid the additional indentation (and increase readability).

Yes, that should make things cleaner. I'll make this change.

Thanks, Archit

The load/unload drm_driver ops are deprecated. They should be removed as they result in creation of devices visible to userspace even before the drm_device is registered.

Drop these ops and use drm_dev_alloc/register and drm_dev_unregister/unref to explicitly create and destroy the drm device in the msm platform driver's bind and unbind ops. With this in use, the drm connectors are only registered once the drm_device is registered.

It also fixes the issue of stray debugfs files after the msm module is removed. With this, all the debugfs files are removed, and allows successive module insertions/removals.

Signed-off-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/msm/msm_drv.c | 125 ++++++++++++++++++++++++------------------ 1 file changed, 71 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 8c5b257..a6db6e6 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -173,13 +173,11 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv, return 0; }

-/* - * DRM operations: - */ - -static int msm_unload(struct drm_device *dev) +static int msm_drm_uninit(struct device *dev) { - struct msm_drm_private *priv = dev->dev_private; + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *ddev = platform_get_drvdata(pdev); + struct msm_drm_private *priv = ddev->dev_private; struct msm_kms *kms = priv->kms; struct msm_gpu *gpu = priv->gpu; struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; @@ -195,33 +193,34 @@ static int msm_unload(struct drm_device *dev) kfree(vbl_ev); }

- drm_kms_helper_poll_fini(dev); + drm_kms_helper_poll_fini(ddev); + + drm_connector_unregister_all(ddev);

- drm_connector_unregister_all(dev); + drm_dev_unregister(ddev);

#ifdef CONFIG_DRM_FBDEV_EMULATION if (fbdev && priv->fbdev) - msm_fbdev_free(dev); + msm_fbdev_free(ddev); #endif - drm_mode_config_cleanup(dev); - drm_vblank_cleanup(dev); + drm_mode_config_cleanup(ddev);

- pm_runtime_get_sync(dev->dev); - drm_irq_uninstall(dev); - pm_runtime_put_sync(dev->dev); + pm_runtime_get_sync(dev); + drm_irq_uninstall(ddev); + pm_runtime_put_sync(dev);

flush_workqueue(priv->wq); destroy_workqueue(priv->wq);

if (kms) { - pm_runtime_disable(dev->dev); + pm_runtime_disable(dev); kms->funcs->destroy(kms); }

if (gpu) { - mutex_lock(&dev->struct_mutex); + mutex_lock(&ddev->struct_mutex); gpu->funcs->pm_suspend(gpu); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&ddev->struct_mutex); gpu->funcs->destroy(gpu); }

@@ -229,13 +228,14 @@ static int msm_unload(struct drm_device *dev) DEFINE_DMA_ATTRS(attrs); dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &attrs); drm_mm_takedown(&priv->vram.mm); - dma_free_attrs(dev->dev, priv->vram.size, NULL, - priv->vram.paddr, &attrs); + dma_free_attrs(dev, priv->vram.size, NULL, + priv->vram.paddr, &attrs); }

- component_unbind_all(dev->dev, dev); + component_unbind_all(dev, ddev);

- dev->dev_private = NULL; + ddev->dev_private = NULL; + drm_dev_unref(ddev);

kfree(priv);

@@ -323,20 +323,30 @@ static int msm_init_vram(struct drm_device *dev) return ret; }

-static int msm_load(struct drm_device *dev, unsigned long flags) +static int msm_drm_init(struct device *dev, struct drm_driver *drv) { - struct platform_device *pdev = dev->platformdev; + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *ddev; struct msm_drm_private *priv; struct msm_kms *kms; int ret;

+ ddev = drm_dev_alloc(drv, dev); + if (!ddev) { + dev_err(dev, "failed to allocate drm_device\n"); + return -ENOMEM; + } + + platform_set_drvdata(pdev, ddev); + ddev->platformdev = pdev; + priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) { - dev_err(dev->dev, "failed to allocate private data\n"); + drm_dev_unref(ddev); return -ENOMEM; }

- dev->dev_private = priv; + ddev->dev_private = priv;

priv->wq = alloc_ordered_workqueue("msm", 0); init_waitqueue_head(&priv->fence_event); @@ -348,25 +358,26 @@ static int msm_load(struct drm_device *dev, unsigned long flags) INIT_WORK(&priv->vblank_ctrl.work, vblank_ctrl_worker); spin_lock_init(&priv->vblank_ctrl.lock);

- drm_mode_config_init(dev); - - platform_set_drvdata(pdev, dev); + drm_mode_config_init(ddev);

/* Bind all our sub-components: */ - ret = component_bind_all(dev->dev, dev); - if (ret) + ret = component_bind_all(dev, ddev); + if (ret) { + kfree(priv); + drm_dev_unref(ddev); return ret; + }

- ret = msm_init_vram(dev); + ret = msm_init_vram(ddev); if (ret) goto fail;

switch (get_mdp_ver(pdev)) { case 4: - kms = mdp4_kms_init(dev); + kms = mdp4_kms_init(ddev); break; case 5: - kms = mdp5_kms_init(dev); + kms = mdp5_kms_init(ddev); break; default: kms = ERR_PTR(-ENODEV); @@ -380,7 +391,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags) * and (for example) use dmabuf/prime to share buffers with * imx drm driver on iMX5 */ - dev_err(dev->dev, "failed to load kms\n"); + dev_err(dev, "failed to load kms\n"); ret = PTR_ERR(kms); goto fail; } @@ -388,56 +399,64 @@ static int msm_load(struct drm_device *dev, unsigned long flags) priv->kms = kms;

if (kms) { - pm_runtime_enable(dev->dev); + pm_runtime_enable(dev); ret = kms->funcs->hw_init(kms); if (ret) { - dev_err(dev->dev, "kms hw init failed: %d\n", ret); + dev_err(dev, "kms hw init failed: %d\n", ret); goto fail; } }

- dev->mode_config.funcs = &mode_config_funcs; + ddev->mode_config.funcs = &mode_config_funcs;

- ret = drm_vblank_init(dev, priv->num_crtcs); + ret = drm_vblank_init(ddev, priv->num_crtcs); if (ret < 0) { - dev_err(dev->dev, "failed to initialize vblank\n"); + dev_err(dev, "failed to initialize vblank\n"); goto fail; }

- pm_runtime_get_sync(dev->dev); - ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0)); - pm_runtime_put_sync(dev->dev); + pm_runtime_get_sync(dev); + ret = drm_irq_install(ddev, platform_get_irq(pdev, 0)); + pm_runtime_put_sync(dev); if (ret < 0) { - dev_err(dev->dev, "failed to install IRQ handler\n"); + dev_err(dev, "failed to install IRQ handler\n"); goto fail; }

- ret = drm_connector_register_all(dev); + ret = drm_dev_register(ddev, 0); + if (ret) + goto fail; + + ret = drm_connector_register_all(ddev); if (ret) { - dev_err(dev->dev, "failed to register connectors\n"); + dev_err(dev, "failed to register connectors\n"); goto fail; }

- drm_mode_config_reset(dev); + drm_mode_config_reset(ddev);

#ifdef CONFIG_DRM_FBDEV_EMULATION if (fbdev) - priv->fbdev = msm_fbdev_init(dev); + priv->fbdev = msm_fbdev_init(ddev); #endif

- ret = msm_debugfs_late_init(dev); + ret = msm_debugfs_late_init(ddev); if (ret) goto fail;

- drm_kms_helper_poll_init(dev); + drm_kms_helper_poll_init(ddev);

return 0;

fail: - msm_unload(dev); + msm_drm_uninit(dev); return ret; }

+/* + * DRM operations: + */ + static void load_gpu(struct drm_device *dev) { static DEFINE_MUTEX(init_lock); @@ -960,8 +979,6 @@ static struct drm_driver msm_driver = { DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_MODESET, - .load = msm_load, - .unload = msm_unload, .open = msm_open, .preclose = msm_preclose, .lastclose = msm_lastclose, @@ -1061,12 +1078,12 @@ static int add_components(struct device *dev, struct component_match **matchptr,

static int msm_drm_bind(struct device *dev) { - return drm_platform_init(&msm_driver, to_platform_device(dev)); + return msm_drm_init(dev, &msm_driver); }

static void msm_drm_unbind(struct device *dev) { - drm_put_dev(platform_get_drvdata(to_platform_device(dev))); + msm_drm_uninit(dev); }

static const struct component_master_ops msm_drm_ops = {

Calling the legacy gpio_free on an invalid GPIO (a GPIO numbered -1) results in kernel warnings. This causes a lot of backtraces when we try to unload the drm/msm module.

Call gpio_free only on valid GPIOs.

Signed-off-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/msm/hdmi/hdmi_connector.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c index 26129bf..e350b2e 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi_connector.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi_connector.c @@ -112,6 +112,9 @@ static int gpio_config(struct hdmi *hdmi, bool on) for (i = 0; i < HDMI_MAX_NUM_GPIO; i++) { struct hdmi_gpio_data gpio = config->gpios[i];

+ if (gpio.num == -1) + continue; + if (gpio.output) { int value = gpio.value ? 0 : 1;

@@ -126,8 +129,10 @@ static int gpio_config(struct hdmi *hdmi, bool on)

return 0; err: - while (i--) - gpio_free(config->gpios[i].num); + while (i--) { + if (config->gpios[i].num != -1) + gpio_free(config->gpios[i].num); + }

return ret; }

The load/unload drm_driver ops are deprecated. They should be removed as they result in creation of devices visible to userspace even before the drm_device is registered.

Drop these ops and use drm_dev_alloc/register and drm_dev_unregister/unref to explicitly create and destroy the drm device in the msm platform driver's bind and unbind ops. With this in use, the drm connectors are only registered once the drm_device is registered.

It also fixes the issue of stray debugfs files after the msm module is removed. With this, all the debugfs files are removed, and allows successive module insertions/removals.

Signed-off-by: Archit Taneja architt@codeaurora.org --- drivers/gpu/drm/msm/msm_drv.c | 125 ++++++++++++++++++++++++------------------ 1 file changed, 71 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index 8c5b257..a6db6e6 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -173,13 +173,11 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv, return 0; }

-/* - * DRM operations: - */ - -static int msm_unload(struct drm_device *dev) +static int msm_drm_uninit(struct device *dev) { - struct msm_drm_private *priv = dev->dev_private; + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *ddev = platform_get_drvdata(pdev); + struct msm_drm_private *priv = ddev->dev_private; struct msm_kms *kms = priv->kms; struct msm_gpu *gpu = priv->gpu; struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; @@ -195,33 +193,34 @@ static int msm_unload(struct drm_device *dev) kfree(vbl_ev); }

- drm_kms_helper_poll_fini(dev); + drm_kms_helper_poll_fini(ddev); + + drm_connector_unregister_all(ddev);

- drm_connector_unregister_all(dev); + drm_dev_unregister(ddev);

#ifdef CONFIG_DRM_FBDEV_EMULATION if (fbdev && priv->fbdev) - msm_fbdev_free(dev); + msm_fbdev_free(ddev); #endif - drm_mode_config_cleanup(dev); - drm_vblank_cleanup(dev); + drm_mode_config_cleanup(ddev);

- pm_runtime_get_sync(dev->dev); - drm_irq_uninstall(dev); - pm_runtime_put_sync(dev->dev); + pm_runtime_get_sync(dev); + drm_irq_uninstall(ddev); + pm_runtime_put_sync(dev);

flush_workqueue(priv->wq); destroy_workqueue(priv->wq);

if (kms) { - pm_runtime_disable(dev->dev); + pm_runtime_disable(dev); kms->funcs->destroy(kms); }

if (gpu) { - mutex_lock(&dev->struct_mutex); + mutex_lock(&ddev->struct_mutex); gpu->funcs->pm_suspend(gpu); - mutex_unlock(&dev->struct_mutex); + mutex_unlock(&ddev->struct_mutex); gpu->funcs->destroy(gpu); }

@@ -229,13 +228,14 @@ static int msm_unload(struct drm_device *dev) DEFINE_DMA_ATTRS(attrs); dma_set_attr(DMA_ATTR_NO_KERNEL_MAPPING, &attrs); drm_mm_takedown(&priv->vram.mm); - dma_free_attrs(dev->dev, priv->vram.size, NULL, - priv->vram.paddr, &attrs); + dma_free_attrs(dev, priv->vram.size, NULL, + priv->vram.paddr, &attrs); }

- component_unbind_all(dev->dev, dev); + component_unbind_all(dev, ddev);

- dev->dev_private = NULL; + ddev->dev_private = NULL; + drm_dev_unref(ddev);

kfree(priv);

@@ -323,20 +323,30 @@ static int msm_init_vram(struct drm_device *dev) return ret; }

-static int msm_load(struct drm_device *dev, unsigned long flags) +static int msm_drm_init(struct device *dev, struct drm_driver *drv) { - struct platform_device *pdev = dev->platformdev; + struct platform_device *pdev = to_platform_device(dev); + struct drm_device *ddev; struct msm_drm_private *priv; struct msm_kms *kms; int ret;

+ ddev = drm_dev_alloc(drv, dev); + if (!ddev) { + dev_err(dev, "failed to allocate drm_device\n"); + return -ENOMEM; + } + + platform_set_drvdata(pdev, ddev); + ddev->platformdev = pdev; + priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) { - dev_err(dev->dev, "failed to allocate private data\n"); + drm_dev_unref(ddev); return -ENOMEM; }

- dev->dev_private = priv; + ddev->dev_private = priv;

priv->wq = alloc_ordered_workqueue("msm", 0); init_waitqueue_head(&priv->fence_event); @@ -348,25 +358,26 @@ static int msm_load(struct drm_device *dev, unsigned long flags) INIT_WORK(&priv->vblank_ctrl.work, vblank_ctrl_worker); spin_lock_init(&priv->vblank_ctrl.lock);

- drm_mode_config_init(dev); - - platform_set_drvdata(pdev, dev); + drm_mode_config_init(ddev);

/* Bind all our sub-components: */ - ret = component_bind_all(dev->dev, dev); - if (ret) + ret = component_bind_all(dev, ddev); + if (ret) { + kfree(priv); + drm_dev_unref(ddev); return ret; + }

- ret = msm_init_vram(dev); + ret = msm_init_vram(ddev); if (ret) goto fail;

switch (get_mdp_ver(pdev)) { case 4: - kms = mdp4_kms_init(dev); + kms = mdp4_kms_init(ddev); break; case 5: - kms = mdp5_kms_init(dev); + kms = mdp5_kms_init(ddev); break; default: kms = ERR_PTR(-ENODEV); @@ -380,7 +391,7 @@ static int msm_load(struct drm_device *dev, unsigned long flags) * and (for example) use dmabuf/prime to share buffers with * imx drm driver on iMX5 */ - dev_err(dev->dev, "failed to load kms\n"); + dev_err(dev, "failed to load kms\n"); ret = PTR_ERR(kms); goto fail; } @@ -388,56 +399,64 @@ static int msm_load(struct drm_device *dev, unsigned long flags) priv->kms = kms;

if (kms) { - pm_runtime_enable(dev->dev); + pm_runtime_enable(dev); ret = kms->funcs->hw_init(kms); if (ret) { - dev_err(dev->dev, "kms hw init failed: %d\n", ret); + dev_err(dev, "kms hw init failed: %d\n", ret); goto fail; } }

- dev->mode_config.funcs = &mode_config_funcs; + ddev->mode_config.funcs = &mode_config_funcs;

- ret = drm_vblank_init(dev, priv->num_crtcs); + ret = drm_vblank_init(ddev, priv->num_crtcs); if (ret < 0) { - dev_err(dev->dev, "failed to initialize vblank\n"); + dev_err(dev, "failed to initialize vblank\n"); goto fail; }

- pm_runtime_get_sync(dev->dev); - ret = drm_irq_install(dev, platform_get_irq(dev->platformdev, 0)); - pm_runtime_put_sync(dev->dev); + pm_runtime_get_sync(dev); + ret = drm_irq_install(ddev, platform_get_irq(pdev, 0)); + pm_runtime_put_sync(dev); if (ret < 0) { - dev_err(dev->dev, "failed to install IRQ handler\n"); + dev_err(dev, "failed to install IRQ handler\n"); goto fail; }

- ret = drm_connector_register_all(dev); + ret = drm_dev_register(ddev, 0); + if (ret) + goto fail; + + ret = drm_connector_register_all(ddev); if (ret) { - dev_err(dev->dev, "failed to register connectors\n"); + dev_err(dev, "failed to register connectors\n"); goto fail; }

- drm_mode_config_reset(dev); + drm_mode_config_reset(ddev);

#ifdef CONFIG_DRM_FBDEV_EMULATION if (fbdev) - priv->fbdev = msm_fbdev_init(dev); + priv->fbdev = msm_fbdev_init(ddev); #endif

- ret = msm_debugfs_late_init(dev); + ret = msm_debugfs_late_init(ddev); if (ret) goto fail;

- drm_kms_helper_poll_init(dev); + drm_kms_helper_poll_init(ddev);

return 0;

fail: - msm_unload(dev); + msm_drm_uninit(dev); return ret; }

+/* + * DRM operations: + */ + static void load_gpu(struct drm_device *dev) { static DEFINE_MUTEX(init_lock); @@ -960,8 +979,6 @@ static struct drm_driver msm_driver = { DRIVER_RENDER | DRIVER_ATOMIC | DRIVER_MODESET, - .load = msm_load, - .unload = msm_unload, .open = msm_open, .preclose = msm_preclose, .lastclose = msm_lastclose, @@ -1061,12 +1078,12 @@ static int add_components(struct device *dev, struct component_match **matchptr,

static int msm_drm_bind(struct device *dev) { - return drm_platform_init(&msm_driver, to_platform_device(dev)); + return msm_drm_init(dev, &msm_driver); }

static void msm_drm_unbind(struct device *dev) { - drm_put_dev(platform_get_drvdata(to_platform_device(dev))); + msm_drm_uninit(dev); }

static const struct component_master_ops msm_drm_ops = {