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