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

[Harry Wentland <harry.wentland@amd.com>]

On 2026-09-30 15:37, Melissa Wen wrote:
> 
> 
> 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 >.<
> 

Thanks, that explains it and I was almost suspecting something like this.

With a rewritten commit message that makes it clear that the subsequent 
patch needs an FB for non-primary planes, in addition to the primary 
plane FB this patch is

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

Harry

> 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,
>>
>