On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote: > > Hi Dmitry > > Thanks for the review. > > One question below. > > On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote: > > On 21/04/2022 23:48, Abhinav Kumar wrote: > >> Using intf_idx even for writeback interfaces is confusing > >> because intf_idx is of type enum dpu_intf but the index used > >> for writeback is of type enum dpu_wb. > >> > >> In addition, this makes it easier to separately check the > >> availability of the two as its possible that there are boards > >> which don't have any physical display connected but can still > >> work in writeback mode. > >> > >> Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> > > > > Looks good, two minor issues bellow. > > > > With them fixed, I'd even squash this patch into the corresponding patch > > of the previous patchset. > > > >> --- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 62 > >> +++++++++++++----------- > >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 4 ++ > >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 2 +- > >> 3 files changed, 40 insertions(+), 28 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> index 9c12841..054d7e4 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > >> @@ -962,7 +962,6 @@ static void > >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc, > >> struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC]; > >> int num_lm, num_ctl, num_pp, num_dsc; > >> unsigned int dsc_mask = 0; > >> - enum dpu_hw_blk_type blk_type; > >> int i; > >> if (!drm_enc) { > >> @@ -1044,17 +1043,11 @@ static void > >> dpu_encoder_virt_atomic_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 (dpu_encoder_get_intf_mode(&dpu_enc->base) == > >> INTF_MODE_WB_LINE) > >> - blk_type = DPU_HW_BLK_WB; > >> - else > >> - blk_type = DPU_HW_BLK_INTF; > >> + if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) > >> + phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, > >> phys->intf_idx); > >> - if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) { > >> - if (blk_type == DPU_HW_BLK_INTF) > >> - phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, > >> phys->intf_idx); > >> - else if (blk_type == DPU_HW_BLK_WB) > >> - phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, > >> phys->intf_idx); > >> - } > >> + if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX) > >> + phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx); > > > > We also need a check for if (phus->hw_intf && phys->hw_wb) HERE > > So there is an error if > > 1) Neither wb NOR intf are present > 2) Both wb AND intf are present for the physical encoder? > > The second check is okay for now to add but considering concurrent > writeback then that wouldn't assumption be wrong since both physical and > wb interfaces can go with the same encoder? To the same encoder, but not to the same physical encoder. Here we check the phys_enc parameters. > > > > >> if (!phys->hw_intf && !phys->hw_wb) { > >> DPU_ERROR_ENC(dpu_enc, > >> @@ -1201,7 +1194,7 @@ 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_or_wb(struct dpu_mdss_cfg > >> *catalog, > >> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > >> enum dpu_intf_type type, u32 controller_id) > >> { > >> int i = 0; > >> @@ -1213,16 +1206,28 @@ static enum dpu_intf > >> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog, > >> 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; > >> - } > >> } > >> return INTF_MAX; > >> } > >> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog, > >> + enum dpu_intf_type type, u32 controller_id) > >> +{ > >> + int i = 0; > >> + > >> + if (type != INTF_WB) > >> + goto end; > >> + > >> + for (i = 0; i < catalog->wb_count; i++) { > >> + if (catalog->wb[i].id == controller_id) > >> + return catalog->wb[i].id; > >> + } > >> + > >> +end: > >> + return WB_MAX; > > > > I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found. > ack, i guess in that case even the places checking the return value of > this function need to be changed. Yes, of course. > > > >> +} > >> + > >> static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, > >> struct dpu_encoder_phys *phy_enc) > >> { > >> @@ -2249,18 +2254,21 @@ 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); > >> - /* > >> - * FIXME: have separate intf_idx and wb_idx to avoid using > >> - * enum dpu_intf type for wb_idx and also to be able to > >> - * not bail out when there is no intf for boards which dont > >> - * have a display connected to them. > >> - * Having a valid wb_idx but not a intf_idx can be a valid > >> - * combination moving forward. > >> - */ > >> - phys_params.intf_idx = > >> dpu_encoder_get_intf_or_wb(dpu_kms->catalog, > >> + phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog, > >> intf_type, > >> controller_id); > >> - if (phys_params.intf_idx == INTF_MAX) { > >> + > >> + phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog, > >> + intf_type, controller_id); > >> + /* > >> + * For boards which have no physical displays, having no > >> interface > >> + * is fine because it can still be used with just writeback. > >> + * If we try without a display on a board which uses a DPU in > >> which > >> + * writeback is not supported, then this will still fail as > >> it will not > >> + * find any writeback in the catalog. > >> + */ > >> + if ((phys_params.intf_idx == INTF_MAX) && > >> + (phys_params.wb_idx == WB_MAX)) { > >> DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type > >> %d, id %d\n", > >> intf_type, controller_id); > >> ret = -EINVAL; > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > >> index 04d037e..f2af07d 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > >> @@ -181,6 +181,7 @@ enum dpu_intr_idx { > >> * @split_role: Role to play in a split-panel configuration > >> * @intf_mode: Interface mode > >> * @intf_idx: Interface index on dpu hardware > >> + * @wb_idx: Writeback index on dpu hardware > >> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes > >> * @enable_state: Enable state tracking > >> * @vblank_refcount: Reference count of vblank request > >> @@ -209,6 +210,7 @@ struct dpu_encoder_phys { > >> enum dpu_enc_split_role split_role; > >> enum dpu_intf_mode intf_mode; > >> enum dpu_intf intf_idx; > >> + enum dpu_wb wb_idx; > >> spinlock_t *enc_spinlock; > >> enum dpu_enc_enable_state enable_state; > >> atomic_t vblank_refcount; > >> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd { > >> * @parent_ops: Callbacks exposed by the parent to the phys_enc > >> * @split_role: Role to play in a split-panel configuration > >> * @intf_idx: Interface index this phys_enc will control > >> + * @wb_idx: Writeback index this phys_enc will control > >> * @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes > >> */ > >> struct dpu_enc_phys_init_params { > >> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params { > >> const struct dpu_encoder_virt_ops *parent_ops; > >> enum dpu_enc_split_role split_role; > >> enum dpu_intf intf_idx; > >> + enum dpu_wb wb_idx; > >> spinlock_t *enc_spinlock; > >> }; > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > >> index ba82e54..2f34a31 100644 > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > >> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf > >> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in > >> * @rm: DPU Resource Manager handle > >> * @wb_idx: WB index > >> */ > >> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum > >> dpu_intf wb_idx) > >> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum > >> dpu_wb wb_idx) > >> { > >> return rm->hw_wb[wb_idx - WB_0]; > >> } > > > > -- With best wishes Dmitry