On 2018-08-14 12:19, Sean Paul wrote:
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.
Checking slave encoder below.
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.
Isn't that a fact? Do we have a topology in DPU where we have more than
one master or slave?
It seems like you could avoid all of this work if you just did the
assignment in
dpu_encoder_virt_add_phys_encs().
That is true! Let me try doing that.
}
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);
+
We are checking for slave encoder being NULL here.
+ 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
Yes. It doesn't matter here.
_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
--
Jeykumar S
