Re: [PATCH v3 3/7] drm/amd/display: don't check colorop status if its in an inactive pipeline

[Melissa Wen]

On 26/06/2026 00:52, John Harrison wrote:
On 6/9/26 13:51, Melissa Wen wrote:
If colorop BYPASS property is true, but the colorop isn't part of an
Should this say 'is false'?

Oops, you are right. I'll fix it in the next version.


John.

active/transient active color pipeline, this colorop status should not
be taken into account when checking if a plane color pipeline is
actually active. For example, if the userspace doesn't explicitly set a
colorop obj to bypass but deactivates its color pipeline by setting
plane COLOR_PIPELINE to bypass, it means that colorop is inactive
regardless of its BYPASS property status.

Reported-by: Sashiko <sashiko-...@kernel.org>
Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color 
pipeline is active")
Signed-off-by: Melissa Wen <m...@igalia.com>
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 +++++++++++++------
  1 file changed, 21 insertions(+), 10 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 ba7f98a87808..2edec3e1b838 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -12590,9 +12590,9 @@ static int add_affected_mst_dsc_crtcs(struct 
drm_atomic_commit *state, struct dr
   * @use_old: if true, inspect the old colorop states; otherwise the 
new ones
   *
   * A color pipeline may be selected (color_pipeline != NULL) but 
still is
- * inactive if every colorop in the chain is bypassed.  Only return
- * true when at least one colorop has bypass == false, meaning the 
cursor
- * would be subjected to the transformation in native mode.
+ * inactive if every colorop in the chain is bypassed. Only return 
true when at
+ * least one active colorop has bypass == false, meaning the cursor 
would be
+ * subjected to the transformation in native mode.
   *
   * Return: true if the pipeline modifies pixels, false otherwise.
   */
@@ -12600,18 +12600,29 @@ static bool 
dm_plane_color_pipeline_active(struct drm_atomic_commit *state,
                         struct drm_plane *plane,
                         bool use_old)
  {
-    struct drm_colorop *colorop;
-    struct drm_colorop_state *old_colorop_state, *new_colorop_state;
-    int i;
+    struct drm_plane_state *plane_state = use_old ?
+                          drm_atomic_get_old_plane_state(state, 
plane) :
+                          drm_atomic_get_new_plane_state(state, plane);
+    struct drm_colorop *colorop, *pipeline;
+    struct drm_colorop_state *cstate;
  -    for_each_oldnew_colorop_in_state(state, colorop, 
old_colorop_state, new_colorop_state, i) {
-        struct drm_colorop_state *cstate = use_old ? 
old_colorop_state : new_colorop_state;
+    pipeline = plane_state ? plane_state->color_pipeline :
+                 plane->state->color_pipeline;
Why would plane_state be null? And if it is, why is it correct to use 
plane->state rather than the old or new state as requested by the 
use_old flag? Seems like there should be a comment to explain this.

Right, about the plane_state, it was a defensive approach for the case 
that we have a colorop change but no changes on plane that make its 
state part of an atomic commit and in this case, there is no difference 
between old/new state, it's just the committed state.
However following the callers of this function, currently plane_state is 
never NULL, so now I think a WARN_ON is enough, instead of handling an 
unreachable case.


  -        if (cstate->colorop->plane != plane)
-            continue;
+    if (!pipeline)
+        return false;
+
+    drm_for_each_colorop_in_pipeline(colorop, pipeline) {
+        cstate = use_old ?
+             drm_atomic_get_old_colorop_state(state, colorop) :
+             drm_atomic_get_new_colorop_state(state, colorop);
+
+        if (!cstate)
+            cstate = colorop->state;
Same question as above. Why would there not be a old/new state and if 
there isn't, why is it correct to use the current state when a check 
against the old/new state was explicitly requested?

Here is a bit different, there is a situation in which just one or two 
colorops of a given color pipeline changes, but others are not touched. 
In this case only colorops that changed will have a old/new states, but 
others only have the committed state for validation.
To state that a given color pipeline can modify pixels or not, we also 
need to check the commited/current state of all colorop in the selected 
colorop pipeline, not only those that were changed in this atomic commit.
I'll add a comment explaining it better.

Thanks,

Melissa


John.

          if (!cstate->bypass)
              return true;
      }
+
      return false;
  }