| Message ID | 1539059262-8326-23-git-send-email-jsanka@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | reserve RM resources in CRTC state | expand |
On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote: > Instead of letting encoder make a centralized reservation for > all of its display DRM components, this path splits the > responsibility between CRTC and Encoder, each requesting > RM for the HW mapping of its own domain. > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 +++++++++++++ > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++---- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 ++++++++++++++++++++++++----- > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++++++++++---- > 4 files changed, 119 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index 0625f56..0536b8a 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -47,6 +47,8 @@ > #define LEFT_MIXER 0 > #define RIGHT_MIXER 1 > > +#define MAX_VDISPLAY_SPLIT 1080 > + > static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate, > struct drm_display_mode *mode) > { > @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, > > for (i = 0; i < cstate->num_mixers; i++) { > struct drm_rect *r = &cstate->lm_bounds[i]; > + > r->x1 = crtc_split_width * i; > r->y1 = 0; > r->x2 = r->x1 + crtc_split_width; > @@ -885,6 +888,7 @@ 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_kms *dpu_kms; > unsigned long flags; > > if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { > @@ -895,6 +899,7 @@ 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); > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) > crtc->state->event = NULL; > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > } > + > + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); > } > > static void dpu_crtc_enable(struct drm_crtc *crtc, > @@ -1004,6 +1011,21 @@ struct plane_state { > u32 pipe_id; > }; > > +static void _dpu_crtc_get_topology( > + struct drm_crtc_state *crtc_state, > + struct drm_display_mode *mode) > +{ > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > + > + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; > + > + /** > + * encoder->atomic_check is invoked before crtc->atomic_check. > + * so dpu_cstate->num_intfs should have a non-zero value. > + */ > + dpu_cstate->num_ctls = dpu_cstate->num_intfs; Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs directly? Also, you don't really need these in their own function, especially if num_ctls goes away. > +} > + > static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > struct drm_crtc_state *state) > { > @@ -1014,6 +1036,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 msm_drm_private *priv; > + struct dpu_kms *dpu_kms; > > int cnt = 0, rc = 0, mixer_width, i, z_pos; > > @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > goto end; > } > > + priv = crtc->dev->dev_private; > + dpu_kms = to_dpu_kms(priv->kms); > + > mode = &state->adjusted_mode; > DPU_DEBUG("%s: check", dpu_crtc->name); > > @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > } > } > > + _dpu_crtc_get_topology(state, mode); > + if (drm_atomic_crtc_needs_modeset(state)) > + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); > + > end: > kfree(pstates); > return rc; > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 5d501c8..ce66309 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -67,8 +67,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: > @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector, > > static void _dpu_encoder_get_topology( > struct dpu_encoder_virt *dpu_enc, > - struct drm_crtc_state *crtc_state, > - struct drm_display_mode *mode) > + struct drm_crtc_state *crtc_state) > { > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > - /* User split topology for width > 1080 */ > - dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; > - dpu_cstate->num_ctls = dpu_enc->num_phys_encs; > dpu_cstate->num_intfs = dpu_enc->num_phys_encs; > } > > @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check( > } > } > > - _dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode); > + _dpu_encoder_get_topology(dpu_enc, crtc_state); > if (!ret && drm_atomic_crtc_needs_modeset(crtc_state)) > - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state); > + ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, crtc_state); > > if (!ret) > drm_mode_set_crtcinfo(adj_mode, 0); > @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) > > DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); > > - dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state); > + dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state); > } > > static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > index 5304597..901b1fc 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm, enum dpu_hw_blk_type type, > return -EINVAL; > } > > -static void _dpu_rm_release_reservation(struct dpu_rm *rm, > - struct dpu_crtc_state *dpu_cstate) > +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm, > + struct dpu_crtc_state *dpu_cstate) > { > int i; > > @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm, > dpu_cstate->hw_ctls[i]->base.id)) > dpu_cstate->hw_ctls[i] = NULL; > } > +} > + > +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm, > + struct dpu_crtc_state *dpu_cstate) > +{ > + int i; > > for (i = 0; i < dpu_cstate->num_intfs; i++) { > if (!dpu_cstate->hw_intfs[i]) > @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm, > } > } > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state) > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state) > { > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > mutex_lock(&rm->rm_lock); > > - _dpu_rm_release_reservation(rm, dpu_cstate); > + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); > > mutex_unlock(&rm->rm_lock); > } > > -int dpu_rm_reserve( > +void dpu_rm_encoder_release(struct dpu_rm *rm, > + struct drm_crtc_state *crtc_state) > +{ > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > + > + mutex_lock(&rm->rm_lock); > + > + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); > + > + mutex_unlock(&rm->rm_lock); > +} > + > +int dpu_rm_crtc_reserve( > struct dpu_rm *rm, > - struct drm_encoder *enc, > struct drm_crtc_state *crtc_state) > { > - struct dpu_encoder_hw_resources hw_res; > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > int ret; > > @@ -499,12 +515,10 @@ int dpu_rm_reserve( > if (!drm_atomic_crtc_needs_modeset(crtc_state)) > return 0; > > - DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n", > - enc->base.id, crtc_state->crtc->base.id); > > - mutex_lock(&rm->rm_lock); > + DRM_DEBUG_KMS("reserving hw for crtc %d\n", crtc_state->crtc->base.id); > > - dpu_encoder_get_hw_resources(enc, &hw_res); > + mutex_lock(&rm->rm_lock); > > ret = _dpu_rm_reserve_lms(rm, dpu_cstate); > if (ret) { > @@ -518,6 +532,37 @@ int dpu_rm_reserve( > goto cleanup_on_fail; > } > > + mutex_unlock(&rm->rm_lock); > + > + return ret; > + > +cleanup_on_fail: > + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); > + > + mutex_unlock(&rm->rm_lock); > + > + return ret; > +} > + > +int dpu_rm_encoder_reserve( > + struct dpu_rm *rm, > + struct drm_encoder *enc, > + struct drm_crtc_state *crtc_state) > +{ > + struct dpu_encoder_hw_resources hw_res; > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > + int ret; > + > + /* Check if this is just a page-flip */ > + if (!drm_atomic_crtc_needs_modeset(crtc_state)) > + return 0; > + > + DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id); > + > + mutex_lock(&rm->rm_lock); > + > + dpu_encoder_get_hw_resources(enc, &hw_res); > + > ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res); > if (ret) { > DPU_ERROR("unable to find appropriate INTF\n"); > @@ -529,7 +574,7 @@ int dpu_rm_reserve( > return ret; > > cleanup_on_fail: > - _dpu_rm_release_reservation(rm, dpu_cstate); > + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); > > mutex_unlock(&rm->rm_lock); > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > index 1626cef..0b1deb0 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > @@ -53,27 +53,45 @@ 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 > + * dpu_rm_encoder_reserve - Given an Encoder, 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. > * @rm: DPU Resource Manager handle > * @drm_enc: DRM Encoder handle > * @crtc_state: Proposed Atomic DRM CRTC State handle > * @Return: 0 on Success otherwise -ERROR > */ > -int dpu_rm_reserve(struct dpu_rm *rm, > +int dpu_rm_encoder_reserve(struct dpu_rm *rm, > struct drm_encoder *drm_enc, > struct drm_crtc_state *crtc_state); > > /** > - * dpu_rm_release - Given the encoder for the display chain, release any > - * HW blocks previously reserved for that use case. > + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections > + * and user requirements, specified through related > + * topology control properties, and reserve hardware blocks to that > + * display chain. > * @rm: DPU Resource Manager handle > - * @crtc_state: atomic DRM state handle > + * @crtc_state: Proposed Atomic DRM CRTC State handle > * @Return: 0 on Success otherwise -ERROR > */ > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state); > +int dpu_rm_crtc_reserve(struct dpu_rm *rm, > + struct drm_crtc_state *crtc_state); > + > +/** > + * dpu_rm_encoder_release - Given the encoder, release any > + * HW blocks previously reserved for that use case. > + * @rm: DPU Resource Manager handle > + * @crtc_state: Proposed Atomic DRM CRTC State handle > + */ > +void dpu_rm_encoder_release(struct dpu_rm *rm, > + struct drm_crtc_state *crtc_state); > + > +/** > + * dpu_rm_crtc_release - Given the crtc, release any > + * HW blocks previously reserved for that use case. > + * @rm: DPU Resource Manager handle > + * @crtc_state: Proposed Atomic DRM CRTC State handle > + */ > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state); > #endif /* __DPU_RM_H__ */ > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
On 2018-10-09 13:41, Sean Paul wrote: > On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote: >> Instead of letting encoder make a centralized reservation for >> all of its display DRM components, this path splits the >> responsibility between CRTC and Encoder, each requesting >> RM for the HW mapping of its own domain. >> >> Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 +++++++++++++ >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++---- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 > ++++++++++++++++++++++++----- >> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++++++++++---- >> 4 files changed, 119 insertions(+), 31 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index 0625f56..0536b8a 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -47,6 +47,8 @@ >> #define LEFT_MIXER 0 >> #define RIGHT_MIXER 1 >> >> +#define MAX_VDISPLAY_SPLIT 1080 >> + >> static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state > *cstate, >> struct drm_display_mode *mode) >> { >> @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct > drm_crtc *crtc, >> >> for (i = 0; i < cstate->num_mixers; i++) { >> struct drm_rect *r = &cstate->lm_bounds[i]; >> + >> r->x1 = crtc_split_width * i; >> r->y1 = 0; >> r->x2 = r->x1 + crtc_split_width; >> @@ -885,6 +888,7 @@ 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_kms *dpu_kms; >> unsigned long flags; >> >> if (!crtc || !crtc->dev || !crtc->dev->dev_private || > !crtc->state) { >> @@ -895,6 +899,7 @@ 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); >> >> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); >> >> @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc >> *crtc) >> crtc->state->event = NULL; >> spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> } >> + >> + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); >> } >> >> static void dpu_crtc_enable(struct drm_crtc *crtc, >> @@ -1004,6 +1011,21 @@ struct plane_state { >> u32 pipe_id; >> }; >> >> +static void _dpu_crtc_get_topology( >> + struct drm_crtc_state *crtc_state, >> + struct drm_display_mode *mode) >> +{ >> + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> + >> + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 > : 1; >> + >> + /** >> + * encoder->atomic_check is invoked before crtc->atomic_check. >> + * so dpu_cstate->num_intfs should have a non-zero value. >> + */ >> + dpu_cstate->num_ctls = dpu_cstate->num_intfs; > > Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs > directly? > Also, > you don't really need these in their own function, especially if > num_ctls > goes > away. > Yes. I can live with just that. But since dpu_cstate maintains HW arrays for each type, I thought it would be more readable if I could use separate variables to track their counts instead of iterating over ctl arrays over dpu_cstate->num_intfs and leaving comments that both will be same for this version of hardware. Also, the counts need not be the same for all the Snapdragon variants. Thanks, Jeykumar S. >> +} >> + >> static int dpu_crtc_atomic_check(struct drm_crtc *crtc, >> struct drm_crtc_state *state) >> { >> @@ -1014,6 +1036,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 msm_drm_private *priv; >> + struct dpu_kms *dpu_kms; >> >> int cnt = 0, rc = 0, mixer_width, i, z_pos; >> >> @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc > *crtc, >> goto end; >> } >> >> + priv = crtc->dev->dev_private; >> + dpu_kms = to_dpu_kms(priv->kms); >> + >> mode = &state->adjusted_mode; >> DPU_DEBUG("%s: check", dpu_crtc->name); >> >> @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct >> drm_crtc > *crtc, >> } >> } >> >> + _dpu_crtc_get_topology(state, mode); >> + if (drm_atomic_crtc_needs_modeset(state)) >> + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); >> + >> end: >> kfree(pstates); >> return rc; >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 5d501c8..ce66309 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -67,8 +67,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: >> @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct > drm_connector *connector, >> >> static void _dpu_encoder_get_topology( >> struct dpu_encoder_virt *dpu_enc, >> - struct drm_crtc_state *crtc_state, >> - struct drm_display_mode *mode) >> + struct drm_crtc_state *crtc_state) >> { >> struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> >> - /* User split topology for width > 1080 */ >> - dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 > : 1; >> - dpu_cstate->num_ctls = dpu_enc->num_phys_encs; >> dpu_cstate->num_intfs = dpu_enc->num_phys_encs; >> } >> >> @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check( >> } >> } >> >> - _dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode); >> + _dpu_encoder_get_topology(dpu_enc, crtc_state); >> if (!ret && drm_atomic_crtc_needs_modeset(crtc_state)) >> - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state); >> + ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, > crtc_state); >> >> if (!ret) >> drm_mode_set_crtcinfo(adj_mode, 0); >> @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct > drm_encoder *drm_enc) >> >> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); >> >> - dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state); >> + dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state); >> } >> >> static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg >> *catalog, >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> index 5304597..901b1fc 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c >> @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm, > enum dpu_hw_blk_type type, >> return -EINVAL; >> } >> >> -static void _dpu_rm_release_reservation(struct dpu_rm *rm, >> - struct dpu_crtc_state *dpu_cstate) >> +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm, >> + struct dpu_crtc_state > *dpu_cstate) >> { >> int i; >> >> @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct > dpu_rm *rm, >> dpu_cstate->hw_ctls[i]->base.id)) >> dpu_cstate->hw_ctls[i] = NULL; >> } >> +} >> + >> +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm, >> + struct dpu_crtc_state > *dpu_cstate) >> +{ >> + int i; >> >> for (i = 0; i < dpu_cstate->num_intfs; i++) { >> if (!dpu_cstate->hw_intfs[i]) >> @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct > dpu_rm *rm, >> } >> } >> >> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state > *crtc_state) >> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state > *crtc_state) >> { >> struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> >> mutex_lock(&rm->rm_lock); >> >> - _dpu_rm_release_reservation(rm, dpu_cstate); >> + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); >> >> mutex_unlock(&rm->rm_lock); >> } >> >> -int dpu_rm_reserve( >> +void dpu_rm_encoder_release(struct dpu_rm *rm, >> + struct drm_crtc_state *crtc_state) >> +{ >> + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> + >> + mutex_lock(&rm->rm_lock); >> + >> + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); >> + >> + mutex_unlock(&rm->rm_lock); >> +} >> + >> +int dpu_rm_crtc_reserve( >> struct dpu_rm *rm, >> - struct drm_encoder *enc, >> struct drm_crtc_state *crtc_state) >> { >> - struct dpu_encoder_hw_resources hw_res; >> struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> int ret; >> >> @@ -499,12 +515,10 @@ int dpu_rm_reserve( >> if (!drm_atomic_crtc_needs_modeset(crtc_state)) >> return 0; >> >> - DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n", >> - enc->base.id, crtc_state->crtc->base.id); >> >> - mutex_lock(&rm->rm_lock); >> + DRM_DEBUG_KMS("reserving hw for crtc %d\n", > crtc_state->crtc->base.id); >> >> - dpu_encoder_get_hw_resources(enc, &hw_res); >> + mutex_lock(&rm->rm_lock); >> >> ret = _dpu_rm_reserve_lms(rm, dpu_cstate); >> if (ret) { >> @@ -518,6 +532,37 @@ int dpu_rm_reserve( >> goto cleanup_on_fail; >> } >> >> + mutex_unlock(&rm->rm_lock); >> + >> + return ret; >> + >> +cleanup_on_fail: >> + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); >> + >> + mutex_unlock(&rm->rm_lock); >> + >> + return ret; >> +} >> + >> +int dpu_rm_encoder_reserve( >> + struct dpu_rm *rm, >> + struct drm_encoder *enc, >> + struct drm_crtc_state *crtc_state) >> +{ >> + struct dpu_encoder_hw_resources hw_res; >> + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); >> + int ret; >> + >> + /* Check if this is just a page-flip */ >> + if (!drm_atomic_crtc_needs_modeset(crtc_state)) >> + return 0; >> + >> + DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id); >> + >> + mutex_lock(&rm->rm_lock); >> + >> + dpu_encoder_get_hw_resources(enc, &hw_res); >> + >> ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res); >> if (ret) { >> DPU_ERROR("unable to find appropriate INTF\n"); >> @@ -529,7 +574,7 @@ int dpu_rm_reserve( >> return ret; >> >> cleanup_on_fail: >> - _dpu_rm_release_reservation(rm, dpu_cstate); >> + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); >> >> mutex_unlock(&rm->rm_lock); >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> index 1626cef..0b1deb0 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h >> @@ -53,27 +53,45 @@ 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 >> + * dpu_rm_encoder_reserve - Given an Encoder, 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. >> * @rm: DPU Resource Manager handle >> * @drm_enc: DRM Encoder handle >> * @crtc_state: Proposed Atomic DRM CRTC State handle >> * @Return: 0 on Success otherwise -ERROR >> */ >> -int dpu_rm_reserve(struct dpu_rm *rm, >> +int dpu_rm_encoder_reserve(struct dpu_rm *rm, >> struct drm_encoder *drm_enc, >> struct drm_crtc_state *crtc_state); >> >> /** >> - * dpu_rm_release - Given the encoder for the display chain, release > any >> - * HW blocks previously reserved for that use case. >> + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections >> + * and user requirements, specified through related >> + * topology control properties, and reserve hardware blocks to that >> + * display chain. >> * @rm: DPU Resource Manager handle >> - * @crtc_state: atomic DRM state handle >> + * @crtc_state: Proposed Atomic DRM CRTC State handle >> * @Return: 0 on Success otherwise -ERROR >> */ >> -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state > *crtc_state); >> +int dpu_rm_crtc_reserve(struct dpu_rm *rm, >> + struct drm_crtc_state *crtc_state); >> + >> +/** >> + * dpu_rm_encoder_release - Given the encoder, release any >> + * HW blocks previously reserved for that use case. >> + * @rm: DPU Resource Manager handle >> + * @crtc_state: Proposed Atomic DRM CRTC State handle >> + */ >> +void dpu_rm_encoder_release(struct dpu_rm *rm, >> + struct drm_crtc_state *crtc_state); >> + >> +/** >> + * dpu_rm_crtc_release - Given the crtc, release any >> + * HW blocks previously reserved for that use case. >> + * @rm: DPU Resource Manager handle >> + * @crtc_state: Proposed Atomic DRM CRTC State handle >> + */ >> +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state > *crtc_state); >> #endif /* __DPU_RM_H__ */ >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > Forum, >> a Linux Foundation Collaborative Project >> >> _______________________________________________ >> Freedreno mailing list >> Freedreno@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/freedreno
On Tue, Oct 09, 2018 at 11:15:02PM -0700, Jeykumar Sankaran wrote: > On 2018-10-09 13:41, Sean Paul wrote: > > On Mon, Oct 08, 2018 at 09:27:39PM -0700, Jeykumar Sankaran wrote: > > > Instead of letting encoder make a centralized reservation for > > > all of its display DRM components, this path splits the > > > responsibility between CRTC and Encoder, each requesting > > > RM for the HW mapping of its own domain. > > > > > > Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> > > > --- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 +++++++++++++ > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++---- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 > > ++++++++++++++++++++++++----- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++++++++++---- > > > 4 files changed, 119 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > index 0625f56..0536b8a 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > > @@ -47,6 +47,8 @@ > > > #define LEFT_MIXER 0 > > > #define RIGHT_MIXER 1 > > > > > > +#define MAX_VDISPLAY_SPLIT 1080 > > > + > > > static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state > > *cstate, > > > struct drm_display_mode *mode) > > > { > > > @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct > > drm_crtc *crtc, > > > > > > for (i = 0; i < cstate->num_mixers; i++) { > > > struct drm_rect *r = &cstate->lm_bounds[i]; > > > + > > > r->x1 = crtc_split_width * i; > > > r->y1 = 0; > > > r->x2 = r->x1 + crtc_split_width; > > > @@ -885,6 +888,7 @@ 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_kms *dpu_kms; > > > unsigned long flags; > > > > > > if (!crtc || !crtc->dev || !crtc->dev->dev_private || > > !crtc->state) { > > > @@ -895,6 +899,7 @@ 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); > > > > > > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); > > > > > > @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc > > > *crtc) > > > crtc->state->event = NULL; > > > spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > > > } > > > + > > > + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); > > > } > > > > > > static void dpu_crtc_enable(struct drm_crtc *crtc, > > > @@ -1004,6 +1011,21 @@ struct plane_state { > > > u32 pipe_id; > > > }; > > > > > > +static void _dpu_crtc_get_topology( > > > + struct drm_crtc_state *crtc_state, > > > + struct drm_display_mode *mode) > > > +{ > > > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > + > > > + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 > > : 1; > > > + > > > + /** > > > + * encoder->atomic_check is invoked before crtc->atomic_check. > > > + * so dpu_cstate->num_intfs should have a non-zero value. > > > + */ > > > + dpu_cstate->num_ctls = dpu_cstate->num_intfs; > > > > Why do we need num_ctls? Can't we just use dpu_cstate->num_intfs > > directly? > > Also, > > you don't really need these in their own function, especially if > > num_ctls > > goes > > away. > > > Yes. I can live with just that. But since dpu_cstate maintains HW arrays > for each type, I thought it would be more readable if I could use > separate variables to track their counts instead of iterating over > ctl arrays over dpu_cstate->num_intfs and leaving comments that both > will be same for this version of hardware. You could change the name to make it more generic. AFAICT, num_h_tiles == num_phys_encs == num_intfs == num_ctls So storing it as num_h_tiles might make more sense. > > Also, the counts need not be the same for all the Snapdragon variants. This is probably a good thing. It doesn't seem like the current driver would work if these values were different, so making it explicit is a good signal that more invasive changes are needed. Sean > > Thanks, > Jeykumar S. > > > +} > > > + > > > static int dpu_crtc_atomic_check(struct drm_crtc *crtc, > > > struct drm_crtc_state *state) > > > { > > > @@ -1014,6 +1036,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 msm_drm_private *priv; > > > + struct dpu_kms *dpu_kms; > > > > > > int cnt = 0, rc = 0, mixer_width, i, z_pos; > > > > > > @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc > > *crtc, > > > goto end; > > > } > > > > > > + priv = crtc->dev->dev_private; > > > + dpu_kms = to_dpu_kms(priv->kms); > > > + > > > mode = &state->adjusted_mode; > > > DPU_DEBUG("%s: check", dpu_crtc->name); > > > > > > @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct > > > drm_crtc > > *crtc, > > > } > > > } > > > > > > + _dpu_crtc_get_topology(state, mode); > > > + if (drm_atomic_crtc_needs_modeset(state)) > > > + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); > > > + > > > end: > > > kfree(pstates); > > > return rc; > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > index 5d501c8..ce66309 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > @@ -67,8 +67,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: > > > @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct > > drm_connector *connector, > > > > > > static void _dpu_encoder_get_topology( > > > struct dpu_encoder_virt *dpu_enc, > > > - struct drm_crtc_state *crtc_state, > > > - struct drm_display_mode *mode) > > > + struct drm_crtc_state *crtc_state) > > > { > > > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > > > > - /* User split topology for width > 1080 */ > > > - dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 > > : 1; > > > - dpu_cstate->num_ctls = dpu_enc->num_phys_encs; > > > dpu_cstate->num_intfs = dpu_enc->num_phys_encs; > > > } > > > > > > @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check( > > > } > > > } > > > > > > - _dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode); > > > + _dpu_encoder_get_topology(dpu_enc, crtc_state); > > > if (!ret && drm_atomic_crtc_needs_modeset(crtc_state)) > > > - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state); > > > + ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, > > crtc_state); > > > > > > if (!ret) > > > drm_mode_set_crtcinfo(adj_mode, 0); > > > @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct > > drm_encoder *drm_enc) > > > > > > DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); > > > > > > - dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state); > > > + dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state); > > > } > > > > > > static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg > > > *catalog, > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > > index 5304597..901b1fc 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c > > > @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm, > > enum dpu_hw_blk_type type, > > > return -EINVAL; > > > } > > > > > > -static void _dpu_rm_release_reservation(struct dpu_rm *rm, > > > - struct dpu_crtc_state *dpu_cstate) > > > +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm, > > > + struct dpu_crtc_state > > *dpu_cstate) > > > { > > > int i; > > > > > > @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct > > dpu_rm *rm, > > > dpu_cstate->hw_ctls[i]->base.id)) > > > dpu_cstate->hw_ctls[i] = NULL; > > > } > > > +} > > > + > > > +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm, > > > + struct dpu_crtc_state > > *dpu_cstate) > > > +{ > > > + int i; > > > > > > for (i = 0; i < dpu_cstate->num_intfs; i++) { > > > if (!dpu_cstate->hw_intfs[i]) > > > @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct > > dpu_rm *rm, > > > } > > > } > > > > > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state > > *crtc_state) > > > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state > > *crtc_state) > > > { > > > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > > > > mutex_lock(&rm->rm_lock); > > > > > > - _dpu_rm_release_reservation(rm, dpu_cstate); > > > + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); > > > > > > mutex_unlock(&rm->rm_lock); > > > } > > > > > > -int dpu_rm_reserve( > > > +void dpu_rm_encoder_release(struct dpu_rm *rm, > > > + struct drm_crtc_state *crtc_state) > > > +{ > > > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > + > > > + mutex_lock(&rm->rm_lock); > > > + > > > + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); > > > + > > > + mutex_unlock(&rm->rm_lock); > > > +} > > > + > > > +int dpu_rm_crtc_reserve( > > > struct dpu_rm *rm, > > > - struct drm_encoder *enc, > > > struct drm_crtc_state *crtc_state) > > > { > > > - struct dpu_encoder_hw_resources hw_res; > > > struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > int ret; > > > > > > @@ -499,12 +515,10 @@ int dpu_rm_reserve( > > > if (!drm_atomic_crtc_needs_modeset(crtc_state)) > > > return 0; > > > > > > - DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n", > > > - enc->base.id, crtc_state->crtc->base.id); > > > > > > - mutex_lock(&rm->rm_lock); > > > + DRM_DEBUG_KMS("reserving hw for crtc %d\n", > > crtc_state->crtc->base.id); > > > > > > - dpu_encoder_get_hw_resources(enc, &hw_res); > > > + mutex_lock(&rm->rm_lock); > > > > > > ret = _dpu_rm_reserve_lms(rm, dpu_cstate); > > > if (ret) { > > > @@ -518,6 +532,37 @@ int dpu_rm_reserve( > > > goto cleanup_on_fail; > > > } > > > > > > + mutex_unlock(&rm->rm_lock); > > > + > > > + return ret; > > > + > > > +cleanup_on_fail: > > > + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); > > > + > > > + mutex_unlock(&rm->rm_lock); > > > + > > > + return ret; > > > +} > > > + > > > +int dpu_rm_encoder_reserve( > > > + struct dpu_rm *rm, > > > + struct drm_encoder *enc, > > > + struct drm_crtc_state *crtc_state) > > > +{ > > > + struct dpu_encoder_hw_resources hw_res; > > > + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); > > > + int ret; > > > + > > > + /* Check if this is just a page-flip */ > > > + if (!drm_atomic_crtc_needs_modeset(crtc_state)) > > > + return 0; > > > + > > > + DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id); > > > + > > > + mutex_lock(&rm->rm_lock); > > > + > > > + dpu_encoder_get_hw_resources(enc, &hw_res); > > > + > > > ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res); > > > if (ret) { > > > DPU_ERROR("unable to find appropriate INTF\n"); > > > @@ -529,7 +574,7 @@ int dpu_rm_reserve( > > > return ret; > > > > > > cleanup_on_fail: > > > - _dpu_rm_release_reservation(rm, dpu_cstate); > > > + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); > > > > > > mutex_unlock(&rm->rm_lock); > > > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > > index 1626cef..0b1deb0 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h > > > @@ -53,27 +53,45 @@ 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 > > > + * dpu_rm_encoder_reserve - Given an Encoder, 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. > > > * @rm: DPU Resource Manager handle > > > * @drm_enc: DRM Encoder handle > > > * @crtc_state: Proposed Atomic DRM CRTC State handle > > > * @Return: 0 on Success otherwise -ERROR > > > */ > > > -int dpu_rm_reserve(struct dpu_rm *rm, > > > +int dpu_rm_encoder_reserve(struct dpu_rm *rm, > > > struct drm_encoder *drm_enc, > > > struct drm_crtc_state *crtc_state); > > > > > > /** > > > - * dpu_rm_release - Given the encoder for the display chain, release > > any > > > - * HW blocks previously reserved for that use case. > > > + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections > > > + * and user requirements, specified through related > > > + * topology control properties, and reserve hardware blocks to that > > > + * display chain. > > > * @rm: DPU Resource Manager handle > > > - * @crtc_state: atomic DRM state handle > > > + * @crtc_state: Proposed Atomic DRM CRTC State handle > > > * @Return: 0 on Success otherwise -ERROR > > > */ > > > -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state > > *crtc_state); > > > +int dpu_rm_crtc_reserve(struct dpu_rm *rm, > > > + struct drm_crtc_state *crtc_state); > > > + > > > +/** > > > + * dpu_rm_encoder_release - Given the encoder, release any > > > + * HW blocks previously reserved for that use case. > > > + * @rm: DPU Resource Manager handle > > > + * @crtc_state: Proposed Atomic DRM CRTC State handle > > > + */ > > > +void dpu_rm_encoder_release(struct dpu_rm *rm, > > > + struct drm_crtc_state *crtc_state); > > > + > > > +/** > > > + * dpu_rm_crtc_release - Given the crtc, release any > > > + * HW blocks previously reserved for that use case. > > > + * @rm: DPU Resource Manager handle > > > + * @crtc_state: Proposed Atomic DRM CRTC State handle > > > + */ > > > +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state > > *crtc_state); > > > #endif /* __DPU_RM_H__ */ > > > -- > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > > Forum, > > > a Linux Foundation Collaborative Project > > > > > > _______________________________________________ > > > Freedreno mailing list > > > Freedreno@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/freedreno > > -- > Jeykumar S
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 0625f56..0536b8a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -47,6 +47,8 @@ #define LEFT_MIXER 0 #define RIGHT_MIXER 1 +#define MAX_VDISPLAY_SPLIT 1080 + static inline int _dpu_crtc_get_mixer_width(struct dpu_crtc_state *cstate, struct drm_display_mode *mode) { @@ -448,6 +450,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, for (i = 0; i < cstate->num_mixers; i++) { struct drm_rect *r = &cstate->lm_bounds[i]; + r->x1 = crtc_split_width * i; r->y1 = 0; r->x2 = r->x1 + crtc_split_width; @@ -885,6 +888,7 @@ 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_kms *dpu_kms; unsigned long flags; if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { @@ -895,6 +899,7 @@ 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); DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); @@ -953,6 +958,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) crtc->state->event = NULL; spin_unlock_irqrestore(&crtc->dev->event_lock, flags); } + + dpu_rm_crtc_release(&dpu_kms->rm, crtc->state); } static void dpu_crtc_enable(struct drm_crtc *crtc, @@ -1004,6 +1011,21 @@ struct plane_state { u32 pipe_id; }; +static void _dpu_crtc_get_topology( + struct drm_crtc_state *crtc_state, + struct drm_display_mode *mode) +{ + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); + + dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; + + /** + * encoder->atomic_check is invoked before crtc->atomic_check. + * so dpu_cstate->num_intfs should have a non-zero value. + */ + dpu_cstate->num_ctls = dpu_cstate->num_intfs; +} + static int dpu_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -1014,6 +1036,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 msm_drm_private *priv; + struct dpu_kms *dpu_kms; int cnt = 0, rc = 0, mixer_width, i, z_pos; @@ -1039,6 +1063,9 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, goto end; } + priv = crtc->dev->dev_private; + dpu_kms = to_dpu_kms(priv->kms); + mode = &state->adjusted_mode; DPU_DEBUG("%s: check", dpu_crtc->name); @@ -1229,6 +1256,10 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc, } } + _dpu_crtc_get_topology(state, mode); + if (drm_atomic_crtc_needs_modeset(state)) + rc = dpu_rm_crtc_reserve(&dpu_kms->rm, state); + end: kfree(pstates); return rc; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 5d501c8..ce66309 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -67,8 +67,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: @@ -557,14 +555,10 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector, static void _dpu_encoder_get_topology( struct dpu_encoder_virt *dpu_enc, - struct drm_crtc_state *crtc_state, - struct drm_display_mode *mode) + struct drm_crtc_state *crtc_state) { struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); - /* User split topology for width > 1080 */ - dpu_cstate->num_mixers = (mode->vdisplay > MAX_VDISPLAY_SPLIT) ? 2 : 1; - dpu_cstate->num_ctls = dpu_enc->num_phys_encs; dpu_cstate->num_intfs = dpu_enc->num_phys_encs; } @@ -623,9 +617,9 @@ static int dpu_encoder_virt_atomic_check( } } - _dpu_encoder_get_topology(dpu_enc, crtc_state, adj_mode); + _dpu_encoder_get_topology(dpu_enc, crtc_state); if (!ret && drm_atomic_crtc_needs_modeset(crtc_state)) - ret = dpu_rm_reserve(&dpu_kms->rm, drm_enc, crtc_state); + ret = dpu_rm_encoder_reserve(&dpu_kms->rm, drm_enc, crtc_state); if (!ret) drm_mode_set_crtcinfo(adj_mode, 0); @@ -1186,7 +1180,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); - dpu_rm_release(&dpu_kms->rm, drm_enc->crtc->state); + dpu_rm_encoder_release(&dpu_kms->rm, drm_enc->crtc->state); } static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 5304597..901b1fc 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -436,8 +436,8 @@ static int _dpu_rm_release_hw(struct dpu_rm *rm, enum dpu_hw_blk_type type, return -EINVAL; } -static void _dpu_rm_release_reservation(struct dpu_rm *rm, - struct dpu_crtc_state *dpu_cstate) +static void _dpu_rm_crtc_release_reservation(struct dpu_rm *rm, + struct dpu_crtc_state *dpu_cstate) { int i; @@ -464,6 +464,12 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm, dpu_cstate->hw_ctls[i]->base.id)) dpu_cstate->hw_ctls[i] = NULL; } +} + +static void _dpu_rm_encoder_release_reservation(struct dpu_rm *rm, + struct dpu_crtc_state *dpu_cstate) +{ + int i; for (i = 0; i < dpu_cstate->num_intfs; i++) { if (!dpu_cstate->hw_intfs[i]) @@ -475,23 +481,33 @@ static void _dpu_rm_release_reservation(struct dpu_rm *rm, } } -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state) +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state) { struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); mutex_lock(&rm->rm_lock); - _dpu_rm_release_reservation(rm, dpu_cstate); + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); mutex_unlock(&rm->rm_lock); } -int dpu_rm_reserve( +void dpu_rm_encoder_release(struct dpu_rm *rm, + struct drm_crtc_state *crtc_state) +{ + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); + + mutex_lock(&rm->rm_lock); + + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); + + mutex_unlock(&rm->rm_lock); +} + +int dpu_rm_crtc_reserve( struct dpu_rm *rm, - struct drm_encoder *enc, struct drm_crtc_state *crtc_state) { - struct dpu_encoder_hw_resources hw_res; struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); int ret; @@ -499,12 +515,10 @@ int dpu_rm_reserve( if (!drm_atomic_crtc_needs_modeset(crtc_state)) return 0; - DRM_DEBUG_KMS("reserving hw for enc %d crtc %d\n", - enc->base.id, crtc_state->crtc->base.id); - mutex_lock(&rm->rm_lock); + DRM_DEBUG_KMS("reserving hw for crtc %d\n", crtc_state->crtc->base.id); - dpu_encoder_get_hw_resources(enc, &hw_res); + mutex_lock(&rm->rm_lock); ret = _dpu_rm_reserve_lms(rm, dpu_cstate); if (ret) { @@ -518,6 +532,37 @@ int dpu_rm_reserve( goto cleanup_on_fail; } + mutex_unlock(&rm->rm_lock); + + return ret; + +cleanup_on_fail: + _dpu_rm_crtc_release_reservation(rm, dpu_cstate); + + mutex_unlock(&rm->rm_lock); + + return ret; +} + +int dpu_rm_encoder_reserve( + struct dpu_rm *rm, + struct drm_encoder *enc, + struct drm_crtc_state *crtc_state) +{ + struct dpu_encoder_hw_resources hw_res; + struct dpu_crtc_state *dpu_cstate = to_dpu_crtc_state(crtc_state); + int ret; + + /* Check if this is just a page-flip */ + if (!drm_atomic_crtc_needs_modeset(crtc_state)) + return 0; + + DRM_DEBUG_KMS("reserving hw for enc %d\n", enc->base.id); + + mutex_lock(&rm->rm_lock); + + dpu_encoder_get_hw_resources(enc, &hw_res); + ret = _dpu_rm_reserve_intfs(rm, dpu_cstate, &hw_res); if (ret) { DPU_ERROR("unable to find appropriate INTF\n"); @@ -529,7 +574,7 @@ int dpu_rm_reserve( return ret; cleanup_on_fail: - _dpu_rm_release_reservation(rm, dpu_cstate); + _dpu_rm_encoder_release_reservation(rm, dpu_cstate); mutex_unlock(&rm->rm_lock); diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h index 1626cef..0b1deb0 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h @@ -53,27 +53,45 @@ 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 + * dpu_rm_encoder_reserve - Given an Encoder, 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. * @rm: DPU Resource Manager handle * @drm_enc: DRM Encoder handle * @crtc_state: Proposed Atomic DRM CRTC State handle * @Return: 0 on Success otherwise -ERROR */ -int dpu_rm_reserve(struct dpu_rm *rm, +int dpu_rm_encoder_reserve(struct dpu_rm *rm, struct drm_encoder *drm_enc, struct drm_crtc_state *crtc_state); /** - * dpu_rm_release - Given the encoder for the display chain, release any - * HW blocks previously reserved for that use case. + * dpu_rm_crtc_reserve - Given a CRTC, analyze the use connections + * and user requirements, specified through related + * topology control properties, and reserve hardware blocks to that + * display chain. * @rm: DPU Resource Manager handle - * @crtc_state: atomic DRM state handle + * @crtc_state: Proposed Atomic DRM CRTC State handle * @Return: 0 on Success otherwise -ERROR */ -void dpu_rm_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state); +int dpu_rm_crtc_reserve(struct dpu_rm *rm, + struct drm_crtc_state *crtc_state); + +/** + * dpu_rm_encoder_release - Given the encoder, release any + * HW blocks previously reserved for that use case. + * @rm: DPU Resource Manager handle + * @crtc_state: Proposed Atomic DRM CRTC State handle + */ +void dpu_rm_encoder_release(struct dpu_rm *rm, + struct drm_crtc_state *crtc_state); + +/** + * dpu_rm_crtc_release - Given the crtc, release any + * HW blocks previously reserved for that use case. + * @rm: DPU Resource Manager handle + * @crtc_state: Proposed Atomic DRM CRTC State handle + */ +void dpu_rm_crtc_release(struct dpu_rm *rm, struct drm_crtc_state *crtc_state); #endif /* __DPU_RM_H__ */
Instead of letting encoder make a centralized reservation for all of its display DRM components, this path splits the responsibility between CRTC and Encoder, each requesting RM for the HW mapping of its own domain. Signed-off-by: Jeykumar Sankaran <jsanka@codeaurora.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 +++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 14 ++---- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 69 ++++++++++++++++++++++++----- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 36 +++++++++++---- 4 files changed, 119 insertions(+), 31 deletions(-)