| Message ID | 1644009445-17320-7-git-send-email-quic_abhinavk@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add writeback block support for DPU | expand |
On 05/02/2022 00:17, Abhinav Kumar wrote: > Make changes to dpu_encoder to support virtual encoder needed > to support writeback for dpu. This patch will change significantly if > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++++++++-------- > 1 file changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index e977c05..947069b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > + enum dpu_hw_blk_type blk_type; > int num_lm, num_ctl, num_pp; > int i, j; > > @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > phys->hw_pp = dpu_enc->hw_pp[i]; > phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); > > + if (phys->intf_mode == INTF_MODE_WB_LINE) > + blk_type = DPU_HW_BLK_WB; > + else > + blk_type = DPU_HW_BLK_INTF; > + > num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, > - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, > + global_state, drm_enc->base.id, blk_type, > hw_blk, ARRAY_SIZE(hw_blk)); > - for (j = 0; j < num_blk; j++) { > - struct dpu_hw_intf *hw_intf; > > - hw_intf = to_dpu_hw_intf(hw_blk[i]); > - if (hw_intf->idx == phys->intf_idx) > - phys->hw_intf = hw_intf; > + if (blk_type == DPU_HW_BLK_WB) { > + for (j = 0; j < num_blk; j++) { > + struct dpu_hw_wb *hw_wb; > + > + hw_wb = to_dpu_hw_wb(hw_blk[i]); > + if (hw_wb->idx == phys->intf_idx) > + phys->hw_wb = hw_wb; > + } > + } else { > + for (j = 0; j < num_blk; j++) { > + struct dpu_hw_intf *hw_intf; > + > + hw_intf = to_dpu_hw_intf(hw_blk[i]); > + if (hw_intf->idx == phys->intf_idx) > + phys->hw_intf = hw_intf; > + } > } I think that if we sequentially call dpu_rm_get_assigned_resources(.., DPU_HW_BLK_INTF, ...) and then dpu_rm_get_assigned_resources(.., DPU_HW_BLK_WB, ...), the code would be cleaner. Or even better get the WB direclty using the provided ID. > > - if (!phys->hw_intf) { > + if (!phys->hw_intf && !phys->hw_wb) { > DPU_ERROR_ENC(dpu_enc, > - "no intf block assigned at idx: %d\n", i); > + "no intf or WB block assigned at idx: %d\n", i); > return; > } > > @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > mutex_unlock(&dpu_enc->enc_lock); > } > > -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog, > enum dpu_intf_type type, u32 controller_id) > { > int i = 0; > > - for (i = 0; i < catalog->intf_count; i++) { > - if (catalog->intf[i].type == type > - && catalog->intf[i].controller_id == controller_id) { > - return catalog->intf[i].id; > + if (type != INTF_WB) { > + for (i = 0; i < catalog->intf_count; i++) { > + if (catalog->intf[i].type == type > + && catalog->intf[i].controller_id == controller_id) { > + return catalog->intf[i].id; > + } > + } > + } else { > + for (i = 0; i < catalog->wb_count; i++) { > + if (catalog->wb[i].id == controller_id) > + return catalog->wb[i].id; > } > } > > @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > case DRM_MODE_ENCODER_TMDS: > intf_type = INTF_DP; > break; > + case DRM_MODE_ENCODER_VIRTUAL: > + intf_type = INTF_WB; > + break; > } > > WARN_ON(disp_info->num_of_h_tiles < 1); > @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", > i, controller_id, phys_params.split_role); > > - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, > + phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog, > intf_type, > controller_id); > if (phys_params.intf_idx == INTF_MAX) { > - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", > + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", > intf_type, controller_id); > ret = -EINVAL; > }
On 2/4/2022 3:36 PM, Dmitry Baryshkov wrote: > On 05/02/2022 00:17, Abhinav Kumar wrote: >> Make changes to dpu_encoder to support virtual encoder needed >> to support writeback for dpu. > > This patch will change significantly if > >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 >> +++++++++++++++++++++-------- >> 1 file changed, 42 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index e977c05..947069b 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct >> drm_encoder *drm_enc, >> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; >> struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; >> struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; >> + enum dpu_hw_blk_type blk_type; >> int num_lm, num_ctl, num_pp; >> int i, j; >> @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct >> drm_encoder *drm_enc, >> phys->hw_pp = dpu_enc->hw_pp[i]; >> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); >> + if (phys->intf_mode == INTF_MODE_WB_LINE) >> + blk_type = DPU_HW_BLK_WB; >> + else >> + blk_type = DPU_HW_BLK_INTF; >> + >> num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, >> - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, >> + global_state, drm_enc->base.id, blk_type, >> hw_blk, ARRAY_SIZE(hw_blk)); >> - for (j = 0; j < num_blk; j++) { >> - struct dpu_hw_intf *hw_intf; >> - hw_intf = to_dpu_hw_intf(hw_blk[i]); >> - if (hw_intf->idx == phys->intf_idx) >> - phys->hw_intf = hw_intf; >> + if (blk_type == DPU_HW_BLK_WB) { >> + for (j = 0; j < num_blk; j++) { >> + struct dpu_hw_wb *hw_wb; >> + >> + hw_wb = to_dpu_hw_wb(hw_blk[i]); >> + if (hw_wb->idx == phys->intf_idx) >> + phys->hw_wb = hw_wb; >> + } >> + } else { >> + for (j = 0; j < num_blk; j++) { >> + struct dpu_hw_intf *hw_intf; >> + >> + hw_intf = to_dpu_hw_intf(hw_blk[i]); >> + if (hw_intf->idx == phys->intf_idx) >> + phys->hw_intf = hw_intf; >> + } >> } > > I think that if we sequentially call dpu_rm_get_assigned_resources(.., > DPU_HW_BLK_INTF, ...) and then dpu_rm_get_assigned_resources(.., > DPU_HW_BLK_WB, ...), the code would be cleaner. > > Or even better get the WB direclty using the provided ID. Yeah i think you have done something like this for INTF on msm-next. Will follow that and post in the next version :) > >> - if (!phys->hw_intf) { >> + if (!phys->hw_intf && !phys->hw_wb) { >> DPU_ERROR_ENC(dpu_enc, >> - "no intf block assigned at idx: %d\n", i); >> + "no intf or WB block assigned at idx: %d\n", i); >> return; >> } >> @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct >> drm_encoder *drm_enc) >> mutex_unlock(&dpu_enc->enc_lock); >> } >> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, >> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg >> *catalog, >> enum dpu_intf_type type, u32 controller_id) >> { >> int i = 0; >> - for (i = 0; i < catalog->intf_count; i++) { >> - if (catalog->intf[i].type == type >> - && catalog->intf[i].controller_id == controller_id) { >> - return catalog->intf[i].id; >> + if (type != INTF_WB) { >> + for (i = 0; i < catalog->intf_count; i++) { >> + if (catalog->intf[i].type == type >> + && catalog->intf[i].controller_id == controller_id) { >> + return catalog->intf[i].id; >> + } >> + } >> + } else { >> + for (i = 0; i < catalog->wb_count; i++) { >> + if (catalog->wb[i].id == controller_id) >> + return catalog->wb[i].id; >> } >> } >> @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> case DRM_MODE_ENCODER_TMDS: >> intf_type = INTF_DP; >> break; >> + case DRM_MODE_ENCODER_VIRTUAL: >> + intf_type = INTF_WB; >> + break; >> } >> WARN_ON(disp_info->num_of_h_tiles < 1); >> @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct >> dpu_encoder_virt *dpu_enc, >> DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", >> i, controller_id, phys_params.split_role); >> - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >> + phys_params.intf_idx = >> dpu_encoder_get_intf_or_wb(dpu_kms->catalog, >> intf_type, >> controller_id); >> if (phys_params.intf_idx == INTF_MAX) { >> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id >> %d\n", >> + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type >> %d, id %d\n", >> intf_type, controller_id); >> ret = -EINVAL; >> } > >
On 2022-02-04 13:17:19, Abhinav Kumar wrote: > Make changes to dpu_encoder to support virtual encoder needed > to support writeback for dpu. > > Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++++++++-------- > 1 file changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index e977c05..947069b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; > struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; > + enum dpu_hw_blk_type blk_type; > int num_lm, num_ctl, num_pp; > int i, j; > > @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > phys->hw_pp = dpu_enc->hw_pp[i]; > phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); > > + if (phys->intf_mode == INTF_MODE_WB_LINE) > + blk_type = DPU_HW_BLK_WB; > + else > + blk_type = DPU_HW_BLK_INTF; > + > num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, > - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, > + global_state, drm_enc->base.id, blk_type, > hw_blk, ARRAY_SIZE(hw_blk)); > - for (j = 0; j < num_blk; j++) { > - struct dpu_hw_intf *hw_intf; > > - hw_intf = to_dpu_hw_intf(hw_blk[i]); > - if (hw_intf->idx == phys->intf_idx) > - phys->hw_intf = hw_intf; > + if (blk_type == DPU_HW_BLK_WB) { > + for (j = 0; j < num_blk; j++) { > + struct dpu_hw_wb *hw_wb; > + > + hw_wb = to_dpu_hw_wb(hw_blk[i]); > + if (hw_wb->idx == phys->intf_idx) > + phys->hw_wb = hw_wb; > + } > + } else { > + for (j = 0; j < num_blk; j++) { > + struct dpu_hw_intf *hw_intf; > + > + hw_intf = to_dpu_hw_intf(hw_blk[i]); > + if (hw_intf->idx == phys->intf_idx) > + phys->hw_intf = hw_intf; > + } It appears the original bit of code iterates j from 0 to num_blks yet only uses i as iteration value. All of this code seems reentrant meaning that executing it more than one times is redundant. You've adopted this mistake, though I'd argue it should use j in hw_blk[i] as that follows the number of elements in hw_blk returned by dpu_rm_get_assigned_resources. I suggest to address this in a separate patch (I can send that too, feel free to add my Reported-by otherwise) and rebase this patch on top of that, substituting i with j here as well. It seems to have been introduced by b954fa6baaca ("drm/msm/dpu: Refactor rm iterator"). - Marijn > } > > - if (!phys->hw_intf) { > + if (!phys->hw_intf && !phys->hw_wb) { > DPU_ERROR_ENC(dpu_enc, > - "no intf block assigned at idx: %d\n", i); > + "no intf or WB block assigned at idx: %d\n", i); > return; > } > > @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > mutex_unlock(&dpu_enc->enc_lock); > } > > -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog, > enum dpu_intf_type type, u32 controller_id) > { > int i = 0; > > - for (i = 0; i < catalog->intf_count; i++) { > - if (catalog->intf[i].type == type > - && catalog->intf[i].controller_id == controller_id) { > - return catalog->intf[i].id; > + if (type != INTF_WB) { > + for (i = 0; i < catalog->intf_count; i++) { > + if (catalog->intf[i].type == type > + && catalog->intf[i].controller_id == controller_id) { > + return catalog->intf[i].id; > + } > + } > + } else { > + for (i = 0; i < catalog->wb_count; i++) { > + if (catalog->wb[i].id == controller_id) > + return catalog->wb[i].id; > } > } > > @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > case DRM_MODE_ENCODER_TMDS: > intf_type = INTF_DP; > break; > + case DRM_MODE_ENCODER_VIRTUAL: > + intf_type = INTF_WB; > + break; > } > > WARN_ON(disp_info->num_of_h_tiles < 1); > @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, > DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", > i, controller_id, phys_params.split_role); > > - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, > + phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog, > intf_type, > controller_id); > if (phys_params.intf_idx == INTF_MAX) { > - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", > + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", > intf_type, controller_id); > ret = -EINVAL; > } > -- > 2.7.4 >
Hi Marijn Thank you for your suggestion. I will address it and add your "Reported by". Thanks Abhinav On 4/14/2022 3:26 PM, Marijn Suijten wrote: > On 2022-02-04 13:17:19, Abhinav Kumar wrote: >> Make changes to dpu_encoder to support virtual encoder needed >> to support writeback for dpu. >> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++++++++-------- >> 1 file changed, 42 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index e977c05..947069b 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, >> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; >> struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; >> struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; >> + enum dpu_hw_blk_type blk_type; >> int num_lm, num_ctl, num_pp; >> int i, j; >> >> @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, >> phys->hw_pp = dpu_enc->hw_pp[i]; >> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); >> >> + if (phys->intf_mode == INTF_MODE_WB_LINE) >> + blk_type = DPU_HW_BLK_WB; >> + else >> + blk_type = DPU_HW_BLK_INTF; >> + >> num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, >> - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, >> + global_state, drm_enc->base.id, blk_type, >> hw_blk, ARRAY_SIZE(hw_blk)); >> - for (j = 0; j < num_blk; j++) { >> - struct dpu_hw_intf *hw_intf; >> >> - hw_intf = to_dpu_hw_intf(hw_blk[i]); >> - if (hw_intf->idx == phys->intf_idx) >> - phys->hw_intf = hw_intf; >> + if (blk_type == DPU_HW_BLK_WB) { >> + for (j = 0; j < num_blk; j++) { >> + struct dpu_hw_wb *hw_wb; >> + >> + hw_wb = to_dpu_hw_wb(hw_blk[i]); >> + if (hw_wb->idx == phys->intf_idx) >> + phys->hw_wb = hw_wb; >> + } >> + } else { >> + for (j = 0; j < num_blk; j++) { >> + struct dpu_hw_intf *hw_intf; >> + >> + hw_intf = to_dpu_hw_intf(hw_blk[i]); >> + if (hw_intf->idx == phys->intf_idx) >> + phys->hw_intf = hw_intf; >> + } > > It appears the original bit of code iterates j from 0 to num_blks yet > only uses i as iteration value. All of this code seems reentrant > meaning that executing it more than one times is redundant. You've > adopted this mistake, though I'd argue it should use j in hw_blk[i] as > that follows the number of elements in hw_blk returned by > dpu_rm_get_assigned_resources. > > I suggest to address this in a separate patch (I can send that too, feel > free to add my Reported-by otherwise) and rebase this patch on top of > that, substituting i with j here as well. It seems to have been > introduced by b954fa6baaca ("drm/msm/dpu: Refactor rm iterator"). > > - Marijn > >> } >> >> - if (!phys->hw_intf) { >> + if (!phys->hw_intf && !phys->hw_wb) { >> DPU_ERROR_ENC(dpu_enc, >> - "no intf block assigned at idx: %d\n", i); >> + "no intf or WB block assigned at idx: %d\n", i); >> return; >> } >> >> @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) >> mutex_unlock(&dpu_enc->enc_lock); >> } >> >> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, >> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog, >> enum dpu_intf_type type, u32 controller_id) >> { >> int i = 0; >> >> - for (i = 0; i < catalog->intf_count; i++) { >> - if (catalog->intf[i].type == type >> - && catalog->intf[i].controller_id == controller_id) { >> - return catalog->intf[i].id; >> + if (type != INTF_WB) { >> + for (i = 0; i < catalog->intf_count; i++) { >> + if (catalog->intf[i].type == type >> + && catalog->intf[i].controller_id == controller_id) { >> + return catalog->intf[i].id; >> + } >> + } >> + } else { >> + for (i = 0; i < catalog->wb_count; i++) { >> + if (catalog->wb[i].id == controller_id) >> + return catalog->wb[i].id; >> } >> } >> >> @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, >> case DRM_MODE_ENCODER_TMDS: >> intf_type = INTF_DP; >> break; >> + case DRM_MODE_ENCODER_VIRTUAL: >> + intf_type = INTF_WB; >> + break; >> } >> >> WARN_ON(disp_info->num_of_h_tiles < 1); >> @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, >> DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", >> i, controller_id, phys_params.split_role); >> >> - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >> + phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog, >> intf_type, >> controller_id); >> if (phys_params.intf_idx == INTF_MAX) { >> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", >> + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", >> intf_type, controller_id); >> ret = -EINVAL; >> } >> -- >> 2.7.4 >>
Hi Marijn Looking at msm-next tip, this code has already been refactored in https://gitlab.freedesktop.org/drm/msm/-/commit/ef58e0ad34365e2c8274b74e6e745b8c180ff0d3 So, I will just rebase my changes on msm-next tip and it should address below comments as well. Thanks Abhinav On 4/14/2022 3:30 PM, Abhinav Kumar wrote: > Hi Marijn > > Thank you for your suggestion. > I will address it and add your "Reported by". > > Thanks > > Abhinav > > On 4/14/2022 3:26 PM, Marijn Suijten wrote: >> On 2022-02-04 13:17:19, Abhinav Kumar wrote: >>> Make changes to dpu_encoder to support virtual encoder needed >>> to support writeback for dpu. >>> >>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 >>> +++++++++++++++++++++-------- >>> 1 file changed, 42 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> index e977c05..947069b 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >>> @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct >>> drm_encoder *drm_enc, >>> struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; >>> struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; >>> struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; >>> + enum dpu_hw_blk_type blk_type; >>> int num_lm, num_ctl, num_pp; >>> int i, j; >>> @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct >>> drm_encoder *drm_enc, >>> phys->hw_pp = dpu_enc->hw_pp[i]; >>> phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); >>> + if (phys->intf_mode == INTF_MODE_WB_LINE) >>> + blk_type = DPU_HW_BLK_WB; >>> + else >>> + blk_type = DPU_HW_BLK_INTF; >>> + >>> num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, >>> - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, >>> + global_state, drm_enc->base.id, blk_type, >>> hw_blk, ARRAY_SIZE(hw_blk)); >>> - for (j = 0; j < num_blk; j++) { >>> - struct dpu_hw_intf *hw_intf; >>> - hw_intf = to_dpu_hw_intf(hw_blk[i]); >>> - if (hw_intf->idx == phys->intf_idx) >>> - phys->hw_intf = hw_intf; >>> + if (blk_type == DPU_HW_BLK_WB) { >>> + for (j = 0; j < num_blk; j++) { >>> + struct dpu_hw_wb *hw_wb; >>> + >>> + hw_wb = to_dpu_hw_wb(hw_blk[i]); >>> + if (hw_wb->idx == phys->intf_idx) >>> + phys->hw_wb = hw_wb; >>> + } >>> + } else { >>> + for (j = 0; j < num_blk; j++) { >>> + struct dpu_hw_intf *hw_intf; >>> + >>> + hw_intf = to_dpu_hw_intf(hw_blk[i]); >>> + if (hw_intf->idx == phys->intf_idx) >>> + phys->hw_intf = hw_intf; >>> + } >> >> It appears the original bit of code iterates j from 0 to num_blks yet >> only uses i as iteration value. All of this code seems reentrant >> meaning that executing it more than one times is redundant. You've >> adopted this mistake, though I'd argue it should use j in hw_blk[i] as >> that follows the number of elements in hw_blk returned by >> dpu_rm_get_assigned_resources. >> >> I suggest to address this in a separate patch (I can send that too, feel >> free to add my Reported-by otherwise) and rebase this patch on top of >> that, substituting i with j here as well. It seems to have been >> introduced by b954fa6baaca ("drm/msm/dpu: Refactor rm iterator"). >> >> - Marijn >> >>> } >>> - if (!phys->hw_intf) { >>> + if (!phys->hw_intf && !phys->hw_wb) { >>> DPU_ERROR_ENC(dpu_enc, >>> - "no intf block assigned at idx: %d\n", i); >>> + "no intf or WB block assigned at idx: %d\n", i); >>> return; >>> } >>> @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct >>> drm_encoder *drm_enc) >>> mutex_unlock(&dpu_enc->enc_lock); >>> } >>> -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, >>> +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg >>> *catalog, >>> enum dpu_intf_type type, u32 controller_id) >>> { >>> int i = 0; >>> - for (i = 0; i < catalog->intf_count; i++) { >>> - if (catalog->intf[i].type == type >>> - && catalog->intf[i].controller_id == controller_id) { >>> - return catalog->intf[i].id; >>> + if (type != INTF_WB) { >>> + for (i = 0; i < catalog->intf_count; i++) { >>> + if (catalog->intf[i].type == type >>> + && catalog->intf[i].controller_id == controller_id) { >>> + return catalog->intf[i].id; >>> + } >>> + } >>> + } else { >>> + for (i = 0; i < catalog->wb_count; i++) { >>> + if (catalog->wb[i].id == controller_id) >>> + return catalog->wb[i].id; >>> } >>> } >>> @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> case DRM_MODE_ENCODER_TMDS: >>> intf_type = INTF_DP; >>> break; >>> + case DRM_MODE_ENCODER_VIRTUAL: >>> + intf_type = INTF_WB; >>> + break; >>> } >>> WARN_ON(disp_info->num_of_h_tiles < 1); >>> @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct >>> dpu_encoder_virt *dpu_enc, >>> DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", >>> i, controller_id, phys_params.split_role); >>> - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, >>> + phys_params.intf_idx = >>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog, >>> intf_type, >>> controller_id); >>> if (phys_params.intf_idx == INTF_MAX) { >>> - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id >>> %d\n", >>> + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type >>> %d, id %d\n", >>> intf_type, controller_id); >>> ret = -EINVAL; >>> } >>> -- >>> 2.7.4 >>>
Hi Abhinav, On 2022-04-15 12:25:55, Abhinav Kumar wrote: > Hi Marijn > > Looking at msm-next tip, this code has already been refactored in > > https://gitlab.freedesktop.org/drm/msm/-/commit/ef58e0ad34365e2c8274b74e6e745b8c180ff0d3 > > So, I will just rebase my changes on msm-next tip and it should address > below comments as well. That's actually great, I love patches that remove a whole lot of (especially bug-containing) lines, awesome Dmitry! Looking forward to the next revision, spotted some minor nits in this revision but it probably makes little sense to point them out here presuming they might have already been addressed on your end or removed altogether. - Marijn
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index e977c05..947069b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -974,6 +974,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, struct dpu_hw_blk *hw_ctl[MAX_CHANNELS_PER_ENC]; struct dpu_hw_blk *hw_lm[MAX_CHANNELS_PER_ENC]; struct dpu_hw_blk *hw_dspp[MAX_CHANNELS_PER_ENC] = { NULL }; + enum dpu_hw_blk_type blk_type; int num_lm, num_ctl, num_pp; int i, j; @@ -1061,20 +1062,36 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, phys->hw_pp = dpu_enc->hw_pp[i]; phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]); + if (phys->intf_mode == INTF_MODE_WB_LINE) + blk_type = DPU_HW_BLK_WB; + else + blk_type = DPU_HW_BLK_INTF; + num_blk = dpu_rm_get_assigned_resources(&dpu_kms->rm, - global_state, drm_enc->base.id, DPU_HW_BLK_INTF, + global_state, drm_enc->base.id, blk_type, hw_blk, ARRAY_SIZE(hw_blk)); - for (j = 0; j < num_blk; j++) { - struct dpu_hw_intf *hw_intf; - hw_intf = to_dpu_hw_intf(hw_blk[i]); - if (hw_intf->idx == phys->intf_idx) - phys->hw_intf = hw_intf; + if (blk_type == DPU_HW_BLK_WB) { + for (j = 0; j < num_blk; j++) { + struct dpu_hw_wb *hw_wb; + + hw_wb = to_dpu_hw_wb(hw_blk[i]); + if (hw_wb->idx == phys->intf_idx) + phys->hw_wb = hw_wb; + } + } else { + for (j = 0; j < num_blk; j++) { + struct dpu_hw_intf *hw_intf; + + hw_intf = to_dpu_hw_intf(hw_blk[i]); + if (hw_intf->idx == phys->intf_idx) + phys->hw_intf = hw_intf; + } } - if (!phys->hw_intf) { + if (!phys->hw_intf && !phys->hw_wb) { DPU_ERROR_ENC(dpu_enc, - "no intf block assigned at idx: %d\n", i); + "no intf or WB block assigned at idx: %d\n", i); return; } @@ -1224,15 +1241,22 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) mutex_unlock(&dpu_enc->enc_lock); } -static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, +static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog, enum dpu_intf_type type, u32 controller_id) { int i = 0; - for (i = 0; i < catalog->intf_count; i++) { - if (catalog->intf[i].type == type - && catalog->intf[i].controller_id == controller_id) { - return catalog->intf[i].id; + if (type != INTF_WB) { + for (i = 0; i < catalog->intf_count; i++) { + if (catalog->intf[i].type == type + && catalog->intf[i].controller_id == controller_id) { + return catalog->intf[i].id; + } + } + } else { + for (i = 0; i < catalog->wb_count; i++) { + if (catalog->wb[i].id == controller_id) + return catalog->wb[i].id; } } @@ -2096,6 +2120,9 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, case DRM_MODE_ENCODER_TMDS: intf_type = INTF_DP; break; + case DRM_MODE_ENCODER_VIRTUAL: + intf_type = INTF_WB; + break; } WARN_ON(disp_info->num_of_h_tiles < 1); @@ -2128,11 +2155,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc, DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n", i, controller_id, phys_params.split_role); - phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, + phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog, intf_type, controller_id); if (phys_params.intf_idx == INTF_MAX) { - DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n", + DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n", intf_type, controller_id); ret = -EINVAL; }
Make changes to dpu_encoder to support virtual encoder needed to support writeback for dpu. Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 57 +++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 15 deletions(-)