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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxx wrote:
From: "Leo (Sunpeng) Li"<sunpeng.li@xxxxxxx>

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@xxxxxxx>

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@xxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx




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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux