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@xxxxxxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
