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

[Alex Hung <alex.hung@amd.com>]

On 8/28/26 04:04, 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?

Sure. This is not a blocker

Reviewed-by: Alex Hung <alex.hung@amd.com>

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