Messages in this thread | | | Date | Mon, 24 Apr 2023 16:23:17 -0700 | Subject | Re: [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs | From | Abhinav Kumar <> |
| |
On 4/24/2023 3:54 PM, Dmitry Baryshkov wrote: > On Tue, 25 Apr 2023 at 01:03, Marijn Suijten > <marijn.suijten@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@somainline.org> >>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@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.
>> >> - 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> > > >
| |