| Message ID | 20230430235732.3341119-2-dmitry.baryshkov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: simplify DPU encoder init | expand |
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; >> }
On 02/05/2023 00:22, Abhinav Kumar wrote: > > > 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. Up to now we have been using 'poll' model, e.g. we specifically asked for the DSC info from the DSI host rather than making msm_dsi set it. So far I don't see a good reason why this should be changed. > >>> >>> 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; >>>> } >>
On 5/1/2023 2:27 PM, Dmitry Baryshkov wrote: > On 02/05/2023 00:22, Abhinav Kumar wrote: >> >> >> 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. > > Up to now we have been using 'poll' model, e.g. we specifically asked > for the DSC info from the DSI host rather than making msm_dsi set it. So > far I don't see a good reason why this should be changed. > Ok got it, so my concern came from the fact that we individually poll each feature today but lets say the number of features keeps growing we will have to combine them all into xxx_xxx_get_disp_info() which fills up all the fields of the display_info in one go. But yes, as long as we do that before calling dpu_encoder_init() it should be fine. Hence, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
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; }
There is no reason to split the dpu_encoder interface into separate _init() and _setup() phases. Merge them into a single function. 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(-)