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@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.

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

On Tue, Aug 07, 2018 at 08:12:32PM -0700, Jeykumar Sankaran wrote:

Avoid querying RM for hw mdp block. Use the one stored in KMS during initialization.

changes in v2:

• none

changes in v3:

• none

Change-Id: I52129b96bd561a5547507d7f567bcaa3dbe554aa Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org

Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 12 +----------- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 9 +-------- 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index 3084675..c8c4612 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -823,7 +823,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init( { struct dpu_encoder_phys *phys_enc = NULL; struct dpu_encoder_phys_cmd *cmd_enc = NULL;

• struct dpu_hw_mdp *hw_mdp; struct dpu_encoder_irq *irq; int i, ret = 0;

@@ -836,14 +835,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init( goto fail; } phys_enc = &cmd_enc->base;

• hw_mdp = dpu_rm_get_mdp(&p->dpu_kms->rm);
• if (IS_ERR_OR_NULL(hw_mdp)) {

• ret = PTR_ERR(hw_mdp);
  

• DPU_ERROR("failed to get mdptop\n");
  

• goto fail_mdp_init;
  

• }
• phys_enc->hw_mdptop = hw_mdp;

• phys_enc->hw_mdptop = p->dpu_kms->hw_mdp; phys_enc->intf_idx = p->intf_idx;

  dpu_encoder_phys_cmd_init_ops(&phys_enc->ops);

@@ -898,8 +890,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init(

return phys_enc;

-fail_mdp_init:

• kfree(cmd_enc);

fail: return ERR_PTR(ret); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 14fc7c2..57ece03 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -829,7 +829,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init( struct dpu_encoder_phys *phys_enc = NULL; struct dpu_encoder_phys_vid *vid_enc = NULL; struct dpu_rm_hw_iter iter;

• struct dpu_hw_mdp *hw_mdp; struct dpu_encoder_irq *irq; int i, ret = 0;

@@ -846,13 +845,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init(

phys_enc = &vid_enc->base;

• hw_mdp = dpu_rm_get_mdp(&p->dpu_kms->rm);
• if (IS_ERR_OR_NULL(hw_mdp)) {

• ret = PTR_ERR(hw_mdp);
  

• DPU_ERROR("failed to get mdptop\n");
  

• goto fail;
  

• }
• phys_enc->hw_mdptop = hw_mdp;

• phys_enc->hw_mdptop = p->dpu_kms->hw_mdp; phys_enc->intf_idx = p->intf_idx;

  /**

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, Aug 07, 2018 at 08:12:33PM -0700, Jeykumar Sankaran wrote:

Instead of iterating for hw ctrl per physical encoder, this patch moves the iterations and assignment to the virtual encoder.

changes in v2:

• none

changes in v3:

• none

Change-Id: I896a8c36d6353986582e9d0fe3da9b2293579d4b Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org

---

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 ++++++++++++++++++++-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 19 ------------------- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 19 ------------------- 3 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index a0ced79..7b82e2d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1017,9 +1017,11 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, struct dpu_kms *dpu_kms; struct list_head *connector_list; struct drm_connector *conn = NULL, *conn_iter;

• struct dpu_rm_hw_iter pp_iter;

• struct dpu_rm_hw_iter pp_iter, ctl_iter; struct msm_display_topology topology; enum dpu_rm_topology_name topology_name;
• struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC];

Instead of setting each one to null in the loop:

struct dpu_hw_ctl *hw_ctl[MAX_CHANNELS_PER_ENC] = { NULL };

Extra line

int i = 0, ret;

if (!drm_enc) { @@ -1067,6 +1069,14 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, dpu_enc->hw_pp[i] = (struct dpu_hw_pingpong *) pp_iter.hw; }

• dpu_rm_init_hw_iter(&ctl_iter, drm_enc->base.id, DPU_HW_BLK_CTL);
• for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {

• hw_ctl[i] = NULL;
  

Remove once hw_ctl is initialized properly

• if (!dpu_rm_get_hw(&dpu_kms->rm, &ctl_iter))
  

• 	break;
  

• hw_ctl[i] = (struct dpu_hw_ctl *)ctl_iter.hw;
  

• }
• topology_name = dpu_rm_get_topology_name(topology); for (i = 0; i < dpu_enc->num_phys_encs; i++) { struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];

@@ -1074,10 +1084,18 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, if (phys) { if (!dpu_enc->hw_pp[i]) { DPU_ERROR_ENC(dpu_enc,

• 		    "invalid pingpong block for the encoder\n");
  

• 		    "no pp block assigned at idx: %d\n", i);
  	return;
  }
  phys->hw_pp = dpu_enc->hw_pp[i];
  

• 	if (!hw_ctl[i]) {
  

• 		DPU_ERROR_ENC(dpu_enc,
  

• 		    "no ctl block assigned at idx: %d\n", i);
  

• 		return;
  

• 	}
  

• 	phys->hw_ctl = hw_ctl[i];
  

• phys->connector = conn->state->connector;
  phys->topology_name = topology_name;
  if (phys->ops.mode_set)
  

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index c8c4612..5c89868 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -196,9 +196,6 @@ static void dpu_encoder_phys_cmd_mode_set( { struct dpu_encoder_phys_cmd *cmd_enc = to_dpu_encoder_phys_cmd(phys_enc);

• struct dpu_rm *rm = &phys_enc->dpu_kms->rm;

• struct dpu_rm_hw_iter iter;

• int i, instance;

  if (!phys_enc || !mode || !adj_mode) { DPU_ERROR("invalid args\n");

@@ -208,22 +205,6 @@ static void dpu_encoder_phys_cmd_mode_set( DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n"); drm_mode_debug_printmodeline(adj_mode);

• instance = phys_enc->split_role == ENC_ROLE_SLAVE ? 1 : 0;
• /* Retrieve previously allocated HW Resources. Shouldn't fail */
• dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_CTL);
• for (i = 0; i <= instance; i++) {

• if (dpu_rm_get_hw(rm, &iter))
  

• 	phys_enc->hw_ctl = (struct dpu_hw_ctl *)iter.hw;
  

• }
• if (IS_ERR_OR_NULL(phys_enc->hw_ctl)) {

• DPU_ERROR_CMDENC(cmd_enc, "failed to init ctl: %ld\n",
  

• 		PTR_ERR(phys_enc->hw_ctl));
  

• phys_enc->hw_ctl = NULL;
  

• return;
  

• }
• _dpu_encoder_phys_cmd_setup_irq_hw_idx(phys_enc);

}

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index 57ece03..c0221cc 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -395,9 +395,6 @@ static void dpu_encoder_phys_vid_mode_set( struct drm_display_mode *mode, struct drm_display_mode *adj_mode) {

• struct dpu_rm *rm;

• struct dpu_rm_hw_iter iter;

• int i, instance; struct dpu_encoder_phys_vid *vid_enc;

  if (!phys_enc || !phys_enc->dpu_kms) {

@@ -405,7 +402,6 @@ static void dpu_encoder_phys_vid_mode_set( return; }

• rm = &phys_enc->dpu_kms->rm; vid_enc = to_dpu_encoder_phys_vid(phys_enc);

  if (adj_mode) {

@@ -414,21 +410,6 @@ static void dpu_encoder_phys_vid_mode_set( DPU_DEBUG_VIDENC(vid_enc, "caching mode:\n"); }

• instance = phys_enc->split_role == ENC_ROLE_SLAVE ? 1 : 0;
• /* Retrieve previously allocated HW Resources. Shouldn't fail */
• dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_CTL);
• for (i = 0; i <= instance; i++) {

• if (dpu_rm_get_hw(rm, &iter))
  

• 	phys_enc->hw_ctl = (struct dpu_hw_ctl *)iter.hw;
  

• }
• if (IS_ERR_OR_NULL(phys_enc->hw_ctl)) {

• DPU_ERROR_VIDENC(vid_enc, "failed to init ctl, %ld\n",
  

• 		PTR_ERR(phys_enc->hw_ctl));
  

• phys_enc->hw_ctl = NULL;
  

• return;
  

• }
• _dpu_encoder_phys_vid_setup_irq_hw_idx(phys_enc);

}

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

On Tue, Aug 07, 2018 at 08:12:34PM -0700, Jeykumar Sankaran wrote:

hw intf blocks are needed only during encoder enable to program timing engines(for video panels). encoder->enable is triggered only after atomic_modeset at which point we assign the resources for the display pipeline. This patch defers the hw_intf look-up until encoder enable.

I'm sure there's a good reason for this, but you haven't stated it. As a casual reader of the patch, I'd be inclined to prefer doing work only once at init vs everytime we enable.

changes in v2:

• none

changes in v3:

• none

Change-Id: Ib0a2253431468151355e50cbad7b91e2b77b6e54 Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org

---

.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 53 +++++++--------------- 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c index c0221cc..a0b3744 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c @@ -462,7 +462,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) { struct msm_drm_private *priv; struct dpu_encoder_phys_vid *vid_enc;

• struct dpu_hw_intf *intf;

• struct dpu_rm_hw_iter iter; struct dpu_hw_ctl *ctl; u32 flush_mask = 0;

@@ -474,11 +474,20 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) priv = phys_enc->parent->dev->dev_private;

vid_enc = to_dpu_encoder_phys_vid(phys_enc);

• intf = vid_enc->hw_intf; ctl = phys_enc->hw_ctl;
• if (!vid_enc->hw_intf || !phys_enc->hw_ctl) {

• DPU_ERROR("invalid hw_intf %d hw_ctl %d\n",
  

• 		vid_enc->hw_intf != 0, phys_enc->hw_ctl != 0);
  

• dpu_rm_init_hw_iter(&iter, phys_enc->parent->base.id, DPU_HW_BLK_INTF);
• while (dpu_rm_get_hw(&phys_enc->dpu_kms->rm, &iter)) {

• struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf *)iter.hw;
  

• if (hw_intf->idx == phys_enc->intf_idx) {
  

• 	vid_enc->hw_intf = hw_intf;
  

• 	break;
  

• }
  

• }
• if (!vid_enc->hw_intf) {

• DPU_ERROR("hw_intf not assigned\n");
  

  return; }

@@ -500,7 +509,7 @@ static void dpu_encoder_phys_vid_enable(struct dpu_encoder_phys *phys_enc) !dpu_encoder_phys_vid_is_master(phys_enc)) goto skip_flush;

• ctl->ops.get_bitmask_intf(ctl, &flush_mask, intf->idx);

• ctl->ops.get_bitmask_intf(ctl, &flush_mask, vid_enc->hw_intf->idx); ctl->ops.update_pending_flush(ctl, flush_mask);

skip_flush: @@ -531,22 +540,13 @@ static void dpu_encoder_phys_vid_get_hw_resources( struct dpu_encoder_hw_resources *hw_res, struct drm_connector_state *conn_state) {

• struct dpu_encoder_phys_vid *vid_enc;

• if (!phys_enc || !hw_res) { DPU_ERROR("invalid arg(s), enc %d hw_res %d conn_state %d\n", phys_enc != 0, hw_res != 0, conn_state != 0); return; }

• vid_enc = to_dpu_encoder_phys_vid(phys_enc);

• if (!vid_enc->hw_intf) {

• DPU_ERROR("invalid arg(s), hw_intf\n");
  

• return;
  

• }

• DPU_DEBUG_VIDENC(vid_enc, "\n");

• hw_res->intfs[vid_enc->hw_intf->idx - INTF_0] = INTF_MODE_VIDEO;

• hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;

}

static int _dpu_encoder_phys_vid_wait_for_vblank( @@ -809,7 +809,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init( { struct dpu_encoder_phys *phys_enc = NULL; struct dpu_encoder_phys_vid *vid_enc = NULL;

• struct dpu_rm_hw_iter iter; struct dpu_encoder_irq *irq; int i, ret = 0;

@@ -829,26 +828,6 @@ struct dpu_encoder_phys *dpu_encoder_phys_vid_init( phys_enc->hw_mdptop = p->dpu_kms->hw_mdp; phys_enc->intf_idx = p->intf_idx;

• /**

• * hw_intf resource permanently assigned to this encoder
  

• * Other resources allocated at atomic commit time by use case
  

• */
  

• dpu_rm_init_hw_iter(&iter, 0, DPU_HW_BLK_INTF);

• while (dpu_rm_get_hw(&p->dpu_kms->rm, &iter)) {

• struct dpu_hw_intf *hw_intf = (struct dpu_hw_intf *)iter.hw;
  

• if (hw_intf->idx == p->intf_idx) {
  

• 	vid_enc->hw_intf = hw_intf;
  

• 	break;
  

• }
  

• }

• if (!vid_enc->hw_intf) {

• ret = -EINVAL;
  

• DPU_ERROR("failed to get hw_intf\n");
  

• goto fail;
  

• }

• DPU_DEBUG_VIDENC(vid_enc, "\n");

  dpu_encoder_phys_vid_init_ops(&phys_enc->ops);

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Tue, Aug 07, 2018 at 08:12:35PM -0700, Jeykumar Sankaran wrote:

Prep changes for state based resource management.

Moves all the hw block tracking for the crtc to the state object.

changes in v2:

• none

changes in v3:

• none

Change-Id: I2816e9e28b27f1126b477d62eb3858a30a652747 Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org

---

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77 +++++++++++++++++--------------- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 25 +++++------ 2 files changed, 54 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 1f2d223..515b0e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -136,9 +136,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 drm_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 (!lm_roi || !drm_rect_visible(lm_roi))

@@ -219,7 +219,7 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc *crtc, 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;
  

@@ -242,7 +242,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;

Unrelated var renames are kind of noisy, but I suppose it's just one, so that's ok.

struct dpu_crtc_mixer *mixer; struct dpu_hw_ctl *ctl; struct dpu_hw_mixer *lm; @@ -253,17 +253,17 @@ 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);
  

  return; }

• for (i = 0; i < dpu_crtc->num_mixers; i++) {

• for (i = 0; i < cstate->num_mixers; i++) { if (!mixer[i].hw_lm || !mixer[i].hw_ctl) { DPU_ERROR("invalid lm or ctl assigned to mixer\n"); return;

@@ -280,7 +280,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)

_dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);

• for (i = 0; i < dpu_crtc->num_mixers; i++) {

• for (i = 0; i < cstate->num_mixers; i++) { ctl = mixer[i].hw_ctl; lm = mixer[i].hw_lm;

@@ -502,7 +502,7 @@ static void _dpu_crtc_setup_mixer_for_encoder( struct drm_crtc *crtc, struct drm_encoder *enc) {

• struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);

• struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc); struct dpu_rm *rm = &dpu_kms->rm; struct dpu_crtc_mixer *mixer;

@@ -514,8 +514,8 @@ static void _dpu_crtc_setup_mixer_for_encoder( dpu_rm_init_hw_iter(&ctl_iter, enc->base.id, DPU_HW_BLK_CTL);

/* Set up all the mixers and ctls reserved by this encoder */

• for (i = dpu_crtc->num_mixers; i < ARRAY_SIZE(dpu_crtc->mixers); i++) {

• mixer = &dpu_crtc->mixers[i];
  

• for (i = cstate->num_mixers; i < ARRAY_SIZE(cstate->mixers); i++) {

• mixer = &cstate->mixers[i];
  
  

  if (!dpu_rm_get_hw(rm, &lm_iter)) break;

@@ -540,7 +540,7 @@ static void _dpu_crtc_setup_mixer_for_encoder(

mixer->encoder = enc;

• dpu_crtc->num_mixers++;
  

• cstate->num_mixers++;
  

  DPU_DEBUG("setup mixer %d: lm %d\n", i, mixer->hw_lm->idx - LM_0); DPU_DEBUG("setup mixer %d: ctl %d\n",

@@ -551,11 +551,11 @@ static void _dpu_crtc_setup_mixer_for_encoder( static void _dpu_crtc_setup_mixers(struct drm_crtc *crtc) { struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);

• struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state); struct drm_encoder *enc;

• dpu_crtc->num_mixers = 0;
• dpu_crtc->mixers_swapped = false;
• memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));

• cstate->num_mixers = 0;

• memset(cstate->mixers, 0, sizeof(cstate->mixers));

  mutex_lock(&dpu_crtc->crtc_lock); /* Check for mixers on all encoders attached to this crtc */

@@ -589,7 +589,7 @@ static void _dpu_crtc_setup_lm_bounds(struct drm_crtc *crtc, adj_mode = &state->adjusted_mode; crtc_split_width = dpu_crtc_get_mixer_width(dpu_crtc, cstate, adj_mode);

• for (i = 0; i < dpu_crtc->num_mixers; i++) {

• for (i = 0; i < cstate->num_mixers; i++) { struct drm_rect *r = &cstate->lm_bounds[i]; r->x1 = crtc_split_width * i; r->y1 = 0;

@@ -606,6 +606,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_crtc_state *old_state) { struct dpu_crtc *dpu_crtc;

• struct dpu_crtc_state *cstate; struct drm_encoder *encoder; struct drm_device *dev; unsigned long flags;

@@ -625,10 +626,11 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, DPU_DEBUG("crtc%d\n", crtc->base.id);

dpu_crtc = to_dpu_crtc(crtc);

• cstate = to_dpu_crtc_state(crtc->state); dev = crtc->dev; smmu_state = &dpu_crtc->smmu_state;

• if (!dpu_crtc->num_mixers) {

• if (!cstate->num_mixers) { _dpu_crtc_setup_mixers(crtc); _dpu_crtc_setup_lm_bounds(crtc, crtc->state); }

@@ -655,7 +657,7 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, * it means we are trying to flush a CRTC whose state is disabled: * nothing else needs to be done. */

• if (unlikely(!dpu_crtc->num_mixers))

• if (unlikely(!cstate->num_mixers)) return;

  _dpu_crtc_blend_setup(crtc);

@@ -719,7 +721,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc, * it means we are trying to flush a CRTC whose state is disabled: * nothing else needs to be done. */

• if (unlikely(!dpu_crtc->num_mixers))

• if (unlikely(!cstate->num_mixers)) return;

  /*

@@ -834,7 +836,7 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) * it means we are trying to start a CRTC whose state is disabled: * nothing else needs to be done. */

• if (unlikely(!dpu_crtc->num_mixers))

• if (unlikely(!cstate->num_mixers)) return;

  DPU_ATRACE_BEGIN("crtc_commit");

@@ -1069,6 +1071,7 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) struct dpu_crtc *dpu_crtc; struct drm_encoder *encoder; struct dpu_crtc_mixer *m;

• struct dpu_crtc_state *cstate; u32 i, misr_status;

  if (!crtc) {

@@ -1076,6 +1079,7 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) return; } dpu_crtc = to_dpu_crtc(crtc);

• cstate = to_dpu_crtc_state(dpu_crtc->base.state);

  mutex_lock(&dpu_crtc->crtc_lock);

@@ -1091,8 +1095,8 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) dpu_encoder_virt_restore(encoder); }

• for (i = 0; i < dpu_crtc->num_mixers; ++i) {
  

• 	m = &dpu_crtc->mixers[i];
  

• for (i = 0; i < cstate->num_mixers; ++i) {
  

• 	m = &cstate->mixers[i];
  if (!m->hw_lm || !m->hw_lm->ops.setup_misr ||
  		!dpu_crtc->misr_enable)
  	continue;
  

@@ -1102,8 +1106,8 @@ static void dpu_crtc_handle_power_event(u32 event_type, void *arg) } break; case DPU_POWER_EVENT_PRE_DISABLE:

• for (i = 0; i < dpu_crtc->num_mixers; ++i) {
  

• 	m = &dpu_crtc->mixers[i];
  

• for (i = 0; i < cstate->num_mixers; ++i) {
  

• 	m = &cstate->mixers[i];
  if (!m->hw_lm || !m->hw_lm->ops.collect_misr ||
  		!dpu_crtc->misr_enable)
  	continue;
  

@@ -1191,9 +1195,8 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) dpu_power_handle_unregister_event(dpu_crtc->phandle, dpu_crtc->power_event);

• memset(dpu_crtc->mixers, 0, sizeof(dpu_crtc->mixers));
• dpu_crtc->num_mixers = 0;
• dpu_crtc->mixers_swapped = false;

• memset(cstate->mixers, 0, sizeof(cstate->mixers));

• cstate->num_mixers = 0;

  /* disable clk & bw control until clk & bw properties are set */ cstate->bw_control = false;

@@ -1552,8 +1555,8 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data)

seq_puts(s, "\n");

• for (i = 0; i < dpu_crtc->num_mixers; ++i) {

• m = &dpu_crtc->mixers[i];
  

• for (i = 0; i < cstate->num_mixers; ++i) {

• m = &cstate->mixers[i];
  

Hmmm, the state accesses in this function aren't properly serialized. Could you please whip up a patch to acquire the modeset locks?

if (!m->hw_lm)
	seq_printf(s, "\tmixer[%d] has no lm\n", i);
else if (!m->hw_ctl)

@@ -1646,6 +1649,7 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file, const char __user *user_buf, size_t count, loff_t *ppos) { struct dpu_crtc *dpu_crtc;

• struct dpu_crtc_state *cstate; struct dpu_crtc_mixer *m; int i = 0, rc; char buf[MISR_BUFF_SIZE + 1];

@@ -1656,6 +1660,7 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file, return -EINVAL;

dpu_crtc = file->private_data;

• cstate = to_dpu_crtc_state(dpu_crtc->base.state);

Same here

buff_copy = min_t(size_t, count, MISR_BUFF_SIZE); if (copy_from_user(buf, user_buf, buff_copy)) { DPU_ERROR("buffer copy failed\n"); @@ -1674,9 +1679,9 @@ static ssize_t _dpu_crtc_misr_setup(struct file *file, mutex_lock(&dpu_crtc->crtc_lock); dpu_crtc->misr_enable = enable; dpu_crtc->misr_frame_count = frame_count;

• for (i = 0; i < dpu_crtc->num_mixers; ++i) {

• for (i = 0; i < cstate->num_mixers; ++i) { dpu_crtc->misr_data[i] = 0;

• m = &dpu_crtc->mixers[i];
  

• m = &cstate->mixers[i];
  

  if (!m->hw_lm || !m->hw_lm->ops.setup_misr) continue;

@@ -1692,6 +1697,7 @@ static ssize_t _dpu_crtc_misr_read(struct file *file, char __user *user_buff, size_t count, loff_t *ppos) { struct dpu_crtc *dpu_crtc;

• struct dpu_crtc_state *cstate; struct dpu_crtc_mixer *m; int i = 0, rc; u32 misr_status;

@@ -1705,6 +1711,7 @@ static ssize_t _dpu_crtc_misr_read(struct file *file, return -EINVAL;

dpu_crtc = file->private_data;

• cstate = to_dpu_crtc_state(dpu_crtc->base.state);

And here

rc = _dpu_crtc_power_enable(dpu_crtc, true); if (rc) return rc; @@ -1716,8 +1723,8 @@ static ssize_t _dpu_crtc_misr_read(struct file *file, goto buff_check; }

• for (i = 0; i < dpu_crtc->num_mixers; ++i) {

• m = &dpu_crtc->mixers[i];
  

• for (i = 0; i < cstate->num_mixers; ++i) {

• m = &cstate->mixers[i];
  

  if (!m->hw_lm || !m->hw_lm->ops.collect_misr) continue;

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index e632651..9177ee6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -121,11 +121,6 @@ struct dpu_crtc_frame_event {

• struct dpu_crtc - virtualized CRTC data structure
• @base : Base drm crtc structure
• @name : ASCII description of this crtc

• • @num_ctls : Number of ctl paths in use
• • @num_mixers : Number of mixers in use
• • @mixers_swapped: Whether the mixers have been swapped for left/right update

• •              especially in the case of DSC Merge.
    

• • @mixers : List of active mixers
  • @event : Pointer to last received drm vblank event. If there is a

  •              pending vblank event, this will be non-null.
    

  • @vsync_count : Running count of received vsync events

@@ -167,12 +162,6 @@ struct dpu_crtc { struct drm_crtc base; char name[DPU_CRTC_NAME_SIZE];

• /* HW Resources reserved for the crtc */
• u32 num_ctls;
• u32 num_mixers;
• bool mixers_swapped;
• struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
• struct drm_pending_vblank_event *event; u32 vsync_count;

@@ -227,6 +216,10 @@ struct dpu_crtc {

• @property_values: Current crtc property values
• @input_fence_timeout_ns : Cached input fence timeout, in ns
• @new_perf: new performance state being requested

• • @num_mixers : Number of mixers in use
• • @mixers : List of active mixers
• • @num_ctls : Number of ctl paths in use
• • @hw_ctls : List of activel ctl paths

activel

*/ struct dpu_crtc_state { struct drm_crtc_state base; @@ -236,8 +229,14 @@ struct dpu_crtc_state { struct drm_rect lm_bounds[CRTC_DUAL_MIXERS];

uint64_t input_fence_timeout_ns;

unrelated whitespace

struct dpu_core_perf_params new_perf;

• /* HW Resources reserved for the crtc */
• u32 num_mixers;
• struct dpu_crtc_mixer mixers[CRTC_DUAL_MIXERS];
• u32 num_ctls;
• struct dpu_hw_ctl *hw_ctls[CRTC_DUAL_MIXERS];

};

#define to_dpu_crtc_state(x) \ @@ -255,7 +254,7 @@ static inline int dpu_crtc_get_mixer_width(struct dpu_crtc *dpu_crtc, if (!dpu_crtc || !cstate || !mode) return 0;

• mixer_width = (dpu_crtc->num_mixers == CRTC_DUAL_MIXERS ?

• mixer_width = (cstate->num_mixers == CRTC_DUAL_MIXERS ? mode->hdisplay / CRTC_DUAL_MIXERS : mode->hdisplay);

Can you please add a new patch that precedes this to move this function into dpu_crtc as a static function?

Sean

return mixer_width;

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Tue, Aug 07, 2018 at 08:12:36PM -0700, Jeykumar Sankaran wrote:

Prep change for state based resource management.

Rename hw_ctl to lm_ctl to mean the ctl associated with the hw layer mixer block.

Did you do this via spatch, sed, etc? Rename patches should contain the invocation to reproduce them since they have a nasty habit of introducing bugs and compilation warnings/errors.

Sean

changes in v2:

• none

changes in v3:

• none

Change-Id: If6e6249e089b89225cdfafe9158f66667509e97b Signed-off-by: Jeykumar Sankaran jsanka@codeaurora.org

---

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 26 +++++++++++++------------- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 4 ++-- 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 515b0e6..0eb369c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -175,7 +175,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);

@@ -264,15 +264,15 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) }

for (i = 0; i < cstate->num_mixers; i++) {

• if (!mixer[i].hw_lm || !mixer[i].hw_ctl) {
  

• if (!mixer[i].hw_lm || !mixer[i].lm_ctl) {
  DPU_ERROR("invalid lm or ctl assigned to mixer\n");
  return;
  

  } mixer[i].mixer_op_mode = 0; mixer[i].flush_mask = 0;

• if (mixer[i].hw_ctl->ops.clear_all_blendstages)
  

• 	mixer[i].hw_ctl->ops.clear_all_blendstages(
  

• 			mixer[i].hw_ctl);
  

• if (mixer[i].lm_ctl->ops.clear_all_blendstages)
  

• 	mixer[i].lm_ctl->ops.clear_all_blendstages(
  

• 			mixer[i].lm_ctl);
  

  }

  /* initialize stage cfg */

@@ -281,7 +281,7 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc) _dpu_crtc_blend_setup_mixer(crtc, dpu_crtc, mixer);

for (i = 0; i < cstate->num_mixers; i++) {

• ctl = mixer[i].hw_ctl;
  

• ctl = mixer[i].lm_ctl;
  

  lm = mixer[i].hw_lm;

  lm->ops.setup_alpha_out(lm, mixer[i].mixer_op_mode);

@@ -525,14 +525,14 @@ static void _dpu_crtc_setup_mixer_for_encoder( if (!dpu_rm_get_hw(rm, &ctl_iter)) { DPU_DEBUG("no ctl assigned to lm %d, using previous\n", mixer->hw_lm->idx - LM_0);

• 	mixer->hw_ctl = last_valid_ctl;
  

• 	mixer->lm_ctl = last_valid_ctl;
  

  } else {

• 	mixer->hw_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
  

• 	last_valid_ctl = mixer->hw_ctl;
  

• 	mixer->lm_ctl = (struct dpu_hw_ctl *)ctl_iter.hw;
  

• 	last_valid_ctl = mixer->lm_ctl;
  

  }

  /* Shouldn't happen, mixers are always >= ctls */

• if (!mixer->hw_ctl) {
  

• if (!mixer->lm_ctl) {
  DPU_ERROR("no valid ctls found for lm %d\n",
  		mixer->hw_lm->idx - LM_0);
  return;
  

@@ -544,7 +544,7 @@ static void _dpu_crtc_setup_mixer_for_encoder( DPU_DEBUG("setup mixer %d: lm %d\n", i, mixer->hw_lm->idx - LM_0); DPU_DEBUG("setup mixer %d: ctl %d\n",

• 		i, mixer->hw_ctl->idx - CTL_0);
  

• 		i, mixer->lm_ctl->idx - CTL_0);
  

  }

}

@@ -1559,11 +1559,11 @@ static int _dpu_debugfs_status_show(struct seq_file *s, void *data) m = &cstate->mixers[i]; if (!m->hw_lm) seq_printf(s, "\tmixer[%d] has no lm\n", i);

• else if (!m->hw_ctl)
  

• else if (!m->lm_ctl)
  seq_printf(s, "\tmixer[%d] has no ctl\n", i);
  

  else seq_printf(s, "\tmixer:%d ctl:%d width:%d height:%d\n",

• 		m->hw_lm->idx - LM_0, m->hw_ctl->idx - CTL_0,
  

• 		m->hw_lm->idx - LM_0, m->lm_ctl->idx - CTL_0,
  	out_width, mode->vdisplay);
  

  }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 9177ee6..5b85ca8 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -83,14 +83,14 @@ struct dpu_crtc_smmu_state_data { /**

• struct dpu_crtc_mixer: stores the map for each virtual pipeline in the CRTC
• @hw_lm: LM HW Driver context

• • @hw_ctl: CTL Path HW driver context

• • @lm_ctl: CTL Path HW driver context
  • @encoder: Encoder attached to this lm & ctl
  • @mixer_op_mode: mixer blending operation mode
  • @flush_mask: mixer flush mask for ctl, mixer and pipe
  */

struct dpu_crtc_mixer { struct dpu_hw_mixer *hw_lm;

• struct dpu_hw_ctl *hw_ctl;

• struct dpu_hw_ctl *lm_ctl; struct drm_encoder *encoder; u32 mixer_op_mode; u32 flush_mask;

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

-- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html