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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6063 bytes --]

On Mon, 21 Feb 2022 22:13:21 -0300
Igor Torrente <igormtorrente@gmail.com> wrote:

> Hi Pekka,
> 
> On 2/21/22 06:18, Pekka Paalanen wrote:
> > On Sun, 20 Feb 2022 22:02:12 -0300
> > Igor Torrente <igormtorrente@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.

These are just suggestions in the direction of research, just in case
you had no idea. ;-)

After all, the UAPI error code is EINVAL, so something in VKMS rejects
the IGT-produced configuration. Figuring out what that configuration is
and why it gets rejected might be useful to find out.

Maybe the original writeback code did not expect planes to be partially
off-screen? Guarding against that would produce EINVAL. This is just a
wild guess, I've never read that code, but it also seems like the
simplest possible mistake to make in good faith - not a bug in code,
just a wrong initial assumption of use cases.

If you found in your testing that the IGT cursor-size-change test
succeeds if ran before writeback test, but fails if ran after the
writeback test, then obviously something in the writeback test is
leaving stray state behind. It could be the test itself, or it could be
a VKMS bug.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]