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.
>
> - Marijn
>
> > > c = kzalloc(sizeof(*c), GFP_KERNEL);
> > > if (!c)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - cfg = _intf_offset(idx, m, addr, &c->hw);
> > > - if (IS_ERR_OR_NULL(cfg)) {
> > > - kfree(c);
> > > - pr_err("failed to create dpu_hw_intf %d\n", idx);
> > > - return ERR_PTR(-EINVAL);
> > > - }
> > > + c->hw.blk_addr = addr + cfg->base;
> > > + c->hw.log_mask = DPU_DBG_MASK_INTF;
> > >
> >
> > <snipped>
--
With best wishes
Dmitry
