[PATCH v2 00/13] drm/msm: move KMS code from msm_drv.c

[PATCH v2 00/13] drm/msm: move KMS code from msm_drv.c

[Dmitry Baryshkov]

Currently both msm_drm_init() and msm_drm_uninit() functions are trying
to handle both normal and headless Adreno cases. This results in a
suboptimal code, since headless case still gets modesetting and atomic
interfaces enabled. Two mentioned functions are a spaghetti of
`if (priv->kms)' conditional code.

Move all KMS-related code (not limiting the init / teardown path) from
msm_drv.c to msm_kms.c, making it more self-contained. This also
disables ATOMIC and MODESET features for the headless case.

Dependencies: [1]
[1] https://patchwork.freedesktop.org/series/105392/

Changes since v1:

- Rebased on top of linux-next / updated version of [1]

Dmitry Baryshkov (13):
  drm/msm/dsi: switch to devm_drm_bridge_add()
  drm/msm/hdmi: switch to devm_drm_bridge_add()
  drm/msm/dp: move pdev from struct dp_display_private to struct msm_dp
  drm/msm/dp: switch to devm_drm_bridge_add()
  drm/msm: remove msm_drm_private::bridges field
  drm/msm: drop pm ops from the headless msm driver
  drm/msm: rename msm_pm_prepare/complete to note the KMS nature
  drm/msm: remove shutdown callback from msm_platform_driver
  drm/msm: rename msm_drv_shutdown() to msm_kms_shutdown()
  drm/msm: switch to drmm_mode_config_init()
  drm/msm: only register 'kms' debug file if KMS is used
  drm/msm: make fb debugfs file available only in KMS case
  drm/msm: carve out KMS code from msm_drv.c

 drivers/gpu/drm/msm/Makefile             |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |   6 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |   6 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |   6 +-
 drivers/gpu/drm/msm/dp/dp_display.c      |  34 +--
 drivers/gpu/drm/msm/dp/dp_display.h      |   1 +
 drivers/gpu/drm/msm/dp/dp_drm.c          |  21 +-
 drivers/gpu/drm/msm/dp/dp_drm.h          |   2 +-
 drivers/gpu/drm/msm/dsi/dsi.c            |  28 +-
 drivers/gpu/drm/msm/dsi/dsi.h            |   3 +-
 drivers/gpu/drm/msm/dsi/dsi_manager.c    |  30 +-
 drivers/gpu/drm/msm/hdmi/hdmi.c          |  22 +-
 drivers/gpu/drm/msm/hdmi/hdmi.h          |   5 +-
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c   |  30 +-
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c      |   3 +-
 drivers/gpu/drm/msm/msm_debugfs.c        |  12 +-
 drivers/gpu/drm/msm/msm_drv.c            | 362 ++---------------------
 drivers/gpu/drm/msm/msm_drv.h            |   9 +-
 drivers/gpu/drm/msm/msm_kms.c            | 345 +++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h            |   3 +
 20 files changed, 451 insertions(+), 478 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_kms.c

-- 
2.39.2

[PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

Make MSM DSI driver use devm_drm_bridge_add() instead of plain
drm_bridge_add(). As the driver doesn't require any additional cleanup,
stop adding created bridge to the priv->bridges array.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
 drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++------------------
 3 files changed, 16 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index d45e43024802..47f327e68471 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
 int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 			 struct drm_encoder *encoder)
 {
-	struct msm_drm_private *priv = dev->dev_private;
 	int ret;
 
-	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
-		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
-		return -ENOSPC;
-	}
-
 	msm_dsi->dev = dev;
 
 	ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev, "failed to modeset init host: %d\n", ret);
-		goto fail;
+		return ret;
 	}
 
 	if (msm_dsi_is_bonded_dsi(msm_dsi) &&
@@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
 
 	msm_dsi->encoder = encoder;
 
-	msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
-	if (IS_ERR(msm_dsi->bridge)) {
-		ret = PTR_ERR(msm_dsi->bridge);
+	ret = msm_dsi_manager_bridge_init(msm_dsi);
+	if (ret) {
 		DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: %d\n", ret);
-		msm_dsi->bridge = NULL;
-		goto fail;
+		return ret;
 	}
 
 	ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
 	if (ret) {
 		DRM_DEV_ERROR(dev->dev,
 			"failed to create dsi connector: %d\n", ret);
-		goto fail;
+		return ret;
 	}
 
-	priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
-
 	return 0;
-fail:
-	/* bridge/connector are normally destroyed by drm: */
-	if (msm_dsi->bridge) {
-		msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
-		msm_dsi->bridge = NULL;
-	}
-
-	return ret;
 }
 
 void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index d21867da78b8..a01c326774a6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -56,8 +56,7 @@ struct msm_dsi {
 };
 
 /* dsi manager */
-struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
-void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
+int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
 int msm_dsi_manager_ext_bridge_init(u8 id);
 int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
 bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 28b8012a21f2..17aa19bb6510 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -466,9 +466,8 @@ static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
 };
 
 /* initialize bridge */
-struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
+int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
 {
-	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct drm_bridge *bridge = NULL;
 	struct dsi_bridge *dsi_bridge;
 	struct drm_encoder *encoder;
@@ -476,31 +475,27 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
 
 	dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
 				sizeof(*dsi_bridge), GFP_KERNEL);
-	if (!dsi_bridge) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!dsi_bridge)
+		return -ENOMEM;
 
-	dsi_bridge->id = id;
+	dsi_bridge->id = msm_dsi->id;
 
 	encoder = msm_dsi->encoder;
 
 	bridge = &dsi_bridge->base;
 	bridge->funcs = &dsi_mgr_bridge_funcs;
 
-	drm_bridge_add(bridge);
+	ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
+	if (ret)
+		return ret;
 
 	ret = drm_bridge_attach(encoder, bridge, NULL, 0);
 	if (ret)
-		goto fail;
+		return ret;
 
-	return bridge;
+	msm_dsi->bridge = bridge;
 
-fail:
-	if (bridge)
-		msm_dsi_manager_bridge_destroy(bridge);
-
-	return ERR_PTR(ret);
+	return 0;
 }
 
 int msm_dsi_manager_ext_bridge_init(u8 id)
@@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
 	return 0;
 }
 
-void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
-{
-	drm_bridge_remove(bridge);
-}
-
 int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
 {
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
-- 
2.39.2

Re: [PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> Make MSM DSI driver use devm_drm_bridge_add() instead of plain
> drm_bridge_add(). As the driver doesn't require any additional cleanup,
> stop adding created bridge to the priv->bridges array.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++------------------
>   3 files changed, 16 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
> index d45e43024802..47f327e68471 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
> @@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>   			 struct drm_encoder *encoder)
>   {
> -	struct msm_drm_private *priv = dev->dev_private;
>   	int ret;
>   
> -	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> -		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> -		return -ENOSPC;
> -	}
> -
>   	msm_dsi->dev = dev;
>   
>   	ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev->dev, "failed to modeset init host: %d\n", ret);
> -		goto fail;
> +		return ret;
>   	}
>   
>   	if (msm_dsi_is_bonded_dsi(msm_dsi) &&
> @@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev,
>   
>   	msm_dsi->encoder = encoder;
>   
> -	msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
> -	if (IS_ERR(msm_dsi->bridge)) {
> -		ret = PTR_ERR(msm_dsi->bridge);
> +	ret = msm_dsi_manager_bridge_init(msm_dsi);
> +	if (ret) {
>   		DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: %d\n", ret);
> -		msm_dsi->bridge = NULL;
> -		goto fail;
> +		return ret;
>   	}
>   
>   	ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev->dev,
>   			"failed to create dsi connector: %d\n", ret);
> -		goto fail;
> +		return ret;
>   	}
>   
> -	priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
> -
>   	return 0;
> -fail:
> -	/* bridge/connector are normally destroyed by drm: */
> -	if (msm_dsi->bridge) {
> -		msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
> -		msm_dsi->bridge = NULL;
> -	}

We can drop msm_dsi_manager_bridge_destroy() now but dont we need to 
keep the part to reset msm_dsi->bridge to NULL in the fail tag if 
msm_dsi_manager_ext_bridge_init() fails?

> -
> -	return ret;
>   }
>   
>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct msm_dsi *msm_dsi)
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index d21867da78b8..a01c326774a6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -56,8 +56,7 @@ struct msm_dsi {
>   };
>   
>   /* dsi manager */
> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
>   int msm_dsi_manager_ext_bridge_init(u8 id);
>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
>   bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 28b8012a21f2..17aa19bb6510 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -466,9 +466,8 @@ static const struct drm_bridge_funcs dsi_mgr_bridge_funcs = {
>   };
>   
>   /* initialize bridge */
> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
>   {
> -	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>   	struct drm_bridge *bridge = NULL;
>   	struct dsi_bridge *dsi_bridge;
>   	struct drm_encoder *encoder;
> @@ -476,31 +475,27 @@ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>   
>   	dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
>   				sizeof(*dsi_bridge), GFP_KERNEL);
> -	if (!dsi_bridge) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (!dsi_bridge)
> +		return -ENOMEM;
>   
> -	dsi_bridge->id = id;
> +	dsi_bridge->id = msm_dsi->id;
>   
>   	encoder = msm_dsi->encoder;
>   
>   	bridge = &dsi_bridge->base;
>   	bridge->funcs = &dsi_mgr_bridge_funcs;
>   
> -	drm_bridge_add(bridge);
> +	ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
> +	if (ret)
> +		return ret;
>   
>   	ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
> -	return bridge;
> +	msm_dsi->bridge = bridge;
>   
> -fail:
> -	if (bridge)
> -		msm_dsi_manager_bridge_destroy(bridge);
> -
> -	return ERR_PTR(ret);
> +	return 0;
>   }
>   
>   int msm_dsi_manager_ext_bridge_init(u8 id)
> @@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
>   	return 0;
>   }
>   
> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
> -{
> -	drm_bridge_remove(bridge);
> -}
> -
>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
>   {
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);

Re: [PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

On 09/10/2023 21:39, Abhinav Kumar wrote:
> 
> 
> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>> Make MSM DSI driver use devm_drm_bridge_add() instead of plain
>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>> stop adding created bridge to the priv->bridges array.
>>
>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
>>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++------------------
>>   3 files changed, 16 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>> b/drivers/gpu/drm/msm/dsi/dsi.c
>> index d45e43024802..47f327e68471 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>> @@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
>>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device 
>> *dev,
>>                struct drm_encoder *encoder)
>>   {
>> -    struct msm_drm_private *priv = dev->dev_private;
>>       int ret;
>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>> -        return -ENOSPC;
>> -    }
>> -
>>       msm_dsi->dev = dev;
>>       ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
>>       if (ret) {
>>           DRM_DEV_ERROR(dev->dev, "failed to modeset init host: %d\n", 
>> ret);
>> -        goto fail;
>> +        return ret;
>>       }
>>       if (msm_dsi_is_bonded_dsi(msm_dsi) &&
>> @@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi 
>> *msm_dsi, struct drm_device *dev,
>>       msm_dsi->encoder = encoder;
>> -    msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
>> -    if (IS_ERR(msm_dsi->bridge)) {
>> -        ret = PTR_ERR(msm_dsi->bridge);
>> +    ret = msm_dsi_manager_bridge_init(msm_dsi);
>> +    if (ret) {
>>           DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: %d\n", 
>> ret);
>> -        msm_dsi->bridge = NULL;
>> -        goto fail;
>> +        return ret;
>>       }
>>       ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
>>       if (ret) {
>>           DRM_DEV_ERROR(dev->dev,
>>               "failed to create dsi connector: %d\n", ret);
>> -        goto fail;
>> +        return ret;
>>       }
>> -    priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
>> -
>>       return 0;
>> -fail:
>> -    /* bridge/connector are normally destroyed by drm: */
>> -    if (msm_dsi->bridge) {
>> -        msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>> -        msm_dsi->bridge = NULL;
>> -    }
> 
> We can drop msm_dsi_manager_bridge_destroy() now but dont we need to 
> keep the part to reset msm_dsi->bridge to NULL in the fail tag if 
> msm_dsi_manager_ext_bridge_init() fails?

What for? This field is not read in the error /unbinding path.
I'll send a followup that drops msm_dsi->bridge completely.

> 
>> -
>> -    return ret;
>>   }
>>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct 
>> msm_dsi *msm_dsi)
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.h
>> index d21867da78b8..a01c326774a6 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -56,8 +56,7 @@ struct msm_dsi {
>>   };
>>   /* dsi manager */
>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
>>   int msm_dsi_manager_ext_bridge_init(u8 id);
>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
>>   bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> index 28b8012a21f2..17aa19bb6510 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> @@ -466,9 +466,8 @@ static const struct drm_bridge_funcs 
>> dsi_mgr_bridge_funcs = {
>>   };
>>   /* initialize bridge */
>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
>>   {
>> -    struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>       struct drm_bridge *bridge = NULL;
>>       struct dsi_bridge *dsi_bridge;
>>       struct drm_encoder *encoder;
>> @@ -476,31 +475,27 @@ struct drm_bridge 
>> *msm_dsi_manager_bridge_init(u8 id)
>>       dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
>>                   sizeof(*dsi_bridge), GFP_KERNEL);
>> -    if (!dsi_bridge) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> +    if (!dsi_bridge)
>> +        return -ENOMEM;
>> -    dsi_bridge->id = id;
>> +    dsi_bridge->id = msm_dsi->id;
>>       encoder = msm_dsi->encoder;
>>       bridge = &dsi_bridge->base;
>>       bridge->funcs = &dsi_mgr_bridge_funcs;
>> -    drm_bridge_add(bridge);
>> +    ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
>> +    if (ret)
>> +        return ret;
>>       ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>       if (ret)
>> -        goto fail;
>> +        return ret;
>> -    return bridge;
>> +    msm_dsi->bridge = bridge;
>> -fail:
>> -    if (bridge)
>> -        msm_dsi_manager_bridge_destroy(bridge);
>> -
>> -    return ERR_PTR(ret);
>> +    return 0;
>>   }
>>   int msm_dsi_manager_ext_bridge_init(u8 id)
>> @@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
>>       return 0;
>>   }
>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
>> -{
>> -    drm_bridge_remove(bridge);
>> -}
>> -
>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
>>   {
>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);

-- 
With best wishes
Dmitry

Re: [PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 11:46 AM, Dmitry Baryshkov wrote:
> On 09/10/2023 21:39, Abhinav Kumar wrote:
>>
>>
>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>> Make MSM DSI driver use devm_drm_bridge_add() instead of plain
>>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>>> stop adding created bridge to the priv->bridges array.
>>>
>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
>>>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 +++++++++------------------
>>>   3 files changed, 16 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi.c
>>> index d45e43024802..47f327e68471 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>>> @@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
>>>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device 
>>> *dev,
>>>                struct drm_encoder *encoder)
>>>   {
>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>       int ret;
>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>> -        return -ENOSPC;
>>> -    }
>>> -
>>>       msm_dsi->dev = dev;
>>>       ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
>>>       if (ret) {
>>>           DRM_DEV_ERROR(dev->dev, "failed to modeset init host: 
>>> %d\n", ret);
>>> -        goto fail;
>>> +        return ret;
>>>       }
>>>       if (msm_dsi_is_bonded_dsi(msm_dsi) &&
>>> @@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi 
>>> *msm_dsi, struct drm_device *dev,
>>>       msm_dsi->encoder = encoder;
>>> -    msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
>>> -    if (IS_ERR(msm_dsi->bridge)) {
>>> -        ret = PTR_ERR(msm_dsi->bridge);
>>> +    ret = msm_dsi_manager_bridge_init(msm_dsi);
>>> +    if (ret) {
>>>           DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: 
>>> %d\n", ret);
>>> -        msm_dsi->bridge = NULL;
>>> -        goto fail;
>>> +        return ret;
>>>       }
>>>       ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
>>>       if (ret) {
>>>           DRM_DEV_ERROR(dev->dev,
>>>               "failed to create dsi connector: %d\n", ret);
>>> -        goto fail;
>>> +        return ret;
>>>       }
>>> -    priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
>>> -
>>>       return 0;
>>> -fail:
>>> -    /* bridge/connector are normally destroyed by drm: */
>>> -    if (msm_dsi->bridge) {
>>> -        msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>>> -        msm_dsi->bridge = NULL;
>>> -    }
>>
>> We can drop msm_dsi_manager_bridge_destroy() now but dont we need to 
>> keep the part to reset msm_dsi->bridge to NULL in the fail tag if 
>> msm_dsi_manager_ext_bridge_init() fails?
> 
> What for? This field is not read in the error /unbinding path.
> I'll send a followup that drops msm_dsi->bridge completely.
> 

Not used in the error path. The behavior before this patch was, if 
msm_dsi_manager_ext_bridge_init failed, it was marking msm_dsi->bridge 
as NULL. Thats what I thought you would want to retain till you drop the 
msm_dsi->bridge.

OR you can even add that line in the if (ret) of 
msm_dsi_manager_ext_bridge_init(msm_dsi->id); failure.

>>
>>> -
>>> -    return ret;
>>>   }
>>>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct 
>>> msm_dsi *msm_dsi)
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>> index d21867da78b8..a01c326774a6 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>> @@ -56,8 +56,7 @@ struct msm_dsi {
>>>   };
>>>   /* dsi manager */
>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
>>>   int msm_dsi_manager_ext_bridge_init(u8 id);
>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
>>>   bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 28b8012a21f2..17aa19bb6510 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -466,9 +466,8 @@ static const struct drm_bridge_funcs 
>>> dsi_mgr_bridge_funcs = {
>>>   };
>>>   /* initialize bridge */
>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
>>>   {
>>> -    struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>       struct drm_bridge *bridge = NULL;
>>>       struct dsi_bridge *dsi_bridge;
>>>       struct drm_encoder *encoder;
>>> @@ -476,31 +475,27 @@ struct drm_bridge 
>>> *msm_dsi_manager_bridge_init(u8 id)
>>>       dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
>>>                   sizeof(*dsi_bridge), GFP_KERNEL);
>>> -    if (!dsi_bridge) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> +    if (!dsi_bridge)
>>> +        return -ENOMEM;
>>> -    dsi_bridge->id = id;
>>> +    dsi_bridge->id = msm_dsi->id;
>>>       encoder = msm_dsi->encoder;
>>>       bridge = &dsi_bridge->base;
>>>       bridge->funcs = &dsi_mgr_bridge_funcs;
>>> -    drm_bridge_add(bridge);
>>> +    ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
>>> +    if (ret)
>>> +        return ret;
>>>       ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>>       if (ret)
>>> -        goto fail;
>>> +        return ret;
>>> -    return bridge;
>>> +    msm_dsi->bridge = bridge;
>>> -fail:
>>> -    if (bridge)
>>> -        msm_dsi_manager_bridge_destroy(bridge);
>>> -
>>> -    return ERR_PTR(ret);
>>> +    return 0;
>>>   }
>>>   int msm_dsi_manager_ext_bridge_init(u8 id)
>>> @@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
>>>       return 0;
>>>   }
>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
>>> -{
>>> -    drm_bridge_remove(bridge);
>>> -}
>>> -
>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
>>>   {
>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
> 

Re: [PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

On 09/10/2023 21:51, Abhinav Kumar wrote:
> 
> 
> On 10/9/2023 11:46 AM, Dmitry Baryshkov wrote:
>> On 09/10/2023 21:39, Abhinav Kumar wrote:
>>>
>>>
>>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>>> Make MSM DSI driver use devm_drm_bridge_add() instead of plain
>>>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>>>> stop adding created bridge to the priv->bridges array.
>>>>
>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
>>>>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 
>>>> +++++++++------------------
>>>>   3 files changed, 16 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi.c
>>>> index d45e43024802..47f327e68471 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>>>> @@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
>>>>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct 
>>>> drm_device *dev,
>>>>                struct drm_encoder *encoder)
>>>>   {
>>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>>       int ret;
>>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>>> -        return -ENOSPC;
>>>> -    }
>>>> -
>>>>       msm_dsi->dev = dev;
>>>>       ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
>>>>       if (ret) {
>>>>           DRM_DEV_ERROR(dev->dev, "failed to modeset init host: 
>>>> %d\n", ret);
>>>> -        goto fail;
>>>> +        return ret;
>>>>       }
>>>>       if (msm_dsi_is_bonded_dsi(msm_dsi) &&
>>>> @@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi 
>>>> *msm_dsi, struct drm_device *dev,
>>>>       msm_dsi->encoder = encoder;
>>>> -    msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
>>>> -    if (IS_ERR(msm_dsi->bridge)) {
>>>> -        ret = PTR_ERR(msm_dsi->bridge);
>>>> +    ret = msm_dsi_manager_bridge_init(msm_dsi);
>>>> +    if (ret) {
>>>>           DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: 
>>>> %d\n", ret);
>>>> -        msm_dsi->bridge = NULL;
>>>> -        goto fail;
>>>> +        return ret;
>>>>       }
>>>>       ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
>>>>       if (ret) {
>>>>           DRM_DEV_ERROR(dev->dev,
>>>>               "failed to create dsi connector: %d\n", ret);
>>>> -        goto fail;
>>>> +        return ret;
>>>>       }
>>>> -    priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
>>>> -
>>>>       return 0;
>>>> -fail:
>>>> -    /* bridge/connector are normally destroyed by drm: */
>>>> -    if (msm_dsi->bridge) {
>>>> -        msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>>>> -        msm_dsi->bridge = NULL;
>>>> -    }
>>>
>>> We can drop msm_dsi_manager_bridge_destroy() now but dont we need to 
>>> keep the part to reset msm_dsi->bridge to NULL in the fail tag if 
>>> msm_dsi_manager_ext_bridge_init() fails?
>>
>> What for? This field is not read in the error /unbinding path.
>> I'll send a followup that drops msm_dsi->bridge completely.
>>
> 
> Not used in the error path. The behavior before this patch was, if 
> msm_dsi_manager_ext_bridge_init failed, it was marking msm_dsi->bridge 
> as NULL. Thats what I thought you would want to retain till you drop the 
> msm_dsi->bridge.

Why would you like to keep the useless behaviour?

> OR you can even add that line in the if (ret) of 
> msm_dsi_manager_ext_bridge_init(msm_dsi->id); failure.
> 
>>>
>>>> -
>>>> -    return ret;
>>>>   }
>>>>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct 
>>>> msm_dsi *msm_dsi)
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> index d21867da78b8..a01c326774a6 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> @@ -56,8 +56,7 @@ struct msm_dsi {
>>>>   };
>>>>   /* dsi manager */
>>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
>>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
>>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
>>>>   int msm_dsi_manager_ext_bridge_init(u8 id);
>>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg);
>>>>   bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len);
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> index 28b8012a21f2..17aa19bb6510 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> @@ -466,9 +466,8 @@ static const struct drm_bridge_funcs 
>>>> dsi_mgr_bridge_funcs = {
>>>>   };
>>>>   /* initialize bridge */
>>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
>>>>   {
>>>> -    struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>       struct drm_bridge *bridge = NULL;
>>>>       struct dsi_bridge *dsi_bridge;
>>>>       struct drm_encoder *encoder;
>>>> @@ -476,31 +475,27 @@ struct drm_bridge 
>>>> *msm_dsi_manager_bridge_init(u8 id)
>>>>       dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
>>>>                   sizeof(*dsi_bridge), GFP_KERNEL);
>>>> -    if (!dsi_bridge) {
>>>> -        ret = -ENOMEM;
>>>> -        goto fail;
>>>> -    }
>>>> +    if (!dsi_bridge)
>>>> +        return -ENOMEM;
>>>> -    dsi_bridge->id = id;
>>>> +    dsi_bridge->id = msm_dsi->id;
>>>>       encoder = msm_dsi->encoder;
>>>>       bridge = &dsi_bridge->base;
>>>>       bridge->funcs = &dsi_mgr_bridge_funcs;
>>>> -    drm_bridge_add(bridge);
>>>> +    ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
>>>> +    if (ret)
>>>> +        return ret;
>>>>       ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>>>       if (ret)
>>>> -        goto fail;
>>>> +        return ret;
>>>> -    return bridge;
>>>> +    msm_dsi->bridge = bridge;
>>>> -fail:
>>>> -    if (bridge)
>>>> -        msm_dsi_manager_bridge_destroy(bridge);
>>>> -
>>>> -    return ERR_PTR(ret);
>>>> +    return 0;
>>>>   }
>>>>   int msm_dsi_manager_ext_bridge_init(u8 id)
>>>> @@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
>>>>       return 0;
>>>>   }
>>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
>>>> -{
>>>> -    drm_bridge_remove(bridge);
>>>> -}
>>>> -
>>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
>>>>   {
>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>

-- 
With best wishes
Dmitry

Re: [PATCH v2 01/13] drm/msm/dsi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 12:01 PM, Dmitry Baryshkov wrote:
> On 09/10/2023 21:51, Abhinav Kumar wrote:
>>
>>
>> On 10/9/2023 11:46 AM, Dmitry Baryshkov wrote:
>>> On 09/10/2023 21:39, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>>>> Make MSM DSI driver use devm_drm_bridge_add() instead of plain
>>>>> drm_bridge_add(). As the driver doesn't require any additional 
>>>>> cleanup,
>>>>> stop adding created bridge to the priv->bridges array.
>>>>>
>>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi.c         | 28 +++++--------------------
>>>>>   drivers/gpu/drm/msm/dsi/dsi.h         |  3 +--
>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 30 
>>>>> +++++++++------------------
>>>>>   3 files changed, 16 insertions(+), 45 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.c 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi.c
>>>>> index d45e43024802..47f327e68471 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.c
>>>>> @@ -215,20 +215,14 @@ void __exit msm_dsi_unregister(void)
>>>>>   int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct 
>>>>> drm_device *dev,
>>>>>                struct drm_encoder *encoder)
>>>>>   {
>>>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>>>       int ret;
>>>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>>>> -        return -ENOSPC;
>>>>> -    }
>>>>> -
>>>>>       msm_dsi->dev = dev;
>>>>>       ret = msm_dsi_host_modeset_init(msm_dsi->host, dev);
>>>>>       if (ret) {
>>>>>           DRM_DEV_ERROR(dev->dev, "failed to modeset init host: 
>>>>> %d\n", ret);
>>>>> -        goto fail;
>>>>> +        return ret;
>>>>>       }
>>>>>       if (msm_dsi_is_bonded_dsi(msm_dsi) &&
>>>>> @@ -242,32 +236,20 @@ int msm_dsi_modeset_init(struct msm_dsi 
>>>>> *msm_dsi, struct drm_device *dev,
>>>>>       msm_dsi->encoder = encoder;
>>>>> -    msm_dsi->bridge = msm_dsi_manager_bridge_init(msm_dsi->id);
>>>>> -    if (IS_ERR(msm_dsi->bridge)) {
>>>>> -        ret = PTR_ERR(msm_dsi->bridge);
>>>>> +    ret = msm_dsi_manager_bridge_init(msm_dsi);
>>>>> +    if (ret) {
>>>>>           DRM_DEV_ERROR(dev->dev, "failed to create dsi bridge: 
>>>>> %d\n", ret);
>>>>> -        msm_dsi->bridge = NULL;
>>>>> -        goto fail;
>>>>> +        return ret;
>>>>>       }
>>>>>       ret = msm_dsi_manager_ext_bridge_init(msm_dsi->id);
>>>>>       if (ret) {
>>>>>           DRM_DEV_ERROR(dev->dev,
>>>>>               "failed to create dsi connector: %d\n", ret);
>>>>> -        goto fail;
>>>>> +        return ret;
>>>>>       }
>>>>> -    priv->bridges[priv->num_bridges++]       = msm_dsi->bridge;
>>>>> -
>>>>>       return 0;
>>>>> -fail:
>>>>> -    /* bridge/connector are normally destroyed by drm: */
>>>>> -    if (msm_dsi->bridge) {
>>>>> -        msm_dsi_manager_bridge_destroy(msm_dsi->bridge);
>>>>> -        msm_dsi->bridge = NULL;
>>>>> -    }
>>>>
>>>> We can drop msm_dsi_manager_bridge_destroy() now but dont we need to 
>>>> keep the part to reset msm_dsi->bridge to NULL in the fail tag if 
>>>> msm_dsi_manager_ext_bridge_init() fails?
>>>
>>> What for? This field is not read in the error /unbinding path.
>>> I'll send a followup that drops msm_dsi->bridge completely.
>>>
>>
>> Not used in the error path. The behavior before this patch was, if 
>> msm_dsi_manager_ext_bridge_init failed, it was marking msm_dsi->bridge 
>> as NULL. Thats what I thought you would want to retain till you drop 
>> the msm_dsi->bridge.
> 
> Why would you like to keep the useless behaviour?
> 

Not sure whether to call it useless. So 
msm_dsi_manager_ext_bridge_init() is referencing msm_dsi->bridge.
msm_dsi_manager_ext_bridge_init() is an exported API. If someone tries 
to call it outside of where its called now, this was the only protection 
against the access?

I guess, you could counter argue that noone calls it that way today.

In that sense, I am fine with this, just wanted to report the gap after 
this patch before I ack.

>> OR you can even add that line in the if (ret) of 
>> msm_dsi_manager_ext_bridge_init(msm_dsi->id); failure.
>>
>>>>
>>>>> -
>>>>> -    return ret;
>>>>>   }
>>>>>   void msm_dsi_snapshot(struct msm_disp_state *disp_state, struct 
>>>>> msm_dsi *msm_dsi)
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> index d21867da78b8..a01c326774a6 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> @@ -56,8 +56,7 @@ struct msm_dsi {
>>>>>   };
>>>>>   /* dsi manager */
>>>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id);
>>>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge);
>>>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi);
>>>>>   int msm_dsi_manager_ext_bridge_init(u8 id);
>>>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg 
>>>>> *msg);
>>>>>   bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 
>>>>> len);
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> index 28b8012a21f2..17aa19bb6510 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> @@ -466,9 +466,8 @@ static const struct drm_bridge_funcs 
>>>>> dsi_mgr_bridge_funcs = {
>>>>>   };
>>>>>   /* initialize bridge */
>>>>> -struct drm_bridge *msm_dsi_manager_bridge_init(u8 id)
>>>>> +int msm_dsi_manager_bridge_init(struct msm_dsi *msm_dsi)
>>>>>   {
>>>>> -    struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>>       struct drm_bridge *bridge = NULL;
>>>>>       struct dsi_bridge *dsi_bridge;
>>>>>       struct drm_encoder *encoder;
>>>>> @@ -476,31 +475,27 @@ struct drm_bridge 
>>>>> *msm_dsi_manager_bridge_init(u8 id)
>>>>>       dsi_bridge = devm_kzalloc(msm_dsi->dev->dev,
>>>>>                   sizeof(*dsi_bridge), GFP_KERNEL);
>>>>> -    if (!dsi_bridge) {
>>>>> -        ret = -ENOMEM;
>>>>> -        goto fail;
>>>>> -    }
>>>>> +    if (!dsi_bridge)
>>>>> +        return -ENOMEM;
>>>>> -    dsi_bridge->id = id;
>>>>> +    dsi_bridge->id = msm_dsi->id;
>>>>>       encoder = msm_dsi->encoder;
>>>>>       bridge = &dsi_bridge->base;
>>>>>       bridge->funcs = &dsi_mgr_bridge_funcs;
>>>>> -    drm_bridge_add(bridge);
>>>>> +    ret = devm_drm_bridge_add(&msm_dsi->pdev->dev, bridge);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>>       ret = drm_bridge_attach(encoder, bridge, NULL, 0);
>>>>>       if (ret)
>>>>> -        goto fail;
>>>>> +        return ret;
>>>>> -    return bridge;
>>>>> +    msm_dsi->bridge = bridge;
>>>>> -fail:
>>>>> -    if (bridge)
>>>>> -        msm_dsi_manager_bridge_destroy(bridge);
>>>>> -
>>>>> -    return ERR_PTR(ret);
>>>>> +    return 0;
>>>>>   }
>>>>>   int msm_dsi_manager_ext_bridge_init(u8 id)
>>>>> @@ -557,11 +552,6 @@ int msm_dsi_manager_ext_bridge_init(u8 id)
>>>>>       return 0;
>>>>>   }
>>>>> -void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge)
>>>>> -{
>>>>> -    drm_bridge_remove(bridge);
>>>>> -}
>>>>> -
>>>>>   int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg)
>>>>>   {
>>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>
> 

[PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
drm_bridge_add(). As the driver doesn't require any additional cleanup,
stop adding created bridge to the priv->bridges array.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
 4 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index b6bcb9f675fe..c8ebd75176bb 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
 int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		struct drm_device *dev, struct drm_encoder *encoder)
 {
-	struct msm_drm_private *priv = dev->dev_private;
 	int ret;
 
-	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
-		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
-		return -ENOSPC;
-	}
-
 	hdmi->dev = dev;
 	hdmi->encoder = encoder;
 
 	hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
 
-	hdmi->bridge = msm_hdmi_bridge_init(hdmi);
-	if (IS_ERR(hdmi->bridge)) {
-		ret = PTR_ERR(hdmi->bridge);
+	ret = msm_hdmi_bridge_init(hdmi);
+	if (ret) {
 		DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n", ret);
-		hdmi->bridge = NULL;
 		goto fail;
 	}
 
@@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		goto fail;
 	}
 
-	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
-
 	return 0;
 
 fail:
-	/* bridge is normally destroyed by drm: */
-	if (hdmi->bridge) {
-		msm_hdmi_bridge_destroy(hdmi->bridge);
-		hdmi->bridge = NULL;
-	}
 	if (hdmi->connector) {
 		hdmi->connector->funcs->destroy(hdmi->connector);
 		hdmi->connector = NULL;
@@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
 		if (priv->hdmi->audio_pdev)
 			platform_device_unregister(priv->hdmi->audio_pdev);
 
+		if (priv->hdmi->bridge)
+			msm_hdmi_hpd_disable(priv->hdmi);
+
 		msm_hdmi_destroy(priv->hdmi);
 		priv->hdmi = NULL;
 	}
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index e8dbee50637f..ec5786440391 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi *hdmi, int rate);
  * hdmi bridge:
  */
 
-struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
-void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
+int msm_hdmi_bridge_init(struct hdmi *hdmi);
 
 void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
 enum drm_connector_status msm_hdmi_bridge_detect(
 		struct drm_bridge *bridge);
 int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
-void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
+void msm_hdmi_hpd_disable(struct hdmi *hdmi);
 
 /*
  * i2c adapter for ddc:
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9b1391d27ed3..0b7a6a56677e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -11,14 +11,6 @@
 #include "msm_kms.h"
 #include "hdmi.h"
 
-void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
-{
-	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
-
-	msm_hdmi_hpd_disable(hdmi_bridge);
-	drm_bridge_remove(bridge);
-}
-
 static void msm_hdmi_power_on(struct drm_bridge *bridge)
 {
 	struct drm_device *dev = bridge->dev;
@@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
 }
 
 /* initialize bridge */
-struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
+int msm_hdmi_bridge_init(struct hdmi *hdmi)
 {
 	struct drm_bridge *bridge = NULL;
 	struct hdmi_bridge *hdmi_bridge;
@@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
 
 	hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
 			sizeof(*hdmi_bridge), GFP_KERNEL);
-	if (!hdmi_bridge) {
-		ret = -ENOMEM;
-		goto fail;
-	}
+	if (!hdmi_bridge)
+		return -ENOMEM;
 
 	hdmi_bridge->hdmi = hdmi;
 	INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
@@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
 		DRM_BRIDGE_OP_DETECT |
 		DRM_BRIDGE_OP_EDID;
 
-	drm_bridge_add(bridge);
+	ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
+	if (ret)
+		return ret;
 
 	ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret)
-		goto fail;
+		return ret;
 
-	return bridge;
+	hdmi->bridge = bridge;
 
-fail:
-	if (bridge)
-		msm_hdmi_bridge_destroy(bridge);
-
-	return ERR_PTR(ret);
+	return 0;
 }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index bfa827b47989..9ce0ffa35417 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	return ret;
 }
 
-void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
+void msm_hdmi_hpd_disable(struct hdmi *hdmi)
 {
-	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	const struct hdmi_platform_config *config = hdmi->config;
 	struct device *dev = &hdmi->pdev->dev;
 	int ret;
-- 
2.39.2

Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
> drm_bridge_add(). As the driver doesn't require any additional cleanup,
> stop adding created bridge to the priv->bridges array.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>   4 files changed, 17 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index b6bcb9f675fe..c8ebd75176bb 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   		struct drm_device *dev, struct drm_encoder *encoder)
>   {
> -	struct msm_drm_private *priv = dev->dev_private;
>   	int ret;
>   
> -	if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> -		DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> -		return -ENOSPC;
> -	}
> -
>   	hdmi->dev = dev;
>   	hdmi->encoder = encoder;
>   
>   	hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>   
> -	hdmi->bridge = msm_hdmi_bridge_init(hdmi);
> -	if (IS_ERR(hdmi->bridge)) {
> -		ret = PTR_ERR(hdmi->bridge);
> +	ret = msm_hdmi_bridge_init(hdmi);
> +	if (ret) {
>   		DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n", ret);
> -		hdmi->bridge = NULL;
>   		goto fail;
>   	}
>   
> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   		goto fail;
>   	}
>   
> -	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
> -
>   	return 0;
>   
>   fail:
> -	/* bridge is normally destroyed by drm: */
> -	if (hdmi->bridge) {
> -		msm_hdmi_bridge_destroy(hdmi->bridge);
> -		hdmi->bridge = NULL;
> -	}
>   	if (hdmi->connector) {
>   		hdmi->connector->funcs->destroy(hdmi->connector);
>   		hdmi->connector = NULL;
> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
>   		if (priv->hdmi->audio_pdev)
>   			platform_device_unregister(priv->hdmi->audio_pdev);
>   
> +		if (priv->hdmi->bridge)
> +			msm_hdmi_hpd_disable(priv->hdmi);
> +

Now is this the only place where hdmi->bridge is used?

Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?

>   		msm_hdmi_destroy(priv->hdmi);
>   		priv->hdmi = NULL;
>   	}
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index e8dbee50637f..ec5786440391 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi *hdmi, int rate);
>    * hdmi bridge:
>    */
>   
> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
> +int msm_hdmi_bridge_init(struct hdmi *hdmi);
>   
>   void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
>   enum drm_connector_status msm_hdmi_bridge_detect(
>   		struct drm_bridge *bridge);
>   int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
> +void msm_hdmi_hpd_disable(struct hdmi *hdmi);
>   
>   /*
>    * i2c adapter for ddc:
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 9b1391d27ed3..0b7a6a56677e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -11,14 +11,6 @@
>   #include "msm_kms.h"
>   #include "hdmi.h"
>   
> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
> -{
> -	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> -
> -	msm_hdmi_hpd_disable(hdmi_bridge);
> -	drm_bridge_remove(bridge);
> -}
> -
>   static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   {
>   	struct drm_device *dev = bridge->dev;
> @@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
>   }
>   
>   /* initialize bridge */
> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
> +int msm_hdmi_bridge_init(struct hdmi *hdmi)
>   {
>   	struct drm_bridge *bridge = NULL;
>   	struct hdmi_bridge *hdmi_bridge;
> @@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
>   
>   	hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
>   			sizeof(*hdmi_bridge), GFP_KERNEL);
> -	if (!hdmi_bridge) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> +	if (!hdmi_bridge)
> +		return -ENOMEM;
>   
>   	hdmi_bridge->hdmi = hdmi;
>   	INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
> @@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
>   		DRM_BRIDGE_OP_DETECT |
>   		DRM_BRIDGE_OP_EDID;
>   
> -	drm_bridge_add(bridge);
> +	ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
> +	if (ret)
> +		return ret;
>   
>   	ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
> -	return bridge;
> +	hdmi->bridge = bridge;
>   
> -fail:
> -	if (bridge)
> -		msm_hdmi_bridge_destroy(bridge);
> -
> -	return ERR_PTR(ret);
> +	return 0;
>   }
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index bfa827b47989..9ce0ffa35417 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	return ret;
>   }
>   
> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
> +void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>   {
> -	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	const struct hdmi_platform_config *config = hdmi->config;
>   	struct device *dev = &hdmi->pdev->dev;
>   	int ret;

Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

On 09/10/2023 22:19, Abhinav Kumar wrote:
> 
> 
> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>> stop adding created bridge to the priv->bridges array.
>>
>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
>>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>>   4 files changed, 17 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index b6bcb9f675fe..c8ebd75176bb 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>           struct drm_device *dev, struct drm_encoder *encoder)
>>   {
>> -    struct msm_drm_private *priv = dev->dev_private;
>>       int ret;
>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>> -        return -ENOSPC;
>> -    }
>> -
>>       hdmi->dev = dev;
>>       hdmi->encoder = encoder;
>>       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>> -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
>> -    if (IS_ERR(hdmi->bridge)) {
>> -        ret = PTR_ERR(hdmi->bridge);
>> +    ret = msm_hdmi_bridge_init(hdmi);
>> +    if (ret) {
>>           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
>> %d\n", ret);
>> -        hdmi->bridge = NULL;
>>           goto fail;
>>       }
>> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>           goto fail;
>>       }
>> -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>> -
>>       return 0;
>>   fail:
>> -    /* bridge is normally destroyed by drm: */
>> -    if (hdmi->bridge) {
>> -        msm_hdmi_bridge_destroy(hdmi->bridge);
>> -        hdmi->bridge = NULL;
>> -    }
>>       if (hdmi->connector) {
>>           hdmi->connector->funcs->destroy(hdmi->connector);
>>           hdmi->connector = NULL;
>> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
>> struct device *master,
>>           if (priv->hdmi->audio_pdev)
>>               platform_device_unregister(priv->hdmi->audio_pdev);
>> +        if (priv->hdmi->bridge)
>> +            msm_hdmi_hpd_disable(priv->hdmi);
>> +
> 
> Now is this the only place where hdmi->bridge is used?
> 
> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
> anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?

Sure, sounds like a good idea, same followup as for the DSI.

> 
>>           msm_hdmi_destroy(priv->hdmi);
>>           priv->hdmi = NULL;
>>       }
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h 
>> b/drivers/gpu/drm/msm/hdmi/hdmi.h
>> index e8dbee50637f..ec5786440391 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
>> @@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi 
>> *hdmi, int rate);
>>    * hdmi bridge:
>>    */
>> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
>> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
>> +int msm_hdmi_bridge_init(struct hdmi *hdmi);
>>   void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
>>   enum drm_connector_status msm_hdmi_bridge_detect(
>>           struct drm_bridge *bridge);
>>   int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
>> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
>> +void msm_hdmi_hpd_disable(struct hdmi *hdmi);
>>   /*
>>    * i2c adapter for ddc:
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c 
>> b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> index 9b1391d27ed3..0b7a6a56677e 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>> @@ -11,14 +11,6 @@
>>   #include "msm_kms.h"
>>   #include "hdmi.h"
>> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
>> -{
>> -    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>> -
>> -    msm_hdmi_hpd_disable(hdmi_bridge);
>> -    drm_bridge_remove(bridge);
>> -}
>> -
>>   static void msm_hdmi_power_on(struct drm_bridge *bridge)
>>   {
>>       struct drm_device *dev = bridge->dev;
>> @@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
>>   }
>>   /* initialize bridge */
>> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
>> +int msm_hdmi_bridge_init(struct hdmi *hdmi)
>>   {
>>       struct drm_bridge *bridge = NULL;
>>       struct hdmi_bridge *hdmi_bridge;
>> @@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
>> hdmi *hdmi)
>>       hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
>>               sizeof(*hdmi_bridge), GFP_KERNEL);
>> -    if (!hdmi_bridge) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> +    if (!hdmi_bridge)
>> +        return -ENOMEM;
>>       hdmi_bridge->hdmi = hdmi;
>>       INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
>> @@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
>> hdmi *hdmi)
>>           DRM_BRIDGE_OP_DETECT |
>>           DRM_BRIDGE_OP_EDID;
>> -    drm_bridge_add(bridge);
>> +    ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
>> +    if (ret)
>> +        return ret;
>>       ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, 
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>       if (ret)
>> -        goto fail;
>> +        return ret;
>> -    return bridge;
>> +    hdmi->bridge = bridge;
>> -fail:
>> -    if (bridge)
>> -        msm_hdmi_bridge_destroy(bridge);
>> -
>> -    return ERR_PTR(ret);
>> +    return 0;
>>   }
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c 
>> b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>> index bfa827b47989..9ce0ffa35417 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>> @@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>>       return ret;
>>   }
>> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
>> +void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>>   {
>> -    struct hdmi *hdmi = hdmi_bridge->hdmi;
>>       const struct hdmi_platform_config *config = hdmi->config;
>>       struct device *dev = &hdmi->pdev->dev;
>>       int ret;

-- 
With best wishes
Dmitry

Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 12:21 PM, Dmitry Baryshkov wrote:
> On 09/10/2023 22:19, Abhinav Kumar wrote:
>>
>>
>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
>>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>>> stop adding created bridge to the priv->bridges array.
>>>
>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>>>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>>>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
>>>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>>>   4 files changed, 17 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> index b6bcb9f675fe..c8ebd75176bb 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>           struct drm_device *dev, struct drm_encoder *encoder)
>>>   {
>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>       int ret;
>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>> -        return -ENOSPC;
>>> -    }
>>> -
>>>       hdmi->dev = dev;
>>>       hdmi->encoder = encoder;
>>>       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>>> -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
>>> -    if (IS_ERR(hdmi->bridge)) {
>>> -        ret = PTR_ERR(hdmi->bridge);
>>> +    ret = msm_hdmi_bridge_init(hdmi);
>>> +    if (ret) {
>>>           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
>>> %d\n", ret);
>>> -        hdmi->bridge = NULL;
>>>           goto fail;
>>>       }
>>> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>           goto fail;
>>>       }
>>> -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>> -
>>>       return 0;
>>>   fail:
>>> -    /* bridge is normally destroyed by drm: */
>>> -    if (hdmi->bridge) {
>>> -        msm_hdmi_bridge_destroy(hdmi->bridge);
>>> -        hdmi->bridge = NULL;
>>> -    }
>>>       if (hdmi->connector) {
>>>           hdmi->connector->funcs->destroy(hdmi->connector);
>>>           hdmi->connector = NULL;
>>> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
>>> struct device *master,
>>>           if (priv->hdmi->audio_pdev)
>>>               platform_device_unregister(priv->hdmi->audio_pdev);
>>> +        if (priv->hdmi->bridge)
>>> +            msm_hdmi_hpd_disable(priv->hdmi);
>>> +
>>
>> Now is this the only place where hdmi->bridge is used?
>>
>> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
>> anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?
> 
> Sure, sounds like a good idea, same followup as for the DSI.

Ok, this is fine then,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> 
>>
>>>           msm_hdmi_destroy(priv->hdmi);
>>>           priv->hdmi = NULL;
>>>       }
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> index e8dbee50637f..ec5786440391 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
>>> @@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi 
>>> *hdmi, int rate);
>>>    * hdmi bridge:
>>>    */
>>> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
>>> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
>>> +int msm_hdmi_bridge_init(struct hdmi *hdmi);
>>>   void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
>>>   enum drm_connector_status msm_hdmi_bridge_detect(
>>>           struct drm_bridge *bridge);
>>>   int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
>>> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
>>> +void msm_hdmi_hpd_disable(struct hdmi *hdmi);
>>>   /*
>>>    * i2c adapter for ddc:
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> index 9b1391d27ed3..0b7a6a56677e 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
>>> @@ -11,14 +11,6 @@
>>>   #include "msm_kms.h"
>>>   #include "hdmi.h"
>>> -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
>>> -{
>>> -    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>>> -
>>> -    msm_hdmi_hpd_disable(hdmi_bridge);
>>> -    drm_bridge_remove(bridge);
>>> -}
>>> -
>>>   static void msm_hdmi_power_on(struct drm_bridge *bridge)
>>>   {
>>>       struct drm_device *dev = bridge->dev;
>>> @@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
>>>   }
>>>   /* initialize bridge */
>>> -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
>>> +int msm_hdmi_bridge_init(struct hdmi *hdmi)
>>>   {
>>>       struct drm_bridge *bridge = NULL;
>>>       struct hdmi_bridge *hdmi_bridge;
>>> @@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
>>> hdmi *hdmi)
>>>       hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
>>>               sizeof(*hdmi_bridge), GFP_KERNEL);
>>> -    if (!hdmi_bridge) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> +    if (!hdmi_bridge)
>>> +        return -ENOMEM;
>>>       hdmi_bridge->hdmi = hdmi;
>>>       INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
>>> @@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
>>> hdmi *hdmi)
>>>           DRM_BRIDGE_OP_DETECT |
>>>           DRM_BRIDGE_OP_EDID;
>>> -    drm_bridge_add(bridge);
>>> +    ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
>>> +    if (ret)
>>> +        return ret;
>>>       ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, 
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>       if (ret)
>>> -        goto fail;
>>> +        return ret;
>>> -    return bridge;
>>> +    hdmi->bridge = bridge;
>>> -fail:
>>> -    if (bridge)
>>> -        msm_hdmi_bridge_destroy(bridge);
>>> -
>>> -    return ERR_PTR(ret);
>>> +    return 0;
>>>   }
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>>> index bfa827b47989..9ce0ffa35417 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
>>> @@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>>>       return ret;
>>>   }
>>> -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
>>> +void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>>>   {
>>> -    struct hdmi *hdmi = hdmi_bridge->hdmi;
>>>       const struct hdmi_platform_config *config = hdmi->config;
>>>       struct device *dev = &hdmi->pdev->dev;
>>>       int ret;
> 

Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

On 09/10/2023 22:21, Dmitry Baryshkov wrote:
> On 09/10/2023 22:19, Abhinav Kumar wrote:
>>
>>
>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
>>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>>> stop adding created bridge to the priv->bridges array.
>>>
>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>>>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>>>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
>>>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>>>   4 files changed, 17 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> index b6bcb9f675fe..c8ebd75176bb 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>           struct drm_device *dev, struct drm_encoder *encoder)
>>>   {
>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>       int ret;
>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>> -        return -ENOSPC;
>>> -    }
>>> -
>>>       hdmi->dev = dev;
>>>       hdmi->encoder = encoder;
>>>       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>>> -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
>>> -    if (IS_ERR(hdmi->bridge)) {
>>> -        ret = PTR_ERR(hdmi->bridge);
>>> +    ret = msm_hdmi_bridge_init(hdmi);
>>> +    if (ret) {
>>>           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
>>> %d\n", ret);
>>> -        hdmi->bridge = NULL;
>>>           goto fail;
>>>       }
>>> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>           goto fail;
>>>       }
>>> -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>> -
>>>       return 0;
>>>   fail:
>>> -    /* bridge is normally destroyed by drm: */
>>> -    if (hdmi->bridge) {
>>> -        msm_hdmi_bridge_destroy(hdmi->bridge);
>>> -        hdmi->bridge = NULL;
>>> -    }
>>>       if (hdmi->connector) {
>>>           hdmi->connector->funcs->destroy(hdmi->connector);
>>>           hdmi->connector = NULL;
>>> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
>>> struct device *master,
>>>           if (priv->hdmi->audio_pdev)
>>>               platform_device_unregister(priv->hdmi->audio_pdev);
>>> +        if (priv->hdmi->bridge)
>>> +            msm_hdmi_hpd_disable(priv->hdmi);
>>> +
>>
>> Now is this the only place where hdmi->bridge is used?
>>
>> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
>> anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?
> 
> Sure, sounds like a good idea, same followup as for the DSI.

I was wrong here. hdmi::bridge is used by the driver (e.g. for HPD 
reporting).

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Abhinav Kumar]

On 10/9/2023 1:53 PM, Dmitry Baryshkov wrote:
> On 09/10/2023 22:21, Dmitry Baryshkov wrote:
>> On 09/10/2023 22:19, Abhinav Kumar wrote:
>>>
>>>
>>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>>> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
>>>> drm_bridge_add(). As the driver doesn't require any additional cleanup,
>>>> stop adding created bridge to the priv->bridges array.
>>>>
>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>>>>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>>>>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 
>>>> ++++++++------------------
>>>>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>>>>   4 files changed, 17 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> index b6bcb9f675fe..c8ebd75176bb 100644
>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>>>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>>           struct drm_device *dev, struct drm_encoder *encoder)
>>>>   {
>>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>>       int ret;
>>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>>> -        return -ENOSPC;
>>>> -    }
>>>> -
>>>>       hdmi->dev = dev;
>>>>       hdmi->encoder = encoder;
>>>>       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>>>> -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
>>>> -    if (IS_ERR(hdmi->bridge)) {
>>>> -        ret = PTR_ERR(hdmi->bridge);
>>>> +    ret = msm_hdmi_bridge_init(hdmi);
>>>> +    if (ret) {
>>>>           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
>>>> %d\n", ret);
>>>> -        hdmi->bridge = NULL;
>>>>           goto fail;
>>>>       }
>>>> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>>           goto fail;
>>>>       }
>>>> -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>>> -
>>>>       return 0;
>>>>   fail:
>>>> -    /* bridge is normally destroyed by drm: */
>>>> -    if (hdmi->bridge) {
>>>> -        msm_hdmi_bridge_destroy(hdmi->bridge);
>>>> -        hdmi->bridge = NULL;
>>>> -    }
>>>>       if (hdmi->connector) {
>>>>           hdmi->connector->funcs->destroy(hdmi->connector);
>>>>           hdmi->connector = NULL;
>>>> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
>>>> struct device *master,
>>>>           if (priv->hdmi->audio_pdev)
>>>>               platform_device_unregister(priv->hdmi->audio_pdev);
>>>> +        if (priv->hdmi->bridge)
>>>> +            msm_hdmi_hpd_disable(priv->hdmi);
>>>> +
>>>
>>> Now is this the only place where hdmi->bridge is used?
>>>
>>> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
>>> anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?
>>
>> Sure, sounds like a good idea, same followup as for the DSI.
> 
> I was wrong here. hdmi::bridge is used by the driver (e.g. for HPD 
> reporting).
> 

hmmm, I thought HPD module uses hdmi_bridge->hdmi. here we are talking 
about hdmi->bridge?

Re: [Freedreno] [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

On 10/10/2023 00:04, Abhinav Kumar wrote:
> 
> 
> On 10/9/2023 1:53 PM, Dmitry Baryshkov wrote:
>> On 09/10/2023 22:21, Dmitry Baryshkov wrote:
>>> On 09/10/2023 22:19, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
>>>>> Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
>>>>> drm_bridge_add(). As the driver doesn't require any additional 
>>>>> cleanup,
>>>>> stop adding created bridge to the priv->bridges array.
>>>>>
>>>>> Reviewed-by: Rob Clark <robdclark@gmail.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
>>>>>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
>>>>>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 
>>>>> ++++++++------------------
>>>>>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
>>>>>   4 files changed, 17 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>>> index b6bcb9f675fe..c8ebd75176bb 100644
>>>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>>>> @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>>>>>   int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>>>           struct drm_device *dev, struct drm_encoder *encoder)
>>>>>   {
>>>>> -    struct msm_drm_private *priv = dev->dev_private;
>>>>>       int ret;
>>>>> -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
>>>>> -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
>>>>> -        return -ENOSPC;
>>>>> -    }
>>>>> -
>>>>>       hdmi->dev = dev;
>>>>>       hdmi->encoder = encoder;
>>>>>       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
>>>>> -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
>>>>> -    if (IS_ERR(hdmi->bridge)) {
>>>>> -        ret = PTR_ERR(hdmi->bridge);
>>>>> +    ret = msm_hdmi_bridge_init(hdmi);
>>>>> +    if (ret) {
>>>>>           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
>>>>> %d\n", ret);
>>>>> -        hdmi->bridge = NULL;
>>>>>           goto fail;
>>>>>       }
>>>>> @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>>>           goto fail;
>>>>>       }
>>>>> -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>>>> -
>>>>>       return 0;
>>>>>   fail:
>>>>> -    /* bridge is normally destroyed by drm: */
>>>>> -    if (hdmi->bridge) {
>>>>> -        msm_hdmi_bridge_destroy(hdmi->bridge);
>>>>> -        hdmi->bridge = NULL;
>>>>> -    }
>>>>>       if (hdmi->connector) {
>>>>>           hdmi->connector->funcs->destroy(hdmi->connector);
>>>>>           hdmi->connector = NULL;
>>>>> @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
>>>>> struct device *master,
>>>>>           if (priv->hdmi->audio_pdev)
>>>>>               platform_device_unregister(priv->hdmi->audio_pdev);
>>>>> +        if (priv->hdmi->bridge)
>>>>> +            msm_hdmi_hpd_disable(priv->hdmi);
>>>>> +
>>>>
>>>> Now is this the only place where hdmi->bridge is used?
>>>>
>>>> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since 
>>>> its anyway protected by if (priv->hdmi) and drop hdmi->bridge 
>>>> completely?
>>>
>>> Sure, sounds like a good idea, same followup as for the DSI.
>>
>> I was wrong here. hdmi::bridge is used by the driver (e.g. for HPD 
>> reporting).
>>
> 
> hmmm, I thought HPD module uses hdmi_bridge->hdmi. here we are talking 
> about hdmi->bridge?

See msm_hdmi_irq().

-- 
With best wishes
Dmitry

[PATCH v2 03/13] drm/msm/dp: move pdev from struct dp_display_private to struct msm_dp

[Dmitry Baryshkov]

The dp_drm needs accessing the DP's platform device. Move pdev to the
public structure.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 25 ++++++++++++-------------
 drivers/gpu/drm/msm/dp/dp_display.h |  1 +
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0e1afff491af..172daa5ad004 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -88,7 +88,6 @@ struct dp_display_private {
 	bool audio_supported;
 
 	struct drm_device *drm_dev;
-	struct platform_device *pdev;
 	struct dentry *root;
 
 	struct dp_parser  *parser;
@@ -595,7 +594,7 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 		return 0;
 	}
 
-	ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
+	ret = dp_display_usbpd_configure_cb(&dp->dp_display.pdev->dev);
 	if (ret) {	/* link train failed */
 		dp->hpd_state = ST_DISCONNECTED;
 	} else {
@@ -643,7 +642,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 		if (dp->link->sink_count == 0) {
 			dp_display_host_phy_exit(dp);
 		}
-		dp_display_notify_disconnect(&dp->pdev->dev);
+		dp_display_notify_disconnect(&dp->dp_display.pdev->dev);
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	} else if (state == ST_DISCONNECT_PENDING) {
@@ -653,7 +652,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 		dp_ctrl_off_link(dp->ctrl);
 		dp_display_host_phy_exit(dp);
 		dp->hpd_state = ST_DISCONNECTED;
-		dp_display_notify_disconnect(&dp->pdev->dev);
+		dp_display_notify_disconnect(&dp->dp_display.pdev->dev);
 		mutex_unlock(&dp->event_mutex);
 		return 0;
 	}
@@ -662,7 +661,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 	 * We don't need separate work for disconnect as
 	 * connect/attention interrupts are disabled
 	 */
-	dp_display_notify_disconnect(&dp->pdev->dev);
+	dp_display_notify_disconnect(&dp->dp_display.pdev->dev);
 
 	if (state == ST_DISPLAY_OFF) {
 		dp->hpd_state = ST_DISCONNECTED;
@@ -704,7 +703,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
 		return 0;
 	}
 
-	dp_display_usbpd_attention_cb(&dp->pdev->dev);
+	dp_display_usbpd_attention_cb(&dp->dp_display.pdev->dev);
 
 	drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
 			dp->dp_display.connector_type, state);
@@ -725,12 +724,12 @@ static void dp_display_deinit_sub_modules(struct dp_display_private *dp)
 static int dp_init_sub_modules(struct dp_display_private *dp)
 {
 	int rc = 0;
-	struct device *dev = &dp->pdev->dev;
+	struct device *dev = &dp->dp_display.pdev->dev;
 	struct dp_panel_in panel_in = {
 		.dev = dev,
 	};
 
-	dp->parser = dp_parser_get(dp->pdev);
+	dp->parser = dp_parser_get(dp->dp_display.pdev);
 	if (IS_ERR(dp->parser)) {
 		rc = PTR_ERR(dp->parser);
 		DRM_ERROR("failed to initialize parser, rc = %d\n", rc);
@@ -791,7 +790,7 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 		goto error_ctrl;
 	}
 
-	dp->audio = dp_audio_get(dp->pdev, dp->panel, dp->catalog);
+	dp->audio = dp_audio_get(dp->dp_display.pdev, dp->panel, dp->catalog);
 	if (IS_ERR(dp->audio)) {
 		rc = PTR_ERR(dp->audio);
 		pr_err("failed to initialize audio, rc = %d\n", rc);
@@ -1197,7 +1196,7 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
-	dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+	dp->irq = irq_of_parse_and_map(dp->dp_display.pdev->dev.of_node, 0);
 	if (!dp->irq) {
 		DRM_ERROR("failed to get irq\n");
 		return -EINVAL;
@@ -1253,7 +1252,7 @@ static int dp_display_probe(struct platform_device *pdev)
 	if (!desc)
 		return -EINVAL;
 
-	dp->pdev = pdev;
+	dp->dp_display.pdev = pdev;
 	dp->name = "drm_dp";
 	dp->id = desc->id;
 	dp->dp_display.connector_type = desc->connector_type;
@@ -1459,7 +1458,7 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	int rc;
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
-	dev = &dp->pdev->dev;
+	dev = &dp->dp_display.pdev->dev;
 
 	dp->debug = dp_debug_get(dev, dp->panel,
 					dp->link, dp->dp_display.connector,
@@ -1479,7 +1478,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 	struct device *dev;
 
 	dp_priv = container_of(dp, struct dp_display_private, dp_display);
-	dev = &dp_priv->pdev->dev;
+	dev = &dp_priv->dp_display.pdev->dev;
 	aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
 
 	if (aux_bus && dp->is_edp) {
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 1e9415ab15d8..f66cdbc35785 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -12,6 +12,7 @@
 
 struct msm_dp {
 	struct drm_device *drm_dev;
+	struct platform_device *pdev;
 	struct device *codec_dev;
 	struct drm_bridge *bridge;
 	struct drm_connector *connector;
-- 
2.39.2

[PATCH v2 04/13] drm/msm/dp: switch to devm_drm_bridge_add()

[Dmitry Baryshkov]

Make MSM DP driver use devm_drm_bridge_add() instead of plain
drm_bridge_add(). As the driver doesn't require any additional cleanup,
stop adding created bridge to the priv->bridges array.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c |  9 ++-------
 drivers/gpu/drm/msm/dp/dp_drm.c     | 21 +++++++++++++--------
 drivers/gpu/drm/msm/dp/dp_drm.h     |  2 +-
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 172daa5ad004..e329e03e068d 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1530,7 +1530,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
 int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
-	struct msm_drm_private *priv = dev->dev_private;
 	struct dp_display_private *dp_priv;
 	int ret;
 
@@ -1548,17 +1547,13 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
-	if (IS_ERR(dp_display->bridge)) {
-		ret = PTR_ERR(dp_display->bridge);
+	ret = dp_bridge_init(dp_display, dev, encoder);
+	if (ret) {
 		DRM_DEV_ERROR(dev->dev,
 			"failed to create dp bridge: %d\n", ret);
-		dp_display->bridge = NULL;
 		return ret;
 	}
 
-	priv->bridges[priv->num_bridges++] = dp_display->bridge;
-
 	dp_display->connector = dp_drm_connector_init(dp_display, encoder);
 	if (IS_ERR(dp_display->connector)) {
 		ret = PTR_ERR(dp_display->connector);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 785d76639497..284ff7df058a 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -272,7 +272,7 @@ static const struct drm_bridge_funcs edp_bridge_ops = {
 	.atomic_check = edp_bridge_atomic_check,
 };
 
-struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
+int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder)
 {
 	int rc;
@@ -281,7 +281,7 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
 
 	dp_bridge = devm_kzalloc(dev->dev, sizeof(*dp_bridge), GFP_KERNEL);
 	if (!dp_bridge)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	dp_bridge->dp_display = dp_display;
 
@@ -307,14 +307,18 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
 			DRM_BRIDGE_OP_MODES;
 	}
 
-	drm_bridge_add(bridge);
+	rc = devm_drm_bridge_add(&dp_display->pdev->dev, bridge);
+	if (rc) {
+		DRM_ERROR("failed to add bridge, rc=%d\n", rc);
+
+		return rc;
+	}
 
 	rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (rc) {
 		DRM_ERROR("failed to attach bridge, rc=%d\n", rc);
-		drm_bridge_remove(bridge);
 
-		return ERR_PTR(rc);
+		return rc;
 	}
 
 	if (dp_display->next_bridge) {
@@ -323,12 +327,13 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *
 					DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 		if (rc < 0) {
 			DRM_ERROR("failed to attach panel bridge: %d\n", rc);
-			drm_bridge_remove(bridge);
-			return ERR_PTR(rc);
+			return rc;
 		}
 	}
 
-	return bridge;
+	dp_display->bridge = bridge;
+
+	return 0;
 }
 
 /* connector initialization */
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index afe79b85e183..b3d684db2383 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -20,7 +20,7 @@ struct msm_dp_bridge {
 #define to_dp_bridge(x)     container_of((x), struct msm_dp_bridge, bridge)
 
 struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct drm_encoder *encoder);
-struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
+int dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
 			struct drm_encoder *encoder);
 
 void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
-- 
2.39.2

[PATCH v2 05/13] drm/msm: remove msm_drm_private::bridges field

[Dmitry Baryshkov]

As all output devices have switched to devm_drm_bridge_add(), we can
drop the bridges array from struct msm_drm_private.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 4 ----
 drivers/gpu/drm/msm/msm_drv.h | 3 ---
 2 files changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 5b937c3879af..7617c456475a 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -240,10 +240,6 @@ static int msm_drm_uninit(struct device *dev)
 
 	drm_mode_config_cleanup(ddev);
 
-	for (i = 0; i < priv->num_bridges; i++)
-		drm_bridge_remove(priv->bridges[i]);
-	priv->num_bridges = 0;
-
 	if (kms) {
 		pm_runtime_get_sync(dev);
 		msm_irq_uninstall(ddev);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 80085d644c1e..a6a29093bbe5 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -206,9 +206,6 @@ struct msm_drm_private {
 
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
-	unsigned int num_bridges;
-	struct drm_bridge *bridges[MAX_BRIDGES];
-
 	/* VRAM carveout, used when no IOMMU: */
 	struct {
 		unsigned long size;
-- 
2.39.2

[PATCH v2 06/13] drm/msm: drop pm ops from the headless msm driver

[Dmitry Baryshkov]

The msm_pm_prepare()/msm_pm_complete() only make sense for the
KMS-enabled devices, they have priv->kms guards inside. Drop global
msm_pm_ops, which were used only by the headless msm device.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7617c456475a..fe885bfd9742 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1122,11 +1122,6 @@ void msm_pm_complete(struct device *dev)
 	drm_mode_config_helper_resume(ddev);
 }
 
-static const struct dev_pm_ops msm_pm_ops = {
-	.prepare = msm_pm_prepare,
-	.complete = msm_pm_complete,
-};
-
 /*
  * Componentized driver support:
  */
@@ -1305,7 +1300,6 @@ static struct platform_driver msm_platform_driver = {
 	.shutdown   = msm_drv_shutdown,
 	.driver     = {
 		.name   = "msm",
-		.pm     = &msm_pm_ops,
 	},
 };
 
-- 
2.39.2

Re: [PATCH v2 06/13] drm/msm: drop pm ops from the headless msm driver

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> The msm_pm_prepare()/msm_pm_complete() only make sense for the
> KMS-enabled devices, they have priv->kms guards inside. Drop global
> msm_pm_ops, which were used only by the headless msm device.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 6 ------
>   1 file changed, 6 deletions(-)
> 


Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 07/13] drm/msm: rename msm_pm_prepare/complete to note the KMS nature

[Dmitry Baryshkov]

Rename the msm_pm_prepare() and msm_pm_complete() to
msm_kms_pm_prepare() and msm_kms_pm_complete() consequently.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 4 ++--
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 4 ++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 4 ++--
 drivers/gpu/drm/msm/msm_drv.c            | 4 ++--
 drivers/gpu/drm/msm/msm_drv.h            | 4 ++--
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 12d604b6b7e0..1f06c0c36533 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1342,8 +1342,8 @@ static const struct dev_pm_ops dpu_pm_ops = {
 	SET_RUNTIME_PM_OPS(dpu_runtime_suspend, dpu_runtime_resume, NULL)
 	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
 				pm_runtime_force_resume)
-	.prepare = msm_pm_prepare,
-	.complete = msm_pm_complete,
+	.prepare = msm_kms_pm_prepare,
+	.complete = msm_kms_pm_complete,
 };
 
 static const struct of_device_id dpu_dt_match[] = {
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 18735bbaf798..386f61556789 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -501,8 +501,8 @@ static int mdp4_kms_init(struct drm_device *dev)
 }
 
 static const struct dev_pm_ops mdp4_pm_ops = {
-	.prepare = msm_pm_prepare,
-	.complete = msm_pm_complete,
+	.prepare = msm_kms_pm_prepare,
+	.complete = msm_kms_pm_complete,
 };
 
 static int mdp4_probe(struct platform_device *pdev)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 68fcfd85d250..d936b6b958d1 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -960,8 +960,8 @@ static __maybe_unused int mdp5_runtime_resume(struct device *dev)
 
 static const struct dev_pm_ops mdp5_pm_ops = {
 	SET_RUNTIME_PM_OPS(mdp5_runtime_suspend, mdp5_runtime_resume, NULL)
-	.prepare = msm_pm_prepare,
-	.complete = msm_pm_complete,
+	.prepare = msm_kms_pm_prepare,
+	.complete = msm_kms_pm_complete,
 };
 
 static const struct of_device_id mdp5_dt_match[] = {
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index fe885bfd9742..76b69f605b9c 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1100,7 +1100,7 @@ static const struct drm_driver msm_driver = {
 	.patchlevel         = MSM_VERSION_PATCHLEVEL,
 };
 
-int msm_pm_prepare(struct device *dev)
+int msm_kms_pm_prepare(struct device *dev)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
 	struct drm_device *ddev = priv ? priv->dev : NULL;
@@ -1111,7 +1111,7 @@ int msm_pm_prepare(struct device *dev)
 	return drm_mode_config_helper_suspend(ddev);
 }
 
-void msm_pm_complete(struct device *dev)
+void msm_kms_pm_complete(struct device *dev)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
 	struct drm_device *ddev = priv ? priv->dev : NULL;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index a6a29093bbe5..a28d93c09492 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -563,8 +563,8 @@ static inline unsigned long timeout_to_jiffies(const ktime_t *timeout)
 
 extern const struct component_master_ops msm_drm_ops;
 
-int msm_pm_prepare(struct device *dev);
-void msm_pm_complete(struct device *dev);
+int msm_kms_pm_prepare(struct device *dev);
+void msm_kms_pm_complete(struct device *dev);
 
 int msm_drv_probe(struct device *dev,
 	int (*kms_init)(struct drm_device *dev),
-- 
2.39.2

Re: [PATCH v2 07/13] drm/msm: rename msm_pm_prepare/complete to note the KMS nature

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> Rename the msm_pm_prepare() and msm_pm_complete() to
> msm_kms_pm_prepare() and msm_kms_pm_complete() consequently.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 4 ++--
>   drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 4 ++--
>   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 4 ++--
>   drivers/gpu/drm/msm/msm_drv.c            | 4 ++--
>   drivers/gpu/drm/msm/msm_drv.h            | 4 ++--
>   5 files changed, 10 insertions(+), 10 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 08/13] drm/msm: remove shutdown callback from msm_platform_driver

[Dmitry Baryshkov]

The msm_drv_shutdown only makes sense for the KMS-enabled devices, while
msm_platform_driver is only used in the headless case. Remove the
shutdown callback from the driver structure.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 76b69f605b9c..c2f989d1ff42 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1297,7 +1297,6 @@ void msm_drv_shutdown(struct platform_device *pdev)
 static struct platform_driver msm_platform_driver = {
 	.probe      = msm_pdev_probe,
 	.remove_new = msm_pdev_remove,
-	.shutdown   = msm_drv_shutdown,
 	.driver     = {
 		.name   = "msm",
 	},
-- 
2.39.2

Re: [PATCH v2 08/13] drm/msm: remove shutdown callback from msm_platform_driver

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> The msm_drv_shutdown only makes sense for the KMS-enabled devices, while
> msm_platform_driver is only used in the headless case. Remove the
> shutdown callback from the driver structure.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 1 -
>   1 file changed, 1 deletion(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 09/13] drm/msm: rename msm_drv_shutdown() to msm_kms_shutdown()

[Dmitry Baryshkov]

The msm_drv_shutdown function should only be used in the KMS case.
Rename it accordingly.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 2 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 2 +-
 drivers/gpu/drm/msm/msm_drv.c            | 2 +-
 drivers/gpu/drm/msm/msm_drv.h            | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 1f06c0c36533..fe7267b3bff5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1370,7 +1370,7 @@ MODULE_DEVICE_TABLE(of, dpu_dt_match);
 static struct platform_driver dpu_driver = {
 	.probe = dpu_dev_probe,
 	.remove_new = dpu_dev_remove,
-	.shutdown = msm_drv_shutdown,
+	.shutdown = msm_kms_shutdown,
 	.driver = {
 		.name = "msm_dpu",
 		.of_match_table = dpu_dt_match,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 386f61556789..4ba1cb74ad76 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -570,7 +570,7 @@ MODULE_DEVICE_TABLE(of, mdp4_dt_match);
 static struct platform_driver mdp4_platform_driver = {
 	.probe      = mdp4_probe,
 	.remove_new = mdp4_remove,
-	.shutdown   = msm_drv_shutdown,
+	.shutdown   = msm_kms_shutdown,
 	.driver     = {
 		.name   = "mdp4",
 		.of_match_table = mdp4_dt_match,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index d936b6b958d1..11d9fc2c6bf5 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -975,7 +975,7 @@ MODULE_DEVICE_TABLE(of, mdp5_dt_match);
 static struct platform_driver mdp5_driver = {
 	.probe = mdp5_dev_probe,
 	.remove_new = mdp5_dev_remove,
-	.shutdown = msm_drv_shutdown,
+	.shutdown = msm_kms_shutdown,
 	.driver = {
 		.name = "msm_mdp",
 		.of_match_table = mdp5_dt_match,
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c2f989d1ff42..8079f408c9ed 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1278,7 +1278,7 @@ static void msm_pdev_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &msm_drm_ops);
 }
 
-void msm_drv_shutdown(struct platform_device *pdev)
+void msm_kms_shutdown(struct platform_device *pdev)
 {
 	struct msm_drm_private *priv = platform_get_drvdata(pdev);
 	struct drm_device *drm = priv ? priv->dev : NULL;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index a28d93c09492..cd5bf658df66 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -569,7 +569,7 @@ void msm_kms_pm_complete(struct device *dev);
 int msm_drv_probe(struct device *dev,
 	int (*kms_init)(struct drm_device *dev),
 	struct msm_kms *kms);
-void msm_drv_shutdown(struct platform_device *pdev);
+void msm_kms_shutdown(struct platform_device *pdev);
 
 
 #endif /* __MSM_DRV_H__ */
-- 
2.39.2

[PATCH v2 10/13] drm/msm: switch to drmm_mode_config_init()

[Dmitry Baryshkov]

Switch to drmm_mode_config_init() instead of drm_mode_config_init().
Drop drm_mode_config_cleanup() calls.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 8079f408c9ed..00ed71c3d503 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -238,8 +238,6 @@ static int msm_drm_uninit(struct device *dev)
 	if (kms)
 		msm_disp_snapshot_destroy(ddev);
 
-	drm_mode_config_cleanup(ddev);
-
 	if (kms) {
 		pm_runtime_get_sync(dev);
 		msm_irq_uninstall(ddev);
@@ -440,11 +438,13 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	might_lock(&priv->lru.lock);
 	fs_reclaim_release(GFP_KERNEL);
 
-	drm_mode_config_init(ddev);
+	ret = drmm_mode_config_init(ddev);
+	if (ret)
+		goto err_destroy_wq;
 
 	ret = msm_init_vram(ddev);
 	if (ret)
-		goto err_cleanup_mode_config;
+		goto err_destroy_wq;
 
 	dma_set_max_seg_size(dev, UINT_MAX);
 
@@ -555,8 +555,7 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 
 err_deinit_vram:
 	msm_deinit_vram(ddev);
-err_cleanup_mode_config:
-	drm_mode_config_cleanup(ddev);
+err_destroy_wq:
 	destroy_workqueue(priv->wq);
 err_put_dev:
 	drm_dev_put(ddev);
-- 
2.39.2

Re: [PATCH v2 10/13] drm/msm: switch to drmm_mode_config_init()

[Abhinav Kumar]

On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> Switch to drmm_mode_config_init() instead of drm_mode_config_init().
> Drop drm_mode_config_cleanup() calls.
> 
> Reviewed-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/msm_drv.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 11/13] drm/msm: only register 'kms' debug file if KMS is used

[Dmitry Baryshkov]

There is little point in having the empty debugfs file which always
returns -ENODEV. Change this file to be created only if KMS is actually
used.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index a0a936f80ae3..06fc632fd6f9 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -314,8 +314,9 @@ void msm_debugfs_init(struct drm_minor *minor)
 	debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
 		dev, &msm_gpu_fops);
 
-	debugfs_create_file("kms", S_IRUSR, minor->debugfs_root,
-		dev, &msm_kms_fops);
+	if (priv->kms)
+		debugfs_create_file("kms", S_IRUSR, minor->debugfs_root,
+				    dev, &msm_kms_fops);
 
 	debugfs_create_u32("hangcheck_period_ms", 0600, minor->debugfs_root,
 		&priv->hangcheck_period);
-- 
2.39.2

[PATCH v2 12/13] drm/msm: make fb debugfs file available only in KMS case

[Dmitry Baryshkov]

Don't register the 'fb' debugfs file, if there is no KMS (and so no
framebuffers).

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_debugfs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_debugfs.c b/drivers/gpu/drm/msm/msm_debugfs.c
index 06fc632fd6f9..04d304eed223 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -266,6 +266,9 @@ static int msm_fb_show(struct seq_file *m, void *arg)
 static struct drm_info_list msm_debugfs_list[] = {
 		{"gem", msm_gem_show},
 		{ "mm", msm_mm_show },
+};
+
+static struct drm_info_list msm_kms_debugfs_list[] = {
 		{ "fb", msm_fb_show },
 };
 
@@ -314,9 +317,13 @@ void msm_debugfs_init(struct drm_minor *minor)
 	debugfs_create_file("gpu", S_IRUSR, minor->debugfs_root,
 		dev, &msm_gpu_fops);
 
-	if (priv->kms)
+	if (priv->kms) {
+		drm_debugfs_create_files(msm_kms_debugfs_list,
+					 ARRAY_SIZE(msm_kms_debugfs_list),
+					 minor->debugfs_root, minor);
 		debugfs_create_file("kms", S_IRUSR, minor->debugfs_root,
 				    dev, &msm_kms_fops);
+	}
 
 	debugfs_create_u32("hangcheck_period_ms", 0600, minor->debugfs_root,
 		&priv->hangcheck_period);
-- 
2.39.2

[PATCH v2 13/13] drm/msm: carve out KMS code from msm_drv.c

[Dmitry Baryshkov]

The msm_drv.c contains generic code intermixed with KMS handling code.
Move all KMS-related code to a separate msm_kms.c file, cleaning up init
code while doing this move. This also prevents msm driver from registering
modesetting / atomic interfaces in the headless case.

Reviewed-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/Makefile  |   1 +
 drivers/gpu/drm/msm/msm_drv.c | 346 ++--------------------------------
 drivers/gpu/drm/msm/msm_kms.c | 345 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/msm_kms.h |   3 +
 4 files changed, 365 insertions(+), 330 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_kms.c

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 8d02d8c33069..49671364fdcf 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -106,6 +106,7 @@ msm-y += \
 	msm_gpu_devfreq.o \
 	msm_io_utils.o \
 	msm_iommu.o \
+	msm_kms.o \
 	msm_perf.o \
 	msm_rd.o \
 	msm_ringbuffer.o \
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 00ed71c3d503..1efb31420094 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -6,30 +6,17 @@
  */
 
 #include <linux/dma-mapping.h>
-#include <linux/fault-inject.h>
-#include <linux/kthread.h>
 #include <linux/of_address.h>
-#include <linux/sched/mm.h>
 #include <linux/uaccess.h>
-#include <uapi/linux/sched/types.h>
 
-#include <drm/drm_aperture.h>
-#include <drm/drm_bridge.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
-#include <drm/drm_prime.h>
 #include <drm/drm_of.h>
-#include <drm/drm_vblank.h>
 
-#include "disp/msm_disp_snapshot.h"
 #include "msm_drv.h"
 #include "msm_debugfs.h"
-#include "msm_fence.h"
-#include "msm_gem.h"
-#include "msm_gpu.h"
 #include "msm_kms.h"
-#include "msm_mmu.h"
 #include "adreno/adreno_gpu.h"
 
 /*
@@ -56,16 +43,6 @@
 
 static void msm_deinit_vram(struct drm_device *ddev);
 
-static const struct drm_mode_config_funcs mode_config_funcs = {
-	.fb_create = msm_framebuffer_create,
-	.atomic_check = msm_atomic_check,
-	.atomic_commit = drm_atomic_helper_commit,
-};
-
-static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = {
-	.atomic_commit_tail = msm_atomic_commit_tail,
-};
-
 static char *vram = "16m";
 MODULE_PARM_DESC(vram, "Configure VRAM size (for devices without IOMMU/GPUMMU)");
 module_param(vram, charp, 0);
@@ -83,125 +60,11 @@ DECLARE_FAULT_ATTR(fail_gem_alloc);
 DECLARE_FAULT_ATTR(fail_gem_iova);
 #endif
 
-static irqreturn_t msm_irq(int irq, void *arg)
-{
-	struct drm_device *dev = arg;
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	return kms->funcs->irq(kms);
-}
-
-static void msm_irq_preinstall(struct drm_device *dev)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	kms->funcs->irq_preinstall(kms);
-}
-
-static int msm_irq_postinstall(struct drm_device *dev)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	BUG_ON(!kms);
-
-	if (kms->funcs->irq_postinstall)
-		return kms->funcs->irq_postinstall(kms);
-
-	return 0;
-}
-
-static int msm_irq_install(struct drm_device *dev, unsigned int irq)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	int ret;
-
-	if (irq == IRQ_NOTCONNECTED)
-		return -ENOTCONN;
-
-	msm_irq_preinstall(dev);
-
-	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
-	if (ret)
-		return ret;
-
-	kms->irq_requested = true;
-
-	ret = msm_irq_postinstall(dev);
-	if (ret) {
-		free_irq(irq, dev);
-		return ret;
-	}
-
-	return 0;
-}
-
-static void msm_irq_uninstall(struct drm_device *dev)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-
-	kms->funcs->irq_uninstall(kms);
-	if (kms->irq_requested)
-		free_irq(kms->irq, dev);
-}
-
-struct msm_vblank_work {
-	struct work_struct work;
-	struct drm_crtc *crtc;
-	bool enable;
-	struct msm_drm_private *priv;
-};
-
-static void vblank_ctrl_worker(struct work_struct *work)
-{
-	struct msm_vblank_work *vbl_work = container_of(work,
-						struct msm_vblank_work, work);
-	struct msm_drm_private *priv = vbl_work->priv;
-	struct msm_kms *kms = priv->kms;
-
-	if (vbl_work->enable)
-		kms->funcs->enable_vblank(kms, vbl_work->crtc);
-	else
-		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
-
-	kfree(vbl_work);
-}
-
-static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
-					struct drm_crtc *crtc, bool enable)
-{
-	struct msm_vblank_work *vbl_work;
-
-	vbl_work = kzalloc(sizeof(*vbl_work), GFP_ATOMIC);
-	if (!vbl_work)
-		return -ENOMEM;
-
-	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
-
-	vbl_work->crtc = crtc;
-	vbl_work->enable = enable;
-	vbl_work->priv = priv;
-
-	queue_work(priv->wq, &vbl_work->work);
-
-	return 0;
-}
-
 static int msm_drm_uninit(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct msm_drm_private *priv = platform_get_drvdata(pdev);
 	struct drm_device *ddev = priv->dev;
-	struct msm_kms *kms = priv->kms;
-	int i;
 
 	/*
 	 * Shutdown the hw if we're far enough along where things might be on.
@@ -212,7 +75,8 @@ static int msm_drm_uninit(struct device *dev)
 	 */
 	if (ddev->registered) {
 		drm_dev_unregister(ddev);
-		drm_atomic_helper_shutdown(ddev);
+		if (priv->kms)
+			drm_atomic_helper_shutdown(ddev);
 	}
 
 	/* We must cancel and cleanup any pending vblank enable/disable
@@ -222,30 +86,13 @@ static int msm_drm_uninit(struct device *dev)
 
 	flush_workqueue(priv->wq);
 
-	/* clean up event worker threads */
-	for (i = 0; i < priv->num_crtcs; i++) {
-		if (priv->event_thread[i].worker)
-			kthread_destroy_worker(priv->event_thread[i].worker);
-	}
-
 	msm_gem_shrinker_cleanup(ddev);
 
-	drm_kms_helper_poll_fini(ddev);
-
 	msm_perf_debugfs_cleanup(priv);
 	msm_rd_debugfs_cleanup(priv);
 
-	if (kms)
-		msm_disp_snapshot_destroy(ddev);
-
-	if (kms) {
-		pm_runtime_get_sync(dev);
-		msm_irq_uninstall(ddev);
-		pm_runtime_put_sync(dev);
-	}
-
-	if (kms && kms->funcs)
-		kms->funcs->destroy(kms);
+	if (priv->kms)
+		msm_drm_kms_uninit(dev);
 
 	msm_deinit_vram(ddev);
 
@@ -259,42 +106,6 @@ static int msm_drm_uninit(struct device *dev)
 	return 0;
 }
 
-struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
-{
-	struct msm_gem_address_space *aspace;
-	struct msm_mmu *mmu;
-	struct device *mdp_dev = dev->dev;
-	struct device *mdss_dev = mdp_dev->parent;
-	struct device *iommu_dev;
-
-	/*
-	 * IOMMUs can be a part of MDSS device tree binding, or the
-	 * MDP/DPU device.
-	 */
-	if (device_iommu_mapped(mdp_dev))
-		iommu_dev = mdp_dev;
-	else
-		iommu_dev = mdss_dev;
-
-	mmu = msm_iommu_new(iommu_dev, 0);
-	if (IS_ERR(mmu))
-		return ERR_CAST(mmu);
-
-	if (!mmu) {
-		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
-		return NULL;
-	}
-
-	aspace = msm_gem_address_space_create(mmu, "mdp_kms",
-		0x1000, 0x100000000 - 0x1000);
-	if (IS_ERR(aspace)) {
-		dev_err(mdp_dev, "aspace create, error %pe\n", aspace);
-		mmu->funcs->destroy(mmu);
-	}
-
-	return aspace;
-}
-
 bool msm_use_mmu(struct drm_device *dev)
 {
 	struct msm_drm_private *priv = dev->dev_private;
@@ -400,8 +211,6 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(dev);
 	struct drm_device *ddev;
-	struct msm_kms *kms;
-	struct drm_crtc *crtc;
 	int ret;
 
 	if (drm_firmware_drivers_only())
@@ -438,9 +247,11 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	might_lock(&priv->lru.lock);
 	fs_reclaim_release(GFP_KERNEL);
 
-	ret = drmm_mode_config_init(ddev);
-	if (ret)
-		goto err_destroy_wq;
+	if (priv->kms_init) {
+		ret = drmm_mode_config_init(ddev);
+		if (ret)
+			goto err_destroy_wq;
+	}
 
 	ret = msm_init_vram(ddev);
 	if (ret)
@@ -453,98 +264,33 @@ static int msm_drm_init(struct device *dev, const struct drm_driver *drv)
 	if (ret)
 		goto err_deinit_vram;
 
-	/* the fw fb could be anywhere in memory */
-	ret = drm_aperture_remove_framebuffers(drv);
-	if (ret)
-		goto err_msm_uninit;
-
 	ret = msm_gem_shrinker_init(ddev);
 	if (ret)
 		goto err_msm_uninit;
 
 	if (priv->kms_init) {
-		ret = priv->kms_init(ddev);
-		if (ret) {
-			DRM_DEV_ERROR(dev, "failed to load kms\n");
-			priv->kms = NULL;
+		ret = msm_drm_kms_init(dev, drv);
+		if (ret)
 			goto err_msm_uninit;
-		}
-		kms = priv->kms;
 	} else {
 		/* valid only for the dummy headless case, where of_node=NULL */
 		WARN_ON(dev->of_node);
-		kms = NULL;
-	}
-
-	/* Enable normalization of plane zpos */
-	ddev->mode_config.normalize_zpos = true;
-
-	if (kms) {
-		kms->dev = ddev;
-		ret = kms->funcs->hw_init(kms);
-		if (ret) {
-			DRM_DEV_ERROR(dev, "kms hw init failed: %d\n", ret);
-			goto err_msm_uninit;
-		}
-	}
-
-	drm_helper_move_panel_connectors_to_head(ddev);
-
-	ddev->mode_config.funcs = &mode_config_funcs;
-	ddev->mode_config.helper_private = &mode_config_helper_funcs;
-
-	drm_for_each_crtc(crtc, ddev) {
-		struct msm_drm_thread *ev_thread;
-
-		/* initialize event thread */
-		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
-		ev_thread->dev = ddev;
-		ev_thread->worker = kthread_create_worker(0, "crtc_event:%d", crtc->base.id);
-		if (IS_ERR(ev_thread->worker)) {
-			ret = PTR_ERR(ev_thread->worker);
-			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
-			ev_thread->worker = NULL;
-			goto err_msm_uninit;
-		}
-
-		sched_set_fifo(ev_thread->worker->task);
-	}
-
-	ret = drm_vblank_init(ddev, priv->num_crtcs);
-	if (ret < 0) {
-		DRM_DEV_ERROR(dev, "failed to initialize vblank\n");
-		goto err_msm_uninit;
-	}
-
-	if (kms) {
-		pm_runtime_get_sync(dev);
-		ret = msm_irq_install(ddev, kms->irq);
-		pm_runtime_put_sync(dev);
-		if (ret < 0) {
-			DRM_DEV_ERROR(dev, "failed to install IRQ handler\n");
-			goto err_msm_uninit;
-		}
+		ddev->driver_features &= ~DRIVER_MODESET;
+		ddev->driver_features &= ~DRIVER_ATOMIC;
 	}
 
 	ret = drm_dev_register(ddev, 0);
 	if (ret)
 		goto err_msm_uninit;
 
-	if (kms) {
-		ret = msm_disp_snapshot_init(ddev);
-		if (ret)
-			DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
-	}
-	drm_mode_config_reset(ddev);
-
 	ret = msm_debugfs_late_init(ddev);
 	if (ret)
 		goto err_msm_uninit;
 
-	drm_kms_helper_poll_init(ddev);
-
-	if (kms)
+	if (priv->kms_init) {
+		drm_kms_helper_poll_init(ddev);
 		msm_fbdev_setup(ddev);
+	}
 
 	return 0;
 
@@ -635,28 +381,6 @@ static void msm_postclose(struct drm_device *dev, struct drm_file *file)
 	context_close(ctx);
 }
 
-int msm_crtc_enable_vblank(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	if (!kms)
-		return -ENXIO;
-	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
-	return vblank_ctrl_queue_work(priv, crtc, true);
-}
-
-void msm_crtc_disable_vblank(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	if (!kms)
-		return;
-	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
-	vblank_ctrl_queue_work(priv, crtc, false);
-}
-
 /*
  * DRM ioctls:
  */
@@ -1099,28 +823,6 @@ static const struct drm_driver msm_driver = {
 	.patchlevel         = MSM_VERSION_PATCHLEVEL,
 };
 
-int msm_kms_pm_prepare(struct device *dev)
-{
-	struct msm_drm_private *priv = dev_get_drvdata(dev);
-	struct drm_device *ddev = priv ? priv->dev : NULL;
-
-	if (!priv || !priv->kms)
-		return 0;
-
-	return drm_mode_config_helper_suspend(ddev);
-}
-
-void msm_kms_pm_complete(struct device *dev)
-{
-	struct msm_drm_private *priv = dev_get_drvdata(dev);
-	struct drm_device *ddev = priv ? priv->dev : NULL;
-
-	if (!priv || !priv->kms)
-		return;
-
-	drm_mode_config_helper_resume(ddev);
-}
-
 /*
  * Componentized driver support:
  */
@@ -1277,22 +979,6 @@ static void msm_pdev_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &msm_drm_ops);
 }
 
-void msm_kms_shutdown(struct platform_device *pdev)
-{
-	struct msm_drm_private *priv = platform_get_drvdata(pdev);
-	struct drm_device *drm = priv ? priv->dev : NULL;
-
-	/*
-	 * Shutdown the hw if we're far enough along where things might be on.
-	 * If we run this too early, we'll end up panicking in any variety of
-	 * places. Since we don't register the drm device until late in
-	 * msm_drm_init, drm_dev->registered is used as an indicator that the
-	 * shutdown will be successful.
-	 */
-	if (drm && drm->registered && priv->kms)
-		drm_atomic_helper_shutdown(drm);
-}
-
 static struct platform_driver msm_platform_driver = {
 	.probe      = msm_pdev_probe,
 	.remove_new = msm_pdev_remove,
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
new file mode 100644
index 000000000000..84c21ec2ceea
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -0,0 +1,345 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2016-2018, 2020-2021 The Linux Foundation. All rights reserved.
+ * Copyright (C) 2013 Red Hat
+ * Author: Rob Clark <robdclark@gmail.com>
+ */
+
+#include <linux/kthread.h>
+#include <linux/sched/mm.h>
+#include <uapi/linux/sched/types.h>
+
+#include <drm/drm_aperture.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_mode_config.h>
+#include <drm/drm_vblank.h>
+
+#include "disp/msm_disp_snapshot.h"
+#include "msm_drv.h"
+#include "msm_gem.h"
+#include "msm_kms.h"
+#include "msm_mmu.h"
+
+static const struct drm_mode_config_funcs mode_config_funcs = {
+	.fb_create = msm_framebuffer_create,
+	.atomic_check = msm_atomic_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs mode_config_helper_funcs = {
+	.atomic_commit_tail = msm_atomic_commit_tail,
+};
+
+static irqreturn_t msm_irq(int irq, void *arg)
+{
+	struct drm_device *dev = arg;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+
+	BUG_ON(!kms);
+
+	return kms->funcs->irq(kms);
+}
+
+static void msm_irq_preinstall(struct drm_device *dev)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+
+	BUG_ON(!kms);
+
+	kms->funcs->irq_preinstall(kms);
+}
+
+static int msm_irq_postinstall(struct drm_device *dev)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+
+	BUG_ON(!kms);
+
+	if (kms->funcs->irq_postinstall)
+		return kms->funcs->irq_postinstall(kms);
+
+	return 0;
+}
+
+static int msm_irq_install(struct drm_device *dev, unsigned int irq)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+	int ret;
+
+	if (irq == IRQ_NOTCONNECTED)
+		return -ENOTCONN;
+
+	msm_irq_preinstall(dev);
+
+	ret = request_irq(irq, msm_irq, 0, dev->driver->name, dev);
+	if (ret)
+		return ret;
+
+	kms->irq_requested = true;
+
+	ret = msm_irq_postinstall(dev);
+	if (ret) {
+		free_irq(irq, dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void msm_irq_uninstall(struct drm_device *dev)
+{
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+
+	kms->funcs->irq_uninstall(kms);
+	if (kms->irq_requested)
+		free_irq(kms->irq, dev);
+}
+
+struct msm_vblank_work {
+	struct work_struct work;
+	struct drm_crtc *crtc;
+	bool enable;
+	struct msm_drm_private *priv;
+};
+
+static void vblank_ctrl_worker(struct work_struct *work)
+{
+	struct msm_vblank_work *vbl_work = container_of(work,
+						struct msm_vblank_work, work);
+	struct msm_drm_private *priv = vbl_work->priv;
+	struct msm_kms *kms = priv->kms;
+
+	if (vbl_work->enable)
+		kms->funcs->enable_vblank(kms, vbl_work->crtc);
+	else
+		kms->funcs->disable_vblank(kms,	vbl_work->crtc);
+
+	kfree(vbl_work);
+}
+
+static int vblank_ctrl_queue_work(struct msm_drm_private *priv,
+				  struct drm_crtc *crtc, bool enable)
+{
+	struct msm_vblank_work *vbl_work;
+
+	vbl_work = kzalloc(sizeof(*vbl_work), GFP_ATOMIC);
+	if (!vbl_work)
+		return -ENOMEM;
+
+	INIT_WORK(&vbl_work->work, vblank_ctrl_worker);
+
+	vbl_work->crtc = crtc;
+	vbl_work->enable = enable;
+	vbl_work->priv = priv;
+
+	queue_work(priv->wq, &vbl_work->work);
+
+	return 0;
+}
+
+int msm_crtc_enable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+	if (!kms)
+		return -ENXIO;
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	return vblank_ctrl_queue_work(priv, crtc, true);
+}
+
+void msm_crtc_disable_vblank(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct msm_drm_private *priv = dev->dev_private;
+	struct msm_kms *kms = priv->kms;
+	if (!kms)
+		return;
+	drm_dbg_vbl(dev, "crtc=%u", crtc->base.id);
+	vblank_ctrl_queue_work(priv, crtc, false);
+}
+
+struct msm_gem_address_space *msm_kms_init_aspace(struct drm_device *dev)
+{
+	struct msm_gem_address_space *aspace;
+	struct msm_mmu *mmu;
+	struct device *mdp_dev = dev->dev;
+	struct device *mdss_dev = mdp_dev->parent;
+	struct device *iommu_dev;
+
+	/*
+	 * IOMMUs can be a part of MDSS device tree binding, or the
+	 * MDP/DPU device.
+	 */
+	if (device_iommu_mapped(mdp_dev))
+		iommu_dev = mdp_dev;
+	else
+		iommu_dev = mdss_dev;
+
+	mmu = msm_iommu_new(iommu_dev, 0);
+	if (IS_ERR(mmu))
+		return ERR_CAST(mmu);
+
+	if (!mmu) {
+		drm_info(dev, "no IOMMU, fallback to phys contig buffers for scanout\n");
+		return NULL;
+	}
+
+	aspace = msm_gem_address_space_create(mmu, "mdp_kms",
+		0x1000, 0x100000000 - 0x1000);
+	if (IS_ERR(aspace)) {
+		dev_err(mdp_dev, "aspace create, error %pe\n", aspace);
+		mmu->funcs->destroy(mmu);
+	}
+
+	return aspace;
+}
+
+void msm_drm_kms_uninit(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct msm_drm_private *priv = platform_get_drvdata(pdev);
+	struct drm_device *ddev = priv->dev;
+	struct msm_kms *kms = priv->kms;
+	int i;
+
+	BUG_ON(!kms);
+
+	/* clean up event worker threads */
+	for (i = 0; i < priv->num_crtcs; i++) {
+		if (priv->event_thread[i].worker)
+			kthread_destroy_worker(priv->event_thread[i].worker);
+	}
+
+	drm_kms_helper_poll_fini(ddev);
+
+	msm_disp_snapshot_destroy(ddev);
+
+	pm_runtime_get_sync(dev);
+	msm_irq_uninstall(ddev);
+	pm_runtime_put_sync(dev);
+
+	if (kms && kms->funcs)
+		kms->funcs->destroy(kms);
+}
+
+int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
+{
+	struct msm_drm_private *priv = dev_get_drvdata(dev);
+	struct drm_device *ddev = priv->dev;
+	struct msm_kms *kms = priv->kms;
+	struct drm_crtc *crtc;
+	int ret;
+
+	/* the fw fb could be anywhere in memory */
+	ret = drm_aperture_remove_framebuffers(drv);
+	if (ret)
+		return ret;
+
+	ret = priv->kms_init(ddev);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to load kms\n");
+		priv->kms = NULL;
+		return ret;
+	}
+
+	/* Enable normalization of plane zpos */
+	ddev->mode_config.normalize_zpos = true;
+
+	ddev->mode_config.funcs = &mode_config_funcs;
+	ddev->mode_config.helper_private = &mode_config_helper_funcs;
+
+	kms->dev = ddev;
+	ret = kms->funcs->hw_init(kms);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "kms hw init failed: %d\n", ret);
+		goto err_msm_uninit;
+	}
+
+	drm_helper_move_panel_connectors_to_head(ddev);
+
+	drm_for_each_crtc(crtc, ddev) {
+		struct msm_drm_thread *ev_thread;
+
+		/* initialize event thread */
+		ev_thread = &priv->event_thread[drm_crtc_index(crtc)];
+		ev_thread->dev = ddev;
+		ev_thread->worker = kthread_create_worker(0, "crtc_event:%d", crtc->base.id);
+		if (IS_ERR(ev_thread->worker)) {
+			ret = PTR_ERR(ev_thread->worker);
+			DRM_DEV_ERROR(dev, "failed to create crtc_event kthread\n");
+			ev_thread->worker = NULL;
+			goto err_msm_uninit;
+		}
+
+		sched_set_fifo(ev_thread->worker->task);
+	}
+
+	ret = drm_vblank_init(ddev, priv->num_crtcs);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "failed to initialize vblank\n");
+		goto err_msm_uninit;
+	}
+
+	pm_runtime_get_sync(dev);
+	ret = msm_irq_install(ddev, kms->irq);
+	pm_runtime_put_sync(dev);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "failed to install IRQ handler\n");
+		goto err_msm_uninit;
+	}
+
+	ret = msm_disp_snapshot_init(ddev);
+	if (ret)
+		DRM_DEV_ERROR(dev, "msm_disp_snapshot_init failed ret = %d\n", ret);
+
+	drm_mode_config_reset(ddev);
+
+	return 0;
+
+err_msm_uninit:
+	return ret;
+}
+
+int msm_kms_pm_prepare(struct device *dev)
+{
+	struct msm_drm_private *priv = dev_get_drvdata(dev);
+	struct drm_device *ddev = priv ? priv->dev : NULL;
+
+	if (!priv || !priv->kms)
+		return 0;
+
+	return drm_mode_config_helper_suspend(ddev);
+}
+
+void msm_kms_pm_complete(struct device *dev)
+{
+	struct msm_drm_private *priv = dev_get_drvdata(dev);
+	struct drm_device *ddev = priv ? priv->dev : NULL;
+
+	if (!priv || !priv->kms)
+		return;
+
+	drm_mode_config_helper_resume(ddev);
+}
+
+void msm_kms_shutdown(struct platform_device *pdev)
+{
+	struct msm_drm_private *priv = platform_get_drvdata(pdev);
+	struct drm_device *drm = priv ? priv->dev : NULL;
+
+	/*
+	 * Shutdown the hw if we're far enough along where things might be on.
+	 * If we run this too early, we'll end up panicking in any variety of
+	 * places. Since we don't register the drm device until late in
+	 * msm_drm_init, drm_dev->registered is used as an indicator that the
+	 * shutdown will be successful.
+	 */
+	if (drm && drm->registered && priv->kms)
+		drm_atomic_helper_shutdown(drm);
+}
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 086a3f1ff956..44aa435d68ce 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -195,4 +195,7 @@ static inline void msm_kms_destroy(struct msm_kms *kms)
 	drm_for_each_crtc_reverse(crtc, dev) \
 		for_each_if (drm_crtc_mask(crtc) & (crtc_mask))
 
+int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv);
+void msm_drm_kms_uninit(struct device *dev);
+
 #endif /* __MSM_KMS_H__ */
-- 
2.39.2