[DPU PATCH 4/4] drm/msm/dpu: use private obj to track hw resources
Jeykumar Sankaran
jsanka at codeaurora.org
Wed Jun 13 19:01:21 UTC 2018
On 2018-06-13 09:44, Jordan Crouse wrote:
> On Tue, Jun 12, 2018 at 06:17:47PM -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.
>>
>> Fixes resource contention due to race conditions between
>> user space and display thread by reserving resources
>> only in atomic check.
>>
>> Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 210 +++---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 59 +-
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 223 ++----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 4 -
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 +-
>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 32 +-
>> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 86 +--
>> 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 | 805
> ++++++---------------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 149 ++--
>> 11 files changed, 534 insertions(+), 1070 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 426e2ad..a484c06 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 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)
>> {
>> @@ -142,9 +144,9 @@ static void _dpu_crtc_program_lm_output_roi(struct
> drm_crtc *crtc)
>> crtc_state = to_dpu_crtc_state(crtc->state);
>>
>> lm_horiz_position = 0;
>> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++) {
>> + for (lm_idx = 0; lm_idx < crtc_state->num_mixers; lm_idx++) {
>> const struct dpu_rect *lm_roi =
> &crtc_state->lm_bounds[lm_idx];
>> - struct dpu_hw_mixer *hw_lm =
> dpu_crtc->mixers[lm_idx].hw_lm;
>> + struct dpu_hw_mixer *hw_lm =
> crtc_state->mixers[lm_idx].hw_lm;
>> struct dpu_hw_mixer_cfg cfg;
>>
>> if (dpu_kms_rect_is_null(lm_roi))
>> @@ -182,7 +184,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> drm_crtc *crtc,
>> return;
>> }
>>
>> - ctl = mixer->hw_ctl;
>> + ctl = mixer->lm_ctl;
>> lm = mixer->hw_lm;
>> stage_cfg = &dpu_crtc->stage_cfg;
>> cstate = to_dpu_crtc_state(crtc->state);
>> @@ -237,7 +239,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> drm_crtc *crtc,
>> format->base.pixel_format, fb ? fb->modifier : 0);
>>
>> /* blend config update */
>> - for (lm_idx = 0; lm_idx < dpu_crtc->num_mixers; lm_idx++)
> {
>> + for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
>> _dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate);
>>
>> mixer[lm_idx].flush_mask |= flush_mask;
>> @@ -260,7 +262,7 @@ static void _dpu_crtc_blend_setup_mixer(struct
> drm_crtc *crtc,
>> static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
>> {
>> struct dpu_crtc *dpu_crtc;
>> - struct dpu_crtc_state *dpu_crtc_state;
>> + struct dpu_crtc_state *cstate;
>> struct dpu_crtc_mixer *mixer;
>> struct dpu_hw_ctl *ctl;
>> struct dpu_hw_mixer *lm;
>> @@ -271,26 +273,26 @@ static void _dpu_crtc_blend_setup(struct
>> drm_crtc
> *crtc)
>> return;
>>
>> dpu_crtc = to_dpu_crtc(crtc);
>> - dpu_crtc_state = to_dpu_crtc_state(crtc->state);
>> - mixer = dpu_crtc->mixers;
>> + cstate = to_dpu_crtc_state(crtc->state);
>> + mixer = cstate->mixers;
>>
>> DPU_DEBUG("%s\n", dpu_crtc->name);
>>
>> - if (dpu_crtc->num_mixers > CRTC_DUAL_MIXERS) {
>> - DPU_ERROR("invalid number mixers: %d\n",
> dpu_crtc->num_mixers);
>> + if (cstate->num_mixers > CRTC_DUAL_MIXERS) {
>> + DPU_ERROR("invalid number mixers: %d\n",
> cstate->num_mixers);
>
> Nit - this could be worded a bit better - "too many mixers" would be
> better, but
> I have to ask - under what circumstances would the number of mixers be
> larger
> than CRTC_DUAL_MIXERS and/or why don't we support a dynamic number of
> mixers?
>
Comes from downstream driver implementation where CRTC iterates through
RM free pool to identify mixers tagged with the current crtc id. If the
previous clean up was screwed up this may have more than 2 mixers. With
the new state based RM, its very unlikely we hit this condition.
We do support dynamic mixer counts. Based on the connector (panel)
resolution,
CRTC allocates 1 or 2 (panel_width > hw mixer width) mixer block(s) on
the first
atomic check. DPU limits max no. of hw mixers that can be ganged up for
a display to 2.
>> return;
>> }
>
> <ship>
>
>> +static void _dpu_crtc_setup_mixers(struct drm_crtc_state *crtc_state)
>> {
>> - struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>> - 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);
>> + if (!cstate->num_mixers || !cstate->num_ctls) {
>> + DPU_ERROR("invalid hw count-lm's:%d ctl's:%d\n",
>> + cstate->num_mixers, cstate->num_ctls);
>
> This can also be reworded - I don't know what lm is, and you shouldn't
> use
> an
> apostrophe - it would just be 'ctls' or 'mixers'. Instead of invalid,
> perhaps
> using "no mixers defined" and "no controls defined".
>
>
>> + return;
>> + }
>
> <snip>
>
>> @@ -1584,6 +1579,23 @@ static int dpu_crtc_atomic_check(struct
>> drm_crtc
> *crtc,
>> }
>> }
>>
>> + /* Resource allocation */
>> + dpu_priv_state = dpu_get_private_state(state->state);
>> +
>
> Ah, here is a use of dpu_get_private_state() that assumes
> dpu_priv_state
> is
> valid - this could use a ERR check but I think it also implies that
> dpu_get_private_state() doesn't have a legitimate reason to return
> NULL.
>
>> + 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);
>> + 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);
>> +
>> end:
>> return rc;
>> }
>
> <snip>
>
>> @@ -657,27 +666,31 @@ 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);
>
> Here is another use of dpu_get_private_state() that assumes a valid
> pointer.
Will add IS_ERR check.
>
>> + 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);
>> + if (ret) {
>> + DPU_ERROR_ENC(dpu_enc,
>> + "failed to allocate hw resources\n");
>> + goto end;
>> }
>>
>> - if (!ret)
>> - drm_mode_set_crtcinfo(adj_mode, 0);
>> + drm_mode_set_crtcinfo(adj_mode, 0);
>>
>> DPU_EVT32(DRMID(drm_enc), adj_mode->flags,
> adj_mode->private_flags);
>> -
>> +end:
>> return ret;
>> }
>
> <snip>
>
>> @@ -1170,35 +1159,48 @@ void dpu_encoder_virt_restore(struct
>> drm_encoder
> *drm_enc)
>> static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>> {
>> struct dpu_encoder_virt *dpu_enc = NULL;
>> + struct dpu_encoder_phys *phys = NULL;
>> int i, ret = 0;
>> - struct drm_display_mode *cur_mode = NULL;
>>
>> if (!drm_enc) {
>> DPU_ERROR("invalid encoder\n");
>> return;
>> }
>> dpu_enc = to_dpu_encoder_virt(drm_enc);
>> - cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>>
>> DPU_DEBUG_ENC(dpu_enc, "\n");
>> - DPU_EVT32(DRMID(drm_enc), cur_mode->hdisplay, cur_mode->vdisplay);
>>
>> dpu_enc->cur_master = NULL;
>> + dpu_enc->cur_slave = NULL;
>> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> - struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> + phys = dpu_enc->phys_encs[i];
>>
>> - if (phys && phys->ops.is_master &&
> phys->ops.is_master(phys)) {
>> - DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
> i);
>> + if (!phys || !phys->ops.is_master)
>> + continue;
>> +
>> + if (phys->ops.is_master(phys)) {
>> + DPU_DEBUG_ENC(dpu_enc, "master is at idx %d\n",
> i);
>> dpu_enc->cur_master = phys;
>> - break;
>> + } else {
>> + DPU_DEBUG_ENC(dpu_enc, "slave is at idx %d\n", i);
>> + dpu_enc->cur_slave = phys;
>> }
>> }
>>
>> if (!dpu_enc->cur_master) {
>> - DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
>> + DPU_ERROR("virt encoder has no master!\n");
>
> I don't think this rises to the occasion of needing a exclamation
> point.
>
> <snip>
Will address rest of the comments in V2.
--
Jeykumar S
More information about the dri-devel
mailing list