diff mbox series

[v3,2/3] drm/msm/dpu: handle failures while initializing displays

Message ID 1544727064-26763-2-git-send-email-jsanka@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series [v3,1/3] drm/msm/dpu: fix documentation for intf_type | expand

Commit Message

Jeykumar Sankaran Dec. 13, 2018, 6:51 p.m. UTC
Bail out KMS hw init on display initialization failures with
proper error logging.

changes in v3:
	- introduced in the series

Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39 +++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 12 deletions(-)

Comments

Sean Paul Dec. 14, 2018, 3:22 p.m. UTC | #1
On Thu, Dec 13, 2018 at 10:51:03AM -0800, Jeykumar Sankaran wrote:
> Bail out KMS hw init on display initialization failures with
> proper error logging.
> 
> changes in v3:
> 	- introduced in the series
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39 +++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 685686e..39c8549 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -405,33 +405,36 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
>  	}
>  }
>  
> -static void _dpu_kms_initialize_dsi(struct drm_device *dev,
> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>  				    struct msm_drm_private *priv,
>  				    struct dpu_kms *dpu_kms)
>  {
>  	struct drm_encoder *encoder = NULL;
> -	int i, rc;
> +	int i, rc = 0;
> +
> +	if (!(priv->dsi[0] || priv->dsi[1]))

It'd be nice to not have to support both blocks if only one is hooked up.
Perhaps move the TODO below above this check.

> +		return rc;
>  
>  	/*TODO: Support two independent DSI connectors */
>  	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> -	if (IS_ERR_OR_NULL(encoder)) {
> +	if (IS_ERR(encoder)) {
>  		DPU_ERROR("encoder init failed for dsi display\n");
> -		return;
> +		return PTR_ERR(encoder);
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> -		if (!priv->dsi[i]) {
> -			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
> -			return;
> -		}
> +		if (!priv->dsi[i])

I don't think this is possible given the check at the beginning of the function

> +			continue;
>  
>  		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>  		if (rc) {
>  			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>  				i, rc);
> -			continue;
> +			return rc;
>  		}
>  	}
> +
> +	return rc;

nit: You can keep rc uninitialized above and just return 0 here.

>  }
>  
>  /**
> @@ -442,16 +445,24 @@ static void _dpu_kms_initialize_dsi(struct drm_device *dev,
>   * @dpu_kms:    Pointer to dpu kms structure
>   * Returns:     Zero on success
>   */
> -static void _dpu_kms_setup_displays(struct drm_device *dev,
> +static int _dpu_kms_setup_displays(struct drm_device *dev,
>  				    struct msm_drm_private *priv,
>  				    struct dpu_kms *dpu_kms)
>  {
> -	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
> +	int rc = 0;

nit: No need to initialize to 0

> +
> +	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
> +	if (rc) {
> +		DPU_ERROR("failed to initialize dsi, rc = %d\n", rc);
> +		return rc;
> +	}
>  
>  	/**
>  	 * Extend this function to initialize other
>  	 * types of displays
>  	 */
> +
> +	return rc;
>  }
>  
>  static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
> @@ -517,7 +528,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  	 * Create encoder and query display drivers to create
>  	 * bridges and connectors
>  	 */
> -	_dpu_kms_setup_displays(dev, priv, dpu_kms);
> +	ret = _dpu_kms_setup_displays(dev, priv, dpu_kms);
> +	if (ret) {
> +		DPU_ERROR("failed to setup display, rc = %d\n", ret);
> +		goto fail;
> +	}
>  
>  	max_crtc_count = min(catalog->mixer_count,
>  			     (u32)dev->mode_config.num_encoder);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
Jordan Crouse Dec. 14, 2018, 3:36 p.m. UTC | #2
On Thu, Dec 13, 2018 at 10:51:03AM -0800, Jeykumar Sankaran wrote:
> Bail out KMS hw init on display initialization failures with
> proper error logging.
> 
> changes in v3:
> 	- introduced in the series
> 
> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39 +++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 685686e..39c8549 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -405,33 +405,36 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
>  	}
>  }
>  
> -static void _dpu_kms_initialize_dsi(struct drm_device *dev,
> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>  				    struct msm_drm_private *priv,
>  				    struct dpu_kms *dpu_kms)
>  {
>  	struct drm_encoder *encoder = NULL;
> -	int i, rc;
> +	int i, rc = 0;
> +
> +	if (!(priv->dsi[0] || priv->dsi[1]))
> +		return rc;
>  
>  	/*TODO: Support two independent DSI connectors */
>  	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> -	if (IS_ERR_OR_NULL(encoder)) {
> +	if (IS_ERR(encoder)) {
>  		DPU_ERROR("encoder init failed for dsi display\n");
> -		return;
> +		return PTR_ERR(encoder);
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
> -		if (!priv->dsi[i]) {
> -			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
> -			return;
> -		}
> +		if (!priv->dsi[i])
> +			continue;
>  
>  		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>  		if (rc) {
>  			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
>  				i, rc);
> -			continue;
> +			return rc;
>  		}
>  	}
> +
> +	return rc;

This also looks like one of those cases where you can drop out of the if
statement to return rc.
>  }
>  
>  /**
> @@ -442,16 +445,24 @@ static void _dpu_kms_initialize_dsi(struct drm_device *dev,
>   * @dpu_kms:    Pointer to dpu kms structure
>   * Returns:     Zero on success
>   */
> -static void _dpu_kms_setup_displays(struct drm_device *dev,
> +static int _dpu_kms_setup_displays(struct drm_device *dev,
>  				    struct msm_drm_private *priv,
>  				    struct dpu_kms *dpu_kms)
>  {
> -	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
> +	int rc = 0;
> +
> +	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
> +	if (rc) {
> +		DPU_ERROR("failed to initialize dsi, rc = %d\n", rc);
> +		return rc;
> +	}
>  
>  	/**
>  	 * Extend this function to initialize other
>  	 * types of displays
>  	 */
> +
> +	return rc;

Here too.

>  }
>  
>  static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
> @@ -517,7 +528,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>  	 * Create encoder and query display drivers to create
>  	 * bridges and connectors
>  	 */
> -	_dpu_kms_setup_displays(dev, priv, dpu_kms);
> +	ret = _dpu_kms_setup_displays(dev, priv, dpu_kms);
> +	if (ret) {
> +		DPU_ERROR("failed to setup display, rc = %d\n", ret);

You don't need to pile on with another error message.

> +		goto fail;
> +	}
>  
>  	max_crtc_count = min(catalog->mixer_count,
>  			     (u32)dev->mode_config.num_encoder);
Jeykumar Sankaran Dec. 17, 2018, 7:35 p.m. UTC | #3
On 2018-12-14 07:22, Sean Paul wrote:
> On Thu, Dec 13, 2018 at 10:51:03AM -0800, Jeykumar Sankaran wrote:
>> Bail out KMS hw init on display initialization failures with
>> proper error logging.
>> 
>> changes in v3:
>> 	- introduced in the series
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39
> +++++++++++++++++++++++----------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 685686e..39c8549 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -405,33 +405,36 @@ static void dpu_kms_wait_for_commit_done(struct
> msm_kms *kms,
>>  	}
>>  }
>> 
>> -static void _dpu_kms_initialize_dsi(struct drm_device *dev,
>> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>  				    struct msm_drm_private *priv,
>>  				    struct dpu_kms *dpu_kms)
>>  {
>>  	struct drm_encoder *encoder = NULL;
>> -	int i, rc;
>> +	int i, rc = 0;
>> +
>> +	if (!(priv->dsi[0] || priv->dsi[1]))
> 
> It'd be nice to not have to support both blocks if only one is hooked 
> up.
> Perhaps move the TODO below above this check.
> 
For dual DSI panels (e.g: MTP panels), both of these blocks will be 
populated.
TODO comment below is to convey that the support for two independent 
single DSI
panels is not yet available in DPU.

Thanks and Regards,
Jeykumar S.
>> +		return rc;
>> 
>>  	/*TODO: Support two independent DSI connectors */
>>  	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>> -	if (IS_ERR_OR_NULL(encoder)) {
>> +	if (IS_ERR(encoder)) {
>>  		DPU_ERROR("encoder init failed for dsi display\n");
>> -		return;
>> +		return PTR_ERR(encoder);
>>  	}
>> 
>>  	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>> -		if (!priv->dsi[i]) {
>> -			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
>> -			return;
>> -		}
>> +		if (!priv->dsi[i])
> 
> I don't think this is possible given the check at the beginning of the
> function
> 
>> +			continue;
>> 
>>  		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>  		if (rc) {
>>  			DPU_ERROR("modeset_init failed for dsi[%d], rc =
> %d\n",
>>  				i, rc);
>> -			continue;
>> +			return rc;
>>  		}
>>  	}
>> +
>> +	return rc;
> 
> nit: You can keep rc uninitialized above and just return 0 here.
> 
>>  }
>> 
>>  /**
>> @@ -442,16 +445,24 @@ static void _dpu_kms_initialize_dsi(struct
> drm_device *dev,
>>   * @dpu_kms:    Pointer to dpu kms structure
>>   * Returns:     Zero on success
>>   */
>> -static void _dpu_kms_setup_displays(struct drm_device *dev,
>> +static int _dpu_kms_setup_displays(struct drm_device *dev,
>>  				    struct msm_drm_private *priv,
>>  				    struct dpu_kms *dpu_kms)
>>  {
>> -	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>> +	int rc = 0;
> 
> nit: No need to initialize to 0
> 
>> +
>> +	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>> +	if (rc) {
>> +		DPU_ERROR("failed to initialize dsi, rc = %d\n", rc);
>> +		return rc;
>> +	}
>> 
>>  	/**
>>  	 * Extend this function to initialize other
>>  	 * types of displays
>>  	 */
>> +
>> +	return rc;
>>  }
>> 
>>  static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
>> @@ -517,7 +528,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
>>  	 * Create encoder and query display drivers to create
>>  	 * bridges and connectors
>>  	 */
>> -	_dpu_kms_setup_displays(dev, priv, dpu_kms);
>> +	ret = _dpu_kms_setup_displays(dev, priv, dpu_kms);
>> +	if (ret) {
>> +		DPU_ERROR("failed to setup display, rc = %d\n", ret);
>> +		goto fail;
>> +	}
>> 
>>  	max_crtc_count = min(catalog->mixer_count,
>>  			     (u32)dev->mode_config.num_encoder);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
Jeykumar Sankaran Dec. 17, 2018, 8:27 p.m. UTC | #4
On 2018-12-14 07:22, Sean Paul wrote:
> On Thu, Dec 13, 2018 at 10:51:03AM -0800, Jeykumar Sankaran wrote:
>> Bail out KMS hw init on display initialization failures with
>> proper error logging.
>> 
>> changes in v3:
>> 	- introduced in the series
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org>
>> ---
>>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 39
> +++++++++++++++++++++++----------
>>  1 file changed, 27 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 685686e..39c8549 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -405,33 +405,36 @@ static void dpu_kms_wait_for_commit_done(struct
> msm_kms *kms,
>>  	}
>>  }
>> 
>> -static void _dpu_kms_initialize_dsi(struct drm_device *dev,
>> +static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>>  				    struct msm_drm_private *priv,
>>  				    struct dpu_kms *dpu_kms)
>>  {
>>  	struct drm_encoder *encoder = NULL;
>> -	int i, rc;
>> +	int i, rc = 0;
>> +
>> +	if (!(priv->dsi[0] || priv->dsi[1]))
> 
> It'd be nice to not have to support both blocks if only one is hooked 
> up.
> Perhaps move the TODO below above this check.
> 
>> +		return rc;
>> 
>>  	/*TODO: Support two independent DSI connectors */
>>  	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
>> -	if (IS_ERR_OR_NULL(encoder)) {
>> +	if (IS_ERR(encoder)) {
>>  		DPU_ERROR("encoder init failed for dsi display\n");
>> -		return;
>> +		return PTR_ERR(encoder);
>>  	}
>> 
>>  	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>> -		if (!priv->dsi[i]) {
>> -			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
>> -			return;
>> -		}
>> +		if (!priv->dsi[i])
> 
> I don't think this is possible given the check at the beginning of the
> function
> 
The check at the beginning returns only if BOTH the dsi blocks are NULL.
In case of full hd panel, only one of the dsi block will be active. The
above check is needed to return on the inactive dsi.

Thanks,
Jeykumar S.
>> +			continue;
>> 
>>  		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>  		if (rc) {
>>  			DPU_ERROR("modeset_init failed for dsi[%d], rc =
> %d\n",
>>  				i, rc);
>> -			continue;
>> +			return rc;
>>  		}
>>  	}
>> +
>> +	return rc;
> 
> nit: You can keep rc uninitialized above and just return 0 here.
> 
>>  }
>> 
>>  /**
>> @@ -442,16 +445,24 @@ static void _dpu_kms_initialize_dsi(struct
> drm_device *dev,
>>   * @dpu_kms:    Pointer to dpu kms structure
>>   * Returns:     Zero on success
>>   */
>> -static void _dpu_kms_setup_displays(struct drm_device *dev,
>> +static int _dpu_kms_setup_displays(struct drm_device *dev,
>>  				    struct msm_drm_private *priv,
>>  				    struct dpu_kms *dpu_kms)
>>  {
>> -	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>> +	int rc = 0;
> 
> nit: No need to initialize to 0
> 
>> +
>> +	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>> +	if (rc) {
>> +		DPU_ERROR("failed to initialize dsi, rc = %d\n", rc);
>> +		return rc;
>> +	}
>> 
>>  	/**
>>  	 * Extend this function to initialize other
>>  	 * types of displays
>>  	 */
>> +
>> +	return rc;
>>  }
>> 
>>  static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
>> @@ -517,7 +528,11 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
> *dpu_kms)
>>  	 * Create encoder and query display drivers to create
>>  	 * bridges and connectors
>>  	 */
>> -	_dpu_kms_setup_displays(dev, priv, dpu_kms);
>> +	ret = _dpu_kms_setup_displays(dev, priv, dpu_kms);
>> +	if (ret) {
>> +		DPU_ERROR("failed to setup display, rc = %d\n", ret);
>> +		goto fail;
>> +	}
>> 
>>  	max_crtc_count = min(catalog->mixer_count,
>>  			     (u32)dev->mode_config.num_encoder);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> _______________________________________________
>> Freedreno mailing list
>> Freedreno@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/freedreno
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 685686e..39c8549 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -405,33 +405,36 @@  static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
 	}
 }
 
-static void _dpu_kms_initialize_dsi(struct drm_device *dev,
+static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
 {
 	struct drm_encoder *encoder = NULL;
-	int i, rc;
+	int i, rc = 0;
+
+	if (!(priv->dsi[0] || priv->dsi[1]))
+		return rc;
 
 	/*TODO: Support two independent DSI connectors */
 	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
-	if (IS_ERR_OR_NULL(encoder)) {
+	if (IS_ERR(encoder)) {
 		DPU_ERROR("encoder init failed for dsi display\n");
-		return;
+		return PTR_ERR(encoder);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
-		if (!priv->dsi[i]) {
-			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
-			return;
-		}
+		if (!priv->dsi[i])
+			continue;
 
 		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
 			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
 				i, rc);
-			continue;
+			return rc;
 		}
 	}
+
+	return rc;
 }
 
 /**
@@ -442,16 +445,24 @@  static void _dpu_kms_initialize_dsi(struct drm_device *dev,
  * @dpu_kms:    Pointer to dpu kms structure
  * Returns:     Zero on success
  */
-static void _dpu_kms_setup_displays(struct drm_device *dev,
+static int _dpu_kms_setup_displays(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
 {
-	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
+	int rc = 0;
+
+	rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms);
+	if (rc) {
+		DPU_ERROR("failed to initialize dsi, rc = %d\n", rc);
+		return rc;
+	}
 
 	/**
 	 * Extend this function to initialize other
 	 * types of displays
 	 */
+
+	return rc;
 }
 
 static void _dpu_kms_drm_obj_destroy(struct dpu_kms *dpu_kms)
@@ -517,7 +528,11 @@  static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 	 * Create encoder and query display drivers to create
 	 * bridges and connectors
 	 */
-	_dpu_kms_setup_displays(dev, priv, dpu_kms);
+	ret = _dpu_kms_setup_displays(dev, priv, dpu_kms);
+	if (ret) {
+		DPU_ERROR("failed to setup display, rc = %d\n", ret);
+		goto fail;
+	}
 
 	max_crtc_count = min(catalog->mixer_count,
 			     (u32)dev->mode_config.num_encoder);