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

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

[José Expósito]

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(-)

-- 
2.25.1

[PATCH 1/3] drm/vkms: refactor overlay plane creation

[José Expósito]

Move the logic to create an overlay plane to its own function.
Refactor, no functional changes.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_output.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 04406bd3ff02..2e805b2d36ae 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -32,6 +32,21 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
 	.get_modes    = vkms_conn_get_modes,
 };
 
+static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
+				  struct drm_crtc *crtc)
+{
+	struct vkms_plane *overlay;
+
+	overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
+	if (IS_ERR(overlay))
+		return PTR_ERR(overlay);
+
+	if (!overlay->base.possible_crtcs)
+		overlay->base.possible_crtcs = drm_crtc_mask(crtc);
+
+	return 0;
+}
+
 int vkms_output_init(struct vkms_device *vkmsdev, int index)
 {
 	struct vkms_output *output = &vkmsdev->output;
@@ -39,7 +54,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	struct drm_connector *connector = &output->connector;
 	struct drm_encoder *encoder = &output->encoder;
 	struct drm_crtc *crtc = &output->crtc;
-	struct vkms_plane *primary, *cursor = NULL, *overlay = NULL;
+	struct vkms_plane *primary, *cursor = NULL;
 	int ret;
 	int writeback;
 
@@ -48,12 +63,9 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		return PTR_ERR(primary);
 
 	if (vkmsdev->config->overlay) {
-		overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
-		if (IS_ERR(overlay))
-			return PTR_ERR(overlay);
-
-		if (!overlay->base.possible_crtcs)
-			overlay->base.possible_crtcs = drm_crtc_mask(crtc);
+		ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
+		if (ret)
+			return ret;
 	}
 
 	if (vkmsdev->config->cursor) {
-- 
2.25.1

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

[José Expósito]

Add a new module parameter to allow to set the number of overlay planes
to create. Set it to 1 by default in order to keep the "enable_overlay"
backwards compatible.

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c    | 5 +++++
 drivers/gpu/drm/vkms/vkms_drv.h    | 1 +
 drivers/gpu/drm/vkms/vkms_output.c | 9 ++++++---
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 0ffe5f0e33f7..bb98f6c6c561 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -51,6 +51,10 @@ static bool enable_overlay;
 module_param_named(enable_overlay, enable_overlay, bool, 0444);
 MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
 
+static unsigned int num_overlay_planes = 1;
+module_param_named(num_overlay_planes, num_overlay_planes, uint, 0444);
+MODULE_PARM_DESC(num_overlay_planes, "Number of overlay planes to create");
+
 DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
 static void vkms_release(struct drm_device *dev)
@@ -229,6 +233,7 @@ static int __init vkms_init(void)
 	config->cursor = enable_cursor;
 	config->writeback = enable_writeback;
 	config->overlay = enable_overlay;
+	config->num_overlay_planes = num_overlay_planes;
 
 	return vkms_create(config);
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index d48c23d40ce5..33bdf717e3cd 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -97,6 +97,7 @@ struct vkms_config {
 	bool writeback;
 	bool cursor;
 	bool overlay;
+	unsigned int num_overlay_planes;
 	/* only set when instantiated */
 	struct vkms_device *dev;
 };
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 2e805b2d36ae..6f26998fdb7e 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -57,15 +57,18 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	struct vkms_plane *primary, *cursor = NULL;
 	int ret;
 	int writeback;
+	unsigned int n;
 
 	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
 	if (vkmsdev->config->overlay) {
-		ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
-		if (ret)
-			return ret;
+		for (n = 0; n < vkmsdev->config->num_overlay_planes; n++) {
+			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
+			if (ret)
+				return ret;
+		}
 	}
 
 	if (vkmsdev->config->cursor) {
-- 
2.25.1

[PATCH 3/3] drm/vkms: drop "Multiple overlay planes" TODO

[José Expósito]

Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
 Documentation/gpu/vkms.rst | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 941f0e7e5eef..9c873c3912cc 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -124,8 +124,6 @@ Add Plane Features
 
 There's lots of plane features we could add support for:
 
-- Multiple overlay planes. [Good to get started]
-
 - Clearing primary plane: clear primary plane before plane composition (at the
   start) for correctness of pixel blend ops. It also guarantees alpha channel
   is cleared in the target buffer for stable crc. [Good to get started]
-- 
2.25.1

Re: [PATCH 2/3] drm/vkms: add support for multiple overlay planes

[Melissa Wen]

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

On 12/13, José Expósito wrote:
> Add a new module parameter to allow to set the number of overlay planes
> to create. Set it to 1 by default in order to keep the "enable_overlay"
> backwards compatible.
> 
Hi José,

in general, lgtm. 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 and to avoid passing "crazy"
numbers by another module option.
Afaik, we are also limited to 32, as you can see in this commit:

2a8d3eac3d6e1
drm: Warn if plane/crtc/encoder/connector index exceeds our 32bit bitmasks

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. Also, changing this number in the future would be pretty
straightfoward.

Thanks,

Melissa

> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.c    | 5 +++++
>  drivers/gpu/drm/vkms/vkms_drv.h    | 1 +
>  drivers/gpu/drm/vkms/vkms_output.c | 9 ++++++---
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 0ffe5f0e33f7..bb98f6c6c561 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -51,6 +51,10 @@ static bool enable_overlay;
>  module_param_named(enable_overlay, enable_overlay, bool, 0444);
>  MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>  
> +static unsigned int num_overlay_planes = 1;
> +module_param_named(num_overlay_planes, num_overlay_planes, uint, 0444);
> +MODULE_PARM_DESC(num_overlay_planes, "Number of overlay planes to create");
> +
>  DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>  
>  static void vkms_release(struct drm_device *dev)
> @@ -229,6 +233,7 @@ static int __init vkms_init(void)
>  	config->cursor = enable_cursor;
>  	config->writeback = enable_writeback;
>  	config->overlay = enable_overlay;
> +	config->num_overlay_planes = num_overlay_planes;
>  
>  	return vkms_create(config);
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index d48c23d40ce5..33bdf717e3cd 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -97,6 +97,7 @@ struct vkms_config {
>  	bool writeback;
>  	bool cursor;
>  	bool overlay;
> +	unsigned int num_overlay_planes;
>  	/* only set when instantiated */
>  	struct vkms_device *dev;
>  };
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 2e805b2d36ae..6f26998fdb7e 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -57,15 +57,18 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>  	struct vkms_plane *primary, *cursor = NULL;
>  	int ret;
>  	int writeback;
> +	unsigned int n;
>  
>  	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
>  	if (IS_ERR(primary))
>  		return PTR_ERR(primary);
>  
>  	if (vkmsdev->config->overlay) {
> -		ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> -		if (ret)
> -			return ret;
> +		for (n = 0; n < vkmsdev->config->num_overlay_planes; n++) {
> +			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	if (vkmsdev->config->cursor) {
> -- 
> 2.25.1
> 

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

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

[Melissa Wen]

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

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
> 

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

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

[José Expósito]

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/20211223081030.16629-1-jose.exposito89@gmail.com/T/