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'?
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 <[email protected]>
Fixes: d3a549f4df78 ("drm/amd/display: Use overlay cursor when color pipeline is
active")
Signed-off-by: Melissa Wen <[email protected]>
---
.../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.
- 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?
John.
if (!cstate->bypass)
return true;
}
+
return false;
}
