Re: [PATCH 22/25] drm/msm/dpu: make crtc and encoder specific HW reservation

Jeykumar Sankaran
2018-10-10T06:15:02Z

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

--
Jeykumar S