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 <jsa...@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?
> 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.
> + 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>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
