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

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 <[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.

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;
>>   }
>