Re: [PATCH 03/29] drm/amd/display: Complete TODO item: use new DRM iterator

[Leo]

On 2017-10-30 12:36 PM, Leo wrote:


On 2017-10-26 11:06 PM, Andrey Grodzovsky wrote:


On 2017-10-26 02:34 PM, Harry Wentland wrote:
From: "Leo (Sunpeng) Li" <sunpeng...@amd.com>

Abandon new_crtcs array and use for_each_new iterator to acquire new
crtcs.

Signed-off-by: Leo (Sunpeng) Li <sunpeng...@amd.com>
Reviewed-by: Harry Wentland <harry.wentl...@amd.com>
---
  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 
+++++++++--------------
  1 file changed, 14 insertions(+), 22 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 442b399a9400..590f80d29b56 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4013,10 +4013,8 @@ static void 
amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
      struct amdgpu_display_manager *dm = &adev->dm;
      struct dm_atomic_state *dm_state;
      uint32_t i, j;
-    uint32_t new_crtcs_count = 0;
      struct drm_crtc *crtc;
      struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-    struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
      unsigned long flags;
      bool wait_for_vblank = true;
      struct drm_connector *connector;
@@ -4075,25 +4073,9 @@ static void 
amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
                  continue;
              }
-
              if (dm_old_crtc_state->stream)
                  remove_stream(adev, acrtc, dm_old_crtc_state->stream);
-
-            /*
-             * this loop saves set mode crtcs
-             * we needed to enable vblanks once all
-             * resources acquired in dc after dc_commit_streams
-             */
-
-            /*TODO move all this into dm_crtc_state, get rid of
-             * new_crtcs array and use old and new atomic states
-             * instead
-             */
-            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 = new_crtc_state->mode;
              crtc->hwmode = new_crtc_state->mode;
@@ -4221,18 +4203,28 @@ static void 
amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
              dm_error("%s: Failed to update stream scaling!\n", 
__func__);
      }
-    for (i = 0; i < new_crtcs_count; i++) {
+    for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
+            new_crtc_state, i) {
          /*
           * loop to enable interrupts on newly arrived crtc
           */
-        struct amdgpu_crtc *acrtc = new_crtcs[i];
+        struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+        bool modeset_needed;
-        new_crtc_state = drm_atomic_get_new_crtc_state(state, 
&acrtc->base);
          dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+        dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
+        modeset_needed = modeset_required(
+                new_crtc_state,
+                dm_new_crtc_state->stream,
+                dm_old_crtc_state->stream);
+
+        if (dm_new_crtc_state->stream == NULL || !modeset_needed)
+            continue;

I feel it's a bit future bug prone to repeat the 2 checks above from 
the initial
for_each_crtc_in_state loop, somebody makes changes there and forget 
about this loop.
The array is ugly but it avoids logic duplication.

I think a better fix would be to combine this for_each section with the
one above it. The only thing keeping them separate (not shown in the
above context) is an `if (adev->dm.freesync_module)` guard, which can be
pulled into the first for_each loop. It would also eliminate a lot of
duplicated initialization code.

Thoughts?

Leo


Ignore what I said, It seems I was looking at the wrong context :) The 
fact this blob is being done after dc_commit_state() is annoying...

The maintainability of this duplicated logic is a good point. Although 
I'm not sure if going back to a shared array using custom iterator logic 
is the best approach. Perhapse we can pull this duplicated logic into a 
seperate function?

Thanks,
Leo




Thanks,
Andrey

          if (adev->dm.freesync_module)
              mod_freesync_notify_mode_change(
-                adev->dm.freesync_module, 
&dm_new_crtc_state->stream, 1);
+                adev->dm.freesync_module,
+                &dm_new_crtc_state->stream, 1);
          manage_dm_interrupts(adev, acrtc, true);
      }

_______________________________________________
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