| Message ID | 1539059262-8326-19-git-send-email-jsanka@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | reserve RM resources in CRTC state | expand |
On Mon, Oct 08, 2018 at 09:27:35PM -0700, Jeykumar Sankaran wrote: > we don't have enough reasons why the HW block looping's > cannot happen in the same function. So merge them. looping's -> looping. So there are reasons one might break them out but not interesting ones? > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 63 ++++++++++++++-------------------- > 1 file changed, 26 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index a79456c..bb59250 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -435,52 +435,39 @@ static int _dpu_rm_reserve_ctls( > return 0; > } > > -static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf( > - struct dpu_rm *rm, > - uint32_t id, > - enum dpu_hw_blk_type type) > -{ > - struct dpu_rm_hw_blk *iter; > - struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; > - > - /* Find the block entry in the rm, and note the reservation */ > - list_for_each_entry(iter, blk_list, list) { > - if (iter->hw->id != id || iter->in_use) > - continue; > - > - trace_dpu_rm_reserve_intf(iter->hw->id, DPU_HW_BLK_INTF); > - > - break; > - } > - > - /* Shouldn't happen since intfs are fixed at probe */ > - if (!iter) { > - DPU_ERROR("couldn't find type %d id %d\n", type, id); > - return NULL; > - } > - > - return iter; > -} > - > -static int _dpu_rm_reserve_intf_related_hw( > +static int _dpu_rm_reserve_intfs( > struct dpu_rm *rm, > struct dpu_crtc_state *dpu_cstate, > struct dpu_encoder_hw_resources *hw_res) > { > - struct dpu_rm_hw_blk *blk; > + struct dpu_rm_hw_blk *iter; > + struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; > int i, num_intfs = 0; > > for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { > + struct dpu_rm_hw_blk *intf_blk = NULL; > + > if (hw_res->intfs[i] == INTF_MODE_NONE) > continue; > > - blk = _dpu_rm_reserve_intf(rm, i + INTF_0, > - DPU_HW_BLK_INTF); > - if (!blk) > - return -ENAVAIL; > + list_for_each_entry(iter, blk_list, list) { > + if (iter->in_use) > + continue; > + > + if (iter->hw->id == (INTF_0 + i)) { > + intf_blk = iter; > + break; > + } > + } > + > + if (!intf_blk) > + return -EINVAL; > > - blk->in_use = true; > - dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw); > + intf_blk->in_use = true; > + dpu_cstate->hw_intfs[num_intfs++] = > + to_dpu_hw_intf(intf_blk->hw); > + > + trace_dpu_rm_reserve_intf(intf_blk->hw->id, DPU_HW_BLK_INTF); > } > > dpu_cstate->num_intfs = num_intfs; > @@ -507,9 +494,11 @@ static int _dpu_rm_make_reservation( > return ret; > } > > - ret = _dpu_rm_reserve_intf_related_hw(rm, dpu_cstate, &reqs->hw_res); > - if (ret) > + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &reqs->hw_res); > + if (ret) { > + DPU_ERROR("unable to find appropriate INTF\n"); Since there is only once consumer of this function, I would move this error message down into the sub-function and provide more debug information - like which INTF wasn't found. > return ret; > + } And you don't need to return ret in this block - you can just drop out to the bottom. > > return ret; > }
On 2018-10-09 09:50, Jordan Crouse wrote: > On Mon, Oct 08, 2018 at 09:27:35PM -0700, Jeykumar Sankaran wrote: >> we don't have enough reasons why the HW block looping's >> cannot happen in the same function. So merge them. > > looping's -> looping. So there are reasons one might break them out > but not interesting ones? > Not just yet. Once we start supporting different type of connectors such as writeback & DP and the parsing logic for the respective type of INTF grows up, we *may* want to split this up. Thanks Jeykumar S. >> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 63 > ++++++++++++++-------------------- >> 1 file changed, 26 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> index a79456c..bb59250 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -435,52 +435,39 @@ static int _dpu_rm_reserve_ctls( >> return 0; >> } >> >> -static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf( >> - struct dpu_rm *rm, >> - uint32_t id, >> - enum dpu_hw_blk_type type) >> -{ >> - struct dpu_rm_hw_blk *iter; >> - struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; >> - >> - /* Find the block entry in the rm, and note the reservation */ >> - list_for_each_entry(iter, blk_list, list) { >> - if (iter->hw->id != id || iter->in_use) >> - continue; >> - >> - trace_dpu_rm_reserve_intf(iter->hw->id, DPU_HW_BLK_INTF); >> - >> - break; >> - } >> - >> - /* Shouldn't happen since intfs are fixed at probe */ >> - if (!iter) { >> - DPU_ERROR("couldn't find type %d id %d\n", type, id); >> - return NULL; >> - } >> - >> - return iter; >> -} >> - >> -static int _dpu_rm_reserve_intf_related_hw( >> +static int _dpu_rm_reserve_intfs( >> struct dpu_rm *rm, >> struct dpu_crtc_state *dpu_cstate, >> struct dpu_encoder_hw_resources *hw_res) >> { >> - struct dpu_rm_hw_blk *blk; >> + struct dpu_rm_hw_blk *iter; >> + struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; >> int i, num_intfs = 0; >> >> for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { >> + struct dpu_rm_hw_blk *intf_blk = NULL; >> + >> if (hw_res->intfs[i] == INTF_MODE_NONE) >> continue; >> >> - blk = _dpu_rm_reserve_intf(rm, i + INTF_0, >> - DPU_HW_BLK_INTF); >> - if (!blk) >> - return -ENAVAIL; >> + list_for_each_entry(iter, blk_list, list) { >> + if (iter->in_use) >> + continue; >> + >> + if (iter->hw->id == (INTF_0 + i)) { >> + intf_blk = iter; >> + break; >> + } >> + } >> + >> + if (!intf_blk) >> + return -EINVAL; >> >> - blk->in_use = true; >> - dpu_cstate->hw_intfs[num_intfs++] = > to_dpu_hw_intf(blk->hw); >> + intf_blk->in_use = true; >> + dpu_cstate->hw_intfs[num_intfs++] = >> + > to_dpu_hw_intf(intf_blk->hw); >> + >> + trace_dpu_rm_reserve_intf(intf_blk->hw->id, > DPU_HW_BLK_INTF); >> } >> >> dpu_cstate->num_intfs = num_intfs; >> @@ -507,9 +494,11 @@ static int _dpu_rm_make_reservation( >> return ret; >> } >> >> - ret = _dpu_rm_reserve_intf_related_hw(rm, dpu_cstate, > &reqs->hw_res); >> - if (ret) >> + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &reqs->hw_res); >> + if (ret) { >> + DPU_ERROR("unable to find appropriate INTF\n"); > > Since there is only once consumer of this function, I would move this > error > message down into the sub-function and provide more debug information - > like > which INTF wasn't found. > >> return ret; >> + } > > And you don't need to return ret in this block - you can just drop out > to > the > bottom. > >> >> return ret; >> }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index a79456c..bb59250 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -435,52 +435,39 @@ static int _dpu_rm_reserve_ctls( return 0; } -static struct dpu_rm_hw_blk *_dpu_rm_reserve_intf( - struct dpu_rm *rm, - uint32_t id, - enum dpu_hw_blk_type type) -{ - struct dpu_rm_hw_blk *iter; - struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; - - /* Find the block entry in the rm, and note the reservation */ - list_for_each_entry(iter, blk_list, list) { - if (iter->hw->id != id || iter->in_use) - continue; - - trace_dpu_rm_reserve_intf(iter->hw->id, DPU_HW_BLK_INTF); - - break; - } - - /* Shouldn't happen since intfs are fixed at probe */ - if (!iter) { - DPU_ERROR("couldn't find type %d id %d\n", type, id); - return NULL; - } - - return iter; -} - -static int _dpu_rm_reserve_intf_related_hw( +static int _dpu_rm_reserve_intfs( struct dpu_rm *rm, struct dpu_crtc_state *dpu_cstate, struct dpu_encoder_hw_resources *hw_res) { - struct dpu_rm_hw_blk *blk; + struct dpu_rm_hw_blk *iter; + struct list_head *blk_list = &rm->hw_blks[DPU_HW_BLK_INTF]; int i, num_intfs = 0; for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { + struct dpu_rm_hw_blk *intf_blk = NULL; + if (hw_res->intfs[i] == INTF_MODE_NONE) continue; - blk = _dpu_rm_reserve_intf(rm, i + INTF_0, - DPU_HW_BLK_INTF); - if (!blk) - return -ENAVAIL; + list_for_each_entry(iter, blk_list, list) { + if (iter->in_use) + continue; + + if (iter->hw->id == (INTF_0 + i)) { + intf_blk = iter; + break; + } + } + + if (!intf_blk) + return -EINVAL; - blk->in_use = true; - dpu_cstate->hw_intfs[num_intfs++] = to_dpu_hw_intf(blk->hw); + intf_blk->in_use = true; + dpu_cstate->hw_intfs[num_intfs++] = + to_dpu_hw_intf(intf_blk->hw); + + trace_dpu_rm_reserve_intf(intf_blk->hw->id, DPU_HW_BLK_INTF); } dpu_cstate->num_intfs = num_intfs; @@ -507,9 +494,11 @@ static int _dpu_rm_make_reservation( return ret; } - ret = _dpu_rm_reserve_intf_related_hw(rm, dpu_cstate, &reqs->hw_res); - if (ret) + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &reqs->hw_res); + if (ret) { + DPU_ERROR("unable to find appropriate INTF\n"); return ret; + } return ret; }
we don't have enough reasons why the HW block looping's cannot happen in the same function. So merge them. Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 63 ++++++++++++++-------------------- 1 file changed, 26 insertions(+), 37 deletions(-)