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@xxxxxxxxxxxxxx>
> ---
> .../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.
<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
<snip />
--
Sean Paul, Software Engineer, Google / Chromium OS
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
