[Freedreno] [PATCH 3/7] drm/msm/dpu: inline _setup_intf_ops()

[Dmitry Baryshkov]

On Sat, 29 Jul 2023 at 21:28, Marijn Suijten
<marijn.suijten at somainline.org> wrote:
>
> On 2023-07-29 02:45:43, Dmitry Baryshkov wrote:
> > On 27/07/2023 23:10, Marijn Suijten wrote:
> > > On 2023-07-27 19:21:00, Dmitry Baryshkov wrote:
> > >> Inline the _setup_intf_ops() function, it makes it easier to handle
> > >> different conditions involving INTF configuration.
> > >>
> > >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > >
> > > Reviewed-by: Marijn Suijten <marijn.suijten at somainline.org>
> > >
> > >> ---
> > >>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 47 +++++++++------------
> > >>   1 file changed, 21 insertions(+), 26 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> index 8ec6505d9e78..7ca772791a73 100644
> > >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> > >> @@ -524,31 +524,6 @@ static void dpu_hw_intf_program_intf_cmd_cfg(struct dpu_hw_intf *ctx,
> > >>    DPU_REG_WRITE(&ctx->hw, INTF_CONFIG2, intf_cfg2);
> > >>   }
> > >>
> > >> -static void _setup_intf_ops(struct dpu_hw_intf_ops *ops,
> > >> -          unsigned long cap, const struct dpu_mdss_version *mdss_rev)
> > >> -{
> > >> -  ops->setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> > >> -  ops->setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> > >> -  ops->get_status = dpu_hw_intf_get_status;
> > >> -  ops->enable_timing = dpu_hw_intf_enable_timing_engine;
> > >> -  ops->get_line_count = dpu_hw_intf_get_line_count;
> > >> -  if (cap & BIT(DPU_INTF_INPUT_CTRL))
> > >> -          ops->bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> > >> -  ops->setup_misr = dpu_hw_intf_setup_misr;
> > >> -  ops->collect_misr = dpu_hw_intf_collect_misr;
> > >> -
> > >> -  if (cap & BIT(DPU_INTF_TE)) {
> > >> -          ops->enable_tearcheck = dpu_hw_intf_enable_te;
> > >> -          ops->disable_tearcheck = dpu_hw_intf_disable_te;
> > >> -          ops->connect_external_te = dpu_hw_intf_connect_external_te;
> > >> -          ops->vsync_sel = dpu_hw_intf_vsync_sel;
> > >> -          ops->disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> > >> -  }
> > >> -
> > >> -  if (mdss_rev->core_major_ver >= 7)
> > >> -          ops->program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> > >> -}
> > >> -
> > >>   struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > >>            void __iomem *addr, const struct dpu_mdss_version *mdss_rev)
> > >>   {
> > >> @@ -571,7 +546,27 @@ struct dpu_hw_intf *dpu_hw_intf_init(const struct dpu_intf_cfg *cfg,
> > >>     */
> > >>    c->idx = cfg->id;
> > >>    c->cap = cfg;
> > >> -  _setup_intf_ops(&c->ops, c->cap->features, mdss_rev);
> > >> +
> > >> +  c->ops.setup_timing_gen = dpu_hw_intf_setup_timing_engine;
> > >> +  c->ops.setup_prg_fetch  = dpu_hw_intf_setup_prg_fetch;
> > >> +  c->ops.get_status = dpu_hw_intf_get_status;
> > >> +  c->ops.enable_timing = dpu_hw_intf_enable_timing_engine;
> > >> +  c->ops.get_line_count = dpu_hw_intf_get_line_count;
> > >> +  if (cfg->features & BIT(DPU_INTF_INPUT_CTRL))
> > >> +          c->ops.bind_pingpong_blk = dpu_hw_intf_bind_pingpong_blk;
> > >
> > > While at it, could we sort these down with the other conditional
> > > callbacks?
> >
> > What kind of sorting do you have in mind?
>
> Moving this conditional ( if (...) ) down with the other conditional
> assignment below, instead of being right in the middle of get_line_count
> and setup_misr, both which are not conditional and make it harder to
> read, especially considering the lack of newlines and/or curly braces.

Ack, sounds good.

>
> > >> +  c->ops.setup_misr = dpu_hw_intf_setup_misr;
> > >> +  c->ops.collect_misr = dpu_hw_intf_collect_misr;
> > >> +
> > >> +  if (cfg->features & BIT(DPU_INTF_TE)) {
> > >
> > > Any clue why we're not using test_bit()?  Feels a bit inconsistent.
> >
> > Yes, some files use test_bit(), others just check the bit directly.
> > Maybe after moving some/most of conditionals to core_major_ver we can
> > clean that too.
>
> Sounds good.
>
> - Marijn
>
> > >> +          c->ops.enable_tearcheck = dpu_hw_intf_enable_te;
> > >> +          c->ops.disable_tearcheck = dpu_hw_intf_disable_te;
> > >> +          c->ops.connect_external_te = dpu_hw_intf_connect_external_te;
> > >> +          c->ops.vsync_sel = dpu_hw_intf_vsync_sel;
> > >> +          c->ops.disable_autorefresh = dpu_hw_intf_disable_autorefresh;
> > >> +  }
> > >> +
> > >> +  if (mdss_rev->core_major_ver >= 7)
> > >> +          c->ops.program_intf_cmd_cfg = dpu_hw_intf_program_intf_cmd_cfg;
> > >>
> > >>    return c;
> > >>   }
> > >> --
> > >> 2.39.2
> > >>
> >
> > --
> > With best wishes
> > Dmitry
> >



-- 
With best wishes
Dmitry