Re: [PATCH V4 0/2] Add virtual hardware module

[Melissa Wen]

On 04/07, Pekka Paalanen wrote:
> On Mon, 5 Apr 2021 11:41:50 +0530
> Sumera Priyadarsini <sylphrenadin@xxxxxxxxx> wrote:
> 
> > This patchset adds support for emulating virtual hardware with VKMS.
> > The virtual hardware mode can be enabled by using the following command
> > while loading the module:
> >         sudo modprobe vkms enable_virtual_hw=1
> 
> Hi,
> 
> every time I see this cover letter subject, I start wondering "what is
> this virtual hardware module, yet another one?" and then I read the
> cover letter and realise it is about adding an option to VKMS.
> 
> The next time you revise this series, could you perhaps clarify the
> subject?
+1
> 
> The idea of having a mode where VKMS behaves like a virtual hardware
> driver is good, IMO. I do think "vblank-less mode" describes it better
> though, because I would assume things like USB display drivers to work
> like this too, and VKMS is already a virtual driver anyway.
> 
> To clarify, as a userspace programmer what I would expect "vblank-less
> mode" to be is that the DRM driver completes pageflips and modesets at
> arbitrary times, perhaps always immediately or perhaps with a variable
> delay that depends on how much processing is needed for the update.
> Also vblank events do not fire and vblank counters do not advance. Is
> this correct?
> 
yes. And I think this description should be clear in both the cover
letter and also the commit message of the patch that add the module
option to vkms.
> 
> Thanks,
> pq
> 
> > 
> > The first patch is prep work for adding virtual_hw mode and refactors
> > the plane composition in vkms by adding a helper function vkms_composer_common()
> > which can be used for both vblank mode and virtual mode.
> > 
> > The second patch adds virtual hardware support as a module option. It
> > adds new atomic helper functions for the virtual mode
> > and modifies the existing atomic helpers for usage by the vblank mode
> > This gives us two sets of drm_crtc_helper_funcs struct for both modes,
> > making the code flow cleaner and easier to debug.
> > 
> > This patchset has been tested with the igt tests- kms_writeback, kms_atomic,
> > kms_lease, kms_flip, kms_pipe_get_crc and preserves results except for
> > subtests related to crc reads and skips tests that rely on vertical
> > blanking. This patchset must be tested after incorporating the
> > igt-tests patch: https://lists.freedesktop.org/archives/igt-dev/2021-February/029355.html

Sumera,

Thanks for your patches.

In addition to Pekka's comments, consider what I comment in each patch
of the series for a next version.

Best regards,

Melissa
> > 
> > Sumera Priyadarsini (2):
> >   drm/vkms: Refactor vkms_composer_worker() to prep for virtual_hw mode
> >   drm/vkms: Add support for virtual hardware mode
> > 
> >  drivers/gpu/drm/vkms/vkms_composer.c | 88 +++++++++++++++++-----------
> >  drivers/gpu/drm/vkms/vkms_crtc.c     | 51 +++++++++++-----
> >  drivers/gpu/drm/vkms/vkms_drv.c      | 18 ++++--
> >  drivers/gpu/drm/vkms/vkms_drv.h      |  4 ++
> >  4 files changed, 109 insertions(+), 52 deletions(-)
> > 
>