[PATCH 0/3] Reserve dspps based on user request

[PATCH 0/3] Reserve dspps based on user request

[Kalyan Thota]

This series will enable color features on sc7280 target which has primary panel as eDP

The series removes dspp allocation based on encoder type and allows the dspp reservation
based on user request via ctm.

The series will release/reserve the dpu resources when ever there is a topology change
to suit the new requirements.

Kalyan Thota (3):
  drm/msm/disp/dpu1: clear dspp reservations in rm release
  drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
    request
  drm/msm/disp/dpu1: reserve the resources on topology change

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
 5 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 0/3] Reserve dspps based on user request

[Kalyan Thota]

This series will enable color features on sc7280 target which has primary panel as eDP

The series removes dspp allocation based on encoder type and allows the dspp reservation
based on user request via ctm.

The series will release/reserve the dpu resources when ever there is a topology change
to suit the new requirements.

Kalyan Thota (3):
  drm/msm/disp/dpu1: clear dspp reservations in rm release
  drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
    request
  drm/msm/disp/dpu1: reserve the resources on topology change

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
 5 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 1/3] drm/msm/disp/dpu1: clear dspp reservations in rm release

[Kalyan Thota]

Clear dspp reservations from the global state during
rm release

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442..718ea0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -572,6 +572,8 @@ void dpu_rm_release(struct dpu_global_state *global_state,
 		ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
 	_dpu_rm_clear_mapping(global_state->dsc_to_enc_id,
 		ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
+	_dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
+		ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
 }
 
 int dpu_rm_reserve(
-- 
2.7.4

[PATCH 1/3] drm/msm/disp/dpu1: clear dspp reservations in rm release

[Kalyan Thota]

Clear dspp reservations from the global state during
rm release

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442..718ea0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -572,6 +572,8 @@ void dpu_rm_release(struct dpu_global_state *global_state,
 		ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
 	_dpu_rm_clear_mapping(global_state->dsc_to_enc_id,
 		ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
+	_dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
+		ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
 }
 
 int dpu_rm_reserve(
-- 
2.7.4

[PATCH 2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

[Kalyan Thota]

Add dspp blocks into the topology for reservation, if there is a ctm
request for that composition.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..8d76cb3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
 			struct dpu_kms *dpu_kms,
-			struct drm_display_mode *mode)
+			struct drm_display_mode *mode,
+			struct drm_crtc_state *crtc_state)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -573,11 +574,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	else
 		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
-		if (dpu_kms->catalog->dspp &&
-			(dpu_kms->catalog->dspp_count >= topology.num_lm))
-			topology.num_dspp = topology.num_lm;
-	}
+	if (dpu_kms->catalog->dspp && crtc_state->ctm &&
+		(dpu_kms->catalog->dspp_count >= topology.num_lm))
+		topology.num_dspp = topology.num_lm;
 
 	topology.num_enc = 0;
 	topology.num_intf = intf_count;
@@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
 	/* Reserve dynamic resources now. */
 	if (!ret) {
-- 
2.7.4

[PATCH 2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

[Kalyan Thota]

Add dspp blocks into the topology for reservation, if there is a ctm
request for that composition.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..8d76cb3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
 			struct dpu_kms *dpu_kms,
-			struct drm_display_mode *mode)
+			struct drm_display_mode *mode,
+			struct drm_crtc_state *crtc_state)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -573,11 +574,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	else
 		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
-		if (dpu_kms->catalog->dspp &&
-			(dpu_kms->catalog->dspp_count >= topology.num_lm))
-			topology.num_dspp = topology.num_lm;
-	}
+	if (dpu_kms->catalog->dspp && crtc_state->ctm &&
+		(dpu_kms->catalog->dspp_count >= topology.num_lm))
+		topology.num_dspp = topology.num_lm;
 
 	topology.num_enc = 0;
 	topology.num_intf = intf_count;
@@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
 	/* Reserve dynamic resources now. */
 	if (!ret) {
-- 
2.7.4

[PATCH 3/3] drm/msm/disp/dpu1: reserve the resources on topology change

[Kalyan Thota]

Some features like ctm can be enabled dynamically. Release and reserve
the dpu resources whenever a topology change occurs such that
required hw blocks are allocated appropriately.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  4 ---
 5 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..89afe04 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -225,6 +225,7 @@ struct dpu_crtc_state {
 
 	enum dpu_crtc_crc_source crc_source;
 	int crc_frame_skip_count;
+	struct msm_display_topology topology;
 };
 
 #define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8d76cb3..db417f5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,18 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
 	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
 };
 
+static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
+					     struct drm_crtc_state *crtc_state,
+					     struct drm_connector_state *conn_state);
+
+static bool _dpu_enc_is_topology_changed(struct drm_crtc_state *crtc_state,
+	struct msm_display_topology topology)
+{
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
+
+	return !!memcmp(&cstate->topology,
+		&topology, sizeof(struct msm_display_topology));
+}
 
 bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
 {
@@ -650,12 +662,16 @@ static int dpu_encoder_virt_atomic_check(
 		 * Release and Allocate resources on every modeset
 		 * Dont allocate when active is false.
 		 */
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		if (drm_atomic_crtc_needs_modeset(crtc_state) ||
+			_dpu_enc_is_topology_changed(crtc_state, topology)) {
 			dpu_rm_release(global_state, drm_enc);
 
-			if (!crtc_state->active_changed || crtc_state->active)
+			if (crtc_state->enable) {
 				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
 						drm_enc, crtc_state, topology);
+				if (!ret)
+					dpu_enc->topology = topology;
+			}
 		}
 	}
 
@@ -1089,7 +1105,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	}
 
 	cstate->num_mixers = num_lm;
-
+	cstate->topology = dpu_enc->topology;
 	dpu_enc->connector = conn_state->connector;
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
@@ -2076,11 +2092,14 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 	ctl->ops.clear_pending_flush(ctl);
 }
 
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
+void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
+	struct drm_crtc_state *crtc_state)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
 	int i;
+	struct list_head *connector_list;
+	struct drm_connector *conn = NULL, *conn_iter;
 
 	if (!drm_enc) {
 		DPU_ERROR("invalid encoder\n");
@@ -2088,6 +2107,20 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
 	}
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
+	connector_list = &drm_enc->dev->mode_config.connector_list;
+	list_for_each_entry(conn_iter, connector_list, head)
+		if (conn_iter->state->best_encoder == drm_enc)
+			conn = conn_iter;
+
+	/*
+	 * In case of modeset, DRM kernel will trigger a atomic_mode_set
+	 * call back, for usecases where there is no mode change but a topology
+	 * change, update the resources from here.
+	 */
+	if (!drm_atomic_crtc_needs_modeset(crtc_state) &&
+		_dpu_enc_is_topology_changed(crtc_state, dpu_enc->topology))
+		dpu_encoder_virt_atomic_mode_set(drm_enc, crtc_state, conn->state);
+
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		phys = dpu_enc->phys_encs[i];
 		if (phys->ops.prepare_commit)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..4cbe20c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -150,8 +150,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
  * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
  *	atomic commit, before any registers are written
  * @drm_enc:    Pointer to previously created drm encoder structure
+ * @crtc_state: Pointer to drm crtc state
  */
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
+void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
+		struct drm_crtc_state *crtc_state);
 
 /**
  * dpu_encoder_set_idle_timeout - set the idle timeout for video
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..f111120 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -455,7 +455,7 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		drm_for_each_encoder_mask(encoder, crtc->dev,
 					  crtc_state->encoder_mask) {
-			dpu_encoder_prepare_commit(encoder);
+			dpu_encoder_prepare_commit(encoder, crtc_state);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 718ea0a..341c3af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -586,10 +586,6 @@ int dpu_rm_reserve(
 	struct dpu_rm_requirements reqs;
 	int ret;
 
-	/* Check if this is just a page-flip */
-	if (!drm_atomic_crtc_needs_modeset(crtc_state))
-		return 0;
-
 	if (IS_ERR(global_state)) {
 		DPU_ERROR("failed to global state\n");
 		return PTR_ERR(global_state);
-- 
2.7.4

[PATCH 3/3] drm/msm/disp/dpu1: reserve the resources on topology change

[Kalyan Thota]

Some features like ctm can be enabled dynamically. Release and reserve
the dpu resources whenever a topology change occurs such that
required hw blocks are allocated appropriately.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  4 ---
 5 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 539b68b..89afe04 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -225,6 +225,7 @@ struct dpu_crtc_state {
 
 	enum dpu_crtc_crc_source crc_source;
 	int crc_frame_skip_count;
+	struct msm_display_topology topology;
 };
 
 #define to_dpu_crtc_state(x) \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 8d76cb3..db417f5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,18 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
 	15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
 };
 
+static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
+					     struct drm_crtc_state *crtc_state,
+					     struct drm_connector_state *conn_state);
+
+static bool _dpu_enc_is_topology_changed(struct drm_crtc_state *crtc_state,
+	struct msm_display_topology topology)
+{
+	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
+
+	return !!memcmp(&cstate->topology,
+		&topology, sizeof(struct msm_display_topology));
+}
 
 bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
 {
@@ -650,12 +662,16 @@ static int dpu_encoder_virt_atomic_check(
 		 * Release and Allocate resources on every modeset
 		 * Dont allocate when active is false.
 		 */
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		if (drm_atomic_crtc_needs_modeset(crtc_state) ||
+			_dpu_enc_is_topology_changed(crtc_state, topology)) {
 			dpu_rm_release(global_state, drm_enc);
 
-			if (!crtc_state->active_changed || crtc_state->active)
+			if (crtc_state->enable) {
 				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
 						drm_enc, crtc_state, topology);
+				if (!ret)
+					dpu_enc->topology = topology;
+			}
 		}
 	}
 
@@ -1089,7 +1105,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	}
 
 	cstate->num_mixers = num_lm;
-
+	cstate->topology = dpu_enc->topology;
 	dpu_enc->connector = conn_state->connector;
 
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
@@ -2076,11 +2092,14 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 	ctl->ops.clear_pending_flush(ctl);
 }
 
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
+void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
+	struct drm_crtc_state *crtc_state)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
 	int i;
+	struct list_head *connector_list;
+	struct drm_connector *conn = NULL, *conn_iter;
 
 	if (!drm_enc) {
 		DPU_ERROR("invalid encoder\n");
@@ -2088,6 +2107,20 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
 	}
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
+	connector_list = &drm_enc->dev->mode_config.connector_list;
+	list_for_each_entry(conn_iter, connector_list, head)
+		if (conn_iter->state->best_encoder == drm_enc)
+			conn = conn_iter;
+
+	/*
+	 * In case of modeset, DRM kernel will trigger a atomic_mode_set
+	 * call back, for usecases where there is no mode change but a topology
+	 * change, update the resources from here.
+	 */
+	if (!drm_atomic_crtc_needs_modeset(crtc_state) &&
+		_dpu_enc_is_topology_changed(crtc_state, dpu_enc->topology))
+		dpu_encoder_virt_atomic_mode_set(drm_enc, crtc_state, conn->state);
+
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		phys = dpu_enc->phys_encs[i];
 		if (phys->ops.prepare_commit)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..4cbe20c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -150,8 +150,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
  * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
  *	atomic commit, before any registers are written
  * @drm_enc:    Pointer to previously created drm encoder structure
+ * @crtc_state: Pointer to drm crtc state
  */
-void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
+void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
+		struct drm_crtc_state *crtc_state);
 
 /**
  * dpu_encoder_set_idle_timeout - set the idle timeout for video
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..f111120 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -455,7 +455,7 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
 		drm_for_each_encoder_mask(encoder, crtc->dev,
 					  crtc_state->encoder_mask) {
-			dpu_encoder_prepare_commit(encoder);
+			dpu_encoder_prepare_commit(encoder, crtc_state);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 718ea0a..341c3af 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -586,10 +586,6 @@ int dpu_rm_reserve(
 	struct dpu_rm_requirements reqs;
 	int ret;
 
-	/* Check if this is just a page-flip */
-	if (!drm_atomic_crtc_needs_modeset(crtc_state))
-		return 0;
-
 	if (IS_ERR(global_state)) {
 		DPU_ERROR("failed to global state\n");
 		return PTR_ERR(global_state);
-- 
2.7.4

Re: [PATCH 1/3] drm/msm/disp/dpu1: clear dspp reservations in rm release

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Clear dspp reservations from the global state during
> rm release
>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>

Fixes: e47616df008b ("drm/msm/dpu: add support for color processing
blocks in dpu driver")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 73b3442..718ea0a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -572,6 +572,8 @@ void dpu_rm_release(struct dpu_global_state *global_state,
>                 ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>         _dpu_rm_clear_mapping(global_state->dsc_to_enc_id,
>                 ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
> +       _dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
> +               ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
>  }
>
>  int dpu_rm_reserve(
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/disp/dpu1: clear dspp reservations in rm release

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Clear dspp reservations from the global state during
> rm release
>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>

Fixes: e47616df008b ("drm/msm/dpu: add support for color processing
blocks in dpu driver")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 73b3442..718ea0a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -572,6 +572,8 @@ void dpu_rm_release(struct dpu_global_state *global_state,
>                 ARRAY_SIZE(global_state->ctl_to_enc_id), enc->base.id);
>         _dpu_rm_clear_mapping(global_state->dsc_to_enc_id,
>                 ARRAY_SIZE(global_state->dsc_to_enc_id), enc->base.id);
> +       _dpu_rm_clear_mapping(global_state->dspp_to_enc_id,
> +               ARRAY_SIZE(global_state->dspp_to_enc_id), enc->base.id);
>  }
>
>  int dpu_rm_reserve(
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Add dspp blocks into the topology for reservation, if there is a ctm
> request for that composition.

... rather than just allocating them for DSI encoders.

With this fixed (and one nit below):

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

>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b..8d76cb3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>  static struct msm_display_topology dpu_encoder_get_topology(
>                         struct dpu_encoder_virt *dpu_enc,
>                         struct dpu_kms *dpu_kms,
> -                       struct drm_display_mode *mode)
> +                       struct drm_display_mode *mode,
> +                       struct drm_crtc_state *crtc_state)
>  {
>         struct msm_display_topology topology = {0};
>         int i, intf_count = 0;
> @@ -573,11 +574,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
>         else
>                 topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>
> -       if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> -               if (dpu_kms->catalog->dspp &&
> -                       (dpu_kms->catalog->dspp_count >= topology.num_lm))
> -                       topology.num_dspp = topology.num_lm;
> -       }
> +       if (dpu_kms->catalog->dspp && crtc_state->ctm &&

Could you please move the second condition to a separate line? Also
possibly it would be good to indent the conditions to the opening
parenthesis.

> +               (dpu_kms->catalog->dspp_count >= topology.num_lm))
> +               topology.num_dspp = topology.num_lm;
>
>         topology.num_enc = 0;
>         topology.num_intf = intf_count;
> @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
>                 }
>         }
>
> -       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>
>         /* Reserve dynamic resources now. */
>         if (!ret) {
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Add dspp blocks into the topology for reservation, if there is a ctm
> request for that composition.

... rather than just allocating them for DSI encoders.

With this fixed (and one nit below):

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

>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b..8d76cb3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>  static struct msm_display_topology dpu_encoder_get_topology(
>                         struct dpu_encoder_virt *dpu_enc,
>                         struct dpu_kms *dpu_kms,
> -                       struct drm_display_mode *mode)
> +                       struct drm_display_mode *mode,
> +                       struct drm_crtc_state *crtc_state)
>  {
>         struct msm_display_topology topology = {0};
>         int i, intf_count = 0;
> @@ -573,11 +574,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
>         else
>                 topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>
> -       if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> -               if (dpu_kms->catalog->dspp &&
> -                       (dpu_kms->catalog->dspp_count >= topology.num_lm))
> -                       topology.num_dspp = topology.num_lm;
> -       }
> +       if (dpu_kms->catalog->dspp && crtc_state->ctm &&

Could you please move the second condition to a separate line? Also
possibly it would be good to indent the conditions to the opening
parenthesis.

> +               (dpu_kms->catalog->dspp_count >= topology.num_lm))
> +               topology.num_dspp = topology.num_lm;
>
>         topology.num_enc = 0;
>         topology.num_intf = intf_count;
> @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
>                 }
>         }
>
> -       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>
>         /* Reserve dynamic resources now. */
>         if (!ret) {
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/disp/dpu1: reserve the resources on topology change

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Some features like ctm can be enabled dynamically. Release and reserve
> the dpu resources whenever a topology change occurs such that
> required hw blocks are allocated appropriately.
>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  4 ---
>  5 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 539b68b..89afe04 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -225,6 +225,7 @@ struct dpu_crtc_state {
>
>         enum dpu_crtc_crc_source crc_source;
>         int crc_frame_skip_count;
> +       struct msm_display_topology topology;
>  };
>
>  #define to_dpu_crtc_state(x) \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 8d76cb3..db417f5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -217,6 +217,18 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
>         15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
>  };
>
> +static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> +                                            struct drm_crtc_state *crtc_state,
> +                                            struct drm_connector_state *conn_state);
> +
> +static bool _dpu_enc_is_topology_changed(struct drm_crtc_state *crtc_state,
> +       struct msm_display_topology topology)
> +{
> +       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
> +
> +       return !!memcmp(&cstate->topology,
> +               &topology, sizeof(struct msm_display_topology));
> +}
>
>  bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
>  {
> @@ -650,12 +662,16 @@ static int dpu_encoder_virt_atomic_check(
>                  * Release and Allocate resources on every modeset
>                  * Dont allocate when active is false.
>                  */
> -               if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +               if (drm_atomic_crtc_needs_modeset(crtc_state) ||
> +                       _dpu_enc_is_topology_changed(crtc_state, topology)) {
>                         dpu_rm_release(global_state, drm_enc);
>
> -                       if (!crtc_state->active_changed || crtc_state->active)
> +                       if (crtc_state->enable) {
>                                 ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>                                                 drm_enc, crtc_state, topology);
> +                               if (!ret)
> +                                       dpu_enc->topology = topology;
> +                       }
>                 }
>         }
>
> @@ -1089,7 +1105,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>         }
>
>         cstate->num_mixers = num_lm;
> -
> +       cstate->topology = dpu_enc->topology;

There is one part of the story that I do not quite like here. The crtc
state now gets the encoder-level information (regarding the num_intf,
num_enc, etc).
Just to clarify, I find it particularly bad that we have to actively
poke into the crtc state from the dpu_encoder.c. I hope to get to it
at some point, most probably while reworking the encoder code to
support  using a single CTL for all the operations.

>         dpu_enc->connector = conn_state->connector;
>
>         for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> @@ -2076,11 +2092,14 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
>         ctl->ops.clear_pending_flush(ctl);
>  }
>
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> +void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
> +       struct drm_crtc_state *crtc_state)
>  {
>         struct dpu_encoder_virt *dpu_enc;
>         struct dpu_encoder_phys *phys;
>         int i;
> +       struct list_head *connector_list;
> +       struct drm_connector *conn = NULL, *conn_iter;
>
>         if (!drm_enc) {
>                 DPU_ERROR("invalid encoder\n");
> @@ -2088,6 +2107,20 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
>         }
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> +       connector_list = &drm_enc->dev->mode_config.connector_list;
> +       list_for_each_entry(conn_iter, connector_list, head)
> +               if (conn_iter->state->best_encoder == drm_enc)
> +                       conn = conn_iter;

I don't think we can poke at conn_iter->state here.

> +
> +       /*
> +        * In case of modeset, DRM kernel will trigger a atomic_mode_set
> +        * call back, for usecases where there is no mode change but a topology
> +        * change, update the resources from here.
> +        */
> +       if (!drm_atomic_crtc_needs_modeset(crtc_state) &&
> +               _dpu_enc_is_topology_changed(crtc_state, dpu_enc->topology))
> +               dpu_encoder_virt_atomic_mode_set(drm_enc, crtc_state, conn->state);

And this is a complete pain. I think it would be easier to set the
crtc_state->mode_changed if we detect the change in the CTM
enablement.
Please start from that and then fill in the necessary bits (in other
words if we need to store the topology or just a boolean flag
`ctm_enabled' / `need_dspp').

> +
>         for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>                 phys = dpu_enc->phys_encs[i];
>                 if (phys->ops.prepare_commit)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236e..4cbe20c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -150,8 +150,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
>   *     atomic commit, before any registers are written
>   * @drm_enc:    Pointer to previously created drm encoder structure
> + * @crtc_state: Pointer to drm crtc state
>   */
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> +void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
> +               struct drm_crtc_state *crtc_state);
>
>  /**
>   * dpu_encoder_set_idle_timeout - set the idle timeout for video
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7a5fabc..f111120 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -455,7 +455,7 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
>         for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>                 drm_for_each_encoder_mask(encoder, crtc->dev,
>                                           crtc_state->encoder_mask) {
> -                       dpu_encoder_prepare_commit(encoder);
> +                       dpu_encoder_prepare_commit(encoder, crtc_state);
>                 }
>         }
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 718ea0a..341c3af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -586,10 +586,6 @@ int dpu_rm_reserve(
>         struct dpu_rm_requirements reqs;
>         int ret;
>
> -       /* Check if this is just a page-flip */
> -       if (!drm_atomic_crtc_needs_modeset(crtc_state))
> -               return 0;
> -
>         if (IS_ERR(global_state)) {
>                 DPU_ERROR("failed to global state\n");
>                 return PTR_ERR(global_state);
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/disp/dpu1: reserve the resources on topology change

[Dmitry Baryshkov]

On Fri, 27 Jan 2023 at 12:15, Kalyan Thota <quic_kalyant@quicinc.com> wrote:
>
> Some features like ctm can be enabled dynamically. Release and reserve
> the dpu resources whenever a topology change occurs such that
> required hw blocks are allocated appropriately.
>
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++++++++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  4 ---
>  5 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 539b68b..89afe04 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -225,6 +225,7 @@ struct dpu_crtc_state {
>
>         enum dpu_crtc_crc_source crc_source;
>         int crc_frame_skip_count;
> +       struct msm_display_topology topology;
>  };
>
>  #define to_dpu_crtc_state(x) \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 8d76cb3..db417f5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -217,6 +217,18 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
>         15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
>  };
>
> +static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> +                                            struct drm_crtc_state *crtc_state,
> +                                            struct drm_connector_state *conn_state);
> +
> +static bool _dpu_enc_is_topology_changed(struct drm_crtc_state *crtc_state,
> +       struct msm_display_topology topology)
> +{
> +       struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
> +
> +       return !!memcmp(&cstate->topology,
> +               &topology, sizeof(struct msm_display_topology));
> +}
>
>  bool dpu_encoder_is_widebus_enabled(const struct drm_encoder *drm_enc)
>  {
> @@ -650,12 +662,16 @@ static int dpu_encoder_virt_atomic_check(
>                  * Release and Allocate resources on every modeset
>                  * Dont allocate when active is false.
>                  */
> -               if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +               if (drm_atomic_crtc_needs_modeset(crtc_state) ||
> +                       _dpu_enc_is_topology_changed(crtc_state, topology)) {
>                         dpu_rm_release(global_state, drm_enc);
>
> -                       if (!crtc_state->active_changed || crtc_state->active)
> +                       if (crtc_state->enable) {
>                                 ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>                                                 drm_enc, crtc_state, topology);
> +                               if (!ret)
> +                                       dpu_enc->topology = topology;
> +                       }
>                 }
>         }
>
> @@ -1089,7 +1105,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>         }
>
>         cstate->num_mixers = num_lm;
> -
> +       cstate->topology = dpu_enc->topology;

There is one part of the story that I do not quite like here. The crtc
state now gets the encoder-level information (regarding the num_intf,
num_enc, etc).
Just to clarify, I find it particularly bad that we have to actively
poke into the crtc state from the dpu_encoder.c. I hope to get to it
at some point, most probably while reworking the encoder code to
support  using a single CTL for all the operations.

>         dpu_enc->connector = conn_state->connector;
>
>         for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> @@ -2076,11 +2092,14 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
>         ctl->ops.clear_pending_flush(ctl);
>  }
>
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
> +void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
> +       struct drm_crtc_state *crtc_state)
>  {
>         struct dpu_encoder_virt *dpu_enc;
>         struct dpu_encoder_phys *phys;
>         int i;
> +       struct list_head *connector_list;
> +       struct drm_connector *conn = NULL, *conn_iter;
>
>         if (!drm_enc) {
>                 DPU_ERROR("invalid encoder\n");
> @@ -2088,6 +2107,20 @@ void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc)
>         }
>         dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> +       connector_list = &drm_enc->dev->mode_config.connector_list;
> +       list_for_each_entry(conn_iter, connector_list, head)
> +               if (conn_iter->state->best_encoder == drm_enc)
> +                       conn = conn_iter;

I don't think we can poke at conn_iter->state here.

> +
> +       /*
> +        * In case of modeset, DRM kernel will trigger a atomic_mode_set
> +        * call back, for usecases where there is no mode change but a topology
> +        * change, update the resources from here.
> +        */
> +       if (!drm_atomic_crtc_needs_modeset(crtc_state) &&
> +               _dpu_enc_is_topology_changed(crtc_state, dpu_enc->topology))
> +               dpu_encoder_virt_atomic_mode_set(drm_enc, crtc_state, conn->state);

And this is a complete pain. I think it would be easier to set the
crtc_state->mode_changed if we detect the change in the CTM
enablement.
Please start from that and then fill in the necessary bits (in other
words if we need to store the topology or just a boolean flag
`ctm_enabled' / `need_dspp').

> +
>         for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>                 phys = dpu_enc->phys_encs[i];
>                 if (phys->ops.prepare_commit)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236e..4cbe20c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -150,8 +150,10 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an
>   *     atomic commit, before any registers are written
>   * @drm_enc:    Pointer to previously created drm encoder structure
> + * @crtc_state: Pointer to drm crtc state
>   */
> -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc);
> +void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc,
> +               struct drm_crtc_state *crtc_state);
>
>  /**
>   * dpu_encoder_set_idle_timeout - set the idle timeout for video
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7a5fabc..f111120 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -455,7 +455,7 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
>         for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>                 drm_for_each_encoder_mask(encoder, crtc->dev,
>                                           crtc_state->encoder_mask) {
> -                       dpu_encoder_prepare_commit(encoder);
> +                       dpu_encoder_prepare_commit(encoder, crtc_state);
>                 }
>         }
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 718ea0a..341c3af 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -586,10 +586,6 @@ int dpu_rm_reserve(
>         struct dpu_rm_requirements reqs;
>         int ret;
>
> -       /* Check if this is just a page-flip */
> -       if (!drm_atomic_crtc_needs_modeset(crtc_state))
> -               return 0;
> -
>         if (IS_ERR(global_state)) {
>                 DPU_ERROR("failed to global state\n");
>                 return PTR_ERR(global_state);
> --
> 2.7.4
>


-- 
With best wishes
Dmitry

Re: [PATCH 0/3] Reserve dspps based on user request

[Marijn Suijten]

On 2023-01-30 21:18:30, Dmitry Baryshkov wrote:
> On 30/01/2023 17:21, Kalyan Thota wrote:
> > This series will enable color features on sc7280 target which has primary panel as eDP
> > 
> > The series removes dspp allocation based on encoder type and allows the dspp reservation
> > based on user request via ctm.
> > 
> > The series will release/reserve the dpu resources when ever there is a topology change
> > to suit the new requirements.
> 
> Nit: the subject of the cover letter should include the version, if you 
> are including one into the individual patches Subject.

Indeed this makes it hard to tell the versions apart, and lore also
confusingly bundles both series in "loose matches on Subject: below".

Nit ^2: and individual patches should still have the PATCH moniker, i.e.
[PATCH v2 1/3].  git format-patch -v2 --cover-letter ... takes care of
/all this/ this for you.

And one more: as DSPP is an abbreviation, can we capitalize it?  So
DSPP / DSPPs in these titles?

> > 
> > Kalyan Thota (3):
> >    drm/msm/disp/dpu1: clear dspp reservations in rm release
> >    drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
> >      request
> >    drm/msm/disp/dpu1: reserve the resources on topology change

We just discussed in the DSC series that the subsystem prefix is
drm/msm/dpu.

- Marijn

> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
> >   5 files changed, 50 insertions(+), 17 deletions(-)
> > 
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 0/3] Reserve dspps based on user request

[Marijn Suijten]

On 2023-01-30 21:18:30, Dmitry Baryshkov wrote:
> On 30/01/2023 17:21, Kalyan Thota wrote:
> > This series will enable color features on sc7280 target which has primary panel as eDP
> > 
> > The series removes dspp allocation based on encoder type and allows the dspp reservation
> > based on user request via ctm.
> > 
> > The series will release/reserve the dpu resources when ever there is a topology change
> > to suit the new requirements.
> 
> Nit: the subject of the cover letter should include the version, if you 
> are including one into the individual patches Subject.

Indeed this makes it hard to tell the versions apart, and lore also
confusingly bundles both series in "loose matches on Subject: below".

Nit ^2: and individual patches should still have the PATCH moniker, i.e.
[PATCH v2 1/3].  git format-patch -v2 --cover-letter ... takes care of
/all this/ this for you.

And one more: as DSPP is an abbreviation, can we capitalize it?  So
DSPP / DSPPs in these titles?

> > 
> > Kalyan Thota (3):
> >    drm/msm/disp/dpu1: clear dspp reservations in rm release
> >    drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
> >      request
> >    drm/msm/disp/dpu1: reserve the resources on topology change

We just discussed in the DSC series that the subsystem prefix is
drm/msm/dpu.

- Marijn

> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
> >   5 files changed, 50 insertions(+), 17 deletions(-)
> > 
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 0/3] Reserve dspps based on user request

[Dmitry Baryshkov]

On 30/01/2023 17:21, Kalyan Thota wrote:
> This series will enable color features on sc7280 target which has primary panel as eDP
> 
> The series removes dspp allocation based on encoder type and allows the dspp reservation
> based on user request via ctm.
> 
> The series will release/reserve the dpu resources when ever there is a topology change
> to suit the new requirements.

Nit: the subject of the cover letter should include the version, if you 
are including one into the individual patches Subject.

> 
> Kalyan Thota (3):
>    drm/msm/disp/dpu1: clear dspp reservations in rm release
>    drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
>      request
>    drm/msm/disp/dpu1: reserve the resources on topology change
> 
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
>   5 files changed, 50 insertions(+), 17 deletions(-)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/3] Reserve dspps based on user request

[Dmitry Baryshkov]

On 30/01/2023 17:21, Kalyan Thota wrote:
> This series will enable color features on sc7280 target which has primary panel as eDP
> 
> The series removes dspp allocation based on encoder type and allows the dspp reservation
> based on user request via ctm.
> 
> The series will release/reserve the dpu resources when ever there is a topology change
> to suit the new requirements.

Nit: the subject of the cover letter should include the version, if you 
are including one into the individual patches Subject.

> 
> Kalyan Thota (3):
>    drm/msm/disp/dpu1: clear dspp reservations in rm release
>    drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
>      request
>    drm/msm/disp/dpu1: reserve the resources on topology change
> 
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
>   5 files changed, 50 insertions(+), 17 deletions(-)
> 

-- 
With best wishes
Dmitry

[PATCH 0/3] Reserve dspps based on user request

[Kalyan Thota]

This series will enable color features on sc7280 target which has primary panel as eDP

The series removes dspp allocation based on encoder type and allows the dspp reservation
based on user request via ctm.

The series will release/reserve the dpu resources when ever there is a topology change
to suit the new requirements.

Kalyan Thota (3):
  drm/msm/disp/dpu1: clear dspp reservations in rm release
  drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
    request
  drm/msm/disp/dpu1: reserve the resources on topology change

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
 5 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 0/3] Reserve dspps based on user request

[Kalyan Thota]

This series will enable color features on sc7280 target which has primary panel as eDP

The series removes dspp allocation based on encoder type and allows the dspp reservation
based on user request via ctm.

The series will release/reserve the dpu resources when ever there is a topology change
to suit the new requirements.

Kalyan Thota (3):
  drm/msm/disp/dpu1: clear dspp reservations in rm release
  drm/msm/disp/dpu1: add dspps into reservation if there is a ctm
    request
  drm/msm/disp/dpu1: reserve the resources on topology change

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 54 +++++++++++++++++++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  6 ++--
 5 files changed, 50 insertions(+), 17 deletions(-)

-- 
2.7.4