On Tue, Aug 07, 2018 at 08:20:11PM -0700, Jeykumar Sankaran wrote: > Switch to state based resource management. This patch > overhauls the resource manager and HW allocation methods by > maintaining the global resource pool and allocated hw > blocks in respective drm component states. > > Global resource manager(RM) is tracked in private object. > Allocation strategy is switched from single point allocation > of HW resources for the display pipeline to per component > based allocation, where each drm component allocates HW > blocks mapped to it's domain and tracks them in their respective > state objects. > > Fix resource contention due to race conditions between > user space and display thread by reserving resources > only in atomic check. > > changes in v2: > - none > changes in v3: > - rebased on [1] > - fix control path bug in split LM topology > > [1] https://gitlab.freedesktop.org/seanpaul/dpu-staging/commits/for-next > > Change-Id: Ie9d42eb3e93257816daf3d36c444a335645d65c6 > Signed-off-by: Jeykumar Sankaran <jsanka@xxxxxxxxxxxxxx> Ok, so after chatting with you yesterday, I decided to review this as one patch. I feel like a bunch of the rm refactors could have been split out, as well as some of the state tracking, renames, etc. I caught a bunch of simple mistakes such as spelling, documentation, indentation, unused code left around. However given the size of the patch, I'm sure there's a bunch I also missed. Can you please make sure you thoroughly review the code before sending a v2 to ensure it's all cleaned up? I'm sorry to be grumpy, it's just hard allocating ~3 contiguous hours of my day to review one patch. If this is split up, I can be much more responsive. Sean > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 165 ++--- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 32 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 133 ++-- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 2 +- > .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 22 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 19 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 8 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 783 ++++++--------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 140 ++-- > 9 files changed, 451 insertions(+), 853 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 0eb369c..dbff870 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -48,6 +48,8 @@ > #define RIGHT_MIXER 1 > > #define MISR_BUFF_SIZE 256 > +#define MAX_VDISPLAY_SPLIT 1080 > + > > static inline struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc) > { > @@ -258,16 +260,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) > > DPU_DEBUG("%s\n", dpu_crtc->name); > > - if (cstate->num_mixers > CRTC_DUAL_MIXERS) { > - DPU_ERROR("invalid number mixers: %d\n", cstate->num_mixers); > - return; > - } > - > for (i = 0; i < cstate->num_mixers; i++) { > - if (!mixer[i].hw_lm || !mixer[i].lm_ctl) { > - DPU_ERROR("invalid lm or ctl assigned to mixer\n"); > - return; > - } > mixer[i].mixer_op_mode = 0; > mixer[i].flush_mask = 0; > if (mixer[i].lm_ctl->ops.clear_all_blendstages) > @@ -498,75 +491,33 @@ void dpu_crtc_complete_commit(struct drm_crtc *crtc, > trace_dpu_crtc_complete_commit(DRMID(crtc)); > } > > -static void _dpu_crtc_setup_mixer_for_encoder( > - struct drm_crtc *crtc, > - struct drm_encoder *enc) > +static void _dpu_crtc_setup_mixers(struct drm_crtc_state *crtc_state) > { > - struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > - struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); > - struct dpu_rm *rm = &dpu_kms->rm; > + struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state); > struct dpu_crtc_mixer *mixer; > - struct dpu_hw_ctl *last_valid_ctl = NULL; > - int i; > - struct dpu_rm_hw_iter lm_iter, ctl_iter; > - > - dpu_rm_init_hw_iter(&lm_iter, enc->base.id, DPU_HW_BLK_LM); > - dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL); > + int i = 0, j = 0; > > - /* Set up all the mixers and ctls reserved by this encoder */ > - for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) { > - mixer = &cstate->mixers[i]; > - > - if (!dpu_rm_get_hw(rm, &lm_iter)) > - break; > - mixer->hw_lm = (struct dpu_hw_mixer *)lm_iter.hw; > - > - /* CTL may be <= LMs, if <, multiple LMs controlled by 1 CTL */ > - if (!dpu_rm_get_hw(rm, &ctl_iter)) { > - DPU_DEBUG("no ctl assigned to lm %d, using previous\n", > - mixer->hw_lm->idx - LM_0); > - mixer->lm_ctl = last_valid_ctl; > - } else { > - mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw; > - last_valid_ctl = mixer->lm_ctl; > - } > - > - /* Shouldn't happen, mixers are always >= ctls */ > - if (!mixer->lm_ctl) { > - DPU_ERROR("no valid ctls found for lm %d\n", > - mixer->hw_lm->idx - LM_0); > - return; > - } > - > - mixer->encoder = enc; > - > - cstate->num_mixers++; > - DPU_DEBUG("setup mixer %d: lm %d\n", > - i, mixer->hw_lm->idx - LM_0); > - DPU_DEBUG("setup mixer %d: ctl %d\n", > - i, mixer->lm_ctl->idx - CTL_0); > + if (cstate->num_mixers < cstate->num_ctls) { > + DPU_ERROR( > + "lm count(%d) < ctl count(%d). No support for such topologies\n", Break this line up please > + cstate->num_mixers, > + cstate->num_ctls); These should be on the same line > + return; This function should return an error, and the return code should be checked in atomic_check so we don't commit an invalid configuration. > } > -} > > -static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) > -{ > - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > - struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); > - struct drm_encoder *enc; > - > - cstate->num_mixers = 0; > - memset(cstate->mixers, 0, sizeof(cstate->mixers)); > + /* Set up all the mixers and ctls reserved by this encoder */ > + for (i = 0; i < cstate->num_mixers; i++) { > + /* 3D merge topology can have num_mixers > num_ctls */ > + if (i < cstate->num_ctls) > + j = i; I think the following is more clear: hw_ctl_idx = min(i, cstate->num_ctls - 1); > > - mutex_lock(&dpu_crtc->crtc_lock); > - /* Check for mixers on all encoders attached to this crtc */ > - list_for_each_entry(enc, &crtc->dev->mode_config.encoder_list, head) { > - if (enc->crtc != crtc) > - continue; > + cstate->mixers[i].lm_ctl = cstate->hw_ctls[j]; > + mixer = &cstate->mixers[i]; This local isn't needed, just use the expansion in the log msg > > - _dpu_crtc_setup_mixer_for_encoder(crtc, enc); > + DPU_DEBUG("setup mixer %d: lm(%d) - ctl(%d)\n", > + i, mixer->hw_lm->idx - LM_0, > + mixer->lm_ctl->idx - CTL_0); Arguments should be aligned with the opening brace. You should probably add a trace for this instead of a DPU_DEBUG. > } > - > - mutex_unlock(&dpu_crtc->crtc_lock); > } > > static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > @@ -606,7 +557,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, > struct drm_crtc_state *old_state) > { > struct dpu_crtc *dpu_crtc; > - struct dpu_crtc_state *cstate; > + struct dpu_crtc_state *new_cstate; > struct drm_encoder *encoder; > struct drm_device *dev; > unsigned long flags; > @@ -626,14 +577,11 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, > DPU_DEBUG("crtc%d\n", crtc->base.id); > > dpu_crtc = to_dpu_crtc(crtc); > - cstate = to_dpu_crtc_state(crtc->state); > + new_cstate = to_dpu_crtc_state(crtc->state); > dev = crtc->dev; > smmu_state = &dpu_crtc->smmu_state; > > - if (!cstate->num_mixers) { > - _dpu_crtc_setup_mixers(crtc); > - _dpu_crtc_setup_lm_bounds(crtc, crtc->state); > - } > + _dpu_crtc_setup_lm_bounds(crtc, crtc->state); It seems like this function can fail if crtc or crtc->state is null, however there's no return value. Fortunately that scenario is impossible, so you can just remove the NULL checks from that function. > > if (dpu_crtc->event) { > WARN_ON(dpu_crtc->event); > @@ -657,7 +605,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, > * it means we are trying to flush a CRTC whose state is disabled: > * nothing else needs to be done. > */ > - if (unlikely(!cstate->num_mixers)) > + if (unlikely(!new_cstate->num_mixers)) It seems like this isn't possible? num_mixers is always set to num_lms when the crtc is active, and 0 when it's not. Since you're already doing enabled/active checks, this check is redundant.A I think having both num_lms and num_mixers is pretty confusing, it took me longer than I'd like to admit to figure out where num_mixers gets its value. I think it'd be a lot easier to just assign the value to cstate->num_mixer directly. > return; > > _dpu_crtc_blend_setup(crtc); > @@ -1069,9 +1017,9 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) > { > struct drm_crtc *crtc = arg; > struct dpu_crtc *dpu_crtc; > + struct dpu_crtc_state *cstate; > struct drm_encoder *encoder; > struct dpu_crtc_mixer *m; > - struct dpu_crtc_state *cstate; > u32 i, misr_status; > > if (!crtc) { > @@ -1079,7 +1027,7 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) > return; > } > dpu_crtc = to_dpu_crtc(crtc); > - cstate = to_dpu_crtc_state(dpu_crtc->base.state); > + cstate = to_dpu_crtc_state(crtc->state); > > mutex_lock(&dpu_crtc->crtc_lock); > > @@ -1138,6 +1086,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > struct drm_display_mode *mode; > struct drm_encoder *encoder; > struct msm_drm_private *priv; > + struct dpu_private_state *dpu_priv_state; > + struct dpu_kms *dpu_kms; > int ret; > unsigned long flags; > > @@ -1149,6 +1099,10 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > cstate = to_dpu_crtc_state(crtc->state); > mode = &cstate->base.adjusted_mode; > priv = crtc->dev->dev_private; > + dpu_kms = to_dpu_kms(priv->kms); > + > + /* accessing after swap state. piv_obj.state is the current state */ s/piv/priv/ > + dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state); Can you please use a helper like Archit did for mdp5 for this with a nice explanation of what it returns when called from check vs. after swap. > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > @@ -1195,13 +1149,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > dpu_power_handle_unregister_event(dpu_crtc->phandle, > dpu_crtc->power_event); > > - memset(cstate->mixers, 0, sizeof(cstate->mixers)); > - cstate->num_mixers = 0; > - > /* disable clk & bw control until clk & bw properties are set */ > cstate->bw_control = false; > cstate->bw_split_vote = false; > > + ret = dpu_rm_release_crtc_res(&dpu_priv_state->rm, cstate); > + if (ret) > + DPU_ERROR("error in releasing crtc resources\n"); Print the return code please > + > mutex_unlock(&dpu_crtc->crtc_lock); > > if (crtc->state->event && !crtc->state->active) { > @@ -1260,6 +1215,31 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, > > } > > +static struct dpu_crtc_topology > +dpu_crtc_get_topology(struct dpu_crtc_state *cstate, > + struct drm_display_mode *mode) > +{ > + struct dpu_crtc_topology topology; > + > + memset(&topology, 0, sizeof(topology)); > + > + /* Use split topology for width > 1080 */ > + topology.num_lms = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; As mentioned above, this can go right in cstate->num_mixers > + topology.num_ctls = cstate->num_intfs; Since this is already accessible in the rm via cstate, it's also not needed in topology. > + > + topology.needs_realloc = (topology.num_lms != cstate->num_mixers) || > + (topology.num_ctls != cstate->num_ctls); This is also derived from cstate and it isn't used anywhere other than locally (in both crtc/encoder), so it can also go. So we can remove num_lms, num_cls, needs_realloc. That leaves us just with num_intfs in topology, and no one uses that! So we can remove the entire struct and rely on just on cstate. > + > + if (topology.needs_realloc) > + DPU_DEBUG( > + "crtc %d needs hw reallocation. lm (%d - %d) ctl(%d - %d)\n", The way to break these up is to put as much on one line as possible and then do another string on the next line, like so: DPU_DEBUG("crtc %d needs hw reallocation. lm (%d - %d) " "ctl(%d - %d)\n", get_crtc_id(cstate), cstate->num_mixers, topology.num_lms, cstate->num_ctls, topology.num_ctls); However, you can usually make your print more concise to avoid this: DPU_DEBUG("crtc %d needs realloc lm(%d - %d) ctl(%d - %d)\n", get_crtc_id(cstate), cstate->num_mixers, topology.num_lms, cstate->num_ctls, topology.num_ctls); > + get_crtc_id(cstate), > + cstate->num_mixers, topology.num_lms, > + cstate->num_ctls, topology.num_ctls); > + > + return topology; > +} > + > struct plane_state { > struct dpu_plane_state *dpu_pstate; > const struct drm_plane_state *drm_pstate; > @@ -1277,6 +1257,8 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > const struct drm_plane_state *pstate; > struct drm_plane *plane; > struct drm_display_mode *mode; > + struct dpu_crtc_topology topology; > + struct dpu_private_state *dpu_priv_state; > > int cnt = 0, rc = 0, mixer_width, i, z_pos; > > @@ -1493,6 +1475,25 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > } > } > > + /* Resource allocation */ > + dpu_priv_state = dpu_get_private_state(state->state); > + if (IS_ERR(dpu_priv_state)) > + goto end; > + > + topology = dpu_crtc_get_topology(cstate, &state->adjusted_mode); > + if (!topology.needs_realloc) > + goto end; > + > + dpu_rm_release_crtc_res(&dpu_priv_state->rm, cstate); Check the return value and fail if necessary. > + rc = dpu_rm_reserve_crtc_res(&dpu_priv_state->rm, cstate, &topology); > + if (rc) { > + DPU_ERROR("failed to allocate resources for crtc: %d\n", > + crtc->base.id); > + goto end; > + } > + > + _dpu_crtc_setup_mixers(state); This can fail, so it should return an error code and you should check it here. > + > end: > kfree(pstates); > return rc; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > index 5b85ca8..15c30a9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > @@ -30,6 +30,8 @@ > /* define the maximum number of in-flight frame events */ > #define DPU_CRTC_FRAME_EVENT_SIZE 4 > > +struct dpu_kms; This file includes dpu_kms.h, and you haven't added anything in this header that references dpu_kms, why is this here? > + > /** > * enum dpu_crtc_client_type: crtc client type > * @RT_CLIENT: RealTime client like video/cmd mode display > @@ -83,15 +85,15 @@ struct dpu_crtc_smmu_state_data { > /** > * struct dpu_crtc_mixer: stores the map for each virtual pipeline in the CRTC > * @hw_lm: LM HW Driver context > - * @lm_ctl: CTL Path HW driver context > - * @encoder: Encoder attached to this lm & ctl > + * @lm_ctl: CTL path for associated LM HW > + * @hw_pp: Pingpong HW driver context > * @mixer_op_mode: mixer blending operation mode > * @flush_mask: mixer flush mask for ctl, mixer and pipe > */ > struct dpu_crtc_mixer { > struct dpu_hw_mixer *hw_lm; > struct dpu_hw_ctl *lm_ctl; > - struct drm_encoder *encoder; > + struct dpu_hw_pingpong *hw_pp; > u32 mixer_op_mode; > u32 flush_mask; > }; > @@ -117,6 +119,13 @@ struct dpu_crtc_frame_event { > */ > #define DPU_CRTC_MAX_EVENT_COUNT 16 > > +struct dpu_crtc_topology { > + bool needs_realloc; > + int num_ctls; > + int num_lms; > + int num_intfs; > +}; > + > /** > * struct dpu_crtc - virtualized CRTC data structure > * @base : Base drm crtc structure > @@ -220,6 +229,8 @@ struct dpu_crtc { > * @mixers : List of active mixers > * @num_ctls : Number of ctl paths in use > * @hw_ctls : List of activel ctl paths > + * @num_intf : Numeer of interfaces in uses s/Numeer/Number/ > + * @hw_intfs : List of interfaces in use > */ > struct dpu_crtc_state { > struct drm_crtc_state base; > @@ -237,11 +248,26 @@ struct dpu_crtc_state { > > u32 num_ctls; > struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS]; > + > + /** > + * as drm encoders doesn't have dedicates state objects s/dedicates/dedicated/ > + * and drm connectors are not owned by DPU, maintain > + * HW interface and other interface related blocks > + * in crtc state This sounds like exactly what the driver private state is for. > + * > + * TODO: No support for clone mode yet where a crtc > + * can be attached with more than one encoder/connector. > + */ > + u32 num_intfs; > + struct dpu_hw_intf *hw_intfs[CRTC_DUAL_MIXERS]; > }; > > #define to_dpu_crtc_state(x) \ > container_of(x, struct dpu_crtc_state, base) > > +/* get crtc id from dpu crtc state*/ > +#define get_crtc_id(x) ((x->base.crtc)->base.id) Better to use crtc->index than the obj id. Also, please convert to static inline function instead of macro. The better approach, IMO, would be to just pass crtc into the functions needing to index it, instead of following backpointers to crtc. That said, as long as the state isn't referenced outside of the appropriate modeset locks, I suppose there's nothing wrong with this. > + > /** > * dpu_crtc_get_mixer_width - get the mixer width > * Mixer width will be same as panel width(/2 for split) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index fe0b563..9f5f59f 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -69,8 +69,6 @@ > > #define IDLE_SHORT_TIMEOUT 1 > > -#define MAX_VDISPLAY_SPLIT 1080 > - > /** > * enum dpu_enc_rc_events - events for resource control state machine > * @DPU_ENC_RC_EVENT_KICKOFF: > @@ -572,25 +570,32 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector, > } > } > > -static struct msm_display_topology dpu_encoder_get_topology( > +static struct dpu_crtc_topology dpu_encoder_get_topology( > struct dpu_encoder_virt *dpu_enc, > - struct dpu_kms *dpu_kms, > - struct drm_display_mode *mode) > + struct dpu_crtc_state *cstate) > { > - struct msm_display_topology topology; > + struct dpu_crtc_topology topology; > int i, intf_count = 0; > > + memset(&topology, 0, sizeof(topology)); > + > for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++) > if (dpu_enc->phys_encs[i]) > intf_count++; > > - /* User split topology for width > 1080 */ > - topology.num_lm = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; > - topology.num_enc = 0; > - topology.num_intf = intf_count; > + topology.num_intfs = intf_count; > + > + topology.needs_realloc = (cstate->num_intfs != topology.num_intfs); > + > + if (topology.needs_realloc) As mentioned above, you can get rid of topology. > + DPU_DEBUG_ENC(dpu_enc, > + "crtc %d needs hw reallocation. intf (%d - %d)\n", > + get_crtc_id(cstate), > + cstate->num_intfs, topology.num_intfs); > > return topology; > } > + > static int dpu_encoder_virt_atomic_check( > struct drm_encoder *drm_enc, > struct drm_crtc_state *crtc_state, > @@ -601,7 +606,10 @@ static int dpu_encoder_virt_atomic_check( > struct dpu_kms *dpu_kms; > const struct drm_display_mode *mode; > struct drm_display_mode *adj_mode; > - struct msm_display_topology topology; > + struct dpu_crtc_topology topology; > + struct dpu_crtc_state *dpu_cstate; > + struct dpu_private_state *dpu_priv_state; > + struct dpu_encoder_hw_resources enc_hw_res; > int i = 0; > int ret = 0; > > @@ -618,6 +626,7 @@ static int dpu_encoder_virt_atomic_check( > dpu_kms = to_dpu_kms(priv->kms); > mode = &crtc_state->mode; > adj_mode = &crtc_state->adjusted_mode; > + dpu_cstate = to_dpu_crtc_state(crtc_state); > trace_dpu_enc_atomic_check(DRMID(drm_enc)); > > /* > @@ -647,28 +656,35 @@ static int dpu_encoder_virt_atomic_check( > } > } > > - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); > + if (ret) > + goto end; > > - /* Reserve dynamic resources now. Indicating AtomicTest phase */ > - if (!ret) { > - /* > - * Avoid reserving resources when mode set is pending. Topology > - * info may not be available to complete reservation. > - */ > - if (drm_atomic_crtc_needs_modeset(crtc_state) > - && dpu_enc->mode_set_complete) { > - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state, > - conn_state, topology, true); > - dpu_enc->mode_set_complete = false; > - } > + /* hw resource allocation */ > + dpu_encoder_get_hw_resources(drm_enc, &enc_hw_res, conn_state); > + > + dpu_priv_state = dpu_get_private_state(crtc_state->state); > + if (IS_ERR(dpu_priv_state)) > + goto end; > + > + topology = dpu_encoder_get_topology(dpu_enc, dpu_cstate); > + if (!topology.needs_realloc) > + goto end; > + > + dpu_rm_release_encoder_res(&dpu_priv_state->rm, dpu_cstate); > + ret = dpu_rm_reserve_encoder_res(&dpu_priv_state->rm, > + dpu_cstate, &enc_hw_res); Wrapped lines should match up with opening brace. > + if (ret) { > + DPU_ERROR_ENC(dpu_enc, > + "failed to allocate hw resources\n"); Same > + goto end; > } > > - if (!ret) > - drm_mode_set_crtcinfo(adj_mode, 0); > + drm_mode_set_crtcinfo(adj_mode, 0); > > trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags, > adj_mode->private_flags); > > +end: > return ret; > } > > @@ -1014,11 +1030,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > struct dpu_kms *dpu_kms; > struct list_head *connector_list; > struct drm_connector *conn = NULL, *conn_iter; > - struct dpu_rm_hw_iter pp_iter, ctl_iter; > - struct msm_display_topology topology; > - struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC]; > - > - int i = 0, ret; > + struct dpu_crtc_state *dpu_cstate; > + int i = 0; > > if (!drm_enc) { > DPU_ERROR("invalid encoder\n"); > @@ -1046,52 +1059,16 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, > return; > } > > - topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode); > - > - /* Reserve dynamic resources now. Indicating non-AtomicTest phase */ > - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, drm_enc->crtc->state, > - conn->state, topology, false); > - if (ret) { > - DPU_ERROR_ENC(dpu_enc, > - "failed to reserve hw resources, %d\n", ret); > - return; > - } > - > - dpu_rm_init_hw_iter(&pp_iter, drm_enc->base.id, DPU_HW_BLK_PINGPONG); > - for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) { > - dpu_enc->hw_pp[i] = NULL; > - if (!dpu_rm_get_hw(&dpu_kms->rm, &pp_iter)) > - break; > - dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw; > - } > - > - dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL); > - for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) { > - hw_ctl[i] = NULL; > - if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter)) > - break; > - hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw; > - } > + dpu_cstate = to_dpu_crtc_state(drm_enc->crtc->state); > > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i]; > > if (phys) { > - if (!dpu_enc->hw_pp[i]) { > - DPU_ERROR_ENC(dpu_enc, > - "no pp block assigned at idx: %d\n", i); > - return; > - } > - phys->hw_pp = dpu_enc->hw_pp[i]; > - > - if (!hw_ctl[i]) { > - DPU_ERROR_ENC(dpu_enc, > - "no ctl block assigned at idx: %d\n", i); > - return; > - } > - phys->hw_ctl = hw_ctl[i]; > - > + dpu_enc->hw_pp[i] = dpu_cstate->mixers[i].hw_pp; > + phys->hw_pp = dpu_cstate->mixers[i].hw_pp; > phys->connector = conn->state->connector; > + phys->hw_ctl = dpu_cstate->mixers[i].lm_ctl; > if (phys->ops.mode_set) > phys->ops.mode_set(phys, mode, adj_mode); > } > @@ -1239,7 +1216,9 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > struct msm_drm_private *priv; > struct dpu_kms *dpu_kms; > struct drm_display_mode *mode; > - int i = 0; > + struct dpu_private_state *dpu_priv_state; > + struct dpu_crtc_state *cstate; > + int rc, i = 0; > > if (!drm_enc) { > DPU_ERROR("invalid encoder\n"); > @@ -1253,6 +1232,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > } > > mode = &drm_enc->crtc->state->adjusted_mode; > + cstate = to_dpu_crtc_state(drm_enc->crtc->state); > > dpu_enc = to_dpu_encoder_virt(drm_enc); > DPU_DEBUG_ENC(dpu_enc, "\n"); > @@ -1260,6 +1240,9 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > priv = drm_enc->dev->dev_private; > dpu_kms = to_dpu_kms(priv->kms); > > + /* accessing after swap state. piv_obj.state is the current state */ s/piv/priv/ > + dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state); > + > trace_dpu_enc_disable(DRMID(drm_enc)); > > /* wait for idle */ > @@ -1289,9 +1272,11 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > > dpu_enc->cur_master = NULL; > > - DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); > + rc = dpu_rm_release_encoder_res(&dpu_priv_state->rm, cstate); > + if (rc) > + DPU_ERROR("error in releasing encoder resources\n"); > > - dpu_rm_release(&dpu_kms->rm, drm_enc); > + DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); > } > > static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > index d08b5d5..1bee60b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h > @@ -374,7 +374,7 @@ static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode( > dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state); > > if (phys_enc->split_role == ENC_ROLE_SOLO && > - (dpu_cstate->num_mixers == 2)) > + (dpu_cstate->num_mixers == 2)) Unrelated change. > return BLEND_3D_H_ROW_INT; > > return BLEND_3D_NONE; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > index 88867c3..0db0d27 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c > @@ -461,35 +461,25 @@ static int dpu_encoder_phys_vid_control_vblank_irq( > > static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) > { > - struct msm_drm_private *priv; > struct dpu_encoder_phys_vid *vid_enc; > - struct dpu_rm_hw_iter iter; > struct dpu_hw_ctl *ctl; > - u32 flush_mask = 0; > + struct dpu_crtc_state *cstate; > + u32 i, flush_mask = 0; Why u32 i instead of int? > > if (!phys_enc || !phys_enc->parent || !phys_enc->parent->dev || > !phys_enc->parent->dev->dev_private) { > DPU_ERROR("invalid encoder/device\n"); > return; > } > - priv = phys_enc->parent->dev->dev_private; > - > vid_enc = to_dpu_encoder_phys_vid(phys_enc); > ctl = phys_enc->hw_ctl; > > - dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_INTF); > - while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) { > - struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf *)iter.hw; > + cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state); > + for (i = 0; i < cstate->num_intfs; i++) { > + struct dpu_hw_intf *hw_intf = cstate->hw_intfs[i]; > > - if (hw_intf->idx == phys_enc->intf_idx) { > + if (hw_intf && (hw_intf->idx == phys_enc->intf_idx)) Why check for hw_intf, isn't that protected by your loop only going to num_intfs? Also, unnecessary parens on the second conditional. > vid_enc->hw_intf = hw_intf; > - break; > - } > - } > - > - if (!vid_enc->hw_intf) { > - DPU_ERROR("hw_intf not assigned\n"); > - return; > } > > DPU_DEBUG_VIDENC(vid_enc, "\n"); > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index 5e87b9d..5892b6c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -630,6 +630,7 @@ static long dpu_kms_round_pixclk(struct msm_kms *kms, unsigned long rate, > static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms) > { > struct drm_device *dev; > + struct dpu_private_state *dpu_priv_state; > int i; > > dev = dpu_kms->dev; > @@ -657,9 +658,12 @@ static void _dpu_kms_hw_destroy(struct dpu_kms *dpu_kms) > } > } > > - if (dpu_kms->rm_init) > - dpu_rm_destroy(&dpu_kms->rm); > - dpu_kms->rm_init = false; > + dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state); > + if (dpu_priv_state) { > + if (dpu_priv_state->rm_init) > + dpu_rm_destroy(&dpu_priv_state->rm); > + dpu_priv_state->rm_init = false; > + } > > if (dpu_kms->catalog) > dpu_hw_catalog_deinit(dpu_kms->catalog); > @@ -965,6 +969,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > struct dpu_kms *dpu_kms; > struct drm_device *dev; > struct msm_drm_private *priv; > + struct dpu_private_state *dpu_priv_state; > int i, rc = -EINVAL; > > if (!kms) { > @@ -1065,16 +1070,16 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > goto power_error; > } > > - rc = dpu_rm_init(&dpu_kms->rm, dpu_kms->catalog, dpu_kms->mmio, > - dpu_kms->dev); > + dpu_priv_state = to_dpu_private_state(dpu_kms->priv_obj.state); > + rc = dpu_rm_init(&dpu_priv_state->rm, dpu_kms->catalog, dpu_kms->mmio); > if (rc) { > DPU_ERROR("rm init failed: %d\n", rc); > goto power_error; > } > > - dpu_kms->rm_init = true; > + dpu_priv_state->rm_init = true; This should be tracked inside rm (set in dpu_rm_init and checked in rm_destroy). Can you please add a follow-on patch for that? > > - dpu_kms->hw_mdp = dpu_rm_get_mdp(&dpu_kms->rm); > + dpu_kms->hw_mdp = dpu_rm_get_mdp(&dpu_priv_state->rm); > if (IS_ERR_OR_NULL(dpu_kms->hw_mdp)) { > rc = PTR_ERR(dpu_kms->hw_mdp); > if (!dpu_kms->hw_mdp) > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > index 2579c983..d24582a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > @@ -29,10 +29,10 @@ > #include "dpu_hw_lm.h" > #include "dpu_hw_interrupts.h" > #include "dpu_hw_top.h" > -#include "dpu_rm.h" > #include "dpu_power_handle.h" > #include "dpu_irq.h" > #include "dpu_core_perf.h" > +#include "dpu_rm.h" > > #define DRMID(x) ((x) ? (x)->base.id : -1) > > @@ -139,9 +139,6 @@ struct dpu_kms { > struct drm_atomic_state *suspend_state; > bool suspend_block; > > - struct dpu_rm rm; > - bool rm_init; > - > struct dpu_hw_vbif *hw_vbif[VBIF_MAX]; > struct dpu_hw_mdp *hw_mdp; > > @@ -157,6 +154,9 @@ struct dpu_kms { > > struct dpu_private_state { > struct drm_private_state base; > + > + struct dpu_rm rm; > + bool rm_init; > }; > > struct vsync_info { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 3444469..e4d8fac 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -21,124 +21,59 @@ > #include "dpu_hw_intf.h" > #include "dpu_encoder.h" > #include "dpu_trace.h" > - > -#define RESERVED_BY_OTHER(h, r) \ > - ((h)->rsvp && ((h)->rsvp->enc_id != (r)->enc_id)) > - > -#define RM_RQ_LOCK(r) ((r)->top_ctrl & BIT(DPU_RM_TOPCTL_RESERVE_LOCK)) > -#define RM_RQ_CLEAR(r) ((r)->top_ctrl & BIT(DPU_RM_TOPCTL_RESERVE_CLEAR)) > -#define RM_RQ_DS(r) ((r)->top_ctrl & BIT(DPU_RM_TOPCTL_DS)) > -#define RM_IS_TOPOLOGY_MATCH(t, r) ((t).num_lm == (r).num_lm && \ > - (t).num_comp_enc == (r).num_enc && \ > - (t).num_intf == (r).num_intf) > - > -struct dpu_rm_topology_def { > - enum dpu_rm_topology_name top_name; > - int num_lm; > - int num_comp_enc; > - int num_intf; > - int num_ctl; > - int needs_split_display; > -}; > - > -static const struct dpu_rm_topology_def g_top_table[] = { > - { DPU_RM_TOPOLOGY_NONE, 0, 0, 0, 0, false }, > - { DPU_RM_TOPOLOGY_SINGLEPIPE, 1, 0, 1, 1, false }, > - { DPU_RM_TOPOLOGY_DUALPIPE, 2, 0, 2, 2, true }, > - { DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE, 2, 0, 1, 1, false }, > -}; > - > -/** > - * struct dpu_rm_requirements - Reservation requirements parameter bundle > - * @top_ctrl: topology control preference from kernel client > - * @top: selected topology for the display > - * @hw_res: Hardware resources required as reported by the encoders > - */ > -struct dpu_rm_requirements { > - uint64_t top_ctrl; > - const struct dpu_rm_topology_def *topology; > - struct dpu_encoder_hw_resources hw_res; > -}; > - > -/** > - * struct dpu_rm_rsvp - Use Case Reservation tagging structure > - * Used to tag HW blocks as reserved by a CRTC->Encoder->Connector chain > - * By using as a tag, rather than lists of pointers to HW blocks used > - * we can avoid some list management since we don't know how many blocks > - * of each type a given use case may require. > - * @list: List head for list of all reservations > - * @seq: Global RSVP sequence number for debugging, especially for > - * differentiating differenct allocations for same encoder. > - * @enc_id: Reservations are tracked by Encoder DRM object ID. > - * CRTCs may be connected to multiple Encoders. > - * An encoder or connector id identifies the display path. > - * @topology DRM<->HW topology use case > - */ > -struct dpu_rm_rsvp { > - struct list_head list; > - uint32_t seq; > - uint32_t enc_id; > - enum dpu_rm_topology_name topology; > -}; > +#include "dpu_rm.h" > > /** > * struct dpu_rm_hw_blk - hardware block tracking list member > - * @list: List head for list of all hardware blocks tracking items > - * @rsvp: Pointer to use case reservation if reserved by a client > - * @rsvp_nxt: Temporary pointer used during reservation to the incoming > - * request. Will be swapped into rsvp if proposal is accepted > * @type: Type of hardware block this structure tracks > + * @drm_id: DRM component ID associated with the HW block drm_id doesn't exist > * @id: Hardware ID number, within it's own space, ie. LM_X > - * @catalog: Pointer to the hardware catalog entry for this block > * @hw: Pointer to the hardware register access object for this block > */ > struct dpu_rm_hw_blk { > - struct list_head list; > - struct dpu_rm_rsvp *rsvp; > - struct dpu_rm_rsvp *rsvp_nxt; > enum dpu_hw_blk_type type; > uint32_t id; > + uint32_t rm_id; This is not documented. > struct dpu_hw_blk *hw; > }; > > -/** > - * dpu_rm_dbg_rsvp_stage - enum of steps in making reservation for event logging > - */ > -enum dpu_rm_dbg_rsvp_stage { > - DPU_RM_STAGE_BEGIN, > - DPU_RM_STAGE_AFTER_CLEAR, > - DPU_RM_STAGE_AFTER_RSVPNEXT, > - DPU_RM_STAGE_FINAL > +static char *blk_name[DPU_HW_BLK_MAX] = { > + "HW_TOP", > + "HW_SSPP", > + "HW_LM", > + "HW_CTL", > + "HW_CDM", > + "HW_PINGPONG", > + "HW_INTF", > + "HW_WB" > }; This is fragile. Just print the index below and remove this. > > -static void _dpu_rm_print_rsvps( > - struct dpu_rm *rm, > - enum dpu_rm_dbg_rsvp_stage stage) > +void dpu_rm_print_state(struct dpu_rm *rm) > { > - struct dpu_rm_rsvp *rsvp; > - struct dpu_rm_hw_blk *blk; > - enum dpu_hw_blk_type type; > + int i; > + > + mutex_lock(&rm->rm_lock); > > - DPU_DEBUG("%d\n", stage); > + DPU_ERROR("DPU RM state:\n"); > + for (i = 0; i < DPU_HW_BLK_MAX; i++) { > + int blk_len = rm->hw_blks_len[i]; > + int *drm_map = rm->hw_drm_map[i]; > + int j; > > - list_for_each_entry(rsvp, &rm->rsvps, list) { > - DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage, rsvp->seq, > - rsvp->enc_id, rsvp->topology); > - } > + if (!blk_len) > + continue; > > - for (type = 0; type < DPU_HW_BLK_MAX; type++) { > - list_for_each_entry(blk, &rm->hw_blks[type], list) { > - if (!blk->rsvp && !blk->rsvp_nxt) > + DPU_ERROR("%s\ttotal: %d\n", blk_name[i], blk_len); > + > + for (j = 0; j < blk_len; j++) { > + if (!drm_map[j]) > continue; > > - DRM_DEBUG_KMS("%d rsvp[s%ue%u->s%ue%u] %d %d\n", stage, > - (blk->rsvp) ? blk->rsvp->seq : 0, > - (blk->rsvp) ? blk->rsvp->enc_id : 0, > - (blk->rsvp_nxt) ? blk->rsvp_nxt->seq : 0, > - (blk->rsvp_nxt) ? blk->rsvp_nxt->enc_id : 0, > - blk->type, blk->id); > + DPU_ERROR("\tidx:%d drm_id: %d\n", j, drm_map[j]); > } > } > + > + mutex_unlock(&rm->rm_lock); > } > > struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm) > @@ -148,17 +83,19 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm) > > void dpu_rm_init_hw_iter( > struct dpu_rm_hw_iter *iter, > - uint32_t enc_id, > + uint32_t drm_id, > enum dpu_hw_blk_type type) > { > memset(iter, 0, sizeof(*iter)); > - iter->enc_id = enc_id; > + iter->drm_id = drm_id; I'm not thrilled about the drm_id tracking, tbh. Everywhere but release, you're setting it to 0 (which is looking for unassigned hw blks). So the better way to do this IMO is to maintain a list of lm and pp in the crtc state for the crtc that's using it. Then in release you just iterate that list. On the rm side, you have a bool tracking whether or not a blk is assigned and skip it when looking for unassigned blocks. This also allows you to get rid of get_crtc_id/get_crtc_index function, which kind of irked me. > iter->type = type; > } > > static bool _dpu_rm_get_hw_locked(struct dpu_rm *rm, struct dpu_rm_hw_iter *i) > { > - struct list_head *blk_list; > + struct dpu_rm_hw_blk **blk_list; > + uint32_t blk_len, index; > + uint32_t *drm_map; > > if (!rm || !i || i->type >= DPU_HW_BLK_MAX) { > DPU_ERROR("invalid rm\n"); > @@ -166,33 +103,30 @@ static bool _dpu_rm_get_hw_locked(struct dpu_rm *rm, struct dpu_rm_hw_iter *i) > } > > i->hw = NULL; > - blk_list = &rm->hw_blks[i->type]; > - > - if (i->blk && (&i->blk->list == blk_list)) { > - DPU_DEBUG("attempt resume iteration past last\n"); > - return false; > - } > - > - i->blk = list_prepare_entry(i->blk, blk_list, list); > + blk_list = rm->hw_blks[i->type]; > + blk_len = rm->hw_blks_len[i->type]; > + drm_map = rm->hw_drm_map[i->type]; I think this is way more complicated than it needs to be. I don't see the benefit in lumping everything into one list. All of the direct callsites of this function know what type of hardware they need, so it's not like the abstraction is useful. I think it'd be much clearer if each hw type had its own pool in the rm, that way you could get rid of hw_blks_len, and dpu_hw_blk_type. With the removal of drm_id/hw_drm_map and rm_lock, that significantly reduces the complexity of dpu_rm. You can also get rid of dpu_rm_hw_iter, and probably some other stuff. > > - list_for_each_entry_continue(i->blk, blk_list, list) { > - struct dpu_rm_rsvp *rsvp = i->blk->rsvp; > + for (index = i->index; index < blk_len; index++) { > + struct dpu_rm_hw_blk *blk = blk_list[index]; > > - if (i->blk->type != i->type) { > - DPU_ERROR("found incorrect block type %d on %d list\n", > - i->blk->type, i->type); > + if (!blk) { > + DPU_ERROR("invalid block. index: %d type: %d\n", > + index, i->type); > return false; > } > > - if ((i->enc_id == 0) || (rsvp && rsvp->enc_id == i->enc_id)) { > - i->hw = i->blk->hw; > + if (drm_map[index] == i->drm_id) { > + i->hw = blk->hw; > + i->index = index + 1; > + i->blk = blk; > DPU_DEBUG("found type %d id %d for enc %d\n", > - i->type, i->blk->id, i->enc_id); > + i->type, blk->id, i->drm_id); > return true; > } > } > > - DPU_DEBUG("no match, type %d for enc %d\n", i->type, i->enc_id); > + DPU_DEBUG("no match, type %d for enc %d\n", i->type, i->drm_id); > > return false; > } > @@ -217,9 +151,6 @@ static void _dpu_rm_hw_destroy(enum dpu_hw_blk_type type, void *hw) > case DPU_HW_BLK_CTL: > dpu_hw_ctl_destroy(hw); > break; > - case DPU_HW_BLK_CDM: > - dpu_hw_cdm_destroy(hw); > - break; You've just slipped in the removal of CDM into this patch. Please separate into a patch preceding this one. > case DPU_HW_BLK_PINGPONG: > dpu_hw_pingpong_destroy(hw); > break; > @@ -239,26 +170,25 @@ static void _dpu_rm_hw_destroy(enum dpu_hw_blk_type type, void *hw) > > int dpu_rm_destroy(struct dpu_rm *rm) > { > - > - struct dpu_rm_rsvp *rsvp_cur, *rsvp_nxt; > - struct dpu_rm_hw_blk *hw_cur, *hw_nxt; > enum dpu_hw_blk_type type; > + uint32_t i; > > if (!rm) { > DPU_ERROR("invalid rm\n"); > return -EINVAL; > } > > - list_for_each_entry_safe(rsvp_cur, rsvp_nxt, &rm->rsvps, list) { > - list_del(&rsvp_cur->list); > - kfree(rsvp_cur); > - } > + for (type = 0; type < DPU_HW_BLK_MAX; type++) { > + uint32_t hw_blk_len = rm->hw_blks_len[type]; > > + for (i = 0; i < hw_blk_len; i++) { > + struct dpu_rm_hw_blk *hw_cur = rm->hw_blks[type][i]; > > - for (type = 0; type < DPU_HW_BLK_MAX; type++) { > - list_for_each_entry_safe(hw_cur, hw_nxt, &rm->hw_blks[type], > - list) { > - list_del(&hw_cur->list); > + if (!hw_cur) { > + DPU_ERROR("Invalid hw block. type:%d i: %d\n", > + type, i); > + return -EINVAL; > + } > _dpu_rm_hw_destroy(hw_cur->type, hw_cur->hw); > kfree(hw_cur); > } > @@ -293,9 +223,6 @@ static int _dpu_rm_hw_blk_create( > case DPU_HW_BLK_CTL: > hw = dpu_hw_ctl_init(id, mmio, cat); > break; > - case DPU_HW_BLK_CDM: > - hw = dpu_hw_cdm_init(id, mmio, cat, hw_mdp); > - break; > case DPU_HW_BLK_PINGPONG: > hw = dpu_hw_pingpong_init(id, mmio, cat); > break; > @@ -327,35 +254,24 @@ static int _dpu_rm_hw_blk_create( > blk->type = type; > blk->id = id; > blk->hw = hw; > - list_add_tail(&blk->list, &rm->hw_blks[type]); > + blk->rm_id = rm->hw_blks_len[type]; > + > + rm->hw_blks[type][rm->hw_blks_len[type]++] = blk; > > return 0; > } > > int dpu_rm_init(struct dpu_rm *rm, > struct dpu_mdss_cfg *cat, > - void __iomem *mmio, > - struct drm_device *dev) > + void __iomem *mmio) > { > int rc, i; > - enum dpu_hw_blk_type type; > - > - if (!rm || !cat || !mmio || !dev) { > - DPU_ERROR("invalid kms\n"); > - return -EINVAL; > - } > > /* Clear, setup lists */ > memset(rm, 0, sizeof(*rm)); > > mutex_init(&rm->rm_lock); Do you actually need this lock? It seems like you shouldn't, and if you do, it's likely there's some improper modeset locking going on. > > - INIT_LIST_HEAD(&rm->rsvps); > - for (type = 0; type < DPU_HW_BLK_MAX; type++) > - INIT_LIST_HEAD(&rm->hw_blks[type]); > - > - rm->dev = dev; > - > /* Some of the sub-blocks require an mdptop to be created */ > rm->hw_mdp = dpu_hw_mdptop_init(MDP_TOP, mmio, cat); > if (IS_ERR_OR_NULL(rm->hw_mdp)) { > @@ -380,18 +296,6 @@ int dpu_rm_init(struct dpu_rm *rm, > DPU_ERROR("failed: lm hw not available\n"); > goto fail; > } > - > - if (!rm->lm_max_width) { > - rm->lm_max_width = lm->sblk->maxwidth; > - } else if (rm->lm_max_width != lm->sblk->maxwidth) { > - /* > - * Don't expect to have hw where lm max widths differ. > - * If found, take the min. > - */ > - DPU_ERROR("unsupported: lm maxwidth differs\n"); > - if (rm->lm_max_width > lm->sblk->maxwidth) > - rm->lm_max_width = lm->sblk->maxwidth; > - } > } > > for (i = 0; i < cat->pingpong_count; i++) { > @@ -426,15 +330,6 @@ int dpu_rm_init(struct dpu_rm *rm, > } > } > > - for (i = 0; i < cat->cdm_count; i++) { > - rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_CDM, > - cat->cdm[i].id, &cat->cdm[i]); > - if (rc) { > - DPU_ERROR("failed: cdm hw not available\n"); > - goto fail; > - } > - } > - > return 0; > > fail: > @@ -448,8 +343,7 @@ int dpu_rm_init(struct dpu_rm *rm, > * proposed use case requirements, incl. hardwired dependent blocks like > * pingpong > * @rm: dpu resource manager handle > - * @rsvp: reservation currently being created > - * @reqs: proposed use case requirements > + * @state: dpu crtc state handle > * @lm: proposed layer mixer, function checks if lm, and all other hardwired > * blocks connected to the lm (pp) is available and appropriate > * @pp: output parameter, pingpong block attached to the layer mixer. > @@ -460,8 +354,7 @@ int dpu_rm_init(struct dpu_rm *rm, > */ > static bool _dpu_rm_check_lm_and_get_connected_blks( > struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - struct dpu_rm_requirements *reqs, > + struct dpu_crtc_state *state, > struct dpu_rm_hw_blk *lm, > struct dpu_rm_hw_blk **pp, > struct dpu_rm_hw_blk *primary_lm) > @@ -486,12 +379,6 @@ static bool _dpu_rm_check_lm_and_get_connected_blks( > } > } > > - /* Already reserved? */ > - if (RESERVED_BY_OTHER(lm, rsvp)) { > - DPU_DEBUG("lm %d already reserved\n", lm_cfg->id); > - return false; > - } > - > dpu_rm_init_hw_iter(&iter, 0, DPU_HW_BLK_PINGPONG); > while (_dpu_rm_get_hw_locked(rm, &iter)) { > if (iter.blk->id == lm_cfg->pingpong) { > @@ -505,35 +392,24 @@ static bool _dpu_rm_check_lm_and_get_connected_blks( > return false; > } > > - if (RESERVED_BY_OTHER(*pp, rsvp)) { > - DPU_DEBUG("lm %d pp %d already reserved\n", lm->id, > - (*pp)->id); > - return false; > - } > - > return true; > } > > static int _dpu_rm_reserve_lms( > - struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - struct dpu_rm_requirements *reqs) > - > + struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_crtc_topology *topology) > { > struct dpu_rm_hw_blk *lm[MAX_BLOCKS]; > struct dpu_rm_hw_blk *pp[MAX_BLOCKS]; > struct dpu_rm_hw_iter iter_i, iter_j; > int lm_count = 0; > - int i, rc = 0; > - > - if (!reqs->topology->num_lm) { > - DPU_ERROR("invalid number of lm: %d\n", reqs->topology->num_lm); > - return -EINVAL; > - } > + int i, drm_id = get_crtc_id(state); > + int *lm_drm_map = rm->hw_drm_map[DPU_HW_BLK_LM]; > + int *pp_drm_map = rm->hw_drm_map[DPU_HW_BLK_PINGPONG]; > > /* Find a primary mixer */ > dpu_rm_init_hw_iter(&iter_i, 0, DPU_HW_BLK_LM); > - while (lm_count != reqs->topology->num_lm && > + while (lm_count != topology->num_lms && > _dpu_rm_get_hw_locked(rm, &iter_i)) { It took me a while to figure how how these nested loops worked. Hopefully splitting out the hw blocks in rm will make things more clear, but we might want to revisit this code. > memset(&lm, 0, sizeof(lm)); > memset(&pp, 0, sizeof(pp)); > @@ -542,8 +418,7 @@ static int _dpu_rm_reserve_lms( > lm[lm_count] = iter_i.blk; > > if (!_dpu_rm_check_lm_and_get_connected_blks( > - rm, rsvp, reqs, lm[lm_count], > - &pp[lm_count], NULL)) > + rm, state, lm[lm_count], &pp[lm_count], NULL)) > continue; > > ++lm_count; > @@ -551,14 +426,14 @@ static int _dpu_rm_reserve_lms( > /* Valid primary mixer found, find matching peers */ > dpu_rm_init_hw_iter(&iter_j, 0, DPU_HW_BLK_LM); > > - while (lm_count != reqs->topology->num_lm && > + while (lm_count != topology->num_lms && > _dpu_rm_get_hw_locked(rm, &iter_j)) { > if (iter_i.blk == iter_j.blk) > continue; > > if (!_dpu_rm_check_lm_and_get_connected_blks( > - rm, rsvp, reqs, iter_j.blk, > - &pp[lm_count], iter_i.blk)) > + rm, state, iter_j.blk, > + &pp[lm_count], iter_i.blk)) > continue; > > lm[lm_count] = iter_j.blk; > @@ -566,7 +441,7 @@ static int _dpu_rm_reserve_lms( > } > } > > - if (lm_count != reqs->topology->num_lm) { > + if (lm_count != topology->num_lms) { > DPU_DEBUG("unable to find appropriate mixers\n"); > return -ENAVAIL; > } > @@ -575,492 +450,264 @@ static int _dpu_rm_reserve_lms( > if (!lm[i]) > break; > > - lm[i]->rsvp_nxt = rsvp; > - pp[i]->rsvp_nxt = rsvp; > + lm_drm_map[lm[i]->rm_id] = drm_id; > + pp_drm_map[pp[i]->rm_id] = drm_id; > > - trace_dpu_rm_reserve_lms(lm[i]->id, lm[i]->type, rsvp->enc_id, > - pp[i]->id); You should either fix and reinstate this trace, or remove it from dpu_trace.h. Please don't leave dangling pointers around. > + state->mixers[i].hw_lm = (struct dpu_hw_mixer *)lm[i]->hw; > + state->mixers[i].hw_pp = (struct dpu_hw_pingpong *)pp[i]->hw; > } > > - return rc; > + state->num_mixers = topology->num_lms; > + > + return 0; > } > > static int _dpu_rm_reserve_ctls( > - struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - const struct dpu_rm_topology_def *top) > + struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_crtc_topology *topology) > { > struct dpu_rm_hw_blk *ctls[MAX_BLOCKS]; > struct dpu_rm_hw_iter iter; > - int i = 0; > + bool needs_split_display; > + int i = 0, drm_id = get_crtc_id(state); > + int *ctl_drm_map = rm->hw_drm_map[DPU_HW_BLK_CTL]; > > memset(&ctls, 0, sizeof(ctls)); You can remove this if you change the bottom loop to: for (i = 0; i < top->num_ctl; i++) { Bonus: ^^ looks nicer too! > > + needs_split_display = (topology->num_ctls == 2); > + > dpu_rm_init_hw_iter(&iter, 0, DPU_HW_BLK_CTL); > while (_dpu_rm_get_hw_locked(rm, &iter)) { > const struct dpu_hw_ctl *ctl = to_dpu_hw_ctl(iter.blk->hw); > unsigned long features = ctl->caps->features; > bool has_split_display; > > - if (RESERVED_BY_OTHER(iter.blk, rsvp)) > - continue; > - > has_split_display = BIT(DPU_CTL_SPLIT_DISPLAY) & features; > > DPU_DEBUG("ctl %d caps 0x%lX\n", iter.blk->id, features); > > - if (top->needs_split_display != has_split_display) > + if (needs_split_display != has_split_display) > continue; > > ctls[i] = iter.blk; > DPU_DEBUG("ctl %d match\n", iter.blk->id); > > - if (++i == top->num_ctl) > + if (++i == topology->num_ctls) > break; > } > > - if (i != top->num_ctl) > - return -ENAVAIL; > - > - for (i = 0; i < ARRAY_SIZE(ctls) && i < top->num_ctl; i++) { > - ctls[i]->rsvp_nxt = rsvp; > - trace_dpu_rm_reserve_ctls(ctls[i]->id, ctls[i]->type, > - rsvp->enc_id); > - } > - > - return 0; > -} > - > -static int _dpu_rm_reserve_cdm( > - struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - uint32_t id, > - enum dpu_hw_blk_type type) Should be done in a separate patch. > -{ > - struct dpu_rm_hw_iter iter; > - > - DRM_DEBUG_KMS("type %d id %d\n", type, id); > - > - dpu_rm_init_hw_iter(&iter, 0, DPU_HW_BLK_CDM); > - while (_dpu_rm_get_hw_locked(rm, &iter)) { > - const struct dpu_hw_cdm *cdm = to_dpu_hw_cdm(iter.blk->hw); > - const struct dpu_cdm_cfg *caps = cdm->caps; > - bool match = false; > - > - if (RESERVED_BY_OTHER(iter.blk, rsvp)) > - continue; > - > - if (type == DPU_HW_BLK_INTF && id != INTF_MAX) > - match = test_bit(id, &caps->intf_connect); > - > - DRM_DEBUG_KMS("iter: type:%d id:%d enc:%d cdm:%lu match:%d\n", > - iter.blk->type, iter.blk->id, rsvp->enc_id, > - caps->intf_connect, match); > + if (i != topology->num_ctls) > + return -EINVAL; > > - if (!match) > - continue; > + for (i = 0; i < ARRAY_SIZE(ctls) && i < topology->num_ctls; i++) { > + ctl_drm_map[ctls[i]->rm_id] = drm_id; > + state->hw_ctls[i] = (struct dpu_hw_ctl *)ctls[i]->hw; > > - trace_dpu_rm_reserve_cdm(iter.blk->id, iter.blk->type, > - rsvp->enc_id); > - iter.blk->rsvp_nxt = rsvp; > - break; > + trace_dpu_rm_reserve_ctls(ctls[i]->type, > + get_crtc_id(state), ctls[i]->id); > } > > - if (!iter.hw) { > - DPU_ERROR("couldn't reserve cdm for type %d id %d\n", type, id); > - return -ENAVAIL; > - } > + state->num_ctls = topology->num_ctls; > > return 0; > } > > -static int _dpu_rm_reserve_intf( > +static struct dpu_rm_hw_blk *_dpu_rm_get_hw_id( > struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > uint32_t id, > - enum dpu_hw_blk_type type, > - bool needs_cdm) > + enum dpu_hw_blk_type type) > { > struct dpu_rm_hw_iter iter; > - int ret = 0; > > /* Find the block entry in the rm, and note the reservation */ > dpu_rm_init_hw_iter(&iter, 0, type); > while (_dpu_rm_get_hw_locked(rm, &iter)) { > if (iter.blk->id != id) > continue; > - > - if (RESERVED_BY_OTHER(iter.blk, rsvp)) { > - DPU_ERROR("type %d id %d already reserved\n", type, id); > - return -ENAVAIL; > - } > - > - iter.blk->rsvp_nxt = rsvp; > - trace_dpu_rm_reserve_intf(iter.blk->id, iter.blk->type, > - rsvp->enc_id); > break; > } > > - /* Shouldn't happen since intfs are fixed at probe */ > if (!iter.hw) { > DPU_ERROR("couldn't find type %d id %d\n", type, id); > - return -EINVAL; > + return NULL; > } > > - if (needs_cdm) > - ret = _dpu_rm_reserve_cdm(rm, rsvp, id, type); > - > - return ret; > + return iter.blk; > } > > static int _dpu_rm_reserve_intf_related_hw( So again, if you just maintain dedicated lists of reserved hardware in the state, this function becomes a whooole lot easier to read and reason about (and maybe you can remove it entirely?) > struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > + struct dpu_crtc_state *state, > struct dpu_encoder_hw_resources *hw_res) > { > - int i, ret = 0; > - u32 id; > + struct dpu_rm_hw_blk *intf[INTF_MAX] = {}; > + u32 i, id, intf_count = 0; > + u32 drm_id = get_crtc_id(state); > + int *intf_drm_map = rm->hw_drm_map[DPU_HW_BLK_INTF]; > > for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { > if (hw_res->intfs[i] == INTF_MODE_NONE) > continue; > - id = i + INTF_0; > - ret = _dpu_rm_reserve_intf(rm, rsvp, id, > - DPU_HW_BLK_INTF, hw_res->needs_cdm); > - if (ret) > - return ret; > - } > - > - return ret; > -} > > -static int _dpu_rm_make_next_rsvp( > - struct dpu_rm *rm, > - struct drm_encoder *enc, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state, > - struct dpu_rm_rsvp *rsvp, > - struct dpu_rm_requirements *reqs) > -{ > - int ret; > - struct dpu_rm_topology_def topology; > - > - /* Create reservation info, tag reserved blocks with it as we go */ > - rsvp->seq = ++rm->rsvp_next_seq; > - rsvp->enc_id = enc->base.id; > - rsvp->topology = reqs->topology->top_name; > - list_add_tail(&rsvp->list, &rm->rsvps); > - > - ret = _dpu_rm_reserve_lms(rm, rsvp, reqs); > - if (ret) { > - DPU_ERROR("unable to find appropriate mixers\n"); > - return ret; > - } > + id = i + INTF_0; > + intf[i] = _dpu_rm_get_hw_id(rm, id, DPU_HW_BLK_INTF); > + if (!intf[i]) > + return -EINVAL; > > - /* > - * Do assignment preferring to give away low-resource CTLs first: > - * - Check mixers without Split Display > - * - Only then allow to grab from CTLs with split display capability > - */ > - _dpu_rm_reserve_ctls(rm, rsvp, reqs->topology); > - if (ret && !reqs->topology->needs_split_display) { > - memcpy(&topology, reqs->topology, sizeof(topology)); > - topology.needs_split_display = true; > - _dpu_rm_reserve_ctls(rm, rsvp, &topology); > + /* Reserve other INTF related blocks if needed */ > } > - if (ret) { > - DPU_ERROR("unable to find appropriate CTL\n"); > - return ret; > - } > - > - /* Assign INTFs and blks whose usage is tied to them: CTL & CDM */ > - ret = _dpu_rm_reserve_intf_related_hw(rm, rsvp, &reqs->hw_res); > - if (ret) > - return ret; > - > - return ret; > -} > - > -static int _dpu_rm_populate_requirements( > - struct dpu_rm *rm, > - struct drm_encoder *enc, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state, > - struct dpu_rm_requirements *reqs, > - struct msm_display_topology req_topology) > -{ > - int i; > > - memset(reqs, 0, sizeof(*reqs)); > - > - dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state); > - > - for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) { > - if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], > - req_topology)) { > - reqs->topology = &g_top_table[i]; > - break; > - } > - } > + for (i = 0; i < ARRAY_SIZE(hw_res->intfs); i++) { > + if (!intf[i]) > + continue; > > - if (!reqs->topology) { > - DPU_ERROR("invalid topology for the display\n"); > - return -EINVAL; > + intf_drm_map[intf[i]->rm_id] = drm_id; > + state->hw_intfs[intf_count++] = > + (struct dpu_hw_intf *)intf[i]->hw; > } > > - /** > - * Set the requirement based on caps if not set from user space > - * This will ensure to select LM tied with DS blocks > - * Currently, DS blocks are tied with LM 0 and LM 1 (primary display) > - */ > - if (!RM_RQ_DS(reqs) && rm->hw_mdp->caps->has_dest_scaler && > - conn_state->connector->connector_type == DRM_MODE_CONNECTOR_DSI) > - reqs->top_ctrl |= BIT(DPU_RM_TOPCTL_DS); > - > - DPU_DEBUG("top_ctrl: 0x%llX\n", reqs->top_ctrl); > - DPU_DEBUG("num_lm: %d num_ctl: %d topology: %d split_display: %d\n", > - reqs->topology->num_lm, reqs->topology->num_ctl, > - reqs->topology->top_name, > - reqs->topology->needs_split_display); > + state->num_intfs = intf_count; > > return 0; > } > > -static struct dpu_rm_rsvp *_dpu_rm_get_rsvp( > +static int _dpu_rm_release_hw_blk( > struct dpu_rm *rm, > - struct drm_encoder *enc) > + struct dpu_crtc_state *state, > + enum dpu_hw_blk_type type) > { > - struct dpu_rm_rsvp *i; > + struct dpu_rm_hw_iter iter; > + int drm_id = get_crtc_id(state); > + int num_released = 0; > + int *drm_map = rm->hw_drm_map[type]; > > - if (!rm || !enc) { > - DPU_ERROR("invalid params\n"); > - return NULL; > + dpu_rm_init_hw_iter(&iter, drm_id, type); > + while (_dpu_rm_get_hw_locked(rm, &iter)) { > + drm_map[iter.blk->rm_id] = 0; > + num_released++; > } > > - if (list_empty(&rm->rsvps)) > - return NULL; > - > - list_for_each_entry(i, &rm->rsvps, list) > - if (i->enc_id == enc->base.id) > - return i; > - > - return NULL; > + return num_released; > } > > -static struct drm_connector *_dpu_rm_get_connector( > - struct drm_encoder *enc) > +static int _dpu_rm_release_lms(struct dpu_rm *rm, struct dpu_crtc_state *state) > { > - struct drm_connector *conn = NULL; > - struct list_head *connector_list = > - &enc->dev->mode_config.connector_list; > + int num_lm, num_pp; > > - list_for_each_entry(conn, connector_list, head) > - if (conn->encoder == enc) > - return conn; > + /* Release LM blocks */ > + num_lm = _dpu_rm_release_hw_blk(rm, state, DPU_HW_BLK_LM); > + > + /* Rlease ping pong blocks */ > + num_pp = _dpu_rm_release_hw_blk(rm, state, DPU_HW_BLK_PINGPONG); > + if (num_pp != num_lm) { > + DPU_ERROR("lm chain count mismatch lm: %d pp:%d\n", > + num_lm, num_pp); > + return 0; > + } > > - return NULL; > + return num_lm; > } > > -/** > - * _dpu_rm_release_rsvp - release resources and release a reservation > - * @rm: KMS handle > - * @rsvp: RSVP pointer to release and release resources for > - */ > -static void _dpu_rm_release_rsvp( > - struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - struct drm_connector *conn) > +int dpu_rm_reserve_crtc_res(struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_crtc_topology *topology) > { > - struct dpu_rm_rsvp *rsvp_c, *rsvp_n; > - struct dpu_rm_hw_blk *blk; > - enum dpu_hw_blk_type type; > - > - if (!rsvp) > - return; > + int rc = 0; > > - DPU_DEBUG("rel rsvp %d enc %d\n", rsvp->seq, rsvp->enc_id); > + mutex_lock(&rm->rm_lock); > > - list_for_each_entry_safe(rsvp_c, rsvp_n, &rm->rsvps, list) { > - if (rsvp == rsvp_c) { > - list_del(&rsvp_c->list); > - break; > - } > + rc = _dpu_rm_reserve_lms(rm, state, topology); > + if (rc) { > + DPU_ERROR("unable to allocate lm for crtc: %d\n", > + get_crtc_id(state)); > + goto reserve_done; > } > > - for (type = 0; type < DPU_HW_BLK_MAX; type++) { > - list_for_each_entry(blk, &rm->hw_blks[type], list) { > - if (blk->rsvp == rsvp) { > - blk->rsvp = NULL; > - DPU_DEBUG("rel rsvp %d enc %d %d %d\n", > - rsvp->seq, rsvp->enc_id, > - blk->type, blk->id); > - } > - if (blk->rsvp_nxt == rsvp) { > - blk->rsvp_nxt = NULL; > - DPU_DEBUG("rel rsvp_nxt %d enc %d %d %d\n", > - rsvp->seq, rsvp->enc_id, > - blk->type, blk->id); > - } > - } > + rc = _dpu_rm_reserve_ctls(rm, state, topology); > + if (rc) { > + DPU_ERROR("unable to allocate ctl for crtc: %d\n", > + get_crtc_id(state)); > + goto reserve_done; > } > > - kfree(rsvp); > +reserve_done: > + mutex_unlock(&rm->rm_lock); > + > + return 0; > } > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_encoder *enc) > -{ > - struct dpu_rm_rsvp *rsvp; > - struct drm_connector *conn; > > - if (!rm || !enc) { > - DPU_ERROR("invalid params\n"); > - return; > - } > +int dpu_rm_release_crtc_res(struct dpu_rm *rm, struct dpu_crtc_state *state) > +{ > + int rc = 0, num_released; > > mutex_lock(&rm->rm_lock); > > - rsvp = _dpu_rm_get_rsvp(rm, enc); > - if (!rsvp) { > - DPU_ERROR("failed to find rsvp for enc %d\n", enc->base.id); > - goto end; > + num_released = _dpu_rm_release_lms(rm, state); > + if (num_released != state->num_mixers) { > + DPU_ERROR( > + "lm release count doesn't match for crtc: %d (%d != %d)\n", > + get_crtc_id(state), num_released, state->num_mixers); > + rc = -EINVAL; > + goto release_done; > } > > - conn = _dpu_rm_get_connector(enc); > - if (!conn) { > - DPU_ERROR("failed to get connector for enc %d\n", enc->base.id); > - goto end; > + num_released = _dpu_rm_release_hw_blk(rm, state, DPU_HW_BLK_CTL); > + if (num_released != state->num_ctls) { > + DPU_ERROR( > + "lm release count doesn't match for crtc: %d (%d != %d)\n", > + get_crtc_id(state), num_released, state->num_ctls); > + rc = -EINVAL; > + goto release_done; > } > - > - _dpu_rm_release_rsvp(rm, rsvp, conn); > -end: > + release_done: > mutex_unlock(&rm->rm_lock); > -} > - > -static int _dpu_rm_commit_rsvp( > - struct dpu_rm *rm, > - struct dpu_rm_rsvp *rsvp, > - struct drm_connector_state *conn_state) > -{ > - struct dpu_rm_hw_blk *blk; > - enum dpu_hw_blk_type type; > - int ret = 0; > - > - /* Swap next rsvp to be the active */ > - for (type = 0; type < DPU_HW_BLK_MAX; type++) { > - list_for_each_entry(blk, &rm->hw_blks[type], list) { > - if (blk->rsvp_nxt) { > - blk->rsvp = blk->rsvp_nxt; > - blk->rsvp_nxt = NULL; > - } > - } > - } > > - if (!ret) > - DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id, > - rsvp->topology); > + state->num_mixers = 0; > + state->num_ctls = 0; > + memset(&state->mixers, 0, sizeof(state->mixers)); > + memset(&state->hw_ctls, 0, sizeof(state->hw_ctls)); > > - return ret; > + return rc; > } > > -int dpu_rm_reserve( > - struct dpu_rm *rm, > - struct drm_encoder *enc, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state, > - struct msm_display_topology topology, > - bool test_only) > +int dpu_rm_reserve_encoder_res( > + struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_encoder_hw_resources *hw_res) > { > - struct dpu_rm_rsvp *rsvp_cur, *rsvp_nxt; > - struct dpu_rm_requirements reqs; > - int ret; > - > - if (!rm || !enc || !crtc_state || !conn_state) { > - DPU_ERROR("invalid arguments\n"); > - return -EINVAL; > - } > - > - /* Check if this is just a page-flip */ > - if (!drm_atomic_crtc_needs_modeset(crtc_state)) > - return 0; > - > - DRM_DEBUG_KMS("reserving hw for conn %d enc %d crtc %d test_only %d\n", > - conn_state->connector->base.id, enc->base.id, > - crtc_state->crtc->base.id, test_only); > + int rc = 0; > > mutex_lock(&rm->rm_lock); > > - _dpu_rm_print_rsvps(rm, DPU_RM_STAGE_BEGIN); > + rc = _dpu_rm_reserve_intf_related_hw(rm, state, hw_res); > + if (rc) > + DPU_ERROR("unable to allocate intf for crtc: %d\n", > + get_crtc_id(state)); > > - ret = _dpu_rm_populate_requirements(rm, enc, crtc_state, > - conn_state, &reqs, topology); > - if (ret) { > - DPU_ERROR("failed to populate hw requirements\n"); > - goto end; > - } > + mutex_unlock(&rm->rm_lock); > > - /* > - * We only support one active reservation per-hw-block. But to implement > - * transactional semantics for test-only, and for allowing failure while > - * modifying your existing reservation, over the course of this > - * function we can have two reservations: > - * Current: Existing reservation > - * Next: Proposed reservation. The proposed reservation may fail, or may > - * be discarded if in test-only mode. > - * If reservation is successful, and we're not in test-only, then we > - * replace the current with the next. > - */ > - rsvp_nxt = kzalloc(sizeof(*rsvp_nxt), GFP_KERNEL); > - if (!rsvp_nxt) { > - ret = -ENOMEM; > - goto end; > - } > + return rc; > +} > > - rsvp_cur = _dpu_rm_get_rsvp(rm, enc); > - > - /* > - * User can request that we clear out any reservation during the > - * atomic_check phase by using this CLEAR bit > - */ > - if (rsvp_cur && test_only && RM_RQ_CLEAR(&reqs)) { > - DPU_DEBUG("test_only & CLEAR: clear rsvp[s%de%d]\n", > - rsvp_cur->seq, rsvp_cur->enc_id); > - _dpu_rm_release_rsvp(rm, rsvp_cur, conn_state->connector); > - rsvp_cur = NULL; > - _dpu_rm_print_rsvps(rm, DPU_RM_STAGE_AFTER_CLEAR); > - } > +int dpu_rm_release_encoder_res(struct dpu_rm *rm, struct dpu_crtc_state *state) > +{ > + int num_released; > + int rc = 0; > > - /* Check the proposed reservation, store it in hw's "next" field */ > - ret = _dpu_rm_make_next_rsvp(rm, enc, crtc_state, conn_state, > - rsvp_nxt, &reqs); > - > - _dpu_rm_print_rsvps(rm, DPU_RM_STAGE_AFTER_RSVPNEXT); > - > - if (ret) { > - DPU_ERROR("failed to reserve hw resources: %d\n", ret); > - _dpu_rm_release_rsvp(rm, rsvp_nxt, conn_state->connector); > - } else if (test_only && !RM_RQ_LOCK(&reqs)) { > - /* > - * Normally, if test_only, test the reservation and then undo > - * However, if the user requests LOCK, then keep the reservation > - * made during the atomic_check phase. > - */ > - DPU_DEBUG("test_only: discard test rsvp[s%de%d]\n", > - rsvp_nxt->seq, rsvp_nxt->enc_id); > - _dpu_rm_release_rsvp(rm, rsvp_nxt, conn_state->connector); > - } else { > - if (test_only && RM_RQ_LOCK(&reqs)) > - DPU_DEBUG("test_only & LOCK: lock rsvp[s%de%d]\n", > - rsvp_nxt->seq, rsvp_nxt->enc_id); > - > - _dpu_rm_release_rsvp(rm, rsvp_cur, conn_state->connector); > - > - ret = _dpu_rm_commit_rsvp(rm, rsvp_nxt, conn_state); > - } > + mutex_lock(&rm->rm_lock); > > - _dpu_rm_print_rsvps(rm, DPU_RM_STAGE_FINAL); > + num_released = _dpu_rm_release_hw_blk(rm, state, DPU_HW_BLK_INTF); > + if (num_released != state->num_intfs) { > + DPU_ERROR( > + "intf release count doesn't match for crtc: %d (%d != %d)\n", > + get_crtc_id(state), num_released, state->num_intfs); > + rc = -EINVAL; > + } > > -end: > mutex_unlock(&rm->rm_lock); > > - return ret; > + state->num_intfs = 0; > + memset(&state->hw_intfs, 0, sizeof(state->hw_intfs)); > + > + return rc; > } > + > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index de52c03..3e1fc7b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -16,61 +16,28 @@ > #define __DPU_RM_H__ > > #include <linux/list.h> > - > #include "msm_kms.h" > #include "dpu_hw_top.h" > +#include "dpu_encoder.h" > > -/** > - * enum dpu_rm_topology_name - HW resource use case in use by connector > - * @DPU_RM_TOPOLOGY_NONE: No topology in use currently > - * @DPU_RM_TOPOLOGY_SINGLEPIPE: 1 LM, 1 PP, 1 INTF/WB > - * @DPU_RM_TOPOLOGY_DUALPIPE: 2 LM, 2 PP, 2 INTF/WB > - * @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE: 2 LM, 2 PP, 3DMux, 1 INTF/WB > - */ > -enum dpu_rm_topology_name { > - DPU_RM_TOPOLOGY_NONE = 0, > - DPU_RM_TOPOLOGY_SINGLEPIPE, > - DPU_RM_TOPOLOGY_DUALPIPE, > - DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE, > - DPU_RM_TOPOLOGY_MAX, > -}; > - > -/** > - * enum dpu_rm_topology_control - HW resource use case in use by connector > - * @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a successful > - * test, reserve the resources for this display. > - * Normal behavior would not impact the reservation > - * list during the AtomicTest phase. > - * @DPU_RM_TOPCTL_RESERVE_CLEAR: If set, in AtomicTest phase, before testing, > - * release any reservation held by this display. > - * Normal behavior would not impact the > - * reservation list during the AtomicTest phase. > - * @DPU_RM_TOPCTL_DS : Require layer mixers with DS capabilities > - */ > -enum dpu_rm_topology_control { > - DPU_RM_TOPCTL_RESERVE_LOCK, > - DPU_RM_TOPCTL_RESERVE_CLEAR, > - DPU_RM_TOPCTL_DS, > -}; > +struct dpu_crtc_state; > +struct dpu_crtc_topology; > > /** > * struct dpu_rm - DPU dynamic hardware resource manager > - * @dev: device handle for event logging purposes > - * @rsvps: list of hardware reservations by each crtc->encoder->connector > + * @hw_blks_len - number of hw blocks per type > * @hw_blks: array of lists of hardware resources present in the system, one > * list per type of hardware block > + * @hw_drm_map - array to track each hw block type assignments > * @hw_mdp: hardware object for mdp_top > * @lm_max_width: cached layer mixer maximum width > - * @rsvp_next_seq: sequence number for next reservation for debugging purposes > * @rm_lock: resource manager mutex > */ > struct dpu_rm { > - struct drm_device *dev; > - struct list_head rsvps; > - struct list_head hw_blks[DPU_HW_BLK_MAX]; > + uint32_t hw_blks_len[DPU_HW_BLK_MAX]; > + struct dpu_rm_hw_blk *hw_blks[DPU_HW_BLK_MAX][MAX_BLOCKS]; > + int hw_drm_map[DPU_HW_BLK_MAX][MAX_BLOCKS]; > struct dpu_hw_mdp *hw_mdp; > - uint32_t lm_max_width; > - uint32_t rsvp_next_seq; > struct mutex rm_lock; > }; > > @@ -90,23 +57,28 @@ struct dpu_rm { > struct dpu_rm_hw_iter { > void *hw; > struct dpu_rm_hw_blk *blk; > - uint32_t enc_id; > + uint32_t drm_id; > enum dpu_hw_blk_type type; > + uint32_t index; > }; > > /** > + * dpu_rm_print_state - prints current RM state on resource pool > + * @rm: DPU Resource Manager handle > + */ > +void dpu_rm_print_state(struct dpu_rm *rm); > + There are no callers for this function, please remove. > +/** > * dpu_rm_init - Read hardware catalog and create reservation tracking objects > * for all HW blocks. > * @rm: DPU Resource Manager handle > * @cat: Pointer to hardware catalog > * @mmio: mapped register io address of MDP > - * @dev: device handle for event logging purposes > * @Return: 0 on Success otherwise -ERROR > */ > int dpu_rm_init(struct dpu_rm *rm, > struct dpu_mdss_cfg *cat, > - void __iomem *mmio, > - struct drm_device *dev); > + void *mmio); > > /** > * dpu_rm_destroy - Free all memory allocated by dpu_rm_init > @@ -116,75 +88,47 @@ int dpu_rm_init(struct dpu_rm *rm, > int dpu_rm_destroy(struct dpu_rm *rm); > > /** > - * dpu_rm_reserve - Given a CRTC->Encoder->Connector display chain, analyze > - * the use connections and user requirements, specified through related > - * topology control properties, and reserve hardware blocks to that > - * display chain. > - * HW blocks can then be accessed through dpu_rm_get_* functions. > - * HW Reservations should be released via dpu_rm_release_hw. > + * dpu_rm_reserve_crtc_res - Reserve HW blocks for CRTC > * @rm: DPU Resource Manager handle > - * @drm_enc: DRM Encoder handle > - * @crtc_state: Proposed Atomic DRM CRTC State handle > - * @conn_state: Proposed Atomic DRM Connector State handle > - * @topology: Pointer to topology info for the display > - * @test_only: Atomic-Test phase, discard results (unless property overrides) > - * @Return: 0 on Success otherwise -ERROR > + * @state: DPU CRTC state to cache HW block handles > + * @topology: topology requirement for the display > + * @Return: 0 on Success otherwise - ERROR > */ > -int dpu_rm_reserve(struct dpu_rm *rm, > - struct drm_encoder *drm_enc, > - struct drm_crtc_state *crtc_state, > - struct drm_connector_state *conn_state, > - struct msm_display_topology topology, > - bool test_only); > +int dpu_rm_reserve_crtc_res(struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_crtc_topology *topology); > > /** > - * dpu_rm_reserve - Given the encoder for the display chain, release any > - * HW blocks previously reserved for that use case. > + * dpu_rm_release_crtc_res - Release HW blocks of the CRTC > * @rm: DPU Resource Manager handle > - * @enc: DRM Encoder handle > - * @Return: 0 on Success otherwise -ERROR > + * @state: DPU CRTC state to cache HW block handles > + * @Return: 0 on Success otherwise - ERROR > */ > -void dpu_rm_release(struct dpu_rm *rm, struct drm_encoder *enc); > +int dpu_rm_release_crtc_res(struct dpu_rm *rm, struct dpu_crtc_state *state); > > /** > - * dpu_rm_get_mdp - Retrieve HW block for MDP TOP. > - * This is never reserved, and is usable by any display. > + * dpu_rm_reserve_encoder_res - Reserve HW blocks for Encoder/Connector > * @rm: DPU Resource Manager handle > - * @Return: Pointer to hw block or NULL > + * @state: DPU CRTC state to cache HW block handles > + * @hw_res: interface block related info > + * @Return: 0 on Success otherwise - ERROR > */ > -struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm); > +int dpu_rm_reserve_encoder_res(struct dpu_rm *rm, struct dpu_crtc_state *state, > + struct dpu_encoder_hw_resources *hw_res); > > /** > - * dpu_rm_init_hw_iter - setup given iterator for new iteration over hw list > - * using dpu_rm_get_hw > - * @iter: iter object to initialize > - * @enc_id: DRM ID of Encoder client wishes to search for, or 0 for Any Encoder > - * @type: Hardware Block Type client wishes to search for. > - */ > -void dpu_rm_init_hw_iter( > - struct dpu_rm_hw_iter *iter, > - uint32_t enc_id, > - enum dpu_hw_blk_type type); > -/** > - * dpu_rm_get_hw - retrieve reserved hw object given encoder and hw type > - * Meant to do a single pass through the hardware list to iteratively > - * retrieve hardware blocks of a given type for a given encoder. > - * Initialize an iterator object. > - * Set hw block type of interest. Set encoder id of interest, 0 for any. > - * Function returns first hw of type for that encoder. > - * Subsequent calls will return the next reserved hw of that type in-order. > - * Iterator HW pointer will be null on failure to find hw. > + * dpu_rm_release_encoder_res - Release HW blocks of the Encoder/Connector > * @rm: DPU Resource Manager handle > - * @iter: iterator object > - * @Return: true on match found, false on no match found > + * @state: DPU CRTC state to cache HW block handles > + * @Return: 0 on Success otherwise - ERROR > */ > -bool dpu_rm_get_hw(struct dpu_rm *rm, struct dpu_rm_hw_iter *iter); You should also remove the definition. > +int dpu_rm_release_encoder_res(struct dpu_rm *rm, struct dpu_crtc_state *state); > > /** > - * dpu_rm_check_property_topctl - validate property bitmask before it is set > - * @val: user's proposed topology control bitmask > - * @Return: 0 on success or error > + * dpu_rm_get_mdp - Retrieve HW block for MDP TOP. > + * This is never reserved, and is usable by any display. > + * @rm: DPU Resource Manager handle > + * @Return: Pointer to hw block or NULL > */ > -int dpu_rm_check_property_topctl(uint64_t val); > +struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm); This function isn't useful, it should be removed in a separate patch. > > #endif /* __DPU_RM_H__ */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > -- Sean Paul, Software Engineer, Google / Chromium OS
