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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

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.

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