by Abhinav Kumar

[permalink] [raw]

Subject: Re: [Freedreno] [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs

On 4/25/2023 2:09 PM, Dmitry Baryshkov wrote:
> On Tue, 25 Apr 2023 at 19:11, Abhinav Kumar <[email protected]> wrote:
>>
>>
>>
>> On 4/25/2023 7:26 AM, Dmitry Baryshkov wrote:
>>> On Tue, 25 Apr 2023 at 11:55, Marijn Suijten
>>> <[email protected]> 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
>>>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>>>>>> Reviewed-by: Dmitry Baryshkov <[email protected]>
>>>>>>>>>>
>>>>>>>>>> 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.
>
> No, it won't. There is the IS_ERR check, not the IS_ERR_OR_NULL()
>

Ack, but isnt that an issue since rm->hw_intf[intf->id - INTF_0] can be
assigned to a NULL hw.

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

2023-04-25 22:42:53

by Marijn Suijten

[permalink] [raw]

Subject: Re: [Freedreno] [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs

On 2023-04-25 14:32:51, Abhinav Kumar wrote:
<snip>
> >>>>> 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.

Sure, I'll keep the check exclusively in dpu_hw_intf_init(). Should I
also move the pingpong==PINGPONG_MAX check into dpu_hw_lm_init()?

> >> 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.
> >
> > No, it won't. There is the IS_ERR check, not the IS_ERR_OR_NULL()

Only DSC currently uses IS_ERR_OR_NULL... We should fix that as
rc=PTR_ERR(hw) on the next should be 0 (actually, the intent is for it
to be undefined I think) for that...

> Ack, but isnt that an issue since rm->hw_intf[intf->id - INTF_0] can be
> assigned to a NULL hw.

Yes, that is exactly the intent here.

> >> That wont happen today because catalog doesnt have such entries but just
> >> wanted to note what gets lost with this change.

It does, we have a few SoCs with type=INTF_NONE. Quoting myself from
above:

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.

- Marijn