On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:

Identify slave-master encoders during initialization and enable the encoders explicitly as the current logic has redundant and ambiguous loops.

Please include a "Changes in vX" section so I don't need to figure it out myself.

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

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46 ++++++++++------------------- 1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 991b22c..b223bd2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -180,6 +180,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; struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];

  bool intfs_swapped;

@@ -1141,7 +1142,8 @@ 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;

• int i, ret = 0;

• struct dpu_encoder_phys *phys = NULL;

• int ret = 0; struct drm_display_mode *cur_mode = NULL;

  if (!drm_enc) {

@@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay, cur_mode->vdisplay);

• dpu_enc->cur_master = NULL;
• for (i = 0; i < dpu_enc->num_phys_encs; i++) {

• struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
  

• /* always enable slave encoder before master */
• phys = dpu_enc->cur_slave;

I think this variable makes things less readable. It made sense in the loop since you're indented an extra level and they're obfuscated anyways, but since they're explicit now, could you please just use dpu_enc->cur_slave/master directly?

With this nit and the added changelog in the commit addressed,

Reviewed-by: Sean Paul seanpaul@chromium.org

• if (phys && phys->ops.enable)

• phys->ops.enable(phys);
  
  

• if (phys && phys->ops.is_master && phys->ops.is_master(phys)) {
  

• 	DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n", i);
  

• 	dpu_enc->cur_master = phys;
  

• 	break;
  

• }
  

• }
• if (!dpu_enc->cur_master) {

• DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
  

• return;
  

• }

• 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) {

@@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) return; }

• for (i = 0; i < dpu_enc->num_phys_encs; i++) {

• struct dpu_encoder_phys *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->cur_master->ops.enable)

• dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
  

• _dpu_encoder_virt_enable_helper(drm_enc);

}

@@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs( ++dpu_enc->num_phys_encs; }

• if (params->split_role == ENC_ROLE_SLAVE)

• dpu_enc->cur_slave = enc;
  

• else

• dpu_enc->cur_master = enc;
  

• return 0;

}

@@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc, if (ret) goto fail;

• dpu_enc->cur_master = NULL; spin_lock_init(&dpu_enc->enc_spinlock);

  atomic_set(&dpu_enc->frame_done_timeout, 0);

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

On Fri, Aug 31, 2018 at 12:16:46PM -0700, Jeykumar Sankaran wrote:

On 2018-08-30 09:24, Sean Paul wrote:

...

On Tue, Aug 28, 2018 at 05:39:54PM -0700, Jeykumar Sankaran wrote:

...

Identify slave-master encoders during initialization and enable the encoders explicitly as the current logic has redundant and ambiguous loops.

Please include a "Changes in vX" section so I don't need to figure it out myself.

Since I split these clean up changes into a new series, I was not sure how to handle the versioning. With "changes in v4" log, Should I also change the prefix labeling to [PATCH v4 05/14]? A couple of changes are also introduced in this series.

Yeah, sometimes it's best just to choose an arbitrarily high version if multiple series' are coming together. In this case it's more simple since we're just adding patches to the series, so v4 would have worked. As far as the new patches, just add:

Changed in v4: - Introduced patch (split from "blah blah") or Changed in v4: - Introduced patch (suggested by Sean)

Sean

Thanks, Jeykumar S.

...

...

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

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 46

++++++++++-------------------

...

1 file changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

...

index 991b22c..b223bd2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -180,6 +180,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; struct dpu_hw_pingpong *hw_pp[MAX_CHANNELS_PER_ENC];

  bool intfs_swapped;

@@ -1141,7 +1142,8 @@ 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;

• int i, ret = 0;

• struct dpu_encoder_phys *phys = NULL;

• int ret = 0; struct drm_display_mode *cur_mode = NULL;

  if (!drm_enc) {

@@ -1154,21 +1156,14 @@ static void dpu_encoder_virt_enable(struct

drm_encoder *drm_enc)

...

trace_dpu_enc_enable(DRMID(drm_enc), cur_mode->hdisplay, cur_mode->vdisplay);

• dpu_enc->cur_master = NULL;
• for (i = 0; i < dpu_enc->num_phys_encs; i++) {

• struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
  

• /* always enable slave encoder before master */
• phys = dpu_enc->cur_slave;

I think this variable makes things less readable. It made sense in the loop since you're indented an extra level and they're obfuscated anyways, but since they're explicit now, could you please just use dpu_enc->cur_slave/master directly?

With this nit and the added changelog in the commit addressed,

Reviewed-by: Sean Paul seanpaul@chromium.org

...

• if (phys && phys->ops.enable)

• phys->ops.enable(phys);
  
  

• if (phys && phys->ops.is_master &&
  

phys->ops.is_master(phys)) {

...

• 	DPU_DEBUG_ENC(dpu_enc, "master is now idx %d\n",
  

i);

...

• 	dpu_enc->cur_master = phys;
  

• 	break;
  

• }
  

• }
• if (!dpu_enc->cur_master) {

• DPU_ERROR("virt encoder has no master! num_phys %d\n", i);
  

• return;
  

• }

• 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) { @@ -1177,21 +1172,6 @@ static void dpu_encoder_virt_enable(struct

drm_encoder *drm_enc)

...

return;

}

• for (i = 0; i < dpu_enc->num_phys_encs; i++) {

• struct dpu_encoder_phys *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->cur_master->ops.enable)

• dpu_enc->cur_master->ops.enable(dpu_enc->cur_master);
  

• _dpu_encoder_virt_enable_helper(drm_enc);

}

@@ -2062,6 +2042,11 @@ static int dpu_encoder_virt_add_phys_encs( ++dpu_enc->num_phys_encs; }

• if (params->split_role == ENC_ROLE_SLAVE)

• dpu_enc->cur_slave = enc;
  

• else

• dpu_enc->cur_master = enc;
  

• return 0;

}

@@ -2228,7 +2213,6 @@ int dpu_encoder_setup(struct drm_device *dev,

struct drm_encoder *enc,

...

if (ret) goto fail;

• dpu_enc->cur_master = NULL; spin_lock_init(&dpu_enc->enc_spinlock);

  atomic_set(&dpu_enc->frame_done_timeout, 0);

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

-- Jeykumar S

On 2018-08-31 07:56, Sean Paul wrote:

On Tue, Aug 28, 2018 at 05:39:59PM -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 v4...

...

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

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60

++++++++++++++++++--------------

...

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++++++------ 2 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

...

index e061847..7ab320d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -163,9 +163,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))

@@ -246,7 +246,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, format);
  
  

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

@@ -281,17 +281,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) {

This is not possible.

...

• 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;

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

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

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

@@ -568,7 +568,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",

@@ -579,11 +579,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));

Why is this necessary?

...

mutex_lock(&dpu_crtc->crtc_lock); /* Check for mixers on all encoders attached to this crtc */ @@ -618,7 +618,7 @@ static void _dpu_crtc_setup_lm_bounds(struct

drm_crtc *crtc,

...

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;

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

@@ -654,10 +655,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); }

@@ -684,7 +686,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);

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

  /*

@@ -863,7 +865,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");

@@ -1097,12 +1099,14 @@ static void dpu_crtc_handle_power_event(u32

event_type, void *arg)

...

struct drm_crtc *crtc = arg; struct dpu_crtc *dpu_crtc; struct drm_encoder *encoder;

• struct dpu_crtc_state *cstate;

  if (!crtc) { DPU_ERROR("invalid crtc\n"); return; } dpu_crtc = to_dpu_crtc(crtc);

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

  mutex_lock(&dpu_crtc->crtc_lock);

@@ -1197,9 +1201,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));

Same question here, why is this necessary?

...

• cstate->num_mixers = 0;

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

@@ -1547,6 +1550,8 @@ static int _dpu_debugfs_status_show(struct

seq_file *s, void *data)

...

dpu_crtc = s->private; crtc = &dpu_crtc->base;

• drm_modeset_lock(&crtc->mutex, NULL);

When I suggested this, I was hoping you'd put it in a separate patch. Additionally, you'll probably want modeset_lock_all instead.

crtc state retrieval is introduced in this patch. So I thought it would make sense to add the locking as well a part of this patch.

...

cstate = to_dpu_crtc_state(crtc->state);

mutex_lock(&dpu_crtc->crtc_lock); @@ -1558,8 +1563,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];
  

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

@@ -1639,6 +1644,7 @@ static int _dpu_debugfs_status_show(struct

seq_file *s, void *data)

...

seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);

mutex_unlock(&dpu_crtc->crtc_lock);

• drm_modeset_unlock(&crtc->mutex);

  return 0;

} diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h

b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h

...

index 5e4dc5c..7aa772f 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

@@ -164,12 +159,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;

@@ -221,6 +210,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 active ctl paths
  */

struct dpu_crtc_state { struct drm_crtc_state base; @@ -232,6 +225,13 @@ struct dpu_crtc_state { uint64_t input_fence_timeout_ns;

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) \

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

On 2018-08-31 07:56, Sean Paul wrote:

On Tue, Aug 28, 2018 at 05:39:59PM -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 v4...

...

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

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 60

++++++++++++++++++--------------

...

drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 22 ++++++------ 2 files changed, 44 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c

...

index e061847..7ab320d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -163,9 +163,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))

@@ -246,7 +246,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, format);
  
  

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

@@ -281,17 +281,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) {

This is not possible.

...

• 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;

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

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

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

@@ -568,7 +568,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",

@@ -579,11 +579,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));

Why is this necessary?

...

mutex_lock(&dpu_crtc->crtc_lock); /* Check for mixers on all encoders attached to this crtc */ @@ -618,7 +618,7 @@ static void _dpu_crtc_setup_lm_bounds(struct

drm_crtc *crtc,

...

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;

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

@@ -654,10 +655,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); }

@@ -684,7 +686,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);

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

  /*

@@ -863,7 +865,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");

@@ -1097,12 +1099,14 @@ static void dpu_crtc_handle_power_event(u32

event_type, void *arg)

...

struct drm_crtc *crtc = arg; struct dpu_crtc *dpu_crtc; struct drm_encoder *encoder;

• struct dpu_crtc_state *cstate;

  if (!crtc) { DPU_ERROR("invalid crtc\n"); return; } dpu_crtc = to_dpu_crtc(crtc);

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

  mutex_lock(&dpu_crtc->crtc_lock);

@@ -1197,9 +1201,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));

Same question here, why is this necessary?

Analysing more on the need for this memset. After a crtc is disabled, it can also be used with a mode where the needed no. of layer mixers can be different than the current one. So isn't it always safe to clear the stale pointers to hw blocks?

Thanks and Regards, Jeykumar S.

...

• cstate->num_mixers = 0;

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

@@ -1547,6 +1550,8 @@ static int _dpu_debugfs_status_show(struct

seq_file *s, void *data)

...

dpu_crtc = s->private; crtc = &dpu_crtc->base;

• drm_modeset_lock(&crtc->mutex, NULL);

When I suggested this, I was hoping you'd put it in a separate patch. Additionally, you'll probably want modeset_lock_all instead.

...

cstate = to_dpu_crtc_state(crtc->state);

mutex_lock(&dpu_crtc->crtc_lock); @@ -1558,8 +1563,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];
  

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

@@ -1639,6 +1644,7 @@ static int _dpu_debugfs_status_show(struct

seq_file *s, void *data)

...

seq_printf(s, "vblank_enable:%d\n", dpu_crtc->vblank_requested);

mutex_unlock(&dpu_crtc->crtc_lock);

• drm_modeset_unlock(&crtc->mutex);

  return 0;

} diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h

b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h

...

index 5e4dc5c..7aa772f 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

@@ -164,12 +159,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;

@@ -221,6 +210,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 active ctl paths
  */

struct dpu_crtc_state { struct drm_crtc_state base; @@ -232,6 +225,13 @@ struct dpu_crtc_state { uint64_t input_fence_timeout_ns;

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) \

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

On Tue, Aug 28, 2018 at 05:40:00PM -0700, Jeykumar Sankaran wrote:

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

sed -i 's/([*@.>])hw_ctl([^s])/\1lm_ctl\2/g' dpu_crtc.c dpu_crtc.h

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

Reviewed-by: Sean Paul seanpaul@chromium.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 7ab320d..aeb0f1a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -202,7 +202,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);

@@ -292,15 +292,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 */

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

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

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

  }

}

@@ -1567,11 +1567,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 7aa772f..9b1056c 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

On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:

Strip down the support for topology enums. It can be replaced with simple hw count checks.

Changes in v4: ...

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

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 ++++-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 36 ++++++---------------- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24 --------------- 6 files changed, 19 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index dbf669e..56ef349 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, struct drm_connector *conn = NULL, *conn_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] = { NULL }; int i = 0, ret;

@@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, 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];

@@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc, phys->hw_ctl = hw_ctl[i];

	phys->connector = conn->state->connector;

• 	phys->topology_name = topology_name;
  if (phys->ops.mode_set)
  	phys->ops.mode_set(phys, mode, adj_mode);
  

  }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 60f809f..c5600e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -35,7 +35,6 @@

• @needs_cdm: Encoder requests a CDM based on pixel format conversion needs
• @display_num_of_h_tiles: Number of horizontal tiles in case of split

•                      interface
  

• • @topology: Topology of the display
  */

struct dpu_encoder_hw_resources { enum dpu_intf_mode intfs[INTF_MAX]; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index b3917e0..b5fc65c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -24,6 +24,7 @@ #include "dpu_hw_top.h" #include "dpu_hw_cdm.h" #include "dpu_encoder.h" +#include "dpu_crtc.h"

#define DPU_ENCODER_NAME_MAX 16

@@ -213,7 +214,6 @@ struct dpu_encoder_irq {

• @split_role: Role to play in a split-panel configuration
• @intf_mode: Interface mode
• @intf_idx: Interface index on dpu hardware

• • @topology_name: topology selected for the display
  • @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes
  • @enable_state: Enable state tracking
  • @vblank_refcount: Reference count of vblank request

@@ -243,7 +243,6 @@ struct dpu_encoder_phys { enum dpu_enc_split_role split_role; enum dpu_intf_mode intf_mode; enum dpu_intf intf_idx;

• enum dpu_rm_topology_name topology_name; spinlock_t *enc_spinlock; enum dpu_enc_enable_state enable_state; atomic_t vblank_refcount;

@@ -361,11 +360,15 @@ struct dpu_encoder_phys *dpu_encoder_phys_cmd_init( static inline enum dpu_3d_blend_mode dpu_encoder_helper_get_3d_blend_mode( struct dpu_encoder_phys *phys_enc) {

• struct dpu_crtc_state *dpu_cstate;

• if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING) return BLEND_3D_NONE;

• dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);

• if (phys_enc->split_role == ENC_ROLE_SOLO &&

•    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
  

•    (dpu_cstate->num_mixers == 2))
  

I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a helper called dpu_crtc_state_is_stereo() instead of littering these checks everywhere.

return BLEND_3D_H_ROW_INT;

return BLEND_3D_NONE; 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 6de13f4..1643e2f 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 @@ -355,13 +355,14 @@ static void dpu_encoder_phys_vid_underrun_irq(void *arg, int irq_idx)

static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys *phys_enc) {

• struct dpu_crtc_state *dpu_cstate;
• if (!phys_enc) return false;

• if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)

• return true;
  

• dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);

• return false;

• return (dpu_cstate->num_ctls > 1);

Unnecessary parens

}

static bool dpu_encoder_phys_vid_needs_single_flush( diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 13c0a36..fbd489f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -33,7 +33,6 @@ (t).num_intf == (r).num_intf)

struct dpu_rm_topology_def {

• enum dpu_rm_topology_name top_name; int num_lm; int num_comp_enc; int num_intf;

@@ -42,10 +41,10 @@ struct dpu_rm_topology_def { };

static const struct dpu_rm_topology_def g_top_table[] = {

• { DPU_RM_TOPOLOGY_NONE, 0, 0, 0, 0, false },
• { DPU_RM_TOPOLOGY_SINGLEPIPE, 1, 0, 1, 1, false },
• { DPU_RM_TOPOLOGY_DUALPIPE, 2, 0, 2, 2, true },
• { DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE, 2, 0, 1, 1, false },

• { 0, 0, 0, 0, false },
• { 1, 0, 1, 1, false },
• { 2, 0, 2, 2, true },
• { 2, 0, 1, 1, false },

You don't need this. The data it provides can be deduced already: num_ctl == num_intf needs_split_display == (num_intf > 1)

};

/** @@ -72,13 +71,11 @@ struct dpu_rm_requirements {

• @enc_id: Reservations are tracked by Encoder DRM object ID.

• CRTCs may be connected to multiple Encoders.
  

• An encoder or connector id identifies the display path.
  

• • @topology DRM<->HW topology use case
  */

struct dpu_rm_rsvp { struct list_head list; uint32_t seq; uint32_t enc_id;

• enum dpu_rm_topology_name topology;

};

/** @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps( DPU_DEBUG("%d\n", stage);

list_for_each_entry(rsvp, &rm->rsvps, list) {

• DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage, rsvp->seq,
  

• 	      rsvp->enc_id, rsvp->topology);
  

• DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
  

• 	      rsvp->enc_id);
  

  }

  for (type = 0; type < DPU_HW_BLK_MAX; type++) {

@@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm *rm) return rm->hw_mdp; }

-enum dpu_rm_topology_name -dpu_rm_get_topology_name(struct msm_display_topology topology) -{

• int i;
• for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)

• if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
  

• 	return g_top_table[i].top_name;
  

• return DPU_RM_TOPOLOGY_NONE;

-}

void dpu_rm_init_hw_iter( struct dpu_rm_hw_iter *iter, uint32_t enc_id, @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp( /* Create reservation info, tag reserved blocks with it as we go */ rsvp->seq = ++rm->rsvp_next_seq; rsvp->enc_id = enc->base.id;

• rsvp->topology = reqs->topology->top_name; list_add_tail(&rsvp->list, &rm->rsvps);

  ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);

@@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(

dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);

• for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {

• for (i = 0; i < ARRAY_SIZE(g_top_table); i++) { if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], req_topology)) { reqs->topology = &g_top_table[i];

Since you can get rid of g_top_table, you can get rid of reqs->topology too.

@@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(

DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n", reqs->top_ctrl, reqs->hw_res.display_num_of_h_tiles);

• DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display: %d\n",

• DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n", reqs->topology->num_lm, reqs->topology->num_ctl,

•       reqs->topology->top_name,
   reqs->topology->needs_split_display);
  
  

  return 0;

@@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp( }

if (!ret)

• DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
  

• 	      rsvp->topology);
  

• DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
  

This debug message doesn't provide any value any longer.

return ret; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h index ffd1841..6fd9e7b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h @@ -21,21 +21,6 @@ #include "dpu_hw_top.h"

/**

• • enum dpu_rm_topology_name - HW resource use case in use by connector
• • @DPU_RM_TOPOLOGY_NONE: No topology in use currently
• • @DPU_RM_TOPOLOGY_SINGLEPIPE: 1 LM, 1 PP, 1 INTF/WB
• • @DPU_RM_TOPOLOGY_DUALPIPE: 2 LM, 2 PP, 2 INTF/WB
• • @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE: 2 LM, 2 PP, 3DMux, 1 INTF/WB
• */

-enum dpu_rm_topology_name {

• DPU_RM_TOPOLOGY_NONE = 0,
• DPU_RM_TOPOLOGY_SINGLEPIPE,
• DPU_RM_TOPOLOGY_DUALPIPE,
• DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
• DPU_RM_TOPOLOGY_MAX,

-};

-/**

• enum dpu_rm_topology_control - HW resource use case in use by connector
• @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a successful

•                          test, reserve the resources for this display.
  

@@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter( */ int dpu_rm_check_property_topctl(uint64_t val);

-/**

• • dpu_rm_get_topology_name - returns the name of the the given topology

• •                        definition
    

• • @topology: topology definition
• • @Return: name of the topology
• */

-enum dpu_rm_topology_name -dpu_rm_get_topology_name(struct msm_display_topology topology);

#endif /* __DPU_RM_H__ */

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

On Tue, Sep 04, 2018 at 04:03:25PM -0700, Jeykumar Sankaran wrote:

On 2018-08-31 09:08, Sean Paul wrote:

...

On Tue, Aug 28, 2018 at 05:40:01PM -0700, Jeykumar Sankaran wrote:

...

Strip down the support for topology enums. It can be replaced with simple hw count checks.

Changes in v4: ...

...

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

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 9 ++++-- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 7 +++-- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 36

++++++----------------

...

drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 24

6 files changed, 19 insertions(+), 61 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

...

index dbf669e..56ef349 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1013,7 +1013,6 @@ static void dpu_encoder_virt_mode_set(struct

drm_encoder *drm_enc,

...

struct drm_connector *conn = NULL, *conn_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] = { NULL }; int i = 0, ret;

@@ -1069,7 +1068,6 @@ static void dpu_encoder_virt_mode_set(struct

drm_encoder *drm_enc,

...

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];

@@ -1089,7 +1087,6 @@ static void dpu_encoder_virt_mode_set(struct

drm_encoder *drm_enc,

...

	phys->hw_ctl = hw_ctl[i];

	phys->connector = conn->state->connector;

• 	phys->topology_name = topology_name;
  if (phys->ops.mode_set)
  	phys->ops.mode_set(phys, mode, adj_mode);
  

  }

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h

...

index 60f809f..c5600e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -35,7 +35,6 @@

• @needs_cdm: Encoder requests a CDM based on pixel format

conversion needs

...

• @display_num_of_h_tiles: Number of horizontal tiles in case of

split

•                      interface
  

• • @topology: Topology of the display
  */

struct dpu_encoder_hw_resources { enum dpu_intf_mode intfs[INTF_MAX]; diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h

...

index b3917e0..b5fc65c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -24,6 +24,7 @@ #include "dpu_hw_top.h" #include "dpu_hw_cdm.h" #include "dpu_encoder.h" +#include "dpu_crtc.h"

#define DPU_ENCODER_NAME_MAX 16

@@ -213,7 +214,6 @@ struct dpu_encoder_irq {

• @split_role: Role to play in a split-panel

configuration

...

• @intf_mode: Interface mode
• @intf_idx: Interface index on dpu hardware

• • @topology_name: topology selected for the display
  • @enc_spinlock: Virtual-Encoder-Wide Spin Lock for IRQ purposes
  • @enable_state: Enable state tracking
  • @vblank_refcount: Reference count of vblank request

@@ -243,7 +243,6 @@ struct dpu_encoder_phys { enum dpu_enc_split_role split_role; enum dpu_intf_mode intf_mode; enum dpu_intf intf_idx;

• enum dpu_rm_topology_name topology_name; spinlock_t *enc_spinlock; enum dpu_enc_enable_state enable_state; atomic_t vblank_refcount;

@@ -361,11 +360,15 @@ struct dpu_encoder_phys

*dpu_encoder_phys_cmd_init(

...

static inline enum dpu_3d_blend_mode

dpu_encoder_helper_get_3d_blend_mode(

...

struct dpu_encoder_phys *phys_enc)

{

• struct dpu_crtc_state *dpu_cstate;

• if (!phys_enc || phys_enc->enable_state == DPU_ENC_DISABLING) return BLEND_3D_NONE;

• dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);

• if (phys_enc->split_role == ENC_ROLE_SOLO &&

•    phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE)
  

•    (dpu_cstate->num_mixers == 2))
  

I guess this should be CRTC_DUAL_MIXERS? Perhaps you should add a helper called dpu_crtc_state_is_stereo() instead of littering these checks everywhere.

...

return BLEND_3D_H_ROW_INT;

return BLEND_3D_NONE; 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 6de13f4..1643e2f 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 @@ -355,13 +355,14 @@ static void dpu_encoder_phys_vid_underrun_irq(void

*arg, int irq_idx)

...

static bool _dpu_encoder_phys_is_dual_ctl(struct dpu_encoder_phys

*phys_enc)

...

{

• struct dpu_crtc_state *dpu_cstate;
• if (!phys_enc) return false;

• if (phys_enc->topology_name == DPU_RM_TOPOLOGY_DUALPIPE)

• return true;
  

• dpu_cstate = to_dpu_crtc_state(phys_enc->parent->crtc->state);

• return false;

• return (dpu_cstate->num_ctls > 1);

Unnecessary parens

...

}

static bool dpu_encoder_phys_vid_needs_single_flush( diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c

b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c

...

index 13c0a36..fbd489f 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -33,7 +33,6 @@ (t).num_intf == (r).num_intf)

struct dpu_rm_topology_def {

• enum dpu_rm_topology_name top_name; int num_lm; int num_comp_enc; int num_intf;

@@ -42,10 +41,10 @@ struct dpu_rm_topology_def { };

static const struct dpu_rm_topology_def g_top_table[] = {

• { DPU_RM_TOPOLOGY_NONE, 0, 0, 0, 0, false },
• { DPU_RM_TOPOLOGY_SINGLEPIPE, 1, 0, 1, 1, false },
• { DPU_RM_TOPOLOGY_DUALPIPE, 2, 0, 2, 2, true },
• { DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE, 2, 0, 1, 1, false },

• { 0, 0, 0, 0, false },
• { 1, 0, 1, 1, false },
• { 2, 0, 2, 2, true },
• { 2, 0, 1, 1, false },

You don't need this. The data it provides can be deduced already: num_ctl == num_intf needs_split_display == (num_intf > 1)

...

};

/** @@ -72,13 +71,11 @@ struct dpu_rm_requirements {

• @enc_id: Reservations are tracked by Encoder DRM object ID.

• CRTCs may be connected to multiple Encoders.
  

• An encoder or connector id identifies the display path.
  

• • @topology DRM<->HW topology use case
  */

struct dpu_rm_rsvp { struct list_head list; uint32_t seq; uint32_t enc_id;

• enum dpu_rm_topology_name topology;

};

/** @@ -122,8 +119,8 @@ static void _dpu_rm_print_rsvps( DPU_DEBUG("%d\n", stage);

list_for_each_entry(rsvp, &rm->rsvps, list) {

• DRM_DEBUG_KMS("%d rsvp[s%ue%u] topology %d\n", stage,
  

rsvp->seq,

...

• 	      rsvp->enc_id, rsvp->topology);
  

• DRM_DEBUG_KMS("%d rsvp[s%ue%u]\n", stage, rsvp->seq,
  

• 	      rsvp->enc_id);
  

  }

  for (type = 0; type < DPU_HW_BLK_MAX; type++) {

@@ -146,18 +143,6 @@ struct dpu_hw_mdp *dpu_rm_get_mdp(struct dpu_rm

*rm)

...

return rm->hw_mdp; }

-enum dpu_rm_topology_name -dpu_rm_get_topology_name(struct msm_display_topology topology) -{

• int i;
• for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++)

• if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], topology))
  

• 	return g_top_table[i].top_name;
  

• return DPU_RM_TOPOLOGY_NONE;

-}

void dpu_rm_init_hw_iter( struct dpu_rm_hw_iter *iter, uint32_t enc_id, @@ -760,7 +745,6 @@ static int _dpu_rm_make_next_rsvp( /* Create reservation info, tag reserved blocks with it as we go

*/

...

rsvp->seq = ++rm->rsvp_next_seq; rsvp->enc_id = enc->base.id;

• rsvp->topology = reqs->topology->top_name; list_add_tail(&rsvp->list, &rm->rsvps);

  ret = _dpu_rm_reserve_lms(rm, rsvp, reqs);

@@ -807,7 +791,7 @@ static int _dpu_rm_populate_requirements(

dpu_encoder_get_hw_resources(enc, &reqs->hw_res, conn_state);

• for (i = 0; i < DPU_RM_TOPOLOGY_MAX; i++) {

• for (i = 0; i < ARRAY_SIZE(g_top_table); i++) { if (RM_IS_TOPOLOGY_MATCH(g_top_table[i], req_topology)) { reqs->topology = &g_top_table[i];

Since you can get rid of g_top_table, you can get rid of reqs->topology too.

DPU RM is using reqs->topology (ie. struct dpu_rm_topology_def) to represent the hw_block requirements for a display. Removing the structure might require changes in rest of RM alloc functions.

If you can hold off on this, I am planning to address your comment in the RM refactor series.

I'd rather it be introduced when it's used. That way it can evalutated on its own merits when we have the full picture. There's a bunch of unused or duplicated code surrounding the RM, and I think it'd be best to clean it up while we all have full context in our heads.

Sean

Jeykumar S.

...

...

@@ -831,9 +815,8 @@ static int _dpu_rm_populate_requirements(

DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n",

reqs->top_ctrl,

...

      reqs->hw_res.display_num_of_h_tiles);

• DRM_DEBUG_KMS("num_lm: %d num_ctl: %d topology: %d split_display:

%d\n",

...

• DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n", reqs->topology->num_lm, reqs->topology->num_ctl,

•       reqs->topology->top_name,
   reqs->topology->needs_split_display);
  
  

  return 0;

@@ -969,8 +952,7 @@ static int _dpu_rm_commit_rsvp( }

if (!ret)

• DRM_DEBUG_KMS("rsrv enc %d topology %d\n", rsvp->enc_id,
  

• 	      rsvp->topology);
  

• DRM_DEBUG_KMS("rsrv enc %d\n", rsvp->enc_id);
  

This debug message doesn't provide any value any longer.

...

return ret; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h

b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h

...

index ffd1841..6fd9e7b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h @@ -21,21 +21,6 @@ #include "dpu_hw_top.h"

/**

• • enum dpu_rm_topology_name - HW resource use case in use by

connector

• • @DPU_RM_TOPOLOGY_NONE: No topology in use

currently

• • @DPU_RM_TOPOLOGY_SINGLEPIPE: 1 LM, 1 PP, 1 INTF/WB
• • @DPU_RM_TOPOLOGY_DUALPIPE: 2 LM, 2 PP, 2 INTF/WB
• • @DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE: 2 LM, 2 PP, 3DMux, 1

INTF/WB

• */

-enum dpu_rm_topology_name {

• DPU_RM_TOPOLOGY_NONE = 0,
• DPU_RM_TOPOLOGY_SINGLEPIPE,
• DPU_RM_TOPOLOGY_DUALPIPE,
• DPU_RM_TOPOLOGY_DUALPIPE_3DMERGE,
• DPU_RM_TOPOLOGY_MAX,

-};

-/**

• enum dpu_rm_topology_control - HW resource use case in use by

connector

...

• @DPU_RM_TOPCTL_RESERVE_LOCK: If set, in AtomicTest phase, after a

successful

...

•                          test, reserve the resources for this
  

display.

...

@@ -187,13 +172,4 @@ void dpu_rm_init_hw_iter( */ int dpu_rm_check_property_topctl(uint64_t val);

-/**

• • dpu_rm_get_topology_name - returns the name of the the given

topology

...

• •                        definition
    

• • @topology: topology definition
• • @Return: name of the topology
• */

-enum dpu_rm_topology_name -dpu_rm_get_topology_name(struct msm_display_topology topology);

#endif /* __DPU_RM_H__ */

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

-- Jeykumar S

On Tue, Aug 28, 2018 at 05:40:02PM -0700, Jeykumar Sankaran wrote:

Encoder H_TILE values are not used for allocating the hw blocks. no. of hw_intf blocks provides the info.

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

Reviewed-by: Sean Paul seanpaul@chromium.org

---

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 ----- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 --- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 3 +-- 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 56ef349..bfc082d 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -175,8 +175,6 @@ struct dpu_encoder_virt { spinlock_t enc_spinlock; uint32_t bus_scaling_client;

• uint32_t display_num_of_h_tiles;
• unsigned int num_phys_encs; struct dpu_encoder_phys *phys_encs[MAX_PHYS_ENCODERS_PER_VIRTUAL]; struct dpu_encoder_phys *cur_master;

@@ -455,7 +453,6 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *drm_enc,

/* Query resources used by phys encs, expected to be without overlap */ memset(hw_res, 0, sizeof(*hw_res));

• hw_res->display_num_of_h_tiles = dpu_enc->display_num_of_h_tiles;

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

@@ -2103,8 +2100,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,

WARN_ON(disp_info->num_of_h_tiles < 1);

• dpu_enc->display_num_of_h_tiles = disp_info->num_of_h_tiles;

• DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);

  if ((disp_info->capabilities & MSM_DISPLAY_CAP_CMD_MODE) ||

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index c5600e6..e453271 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -33,13 +33,10 @@

• Encoder functions and data types
• @intfs: Interfaces this encoder is using, INTF_MODE_NONE if unused
• @needs_cdm: Encoder requests a CDM based on pixel format conversion needs

• • @display_num_of_h_tiles: Number of horizontal tiles in case of split

• •                      interface
    

  */

struct dpu_encoder_hw_resources { enum dpu_intf_mode intfs[INTF_MAX]; bool needs_cdm;

• u32 display_num_of_h_tiles;

};

/** diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index fbd489f..388ae65 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -813,8 +813,7 @@ static int _dpu_rm_populate_requirements( conn_state->connector->connector_type == DRM_MODE_CONNECTOR_DSI) reqs->top_ctrl |= BIT(DPU_RM_TOPCTL_DS);

• DRM_DEBUG_KMS("top_ctrl: 0x%llX num_h_tiles: %d\n", reqs->top_ctrl,

•       reqs->hw_res.display_num_of_h_tiles);
  

• DRM_DEBUG_KMS("top_ctrl: 0x%llX\n", reqs->top_ctrl); DRM_DEBUG_KMS("num_lm: %d num_ctl: %d split_display: %d\n", reqs->topology->num_lm, reqs->topology->num_ctl, reqs->topology->needs_split_display);

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

On Tue, Aug 28, 2018 at 05:40:03PM -0700, Jeykumar Sankaran wrote:

Support for CDM block is not present in DPU. Remove CDM handlers from resource manager.

I think there's some leftovers that can be cleaned up...

$ ack -i cdm disp/dpu1/dpu_hw_mdss.h 103: DPU_HW_BLK_CDM, 192:enum dpu_cdm { 193: CDM_0 = 1, 194: CDM_1, 195: CDM_MAX 454:#define DPU_DBG_MASK_CDM (1 << 1)

disp/dpu1/dpu_hw_catalog.h 533: * struct dpu_cdm_cfg - information of chroma down blocks 537: * @intf_connect Bitmask of INTF IDs this CDM can connect to 539:struct dpu_cdm_cfg { 737: u32 cdm_count; 738: struct dpu_cdm_cfg *cdm; 776:#define BLK_CDM(s) ((s)->cdm)

disp/dpu1/dpu_hw_catalog.c 324: * CDM sub blocks config 326:static struct dpu_cdm_cfg sdm845_cdm[] = { 328: .name = "cdm_0", .id = CDM_0, 461: .cdm_count = ARRAY_SIZE(sdm845_cdm), 462: .cdm = sdm845_cdm,

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

drivers/gpu/drm/msm/Makefile | 1 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 5 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c | 323 ----------------------- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h | 139 ---------- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 14 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 4 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c | 18 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h | 17 -- drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 68 +---- drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h | 4 - 11 files changed, 2 insertions(+), 593 deletions(-) delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c delete mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile index 261fa79..19ab521 100644 --- a/drivers/gpu/drm/msm/Makefile +++ b/drivers/gpu/drm/msm/Makefile @@ -58,7 +58,6 @@ msm-y := \ disp/dpu1/dpu_formats.o \ disp/dpu1/dpu_hw_blk.o \ disp/dpu1/dpu_hw_catalog.o \

• disp/dpu1/dpu_hw_cdm.o \ disp/dpu1/dpu_hw_ctl.o \ disp/dpu1/dpu_hw_interrupts.o \ disp/dpu1/dpu_hw_intf.o \

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index e453271..e844afa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -32,11 +32,9 @@ /**

• Encoder functions and data types
• @intfs: Interfaces this encoder is using, INTF_MODE_NONE if unused

• • @needs_cdm: Encoder requests a CDM based on pixel format conversion needs
  */

struct dpu_encoder_hw_resources { enum dpu_intf_mode intfs[INTF_MAX];

• bool needs_cdm;

};

/** diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h index b5fc65c..f796683 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h @@ -22,7 +22,6 @@ #include "dpu_hw_pingpong.h" #include "dpu_hw_ctl.h" #include "dpu_hw_top.h" -#include "dpu_hw_cdm.h" #include "dpu_encoder.h" #include "dpu_crtc.h"

@@ -205,8 +204,6 @@ struct dpu_encoder_irq {

• @parent_ops: Callbacks exposed by the parent to the phys_enc
• @hw_mdptop: Hardware interface to the top registers
• @hw_ctl: Hardware interface to the ctl registers

• • @hw_cdm: Hardware interface to the cdm registers
• • @cdm_cfg: Chroma-down hardware configuration
  • @hw_pp: Hardware interface to the ping pong registers
  • @dpu_kms: Pointer to the dpu_kms top level
  • @cached_mode: DRM mode cached at mode_set time, acted on in enable

@@ -235,8 +232,6 @@ struct dpu_encoder_phys { const struct dpu_encoder_virt_ops *parent_ops; struct dpu_hw_mdp *hw_mdptop; struct dpu_hw_ctl *hw_ctl;

• struct dpu_hw_cdm *hw_cdm;
• struct dpu_hw_cdm_cfg cdm_cfg; struct dpu_hw_pingpong *hw_pp; struct dpu_kms *dpu_kms; struct drm_display_mode cached_mode;

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c deleted file mode 100644 index 554874b..0000000 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.c +++ /dev/null @@ -1,323 +0,0 @@ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 and
• • only version 2 as published by the Free Software Foundation.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

-#include "dpu_hw_mdss.h" -#include "dpu_hwio.h" -#include "dpu_hw_catalog.h" -#include "dpu_hw_cdm.h" -#include "dpu_dbg.h" -#include "dpu_kms.h"

-#define CDM_CSC_10_OPMODE 0x000 -#define CDM_CSC_10_BASE 0x004

-#define CDM_CDWN2_OP_MODE 0x100 -#define CDM_CDWN2_CLAMP_OUT 0x104 -#define CDM_CDWN2_PARAMS_3D_0 0x108 -#define CDM_CDWN2_PARAMS_3D_1 0x10C -#define CDM_CDWN2_COEFF_COSITE_H_0 0x110 -#define CDM_CDWN2_COEFF_COSITE_H_1 0x114 -#define CDM_CDWN2_COEFF_COSITE_H_2 0x118 -#define CDM_CDWN2_COEFF_OFFSITE_H_0 0x11C -#define CDM_CDWN2_COEFF_OFFSITE_H_1 0x120 -#define CDM_CDWN2_COEFF_OFFSITE_H_2 0x124 -#define CDM_CDWN2_COEFF_COSITE_V 0x128 -#define CDM_CDWN2_COEFF_OFFSITE_V 0x12C -#define CDM_CDWN2_OUT_SIZE 0x130

-#define CDM_HDMI_PACK_OP_MODE 0x200 -#define CDM_CSC_10_MATRIX_COEFF_0 0x004

-/**

• • Horizontal coefficients for cosite chroma downscale
• • s13 representation of coefficients
• */

-static u32 cosite_h_coeff[] = {0x00000016, 0x000001cc, 0x0100009e};

-/**

• • Horizontal coefficients for offsite chroma downscale
• */

-static u32 offsite_h_coeff[] = {0x000b0005, 0x01db01eb, 0x00e40046};

-/**

• • Vertical coefficients for cosite chroma downscale
• */

-static u32 cosite_v_coeff[] = {0x00080004}; -/**

• • Vertical coefficients for offsite chroma downscale
• */

-static u32 offsite_v_coeff[] = {0x00060002};

-/* Limited Range rgb2yuv coeff with clamp and bias values for CSC 10 module */ -static struct dpu_csc_cfg rgb2yuv_cfg = {

• {

• 0x0083, 0x0102, 0x0032,
  

• 0x1fb5, 0x1f6c, 0x00e1,
  

• 0x00e1, 0x1f45, 0x1fdc
  

• },
• { 0x00, 0x00, 0x00 },
• { 0x0040, 0x0200, 0x0200 },
• { 0x000, 0x3ff, 0x000, 0x3ff, 0x000, 0x3ff },
• { 0x040, 0x3ac, 0x040, 0x3c0, 0x040, 0x3c0 },

-};

-static struct dpu_cdm_cfg *_cdm_offset(enum dpu_cdm cdm,

• struct dpu_mdss_cfg *m,
  

• void __iomem *addr,
  

• struct dpu_hw_blk_reg_map *b)
  

-{

• int i;
• for (i = 0; i < m->cdm_count; i++) {

• if (cdm == m->cdm[i].id) {
  

• 	b->base_off = addr;
  

• 	b->blk_off = m->cdm[i].base;
  

• 	b->length = m->cdm[i].len;
  

• 	b->hwversion = m->hwversion;
  

• 	b->log_mask = DPU_DBG_MASK_CDM;
  

• 	return &m->cdm[i];
  

• }
  

• }
• return ERR_PTR(-EINVAL);

-}

-static int dpu_hw_cdm_setup_csc_10bit(struct dpu_hw_cdm *ctx,

• struct dpu_csc_cfg *data)
  

-{

• dpu_hw_csc_setup(&ctx->hw, CDM_CSC_10_MATRIX_COEFF_0, data, true);
• return 0;

-}

-static int dpu_hw_cdm_setup_cdwn(struct dpu_hw_cdm *ctx,

• struct dpu_hw_cdm_cfg *cfg)
  

-{

• struct dpu_hw_blk_reg_map *c = &ctx->hw;
• u32 opmode = 0;
• u32 out_size = 0;
• if (cfg->output_bit_depth == CDM_CDWN_OUTPUT_10BIT)

• opmode &= ~BIT(7);
  

• else

• opmode |= BIT(7);
  

• /* ENABLE DWNS_H bit */
• opmode |= BIT(1);
• switch (cfg->h_cdwn_type) {
• case CDM_CDWN_DISABLE:

• /* CLEAR METHOD_H field */
  

• opmode &= ~(0x18);
  

• /* CLEAR DWNS_H bit */
  

• opmode &= ~BIT(1);
  

• break;
  

• case CDM_CDWN_PIXEL_DROP:

• /* Clear METHOD_H field (pixel drop is 0) */
  

• opmode &= ~(0x18);
  

• break;
  

• case CDM_CDWN_AVG:

• /* Clear METHOD_H field (Average is 0x1) */
  

• opmode &= ~(0x18);
  

• opmode |= (0x1 << 0x3);
  

• break;
  

• case CDM_CDWN_COSITE:

• /* Clear METHOD_H field (Average is 0x2) */
  

• opmode &= ~(0x18);
  

• opmode |= (0x2 << 0x3);
  

• /* Co-site horizontal coefficients */
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_0,
  

• 		cosite_h_coeff[0]);
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_1,
  

• 		cosite_h_coeff[1]);
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_COSITE_H_2,
  

• 		cosite_h_coeff[2]);
  

• break;
  

• case CDM_CDWN_OFFSITE:

• /* Clear METHOD_H field (Average is 0x3) */
  

• opmode &= ~(0x18);
  

• opmode |= (0x3 << 0x3);
  

• /* Off-site horizontal coefficients */
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_0,
  

• 		offsite_h_coeff[0]);
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_1,
  

• 		offsite_h_coeff[1]);
  

• DPU_REG_WRITE(c, CDM_CDWN2_COEFF_OFFSITE_H_2,
  

• 		offsite_h_coeff[2]);
  

• break;
  

• default:

• pr_err("%s invalid horz down sampling type\n", __func__);
  

• return -EINVAL;
  

• }
• /* ENABLE DWNS_V bit */
• opmode |= BIT(2);
• switch (cfg->v_cdwn_type) {
• case CDM_CDWN_DISABLE:

• /* CLEAR METHOD_V field */
  

• opmode &= ~(0x60);
  

• /* CLEAR DWNS_V bit */
  

• opmode &= ~BIT(2);
  

• break;
  

• case CDM_CDWN_PIXEL_DROP:

• /* Clear METHOD_V field (pixel drop is 0) */
  

• opmode &= ~(0x60);
  

• break;
  

• case CDM_CDWN_AVG:

• /* Clear METHOD_V field (Average is 0x1) */
  

• opmode &= ~(0x60);
  

• opmode |= (0x1 << 0x5);
  

• break;
  

• case CDM_CDWN_COSITE:

• /* Clear METHOD_V field (Average is 0x2) */
  

• opmode &= ~(0x60);
  

• opmode |= (0x2 << 0x5);
  

• /* Co-site vertical coefficients */
  

• DPU_REG_WRITE(c,
  

• 		CDM_CDWN2_COEFF_COSITE_V,
  

• 		cosite_v_coeff[0]);
  

• break;
  

• case CDM_CDWN_OFFSITE:

• /* Clear METHOD_V field (Average is 0x3) */
  

• opmode &= ~(0x60);
  

• opmode |= (0x3 << 0x5);
  

• /* Off-site vertical coefficients */
  

• DPU_REG_WRITE(c,
  

• 		CDM_CDWN2_COEFF_OFFSITE_V,
  

• 		offsite_v_coeff[0]);
  

• break;
  

• default:

• return -EINVAL;
  

• }
• if (cfg->v_cdwn_type || cfg->h_cdwn_type)

• opmode |= BIT(0); /* EN CDWN module */
  

• else

• opmode &= ~BIT(0);
  

• out_size = (cfg->output_width & 0xFFFF) |

• ((cfg->output_height & 0xFFFF) << 16);
  

• DPU_REG_WRITE(c, CDM_CDWN2_OUT_SIZE, out_size);
• DPU_REG_WRITE(c, CDM_CDWN2_OP_MODE, opmode);
• DPU_REG_WRITE(c, CDM_CDWN2_CLAMP_OUT,

• 	((0x3FF << 16) | 0x0));
  

• return 0;

-}

-static int dpu_hw_cdm_enable(struct dpu_hw_cdm *ctx,

• struct dpu_hw_cdm_cfg *cdm)
  

-{

• struct dpu_hw_blk_reg_map *c = &ctx->hw;
• const struct dpu_format *fmt = cdm->output_fmt;
• struct cdm_output_cfg cdm_cfg = { 0 };
• u32 opmode = 0;
• u32 csc = 0;
• if (!DPU_FORMAT_IS_YUV(fmt))

• return -EINVAL;
  

• if (cdm->output_type == CDM_CDWN_OUTPUT_HDMI) {

• if (fmt->chroma_sample != DPU_CHROMA_H1V2)
  

• 	return -EINVAL; /*unsupported format */
  

• opmode = BIT(0);
  

• opmode |= (fmt->chroma_sample << 1);
  

• cdm_cfg.intf_en = true;
  

• }
• csc |= BIT(2);
• csc &= ~BIT(1);
• csc |= BIT(0);
• if (ctx->hw_mdp && ctx->hw_mdp->ops.setup_cdm_output)

• ctx->hw_mdp->ops.setup_cdm_output(ctx->hw_mdp, &cdm_cfg);
  

• DPU_REG_WRITE(c, CDM_CSC_10_OPMODE, csc);
• DPU_REG_WRITE(c, CDM_HDMI_PACK_OP_MODE, opmode);
• return 0;

-}

-static void dpu_hw_cdm_disable(struct dpu_hw_cdm *ctx) -{

• struct cdm_output_cfg cdm_cfg = { 0 };
• if (ctx->hw_mdp && ctx->hw_mdp->ops.setup_cdm_output)

• ctx->hw_mdp->ops.setup_cdm_output(ctx->hw_mdp, &cdm_cfg);
  

-}

-static void _setup_cdm_ops(struct dpu_hw_cdm_ops *ops,

• unsigned long features)

-{

• ops->setup_csc_data = dpu_hw_cdm_setup_csc_10bit;
• ops->setup_cdwn = dpu_hw_cdm_setup_cdwn;
• ops->enable = dpu_hw_cdm_enable;
• ops->disable = dpu_hw_cdm_disable;

-}

-static struct dpu_hw_blk_ops dpu_hw_ops = {

• .start = NULL,
• .stop = NULL,

-};

-struct dpu_hw_cdm *dpu_hw_cdm_init(enum dpu_cdm idx,

• void __iomem *addr,
  

• struct dpu_mdss_cfg *m,
  

• struct dpu_hw_mdp *hw_mdp)
  

-{

• struct dpu_hw_cdm *c;
• struct dpu_cdm_cfg *cfg;
• int rc;
• c = kzalloc(sizeof(*c), GFP_KERNEL);
• if (!c)

• return ERR_PTR(-ENOMEM);
  

• cfg = _cdm_offset(idx, m, addr, &c->hw);
• if (IS_ERR_OR_NULL(cfg)) {

• kfree(c);
  

• return ERR_PTR(-EINVAL);
  

• }
• c->idx = idx;
• c->caps = cfg;
• _setup_cdm_ops(&c->ops, c->caps->features);
• c->hw_mdp = hw_mdp;
• rc = dpu_hw_blk_init(&c->base, DPU_HW_BLK_CDM, idx, &dpu_hw_ops);
• if (rc) {

• DPU_ERROR("failed to init hw blk %d\n", rc);
  

• goto blk_init_error;
  

• }
• /*

• * Perform any default initialization for the chroma down module
  

• * @setup default csc coefficients
  

• */
  

• dpu_hw_cdm_setup_csc_10bit(c, &rgb2yuv_cfg);
• return c;

-blk_init_error:

• kzfree(c);
• return ERR_PTR(rc);

-}

-void dpu_hw_cdm_destroy(struct dpu_hw_cdm *cdm) -{

• if (cdm)

• dpu_hw_blk_destroy(&cdm->base);
  

• kfree(cdm);

-} diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h deleted file mode 100644 index 5cceb1e..0000000 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_cdm.h +++ /dev/null @@ -1,139 +0,0 @@ -/* Copyright (c) 2015-2018, The Linux Foundation. All rights reserved.

• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 and
• • only version 2 as published by the Free Software Foundation.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• */

-#ifndef _DPU_HW_CDM_H -#define _DPU_HW_CDM_H

-#include "dpu_hw_mdss.h" -#include "dpu_hw_top.h" -#include "dpu_hw_blk.h"

-struct dpu_hw_cdm;

-struct dpu_hw_cdm_cfg {

• u32 output_width;
• u32 output_height;
• u32 output_bit_depth;
• u32 h_cdwn_type;
• u32 v_cdwn_type;
• const struct dpu_format *output_fmt;
• u32 output_type;
• int flags;

-};

-enum dpu_hw_cdwn_type {

• CDM_CDWN_DISABLE,
• CDM_CDWN_PIXEL_DROP,
• CDM_CDWN_AVG,
• CDM_CDWN_COSITE,
• CDM_CDWN_OFFSITE,

-};

-enum dpu_hw_cdwn_output_type {

• CDM_CDWN_OUTPUT_HDMI,
• CDM_CDWN_OUTPUT_WB,

-};

-enum dpu_hw_cdwn_output_bit_depth {

• CDM_CDWN_OUTPUT_8BIT,
• CDM_CDWN_OUTPUT_10BIT,

-};

-/**

• • struct dpu_hw_cdm_ops : Interface to the chroma down Hw driver functions

• •                     Assumption is these functions will be called after
    

• •                     clocks are enabled
    

• • @setup_csc: Programs the csc matrix
• • @setup_cdwn: Sets up the chroma down sub module
• • @enable: Enables the output to interface and programs the

• •                     output packer
    

• • @disable: Puts the cdm in bypass mode
• */

-struct dpu_hw_cdm_ops {

• /**

• * Programs the CSC matrix for conversion from RGB space to YUV space,
  

• * it is optional to call this function as this matrix is automatically
  

• * set during initialization, user should call this if it wants
  

• * to program a different matrix than default matrix.
  

• * @cdm:          Pointer to the chroma down context structure
  

• * @data          Pointer to CSC configuration data
  

• * return:        0 if success; error code otherwise
  

• */
  

• int (*setup_csc_data)(struct dpu_hw_cdm *cdm,

• 	struct dpu_csc_cfg *data);
  

• /**

• * Programs the Chroma downsample part.
  

• * @cdm         Pointer to chroma down context
  

• */
  

• int (*setup_cdwn)(struct dpu_hw_cdm *cdm,
• struct dpu_hw_cdm_cfg *cfg);
• /**

• * Enable the CDM module
  

• * @cdm         Pointer to chroma down context
  

• */
  

• int (*enable)(struct dpu_hw_cdm *cdm,
• struct dpu_hw_cdm_cfg *cfg);
• /**

• * Disable the CDM module
  

• * @cdm         Pointer to chroma down context
  

• */
  

• void (*disable)(struct dpu_hw_cdm *cdm);

-};

-struct dpu_hw_cdm {

• struct dpu_hw_blk base;
• struct dpu_hw_blk_reg_map hw;
• /* chroma down */
• const struct dpu_cdm_cfg *caps;
• enum dpu_cdm idx;
• /* mdp top hw driver */
• struct dpu_hw_mdp *hw_mdp;
• /* ops */
• struct dpu_hw_cdm_ops ops;

-};

-/**

• • dpu_hw_cdm - convert base object dpu_hw_base to container
• • @hw: Pointer to base hardware block
• • return: Pointer to hardware block container
• */

-static inline struct dpu_hw_cdm *to_dpu_hw_cdm(struct dpu_hw_blk *hw) -{

• return container_of(hw, struct dpu_hw_cdm, base);

-}

-/**

• • dpu_hw_cdm_init - initializes the cdm hw driver object.
• • should be called once before accessing every cdm.
• • @idx: cdm index for which driver object is required
• • @addr: mapped register io address of MDP
• • @m : pointer to mdss catalog data
• • @hw_mdp: pointer to mdp top hw driver object
• */

-struct dpu_hw_cdm *dpu_hw_cdm_init(enum dpu_cdm idx,

• void __iomem *addr,
  

• struct dpu_mdss_cfg *m,
  

• struct dpu_hw_mdp *hw_mdp);
  

-/**

• • dpu_hw_cdm_destroy - destroys CDM driver context
• • @cdm: pointer to CDM driver context
• */

-void dpu_hw_cdm_destroy(struct dpu_hw_cdm *cdm);

-#endif /*_DPU_HW_CDM_H */ diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c index 06be7cf..b394a18 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c @@ -224,19 +224,6 @@ static inline int dpu_hw_ctl_get_bitmask_intf(struct dpu_hw_ctl *ctx, return 0; }

-static inline int dpu_hw_ctl_get_bitmask_cdm(struct dpu_hw_ctl *ctx,

• u32 *flushbits, enum dpu_cdm cdm)
  

-{

• switch (cdm) {
• case CDM_0:

• *flushbits |= BIT(26);
  

• break;
  

• default:

• return -EINVAL;
  

• }
• return 0;

-}

static u32 dpu_hw_ctl_poll_reset_status(struct dpu_hw_ctl *ctx, u32 timeout_us) { struct dpu_hw_blk_reg_map *c = &ctx->hw; @@ -485,7 +472,6 @@ static void _setup_ctl_ops(struct dpu_hw_ctl_ops *ops, ops->get_bitmask_sspp = dpu_hw_ctl_get_bitmask_sspp; ops->get_bitmask_mixer = dpu_hw_ctl_get_bitmask_mixer; ops->get_bitmask_intf = dpu_hw_ctl_get_bitmask_intf;

• ops->get_bitmask_cdm = dpu_hw_ctl_get_bitmask_cdm;

};

static struct dpu_hw_blk_ops dpu_hw_ops = { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h index c66a71f..6f313fa 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h @@ -142,10 +142,6 @@ struct dpu_hw_ctl_ops { u32 *flushbits, enum dpu_intf blk);

• int (*get_bitmask_cdm)(struct dpu_hw_ctl *ctx,

• u32 *flushbits,
  

• enum dpu_cdm blk);
  

• /**
  • Set all blend stages to disabled
  • @ctx : ctl path ctx pointer

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c index db2798e..b878125 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c @@ -98,23 +98,6 @@ static void dpu_hw_setup_split_pipe(struct dpu_hw_mdp *mdp, DPU_REG_WRITE(c, SPLIT_DISPLAY_EN, cfg->en & 0x1); }

-static void dpu_hw_setup_cdm_output(struct dpu_hw_mdp *mdp,

• struct cdm_output_cfg *cfg)
  

-{

• struct dpu_hw_blk_reg_map *c;
• u32 out_ctl = 0;
• if (!mdp || !cfg)

• return;
  

• c = &mdp->hw;
• if (cfg->intf_en)

• out_ctl |= BIT(19);
  

• DPU_REG_WRITE(c, MDP_OUT_CTL_0, out_ctl);

-}

static bool dpu_hw_setup_clk_force_ctrl(struct dpu_hw_mdp *mdp, enum dpu_clk_ctrl_type clk_ctrl, bool enable) { @@ -307,7 +290,6 @@ static void _setup_mdp_ops(struct dpu_hw_mdp_ops *ops, unsigned long cap) { ops->setup_split_pipe = dpu_hw_setup_split_pipe;

• ops->setup_cdm_output = dpu_hw_setup_cdm_output; ops->setup_clk_force_ctrl = dpu_hw_setup_clk_force_ctrl; ops->get_danger_status = dpu_hw_get_danger_status; ops->setup_vsync_source = dpu_hw_setup_vsync_source;

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h index 899925a..192e338 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.h @@ -52,14 +52,6 @@ struct split_pipe_cfg { };

/**

• • struct cdm_output_cfg: output configuration for cdm
• • @intf_en : enable/disable interface output
• */

-struct cdm_output_cfg {

• bool intf_en;

-};

-/**

• struct dpu_danger_safe_status: danger and safe status signals
• @mdp: top level status
• @sspp: source pipe status

@@ -89,7 +81,6 @@ struct dpu_vsync_source_cfg {

• Assumption is these functions will be called after clocks are enabled.
• @setup_split_pipe : Programs the pipe control registers
• @setup_pp_split : Programs the pp split control registers

• • @setup_cdm_output : programs cdm control
  • @setup_traffic_shaper : programs traffic shaper control
  */

struct dpu_hw_mdp_ops { @@ -102,14 +93,6 @@ struct dpu_hw_mdp_ops { struct split_pipe_cfg *p);

/**

• * setup_cdm_output() : Setup selection control of the cdm data path
  

• * @mdp  : mdp top context driver
  

• * @cfg  : cdm output configuration
  

• */
  

• void (*setup_cdm_output)(struct dpu_hw_mdp *mdp,

• 	struct cdm_output_cfg *cfg);
  

• /**
  • setup_traffic_shaper() : Setup traffic shaper control
  • @mdp : mdp top context driver
  • @cfg : traffic shaper configuration

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c index 388ae65..e08b3bd 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c @@ -16,7 +16,6 @@ #include "dpu_kms.h" #include "dpu_hw_lm.h" #include "dpu_hw_ctl.h" -#include "dpu_hw_cdm.h" #include "dpu_hw_pingpong.h" #include "dpu_hw_intf.h" #include "dpu_encoder.h" @@ -214,9 +213,6 @@ static void _dpu_rm_hw_destroy(enum dpu_hw_blk_type type, void *hw) case DPU_HW_BLK_CTL: dpu_hw_ctl_destroy(hw); break;

• case DPU_HW_BLK_CDM:

• dpu_hw_cdm_destroy(hw);
  

• break;
  

  case DPU_HW_BLK_PINGPONG: dpu_hw_pingpong_destroy(hw); break;

@@ -290,9 +286,6 @@ static int _dpu_rm_hw_blk_create( case DPU_HW_BLK_CTL: hw = dpu_hw_ctl_init(id, mmio, cat); break;

• case DPU_HW_BLK_CDM:

• hw = dpu_hw_cdm_init(id, mmio, cat, hw_mdp);
  

• break;
  

  case DPU_HW_BLK_PINGPONG: hw = dpu_hw_pingpong_init(id, mmio, cat); break;

@@ -423,15 +416,6 @@ int dpu_rm_init(struct dpu_rm *rm, } }

• for (i = 0; i < cat->cdm_count; i++) {

• rc = _dpu_rm_hw_blk_create(rm, cat, mmio, DPU_HW_BLK_CDM,
  

• 		cat->cdm[i].id, &cat->cdm[i]);
  

• if (rc) {
  

• 	DPU_ERROR("failed: cdm hw not available\n");
  

• 	goto fail;
  

• }
  

• }
• return 0;

fail: @@ -628,55 +612,11 @@ static int _dpu_rm_reserve_ctls( return 0; }

-static int _dpu_rm_reserve_cdm(

• struct dpu_rm *rm,
  

• struct dpu_rm_rsvp *rsvp,
  

• uint32_t id,
  

• enum dpu_hw_blk_type type)
  

-{

• struct dpu_rm_hw_iter iter;
• DRM_DEBUG_KMS("type %d id %d\n", type, id);
• dpu_rm_init_hw_iter(&iter, 0, DPU_HW_BLK_CDM);
• while (_dpu_rm_get_hw_locked(rm, &iter)) {

• const struct dpu_hw_cdm *cdm = to_dpu_hw_cdm(iter.blk->hw);
  

• const struct dpu_cdm_cfg *caps = cdm->caps;
  

• bool match = false;
  

• if (RESERVED_BY_OTHER(iter.blk, rsvp))
  

• 	continue;
  

• if (type == DPU_HW_BLK_INTF && id != INTF_MAX)
  

• 	match = test_bit(id, &caps->intf_connect);
  

• DRM_DEBUG_KMS("iter: type:%d id:%d enc:%d cdm:%lu match:%d\n",
  

• 	      iter.blk->type, iter.blk->id, rsvp->enc_id,
  

• 	      caps->intf_connect, match);
  

• if (!match)
  

• 	continue;
  

• trace_dpu_rm_reserve_cdm(iter.blk->id, iter.blk->type,
  

• 			 rsvp->enc_id);
  

• iter.blk->rsvp_nxt = rsvp;
  

• break;
  

• }
• if (!iter.hw) {

• DPU_ERROR("couldn't reserve cdm for type %d id %d\n", type, id);
  

• return -ENAVAIL;
  

• }
• return 0;

-}

static int _dpu_rm_reserve_intf( struct dpu_rm *rm, struct dpu_rm_rsvp *rsvp, uint32_t id,

• enum dpu_hw_blk_type type,
  

• bool needs_cdm)
  

• enum dpu_hw_blk_type type)
  

{ struct dpu_rm_hw_iter iter; int ret = 0; @@ -704,9 +644,6 @@ static int _dpu_rm_reserve_intf( return -EINVAL; }

• if (needs_cdm)

• ret = _dpu_rm_reserve_cdm(rm, rsvp, id, type);
  

• return ret;

}

@@ -723,7 +660,7 @@ static int _dpu_rm_reserve_intf_related_hw( continue; id = i + INTF_0; ret = _dpu_rm_reserve_intf(rm, rsvp, id,

• 		DPU_HW_BLK_INTF, hw_res->needs_cdm);
  

• 		DPU_HW_BLK_INTF);
  

  if (ret) return ret; }

@@ -769,7 +706,6 @@ static int _dpu_rm_make_next_rsvp( return ret; }

• /* Assign INTFs and blks whose usage is tied to them: CTL & CDM */ ret = _dpu_rm_reserve_intf_related_hw(rm, rsvp, &reqs->hw_res); if (ret) return ret;

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h index ae0ca50..0be51db 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h @@ -868,10 +868,6 @@ TP_printk("id:%d type:%d enc_id:%u", __entry->id, __entry->type, __entry->enc_id) ); -DEFINE_EVENT(dpu_rm_iter_template, dpu_rm_reserve_cdm,

• TP_PROTO(uint32_t id, enum dpu_hw_blk_type type, uint32_t enc_id),
• TP_ARGS(id, type, enc_id)

-); DEFINE_EVENT(dpu_rm_iter_template, dpu_rm_reserve_intf, TP_PROTO(uint32_t id, enum dpu_hw_blk_type type, uint32_t enc_id), TP_ARGS(id, type, enc_id) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project