Re: [RFC PATCH v2 05/13] drm/msm/dpu: get rid of struct dpu_rm_requirements

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 5/17/2023 4:53 PM, Abhinav Kumar wrote:
> On 5/14/2023 10:06 AM, Dmitry Baryshkov wrote:
> > On Sat, 13 May 2023 at 01:39, Abhinav Kumar 
> > <quic_abhinavk@xxxxxxxxxxx> wrote:
> > > On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
> > > > The struct dpu_rm_requirements was used to wrap display topology and
> > > > hw resources, which meant INTF indices. As of commit ef58e0ad3436
> > > > ("drm/msm/dpu: get INTF blocks directly rather than through RM") the hw
> > > > resources struct was removed, leaving struct dpu_rm_requirements
> > > > containing a single field (topology). Remove the useless wrapper.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > >
> > > Irrespective of where we plan to have the topology, this change doesn't
> > > seem incorrect as such.
> > >
> > > The only thing I can think of is when we need more information to be
> > > passed to the RM to allocate the blocks in addition to the topology this
> > > struct could have been expanded.
> > >
> > > So one example I can think of is lets say I want to add CDM block
> > > support. Then that information is outside of topology today because I
> > > will use CDM if my output format is yuv. It has nothing to do with
> > > topology but that block still needs to come from RM.
> >
> > I'd say, it is a part of the topology. CDM blocks are a part of the
> > pipeline, aren't they?
> >
> > If you prefer, we can rename msm_display_topology to 
> > dpu_rm_requirements itself.
>
> I am fine with renaming msm_display_topology to dpu_rm_requirements.
>
> Because making CDM part of topology wont be right.

Just to add, perhaps rename it to dpu_rm_display_requirements

> > > I know that usually I have lost on these type of discussions saying that
> > > if the code is not there yet, it should be dropped but I do have a plan
> > > to add that support soon probably by the next cycle. That time we will
> > > need some sort of wrapper to hold the topology and "extra" information
> > > to allocate the blocks.
> > >
> > > One alternative ofcourse is to expand dpu_rm_reserve() to accept
> > > something like "needs_cdm" but this is not scalable.
> > >
> > > Thoughts?
> > >
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  2 +-
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      | 69 
> > > > +++++++--------------
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h      |  2 +-
> > > >    3 files changed, 23 insertions(+), 50 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > index 4ee708264f3b..a2cb23dea0b8 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > @@ -638,7 +638,7 @@ static int dpu_encoder_virt_atomic_check(
> > > >
> > > >                if (!crtc_state->active_changed || crtc_state->enable)
> > > >                        ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> > > > -                                     drm_enc, crtc_state, topology);
> > > > +                                     drm_enc, crtc_state, &topology);
> > > >        }
> > > >
> > > >        trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), 
> > > > adj_mode->flags);
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > > index f4dda88a73f7..952e139c0234 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> > > > @@ -24,15 +24,6 @@ static inline bool reserved_by_other(uint32_t 
> > > > *res_map, int idx,
> > > >        return res_map[idx] && res_map[idx] != enc_id;
> > > >    }
> > > >
> > > > -/**
> > > > - * struct dpu_rm_requirements - Reservation requirements parameter 
> > > > bundle
> > > > - * @topology:  selected topology for the display
> > > > - * @hw_res:     Hardware resources required as reported by the 
> > > > encoders
> > > > - */
> > > > -struct dpu_rm_requirements {
> > > > -     struct msm_display_topology topology;
> > > > -};
> > > > -
> > > >    int dpu_rm_destroy(struct dpu_rm *rm)
> > > >    {
> > > >        int i;
> > > > @@ -329,14 +320,13 @@ static bool _dpu_rm_check_lm_peer(struct 
> > > > dpu_rm *rm, int primary_idx,
> > > >     *      mixer in rm->pingpong_blks[].
> > > >     * @dspp_idx: output parameter, index of dspp block attached to 
> > > > the layer
> > > >     *      mixer in rm->dspp_blks[].
> > > > - * @reqs: input parameter, rm requirements for HW blocks needed in the
> > > > - *      datapath.
> > > > + * @topology:  selected topology for the display
> > > >     * Return: true if lm matches all requirements, false otherwise
> > > >     */
> > > >    static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm 
> > > > *rm,
> > > >                struct dpu_global_state *global_state,
> > > >                uint32_t enc_id, int lm_idx, int *pp_idx, int *dspp_idx,
> > > > -             struct dpu_rm_requirements *reqs)
> > > > +             struct msm_display_topology *topology)
> > > >    {
> > > >        const struct dpu_lm_cfg *lm_cfg;
> > > >        int idx;
> > > > @@ -361,7 +351,7 @@ static bool 
> > > > _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
> > > >        }
> > > >        *pp_idx = idx;
> > > >
> > > > -     if (!reqs->topology.num_dspp)
> > > > +     if (!topology->num_dspp)
> > > >                return true;
> > > >
> > > >        idx = lm_cfg->dspp - DSPP_0;
> > > > @@ -383,7 +373,7 @@ static bool 
> > > > _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
> > > >    static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >                               struct dpu_global_state *global_state,
> > > >                               uint32_t enc_id,
> > > > -                            struct dpu_rm_requirements *reqs)
> > > > +                            struct msm_display_topology *topology)
> > > >
> > > >    {
> > > >        int lm_idx[MAX_BLOCKS];
> > > > @@ -391,14 +381,14 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >        int dspp_idx[MAX_BLOCKS] = {0};
> > > >        int i, j, lm_count = 0;
> > > >
> > > > -     if (!reqs->topology.num_lm) {
> > > > -             DPU_ERROR("invalid number of lm: %d\n", 
> > > > reqs->topology.num_lm);
> > > > +     if (!topology->num_lm) {
> > > > +             DPU_ERROR("invalid number of lm: %d\n", 
> > > > topology->num_lm);
> > > >                return -EINVAL;
> > > >        }
> > > >
> > > >        /* Find a primary mixer */
> > > >        for (i = 0; i < ARRAY_SIZE(rm->mixer_blks) &&
> > > > -                     lm_count < reqs->topology.num_lm; i++) {
> > > > +                     lm_count < topology->num_lm; i++) {
> > > >                if (!rm->mixer_blks[i])
> > > >                        continue;
> > > >
> > > > @@ -407,7 +397,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >
> > > >                if (!_dpu_rm_check_lm_and_get_connected_blks(rm, 
> > > > global_state,
> > > >                                enc_id, i, &pp_idx[lm_count],
> > > > -                             &dspp_idx[lm_count], reqs)) {
> > > > +                             &dspp_idx[lm_count], topology)) {
> > > >                        continue;
> > > >                }
> > > >
> > > > @@ -415,7 +405,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >
> > > >                /* Valid primary mixer found, find matching peers */
> > > >                for (j = i + 1; j < ARRAY_SIZE(rm->mixer_blks) &&
> > > > -                             lm_count < reqs->topology.num_lm; j++) {
> > > > +                             lm_count < topology->num_lm; j++) {
> > > >                        if (!rm->mixer_blks[j])
> > > >                                continue;
> > > >
> > > > @@ -428,7 +418,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >                        if (!_dpu_rm_check_lm_and_get_connected_blks(rm,
> > > >                                        global_state, enc_id, j,
> > > >                                        &pp_idx[lm_count], 
> > > > &dspp_idx[lm_count],
> > > > -                                     reqs)) {
> > > > +                                     topology)) {
> > > >                                continue;
> > > >                        }
> > > >
> > > > @@ -437,7 +427,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >                }
> > > >        }
> > > >
> > > > -     if (lm_count != reqs->topology.num_lm) {
> > > > +     if (lm_count != topology->num_lm) {
> > > >                DPU_DEBUG("unable to find appropriate mixers\n");
> > > >                return -ENAVAIL;
> > > >        }
> > > > @@ -446,7 +436,7 @@ static int _dpu_rm_reserve_lms(struct dpu_rm *rm,
> > > >                global_state->mixer_to_enc_id[lm_idx[i]] = enc_id;
> > > >                global_state->pingpong_to_enc_id[pp_idx[i]] = enc_id;
> > > >                global_state->dspp_to_enc_id[dspp_idx[i]] =
> > > > -                     reqs->topology.num_dspp ? enc_id : 0;
> > > > +                     topology->num_dspp ? enc_id : 0;
> > > >
> > > >                trace_dpu_rm_reserve_lms(lm_idx[i] + LM_0, enc_id,
> > > >                                         pp_idx[i] + PINGPONG_0);
> > > > @@ -539,44 +529,30 @@ static int _dpu_rm_make_reservation(
> > > >                struct dpu_rm *rm,
> > > >                struct dpu_global_state *global_state,
> > > >                struct drm_encoder *enc,
> > > > -             struct dpu_rm_requirements *reqs)
> > > > +             struct msm_display_topology *topology)
> > > >    {
> > > >        int ret;
> > > >
> > > > -     ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id, reqs);
> > > > +     ret = _dpu_rm_reserve_lms(rm, global_state, enc->base.id, 
> > > > topology);
> > > >        if (ret) {
> > > >                DPU_ERROR("unable to find appropriate mixers\n");
> > > >                return ret;
> > > >        }
> > > >
> > > >        ret = _dpu_rm_reserve_ctls(rm, global_state, enc->base.id,
> > > > -                             &reqs->topology);
> > > > +                                topology);
> > > >        if (ret) {
> > > >                DPU_ERROR("unable to find appropriate CTL\n");
> > > >                return ret;
> > > >        }
> > > >
> > > > -     ret  = _dpu_rm_reserve_dsc(rm, global_state, enc, 
> > > > &reqs->topology);
> > > > +     ret  = _dpu_rm_reserve_dsc(rm, global_state, enc, topology);
> > > >        if (ret)
> > > >                return ret;
> > > >
> > > >        return ret;
> > > >    }
> > > >
> > > > -static int _dpu_rm_populate_requirements(
> > > > -             struct drm_encoder *enc,
> > > > -             struct dpu_rm_requirements *reqs,
> > > > -             struct msm_display_topology req_topology)
> > > > -{
> > > > -     reqs->topology = req_topology;
> > > > -
> > > > -     DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
> > > > -                   reqs->topology.num_lm, reqs->topology.num_dsc,
> > > > -                   reqs->topology.num_intf);
> > > > -
> > > > -     return 0;
> > > > -}
> > > > -
> > > >    static void _dpu_rm_clear_mapping(uint32_t *res_mapping, int cnt,
> > > >                                  uint32_t enc_id)
> > > >    {
> > > > @@ -608,9 +584,8 @@ int dpu_rm_reserve(
> > > >                struct dpu_global_state *global_state,
> > > >                struct drm_encoder *enc,
> > > >                struct drm_crtc_state *crtc_state,
> > > > -             struct msm_display_topology topology)
> > > > +             struct msm_display_topology *topology)
> > > >    {
> > > > -     struct dpu_rm_requirements reqs;
> > > >        int ret;
> > > >
> > > >        /* Check if this is just a page-flip */
> > > > @@ -625,13 +600,11 @@ int dpu_rm_reserve(
> > > >        DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n",
> > > >                      enc->base.id, crtc_state->crtc->base.id);
> > > >
> > > > -     ret = _dpu_rm_populate_requirements(enc, &reqs, topology);
> > > > -     if (ret) {
> > > > -             DPU_ERROR("failed to populate hw requirements\n");
> > > > -             return ret;
> > > > -     }
> > > > +     DRM_DEBUG_KMS("num_lm: %d num_dsc: %d num_intf: %d\n",
> > > > +                   topology->num_lm, topology->num_dsc,
> > > > +                   topology->num_intf);
> > > >
> > > > -     ret = _dpu_rm_make_reservation(rm, global_state, enc, &reqs);
> > > > +     ret = _dpu_rm_make_reservation(rm, global_state, enc, topology);
> > > >        if (ret)
> > > >                DPU_ERROR("failed to reserve hw resources: %d\n", ret);
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > > index d62c2edb2460..f05697462856 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> > > > @@ -71,7 +71,7 @@ int dpu_rm_reserve(struct dpu_rm *rm,
> > > >                struct dpu_global_state *global_state,
> > > >                struct drm_encoder *drm_enc,
> > > >                struct drm_crtc_state *crtc_state,
> > > > -             struct msm_display_topology topology);
> > > > +             struct msm_display_topology *topology);
> > > >
> > > >    /**
> > > >     * dpu_rm_reserve - Given the encoder for the display chain, 
> > > > release any