On 2021-07-10 12:38, Dmitry Baryshkov wrote:
On 10/07/2021 03:30, abhinavk@xxxxxxxxxxxxxx wrote:On 2021-07-09 16:50, Dmitry Baryshkov wrote:Move setting up encoders from set_encoder_mode to _dpu_kms_initialize_dsi() / _dpu_kms_initialize_displayport(). Thisallows us to support not only "single DSI" and "bonded DSI" but also "two independent DSI" configurations. In future this would also help addingsupport for multiple DP connectors. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> ---drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 130 ++++++++++++++----------1 file changed, 79 insertions(+), 51 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 1d3a4f395e74..e14eb8f94cd7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -466,17 +466,16 @@ static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask) dpu_kms_wait_for_commit_done(kms, crtc); } -static int _dpu_kms_initialize_dsi(struct drm_device *dev, - struct msm_drm_private *priv, - struct dpu_kms *dpu_kms) +static int _dpu_kms_initialize_dsi_encoder(struct drm_device *dev, + struct msm_drm_private *priv, + struct dpu_kms *dpu_kms, + int dsi_id, int dsi_id1) { + struct msm_dsi *dsi = priv->dsi[dsi_id]; struct drm_encoder *encoder = NULL; - int i, rc = 0; - - if (!(priv->dsi[0] || priv->dsi[1])) - return rc; + struct msm_display_info info; + int rc = 0; - /*TODO: Support two independent DSI connectors */ encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI); if (IS_ERR(encoder)) { DPU_ERROR("encoder init failed for dsi display\n");@@ -485,19 +484,74 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,priv->encoders[priv->num_encoders++] = encoder; - for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { - if (!priv->dsi[i]) - continue; + rc = msm_dsi_modeset_init(dsi, dev, encoder); + if (rc) { + DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n", + dsi_id, rc); + return rc; + } + + memset(&info, 0, sizeof(info)); + info.intf_type = encoder->encoder_type; + info.capabilities = msm_dsi_is_cmd_mode(dsi) ? + MSM_DISPLAY_CAP_CMD_MODE : + MSM_DISPLAY_CAP_VID_MODE; + info.h_tile_instance[info.num_of_h_tiles++] = dsi_id; - rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder); + /* For the bonded DSI setup we have second DSI host */ + if (dsi_id1 >= 0) { + struct msm_dsi *dsi1 = priv->dsi[dsi_id1]; + + rc = msm_dsi_modeset_init(dsi1, dev, encoder); if (rc) { DPU_ERROR("modeset_init failed for dsi[%d], rc = %d\n", - i, rc); - break; + dsi_id1, rc); + return rc; } + + info.h_tile_instance[info.num_of_h_tiles++] = dsi_id1; } - return rc; + 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; +} + +static int _dpu_kms_initialize_dsi(struct drm_device *dev, + struct msm_drm_private *priv, + struct dpu_kms *dpu_kms) +{ + int i, rc = 0; + + if (!(priv->dsi[0] || priv->dsi[1])) + return rc; + + /* + * We support following confiurations: + * - Single DSI host (dsi0 or dsi1) + * - Two independent DSI hosts + * - Bonded DSI0 and DSI1 hosts + * + * TODO: Support swapping DSI0 and DSI1 in the bonded setup. + */+ if (priv->dsi[0] && priv->dsi[1] && msm_dsi_is_bonded_dsi(priv->dsi[0])) + return _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, 0, 1);+ + for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { + if (!priv->dsi[i]) + continue; ++ rc = _dpu_kms_initialize_dsi_encoder(dev, priv, dpu_kms, i, -1);+ if (rc) + return rc; + } + + return 0; }Can we simplify this a bit like below? static int _dpu_kms_initialize_dsi(struct drm_device *dev, struct msm_drm_private *priv, struct dpu_kms *dpu_kms) { int i, rc = 0; if (!(priv->dsi[0] || priv->dsi[1])) return rc; /* * We support following confiurations: * - Single DSI host (dsi0 or dsi1) * - Two independent DSI hosts * - Bonded DSI0 and DSI1 hosts * * TODO: Support swapping DSI0 and DSI1 in the bonded setup. for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { if (!priv->dsi[i]) continue;rc = _dpu_kms_initialize_dsi(dev, priv, dpu_kms); // this API does everything except encoder setupif (rc) return rc; if (!is_bonded_dsi) _dpu_kms_initialize_dsi_encoder(...); else if (dsi_0) // only one encoder setup for dsi_0 _dpu_kms_initialize_dsi_encoder(...); } } Let me know if you think this is a little less complicated.I don't think this will work out of the box, currently we pass encoder to DSI initialization (modeset_init). Adding extra cases in the middle of the loop also doesn't seem like a clear solution.
In that case the previous attempt was actually a little better with the change I suggested of using the msm_dsi_is_bonded_dsi() API instead of hard-coding the encoder to NULL.
This one has some additional changes like passing 1 and/or -1 of the dsi1. Just felt a bit of an
overkill. The previous one was better than this one.
static int _dpu_kms_initialize_displayport(struct drm_device *dev, @@ -505,6 +559,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, struct dpu_kms *dpu_kms) { struct drm_encoder *encoder = NULL; + struct msm_display_info info; int rc = 0; if (!priv->dp) @@ -516,6 +571,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev, return PTR_ERR(encoder); } + memset(&info, 0, sizeof(info)); rc = msm_dp_modeset_init(priv->dp, dev, encoder); if (rc) { DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);@@ -524,6 +580,14 @@ static int _dpu_kms_initialize_displayport(structdrm_device *dev, } priv->encoders[priv->num_encoders++] = encoder; + + info.num_of_h_tiles = 1; + info.capabilities = MSM_DISPLAY_CAP_VID_MODE; + info.intf_type = encoder->encoder_type; + 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; } @@ -726,41 +790,6 @@ static void dpu_kms_destroy(struct msm_kms *kms) msm_kms_destroy(&dpu_kms->base); } -static void _dpu_kms_set_encoder_mode(struct msm_kms *kms, - struct drm_encoder *encoder, - bool cmd_mode) -{ - struct msm_display_info info; - struct msm_drm_private *priv = encoder->dev->dev_private; - int i, rc = 0; - - memset(&info, 0, sizeof(info)); - - info.intf_type = encoder->encoder_type; - info.capabilities = cmd_mode ? MSM_DISPLAY_CAP_CMD_MODE : - MSM_DISPLAY_CAP_VID_MODE; - - switch (info.intf_type) { - case DRM_MODE_ENCODER_DSI: - /* TODO: No support for DSI swap */ - for (i = 0; i < ARRAY_SIZE(priv->dsi); i++) { - if (priv->dsi[i]) { - info.h_tile_instance[info.num_of_h_tiles] = i; - info.num_of_h_tiles++; - } - } - break; - case DRM_MODE_ENCODER_TMDS: - info.num_of_h_tiles = 1; - break; - } - - rc = dpu_encoder_setup(encoder->dev, encoder, &info); - if (rc) - DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n", - encoder->base.id, rc); -} - static irqreturn_t dpu_irq(struct msm_kms *kms) { struct dpu_kms *dpu_kms = to_dpu_kms(kms); @@ -863,7 +892,6 @@ static const struct msm_kms_funcs kms_funcs = { .get_format = dpu_get_msm_format, .round_pixclk = dpu_kms_round_pixclk, .destroy = dpu_kms_destroy, - .set_encoder_mode = _dpu_kms_set_encoder_mode, .snapshot = dpu_kms_mdp_snapshot, #ifdef CONFIG_DEBUG_FS .debugfs_init = dpu_kms_debugfs_init,
