[Freedreno] [PATCH v5 6/6] drm/msm/dpu: move VBIF blocks handling to dpu_rm

Dmitry Baryshkov dmitry.baryshkov at linaro.org
Mon Feb 14 22:39:49 UTC 2022


On 15/02/2022 01:04, Abhinav Kumar wrote:
> 
> 
> On 2/14/2022 12:56 PM, Dmitry Baryshkov wrote:
>> On 14/02/2022 22:53, Abhinav Kumar wrote:
>>>
>>>
>>> On 1/21/2022 1:06 PM, Dmitry Baryshkov wrote:
>>>> Move handling of VBIF blocks into dpu_rm. This serves the purpose of
>>>> unification of handling of all hardware blocks inside the DPU driver.
>>>> This removes hand-coded loops in dpu_vbif (which look for necessary 
>>>> VBIF
>>>> instance by looping through the dpu_kms->hw_vbif and comparing
>>>> vbif_idx).
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>
>>> I have a slightly different idea about this. Let me know what you think.
>>>
>>> VBIF is a bus interface for the dpu to fetch from. I am not sure if 
>>> pulling it in the RM is right because its not a dedicated HW block like
>>> the others in the RM.
>>
>> It's not a hardware block, but a it's still a hardware resource 
>> (hardware instance). It is described in the hw catalog. Thus I 
>> suggested moving it to dpu_rm.
>>
>> As you have seen, from my previous iterations of this patchset, I 
>> tried   to move things out of dpu_rm. After some hacking, I saw that 
>> having alloc/free loops in several places seems like a worse idea. So 
>> I moved dpu_hw_intf back to dpu_rm and then moved dpu_hw_vbif to 
>> dpu_rm too.
>>
> Actually for some reason, I only see two revs here:
> 
> https://patchwork.freedesktop.org/series/99175/#rev1

Yeah, I'm not sure why patchwork created new series rather than new 
revision.

> 
> Hence, I didnt check the previous patchsets from patchwork to see the 
> evolution.
> 

https://patchwork.freedesktop.org/patch/464353/


>>>
>>> But, I agree with your problem statement of hand-coded loops.
>>>
>>> So instead, why dont you just have a helper in the dpu_vbif.c to get
>>> you the vbif hw for the passed index like, maybe something like this?
>>>
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>> @@ -11,6 +11,19 @@
>>>   #include "dpu_hw_vbif.h"
>>>   #include "dpu_trace.h"
>>>
>>> +static dpu_hw_vbif *dpu_vbif_get_hw(struct dpu_kms *dpu_kms, u32 
>>> vbif_idx)
>>> +{
>>> +       int i;
>>> +
>>> +       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>> +               if (dpu_kms->hw_vbif[i] &&
>>> +                               dpu_kms->hw_vbif[i]->idx == vbif_idx)
>>> +                       vbif = dpu_kms->hw_vbif[i];
>>> +       }
>>> +
>>> +       return vbif;
>>> +}
>>> +
>>
>> You see, this code still bears an idea of looping through hw_vbif 
>> entries looking for the correct one (we can directly access 
>> hw_vbif[idx - VBIF_0] instead).
>>
>> And also the alloc/destroy loops are very similar to rm ones, but are 
>> working against the array in dpu_kms.
>>
>> One of the previous iterations had neearly the same idea as yours 
>> patch proposes, but I later abandoned this idea.
>>
>> I'm trying to place common code nearby, so that there is a less chance 
>> of an error.
> 
> This loop is being used to find the vbif matching the index only in two 
> places today dpu_vbif_set_ot_limit and dpu_vbif_set_qos_remap.

Here I was talking about the loops to allocate and destroy VBIFs.

> 
> Today and from whatever I see even in downstream (which has support for 
> more newer chipsets), there is only one VBIF
> instance in the catalog and always with the index 0.

When should we use VBIF_NRT? Judging from the _NRT suffix I thought that 
it's used for WB2, but it doesn't seem to be true.

> So to be honest, even that loop is an overkill today because the index 
> seems to be always 0 and there is only one instance so the loop seems to 
> break out at the first occurrence.

It's always better to remove a loop rather than to break from it.

> 
> Thats why I was wondering whether moving VBIF to RM is an overkill for 
> this and just the simple cleanup i was suggesting was enough as that 
> loop itself is an overkill today for one instance of vbif.

Then we might as well drop an array and just leave a single vbif_0.

Just checked downstream device trees. 8996 declares two VBIFs. It looks 
like it's the only user of VBIF_1. Any comments? What would we loose for 
(possible) 8996 support in DPU if we drop VBIF_1 / VBIF_NRT support?

> 
>>
>>>   /**
>>>    * _dpu_vbif_wait_for_xin_halt - wait for the xin to halt
>>>    * @vbif:      Pointer to hardware vbif driver
>>> @@ -156,11 +169,7 @@ void dpu_vbif_set_ot_limit(struct dpu_kms *dpu_kms,
>>>
>>>          mdp = dpu_kms->hw_mdp;
>>>
>>> -       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>> -               if (dpu_kms->hw_vbif[i] &&
>>> -                               dpu_kms->hw_vbif[i]->idx == 
>>> params->vbif_idx)
>>> -                       vbif = dpu_kms->hw_vbif[i];
>>> -       }
>>> +       vbif = dpu_vbif_get_hw(dpu_kms, params->vbif_idx);
>>>
>>>          if (!vbif || !mdp) {
>>>                  DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
>>> @@ -216,13 +225,7 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
>>> *dpu_kms,
>>>          }
>>>          mdp = dpu_kms->hw_mdp;
>>>
>>> -       for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>> -               if (dpu_kms->hw_vbif[i] &&
>>> -                               dpu_kms->hw_vbif[i]->idx == 
>>> params->vbif_idx) {
>>> -                       vbif = dpu_kms->hw_vbif[i];
>>> -                       break;
>>> -               }
>>> -       }
>>> +       vbif = dpu_vbif_get_hw(params->vbif_idx);
>>>
>>>
>>>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h |  1 +
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 28 
>>>> +--------------------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  1 -
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 19 ++++++++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      | 12 +++++++++
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c    | 26 ++++++-------------
>>>>   6 files changed, 40 insertions(+), 47 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
>>>> index 6417aa28d32c..895e86dabcb6 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h
>>>> @@ -8,6 +8,7 @@
>>>>   #include "dpu_hw_catalog.h"
>>>>   #include "dpu_hw_mdss.h"
>>>>   #include "dpu_hw_util.h"
>>>> +#include "dpu_hw_blk.h"
>>>>   struct dpu_hw_vbif;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 47fe11a84a77..4a1983d8561b 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -782,8 +782,6 @@ static long dpu_kms_round_pixclk(struct msm_kms 
>>>> *kms, unsigned long rate,
>>>>   static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms)
>>>>   {
>>>> -    int i;
>>>> -
>>>>       if (dpu_kms->hw_intr)
>>>>           dpu_hw_intr_destroy(dpu_kms->hw_intr);
>>>>       dpu_kms->hw_intr = NULL;
>>>> @@ -791,15 +789,6 @@ static void _dpu_kms_hw_destroy(struct dpu_kms 
>>>> *dpu_kms)
>>>>       /* safe to call these more than once during shutdown */
>>>>       _dpu_kms_mmu_destroy(dpu_kms);
>>>> -    if (dpu_kms->catalog) {
>>>> -        for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
>>>> -            u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
>>>> -
>>>> -            if ((vbif_idx < VBIF_MAX) && dpu_kms->hw_vbif[vbif_idx])
>>>> -                dpu_hw_vbif_destroy(dpu_kms->hw_vbif[vbif_idx]);
>>>> -        }
>>>> -    }
>>>> -
>>>>       if (dpu_kms->rm_init)
>>>>           dpu_rm_destroy(&dpu_kms->rm);
>>>>       dpu_kms->rm_init = false;
>>>> @@ -1027,7 +1016,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>>>   {
>>>>       struct dpu_kms *dpu_kms;
>>>>       struct drm_device *dev;
>>>> -    int i, rc = -EINVAL;
>>>> +    int rc = -EINVAL;
>>>>       if (!kms) {
>>>>           DPU_ERROR("invalid kms\n");
>>>> @@ -1116,21 +1105,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>>>           goto power_error;
>>>>       }
>>>> -    for (i = 0; i < dpu_kms->catalog->vbif_count; i++) {
>>>> -        u32 vbif_idx = dpu_kms->catalog->vbif[i].id;
>>>> -
>>>> -        dpu_kms->hw_vbif[i] = dpu_hw_vbif_init(vbif_idx,
>>>> -                dpu_kms->vbif[vbif_idx], dpu_kms->catalog);
>>>> -        if (IS_ERR_OR_NULL(dpu_kms->hw_vbif[vbif_idx])) {
>>>> -            rc = PTR_ERR(dpu_kms->hw_vbif[vbif_idx]);
>>>> -            if (!dpu_kms->hw_vbif[vbif_idx])
>>>> -                rc = -EINVAL;
>>>> -            DPU_ERROR("failed to init vbif %d: %d\n", vbif_idx, rc);
>>>> -            dpu_kms->hw_vbif[vbif_idx] = NULL;
>>>> -            goto power_error;
>>>> -        }
>>>> -    }
>>>> -
>>>>       rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
>>>>               _dpu_kms_get_clk(dpu_kms, "core"));
>>>>       if (rc) {
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> index 3f518c809e33..b96c901483ae 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
>>>> @@ -105,7 +105,6 @@ struct dpu_kms {
>>>>       struct dpu_rm rm;
>>>>       bool rm_init;
>>>> -    struct dpu_hw_vbif *hw_vbif[VBIF_MAX];
>>>>       struct dpu_hw_mdp *hw_mdp;
>>>>       bool has_danger_ctrl;
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> index 7497538adae1..6d49666c4e77 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -76,6 +76,8 @@ int dpu_rm_destroy(struct dpu_rm *rm)
>>>>       }
>>>>       for (i = 0; i < ARRAY_SIZE(rm->hw_intf); i++)
>>>>           dpu_hw_intf_destroy(rm->hw_intf[i]);
>>>> +    for (i = 0; i < ARRAY_SIZE(rm->hw_vbif); i++)
>>>> +        dpu_hw_vbif_destroy(rm->hw_vbif[i]);
>>>>       return 0;
>>>>   }
>>>> @@ -210,6 +212,23 @@ int dpu_rm_init(struct dpu_rm *rm,
>>>>           rm->dspp_blks[dspp->id - DSPP_0] = &hw->base;
>>>>       }
>>>> +    for (i = 0; i < cat->vbif_count; i++) {
>>>> +        struct dpu_hw_vbif *hw;
>>>> +        const struct dpu_vbif_cfg *vbif = &cat->vbif[i];
>>>> +
>>>> +        if (vbif->id < VBIF_0 || vbif->id >= VBIF_MAX) {
>>>> +            DPU_ERROR("skip vbif %d with invalid id\n", vbif->id);
>>>> +            continue;
>>>> +        }
>>>> +        hw = dpu_hw_vbif_init(vbif->id, mmio, cat);
>>>> +        if (IS_ERR(hw)) {
>>>> +            rc = PTR_ERR(hw);
>>>> +            DPU_ERROR("failed vbif object creation: err %d\n", rc);
>>>> +            goto fail;
>>>> +        }
>>>> +        rm->hw_vbif[vbif->id - VBIF_0] = hw;
>>>> +    }
>>>> +
>>>>       return 0;
>>>>   fail:
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> index 9b13200a050a..a15977bdceeb 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> @@ -20,6 +20,7 @@ struct dpu_global_state;
>>>>    * @ctl_blks: array of ctl hardware resources
>>>>    * @hw_intf: array of intf hardware resources
>>>>    * @dspp_blks: array of dspp hardware resources
>>>> + * @hw_vbif: array of vbif hardware resources
>>>>    */
>>>>   struct dpu_rm {
>>>>       struct dpu_hw_blk *pingpong_blks[PINGPONG_MAX - PINGPONG_0];
>>>> @@ -28,6 +29,7 @@ struct dpu_rm {
>>>>       struct dpu_hw_intf *hw_intf[INTF_MAX - INTF_0];
>>>>       struct dpu_hw_blk *dspp_blks[DSPP_MAX - DSPP_0];
>>>>       struct dpu_hw_blk *merge_3d_blks[MERGE_3D_MAX - MERGE_3D_0];
>>>> +    struct dpu_hw_vbif *hw_vbif[VBIF_MAX - VBIF_0];
>>>>   };
>>>>   /**
>>>> @@ -95,5 +97,15 @@ static inline struct dpu_hw_intf 
>>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
>>>>       return rm->hw_intf[intf_idx - INTF_0];
>>>>   }
>>>> +/**
>>>> + * dpu_rm_get_vbif - Return a struct dpu_hw_vbif instance given 
>>>> it's index.
>>>> + * @rm: DPU Resource Manager handle
>>>> + * @vbif_idx: VBIF's index
>>>> + */
>>>> +static inline struct dpu_hw_vbif *dpu_rm_get_vbif(struct dpu_rm 
>>>> *rm, enum dpu_vbif vbif_idx)
>>>> +{
>>>> +    return rm->hw_vbif[vbif_idx - VBIF_0];
>>>> +}
>>>> +
>>>>   #endif /* __DPU_RM_H__ */
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>>> index 21d20373eb8b..00ac2aa81651 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
>>>> @@ -152,15 +152,10 @@ void dpu_vbif_set_ot_limit(struct dpu_kms 
>>>> *dpu_kms,
>>>>       struct dpu_hw_mdp *mdp;
>>>>       bool forced_on = false;
>>>>       u32 ot_lim;
>>>> -    int ret, i;
>>>> +    int ret;
>>>>       mdp = dpu_kms->hw_mdp;
>>>> -
>>>> -    for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>>> -        if (dpu_kms->hw_vbif[i] &&
>>>> -                dpu_kms->hw_vbif[i]->idx == params->vbif_idx)
>>>> -            vbif = dpu_kms->hw_vbif[i];
>>>> -    }
>>>> +    vbif = dpu_rm_get_vbif(&dpu_kms->rm, params->vbif_idx);
>>>>       if (!vbif || !mdp) {
>>>>           DRM_DEBUG_ATOMIC("invalid arguments vbif %d mdp %d\n",
>>>> @@ -216,14 +211,7 @@ void dpu_vbif_set_qos_remap(struct dpu_kms 
>>>> *dpu_kms,
>>>>       }
>>>>       mdp = dpu_kms->hw_mdp;
>>>> -    for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>>> -        if (dpu_kms->hw_vbif[i] &&
>>>> -                dpu_kms->hw_vbif[i]->idx == params->vbif_idx) {
>>>> -            vbif = dpu_kms->hw_vbif[i];
>>>> -            break;
>>>> -        }
>>>> -    }
>>>> -
>>>> +    vbif = dpu_rm_get_vbif(&dpu_kms->rm, params->vbif_idx);
>>>>       if (!vbif || !vbif->cap) {
>>>>           DPU_ERROR("invalid vbif %d\n", params->vbif_idx);
>>>>           return;
>>>> @@ -261,8 +249,8 @@ void dpu_vbif_clear_errors(struct dpu_kms *dpu_kms)
>>>>       struct dpu_hw_vbif *vbif;
>>>>       u32 i, pnd, src;
>>>> -    for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>>> -        vbif = dpu_kms->hw_vbif[i];
>>>> +    for (i = VBIF_0; i < VBIF_MAX; i++) {
>>>> +        vbif = dpu_rm_get_vbif(&dpu_kms->rm, i);
>>>>           if (vbif && vbif->ops.clear_errors) {
>>>>               vbif->ops.clear_errors(vbif, &pnd, &src);
>>>>               if (pnd || src) {
>>>> @@ -278,8 +266,8 @@ void dpu_vbif_init_memtypes(struct dpu_kms 
>>>> *dpu_kms)
>>>>       struct dpu_hw_vbif *vbif;
>>>>       int i, j;
>>>> -    for (i = 0; i < ARRAY_SIZE(dpu_kms->hw_vbif); i++) {
>>>> -        vbif = dpu_kms->hw_vbif[i];
>>>> +    for (i = VBIF_0; i < VBIF_MAX; i++) {
>>>> +        vbif = dpu_rm_get_vbif(&dpu_kms->rm, i);
>>>>           if (vbif && vbif->cap && vbif->ops.set_mem_type) {
>>>>               for (j = 0; j < vbif->cap->memtype_count; j++)
>>>>                   vbif->ops.set_mem_type(
>>
>>


-- 
With best wishes
Dmitry


More information about the dri-devel mailing list