Re: [PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 4/30/2023 4:57 PM, Dmitry Baryshkov wrote:
> There is no reason to split the dpu_encoder interface into separate
> _init() and _setup() phases. Merge them into a single function.

I think the reason for having this split was to pass a valid encoder to 
the interface_modeset_init() and then do the rest of encoder 
initialization after modeset_init().

Looking at the current code, one issue i am seeing is that you will now 
initialize the dpu_encoder's msm_display_info along with dpu_encoder_init().

Most of it is fine but in the case of bonded_dsi(), I see an issue.

The info.num_of_h_tiles++ happens after the modeset_init() of the second 
dsi but now it has been moved earlier.

If for some reason, msm_dsi_modeset_init() fails for the second DSI, 
num_of_h_tiles will still be 2 now.

Even for multi-DP case, the idea originally was the encoder gets setup 
for that display after the modeset_init() of that display.

This might become more relevant for DSC perhaps. So lets say first 
encoder needs DSC and second doesnt, we would know that only post 
msm_dp_modeset_init().

So I think if you move the memcpy(&dpu_enc->disp_info, disp_info, 
sizeof(*disp_info)); line out to be done after modeset_init(), this 
change would look okay.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +++++--------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 14 +---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 87 ++++++++-------------
>   3 files changed, 56 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index b34416cbd0f5..32785cb1b079 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2377,7 +2377,8 @@ static const struct drm_encoder_funcs dpu_encoder_funcs = {
>   		.early_unregister = dpu_encoder_early_unregister,
>   };
>   
> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> +		int drm_enc_mode,
>   		struct msm_display_info *disp_info)
>   {
>   	struct msm_drm_private *priv = dev->dev_private;
> @@ -2386,7 +2387,23 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	struct dpu_encoder_virt *dpu_enc = NULL;
>   	int ret = 0;
>   
> -	dpu_enc = to_dpu_encoder_virt(enc);
> +	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
> +	if (!dpu_enc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
> +			       drm_enc_mode, NULL);
> +	if (ret) {
> +		devm_kfree(dev->dev, dpu_enc);
> +		return ERR_PTR(ret);
> +	}
> +
> +	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> +
> +	spin_lock_init(&dpu_enc->enc_spinlock);
> +	dpu_enc->enabled = false;
> +	mutex_init(&dpu_enc->enc_lock);
> +	mutex_init(&dpu_enc->rc_lock);
>   
>   	ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
>   	if (ret)
> @@ -2415,44 +2432,14 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   
>   	DPU_DEBUG_ENC(dpu_enc, "created\n");
>   
> -	return ret;
> +	return &dpu_enc->base;
>   
>   fail:
>   	DPU_ERROR("failed to create encoder\n");
>   	if (drm_enc)
>   		dpu_encoder_destroy(drm_enc);
>   
> -	return ret;
> -
> -
> -}
> -
> -struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> -		int drm_enc_mode)
> -{
> -	struct dpu_encoder_virt *dpu_enc = NULL;
> -	int rc = 0;
> -
> -	dpu_enc = devm_kzalloc(dev->dev, sizeof(*dpu_enc), GFP_KERNEL);
> -	if (!dpu_enc)
> -		return ERR_PTR(-ENOMEM);
> -
> -
> -	rc = drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
> -							  drm_enc_mode, NULL);
> -	if (rc) {
> -		devm_kfree(dev->dev, dpu_enc);
> -		return ERR_PTR(rc);
> -	}
> -
> -	drm_encoder_helper_add(&dpu_enc->base, &dpu_encoder_helper_funcs);
> -
> -	spin_lock_init(&dpu_enc->enc_spinlock);
> -	dpu_enc->enabled = false;
> -	mutex_init(&dpu_enc->enc_lock);
> -	mutex_init(&dpu_enc->rc_lock);
> -
> -	return &dpu_enc->base;
> +	return ERR_PTR(ret);
>   }
>   
>   int dpu_encoder_wait_for_event(struct drm_encoder *drm_enc,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 6d14f84dd43f..90e1925d7770 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -130,20 +130,12 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *encoder);
>   /**
>    * dpu_encoder_init - initialize virtual encoder object
>    * @dev:        Pointer to drm device structure
> + * @drm_enc_mode: corresponding DRM_MODE_ENCODER_* constant
>    * @disp_info:  Pointer to display information structure
>    * Returns:     Pointer to newly created drm encoder
>    */
> -struct drm_encoder *dpu_encoder_init(
> -		struct drm_device *dev,
> -		int drm_enc_mode);
> -
> -/**
> - * dpu_encoder_setup - setup dpu_encoder for the display probed
> - * @dev:		Pointer to drm device structure
> - * @enc:		Pointer to the drm_encoder
> - * @disp_info:	Pointer to the display info
> - */
> -int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
> +		int drm_enc_mode,
>   		struct msm_display_info *disp_info);
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e1fd7b0f39cd..10bd0fd4ff48 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -535,15 +535,23 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   		    !msm_dsi_is_master_dsi(priv->dsi[i]))
>   			continue;
>   
> -		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI);
> +		memset(&info, 0, sizeof(info));
> +		info.intf_type = INTF_DSI;
> +
> +		info.h_tile_instance[info.num_of_h_tiles++] = i;
> +		if (msm_dsi_is_bonded_dsi(priv->dsi[i]))
> +			info.h_tile_instance[info.num_of_h_tiles++] = other;
> +
> +		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
> +
> +		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
> +
> +		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
>   		if (IS_ERR(encoder)) {
>   			DPU_ERROR("encoder init failed for dsi display\n");
>   			return PTR_ERR(encoder);
>   		}
>   
> -		memset(&info, 0, sizeof(info));
> -		info.intf_type = INTF_DSI;
> -
>   		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>   		if (rc) {
>   			DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n",
> @@ -551,11 +559,6 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   			break;
>   		}
>   
> -		info.h_tile_instance[info.num_of_h_tiles++] = i;
> -		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
> -
> -		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
> -
>   		if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) {
>   			rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder);
>   			if (rc) {
> @@ -563,14 +566,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   					other, rc);
>   				break;
>   			}
> -
> -			info.h_tile_instance[info.num_of_h_tiles++] = other;
>   		}
> -
> -		rc = dpu_encoder_setup(dev, encoder, &info);
> -		if (rc)
> -			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
>   	}
>   
>   	return rc;
> @@ -589,29 +585,23 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>   		if (!priv->dp[i])
>   			continue;
>   
> -		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> +		memset(&info, 0, sizeof(info));
> +		info.num_of_h_tiles = 1;
> +		info.h_tile_instance[0] = i;
> +		info.intf_type = INTF_DP;
> +
> +		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>   		if (IS_ERR(encoder)) {
>   			DPU_ERROR("encoder init failed for dsi display\n");
>   			return PTR_ERR(encoder);
>   		}
>   
> -		memset(&info, 0, sizeof(info));
>   		rc = msm_dp_modeset_init(priv->dp[i], dev, encoder);
>   		if (rc) {
>   			DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
>   			drm_encoder_cleanup(encoder);
>   			return rc;
>   		}
> -
> -		info.num_of_h_tiles = 1;
> -		info.h_tile_instance[0] = i;
> -		info.intf_type = INTF_DP;
> -		rc = dpu_encoder_setup(dev, encoder, &info);
> -		if (rc) {
> -			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
> -			return rc;
> -		}
>   	}
>   
>   	return 0;
> @@ -629,13 +619,17 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
>   	if (!priv->hdmi)
>   		return 0;
>   
> -	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
> +	memset(&info, 0, sizeof(info));
> +	info.num_of_h_tiles = 1;
> +	info.h_tile_instance[0] = i;
> +	info.intf_type = INTF_HDMI;
> +
> +	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS, &info);
>   	if (IS_ERR(encoder)) {
>   		DPU_ERROR("encoder init failed for HDMI display\n");
>   		return PTR_ERR(encoder);
>   	}
>   
> -	memset(&info, 0, sizeof(info));
>   	rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
>   	if (rc) {
>   		DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
> @@ -643,16 +637,6 @@ static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
>   		return rc;
>   	}
>   
> -	info.num_of_h_tiles = 1;
> -	info.h_tile_instance[0] = i;
> -	info.intf_type = INTF_HDMI;
> -	rc = dpu_encoder_setup(dev, encoder, &info);
> -	if (rc) {
> -		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -			  encoder->base.id, rc);
> -		return rc;
> -	}
> -
>   	return 0;
>   }
>   
> @@ -664,14 +648,19 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
>   	struct msm_display_info info;
>   	int rc;
>   
> -	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL);
> +	memset(&info, 0, sizeof(info));
> +
> +	info.num_of_h_tiles = 1;
> +	/* use only WB idx 2 instance for DPU */
> +	info.h_tile_instance[0] = WB_2;
> +	info.intf_type = INTF_WB;
> +
> +	encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_VIRTUAL, &info);
>   	if (IS_ERR(encoder)) {
>   		DPU_ERROR("encoder init failed for dsi display\n");
>   		return PTR_ERR(encoder);
>   	}
>   
> -	memset(&info, 0, sizeof(info));
> -
>   	rc = dpu_writeback_init(dev, encoder, wb_formats,
>   			n_formats);
>   	if (rc) {
> @@ -680,18 +669,6 @@ static int _dpu_kms_initialize_writeback(struct drm_device *dev,
>   		return rc;
>   	}
>   
> -	info.num_of_h_tiles = 1;
> -	/* use only WB idx 2 instance for DPU */
> -	info.h_tile_instance[0] = WB_2;
> -	info.intf_type = INTF_WB;
> -
> -	rc = dpu_encoder_setup(dev, encoder, &info);
> -	if (rc) {
> -		DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> -				  encoder->base.id, rc);
> -		return rc;
> -	}
> -
>   	return 0;
>   }
>