[DPU PATCH 5/6] drm/msm: hook up DPU with upstream DSI

[Jeykumar Sankaran]

On 2018-04-19 08:38, Sean Paul wrote:
> On Mon, Apr 16, 2018 at 11:22:20AM -0700, Jeykumar Sankaran wrote:
>> Switch DPU from dsi-staging to upstream dsi driver. To make
>> the switch atomic, this change includes:
>> - remove dpu connector layers
>> - clean up dpu connector dependencies in encoder/crtc
>> - compile out writeback and display port drivers
>> - compile out dsi-staging driver (separate patch submitted to
>>   remove the driver)
>> - adapt upstream device hierarchy
>> 
>> Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
>> ---
>>  .../config/arm64/chromiumos-arm64.flavour.config   |    4 +-
>>  .../arm64/chromiumos-qualcomm.flavour.config       |    4 +-
> 
> These files aren't in upstream.
> 
> <snip />
> 
>> +
>> +struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
>> +		int drm_enc_mode)
>> +{
>> +	struct dpu_encoder_virt *dpu_enc = NULL;
>> +
>> +	dpu_enc = kzalloc(sizeof(*dpu_enc), GFP_KERNEL);
> 
> You should probably use devm_kzalloc.
> 
>> +	if (!dpu_enc)
>> +		return ERR_PTR(ENOMEM);
>> +
>> +	drm_encoder_init(dev, &dpu_enc->base, &dpu_encoder_funcs,
>> +			drm_enc_mode, NULL);
> 
> Check the return value?
> <snip />
> 
>> -#ifdef CONFIG_DRM_MSM_DSI_STAGING
>>  static void _dpu_kms_initialize_dsi(struct drm_device *dev,
>>  				    struct msm_drm_private *priv,
>> -				    struct dpu_kms *dpu_kms,
>> -				    unsigned max_encoders)
>> +				    struct dpu_kms *dpu_kms)
>>  {
>> -	static const struct dpu_connector_ops dsi_ops = {
>> -		.post_init =  dsi_conn_post_init,
>> -		.detect =     dsi_conn_detect,
>> -		.get_modes =  dsi_connector_get_modes,
>> -		.put_modes =  dsi_connector_put_modes,
>> -		.mode_valid = dsi_conn_mode_valid,
>> -		.get_info =   dsi_display_get_info,
>> -		.set_backlight = dsi_display_set_backlight,
>> -		.soft_reset   = dsi_display_soft_reset,
>> -		.pre_kickoff  = dsi_conn_pre_kickoff,
>> -		.clk_ctrl = dsi_display_clk_ctrl,
>> -		.set_power = dsi_display_set_power,
>> -		.get_mode_info = dsi_conn_get_mode_info,
>> -		.get_dst_format = dsi_display_get_dst_format,
>> -		.post_kickoff = dsi_conn_post_kickoff
>> -	};
>> -	struct msm_display_info info;
>> -	struct drm_encoder *encoder;
>> -	void *display, *connector;
>> +	struct drm_encoder *encoder = NULL;
>>  	int i, rc;
>> 
>> -	/* dsi */
>> -	for (i = 0; i < dpu_kms->dsi_display_count &&
>> -		priv->num_encoders < max_encoders; ++i) {
>> -		display = dpu_kms->dsi_displays[i];
>> -		encoder = NULL;
>> +	/*TODO: Support two independent DSI connectors */
>> +	encoder = dpu_encoder_init(dev, DRM_MODE_CONNECTOR_DSI);
>> +	if (IS_ERR_OR_NULL(encoder)) {
>> +		DPU_ERROR("encoder init failed for dsi %d\n", i);
> 
> Is i meaningful here? It seems like it's uninitialized...
> 
>> +		return;
>> +	}
>> 
>> -		memset(&info, 0x0, sizeof(info));
>> -		rc = dsi_display_get_info(&info, display);
>> -		if (rc) {
>> -			DPU_ERROR("dsi get_info %d failed\n", i);
>> -			continue;
>> -		}
>> +	priv->encoders[priv->num_encoders++] = encoder;
>> 
>> -		encoder = dpu_encoder_init(dev, &info);
>> -		if (IS_ERR_OR_NULL(encoder)) {
>> -			DPU_ERROR("encoder init failed for dsi %d\n", i);
>> -			continue;
>> +	for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) {
>> +		if (!priv->dsi[i]) {
>> +			DPU_DEBUG("invalid msm_dsi for ctrl %d\n", i);
>> +			return;
>>  		}
>> 
>> -		rc = dsi_display_drm_bridge_init(display, encoder);
>> +		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>  		if (rc) {
>> -			DPU_ERROR("dsi bridge %d init failed, %d\n", i,
> rc);
>> -			dpu_encoder_destroy(encoder);
>> +			DPU_ERROR("modeset_init failed for dsi[%d]\n", i);
> 
> Printing rc would be nice here, same for above.
> 
>>  			continue;
>>  		}
>> -
>> -		connector = dpu_connector_init(dev,
>> -					encoder,
>> -					0,
>> -					display,
>> -					&dsi_ops,
>> -					DRM_CONNECTOR_POLL_HPD,
>> -					DRM_MODE_CONNECTOR_DSI);
>> -		if (connector) {
>> -			priv->encoders[priv->num_encoders++] = encoder;
>> -		} else {
>> -			DPU_ERROR("dsi %d connector init failed\n", i);
>> -			dsi_display_drm_bridge_deinit(display);
>> -			dpu_encoder_destroy(encoder);
>> -		}
>>  	}
>>  }
>> -#endif
>> +
>> 
>>  #ifdef CONFIG_DRM_MSM_WRITEBACK
>>  static void _dpu_kms_initialize_wb(struct drm_device *dev,
>>  				   struct msm_drm_private *priv,
>>  				   struct dpu_kms *dpu_kms,
>> -				   unsigned max_encoders)
>> +				   unsigned int max_encoders)
>>  {
>>  	static const struct dpu_connector_ops wb_ops = {
>>  		.post_init =    dpu_wb_connector_post_init,
>> @@ -800,7 +710,7 @@ static void _dpu_kms_initialize_wb(struct 
>> drm_device
> *dev,
>>  static void _dpu_kms_initialize_dp(struct drm_device *dev,
>>  				   struct msm_drm_private *priv,
>>  				   struct dpu_kms *dpu_kms,
>> -				   unsigned max_encoders)
>> +				   unsigned int max_encoders)
> 
> These 2 type changes are just diff noise, please remove.
> 
>>  {
>>  	static const struct dpu_connector_ops dp_ops = {
>>  		.post_init  = dp_connector_post_init,
>> @@ -872,18 +782,7 @@ static void _dpu_kms_setup_displays(struct
> drm_device *dev,
>>  				    struct msm_drm_private *priv,
>>  				    struct dpu_kms *dpu_kms)
>>  {
>> -	unsigned max_encoders;
>> -
>> -	max_encoders = dpu_kms->dsi_display_count +
> dpu_kms->wb_display_count +
>> -				dpu_kms->dp_display_count;
>> -	if (max_encoders > ARRAY_SIZE(priv->encoders)) {
>> -		max_encoders = ARRAY_SIZE(priv->encoders);
>> -		DPU_ERROR("capping number of displays to %d",
> max_encoders);
>> -	}
>> -
>> -#ifdef CONFIG_DRM_MSM_DSI_STAGING
>> -	_dpu_kms_initialize_dsi(dev, priv, dpu_kms, max_encoders);
>> -#endif
>> +	_dpu_kms_initialize_dsi(dev, priv, dpu_kms);
>> 
>>  #ifdef CONFIG_DRM_MSM_WRITEBACK
>>  	_dpu_kms_initialize_wb(dev, priv, dpu_kms, max_encoders);
>> @@ -1521,6 +1420,7 @@ static int dpu_kms_atomic_check(struct msm_kms
> *kms,
>>  		dpu_kms->aspace[domain] : NULL;
>>  }
>> 
>> +#ifdef CONFIG_DRM_MSM_DISPLAYPORT
> 
> This seems suspicious. Why different behavior depending on DisplayPort?
> The
> function loops through all connectors, so this is something that should
> work
> regardless of whether DP is connected.
> 
The reason why this kms_func hook was introduced is to notify poll 
enabled
connectors (DP or HDMI) to send the HPD event notification through
DPU connector hook. Since the DPU connector layer is removed, this call
is meaningless.

To address your comments w.r.t the difference in handling for
non-dsi displays, I am planning to remove their handling codes from DPU 
but
retaining respective driver files.

> <snip />
>> @@ -2001,6 +1891,8 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>  		goto power_error;
>>  	}
>> 
>> +	if (dpu_kms->aspace[0])
>> +		kms->aspace = dpu_kms->aspace[0];
> 
> In future, stuff like this should be in a separate patch, instead of
> hidden in
> this huge glob. It seems like it's needed for the dsi driver, but I 
> almost
> missed it. Splitting this isolated stuff out makes it much easier for
> reviewers
> since you can explain why it's needed in the commit message vs me 
> having
> to look
> through the code to infer its usage.
> 
> Sean

Agreed. Let me try separating it out and move it down to the base.
> 
> <snip />

Will take care of rest of the comments in v2.
-- 
Jeykumar S