lkml.org 
[lkml]   [2023]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 3/3] drm/msm/dpu: Pass catalog pointers directly from RM instead of IDs
    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@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.

    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.

    - Marijn

    \
     
     \ /
      Last update: 2023-04-25 09:16    [W:4.424 / U:0.208 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site