Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4241 bytes --]

Hi

Am 12.07.21 um 16:26 schrieb Sumera Priyadarsini:
> On Mon, Jul 12, 2021 at 6:53 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 12.07.21 um 13:56 schrieb Sumera Priyadarsini:
>>> On Mon, Jul 5, 2021 at 1:16 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Vkms copies each plane's framebuffer into the output buffer; essentially
>>>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>>>> handles the details of mapping/unmapping shadow buffers into memory for
>>>> active planes.
>>>>
>>>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>>>> test exposure to shadow-plane helpers.
>>>>
>>>> Thomas Zimmermann (4):
>>>>     drm/gem: Export implementation of shadow-plane helpers
>>>>     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>>>     drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>>>     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>>>>
>>>>    drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
>>>>    drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
>>>>    drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
>>>>    drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
>>>>    include/drm/drm_gem_atomic_helper.h     |  6 +++
>>>>    5 files changed, 86 insertions(+), 64 deletions(-)
>>>>
>>>>
>>>> base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
>>>> --
>>>> 2.32.0
>>>>
>>> Hi,
>>>
>>> Thanks for the patches. The switch to shadow-plane helpers also solved
>>> a bug that was causing a kernel
>>> panic during some IGT kms_flip subtests on the vkms virtual hw patch.
>>
>> Melissa mention something like that as well and I don't really
>> understand. Patch 3 removes an error message from the code, but is the
>> actual bug also gone?
> 
> Yes, I think so. Earlier, while testing the vkms virtual hw patch, the
> tests were
> not just failing, but the vmap fail also preceeded a page fault which required a
> whole restart. Check these logs around line 303:
> https://pastebin.pl/view/03b750be.
> 

With the help of your log, I can see what's happening. The current vkms 
code reports an error in vmap, but does nothing about it. [1] So later 
during the commit, it operates with a bogus value for vaddr.

The shared helper returns the error into the atomic modesetting 
machinery, [2] which then aborts the commit. It never gets to the point 
of using an invalid address. So no kernel panic occurs.

> I could be wrong but I think if the same bug was still present, then
> the kernel panic
> would also happen even if the error message was not being returned.

I'm pretty sure the vmap issue is still there. But as the shared code 
handles it correctly without a notice to the kernel log; and it doesn't 
crash the kernel any longer.

But the vmap problem is caused by some other factor unrelated to vkms.
Booting the test kernel with drm.debug=0x1ff on the kernel command line 
would probably turn up some sort of error message.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_plane.c#L166
[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem_atomic_helper.c#L299

> 
> Cheers,
> Sumera
> 
>>
>> There's little difference between vkms' original code and the shared
>> helper; except for the order of operations in prepare_fb. The shared
>> helper synchronizes fences before mapping; vkms mapped first.
>>
>> (Maybe the shared helper should warn about failed vmaps as well. But
>> that's for another patch.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> Tested-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
>>>
>>> Cheers,
>>> Sumera
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


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