[PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Mon May 1 20:45:05 UTC 2023
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 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;
>> }
--
With best wishes
Dmitry
More information about the dri-devel
mailing list