Re: [RFC PATCH 1/3] drm/exynos: make kms drivers to be independent modules

[Andrzej Hajda <a.hajda@samsung.com>]

On 11/18/2014 04:26 PM, Inki Dae wrote:
> This patch makes kms drivers to be independent modules.
> For this, it removes all register codes to kms drivers
> from exynos_drm_drv module and adds module_init/exit
> for each kms driver so that each kms driver can be
> called independently.


It is hard to test if modules are working because I am not able to build
modules :), see comments below.


> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> ---
>  drivers/gpu/drm/exynos/Kconfig           |    8 +++---
>  drivers/gpu/drm/exynos/exynos_dp_core.c  |   13 +++++++++
>  drivers/gpu/drm/exynos/exynos_drm_drv.c  |   43 +++---------------------------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h  |    5 ----
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c  |   13 +++++++++
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |   13 +++++++++
>  drivers/gpu/drm/exynos/exynos_hdmi.c     |   13 +++++++++
>  drivers/gpu/drm/exynos/exynos_mixer.c    |   13 +++++++++
>  8 files changed, 72 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
> index 7f9f6f9..3d5fa69 100644
> --- a/drivers/gpu/drm/exynos/Kconfig
> +++ b/drivers/gpu/drm/exynos/Kconfig
> @@ -25,7 +25,7 @@ config DRM_EXYNOS_DMABUF
>  	  Choose this option if you want to use DMABUF feature for DRM.
>  
>  config DRM_EXYNOS_FIMD
> -	bool "Exynos DRM FIMD"
> +	tristate "Exynos DRM FIMD"
>  	depends on DRM_EXYNOS && !FB_S3C
>  	select FB_MODE_HELPERS
>  	select MFD_SYSCON
> @@ -41,7 +41,7 @@ config DRM_EXYNOS_DPI
>  	  This enables support for Exynos parallel output.
>  
>  config DRM_EXYNOS_DSI
> -	bool "EXYNOS DRM MIPI-DSI driver support"
> +	tristate "EXYNOS DRM MIPI-DSI driver support"
>  	depends on DRM_EXYNOS_FIMD
>  	select DRM_MIPI_DSI
>  	select DRM_PANEL
> @@ -50,7 +50,7 @@ config DRM_EXYNOS_DSI
>  	  This enables support for Exynos MIPI-DSI device.
>  
>  config DRM_EXYNOS_DP
> -	bool "EXYNOS DRM DP driver support"
> +	tristate "EXYNOS DRM DP driver support"
>  	depends on DRM_EXYNOS_FIMD && ARCH_EXYNOS && (DRM_PTN3460=n || DRM_PTN3460=y || DRM_PTN3460=DRM_EXYNOS)
>  	default DRM_EXYNOS
>  	select DRM_PANEL
> @@ -58,7 +58,7 @@ config DRM_EXYNOS_DP
>  	  This enables support for DP device.
>  
>  config DRM_EXYNOS_HDMI
> -	bool "Exynos DRM HDMI"
> +	tristate "Exynos DRM HDMI"
>  	depends on DRM_EXYNOS && !VIDEO_SAMSUNG_S5P_TV
>  	help
>  	  Choose this option if you want to use Exynos HDMI for DRM.


Without corresponding changes in Makefiles you cannot build it as modules.


> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index ed818b9..b08d97b 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -1408,6 +1408,19 @@ struct platform_driver dp_driver = {
>  	},
>  };
>  
> +static int exynos_dp_init(void)
> +{
> +	return platform_driver_register(&dp_driver);
> +}
> +
> +static void exynos_dp_exit(void)
> +{
> +	platform_driver_unregister(&dp_driver);
> +}
> +
> +module_init(exynos_dp_init);
> +module_exit(exynos_dp_exit);
> +

Here and in other modules you can use module_platform_driver macro.

>  MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
>  MODULE_DESCRIPTION("Samsung SoC DP Driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index eab12f0..02d4772 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -484,12 +484,6 @@ static struct component_match *exynos_drm_match_add(struct device *dev)
>  
>  	mutex_lock(&drm_component_lock);
>  
> -	/* Do not retry to probe if there is no any kms driver regitered. */
> -	if (list_empty(&drm_component_list)) {
> -		mutex_unlock(&drm_component_lock);
> -		return ERR_PTR(-ENODEV);
> -	}
> -

It is hard to guess how and why it should work with modules.
For example what should happen if fimd/dsi modules are loaded but
hdmi/mixer not. When exynos_drv should wait with initialization for hdmi
and when it should create drmdev.

DRM system is rather monolithic splitting its components to modules
seems strange.

Regards
Andrzej

>  	list_for_each_entry(cdev, &drm_component_list, list) {
>  		/*
>  		 * Add components to master only in case that crtc and
> @@ -545,22 +539,6 @@ static const struct component_master_ops exynos_drm_ops = {
>  	.unbind		= exynos_drm_unbind,
>  };
>  
> -static struct platform_driver *const exynos_drm_kms_drivers[] = {
> -#ifdef CONFIG_DRM_EXYNOS_FIMD
> -	&fimd_driver,
> -#endif
> -#ifdef CONFIG_DRM_EXYNOS_DP
> -	&dp_driver,
> -#endif
> -#ifdef CONFIG_DRM_EXYNOS_DSI
> -	&dsi_driver,
> -#endif
> -#ifdef CONFIG_DRM_EXYNOS_HDMI
> -	&mixer_driver,
> -	&hdmi_driver,
> -#endif
> -};
> -
>  static struct platform_driver *const exynos_drm_non_kms_drivers[] = {
>  #ifdef CONFIG_DRM_EXYNOS_G2D
>  	&g2d_driver,
> @@ -587,22 +565,14 @@ static int exynos_drm_platform_probe(struct platform_device *pdev)
>  	pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
>  	exynos_drm_driver.num_ioctls = ARRAY_SIZE(exynos_ioctls);
>  
> -	for (i = 0; i < ARRAY_SIZE(exynos_drm_kms_drivers); ++i) {
> -		ret = platform_driver_register(exynos_drm_kms_drivers[i]);
> -		if (ret < 0)
> -			goto err_unregister_kms_drivers;
> -	}
> -
>  	match = exynos_drm_match_add(&pdev->dev);
> -	if (IS_ERR(match)) {
> -		ret = PTR_ERR(match);
> -		goto err_unregister_kms_drivers;
> -	}
> +	if (IS_ERR(match))
> +		return PTR_ERR(match);
>  
>  	ret = component_master_add_with_match(&pdev->dev, &exynos_drm_ops,
>  						match);
>  	if (ret < 0)
> -		goto err_unregister_kms_drivers;
> +		return ret;
>  
>  	for (j = 0; j < ARRAY_SIZE(exynos_drm_non_kms_drivers); ++j) {
>  		ret = platform_driver_register(exynos_drm_non_kms_drivers[j]);
> @@ -632,10 +602,6 @@ err_unregister_non_kms_drivers:
>  err_del_component_master:
>  	component_master_del(&pdev->dev, &exynos_drm_ops);
>  
> -err_unregister_kms_drivers:
> -	while (--i >= 0)
> -		platform_driver_unregister(exynos_drm_kms_drivers[i]);
> -
>  	return ret;
>  }
>  
> @@ -654,9 +620,6 @@ static int exynos_drm_platform_remove(struct platform_device *pdev)
>  
>  	component_master_del(&pdev->dev, &exynos_drm_ops);
>  
> -	for (i = ARRAY_SIZE(exynos_drm_kms_drivers) - 1; i >= 0; --i)
> -		platform_driver_unregister(exynos_drm_kms_drivers[i]);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 262a459..352a9f9 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -331,11 +331,6 @@ int exynos_drm_component_add(struct device *dev,
>  void exynos_drm_component_del(struct device *dev,
>  				enum exynos_drm_device_type dev_type);
>  
> -extern struct platform_driver fimd_driver;
> -extern struct platform_driver dp_driver;
> -extern struct platform_driver dsi_driver;
> -extern struct platform_driver mixer_driver;
> -extern struct platform_driver hdmi_driver;
>  extern struct platform_driver exynos_drm_common_hdmi_driver;
>  extern struct platform_driver vidi_driver;
>  extern struct platform_driver g2d_driver;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 66d427e..ebc3383 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -1799,6 +1799,19 @@ struct platform_driver dsi_driver = {
>  	},
>  };
>  
> +static int exynos_dsi_driver_init(void)
> +{
> +	return platform_driver_register(&dsi_driver);
> +}
> +
> +static void exynos_dsi_driver_exit(void)
> +{
> +	platform_driver_unregister(&dsi_driver);
> +}
> +
> +module_init(exynos_dsi_driver_init);
> +module_exit(exynos_dsi_driver_exit);
> +
>  MODULE_AUTHOR("Tomasz Figa <t.figa@samsung.com>");
>  MODULE_AUTHOR("Andrzej Hajda <a.hajda@samsung.com>");
>  MODULE_DESCRIPTION("Samsung SoC MIPI DSI Master");
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 0673a39..eed51c6 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -1240,3 +1240,16 @@ struct platform_driver fimd_driver = {
>  		.of_match_table = fimd_driver_dt_match,
>  	},
>  };
> +
> +static int exynos_drm_fimd_init(void)
> +{
> +	return platform_driver_register(&fimd_driver);
> +}
> +
> +static void exynos_drm_fimd_exit(void)
> +{
> +	platform_driver_unregister(&fimd_driver);
> +}
> +
> +module_init(exynos_drm_fimd_init);
> +module_exit(exynos_drm_fimd_exit);
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index 563a19e..83198f7 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -2537,3 +2537,16 @@ struct platform_driver hdmi_driver = {
>  		.of_match_table = hdmi_match_types,
>  	},
>  };
> +
> +static int hdmi_init(void)
> +{
> +	return platform_driver_register(&hdmi_driver);
> +}
> +
> +static void hdmi_exit(void)
> +{
> +	platform_driver_unregister(&hdmi_driver);
> +}
> +
> +module_init(hdmi_init);
> +module_exit(hdmi_exit);
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index a41c84e..4d812f1 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -1349,3 +1349,16 @@ struct platform_driver mixer_driver = {
>  	.remove = mixer_remove,
>  	.id_table	= mixer_driver_types,
>  };
> +
> +static int mixer_init(void)
> +{
> +	return platform_driver_register(&mixer_driver);
> +}
> +
> +static void mixer_exit(void)
> +{
> +	platform_driver_unregister(&mixer_driver);
> +}
> +
> +module_init(mixer_init);
> +module_exit(mixer_exit);
> 

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