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@xxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno
--
Sean Paul, Software Engineer, Google / Chromium OS
