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

Submitted by Jeykumar Sankaran on Oct. 9, 2018, 4:27 a.m.

Details

Message ID 1539059262-8326-19-git-send-email-jsanka@codeaurora.org
State New
Headers show
Series "reserve RM resources in CRTC state" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jeykumar Sankaran Oct. 9, 2018, 4:27 a.m.
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(-)

Patch hide | download patch | download mbox

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

Comments

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