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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 25 Apr 2023 at 19:11, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
>
>
> On 4/25/2023 7:26 AM, Dmitry Baryshkov wrote:
> > On Tue, 25 Apr 2023 at 11:55, Marijn Suijten
> > <marijn.suijten@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >>>>>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>>>>>>>
> >>>>>>>> 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()

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



-- 
With best wishes
Dmitry



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux