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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/05/2023 00:22, Abhinav Kumar wrote:


On 5/1/2023 1:45 PM, Dmitry Baryshkov wrote:
On 01/05/2023 22:58, Abhinav Kumar wrote:


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.

If msm_dsi_modeset_init() fails, the function will err out and fail dpu_kms initialization. So it's not important, what is the value of num_h_tiles in this case.


But I still feel the msm_display_info should be saved in the dpu encoder after the modeset_init() and not before. That way if some display interface specific init is done in the modeset_init(), we save the info after that.

Up to now we have been using 'poll' model, e.g. we specifically asked for the DSC info from the DSI host rather than making msm_dsi set it. So far I don't see a good reason why this should be changed.



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;
  }


--
With best wishes
Dmitry




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux