[0/3] drm/vkms: add support for multiple overlay planes

Message

José Expósito Dec. 13, 2021, 6:11 p.m. UTC

Hi all,

First of all, let me quickly introduce myself. I'm José Expósito and
I'm a hobbyist open source developer.
My contributions are usually in the kernel input/HID subsystems and
in its userspace library (libinput) as well as other projects.

I'm trying to learn more about the GPU and I found VKMS as a good
project to get started.

So, now that you have been warned about my (lack) of experience in this
subsystem, let's get into the patches.

The series adds support for multiple overlay planes by adding a new
module parameter that allows to configure the number of overlay planes
to add.

I checked that the planes are properly created using drm_info[1] and
also executed the IGT test "kms_plane" to make sure that the planes
were listed in the output.
In addition, I checked the vkms_composer and -even though I'm not sure
how to properly test it- it looks like it is already prepared to
compose an arbitrary number of planes.
Having said that, I really hope I didn't make any obvious mistakes.

I noticed that the docs say:
> For all of these, we also want to review the igt test coverage and
> make sure all relevant igt testcases work on vkms

I ran some plane related tests, but some of them were already failing
without my code, so I'd appreciate some help with this bit.

Thank you very much in advance for your time. Any suggestions to
improve this patchset or about what to work on next are very welcome.

Jose

[1] https://github.com/ascent12/drm_info

José Expósito (3):
  drm/vkms: refactor overlay plane creation
  drm/vkms: add support for multiple overlay planes
  drm/vkms: drop "Multiple overlay planes" TODO

 Documentation/gpu/vkms.rst         |  2 --
 drivers/gpu/drm/vkms/vkms_drv.c    |  5 +++++
 drivers/gpu/drm/vkms/vkms_drv.h    |  1 +
 drivers/gpu/drm/vkms/vkms_output.c | 29 ++++++++++++++++++++++-------
 4 files changed, 28 insertions(+), 9 deletions(-)

Comments

Melissa Wen Dec. 23, 2021, 8:35 p.m. UTC | #1

On 12/13, José Expósito wrote:
> Hi all,
> 
> First of all, let me quickly introduce myself. I'm José Expósito and
> I'm a hobbyist open source developer.
> My contributions are usually in the kernel input/HID subsystems and
> in its userspace library (libinput) as well as other projects.
> 
> I'm trying to learn more about the GPU and I found VKMS as a good
> project to get started.
> 
> So, now that you have been warned about my (lack) of experience in this
> subsystem, let's get into the patches.
> 
> The series adds support for multiple overlay planes by adding a new
> module parameter that allows to configure the number of overlay planes
> to add.
> 
> I checked that the planes are properly created using drm_info[1] and
> also executed the IGT test "kms_plane" to make sure that the planes
> were listed in the output.
> In addition, I checked the vkms_composer and -even though I'm not sure
> how to properly test it- it looks like it is already prepared to
> compose an arbitrary number of planes.
> Having said that, I really hope I didn't make any obvious mistakes.
> 
> I noticed that the docs say:
> > For all of these, we also want to review the igt test coverage and
> > make sure all relevant igt testcases work on vkms
> 
> I ran some plane related tests, but some of them were already failing
> without my code, so I'd appreciate some help with this bit.

Hi José,

What test did you run? Indeed, not all kms tests are passing and fixes
are welcome :)

Last time, I used these testcases for overlay: kms_plane_cursor,
kms_atomic; and these tests were fine too: kms_cursor_crc, kms_writeback,
kms_flip

Did you find any regression? Also, a good practice is to report in the
commit message which IGT tests you used to check the proposed changes.

Btw, thanks for your patchset.

Melissa

> 
> Thank you very much in advance for your time. Any suggestions to
> improve this patchset or about what to work on next are very welcome.
> 
> Jose
> 
> [1] https://github.com/ascent12/drm_info
> 
> José Expósito (3):
>   drm/vkms: refactor overlay plane creation
>   drm/vkms: add support for multiple overlay planes
>   drm/vkms: drop "Multiple overlay planes" TODO
> 
>  Documentation/gpu/vkms.rst         |  2 --
>  drivers/gpu/drm/vkms/vkms_drv.c    |  5 +++++
>  drivers/gpu/drm/vkms/vkms_drv.h    |  1 +
>  drivers/gpu/drm/vkms/vkms_output.c | 29 ++++++++++++++++++++++-------
>  4 files changed, 28 insertions(+), 9 deletions(-)
> 
> -- 
> 2.25.1
>

José Expósito Dec. 24, 2021, 11:55 a.m. UTC | #2

Hi Melissa,

Thank you very much for your review.

> On Thu, Dec 23, 2021 at 07:35:48PM -0100, Melissa Wen wrote:
> What test did you run? Indeed, not all kms tests are passing and fixes
> are welcome :)
> 
> Last time, I used these testcases for overlay: kms_plane_cursor,
> kms_atomic; and these tests were fine too: kms_cursor_crc, kms_writeback,
> kms_flip

For the different patches I have been working on I have tested mainly
with kms_atomic, kms_plane_cursor and kms_plane_alpha_blend.

For some reason, kms_cursor_crc suspends my PC. I still need to
investigate the cause.

I'll include a table with success/skip/fail tests before and after
the patch on v2 :)

> However, I think we need some limits for this number
> of planes. I would suggest to just expand the enable_overlay option to
> expose a predefined number of planes
> [...]
> I don't have a strong opinion on an exact/practical number. I took a
> quick look at other drivers and exposing 8 planes seems reasonable to
> me.

8 planes sound reasonable to me, I'll change it and send a revision
of [1] as well using the new constant.

Thanks again for taking the time to review this,
José Expósito

[1] https://lore.kernel.org/dri-devel/[e-mail]/T/