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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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.

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

-- 
With best wishes
Dmitry