[PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon May 1 19:58:15 UTC 2023
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 at linaro.org>
> ---
> 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;
> }
>
More information about the dri-devel
mailing list