[PATCH 0/6] Use new DRM API where possible, and cleanups.

[PATCH 0/6] Use new DRM API where possible, and cleanups.

[sunpeng.li]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Hi Dave,

This series reworks the previous patch. Patch 1 is a v2 of the previous,
and additional patches are from the feedback received. They apply on top
of your drm-next-amd-dc-staging branch.

Thanks,
Leo

Leo (Sunpeng) Li (6):
  drm/amd/display: Use DRM new-style object iterators.
  drm/amd/display: Use new DRM API where possible
  drm/amd/display: Unify DRM state variable namings.
  drm/amd/display: Unify amdgpu_dm state variable namings.
  drm/amd/display: Fix typo
  drm/amd/display: Remove useless pcrtc pointer

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 320 +++++++++++-----------
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +-
 2 files changed, 156 insertions(+), 167 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[sunpeng.li]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Use the correct for_each_new/old_* iterators instead of for_each_*

List of affected functions:

amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
    - Old from_state_var flag was always choosing the new state

amdgpu_dm_display_resume: use for_each_new
    - drm_atomic_helper_duplicate_state is called during suspend to
      cache the state
    - It sets 'state' within the state triplet to 'new_state'

amdgpu_dm_commit_planes: use for_each_old
    - Called after the state was swapped (via atomic commit tail)

amdgpu_dm_atomic_commit: use for_each_new
    - Called before the state is swapped

amdgpu_dm_atomic_commit_tail: use for_each_old
    - Called after the state was swapped

dm_update_crtcs_state: use for_each_new
    - Called before the state is swapped (via atomic check)

amdgpu_dm_atomic_check: use for_each_new
    - Called before the state is swapped

v2: Split out typo fixes to a new patch.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 9bfe1f9..cc024ab 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var)
+	struct drm_crtc *crtc)
 {
 	uint32_t i;
 	struct drm_connector_state *conn_state;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc_from_state;
 
-	for_each_new_connector_in_state(
-		state,
-		connector,
-		conn_state,
-		i) {
-		crtc_from_state =
-			from_state_var ?
-				conn_state->crtc :
-				connector->state->crtc;
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		crtc_from_state = conn_state->crtc;
 
 		if (crtc_from_state == crtc)
 			return to_amdgpu_dm_connector(connector);
@@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 
 	/* update planes when needed */
-	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		struct drm_plane_state *plane_state = plane->state;
 		struct drm_crtc *crtc = plane_state->crtc;
 		struct drm_framebuffer *fb = plane_state->fb;
@@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
 	dm_state = to_dm_atomic_state(state);
 
 	/* update changed items */
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 		struct drm_crtc_state *new_state = crtc->state;
 
@@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
 			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
 
 			new_stream = new_acrtc_state->stream;
-			aconnector =
-				amdgpu_dm_find_first_crct_matching_connector(
+			aconnector = amdgpu_dm_find_first_crct_matching_connector(
 					state,
-					&new_crtcs[i]->base,
-					false);
+					&new_crtcs[i]->base);
 			if (!aconnector) {
 				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
 					 "skipping freesync init\n",
@@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* Handle scaling and undersacn changes*/
-	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
+	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
 		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 		struct dm_connector_state *con_new_state =
 				to_dm_connector_state(aconnector->base.state);
@@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* update planes when needed per crtc*/
-	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
+	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
 		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
 
 		if (new_acrtc_state->stream)
@@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
 	 * mark consumed event for drm_atomic_helper_commit_hw_done
 	 */
 	spin_lock_irqsave(&adev->ddev->event_lock, flags);
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
 		if (acrtc->base.state->event)
@@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
 		new_acrtc_state = to_dm_crtc_state(crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
-		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
+		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
 		if (aconnector) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 630e6cd..1c55a0b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var);
+	struct drm_crtc *crtc);
 
 
 struct amdgpu_framebuffer;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/amd/display: Use new DRM API where possible

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

To conform to DRM's new API, we should not be accessing a DRM object's
internal state directly. Rather, the DRM for_each_old/new_* iterators,
and drm_atomic_get_old/new_* interface should be used.

This is an ongoing process. For now, update the DRM-facing atomic
functions, where the atomic state object is given.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++++++++++-----------
 1 file changed, 66 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index cc024ab..d4426b3 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 {
 	uint32_t i;
 	struct drm_plane *plane;
-	struct drm_plane_state *old_plane_state;
+	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct dc_stream_state *dc_stream_attach;
 	struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
 	struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
-	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
+	struct drm_crtc_state *new_pcrtc_state =
+			drm_atomic_get_new_crtc_state(state, pcrtc);
+	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
 	int planes_count = 0;
 	unsigned long flags;
 
 	/* update planes when needed */
-	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
-		struct drm_plane_state *plane_state = plane->state;
-		struct drm_crtc *crtc = plane_state->crtc;
-		struct drm_framebuffer *fb = plane_state->fb;
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		struct drm_crtc *crtc = new_plane_state->crtc;
+		struct drm_crtc_state *new_crtc_state =
+				drm_atomic_get_new_crtc_state(state, crtc);
+		struct drm_framebuffer *fb = new_plane_state->fb;
 		bool pflip_needed;
-		struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
+		struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_plane_state);
 
 		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
 			handle_cursor_update(plane, old_plane_state);
 			continue;
 		}
 
-		if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
+		if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
 			continue;
 
 		pflip_needed = !state->allow_modeset;
@@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			dc_stream_attach = acrtc_state->stream;
 			planes_count++;
 
-		} else if (crtc->state->planes_changed) {
+		} else if (new_crtc_state->planes_changed) {
 			/* Assume even ONE crtc with immediate flip means
 			 * entire can't wait for VBLANK
 			 * TODO Check if it's correct
 			 */
 			*wait_for_vblank =
-					pcrtc->state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
+					new_pcrtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
 				false : true;
 
 			/* TODO: Needs rework for multiplane flip */
@@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	if (planes_count) {
 		unsigned long flags;
 
-		if (pcrtc->state->event) {
+		if (new_pcrtc_state->event) {
 
 			drm_crtc_vblank_get(pcrtc);
 
@@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
 		bool nonblock)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_state;
+	struct drm_crtc_state *old_crtc_state, *new_state;
 	struct amdgpu_device *adev = dev->dev_private;
 	int i;
 
@@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
 	 * it will update crtc->dm_crtc_state->stream pointer which is used in
 	 * the ISRs.
 	 */
-	for_each_new_crtc_in_state(state, crtc, new_state, i) {
-		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(crtc->state);
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, i) {
+		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
 		if (drm_atomic_crtc_needs_modeset(new_state) && old_acrtc_state->stream)
@@ -4002,13 +4005,13 @@ void amdgpu_dm_atomic_commit_tail(
 	uint32_t i, j;
 	uint32_t new_crtcs_count = 0;
 	struct drm_crtc *crtc, *pcrtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
 	struct dc_stream_state *new_stream = NULL;
 	unsigned long flags;
 	bool wait_for_vblank = true;
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state;
+	struct drm_connector_state *old_conn_state, *new_con_state;
 	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
 
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
@@ -4016,11 +4019,10 @@ void amdgpu_dm_atomic_commit_tail(
 	dm_state = to_dm_atomic_state(state);
 
 	/* update changed items */
-	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-		struct drm_crtc_state *new_state = crtc->state;
 
-		new_acrtc_state = to_dm_crtc_state(new_state);
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
 
 		DRM_DEBUG_DRIVER(
@@ -4028,18 +4030,18 @@ void amdgpu_dm_atomic_commit_tail(
 			"planes_changed:%d, mode_changed:%d,active_changed:%d,"
 			"connectors_changed:%d\n",
 			acrtc->crtc_id,
-			new_state->enable,
-			new_state->active,
-			new_state->planes_changed,
-			new_state->mode_changed,
-			new_state->active_changed,
-			new_state->connectors_changed);
+			new_crtc_state->enable,
+			new_crtc_state->active,
+			new_crtc_state->planes_changed,
+			new_crtc_state->mode_changed,
+			new_crtc_state->active_changed,
+			new_crtc_state->connectors_changed);
 
 		/* handles headless hotplug case, updating new_state and
 		 * aconnector as needed
 		 */
 
-		if (modeset_required(new_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
+		if (modeset_required(new_crtc_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
 
 			DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc);
 
@@ -4082,10 +4084,11 @@ void amdgpu_dm_atomic_commit_tail(
 			new_crtcs[new_crtcs_count] = acrtc;
 			new_crtcs_count++;
 
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
 			acrtc->enabled = true;
-			acrtc->hw_mode = crtc->state->mode;
-			crtc->hwmode = crtc->state->mode;
-		} else if (modereset_required(new_state)) {
+			acrtc->hw_mode = new_crtc_state->mode;
+			crtc->hwmode = new_crtc_state->mode;
+		} else if (modereset_required(new_crtc_state)) {
 			DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
 
 			/* i.e. reset mode */
@@ -4102,7 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
 		for (i = 0; i < new_crtcs_count; i++) {
 			struct amdgpu_dm_connector *aconnector = NULL;
 
-			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
+			new_crtc_state = drm_atomic_get_new_crtc_state(state,
+					&new_crtcs[i]->base);
+			new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 
 			new_stream = new_acrtc_state->stream;
 			aconnector = amdgpu_dm_find_first_crct_matching_connector(
@@ -4123,11 +4128,10 @@ void amdgpu_dm_atomic_commit_tail(
 	if (dm_state->context)
 		WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
 
-
-	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		new_acrtc_state = to_dm_crtc_state(crtc->state);
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 
 		if (new_acrtc_state->stream != NULL) {
 			const struct dc_stream_status *status =
@@ -4141,24 +4145,24 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* Handle scaling and undersacn changes*/
-	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
-		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
-		struct dm_connector_state *con_new_state =
-				to_dm_connector_state(aconnector->base.state);
-		struct dm_connector_state *con_old_state =
-				to_dm_connector_state(old_conn_state);
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_con_state, i) {
+		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
+		struct dm_connector_state *con_old_state = to_dm_connector_state(old_conn_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
 		struct dc_stream_status *status = NULL;
 
+		if (acrtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
+
 		/* Skip any modesets/resets */
-		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
+		if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
 		/* Skip any thing not scale or underscan changes */
 		if (!is_scaling_state_different(con_new_state, con_old_state))
 			continue;
 
-		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 
 		update_stream_scaling_settings(&con_new_state->base.crtc->mode,
 				con_new_state, (struct dc_stream_state *)new_acrtc_state->stream);
@@ -4185,7 +4189,8 @@ void amdgpu_dm_atomic_commit_tail(
 		 */
 		struct amdgpu_crtc *acrtc = new_crtcs[i];
 
-		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 
 		if (adev->dm.freesync_module)
 			mod_freesync_notify_mode_change(
@@ -4195,8 +4200,8 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* update planes when needed per crtc*/
-	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
-		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
+	for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 
 		if (new_acrtc_state->stream)
 			amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
@@ -4208,13 +4213,12 @@ void amdgpu_dm_atomic_commit_tail(
 	 * mark consumed event for drm_atomic_helper_commit_hw_done
 	 */
 	spin_lock_irqsave(&adev->ddev->event_lock, flags);
-	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 
-		if (acrtc->base.state->event)
-			drm_send_event_locked(dev, &crtc->state->event->base);
+		if (new_crtc_state->event)
+			drm_send_event_locked(dev, &new_crtc_state->event->base);
 
-		acrtc->base.state->event = NULL;
+		new_crtc_state->event = NULL;
 	}
 	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
 
@@ -4371,7 +4375,7 @@ static int dm_update_crtcs_state(
 		bool *lock_and_validation_needed)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *crtc_state;
 	int i;
 	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
@@ -4380,7 +4384,7 @@ static int dm_update_crtcs_state(
 
 	/*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
 	/* update changed items */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
 		struct amdgpu_crtc *acrtc = NULL;
 		struct amdgpu_dm_connector *aconnector = NULL;
 		struct drm_connector_state *conn_state = NULL;
@@ -4388,7 +4392,7 @@ static int dm_update_crtcs_state(
 
 		new_stream = NULL;
 
-		old_acrtc_state = to_dm_crtc_state(crtc->state);
+		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
 		new_acrtc_state = to_dm_crtc_state(crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
@@ -4521,7 +4525,7 @@ static int dm_update_planes_state(
 		bool *lock_and_validation_needed)
 {
 	struct drm_crtc *new_plane_crtc, *old_plane_crtc;
-	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	struct dm_crtc_state *new_acrtc_state, *old_acrtc_state;
@@ -4552,10 +4556,9 @@ static int dm_update_planes_state(
 			if (!old_plane_crtc)
 				continue;
 
-			old_acrtc_state = to_dm_crtc_state(
-					drm_atomic_get_old_crtc_state(
-							state,
-							old_plane_crtc));
+			old_crtc_state = drm_atomic_get_old_crtc_state(
+					state, old_plane_crtc);
+			old_acrtc_state = to_dm_crtc_state(old_crtc_state);
 
 			if (!old_acrtc_state->stream)
 				continue;
@@ -4643,7 +4646,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	struct dc *dc = adev->dm.dc;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
 	struct drm_connector *connector;
-	struct drm_connector_state *conn_state;
+	struct drm_connector_state *old_con_state, *conn_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 
@@ -4710,16 +4713,14 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	 * new stream into context w\o causing full reset. Need to
 	 * decide how to handle.
 	 */
-	for_each_new_connector_in_state(state, connector, conn_state, i) {
-		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
-		struct dm_connector_state *con_old_state =
-				to_dm_connector_state(aconnector->base.state);
-		struct dm_connector_state *con_new_state =
-						to_dm_connector_state(conn_state);
+	for_each_oldnew_connector_in_state(state, connector, old_con_state, conn_state, i) {
+		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
+		struct dm_connector_state *con_new_state = to_dm_connector_state(conn_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
 
 		/* Skip any modesets/resets */
-		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
+		if (!acrtc || drm_atomic_crtc_needs_modeset(
+				drm_atomic_get_new_crtc_state(state, &acrtc->base)))
 			continue;
 
 		/* Skip any thing not scale or underscan changes */
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/6] drm/amd/display: Unify DRM state variable namings.

[sunpeng.li]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Use new_*_state and old_*_state for their respective new/old DRM object
states.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 +++++++++++------------
 1 file changed, 40 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index d4426b3..fe0b658 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -573,12 +573,12 @@ struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_crtc *crtc)
 {
 	uint32_t i;
-	struct drm_connector_state *conn_state;
+	struct drm_connector_state *new_con_state;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc_from_state;
 
-	for_each_new_connector_in_state(state, connector, conn_state, i) {
-		crtc_from_state = conn_state->crtc;
+	for_each_new_connector_in_state(state, connector, new_con_state, i) {
+		crtc_from_state = new_con_state->crtc;
 
 		if (crtc_from_state == crtc)
 			return to_amdgpu_dm_connector(connector);
@@ -608,7 +608,7 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev)
 	struct amdgpu_dm_connector *aconnector;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 	int ret = 0;
 	int i;
 
@@ -644,8 +644,8 @@ int amdgpu_dm_display_resume(struct amdgpu_device *adev)
 	}
 
 	/* Force mode set in atomic comit */
-	for_each_new_crtc_in_state(adev->dm.cached_state, crtc, crtc_state, i)
-			crtc_state->active_changed = true;
+	for_each_new_crtc_in_state(adev->dm.cached_state, crtc, new_crtc_state, i)
+		new_crtc_state->active_changed = true;
 
 	ret = drm_atomic_helper_resume(ddev, adev->dm.cached_state);
 
@@ -3971,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
 		bool nonblock)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state, *new_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct amdgpu_device *adev = dev->dev_private;
 	int i;
 
@@ -3982,11 +3982,11 @@ int amdgpu_dm_atomic_commit(
 	 * it will update crtc->dm_crtc_state->stream pointer which is used in
 	 * the ISRs.
 	 */
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		if (drm_atomic_crtc_needs_modeset(new_state) && old_acrtc_state->stream)
+		if (drm_atomic_crtc_needs_modeset(new_crtc_state) && old_acrtc_state->stream)
 			manage_dm_interrupts(adev, acrtc, false);
 	}
 
@@ -4011,7 +4011,7 @@ void amdgpu_dm_atomic_commit_tail(
 	unsigned long flags;
 	bool wait_for_vblank = true;
 	struct drm_connector *connector;
-	struct drm_connector_state *old_conn_state, *new_con_state;
+	struct drm_connector_state *old_con_state, *new_con_state;
 	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
 
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
@@ -4145,9 +4145,9 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* Handle scaling and undersacn changes*/
-	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_con_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
-		struct dm_connector_state *con_old_state = to_dm_connector_state(old_conn_state);
+		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
 		struct dc_stream_status *status = NULL;
 
@@ -4375,7 +4375,7 @@ static int dm_update_crtcs_state(
 		bool *lock_and_validation_needed)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state, *crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
@@ -4384,34 +4384,34 @@ static int dm_update_crtcs_state(
 
 	/*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
 	/* update changed items */
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
+	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = NULL;
 		struct amdgpu_dm_connector *aconnector = NULL;
-		struct drm_connector_state *conn_state = NULL;
+		struct drm_connector_state *new_con_state = NULL;
 		struct dm_connector_state *dm_conn_state = NULL;
 
 		new_stream = NULL;
 
 		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
-		new_acrtc_state = to_dm_crtc_state(crtc_state);
+		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
 		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
 		if (aconnector) {
-			conn_state = drm_atomic_get_connector_state(state,
-								    &aconnector->base);
+			new_con_state = drm_atomic_get_connector_state(state,
+ 								    &aconnector->base);
 
-			if (IS_ERR(conn_state)) {
-				ret = PTR_ERR_OR_ZERO(conn_state);
+			if (IS_ERR(new_con_state)) {
+				ret = PTR_ERR_OR_ZERO(new_con_state);
 				break;
 			}
 
-			dm_conn_state = to_dm_connector_state(conn_state);
+			dm_conn_state = to_dm_connector_state(new_con_state);
 
 			new_stream = create_stream_for_sink(aconnector,
-							    &crtc_state->mode,
+							     &new_crtc_state->mode,
 							    dm_conn_state);
 
 			/*
@@ -4431,14 +4431,14 @@ static int dm_update_crtcs_state(
 		if (dc_is_stream_unchanged(new_stream,
 				old_acrtc_state->stream)) {
 
-				crtc_state->mode_changed = false;
+			new_crtc_state->mode_changed = false;
 
-				DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",
-					      crtc_state->mode_changed);
+			DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",
+					new_crtc_state->mode_changed);
 		}
 
 
-		if (!drm_atomic_crtc_needs_modeset(crtc_state))
+		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			goto next_crtc;
 
 		DRM_DEBUG_DRIVER(
@@ -4446,12 +4446,12 @@ static int dm_update_crtcs_state(
 			"planes_changed:%d, mode_changed:%d,active_changed:%d,"
 			"connectors_changed:%d\n",
 			acrtc->crtc_id,
-			crtc_state->enable,
-			crtc_state->active,
-			crtc_state->planes_changed,
-			crtc_state->mode_changed,
-			crtc_state->active_changed,
-			crtc_state->connectors_changed);
+			new_crtc_state->enable,
+			new_crtc_state->active,
+			new_crtc_state->planes_changed,
+			new_crtc_state->mode_changed,
+			new_crtc_state->active_changed,
+			new_crtc_state->connectors_changed);
 
 		/* Remove stream for any changed/disabled CRTC */
 		if (!enable) {
@@ -4478,10 +4478,10 @@ static int dm_update_crtcs_state(
 
 		} else {/* Add stream for any updated/enabled CRTC */
 
-			if (modereset_required(crtc_state))
+			if (modereset_required(new_crtc_state))
 				goto next_crtc;
 
-			if (modeset_required(crtc_state, new_stream,
+			if (modeset_required(new_crtc_state, new_stream,
 					     old_acrtc_state->stream)) {
 
 				WARN_ON(new_acrtc_state->stream);
@@ -4646,9 +4646,9 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	struct dc *dc = adev->dm.dc;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
 	struct drm_connector *connector;
-	struct drm_connector_state *old_con_state, *conn_state;
+	struct drm_connector_state *old_con_state, *new_con_state;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 
 	/*
 	 * This bool will be set for true for any modeset/reset
@@ -4667,8 +4667,8 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	 * Hack: Commit needs planes right now, specifically for gamma
 	 * TODO rework commit to check CRTC for gamma change
 	 */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		if (crtc_state->color_mgmt_changed) {
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->color_mgmt_changed) {
 			ret = drm_atomic_add_affected_planes(state, crtc);
 			if (ret)
 				goto fail;
@@ -4713,9 +4713,9 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	 * new stream into context w\o causing full reset. Need to
 	 * decide how to handle.
 	 */
-	for_each_oldnew_connector_in_state(state, connector, old_con_state, conn_state, i) {
+	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
-		struct dm_connector_state *con_new_state = to_dm_connector_state(conn_state);
+		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
 
 		/* Skip any modesets/resets */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/amd/display: Unify amdgpu_dm state variable namings.

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Use dm_new_*_state and dm_old_*_state for their respective amdgpu_dm new
and old object states. Helps with readability, and enforces use of new
DRM api (choose either new, or old).

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 137 +++++++++++-----------
 1 file changed, 68 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index fe0b658..de88ee1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3890,7 +3890,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 				drm_atomic_get_new_crtc_state(state, crtc);
 		struct drm_framebuffer *fb = new_plane_state->fb;
 		bool pflip_needed;
-		struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_plane_state);
+		struct dm_plane_state *dm_new_plane_state = to_dm_plane_state(new_plane_state);
 
 		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
 			handle_cursor_update(plane, old_plane_state);
@@ -3914,9 +3914,9 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 
 		if (!pflip_needed) {
-			WARN_ON(!dm_plane_state->dc_state);
+			WARN_ON(!dm_new_plane_state->dc_state);
 
-			plane_states_constructed[planes_count] = dm_plane_state->dc_state;
+			plane_states_constructed[planes_count] = dm_new_plane_state->dc_state;
 
 			dc_stream_attach = acrtc_state->stream;
 			planes_count++;
@@ -3983,10 +3983,10 @@ int amdgpu_dm_atomic_commit(
 	 * the ISRs.
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
-		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
+		struct dm_crtc_state *dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		if (drm_atomic_crtc_needs_modeset(new_crtc_state) && old_acrtc_state->stream)
+		if (drm_atomic_crtc_needs_modeset(new_crtc_state) && dm_old_crtc_state->stream)
 			manage_dm_interrupts(adev, acrtc, false);
 	}
 
@@ -4012,7 +4012,7 @@ void amdgpu_dm_atomic_commit_tail(
 	bool wait_for_vblank = true;
 	struct drm_connector *connector;
 	struct drm_connector_state *old_con_state, *new_con_state;
-	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
+	struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
 
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
 
@@ -4022,8 +4022,8 @@ void amdgpu_dm_atomic_commit_tail(
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
-		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 
 		DRM_DEBUG_DRIVER(
 			"amdgpu_crtc id:%d crtc_state_flags: enable:%d, active:%d, "
@@ -4041,11 +4041,11 @@ void amdgpu_dm_atomic_commit_tail(
 		 * aconnector as needed
 		 */
 
-		if (modeset_required(new_crtc_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
+		if (modeset_required(new_crtc_state, dm_new_crtc_state->stream, dm_old_crtc_state->stream)) {
 
 			DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc);
 
-			if (!new_acrtc_state->stream) {
+			if (!dm_new_crtc_state->stream) {
 				/*
 				 * this could happen because of issues with
 				 * userspace notifications delivery.
@@ -4067,8 +4067,8 @@ void amdgpu_dm_atomic_commit_tail(
 			}
 
 
-			if (old_acrtc_state->stream)
-				remove_stream(adev, acrtc, old_acrtc_state->stream);
+			if (dm_old_crtc_state->stream)
+				remove_stream(adev, acrtc, dm_old_crtc_state->stream);
 
 
 			/*
@@ -4092,8 +4092,8 @@ void amdgpu_dm_atomic_commit_tail(
 			DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
 
 			/* i.e. reset mode */
-			if (old_acrtc_state->stream)
-				remove_stream(adev, acrtc, old_acrtc_state->stream);
+			if (dm_old_crtc_state->stream)
+				remove_stream(adev, acrtc, dm_old_crtc_state->stream);
 		}
 	} /* for_each_crtc_in_state() */
 
@@ -4107,9 +4107,9 @@ void amdgpu_dm_atomic_commit_tail(
 
 			new_crtc_state = drm_atomic_get_new_crtc_state(state,
 					&new_crtcs[i]->base);
-			new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
-			new_stream = new_acrtc_state->stream;
+			new_stream = dm_new_crtc_state->stream;
 			aconnector = amdgpu_dm_find_first_crct_matching_connector(
 					state,
 					&new_crtcs[i]->base);
@@ -4131,14 +4131,14 @@ void amdgpu_dm_atomic_commit_tail(
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
-		if (new_acrtc_state->stream != NULL) {
+		if (dm_new_crtc_state->stream != NULL) {
 			const struct dc_stream_status *status =
-					dc_stream_get_status(new_acrtc_state->stream);
+					dc_stream_get_status(dm_new_crtc_state->stream);
 
 			if (!status)
-				DC_ERR("got no status for stream %p on acrtc%p\n", new_acrtc_state->stream, acrtc);
+				DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);
 			else
 				acrtc->otg_inst = status->primary_otg_inst;
 		}
@@ -4146,9 +4146,9 @@ void amdgpu_dm_atomic_commit_tail(
 
 	/* Handle scaling and undersacn changes*/
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
-		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
-		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
+		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
+		struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
+		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 		struct dc_stream_status *status = NULL;
 
 		if (acrtc)
@@ -4159,19 +4159,19 @@ void amdgpu_dm_atomic_commit_tail(
 			continue;
 
 		/* Skip any thing not scale or underscan changes */
-		if (!is_scaling_state_different(con_new_state, con_old_state))
+		if (!is_scaling_state_different(dm_new_con_state, dm_old_con_state))
 			continue;
 
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
-		update_stream_scaling_settings(&con_new_state->base.crtc->mode,
-				con_new_state, (struct dc_stream_state *)new_acrtc_state->stream);
+		update_stream_scaling_settings(&dm_new_con_state->base.crtc->mode,
+				dm_new_con_state, (struct dc_stream_state *)dm_new_crtc_state->stream);
 
-		status = dc_stream_get_status(new_acrtc_state->stream);
+		status = dc_stream_get_status(dm_new_crtc_state->stream);
 		WARN_ON(!status);
 		WARN_ON(!status->plane_count);
 
-		if (!new_acrtc_state->stream)
+		if (!dm_new_crtc_state->stream)
 			continue;
 
 		/*TODO How it works with MPO ?*/
@@ -4179,7 +4179,7 @@ void amdgpu_dm_atomic_commit_tail(
 				dm->dc,
 				status->plane_states,
 				status->plane_count,
-				new_acrtc_state->stream))
+				dm_new_crtc_state->stream))
 			dm_error("%s: Failed to update stream scaling!\n", __func__);
 	}
 
@@ -4190,20 +4190,20 @@ void amdgpu_dm_atomic_commit_tail(
 		struct amdgpu_crtc *acrtc = new_crtcs[i];
 
 		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
 		if (adev->dm.freesync_module)
 			mod_freesync_notify_mode_change(
-				adev->dm.freesync_module, &new_acrtc_state->stream, 1);
+				adev->dm.freesync_module, &dm_new_crtc_state->stream, 1);
 
 		manage_dm_interrupts(adev, acrtc, true);
 	}
 
 	/* update planes when needed per crtc*/
 	for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
-		if (new_acrtc_state->stream)
+		if (dm_new_crtc_state->stream)
 			amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
 	}
 
@@ -4377,7 +4377,7 @@ static int dm_update_crtcs_state(
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
-	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
+	struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
 	struct dc_stream_state *new_stream;
 	int ret = 0;
@@ -4392,8 +4392,8 @@ static int dm_update_crtcs_state(
 
 		new_stream = NULL;
 
-		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
-		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+		dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
+		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
 		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
@@ -4428,8 +4428,7 @@ static int dm_update_crtcs_state(
 			}
 		}
 
-		if (dc_is_stream_unchanged(new_stream,
-				old_acrtc_state->stream)) {
+		if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream)) {
 
 			new_crtc_state->mode_changed = false;
 
@@ -4456,7 +4455,7 @@ static int dm_update_crtcs_state(
 		/* Remove stream for any changed/disabled CRTC */
 		if (!enable) {
 
-			if (!old_acrtc_state->stream)
+			if (!dm_old_crtc_state->stream)
 				goto next_crtc;
 
 			DRM_DEBUG_DRIVER("Disabling DRM crtc: %d\n",
@@ -4466,13 +4465,13 @@ static int dm_update_crtcs_state(
 			if (!dc_remove_stream_from_ctx(
 					dc,
 					dm_state->context,
-					old_acrtc_state->stream)) {
+					dm_old_crtc_state->stream)) {
 				ret = -EINVAL;
 				goto fail;
 			}
 
-			dc_stream_release(old_acrtc_state->stream);
-			new_acrtc_state->stream = NULL;
+			dc_stream_release(dm_old_crtc_state->stream);
+			dm_new_crtc_state->stream = NULL;
 
 			*lock_and_validation_needed = true;
 
@@ -4482,11 +4481,11 @@ static int dm_update_crtcs_state(
 				goto next_crtc;
 
 			if (modeset_required(new_crtc_state, new_stream,
-					     old_acrtc_state->stream)) {
+					     dm_old_crtc_state->stream)) {
 
-				WARN_ON(new_acrtc_state->stream);
+				WARN_ON(dm_new_crtc_state->stream);
 
-				new_acrtc_state->stream = new_stream;
+				dm_new_crtc_state->stream = new_stream;
 				dc_stream_retain(new_stream);
 
 				DRM_DEBUG_DRIVER("Enabling DRM crtc: %d\n",
@@ -4495,7 +4494,7 @@ static int dm_update_crtcs_state(
 				if (!dc_add_stream_to_ctx(
 						dc,
 						dm_state->context,
-						new_acrtc_state->stream)) {
+						dm_new_crtc_state->stream)) {
 					ret = -EINVAL;
 					goto fail;
 				}
@@ -4528,9 +4527,9 @@ static int dm_update_planes_state(
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state, *new_plane_state;
-	struct dm_crtc_state *new_acrtc_state, *old_acrtc_state;
+	struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
 	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
-	struct dm_plane_state *new_dm_plane_state, *old_dm_plane_state;
+	struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
 	int i ;
 	/* TODO return page_flip_needed() function */
 	bool pflip_needed  = !state->allow_modeset;
@@ -4543,8 +4542,8 @@ static int dm_update_planes_state(
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		new_plane_crtc = new_plane_state->crtc;
 		old_plane_crtc = old_plane_state->crtc;
-		new_dm_plane_state = to_dm_plane_state(new_plane_state);
-		old_dm_plane_state = to_dm_plane_state(old_plane_state);
+		dm_new_plane_state = to_dm_plane_state(new_plane_state);
+		dm_old_plane_state = to_dm_plane_state(old_plane_state);
 
 		/*TODO Implement atomic check for cursor plane */
 		if (plane->type == DRM_PLANE_TYPE_CURSOR)
@@ -4558,9 +4557,9 @@ static int dm_update_planes_state(
 
 			old_crtc_state = drm_atomic_get_old_crtc_state(
 					state, old_plane_crtc);
-			old_acrtc_state = to_dm_crtc_state(old_crtc_state);
+			dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
 
-			if (!old_acrtc_state->stream)
+			if (!dm_old_crtc_state->stream)
 				continue;
 
 			DRM_DEBUG_DRIVER("Disabling DRM plane: %d on DRM crtc %d\n",
@@ -4568,8 +4567,8 @@ static int dm_update_planes_state(
 
 			if (!dc_remove_plane_from_context(
 					dc,
-					old_acrtc_state->stream,
-					old_dm_plane_state->dc_state,
+					dm_old_crtc_state->stream,
+					dm_old_plane_state->dc_state,
 					dm_state->context)) {
 
 				ret = EINVAL;
@@ -4577,8 +4576,8 @@ static int dm_update_planes_state(
 			}
 
 
-			dc_plane_state_release(old_dm_plane_state->dc_state);
-			new_dm_plane_state->dc_state = NULL;
+			dc_plane_state_release(dm_old_plane_state->dc_state);
+			dm_new_plane_state->dc_state = NULL;
 
 			*lock_and_validation_needed = true;
 
@@ -4591,27 +4590,27 @@ static int dm_update_planes_state(
 				continue;
 
 			new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_crtc);
-			new_acrtc_state = to_dm_crtc_state(new_crtc_state);
+			dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
-			if (!new_acrtc_state->stream)
+			if (!dm_new_crtc_state->stream)
 				continue;
 
 
-			WARN_ON(new_dm_plane_state->dc_state);
+			WARN_ON(dm_new_plane_state->dc_state);
 
-			new_dm_plane_state->dc_state = dc_create_plane_state(dc);
+			dm_new_plane_state->dc_state = dc_create_plane_state(dc);
 
 			DRM_DEBUG_DRIVER("Enabling DRM plane: %d on DRM crtc %d\n",
 					plane->base.id, new_plane_crtc->base.id);
 
-			if (!new_dm_plane_state->dc_state) {
+			if (!dm_new_plane_state->dc_state) {
 				ret = -EINVAL;
 				return ret;
 			}
 
 			ret = fill_plane_attributes(
 				new_plane_crtc->dev->dev_private,
-				new_dm_plane_state->dc_state,
+				dm_new_plane_state->dc_state,
 				new_plane_state,
 				new_crtc_state,
 				false);
@@ -4621,8 +4620,8 @@ static int dm_update_planes_state(
 
 			if (!dc_add_plane_to_context(
 					dc,
-					new_acrtc_state->stream,
-					new_dm_plane_state->dc_state,
+					dm_new_crtc_state->stream,
+					dm_new_plane_state->dc_state,
 					dm_state->context)) {
 
 				ret = -EINVAL;
@@ -4714,9 +4713,9 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	 * decide how to handle.
 	 */
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
-		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
-		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
-		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
+		struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
+		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
+		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(dm_new_con_state->base.crtc);
 
 		/* Skip any modesets/resets */
 		if (!acrtc || drm_atomic_crtc_needs_modeset(
@@ -4724,7 +4723,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 			continue;
 
 		/* Skip any thing not scale or underscan changes */
-		if (!is_scaling_state_different(con_new_state, con_old_state))
+		if (!is_scaling_state_different(dm_new_con_state, dm_old_con_state))
 			continue;
 
 		lock_and_validation_needed = true;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/6] drm/amd/display: Fix typo

[sunpeng.li]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

undersacn -> underscan

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index de88ee1..67222ff 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4144,7 +4144,7 @@ void amdgpu_dm_atomic_commit_tail(
 		}
 	}
 
-	/* Handle scaling and undersacn changes*/
+	/* Handle scaling and underscan changes*/
 	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
 		struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
 		struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
@@ -4707,7 +4707,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
 	 if (ret)
 		 goto fail;
 
-	/* Check scaling and undersacn changes*/
+	/* Check scaling and underscan changes*/
 	/*TODO Removed scaling changes validation due to inability to commit
 	 * new stream into context w\o causing full reset. Need to
 	 * decide how to handle.
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/amd/display: Remove useless pcrtc pointer

[sunpeng.li-5C7GfCeVMHo]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

in amdgpu_dm_atomic_commit_tail. Just use crtc instead.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 67222ff..f9b5769 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4004,7 +4004,7 @@ void amdgpu_dm_atomic_commit_tail(
 	struct dm_atomic_state *dm_state;
 	uint32_t i, j;
 	uint32_t new_crtcs_count = 0;
-	struct drm_crtc *crtc, *pcrtc;
+	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
 	struct dc_stream_state *new_stream = NULL;
@@ -4200,11 +4200,11 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* update planes when needed per crtc*/
-	for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
+	for_each_new_crtc_in_state(state, crtc, new_crtc_state, j) {
 		dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
 
 		if (dm_new_crtc_state->stream)
-			amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
+			amdgpu_dm_commit_planes(state, dev, dm, crtc, &wait_for_vblank);
 	}
 
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[Andrey Grodzovsky]

[-- Attachment #1.1: Type: text/plain, Size: 6304 bytes --]



On 10/12/2017 05:15 PM, sunpeng.li-5C7GfCeVMHo@public.gmane.org wrote:
> From: "Leo (Sunpeng) Li"<sunpeng.li-5C7GfCeVMHo@public.gmane.org>
>
> Use the correct for_each_new/old_* iterators instead of for_each_*
>
> List of affected functions:
>
> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>      - Old from_state_var flag was always choosing the new state
>
> amdgpu_dm_display_resume: use for_each_new
>      - drm_atomic_helper_duplicate_state is called during suspend to
>        cache the state
>      - It sets 'state' within the state triplet to 'new_state'

It seems to me you missed that one.

Thanks,
Andrey

>
> amdgpu_dm_commit_planes: use for_each_old
>      - Called after the state was swapped (via atomic commit tail)
>
> amdgpu_dm_atomic_commit: use for_each_new
>      - Called before the state is swapped
>
> amdgpu_dm_atomic_commit_tail: use for_each_old
>      - Called after the state was swapped
>
> dm_update_crtcs_state: use for_each_new
>      - Called before the state is swapped (via atomic check)
>
> amdgpu_dm_atomic_check: use for_each_new
>      - Called before the state is swapped
>
> v2: Split out typo fixes to a new patch.
>
> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li-5C7GfCeVMHo@public.gmane.org>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>   2 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9bfe1f9..cc024ab 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var)
> +	struct drm_crtc *crtc)
>   {
>   	uint32_t i;
>   	struct drm_connector_state *conn_state;
>   	struct drm_connector *connector;
>   	struct drm_crtc *crtc_from_state;
>   
> -	for_each_new_connector_in_state(
> -		state,
> -		connector,
> -		conn_state,
> -		i) {
> -		crtc_from_state =
> -			from_state_var ?
> -				conn_state->crtc :
> -				connector->state->crtc;
> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> +		crtc_from_state = conn_state->crtc;
>   
>   		if (crtc_from_state == crtc)
>   			return to_amdgpu_dm_connector(connector);
> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   	unsigned long flags;
>   
>   	/* update planes when needed */
> -	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
> +	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>   		struct drm_plane_state *plane_state = plane->state;
>   		struct drm_crtc *crtc = plane_state->crtc;
>   		struct drm_framebuffer *fb = plane_state->fb;
> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	dm_state = to_dm_atomic_state(state);
>   
>   	/* update changed items */
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   		struct drm_crtc_state *new_state = crtc->state;
>   
> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>   			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>   
>   			new_stream = new_acrtc_state->stream;
> -			aconnector =
> -				amdgpu_dm_find_first_crct_matching_connector(
> +			aconnector = amdgpu_dm_find_first_crct_matching_connector(
>   					state,
> -					&new_crtcs[i]->base,
> -					false);
> +					&new_crtcs[i]->base);
>   			if (!aconnector) {
>   				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
>   					 "skipping freesync init\n",
> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* Handle scaling and undersacn changes*/
> -	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
> +	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>   		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>   		struct dm_connector_state *con_new_state =
>   				to_dm_connector_state(aconnector->base.state);
> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* update planes when needed per crtc*/
> -	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
> +	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>   		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>   
>   		if (new_acrtc_state->stream)
> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	 * mark consumed event for drm_atomic_helper_commit_hw_done
>   	 */
>   	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   
>   		if (acrtc->base.state->event)
> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>   		new_acrtc_state = to_dm_crtc_state(crtc_state);
>   		acrtc = to_amdgpu_crtc(crtc);
>   
> -		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
> +		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>   
>   		/* TODO This hack should go away */
>   		if (aconnector) {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 630e6cd..1c55a0b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var);
> +	struct drm_crtc *crtc);
>   
>   
>   struct amdgpu_framebuffer;
> -- 2.7.4 _______________________________________________ amd-gfx 
> mailing list amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org 
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 7602 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[Leo]

On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
> 
> 
> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>
>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>
>> List of affected functions:
>>
>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>      - Old from_state_var flag was always choosing the new state
>>
>> amdgpu_dm_display_resume: use for_each_new
>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>        cache the state
>>      - It sets 'state' within the state triplet to 'new_state'
> 
> It seems to me you missed that one.
> 
> Thanks,
> Andrey
> 

Good catch, seems like that change was stripped out while I was cp-ing
from the internal tree. Some changes in this function have not been 
promoted to Dave's branch yet.

I'll remove this comment for now, I think it makes sense to have a 
follow-up patch to this after more changes have been promoted.

Thanks,
Leo

>>
>> amdgpu_dm_commit_planes: use for_each_old
>>      - Called after the state was swapped (via atomic commit tail)
>>
>> amdgpu_dm_atomic_commit: use for_each_new
>>      - Called before the state is swapped
>>
>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>      - Called after the state was swapped
>>
>> dm_update_crtcs_state: use for_each_new
>>      - Called before the state is swapped (via atomic check)
>>
>> amdgpu_dm_atomic_check: use for_each_new
>>      - Called before the state is swapped
>>
>> v2: Split out typo fixes to a new patch.
>>
>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>> ++++++++---------------
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 9bfe1f9..cc024ab 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var)
>> +    struct drm_crtc *crtc)
>>   {
>>       uint32_t i;
>>       struct drm_connector_state *conn_state;
>>       struct drm_connector *connector;
>>       struct drm_crtc *crtc_from_state;
>> -    for_each_new_connector_in_state(
>> -        state,
>> -        connector,
>> -        conn_state,
>> -        i) {
>> -        crtc_from_state =
>> -            from_state_var ?
>> -                conn_state->crtc :
>> -                connector->state->crtc;
>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>> +        crtc_from_state = conn_state->crtc;
>>           if (crtc_from_state == crtc)
>>               return to_amdgpu_dm_connector(connector);
>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>> drm_atomic_state *state,
>>       unsigned long flags;
>>       /* update planes when needed */
>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>           struct drm_plane_state *plane_state = plane->state;
>>           struct drm_crtc *crtc = plane_state->crtc;
>>           struct drm_framebuffer *fb = plane_state->fb;
>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       dm_state = to_dm_atomic_state(state);
>>       /* update changed items */
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>           struct drm_crtc_state *new_state = crtc->state;
>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>               new_acrtc_state = 
>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>               new_stream = new_acrtc_state->stream;
>> -            aconnector =
>> -                amdgpu_dm_find_first_crct_matching_connector(
>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>                       state,
>> -                    &new_crtcs[i]->base,
>> -                    false);
>> +                    &new_crtcs[i]->base);
>>               if (!aconnector) {
>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>> connector for acrtc id:%d "
>>                        "skipping freesync init\n",
>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* Handle scaling and undersacn changes*/
>> -    for_each_new_connector_in_state(state, connector, old_conn_state, 
>> i) {
>> +    for_each_old_connector_in_state(state, connector, old_conn_state, 
>> i) {
>>           struct amdgpu_dm_connector *aconnector = 
>> to_amdgpu_dm_connector(connector);
>>           struct dm_connector_state *con_new_state =
>>                   to_dm_connector_state(aconnector->base.state);
>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* update planes when needed per crtc*/
>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>           if (new_acrtc_state->stream)
>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>        */
>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>           if (acrtc->base.state->event)
>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>           acrtc = to_amdgpu_crtc(crtc);
>> -        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>> +        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>           /* TODO This hack should go away */
>>           if (aconnector) {
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 630e6cd..1c55a0b 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var);
>> +    struct drm_crtc *crtc);
>>   struct amdgpu_framebuffer;
>> -- 2.7.4 _______________________________________________ amd-gfx 
>> mailing list amd-gfx@lists.freedesktop.org 
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[Andrey Grodzovsky]

On 10/13/2017 11:41 AM, Leo wrote:
>
>
> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>
>>
>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>
>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>
>>> List of affected functions:
>>>
>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>      - Old from_state_var flag was always choosing the new state
>>>
>>> amdgpu_dm_display_resume: use for_each_new
>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>        cache the state
>>>      - It sets 'state' within the state triplet to 'new_state'
>>
>> It seems to me you missed that one.
>>
>> Thanks,
>> Andrey
>>
>
> Good catch, seems like that change was stripped out while I was cp-ing
> from the internal tree. Some changes in this function have not been 
> promoted to Dave's branch yet.
>
> I'll remove this comment for now, I think it makes sense to have a 
> follow-up patch to this after more changes have been promoted.
>
> Thanks,
> Leo

With that fixed the change is Reviewed-by: Andrey Grodzovsky 
<andrey.grodzovsky@amd.com>
>
>>>
>>> amdgpu_dm_commit_planes: use for_each_old
>>>      - Called after the state was swapped (via atomic commit tail)
>>>
>>> amdgpu_dm_atomic_commit: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>      - Called after the state was swapped
>>>
>>> dm_update_crtcs_state: use for_each_new
>>>      - Called before the state is swapped (via atomic check)
>>>
>>> amdgpu_dm_atomic_check: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> v2: Split out typo fixes to a new patch.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>> ++++++++---------------
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 9bfe1f9..cc024ab 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var)
>>> +    struct drm_crtc *crtc)
>>>   {
>>>       uint32_t i;
>>>       struct drm_connector_state *conn_state;
>>>       struct drm_connector *connector;
>>>       struct drm_crtc *crtc_from_state;
>>> -    for_each_new_connector_in_state(
>>> -        state,
>>> -        connector,
>>> -        conn_state,
>>> -        i) {
>>> -        crtc_from_state =
>>> -            from_state_var ?
>>> -                conn_state->crtc :
>>> -                connector->state->crtc;
>>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>> +        crtc_from_state = conn_state->crtc;
>>>           if (crtc_from_state == crtc)
>>>               return to_amdgpu_dm_connector(connector);
>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>       unsigned long flags;
>>>       /* update planes when needed */
>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>           struct drm_plane_state *plane_state = plane->state;
>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>           struct drm_framebuffer *fb = plane_state->fb;
>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       dm_state = to_dm_atomic_state(state);
>>>       /* update changed items */
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>           struct drm_crtc_state *new_state = crtc->state;
>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>               new_acrtc_state = 
>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>               new_stream = new_acrtc_state->stream;
>>> -            aconnector =
>>> -                amdgpu_dm_find_first_crct_matching_connector(
>>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>>                       state,
>>> -                    &new_crtcs[i]->base,
>>> -                    false);
>>> +                    &new_crtcs[i]->base);
>>>               if (!aconnector) {
>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>> connector for acrtc id:%d "
>>>                        "skipping freesync init\n",
>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* Handle scaling and undersacn changes*/
>>> -    for_each_new_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>> +    for_each_old_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>>           struct amdgpu_dm_connector *aconnector = 
>>> to_amdgpu_dm_connector(connector);
>>>           struct dm_connector_state *con_new_state =
>>> to_dm_connector_state(aconnector->base.state);
>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* update planes when needed per crtc*/
>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>           if (new_acrtc_state->stream)
>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>        */
>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>           if (acrtc->base.state->event)
>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>           acrtc = to_amdgpu_crtc(crtc);
>>> -        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>> +        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>           /* TODO This hack should go away */
>>>           if (aconnector) {
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> index 630e6cd..1c55a0b 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var);
>>> +    struct drm_crtc *crtc);
>>>   struct amdgpu_framebuffer;
>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>> mailing list amd-gfx@lists.freedesktop.org 
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Harry Wentland]

On 2017-10-12 05:15 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> To conform to DRM's new API, we should not be accessing a DRM object's
> internal state directly. Rather, the DRM for_each_old/new_* iterators,
> and drm_atomic_get_old/new_* interface should be used.
> 
> This is an ongoing process. For now, update the DRM-facing atomic
> functions, where the atomic state object is given.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++++++++++-----------
>  1 file changed, 66 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cc024ab..d4426b3 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  {
>  	uint32_t i;
>  	struct drm_plane *plane;
> -	struct drm_plane_state *old_plane_state;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct dc_stream_state *dc_stream_attach;
>  	struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
>  	struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
> -	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
> +	struct drm_crtc_state *new_pcrtc_state =
> +			drm_atomic_get_new_crtc_state(state, pcrtc);
> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>  	int planes_count = 0;
>  	unsigned long flags;
>  
>  	/* update planes when needed */
> -	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> -		struct drm_plane_state *plane_state = plane->state;
> -		struct drm_crtc *crtc = plane_state->crtc;
> -		struct drm_framebuffer *fb = plane_state->fb;
> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> +		struct drm_crtc *crtc = new_plane_state->crtc;
> +		struct drm_crtc_state *new_crtc_state =
> +				drm_atomic_get_new_crtc_state(state, crtc);
> +		struct drm_framebuffer *fb = new_plane_state->fb;
>  		bool pflip_needed;
> -		struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
> +		struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_plane_state);
>  
>  		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>  			handle_cursor_update(plane, old_plane_state);
>  			continue;
>  		}
>  
> -		if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
> +		if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
>  			continue;
>  
>  		pflip_needed = !state->allow_modeset;
> @@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  			dc_stream_attach = acrtc_state->stream;
>  			planes_count++;
>  
> -		} else if (crtc->state->planes_changed) {
> +		} else if (new_crtc_state->planes_changed) {
>  			/* Assume even ONE crtc with immediate flip means
>  			 * entire can't wait for VBLANK
>  			 * TODO Check if it's correct
>  			 */
>  			*wait_for_vblank =
> -					pcrtc->state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
> +					new_pcrtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
>  				false : true;
>  
>  			/* TODO: Needs rework for multiplane flip */
> @@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  	if (planes_count) {
>  		unsigned long flags;
>  
> -		if (pcrtc->state->event) {
> +		if (new_pcrtc_state->event) {
>  
>  			drm_crtc_vblank_get(pcrtc);
>  
> @@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
>  		bool nonblock)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *new_state;
> +	struct drm_crtc_state *old_crtc_state, *new_state;
>  	struct amdgpu_device *adev = dev->dev_private;
>  	int i;
>  
> @@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
>  	 * it will update crtc->dm_crtc_state->stream pointer which is used in
>  	 * the ISRs.
>  	 */
> -	for_each_new_crtc_in_state(state, crtc, new_state, i) {
> -		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(crtc->state);
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, i) {
> +		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  
>  		if (drm_atomic_crtc_needs_modeset(new_state) && old_acrtc_state->stream)
> @@ -4002,13 +4005,13 @@ void amdgpu_dm_atomic_commit_tail(
>  	uint32_t i, j;
>  	uint32_t new_crtcs_count = 0;
>  	struct drm_crtc *crtc, *pcrtc;
> -	struct drm_crtc_state *old_crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
>  	struct dc_stream_state *new_stream = NULL;
>  	unsigned long flags;
>  	bool wait_for_vblank = true;
>  	struct drm_connector *connector;
> -	struct drm_connector_state *old_conn_state;
> +	struct drm_connector_state *old_conn_state, *new_con_state;
>  	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>  
>  	drm_atomic_helper_update_legacy_modeset_state(dev, state);
> @@ -4016,11 +4019,10 @@ void amdgpu_dm_atomic_commit_tail(
>  	dm_state = to_dm_atomic_state(state);
>  
>  	/* update changed items */
> -	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> -		struct drm_crtc_state *new_state = crtc->state;
>  
> -		new_acrtc_state = to_dm_crtc_state(new_state);
> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>  
>  		DRM_DEBUG_DRIVER(
> @@ -4028,18 +4030,18 @@ void amdgpu_dm_atomic_commit_tail(
>  			"planes_changed:%d, mode_changed:%d,active_changed:%d,"
>  			"connectors_changed:%d\n",
>  			acrtc->crtc_id,
> -			new_state->enable,
> -			new_state->active,
> -			new_state->planes_changed,
> -			new_state->mode_changed,
> -			new_state->active_changed,
> -			new_state->connectors_changed);
> +			new_crtc_state->enable,
> +			new_crtc_state->active,
> +			new_crtc_state->planes_changed,
> +			new_crtc_state->mode_changed,
> +			new_crtc_state->active_changed,
> +			new_crtc_state->connectors_changed);
>  
>  		/* handles headless hotplug case, updating new_state and
>  		 * aconnector as needed
>  		 */
>  
> -		if (modeset_required(new_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
> +		if (modeset_required(new_crtc_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
>  
>  			DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc);
>  
> @@ -4082,10 +4084,11 @@ void amdgpu_dm_atomic_commit_tail(
>  			new_crtcs[new_crtcs_count] = acrtc;
>  			new_crtcs_count++;
>  
> +			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>  			acrtc->enabled = true;
> -			acrtc->hw_mode = crtc->state->mode;
> -			crtc->hwmode = crtc->state->mode;
> -		} else if (modereset_required(new_state)) {
> +			acrtc->hw_mode = new_crtc_state->mode;
> +			crtc->hwmode = new_crtc_state->mode;
> +		} else if (modereset_required(new_crtc_state)) {
>  			DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
>  
>  			/* i.e. reset mode */
> @@ -4102,7 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>  		for (i = 0; i < new_crtcs_count; i++) {
>  			struct amdgpu_dm_connector *aconnector = NULL;
>  
> -			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
> +			new_crtc_state = drm_atomic_get_new_crtc_state(state,
> +					&new_crtcs[i]->base);
> +			new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  
>  			new_stream = new_acrtc_state->stream;
>  			aconnector = amdgpu_dm_find_first_crct_matching_connector(
> @@ -4123,11 +4128,10 @@ void amdgpu_dm_atomic_commit_tail(
>  	if (dm_state->context)
>  		WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
>  
> -
> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  
> -		new_acrtc_state = to_dm_crtc_state(crtc->state);
> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  
>  		if (new_acrtc_state->stream != NULL) {
>  			const struct dc_stream_status *status =
> @@ -4141,24 +4145,24 @@ void amdgpu_dm_atomic_commit_tail(
>  	}
>  
>  	/* Handle scaling and undersacn changes*/
> -	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
> -		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> -		struct dm_connector_state *con_new_state =
> -				to_dm_connector_state(aconnector->base.state);
> -		struct dm_connector_state *con_old_state =
> -				to_dm_connector_state(old_conn_state);
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_con_state, i) {
> +		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
> +		struct dm_connector_state *con_old_state = to_dm_connector_state(old_conn_state);
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>  		struct dc_stream_status *status = NULL;
>  
> +		if (acrtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
> +
>  		/* Skip any modesets/resets */
> -		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
> +		if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
>  			continue;
>  
>  		/* Skip any thing not scale or underscan changes */
>  		if (!is_scaling_state_different(con_new_state, con_old_state))
>  			continue;
>  
> -		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  
>  		update_stream_scaling_settings(&con_new_state->base.crtc->mode,
>  				con_new_state, (struct dc_stream_state *)new_acrtc_state->stream);
> @@ -4185,7 +4189,8 @@ void amdgpu_dm_atomic_commit_tail(
>  		 */
>  		struct amdgpu_crtc *acrtc = new_crtcs[i];
>  
> -		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  
>  		if (adev->dm.freesync_module)
>  			mod_freesync_notify_mode_change(
> @@ -4195,8 +4200,8 @@ void amdgpu_dm_atomic_commit_tail(
>  	}
>  
>  	/* update planes when needed per crtc*/
> -	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
> -		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
> +	for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>  
>  		if (new_acrtc_state->stream)
>  			amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
> @@ -4208,13 +4213,12 @@ void amdgpu_dm_atomic_commit_tail(
>  	 * mark consumed event for drm_atomic_helper_commit_hw_done
>  	 */
>  	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> -	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> -		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {

Andrey might be able to comment on this better, but weren't we intentionally
trying to iterate the old crtc here to make sure we send any even on it?

Harry

>  
> -		if (acrtc->base.state->event)
> -			drm_send_event_locked(dev, &crtc->state->event->base);
> +		if (new_crtc_state->event)
> +			drm_send_event_locked(dev, &new_crtc_state->event->base);
>  
> -		acrtc->base.state->event = NULL;
> +		new_crtc_state->event = NULL;
>  	}
>  	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>  
> @@ -4371,7 +4375,7 @@ static int dm_update_crtcs_state(
>  		bool *lock_and_validation_needed)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *crtc_state;
>  	int i;
>  	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>  	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
> @@ -4380,7 +4384,7 @@ static int dm_update_crtcs_state(
>  
>  	/*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
>  	/* update changed items */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = NULL;
>  		struct amdgpu_dm_connector *aconnector = NULL;
>  		struct drm_connector_state *conn_state = NULL;
> @@ -4388,7 +4392,7 @@ static int dm_update_crtcs_state(
>  
>  		new_stream = NULL;
>  
> -		old_acrtc_state = to_dm_crtc_state(crtc->state);
> +		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>  		new_acrtc_state = to_dm_crtc_state(crtc_state);
>  		acrtc = to_amdgpu_crtc(crtc);
>  
> @@ -4521,7 +4525,7 @@ static int dm_update_planes_state(
>  		bool *lock_and_validation_needed)
>  {
>  	struct drm_crtc *new_plane_crtc, *old_plane_crtc;
> -	struct drm_crtc_state *new_crtc_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	struct drm_plane *plane;
>  	struct drm_plane_state *old_plane_state, *new_plane_state;
>  	struct dm_crtc_state *new_acrtc_state, *old_acrtc_state;
> @@ -4552,10 +4556,9 @@ static int dm_update_planes_state(
>  			if (!old_plane_crtc)
>  				continue;
>  
> -			old_acrtc_state = to_dm_crtc_state(
> -					drm_atomic_get_old_crtc_state(
> -							state,
> -							old_plane_crtc));
> +			old_crtc_state = drm_atomic_get_old_crtc_state(
> +					state, old_plane_crtc);
> +			old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>  
>  			if (!old_acrtc_state->stream)
>  				continue;
> @@ -4643,7 +4646,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>  	struct dc *dc = adev->dm.dc;
>  	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>  	struct drm_connector *connector;
> -	struct drm_connector_state *conn_state;
> +	struct drm_connector_state *old_con_state, *conn_state;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  
> @@ -4710,16 +4713,14 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>  	 * new stream into context w\o causing full reset. Need to
>  	 * decide how to handle.
>  	 */
> -	for_each_new_connector_in_state(state, connector, conn_state, i) {
> -		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
> -		struct dm_connector_state *con_old_state =
> -				to_dm_connector_state(aconnector->base.state);
> -		struct dm_connector_state *con_new_state =
> -						to_dm_connector_state(conn_state);
> +	for_each_oldnew_connector_in_state(state, connector, old_con_state, conn_state, i) {
> +		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
> +		struct dm_connector_state *con_new_state = to_dm_connector_state(conn_state);
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>  
>  		/* Skip any modesets/resets */
> -		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
> +		if (!acrtc || drm_atomic_crtc_needs_modeset(
> +				drm_atomic_get_new_crtc_state(state, &acrtc->base)))
>  			continue;
>  
>  		/* Skip any thing not scale or underscan changes */
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm/amd/display: Fix typo

[Alex Deucher]

On Thu, Oct 12, 2017 at 5:15 PM,  <sunpeng.li@amd.com> wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>
> undersacn -> underscan
>
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index de88ee1..67222ff 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4144,7 +4144,7 @@ void amdgpu_dm_atomic_commit_tail(
>                 }
>         }
>
> -       /* Handle scaling and undersacn changes*/
> +       /* Handle scaling and underscan changes*/
>         for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
>                 struct dm_connector_state *dm_new_con_state = to_dm_connector_state(new_con_state);
>                 struct dm_connector_state *dm_old_con_state = to_dm_connector_state(old_con_state);
> @@ -4707,7 +4707,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>          if (ret)
>                  goto fail;
>
> -       /* Check scaling and undersacn changes*/
> +       /* Check scaling and underscan changes*/
>         /*TODO Removed scaling changes validation due to inability to commit
>          * new stream into context w\o causing full reset. Need to
>          * decide how to handle.
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/6] drm/amd/display: Remove useless pcrtc pointer

[Alex Deucher]

On Thu, Oct 12, 2017 at 5:15 PM,  <sunpeng.li@amd.com> wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>
> in amdgpu_dm_atomic_commit_tail. Just use crtc instead.
>
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 67222ff..f9b5769 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -4004,7 +4004,7 @@ void amdgpu_dm_atomic_commit_tail(
>         struct dm_atomic_state *dm_state;
>         uint32_t i, j;
>         uint32_t new_crtcs_count = 0;
> -       struct drm_crtc *crtc, *pcrtc;
> +       struct drm_crtc *crtc;
>         struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>         struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
>         struct dc_stream_state *new_stream = NULL;
> @@ -4200,11 +4200,11 @@ void amdgpu_dm_atomic_commit_tail(
>         }
>
>         /* update planes when needed per crtc*/
> -       for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
> +       for_each_new_crtc_in_state(state, crtc, new_crtc_state, j) {
>                 dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>
>                 if (dm_new_crtc_state->stream)
> -                       amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
> +                       amdgpu_dm_commit_planes(state, dev, dm, crtc, &wait_for_vblank);
>         }
>
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Use new DRM API where possible, and cleanups.

[Harry Wentland]

Patches 3-6 are
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

On 2017-10-12 05:15 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> Hi Dave,
> 
> This series reworks the previous patch. Patch 1 is a v2 of the previous,
> and additional patches are from the feedback received. They apply on top
> of your drm-next-amd-dc-staging branch.
> 
> Thanks,
> Leo
> 
> Leo (Sunpeng) Li (6):
>   drm/amd/display: Use DRM new-style object iterators.
>   drm/amd/display: Use new DRM API where possible
>   drm/amd/display: Unify DRM state variable namings.
>   drm/amd/display: Unify amdgpu_dm state variable namings.
>   drm/amd/display: Fix typo
>   drm/amd/display: Remove useless pcrtc pointer
> 
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 320 +++++++++++-----------
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +-
>  2 files changed, 156 insertions(+), 167 deletions(-)
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[Leo]

On 2017-10-13 11:56 AM, Andrey Grodzovsky wrote:
> 
> 
> On 10/13/2017 11:41 AM, Leo wrote:
>>
>>
>> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>>
>>>
>>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>>
>>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>>
>>>> List of affected functions:
>>>>
>>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>>      - Old from_state_var flag was always choosing the new state
>>>>
>>>> amdgpu_dm_display_resume: use for_each_new
>>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>>        cache the state
>>>>      - It sets 'state' within the state triplet to 'new_state'
>>>
>>> It seems to me you missed that one.
>>>
>>> Thanks,
>>> Andrey
>>>
>>
>> Good catch, seems like that change was stripped out while I was cp-ing
>> from the internal tree. Some changes in this function have not been 
>> promoted to Dave's branch yet.
>>
>> I'll remove this comment for now, I think it makes sense to have a 
>> follow-up patch to this after more changes have been promoted.
>>
>> Thanks,
>> Leo
> 
> With that fixed the change is Reviewed-by: Andrey Grodzovsky 
> <andrey.grodzovsky@amd.com>

On second look, this comment is addressing the change within Dave's
patch, on which this series apply. I was trying to justify all the 
changes made, including the ones already done by Dave. See here:

https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next-amd-dc-staging&id=e7b8e99bed73e9c42f1c074ad6009cb59a79bd52

I think changing "List of affected functions" to "The following 
functions were considered" would make it less confusing, and will
still make sense if it gets squashed with Dave's patch.

Leo

>>
>>>>
>>>> amdgpu_dm_commit_planes: use for_each_old
>>>>      - Called after the state was swapped (via atomic commit tail)
>>>>
>>>> amdgpu_dm_atomic_commit: use for_each_new
>>>>      - Called before the state is swapped
>>>>
>>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>>      - Called after the state was swapped
>>>>
>>>> dm_update_crtcs_state: use for_each_new
>>>>      - Called before the state is swapped (via atomic check)
>>>>
>>>> amdgpu_dm_atomic_check: use for_each_new
>>>>      - Called before the state is swapped
>>>>
>>>> v2: Split out typo fixes to a new patch.
>>>>
>>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>>> ++++++++---------------
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 9bfe1f9..cc024ab 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>>   struct amdgpu_dm_connector 
>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>       struct drm_atomic_state *state,
>>>> -    struct drm_crtc *crtc,
>>>> -    bool from_state_var)
>>>> +    struct drm_crtc *crtc)
>>>>   {
>>>>       uint32_t i;
>>>>       struct drm_connector_state *conn_state;
>>>>       struct drm_connector *connector;
>>>>       struct drm_crtc *crtc_from_state;
>>>> -    for_each_new_connector_in_state(
>>>> -        state,
>>>> -        connector,
>>>> -        conn_state,
>>>> -        i) {
>>>> -        crtc_from_state =
>>>> -            from_state_var ?
>>>> -                conn_state->crtc :
>>>> -                connector->state->crtc;
>>>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>>> +        crtc_from_state = conn_state->crtc;
>>>>           if (crtc_from_state == crtc)
>>>>               return to_amdgpu_dm_connector(connector);
>>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>>> drm_atomic_state *state,
>>>>       unsigned long flags;
>>>>       /* update planes when needed */
>>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>>           struct drm_plane_state *plane_state = plane->state;
>>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>>           struct drm_framebuffer *fb = plane_state->fb;
>>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       dm_state = to_dm_atomic_state(state);
>>>>       /* update changed items */
>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>           struct drm_crtc_state *new_state = crtc->state;
>>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>>               new_acrtc_state = 
>>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>>               new_stream = new_acrtc_state->stream;
>>>> -            aconnector =
>>>> -                amdgpu_dm_find_first_crct_matching_connector(
>>>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>>>                       state,
>>>> -                    &new_crtcs[i]->base,
>>>> -                    false);
>>>> +                    &new_crtcs[i]->base);
>>>>               if (!aconnector) {
>>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>>> connector for acrtc id:%d "
>>>>                        "skipping freesync init\n",
>>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       }
>>>>       /* Handle scaling and undersacn changes*/
>>>> -    for_each_new_connector_in_state(state, connector, 
>>>> old_conn_state, i) {
>>>> +    for_each_old_connector_in_state(state, connector, 
>>>> old_conn_state, i) {
>>>>           struct amdgpu_dm_connector *aconnector = 
>>>> to_amdgpu_dm_connector(connector);
>>>>           struct dm_connector_state *con_new_state =
>>>> to_dm_connector_state(aconnector->base.state);
>>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       }
>>>>       /* update planes when needed per crtc*/
>>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>>           if (new_acrtc_state->stream)
>>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>>        */
>>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>           if (acrtc->base.state->event)
>>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>>           acrtc = to_amdgpu_crtc(crtc);
>>>> -        aconnector = 
>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>>> +        aconnector = 
>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>>           /* TODO This hack should go away */
>>>>           if (aconnector) {
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> index 630e6cd..1c55a0b 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>>   struct amdgpu_dm_connector 
>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>       struct drm_atomic_state *state,
>>>> -    struct drm_crtc *crtc,
>>>> -    bool from_state_var);
>>>> +    struct drm_crtc *crtc);
>>>>   struct amdgpu_framebuffer;
>>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>>> mailing list amd-gfx@lists.freedesktop.org 
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Andrey Grodzovsky]

On 10/13/2017 12:18 PM, Harry Wentland wrote:
> On 2017-10-12 05:15 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> To conform to DRM's new API, we should not be accessing a DRM object's
>> internal state directly. Rather, the DRM for_each_old/new_* iterators,
>> and drm_atomic_get_old/new_* interface should be used.
>>
>> This is an ongoing process. For now, update the DRM-facing atomic
>> functions, where the atomic state object is given.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++++++++++-----------
>>   1 file changed, 66 insertions(+), 65 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index cc024ab..d4426b3 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   {
>>   	uint32_t i;
>>   	struct drm_plane *plane;
>> -	struct drm_plane_state *old_plane_state;
>> +	struct drm_plane_state *old_plane_state, *new_plane_state;
>>   	struct dc_stream_state *dc_stream_attach;
>>   	struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
>>   	struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
>> -	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
>> +	struct drm_crtc_state *new_pcrtc_state =
>> +			drm_atomic_get_new_crtc_state(state, pcrtc);
>> +	struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>>   	int planes_count = 0;
>>   	unsigned long flags;
>>   
>>   	/* update planes when needed */
>> -	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>> -		struct drm_plane_state *plane_state = plane->state;
>> -		struct drm_crtc *crtc = plane_state->crtc;
>> -		struct drm_framebuffer *fb = plane_state->fb;
>> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>> +		struct drm_crtc *crtc = new_plane_state->crtc;
>> +		struct drm_crtc_state *new_crtc_state =
>> +				drm_atomic_get_new_crtc_state(state, crtc);
>> +		struct drm_framebuffer *fb = new_plane_state->fb;
>>   		bool pflip_needed;
>> -		struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
>> +		struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_plane_state);
>>   
>>   		if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>>   			handle_cursor_update(plane, old_plane_state);
>>   			continue;
>>   		}
>>   
>> -		if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
>> +		if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
>>   			continue;
>>   
>>   		pflip_needed = !state->allow_modeset;
>> @@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   			dc_stream_attach = acrtc_state->stream;
>>   			planes_count++;
>>   
>> -		} else if (crtc->state->planes_changed) {
>> +		} else if (new_crtc_state->planes_changed) {
>>   			/* Assume even ONE crtc with immediate flip means
>>   			 * entire can't wait for VBLANK
>>   			 * TODO Check if it's correct
>>   			 */
>>   			*wait_for_vblank =
>> -					pcrtc->state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
>> +					new_pcrtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
>>   				false : true;
>>   
>>   			/* TODO: Needs rework for multiplane flip */
>> @@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>   	if (planes_count) {
>>   		unsigned long flags;
>>   
>> -		if (pcrtc->state->event) {
>> +		if (new_pcrtc_state->event) {
>>   
>>   			drm_crtc_vblank_get(pcrtc);
>>   
>> @@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
>>   		bool nonblock)
>>   {
>>   	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *new_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_state;
>>   	struct amdgpu_device *adev = dev->dev_private;
>>   	int i;
>>   
>> @@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
>>   	 * it will update crtc->dm_crtc_state->stream pointer which is used in
>>   	 * the ISRs.
>>   	 */
>> -	for_each_new_crtc_in_state(state, crtc, new_state, i) {
>> -		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(crtc->state);
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, i) {
>> +		struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>   
>>   		if (drm_atomic_crtc_needs_modeset(new_state) && old_acrtc_state->stream)
>> @@ -4002,13 +4005,13 @@ void amdgpu_dm_atomic_commit_tail(
>>   	uint32_t i, j;
>>   	uint32_t new_crtcs_count = 0;
>>   	struct drm_crtc *crtc, *pcrtc;
>> -	struct drm_crtc_state *old_crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>   	struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
>>   	struct dc_stream_state *new_stream = NULL;
>>   	unsigned long flags;
>>   	bool wait_for_vblank = true;
>>   	struct drm_connector *connector;
>> -	struct drm_connector_state *old_conn_state;
>> +	struct drm_connector_state *old_conn_state, *new_con_state;
>>   	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>>   
>>   	drm_atomic_helper_update_legacy_modeset_state(dev, state);
>> @@ -4016,11 +4019,10 @@ void amdgpu_dm_atomic_commit_tail(
>>   	dm_state = to_dm_atomic_state(state);
>>   
>>   	/* update changed items */
>> -	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> -		struct drm_crtc_state *new_state = crtc->state;
>>   
>> -		new_acrtc_state = to_dm_crtc_state(new_state);
>> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>   
>>   		DRM_DEBUG_DRIVER(
>> @@ -4028,18 +4030,18 @@ void amdgpu_dm_atomic_commit_tail(
>>   			"planes_changed:%d, mode_changed:%d,active_changed:%d,"
>>   			"connectors_changed:%d\n",
>>   			acrtc->crtc_id,
>> -			new_state->enable,
>> -			new_state->active,
>> -			new_state->planes_changed,
>> -			new_state->mode_changed,
>> -			new_state->active_changed,
>> -			new_state->connectors_changed);
>> +			new_crtc_state->enable,
>> +			new_crtc_state->active,
>> +			new_crtc_state->planes_changed,
>> +			new_crtc_state->mode_changed,
>> +			new_crtc_state->active_changed,
>> +			new_crtc_state->connectors_changed);
>>   
>>   		/* handles headless hotplug case, updating new_state and
>>   		 * aconnector as needed
>>   		 */
>>   
>> -		if (modeset_required(new_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
>> +		if (modeset_required(new_crtc_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
>>   
>>   			DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc);
>>   
>> @@ -4082,10 +4084,11 @@ void amdgpu_dm_atomic_commit_tail(
>>   			new_crtcs[new_crtcs_count] = acrtc;
>>   			new_crtcs_count++;
>>   
>> +			new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>   			acrtc->enabled = true;
>> -			acrtc->hw_mode = crtc->state->mode;
>> -			crtc->hwmode = crtc->state->mode;
>> -		} else if (modereset_required(new_state)) {
>> +			acrtc->hw_mode = new_crtc_state->mode;
>> +			crtc->hwmode = new_crtc_state->mode;
>> +		} else if (modereset_required(new_crtc_state)) {
>>   			DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
>>   
>>   			/* i.e. reset mode */
>> @@ -4102,7 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>   		for (i = 0; i < new_crtcs_count; i++) {
>>   			struct amdgpu_dm_connector *aconnector = NULL;
>>   
>> -			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>> +			new_crtc_state = drm_atomic_get_new_crtc_state(state,
>> +					&new_crtcs[i]->base);
>> +			new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   
>>   			new_stream = new_acrtc_state->stream;
>>   			aconnector = amdgpu_dm_find_first_crct_matching_connector(
>> @@ -4123,11 +4128,10 @@ void amdgpu_dm_atomic_commit_tail(
>>   	if (dm_state->context)
>>   		WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
>>   
>> -
>> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>   
>> -		new_acrtc_state = to_dm_crtc_state(crtc->state);
>> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   
>>   		if (new_acrtc_state->stream != NULL) {
>>   			const struct dc_stream_status *status =
>> @@ -4141,24 +4145,24 @@ void amdgpu_dm_atomic_commit_tail(
>>   	}
>>   
>>   	/* Handle scaling and undersacn changes*/
>> -	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>> -		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> -		struct dm_connector_state *con_new_state =
>> -				to_dm_connector_state(aconnector->base.state);
>> -		struct dm_connector_state *con_old_state =
>> -				to_dm_connector_state(old_conn_state);
>> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_con_state, i) {
>> +		struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
>> +		struct dm_connector_state *con_old_state = to_dm_connector_state(old_conn_state);
>>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>>   		struct dc_stream_status *status = NULL;
>>   
>> +		if (acrtc)
>> +			new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
>> +
>>   		/* Skip any modesets/resets */
>> -		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
>> +		if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
>>   			continue;
>>   
>>   		/* Skip any thing not scale or underscan changes */
>>   		if (!is_scaling_state_different(con_new_state, con_old_state))
>>   			continue;
>>   
>> -		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
>> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   
>>   		update_stream_scaling_settings(&con_new_state->base.crtc->mode,
>>   				con_new_state, (struct dc_stream_state *)new_acrtc_state->stream);
>> @@ -4185,7 +4189,8 @@ void amdgpu_dm_atomic_commit_tail(
>>   		 */
>>   		struct amdgpu_crtc *acrtc = new_crtcs[i];
>>   
>> -		new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
>> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
>> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   
>>   		if (adev->dm.freesync_module)
>>   			mod_freesync_notify_mode_change(
>> @@ -4195,8 +4200,8 @@ void amdgpu_dm_atomic_commit_tail(
>>   	}
>>   
>>   	/* update planes when needed per crtc*/
>> -	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>> -		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>> +	for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
>> +		new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>   
>>   		if (new_acrtc_state->stream)
>>   			amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
>> @@ -4208,13 +4213,12 @@ void amdgpu_dm_atomic_commit_tail(
>>   	 * mark consumed event for drm_atomic_helper_commit_hw_done
>>   	 */
>>   	spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> -	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>> -		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> +	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> Andrey might be able to comment on this better, but weren't we intentionally
> trying to iterate the old crtc here to make sure we send any even on it?
>
> Harry

The states we are interested in are still the new states because that 
where the event we want
to send resides (drm_send_event_locked(dev, &crtc->state->event->base), 
with the introduction of for_each_new*
iterators we should use them to iterate the new states.

Thanks,
Andrey

>
>>   
>> -		if (acrtc->base.state->event)
>> -			drm_send_event_locked(dev, &crtc->state->event->base);
>> +		if (new_crtc_state->event)
>> +			drm_send_event_locked(dev, &new_crtc_state->event->base);
>>   
>> -		acrtc->base.state->event = NULL;
>> +		new_crtc_state->event = NULL;
>>   	}
>>   	spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>>   
>> @@ -4371,7 +4375,7 @@ static int dm_update_crtcs_state(
>>   		bool *lock_and_validation_needed)
>>   {
>>   	struct drm_crtc *crtc;
>> -	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *crtc_state;
>>   	int i;
>>   	struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>>   	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>> @@ -4380,7 +4384,7 @@ static int dm_update_crtcs_state(
>>   
>>   	/*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
>>   	/* update changed items */
>> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
>>   		struct amdgpu_crtc *acrtc = NULL;
>>   		struct amdgpu_dm_connector *aconnector = NULL;
>>   		struct drm_connector_state *conn_state = NULL;
>> @@ -4388,7 +4392,7 @@ static int dm_update_crtcs_state(
>>   
>>   		new_stream = NULL;
>>   
>> -		old_acrtc_state = to_dm_crtc_state(crtc->state);
>> +		old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>   		new_acrtc_state = to_dm_crtc_state(crtc_state);
>>   		acrtc = to_amdgpu_crtc(crtc);
>>   
>> @@ -4521,7 +4525,7 @@ static int dm_update_planes_state(
>>   		bool *lock_and_validation_needed)
>>   {
>>   	struct drm_crtc *new_plane_crtc, *old_plane_crtc;
>> -	struct drm_crtc_state *new_crtc_state;
>> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>   	struct drm_plane *plane;
>>   	struct drm_plane_state *old_plane_state, *new_plane_state;
>>   	struct dm_crtc_state *new_acrtc_state, *old_acrtc_state;
>> @@ -4552,10 +4556,9 @@ static int dm_update_planes_state(
>>   			if (!old_plane_crtc)
>>   				continue;
>>   
>> -			old_acrtc_state = to_dm_crtc_state(
>> -					drm_atomic_get_old_crtc_state(
>> -							state,
>> -							old_plane_crtc));
>> +			old_crtc_state = drm_atomic_get_old_crtc_state(
>> +					state, old_plane_crtc);
>> +			old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>   
>>   			if (!old_acrtc_state->stream)
>>   				continue;
>> @@ -4643,7 +4646,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>   	struct dc *dc = adev->dm.dc;
>>   	struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>>   	struct drm_connector *connector;
>> -	struct drm_connector_state *conn_state;
>> +	struct drm_connector_state *old_con_state, *conn_state;
>>   	struct drm_crtc *crtc;
>>   	struct drm_crtc_state *crtc_state;
>>   
>> @@ -4710,16 +4713,14 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>   	 * new stream into context w\o causing full reset. Need to
>>   	 * decide how to handle.
>>   	 */
>> -	for_each_new_connector_in_state(state, connector, conn_state, i) {
>> -		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>> -		struct dm_connector_state *con_old_state =
>> -				to_dm_connector_state(aconnector->base.state);
>> -		struct dm_connector_state *con_new_state =
>> -						to_dm_connector_state(conn_state);
>> +	for_each_oldnew_connector_in_state(state, connector, old_con_state, conn_state, i) {
>> +		struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
>> +		struct dm_connector_state *con_new_state = to_dm_connector_state(conn_state);
>>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>>   
>>   		/* Skip any modesets/resets */
>> -		if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
>> +		if (!acrtc || drm_atomic_crtc_needs_modeset(
>> +				drm_atomic_get_new_crtc_state(state, &acrtc->base)))
>>   			continue;
>>   
>>   		/* Skip any thing not scale or underscan changes */
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2 1/6] drm/amd/display: Use DRM new-style object iterators.

[Andrey Grodzovsky]

On 10/13/2017 12:35 PM, Leo wrote:
>
>
> On 2017-10-13 11:56 AM, Andrey Grodzovsky wrote:
>>
>>
>> On 10/13/2017 11:41 AM, Leo wrote:
>>>
>>>
>>> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>>>
>>>>
>>>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>>>
>>>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>>>
>>>>> List of affected functions:
>>>>>
>>>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>>>      - Old from_state_var flag was always choosing the new state
>>>>>
>>>>> amdgpu_dm_display_resume: use for_each_new
>>>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>>>        cache the state
>>>>>      - It sets 'state' within the state triplet to 'new_state'
>>>>
>>>> It seems to me you missed that one.
>>>>
>>>> Thanks,
>>>> Andrey
>>>>
>>>
>>> Good catch, seems like that change was stripped out while I was cp-ing
>>> from the internal tree. Some changes in this function have not been 
>>> promoted to Dave's branch yet.
>>>
>>> I'll remove this comment for now, I think it makes sense to have a 
>>> follow-up patch to this after more changes have been promoted.
>>>
>>> Thanks,
>>> Leo
>>
>> With that fixed the change is Reviewed-by: Andrey Grodzovsky 
>> <andrey.grodzovsky@amd.com>
>
> On second look, this comment is addressing the change within Dave's
> patch, on which this series apply. I was trying to justify all the 
> changes made, including the ones already done by Dave. See here:
>
> https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next-amd-dc-staging&id=e7b8e99bed73e9c42f1c074ad6009cb59a79bd52 
>
>
> I think changing "List of affected functions" to "The following 
> functions were considered" would make it less confusing, and will
> still make sense if it gets squashed with Dave's patch.
>
> Leo

Makes sense to me to.

Thanks,
Andrey

>
>>>
>>>>>
>>>>> amdgpu_dm_commit_planes: use for_each_old
>>>>>      - Called after the state was swapped (via atomic commit tail)
>>>>>
>>>>> amdgpu_dm_atomic_commit: use for_each_new
>>>>>      - Called before the state is swapped
>>>>>
>>>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>>>      - Called after the state was swapped
>>>>>
>>>>> dm_update_crtcs_state: use for_each_new
>>>>>      - Called before the state is swapped (via atomic check)
>>>>>
>>>>> amdgpu_dm_atomic_check: use for_each_new
>>>>>      - Called before the state is swapped
>>>>>
>>>>> v2: Split out typo fixes to a new patch.
>>>>>
>>>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>>>> ++++++++---------------
>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index 9bfe1f9..cc024ab 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>>>   struct amdgpu_dm_connector 
>>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>>       struct drm_atomic_state *state,
>>>>> -    struct drm_crtc *crtc,
>>>>> -    bool from_state_var)
>>>>> +    struct drm_crtc *crtc)
>>>>>   {
>>>>>       uint32_t i;
>>>>>       struct drm_connector_state *conn_state;
>>>>>       struct drm_connector *connector;
>>>>>       struct drm_crtc *crtc_from_state;
>>>>> -    for_each_new_connector_in_state(
>>>>> -        state,
>>>>> -        connector,
>>>>> -        conn_state,
>>>>> -        i) {
>>>>> -        crtc_from_state =
>>>>> -            from_state_var ?
>>>>> -                conn_state->crtc :
>>>>> -                connector->state->crtc;
>>>>> +    for_each_new_connector_in_state(state, connector, conn_state, 
>>>>> i) {
>>>>> +        crtc_from_state = conn_state->crtc;
>>>>>           if (crtc_from_state == crtc)
>>>>>               return to_amdgpu_dm_connector(connector);
>>>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>>>> drm_atomic_state *state,
>>>>>       unsigned long flags;
>>>>>       /* update planes when needed */
>>>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>>>           struct drm_plane_state *plane_state = plane->state;
>>>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>>>           struct drm_framebuffer *fb = plane_state->fb;
>>>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       dm_state = to_dm_atomic_state(state);
>>>>>       /* update changed items */
>>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>           struct drm_crtc_state *new_state = crtc->state;
>>>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>               new_acrtc_state = 
>>>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>>>               new_stream = new_acrtc_state->stream;
>>>>> -            aconnector =
>>>>> - amdgpu_dm_find_first_crct_matching_connector(
>>>>> +            aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(
>>>>>                       state,
>>>>> -                    &new_crtcs[i]->base,
>>>>> -                    false);
>>>>> +                    &new_crtcs[i]->base);
>>>>>               if (!aconnector) {
>>>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>>>> connector for acrtc id:%d "
>>>>>                        "skipping freesync init\n",
>>>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       }
>>>>>       /* Handle scaling and undersacn changes*/
>>>>> -    for_each_new_connector_in_state(state, connector, 
>>>>> old_conn_state, i) {
>>>>> +    for_each_old_connector_in_state(state, connector, 
>>>>> old_conn_state, i) {
>>>>>           struct amdgpu_dm_connector *aconnector = 
>>>>> to_amdgpu_dm_connector(connector);
>>>>>           struct dm_connector_state *con_new_state =
>>>>> to_dm_connector_state(aconnector->base.state);
>>>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       }
>>>>>       /* update planes when needed per crtc*/
>>>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>>>           if (new_acrtc_state->stream)
>>>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>>>        */
>>>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>           if (acrtc->base.state->event)
>>>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>>>           acrtc = to_amdgpu_crtc(crtc);
>>>>> -        aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>>>> +        aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>>>           /* TODO This hack should go away */
>>>>>           if (aconnector) {
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> index 630e6cd..1c55a0b 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>>>   struct amdgpu_dm_connector 
>>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>>       struct drm_atomic_state *state,
>>>>> -    struct drm_crtc *crtc,
>>>>> -    bool from_state_var);
>>>>> +    struct drm_crtc *crtc);
>>>>>   struct amdgpu_framebuffer;
>>>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>>>> mailing list amd-gfx@lists.freedesktop.org 
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>>
>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] Use new DRM API where possible, and cleanups.

[Maarten Lankhorst]

Op 12-10-17 om 23:15 schreef sunpeng.li@amd.com:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>
> Hi Dave,
>
> This series reworks the previous patch. Patch 1 is a v2 of the previous,
> and additional patches are from the feedback received. They apply on top
> of your drm-next-amd-dc-staging branch.
>
> Thanks,
> Leo
>
> Leo (Sunpeng) Li (6):
>   drm/amd/display: Use DRM new-style object iterators.
>   drm/amd/display: Use new DRM API where possible
>   drm/amd/display: Unify DRM state variable namings.
>   drm/amd/display: Unify amdgpu_dm state variable namings.
>   drm/amd/display: Fix typo
>   drm/amd/display: Remove useless pcrtc pointer
>
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 320 +++++++++++-----------
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |   3 +-
>  2 files changed, 156 insertions(+), 167 deletions(-)
>
Better, only scanned through the series but

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/6] drm/amd/display: Use new DRM API where possible

[Harry Wentland]

On 2017-10-13 01:26 PM, Andrey Grodzovsky wrote:
> 
> 
> On 10/13/2017 12:18 PM, Harry Wentland wrote:
>> On 2017-10-12 05:15 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> To conform to DRM's new API, we should not be accessing a DRM object's
>>> internal state directly. Rather, the DRM for_each_old/new_* iterators,
>>> and drm_atomic_get_old/new_* interface should be used.
>>>
>>> This is an ongoing process. For now, update the DRM-facing atomic
>>> functions, where the atomic state object is given.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 131 +++++++++++-----------
>>>   1 file changed, 66 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index cc024ab..d4426b3 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -3873,28 +3873,31 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>   {
>>>       uint32_t i;
>>>       struct drm_plane *plane;
>>> -    struct drm_plane_state *old_plane_state;
>>> +    struct drm_plane_state *old_plane_state, *new_plane_state;
>>>       struct dc_stream_state *dc_stream_attach;
>>>       struct dc_plane_state *plane_states_constructed[MAX_SURFACES];
>>>       struct amdgpu_crtc *acrtc_attach = to_amdgpu_crtc(pcrtc);
>>> -    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(pcrtc->state);
>>> +    struct drm_crtc_state *new_pcrtc_state =
>>> +            drm_atomic_get_new_crtc_state(state, pcrtc);
>>> +    struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
>>>       int planes_count = 0;
>>>       unsigned long flags;
>>>         /* update planes when needed */
>>> -    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>> -        struct drm_plane_state *plane_state = plane->state;
>>> -        struct drm_crtc *crtc = plane_state->crtc;
>>> -        struct drm_framebuffer *fb = plane_state->fb;
>>> +    for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>>> +        struct drm_crtc *crtc = new_plane_state->crtc;
>>> +        struct drm_crtc_state *new_crtc_state =
>>> +                drm_atomic_get_new_crtc_state(state, crtc);
>>> +        struct drm_framebuffer *fb = new_plane_state->fb;
>>>           bool pflip_needed;
>>> -        struct dm_plane_state *dm_plane_state = to_dm_plane_state(plane_state);
>>> +        struct dm_plane_state *dm_plane_state = to_dm_plane_state(new_plane_state);
>>>             if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>>>               handle_cursor_update(plane, old_plane_state);
>>>               continue;
>>>           }
>>>   -        if (!fb || !crtc || pcrtc != crtc || !crtc->state->active)
>>> +        if (!fb || !crtc || pcrtc != crtc || !new_crtc_state->active)
>>>               continue;
>>>             pflip_needed = !state->allow_modeset;
>>> @@ -3918,13 +3921,13 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>               dc_stream_attach = acrtc_state->stream;
>>>               planes_count++;
>>>   -        } else if (crtc->state->planes_changed) {
>>> +        } else if (new_crtc_state->planes_changed) {
>>>               /* Assume even ONE crtc with immediate flip means
>>>                * entire can't wait for VBLANK
>>>                * TODO Check if it's correct
>>>                */
>>>               *wait_for_vblank =
>>> -                    pcrtc->state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
>>> +                    new_pcrtc_state->pageflip_flags & DRM_MODE_PAGE_FLIP_ASYNC ?
>>>                   false : true;
>>>                 /* TODO: Needs rework for multiplane flip */
>>> @@ -3942,7 +3945,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>>>       if (planes_count) {
>>>           unsigned long flags;
>>>   -        if (pcrtc->state->event) {
>>> +        if (new_pcrtc_state->event) {
>>>                 drm_crtc_vblank_get(pcrtc);
>>>   @@ -3968,7 +3971,7 @@ int amdgpu_dm_atomic_commit(
>>>           bool nonblock)
>>>   {
>>>       struct drm_crtc *crtc;
>>> -    struct drm_crtc_state *new_state;
>>> +    struct drm_crtc_state *old_crtc_state, *new_state;
>>>       struct amdgpu_device *adev = dev->dev_private;
>>>       int i;
>>>   @@ -3979,8 +3982,8 @@ int amdgpu_dm_atomic_commit(
>>>        * it will update crtc->dm_crtc_state->stream pointer which is used in
>>>        * the ISRs.
>>>        */
>>> -    for_each_new_crtc_in_state(state, crtc, new_state, i) {
>>> -        struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(crtc->state);
>>> +    for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_state, i) {
>>> +        struct dm_crtc_state *old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>             if (drm_atomic_crtc_needs_modeset(new_state) && old_acrtc_state->stream)
>>> @@ -4002,13 +4005,13 @@ void amdgpu_dm_atomic_commit_tail(
>>>       uint32_t i, j;
>>>       uint32_t new_crtcs_count = 0;
>>>       struct drm_crtc *crtc, *pcrtc;
>>> -    struct drm_crtc_state *old_crtc_state;
>>> +    struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>>       struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
>>>       struct dc_stream_state *new_stream = NULL;
>>>       unsigned long flags;
>>>       bool wait_for_vblank = true;
>>>       struct drm_connector *connector;
>>> -    struct drm_connector_state *old_conn_state;
>>> +    struct drm_connector_state *old_conn_state, *new_con_state;
>>>       struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>>>         drm_atomic_helper_update_legacy_modeset_state(dev, state);
>>> @@ -4016,11 +4019,10 @@ void amdgpu_dm_atomic_commit_tail(
>>>       dm_state = to_dm_atomic_state(state);
>>>         /* update changed items */
>>> -    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>> -        struct drm_crtc_state *new_state = crtc->state;
>>>   -        new_acrtc_state = to_dm_crtc_state(new_state);
>>> +        new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>           old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>>             DRM_DEBUG_DRIVER(
>>> @@ -4028,18 +4030,18 @@ void amdgpu_dm_atomic_commit_tail(
>>>               "planes_changed:%d, mode_changed:%d,active_changed:%d,"
>>>               "connectors_changed:%d\n",
>>>               acrtc->crtc_id,
>>> -            new_state->enable,
>>> -            new_state->active,
>>> -            new_state->planes_changed,
>>> -            new_state->mode_changed,
>>> -            new_state->active_changed,
>>> -            new_state->connectors_changed);
>>> +            new_crtc_state->enable,
>>> +            new_crtc_state->active,
>>> +            new_crtc_state->planes_changed,
>>> +            new_crtc_state->mode_changed,
>>> +            new_crtc_state->active_changed,
>>> +            new_crtc_state->connectors_changed);
>>>             /* handles headless hotplug case, updating new_state and
>>>            * aconnector as needed
>>>            */
>>>   -        if (modeset_required(new_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
>>> +        if (modeset_required(new_crtc_state, new_acrtc_state->stream, old_acrtc_state->stream)) {
>>>                 DRM_DEBUG_DRIVER("Atomic commit: SET crtc id %d: [%p]\n", acrtc->crtc_id, acrtc);
>>>   @@ -4082,10 +4084,11 @@ void amdgpu_dm_atomic_commit_tail(
>>>               new_crtcs[new_crtcs_count] = acrtc;
>>>               new_crtcs_count++;
>>>   +            new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>>               acrtc->enabled = true;
>>> -            acrtc->hw_mode = crtc->state->mode;
>>> -            crtc->hwmode = crtc->state->mode;
>>> -        } else if (modereset_required(new_state)) {
>>> +            acrtc->hw_mode = new_crtc_state->mode;
>>> +            crtc->hwmode = new_crtc_state->mode;
>>> +        } else if (modereset_required(new_crtc_state)) {
>>>               DRM_DEBUG_DRIVER("Atomic commit: RESET. crtc id %d:[%p]\n", acrtc->crtc_id, acrtc);
>>>                 /* i.e. reset mode */
>>> @@ -4102,7 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>           for (i = 0; i < new_crtcs_count; i++) {
>>>               struct amdgpu_dm_connector *aconnector = NULL;
>>>   -            new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>>> +            new_crtc_state = drm_atomic_get_new_crtc_state(state,
>>> +                    &new_crtcs[i]->base);
>>> +            new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>                 new_stream = new_acrtc_state->stream;
>>>               aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>> @@ -4123,11 +4128,10 @@ void amdgpu_dm_atomic_commit_tail(
>>>       if (dm_state->context)
>>>           WARN_ON(!dc_commit_state(dm->dc, dm_state->context));
>>>   -
>>> -    list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>> +    for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>   -        new_acrtc_state = to_dm_crtc_state(crtc->state);
>>> +        new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>             if (new_acrtc_state->stream != NULL) {
>>>               const struct dc_stream_status *status =
>>> @@ -4141,24 +4145,24 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>         /* Handle scaling and undersacn changes*/
>>> -    for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>>> -        struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>>> -        struct dm_connector_state *con_new_state =
>>> -                to_dm_connector_state(aconnector->base.state);
>>> -        struct dm_connector_state *con_old_state =
>>> -                to_dm_connector_state(old_conn_state);
>>> +    for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_con_state, i) {
>>> +        struct dm_connector_state *con_new_state = to_dm_connector_state(new_con_state);
>>> +        struct dm_connector_state *con_old_state = to_dm_connector_state(old_conn_state);
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>>>           struct dc_stream_status *status = NULL;
>>>   +        if (acrtc)
>>> +            new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
>>> +
>>>           /* Skip any modesets/resets */
>>> -        if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
>>> +        if (!acrtc || drm_atomic_crtc_needs_modeset(new_crtc_state))
>>>               continue;
>>>             /* Skip any thing not scale or underscan changes */
>>>           if (!is_scaling_state_different(con_new_state, con_old_state))
>>>               continue;
>>>   -        new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>> +        new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>             update_stream_scaling_settings(&con_new_state->base.crtc->mode,
>>>                   con_new_state, (struct dc_stream_state *)new_acrtc_state->stream);
>>> @@ -4185,7 +4189,8 @@ void amdgpu_dm_atomic_commit_tail(
>>>            */
>>>           struct amdgpu_crtc *acrtc = new_crtcs[i];
>>>   -        new_acrtc_state = to_dm_crtc_state(acrtc->base.state);
>>> +        new_crtc_state = drm_atomic_get_new_crtc_state(state, &acrtc->base);
>>> +        new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>             if (adev->dm.freesync_module)
>>>               mod_freesync_notify_mode_change(
>>> @@ -4195,8 +4200,8 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>         /* update planes when needed per crtc*/
>>> -    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>> -        new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>> +    for_each_new_crtc_in_state(state, pcrtc, new_crtc_state, j) {
>>> +        new_acrtc_state = to_dm_crtc_state(new_crtc_state);
>>>             if (new_acrtc_state->stream)
>>>               amdgpu_dm_commit_planes(state, dev, dm, pcrtc, &wait_for_vblank);
>>> @@ -4208,13 +4213,12 @@ void amdgpu_dm_atomic_commit_tail(
>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>        */
>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> -    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> -        struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>> +    for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>> Andrey might be able to comment on this better, but weren't we intentionally
>> trying to iterate the old crtc here to make sure we send any even on it?
>>
>> Harry
> 
> The states we are interested in are still the new states because that where the event we want
> to send resides (drm_send_event_locked(dev, &crtc->state->event->base), with the introduction of for_each_new*
> iterators we should use them to iterate the new states.
> 

I think this just looked odd because of the patches were structured but
the logic is sound.

Series is
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> Thanks,
> Andrey
> 
>>
>>>   -        if (acrtc->base.state->event)
>>> -            drm_send_event_locked(dev, &crtc->state->event->base);
>>> +        if (new_crtc_state->event)
>>> +            drm_send_event_locked(dev, &new_crtc_state->event->base);
>>>   -        acrtc->base.state->event = NULL;
>>> +        new_crtc_state->event = NULL;
>>>       }
>>>       spin_unlock_irqrestore(&adev->ddev->event_lock, flags);
>>>   @@ -4371,7 +4375,7 @@ static int dm_update_crtcs_state(
>>>           bool *lock_and_validation_needed)
>>>   {
>>>       struct drm_crtc *crtc;
>>> -    struct drm_crtc_state *crtc_state;
>>> +    struct drm_crtc_state *old_crtc_state, *crtc_state;
>>>       int i;
>>>       struct dm_crtc_state *old_acrtc_state, *new_acrtc_state;
>>>       struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>>> @@ -4380,7 +4384,7 @@ static int dm_update_crtcs_state(
>>>         /*TODO Move this code into dm_crtc_atomic_check once we get rid of dc_validation_set */
>>>       /* update changed items */
>>> -    for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> +    for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = NULL;
>>>           struct amdgpu_dm_connector *aconnector = NULL;
>>>           struct drm_connector_state *conn_state = NULL;
>>> @@ -4388,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>             new_stream = NULL;
>>>   -        old_acrtc_state = to_dm_crtc_state(crtc->state);
>>> +        old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>           acrtc = to_amdgpu_crtc(crtc);
>>>   @@ -4521,7 +4525,7 @@ static int dm_update_planes_state(
>>>           bool *lock_and_validation_needed)
>>>   {
>>>       struct drm_crtc *new_plane_crtc, *old_plane_crtc;
>>> -    struct drm_crtc_state *new_crtc_state;
>>> +    struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>>       struct drm_plane *plane;
>>>       struct drm_plane_state *old_plane_state, *new_plane_state;
>>>       struct dm_crtc_state *new_acrtc_state, *old_acrtc_state;
>>> @@ -4552,10 +4556,9 @@ static int dm_update_planes_state(
>>>               if (!old_plane_crtc)
>>>                   continue;
>>>   -            old_acrtc_state = to_dm_crtc_state(
>>> -                    drm_atomic_get_old_crtc_state(
>>> -                            state,
>>> -                            old_plane_crtc));
>>> +            old_crtc_state = drm_atomic_get_old_crtc_state(
>>> +                    state, old_plane_crtc);
>>> +            old_acrtc_state = to_dm_crtc_state(old_crtc_state);
>>>                 if (!old_acrtc_state->stream)
>>>                   continue;
>>> @@ -4643,7 +4646,7 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>       struct dc *dc = adev->dm.dc;
>>>       struct dm_atomic_state *dm_state = to_dm_atomic_state(state);
>>>       struct drm_connector *connector;
>>> -    struct drm_connector_state *conn_state;
>>> +    struct drm_connector_state *old_con_state, *conn_state;
>>>       struct drm_crtc *crtc;
>>>       struct drm_crtc_state *crtc_state;
>>>   @@ -4710,16 +4713,14 @@ int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>        * new stream into context w\o causing full reset. Need to
>>>        * decide how to handle.
>>>        */
>>> -    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>> -        struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>>> -        struct dm_connector_state *con_old_state =
>>> -                to_dm_connector_state(aconnector->base.state);
>>> -        struct dm_connector_state *con_new_state =
>>> -                        to_dm_connector_state(conn_state);
>>> +    for_each_oldnew_connector_in_state(state, connector, old_con_state, conn_state, i) {
>>> +        struct dm_connector_state *con_old_state = to_dm_connector_state(old_con_state);
>>> +        struct dm_connector_state *con_new_state = to_dm_connector_state(conn_state);
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(con_new_state->base.crtc);
>>>             /* Skip any modesets/resets */
>>> -        if (!acrtc || drm_atomic_crtc_needs_modeset(acrtc->base.state))
>>> +        if (!acrtc || drm_atomic_crtc_needs_modeset(
>>> +                drm_atomic_get_new_crtc_state(state, &acrtc->base)))
>>>               continue;
>>>             /* Skip any thing not scale or underscan changes */
>>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 1/6] drm/amd/display: Use DRM new-style object iterators.

[sunpeng.li]

From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Use the correct for_each_new/old_* iterators instead of for_each_*

The following functions were considered:

amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
    - Old from_state_var flag was always choosing the new state

amdgpu_dm_display_resume: use for_each_new
    - drm_atomic_helper_duplicate_state is called during suspend to
      cache the state
    - It sets 'state' within the state triplet to 'new_state'

amdgpu_dm_commit_planes: use for_each_old
    - Called after the state was swapped (via atomic commit tail)

amdgpu_dm_atomic_commit: use for_each_new
    - Called before the state is swapped

amdgpu_dm_atomic_commit_tail: use for_each_old
    - Called after the state was swapped

dm_update_crtcs_state: use for_each_new
    - Called before the state is swapped (via atomic check)

amdgpu_dm_atomic_check: use for_each_new
    - Called before the state is swapped

v2: Split out typo fixes to a new patch.

v3: Say "functions considered" instead of "affected functions". The
    latter implies that changes are made to each.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 9bfe1f9..cc024ab 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var)
+	struct drm_crtc *crtc)
 {
 	uint32_t i;
 	struct drm_connector_state *conn_state;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc_from_state;
 
-	for_each_new_connector_in_state(
-		state,
-		connector,
-		conn_state,
-		i) {
-		crtc_from_state =
-			from_state_var ?
-				conn_state->crtc :
-				connector->state->crtc;
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		crtc_from_state = conn_state->crtc;
 
 		if (crtc_from_state == crtc)
 			return to_amdgpu_dm_connector(connector);
@@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 
 	/* update planes when needed */
-	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		struct drm_plane_state *plane_state = plane->state;
 		struct drm_crtc *crtc = plane_state->crtc;
 		struct drm_framebuffer *fb = plane_state->fb;
@@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
 	dm_state = to_dm_atomic_state(state);
 
 	/* update changed items */
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 		struct drm_crtc_state *new_state = crtc->state;
 
@@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
 			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
 
 			new_stream = new_acrtc_state->stream;
-			aconnector =
-				amdgpu_dm_find_first_crct_matching_connector(
+			aconnector = amdgpu_dm_find_first_crct_matching_connector(
 					state,
-					&new_crtcs[i]->base,
-					false);
+					&new_crtcs[i]->base);
 			if (!aconnector) {
 				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
 					 "skipping freesync init\n",
@@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* Handle scaling and undersacn changes*/
-	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
+	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
 		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 		struct dm_connector_state *con_new_state =
 				to_dm_connector_state(aconnector->base.state);
@@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* update planes when needed per crtc*/
-	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
+	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
 		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
 
 		if (new_acrtc_state->stream)
@@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
 	 * mark consumed event for drm_atomic_helper_commit_hw_done
 	 */
 	spin_lock_irqsave(&adev->ddev->event_lock, flags);
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
 		if (acrtc->base.state->event)
@@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
 		new_acrtc_state = to_dm_crtc_state(crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
-		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
+		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
 		if (aconnector) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 630e6cd..1c55a0b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var);
+	struct drm_crtc *crtc);
 
 
 struct amdgpu_framebuffer;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/amd/display: Use DRM new-style object iterators.

[Harry Wentland]

On 2017-10-13 03:29 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
> 
> Use the correct for_each_new/old_* iterators instead of for_each_*
> 
> The following functions were considered:
> 
> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>     - Old from_state_var flag was always choosing the new state
> 
> amdgpu_dm_display_resume: use for_each_new
>     - drm_atomic_helper_duplicate_state is called during suspend to
>       cache the state
>     - It sets 'state' within the state triplet to 'new_state'
> 
> amdgpu_dm_commit_planes: use for_each_old
>     - Called after the state was swapped (via atomic commit tail)
> 
> amdgpu_dm_atomic_commit: use for_each_new
>     - Called before the state is swapped
> 
> amdgpu_dm_atomic_commit_tail: use for_each_old
>     - Called after the state was swapped
> 
> dm_update_crtcs_state: use for_each_new
>     - Called before the state is swapped (via atomic check)
> 
> amdgpu_dm_atomic_check: use for_each_new
>     - Called before the state is swapped
> 
> v2: Split out typo fixes to a new patch.
> 
> v3: Say "functions considered" instead of "affected functions". The
>     latter implies that changes are made to each.
> 
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>

Patches 1-2 (v3) are also
Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>  2 files changed, 12 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9bfe1f9..cc024ab 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>  
>  struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>  	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var)
> +	struct drm_crtc *crtc)
>  {
>  	uint32_t i;
>  	struct drm_connector_state *conn_state;
>  	struct drm_connector *connector;
>  	struct drm_crtc *crtc_from_state;
>  
> -	for_each_new_connector_in_state(
> -		state,
> -		connector,
> -		conn_state,
> -		i) {
> -		crtc_from_state =
> -			from_state_var ?
> -				conn_state->crtc :
> -				connector->state->crtc;
> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> +		crtc_from_state = conn_state->crtc;
>  
>  		if (crtc_from_state == crtc)
>  			return to_amdgpu_dm_connector(connector);
> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>  	unsigned long flags;
>  
>  	/* update planes when needed */
> -	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
> +	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>  		struct drm_plane_state *plane_state = plane->state;
>  		struct drm_crtc *crtc = plane_state->crtc;
>  		struct drm_framebuffer *fb = plane_state->fb;
> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>  	dm_state = to_dm_atomic_state(state);
>  
>  	/* update changed items */
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  		struct drm_crtc_state *new_state = crtc->state;
>  
> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>  			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>  
>  			new_stream = new_acrtc_state->stream;
> -			aconnector =
> -				amdgpu_dm_find_first_crct_matching_connector(
> +			aconnector = amdgpu_dm_find_first_crct_matching_connector(
>  					state,
> -					&new_crtcs[i]->base,
> -					false);
> +					&new_crtcs[i]->base);
>  			if (!aconnector) {
>  				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
>  					 "skipping freesync init\n",
> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>  	}
>  
>  	/* Handle scaling and undersacn changes*/
> -	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
> +	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>  		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>  		struct dm_connector_state *con_new_state =
>  				to_dm_connector_state(aconnector->base.state);
> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>  	}
>  
>  	/* update planes when needed per crtc*/
> -	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
> +	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>  		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>  
>  		if (new_acrtc_state->stream)
> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>  	 * mark consumed event for drm_atomic_helper_commit_hw_done
>  	 */
>  	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>  		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  
>  		if (acrtc->base.state->event)
> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>  		new_acrtc_state = to_dm_crtc_state(crtc_state);
>  		acrtc = to_amdgpu_crtc(crtc);
>  
> -		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
> +		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>  
>  		/* TODO This hack should go away */
>  		if (aconnector) {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 630e6cd..1c55a0b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>  
>  struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>  	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var);
> +	struct drm_crtc *crtc);
>  
>  
>  struct amdgpu_framebuffer;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 1/6] drm/amd/display: Use DRM new-style object iterators.

[Andrey Grodzovsky]

On 10/13/2017 03:29 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>
> Use the correct for_each_new/old_* iterators instead of for_each_*
>
> The following functions were considered:
>
> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>      - Old from_state_var flag was always choosing the new state
>
> amdgpu_dm_display_resume: use for_each_new
>      - drm_atomic_helper_duplicate_state is called during suspend to
>        cache the state
>      - It sets 'state' within the state triplet to 'new_state'
>
> amdgpu_dm_commit_planes: use for_each_old
>      - Called after the state was swapped (via atomic commit tail)
>
> amdgpu_dm_atomic_commit: use for_each_new
>      - Called before the state is swapped
>
> amdgpu_dm_atomic_commit_tail: use for_each_old
>      - Called after the state was swapped
>
> dm_update_crtcs_state: use for_each_new
>      - Called before the state is swapped (via atomic check)
>
> amdgpu_dm_atomic_check: use for_each_new
>      - Called before the state is swapped
>
> v2: Split out typo fixes to a new patch.
>
> v3: Say "functions considered" instead of "affected functions". The
>      latter implies that changes are made to each.
>
> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>   2 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9bfe1f9..cc024ab 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var)
> +	struct drm_crtc *crtc)
>   {
>   	uint32_t i;
>   	struct drm_connector_state *conn_state;
>   	struct drm_connector *connector;
>   	struct drm_crtc *crtc_from_state;
>   
> -	for_each_new_connector_in_state(
> -		state,
> -		connector,
> -		conn_state,
> -		i) {
> -		crtc_from_state =
> -			from_state_var ?
> -				conn_state->crtc :
> -				connector->state->crtc;
> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> +		crtc_from_state = conn_state->crtc;
>   
>   		if (crtc_from_state == crtc)
>   			return to_amdgpu_dm_connector(connector);
> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   	unsigned long flags;
>   
>   	/* update planes when needed */
> -	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
> +	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>   		struct drm_plane_state *plane_state = plane->state;
>   		struct drm_crtc *crtc = plane_state->crtc;
>   		struct drm_framebuffer *fb = plane_state->fb;
> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	dm_state = to_dm_atomic_state(state);
>   
>   	/* update changed items */
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {

Better just use for_each_new_old here, so you can get both old and new 
from the iterator.

>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   		struct drm_crtc_state *new_state = crtc->state;
>   
> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>   			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>   
>   			new_stream = new_acrtc_state->stream;
> -			aconnector =
> -				amdgpu_dm_find_first_crct_matching_connector(
> +			aconnector = amdgpu_dm_find_first_crct_matching_connector(
>   					state,
> -					&new_crtcs[i]->base,
> -					false);
> +					&new_crtcs[i]->base);
>   			if (!aconnector) {
>   				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
>   					 "skipping freesync init\n",
> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* Handle scaling and undersacn changes*/
> -	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
> +	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>   		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>   		struct dm_connector_state *con_new_state =
>   				to_dm_connector_state(aconnector->base.state);
> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* update planes when needed per crtc*/
> -	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
> +	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>   		new_acrtc_state = to_dm_crtc_state(pcrtc->state);

Why did you switch to for_each_old iterator here ? if you use the 
for_each_new
iterator you can get rid of new_acrtc_state = 
to_dm_crtc_state(pcrtc->state);
and just use the iterator's state
>   
>   		if (new_acrtc_state->stream)
> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	 * mark consumed event for drm_atomic_helper_commit_hw_done
>   	 */
>   	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);

Same as above

Thanks,
Andrey

>   
>   		if (acrtc->base.state->event)
> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>   		new_acrtc_state = to_dm_crtc_state(crtc_state);
>   		acrtc = to_amdgpu_crtc(crtc);
>   
> -		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
> +		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>   
>   		/* TODO This hack should go away */
>   		if (aconnector) {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 630e6cd..1c55a0b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var);
> +	struct drm_crtc *crtc);
>   
>   
>   struct amdgpu_framebuffer;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/amd/display: Use DRM new-style object iterators.

[Leo]

On 2017-10-13 04:36 PM, Andrey Grodzovsky wrote:
> 
> 
> On 10/13/2017 03:29 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>
>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>
>> The following functions were considered:
>>
>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>      - Old from_state_var flag was always choosing the new state
>>
>> amdgpu_dm_display_resume: use for_each_new
>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>        cache the state
>>      - It sets 'state' within the state triplet to 'new_state'
>>
>> amdgpu_dm_commit_planes: use for_each_old
>>      - Called after the state was swapped (via atomic commit tail)
>>
>> amdgpu_dm_atomic_commit: use for_each_new
>>      - Called before the state is swapped
>>
>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>      - Called after the state was swapped
>>
>> dm_update_crtcs_state: use for_each_new
>>      - Called before the state is swapped (via atomic check)
>>
>> amdgpu_dm_atomic_check: use for_each_new
>>      - Called before the state is swapped
>>
>> v2: Split out typo fixes to a new patch.
>>
>> v3: Say "functions considered" instead of "affected functions". The
>>      latter implies that changes are made to each.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>> ++++++++---------------
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 9bfe1f9..cc024ab 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var)
>> +    struct drm_crtc *crtc)
>>   {
>>       uint32_t i;
>>       struct drm_connector_state *conn_state;
>>       struct drm_connector *connector;
>>       struct drm_crtc *crtc_from_state;
>> -    for_each_new_connector_in_state(
>> -        state,
>> -        connector,
>> -        conn_state,
>> -        i) {
>> -        crtc_from_state =
>> -            from_state_var ?
>> -                conn_state->crtc :
>> -                connector->state->crtc;
>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>> +        crtc_from_state = conn_state->crtc;
>>           if (crtc_from_state == crtc)
>>               return to_amdgpu_dm_connector(connector);
>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>> drm_atomic_state *state,
>>       unsigned long flags;
>>       /* update planes when needed */
>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>           struct drm_plane_state *plane_state = plane->state;
>>           struct drm_crtc *crtc = plane_state->crtc;
>>           struct drm_framebuffer *fb = plane_state->fb;
>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       dm_state = to_dm_atomic_state(state);
>>       /* update changed items */
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> 
> Better just use for_each_new_old here, so you can get both old and new 
> from the iterator.
> 
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>           struct drm_crtc_state *new_state = crtc->state;
>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>               new_acrtc_state = 
>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>               new_stream = new_acrtc_state->stream;
>> -            aconnector =
>> -                amdgpu_dm_find_first_crct_matching_connector(
>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>                       state,
>> -                    &new_crtcs[i]->base,
>> -                    false);
>> +                    &new_crtcs[i]->base);
>>               if (!aconnector) {
>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>> connector for acrtc id:%d "
>>                        "skipping freesync init\n",
>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* Handle scaling and undersacn changes*/
>> -    for_each_new_connector_in_state(state, connector, old_conn_state, 
>> i) {
>> +    for_each_old_connector_in_state(state, connector, old_conn_state, 
>> i) {
>>           struct amdgpu_dm_connector *aconnector = 
>> to_amdgpu_dm_connector(connector);
>>           struct dm_connector_state *con_new_state =
>>                   to_dm_connector_state(aconnector->base.state);
>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* update planes when needed per crtc*/
>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
> 
> Why did you switch to for_each_old iterator here ? if you use the 
> for_each_new
> iterator you can get rid of new_acrtc_state = 
> to_dm_crtc_state(pcrtc->state);
> and just use the iterator's state
>>           if (new_acrtc_state->stream)
>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>        */
>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> 
> Same as above
> 
> Thanks,
> Andrey
> 

Patches 1/6 and 2/6 should really be squashed, sorry for the confusion.
What you've pointed out is addressed in 2/6 :)

Leo

>>           if (acrtc->base.state->event)
>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>           acrtc = to_amdgpu_crtc(crtc);
>> -        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>> +        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>           /* TODO This hack should go away */
>>           if (aconnector) {
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 630e6cd..1c55a0b 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var);
>> +    struct drm_crtc *crtc);
>>   struct amdgpu_framebuffer;
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3 1/6] drm/amd/display: Use DRM new-style object iterators.

[Andrey Grodzovsky]

On 10/13/2017 05:01 PM, Leo wrote:
>
>
> On 2017-10-13 04:36 PM, Andrey Grodzovsky wrote:
>>
>>
>> On 10/13/2017 03:29 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>
>>>
>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>
>>> The following functions were considered:
>>>
>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>      - Old from_state_var flag was always choosing the new state
>>>
>>> amdgpu_dm_display_resume: use for_each_new
>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>        cache the state
>>>      - It sets 'state' within the state triplet to 'new_state'
>>>
>>> amdgpu_dm_commit_planes: use for_each_old
>>>      - Called after the state was swapped (via atomic commit tail)
>>>
>>> amdgpu_dm_atomic_commit: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>      - Called after the state was swapped
>>>
>>> dm_update_crtcs_state: use for_each_new
>>>      - Called before the state is swapped (via atomic check)
>>>
>>> amdgpu_dm_atomic_check: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> v2: Split out typo fixes to a new patch.
>>>
>>> v3: Say "functions considered" instead of "affected functions". The
>>>      latter implies that changes are made to each.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>> ++++++++---------------
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 9bfe1f9..cc024ab 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var)
>>> +    struct drm_crtc *crtc)
>>>   {
>>>       uint32_t i;
>>>       struct drm_connector_state *conn_state;
>>>       struct drm_connector *connector;
>>>       struct drm_crtc *crtc_from_state;
>>> -    for_each_new_connector_in_state(
>>> -        state,
>>> -        connector,
>>> -        conn_state,
>>> -        i) {
>>> -        crtc_from_state =
>>> -            from_state_var ?
>>> -                conn_state->crtc :
>>> -                connector->state->crtc;
>>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>> +        crtc_from_state = conn_state->crtc;
>>>           if (crtc_from_state == crtc)
>>>               return to_amdgpu_dm_connector(connector);
>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>       unsigned long flags;
>>>       /* update planes when needed */
>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>           struct drm_plane_state *plane_state = plane->state;
>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>           struct drm_framebuffer *fb = plane_state->fb;
>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       dm_state = to_dm_atomic_state(state);
>>>       /* update changed items */
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>
>> Better just use for_each_new_old here, so you can get both old and 
>> new from the iterator.
>>
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>           struct drm_crtc_state *new_state = crtc->state;
>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>               new_acrtc_state = 
>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>               new_stream = new_acrtc_state->stream;
>>> -            aconnector =
>>> -                amdgpu_dm_find_first_crct_matching_connector(
>>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>>                       state,
>>> -                    &new_crtcs[i]->base,
>>> -                    false);
>>> +                    &new_crtcs[i]->base);
>>>               if (!aconnector) {
>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>> connector for acrtc id:%d "
>>>                        "skipping freesync init\n",
>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* Handle scaling and undersacn changes*/
>>> -    for_each_new_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>> +    for_each_old_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>>           struct amdgpu_dm_connector *aconnector = 
>>> to_amdgpu_dm_connector(connector);
>>>           struct dm_connector_state *con_new_state =
>>> to_dm_connector_state(aconnector->base.state);
>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* update planes when needed per crtc*/
>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>
>> Why did you switch to for_each_old iterator here ? if you use the 
>> for_each_new
>> iterator you can get rid of new_acrtc_state = 
>> to_dm_crtc_state(pcrtc->state);
>> and just use the iterator's state
>>>           if (new_acrtc_state->stream)
>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>        */
>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>
>> Same as above
>>
>> Thanks,
>> Andrey
>>
>
> Patches 1/6 and 2/6 should really be squashed, sorry for the confusion.
> What you've pointed out is addressed in 2/6 :)
>
> Leo

Oh, I see.

Thanks,
Andrey

>
>>>           if (acrtc->base.state->event)
>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>           acrtc = to_amdgpu_crtc(crtc);
>>> -        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>> +        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>           /* TODO This hack should go away */
>>>           if (aconnector) {
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> index 630e6cd..1c55a0b 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var);
>>> +    struct drm_crtc *crtc);
>>>   struct amdgpu_framebuffer;
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel