[RFC 0/4] Separate wb_idx and intf_idx in dpu_encoder

[RFC 0/4] Separate wb_idx and intf_idx in dpu_encoder

[Abhinav Kumar]

As promised here [1], this is a follow up change to separate out
wb_idx and intf_idx for better clarity in dpu_encoder.

This also helps to easily handle boards which do not have a physical
display but can still be validated using writeback interface.

In addition, this also takes care of adding wb_idx to existing DRM prints
and traces.

Currently posting this as a RFC to get feedback on this approach and if
the reviews are positive, I can easily absorb this in the DPU writeback
series [2]

[1] https://patchwork.freedesktop.org/patch/482637/?series=99724&rev=2#comment_868460
[2] https://patchwork.freedesktop.org/series/99724/#rev3

Abhinav Kumar (4):
  drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces
  drm/msm/dpu: start using wb_idx in dpu_encoder_phys_wb
  drm/msm/dpu: add wb_idx to existing DRM prints in dpu_encoder
  drm/msm/dpu: add wb_idx to DRM traces in dpu_encoder

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c        | 125 +++++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h   |   4 +
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c    |  10 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h             |   2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h          |  26 +++--
 5 files changed, 93 insertions(+), 74 deletions(-)

-- 
2.7.4

[RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Abhinav Kumar]

Using intf_idx even for writeback interfaces is confusing
because intf_idx is of type enum dpu_intf but the index used
for writeback is of type enum dpu_wb.

In addition, this makes it easier to separately check the
availability of the two as its possible that there are boards
which don't have any physical display connected but can still
work in writeback mode.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62 +++++++++++++-----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
 3 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c12841..054d7e4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -962,7 +962,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
 	int num_lm, num_ctl, num_pp, num_dsc;
 	unsigned int dsc_mask = 0;
-	enum dpu_hw_blk_type blk_type;
 	int i;
 
 	if (!drm_enc) {
@@ -1044,17 +1043,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 		phys->hw_pp = dpu_enc->hw_pp[i];
 		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
 
-		if (dpu_encoder_get_intf_mode(&dpu_enc->base) == INTF_MODE_WB_LINE)
-			blk_type = DPU_HW_BLK_WB;
-		else
-			blk_type = DPU_HW_BLK_INTF;
+		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
+			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
 
-		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
-			if (blk_type == DPU_HW_BLK_INTF)
-				phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
-			else if (blk_type == DPU_HW_BLK_WB)
-				phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->intf_idx);
-		}
+		if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
+			phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
 
 		if (!phys->hw_intf && !phys->hw_wb) {
 			DPU_ERROR_ENC(dpu_enc,
@@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 	mutex_unlock(&dpu_enc->enc_lock);
 }
 
-static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
+static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
 		enum dpu_intf_type type, u32 controller_id)
 {
 	int i = 0;
@@ -1213,16 +1206,28 @@ static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
 				return catalog->intf[i].id;
 			}
 		}
-	} else {
-		for (i = 0; i < catalog->wb_count; i++) {
-			if (catalog->wb[i].id == controller_id)
-				return catalog->wb[i].id;
-		}
 	}
 
 	return INTF_MAX;
 }
 
+static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
+		enum dpu_intf_type type, u32 controller_id)
+{
+	int i = 0;
+
+	if (type != INTF_WB)
+		goto end;
+
+	for (i = 0; i < catalog->wb_count; i++) {
+		if (catalog->wb[i].id == controller_id)
+			return catalog->wb[i].id;
+	}
+
+end:
+	return WB_MAX;
+}
+
 static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
 		struct dpu_encoder_phys *phy_enc)
 {
@@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
 				i, controller_id, phys_params.split_role);
 
-		/*
-		 * FIXME: have separate intf_idx and wb_idx to avoid using
-		 * enum dpu_intf type for wb_idx and also to be able to
-		 * not bail out when there is no intf for boards which dont
-		 * have a display connected to them.
-		 * Having a valid wb_idx but not a intf_idx can be a valid
-		 * combination moving forward.
-		 */
-		phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
+		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
 													intf_type,
 													controller_id);
-		if (phys_params.intf_idx == INTF_MAX) {
+
+		phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
+				intf_type, controller_id);
+		/*
+		 * For boards which have no physical displays, having no interface
+		 * is fine because it can still be used with just writeback.
+		 * If we try without a display on a board which uses a DPU in which
+		 * writeback is not supported, then this will still fail as it will not
+		 * find any writeback in the catalog.
+		 */
+		if ((phys_params.intf_idx == INTF_MAX) &&
+				(phys_params.wb_idx == WB_MAX)) {
 			DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n",
 						  intf_type, controller_id);
 			ret = -EINVAL;
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 04d037e..f2af07d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -181,6 +181,7 @@ enum dpu_intr_idx {
  * @split_role:		Role to play in a split-panel configuration
  * @intf_mode:		Interface mode
  * @intf_idx:		Interface index on dpu hardware
+ * @wb_idx:			Writeback index on dpu hardware
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  * @enable_state:	Enable state tracking
  * @vblank_refcount:	Reference count of vblank request
@@ -209,6 +210,7 @@ struct dpu_encoder_phys {
 	enum dpu_enc_split_role split_role;
 	enum dpu_intf_mode intf_mode;
 	enum dpu_intf intf_idx;
+	enum dpu_wb wb_idx;
 	spinlock_t *enc_spinlock;
 	enum dpu_enc_enable_state enable_state;
 	atomic_t vblank_refcount;
@@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
  * @parent_ops:		Callbacks exposed by the parent to the phys_enc
  * @split_role:		Role to play in a split-panel configuration
  * @intf_idx:		Interface index this phys_enc will control
+ * @wb_idx:			Writeback index this phys_enc will control
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
  */
 struct dpu_enc_phys_init_params {
@@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
 	const struct dpu_encoder_virt_ops *parent_ops;
 	enum dpu_enc_split_role split_role;
 	enum dpu_intf intf_idx;
+	enum dpu_wb wb_idx;
 	spinlock_t *enc_spinlock;
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
index ba82e54..2f34a31 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
@@ -103,7 +103,7 @@ static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
  * @rm: DPU Resource Manager handle
  * @wb_idx: WB index
  */
-static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum dpu_intf wb_idx)
+static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum dpu_wb wb_idx)
 {
 	return rm->hw_wb[wb_idx - WB_0];
 }
-- 
2.7.4

[RFC 2/4] drm/msm/dpu: start using wb_idx in dpu_encoder_phys_wb

[Abhinav Kumar]

Convert all the usages of intf_idx to wb_idx in
dpu_encoder_phys_wb.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
index 563ca08..cb5c7da 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c
@@ -250,7 +250,7 @@ static int dpu_encoder_phys_wb_atomic_check(
 	const struct drm_display_mode *mode;
 
 	DPU_DEBUG("[atomic_check:%d, \"%s\",%d,%d]\n",
-			phys_enc->intf_idx, mode->name, mode->hdisplay, mode->vdisplay);
+			phys_enc->wb_idx, mode->name, mode->hdisplay, mode->vdisplay);
 
 	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
 		return 0;
@@ -584,7 +584,7 @@ static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys *phys_enc)
  */
 static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys *phys_enc)
 {
-	DPU_DEBUG("[wb:%d]\n", phys_enc->intf_idx - INTF_0);
+	DPU_DEBUG("[wb:%d]\n", phys_enc->wb_idx - WB_0);
 
 	if (!phys_enc)
 		return;
@@ -730,7 +730,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 
 	phys_enc = &wb_enc->base;
 	phys_enc->hw_mdptop = p->dpu_kms->hw_mdp;
-	phys_enc->intf_idx = p->intf_idx;
+	phys_enc->wb_idx = p->wb_idx;
 
 	dpu_encoder_phys_wb_init_ops(&phys_enc->ops);
 	phys_enc->parent = p->parent;
@@ -738,7 +738,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 	phys_enc->dpu_kms = p->dpu_kms;
 	phys_enc->split_role = p->split_role;
 	phys_enc->intf_mode = INTF_MODE_WB_LINE;
-	phys_enc->intf_idx = p->intf_idx;
+	phys_enc->wb_idx = p->wb_idx;
 	phys_enc->enc_spinlock = p->enc_spinlock;
 
 	atomic_set(&wb_enc->wbirq_refcount, 0);
@@ -754,7 +754,7 @@ struct dpu_encoder_phys *dpu_encoder_phys_wb_init(
 	phys_enc->enable_state = DPU_ENC_DISABLED;
 
 	DPU_DEBUG("Created dpu_encoder_phys for wb %d\n",
-			phys_enc->intf_idx);
+			phys_enc->wb_idx);
 
 	return phys_enc;
 
-- 
2.7.4

[RFC 3/4] drm/msm/dpu: add wb_idx to existing DRM prints in dpu_encoder

[Abhinav Kumar]

Add wb_idx to existing DRM prints in dpu_encoder and also
print the intf_mode so that its clear that for any INTF_CMD/VID
there will be a valid intf_idx and any INTF_WB_* there will be a
valid wb_idx.

Update the debugfs to add the same information. Here is a sample
output with this change:

root:/sys/kernel/debug/dri/0/encoder31# cat status
intf:1  wb:-1  vsync: 31  underrun: 0    mode: INTF_MODE_VIDEO
root:/sys/kernel/debug/dri/0/encoder33# cat status
intf:-1  wb:2  vsync:  7  underrun: 0    mode: INTF_MODE_WB_LINE

Also remove DPU_DEBUG_PHYS macros as its unused because the
respective dpu_encoder_phys_* files have their own macros.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 50 +++++++++++++----------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 054d7e4..871ce87 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -37,18 +37,6 @@
 #define DPU_ERROR_ENC(e, fmt, ...) DPU_ERROR("enc%d " fmt,\
 		(e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
 
-#define DPU_DEBUG_PHYS(p, fmt, ...) DRM_DEBUG_ATOMIC("enc%d intf%d pp%d " fmt,\
-		(p) ? (p)->parent->base.id : -1, \
-		(p) ? (p)->intf_idx - INTF_0 : -1, \
-		(p) ? ((p)->hw_pp ? (p)->hw_pp->idx - PINGPONG_0 : -1) : -1, \
-		##__VA_ARGS__)
-
-#define DPU_ERROR_PHYS(p, fmt, ...) DPU_ERROR("enc%d intf%d pp%d " fmt,\
-		(p) ? (p)->parent->base.id : -1, \
-		(p) ? (p)->intf_idx - INTF_0 : -1, \
-		(p) ? ((p)->hw_pp ? (p)->hw_pp->idx - PINGPONG_0 : -1) : -1, \
-		##__VA_ARGS__)
-
 /*
  * Two to anticipate panels that can do cmd/vid dynamic switching
  * plan is to create all possible physical encoder types, and switch between
@@ -262,12 +250,28 @@ static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bp
 	hw_pp->ops.setup_dither(hw_pp, &dither_cfg);
 }
 
+static char *dpu_encoder_helper_get_intf_type(enum dpu_intf_mode intf_mode)
+{
+	if (intf_mode == INTF_MODE_VIDEO)
+		return "INTF_MODE_VIDEO";
+	else if (intf_mode == INTF_MODE_CMD)
+		return "INTF_MODE_CMD";
+	else if (intf_mode == INTF_MODE_WB_BLOCK)
+		return "INTF_MODE_WB_BLOCK";
+	else if (intf_mode == INTF_MODE_WB_LINE)
+		return "INTF_MODE_WB_LINE";
+	else
+		return "INTF_MODE_UNKNOWN";
+}
+
 void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
 		enum dpu_intr_idx intr_idx)
 {
-	DRM_ERROR("irq timeout id=%u, intf=%d, pp=%d, intr=%d\n",
-		  DRMID(phys_enc->parent), phys_enc->intf_idx - INTF_0,
-		  phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
+	DRM_ERROR("irq timeout id=%u, intf_mode=%s intf=%d wb=%d, pp=%d, intr=%d\n",
+			DRMID(phys_enc->parent),
+			dpu_encoder_helper_get_intf_type(phys_enc->intf_mode),
+			phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
+			phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
 
 	if (phys_enc->parent_ops->handle_frame_done)
 		phys_enc->parent_ops->handle_frame_done(
@@ -2042,22 +2046,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
 
-		seq_printf(s, "intf:%d    vsync:%8d     underrun:%8d    ",
-				phys->intf_idx - INTF_0,
+		seq_printf(s, "intf:%d  wb:%d  vsync:%8d     underrun:%8d    ",
+				phys->intf_idx - INTF_0, phys->wb_idx - WB_0,
 				atomic_read(&phys->vsync_cnt),
 				atomic_read(&phys->underrun_cnt));
 
-		switch (phys->intf_mode) {
-		case INTF_MODE_VIDEO:
-			seq_puts(s, "mode: video\n");
-			break;
-		case INTF_MODE_CMD:
-			seq_puts(s, "mode: command\n");
-			break;
-		default:
-			seq_puts(s, "mode: ???\n");
-			break;
-		}
+		seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode));
 	}
 	mutex_unlock(&dpu_enc->enc_lock);
 
-- 
2.7.4

[RFC 4/4] drm/msm/dpu: add wb_idx to DRM traces in dpu_encoder

[Abhinav Kumar]

Change the DRM traces to include both the intf_mode
and wb_idx similar to the DRM prints in the previous change.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   | 26 ++++++++++++++++++--------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 871ce87..42affd3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1345,8 +1345,9 @@ static void dpu_encoder_frame_done_callback(
 			 * suppress frame_done without waiter,
 			 * likely autorefresh
 			 */
-			trace_dpu_enc_frame_done_cb_not_busy(DRMID(drm_enc),
-					event, ready_phys->intf_idx);
+			trace_dpu_enc_frame_done_cb_not_busy(DRMID(drm_enc), event,
+					dpu_encoder_helper_get_intf_type(ready_phys->intf_mode),
+					ready_phys->intf_idx, ready_phys->wb_idx);
 			return;
 		}
 
@@ -1424,9 +1425,11 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 	if (ctl->ops.get_pending_flush)
 		ret = ctl->ops.get_pending_flush(ctl);
 
-	trace_dpu_enc_trigger_flush(DRMID(drm_enc), phys->intf_idx,
-				    pending_kickoff_cnt, ctl->idx,
-				    extra_flush_bits, ret);
+	trace_dpu_enc_trigger_flush(DRMID(drm_enc),
+			dpu_encoder_helper_get_intf_type(phys->intf_mode),
+			phys->intf_idx, phys->wb_idx,
+			pending_kickoff_cnt, ctl->idx,
+			extra_flush_bits, ret);
 }
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
index 58b411f..1106d44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
@@ -380,20 +380,26 @@ TRACE_EVENT(dpu_enc_rc,
 );
 
 TRACE_EVENT(dpu_enc_frame_done_cb_not_busy,
-	TP_PROTO(uint32_t drm_id, u32 event, enum dpu_intf intf_idx),
-	TP_ARGS(drm_id, event, intf_idx),
+	TP_PROTO(uint32_t drm_id, u32 event, char *intf_mode, enum dpu_intf intf_idx,
+			enum dpu_wb wb_idx),
+	TP_ARGS(drm_id, event, intf_mode, intf_idx, wb_idx),
 	TP_STRUCT__entry(
 		__field(	uint32_t,	drm_id		)
 		__field(	u32,		event		)
+		__string(	intf_mode_str,		intf_mode	)
 		__field(	enum dpu_intf,	intf_idx	)
+		__field(    enum dpu_wb,  wb_idx    )
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
 		__entry->event = event;
+		__assign_str(intf_mode_str, intf_mode);
 		__entry->intf_idx = intf_idx;
+		__entry->wb_idx = wb_idx;
 	),
-	TP_printk("id=%u, event=%u, intf=%d", __entry->drm_id, __entry->event,
-		  __entry->intf_idx)
+	TP_printk("id=%u, event=%u, intf_mode=%s intf=%d wb=%d", __entry->drm_id,
+			__entry->event, __get_str(intf_mode_str),
+			__entry->intf_idx, __entry->wb_idx)
 );
 
 TRACE_EVENT(dpu_enc_frame_done_cb,
@@ -415,14 +421,16 @@ TRACE_EVENT(dpu_enc_frame_done_cb,
 );
 
 TRACE_EVENT(dpu_enc_trigger_flush,
-	TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
+	TP_PROTO(uint32_t drm_id, char *intf_mode, enum dpu_intf intf_idx, enum dpu_wb wb_idx,
 		 int pending_kickoff_cnt, int ctl_idx, u32 extra_flush_bits,
 		 u32 pending_flush_ret),
-	TP_ARGS(drm_id, intf_idx, pending_kickoff_cnt, ctl_idx,
+	TP_ARGS(drm_id, intf_mode, intf_idx, pending_kickoff_cnt, ctl_idx,
 		extra_flush_bits, pending_flush_ret),
 	TP_STRUCT__entry(
 		__field(	uint32_t,	drm_id			)
+		__string(	intf_mode_str,	intf_mode	)
 		__field(	enum dpu_intf,	intf_idx		)
+		__field(    enum dpu_wb,  wb_idx        )
 		__field(	int,		pending_kickoff_cnt	)
 		__field(	int,		ctl_idx			)
 		__field(	u32,		extra_flush_bits	)
@@ -430,15 +438,17 @@ TRACE_EVENT(dpu_enc_trigger_flush,
 	),
 	TP_fast_assign(
 		__entry->drm_id = drm_id;
+		__assign_str(intf_mode_str, intf_mode);
 		__entry->intf_idx = intf_idx;
+		__entry->wb_idx = wb_idx;
 		__entry->pending_kickoff_cnt = pending_kickoff_cnt;
 		__entry->ctl_idx = ctl_idx;
 		__entry->extra_flush_bits = extra_flush_bits;
 		__entry->pending_flush_ret = pending_flush_ret;
 	),
-	TP_printk("id=%u, intf_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
+	TP_printk("id=%u, intf_mode=%s, intf_idx=%d, wb_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
 		  "extra_flush_bits=0x%x pending_flush_ret=0x%x",
-		  __entry->drm_id, __entry->intf_idx,
+		  __entry->drm_id, __get_str(intf_mode_str), __entry->intf_idx, __entry->wb_idx,
 		  __entry->pending_kickoff_cnt, __entry->ctl_idx,
 		  __entry->extra_flush_bits, __entry->pending_flush_ret)
 );
-- 
2.7.4

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Dmitry Baryshkov]

On 21/04/2022 23:48, Abhinav Kumar wrote:
> Using intf_idx even for writeback interfaces is confusing
> because intf_idx is of type enum dpu_intf but the index used
> for writeback is of type enum dpu_wb.
> 
> In addition, this makes it easier to separately check the
> availability of the two as its possible that there are boards
> which don't have any physical display connected but can still
> work in writeback mode.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Looks good, two minor issues bellow.

With them fixed, I'd even squash this patch into the corresponding patch 
of the previous patchset.

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62 +++++++++++++-----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
>   3 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c12841..054d7e4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -962,7 +962,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>   	int num_lm, num_ctl, num_pp, num_dsc;
>   	unsigned int dsc_mask = 0;
> -	enum dpu_hw_blk_type blk_type;
>   	int i;
>   
>   	if (!drm_enc) {
> @@ -1044,17 +1043,11 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   		phys->hw_pp = dpu_enc->hw_pp[i];
>   		phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>   
> -		if (dpu_encoder_get_intf_mode(&dpu_enc->base) == INTF_MODE_WB_LINE)
> -			blk_type = DPU_HW_BLK_WB;
> -		else
> -			blk_type = DPU_HW_BLK_INTF;
> +		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> +			phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
>   
> -		if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> -			if (blk_type == DPU_HW_BLK_INTF)
> -				phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, phys->intf_idx);
> -			else if (blk_type == DPU_HW_BLK_WB)
> -				phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->intf_idx);
> -		}
> +		if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> +			phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
>   

We also need a check for if (phus->hw_intf && phys->hw_wb) HERE

>   		if (!phys->hw_intf && !phys->hw_wb) {
>   			DPU_ERROR_ENC(dpu_enc,
> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>   	mutex_unlock(&dpu_enc->enc_lock);
>   }
>   
> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
>   		enum dpu_intf_type type, u32 controller_id)
>   {
>   	int i = 0;
> @@ -1213,16 +1206,28 @@ static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
>   				return catalog->intf[i].id;
>   			}
>   		}
> -	} else {
> -		for (i = 0; i < catalog->wb_count; i++) {
> -			if (catalog->wb[i].id == controller_id)
> -				return catalog->wb[i].id;
> -		}
>   	}
>   
>   	return INTF_MAX;
>   }
>   
> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> +		enum dpu_intf_type type, u32 controller_id)
> +{
> +	int i = 0;
> +
> +	if (type != INTF_WB)
> +		goto end;
> +
> +	for (i = 0; i < catalog->wb_count; i++) {
> +		if (catalog->wb[i].id == controller_id)
> +			return catalog->wb[i].id;
> +	}
> +
> +end:
> +	return WB_MAX;

I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.

> +}
> +
>   static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>   		struct dpu_encoder_phys *phy_enc)
>   {
> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   		DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>   				i, controller_id, phys_params.split_role);
>   
> -		/*
> -		 * FIXME: have separate intf_idx and wb_idx to avoid using
> -		 * enum dpu_intf type for wb_idx and also to be able to
> -		 * not bail out when there is no intf for boards which dont
> -		 * have a display connected to them.
> -		 * Having a valid wb_idx but not a intf_idx can be a valid
> -		 * combination moving forward.
> -		 */
> -		phys_params.intf_idx = dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> +		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>   													intf_type,
>   													controller_id);
> -		if (phys_params.intf_idx == INTF_MAX) {
> +
> +		phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
> +				intf_type, controller_id);
> +		/*
> +		 * For boards which have no physical displays, having no interface
> +		 * is fine because it can still be used with just writeback.
> +		 * If we try without a display on a board which uses a DPU in which
> +		 * writeback is not supported, then this will still fail as it will not
> +		 * find any writeback in the catalog.
> +		 */
> +		if ((phys_params.intf_idx == INTF_MAX) &&
> +				(phys_params.wb_idx == WB_MAX)) {
>   			DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type %d, id %d\n",
>   						  intf_type, controller_id);
>   			ret = -EINVAL;
> 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 04d037e..f2af07d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
>    * @split_role:		Role to play in a split-panel configuration
>    * @intf_mode:		Interface mode
>    * @intf_idx:		Interface index on dpu hardware
> + * @wb_idx:			Writeback index on dpu hardware
>    * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
>    * @enable_state:	Enable state tracking
>    * @vblank_refcount:	Reference count of vblank request
> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
>   	enum dpu_enc_split_role split_role;
>   	enum dpu_intf_mode intf_mode;
>   	enum dpu_intf intf_idx;
> +	enum dpu_wb wb_idx;
>   	spinlock_t *enc_spinlock;
>   	enum dpu_enc_enable_state enable_state;
>   	atomic_t vblank_refcount;
> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
>    * @parent_ops:		Callbacks exposed by the parent to the phys_enc
>    * @split_role:		Role to play in a split-panel configuration
>    * @intf_idx:		Interface index this phys_enc will control
> + * @wb_idx:			Writeback index this phys_enc will control
>    * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
>    */
>   struct dpu_enc_phys_init_params {
> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
>   	const struct dpu_encoder_virt_ops *parent_ops;
>   	enum dpu_enc_split_role split_role;
>   	enum dpu_intf intf_idx;
> +	enum dpu_wb wb_idx;
>   	spinlock_t *enc_spinlock;
>   };
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> index ba82e54..2f34a31 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
>    * @rm: DPU Resource Manager handle
>    * @wb_idx: WB index
>    */
> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum dpu_intf wb_idx)
> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum dpu_wb wb_idx)
>   {
>   	return rm->hw_wb[wb_idx - WB_0];
>   }


-- 
With best wishes
Dmitry

Re: [RFC 3/4] drm/msm/dpu: add wb_idx to existing DRM prints in dpu_encoder

[Dmitry Baryshkov]

On 21/04/2022 23:48, Abhinav Kumar wrote:
> Add wb_idx to existing DRM prints in dpu_encoder and also
> print the intf_mode so that its clear that for any INTF_CMD/VID
> there will be a valid intf_idx and any INTF_WB_* there will be a
> valid wb_idx.
> 
> Update the debugfs to add the same information. Here is a sample
> output with this change:
> 
> root:/sys/kernel/debug/dri/0/encoder31# cat status
> intf:1  wb:-1  vsync: 31  underrun: 0    mode: INTF_MODE_VIDEO
> root:/sys/kernel/debug/dri/0/encoder33# cat status
> intf:-1  wb:2  vsync:  7  underrun: 0    mode: INTF_MODE_WB_LINE
> 
> Also remove DPU_DEBUG_PHYS macros as its unused because the
> respective dpu_encoder_phys_* files have their own macros.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 50 +++++++++++++----------------
>   1 file changed, 22 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 054d7e4..871ce87 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -37,18 +37,6 @@
>   #define DPU_ERROR_ENC(e, fmt, ...) DPU_ERROR("enc%d " fmt,\
>   		(e) ? (e)->base.base.id : -1, ##__VA_ARGS__)
>   
> -#define DPU_DEBUG_PHYS(p, fmt, ...) DRM_DEBUG_ATOMIC("enc%d intf%d pp%d " fmt,\
> -		(p) ? (p)->parent->base.id : -1, \
> -		(p) ? (p)->intf_idx - INTF_0 : -1, \
> -		(p) ? ((p)->hw_pp ? (p)->hw_pp->idx - PINGPONG_0 : -1) : -1, \
> -		##__VA_ARGS__)
> -
> -#define DPU_ERROR_PHYS(p, fmt, ...) DPU_ERROR("enc%d intf%d pp%d " fmt,\
> -		(p) ? (p)->parent->base.id : -1, \
> -		(p) ? (p)->intf_idx - INTF_0 : -1, \
> -		(p) ? ((p)->hw_pp ? (p)->hw_pp->idx - PINGPONG_0 : -1) : -1, \
> -		##__VA_ARGS__)
> -
>   /*
>    * Two to anticipate panels that can do cmd/vid dynamic switching
>    * plan is to create all possible physical encoder types, and switch between
> @@ -262,12 +250,28 @@ static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bp
>   	hw_pp->ops.setup_dither(hw_pp, &dither_cfg);
>   }
>   
> +static char *dpu_encoder_helper_get_intf_type(enum dpu_intf_mode intf_mode)
> +{

I'd rather convert this to switch() {...}

With that fixed:

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> +	if (intf_mode == INTF_MODE_VIDEO)
> +		return "INTF_MODE_VIDEO";
> +	else if (intf_mode == INTF_MODE_CMD)
> +		return "INTF_MODE_CMD";
> +	else if (intf_mode == INTF_MODE_WB_BLOCK)
> +		return "INTF_MODE_WB_BLOCK";
> +	else if (intf_mode == INTF_MODE_WB_LINE)
> +		return "INTF_MODE_WB_LINE";
> +	else
> +		return "INTF_MODE_UNKNOWN";
> +}
> +
>   void dpu_encoder_helper_report_irq_timeout(struct dpu_encoder_phys *phys_enc,
>   		enum dpu_intr_idx intr_idx)
>   {
> -	DRM_ERROR("irq timeout id=%u, intf=%d, pp=%d, intr=%d\n",
> -		  DRMID(phys_enc->parent), phys_enc->intf_idx - INTF_0,
> -		  phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
> +	DRM_ERROR("irq timeout id=%u, intf_mode=%s intf=%d wb=%d, pp=%d, intr=%d\n",
> +			DRMID(phys_enc->parent),
> +			dpu_encoder_helper_get_intf_type(phys_enc->intf_mode),
> +			phys_enc->intf_idx - INTF_0, phys_enc->wb_idx - WB_0,
> +			phys_enc->hw_pp->idx - PINGPONG_0, intr_idx);
>   
>   	if (phys_enc->parent_ops->handle_frame_done)
>   		phys_enc->parent_ops->handle_frame_done(
> @@ -2042,22 +2046,12 @@ static int _dpu_encoder_status_show(struct seq_file *s, void *data)
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>   		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>   
> -		seq_printf(s, "intf:%d    vsync:%8d     underrun:%8d    ",
> -				phys->intf_idx - INTF_0,
> +		seq_printf(s, "intf:%d  wb:%d  vsync:%8d     underrun:%8d    ",
> +				phys->intf_idx - INTF_0, phys->wb_idx - WB_0,
>   				atomic_read(&phys->vsync_cnt),
>   				atomic_read(&phys->underrun_cnt));
>   
> -		switch (phys->intf_mode) {
> -		case INTF_MODE_VIDEO:
> -			seq_puts(s, "mode: video\n");
> -			break;
> -		case INTF_MODE_CMD:
> -			seq_puts(s, "mode: command\n");
> -			break;
> -		default:
> -			seq_puts(s, "mode: ???\n");
> -			break;
> -		}
> +		seq_printf(s, "mode: %s\n", dpu_encoder_helper_get_intf_type(phys->intf_mode));
>   	}
>   	mutex_unlock(&dpu_enc->enc_lock);
>   


-- 
With best wishes
Dmitry

Re: [RFC 4/4] drm/msm/dpu: add wb_idx to DRM traces in dpu_encoder

[Dmitry Baryshkov]

On 21/04/2022 23:48, Abhinav Kumar wrote:
> Change the DRM traces to include both the intf_mode
> and wb_idx similar to the DRM prints in the previous change.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h   | 26 ++++++++++++++++++--------
>   2 files changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 871ce87..42affd3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1345,8 +1345,9 @@ static void dpu_encoder_frame_done_callback(
>   			 * suppress frame_done without waiter,
>   			 * likely autorefresh
>   			 */
> -			trace_dpu_enc_frame_done_cb_not_busy(DRMID(drm_enc),
> -					event, ready_phys->intf_idx);
> +			trace_dpu_enc_frame_done_cb_not_busy(DRMID(drm_enc), event,
> +					dpu_encoder_helper_get_intf_type(ready_phys->intf_mode),
> +					ready_phys->intf_idx, ready_phys->wb_idx);
>   			return;
>   		}
>   
> @@ -1424,9 +1425,11 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
>   	if (ctl->ops.get_pending_flush)
>   		ret = ctl->ops.get_pending_flush(ctl);
>   
> -	trace_dpu_enc_trigger_flush(DRMID(drm_enc), phys->intf_idx,
> -				    pending_kickoff_cnt, ctl->idx,
> -				    extra_flush_bits, ret);
> +	trace_dpu_enc_trigger_flush(DRMID(drm_enc),
> +			dpu_encoder_helper_get_intf_type(phys->intf_mode),
> +			phys->intf_idx, phys->wb_idx,
> +			pending_kickoff_cnt, ctl->idx,
> +			extra_flush_bits, ret);
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> index 58b411f..1106d44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h
> @@ -380,20 +380,26 @@ TRACE_EVENT(dpu_enc_rc,
>   );
>   
>   TRACE_EVENT(dpu_enc_frame_done_cb_not_busy,
> -	TP_PROTO(uint32_t drm_id, u32 event, enum dpu_intf intf_idx),
> -	TP_ARGS(drm_id, event, intf_idx),
> +	TP_PROTO(uint32_t drm_id, u32 event, char *intf_mode, enum dpu_intf intf_idx,
> +			enum dpu_wb wb_idx),
> +	TP_ARGS(drm_id, event, intf_mode, intf_idx, wb_idx),
>   	TP_STRUCT__entry(
>   		__field(	uint32_t,	drm_id		)
>   		__field(	u32,		event		)
> +		__string(	intf_mode_str,		intf_mode	)
>   		__field(	enum dpu_intf,	intf_idx	)
> +		__field(    enum dpu_wb,  wb_idx    )
>   	),
>   	TP_fast_assign(
>   		__entry->drm_id = drm_id;
>   		__entry->event = event;
> +		__assign_str(intf_mode_str, intf_mode);
>   		__entry->intf_idx = intf_idx;
> +		__entry->wb_idx = wb_idx;
>   	),
> -	TP_printk("id=%u, event=%u, intf=%d", __entry->drm_id, __entry->event,
> -		  __entry->intf_idx)
> +	TP_printk("id=%u, event=%u, intf_mode=%s intf=%d wb=%d", __entry->drm_id,
> +			__entry->event, __get_str(intf_mode_str),
> +			__entry->intf_idx, __entry->wb_idx)
>   );
>   
>   TRACE_EVENT(dpu_enc_frame_done_cb,
> @@ -415,14 +421,16 @@ TRACE_EVENT(dpu_enc_frame_done_cb,
>   );
>   
>   TRACE_EVENT(dpu_enc_trigger_flush,
> -	TP_PROTO(uint32_t drm_id, enum dpu_intf intf_idx,
> +	TP_PROTO(uint32_t drm_id, char *intf_mode, enum dpu_intf intf_idx, enum dpu_wb wb_idx,
>   		 int pending_kickoff_cnt, int ctl_idx, u32 extra_flush_bits,
>   		 u32 pending_flush_ret),
> -	TP_ARGS(drm_id, intf_idx, pending_kickoff_cnt, ctl_idx,
> +	TP_ARGS(drm_id, intf_mode, intf_idx, pending_kickoff_cnt, ctl_idx,
>   		extra_flush_bits, pending_flush_ret),
>   	TP_STRUCT__entry(
>   		__field(	uint32_t,	drm_id			)
> +		__string(	intf_mode_str,	intf_mode	)
>   		__field(	enum dpu_intf,	intf_idx		)
> +		__field(    enum dpu_wb,  wb_idx        )
>   		__field(	int,		pending_kickoff_cnt	)
>   		__field(	int,		ctl_idx			)
>   		__field(	u32,		extra_flush_bits	)
> @@ -430,15 +438,17 @@ TRACE_EVENT(dpu_enc_trigger_flush,
>   	),
>   	TP_fast_assign(
>   		__entry->drm_id = drm_id;
> +		__assign_str(intf_mode_str, intf_mode);
>   		__entry->intf_idx = intf_idx;
> +		__entry->wb_idx = wb_idx;
>   		__entry->pending_kickoff_cnt = pending_kickoff_cnt;
>   		__entry->ctl_idx = ctl_idx;
>   		__entry->extra_flush_bits = extra_flush_bits;
>   		__entry->pending_flush_ret = pending_flush_ret;
>   	),
> -	TP_printk("id=%u, intf_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
> +	TP_printk("id=%u, intf_mode=%s, intf_idx=%d, wb_idx=%d, pending_kickoff_cnt=%d ctl_idx=%d "
>   		  "extra_flush_bits=0x%x pending_flush_ret=0x%x",
> -		  __entry->drm_id, __entry->intf_idx,
> +		  __entry->drm_id, __get_str(intf_mode_str), __entry->intf_idx, __entry->wb_idx,
>   		  __entry->pending_kickoff_cnt, __entry->ctl_idx,
>   		  __entry->extra_flush_bits, __entry->pending_flush_ret)
>   );


-- 
With best wishes
Dmitry

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Abhinav Kumar]

Hi Dmitry

Thanks for the review.

One question below.

On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> On 21/04/2022 23:48, Abhinav Kumar wrote:
>> Using intf_idx even for writeback interfaces is confusing
>> because intf_idx is of type enum dpu_intf but the index used
>> for writeback is of type enum dpu_wb.
>>
>> In addition, this makes it easier to separately check the
>> availability of the two as its possible that there are boards
>> which don't have any physical display connected but can still
>> work in writeback mode.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> Looks good, two minor issues bellow.
> 
> With them fixed, I'd even squash this patch into the corresponding patch 
> of the previous patchset.
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62 
>> +++++++++++++-----------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
>>   3 files changed, 40 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 9c12841..054d7e4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -962,7 +962,6 @@ static void 
>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>       struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>>       int num_lm, num_ctl, num_pp, num_dsc;
>>       unsigned int dsc_mask = 0;
>> -    enum dpu_hw_blk_type blk_type;
>>       int i;
>>       if (!drm_enc) {
>> @@ -1044,17 +1043,11 @@ static void 
>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>           phys->hw_pp = dpu_enc->hw_pp[i];
>>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) == 
>> INTF_MODE_WB_LINE)
>> -            blk_type = DPU_HW_BLK_WB;
>> -        else
>> -            blk_type = DPU_HW_BLK_INTF;
>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> phys->intf_idx);
>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
>> -            if (blk_type == DPU_HW_BLK_INTF)
>> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm, 
>> phys->intf_idx);
>> -            else if (blk_type == DPU_HW_BLK_WB)
>> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, 
>> phys->intf_idx);
>> -        }
>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
> 
> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE

So there is an error if

1) Neither wb NOR intf are present
2) Both wb AND intf are present for the physical encoder?

The second check is okay for now to add but considering concurrent 
writeback then that wouldn't assumption be wrong since both physical and 
wb interfaces can go with the same encoder?

> 
>>           if (!phys->hw_intf && !phys->hw_wb) {
>>               DPU_ERROR_ENC(dpu_enc,
>> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct 
>> drm_encoder *drm_enc)
>>       mutex_unlock(&dpu_enc->enc_lock);
>>   }
>> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg 
>> *catalog,
>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
>>           enum dpu_intf_type type, u32 controller_id)
>>   {
>>       int i = 0;
>> @@ -1213,16 +1206,28 @@ static enum dpu_intf 
>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
>>                   return catalog->intf[i].id;
>>               }
>>           }
>> -    } else {
>> -        for (i = 0; i < catalog->wb_count; i++) {
>> -            if (catalog->wb[i].id == controller_id)
>> -                return catalog->wb[i].id;
>> -        }
>>       }
>>       return INTF_MAX;
>>   }
>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
>> +        enum dpu_intf_type type, u32 controller_id)
>> +{
>> +    int i = 0;
>> +
>> +    if (type != INTF_WB)
>> +        goto end;
>> +
>> +    for (i = 0; i < catalog->wb_count; i++) {
>> +        if (catalog->wb[i].id == controller_id)
>> +            return catalog->wb[i].id;
>> +    }
>> +
>> +end:
>> +    return WB_MAX;
> 
> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
ack, i guess in that case even the places checking the return value of 
this function need to be changed.
> 
>> +}
>> +
>>   static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>           struct dpu_encoder_phys *phy_enc)
>>   {
>> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>           DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>                   i, controller_id, phys_params.split_role);
>> -        /*
>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
>> -         * enum dpu_intf type for wb_idx and also to be able to
>> -         * not bail out when there is no intf for boards which dont
>> -         * have a display connected to them.
>> -         * Having a valid wb_idx but not a intf_idx can be a valid
>> -         * combination moving forward.
>> -         */
>> -        phys_params.intf_idx = 
>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>                                                       intf_type,
>>                                                       controller_id);
>> -        if (phys_params.intf_idx == INTF_MAX) {
>> +
>> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>> +                intf_type, controller_id);
>> +        /*
>> +         * For boards which have no physical displays, having no 
>> interface
>> +         * is fine because it can still be used with just writeback.
>> +         * If we try without a display on a board which uses a DPU in 
>> which
>> +         * writeback is not supported, then this will still fail as 
>> it will not
>> +         * find any writeback in the catalog.
>> +         */
>> +        if ((phys_params.intf_idx == INTF_MAX) &&
>> +                (phys_params.wb_idx == WB_MAX)) {
>>               DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type 
>> %d, id %d\n",
>>                             intf_type, controller_id);
>>               ret = -EINVAL;
>> 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 04d037e..f2af07d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
>>    * @split_role:        Role to play in a split-panel configuration
>>    * @intf_mode:        Interface mode
>>    * @intf_idx:        Interface index on dpu hardware
>> + * @wb_idx:            Writeback index on dpu hardware
>>    * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>    * @enable_state:    Enable state tracking
>>    * @vblank_refcount:    Reference count of vblank request
>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
>>       enum dpu_enc_split_role split_role;
>>       enum dpu_intf_mode intf_mode;
>>       enum dpu_intf intf_idx;
>> +    enum dpu_wb wb_idx;
>>       spinlock_t *enc_spinlock;
>>       enum dpu_enc_enable_state enable_state;
>>       atomic_t vblank_refcount;
>> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
>>    * @parent_ops:        Callbacks exposed by the parent to the phys_enc
>>    * @split_role:        Role to play in a split-panel configuration
>>    * @intf_idx:        Interface index this phys_enc will control
>> + * @wb_idx:            Writeback index this phys_enc will control
>>    * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>    */
>>   struct dpu_enc_phys_init_params {
>> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
>>       const struct dpu_encoder_virt_ops *parent_ops;
>>       enum dpu_enc_split_role split_role;
>>       enum dpu_intf intf_idx;
>> +    enum dpu_wb wb_idx;
>>       spinlock_t *enc_spinlock;
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> index ba82e54..2f34a31 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf 
>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
>>    * @rm: DPU Resource Manager handle
>>    * @wb_idx: WB index
>>    */
>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum 
>> dpu_intf wb_idx)
>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum 
>> dpu_wb wb_idx)
>>   {
>>       return rm->hw_wb[wb_idx - WB_0];
>>   }
> 
> 

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Dmitry
>
> Thanks for the review.
>
> One question below.
>
> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> > On 21/04/2022 23:48, Abhinav Kumar wrote:
> >> Using intf_idx even for writeback interfaces is confusing
> >> because intf_idx is of type enum dpu_intf but the index used
> >> for writeback is of type enum dpu_wb.
> >>
> >> In addition, this makes it easier to separately check the
> >> availability of the two as its possible that there are boards
> >> which don't have any physical display connected but can still
> >> work in writeback mode.
> >>
> >> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >
> > Looks good, two minor issues bellow.
> >
> > With them fixed, I'd even squash this patch into the corresponding patch
> > of the previous patchset.
> >
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
> >> +++++++++++++-----------
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
> >>   3 files changed, 40 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 9c12841..054d7e4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -962,7 +962,6 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>       struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >>       int num_lm, num_ctl, num_pp, num_dsc;
> >>       unsigned int dsc_mask = 0;
> >> -    enum dpu_hw_blk_type blk_type;
> >>       int i;
> >>       if (!drm_enc) {
> >> @@ -1044,17 +1043,11 @@ static void
> >> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>           phys->hw_pp = dpu_enc->hw_pp[i];
> >>           phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
> >> INTF_MODE_WB_LINE)
> >> -            blk_type = DPU_HW_BLK_WB;
> >> -        else
> >> -            blk_type = DPU_HW_BLK_INTF;
> >> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> >> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >> phys->intf_idx);
> >> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >> -            if (blk_type == DPU_HW_BLK_INTF)
> >> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >> phys->intf_idx);
> >> -            else if (blk_type == DPU_HW_BLK_WB)
> >> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
> >> phys->intf_idx);
> >> -        }
> >> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
> >
> > We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
>
> So there is an error if
>
> 1) Neither wb NOR intf are present
> 2) Both wb AND intf are present for the physical encoder?
>
> The second check is okay for now to add but considering concurrent
> writeback then that wouldn't assumption be wrong since both physical and
> wb interfaces can go with the same encoder?

To the same encoder, but not to the same physical encoder. Here we
check the phys_enc parameters.

>
> >
> >>           if (!phys->hw_intf && !phys->hw_wb) {
> >>               DPU_ERROR_ENC(dpu_enc,
> >> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> >> drm_encoder *drm_enc)
> >>       mutex_unlock(&dpu_enc->enc_lock);
> >>   }
> >> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >> *catalog,
> >> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >>           enum dpu_intf_type type, u32 controller_id)
> >>   {
> >>       int i = 0;
> >> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >>                   return catalog->intf[i].id;
> >>               }
> >>           }
> >> -    } else {
> >> -        for (i = 0; i < catalog->wb_count; i++) {
> >> -            if (catalog->wb[i].id == controller_id)
> >> -                return catalog->wb[i].id;
> >> -        }
> >>       }
> >>       return INTF_MAX;
> >>   }
> >> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> >> +        enum dpu_intf_type type, u32 controller_id)
> >> +{
> >> +    int i = 0;
> >> +
> >> +    if (type != INTF_WB)
> >> +        goto end;
> >> +
> >> +    for (i = 0; i < catalog->wb_count; i++) {
> >> +        if (catalog->wb[i].id == controller_id)
> >> +            return catalog->wb[i].id;
> >> +    }
> >> +
> >> +end:
> >> +    return WB_MAX;
> >
> > I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
> ack, i guess in that case even the places checking the return value of
> this function need to be changed.

Yes, of course.

> >
> >> +}
> >> +
> >>   static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >>           struct dpu_encoder_phys *phy_enc)
> >>   {
> >> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
> >> dpu_encoder_virt *dpu_enc,
> >>           DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>                   i, controller_id, phys_params.split_role);
> >> -        /*
> >> -         * FIXME: have separate intf_idx and wb_idx to avoid using
> >> -         * enum dpu_intf type for wb_idx and also to be able to
> >> -         * not bail out when there is no intf for boards which dont
> >> -         * have a display connected to them.
> >> -         * Having a valid wb_idx but not a intf_idx can be a valid
> >> -         * combination moving forward.
> >> -         */
> >> -        phys_params.intf_idx =
> >> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> >> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> >>                                                       intf_type,
> >>                                                       controller_id);
> >> -        if (phys_params.intf_idx == INTF_MAX) {
> >> +
> >> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
> >> +                intf_type, controller_id);
> >> +        /*
> >> +         * For boards which have no physical displays, having no
> >> interface
> >> +         * is fine because it can still be used with just writeback.
> >> +         * If we try without a display on a board which uses a DPU in
> >> which
> >> +         * writeback is not supported, then this will still fail as
> >> it will not
> >> +         * find any writeback in the catalog.
> >> +         */
> >> +        if ((phys_params.intf_idx == INTF_MAX) &&
> >> +                (phys_params.wb_idx == WB_MAX)) {
> >>               DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type
> >> %d, id %d\n",
> >>                             intf_type, controller_id);
> >>               ret = -EINVAL;
> >> 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 04d037e..f2af07d 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
> >>    * @split_role:        Role to play in a split-panel configuration
> >>    * @intf_mode:        Interface mode
> >>    * @intf_idx:        Interface index on dpu hardware
> >> + * @wb_idx:            Writeback index on dpu hardware
> >>    * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>    * @enable_state:    Enable state tracking
> >>    * @vblank_refcount:    Reference count of vblank request
> >> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
> >>       enum dpu_enc_split_role split_role;
> >>       enum dpu_intf_mode intf_mode;
> >>       enum dpu_intf intf_idx;
> >> +    enum dpu_wb wb_idx;
> >>       spinlock_t *enc_spinlock;
> >>       enum dpu_enc_enable_state enable_state;
> >>       atomic_t vblank_refcount;
> >> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
> >>    * @parent_ops:        Callbacks exposed by the parent to the phys_enc
> >>    * @split_role:        Role to play in a split-panel configuration
> >>    * @intf_idx:        Interface index this phys_enc will control
> >> + * @wb_idx:            Writeback index this phys_enc will control
> >>    * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>    */
> >>   struct dpu_enc_phys_init_params {
> >> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
> >>       const struct dpu_encoder_virt_ops *parent_ops;
> >>       enum dpu_enc_split_role split_role;
> >>       enum dpu_intf intf_idx;
> >> +    enum dpu_wb wb_idx;
> >>       spinlock_t *enc_spinlock;
> >>   };
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >> index ba82e54..2f34a31 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
> >> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
> >>    * @rm: DPU Resource Manager handle
> >>    * @wb_idx: WB index
> >>    */
> >> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >> dpu_intf wb_idx)
> >> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >> dpu_wb wb_idx)
> >>   {
> >>       return rm->hw_wb[wb_idx - WB_0];
> >>   }
> >
> >



-- 
With best wishes
Dmitry

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Abhinav Kumar]

On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>> Hi Dmitry
>>
>> Thanks for the review.
>>
>> One question below.
>>
>> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
>>> On 21/04/2022 23:48, Abhinav Kumar wrote:
>>>> Using intf_idx even for writeback interfaces is confusing
>>>> because intf_idx is of type enum dpu_intf but the index used
>>>> for writeback is of type enum dpu_wb.
>>>>
>>>> In addition, this makes it easier to separately check the
>>>> availability of the two as its possible that there are boards
>>>> which don't have any physical display connected but can still
>>>> work in writeback mode.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>
>>> Looks good, two minor issues bellow.
>>>
>>> With them fixed, I'd even squash this patch into the corresponding patch
>>> of the previous patchset.
>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
>>>> +++++++++++++-----------
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
>>>>    3 files changed, 40 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 9c12841..054d7e4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -962,7 +962,6 @@ static void
>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>        struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>>>>        int num_lm, num_ctl, num_pp, num_dsc;
>>>>        unsigned int dsc_mask = 0;
>>>> -    enum dpu_hw_blk_type blk_type;
>>>>        int i;
>>>>        if (!drm_enc) {
>>>> @@ -1044,17 +1043,11 @@ static void
>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>            phys->hw_pp = dpu_enc->hw_pp[i];
>>>>            phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
>>>> INTF_MODE_WB_LINE)
>>>> -            blk_type = DPU_HW_BLK_WB;
>>>> -        else
>>>> -            blk_type = DPU_HW_BLK_INTF;
>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
>>>> phys->intf_idx);
>>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
>>>> -            if (blk_type == DPU_HW_BLK_INTF)
>>>> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
>>>> phys->intf_idx);
>>>> -            else if (blk_type == DPU_HW_BLK_WB)
>>>> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
>>>> phys->intf_idx);
>>>> -        }
>>>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
>>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
>>>
>>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
>>
>> So there is an error if
>>
>> 1) Neither wb NOR intf are present
>> 2) Both wb AND intf are present for the physical encoder?
>>
>> The second check is okay for now to add but considering concurrent
>> writeback then that wouldn't assumption be wrong since both physical and
>> wb interfaces can go with the same encoder?
> 
> To the same encoder, but not to the same physical encoder. Here we
> check the phys_enc parameters.

Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
Get the acks on them.

Then will absorb into WB series and re-post it.

> 
>>
>>>
>>>>            if (!phys->hw_intf && !phys->hw_wb) {
>>>>                DPU_ERROR_ENC(dpu_enc,
>>>> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
>>>> drm_encoder *drm_enc)
>>>>        mutex_unlock(&dpu_enc->enc_lock);
>>>>    }
>>>> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
>>>> *catalog,
>>>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
>>>>            enum dpu_intf_type type, u32 controller_id)
>>>>    {
>>>>        int i = 0;
>>>> @@ -1213,16 +1206,28 @@ static enum dpu_intf
>>>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
>>>>                    return catalog->intf[i].id;
>>>>                }
>>>>            }
>>>> -    } else {
>>>> -        for (i = 0; i < catalog->wb_count; i++) {
>>>> -            if (catalog->wb[i].id == controller_id)
>>>> -                return catalog->wb[i].id;
>>>> -        }
>>>>        }
>>>>        return INTF_MAX;
>>>>    }
>>>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
>>>> +        enum dpu_intf_type type, u32 controller_id)
>>>> +{
>>>> +    int i = 0;
>>>> +
>>>> +    if (type != INTF_WB)
>>>> +        goto end;
>>>> +
>>>> +    for (i = 0; i < catalog->wb_count; i++) {
>>>> +        if (catalog->wb[i].id == controller_id)
>>>> +            return catalog->wb[i].id;
>>>> +    }
>>>> +
>>>> +end:
>>>> +    return WB_MAX;
>>>
>>> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
>> ack, i guess in that case even the places checking the return value of
>> this function need to be changed.
> 
> Yes, of course.
> 
>>>
>>>> +}
>>>> +
>>>>    static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>>>            struct dpu_encoder_phys *phy_enc)
>>>>    {
>>>> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
>>>> dpu_encoder_virt *dpu_enc,
>>>>            DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>                    i, controller_id, phys_params.split_role);
>>>> -        /*
>>>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
>>>> -         * enum dpu_intf type for wb_idx and also to be able to
>>>> -         * not bail out when there is no intf for boards which dont
>>>> -         * have a display connected to them.
>>>> -         * Having a valid wb_idx but not a intf_idx can be a valid
>>>> -         * combination moving forward.
>>>> -         */
>>>> -        phys_params.intf_idx =
>>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
>>>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>>>                                                        intf_type,
>>>>                                                        controller_id);
>>>> -        if (phys_params.intf_idx == INTF_MAX) {
>>>> +
>>>> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>>>> +                intf_type, controller_id);
>>>> +        /*
>>>> +         * For boards which have no physical displays, having no
>>>> interface
>>>> +         * is fine because it can still be used with just writeback.
>>>> +         * If we try without a display on a board which uses a DPU in
>>>> which
>>>> +         * writeback is not supported, then this will still fail as
>>>> it will not
>>>> +         * find any writeback in the catalog.
>>>> +         */
>>>> +        if ((phys_params.intf_idx == INTF_MAX) &&
>>>> +                (phys_params.wb_idx == WB_MAX)) {
>>>>                DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type
>>>> %d, id %d\n",
>>>>                              intf_type, controller_id);
>>>>                ret = -EINVAL;
>>>> 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 04d037e..f2af07d 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
>>>>     * @split_role:        Role to play in a split-panel configuration
>>>>     * @intf_mode:        Interface mode
>>>>     * @intf_idx:        Interface index on dpu hardware
>>>> + * @wb_idx:            Writeback index on dpu hardware
>>>>     * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>>>     * @enable_state:    Enable state tracking
>>>>     * @vblank_refcount:    Reference count of vblank request
>>>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
>>>>        enum dpu_enc_split_role split_role;
>>>>        enum dpu_intf_mode intf_mode;
>>>>        enum dpu_intf intf_idx;
>>>> +    enum dpu_wb wb_idx;
>>>>        spinlock_t *enc_spinlock;
>>>>        enum dpu_enc_enable_state enable_state;
>>>>        atomic_t vblank_refcount;
>>>> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
>>>>     * @parent_ops:        Callbacks exposed by the parent to the phys_enc
>>>>     * @split_role:        Role to play in a split-panel configuration
>>>>     * @intf_idx:        Interface index this phys_enc will control
>>>> + * @wb_idx:            Writeback index this phys_enc will control
>>>>     * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>>>     */
>>>>    struct dpu_enc_phys_init_params {
>>>> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
>>>>        const struct dpu_encoder_virt_ops *parent_ops;
>>>>        enum dpu_enc_split_role split_role;
>>>>        enum dpu_intf intf_idx;
>>>> +    enum dpu_wb wb_idx;
>>>>        spinlock_t *enc_spinlock;
>>>>    };
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> index ba82e54..2f34a31 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
>>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
>>>>     * @rm: DPU Resource Manager handle
>>>>     * @wb_idx: WB index
>>>>     */
>>>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
>>>> dpu_intf wb_idx)
>>>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
>>>> dpu_wb wb_idx)
>>>>    {
>>>>        return rm->hw_wb[wb_idx - WB_0];
>>>>    }
>>>
>>>
> 
> 
> 

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> > On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >> Hi Dmitry
> >>
> >> Thanks for the review.
> >>
> >> One question below.
> >>
> >> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> >>> On 21/04/2022 23:48, Abhinav Kumar wrote:
> >>>> Using intf_idx even for writeback interfaces is confusing
> >>>> because intf_idx is of type enum dpu_intf but the index used
> >>>> for writeback is of type enum dpu_wb.
> >>>>
> >>>> In addition, this makes it easier to separately check the
> >>>> availability of the two as its possible that there are boards
> >>>> which don't have any physical display connected but can still
> >>>> work in writeback mode.
> >>>>
> >>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>
> >>> Looks good, two minor issues bellow.
> >>>
> >>> With them fixed, I'd even squash this patch into the corresponding patch
> >>> of the previous patchset.
> >>>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
> >>>> +++++++++++++-----------
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
> >>>>    3 files changed, 40 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> index 9c12841..054d7e4 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>> @@ -962,7 +962,6 @@ static void
> >>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>>        struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >>>>        int num_lm, num_ctl, num_pp, num_dsc;
> >>>>        unsigned int dsc_mask = 0;
> >>>> -    enum dpu_hw_blk_type blk_type;
> >>>>        int i;
> >>>>        if (!drm_enc) {
> >>>> @@ -1044,17 +1043,11 @@ static void
> >>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>>            phys->hw_pp = dpu_enc->hw_pp[i];
> >>>>            phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >>>> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
> >>>> INTF_MODE_WB_LINE)
> >>>> -            blk_type = DPU_HW_BLK_WB;
> >>>> -        else
> >>>> -            blk_type = DPU_HW_BLK_INTF;
> >>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> >>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >>>> phys->intf_idx);
> >>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >>>> -            if (blk_type == DPU_HW_BLK_INTF)
> >>>> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >>>> phys->intf_idx);
> >>>> -            else if (blk_type == DPU_HW_BLK_WB)
> >>>> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
> >>>> phys->intf_idx);
> >>>> -        }
> >>>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
> >>>
> >>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
> >>
> >> So there is an error if
> >>
> >> 1) Neither wb NOR intf are present
> >> 2) Both wb AND intf are present for the physical encoder?
> >>
> >> The second check is okay for now to add but considering concurrent
> >> writeback then that wouldn't assumption be wrong since both physical and
> >> wb interfaces can go with the same encoder?
> >
> > To the same encoder, but not to the same physical encoder. Here we
> > check the phys_enc parameters.
>
> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
> Get the acks on them.
>
> Then will absorb into WB series and re-post it.

Sounds like a good plan!

>
> >
> >>
> >>>
> >>>>            if (!phys->hw_intf && !phys->hw_wb) {
> >>>>                DPU_ERROR_ENC(dpu_enc,
> >>>> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> >>>> drm_encoder *drm_enc)
> >>>>        mutex_unlock(&dpu_enc->enc_lock);
> >>>>    }
> >>>> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >>>> *catalog,
> >>>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >>>>            enum dpu_intf_type type, u32 controller_id)
> >>>>    {
> >>>>        int i = 0;
> >>>> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >>>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >>>>                    return catalog->intf[i].id;
> >>>>                }
> >>>>            }
> >>>> -    } else {
> >>>> -        for (i = 0; i < catalog->wb_count; i++) {
> >>>> -            if (catalog->wb[i].id == controller_id)
> >>>> -                return catalog->wb[i].id;
> >>>> -        }
> >>>>        }
> >>>>        return INTF_MAX;
> >>>>    }
> >>>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> >>>> +        enum dpu_intf_type type, u32 controller_id)
> >>>> +{
> >>>> +    int i = 0;
> >>>> +
> >>>> +    if (type != INTF_WB)
> >>>> +        goto end;
> >>>> +
> >>>> +    for (i = 0; i < catalog->wb_count; i++) {
> >>>> +        if (catalog->wb[i].id == controller_id)
> >>>> +            return catalog->wb[i].id;
> >>>> +    }
> >>>> +
> >>>> +end:
> >>>> +    return WB_MAX;
> >>>
> >>> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
> >> ack, i guess in that case even the places checking the return value of
> >> this function need to be changed.
> >
> > Yes, of course.
> >
> >>>
> >>>> +}
> >>>> +
> >>>>    static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >>>>            struct dpu_encoder_phys *phy_enc)
> >>>>    {
> >>>> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
> >>>> dpu_encoder_virt *dpu_enc,
> >>>>            DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>>>                    i, controller_id, phys_params.split_role);
> >>>> -        /*
> >>>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
> >>>> -         * enum dpu_intf type for wb_idx and also to be able to
> >>>> -         * not bail out when there is no intf for boards which dont
> >>>> -         * have a display connected to them.
> >>>> -         * Having a valid wb_idx but not a intf_idx can be a valid
> >>>> -         * combination moving forward.
> >>>> -         */
> >>>> -        phys_params.intf_idx =
> >>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> >>>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> >>>>                                                        intf_type,
> >>>>                                                        controller_id);
> >>>> -        if (phys_params.intf_idx == INTF_MAX) {
> >>>> +
> >>>> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
> >>>> +                intf_type, controller_id);
> >>>> +        /*
> >>>> +         * For boards which have no physical displays, having no
> >>>> interface
> >>>> +         * is fine because it can still be used with just writeback.
> >>>> +         * If we try without a display on a board which uses a DPU in
> >>>> which
> >>>> +         * writeback is not supported, then this will still fail as
> >>>> it will not
> >>>> +         * find any writeback in the catalog.
> >>>> +         */
> >>>> +        if ((phys_params.intf_idx == INTF_MAX) &&
> >>>> +                (phys_params.wb_idx == WB_MAX)) {
> >>>>                DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type
> >>>> %d, id %d\n",
> >>>>                              intf_type, controller_id);
> >>>>                ret = -EINVAL;
> >>>> 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 04d037e..f2af07d 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
> >>>>     * @split_role:        Role to play in a split-panel configuration
> >>>>     * @intf_mode:        Interface mode
> >>>>     * @intf_idx:        Interface index on dpu hardware
> >>>> + * @wb_idx:            Writeback index on dpu hardware
> >>>>     * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>>>     * @enable_state:    Enable state tracking
> >>>>     * @vblank_refcount:    Reference count of vblank request
> >>>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
> >>>>        enum dpu_enc_split_role split_role;
> >>>>        enum dpu_intf_mode intf_mode;
> >>>>        enum dpu_intf intf_idx;
> >>>> +    enum dpu_wb wb_idx;
> >>>>        spinlock_t *enc_spinlock;
> >>>>        enum dpu_enc_enable_state enable_state;
> >>>>        atomic_t vblank_refcount;
> >>>> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
> >>>>     * @parent_ops:        Callbacks exposed by the parent to the phys_enc
> >>>>     * @split_role:        Role to play in a split-panel configuration
> >>>>     * @intf_idx:        Interface index this phys_enc will control
> >>>> + * @wb_idx:            Writeback index this phys_enc will control
> >>>>     * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>>>     */
> >>>>    struct dpu_enc_phys_init_params {
> >>>> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
> >>>>        const struct dpu_encoder_virt_ops *parent_ops;
> >>>>        enum dpu_enc_split_role split_role;
> >>>>        enum dpu_intf intf_idx;
> >>>> +    enum dpu_wb wb_idx;
> >>>>        spinlock_t *enc_spinlock;
> >>>>    };
> >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>> index ba82e54..2f34a31 100644
> >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
> >>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
> >>>>     * @rm: DPU Resource Manager handle
> >>>>     * @wb_idx: WB index
> >>>>     */
> >>>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>> dpu_intf wb_idx)
> >>>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>> dpu_wb wb_idx)
> >>>>    {
> >>>>        return rm->hw_wb[wb_idx - WB_0];
> >>>>    }
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Abhinav Kumar]

Hi Dmitry

On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
> On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
>>> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>> Hi Dmitry
>>>>
>>>> Thanks for the review.
>>>>
>>>> One question below.
>>>>
>>>> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
>>>>> On 21/04/2022 23:48, Abhinav Kumar wrote:
>>>>>> Using intf_idx even for writeback interfaces is confusing
>>>>>> because intf_idx is of type enum dpu_intf but the index used
>>>>>> for writeback is of type enum dpu_wb.
>>>>>>
>>>>>> In addition, this makes it easier to separately check the
>>>>>> availability of the two as its possible that there are boards
>>>>>> which don't have any physical display connected but can still
>>>>>> work in writeback mode.
>>>>>>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>
>>>>> Looks good, two minor issues bellow.
>>>>>
>>>>> With them fixed, I'd even squash this patch into the corresponding patch
>>>>> of the previous patchset.
>>>>>
>>>>>> ---
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
>>>>>> +++++++++++++-----------
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
>>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
>>>>>>     3 files changed, 40 insertions(+), 28 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> index 9c12841..054d7e4 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>>>> @@ -962,7 +962,6 @@ static void
>>>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>>>         struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>>>>>>         int num_lm, num_ctl, num_pp, num_dsc;
>>>>>>         unsigned int dsc_mask = 0;
>>>>>> -    enum dpu_hw_blk_type blk_type;
>>>>>>         int i;
>>>>>>         if (!drm_enc) {
>>>>>> @@ -1044,17 +1043,11 @@ static void
>>>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>>>             phys->hw_pp = dpu_enc->hw_pp[i];
>>>>>>             phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
>>>>>> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
>>>>>> INTF_MODE_WB_LINE)
>>>>>> -            blk_type = DPU_HW_BLK_WB;
>>>>>> -        else
>>>>>> -            blk_type = DPU_HW_BLK_INTF;
>>>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
>>>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
>>>>>> phys->intf_idx);
>>>>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
>>>>>> -            if (blk_type == DPU_HW_BLK_INTF)
>>>>>> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
>>>>>> phys->intf_idx);
>>>>>> -            else if (blk_type == DPU_HW_BLK_WB)
>>>>>> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
>>>>>> phys->intf_idx);
>>>>>> -        }
>>>>>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
>>>>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
>>>>>
>>>>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
>>>>
>>>> So there is an error if
>>>>
>>>> 1) Neither wb NOR intf are present
>>>> 2) Both wb AND intf are present for the physical encoder?
>>>>
>>>> The second check is okay for now to add but considering concurrent
>>>> writeback then that wouldn't assumption be wrong since both physical and
>>>> wb interfaces can go with the same encoder?
>>>
>>> To the same encoder, but not to the same physical encoder. Here we
>>> check the phys_enc parameters.
>>
>> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
>> Get the acks on them.
>>
>> Then will absorb into WB series and re-post it.
> 
> Sounds like a good plan!
> 
>>
>>>
>>>>
>>>>>
>>>>>>             if (!phys->hw_intf && !phys->hw_wb) {
>>>>>>                 DPU_ERROR_ENC(dpu_enc,
>>>>>> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
>>>>>> drm_encoder *drm_enc)
>>>>>>         mutex_unlock(&dpu_enc->enc_lock);
>>>>>>     }
>>>>>> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
>>>>>> *catalog,
>>>>>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
>>>>>>             enum dpu_intf_type type, u32 controller_id)
>>>>>>     {
>>>>>>         int i = 0;
>>>>>> @@ -1213,16 +1206,28 @@ static enum dpu_intf
>>>>>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
>>>>>>                     return catalog->intf[i].id;
>>>>>>                 }
>>>>>>             }
>>>>>> -    } else {
>>>>>> -        for (i = 0; i < catalog->wb_count; i++) {
>>>>>> -            if (catalog->wb[i].id == controller_id)
>>>>>> -                return catalog->wb[i].id;
>>>>>> -        }
>>>>>>         }
>>>>>>         return INTF_MAX;
>>>>>>     }
>>>>>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
>>>>>> +        enum dpu_intf_type type, u32 controller_id)
>>>>>> +{
>>>>>> +    int i = 0;
>>>>>> +
>>>>>> +    if (type != INTF_WB)
>>>>>> +        goto end;
>>>>>> +
>>>>>> +    for (i = 0; i < catalog->wb_count; i++) {
>>>>>> +        if (catalog->wb[i].id == controller_id)
>>>>>> +            return catalog->wb[i].id;
>>>>>> +    }
>>>>>> +
>>>>>> +end:
>>>>>> +    return WB_MAX;
>>>>>
>>>>> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
>>>> ack, i guess in that case even the places checking the return value of
>>>> this function need to be changed.
>>>
>>> Yes, of course.

INTF_NONE/WB_NONE is not of enum dpu_intf or enum dpu_wb, its of type 
enum dpu_intf_mode

Do we want to add them to dpu_intf/dpu_wb with a -1 value OR leave it 
as-it-is.

>>>
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>     static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>>>>>>             struct dpu_encoder_phys *phy_enc)
>>>>>>     {
>>>>>> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
>>>>>> dpu_encoder_virt *dpu_enc,
>>>>>>             DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
>>>>>>                     i, controller_id, phys_params.split_role);
>>>>>> -        /*
>>>>>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
>>>>>> -         * enum dpu_intf type for wb_idx and also to be able to
>>>>>> -         * not bail out when there is no intf for boards which dont
>>>>>> -         * have a display connected to them.
>>>>>> -         * Having a valid wb_idx but not a intf_idx can be a valid
>>>>>> -         * combination moving forward.
>>>>>> -         */
>>>>>> -        phys_params.intf_idx =
>>>>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
>>>>>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>>>>>                                                         intf_type,
>>>>>>                                                         controller_id);
>>>>>> -        if (phys_params.intf_idx == INTF_MAX) {
>>>>>> +
>>>>>> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
>>>>>> +                intf_type, controller_id);
>>>>>> +        /*
>>>>>> +         * For boards which have no physical displays, having no
>>>>>> interface
>>>>>> +         * is fine because it can still be used with just writeback.
>>>>>> +         * If we try without a display on a board which uses a DPU in
>>>>>> which
>>>>>> +         * writeback is not supported, then this will still fail as
>>>>>> it will not
>>>>>> +         * find any writeback in the catalog.
>>>>>> +         */
>>>>>> +        if ((phys_params.intf_idx == INTF_MAX) &&
>>>>>> +                (phys_params.wb_idx == WB_MAX)) {
>>>>>>                 DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type
>>>>>> %d, id %d\n",
>>>>>>                               intf_type, controller_id);
>>>>>>                 ret = -EINVAL;
>>>>>> 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 04d037e..f2af07d 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>>>>>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
>>>>>>      * @split_role:        Role to play in a split-panel configuration
>>>>>>      * @intf_mode:        Interface mode
>>>>>>      * @intf_idx:        Interface index on dpu hardware
>>>>>> + * @wb_idx:            Writeback index on dpu hardware
>>>>>>      * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>>>>>      * @enable_state:    Enable state tracking
>>>>>>      * @vblank_refcount:    Reference count of vblank request
>>>>>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
>>>>>>         enum dpu_enc_split_role split_role;
>>>>>>         enum dpu_intf_mode intf_mode;
>>>>>>         enum dpu_intf intf_idx;
>>>>>> +    enum dpu_wb wb_idx;
>>>>>>         spinlock_t *enc_spinlock;
>>>>>>         enum dpu_enc_enable_state enable_state;
>>>>>>         atomic_t vblank_refcount;
>>>>>> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
>>>>>>      * @parent_ops:        Callbacks exposed by the parent to the phys_enc
>>>>>>      * @split_role:        Role to play in a split-panel configuration
>>>>>>      * @intf_idx:        Interface index this phys_enc will control
>>>>>> + * @wb_idx:            Writeback index this phys_enc will control
>>>>>>      * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
>>>>>>      */
>>>>>>     struct dpu_enc_phys_init_params {
>>>>>> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
>>>>>>         const struct dpu_encoder_virt_ops *parent_ops;
>>>>>>         enum dpu_enc_split_role split_role;
>>>>>>         enum dpu_intf intf_idx;
>>>>>> +    enum dpu_wb wb_idx;
>>>>>>         spinlock_t *enc_spinlock;
>>>>>>     };
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>>> index ba82e54..2f34a31 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
>>>>>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
>>>>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
>>>>>>      * @rm: DPU Resource Manager handle
>>>>>>      * @wb_idx: WB index
>>>>>>      */
>>>>>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
>>>>>> dpu_intf wb_idx)
>>>>>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
>>>>>> dpu_wb wb_idx)
>>>>>>     {
>>>>>>         return rm->hw_wb[wb_idx - WB_0];
>>>>>>     }
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
> 

Re: [RFC 1/4] drm/msm/dpu: introduce a wb_idx to be used for writeback interfaces

[Dmitry Baryshkov]

On Fri, 22 Apr 2022 at 21:18, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
> Hi Dmitry
>
> On 4/22/2022 3:37 AM, Dmitry Baryshkov wrote:
> > On Fri, 22 Apr 2022 at 04:59, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 4/21/2022 5:22 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 22 Apr 2022 at 02:07, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>> Hi Dmitry
> >>>>
> >>>> Thanks for the review.
> >>>>
> >>>> One question below.
> >>>>
> >>>> On 4/21/2022 3:40 PM, Dmitry Baryshkov wrote:
> >>>>> On 21/04/2022 23:48, Abhinav Kumar wrote:
> >>>>>> Using intf_idx even for writeback interfaces is confusing
> >>>>>> because intf_idx is of type enum dpu_intf but the index used
> >>>>>> for writeback is of type enum dpu_wb.
> >>>>>>
> >>>>>> In addition, this makes it easier to separately check the
> >>>>>> availability of the two as its possible that there are boards
> >>>>>> which don't have any physical display connected but can still
> >>>>>> work in writeback mode.
> >>>>>>
> >>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> >>>>>
> >>>>> Looks good, two minor issues bellow.
> >>>>>
> >>>>> With them fixed, I'd even squash this patch into the corresponding patch
> >>>>> of the previous patchset.
> >>>>>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 62
> >>>>>> +++++++++++++-----------
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  4 ++
> >>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h           |  2 +-
> >>>>>>     3 files changed, 40 insertions(+), 28 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> index 9c12841..054d7e4 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >>>>>> @@ -962,7 +962,6 @@ static void
> >>>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>>>>         struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
> >>>>>>         int num_lm, num_ctl, num_pp, num_dsc;
> >>>>>>         unsigned int dsc_mask = 0;
> >>>>>> -    enum dpu_hw_blk_type blk_type;
> >>>>>>         int i;
> >>>>>>         if (!drm_enc) {
> >>>>>> @@ -1044,17 +1043,11 @@ static void
> >>>>>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> >>>>>>             phys->hw_pp = dpu_enc->hw_pp[i];
> >>>>>>             phys->hw_ctl = to_dpu_hw_ctl(hw_ctl[i]);
> >>>>>> -        if (dpu_encoder_get_intf_mode(&dpu_enc->base) ==
> >>>>>> INTF_MODE_WB_LINE)
> >>>>>> -            blk_type = DPU_HW_BLK_WB;
> >>>>>> -        else
> >>>>>> -            blk_type = DPU_HW_BLK_INTF;
> >>>>>> +        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX)
> >>>>>> +            phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >>>>>> phys->intf_idx);
> >>>>>> -        if (phys->intf_idx >= INTF_0 && phys->intf_idx < INTF_MAX) {
> >>>>>> -            if (blk_type == DPU_HW_BLK_INTF)
> >>>>>> -                phys->hw_intf = dpu_rm_get_intf(&dpu_kms->rm,
> >>>>>> phys->intf_idx);
> >>>>>> -            else if (blk_type == DPU_HW_BLK_WB)
> >>>>>> -                phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm,
> >>>>>> phys->intf_idx);
> >>>>>> -        }
> >>>>>> +        if (phys->wb_idx >= WB_0 && phys->wb_idx < WB_MAX)
> >>>>>> +            phys->hw_wb = dpu_rm_get_wb(&dpu_kms->rm, phys->wb_idx);
> >>>>>
> >>>>> We also need a check for if (phus->hw_intf && phys->hw_wb) HERE
> >>>>
> >>>> So there is an error if
> >>>>
> >>>> 1) Neither wb NOR intf are present
> >>>> 2) Both wb AND intf are present for the physical encoder?
> >>>>
> >>>> The second check is okay for now to add but considering concurrent
> >>>> writeback then that wouldn't assumption be wrong since both physical and
> >>>> wb interfaces can go with the same encoder?
> >>>
> >>> To the same encoder, but not to the same physical encoder. Here we
> >>> check the phys_enc parameters.
> >>
> >> Ok got it, let me re-spin this RFC with patches 2 & 3 squashed.
> >> Get the acks on them.
> >>
> >> Then will absorb into WB series and re-post it.
> >
> > Sounds like a good plan!
> >
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>             if (!phys->hw_intf && !phys->hw_wb) {
> >>>>>>                 DPU_ERROR_ENC(dpu_enc,
> >>>>>> @@ -1201,7 +1194,7 @@ static void dpu_encoder_virt_disable(struct
> >>>>>> drm_encoder *drm_enc)
> >>>>>>         mutex_unlock(&dpu_enc->enc_lock);
> >>>>>>     }
> >>>>>> -static enum dpu_intf dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg
> >>>>>> *catalog,
> >>>>>> +static enum dpu_intf dpu_encoder_get_intf(struct dpu_mdss_cfg *catalog,
> >>>>>>             enum dpu_intf_type type, u32 controller_id)
> >>>>>>     {
> >>>>>>         int i = 0;
> >>>>>> @@ -1213,16 +1206,28 @@ static enum dpu_intf
> >>>>>> dpu_encoder_get_intf_or_wb(struct dpu_mdss_cfg *catalog,
> >>>>>>                     return catalog->intf[i].id;
> >>>>>>                 }
> >>>>>>             }
> >>>>>> -    } else {
> >>>>>> -        for (i = 0; i < catalog->wb_count; i++) {
> >>>>>> -            if (catalog->wb[i].id == controller_id)
> >>>>>> -                return catalog->wb[i].id;
> >>>>>> -        }
> >>>>>>         }
> >>>>>>         return INTF_MAX;
> >>>>>>     }
> >>>>>> +static enum dpu_wb dpu_encoder_get_wb(struct dpu_mdss_cfg *catalog,
> >>>>>> +        enum dpu_intf_type type, u32 controller_id)
> >>>>>> +{
> >>>>>> +    int i = 0;
> >>>>>> +
> >>>>>> +    if (type != INTF_WB)
> >>>>>> +        goto end;
> >>>>>> +
> >>>>>> +    for (i = 0; i < catalog->wb_count; i++) {
> >>>>>> +        if (catalog->wb[i].id == controller_id)
> >>>>>> +            return catalog->wb[i].id;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +end:
> >>>>>> +    return WB_MAX;
> >>>>>
> >>>>> I'd return INTF_NONE/WB_NONE if the interface or WB unit was not found.
> >>>> ack, i guess in that case even the places checking the return value of
> >>>> this function need to be changed.
> >>>
> >>> Yes, of course.
>
> INTF_NONE/WB_NONE is not of enum dpu_intf or enum dpu_wb, its of type
> enum dpu_intf_mode
>
> Do we want to add them to dpu_intf/dpu_wb with a -1 value OR leave it
> as-it-is.

Let's leave it as is then.

>
> >>>
> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>>     static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> >>>>>>             struct dpu_encoder_phys *phy_enc)
> >>>>>>     {
> >>>>>> @@ -2249,18 +2254,21 @@ static int dpu_encoder_setup_display(struct
> >>>>>> dpu_encoder_virt *dpu_enc,
> >>>>>>             DPU_DEBUG("h_tile_instance %d = %d, split_role %d\n",
> >>>>>>                     i, controller_id, phys_params.split_role);
> >>>>>> -        /*
> >>>>>> -         * FIXME: have separate intf_idx and wb_idx to avoid using
> >>>>>> -         * enum dpu_intf type for wb_idx and also to be able to
> >>>>>> -         * not bail out when there is no intf for boards which dont
> >>>>>> -         * have a display connected to them.
> >>>>>> -         * Having a valid wb_idx but not a intf_idx can be a valid
> >>>>>> -         * combination moving forward.
> >>>>>> -         */
> >>>>>> -        phys_params.intf_idx =
> >>>>>> dpu_encoder_get_intf_or_wb(dpu_kms->catalog,
> >>>>>> +        phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> >>>>>>                                                         intf_type,
> >>>>>>                                                         controller_id);
> >>>>>> -        if (phys_params.intf_idx == INTF_MAX) {
> >>>>>> +
> >>>>>> +        phys_params.wb_idx = dpu_encoder_get_wb(dpu_kms->catalog,
> >>>>>> +                intf_type, controller_id);
> >>>>>> +        /*
> >>>>>> +         * For boards which have no physical displays, having no
> >>>>>> interface
> >>>>>> +         * is fine because it can still be used with just writeback.
> >>>>>> +         * If we try without a display on a board which uses a DPU in
> >>>>>> which
> >>>>>> +         * writeback is not supported, then this will still fail as
> >>>>>> it will not
> >>>>>> +         * find any writeback in the catalog.
> >>>>>> +         */
> >>>>>> +        if ((phys_params.intf_idx == INTF_MAX) &&
> >>>>>> +                (phys_params.wb_idx == WB_MAX)) {
> >>>>>>                 DPU_ERROR_ENC(dpu_enc, "could not get intf or wb: type
> >>>>>> %d, id %d\n",
> >>>>>>                               intf_type, controller_id);
> >>>>>>                 ret = -EINVAL;
> >>>>>> 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 04d037e..f2af07d 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> >>>>>> @@ -181,6 +181,7 @@ enum dpu_intr_idx {
> >>>>>>      * @split_role:        Role to play in a split-panel configuration
> >>>>>>      * @intf_mode:        Interface mode
> >>>>>>      * @intf_idx:        Interface index on dpu hardware
> >>>>>> + * @wb_idx:            Writeback index on dpu hardware
> >>>>>>      * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>>>>>      * @enable_state:    Enable state tracking
> >>>>>>      * @vblank_refcount:    Reference count of vblank request
> >>>>>> @@ -209,6 +210,7 @@ struct dpu_encoder_phys {
> >>>>>>         enum dpu_enc_split_role split_role;
> >>>>>>         enum dpu_intf_mode intf_mode;
> >>>>>>         enum dpu_intf intf_idx;
> >>>>>> +    enum dpu_wb wb_idx;
> >>>>>>         spinlock_t *enc_spinlock;
> >>>>>>         enum dpu_enc_enable_state enable_state;
> >>>>>>         atomic_t vblank_refcount;
> >>>>>> @@ -275,6 +277,7 @@ struct dpu_encoder_phys_cmd {
> >>>>>>      * @parent_ops:        Callbacks exposed by the parent to the phys_enc
> >>>>>>      * @split_role:        Role to play in a split-panel configuration
> >>>>>>      * @intf_idx:        Interface index this phys_enc will control
> >>>>>> + * @wb_idx:            Writeback index this phys_enc will control
> >>>>>>      * @enc_spinlock:    Virtual-Encoder-Wide Spin Lock for IRQ purposes
> >>>>>>      */
> >>>>>>     struct dpu_enc_phys_init_params {
> >>>>>> @@ -283,6 +286,7 @@ struct dpu_enc_phys_init_params {
> >>>>>>         const struct dpu_encoder_virt_ops *parent_ops;
> >>>>>>         enum dpu_enc_split_role split_role;
> >>>>>>         enum dpu_intf intf_idx;
> >>>>>> +    enum dpu_wb wb_idx;
> >>>>>>         spinlock_t *enc_spinlock;
> >>>>>>     };
> >>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> index ba82e54..2f34a31 100644
> >>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h
> >>>>>> @@ -103,7 +103,7 @@ static inline struct dpu_hw_intf
> >>>>>> *dpu_rm_get_intf(struct dpu_rm *rm, enum dpu_in
> >>>>>>      * @rm: DPU Resource Manager handle
> >>>>>>      * @wb_idx: WB index
> >>>>>>      */
> >>>>>> -static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>>>> dpu_intf wb_idx)
> >>>>>> +static inline struct dpu_hw_wb *dpu_rm_get_wb(struct dpu_rm *rm, enum
> >>>>>> dpu_wb wb_idx)
> >>>>>>     {
> >>>>>>         return rm->hw_wb[wb_idx - WB_0];
> >>>>>>     }
> >>>>>
> >>>>>
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry