[PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
Abhinav Kumar
quic_abhinavk at quicinc.com
Mon May 1 21:22:43 UTC 2023
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 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