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@xxxxxxxxxxxxxx>
> ---
> 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)
> continue;
>
> - 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
>
--
Sean Paul, Software Engineer, Google / Chromium OS
