[igt-dev] [PATCH i-g-t v1] tests/kms_universal_plane: Skip pageflip_test_pipe subtest if pipe has more than 1 primary plane

[Abhinav Kumar]

On 5/2/2022 3:42 PM, Rob Clark wrote:
> On Mon, May 2, 2022 at 3:31 PM Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>>
>>
>>
>> On 5/2/2022 2:50 PM, Rob Clark wrote:
>>> On Thu, Apr 28, 2022 at 6:11 PM Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>>>>
>>>> Currently, IGT assumes that the first primary plane it encounters in
>>>> display->planes is the primary plane being assigned to the current pipe
>>>> in the driver [1]. However, this is not always the case when planes are
>>>> being shared between pipes and there are multiple possible primary
>>>> planes.
>>>
>>> I wonder if it is actually worse than that.. igt_pipe_t seems designed
>>> around the idea that CRTCs have exclusive ownership of planes.  Not
>>> sure if there are any tests that drive multiple CRTCs at the same
>>> time, which could end up trying to use the same plane on multiple
>>> pipes..
>>>
>>> BR,
>>> -R
>> Yep, there are at least a few cases in kms_atomic_transition where, with
>> DP connected, IGT will try to do a commit for 2 CRTCs within the same
>> commit [1]. Planning to submit a patch addressing that after we sort out
>> the shared planes-related failures for the no DP case.
> 
> Ok.. then perhaps until igt handles planes that can move between CRTCs
> better, we should statically assign planes to no more than a single
> pipe in igt_display_require()?  I _think_ that would solve both
> issues..
> 
> BR,
> -R

There are two concerns / issues with this:

1) How would we know which planes to assign to which pipe?
Today we use possible_crtcs and both the crtcs are listed for the plane 
in the possible_crtcs.

2) Even if we assign one of the primary planes like IGT does today, we 
do not know if whatever we picked is what was selected even in the driver.

The only way to reliably do this is if driver exposes something like 
"primary_plane_id" in the drm_mode_crtc?

111 struct drm_mode_crtc {
112 	__u64 set_connectors_ptr;
113 	__u32 count_connectors;
114
115 	__u32 crtc_id; /**< Id */
116 	__u32 fb_id; /**< Id of framebuffer */
117
118 	__u32 x, y; /**< Position on the frameuffer */
119
120 	__u32 gamma_size;
121 	__u32 mode_valid;
122 	struct drm_mode_modeinfo mode;
123 };

> 
>> (BTW, to elaborate some more on the issue we were facing with this
>> subtest on trogdor: an -EBUSY was being thrown from the page flip ioctl
>> [2] because we were assigning an fb to an unused primary plane.)
>>
>> Best,
>>
>> Jessica Zhang
>>
>> [1]
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_atomic_transition.c#L709
>> (note: `mask` in this method is a mask for which CRTC to commit to, and
>> with DP connected it can go up to 3)
>>
>> [2]
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/commit/ed944b45563c694dc6373bc48dc83b8ba7edb19f
>>
>>>
>>>> It's possible for IGT to set pipe->plane_primary to a primary plane
>>>> that's different from the one being used in the driver, since IGT uses
>>>> possible_crtcs to find the primary planes and stops at the first match.
>>>>
>>>> Unfortunately, DRM doesn't expose which primary plane is being used by a
>>>> pipe, so let's skip pageflip_test_pipe for cases where there are
>>>> multiple possible primary planes.
>>>>
>>>> [1]
>>>> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/lib/igt_kms.c#L2305
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>> ---
>>>>    tests/kms_universal_plane.c | 14 +++++++++++++-
>>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
>>>> index 3cb6d704a6ef..95cae2be1aaf 100644
>>>> --- a/tests/kms_universal_plane.c
>>>> +++ b/tests/kms_universal_plane.c
>>>> @@ -470,13 +470,25 @@ static void
>>>>    pageflip_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
>>>>    {
>>>>           pageflip_test_t test = { .data = data };
>>>> -       igt_plane_t *primary;
>>>> +       igt_plane_t *primary, *plane;
>>>>           struct timeval timeout = { .tv_sec = 0, .tv_usec = 500 };
>>>>           drmEventContext evctx = { .version = 2 };
>>>>
>>>>           fd_set fds;
>>>> +       int primary_plane_count = 0;
>>>>           int ret = 0;
>>>>
>>>> +       for_each_plane_on_pipe(&data->display, pipe, plane)
>>>> +               if (plane->type == DRM_PLANE_TYPE_PRIMARY)
>>>> +                       primary_plane_count++;
>>>> +
>>>> +       /*
>>>> +        * Skip subtest when there are multiple primary planes since we
>>>> +        * aren't able to distinguish which primary plane is actually being
>>>> +        * assigned a pipe
>>>> +        */
>>>> +       igt_skip_on(primary_plane_count > 1);
>>>> +
>>>>           igt_require_pipe(&data->display, pipe);
>>>>
>>>>           igt_output_set_pipe(output, pipe);
>>>> --
>>>> 2.31.0
>>>>