* [PATCH RFC 01/15] drm/vkms: Remove useles devres group
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-19 14:23 ` Daniel Vetter
2024-08-14 14:36 ` [PATCH RFC 02/15] drm/vkms: remove possible crtc from parameters Louis Chauvet
` (13 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As the driver now uses drm managed allocation, the devres group is not
needed anymore, so remove it.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_drv.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index e79832e10f3c..7ac3ab7e16e5 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -297,16 +297,11 @@ static int vkms_create(struct vkms_config *config)
if (IS_ERR(pdev))
return PTR_ERR(pdev);
- if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
- ret = -ENOMEM;
- goto out_unregister;
- }
-
vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
struct vkms_device, drm);
if (IS_ERR(vkms_device)) {
ret = PTR_ERR(vkms_device);
- goto out_devres;
+ goto out_unregister;
}
vkms_device->platform = pdev;
vkms_device->config = config;
@@ -317,32 +312,30 @@ static int vkms_create(struct vkms_config *config)
if (ret) {
DRM_ERROR("Could not initialize DMA support\n");
- goto out_devres;
+ goto out_unregister;
}
ret = drm_vblank_init(&vkms_device->drm, 1);
if (ret) {
DRM_ERROR("Failed to vblank\n");
- goto out_devres;
+ goto out_unregister;
}
ret = vkms_modeset_init(vkms_device);
if (ret)
- goto out_devres;
+ goto out_unregister;
drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
ARRAY_SIZE(vkms_config_debugfs_list));
ret = drm_dev_register(&vkms_device->drm, 0);
if (ret)
- goto out_devres;
+ goto out_unregister;
drm_fbdev_shmem_setup(&vkms_device->drm, 0);
return 0;
-out_devres:
- devres_release_group(&pdev->dev, NULL);
out_unregister:
platform_device_unregister(pdev);
return ret;
@@ -383,7 +376,6 @@ static void vkms_destroy(struct vkms_config *config)
drm_dev_unregister(&config->dev->drm);
drm_atomic_helper_shutdown(&config->dev->drm);
- devres_release_group(&pdev->dev, NULL);
platform_device_unregister(pdev);
config->dev = NULL;
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 01/15] drm/vkms: Remove useles devres group
2024-08-14 14:36 ` [PATCH RFC 01/15] drm/vkms: Remove useles devres group Louis Chauvet
@ 2024-08-19 14:23 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2024-08-19 14:23 UTC (permalink / raw)
To: Louis Chauvet
Cc: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, dri-devel, arthurgrillo,
linux-kernel, jeremie.dautheribes, miquel.raynal,
thomas.petazzoni, seanpaul, nicolejadeyee
On Wed, Aug 14, 2024 at 04:36:23PM +0200, Louis Chauvet wrote:
> As the driver now uses drm managed allocation, the devres group is not
> needed anymore, so remove it.
drmm isn't devres, and you still have a devres managed resource here,
namely devm_drm_dev_alloc. The reason I suggest in the review on google's
series for configfs to nuke this is that they switched over to making vkms
a proper platform driver, in which case you get a devres group
automatically for your driver binding.
But neither the explicit one or the driver binding is one devres too few
:-)
Cheers, Sima
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 18 +++++-------------
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index e79832e10f3c..7ac3ab7e16e5 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -297,16 +297,11 @@ static int vkms_create(struct vkms_config *config)
> if (IS_ERR(pdev))
> return PTR_ERR(pdev);
>
> - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> - ret = -ENOMEM;
> - goto out_unregister;
> - }
> -
> vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
> struct vkms_device, drm);
> if (IS_ERR(vkms_device)) {
> ret = PTR_ERR(vkms_device);
> - goto out_devres;
> + goto out_unregister;
> }
> vkms_device->platform = pdev;
> vkms_device->config = config;
> @@ -317,32 +312,30 @@ static int vkms_create(struct vkms_config *config)
>
> if (ret) {
> DRM_ERROR("Could not initialize DMA support\n");
> - goto out_devres;
> + goto out_unregister;
> }
>
> ret = drm_vblank_init(&vkms_device->drm, 1);
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> - goto out_devres;
> + goto out_unregister;
> }
>
> ret = vkms_modeset_init(vkms_device);
> if (ret)
> - goto out_devres;
> + goto out_unregister;
>
> drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
> ARRAY_SIZE(vkms_config_debugfs_list));
>
> ret = drm_dev_register(&vkms_device->drm, 0);
> if (ret)
> - goto out_devres;
> + goto out_unregister;
>
> drm_fbdev_shmem_setup(&vkms_device->drm, 0);
>
> return 0;
>
> -out_devres:
> - devres_release_group(&pdev->dev, NULL);
> out_unregister:
> platform_device_unregister(pdev);
> return ret;
> @@ -383,7 +376,6 @@ static void vkms_destroy(struct vkms_config *config)
>
> drm_dev_unregister(&config->dev->drm);
> drm_atomic_helper_shutdown(&config->dev->drm);
> - devres_release_group(&pdev->dev, NULL);
> platform_device_unregister(pdev);
>
> config->dev = NULL;
>
> --
> 2.44.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 02/15] drm/vkms: remove possible crtc from parameters
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 01/15] drm/vkms: Remove useles devres group Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:33 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 03/15] drm/vkms: Extract vkms_config header Louis Chauvet
` (12 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As the crtc mask is dynamic, avoid hardcoding it. It is already computed
once all the planes are created, so it should be a no-op
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_drv.c | 10 +++++-----
drivers/gpu/drm/vkms/vkms_plane.c | 5 +++--
drivers/gpu/drm/vkms/vkms_plane.h | 4 +---
3 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 7ac3ab7e16e5..e71b45fcb9b8 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -166,7 +166,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
.get_modes = vkms_conn_get_modes,
};
-static int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc)
+static int vkms_output_init(struct vkms_device *vkmsdev)
{
struct drm_device *dev = &vkmsdev->drm;
struct drm_connector *connector;
@@ -184,20 +184,20 @@ static int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc)
* The overlay and cursor planes are not mandatory, but can be used to perform complex
* composition.
*/
- primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, possible_crtc);
+ primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
if (IS_ERR(primary))
return PTR_ERR(primary);
if (vkmsdev->config->overlay) {
for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
- overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, possible_crtc);
+ overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY);
if (IS_ERR(overlay))
return PTR_ERR(overlay);
}
}
if (vkmsdev->config->cursor) {
- cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, possible_crtc);
+ cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
if (IS_ERR(cursor))
return PTR_ERR(cursor);
}
@@ -284,7 +284,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
dev->mode_config.preferred_depth = 0;
dev->mode_config.helper_private = &vkms_mode_config_helpers;
- return vkms_output_init(vkmsdev, 0);
+ return vkms_output_init(vkmsdev);
}
static int vkms_create(struct vkms_config *config)
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index e549c9523a34..b5740c27180b 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -5,6 +5,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_blend.h>
+#include <drm/drm_plane.h>
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
@@ -222,12 +223,12 @@ static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
};
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type, int possible_crtc_index)
+ enum drm_plane_type type)
{
struct drm_device *dev = &vkmsdev->drm;
struct vkms_plane *plane;
- plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << possible_crtc_index,
+ plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
&vkms_plane_funcs,
vkms_formats, ARRAY_SIZE(vkms_formats),
NULL, type, NULL);
diff --git a/drivers/gpu/drm/vkms/vkms_plane.h b/drivers/gpu/drm/vkms/vkms_plane.h
index 90554c9fe250..95b2428331b8 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.h
+++ b/drivers/gpu/drm/vkms/vkms_plane.h
@@ -52,11 +52,9 @@ struct vkms_frame_info {
*
* @vkmsdev: vkms device containing the plane
* @type: type of plane to initialize
- * @possible_crtc_index: Crtc which can be attached to the plane. The caller must ensure that
- * possible_crtc_index is positive and less or equals to 31.
*/
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type, int possible_crtc_index);
+ enum drm_plane_type type);
#define drm_plane_state_to_vkms_plane_state(target) \
container_of(target, struct vkms_plane_state, base.base)
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 02/15] drm/vkms: remove possible crtc from parameters
2024-08-14 14:36 ` [PATCH RFC 02/15] drm/vkms: remove possible crtc from parameters Louis Chauvet
@ 2024-09-05 12:33 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:33 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, linux-kernel, maarten.lankhorst, mairacanal,
melissa.srw, miquel.raynal, mripard, nicolejadeyee,
rodrigosiqueiramelo, seanpaul, thomas.petazzoni, tzimmermann
> As the crtc mask is dynamic, avoid hardcoding it. It is already computed
> once all the planes are created, so it should be a no-op
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 10 +++++-----
> drivers/gpu/drm/vkms/vkms_plane.c | 5 +++--
> drivers/gpu/drm/vkms/vkms_plane.h | 4 +---
> 3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 7ac3ab7e16e5..e71b45fcb9b8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -166,7 +166,7 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc)
> +static int vkms_output_init(struct vkms_device *vkmsdev)
> {
> struct drm_device *dev = &vkmsdev->drm;
> struct drm_connector *connector;
> @@ -184,20 +184,20 @@ static int vkms_output_init(struct vkms_device *vkmsdev, int possible_crtc)
> * The overlay and cursor planes are not mandatory, but can be used to perform complex
> * composition.
> */
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, possible_crtc);
> + primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> if (vkmsdev->config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, possible_crtc);
> + overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY);
> if (IS_ERR(overlay))
> return PTR_ERR(overlay);
> }
> }
>
> if (vkmsdev->config->cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, possible_crtc);
> + cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> if (IS_ERR(cursor))
> return PTR_ERR(cursor);
> }
> @@ -284,7 +284,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> dev->mode_config.preferred_depth = 0;
> dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
> - return vkms_output_init(vkmsdev, 0);
> + return vkms_output_init(vkmsdev);
> }
>
> static int vkms_create(struct vkms_config *config)
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index e549c9523a34..b5740c27180b 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -5,6 +5,7 @@
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_blend.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_fourcc.h>
> #include <drm/drm_gem_atomic_helper.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> @@ -222,12 +223,12 @@ static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
> };
>
> struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type, int possible_crtc_index)
> + enum drm_plane_type type)
> {
> struct drm_device *dev = &vkmsdev->drm;
> struct vkms_plane *plane;
>
> - plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << possible_crtc_index,
> + plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
Shouldn't the index be BIT(0)? I see in the commit message that
possible_crtcs is computed once all planes are added, but I
couldn't find where. It'd be nice if you could point to the
function that sets the correct CRTC mask in the commit message.
> &vkms_plane_funcs,
> vkms_formats, ARRAY_SIZE(vkms_formats),
> NULL, type, NULL);
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.h b/drivers/gpu/drm/vkms/vkms_plane.h
> index 90554c9fe250..95b2428331b8 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.h
> +++ b/drivers/gpu/drm/vkms/vkms_plane.h
> @@ -52,11 +52,9 @@ struct vkms_frame_info {
> *
> * @vkmsdev: vkms device containing the plane
> * @type: type of plane to initialize
> - * @possible_crtc_index: Crtc which can be attached to the plane. The caller must ensure that
> - * possible_crtc_index is positive and less or equals to 31.
> */
> struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type, int possible_crtc_index);
> + enum drm_plane_type type);
>
> #define drm_plane_state_to_vkms_plane_state(target) \
> container_of(target, struct vkms_plane_state, base.base)
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 03/15] drm/vkms: Extract vkms_config header
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 01/15] drm/vkms: Remove useles devres group Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 02/15] drm/vkms: remove possible crtc from parameters Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:33 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 04/15] drm/vkms: Add a validation function for vkms configuration Louis Chauvet
` (11 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet, José Expósito
Creating a new vkms_config structure will be more complex once we
start adding more options.
Extract the vkms_config structure to its own header and source files
and add functions to create and delete a vkms_config and to initialize
debugfs.
Refactor, no functional changes.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
[Changes: Cherry picked and conflict solve]
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
drivers/gpu/drm/vkms/Makefile | 3 ++-
drivers/gpu/drm/vkms/vkms_config.c | 45 ++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_config.h | 32 +++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_drv.c | 41 ++++++++++------------------------
drivers/gpu/drm/vkms/vkms_drv.h | 15 +------------
5 files changed, 91 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 8d3e46dde635..2b6db5870b97 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -6,7 +6,8 @@ vkms-y := \
vkms_formats.o \
vkms_crtc.o \
vkms_composer.o \
- vkms_writeback.o
+ vkms_writeback.o \
+ vkms_config.o
obj-$(CONFIG_DRM_VKMS) += vkms.o
obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
new file mode 100644
index 000000000000..ad5d814e6e83
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <drm/drm_debugfs.h>
+
+#include "vkms_config.h"
+#include "vkms_drv.h"
+
+struct vkms_config *vkms_config_create(void)
+{
+ struct vkms_config *config;
+
+ config = kzalloc(sizeof(*config), GFP_KERNEL);
+ if (!config)
+ return ERR_PTR(-ENOMEM);
+
+ return config;
+}
+
+void vkms_config_destroy(struct vkms_config *config)
+{
+ kfree(config);
+}
+
+static int vkms_config_show(struct seq_file *m, void *data)
+{
+ struct drm_debugfs_entry *entry = m->private;
+ struct drm_device *dev = entry->dev;
+ struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
+
+ seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
+ seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
+ seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
+
+ return 0;
+}
+
+static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
+ { "vkms_config", vkms_config_show, 0 },
+};
+
+void vkms_config_register_debugfs(struct vkms_device *vkms_device)
+{
+ drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
+ ARRAY_SIZE(vkms_config_debugfs_list));
+}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
new file mode 100644
index 000000000000..b28483173874
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#ifndef _VKMS_CONFIG_H
+#define _VKMS_CONFIG_H
+
+#include <linux/types.h>
+#include "vkms_drv.h"
+
+/**
+ * struct vkms_config - General configuration for VKMS driver
+ *
+ * @writeback: If true, a writeback buffer can be attached to the CRTC
+ * @cursor: If true, a cursor plane is created in the VKMS device
+ * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
+ * @dev: Used to store the current vkms device. Only set when the device is instancied.
+ */
+struct vkms_config {
+ bool writeback;
+ bool cursor;
+ bool overlay;
+ struct vkms_device *dev;
+};
+
+/**
+ * vkms_config_register_debugfs() - Register the debugfs file to display current configuration
+ */
+void vkms_config_register_debugfs(struct vkms_device *vkms_device);
+
+struct vkms_config *vkms_config_create(void);
+void vkms_config_destroy(struct vkms_config *config);
+
+#endif //_VKMS_CONFIG_H
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index e71b45fcb9b8..dbdb1f565a8f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -28,6 +28,7 @@
#include "vkms_drv.h"
#include "vkms_crtc.h"
+#include "vkms_config.h"
#include <drm/drm_print.h>
#include <drm/drm_debugfs.h>
@@ -85,22 +86,6 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
drm_atomic_helper_cleanup_planes(dev, old_state);
}
-static int vkms_config_show(struct seq_file *m, void *data)
-{
- struct drm_debugfs_entry *entry = m->private;
- struct drm_device *dev = entry->dev;
- struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
-
- seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
- seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
- seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
-
- return 0;
-}
-
-static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
- { "vkms_config", vkms_config_show, 0 },
-};
static const struct drm_driver vkms_driver = {
.driver_features = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
@@ -325,8 +310,7 @@ static int vkms_create(struct vkms_config *config)
if (ret)
goto out_unregister;
- drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
- ARRAY_SIZE(vkms_config_debugfs_list));
+ vkms_config_register_debugfs(vkms_device);
ret = drm_dev_register(&vkms_device->drm, 0);
if (ret)
@@ -344,21 +328,18 @@ static int vkms_create(struct vkms_config *config)
static int __init vkms_init(void)
{
int ret;
- struct vkms_config *config;
-
- config = kmalloc(sizeof(*config), GFP_KERNEL);
- if (!config)
- return -ENOMEM;
- default_config = config;
+ default_config = vkms_config_create();
+ if (IS_ERR(default_config))
+ return PTR_ERR(default_config);
- config->cursor = enable_cursor;
- config->writeback = enable_writeback;
- config->overlay = enable_overlay;
+ default_config->cursor = enable_cursor;
+ default_config->writeback = enable_writeback;
+ default_config->overlay = enable_overlay;
- ret = vkms_create(config);
+ ret = vkms_create(default_config);
if (ret)
- kfree(config);
+ vkms_config_destroy(default_config);
return ret;
}
@@ -386,7 +367,7 @@ static void __exit vkms_exit(void)
if (default_config->dev)
vkms_destroy(default_config);
- kfree(default_config);
+ vkms_config_destroy(default_config);
}
module_init(vkms_init);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 08d0ef106e37..64574695f655 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -25,20 +25,7 @@
#define VKMS_LUT_SIZE 256
-/**
- * struct vkms_config - General configuration for VKMS driver
- *
- * @writeback: If true, a writeback buffer can be attached to the CRTC
- * @cursor: If true, a cursor plane is created in the VKMS device
- * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
- * @dev: Used to store the current vkms device. Only set when the device is instancied.
- */
-struct vkms_config {
- bool writeback;
- bool cursor;
- bool overlay;
- struct vkms_device *dev;
-};
+struct vkms_config;
/**
* struct vkms_device - Description of a vkms device
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 03/15] drm/vkms: Extract vkms_config header
2024-08-14 14:36 ` [PATCH RFC 03/15] drm/vkms: Extract vkms_config header Louis Chauvet
@ 2024-09-05 12:33 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:33 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, jose.exposito89, linux-kernel,
maarten.lankhorst, mairacanal, melissa.srw, miquel.raynal,
mripard, nicolejadeyee, rodrigosiqueiramelo, seanpaul,
thomas.petazzoni, tzimmermann
> Creating a new vkms_config structure will be more complex once we
> start adding more options.
>
> Extract the vkms_config structure to its own header and source files
> and add functions to create and delete a vkms_config and to initialize
> debugfs.
>
> Refactor, no functional changes.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> [Changes: Cherry picked and conflict solve]
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
> drivers/gpu/drm/vkms/Makefile | 3 ++-
> drivers/gpu/drm/vkms/vkms_config.c | 45 ++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_config.h | 32 +++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_drv.c | 41 ++++++++++------------------------
> drivers/gpu/drm/vkms/vkms_drv.h | 15 +------------
> 5 files changed, 91 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 8d3e46dde635..2b6db5870b97 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -6,7 +6,8 @@ vkms-y := \
> vkms_formats.o \
> vkms_crtc.o \
> vkms_composer.o \
> - vkms_writeback.o
> + vkms_writeback.o \
> + vkms_config.o
>
> obj-$(CONFIG_DRM_VKMS) += vkms.o
> obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> new file mode 100644
> index 000000000000..ad5d814e6e83
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -0,0 +1,45 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <drm/drm_debugfs.h>
> +
> +#include "vkms_config.h"
> +#include "vkms_drv.h"
I believe there are some missing includes. In the patch of my
configfs series equivalent to this one I have:
#include <linux/slab.h>
#include <drm/drm_print.h>
#include <drm/drm_debugfs.h>
#include "vkms_config.h"
#include "vkms_drv.h"
> +
> +struct vkms_config *vkms_config_create(void)
> +{
> + struct vkms_config *config;
> +
> + config = kzalloc(sizeof(*config), GFP_KERNEL);
> + if (!config)
> + return ERR_PTR(-ENOMEM);
> +
> + return config;
> +}
> +
> +void vkms_config_destroy(struct vkms_config *config)
> +{
> + kfree(config);
> +}
> +
> +static int vkms_config_show(struct seq_file *m, void *data)
> +{
> + struct drm_debugfs_entry *entry = m->private;
> + struct drm_device *dev = entry->dev;
> + struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> +
> + seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> + seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
> + seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
> +
> + return 0;
> +}
> +
> +static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
> + { "vkms_config", vkms_config_show, 0 },
> +};
> +
> +void vkms_config_register_debugfs(struct vkms_device *vkms_device)
> +{
> + drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
> + ARRAY_SIZE(vkms_config_debugfs_list));
> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> new file mode 100644
> index 000000000000..b28483173874
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _VKMS_CONFIG_H
> +#define _VKMS_CONFIG_H
#ifndef _VKMS_CONFIG_H_
#define _VKMS_CONFIG_H_
> +
> +#include <linux/types.h>
> +#include "vkms_drv.h"
You can avoid this import by forward declaring "vkms_device":
struct vkms_device;
> +
> +/**
> + * struct vkms_config - General configuration for VKMS driver
> + *
> + * @writeback: If true, a writeback buffer can be attached to the CRTC
> + * @cursor: If true, a cursor plane is created in the VKMS device
> + * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
> + * @dev: Used to store the current vkms device. Only set when the device is instancied.
> + */
> +struct vkms_config {
> + bool writeback;
> + bool cursor;
> + bool overlay;
> + struct vkms_device *dev;
> +};
> +
> +/**
> + * vkms_config_register_debugfs() - Register the debugfs file to display current configuration
> + */
> +void vkms_config_register_debugfs(struct vkms_device *vkms_device);
> +
> +struct vkms_config *vkms_config_create(void);
> +void vkms_config_destroy(struct vkms_config *config);
> +
> +#endif //_VKMS_CONFIG_H
#endif /* _VKMS_CONFIG_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index e71b45fcb9b8..dbdb1f565a8f 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -28,6 +28,7 @@
>
> #include "vkms_drv.h"
> #include "vkms_crtc.h"
> +#include "vkms_config.h"
>
> #include <drm/drm_print.h>
> #include <drm/drm_debugfs.h>
And here you can drop:
#include <drm/drm_print.h>
#include <drm/drm_debugfs.h>
> @@ -85,22 +86,6 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> drm_atomic_helper_cleanup_planes(dev, old_state);
> }
>
> -static int vkms_config_show(struct seq_file *m, void *data)
> -{
> - struct drm_debugfs_entry *entry = m->private;
> - struct drm_device *dev = entry->dev;
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> -
> - seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> - seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
> - seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
> -
> - return 0;
> -}
> -
> -static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
> - { "vkms_config", vkms_config_show, 0 },
> -};
>
> static const struct drm_driver vkms_driver = {
> .driver_features = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> @@ -325,8 +310,7 @@ static int vkms_create(struct vkms_config *config)
> if (ret)
> goto out_unregister;
>
> - drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
> - ARRAY_SIZE(vkms_config_debugfs_list));
> + vkms_config_register_debugfs(vkms_device);
>
> ret = drm_dev_register(&vkms_device->drm, 0);
> if (ret)
> @@ -344,21 +328,18 @@ static int vkms_create(struct vkms_config *config)
> static int __init vkms_init(void)
> {
> int ret;
> - struct vkms_config *config;
> -
> - config = kmalloc(sizeof(*config), GFP_KERNEL);
> - if (!config)
> - return -ENOMEM;
>
> - default_config = config;
> + default_config = vkms_config_create();
> + if (IS_ERR(default_config))
> + return PTR_ERR(default_config);
>
> - config->cursor = enable_cursor;
> - config->writeback = enable_writeback;
> - config->overlay = enable_overlay;
> + default_config->cursor = enable_cursor;
> + default_config->writeback = enable_writeback;
> + default_config->overlay = enable_overlay;
>
> - ret = vkms_create(config);
> + ret = vkms_create(default_config);
> if (ret)
> - kfree(config);
> + vkms_config_destroy(default_config);
>
> return ret;
> }
> @@ -386,7 +367,7 @@ static void __exit vkms_exit(void)
> if (default_config->dev)
> vkms_destroy(default_config);
>
> - kfree(default_config);
> + vkms_config_destroy(default_config);
> }
>
> module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 08d0ef106e37..64574695f655 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -25,20 +25,7 @@
>
> #define VKMS_LUT_SIZE 256
>
> -/**
> - * struct vkms_config - General configuration for VKMS driver
> - *
> - * @writeback: If true, a writeback buffer can be attached to the CRTC
> - * @cursor: If true, a cursor plane is created in the VKMS device
> - * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
> - * @dev: Used to store the current vkms device. Only set when the device is instancied.
> - */
> -struct vkms_config {
> - bool writeback;
> - bool cursor;
> - bool overlay;
> - struct vkms_device *dev;
> -};
> +struct vkms_config;
>
> /**
> * struct vkms_device - Description of a vkms device
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 04/15] drm/vkms: Add a validation function for vkms configuration
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (2 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 03/15] drm/vkms: Extract vkms_config header Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:33 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 05/15] drm/vkms: Move default_config creation to its own function Louis Chauvet
` (10 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As the configuration will be used by userspace, add a validator to avoid
creating a broken DRM device
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 5 +++++
drivers/gpu/drm/vkms/vkms_config.h | 14 ++++++++++++++
2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index ad5d814e6e83..d8348af9587e 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -21,6 +21,11 @@ void vkms_config_destroy(struct vkms_config *config)
kfree(config);
}
+bool vkms_config_is_valid(struct vkms_config *config)
+{
+ return true;
+}
+
static int vkms_config_show(struct seq_file *m, void *data)
{
struct drm_debugfs_entry *entry = m->private;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index b28483173874..363f5bc8f64b 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -29,4 +29,18 @@ void vkms_config_register_debugfs(struct vkms_device *vkms_device);
struct vkms_config *vkms_config_create(void);
void vkms_config_destroy(struct vkms_config *config);
+/**
+ * vkms_config_is_valid() - Validate a configuration
+ *
+ * Check if all the property defined in the configuration are valids. This will return false for
+ * example if:
+ * - no or many primary planes are present;
+ * - the default rotation of a plane is not in its supported rotation;
+ * - a CRTC don't have any encoder...
+ *
+ * @vkms_config: Configuration to validate
+ */
+bool vkms_config_is_valid(struct vkms_config *vkms_config);
+
+
#endif //_VKMS_CONFIG_H
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 04/15] drm/vkms: Add a validation function for vkms configuration
2024-08-14 14:36 ` [PATCH RFC 04/15] drm/vkms: Add a validation function for vkms configuration Louis Chauvet
@ 2024-09-05 12:33 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:33 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, linux-kernel, maarten.lankhorst, mairacanal,
melissa.srw, miquel.raynal, mripard, nicolejadeyee,
rodrigosiqueiramelo, seanpaul, thomas.petazzoni, tzimmermann
> As the configuration will be used by userspace, add a validator to avoid
> creating a broken DRM device
This is something I considered to include in my configfs series, however,
I'm not sure if the set of rules used to validate a device configuration
are already implemented somewhere in the DRM core and we could try to
expose them to avoid duplicating them.
I'd be nice if someone with a better understanding of the DRM core code
could give us some pointers about the best way to implemet these validation
rules.
Also, if we end up implemeting the rules in VKMS, it'd be nice to print
to dmesg some information so the user can figure out what and why failed.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_config.c | 5 +++++
> drivers/gpu/drm/vkms/vkms_config.h | 14 ++++++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index ad5d814e6e83..d8348af9587e 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -21,6 +21,11 @@ void vkms_config_destroy(struct vkms_config *config)
> kfree(config);
> }
>
> +bool vkms_config_is_valid(struct vkms_config *config)
> +{
> + return true;
> +}
> +
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> struct drm_debugfs_entry *entry = m->private;
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index b28483173874..363f5bc8f64b 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -29,4 +29,18 @@ void vkms_config_register_debugfs(struct vkms_device *vkms_device);
> struct vkms_config *vkms_config_create(void);
> void vkms_config_destroy(struct vkms_config *config);
>
> +/**
> + * vkms_config_is_valid() - Validate a configuration
> + *
> + * Check if all the property defined in the configuration are valids. This will return false for
> + * example if:
> + * - no or many primary planes are present;
> + * - the default rotation of a plane is not in its supported rotation;
> + * - a CRTC don't have any encoder...
> + *
> + * @vkms_config: Configuration to validate
> + */
> +bool vkms_config_is_valid(struct vkms_config *vkms_config);
> +
> +
> #endif //_VKMS_CONFIG_H
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 05/15] drm/vkms: Move default_config creation to its own function
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (3 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 04/15] drm/vkms: Add a validation function for vkms configuration Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 06/15] drm/vkms: Introduce plane configuration Louis Chauvet
` (9 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet, José Expósito
Extract the initialization of the default configuration to a function.
Refactor, no functional changes.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
[Changes: Cherry pick and solve conflicts]
Signed-off-by: José Expósito <jose.exposito89@gmail.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 16 ++++++++++++++++
drivers/gpu/drm/vkms/vkms_config.h | 9 +++++++++
drivers/gpu/drm/vkms/vkms_drv.c | 7 +------
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index d8348af9587e..27b49a2ad9c8 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -16,6 +16,22 @@ struct vkms_config *vkms_config_create(void)
return config;
}
+struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable_overlay,
+ bool enable_cursor)
+{
+ struct vkms_config *vkms_config = vkms_config_create();
+
+ if (IS_ERR(vkms_config))
+ return vkms_config;
+
+ vkms_config->writeback = enable_writeback;
+ vkms_config->overlay = enable_overlay;
+ vkms_config->cursor = enable_cursor;
+
+ return vkms_config;
+}
+
+
void vkms_config_destroy(struct vkms_config *config)
{
kfree(config);
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 363f5bc8f64b..50c3b021a66b 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -43,4 +43,13 @@ void vkms_config_destroy(struct vkms_config *config);
bool vkms_config_is_valid(struct vkms_config *vkms_config);
+/**
+ * vkms_config_alloc_default() - Allocate the configuration for the default device
+ * @enable_writeback: Enable the writeback connector for this configuration
+ * @enable_overlay: Create some overlay planes
+ * @enable_cursor: Create a cursor plane
+ */
+struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable_overlay,
+ bool enable_cursor);
+
#endif //_VKMS_CONFIG_H
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index dbdb1f565a8f..af237daa3c5c 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -328,15 +328,10 @@ static int vkms_create(struct vkms_config *config)
static int __init vkms_init(void)
{
int ret;
-
- default_config = vkms_config_create();
+ default_config = vkms_config_alloc_default(enable_writeback, enable_overlay, enable_cursor);
if (IS_ERR(default_config))
return PTR_ERR(default_config);
- default_config->cursor = enable_cursor;
- default_config->writeback = enable_writeback;
- default_config->overlay = enable_overlay;
-
ret = vkms_create(default_config);
if (ret)
vkms_config_destroy(default_config);
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 06/15] drm/vkms: Introduce plane configuration
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (4 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 05/15] drm/vkms: Move default_config creation to its own function Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:33 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 07/15] drm/vkms: Introduce plane name configuration Louis Chauvet
` (8 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
The current vkms driver only allows the usage of one primary, eight
overlays and one cursor plane. This new configuration structure aims to
make the configuration more flexible.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 90 ++++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/vkms/vkms_config.h | 44 +++++++++++++++++--
drivers/gpu/drm/vkms/vkms_drv.c | 33 +++++---------
drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
drivers/gpu/drm/vkms/vkms_plane.h | 3 +-
5 files changed, 141 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 27b49a2ad9c8..b1c6160d350f 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -13,32 +13,111 @@ struct vkms_config *vkms_config_create(void)
if (!config)
return ERR_PTR(-ENOMEM);
+ INIT_LIST_HEAD(&config->planes);
+
return config;
}
struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable_overlay,
bool enable_cursor)
{
+ struct vkms_config_plane *plane;
struct vkms_config *vkms_config = vkms_config_create();
if (IS_ERR(vkms_config))
return vkms_config;
vkms_config->writeback = enable_writeback;
- vkms_config->overlay = enable_overlay;
- vkms_config->cursor = enable_cursor;
+
+ plane = vkms_config_create_plane(vkms_config);
+ if (!plane)
+ goto err_alloc;
+
+ plane->type = DRM_PLANE_TYPE_PRIMARY;
+
+ if (enable_overlay) {
+ for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
+ plane = vkms_config_create_plane(vkms_config);
+ if (!plane)
+ goto err_alloc;
+ plane->type = DRM_PLANE_TYPE_OVERLAY;
+ }
+ }
+ if (enable_cursor) {
+ plane = vkms_config_create_plane(vkms_config);
+ if (!plane)
+ goto err_alloc;
+ plane->type = DRM_PLANE_TYPE_CURSOR;
+ }
return vkms_config;
+err_alloc:
+ vkms_config_destroy(vkms_config);
+ return ERR_PTR(-ENOMEM);
}
+struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config)
+{
+ if (!vkms_config)
+ return NULL;
+
+ struct vkms_config_plane *vkms_config_overlay = kzalloc(sizeof(*vkms_config_overlay),
+ GFP_KERNEL);
+
+ if (!vkms_config_overlay)
+ return NULL;
+
+ vkms_config_overlay->type = DRM_PLANE_TYPE_OVERLAY;
+
+ list_add(&vkms_config_overlay->link, &vkms_config->planes);
+
+ return vkms_config_overlay;
+}
+
+void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
+{
+ if (!vkms_config_overlay)
+ return;
+ list_del(&vkms_config_overlay->link);
+ kfree(vkms_config_overlay);
+}
void vkms_config_destroy(struct vkms_config *config)
{
+ struct vkms_config_plane *vkms_config_plane, *tmp_plane;
+
+ list_for_each_entry_safe(vkms_config_plane, tmp_plane, &config->planes, link) {
+ vkms_config_delete_plane(vkms_config_plane);
+ }
+
kfree(config);
}
bool vkms_config_is_valid(struct vkms_config *config)
{
+ struct vkms_config_plane *config_plane;
+
+ bool has_cursor = false;
+ bool has_primary = false;
+
+ list_for_each_entry(config_plane, &config->planes, link) {
+ if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
+ // Multiple primary planes for only one CRTC
+ if (has_primary)
+ return false;
+ has_primary = true;
+ }
+ if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
+ // Multiple cursor planes for only one CRTC
+ if (has_cursor)
+ return false;
+ has_cursor = true;
+ }
+ }
+
+ if (!has_primary)
+ return false;
+
return true;
}
@@ -47,10 +126,13 @@ static int vkms_config_show(struct seq_file *m, void *data)
struct drm_debugfs_entry *entry = m->private;
struct drm_device *dev = entry->dev;
struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
+ struct vkms_config_plane *config_plane;
seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
- seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
- seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
+ list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
+ seq_puts(m, "plane:\n");
+ seq_printf(m, "\ttype: %d\n", config_plane->type);
+ }
return 0;
}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 50c3b021a66b..77c1c3934189 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -10,15 +10,34 @@
* struct vkms_config - General configuration for VKMS driver
*
* @writeback: If true, a writeback buffer can be attached to the CRTC
- * @cursor: If true, a cursor plane is created in the VKMS device
- * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
+ * @planes: List of planes configured for this device. They are created by the function
+ * vkms_config_create_plane().
* @dev: Used to store the current vkms device. Only set when the device is instancied.
*/
struct vkms_config {
bool writeback;
- bool cursor;
- bool overlay;
struct vkms_device *dev;
+
+ struct list_head planes;
+};
+
+/**
+ * struct vkms_config_plane
+ *
+ * @link: Link to the others planes
+ * @type: Type of the plane. The creator of configuration needs to ensures that at least one
+ * plane is primary.
+ * @plane: Internal usage. This pointer should never be considered as valid. It can be used to
+ * store a temporary reference to a vkms plane during device creation. This pointer is
+ * not managed by the configuration and must be managed by other means.
+ */
+struct vkms_config_plane {
+ struct list_head link;
+
+ enum drm_plane_type type;
+
+ /* Internal usage */
+ struct vkms_plane *plane;
};
/**
@@ -42,6 +61,23 @@ void vkms_config_destroy(struct vkms_config *config);
*/
bool vkms_config_is_valid(struct vkms_config *vkms_config);
+/**
+ * vkms_config_create_plane() - Create a plane configuration
+ *
+ * This will allocate and add a new plane to @vkms_config. This plane will have by default the
+ * maximum supported values.
+ * @vkms_config: Configuration where to insert new plane
+ */
+struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config);
+
+/**
+ * vkms_config_delete_plane() - Remove a plane configuration and frees its memory
+ *
+ * This will delete a plane configuration from the parent configuration. This will NOT
+ * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane.
+ * @vkms_config_plane: Plane configuration to cleanup
+ */
+void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane);
/**
* vkms_config_alloc_default() - Allocate the configuration for the default device
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index af237daa3c5c..403006a0bb61 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -153,38 +153,27 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
static int vkms_output_init(struct vkms_device *vkmsdev)
{
+ struct vkms_config_plane *config_plane;
struct drm_device *dev = &vkmsdev->drm;
struct drm_connector *connector;
struct drm_encoder *encoder;
struct vkms_crtc *crtc;
struct drm_plane *plane;
- struct vkms_plane *primary, *cursor, *overlay = NULL;
+ struct vkms_plane *primary, *cursor = NULL;
int ret;
int writeback;
- unsigned int n;
- /*
- * Initialize used plane. One primary plane is required to perform the composition.
- *
- * The overlay and cursor planes are not mandatory, but can be used to perform complex
- * composition.
- */
- primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
- if (IS_ERR(primary))
- return PTR_ERR(primary);
-
- if (vkmsdev->config->overlay) {
- for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
- overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY);
- if (IS_ERR(overlay))
- return PTR_ERR(overlay);
+ list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
+ config_plane->plane = vkms_plane_init(vkmsdev, config_plane);
+ if (IS_ERR(config_plane->plane)) {
+ ret = PTR_ERR(config_plane->plane);
+ return ret;
}
- }
- if (vkmsdev->config->cursor) {
- cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
- if (IS_ERR(cursor))
- return PTR_ERR(cursor);
+ if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
+ primary = config_plane->plane;
+ else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
+ cursor = config_plane->plane;
}
/* [1]: Initialize the crtc component */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index b5740c27180b..dc9bccf60071 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -223,7 +223,7 @@ static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
};
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type)
+ struct vkms_config_plane *config)
{
struct drm_device *dev = &vkmsdev->drm;
struct vkms_plane *plane;
@@ -231,7 +231,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
&vkms_plane_funcs,
vkms_formats, ARRAY_SIZE(vkms_formats),
- NULL, type, NULL);
+ NULL, config->type, NULL);
if (IS_ERR(plane))
return plane;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.h b/drivers/gpu/drm/vkms/vkms_plane.h
index 95b2428331b8..82049966ee7e 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.h
+++ b/drivers/gpu/drm/vkms/vkms_plane.h
@@ -9,6 +9,7 @@
#include <linux/iosys-map.h>
#include "vkms_formats.h"
+#include "vkms_config.h"
struct vkms_plane {
struct drm_plane base;
@@ -54,7 +55,7 @@ struct vkms_frame_info {
* @type: type of plane to initialize
*/
struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
- enum drm_plane_type type);
+ struct vkms_config_plane *config);
#define drm_plane_state_to_vkms_plane_state(target) \
container_of(target, struct vkms_plane_state, base.base)
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 06/15] drm/vkms: Introduce plane configuration
2024-08-14 14:36 ` [PATCH RFC 06/15] drm/vkms: Introduce plane configuration Louis Chauvet
@ 2024-09-05 12:33 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:33 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, linux-kernel, maarten.lankhorst, mairacanal,
melissa.srw, miquel.raynal, mripard, nicolejadeyee,
rodrigosiqueiramelo, seanpaul, thomas.petazzoni, tzimmermann
> The current vkms driver only allows the usage of one primary, eight
> overlays and one cursor plane. This new configuration structure aims to
> make the configuration more flexible.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_config.c | 90 ++++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/vkms/vkms_config.h | 44 +++++++++++++++++--
> drivers/gpu/drm/vkms/vkms_drv.c | 33 +++++---------
> drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
> drivers/gpu/drm/vkms/vkms_plane.h | 3 +-
> 5 files changed, 141 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index 27b49a2ad9c8..b1c6160d350f 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -13,32 +13,111 @@ struct vkms_config *vkms_config_create(void)
> if (!config)
> return ERR_PTR(-ENOMEM);
>
> + INIT_LIST_HEAD(&config->planes);
> +
> return config;
> }
>
> struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable_overlay,
> bool enable_cursor)
> {
> + struct vkms_config_plane *plane;
> struct vkms_config *vkms_config = vkms_config_create();
>
> if (IS_ERR(vkms_config))
> return vkms_config;
>
> vkms_config->writeback = enable_writeback;
> - vkms_config->overlay = enable_overlay;
> - vkms_config->cursor = enable_cursor;
> +
> + plane = vkms_config_create_plane(vkms_config);
> + if (!plane)
> + goto err_alloc;
> +
> + plane->type = DRM_PLANE_TYPE_PRIMARY;
Since all planes need to have a type, I think it'd make sense
to make "type" a param of vkms_config_create_plane().
> +
> + if (enable_overlay) {
> + for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
> + plane = vkms_config_create_plane(vkms_config);
> + if (!plane)
> + goto err_alloc;
> + plane->type = DRM_PLANE_TYPE_OVERLAY;
> + }
> + }
> + if (enable_cursor) {
> + plane = vkms_config_create_plane(vkms_config);
> + if (!plane)
> + goto err_alloc;
> + plane->type = DRM_PLANE_TYPE_CURSOR;
> + }
>
> return vkms_config;
> +err_alloc:
> + vkms_config_destroy(vkms_config);
> + return ERR_PTR(-ENOMEM);
> }
>
> +struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config)
> +{
> + if (!vkms_config)
> + return NULL;
> +
> + struct vkms_config_plane *vkms_config_overlay = kzalloc(sizeof(*vkms_config_overlay),
> + GFP_KERNEL);
Nit: I guess the variable is named "vkms_config_overlay" due to a rebase.
As "struct vkms_config_plane" is used to configure any plane, it'd be
great to rename the variable to reflect it.
> +
> + if (!vkms_config_overlay)
> + return NULL;
> +
> + vkms_config_overlay->type = DRM_PLANE_TYPE_OVERLAY;
> +
> + list_add(&vkms_config_overlay->link, &vkms_config->planes);
> +
> + return vkms_config_overlay;
> +}
> +
> +void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
> +{
> + if (!vkms_config_overlay)
> + return;
> + list_del(&vkms_config_overlay->link);
> + kfree(vkms_config_overlay);
> +}
>
> void vkms_config_destroy(struct vkms_config *config)
> {
> + struct vkms_config_plane *vkms_config_plane, *tmp_plane;
> +
> + list_for_each_entry_safe(vkms_config_plane, tmp_plane, &config->planes, link) {
> + vkms_config_delete_plane(vkms_config_plane);
> + }
You can drop the extra curly brackets here.
> +
> kfree(config);
> }
>
> bool vkms_config_is_valid(struct vkms_config *config)
> {
> + struct vkms_config_plane *config_plane;
> +
> + bool has_cursor = false;
> + bool has_primary = false;
> +
> + list_for_each_entry(config_plane, &config->planes, link) {
> + if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + // Multiple primary planes for only one CRTC
> + if (has_primary)
> + return false;
> + has_primary = true;
> + }
> + if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
> + // Multiple cursor planes for only one CRTC
> + if (has_cursor)
> + return false;
> + has_cursor = true;
> + }
> + }
> +
> + if (!has_primary)
> + return false;
> +
> return true;
> }
>
> @@ -47,10 +126,13 @@ static int vkms_config_show(struct seq_file *m, void *data)
> struct drm_debugfs_entry *entry = m->private;
> struct drm_device *dev = entry->dev;
> struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> + struct vkms_config_plane *config_plane;
>
> seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> - seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
> - seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
> + list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
> + seq_puts(m, "plane:\n");
> + seq_printf(m, "\ttype: %d\n", config_plane->type);
> + }
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 50c3b021a66b..77c1c3934189 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -10,15 +10,34 @@
> * struct vkms_config - General configuration for VKMS driver
> *
> * @writeback: If true, a writeback buffer can be attached to the CRTC
> - * @cursor: If true, a cursor plane is created in the VKMS device
> - * @overlay: If true, NUM_OVERLAY_PLANES will be created for the VKMS device
> + * @planes: List of planes configured for this device. They are created by the function
> + * vkms_config_create_plane().
> * @dev: Used to store the current vkms device. Only set when the device is instancied.
> */
> struct vkms_config {
> bool writeback;
> - bool cursor;
> - bool overlay;
> struct vkms_device *dev;
> +
> + struct list_head planes;
> +};
> +
> +/**
> + * struct vkms_config_plane
> + *
> + * @link: Link to the others planes
> + * @type: Type of the plane. The creator of configuration needs to ensures that at least one
> + * plane is primary.
> + * @plane: Internal usage. This pointer should never be considered as valid. It can be used to
> + * store a temporary reference to a vkms plane during device creation. This pointer is
> + * not managed by the configuration and must be managed by other means.
> + */
> +struct vkms_config_plane {
> + struct list_head link;
> +
> + enum drm_plane_type type;
> +
> + /* Internal usage */
> + struct vkms_plane *plane;
> };
>
> /**
> @@ -42,6 +61,23 @@ void vkms_config_destroy(struct vkms_config *config);
> */
> bool vkms_config_is_valid(struct vkms_config *vkms_config);
>
> +/**
> + * vkms_config_create_plane() - Create a plane configuration
> + *
> + * This will allocate and add a new plane to @vkms_config. This plane will have by default the
> + * maximum supported values.
> + * @vkms_config: Configuration where to insert new plane
> + */
> +struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config);
> +
> +/**
> + * vkms_config_delete_plane() - Remove a plane configuration and frees its memory
> + *
> + * This will delete a plane configuration from the parent configuration. This will NOT
> + * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane.
> + * @vkms_config_plane: Plane configuration to cleanup
> + */
> +void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane);
>
> /**
> * vkms_config_alloc_default() - Allocate the configuration for the default device
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index af237daa3c5c..403006a0bb61 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -153,38 +153,27 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>
> static int vkms_output_init(struct vkms_device *vkmsdev)
> {
> + struct vkms_config_plane *config_plane;
> struct drm_device *dev = &vkmsdev->drm;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> struct vkms_crtc *crtc;
> struct drm_plane *plane;
> - struct vkms_plane *primary, *cursor, *overlay = NULL;
> + struct vkms_plane *primary, *cursor = NULL;
> int ret;
> int writeback;
> - unsigned int n;
>
> - /*
> - * Initialize used plane. One primary plane is required to perform the composition.
> - *
> - * The overlay and cursor planes are not mandatory, but can be used to perform complex
> - * composition.
> - */
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> - if (IS_ERR(primary))
> - return PTR_ERR(primary);
> -
> - if (vkmsdev->config->overlay) {
> - for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY);
> - if (IS_ERR(overlay))
> - return PTR_ERR(overlay);
> + list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
> + config_plane->plane = vkms_plane_init(vkmsdev, config_plane);
> + if (IS_ERR(config_plane->plane)) {
> + ret = PTR_ERR(config_plane->plane);
> + return ret;
> }
> - }
>
> - if (vkmsdev->config->cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> - if (IS_ERR(cursor))
> - return PTR_ERR(cursor);
> + if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
> + primary = config_plane->plane;
> + else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
> + cursor = config_plane->plane;
> }
>
> /* [1]: Initialize the crtc component */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index b5740c27180b..dc9bccf60071 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -223,7 +223,7 @@ static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
> };
>
> struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type)
> + struct vkms_config_plane *config)
> {
> struct drm_device *dev = &vkmsdev->drm;
> struct vkms_plane *plane;
> @@ -231,7 +231,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
> &vkms_plane_funcs,
> vkms_formats, ARRAY_SIZE(vkms_formats),
> - NULL, type, NULL);
> + NULL, config->type, NULL);
> if (IS_ERR(plane))
> return plane;
>
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.h b/drivers/gpu/drm/vkms/vkms_plane.h
> index 95b2428331b8..82049966ee7e 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.h
> +++ b/drivers/gpu/drm/vkms/vkms_plane.h
> @@ -9,6 +9,7 @@
> #include <linux/iosys-map.h>
>
> #include "vkms_formats.h"
> +#include "vkms_config.h"
>
> struct vkms_plane {
> struct drm_plane base;
> @@ -54,7 +55,7 @@ struct vkms_frame_info {
> * @type: type of plane to initialize
> */
> struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> - enum drm_plane_type type);
> + struct vkms_config_plane *config);
>
> #define drm_plane_state_to_vkms_plane_state(target) \
> container_of(target, struct vkms_plane_state, base.base)
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 07/15] drm/vkms: Introduce plane name configuration
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (5 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 06/15] drm/vkms: Introduce plane configuration Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:34 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 08/15] drm/vkms: Introduce plane rotation configuration Louis Chauvet
` (7 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As a plane will be a folder in ConfigFS, add name configuration for plane
so it will reflect the folder name.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 14 ++++++++++++++
drivers/gpu/drm/vkms/vkms_config.h | 2 ++
drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index b1c6160d350f..b8e235a22e90 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -34,6 +34,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
goto err_alloc;
plane->type = DRM_PLANE_TYPE_PRIMARY;
+ plane->name = kzalloc(sizeof("primary"), GFP_KERNEL);
+ if (!plane->name)
+ goto err_alloc;
+ sprintf(plane->name, "primary");
if (enable_overlay) {
for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
@@ -41,6 +45,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
if (!plane)
goto err_alloc;
plane->type = DRM_PLANE_TYPE_OVERLAY;
+ plane->name = kzalloc(10, GFP_KERNEL);
+ if (!plane->name)
+ goto err_alloc;
+ snprintf(plane->name, 10, "plane-%d", i);
}
}
if (enable_cursor) {
@@ -48,6 +56,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
if (!plane)
goto err_alloc;
plane->type = DRM_PLANE_TYPE_CURSOR;
+ plane->name = kzalloc(sizeof("cursor"), GFP_KERNEL);
+ if (!plane->name)
+ goto err_alloc;
+ sprintf(plane->name, "cursor");
}
return vkms_config;
@@ -79,6 +91,7 @@ void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
if (!vkms_config_overlay)
return;
list_del(&vkms_config_overlay->link);
+ kfree(vkms_config_overlay->name);
kfree(vkms_config_overlay);
}
@@ -131,6 +144,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
seq_puts(m, "plane:\n");
+ seq_printf(m, "\tname: %s\n", config_plane->name);
seq_printf(m, "\ttype: %d\n", config_plane->type);
}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 77c1c3934189..792c5e904aa1 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -25,6 +25,7 @@ struct vkms_config {
* struct vkms_config_plane
*
* @link: Link to the others planes
+ * @name: Name of the plane
* @type: Type of the plane. The creator of configuration needs to ensures that at least one
* plane is primary.
* @plane: Internal usage. This pointer should never be considered as valid. It can be used to
@@ -34,6 +35,7 @@ struct vkms_config {
struct vkms_config_plane {
struct list_head link;
+ char *name;
enum drm_plane_type type;
/* Internal usage */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index dc9bccf60071..d2b1b524499f 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -231,7 +231,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
&vkms_plane_funcs,
vkms_formats, ARRAY_SIZE(vkms_formats),
- NULL, config->type, NULL);
+ NULL, config->type, config->name);
if (IS_ERR(plane))
return plane;
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 07/15] drm/vkms: Introduce plane name configuration
2024-08-14 14:36 ` [PATCH RFC 07/15] drm/vkms: Introduce plane name configuration Louis Chauvet
@ 2024-09-05 12:34 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:34 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, linux-kernel, maarten.lankhorst, mairacanal,
melissa.srw, miquel.raynal, mripard, nicolejadeyee,
rodrigosiqueiramelo, seanpaul, thomas.petazzoni, tzimmermann
> As a plane will be a folder in ConfigFS, add name configuration for plane
> so it will reflect the folder name.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_config.c | 14 ++++++++++++++
> drivers/gpu/drm/vkms/vkms_config.h | 2 ++
> drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index b1c6160d350f..b8e235a22e90 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -34,6 +34,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> goto err_alloc;
>
> plane->type = DRM_PLANE_TYPE_PRIMARY;
> + plane->name = kzalloc(sizeof("primary"), GFP_KERNEL);
> + if (!plane->name)
> + goto err_alloc;
> + sprintf(plane->name, "primary");
I think that, since all planes will have a name, you could make the
name a function param and kmemdup() it in vkms_config_create_plane().
The same could apply for CRTCs and encoders in patches 13 and 14.
>
> if (enable_overlay) {
> for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
> @@ -41,6 +45,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> if (!plane)
> goto err_alloc;
> plane->type = DRM_PLANE_TYPE_OVERLAY;
> + plane->name = kzalloc(10, GFP_KERNEL);
> + if (!plane->name)
> + goto err_alloc;
> + snprintf(plane->name, 10, "plane-%d", i);
> }
> }
> if (enable_cursor) {
> @@ -48,6 +56,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> if (!plane)
> goto err_alloc;
> plane->type = DRM_PLANE_TYPE_CURSOR;
> + plane->name = kzalloc(sizeof("cursor"), GFP_KERNEL);
> + if (!plane->name)
> + goto err_alloc;
> + sprintf(plane->name, "cursor");
> }
>
> return vkms_config;
> @@ -79,6 +91,7 @@ void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
> if (!vkms_config_overlay)
> return;
> list_del(&vkms_config_overlay->link);
> + kfree(vkms_config_overlay->name);
> kfree(vkms_config_overlay);
> }
>
> @@ -131,6 +144,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
> seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
> seq_puts(m, "plane:\n");
> + seq_printf(m, "\tname: %s\n", config_plane->name);
> seq_printf(m, "\ttype: %d\n", config_plane->type);
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index 77c1c3934189..792c5e904aa1 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -25,6 +25,7 @@ struct vkms_config {
> * struct vkms_config_plane
> *
> * @link: Link to the others planes
> + * @name: Name of the plane
> * @type: Type of the plane. The creator of configuration needs to ensures that at least one
> * plane is primary.
> * @plane: Internal usage. This pointer should never be considered as valid. It can be used to
> @@ -34,6 +35,7 @@ struct vkms_config {
> struct vkms_config_plane {
> struct list_head link;
>
> + char *name;
> enum drm_plane_type type;
>
> /* Internal usage */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index dc9bccf60071..d2b1b524499f 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -231,7 +231,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
> &vkms_plane_funcs,
> vkms_formats, ARRAY_SIZE(vkms_formats),
> - NULL, config->type, NULL);
> + NULL, config->type, config->name);
> if (IS_ERR(plane))
> return plane;
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 08/15] drm/vkms: Introduce plane rotation configuration
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (6 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 07/15] drm/vkms: Introduce plane name configuration Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 09/15] drm/vkms: Introduce configuration for plane color encoding Louis Chauvet
` (6 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
VKMS driver supports all the rotation on planes, but for testing it can be
useful to only advertise few of them. This new configuration interface
will allow configuring the rotation per planes.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 9 +++++++++
drivers/gpu/drm/vkms/vkms_config.h | 4 ++++
drivers/gpu/drm/vkms/vkms_plane.c | 5 +++--
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index b8e235a22e90..2a24da9c0fc9 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -80,6 +80,8 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_conf
return NULL;
vkms_config_overlay->type = DRM_PLANE_TYPE_OVERLAY;
+ vkms_config_overlay->supported_rotations = DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK;
+ vkms_config_overlay->default_rotation = DRM_MODE_ROTATE_0;
list_add(&vkms_config_overlay->link, &vkms_config->planes);
@@ -114,6 +116,11 @@ bool vkms_config_is_valid(struct vkms_config *config)
bool has_primary = false;
list_for_each_entry(config_plane, &config->planes, link) {
+ // Default rotation not in supported rotations
+ if ((config_plane->default_rotation & config_plane->supported_rotations) !=
+ config_plane->default_rotation)
+ return false;
+
if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
// Multiple primary planes for only one CRTC
if (has_primary)
@@ -146,6 +153,8 @@ static int vkms_config_show(struct seq_file *m, void *data)
seq_puts(m, "plane:\n");
seq_printf(m, "\tname: %s\n", config_plane->name);
seq_printf(m, "\ttype: %d\n", config_plane->type);
+ seq_printf(m, "\tsupported rotations: 0x%x\n", config_plane->supported_rotations);
+ seq_printf(m, "\tdefault rotation: 0x%x\n", config_plane->default_rotation);
}
return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 792c5e904aa1..77a72a3a637c 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -28,6 +28,8 @@ struct vkms_config {
* @name: Name of the plane
* @type: Type of the plane. The creator of configuration needs to ensures that at least one
* plane is primary.
+ * @default_rotation: Default rotation that should be used by this plane
+ * @supported_rotation: Rotation that this plane will support
* @plane: Internal usage. This pointer should never be considered as valid. It can be used to
* store a temporary reference to a vkms plane during device creation. This pointer is
* not managed by the configuration and must be managed by other means.
@@ -37,6 +39,8 @@ struct vkms_config_plane {
char *name;
enum drm_plane_type type;
+ unsigned int default_rotation;
+ unsigned int supported_rotations;
/* Internal usage */
struct vkms_plane *plane;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index d2b1b524499f..3a2b8b0b5980 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -237,8 +237,9 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
drm_plane_helper_add(&plane->base, &vkms_plane_helper_funcs);
- drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
- DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
+ drm_plane_create_rotation_property(&plane->base,
+ config->default_rotation,
+ config->supported_rotations);
drm_plane_create_color_properties(&plane->base,
BIT(DRM_COLOR_YCBCR_BT601) |
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 09/15] drm/vkms: Introduce configuration for plane color encoding
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (7 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 08/15] drm/vkms: Introduce plane rotation configuration Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 10/15] drm/vkms: Introduce configuration for plane color range Louis Chauvet
` (5 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
VKMS driver supports all the color encoding on planes, but for testing it
can be useful to only advertise few of them. This new configuration
interface will allow configuring the color encoding per planes.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 15 +++++++++++++++
drivers/gpu/drm/vkms/vkms_config.h | 4 ++++
drivers/gpu/drm/vkms/vkms_plane.c | 6 ++----
3 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 2a24da9c0fc9..dabb8cb13314 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -82,6 +82,11 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_conf
vkms_config_overlay->type = DRM_PLANE_TYPE_OVERLAY;
vkms_config_overlay->supported_rotations = DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK;
vkms_config_overlay->default_rotation = DRM_MODE_ROTATE_0;
+ vkms_config_overlay->supported_color_encoding = BIT(DRM_COLOR_YCBCR_BT601) |
+ BIT(DRM_COLOR_YCBCR_BT709) |
+ BIT(DRM_COLOR_YCBCR_BT2020);
+ vkms_config_overlay->default_color_encoding = DRM_COLOR_YCBCR_BT601;
+
list_add(&vkms_config_overlay->link, &vkms_config->planes);
@@ -121,6 +126,12 @@ bool vkms_config_is_valid(struct vkms_config *config)
config_plane->default_rotation)
return false;
+ // Default color range not in supported color range
+ if ((BIT(config_plane->default_color_encoding) &
+ config_plane->supported_color_encoding) !=
+ BIT(config_plane->default_color_encoding))
+ return false;
+
if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
// Multiple primary planes for only one CRTC
if (has_primary)
@@ -155,6 +166,10 @@ static int vkms_config_show(struct seq_file *m, void *data)
seq_printf(m, "\ttype: %d\n", config_plane->type);
seq_printf(m, "\tsupported rotations: 0x%x\n", config_plane->supported_rotations);
seq_printf(m, "\tdefault rotation: 0x%x\n", config_plane->default_rotation);
+ seq_printf(m, "\tsupported color encoding: 0x%x\n",
+ config_plane->supported_color_encoding);
+ seq_printf(m, "\tdefault color encoding: %d\n",
+ config_plane->default_color_encoding);
}
return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 77a72a3a637c..c8802bfb9ab2 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -30,6 +30,8 @@ struct vkms_config {
* plane is primary.
* @default_rotation: Default rotation that should be used by this plane
* @supported_rotation: Rotation that this plane will support
+ * @default_color_encoding: Default color encoding that should be used by this plane
+ * @supported_color_encoding: Color encoding that this plane will support
* @plane: Internal usage. This pointer should never be considered as valid. It can be used to
* store a temporary reference to a vkms plane during device creation. This pointer is
* not managed by the configuration and must be managed by other means.
@@ -41,6 +43,8 @@ struct vkms_config_plane {
enum drm_plane_type type;
unsigned int default_rotation;
unsigned int supported_rotations;
+ enum drm_color_encoding default_color_encoding;
+ unsigned int supported_color_encoding;
/* Internal usage */
struct vkms_plane *plane;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 3a2b8b0b5980..ef20da7ccb17 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -242,12 +242,10 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
config->supported_rotations);
drm_plane_create_color_properties(&plane->base,
- BIT(DRM_COLOR_YCBCR_BT601) |
- BIT(DRM_COLOR_YCBCR_BT709) |
- BIT(DRM_COLOR_YCBCR_BT2020),
+ config->supported_color_encoding,
BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
BIT(DRM_COLOR_YCBCR_FULL_RANGE),
- DRM_COLOR_YCBCR_BT601,
+ config->default_color_encoding,
DRM_COLOR_YCBCR_FULL_RANGE);
return plane;
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 10/15] drm/vkms: Introduce configuration for plane color range
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (8 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 09/15] drm/vkms: Introduce configuration for plane color encoding Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup Louis Chauvet
` (4 subsequent siblings)
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
VKMS driver supports all the color range on planes, but for testing it can
be useful to only advertise few of them. This new configuration interface
will allow configuring the color range per planes.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 15 ++++++++++++++-
drivers/gpu/drm/vkms/vkms_config.h | 4 ++++
drivers/gpu/drm/vkms/vkms_plane.c | 5 ++---
3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index dabb8cb13314..e9e30974523a 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -86,7 +86,9 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_conf
BIT(DRM_COLOR_YCBCR_BT709) |
BIT(DRM_COLOR_YCBCR_BT2020);
vkms_config_overlay->default_color_encoding = DRM_COLOR_YCBCR_BT601;
-
+ vkms_config_overlay->supported_color_range = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+ BIT(DRM_COLOR_YCBCR_FULL_RANGE);
+ vkms_config_overlay->default_color_range = DRM_COLOR_YCBCR_FULL_RANGE;
list_add(&vkms_config_overlay->link, &vkms_config->planes);
@@ -132,6 +134,13 @@ bool vkms_config_is_valid(struct vkms_config *config)
BIT(config_plane->default_color_encoding))
return false;
+
+ // Default color range not in supported color range
+ if ((BIT(config_plane->default_color_range) &
+ config_plane->supported_color_range) !=
+ BIT(config_plane->default_color_range))
+ return false;
+
if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
// Multiple primary planes for only one CRTC
if (has_primary)
@@ -170,6 +179,10 @@ static int vkms_config_show(struct seq_file *m, void *data)
config_plane->supported_color_encoding);
seq_printf(m, "\tdefault color encoding: %d\n",
config_plane->default_color_encoding);
+ seq_printf(m, "\tsupported color range: 0x%x\n",
+ config_plane->supported_color_range);
+ seq_printf(m, "\tdefault color range: %d\n",
+ config_plane->default_color_range);
}
return 0;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index c8802bfb9ab2..d668aeab9d26 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -32,6 +32,8 @@ struct vkms_config {
* @supported_rotation: Rotation that this plane will support
* @default_color_encoding: Default color encoding that should be used by this plane
* @supported_color_encoding: Color encoding that this plane will support
+ * @default_color_range: Default color range that should be used by this plane
+ * @supported_color_range: Color range that this plane will support
* @plane: Internal usage. This pointer should never be considered as valid. It can be used to
* store a temporary reference to a vkms plane during device creation. This pointer is
* not managed by the configuration and must be managed by other means.
@@ -45,6 +47,8 @@ struct vkms_config_plane {
unsigned int supported_rotations;
enum drm_color_encoding default_color_encoding;
unsigned int supported_color_encoding;
+ enum drm_color_range default_color_range;
+ unsigned int supported_color_range;
/* Internal usage */
struct vkms_plane *plane;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index ef20da7ccb17..e7e890f730dd 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -243,10 +243,9 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
drm_plane_create_color_properties(&plane->base,
config->supported_color_encoding,
- BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
- BIT(DRM_COLOR_YCBCR_FULL_RANGE),
+ config->supported_color_range,
config->default_color_encoding,
- DRM_COLOR_YCBCR_FULL_RANGE);
+ config->default_color_range);
return plane;
}
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (9 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 10/15] drm/vkms: Introduce configuration for plane color range Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-27 14:33 ` Maxime Ripard
2024-08-14 14:36 ` [PATCH RFC 12/15] drm/vkms: Add configuration for CRTCs and encoders Louis Chauvet
` (3 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
Currently drm_writeback_connector are created by
drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
Both of the function uses drm_connector_init and drm_encoder_init, but
there is no way to properly clean those structure from outside.
This patch introduce the new function drm_writeback_connector_cleanup to
allow a proper cleanup.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
include/drm/drm_writeback.h | 11 +++++++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index a031c335bdb9..505a4eb40f93 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
wb_connector->encoder.possible_crtcs = possible_crtcs;
+ wb_connector->managed_encoder = true;
ret = drm_encoder_init(dev, &wb_connector->encoder,
&drm_writeback_encoder_funcs,
@@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
+void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
+{
+ drm_connector_cleanup(&wb_connector->base);
+ drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
+ if (wb_connector->managed_encoder)
+ drm_encoder_cleanup(&wb_connector->encoder);
+}
+EXPORT_SYMBOL(drm_writeback_connector_cleanup);
+
int drm_writeback_set_fb(struct drm_connector_state *conn_state,
struct drm_framebuffer *fb)
{
diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
index 17e576c80169..e651c0c0c84c 100644
--- a/include/drm/drm_writeback.h
+++ b/include/drm/drm_writeback.h
@@ -35,6 +35,15 @@ struct drm_writeback_connector {
*/
struct drm_encoder encoder;
+ /**
+ * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
+ *
+ * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
+ * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
+ * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
+ */
+ bool managed_encoder;
+
/**
* @pixel_formats_blob_ptr:
*
@@ -161,6 +170,8 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
const struct drm_connector_funcs *con_funcs, const u32 *formats,
int n_formats);
+void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector);
+
int drm_writeback_set_fb(struct drm_connector_state *conn_state,
struct drm_framebuffer *fb);
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-14 14:36 ` [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup Louis Chauvet
@ 2024-08-27 14:33 ` Maxime Ripard
2024-08-27 15:42 ` Louis Chauvet
0 siblings, 1 reply; 28+ messages in thread
From: Maxime Ripard @ 2024-08-27 14:33 UTC (permalink / raw)
To: Louis Chauvet
Cc: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, David Airlie,
dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee
[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]
Hi,
On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote:
> Currently drm_writeback_connector are created by
> drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
> Both of the function uses drm_connector_init and drm_encoder_init, but
> there is no way to properly clean those structure from outside.
>
> This patch introduce the new function drm_writeback_connector_cleanup to
> allow a proper cleanup.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
> include/drm/drm_writeback.h | 11 +++++++++++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index a031c335bdb9..505a4eb40f93 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>
> wb_connector->encoder.possible_crtcs = possible_crtcs;
> + wb_connector->managed_encoder = true;
>
> ret = drm_encoder_init(dev, &wb_connector->encoder,
> &drm_writeback_encoder_funcs,
> @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
>
> +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
> +{
> + drm_connector_cleanup(&wb_connector->base);
> + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> + if (wb_connector->managed_encoder)
> + drm_encoder_cleanup(&wb_connector->encoder);
> +}
> +EXPORT_SYMBOL(drm_writeback_connector_cleanup);
> +
> int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> struct drm_framebuffer *fb)
> {
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 17e576c80169..e651c0c0c84c 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -35,6 +35,15 @@ struct drm_writeback_connector {
> */
> struct drm_encoder encoder;
>
> + /**
> + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
> + *
> + * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
> + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
> + * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
> + */
> + bool managed_encoder;
> +
I think we should rather create drmm_writeback_connector variants,
and make both deprecated in favor of these new functions.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-27 14:33 ` Maxime Ripard
@ 2024-08-27 15:42 ` Louis Chauvet
2024-08-28 8:35 ` Jani Nikula
0 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-27 15:42 UTC (permalink / raw)
To: Maxime Ripard
Cc: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, David Airlie,
dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee
Le 27/08/24 - 16:33, Maxime Ripard a écrit :
> Hi,
>
> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote:
> > Currently drm_writeback_connector are created by
> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
> > Both of the function uses drm_connector_init and drm_encoder_init, but
> > there is no way to properly clean those structure from outside.
> >
> > This patch introduce the new function drm_writeback_connector_cleanup to
> > allow a proper cleanup.
> >
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
> > include/drm/drm_writeback.h | 11 +++++++++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > index a031c335bdb9..505a4eb40f93 100644
> > --- a/drivers/gpu/drm/drm_writeback.c
> > +++ b/drivers/gpu/drm/drm_writeback.c
> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> >
> > wb_connector->encoder.possible_crtcs = possible_crtcs;
> > + wb_connector->managed_encoder = true;
> >
> > ret = drm_encoder_init(dev, &wb_connector->encoder,
> > &drm_writeback_encoder_funcs,
> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > }
> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
> >
> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
> > +{
> > + drm_connector_cleanup(&wb_connector->base);
> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> > + if (wb_connector->managed_encoder)
> > + drm_encoder_cleanup(&wb_connector->encoder);
> > +}
> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup);
> > +
> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > struct drm_framebuffer *fb)
> > {
> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > index 17e576c80169..e651c0c0c84c 100644
> > --- a/include/drm/drm_writeback.h
> > +++ b/include/drm/drm_writeback.h
> > @@ -35,6 +35,15 @@ struct drm_writeback_connector {
> > */
> > struct drm_encoder encoder;
> >
> > + /**
> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
> > + *
> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
> > + */
> > + bool managed_encoder;
> > +
>
> I think we should rather create drmm_writeback_connector variants,
> and make both deprecated in favor of these new functions.
Hi,
I can try to do it. If I understand correctly, you want to create two
functions like this?
int drmm_writeback_connector_init([...]) {
/* drmm and alloc as we want to let drm core to manage this
encoder, no need to store it in drm_writeback_connector
*/
enc = drmm_plain_encoder_alloc(...);
return drmm_writeback_connector_init_with_encoder([...], enc);
}
int drmm_writeback_connector_init_with_encoder([...], enc) {
con = drmm_connector_init([...]);
drm_connector_attach_encoder(enc, con);
/* Needed for pixel_formats_blob_ptr, base is already
managed by drmm_connector_init. Maybe cleaning
job_queue is also needed? */
drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup)
}
Louis Chauvet
> Maxime
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-27 15:42 ` Louis Chauvet
@ 2024-08-28 8:35 ` Jani Nikula
2024-08-28 9:07 ` Louis Chauvet
0 siblings, 1 reply; 28+ messages in thread
From: Jani Nikula @ 2024-08-28 8:35 UTC (permalink / raw)
To: Louis Chauvet, Maxime Ripard
Cc: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Thomas Zimmermann, David Airlie,
dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee
On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> Le 27/08/24 - 16:33, Maxime Ripard a écrit :
>> Hi,
>>
>> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote:
>> > Currently drm_writeback_connector are created by
>> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
>> > Both of the function uses drm_connector_init and drm_encoder_init, but
>> > there is no way to properly clean those structure from outside.
>> >
>> > This patch introduce the new function drm_writeback_connector_cleanup to
>> > allow a proper cleanup.
>> >
>> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> > ---
>> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
>> > include/drm/drm_writeback.h | 11 +++++++++++
>> > 2 files changed, 21 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
>> > index a031c335bdb9..505a4eb40f93 100644
>> > --- a/drivers/gpu/drm/drm_writeback.c
>> > +++ b/drivers/gpu/drm/drm_writeback.c
>> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
>> >
>> > wb_connector->encoder.possible_crtcs = possible_crtcs;
>> > + wb_connector->managed_encoder = true;
>> >
>> > ret = drm_encoder_init(dev, &wb_connector->encoder,
>> > &drm_writeback_encoder_funcs,
>> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
>> > }
>> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
>> >
>> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
>> > +{
>> > + drm_connector_cleanup(&wb_connector->base);
>> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
>> > + if (wb_connector->managed_encoder)
>> > + drm_encoder_cleanup(&wb_connector->encoder);
>> > +}
>> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup);
>> > +
>> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>> > struct drm_framebuffer *fb)
>> > {
>> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
>> > index 17e576c80169..e651c0c0c84c 100644
>> > --- a/include/drm/drm_writeback.h
>> > +++ b/include/drm/drm_writeback.h
>> > @@ -35,6 +35,15 @@ struct drm_writeback_connector {
>> > */
>> > struct drm_encoder encoder;
>> >
>> > + /**
>> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
>> > + *
>> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
>> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
>> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
>> > + */
>> > + bool managed_encoder;
>> > +
>>
>> I think we should rather create drmm_writeback_connector variants,
>> and make both deprecated in favor of these new functions.
>
> Hi,
>
> I can try to do it. If I understand correctly, you want to create two
> functions like this?
>
> int drmm_writeback_connector_init([...]) {
> /* drmm and alloc as we want to let drm core to manage this
> encoder, no need to store it in drm_writeback_connector
> */
> enc = drmm_plain_encoder_alloc(...);
>
> return drmm_writeback_connector_init_with_encoder([...], enc);
> }
>
> int drmm_writeback_connector_init_with_encoder([...], enc) {
> con = drmm_connector_init([...]);
>
> drm_connector_attach_encoder(enc, con);
>
> /* Needed for pixel_formats_blob_ptr, base is already
> managed by drmm_connector_init. Maybe cleaning
> job_queue is also needed? */
> drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup)
> }
Why add two variants, when you can have one and pass NULL for encoder?
We have the _init_with_encoder variant only because nobody bothered to
clean up existing call sites.
Side note, I'd still like to be able to pass driver's own allocated
connector instead of having writeback midlayer force it on you.
BR,
Jani.
>
> Louis Chauvet
>
>> Maxime
>
>
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-28 8:35 ` Jani Nikula
@ 2024-08-28 9:07 ` Louis Chauvet
2024-08-28 15:17 ` Maxime Ripard
0 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-28 9:07 UTC (permalink / raw)
To: Jani Nikula
Cc: Maxime Ripard, Rodrigo Siqueira, Melissa Wen, Maíra Canal,
Haneen Mohammed, Daniel Vetter, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, dri-devel, arthurgrillo,
linux-kernel, jeremie.dautheribes, miquel.raynal,
thomas.petazzoni, seanpaul, nicolejadeyee
Le 28/08/24 - 11:35, Jani Nikula a écrit :
> On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> > Le 27/08/24 - 16:33, Maxime Ripard a écrit :
> >> Hi,
> >>
> >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote:
> >> > Currently drm_writeback_connector are created by
> >> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
> >> > Both of the function uses drm_connector_init and drm_encoder_init, but
> >> > there is no way to properly clean those structure from outside.
> >> >
> >> > This patch introduce the new function drm_writeback_connector_cleanup to
> >> > allow a proper cleanup.
> >> >
> >> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> >> > ---
> >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
> >> > include/drm/drm_writeback.h | 11 +++++++++++
> >> > 2 files changed, 21 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> >> > index a031c335bdb9..505a4eb40f93 100644
> >> > --- a/drivers/gpu/drm/drm_writeback.c
> >> > +++ b/drivers/gpu/drm/drm_writeback.c
> >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> >> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> >> >
> >> > wb_connector->encoder.possible_crtcs = possible_crtcs;
> >> > + wb_connector->managed_encoder = true;
> >> >
> >> > ret = drm_encoder_init(dev, &wb_connector->encoder,
> >> > &drm_writeback_encoder_funcs,
> >> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> >> > }
> >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
> >> >
> >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
> >> > +{
> >> > + drm_connector_cleanup(&wb_connector->base);
> >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> >> > + if (wb_connector->managed_encoder)
> >> > + drm_encoder_cleanup(&wb_connector->encoder);
> >> > +}
> >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup);
> >> > +
> >> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> >> > struct drm_framebuffer *fb)
> >> > {
> >> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> >> > index 17e576c80169..e651c0c0c84c 100644
> >> > --- a/include/drm/drm_writeback.h
> >> > +++ b/include/drm/drm_writeback.h
> >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector {
> >> > */
> >> > struct drm_encoder encoder;
> >> >
> >> > + /**
> >> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
> >> > + *
> >> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
> >> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
> >> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
> >> > + */
> >> > + bool managed_encoder;
> >> > +
> >>
> >> I think we should rather create drmm_writeback_connector variants,
> >> and make both deprecated in favor of these new functions.
> >
> > Hi,
> >
> > I can try to do it. If I understand correctly, you want to create two
> > functions like this?
> >
> > int drmm_writeback_connector_init([...]) {
> > /* drmm and alloc as we want to let drm core to manage this
> > encoder, no need to store it in drm_writeback_connector
> > */
> > enc = drmm_plain_encoder_alloc(...);
> >
> > return drmm_writeback_connector_init_with_encoder([...], enc);
> > }
> >
> > int drmm_writeback_connector_init_with_encoder([...], enc) {
> > con = drmm_connector_init([...]);
> >
> > drm_connector_attach_encoder(enc, con);
> >
> > /* Needed for pixel_formats_blob_ptr, base is already
> > managed by drmm_connector_init. Maybe cleaning
> > job_queue is also needed? */
> > drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup)
> > }
>
> Why add two variants, when you can have one and pass NULL for encoder?
> We have the _init_with_encoder variant only because nobody bothered to
> clean up existing call sites.
I just followed the existing code, but yes, I can make only one function
and create the encoder if the pointer is NULL.
> Side note, I'd still like to be able to pass driver's own allocated
> connector instead of having writeback midlayer force it on you.
I just checked, it seems non-trivial to make this change and I don't feel
confident to change this much the drm core.
Louis Chauvet
> BR,
> Jani.
>
>
> >
> > Louis Chauvet
> >
> >> Maxime
> >
> >
>
> --
> Jani Nikula, Intel
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup
2024-08-28 9:07 ` Louis Chauvet
@ 2024-08-28 15:17 ` Maxime Ripard
0 siblings, 0 replies; 28+ messages in thread
From: Maxime Ripard @ 2024-08-28 15:17 UTC (permalink / raw)
To: Jani Nikula, Rodrigo Siqueira, Melissa Wen, Maíra Canal,
Haneen Mohammed, Daniel Vetter, Maarten Lankhorst,
Thomas Zimmermann, David Airlie, dri-devel, arthurgrillo,
linux-kernel, jeremie.dautheribes, miquel.raynal,
thomas.petazzoni, seanpaul, nicolejadeyee
[-- Attachment #1: Type: text/plain, Size: 4994 bytes --]
On Wed, Aug 28, 2024 at 11:07:37AM GMT, Louis Chauvet wrote:
> Le 28/08/24 - 11:35, Jani Nikula a écrit :
> > On Tue, 27 Aug 2024, Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> > > Le 27/08/24 - 16:33, Maxime Ripard a écrit :
> > >> Hi,
> > >>
> > >> On Wed, Aug 14, 2024 at 04:36:33PM GMT, Louis Chauvet wrote:
> > >> > Currently drm_writeback_connector are created by
> > >> > drm_writeback_connector_init or drm_writeback_connector_init_with_encoder.
> > >> > Both of the function uses drm_connector_init and drm_encoder_init, but
> > >> > there is no way to properly clean those structure from outside.
> > >> >
> > >> > This patch introduce the new function drm_writeback_connector_cleanup to
> > >> > allow a proper cleanup.
> > >> >
> > >> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > >> > ---
> > >> > drivers/gpu/drm/drm_writeback.c | 10 ++++++++++
> > >> > include/drm/drm_writeback.h | 11 +++++++++++
> > >> > 2 files changed, 21 insertions(+)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> > >> > index a031c335bdb9..505a4eb40f93 100644
> > >> > --- a/drivers/gpu/drm/drm_writeback.c
> > >> > +++ b/drivers/gpu/drm/drm_writeback.c
> > >> > @@ -184,6 +184,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
> > >> > drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
> > >> >
> > >> > wb_connector->encoder.possible_crtcs = possible_crtcs;
> > >> > + wb_connector->managed_encoder = true;
> > >> >
> > >> > ret = drm_encoder_init(dev, &wb_connector->encoder,
> > >> > &drm_writeback_encoder_funcs,
> > >> > @@ -290,6 +291,15 @@ int drm_writeback_connector_init_with_encoder(struct drm_device *dev,
> > >> > }
> > >> > EXPORT_SYMBOL(drm_writeback_connector_init_with_encoder);
> > >> >
> > >> > +void drm_writeback_connector_cleanup(struct drm_writeback_connector *wb_connector)
> > >> > +{
> > >> > + drm_connector_cleanup(&wb_connector->base);
> > >> > + drm_property_blob_put(wb_connector->pixel_formats_blob_ptr);
> > >> > + if (wb_connector->managed_encoder)
> > >> > + drm_encoder_cleanup(&wb_connector->encoder);
> > >> > +}
> > >> > +EXPORT_SYMBOL(drm_writeback_connector_cleanup);
> > >> > +
> > >> > int drm_writeback_set_fb(struct drm_connector_state *conn_state,
> > >> > struct drm_framebuffer *fb)
> > >> > {
> > >> > diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> > >> > index 17e576c80169..e651c0c0c84c 100644
> > >> > --- a/include/drm/drm_writeback.h
> > >> > +++ b/include/drm/drm_writeback.h
> > >> > @@ -35,6 +35,15 @@ struct drm_writeback_connector {
> > >> > */
> > >> > struct drm_encoder encoder;
> > >> >
> > >> > + /**
> > >> > + * @managed_encoder: Sets to true if @encoder was created by drm_writeback_connector_init()
> > >> > + *
> > >> > + * If the user used drm_writeback_connector_init_with_encoder() to create the connector,
> > >> > + * @encoder is not valid and not managed by drm_writeback_connector. This fields allows
> > >> > + * the drm_writeback_cleanup() function to properly destroy the encoder if needed.
> > >> > + */
> > >> > + bool managed_encoder;
> > >> > +
> > >>
> > >> I think we should rather create drmm_writeback_connector variants,
> > >> and make both deprecated in favor of these new functions.
> > >
> > > Hi,
> > >
> > > I can try to do it. If I understand correctly, you want to create two
> > > functions like this?
> > >
> > > int drmm_writeback_connector_init([...]) {
> > > /* drmm and alloc as we want to let drm core to manage this
> > > encoder, no need to store it in drm_writeback_connector
> > > */
> > > enc = drmm_plain_encoder_alloc(...);
> > >
> > > return drmm_writeback_connector_init_with_encoder([...], enc);
> > > }
> > >
> > > int drmm_writeback_connector_init_with_encoder([...], enc) {
> > > con = drmm_connector_init([...]);
> > >
> > > drm_connector_attach_encoder(enc, con);
> > >
> > > /* Needed for pixel_formats_blob_ptr, base is already
> > > managed by drmm_connector_init. Maybe cleaning
> > > job_queue is also needed? */
> > > drmm_add_action_or_reset([...], &drm_writeback_connector_cleanup)
> > > }
> >
> > Why add two variants, when you can have one and pass NULL for encoder?
> > We have the _init_with_encoder variant only because nobody bothered to
> > clean up existing call sites.
>
> I just followed the existing code, but yes, I can make only one function
> and create the encoder if the pointer is NULL.
>
> > Side note, I'd still like to be able to pass driver's own allocated
> > connector instead of having writeback midlayer force it on you.
>
> I just checked, it seems non-trivial to make this change and I don't feel
> confident to change this much the drm core.
It's a great opportunity to add unit tests then :)
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 12/15] drm/vkms: Add configuration for CRTCs and encoders
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (10 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 11/15] drm: writeback: Add drm_writeback_connector cleanup Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-09-05 12:34 ` José Expósito
2024-08-14 14:36 ` [PATCH RFC 13/15] drm/vkms: Add name configuration for encoders Louis Chauvet
` (2 subsequent siblings)
14 siblings, 1 reply; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
The current VKMS driver can only uses one CRTC and one encoder. This patch
introduce in the same time CRTC and encoders as they are tighly linked.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 266 +++++++++++++++++++++++++++++++++----
drivers/gpu/drm/vkms/vkms_config.h | 102 +++++++++++++-
drivers/gpu/drm/vkms/vkms_crtc.c | 20 ++-
drivers/gpu/drm/vkms/vkms_crtc.h | 3 +-
drivers/gpu/drm/vkms/vkms_drv.c | 87 ++++++------
5 files changed, 405 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index e9e30974523a..fcd4a128c21b 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -14,6 +14,8 @@ struct vkms_config *vkms_config_create(void)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&config->planes);
+ INIT_LIST_HEAD(&config->crtcs);
+ INIT_LIST_HEAD(&config->encoders);
return config;
}
@@ -22,12 +24,24 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
bool enable_cursor)
{
struct vkms_config_plane *plane;
+ struct vkms_config_encoder *encoder;
+ struct vkms_config_crtc *crtc;
struct vkms_config *vkms_config = vkms_config_create();
if (IS_ERR(vkms_config))
return vkms_config;
- vkms_config->writeback = enable_writeback;
+ crtc = vkms_config_create_crtc(vkms_config);
+ if (!crtc)
+ goto err_alloc;
+ crtc->writeback = enable_writeback;
+
+ encoder = vkms_config_create_encoder(vkms_config);
+ if (!encoder)
+ goto err_alloc;
+
+ if (vkms_config_encoder_attach_crtc(encoder, crtc))
+ goto err_alloc;
plane = vkms_config_create_plane(vkms_config);
if (!plane)
@@ -39,6 +53,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
goto err_alloc;
sprintf(plane->name, "primary");
+ if (vkms_config_plane_attach_crtc(plane, crtc))
+ goto err_alloc;
+
if (enable_overlay) {
for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
plane = vkms_config_create_plane(vkms_config);
@@ -49,6 +66,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
if (!plane->name)
goto err_alloc;
snprintf(plane->name, 10, "plane-%d", i);
+
+ if (vkms_config_plane_attach_crtc(plane, crtc))
+ goto err_alloc;
}
}
if (enable_cursor) {
@@ -60,6 +80,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
if (!plane->name)
goto err_alloc;
sprintf(plane->name, "cursor");
+
+ if (vkms_config_plane_attach_crtc(plane, crtc))
+ goto err_alloc;
}
return vkms_config;
@@ -89,38 +112,196 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_conf
vkms_config_overlay->supported_color_range = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
BIT(DRM_COLOR_YCBCR_FULL_RANGE);
vkms_config_overlay->default_color_range = DRM_COLOR_YCBCR_FULL_RANGE;
+ xa_init_flags(&vkms_config_overlay->possible_crtcs, XA_FLAGS_ALLOC);
list_add(&vkms_config_overlay->link, &vkms_config->planes);
return vkms_config_overlay;
}
-void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
+struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *vkms_config)
{
- if (!vkms_config_overlay)
+ if (!vkms_config)
+ return NULL;
+
+ struct vkms_config_crtc *vkms_config_crtc = kzalloc(sizeof(*vkms_config_crtc),
+ GFP_KERNEL);
+
+ if (!vkms_config_crtc)
+ return NULL;
+
+ list_add(&vkms_config_crtc->link, &vkms_config->crtcs);
+ xa_init_flags(&vkms_config_crtc->possible_planes, XA_FLAGS_ALLOC);
+ xa_init_flags(&vkms_config_crtc->possible_encoders, XA_FLAGS_ALLOC);
+
+ return vkms_config_crtc;
+}
+
+struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *vkms_config)
+{
+ if (!vkms_config)
+ return NULL;
+
+ struct vkms_config_encoder *vkms_config_encoder = kzalloc(sizeof(*vkms_config_encoder),
+ GFP_KERNEL);
+
+ if (!vkms_config_encoder)
+ return NULL;
+
+ list_add(&vkms_config_encoder->link, &vkms_config->encoders);
+ xa_init_flags(&vkms_config_encoder->possible_crtcs, XA_FLAGS_ALLOC);
+
+ return vkms_config_encoder;
+}
+
+void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane,
+ struct vkms_config *vkms_config)
+{
+ struct vkms_config_crtc *crtc_config;
+ struct vkms_config_plane *plane;
+
+ if (!vkms_config_plane)
+ return;
+ list_del(&vkms_config_plane->link);
+ xa_destroy(&vkms_config_plane->possible_crtcs);
+
+ list_for_each_entry(crtc_config, &vkms_config->crtcs, link) {
+ unsigned long idx = 0;
+
+ xa_for_each(&crtc_config->possible_planes, idx, plane) {
+ if (plane == vkms_config_plane)
+ xa_erase(&crtc_config->possible_planes, idx);
+ }
+ }
+
+ kfree(vkms_config_plane->name);
+ kfree(vkms_config_plane);
+}
+
+void vkms_config_delete_crtc(struct vkms_config_crtc *vkms_config_crtc,
+ struct vkms_config *vkms_config)
+{
+ struct vkms_config_crtc *crtc_config;
+ struct vkms_config_plane *plane_config;
+ struct vkms_config_encoder *encoder_config;
+
+ if (!vkms_config_crtc)
+ return;
+ list_del(&vkms_config_crtc->link);
+ xa_destroy(&vkms_config_crtc->possible_planes);
+ xa_destroy(&vkms_config_crtc->possible_encoders);
+
+ list_for_each_entry(plane_config, &vkms_config->planes, link) {
+ unsigned long idx = 0;
+
+ xa_for_each(&plane_config->possible_crtcs, idx, crtc_config) {
+ if (crtc_config == vkms_config_crtc)
+ xa_erase(&plane_config->possible_crtcs, idx);
+ }
+ }
+
+ list_for_each_entry(encoder_config, &vkms_config->encoders, link) {
+ unsigned long idx = 0;
+
+ xa_for_each(&encoder_config->possible_crtcs, idx, crtc_config) {
+ if (crtc_config == vkms_config_crtc)
+ xa_erase(&encoder_config->possible_crtcs, idx);
+ }
+ }
+
+ kfree(vkms_config_crtc);
+}
+
+void vkms_config_delete_encoder(struct vkms_config_encoder *vkms_config_encoder,
+ struct vkms_config *vkms_config)
+{
+ if (!vkms_config_encoder)
return;
- list_del(&vkms_config_overlay->link);
- kfree(vkms_config_overlay->name);
- kfree(vkms_config_overlay);
+ list_del(&vkms_config_encoder->link);
+ xa_destroy(&vkms_config_encoder->possible_crtcs);
+
+ struct vkms_config_crtc *crtc_config;
+ struct vkms_config_encoder *encoder;
+
+ list_for_each_entry(crtc_config, &vkms_config->crtcs, link) {
+ unsigned long idx = 0;
+
+ xa_for_each(&crtc_config->possible_encoders, idx, encoder) {
+ if (encoder == vkms_config_encoder)
+ xa_erase(&crtc_config->possible_encoders, idx);
+ }
+ }
+
+ kfree(vkms_config_encoder);
}
void vkms_config_destroy(struct vkms_config *config)
{
struct vkms_config_plane *vkms_config_plane, *tmp_plane;
+ struct vkms_config_encoder *vkms_config_encoder, *tmp_encoder;
+ struct vkms_config_crtc *vkms_config_crtc, *tmp_crtc;
+
list_for_each_entry_safe(vkms_config_plane, tmp_plane, &config->planes, link) {
- vkms_config_delete_plane(vkms_config_plane);
+ vkms_config_delete_plane(vkms_config_plane, config);
+ }
+ list_for_each_entry_safe(vkms_config_encoder, tmp_encoder, &config->encoders, link) {
+ vkms_config_delete_encoder(vkms_config_encoder, config);
+ }
+ list_for_each_entry_safe(vkms_config_crtc, tmp_crtc, &config->crtcs, link) {
+ vkms_config_delete_crtc(vkms_config_crtc, config);
}
kfree(config);
}
+int __must_check vkms_config_plane_attach_crtc(struct vkms_config_plane *vkms_config_plane,
+ struct vkms_config_crtc *vkms_config_crtc)
+{
+ u32 crtc_idx, encoder_idx;
+ int ret;
+
+ ret = xa_alloc(&vkms_config_plane->possible_crtcs, &crtc_idx, vkms_config_crtc,
+ xa_limit_32b, GFP_KERNEL);
+ if (ret)
+ return ret;
+
+ ret = xa_alloc(&vkms_config_crtc->possible_planes, &encoder_idx, vkms_config_plane,
+ xa_limit_32b, GFP_KERNEL);
+ if (ret) {
+ xa_erase(&vkms_config_plane->possible_crtcs, crtc_idx);
+ return ret;
+ }
+
+ return ret;
+}
+
+int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *vkms_config_encoder,
+ struct vkms_config_crtc *vkms_config_crtc)
+{
+ u32 crtc_idx, encoder_idx;
+ int ret;
+
+ ret = xa_alloc(&vkms_config_encoder->possible_crtcs, &crtc_idx, vkms_config_crtc,
+ xa_limit_32b, GFP_KERNEL);
+ if (ret)
+ return ret;
+
+ ret = xa_alloc(&vkms_config_crtc->possible_encoders, &encoder_idx, vkms_config_encoder,
+ xa_limit_32b, GFP_KERNEL);
+ if (ret) {
+ xa_erase(&vkms_config_encoder->possible_crtcs, crtc_idx);
+ return ret;
+ }
+
+ return ret;
+}
+
bool vkms_config_is_valid(struct vkms_config *config)
{
struct vkms_config_plane *config_plane;
-
- bool has_cursor = false;
- bool has_primary = false;
+ struct vkms_config_crtc *config_crtc;
+ struct vkms_config_encoder *config_encoder;
list_for_each_entry(config_plane, &config->planes, link) {
// Default rotation not in supported rotations
@@ -141,22 +322,47 @@ bool vkms_config_is_valid(struct vkms_config *config)
BIT(config_plane->default_color_range))
return false;
- if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
- // Multiple primary planes for only one CRTC
- if (has_primary)
- return false;
- has_primary = true;
- }
- if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
- // Multiple cursor planes for only one CRTC
- if (has_cursor)
- return false;
- has_cursor = true;
- }
+ // No CRTC linked to this plane
+ if (xa_empty(&config_plane->possible_crtcs))
+ return false;
+ }
+
+ list_for_each_entry(config_encoder, &config->encoders, link) {
+ // No CRTC linked to this encoder
+ if (xa_empty(&config_encoder->possible_crtcs))
+ return false;
}
- if (!has_primary)
- return false;
+ list_for_each_entry(config_crtc, &config->crtcs, link) {
+ bool has_primary = false;
+ bool has_cursor = false;
+ unsigned long idx = 0;
+
+ // No encoder attached to this CRTC
+ if (xa_empty(&config_crtc->possible_encoders))
+ return false;
+
+ xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
+ if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
+ // Multiple primary planes for only one CRTC
+ if (has_primary)
+ return false;
+
+ has_primary = true;
+ }
+ if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
+ // Multiple cursor planes for only one CRTC
+ if (has_cursor)
+ return false;
+
+ has_cursor = true;
+ }
+ }
+
+ // No primary plane for this CRTC
+ if (!has_primary)
+ return false;
+ }
return true;
}
@@ -167,8 +373,9 @@ static int vkms_config_show(struct seq_file *m, void *data)
struct drm_device *dev = entry->dev;
struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
struct vkms_config_plane *config_plane;
+ struct vkms_config_crtc *config_crtc;
+ struct vkms_config_encoder *config_encoder;
- seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
seq_puts(m, "plane:\n");
seq_printf(m, "\tname: %s\n", config_plane->name);
@@ -185,6 +392,15 @@ static int vkms_config_show(struct seq_file *m, void *data)
config_plane->default_color_range);
}
+ list_for_each_entry(config_encoder, &vkmsdev->config->encoders, link) {
+ seq_puts(m, "encoder:\n");
+ }
+
+ list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
+ seq_puts(m, "crtc:\n");
+ seq_printf(m, "\twriteback: %d\n", config_crtc->writeback);
+ }
+
return 0;
}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index d668aeab9d26..8f247fc09373 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -9,16 +9,59 @@
/**
* struct vkms_config - General configuration for VKMS driver
*
- * @writeback: If true, a writeback buffer can be attached to the CRTC
* @planes: List of planes configured for this device. They are created by the function
* vkms_config_create_plane().
+ * @crtcs: List of crtcs configured for this device. They are created by the function
+ * vkms_config_create_crtc().
+ * @encoders: List of encoders configured for this device. They are created by the function
+ * vkms_config_create_encoder().
* @dev: Used to store the current vkms device. Only set when the device is instancied.
*/
struct vkms_config {
- bool writeback;
struct vkms_device *dev;
struct list_head planes;
+ struct list_head crtcs;
+ struct list_head encoders;
+};
+
+/**
+ * struct vkms_config_crtc
+ *
+ * @link: Link to the others CRTCs
+ * @possible_planes: List of planes that can be used with this CRTC
+ * @possible_encoders: List of encoders that can be used with this CRTC
+ * @crtc: Internal usage. This pointer should never be considered as valid. It can be used to
+ * store a temporary reference to a vkms crtc during device creation. This pointer is
+ * not managed by the configuration and must be managed by other means.
+ */
+struct vkms_config_crtc {
+ struct list_head link;
+
+ bool writeback;
+ struct xarray possible_planes;
+ struct xarray possible_encoders;
+
+ /* Internal usage */
+ struct vkms_crtc *crtc;
+};
+
+/**
+ * struct vkms_config_encoder
+ *
+ * @link: Link to the others encoders
+ * @possible_crtcs: List of CRTC that can be used with this encoder
+ * @encoder: Internal usage. This pointer should never be considered as valid. It can be used to
+ * store a temporary reference to a vkms encoder during device creation. This pointer is
+ * not managed by the configuration and must be managed by other means.
+ */
+struct vkms_config_encoder {
+ struct list_head link;
+
+ struct xarray possible_crtcs;
+
+ /* Internal usage */
+ struct drm_encoder *encoder;
};
/**
@@ -34,6 +77,7 @@ struct vkms_config {
* @supported_color_encoding: Color encoding that this plane will support
* @default_color_range: Default color range that should be used by this plane
* @supported_color_range: Color range that this plane will support
+ * @possible_crtcs: List of CRTC that can be used with this plane.
* @plane: Internal usage. This pointer should never be considered as valid. It can be used to
* store a temporary reference to a vkms plane during device creation. This pointer is
* not managed by the configuration and must be managed by other means.
@@ -50,6 +94,7 @@ struct vkms_config_plane {
enum drm_color_range default_color_range;
unsigned int supported_color_range;
+ struct xarray possible_crtcs;
/* Internal usage */
struct vkms_plane *plane;
};
@@ -84,14 +129,63 @@ bool vkms_config_is_valid(struct vkms_config *vkms_config);
*/
struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config);
+/**
+ * vkms_config_create_crtc() - Create a crtc configuration
+ *
+ * This will allocate and add a new crtc configuration to @vkms_config.
+ * @vkms_config: Configuration where to insert new crtc configuration
+ */
+struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *vkms_config);
+
+/**
+ * vkms_config_create_encoder() - Create an encoder configuration
+ *
+ * This will allocate and add a new encoder configuration to @vkms_config.
+ * @vkms_config: Configuration where to insert new encoder configuration
+ */
+struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *vkms_config);
+
+int __must_check vkms_config_plane_attach_crtc(struct vkms_config_plane *vkms_config_plane,
+ struct vkms_config_crtc *vkms_config_crtc);
+int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *vkms_config_encoder,
+ struct vkms_config_crtc *vkms_config_crtc);
+
/**
* vkms_config_delete_plane() - Remove a plane configuration and frees its memory
*
* This will delete a plane configuration from the parent configuration. This will NOT
- * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane.
+ * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane. It will also remove
+ * any reference to this plane in @vkms_config.
+ *
* @vkms_config_plane: Plane configuration to cleanup
+ * @vkms_config: Parent configuration
+ */
+void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane,
+ struct vkms_config *vkms_config);
+/**
+ * vkms_config_delete_crtc() - Remove a CRTC configuration and frees its memory
+ *
+ * This will delete a CRTC configuration from the parent configuration. This will NOT
+ * cleanup and frees the vkms_crtc that can be stored in @vkms_config_crtc. It will also remove
+ * any reference to this CRTC in @vkms_config.
+ *
+ * @vkms_config_crtc: Plane configuration to cleanup
+ * @vkms_config: Parent configuration
+ */
+void vkms_config_delete_crtc(struct vkms_config_crtc *vkms_config_crtc,
+ struct vkms_config *vkms_config);
+/**
+ * vkms_config_delete_encoder() - Remove an encoder configuration and frees its memory
+ *
+ * This will delete an encoder configuration from the parent configuration. This will NOT
+ * cleanup and frees the vkms_encoder that can be stored in @vkms_config_encoder. It will also
+ * remove any reference to this CRTC in @vkms_config.
+ *
+ * @vkms_config_encoder: Plane configuration to cleanup
+ * @vkms_config: Parent configuration
*/
-void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane);
+void vkms_config_delete_encoder(struct vkms_config_encoder *vkms_config_encoder,
+ struct vkms_config *vkms_config);
/**
* vkms_config_alloc_default() - Allocate the configuration for the default device
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 6f6d3118b2f2..654238dbba7f 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -281,9 +281,17 @@ static void vkms_crtc_destroy_workqueue(struct drm_device *dev, void *raw_vkms_c
destroy_workqueue(vkms_crtc->composer_workq);
}
+static void vkms_crtc_cleanup_writeback_connector(struct drm_device *dev, void *data)
+{
+ struct vkms_crtc *vkms_crtc = data;
+
+ drm_writeback_connector_cleanup(&vkms_crtc->wb_connector);
+}
+
struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
struct drm_plane *primary,
- struct drm_plane *cursor)
+ struct drm_plane *cursor,
+ struct vkms_config_crtc *config)
{
struct drm_device *dev = &vkmsdev->drm;
struct vkms_crtc *vkms_crtc;
@@ -317,5 +325,15 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
if (ret)
return ERR_PTR(ret);
+ if (config->writeback) {
+ ret = vkms_enable_writeback_connector(vkmsdev, vkms_crtc);
+ if (ret)
+ return ERR_PTR(ret);
+ ret = drmm_add_action_or_reset(&vkmsdev->drm, vkms_crtc_cleanup_writeback_connector,
+ vkms_crtc);
+ if (ret)
+ return ERR_PTR(ret);
+ }
+
return vkms_crtc;
}
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.h b/drivers/gpu/drm/vkms/vkms_crtc.h
index dde28884a0a5..a271e95f1cfe 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.h
+++ b/drivers/gpu/drm/vkms/vkms_crtc.h
@@ -86,5 +86,6 @@ struct vkms_crtc {
*/
struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
struct drm_plane *primary,
- struct drm_plane *cursor);
+ struct drm_plane *cursor,
+ struct vkms_config_crtc *config);
#endif //_VKMS_CRTC_H
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 403006a0bb61..6deff5099322 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -153,13 +153,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
static int vkms_output_init(struct vkms_device *vkmsdev)
{
- struct vkms_config_plane *config_plane;
+ struct vkms_config_encoder *config_encoder;
struct drm_device *dev = &vkmsdev->drm;
+ struct vkms_config_plane *config_plane;
+ struct vkms_config_crtc *config_crtc;
struct drm_connector *connector;
- struct drm_encoder *encoder;
- struct vkms_crtc *crtc;
- struct drm_plane *plane;
- struct vkms_plane *primary, *cursor = NULL;
+ unsigned long idx;
int ret;
int writeback;
@@ -169,22 +168,30 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
ret = PTR_ERR(config_plane->plane);
return ret;
}
-
- if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
- primary = config_plane->plane;
- else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
- cursor = config_plane->plane;
}
- /* [1]: Initialize the crtc component */
- crtc = vkms_crtc_init(vkmsdev, &primary->base,
- cursor ? &cursor->base : NULL);
- if (IS_ERR(crtc))
- return PTR_ERR(crtc);
+ list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
+ struct drm_plane *primary = NULL, *cursor = NULL;
+
+ xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
+ if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
+ primary = &config_plane->plane->base;
+ else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
+ cursor = &config_plane->plane->base;
+ }
+
+ config_crtc->crtc = vkms_crtc_init(vkmsdev, primary, cursor, config_crtc);
- /* Enable the output's CRTC for all the planes */
- drm_for_each_plane(plane, &vkmsdev->drm) {
- plane->possible_crtcs |= drm_crtc_mask(&crtc->base);
+ if (IS_ERR(config_crtc->crtc)) {
+ ret = PTR_ERR(config_crtc->crtc);
+ return ret;
+ }
+ }
+
+ list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
+ xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
+ config_plane->plane->base.possible_crtcs |= drm_crtc_mask(&config_crtc->crtc->base);
+ }
}
/* Initialize the connector component */
@@ -201,32 +208,28 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
- /* Initialize the encoder component */
- encoder = drmm_kzalloc(&vkmsdev->drm, sizeof(*encoder), GFP_KERNEL);
- if (!encoder)
- return -ENOMEM;
-
- ret = drmm_encoder_init(dev, encoder, &vkms_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
- if (ret) {
- DRM_ERROR("Failed to init encoder\n");
- return ret;
- }
-
- encoder->possible_crtcs = drm_crtc_mask(&crtc->base);
- /* Attach the encoder and the connector */
- ret = drm_connector_attach_encoder(connector, encoder);
- if (ret) {
- DRM_ERROR("Failed to attach connector to encoder\n");
- return ret;
- }
+ list_for_each_entry(config_encoder, &vkmsdev->config->encoders, link) {
+ config_encoder->encoder = drmm_kzalloc(&vkmsdev->drm,
+ sizeof(*config_encoder->encoder),
+ GFP_KERNEL);
+ if (!config_encoder->encoder)
+ return -ENOMEM;
+ ret = drmm_encoder_init(dev, config_encoder->encoder, &vkms_encoder_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
+ if (ret) {
+ DRM_ERROR("Failed to init encoder\n");
+ return ret;
+ }
- /* Initialize the writeback component */
- if (vkmsdev->config->writeback) {
- writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
- if (writeback)
- DRM_ERROR("Failed to init writeback connector\n");
+ xa_for_each(&config_encoder->possible_crtcs, idx, config_crtc) {
+ config_encoder->encoder->possible_crtcs |= drm_crtc_mask(&config_crtc->crtc->base);
+ }
+ if (IS_ERR(config_encoder->encoder))
+ return PTR_ERR(config_encoder->encoder);
+ ret = drm_connector_attach_encoder(connector, config_encoder->encoder);
+ if (ret)
+ return ret;
}
drm_mode_config_reset(dev);
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 12/15] drm/vkms: Add configuration for CRTCs and encoders
2024-08-14 14:36 ` [PATCH RFC 12/15] drm/vkms: Add configuration for CRTCs and encoders Louis Chauvet
@ 2024-09-05 12:34 ` José Expósito
0 siblings, 0 replies; 28+ messages in thread
From: José Expósito @ 2024-09-05 12:34 UTC (permalink / raw)
To: louis.chauvet
Cc: airlied, arthurgrillo, daniel, dri-devel, hamohammed.sa,
jeremie.dautheribes, linux-kernel, maarten.lankhorst, mairacanal,
melissa.srw, miquel.raynal, mripard, nicolejadeyee,
rodrigosiqueiramelo, seanpaul, thomas.petazzoni, tzimmermann
> The current VKMS driver can only uses one CRTC and one encoder. This patch
> introduce in the same time CRTC and encoders as they are tighly linked.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_config.c | 266 +++++++++++++++++++++++++++++++++----
> drivers/gpu/drm/vkms/vkms_config.h | 102 +++++++++++++-
> drivers/gpu/drm/vkms/vkms_crtc.c | 20 ++-
> drivers/gpu/drm/vkms/vkms_crtc.h | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 87 ++++++------
> 5 files changed, 405 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
> index e9e30974523a..fcd4a128c21b 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> @@ -14,6 +14,8 @@ struct vkms_config *vkms_config_create(void)
> return ERR_PTR(-ENOMEM);
>
> INIT_LIST_HEAD(&config->planes);
> + INIT_LIST_HEAD(&config->crtcs);
> + INIT_LIST_HEAD(&config->encoders);
>
> return config;
> }
> @@ -22,12 +24,24 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> bool enable_cursor)
> {
> struct vkms_config_plane *plane;
> + struct vkms_config_encoder *encoder;
> + struct vkms_config_crtc *crtc;
> struct vkms_config *vkms_config = vkms_config_create();
>
> if (IS_ERR(vkms_config))
> return vkms_config;
>
> - vkms_config->writeback = enable_writeback;
> + crtc = vkms_config_create_crtc(vkms_config);
> + if (!crtc)
> + goto err_alloc;
> + crtc->writeback = enable_writeback;
> +
> + encoder = vkms_config_create_encoder(vkms_config);
> + if (!encoder)
> + goto err_alloc;
> +
> + if (vkms_config_encoder_attach_crtc(encoder, crtc))
> + goto err_alloc;
>
> plane = vkms_config_create_plane(vkms_config);
> if (!plane)
> @@ -39,6 +53,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> goto err_alloc;
> sprintf(plane->name, "primary");
>
> + if (vkms_config_plane_attach_crtc(plane, crtc))
> + goto err_alloc;
> +
> if (enable_overlay) {
> for (int i = 0; i < NUM_OVERLAY_PLANES; i++) {
> plane = vkms_config_create_plane(vkms_config);
> @@ -49,6 +66,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> if (!plane->name)
> goto err_alloc;
> snprintf(plane->name, 10, "plane-%d", i);
> +
> + if (vkms_config_plane_attach_crtc(plane, crtc))
> + goto err_alloc;
> }
> }
> if (enable_cursor) {
> @@ -60,6 +80,9 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
> if (!plane->name)
> goto err_alloc;
> sprintf(plane->name, "cursor");
> +
> + if (vkms_config_plane_attach_crtc(plane, crtc))
> + goto err_alloc;
> }
>
> return vkms_config;
> @@ -89,38 +112,196 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_conf
> vkms_config_overlay->supported_color_range = BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
> BIT(DRM_COLOR_YCBCR_FULL_RANGE);
> vkms_config_overlay->default_color_range = DRM_COLOR_YCBCR_FULL_RANGE;
> + xa_init_flags(&vkms_config_overlay->possible_crtcs, XA_FLAGS_ALLOC);
>
> list_add(&vkms_config_overlay->link, &vkms_config->planes);
>
> return vkms_config_overlay;
> }
>
> -void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_overlay)
> +struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *vkms_config)
> {
> - if (!vkms_config_overlay)
> + if (!vkms_config)
> + return NULL;
> +
> + struct vkms_config_crtc *vkms_config_crtc = kzalloc(sizeof(*vkms_config_crtc),
> + GFP_KERNEL);
> +
> + if (!vkms_config_crtc)
> + return NULL;
> +
> + list_add(&vkms_config_crtc->link, &vkms_config->crtcs);
> + xa_init_flags(&vkms_config_crtc->possible_planes, XA_FLAGS_ALLOC);
> + xa_init_flags(&vkms_config_crtc->possible_encoders, XA_FLAGS_ALLOC);
> +
> + return vkms_config_crtc;
> +}
> +
> +struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *vkms_config)
> +{
> + if (!vkms_config)
> + return NULL;
> +
> + struct vkms_config_encoder *vkms_config_encoder = kzalloc(sizeof(*vkms_config_encoder),
> + GFP_KERNEL);
> +
> + if (!vkms_config_encoder)
> + return NULL;
> +
> + list_add(&vkms_config_encoder->link, &vkms_config->encoders);
> + xa_init_flags(&vkms_config_encoder->possible_crtcs, XA_FLAGS_ALLOC);
> +
> + return vkms_config_encoder;
> +}
> +
> +void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane,
> + struct vkms_config *vkms_config)
> +{
> + struct vkms_config_crtc *crtc_config;
> + struct vkms_config_plane *plane;
> +
> + if (!vkms_config_plane)
> + return;
> + list_del(&vkms_config_plane->link);
> + xa_destroy(&vkms_config_plane->possible_crtcs);
> +
> + list_for_each_entry(crtc_config, &vkms_config->crtcs, link) {
> + unsigned long idx = 0;
> +
> + xa_for_each(&crtc_config->possible_planes, idx, plane) {
> + if (plane == vkms_config_plane)
> + xa_erase(&crtc_config->possible_planes, idx);
> + }
> + }
> +
> + kfree(vkms_config_plane->name);
> + kfree(vkms_config_plane);
> +}
> +
> +void vkms_config_delete_crtc(struct vkms_config_crtc *vkms_config_crtc,
> + struct vkms_config *vkms_config)
> +{
> + struct vkms_config_crtc *crtc_config;
> + struct vkms_config_plane *plane_config;
> + struct vkms_config_encoder *encoder_config;
> +
> + if (!vkms_config_crtc)
> + return;
> + list_del(&vkms_config_crtc->link);
> + xa_destroy(&vkms_config_crtc->possible_planes);
> + xa_destroy(&vkms_config_crtc->possible_encoders);
> +
> + list_for_each_entry(plane_config, &vkms_config->planes, link) {
> + unsigned long idx = 0;
> +
> + xa_for_each(&plane_config->possible_crtcs, idx, crtc_config) {
> + if (crtc_config == vkms_config_crtc)
> + xa_erase(&plane_config->possible_crtcs, idx);
> + }
> + }
> +
> + list_for_each_entry(encoder_config, &vkms_config->encoders, link) {
> + unsigned long idx = 0;
> +
> + xa_for_each(&encoder_config->possible_crtcs, idx, crtc_config) {
> + if (crtc_config == vkms_config_crtc)
> + xa_erase(&encoder_config->possible_crtcs, idx);
> + }
> + }
> +
> + kfree(vkms_config_crtc);
> +}
> +
> +void vkms_config_delete_encoder(struct vkms_config_encoder *vkms_config_encoder,
> + struct vkms_config *vkms_config)
> +{
> + if (!vkms_config_encoder)
> return;
> - list_del(&vkms_config_overlay->link);
> - kfree(vkms_config_overlay->name);
> - kfree(vkms_config_overlay);
> + list_del(&vkms_config_encoder->link);
> + xa_destroy(&vkms_config_encoder->possible_crtcs);
> +
> + struct vkms_config_crtc *crtc_config;
> + struct vkms_config_encoder *encoder;
> +
> + list_for_each_entry(crtc_config, &vkms_config->crtcs, link) {
> + unsigned long idx = 0;
> +
> + xa_for_each(&crtc_config->possible_encoders, idx, encoder) {
> + if (encoder == vkms_config_encoder)
> + xa_erase(&crtc_config->possible_encoders, idx);
> + }
> + }
> +
> + kfree(vkms_config_encoder);
> }
>
> void vkms_config_destroy(struct vkms_config *config)
> {
> struct vkms_config_plane *vkms_config_plane, *tmp_plane;
> + struct vkms_config_encoder *vkms_config_encoder, *tmp_encoder;
> + struct vkms_config_crtc *vkms_config_crtc, *tmp_crtc;
> +
>
> list_for_each_entry_safe(vkms_config_plane, tmp_plane, &config->planes, link) {
> - vkms_config_delete_plane(vkms_config_plane);
> + vkms_config_delete_plane(vkms_config_plane, config);
> + }
> + list_for_each_entry_safe(vkms_config_encoder, tmp_encoder, &config->encoders, link) {
> + vkms_config_delete_encoder(vkms_config_encoder, config);
> + }
> + list_for_each_entry_safe(vkms_config_crtc, tmp_crtc, &config->crtcs, link) {
> + vkms_config_delete_crtc(vkms_config_crtc, config);
> }
>
> kfree(config);
> }
>
> +int __must_check vkms_config_plane_attach_crtc(struct vkms_config_plane *vkms_config_plane,
> + struct vkms_config_crtc *vkms_config_crtc)
> +{
> + u32 crtc_idx, encoder_idx;
> + int ret;
> +
> + ret = xa_alloc(&vkms_config_plane->possible_crtcs, &crtc_idx, vkms_config_crtc,
> + xa_limit_32b, GFP_KERNEL);
> + if (ret)
> + return ret;
> +
> + ret = xa_alloc(&vkms_config_crtc->possible_planes, &encoder_idx, vkms_config_plane,
> + xa_limit_32b, GFP_KERNEL);
> + if (ret) {
> + xa_erase(&vkms_config_plane->possible_crtcs, crtc_idx);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> +int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *vkms_config_encoder,
> + struct vkms_config_crtc *vkms_config_crtc)
> +{
> + u32 crtc_idx, encoder_idx;
> + int ret;
> +
> + ret = xa_alloc(&vkms_config_encoder->possible_crtcs, &crtc_idx, vkms_config_crtc,
> + xa_limit_32b, GFP_KERNEL);
> + if (ret)
> + return ret;
> +
> + ret = xa_alloc(&vkms_config_crtc->possible_encoders, &encoder_idx, vkms_config_encoder,
> + xa_limit_32b, GFP_KERNEL);
> + if (ret) {
> + xa_erase(&vkms_config_encoder->possible_crtcs, crtc_idx);
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> bool vkms_config_is_valid(struct vkms_config *config)
I still need to look into the xa_* code in more detail, but I think
that it is a great solution for the problem I had handling possible_crtcs
and possible_encoders.
About the validation, If I undertood the docs correctly:
"All drivers should provide one primary plane per CRTC"
https://www.kernel.org/doc/html/v4.17/gpu/drm-kms.html#plane-abstraction
We'd need to check that the number of primary planes is equal to the number
of CRTCs and that a configuration to assing one primary plane to each CRTC
is available.
To illustrate it with an example, this is a valid config:
CRTC 0
CRTC 1
Primary Plane 0: possible_crtcs -> CRTC 0 and CRTC 1
Primary Plane 1: possible_crtcs -> CRTC 0
But this is not a valid config because there are no compatible planes with
CRTC 1:
CRTC 0
CRTC 1
Primary Plane 0: possible_crtcs -> CRTC 0
Primary Plane 1: possible_crtcs -> CRTC 0
I think that your code would fail in the first example because of:
// Multiple primary planes for only one CRTC
if (has_primary)
return false;
Also, we'd need to check that the number of CRTCs and encoders is <= 32.
> {
> struct vkms_config_plane *config_plane;
> -
> - bool has_cursor = false;
> - bool has_primary = false;
> + struct vkms_config_crtc *config_crtc;
> + struct vkms_config_encoder *config_encoder;
>
> list_for_each_entry(config_plane, &config->planes, link) {
> // Default rotation not in supported rotations
> @@ -141,22 +322,47 @@ bool vkms_config_is_valid(struct vkms_config *config)
> BIT(config_plane->default_color_range))
> return false;
>
> - if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
> - // Multiple primary planes for only one CRTC
> - if (has_primary)
> - return false;
> - has_primary = true;
> - }
> - if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
> - // Multiple cursor planes for only one CRTC
> - if (has_cursor)
> - return false;
> - has_cursor = true;
> - }
> + // No CRTC linked to this plane
> + if (xa_empty(&config_plane->possible_crtcs))
> + return false;
> + }
> +
> + list_for_each_entry(config_encoder, &config->encoders, link) {
> + // No CRTC linked to this encoder
> + if (xa_empty(&config_encoder->possible_crtcs))
> + return false;
> }
>
> - if (!has_primary)
> - return false;
> + list_for_each_entry(config_crtc, &config->crtcs, link) {
> + bool has_primary = false;
> + bool has_cursor = false;
> + unsigned long idx = 0;
> +
> + // No encoder attached to this CRTC
> + if (xa_empty(&config_crtc->possible_encoders))
> + return false;
> +
> + xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
> + if (config_plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + // Multiple primary planes for only one CRTC
> + if (has_primary)
> + return false;
> +
> + has_primary = true;
> + }
> + if (config_plane->type == DRM_PLANE_TYPE_CURSOR) {
> + // Multiple cursor planes for only one CRTC
> + if (has_cursor)
> + return false;
> +
> + has_cursor = true;
> + }
> + }
> +
> + // No primary plane for this CRTC
> + if (!has_primary)
> + return false;
> + }
>
> return true;
> }
> @@ -167,8 +373,9 @@ static int vkms_config_show(struct seq_file *m, void *data)
> struct drm_device *dev = entry->dev;
> struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> struct vkms_config_plane *config_plane;
> + struct vkms_config_crtc *config_crtc;
> + struct vkms_config_encoder *config_encoder;
>
> - seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> list_for_each_entry(config_plane, &vkmsdev->config->planes, link) {
> seq_puts(m, "plane:\n");
> seq_printf(m, "\tname: %s\n", config_plane->name);
> @@ -185,6 +392,15 @@ static int vkms_config_show(struct seq_file *m, void *data)
> config_plane->default_color_range);
> }
>
> + list_for_each_entry(config_encoder, &vkmsdev->config->encoders, link) {
> + seq_puts(m, "encoder:\n");
> + }
> +
> + list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
> + seq_puts(m, "crtc:\n");
> + seq_printf(m, "\twriteback: %d\n", config_crtc->writeback);
> + }
> +
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
> index d668aeab9d26..8f247fc09373 100644
> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -9,16 +9,59 @@
> /**
> * struct vkms_config - General configuration for VKMS driver
> *
> - * @writeback: If true, a writeback buffer can be attached to the CRTC
> * @planes: List of planes configured for this device. They are created by the function
> * vkms_config_create_plane().
> + * @crtcs: List of crtcs configured for this device. They are created by the function
> + * vkms_config_create_crtc().
> + * @encoders: List of encoders configured for this device. They are created by the function
> + * vkms_config_create_encoder().
> * @dev: Used to store the current vkms device. Only set when the device is instancied.
> */
> struct vkms_config {
> - bool writeback;
> struct vkms_device *dev;
>
> struct list_head planes;
> + struct list_head crtcs;
> + struct list_head encoders;
> +};
> +
> +/**
> + * struct vkms_config_crtc
> + *
> + * @link: Link to the others CRTCs
> + * @possible_planes: List of planes that can be used with this CRTC
> + * @possible_encoders: List of encoders that can be used with this CRTC
> + * @crtc: Internal usage. This pointer should never be considered as valid. It can be used to
> + * store a temporary reference to a vkms crtc during device creation. This pointer is
> + * not managed by the configuration and must be managed by other means.
> + */
> +struct vkms_config_crtc {
> + struct list_head link;
> +
> + bool writeback;
> + struct xarray possible_planes;
> + struct xarray possible_encoders;
> +
> + /* Internal usage */
> + struct vkms_crtc *crtc;
> +};
> +
> +/**
> + * struct vkms_config_encoder
> + *
> + * @link: Link to the others encoders
> + * @possible_crtcs: List of CRTC that can be used with this encoder
> + * @encoder: Internal usage. This pointer should never be considered as valid. It can be used to
> + * store a temporary reference to a vkms encoder during device creation. This pointer is
> + * not managed by the configuration and must be managed by other means.
> + */
> +struct vkms_config_encoder {
> + struct list_head link;
> +
> + struct xarray possible_crtcs;
> +
> + /* Internal usage */
> + struct drm_encoder *encoder;
> };
>
> /**
> @@ -34,6 +77,7 @@ struct vkms_config {
> * @supported_color_encoding: Color encoding that this plane will support
> * @default_color_range: Default color range that should be used by this plane
> * @supported_color_range: Color range that this plane will support
> + * @possible_crtcs: List of CRTC that can be used with this plane.
> * @plane: Internal usage. This pointer should never be considered as valid. It can be used to
> * store a temporary reference to a vkms plane during device creation. This pointer is
> * not managed by the configuration and must be managed by other means.
> @@ -50,6 +94,7 @@ struct vkms_config_plane {
> enum drm_color_range default_color_range;
> unsigned int supported_color_range;
>
> + struct xarray possible_crtcs;
> /* Internal usage */
> struct vkms_plane *plane;
> };
> @@ -84,14 +129,63 @@ bool vkms_config_is_valid(struct vkms_config *vkms_config);
> */
> struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *vkms_config);
>
> +/**
> + * vkms_config_create_crtc() - Create a crtc configuration
> + *
> + * This will allocate and add a new crtc configuration to @vkms_config.
> + * @vkms_config: Configuration where to insert new crtc configuration
> + */
> +struct vkms_config_crtc *vkms_config_create_crtc(struct vkms_config *vkms_config);
> +
> +/**
> + * vkms_config_create_encoder() - Create an encoder configuration
> + *
> + * This will allocate and add a new encoder configuration to @vkms_config.
> + * @vkms_config: Configuration where to insert new encoder configuration
> + */
> +struct vkms_config_encoder *vkms_config_create_encoder(struct vkms_config *vkms_config);
> +
> +int __must_check vkms_config_plane_attach_crtc(struct vkms_config_plane *vkms_config_plane,
> + struct vkms_config_crtc *vkms_config_crtc);
> +int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *vkms_config_encoder,
> + struct vkms_config_crtc *vkms_config_crtc);
> +
> /**
> * vkms_config_delete_plane() - Remove a plane configuration and frees its memory
> *
> * This will delete a plane configuration from the parent configuration. This will NOT
> - * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane.
> + * cleanup and frees the vkms_plane that can be stored in @vkms_config_plane. It will also remove
> + * any reference to this plane in @vkms_config.
> + *
> * @vkms_config_plane: Plane configuration to cleanup
> + * @vkms_config: Parent configuration
> + */
> +void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane,
> + struct vkms_config *vkms_config);
> +/**
> + * vkms_config_delete_crtc() - Remove a CRTC configuration and frees its memory
> + *
> + * This will delete a CRTC configuration from the parent configuration. This will NOT
> + * cleanup and frees the vkms_crtc that can be stored in @vkms_config_crtc. It will also remove
> + * any reference to this CRTC in @vkms_config.
> + *
> + * @vkms_config_crtc: Plane configuration to cleanup
> + * @vkms_config: Parent configuration
> + */
> +void vkms_config_delete_crtc(struct vkms_config_crtc *vkms_config_crtc,
> + struct vkms_config *vkms_config);
> +/**
> + * vkms_config_delete_encoder() - Remove an encoder configuration and frees its memory
> + *
> + * This will delete an encoder configuration from the parent configuration. This will NOT
> + * cleanup and frees the vkms_encoder that can be stored in @vkms_config_encoder. It will also
> + * remove any reference to this CRTC in @vkms_config.
> + *
> + * @vkms_config_encoder: Plane configuration to cleanup
> + * @vkms_config: Parent configuration
> */
> -void vkms_config_delete_plane(struct vkms_config_plane *vkms_config_plane);
> +void vkms_config_delete_encoder(struct vkms_config_encoder *vkms_config_encoder,
> + struct vkms_config *vkms_config);
>
> /**
> * vkms_config_alloc_default() - Allocate the configuration for the default device
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 6f6d3118b2f2..654238dbba7f 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -281,9 +281,17 @@ static void vkms_crtc_destroy_workqueue(struct drm_device *dev, void *raw_vkms_c
> destroy_workqueue(vkms_crtc->composer_workq);
> }
>
> +static void vkms_crtc_cleanup_writeback_connector(struct drm_device *dev, void *data)
> +{
> + struct vkms_crtc *vkms_crtc = data;
> +
> + drm_writeback_connector_cleanup(&vkms_crtc->wb_connector);
> +}
> +
> struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> struct drm_plane *primary,
> - struct drm_plane *cursor)
> + struct drm_plane *cursor,
> + struct vkms_config_crtc *config)
> {
> struct drm_device *dev = &vkmsdev->drm;
> struct vkms_crtc *vkms_crtc;
> @@ -317,5 +325,15 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> if (ret)
> return ERR_PTR(ret);
>
> + if (config->writeback) {
> + ret = vkms_enable_writeback_connector(vkmsdev, vkms_crtc);
> + if (ret)
> + return ERR_PTR(ret);
> + ret = drmm_add_action_or_reset(&vkmsdev->drm, vkms_crtc_cleanup_writeback_connector,
> + vkms_crtc);
> + if (ret)
> + return ERR_PTR(ret);
> + }
> +
> return vkms_crtc;
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.h b/drivers/gpu/drm/vkms/vkms_crtc.h
> index dde28884a0a5..a271e95f1cfe 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.h
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.h
> @@ -86,5 +86,6 @@ struct vkms_crtc {
> */
> struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
> struct drm_plane *primary,
> - struct drm_plane *cursor);
> + struct drm_plane *cursor,
> + struct vkms_config_crtc *config);
> #endif //_VKMS_CRTC_H
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 403006a0bb61..6deff5099322 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -153,13 +153,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
>
> static int vkms_output_init(struct vkms_device *vkmsdev)
> {
> - struct vkms_config_plane *config_plane;
> + struct vkms_config_encoder *config_encoder;
> struct drm_device *dev = &vkmsdev->drm;
> + struct vkms_config_plane *config_plane;
> + struct vkms_config_crtc *config_crtc;
> struct drm_connector *connector;
> - struct drm_encoder *encoder;
> - struct vkms_crtc *crtc;
> - struct drm_plane *plane;
> - struct vkms_plane *primary, *cursor = NULL;
> + unsigned long idx;
> int ret;
> int writeback;
>
> @@ -169,22 +168,30 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
> ret = PTR_ERR(config_plane->plane);
> return ret;
> }
> -
> - if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
> - primary = config_plane->plane;
> - else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
> - cursor = config_plane->plane;
> }
>
> - /* [1]: Initialize the crtc component */
> - crtc = vkms_crtc_init(vkmsdev, &primary->base,
> - cursor ? &cursor->base : NULL);
> - if (IS_ERR(crtc))
> - return PTR_ERR(crtc);
> + list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
> + struct drm_plane *primary = NULL, *cursor = NULL;
> +
> + xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
> + if (config_plane->type == DRM_PLANE_TYPE_PRIMARY)
> + primary = &config_plane->plane->base;
> + else if (config_plane->type == DRM_PLANE_TYPE_CURSOR)
> + cursor = &config_plane->plane->base;
> + }
> +
> + config_crtc->crtc = vkms_crtc_init(vkmsdev, primary, cursor, config_crtc);
>
> - /* Enable the output's CRTC for all the planes */
> - drm_for_each_plane(plane, &vkmsdev->drm) {
> - plane->possible_crtcs |= drm_crtc_mask(&crtc->base);
> + if (IS_ERR(config_crtc->crtc)) {
> + ret = PTR_ERR(config_crtc->crtc);
> + return ret;
> + }
> + }
> +
> + list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
> + xa_for_each(&config_crtc->possible_planes, idx, config_plane) {
> + config_plane->plane->base.possible_crtcs |= drm_crtc_mask(&config_crtc->crtc->base);
> + }
> }
>
> /* Initialize the connector component */
> @@ -201,32 +208,28 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
>
> drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
>
> - /* Initialize the encoder component */
> - encoder = drmm_kzalloc(&vkmsdev->drm, sizeof(*encoder), GFP_KERNEL);
> - if (!encoder)
> - return -ENOMEM;
> -
> - ret = drmm_encoder_init(dev, encoder, &vkms_encoder_funcs,
> - DRM_MODE_ENCODER_VIRTUAL, NULL);
> - if (ret) {
> - DRM_ERROR("Failed to init encoder\n");
> - return ret;
> - }
> -
> - encoder->possible_crtcs = drm_crtc_mask(&crtc->base);
>
> - /* Attach the encoder and the connector */
> - ret = drm_connector_attach_encoder(connector, encoder);
> - if (ret) {
> - DRM_ERROR("Failed to attach connector to encoder\n");
> - return ret;
> - }
> + list_for_each_entry(config_encoder, &vkmsdev->config->encoders, link) {
> + config_encoder->encoder = drmm_kzalloc(&vkmsdev->drm,
> + sizeof(*config_encoder->encoder),
> + GFP_KERNEL);
> + if (!config_encoder->encoder)
> + return -ENOMEM;
> + ret = drmm_encoder_init(dev, config_encoder->encoder, &vkms_encoder_funcs,
> + DRM_MODE_ENCODER_VIRTUAL, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to init encoder\n");
> + return ret;
> + }
>
> - /* Initialize the writeback component */
> - if (vkmsdev->config->writeback) {
> - writeback = vkms_enable_writeback_connector(vkmsdev, crtc);
> - if (writeback)
> - DRM_ERROR("Failed to init writeback connector\n");
> + xa_for_each(&config_encoder->possible_crtcs, idx, config_crtc) {
> + config_encoder->encoder->possible_crtcs |= drm_crtc_mask(&config_crtc->crtc->base);
> + }
> + if (IS_ERR(config_encoder->encoder))
> + return PTR_ERR(config_encoder->encoder);
> + ret = drm_connector_attach_encoder(connector, config_encoder->encoder);
> + if (ret)
> + return ret;
> }
>
> drm_mode_config_reset(dev);
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 13/15] drm/vkms: Add name configuration for encoders
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (11 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 12/15] drm/vkms: Add configuration for CRTCs and encoders Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 14/15] drm/vkms: Add name configuration for CRTCs Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 15/15] drm/vkms: Add test for config structure Louis Chauvet
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As an encoder will be a directory in ConfigFS, add the configuration for
encoder name so we will be able to reflect the configfs directory name in
the drm name.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 6 ++++++
drivers/gpu/drm/vkms/vkms_config.h | 2 ++
drivers/gpu/drm/vkms/vkms_drv.c | 2 +-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index fcd4a128c21b..7ef525091967 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -39,6 +39,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
encoder = vkms_config_create_encoder(vkms_config);
if (!encoder)
goto err_alloc;
+ encoder->name = kzalloc(sizeof("Main Encoder"), GFP_KERNEL);
+ if (!encoder->name)
+ goto err_alloc;
+ sprintf(encoder->name, "Main Encoder");
if (vkms_config_encoder_attach_crtc(encoder, crtc))
goto err_alloc;
@@ -232,6 +236,7 @@ void vkms_config_delete_encoder(struct vkms_config_encoder *vkms_config_encoder,
}
}
+ kfree(vkms_config_encoder->name);
kfree(vkms_config_encoder);
}
@@ -394,6 +399,7 @@ static int vkms_config_show(struct seq_file *m, void *data)
list_for_each_entry(config_encoder, &vkmsdev->config->encoders, link) {
seq_puts(m, "encoder:\n");
+ seq_printf(m, "\tname: %s\n", config_encoder->name);
}
list_for_each_entry(config_crtc, &vkmsdev->config->crtcs, link) {
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 8f247fc09373..4223edd94ec2 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -50,6 +50,7 @@ struct vkms_config_crtc {
* struct vkms_config_encoder
*
* @link: Link to the others encoders
+ * @name: Name of the encoder
* @possible_crtcs: List of CRTC that can be used with this encoder
* @encoder: Internal usage. This pointer should never be considered as valid. It can be used to
* store a temporary reference to a vkms encoder during device creation. This pointer is
@@ -58,6 +59,7 @@ struct vkms_config_crtc {
struct vkms_config_encoder {
struct list_head link;
+ char *name;
struct xarray possible_crtcs;
/* Internal usage */
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 6deff5099322..cd3920270905 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -216,7 +216,7 @@ static int vkms_output_init(struct vkms_device *vkmsdev)
if (!config_encoder->encoder)
return -ENOMEM;
ret = drmm_encoder_init(dev, config_encoder->encoder, &vkms_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
+ DRM_MODE_ENCODER_VIRTUAL, config_encoder->name);
if (ret) {
DRM_ERROR("Failed to init encoder\n");
return ret;
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 14/15] drm/vkms: Add name configuration for CRTCs
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (12 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 13/15] drm/vkms: Add name configuration for encoders Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
2024-08-14 14:36 ` [PATCH RFC 15/15] drm/vkms: Add test for config structure Louis Chauvet
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
As a CRTC will be a directory in ConfigFS, add the name configuration for
CRTC name so we will be able to reflect the configfs directory name in the
drm name.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/vkms_config.c | 5 +++++
drivers/gpu/drm/vkms/vkms_config.h | 2 ++
drivers/gpu/drm/vkms/vkms_crtc.c | 2 +-
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 7ef525091967..a6bf7ae6d216 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -35,6 +35,10 @@ struct vkms_config *vkms_config_alloc_default(bool enable_writeback, bool enable
if (!crtc)
goto err_alloc;
crtc->writeback = enable_writeback;
+ crtc->name = kzalloc(sizeof("Main CRTC"), GFP_KERNEL);
+ if (!crtc->name)
+ goto err_alloc;
+ sprintf(crtc->name, "Main CRTC");
encoder = vkms_config_create_encoder(vkms_config);
if (!encoder)
@@ -213,6 +217,7 @@ void vkms_config_delete_crtc(struct vkms_config_crtc *vkms_config_crtc,
}
}
+ kfree(vkms_config_crtc->name);
kfree(vkms_config_crtc);
}
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 4223edd94ec2..4a4c16dea785 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -29,6 +29,7 @@ struct vkms_config {
* struct vkms_config_crtc
*
* @link: Link to the others CRTCs
+ * @name: Name of the CRTC
* @possible_planes: List of planes that can be used with this CRTC
* @possible_encoders: List of encoders that can be used with this CRTC
* @crtc: Internal usage. This pointer should never be considered as valid. It can be used to
@@ -38,6 +39,7 @@ struct vkms_config {
struct vkms_config_crtc {
struct list_head link;
+ char *name;
bool writeback;
struct xarray possible_planes;
struct xarray possible_encoders;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 654238dbba7f..5b659d67b9ce 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -298,7 +298,7 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
int ret;
vkms_crtc = drmm_crtc_alloc_with_planes(dev, struct vkms_crtc, base, primary, cursor,
- &vkms_crtc_funcs, NULL);
+ &vkms_crtc_funcs, config->name);
if (IS_ERR(vkms_crtc)) {
DRM_DEV_ERROR(vkmsdev->drm.dev, "Failed to init CRTC\n");
return vkms_crtc;
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 15/15] drm/vkms: Add test for config structure
2024-08-14 14:36 [PATCH RFC 00/15] drm/vkms: Introduce detailed configuration Louis Chauvet
` (13 preceding siblings ...)
2024-08-14 14:36 ` [PATCH RFC 14/15] drm/vkms: Add name configuration for CRTCs Louis Chauvet
@ 2024-08-14 14:36 ` Louis Chauvet
14 siblings, 0 replies; 28+ messages in thread
From: Louis Chauvet @ 2024-08-14 14:36 UTC (permalink / raw)
To: Rodrigo Siqueira, Melissa Wen, Maíra Canal, Haneen Mohammed,
Daniel Vetter, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie
Cc: dri-devel, arthurgrillo, linux-kernel, jeremie.dautheribes,
miquel.raynal, thomas.petazzoni, seanpaul, nicolejadeyee,
Louis Chauvet
The config structure is a bit complex in term of memory management. Add
basic test to avoid breaking it in the future.
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
drivers/gpu/drm/vkms/tests/Makefile | 1 +
drivers/gpu/drm/vkms/tests/vkms_config_test.c | 135 ++++++++++++++++++++++++++
2 files changed, 136 insertions(+)
diff --git a/drivers/gpu/drm/vkms/tests/Makefile b/drivers/gpu/drm/vkms/tests/Makefile
index 2d1df668569e..ec71b5fe7f9d 100644
--- a/drivers/gpu/drm/vkms/tests/Makefile
+++ b/drivers/gpu/drm/vkms/tests/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_format_test.o
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_config_test.o
diff --git a/drivers/gpu/drm/vkms/tests/vkms_config_test.c b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
new file mode 100644
index 000000000000..6ec0021a8df3
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_config_test.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <kunit/test.h>
+
+#include "../vkms_config.h"
+
+static void vkms_config_test_basic_allocation(struct kunit *test)
+{
+ struct vkms_config *config = vkms_config_create();
+
+ KUNIT_EXPECT_TRUE_MSG(test, list_empty(&config->encoders),
+ "Encoder list is not empty after allocation");
+ KUNIT_EXPECT_TRUE_MSG(test, list_empty(&config->crtcs),
+ "CRTC list is not empty after allocation");
+ KUNIT_EXPECT_TRUE_MSG(test, list_empty(&config->planes),
+ "Plane list is not empty after allocation");
+
+ vkms_config_destroy(config);
+}
+
+static void vkms_config_test_simple_config(struct kunit *test)
+{
+ struct vkms_config *config = vkms_config_create();
+
+ struct vkms_config_plane *plane_1 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_2 = vkms_config_create_plane(config);
+ struct vkms_config_crtc *crtc = vkms_config_create_crtc(config);
+ struct vkms_config_encoder *encoder = vkms_config_create_encoder(config);
+
+ KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->planes), 2);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->crtcs), 1);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->encoders), 1);
+
+ plane_1->type = DRM_PLANE_TYPE_PRIMARY;
+ plane_2->type = DRM_PLANE_TYPE_CURSOR;
+
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_1, crtc), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_2, crtc), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder, crtc), 0);
+
+ KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
+
+ vkms_config_delete_plane(plane_1, config);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->planes), 1);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->crtcs), 1);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->encoders), 1);
+
+ KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
+
+ plane_2->type = DRM_PLANE_TYPE_PRIMARY;
+
+ KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
+
+ vkms_config_destroy(config);
+}
+
+static void vkms_config_test_complex_config(struct kunit *test)
+{
+ struct vkms_config *config = vkms_config_create();
+
+ struct vkms_config_plane *plane_1 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_2 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_3 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_4 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_5 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_6 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_7 = vkms_config_create_plane(config);
+ struct vkms_config_plane *plane_8 = vkms_config_create_plane(config);
+ struct vkms_config_crtc *crtc_1 = vkms_config_create_crtc(config);
+ struct vkms_config_crtc *crtc_2 = vkms_config_create_crtc(config);
+ struct vkms_config_encoder *encoder_1 = vkms_config_create_encoder(config);
+ struct vkms_config_encoder *encoder_2 = vkms_config_create_encoder(config);
+ struct vkms_config_encoder *encoder_3 = vkms_config_create_encoder(config);
+ struct vkms_config_encoder *encoder_4 = vkms_config_create_encoder(config);
+
+ KUNIT_EXPECT_FALSE(test, vkms_config_is_valid(config));
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->planes), 8);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->crtcs), 2);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->encoders), 4);
+
+ plane_1->type = DRM_PLANE_TYPE_PRIMARY;
+ plane_2->type = DRM_PLANE_TYPE_CURSOR;
+ plane_3->type = DRM_PLANE_TYPE_OVERLAY;
+ plane_4->type = DRM_PLANE_TYPE_OVERLAY;
+ plane_5->type = DRM_PLANE_TYPE_PRIMARY;
+ plane_6->type = DRM_PLANE_TYPE_CURSOR;
+ plane_7->type = DRM_PLANE_TYPE_OVERLAY;
+ plane_8->type = DRM_PLANE_TYPE_OVERLAY;
+
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_1, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_2, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_3, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_4, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_5, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_6, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_7, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_8, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_3, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_plane_attach_crtc(plane_4, crtc_2), 0);
+
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder_1, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder_2, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder_3, crtc_1), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder_3, crtc_2), 0);
+ KUNIT_EXPECT_EQ(test, vkms_config_encoder_attach_crtc(encoder_4, crtc_2), 0);
+
+ KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
+
+ vkms_config_delete_plane(plane_4, config);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->planes), 7);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->crtcs), 2);
+ KUNIT_EXPECT_EQ(test, list_count_nodes(&config->encoders), 4);
+
+ KUNIT_EXPECT_TRUE(test, vkms_config_is_valid(config));
+
+ vkms_config_destroy(config);
+}
+
+static struct kunit_case vkms_config_test_cases[] = {
+ KUNIT_CASE(vkms_config_test_basic_allocation),
+ KUNIT_CASE(vkms_config_test_simple_config),
+ KUNIT_CASE(vkms_config_test_complex_config),
+ {}
+};
+
+static struct kunit_suite vkms_config_test_suite = {
+ .name = "vkms-config",
+ .test_cases = vkms_config_test_cases,
+};
+
+kunit_test_suite(vkms_config_test_suite);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Kunit test for vkms config utility");
--
2.44.2
^ permalink raw reply related [flat|nested] 28+ messages in thread