Re: [PATCH 18/25] drm/msm/dpu: merge RM interface reservation helpers

Jeykumar Sankaran
2018-10-09T18:20:51Z

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;
>> }

--
Jeykumar S