Re: [i-g-t] tests/kms_colorop: Use local variable for color pipeline cap

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

On 9/1/2026 10:24 PM, Kamil Konieczny wrote:
> Hi Borah,,
> On 2026-08-28 at 15:34:27 +0530, Borah, Chaitanya Kumar wrote:
>>
>>
>> On 8/28/2026 5:45 AM, Alex Hung wrote:
>>>
>>>
>>> On 8/26/26 23:31, Sharma, Swati2 wrote:
>>>> Hi Chaitanya
>>>>
>>>> Patch LGTM
>>>>
>>>> Reviewed-by: Swati Sharma <swati2.sharma@intel.com>
>>>>
>>>> Also, it seems we have duplicate igt_display_require(&display,
>>>> display.drm_fd);
>>>>
>>>> It is required once. Anyways, its a separate patch.
>>>>
>>>> Alex,
>>>>
>>>> Can you please test on AMD driver with above change?
>>>
>>> yes it ran fine. Some inline comments below.
>>>
>>>>
>>>> On 03-08-2026 01:33 pm, Chaitanya Kumar Borah wrote:
>>>>> Following the pattern in kms_color_pipeline, set
>>>>> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE once and stash the result in a
>>>>> local variable, then propagate it to display->has_plane_color_pipeline
>>>>> after the final igt_display_require() (which reinitializes the display
>>>>> state). This avoids calling drmSetClientCap() twice.
>>>>>
>>>>> Cc: Swati Sharma <swati2.sharma@intel.com>
>>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>>> Cc: Alex Hung <alex.hung@amd.com>
>>>>> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>>>>> ---
>>>>>    tests/kms_colorop.c | 9 +++++----
>>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
>>>>> index 16db0b8ad..d573d2ceb 100644
>>>>> --- a/tests/kms_colorop.c
>>>>> +++ b/tests/kms_colorop.c
>>>>> @@ -406,6 +406,7 @@ int igt_main_args("d", long_options,
>>>>> help_str, opt_handler, NULL)
>>>>>        igt_display_t display;
>>>>>        int i, j, ret;
>>>>> +    int has_plane_color_pipeline = 0;
>>> can this be bool?
>>>
>>>>>        igt_fixture() {
>>>>>            display.drm_fd = drm_open_driver_master(DRIVER_ANY);
>>>>> @@ -417,15 +418,15 @@ int igt_main_args("d", long_options,
>>>>> help_str, opt_handler, NULL)
>>>>>            igt_require_f(!ret, "error setting
>>>>> DRM_CLIENT_CAP_WRITEBACK_CONNECTORS\n");
>>>>> -        igt_display_require(&display, display.drm_fd);
>>>>>            if (drmSetClientCap(display.drm_fd,
>>>>> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE, 1) == 0)
>>>>> -            display.has_plane_color_pipeline = 1;
>>>>> +            has_plane_color_pipeline = 1;
>>>>> +
>>>>> +        igt_display_require(&display, display.drm_fd);
>>> is this igt_display_require() necessary?
>>>
>>
>> Swati flagged this too and this is probably unnecessary. However, I thought
>> of handling it in a separate patch. What do you think?
> 
> Why waiting for next change when it is already spotted? Lets keep
> number of commits low, so git history will be shorter. imho you
> could fix it and add r-b you already got.
> 
To me, these look like two logically independent and bisectable changes.
But I can do a while-on-it and combine them together.

==
Chaitanya

> Regards,
> Kamil
> 
>>
>>>>>            kmstest_set_vt_graphics_mode();
>>>>>            igt_display_require(&display, display.drm_fd);
>>>>> -        if (drmSetClientCap(display.drm_fd,
>>>>> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE, 1) == 0)
>>>>> -            display.has_plane_color_pipeline = 1;
>>>>> +        display.has_plane_color_pipeline = has_plane_color_pipeline;
>>>>>            igt_require(display.is_atomic);
>>>>>        }
>>>
>>