[PATCH v4 7/9] drm: vkms: Refactor the plane composer to accept new formats

[Igor Torrente]

Hi Pekka,

On 2/21/22 06:18, Pekka Paalanen wrote:
> On Sun, 20 Feb 2022 22:02:12 -0300
> Igor Torrente <igormtorrente at gmail.com> wrote:
> 
>> Hi Melissa,
>>
>> On 2/9/22 18:45, Melissa Wen wrote:
>>> On 02/08, Igor Torrente wrote:
>>>> Hi Melissa,
>>>>
>>>> On 2/8/22 07:40, Melissa Wen wrote:
>>>>> On 01/21, Igor Torrente wrote:
>>>>>> Currently the blend function only accepts XRGB_8888 and ARGB_8888
>>>>>> as a color input.
>>>>>>
>>>>>> This patch refactors all the functions related to the plane composition
>>>>>> to overcome this limitation.
>>>>>>
>>>>>> A new internal format(`struct pixel`) is introduced to deal with all
>>>>>> possible inputs. It consists of 16 bits fields that represent each of
>>>>>> the channels.
>>>>>>
>>>>>> The pixels blend is done using this internal format. And new handlers
>>>>>> are being added to convert a specific format to/from this internal format.
>>>>>>
>>>>>> So the blend operation depends on these handlers to convert to this common
>>>>>> format. The blended result, if necessary, is converted to the writeback
>>>>>> buffer format.
>>>>>>
>>>>>> This patch introduces three major differences to the blend function.
>>>>>> 1 - All the planes are blended at once.
>>>>>> 2 - The blend calculus is done as per line instead of per pixel.
>>>>>> 3 - It is responsible to calculates the CRC and writing the writeback
>>>>>>        buffer(if necessary).
>>>>>>
>>>>>> These changes allow us to allocate way less memory in the intermediate
>>>>>> buffer to compute these operations. Because now we don't need to
>>>>>> have the entire intermediate image lines at once, just one line is
>>>>>> enough.
>>>>>>
>>>>>> | Memory consumption (output dimensions) |
>>>>>> |:--------------------------------------:|
>>>>>> |       Current      |     This patch    |
>>>>>> |:------------------:|:-----------------:|
>>>>>> |   Width * Heigth   |     2 * Width     |
>>>>>>
>>>>>> Beyond memory, we also have a minor performance benefit from all
>>>>>> these changes. Results running the IGT tests `*kms_cursor_crc*`:
>>>>>>   
>>>>> First, thanks for this improvement.
>>>>>
>>>>> Some recent changes in kms_cursor_crc caused VKMS to fail in most test
>>>>> cases (iirc, only size-change and alpha-opaque are passing currently).
>>>>
>>>> I updated my igt and kernel(from drm_misc/drm-misc-next) to the latest
>>>> commit[1][2] and I'm getting mixed results. Sometimes most of the test
>>>> passes, sometimes almost nothing passes.
>>> hmm.. is it happening when running kms_cursor_crc? Is the results
>>> variation random or is it possible to follow a set of steps to reproduce
>>> it? When failing, what is the reason displayed by the log?
>>
>> I investigated it a little bit and discovered that the KMS
>> cursor(".*kms_cursor_crc*" ) are failing after the execution of
>> writeback tests(".*kms_writeback.*").
>>
>> I don't know what is causing it, but they are failing while trying to
>> commit the KMS changes.
>>
>> out.txt:
>> IGT-Version: 1.26-NO-GIT (x86_64) (Linux: 5.17.0-rc2 x86_64)
>> Stack trace:
>>     #0 ../lib/igt_core.c:1754 __igt_fail_assert()
>>     #1 ../lib/igt_kms.c:3795 do_display_commit()
>>     #2 ../lib/igt_kms.c:3901 igt_display_commit2()
>>     #3 ../tests/kms_cursor_crc.c:820 __igt_unique____real_main814()
>>     #4 ../tests/kms_cursor_crc.c:814 main()
>>     #5 ../csu/libc-start.c:308 __libc_start_main()
>>     #6 [_start+0x2a]
>> Subtest pipe-A-cursor-size-change: FAIL
>>
>> err.txt:
>> (kms_cursor_crc:1936) igt_kms-CRITICAL: Test assertion failure function
>> do_display_commit, file ../lib/igt_kms.c:3795:
>> (kms_cursor_crc:1936) igt_kms-CRITICAL: Failed assertion: ret == 0
>> (kms_cursor_crc:1936) igt_kms-CRITICAL: Last errno: 22, Invalid argument
>> (kms_cursor_crc:1936) igt_kms-CRITICAL: error: -22 != 0
>>
>>>
>>>   From my side, only the first two subtest of kms_cursor_crc is passing
>>> before this patch. And after your changes here, all subtests are
>>> successful again, except those related to 32x10 cursor size (that needs
>>> futher investigation). I didn't check how the recent changes in
>>> kms_cursor_crc affect VKMS performance on it, but I bet that clearing
>>> the alpha channel is the reason to have the performance back.
>>
>> Yeah, I also don't understand why the 32x10 cursor tests are failing.
>>
> 
> Hi,
> 
> are the tests putting the cursor partially outside of the CRTC area?
> Or partially outside of primary plane area (which IIRC you used when you
> should have used the CRTC area?)?
> 
> Does the writeback test forget to unlink the writeback connector? Or
> does VKMS not handle unlinking the writeback connector?

I don't know the answer to all these questions.

I did try to find the commit that introduces this issue, and I found
that it's happening since the writeback was introduced in Aug
2020(dbd9d80c).

And the failure related to the 32x10 cursor was happening before my
changes.

> 
> If both are a problem, the latter would be just an unrelated bug that
> exposes the first bug in VKMS, because whether writeback is used or not
> probably should not affect where the cursor plane is allowed to be.

Yeah, I don't think those a related.

Best Regards.
---
Igor Torrente

> 
> 
> Thanks,
> pq