[PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs
Abhinav Kumar
quic_abhinavk at quicinc.com
Tue Apr 25 16:11:10 UTC 2023
On 4/25/2023 7:26 AM, Dmitry Baryshkov wrote:
> On Tue, 25 Apr 2023 at 11:55, Marijn Suijten
> <marijn.suijten at somainline.org> wrote:
>>
>> On 2023-04-25 10:54:47, Dmitry Baryshkov wrote:
>>> On 25/04/2023 10:16, Marijn Suijten wrote:
>>>> On 2023-04-24 16:23:17, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 4/24/2023 3:54 PM, Dmitry Baryshkov wrote:
>>>>>> On Tue, 25 Apr 2023 at 01:03, Marijn Suijten
>>>>>> <marijn.suijten at somainline.org> wrote:
>>>>>>>
>>>>>>> On 2023-04-21 16:25:15, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/21/2023 1:53 PM, Marijn Suijten wrote:
>>>>>>>>> The Resource Manager already iterates over all available blocks from the
>>>>>>>>> catalog, only to pass their ID to a dpu_hw_xxx_init() function which
>>>>>>>>> uses an _xxx_offset() helper to search for and find the exact same
>>>>>>>>> catalog pointer again to initialize the block with, fallible error
>>>>>>>>> handling and all.
>>>>>>>>>
>>>>>>>>> Instead, pass const pointers to the catalog entries directly to these
>>>>>>>>> _init functions and drop the for loops entirely, saving on both
>>>>>>>>> readability complexity and unnecessary cycles at boot.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marijn Suijten <marijn.suijten at somainline.org>
>>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>>
>>>>>>>> Overall, a nice cleanup!
>>>>>>>>
>>>>>>>> One comment below.
>>>>>>>>
>>>>>>>>> ---
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 37 +++++----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 14 ++++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 32 +++---------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 11 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.c | 38 ++++-----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dspp.h | 12 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.h | 2 +-
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 40 ++++++-----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h | 12 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.c | 38 ++++-----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_lm.h | 10 +++---
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.c | 33 +++----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_merge3d.h | 14 ++++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 33 +++----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14 ++++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c | 39 ++++------------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h | 12 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.c | 33 +++----------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_vbif.h | 11 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.c | 33 ++++---------------
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_wb.h | 11 +++----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 17 +++++-----
>>>>>>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 18 +++++-----
>>>>>>>>> 23 files changed, 139 insertions(+), 375 deletions(-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> <snipped>
>>>>>>>>
>>>>>>>>> -struct dpu_hw_intf *dpu_hw_intf_init(enum dpu_intf idx,
>>>>>>>>> - void __iomem *addr,
>>>>>>>>> - const struct dpu_mdss_cfg *m)
>>>>>>>>> +struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
>>>>>>>>> + void __iomem *addr)
>>>>>>>>> {
>>>>>>>>> struct dpu_hw_intf *c;
>>>>>>>>> - const struct dpu_intf_cfg *cfg;
>>>>>>>>> +
>>>>>>>>> + if (cfg->type == INTF_NONE) {
>>>>>>>>> + pr_err("Cannot create interface hw object for INTF_NONE type\n");
>>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> The caller of dpu_hw_intf_init which is the RM already has protection
>>>>>>>> for INTF_NONE, see below
>>>>>>>>
>>>>>>>> for (i = 0; i < cat->intf_count; i++) {
>>>>>>>> struct dpu_hw_intf *hw;
>>>>>>>> const struct dpu_intf_cfg *intf = &cat->intf[i];
>>>>>>>>
>>>>>>>> if (intf->type == INTF_NONE) {
>>>>>>>> DPU_DEBUG("skip intf %d with type none\n", i);
>>>>>>>> continue;
>>>>>>>> }
>>>>>>>> if (intf->id < INTF_0 || intf->id >= INTF_MAX) {
>>>>>>>> DPU_ERROR("skip intf %d with invalid id\n",
>>>>>>>> intf->id);
>>>>>>>> continue;
>>>>>>>> }
>>>>>>>> hw = dpu_hw_intf_init(intf->id, mmio, cat);
>>>>>>>>
>>>>>>>> So this part can be dropped.
>>>>>>>
>>>>>>> I mainly intended to keep original validation where _intf_offset would
>>>>>>> skip INTF_NONE, and error out. RM init is hence expected to filter out
>>>>>>> INTF_NONE instead of running into that `-EINVAL`, which I maintained
>>>>>>> here.
>>>>>>>
>>>>>>> If you think there won't be another caller of dpu_hw_intf_init, and that
>>>>>>> such validation is hence excessive, I can remove it in a followup v3.
>>>>>>
>>>>>> I'd prefer to see the checks at dpu_rm to be dropped.
>>>>>> dpu_hw_intf_init() (and other dpu_hw_foo_init() functions) should be
>>>>>> self-contained. If they can not init HW block (e.g. because the index
>>>>>> is out of the boundaries), they should return an error.
>>>>>>
>>>>>
>>>>> They already do that today because even without this it will call into
>>>>> _intf_offset() and that will bail out for INTF_NONE.
>>>>>
>>>>> I feel this is a duplicated check because the caller with the loop needs
>>>>> to validate the index before passing it to dpu_hw_intf_init() otherwise
>>>>> the loop will get broken at the first return of the error and rest of
>>>>> the blocks will also not be initialized.
>>>>
>>>> To both: keep in mind that the range-checks we want to remove from
>>>> dpu_rm_init validate the ID (index?) of a block. This check is for the
>>>> *TYPE* of an INTF block, to skip it gracefully if no hardware is mapped
>>>> there. As per the first patch of this series SM6115/QCM2290 only have a
>>>> DSI interface which always sits at ID 1, and ID 0 has its TYPE set to
>>>> INTF_NONE and is skipped.
>>>>
>>>> Hence we _should_ keep the graceful TYPE check in dpu_rm_init() to skip
>>>> calling this function _and assigning it to the rm->hw_intf array_. But
>>>> I can remove the second TYPE check here in dpu_hw_intf_init() if you
>>>> prefer.
>>>
>>> We can return NULL from dpu_hw_foo_init(), which would mean that the
>>> block was skipped or is not present.
>>
>> An then replace the `if INTF_NONE continue` logic in dpu_rm_init with a
>> check for NULL that skips, and a check for IS_ERR` that goes to `fail`?
>
> You can just drop the INTF_NONE in dpu_rm. If dpu_hw_intf_init()
> returns NULL, the rest of the code in dpu_rm will work correctly.
>
The only thing lost will be that the loop in the RM will break at the
first instance of NULL so if the loop has valid intf blocks later, those
will also not get initialized.
That wont happen today because catalog doesnt have such entries but just
wanted to note what gets lost with this change.
Otherwise, I am okay with that.
>>
>> Should I do that in a new or the same patch for v3?
>>
>> Note that there's a similar check for the `pingpong` "id" member of
>> every Layer Mixer.
>>
>> - Marijn
>
>
>
More information about the dri-devel
mailing list