Re: [PATCH i-g-t v5 4/8] tests/kms_properties: give non-primary planes their own fb

[Melissa Wen <mwen@igalia.com>]

On 30/09/2026 18:06, Harry Wentland wrote:
>
>
> On 2026-09-02 13:58, Melissa Wen wrote:
>> On AMD drivers, a CRTC remains active only while its primary plane is
>> enabled; therefore, handing the primary's fb over to a non-primary plane
>> would take the CRTC down with it. Create a dedicated fb for each
>> non-primary plane before testing its colorops and discard it again
>> afterward, leaving the primary plane and its prepare_crtc() fb intact.
>> This is groundwork for the next commit, which checks colorop properties
>> on an active color pipeline.
>>
>
> This patch confuses me a bit. You mention that amdgpu needs an
> FB on primary planes for a crtc to be active, which makes sense.
> But this patch then only deals with non-primary planes. Does
> amdgpu have a need for (non-primary) planes to have an attached
> FB before allowing a COLOR_PIPELINE?

Sorry, reading it again I see I could have explained better what I'm 
actually changing.

The issue this commit wants to solve is: we can only test/change colorop 
properties of a given color pipeline if its color pipeline is active 
(set in COLOR PIPELINE plane prop) which also means its plane has to be 
active too.
In the original version, the primary plane is active, but other planes 
in a given CRTC isn't.

Before iterating over planes in a given CRTC, a frame buffer is already 
attached to the primary plane (by prepare_crtc), yes.
But then, when iterating over planes to check their color pipelines, if 
it's not a primary plane, we need to set a separate fb to activate it 
before setting the COLOR PIPELINE prop, keeping the primary plane active 
with their own fb.
I guess this confusing commit message came from my attempts to solve it 
by initially reusing the "prepare_crtc" fb on overlay planes to activate 
it >.<

I hope I was able to explain it better now. I'll rewrite the commit message.

Melissa


>
> Harry
>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>> ---
>>
>> v2:
>> - detach different changes from a single commit (Chaitanya)
>> v3:
>> - move hunk from next patch to fix mem leak (Alex H/Chaitanya)
>> ---
>>   tests/kms_properties.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_properties.c b/tests/kms_properties.c
>> index 2b4cb152b..c55a271da 100644
>> --- a/tests/kms_properties.c
>> +++ b/tests/kms_properties.c
>> @@ -237,7 +237,7 @@ static void 
>> run_colorop_property_tests(igt_display_t *display,
>>                          igt_crtc_t *crtc, igt_output_t *output,
>>                          bool atomic)
>>   {
>> -    struct igt_fb fb;
>> +    struct igt_fb fb, afb;
>>       igt_plane_t *plane;
>>       igt_colorop_t *colorop;
>>       int i;
>> @@ -255,6 +255,18 @@ static void 
>> run_colorop_property_tests(igt_display_t *display,
>>                igt_crtc_name(crtc), plane->index,
>>                kmstest_plane_type_name(plane->type), output->name);
>>   +        /* A non-primary plane needs an fb of its own: AMD keeps the
>> +         * CRTC active only while the primary plane is enabled.
>> +         */
>> +        if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> +            drmModeModeInfo *mode = igt_output_get_mode(output);
>> +
>> +            igt_create_pattern_fb(display->drm_fd, mode->hdisplay, 
>> mode->vdisplay,
>> +                          DRM_FORMAT_XRGB8888, 
>> DRM_FORMAT_MOD_LINEAR, &afb);
>> +
>> +            igt_plane_set_fb(plane, &afb);
>> +        }
>> +
>>           /* iterate over all color pipelines on plane */
>>           for (i = 0; i < plane->num_color_pipelines; ++i) {
>>               /* iterate over all colorops in pipeline*/
>> @@ -272,6 +284,12 @@ static void 
>> run_colorop_property_tests(igt_display_t *display,
>>                   colorop = igt_find_colorop(display, colorop_id);
>>               }
>>           }
>> +
>> +        /* only the fb created above needs to go away here */
>> +        if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
>> +            igt_plane_set_fb(plane, NULL);
>> +            igt_remove_fb(display->drm_fd, &afb);
>> +        }
>>       }
>>         cleanup_crtc(display, crtc, output,
>