Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 10 Apr 2021 09:21:34 -0300
From:   Melissa Wen <melissa.srw@gmail.com>
To:     Pekka Paalanen <ppaalanen@gmail.com>
Cc:     Sumera Priyadarsini <sylphrenadin@gmail.com>,
        hamohammed.sa@gmail.com, rodrigosiqueiramelo@gmail.com,
        airlied@linux.ie, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH V4 0/2] Add virtual hardware module
Message-ID: <20210410122134.7dlbadm5blkm32zg@smtp.gmail.com>
References: <cover.1617602076.git.sylphrenadin@gmail.com>
 <20210407101258.72261c5d@eldfell>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210407101258.72261c5d@eldfell>
Precedence: bulk

On 04/07, Pekka Paalanen wrote:
> On Mon, 5 Apr 2021 11:41:50 +0530
> Sumera Priyadarsini <sylphrenadin@gmail.com> 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(-)
> > 
>