[igt-dev] [PATCH i-g-t v2] tests/kms_plane_scaling: Increase buffer size if driver doesn't support larger scale factors

[Jessica Zhang]

On 12/9/2021 10:11 PM, Petri Latvala wrote:
> On Thu, Dec 09, 2021 at 12:54:58PM -0800, Jessica Zhang wrote:
>>
>>
>> On 12/9/2021 3:26 AM, Petri Latvala wrote:
>>> On Wed, Dec 08, 2021 at 04:27:16PM -0800, Jessica Zhang wrote:
>>>> Catch edge cases where driver doesn't support larger scale factors or
>>>> pipe doesn't support scaling.
>>>>
>>>> Currently, a 20x20 framebuffer is passed in to be upscaled. However,
>>>> this will cause issues with other drivers as they may not support larger
>>>> scale factors or may not support scaling at all for certain planes.
>>>>
>>>> This avoids failures due to invalid scale factor by trying
>>>> the original 20x20 framebuffer commit, then trying to commit larger
>>>> framebuffers up to and including unity scale.
>>>>
>>>> Changes since V1:
>>>> - try_commit_with_fb_size: Changed igt_try_commit2 to
>>>>     igt_display_try_commit_atomic instead to avoid redundant commits
>>>> - try_commit_with_fb_size: Moved calls to clear framebuffer to outside
>>>>     of success condition and moved cleanup_crtc back to outside of method
>>>>
>>>> Tested-on: Qualcomm RB5 (sdm845)
>>>>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>> ---
>>>>    tests/kms_plane_scaling.c | 55 ++++++++++++++++++++++++++++++++-------
>>>>    1 file changed, 46 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>>>> index 85db11ee..971c1532 100644
>>>> --- a/tests/kms_plane_scaling.c
>>>> +++ b/tests/kms_plane_scaling.c
>>>> @@ -1,5 +1,6 @@
>>>>    /*
>>>>     * Copyright © 2013,2014 Intel Corporation
>>>> + * Copyright (c) 2021 Qualcomm Innovation Center, Inc. All rights reserved.
>>>>     *
>>>>     * Permission is hereby granted, free of charge, to any person obtaining a
>>>>     * copy of this software and associated documentation files (the "Software"),
>>>> @@ -118,6 +119,32 @@ static void prepare_crtc(data_t *data, igt_output_t *output, enum pipe pipe,
>>>>    	igt_display_commit2(display, COMMIT_ATOMIC);
>>>>    }
>>>> +static int try_commit_with_fb_size(int width, int height, igt_rotation_t rot,
>>>> +		                            igt_display_t *display, data_t *d,
>>>> +                                   igt_plane_t *plane,
>>>> +		                            drmModeModeInfo *mode)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/* Check min to full resolution upscaling */
>>>> +	igt_fb_set_position(&d->fb[0], plane, 0, 0);
>>>> +	igt_fb_set_size(&d->fb[0], plane, width, height);
>>>> +	igt_plane_set_position(plane, 0, 0);
>>>> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>>>> +	igt_plane_set_rotation(plane, rot);
>>>> +
>>>> +	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
>>>> +		                                DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>>>> +
>>>> +	if (!ret)
>>>> +		igt_display_commit2(display, COMMIT_ATOMIC);
>>>
>>> Now this looks more understandable but I still have one "why"
>>> remaining: Why test_only the commit if you're anyway going to perform
>>> the commit? You could just have this as one try_commit, without
>>> test_only, and let it perform it, as there's nothing done differently
>>> here based on success/failure.
>>>
>> The original intention was to test that a commit will go through before
>> actually doing the commit. This way, the test would be more optimized since
>> we would only perform a commit when we know it will succeed. Otherwise, in a
>> worst case scenario, we'd be performing 3 or 4 commits per pipe/format for a
>> subtest like pipe-*-scaler-with-pixel-format.
> 
> But the kernel does the same checks for TEST_ONLY as for actually
> doing the commit. For this kind of optimization assertion I have to
> ask for some numbers to prove it's saving time enough to be worth of
> the more complicated code.
> 
> Doing a TEST_ONLY commit followed by doing the same commit without
> TEST_ONLY surely is slower when the commit will succeed at least.
> 

Sorry if this wasn't clear in my previous reply -- I'm focusing on 
optimizing for cases where scaling a 20x20 buffer isn't supported and 
for failure cases. For example, if we just call try_commit, we would be 
doing 3 commits per format per pipe in cases where the driver only 
supports unity scale. Since TEST_ONLY will only perform a check without 
performing an actual commit [1], wouldn't it be more optimal to only 
perform a commit if the commit will succeed in cases like this?

This approach is based on instances of try_commit_atomic in other tests 
[2, 3] where we catch an error from the commit and adjust the test 
accordingly, which is what this patch wants to emulate.

Thanks,
Jessica Zhang

[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/drm_atomic_uapi.c#L1456
[2] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_plane_scaling.c#L660
[3] 
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_atomic_transition.c#L75

>