[PATCH 1/7] drm/msm/dpu: merge dpu_encoder_init() and dpu_encoder_setup()
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Mon May 1 21:27:18 UTC 2023
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 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