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

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > ---
> > > >   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