On Tue, Aug 07, 2018 at 08:12:31PM -0700, Jeykumar Sankaran wrote:
Identify slave-master encoders and program them explicitly.
You've got the what, but could you please explain the why?
changes in v2:
- none
changes in v3:
- none
Change-Id: I0ebfada05bd7f8437f842ad860490a678aa8f4cd Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39 ++++++++++++++++++----------- 1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 1b4de34..a0ced79 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -184,6 +184,7 @@ struct dpu_encoder_virt { unsigned int num_phys_encs; struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; struct dpu_encoder_phys *cur_master;
- struct dpu_encoder_phys *cur_slave;
You only use this in one function, why not just make it a local?
struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];
bool intfs_swapped; @@ -1153,6 +1154,7 @@ 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;
@@ -1160,6 +1162,7 @@ static void dpu_encoder_virt_enable(struct drm_encoder *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;
@@ -1167,21 +1170,36 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) cur_mode->vdisplay);
dpu_enc->cur_master = NULL;
- dpu_enc->cur_slave = NULL;
There's no benefit to setting this NULL. cur_master is set to NULL so it can be checked after the loop. Since you're not checking this, it's not necessary.
I suppose you might also want to clear this on disable like master.
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)
I don't think it's possible for phys to be NULL, is it?
continue;
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->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;
You're making an assumption here that there can only be one master and there can only be one slave.
It seems like you could avoid all of this work if you just did the assignment in dpu_encoder_virt_add_phys_encs().
}
if (!dpu_enc->cur_master) {
DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
DPU_ERROR("virt encoder has no master identified\n");return; }
/* always enable slave encoder before master */
phys = dpu_enc->cur_slave;
if (phys && phys->ops.enable)
phys->ops.enable(phys);phys = dpu_enc->cur_master;
if (phys && phys->ops.enable)
phys->ops.enable(phys);ret = dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_KICKOFF); if (ret) { DPU_ERROR_ENC(dpu_enc, "dpu resource control failed: %d\n",
@@ -1190,25 +1208,16 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) }
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 != dpu_enc->cur_master) {if (phys->ops.enable)phys->ops.enable(phys);}if (dpu_enc->misr_enable && (dpu_enc->disp_info.capabilities & MSM_DISPLAY_CAP_VID_MODE) && phys->ops.setup_misr) phys->ops.setup_misr(phys, true, dpu_enc->misr_frame_count); }
if (dpu_enc->cur_master->ops.enable)
dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
There's a change in functionality here. Previously you could call setup_misr for slaves after they are enabled, but before master is enabled. Now you're calling it after all are enabled.
I'm guessing it doesn't matter since it was previously called on master before it was enabled, but I figure I'd point this out.
Sean
_dpu_encoder_virt_enable_helper(drm_enc); }
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project