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

2018-10-09 Thread Jeykumar Sankaran 

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 
---
 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 = >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 = >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,

>hw_res);

-   if (ret)
+   ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, >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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

2018-10-09 Thread Jordan Crouse 
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 
> ---
>  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 = >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 = >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, >hw_res);
> - if (ret)
> + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, >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;
>  }

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

2018-10-08 Thread Jeykumar Sankaran 
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 
---
 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 = >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 = >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, >hw_res);
-   if (ret)
+   ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, >hw_res);
+   if (ret) {
+   DPU_ERROR("unable to find appropriate INTF\n");
return ret;
+   }
 
return ret;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno