Re: [PATCH i-g-t 1/2] Revert "lib/kms: Set display->has_plane_color_pipeline automatically"

On 7/22/2026 11:53 AM, Sharma, Swati2 wrote:
> Hi Chaitanya
> 
> On 21-07-2026 04:30 pm, Borah, Chaitanya Kumar wrote:
>>
>>
>> On 7/21/2026 4:25 PM, Sharma, Swati2 wrote:
>>> Hi Chaitanya,
>>>
>>> On 14-07-2026 08:15 pm, Chaitanya Kumar Borah wrote:
>>>> This reverts commit be4e98c3b79e8bb843f7bb762616742691fee72e.
>>>>
>>>> Enabling DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE from the common
>>>> igt_display_require() path makes all IGT display clients opt into the
>>>> new plane color pipeline UAPI.
>>>>
>>>> With the color pipeline client cap enabled, legacy plane COLOR_ENCODING
>>>> and COLOR_RANGE properties are ignored during commit preparation.
>>>> The driver cannot distinguish this case, because enum 
>>>> drm_color_encoding
>>>> and enum drm_color_range do not provide an explicit uninitialized 
>>>> value.
>>>>
>>>> Revert the global opt-in and keep the cap enablement local to the
>>>> color-pipeline-aware tests. kms_properties colorop coverage shall be
>>>> preserved separately.
>>>>
>>>> Cc: John Harrison <[email protected]>
>>>> Cc: Kamil Konieczny <[email protected]>
>>>> Cc: Swati Sharma <[email protected]>
>>>> Signed-off-by: Chaitanya Kumar Borah <[email protected]>
>>>> ---
>>>>   lib/igt_kms.c              | 2 --
>>>>   tests/kms_color_pipeline.c | 5 +++++
>>>>   tests/kms_colorop.c        | 4 ++++
>>>>   3 files changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
>>>> index 2eefb773b..c56127c42 100644
>>>> --- a/lib/igt_kms.c
>>>> +++ b/lib/igt_kms.c
>>>> @@ -3201,8 +3201,6 @@ void igt_display_require(igt_display_t 
>>>> *display, int drm_fd)
>>>>       if (drmSetClientCap(drm_fd, 
>>>> LOCAL_DRM_CLIENT_CAP_CURSOR_PLANE_HOTSPOT, 1) == 0)
>>>>           display->has_virt_cursor_plane = 1;
>>>> -    if (drmSetClientCap(drm_fd, 
>>>> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE, 1) == 0)
>>>> -        display->has_plane_color_pipeline = 1;
>>>>       plane_resources = drmModeGetPlaneResources(display->drm_fd);
>>>>       igt_assert(plane_resources);
>>>> diff --git a/tests/kms_color_pipeline.c b/tests/kms_color_pipeline.c
>>>> index 6119a830f..78860a845 100644
>>>> --- a/tests/kms_color_pipeline.c
>>>> +++ b/tests/kms_color_pipeline.c
>>>> @@ -349,6 +349,7 @@ run_tests_for_plane(data_t *data)
>>>>   int igt_main()
>>>>   {
>>>> +    int has_plane_color_pipeline = 0;
>>>>       data_t data = {};
>>>>       igt_fixture() {
>>>> @@ -363,9 +364,13 @@ int igt_main()
>>>>           if (drmSetClientCap(data.drm_fd, DRM_CLIENT_CAP_ATOMIC, 1) 
>>>> == 0)
>>>>               data.display.is_atomic = 1;
>>>> +        if (drmSetClientCap(data.drm_fd, 
>>>> DRM_CLIENT_CAP_PLANE_COLOR_PIPELINE, 1) == 0)
>>>> +            has_plane_color_pipeline = 1;
>>>> +
>>>>           kmstest_set_vt_graphics_mode();
>>>>           igt_display_require(&data.display, data.drm_fd);
>>>> +        data.display.has_plane_color_pipeline = 
>>>> has_plane_color_pipeline;
>>>>           igt_require(data.display.is_atomic);
>>>>       }
>>>> diff --git a/tests/kms_colorop.c b/tests/kms_colorop.c
>>>> index 4db83470d..16db0b8ad 100644
>>>> --- a/tests/kms_colorop.c
>>>> +++ b/tests/kms_colorop.c
>>>> @@ -418,10 +418,14 @@ 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;
>>>>           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;
>>>
>>> Can't we follow same local variable pattern as kms_color_pipeline? 
>>> That will avoid calling drmSetClientCap() twice.
>>>
>>
>> We can but since this is a pure revert we can take it up in a separate 
>> patch.
>>
>> I can float one if this gets approved.
> 
> Sure, lets get this fixed as well as part of a new patch.
> 

Thank you for the review, Swati. I have a patch ready. I will float it 
as soon as these are merged.

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;

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

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


> Above changes LGTM
> 
> Reviewed-by: Swati Sharma <[email protected]>
> 
>>
>> ==
>> Chaitanya
>>
>>>
>>>> igt_require(display.is_atomic);
>>>>       }
>>