* [PATCH 1/5] drm/vkms: Merge default_config and device
[not found] <20220722213214.1377835-1-jshargo@chromium.org>
@ 2022-07-22 21:32 ` Jim Shargo
2022-08-05 15:39 ` Sean Paul
2022-07-22 21:32 ` [PATCH 2/5] drm/vkms: VKMS now supports more than one "card" Jim Shargo
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jim Shargo @ 2022-07-22 21:32 UTC (permalink / raw)
To: jshargo, Rodrigo Siqueira, Melissa Wen, Haneen Mohammed,
Daniel Vetter, David Airlie
Cc: Jim Shargo, linux-kernel, dri-devel
This is a small refactor to make ConfigFS support easier.
vkms_config is now a member of vkms_device and we now store a top-level
reference to vkms_device.
This should be a no-op refactor.
Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
drivers/gpu/drm/vkms/vkms_drv.c | 58 +++++++++---------------------
drivers/gpu/drm/vkms/vkms_drv.h | 5 ++-
drivers/gpu/drm/vkms/vkms_output.c | 6 ++--
3 files changed, 22 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 0ffe5f0e33f7..81ed9417e511 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -37,7 +37,7 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-static struct vkms_config *default_config;
+static struct vkms_device *vkms_device;
static bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
@@ -91,13 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
static int vkms_config_show(struct seq_file *m, void *data)
{
- struct drm_info_node *node = (struct drm_info_node *)m->private;
- struct drm_device *dev = node->minor->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);
+ seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
+ seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
+ seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
return 0;
}
@@ -158,11 +154,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
return vkms_output_init(vkmsdev, 0);
}
-static int vkms_create(struct vkms_config *config)
+static int vkms_create(void)
{
int ret;
struct platform_device *pdev;
- struct vkms_device *vkms_device;
pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
if (IS_ERR(pdev))
@@ -179,9 +174,11 @@ static int vkms_create(struct vkms_config *config)
ret = PTR_ERR(vkms_device);
goto out_devres;
}
+
vkms_device->platform = pdev;
- vkms_device->config = config;
- config->dev = vkms_device;
+ vkms_device->config.cursor = enable_cursor;
+ vkms_device->config.writeback = enable_writeback;
+ vkms_device->config.overlay = enable_overlay;
ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
DMA_BIT_MASK(64));
@@ -207,6 +204,8 @@ static int vkms_create(struct vkms_config *config)
drm_fbdev_generic_setup(&vkms_device->drm, 0);
+ vkms_device->initialized = true;
+
return 0;
out_devres:
@@ -218,46 +217,23 @@ static int vkms_create(struct vkms_config *config)
static int __init vkms_init(void)
{
- struct vkms_config *config;
-
- config = kmalloc(sizeof(*config), GFP_KERNEL);
- if (!config)
- return -ENOMEM;
-
- default_config = config;
-
- config->cursor = enable_cursor;
- config->writeback = enable_writeback;
- config->overlay = enable_overlay;
-
- return vkms_create(config);
+ return vkms_create();
}
-static void vkms_destroy(struct vkms_config *config)
+static void __exit vkms_exit(void)
{
struct platform_device *pdev;
- if (!config->dev) {
- DRM_INFO("vkms_device is NULL.\n");
+ if (!vkms_device || !vkms_device->initialized) {
return;
}
- pdev = config->dev->platform;
+ pdev = vkms_device->platform;
- drm_dev_unregister(&config->dev->drm);
- drm_atomic_helper_shutdown(&config->dev->drm);
+ drm_dev_unregister(&vkms_device->drm);
+ drm_atomic_helper_shutdown(&vkms_device->drm);
devres_release_group(&pdev->dev, NULL);
platform_device_unregister(pdev);
-
- config->dev = NULL;
-}
-
-static void __exit vkms_exit(void)
-{
- if (default_config->dev)
- vkms_destroy(default_config);
-
- kfree(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 91e63b12f60f..c7ebc4ee6b14 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -99,15 +99,14 @@ struct vkms_config {
bool writeback;
bool cursor;
bool overlay;
- /* only set when instantiated */
- struct vkms_device *dev;
};
struct vkms_device {
struct drm_device drm;
struct platform_device *platform;
struct vkms_output output;
- const struct vkms_config *config;
+ struct vkms_config config;
+ bool initialized;
};
#define drm_crtc_to_vkms_output(target) \
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index ba0e82ae549a..d0061c82003a 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -63,7 +63,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
if (IS_ERR(primary))
return PTR_ERR(primary);
- if (vkmsdev->config->overlay) {
+ if (vkmsdev->config.overlay) {
for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
if (ret)
@@ -71,7 +71,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
}
}
- if (vkmsdev->config->cursor) {
+ if (vkmsdev->config.cursor) {
cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
if (IS_ERR(cursor))
return PTR_ERR(cursor);
@@ -103,7 +103,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
- if (vkmsdev->config->writeback) {
+ if (vkmsdev->config.writeback) {
writeback = vkms_enable_writeback_connector(vkmsdev);
if (writeback)
DRM_ERROR("Failed to init writeback connector\n");
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/5] drm/vkms: Merge default_config and device
2022-07-22 21:32 ` [PATCH 1/5] drm/vkms: Merge default_config and device Jim Shargo
@ 2022-08-05 15:39 ` Sean Paul
0 siblings, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-05 15:39 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Jul 22, 2022 at 05:32:09PM -0400, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier.
>
> vkms_config is now a member of vkms_device and we now store a top-level
> reference to vkms_device.
>
> This should be a no-op refactor.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 58 +++++++++---------------------
> drivers/gpu/drm/vkms/vkms_drv.h | 5 ++-
> drivers/gpu/drm/vkms/vkms_output.c | 6 ++--
> 3 files changed, 22 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 0ffe5f0e33f7..81ed9417e511 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -37,7 +37,7 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -static struct vkms_config *default_config;
> +static struct vkms_device *vkms_device;
I think this should be stored in the platform device data on registration as
opposed to a global.
>
> static bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> @@ -91,13 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> - struct drm_info_node *node = (struct drm_info_node *)m->private;
> - struct drm_device *dev = node->minor->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);
> + seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> + seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> + seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
>
> return 0;
> }
> @@ -158,11 +154,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> return vkms_output_init(vkmsdev, 0);
> }
>
> -static int vkms_create(struct vkms_config *config)
> +static int vkms_create(void)
> {
> int ret;
> struct platform_device *pdev;
> - struct vkms_device *vkms_device;
>
> pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> if (IS_ERR(pdev))
> @@ -179,9 +174,11 @@ static int vkms_create(struct vkms_config *config)
> ret = PTR_ERR(vkms_device);
> goto out_devres;
> }
> +
In order to avoid the vkms_device global you would call platform_set_drvdata()
here and use platform_get_drvdata() to retrieve it elsewhere.
> vkms_device->platform = pdev;
> - vkms_device->config = config;
> - config->dev = vkms_device;
> + vkms_device->config.cursor = enable_cursor;
> + vkms_device->config.writeback = enable_writeback;
> + vkms_device->config.overlay = enable_overlay;
>
> ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> DMA_BIT_MASK(64));
> @@ -207,6 +204,8 @@ static int vkms_create(struct vkms_config *config)
>
> drm_fbdev_generic_setup(&vkms_device->drm, 0);
>
> + vkms_device->initialized = true;
> +
Do we really need this? If so, is there a race between this and the check in vkms_exit(), or do you get serialization for free from module init/exit?
> return 0;
>
> out_devres:
> @@ -218,46 +217,23 @@ static int vkms_create(struct vkms_config *config)
>
> static int __init vkms_init(void)
> {
> - struct vkms_config *config;
> -
> - config = kmalloc(sizeof(*config), GFP_KERNEL);
> - if (!config)
> - return -ENOMEM;
> -
> - default_config = config;
> -
> - config->cursor = enable_cursor;
> - config->writeback = enable_writeback;
> - config->overlay = enable_overlay;
> -
> - return vkms_create(config);
> + return vkms_create();
> }
>
> -static void vkms_destroy(struct vkms_config *config)
> +static void __exit vkms_exit(void)
> {
> struct platform_device *pdev;
>
> - if (!config->dev) {
> - DRM_INFO("vkms_device is NULL.\n");
> + if (!vkms_device || !vkms_device->initialized) {
> return;
> }
>
> - pdev = config->dev->platform;
> + pdev = vkms_device->platform;
>
> - drm_dev_unregister(&config->dev->drm);
> - drm_atomic_helper_shutdown(&config->dev->drm);
> + drm_dev_unregister(&vkms_device->drm);
> + drm_atomic_helper_shutdown(&vkms_device->drm);
> devres_release_group(&pdev->dev, NULL);
> platform_device_unregister(pdev);
> -
> - config->dev = NULL;
> -}
> -
> -static void __exit vkms_exit(void)
> -{
> - if (default_config->dev)
> - vkms_destroy(default_config);
> -
> - kfree(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 91e63b12f60f..c7ebc4ee6b14 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -99,15 +99,14 @@ struct vkms_config {
> bool writeback;
> bool cursor;
> bool overlay;
> - /* only set when instantiated */
> - struct vkms_device *dev;
> };
>
> struct vkms_device {
> struct drm_device drm;
> struct platform_device *platform;
> struct vkms_output output;
> - const struct vkms_config *config;
> + struct vkms_config config;
> + bool initialized;
> };
>
> #define drm_crtc_to_vkms_output(target) \
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index ba0e82ae549a..d0061c82003a 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -63,7 +63,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> - if (vkmsdev->config->overlay) {
> + if (vkmsdev->config.overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> if (ret)
> @@ -71,7 +71,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> }
> }
>
> - if (vkmsdev->config->cursor) {
> + if (vkmsdev->config.cursor) {
> cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> if (IS_ERR(cursor))
> return PTR_ERR(cursor);
> @@ -103,7 +103,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> - if (vkmsdev->config->writeback) {
> + if (vkmsdev->config.writeback) {
> writeback = vkms_enable_writeback_connector(vkmsdev);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/5] drm/vkms: VKMS now supports more than one "card"
[not found] <20220722213214.1377835-1-jshargo@chromium.org>
2022-07-22 21:32 ` [PATCH 1/5] drm/vkms: Merge default_config and device Jim Shargo
@ 2022-07-22 21:32 ` Jim Shargo
2022-08-05 18:36 ` Sean Paul
2022-07-22 21:32 ` [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card Jim Shargo
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Jim Shargo @ 2022-07-22 21:32 UTC (permalink / raw)
To: jshargo, Rodrigo Siqueira, Melissa Wen, Haneen Mohammed,
Daniel Vetter, David Airlie
Cc: Jim Shargo, linux-kernel, dri-devel
This is a small refactor to make ConfigFS support easier.
We now store the vkms_device statically, and maintain a list of
"cards", each representing a different virtual DRM driver.
We also make it clear when a card is "default", that is created at
initialization, and not. This is because, due to limitations on what we
can do with the configfs API, the default card won't be reflected in
configfs and is initialized in a unique way.
Since we're only managing a single card, this should be a no-op.
Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
drivers/gpu/drm/vkms/vkms_crtc.c | 11 +-
drivers/gpu/drm/vkms/vkms_drv.c | 160 ++++++++++++++++----------
drivers/gpu/drm/vkms/vkms_drv.h | 32 ++++--
drivers/gpu/drm/vkms/vkms_output.c | 25 ++--
drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++--
6 files changed, 158 insertions(+), 94 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 57bbd32e9beb..c1b632952532 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -62,9 +62,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
static int vkms_enable_vblank(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
+ struct vkms_card *card = drm_device_to_vkms_card(dev);
+ struct vkms_output *out = &card->output;
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
drm_calc_timestamping_constants(crtc, &crtc->mode);
@@ -78,7 +79,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
static void vkms_disable_vblank(struct drm_crtc *crtc)
{
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct drm_device *dev = crtc->dev;
+ struct vkms_card *card = drm_device_to_vkms_card(dev);
+ struct vkms_output *out = &card->output;
hrtimer_cancel(&out->vblank_hrtimer);
}
@@ -88,9 +91,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
bool in_vblank_irq)
{
struct drm_device *dev = crtc->dev;
+ struct vkms_card *card = drm_device_to_vkms_card(dev);
+ struct vkms_output *output = &card->output;
unsigned int pipe = crtc->index;
- struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
- struct vkms_output *output = &vkmsdev->output;
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
if (!READ_ONCE(vblank->enabled)) {
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 81ed9417e511..92fbade2199b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -9,9 +9,9 @@
* the GPU in DRM API tests.
*/
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/platform_device.h>
-#include <linux/dma-mapping.h>
#include <drm/drm_gem.h>
#include <drm/drm_atomic.h>
@@ -37,7 +37,7 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-static struct vkms_device *vkms_device;
+static struct vkms_device vkms_device;
static bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
@@ -55,9 +55,9 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
static void vkms_release(struct drm_device *dev)
{
- struct vkms_device *vkms = drm_device_to_vkms_device(dev);
+ struct vkms_card *card = drm_device_to_vkms_card(dev);
- destroy_workqueue(vkms->output.composer_workq);
+ destroy_workqueue(card->output.composer_workq);
}
static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
@@ -91,9 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
static int vkms_config_show(struct seq_file *m, void *data)
{
- seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
- seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
- seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
+ seq_printf(m, "writeback=%d\n", vkms_device.config.writeback);
+ seq_printf(m, "cursor=%d\n", vkms_device.config.cursor);
+ seq_printf(m, "overlay=%d\n", vkms_device.config.overlay);
return 0;
}
@@ -133,9 +133,9 @@ static const struct drm_mode_config_helper_funcs vkms_mode_config_helpers = {
.atomic_commit_tail = vkms_atomic_commit_tail,
};
-static int vkms_modeset_init(struct vkms_device *vkmsdev)
+static int vkms_modeset_init(struct vkms_card *card)
{
- struct drm_device *dev = &vkmsdev->drm;
+ struct drm_device *dev = &card->drm;
drm_mode_config_init(dev);
dev->mode_config.funcs = &vkms_mode_funcs;
@@ -151,89 +151,133 @@ 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 card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
}
-static int vkms_create(void)
+static void vkms_device_destroy(void)
{
- int ret;
+ struct vkms_card *card, *n;
+
+ list_for_each_entry_safe(card, n, &vkms_device.cards, node) {
+ vkms_card_destroy(card);
+ }
+
+ memset(&vkms_device, 0, sizeof(struct vkms_device));
+}
+
+struct vkms_card *vkms_card_init(const char *name, bool is_default)
+{
+ char unique_name[64] = { 0 };
struct platform_device *pdev;
+ struct vkms_card *card;
+ void *grp;
+ int ret;
- pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+ ret = snprintf(unique_name, ARRAY_SIZE(unique_name), "%s-%s",
+ DRIVER_NAME, name);
+ if (ARRAY_SIZE(unique_name) <= ret) {
+ DRM_WARN("Truncated vkms card driver name '%s-%s' to '%s'\n",
+ DRIVER_NAME, name, unique_name);
+ }
+
+ pdev = platform_device_register_simple(unique_name, -1, NULL, 0);
if (IS_ERR(pdev))
- return PTR_ERR(pdev);
+ return ERR_PTR(PTR_ERR(pdev));
- if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
- ret = -ENOMEM;
- goto out_unregister;
+ grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
+ if (!grp) {
+ ret = ENOMEM;
+ goto out_platform_device;
}
- 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;
+ card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
+ drm);
+ if (IS_ERR(card)) {
+ ret = PTR_ERR(card);
+ goto out_release_group;
}
-
- vkms_device->platform = pdev;
- vkms_device->config.cursor = enable_cursor;
- vkms_device->config.writeback = enable_writeback;
- vkms_device->config.overlay = enable_overlay;
- ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
- DMA_BIT_MASK(64));
+ strncpy(card->name_buf, unique_name, ARRAY_SIZE(card->name_buf) - 1);
+ card->platform = pdev;
+ card->drm.unique = card->name_buf;
+ card->vkms_device = &vkms_device;
+ card->is_default = is_default;
+ card->is_registered = false;
+ card->resource_group_id = grp;
+ ret = dma_coerce_mask_and_coherent(card->drm.dev, DMA_BIT_MASK(64));
if (ret) {
DRM_ERROR("Could not initialize DMA support\n");
- goto out_devres;
+ goto out_release_group;
}
- ret = drm_vblank_init(&vkms_device->drm, 1);
+ ret = drm_vblank_init(&card->drm, 1);
if (ret) {
DRM_ERROR("Failed to vblank\n");
- goto out_devres;
+ goto out_release_group;
}
- ret = vkms_modeset_init(vkms_device);
- if (ret)
- goto out_devres;
-
- ret = drm_dev_register(&vkms_device->drm, 0);
- if (ret)
- goto out_devres;
+ ret = vkms_modeset_init(card);
+ if (ret) {
+ DRM_ERROR("Unable to initialize modesetting");
+ goto out_release_group;
+ }
- drm_fbdev_generic_setup(&vkms_device->drm, 0);
+ ret = drm_dev_register(&card->drm, 0);
+ if (ret) {
+ DRM_ERROR("Unable to register card");
+ return ERR_PTR(ret);
+ }
- vkms_device->initialized = true;
+ drm_fbdev_generic_setup(&card->drm, 0);
+ card->is_registered = true;
- return 0;
+ devres_close_group(&pdev->dev, grp);
+ list_add_tail(&card->node, &vkms_device.cards);
+
+ return card;
-out_devres:
- devres_release_group(&pdev->dev, NULL);
-out_unregister:
+out_release_group:
+ devres_release_group(&pdev->dev, grp);
+out_platform_device:
platform_device_unregister(pdev);
- return ret;
+ return ERR_PTR(ret);
}
-static int __init vkms_init(void)
+void vkms_card_destroy(struct vkms_card *card)
{
- return vkms_create();
+ list_del(&card->node);
+ if (card->is_registered) {
+ drm_dev_unregister(&card->drm);
+ drm_atomic_helper_shutdown(&card->drm);
+ devres_release_group(&card->platform->dev,
+ card->resource_group_id);
+ platform_device_unregister(card->platform);
+ }
}
-static void __exit vkms_exit(void)
+static int __init vkms_init(void)
{
- struct platform_device *pdev;
-
- if (!vkms_device || !vkms_device->initialized) {
- return;
+ struct vkms_card *card;
+
+ vkms_device.config.cursor = enable_cursor;
+ vkms_device.config.writeback = enable_writeback;
+ vkms_device.config.overlay = enable_overlay;
+ INIT_LIST_HEAD(&vkms_device.cards);
+
+ card = vkms_card_init("default", /* configfs */ NULL);
+ if (IS_ERR(card)) {
+ DRM_ERROR("Unable to init default card");
+ vkms_device_destroy();
+ return PTR_ERR(card);
}
- pdev = vkms_device->platform;
+ return 0;
+}
- drm_dev_unregister(&vkms_device->drm);
- drm_atomic_helper_shutdown(&vkms_device->drm);
- devres_release_group(&pdev->dev, NULL);
- platform_device_unregister(pdev);
+static void __exit vkms_exit(void)
+{
+ vkms_device_destroy();
}
module_init(vkms_init);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index c7ebc4ee6b14..1a98b81d6f22 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -4,6 +4,7 @@
#define _VKMS_DRV_H_
#include <linux/hrtimer.h>
+#include <linux/kernel.h>
#include <drm/drm.h>
#include <drm/drm_gem.h>
@@ -101,19 +102,29 @@ struct vkms_config {
bool overlay;
};
-struct vkms_device {
- struct drm_device drm;
+struct vkms_card {
struct platform_device *platform;
+ struct drm_device drm;
+ struct list_head node;
struct vkms_output output;
+ struct vkms_device *vkms_device;
+ /* storage for the value of drm->unique, giving this dev a unique busid */
+ char name_buf[64];
+ void *resource_group_id;
+ bool is_default;
+ bool is_registered;
+};
+
+struct vkms_device {
+ struct list_head cards;
struct vkms_config config;
- bool initialized;
};
#define drm_crtc_to_vkms_output(target) \
container_of(target, struct vkms_output, crtc)
-#define drm_device_to_vkms_device(target) \
- container_of(target, struct vkms_device, drm)
+#define drm_device_to_vkms_card(target) \
+ container_of(target, struct vkms_card, drm)
#define to_vkms_crtc_state(target)\
container_of(target, struct vkms_crtc_state, base)
@@ -121,13 +132,18 @@ struct vkms_device {
#define to_vkms_plane_state(target)\
container_of(target, struct vkms_plane_state, base.base)
+/* Cards */
+struct vkms_card *vkms_card_init(const char *name, bool is_default);
+void vkms_card_destroy(struct vkms_card *card);
+
/* CRTC */
int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
struct drm_plane *primary, struct drm_plane *cursor);
-int vkms_output_init(struct vkms_device *vkmsdev, int index);
+int vkms_output_init_default(struct vkms_card *card, int index);
+int vkms_output_init(struct vkms_card *card, int index);
-struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
+struct vkms_plane *vkms_plane_init(struct vkms_card *card,
enum drm_plane_type type, int index);
/* CRC Support */
@@ -142,6 +158,6 @@ void vkms_composer_worker(struct work_struct *work);
void vkms_set_composer(struct vkms_output *out, bool enabled);
/* Writeback */
-int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
+int vkms_enable_writeback_connector(struct vkms_card *card);
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index d0061c82003a..dafd47c0a54d 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -32,12 +32,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
.get_modes = vkms_conn_get_modes,
};
-static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
+static int vkms_add_overlay_plane(struct vkms_card *card, int index,
struct drm_crtc *crtc)
{
struct vkms_plane *overlay;
- overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
+ overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
if (IS_ERR(overlay))
return PTR_ERR(overlay);
@@ -47,10 +47,11 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
return 0;
}
-int vkms_output_init(struct vkms_device *vkmsdev, int index)
+int vkms_output_init_default(struct vkms_card *card, int index)
{
- struct vkms_output *output = &vkmsdev->output;
- struct drm_device *dev = &vkmsdev->drm;
+ const struct vkms_config *config = &card->vkms_device->config;
+ struct vkms_output *output = &card->output;
+ struct drm_device *dev = &card->drm;
struct drm_connector *connector = &output->connector;
struct drm_encoder *encoder = &output->encoder;
struct drm_crtc *crtc = &output->crtc;
@@ -59,20 +60,20 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
int writeback;
unsigned int n;
- primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
+ primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
if (IS_ERR(primary))
return PTR_ERR(primary);
- if (vkmsdev->config.overlay) {
+ if (config->overlay) {
for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
- ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
+ ret = vkms_add_overlay_plane(card, index, crtc);
if (ret)
return ret;
}
}
- if (vkmsdev->config.cursor) {
- cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
+ if (config->cursor) {
+ cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
if (IS_ERR(cursor))
return PTR_ERR(cursor);
}
@@ -103,8 +104,8 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
goto err_attach;
}
- if (vkmsdev->config.writeback) {
- writeback = vkms_enable_writeback_connector(vkmsdev);
+ if (config->writeback) {
+ writeback = vkms_enable_writeback_connector(card);
if (writeback)
DRM_ERROR("Failed to init writeback connector\n");
}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index d8eb674b49a6..28abd61a0bb9 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -158,10 +158,10 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
};
-struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
+struct vkms_plane *vkms_plane_init(struct vkms_card *card,
enum drm_plane_type type, int index)
{
- struct drm_device *dev = &vkmsdev->drm;
+ struct drm_device *dev = &card->drm;
const struct drm_plane_helper_funcs *funcs;
struct vkms_plane *plane;
const u32 *formats;
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index af1604dfbbaf..681e7267d688 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -94,15 +94,15 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
struct drm_writeback_job *job)
{
struct vkms_writeback_job *vkmsjob = job->priv;
- struct vkms_device *vkmsdev;
+ struct vkms_card *card;
if (!job->fb)
return;
drm_gem_fb_vunmap(job->fb, vkmsjob->map);
- vkmsdev = drm_device_to_vkms_device(job->fb->dev);
- vkms_set_composer(&vkmsdev->output, false);
+ card = drm_device_to_vkms_card(job->fb->dev);
+ vkms_set_composer(&card->output, false);
kfree(vkmsjob);
}
@@ -111,8 +111,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
{
struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
conn);
- struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
- struct vkms_output *output = &vkmsdev->output;
+ struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
+ struct vkms_output *output = &card->output;
struct drm_writeback_connector *wb_conn = &output->wb_connector;
struct drm_connector_state *conn_state = wb_conn->base.state;
struct vkms_crtc_state *crtc_state = output->composer_state;
@@ -120,7 +120,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
if (!conn_state)
return;
- vkms_set_composer(&vkmsdev->output, true);
+ vkms_set_composer(&card->output, true);
spin_lock_irq(&output->composer_lock);
crtc_state->active_writeback = conn_state->writeback_job->priv;
@@ -136,14 +136,14 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
.atomic_commit = vkms_wb_atomic_commit,
};
-int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
+int vkms_enable_writeback_connector(struct vkms_card *card)
{
- struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
+ struct drm_writeback_connector *wb = &card->output.wb_connector;
- vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
+ card->output.wb_connector.encoder.possible_crtcs = 1;
drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
- return drm_writeback_connector_init(&vkmsdev->drm, wb,
+ return drm_writeback_connector_init(&card->drm, wb,
&vkms_wb_connector_funcs,
&vkms_wb_encoder_helper_funcs,
vkms_wb_formats,
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/5] drm/vkms: VKMS now supports more than one "card"
2022-07-22 21:32 ` [PATCH 2/5] drm/vkms: VKMS now supports more than one "card" Jim Shargo
@ 2022-08-05 18:36 ` Sean Paul
0 siblings, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-05 18:36 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier.
>
> We now store the vkms_device statically, and maintain a list of
> "cards", each representing a different virtual DRM driver.
>
> We also make it clear when a card is "default", that is created at
> initialization, and not. This is because, due to limitations on what we
> can do with the configfs API, the default card won't be reflected in
> configfs and is initialized in a unique way.
>
> Since we're only managing a single card, this should be a no-op.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
Re-sending the review with the correct subject and reply-to. Let's use this one
for discussion and pretend the other one doesn't exist :-)
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 11 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 160 ++++++++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.h | 32 ++++--
> drivers/gpu/drm/vkms/vkms_output.c | 25 ++--
> drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
> drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++--
> 6 files changed, 158 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..c1b632952532 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -62,9 +62,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> @@ -78,7 +79,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
>
> hrtimer_cancel(&out->vblank_hrtimer);
> }
> @@ -88,9 +91,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *output = &card->output;
> unsigned int pipe = crtc->index;
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> - struct vkms_output *output = &vkmsdev->output;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> if (!READ_ONCE(vblank->enabled)) {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 81ed9417e511..92fbade2199b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,9 +9,9 @@
> * the GPU in DRM API tests.
> */
>
> +#include <linux/kernel.h>
Unless you're using something directly from this header, it's probably best
not to include it as it pulls in a _lot_ of stuff.
> #include <linux/module.h>
> #include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
>
> #include <drm/drm_gem.h>
> #include <drm/drm_atomic.h>
> @@ -37,7 +37,7 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -static struct vkms_device *vkms_device;
> +static struct vkms_device vkms_device;
>
> static bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> @@ -55,9 +55,9 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>
> static void vkms_release(struct drm_device *dev)
> {
> - struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(vkms->output.composer_workq);
> + destroy_workqueue(card->output.composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -91,9 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> - seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> - seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> - seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
> + seq_printf(m, "writeback=%d\n", vkms_device.config.writeback);
> + seq_printf(m, "cursor=%d\n", vkms_device.config.cursor);
> + seq_printf(m, "overlay=%d\n", vkms_device.config.overlay);
>
> return 0;
> }
> @@ -133,9 +133,9 @@ static const struct drm_mode_config_helper_funcs vkms_mode_config_helpers = {
> .atomic_commit_tail = vkms_atomic_commit_tail,
> };
>
> -static int vkms_modeset_init(struct vkms_device *vkmsdev)
> +static int vkms_modeset_init(struct vkms_card *card)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
>
> drm_mode_config_init(dev);
> dev->mode_config.funcs = &vkms_mode_funcs;
> @@ -151,89 +151,133 @@ 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 card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
AFAICT if card is not default this failure will roll up to vkms_card_init and
to vkms_init() which will cause everything to get torn down. So we can probably
check at the start of the function and fail early.
Also, minor nit: Ternary operators are usually discouraged.
> }
>
> -static int vkms_create(void)
> +static void vkms_device_destroy(void)
> {
> - int ret;
> + struct vkms_card *card, *n;
> +
> + list_for_each_entry_safe(card, n, &vkms_device.cards, node) {
> + vkms_card_destroy(card);
> + }
Nit: no braces
> +
> + memset(&vkms_device, 0, sizeof(struct vkms_device));
Nit: You could do sizeof(vkms_device) here (sidebar: naming the variable the
same as the struct is unfortunate).
That said, this is either unnecessary or not a good idea since it blows away at
least the backpointer to platform_device, clears drm_device, and other items
which have proper destroy/clean-up functions. So we either don't need this, or
we need to clean up everything in vkms_device explicitly.
> +}
> +
> +struct vkms_card *vkms_card_init(const char *name, bool is_default)
> +{
> + char unique_name[64] = { 0 };
> struct platform_device *pdev;
> + struct vkms_card *card;
> + void *grp;
> + int ret;
>
> - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> + ret = snprintf(unique_name, ARRAY_SIZE(unique_name), "%s-%s",
> + DRIVER_NAME, name);
> + if (ARRAY_SIZE(unique_name) <= ret) {
> + DRM_WARN("Truncated vkms card driver name '%s-%s' to '%s'\n",
Nit: The DRM_* print macros are deprecated in favor of the drm_* print
functions. You probably also want to use a categorized message here instead of
just warn, perhaps drm_dbg() which defaults to DRIVER category.
You might consider converting the driver to drm_* before this patchset so we're
working from a consistent base.
> + DRIVER_NAME, name, unique_name);
> + }
> +
> + pdev = platform_device_register_simple(unique_name, -1, NULL, 0);
> if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> + return ERR_PTR(PTR_ERR(pdev));
return pdev;
>
> - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> - ret = -ENOMEM;
> - goto out_unregister;
> + grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
> + if (!grp) {
> + ret = ENOMEM;
> + goto out_platform_device;
> }
>
> - 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;
> + card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
> + drm);
> + if (IS_ERR(card)) {
> + ret = PTR_ERR(card);
> + goto out_release_group;
> }
> -
> - vkms_device->platform = pdev;
> - vkms_device->config.cursor = enable_cursor;
> - vkms_device->config.writeback = enable_writeback;
> - vkms_device->config.overlay = enable_overlay;
>
> - ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> - DMA_BIT_MASK(64));
> + strncpy(card->name_buf, unique_name, ARRAY_SIZE(card->name_buf) - 1);
> + card->platform = pdev;
> + card->drm.unique = card->name_buf;
> + card->vkms_device = &vkms_device;
> + card->is_default = is_default;
> + card->is_registered = false;
> + card->resource_group_id = grp;
>
> + ret = dma_coerce_mask_and_coherent(card->drm.dev, DMA_BIT_MASK(64));
> if (ret) {
> DRM_ERROR("Could not initialize DMA support\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = drm_vblank_init(&vkms_device->drm, 1);
> + ret = drm_vblank_init(&card->drm, 1);
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = vkms_modeset_init(vkms_device);
> - if (ret)
> - goto out_devres;
> -
> - ret = drm_dev_register(&vkms_device->drm, 0);
> - if (ret)
> - goto out_devres;
> + ret = vkms_modeset_init(card);
> + if (ret) {
> + DRM_ERROR("Unable to initialize modesetting");
> + goto out_release_group;
> + }
>
> - drm_fbdev_generic_setup(&vkms_device->drm, 0);
> + ret = drm_dev_register(&card->drm, 0);
> + if (ret) {
> + DRM_ERROR("Unable to register card");
> + return ERR_PTR(ret);
No cleanup required if this fails?
> + }
>
> - vkms_device->initialized = true;
> + drm_fbdev_generic_setup(&card->drm, 0);
> + card->is_registered = true;
Re-raising the race concern here.
>
> - return 0;
> + devres_close_group(&pdev->dev, grp);
> + list_add_tail(&card->node, &vkms_device.cards);
I am still unclear why we need to bookkeep the cards ourselves since we're
spawning a new platform_device for each drm_device. It feels like perhaps it
would be easier to spawn a new platform_driver for each new drm device with its
own .probe & .remove function which keeps track of its own instance. This would
avoid the necessity of vkms_card, each platform_driver would have its own
vkms_device containing a drm_device.
I'm assuming that rmmod would invoke .remove on all the sub platform_drivers,
but if not you could keep track of them in the platform device and unregister
each one on unload which would be a lot less statekeeping.
> +
> + return card;
>
> -out_devres:
> - devres_release_group(&pdev->dev, NULL);
> -out_unregister:
> +out_release_group:
> + devres_release_group(&pdev->dev, grp);
> +out_platform_device:
> platform_device_unregister(pdev);
> - return ret;
> + return ERR_PTR(ret);
> }
>
> -static int __init vkms_init(void)
> +void vkms_card_destroy(struct vkms_card *card)
> {
> - return vkms_create();
> + list_del(&card->node);
> + if (card->is_registered) {
Nit: You could save yourself some indentation here and exit early.
> + drm_dev_unregister(&card->drm);
> + drm_atomic_helper_shutdown(&card->drm);
> + devres_release_group(&card->platform->dev,
> + card->resource_group_id);
> + platform_device_unregister(card->platform);
> + }
> }
>
> -static void __exit vkms_exit(void)
> +static int __init vkms_init(void)
> {
> - struct platform_device *pdev;
> -
> - if (!vkms_device || !vkms_device->initialized) {
> - return;
> + struct vkms_card *card;
> +
> + vkms_device.config.cursor = enable_cursor;
> + vkms_device.config.writeback = enable_writeback;
> + vkms_device.config.overlay = enable_overlay;
> + INIT_LIST_HEAD(&vkms_device.cards);
> +
> + card = vkms_card_init("default", /* configfs */ NULL);
This passes NULL for the bool is_default argument. Presumably this should be
true, or perhaps you don't need the second arg at all right now.
> + if (IS_ERR(card)) {
> + DRM_ERROR("Unable to init default card");
> + vkms_device_destroy();
> + return PTR_ERR(card);
> }
>
> - pdev = vkms_device->platform;
> + return 0;
> +}
>
> - drm_dev_unregister(&vkms_device->drm);
> - drm_atomic_helper_shutdown(&vkms_device->drm);
> - devres_release_group(&pdev->dev, NULL);
> - platform_device_unregister(pdev);
> +static void __exit vkms_exit(void)
> +{
> + vkms_device_destroy();
> }
>
> module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c7ebc4ee6b14..1a98b81d6f22 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -4,6 +4,7 @@
> #define _VKMS_DRV_H_
>
> #include <linux/hrtimer.h>
> +#include <linux/kernel.h>
From kernel.h:
* This header has combined a lot of unrelated to each other stuff.
* The process of splitting its content is in progress while keeping
* backward compatibility. That's why it's highly recommended NOT to
* include this header inside another header file, especially under
* generic or architectural include/ directory.
>
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> @@ -101,19 +102,29 @@ struct vkms_config {
> bool overlay;
> };
>
> -struct vkms_device {
> - struct drm_device drm;
> +struct vkms_card {
> struct platform_device *platform;
> + struct drm_device drm;
> + struct list_head node;
Nit: node is pretty ambiguous
> struct vkms_output output;
> + struct vkms_device *vkms_device;
> + /* storage for the value of drm->unique, giving this dev a unique busid */
I didn't quite grok this comment, mind expanding?
> + char name_buf[64];
> + void *resource_group_id;
> + bool is_default;
> + bool is_registered;
> +};
> +
> +struct vkms_device {
> + struct list_head cards;
> struct vkms_config config;
> - bool initialized;
> };
>
> #define drm_crtc_to_vkms_output(target) \
> container_of(target, struct vkms_output, crtc)
>
> -#define drm_device_to_vkms_device(target) \
> - container_of(target, struct vkms_device, drm)
> +#define drm_device_to_vkms_card(target) \
> + container_of(target, struct vkms_card, drm)
>
> #define to_vkms_crtc_state(target)\
> container_of(target, struct vkms_crtc_state, base)
> @@ -121,13 +132,18 @@ struct vkms_device {
> #define to_vkms_plane_state(target)\
> container_of(target, struct vkms_plane_state, base.base)
>
> +/* Cards */
> +struct vkms_card *vkms_card_init(const char *name, bool is_default);
> +void vkms_card_destroy(struct vkms_card *card);
> +
> /* CRTC */
> int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane *cursor);
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index);
> +int vkms_output_init_default(struct vkms_card *card, int index);
> +int vkms_output_init(struct vkms_card *card, int index);
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index);
>
> /* CRC Support */
> @@ -142,6 +158,6 @@ void vkms_composer_worker(struct work_struct *work);
> void vkms_set_composer(struct vkms_output *out, bool enabled);
>
> /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> +int vkms_enable_writeback_connector(struct vkms_card *card);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index d0061c82003a..dafd47c0a54d 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,12 +32,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> +static int vkms_add_overlay_plane(struct vkms_card *card, int index,
> struct drm_crtc *crtc)
> {
> struct vkms_plane *overlay;
>
> - overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
> + overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
> if (IS_ERR(overlay))
> return PTR_ERR(overlay);
>
> @@ -47,10 +47,11 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> return 0;
> }
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index)
> +int vkms_output_init_default(struct vkms_card *card, int index)
> {
> - struct vkms_output *output = &vkmsdev->output;
> - struct drm_device *dev = &vkmsdev->drm;
> + const struct vkms_config *config = &card->vkms_device->config;
> + struct vkms_output *output = &card->output;
> + struct drm_device *dev = &card->drm;
> struct drm_connector *connector = &output->connector;
> struct drm_encoder *encoder = &output->encoder;
> struct drm_crtc *crtc = &output->crtc;
> @@ -59,20 +60,20 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> int writeback;
> unsigned int n;
>
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> + primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> - if (vkmsdev->config.overlay) {
> + if (config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> + ret = vkms_add_overlay_plane(card, index, crtc);
> if (ret)
> return ret;
> }
> }
>
> - if (vkmsdev->config.cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> + if (config->cursor) {
> + cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
> if (IS_ERR(cursor))
> return PTR_ERR(cursor);
> }
> @@ -103,8 +104,8 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> - if (vkmsdev->config.writeback) {
> - writeback = vkms_enable_writeback_connector(vkmsdev);
> + if (config->writeback) {
> + writeback = vkms_enable_writeback_connector(card);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index d8eb674b49a6..28abd61a0bb9 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -158,10 +158,10 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> };
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
> const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> const u32 *formats;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index af1604dfbbaf..681e7267d688 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -94,15 +94,15 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> struct drm_writeback_job *job)
> {
> struct vkms_writeback_job *vkmsjob = job->priv;
> - struct vkms_device *vkmsdev;
> + struct vkms_card *card;
>
> if (!job->fb)
> return;
>
> drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>
> - vkmsdev = drm_device_to_vkms_device(job->fb->dev);
> - vkms_set_composer(&vkmsdev->output, false);
> + card = drm_device_to_vkms_card(job->fb->dev);
> + vkms_set_composer(&card->output, false);
> kfree(vkmsjob);
> }
>
> @@ -111,8 +111,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> {
> struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> conn);
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> - struct vkms_output *output = &vkmsdev->output;
> + struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
> + struct vkms_output *output = &card->output;
> struct drm_writeback_connector *wb_conn = &output->wb_connector;
> struct drm_connector_state *conn_state = wb_conn->base.state;
> struct vkms_crtc_state *crtc_state = output->composer_state;
> @@ -120,7 +120,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> if (!conn_state)
> return;
>
> - vkms_set_composer(&vkmsdev->output, true);
> + vkms_set_composer(&card->output, true);
>
> spin_lock_irq(&output->composer_lock);
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> @@ -136,14 +136,14 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> .atomic_commit = vkms_wb_atomic_commit,
> };
>
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> +int vkms_enable_writeback_connector(struct vkms_card *card)
> {
> - struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> + struct drm_writeback_connector *wb = &card->output.wb_connector;
>
> - vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> + card->output.wb_connector.encoder.possible_crtcs = 1;
> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>
> - return drm_writeback_connector_init(&vkmsdev->drm, wb,
> + return drm_writeback_connector_init(&card->drm, wb,
> &vkms_wb_connector_funcs,
> &vkms_wb_encoder_helper_funcs,
> vkms_wb_formats,
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
[not found] <20220722213214.1377835-1-jshargo@chromium.org>
2022-07-22 21:32 ` [PATCH 1/5] drm/vkms: Merge default_config and device Jim Shargo
2022-07-22 21:32 ` [PATCH 2/5] drm/vkms: VKMS now supports more than one "card" Jim Shargo
@ 2022-07-22 21:32 ` Jim Shargo
2022-08-05 18:27 ` Sean Paul
2022-08-05 20:03 ` Sean Paul
2022-07-22 21:32 ` [PATCH 4/5] drm/vkms: Add ConfigFS scaffolding to VKMS Jim Shargo
2022-07-22 21:32 ` [PATCH 5/5] drm/vkms: Support registering configfs devices Jim Shargo
4 siblings, 2 replies; 12+ messages in thread
From: Jim Shargo @ 2022-07-22 21:32 UTC (permalink / raw)
To: jshargo, Rodrigo Siqueira, Melissa Wen, Haneen Mohammed,
Daniel Vetter, David Airlie
Cc: Jim Shargo, linux-kernel, dri-devel
This change supports multiple CRTCs, encoders, connectors instead of one
of each per card.
Since ConfigFS-based devices will support multiple crtcs, it's useful to
move all of the writeback/composition data from being a per-output thing
to being a per-CRTC thing.
Since there's still only ever one CRTC, this should be a no-op refactor.
Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
drivers/gpu/drm/vkms/vkms_composer.c | 28 +++----
drivers/gpu/drm/vkms/vkms_crtc.c | 91 +++++++++++----------
drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
drivers/gpu/drm/vkms/vkms_drv.h | 66 ++++++++++------
drivers/gpu/drm/vkms/vkms_output.c | 110 ++++++++++++++++++--------
drivers/gpu/drm/vkms/vkms_plane.c | 39 ++++++---
drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++---
7 files changed, 225 insertions(+), 137 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index c6a1036bf2ea..d27c0831d857 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -221,7 +221,7 @@ void vkms_composer_worker(struct work_struct *work)
struct vkms_crtc_state,
composer_work);
struct drm_crtc *crtc = crtc_state->base.crtc;
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
struct vkms_composer *primary_composer = NULL;
struct vkms_plane_state *act_plane = NULL;
bool crc_pending, wb_pending;
@@ -230,7 +230,7 @@ void vkms_composer_worker(struct work_struct *work)
u64 frame_start, frame_end;
int ret;
- spin_lock_irq(&out->composer_lock);
+ spin_lock_irq(&vkms_crtc->composer_lock);
frame_start = crtc_state->frame_start;
frame_end = crtc_state->frame_end;
crc_pending = crtc_state->crc_pending;
@@ -238,7 +238,7 @@ void vkms_composer_worker(struct work_struct *work)
crtc_state->frame_start = 0;
crtc_state->frame_end = 0;
crtc_state->crc_pending = false;
- spin_unlock_irq(&out->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
/*
* We raced with the vblank hrtimer and previous work already computed
@@ -270,10 +270,10 @@ void vkms_composer_worker(struct work_struct *work)
crc32 = compute_crc(vaddr_out, primary_composer);
if (wb_pending) {
- drm_writeback_signal_completion(&out->wb_connector, 0);
- spin_lock_irq(&out->composer_lock);
+ drm_writeback_signal_completion(&vkms_crtc->wb_connector, 0);
+ spin_lock_irq(&vkms_crtc->composer_lock);
crtc_state->wb_pending = false;
- spin_unlock_irq(&out->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
} else {
kfree(vaddr_out);
}
@@ -325,25 +325,25 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *src_name,
return 0;
}
-void vkms_set_composer(struct vkms_output *out, bool enabled)
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled)
{
bool old_enabled;
if (enabled)
- drm_crtc_vblank_get(&out->crtc);
+ drm_crtc_vblank_get(&vkms_crtc->base);
- spin_lock_irq(&out->lock);
- old_enabled = out->composer_enabled;
- out->composer_enabled = enabled;
- spin_unlock_irq(&out->lock);
+ spin_lock_irq(&vkms_crtc->lock);
+ old_enabled = vkms_crtc->composer_enabled;
+ vkms_crtc->composer_enabled = enabled;
+ spin_unlock_irq(&vkms_crtc->lock);
if (old_enabled)
- drm_crtc_vblank_put(&out->crtc);
+ drm_crtc_vblank_put(&vkms_crtc->base);
}
int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
{
- struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *out = drm_crtc_to_vkms_crtc(crtc);
bool enabled = false;
int ret = 0;
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index c1b632952532..d93678d984ae 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -11,35 +11,34 @@
static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
{
- struct vkms_output *output = container_of(timer, struct vkms_output,
- vblank_hrtimer);
- struct drm_crtc *crtc = &output->crtc;
+ struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
+ struct drm_crtc *crtc = &vkms_crtc->base;
struct vkms_crtc_state *state;
u64 ret_overrun;
bool ret, fence_cookie;
fence_cookie = dma_fence_begin_signalling();
- ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
- output->period_ns);
+ ret_overrun = hrtimer_forward_now(&vkms_crtc->vblank_hrtimer,
+ vkms_crtc->period_ns);
if (ret_overrun != 1)
pr_warn("%s: vblank timer overrun\n", __func__);
- spin_lock(&output->lock);
+ spin_lock(&vkms_crtc->lock);
ret = drm_crtc_handle_vblank(crtc);
if (!ret)
DRM_ERROR("vkms failure on handling vblank");
- state = output->composer_state;
- spin_unlock(&output->lock);
+ state = vkms_crtc->composer_state;
+ spin_unlock(&vkms_crtc->lock);
- if (state && output->composer_enabled) {
+ if (state && vkms_crtc->composer_enabled) {
u64 frame = drm_crtc_accurate_vblank_count(crtc);
/* update frame_start only if a queued vkms_composer_worker()
* has read the data
*/
- spin_lock(&output->composer_lock);
+ spin_lock(&vkms_crtc->composer_lock);
if (!state->crc_pending)
state->frame_start = frame;
else
@@ -47,9 +46,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
state->frame_start, frame);
state->frame_end = frame;
state->crc_pending = true;
- spin_unlock(&output->composer_lock);
+ spin_unlock(&vkms_crtc->composer_lock);
- ret = queue_work(output->composer_workq, &state->composer_work);
+ ret = queue_work(vkms_crtc->composer_workq, &state->composer_work);
if (!ret)
DRM_DEBUG_DRIVER("Composer worker already queued\n");
}
@@ -62,28 +61,24 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
static int vkms_enable_vblank(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *out = &card->output;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
unsigned int pipe = drm_crtc_index(crtc);
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
drm_calc_timestamping_constants(crtc, &crtc->mode);
- hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- out->vblank_hrtimer.function = &vkms_vblank_simulate;
- out->period_ns = ktime_set(0, vblank->framedur_ns);
- hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
+ hrtimer_init(&vkms_crtc->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ vkms_crtc->vblank_hrtimer.function = &vkms_vblank_simulate;
+ vkms_crtc->period_ns = ktime_set(0, vblank->framedur_ns);
+ hrtimer_start(&vkms_crtc->vblank_hrtimer, vkms_crtc->period_ns, HRTIMER_MODE_REL);
return 0;
}
static void vkms_disable_vblank(struct drm_crtc *crtc)
{
- struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *out = &card->output;
-
- hrtimer_cancel(&out->vblank_hrtimer);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
+ hrtimer_cancel(&vkms_crtc->vblank_hrtimer);
}
static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
@@ -91,8 +86,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
bool in_vblank_irq)
{
struct drm_device *dev = crtc->dev;
- struct vkms_card *card = drm_device_to_vkms_card(dev);
- struct vkms_output *output = &card->output;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
unsigned int pipe = crtc->index;
struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
@@ -101,7 +95,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
return true;
}
- *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
+ *vblank_time = READ_ONCE(vkms_crtc->vblank_hrtimer.node.expires);
if (WARN_ON(*vblank_time == vblank->time))
return true;
@@ -113,7 +107,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
* the vblank core expects. Therefore we need to always correct the
* timestampe by one frame.
*/
- *vblank_time -= output->period_ns;
+ *vblank_time -= vkms_crtc->period_ns;
return true;
}
@@ -240,18 +234,18 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
- struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
/* This lock is held across the atomic commit to block vblank timer
* from scheduling vkms_composer_worker until the composer is updated
*/
- spin_lock_irq(&vkms_output->lock);
+ spin_lock_irq(&vkms_crtc->lock);
}
static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
- struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
if (crtc->state->event) {
spin_lock(&crtc->dev->event_lock);
@@ -266,9 +260,9 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
crtc->state->event = NULL;
}
- vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
+ vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
- spin_unlock_irq(&vkms_output->lock);
+ spin_unlock_irq(&vkms_crtc->lock);
}
static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
@@ -279,27 +273,38 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
.atomic_disable = vkms_crtc_atomic_disable,
};
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
+struct vkms_crtc *vkms_crtc_init(struct vkms_card *card,
struct drm_plane *primary, struct drm_plane *cursor)
{
- struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
+ struct drm_device *dev = &card->drm;
+ struct vkms_crtc *vkms_crtc;
int ret;
- ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
+ if (card->output.num_crtcs >= VKMS_MAX_OUTPUT_OBJECTS) {
+ return ERR_PTR(ENOMEM);
+ }
+ vkms_crtc = &card->output.crtcs[card->output.num_crtcs++];
+
+ ret = drm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
&vkms_crtc_funcs, NULL);
if (ret) {
DRM_ERROR("Failed to init CRTC\n");
- return ret;
+ goto out_error;
}
- drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
+ drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
- spin_lock_init(&vkms_out->lock);
- spin_lock_init(&vkms_out->composer_lock);
+ spin_lock_init(&vkms_crtc->lock);
+ spin_lock_init(&vkms_crtc->composer_lock);
- vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
- if (!vkms_out->composer_workq)
- return -ENOMEM;
+ vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
+ if (!vkms_crtc->composer_workq)
+ goto out_error;
+
+ return vkms_crtc;
- return ret;
+out_error:
+ memset(vkms_crtc, 0, sizeof(*vkms_crtc));
+ card->output.num_crtcs -= 1;
+ return ERR_PTR(ret);
}
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 92fbade2199b..266c9f643f70 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
{
struct vkms_card *card = drm_device_to_vkms_card(dev);
- destroy_workqueue(card->output.composer_workq);
+ for (int i = 0; i < card->output.num_crtcs; i++)
+ destroy_workqueue(card->output.crtcs[i].composer_workq);
}
static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 1a98b81d6f22..30ae06d7af71 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -23,6 +23,8 @@
#define NUM_OVERLAY_PLANES 8
+#define VKMS_MAX_OUTPUT_OBJECTS 16
+
struct vkms_writeback_job {
struct iosys_map map[DRM_FORMAT_MAX_PLANES];
struct iosys_map data[DRM_FORMAT_MAX_PLANES];
@@ -51,6 +53,27 @@ struct vkms_plane {
struct drm_plane base;
};
+struct vkms_output;
+
+struct vkms_crtc {
+ struct drm_crtc base;
+
+ struct drm_writeback_connector wb_connector;
+ struct hrtimer vblank_hrtimer;
+ ktime_t period_ns;
+ struct drm_pending_vblank_event *event;
+ /* ordered wq for composer_work */
+ struct workqueue_struct *composer_workq;
+ /* protects concurrent access to composer */
+ spinlock_t lock;
+
+ /* protected by @lock */
+ bool composer_enabled;
+ struct vkms_crtc_state *composer_state;
+
+ spinlock_t composer_lock;
+};
+
/**
* vkms_crtc_state - Driver specific CRTC state
* @base: base CRTC state
@@ -75,23 +98,14 @@ struct vkms_crtc_state {
};
struct vkms_output {
- struct drm_crtc crtc;
- struct drm_encoder encoder;
- struct drm_connector connector;
- struct drm_writeback_connector wb_connector;
- struct hrtimer vblank_hrtimer;
- ktime_t period_ns;
- struct drm_pending_vblank_event *event;
- /* ordered wq for composer_work */
- struct workqueue_struct *composer_workq;
- /* protects concurrent access to composer */
- spinlock_t lock;
-
- /* protected by @lock */
- bool composer_enabled;
- struct vkms_crtc_state *composer_state;
-
- spinlock_t composer_lock;
+ int num_crtcs;
+ struct vkms_crtc crtcs[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_encoders;
+ struct drm_encoder encoders[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_connectors;
+ struct drm_connector connectors[VKMS_MAX_OUTPUT_OBJECTS];
+ int num_planes;
+ struct vkms_plane planes[VKMS_MAX_OUTPUT_OBJECTS];
};
struct vkms_device;
@@ -120,12 +134,15 @@ struct vkms_device {
struct vkms_config config;
};
-#define drm_crtc_to_vkms_output(target) \
- container_of(target, struct vkms_output, crtc)
+#define drm_crtc_to_vkms_crtc(crtc)\
+ container_of(crtc, struct vkms_crtc, base)
-#define drm_device_to_vkms_card(target) \
+#define drm_device_to_vkms_card(target) \
container_of(target, struct vkms_card, drm)
+#define timer_to_vkms_crtc(timer)\
+ container_of(timer, struct vkms_crtc, vblank_hrtimer)
+
#define to_vkms_crtc_state(target)\
container_of(target, struct vkms_crtc_state, base)
@@ -137,14 +154,13 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default);
void vkms_card_destroy(struct vkms_card *card);
/* CRTC */
-int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
- struct drm_plane *primary, struct drm_plane *cursor);
+struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
int vkms_output_init_default(struct vkms_card *card, int index);
int vkms_output_init(struct vkms_card *card, int index);
struct vkms_plane *vkms_plane_init(struct vkms_card *card,
- enum drm_plane_type type, int index);
+ enum drm_plane_type type);
/* CRC Support */
const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
@@ -155,9 +171,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
/* Composer Support */
void vkms_composer_worker(struct work_struct *work);
-void vkms_set_composer(struct vkms_output *out, bool enabled);
+void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
/* Writeback */
-int vkms_enable_writeback_connector(struct vkms_card *card);
+int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index dafd47c0a54d..2b72d8e763a8 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -32,19 +32,44 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
.get_modes = vkms_conn_get_modes,
};
-static int vkms_add_overlay_plane(struct vkms_card *card, int index,
- struct drm_crtc *crtc)
+static struct drm_connector *vkms_connector_init(struct vkms_card *card)
{
- struct vkms_plane *overlay;
+ struct drm_connector *connector;
+ int ret;
- overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
- if (IS_ERR(overlay))
- return PTR_ERR(overlay);
+ if (card->output.num_connectors >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
- if (!overlay->base.possible_crtcs)
- overlay->base.possible_crtcs = drm_crtc_mask(crtc);
+ connector = &card->output.connectors[card->output.num_connectors++];
+ ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
+ DRM_MODE_CONNECTOR_VIRTUAL);
+ if (ret) {
+ memset(connector, 0, sizeof(*connector));
+ card->output.num_connectors -= 1;
+ return ERR_PTR(ret);
+ }
- return 0;
+ drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
+
+ return connector;
+}
+
+static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
+{
+ struct drm_encoder *encoder;
+ int ret;
+
+ if (card->output.num_encoders >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
+
+ encoder = &card->output.encoders[card->output.num_encoders++];
+ ret = drm_simple_encoder_init(&card->drm, encoder, DRM_MODE_ENCODER_VIRTUAL);
+ if (ret) {
+ memset(encoder, 0, sizeof(*encoder));
+ card->output.num_encoders -= 1;
+ return ERR_PTR(ret);
+ }
+ return encoder;
}
int vkms_output_init_default(struct vkms_card *card, int index)
@@ -52,51 +77,65 @@ int vkms_output_init_default(struct vkms_card *card, int index)
const struct vkms_config *config = &card->vkms_device->config;
struct vkms_output *output = &card->output;
struct drm_device *dev = &card->drm;
- struct drm_connector *connector = &output->connector;
- struct drm_encoder *encoder = &output->encoder;
- struct drm_crtc *crtc = &output->crtc;
+ struct drm_connector *connector;
+ struct drm_encoder *encoder;
+ struct vkms_crtc *vkms_crtc;
struct vkms_plane *primary, *cursor = NULL;
int ret;
int writeback;
unsigned int n;
- primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
+ primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY);
if (IS_ERR(primary))
return PTR_ERR(primary);
if (config->overlay) {
for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
- ret = vkms_add_overlay_plane(card, index, crtc);
- if (ret)
- return ret;
+ struct vkms_plane *overlay =
+ vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
+ if (IS_ERR(overlay)) {
+ ret = PTR_ERR(overlay);
+ goto err_planes;
+ }
}
}
if (config->cursor) {
- cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
- if (IS_ERR(cursor))
- return PTR_ERR(cursor);
+ cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
+ if (IS_ERR(cursor)) {
+ ret = PTR_ERR(cursor);
+ goto err_planes;
+ }
}
- ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
- if (ret)
- return ret;
+ vkms_crtc = vkms_crtc_init(card, &primary->base,
+ cursor ? &cursor->base : NULL);
+ if (IS_ERR(vkms_crtc)) {
+ DRM_ERROR("Failed to init crtc\n");
+ ret = PTR_ERR(vkms_crtc);
+ goto err_planes;
+ }
- ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
- DRM_MODE_CONNECTOR_VIRTUAL);
- if (ret) {
+ for (int i = 0; i < card->output.num_planes; i++) {
+ card->output.planes[i].base.possible_crtcs |=
+ drm_crtc_mask(&vkms_crtc->base);
+ }
+
+ connector = vkms_connector_init(card);
+ if (IS_ERR(connector)) {
DRM_ERROR("Failed to init connector\n");
+ ret = PTR_ERR(connector);
goto err_connector;
}
- drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
-
- ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
- if (ret) {
+ encoder = vkms_encoder_init(card);
+ if (IS_ERR(encoder)) {
DRM_ERROR("Failed to init encoder\n");
+ ret = PTR_ERR(encoder);
goto err_encoder;
}
- encoder->possible_crtcs = 1;
+ encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
+
ret = drm_connector_attach_encoder(connector, encoder);
if (ret) {
@@ -105,7 +144,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
}
if (config->writeback) {
- writeback = vkms_enable_writeback_connector(card);
+ writeback = vkms_enable_writeback_connector(card, vkms_crtc);
if (writeback)
DRM_ERROR("Failed to init writeback connector\n");
}
@@ -121,7 +160,14 @@ int vkms_output_init_default(struct vkms_card *card, int index)
drm_connector_cleanup(connector);
err_connector:
- drm_crtc_cleanup(crtc);
+ drm_crtc_cleanup(&vkms_crtc->base);
+
+err_planes:
+ for (int i = 0; i < output->num_planes; i++) {
+ drm_plane_cleanup(&output->planes[i].base);
+ }
+
+ memset(output, 0, sizeof(struct vkms_output));
return ret;
}
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 28abd61a0bb9..5f49257fe16e 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -7,6 +7,7 @@
#include <drm/drm_fourcc.h>
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_plane.h>
#include <drm/drm_plane_helper.h>
#include "vkms_drv.h"
@@ -65,6 +66,19 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
kfree(vkms_state);
}
+static void vkms_plane_destroy(struct drm_plane *plane)
+{
+ struct vkms_plane *vkms_plane =
+ container_of(plane, struct vkms_plane, base);
+
+ if (plane->state) {
+ vkms_plane_destroy_state(plane, plane->state);
+ plane->state = NULL;
+ }
+
+ memset(vkms_plane, 0, sizeof(struct vkms_plane));
+}
+
static void vkms_plane_reset(struct drm_plane *plane)
{
struct vkms_plane_state *vkms_state;
@@ -86,8 +100,9 @@ static void vkms_plane_reset(struct drm_plane *plane)
static const struct drm_plane_funcs vkms_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .reset = vkms_plane_reset,
- .atomic_duplicate_state = vkms_plane_duplicate_state,
+ .destroy = vkms_plane_destroy,
+ .reset = vkms_plane_reset,
+ .atomic_duplicate_state = vkms_plane_duplicate_state,
.atomic_destroy_state = vkms_plane_destroy_state,
};
@@ -159,13 +174,20 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
};
struct vkms_plane *vkms_plane_init(struct vkms_card *card,
- enum drm_plane_type type, int index)
+ enum drm_plane_type type)
{
struct drm_device *dev = &card->drm;
+ struct vkms_output *output = &card->output;
const struct drm_plane_helper_funcs *funcs;
struct vkms_plane *plane;
const u32 *formats;
int nformats;
+ int ret;
+
+ if (output->num_planes >= VKMS_MAX_OUTPUT_OBJECTS)
+ return ERR_PTR(ENOMEM);
+
+ plane = &output->planes[output->num_planes++];
switch (type) {
case DRM_PLANE_TYPE_PRIMARY:
@@ -186,12 +208,11 @@ struct vkms_plane *vkms_plane_init(struct vkms_card *card,
break;
}
- plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
- &vkms_plane_funcs,
- formats, nformats,
- NULL, type, NULL);
- if (IS_ERR(plane))
- return plane;
+ ret = drm_universal_plane_init(dev, &plane->base, 0,
+ &vkms_plane_funcs, formats, nformats,
+ NULL, type, NULL);
+ if (ret)
+ return ERR_PTR(ret);
drm_plane_helper_add(&plane->base, funcs);
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index 681e7267d688..68c7a77ab93f 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0+
#include <linux/iosys-map.h>
+#include <linux/kernel.h>
#include <drm/drm_atomic.h>
#include <drm/drm_fourcc.h>
@@ -94,15 +95,14 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
struct drm_writeback_job *job)
{
struct vkms_writeback_job *vkmsjob = job->priv;
- struct vkms_card *card;
+ struct vkms_crtc *vkms_crtc = container_of(connector, struct vkms_crtc, wb_connector);
if (!job->fb)
return;
drm_gem_fb_vunmap(job->fb, vkmsjob->map);
- card = drm_device_to_vkms_card(job->fb->dev);
- vkms_set_composer(&card->output, false);
+ vkms_set_composer(vkms_crtc, false);
kfree(vkmsjob);
}
@@ -111,21 +111,20 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
{
struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
conn);
- struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
- struct vkms_output *output = &card->output;
- struct drm_writeback_connector *wb_conn = &output->wb_connector;
+ struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(connector_state->crtc);
+ struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
struct drm_connector_state *conn_state = wb_conn->base.state;
- struct vkms_crtc_state *crtc_state = output->composer_state;
+ struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
if (!conn_state)
return;
- vkms_set_composer(&card->output, true);
+ vkms_set_composer(vkms_crtc, true);
- spin_lock_irq(&output->composer_lock);
+ spin_lock_irq(&vkms_crtc->composer_lock);
crtc_state->active_writeback = conn_state->writeback_job->priv;
crtc_state->wb_pending = true;
- spin_unlock_irq(&output->composer_lock);
+ spin_unlock_irq(&vkms_crtc->composer_lock);
drm_writeback_queue_job(wb_conn, connector_state);
}
@@ -136,11 +135,11 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
.atomic_commit = vkms_wb_atomic_commit,
};
-int vkms_enable_writeback_connector(struct vkms_card *card)
+int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc)
{
- struct drm_writeback_connector *wb = &card->output.wb_connector;
+ struct drm_writeback_connector *wb = &vkms_crtc->wb_connector;
- card->output.wb_connector.encoder.possible_crtcs = 1;
+ vkms_crtc->wb_connector.encoder.possible_crtcs = 1;
drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
return drm_writeback_connector_init(&card->drm, wb,
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
2022-07-22 21:32 ` [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card Jim Shargo
@ 2022-08-05 18:27 ` Sean Paul
2022-08-05 18:34 ` Sean Paul
2022-08-05 20:03 ` Sean Paul
1 sibling, 1 reply; 12+ messages in thread
From: Sean Paul @ 2022-08-05 18:27 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier.
>
> We now store the vkms_device statically, and maintain a list of
> "cards", each representing a different virtual DRM driver.
>
> We also make it clear when a card is "default", that is created at
> initialization, and not. This is because, due to limitations on what we
> can do with the configfs API, the default card won't be reflected in
> configfs and is initialized in a unique way.
>
> Since we're only managing a single card, this should be a no-op.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 11 +-
> drivers/gpu/drm/vkms/vkms_drv.c | 160 ++++++++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.h | 32 ++++--
> drivers/gpu/drm/vkms/vkms_output.c | 25 ++--
> drivers/gpu/drm/vkms/vkms_plane.c | 4 +-
> drivers/gpu/drm/vkms/vkms_writeback.c | 20 ++--
> 6 files changed, 158 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 57bbd32e9beb..c1b632952532 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -62,9 +62,10 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> @@ -78,7 +79,9 @@ static int vkms_enable_vblank(struct drm_crtc *crtc)
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct vkms_output *out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *out = &card->output;
>
> hrtimer_cancel(&out->vblank_hrtimer);
> }
> @@ -88,9 +91,9 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
> + struct vkms_output *output = &card->output;
> unsigned int pipe = crtc->index;
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> - struct vkms_output *output = &vkmsdev->output;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> if (!READ_ONCE(vblank->enabled)) {
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 81ed9417e511..92fbade2199b 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,9 +9,9 @@
> * the GPU in DRM API tests.
> */
>
> +#include <linux/kernel.h>
Unless you're using something directly from this header, it's probably best not to include it as it pulls in a _lot_ of stuff.
> #include <linux/module.h>
> #include <linux/platform_device.h>
> -#include <linux/dma-mapping.h>
>
> #include <drm/drm_gem.h>
> #include <drm/drm_atomic.h>
> @@ -37,7 +37,7 @@
> #define DRIVER_MAJOR 1
> #define DRIVER_MINOR 0
>
> -static struct vkms_device *vkms_device;
> +static struct vkms_device vkms_device;
>
> static bool enable_cursor = true;
> module_param_named(enable_cursor, enable_cursor, bool, 0444);
> @@ -55,9 +55,9 @@ DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>
> static void vkms_release(struct drm_device *dev)
> {
> - struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> + struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(vkms->output.composer_workq);
> + destroy_workqueue(card->output.composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> @@ -91,9 +91,9 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>
> static int vkms_config_show(struct seq_file *m, void *data)
> {
> - seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> - seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> - seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
> + seq_printf(m, "writeback=%d\n", vkms_device.config.writeback);
> + seq_printf(m, "cursor=%d\n", vkms_device.config.cursor);
> + seq_printf(m, "overlay=%d\n", vkms_device.config.overlay);
>
> return 0;
> }
> @@ -133,9 +133,9 @@ static const struct drm_mode_config_helper_funcs vkms_mode_config_helpers = {
> .atomic_commit_tail = vkms_atomic_commit_tail,
> };
>
> -static int vkms_modeset_init(struct vkms_device *vkmsdev)
> +static int vkms_modeset_init(struct vkms_card *card)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
>
> drm_mode_config_init(dev);
> dev->mode_config.funcs = &vkms_mode_funcs;
> @@ -151,89 +151,133 @@ 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 card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
AFAICT if card is not default this failure will roll up to vkms_card_init and to vkms_init() which will cause everything to get torn down. So we can probably check at the start of the function and fail early.
Also, minor nit: Ternary operators are usually discouraged.
> }
>
> -static int vkms_create(void)
> +static void vkms_device_destroy(void)
> {
> - int ret;
> + struct vkms_card *card, *n;
> +
> + list_for_each_entry_safe(card, n, &vkms_device.cards, node) {
> + vkms_card_destroy(card);
> + }
Nit: no braces
> +
> + memset(&vkms_device, 0, sizeof(struct vkms_device));
Nit: You could do sizeof(vkms_device) here
That said, this is either unnecessary or not a good idea since it blows away at least the backpointer to platform_device, clears drm_device, and other items which have proper destroy/clean-up functions. So we either don't need this, or we need to clean up everything in vkms_device explicitly.
> +}
> +
> +struct vkms_card *vkms_card_init(const char *name, bool is_default)
> +{
> + char unique_name[64] = { 0 };
> struct platform_device *pdev;
> + struct vkms_card *card;
> + void *grp;
> + int ret;
>
> - pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> + ret = snprintf(unique_name, ARRAY_SIZE(unique_name), "%s-%s",
> + DRIVER_NAME, name);
> + if (ARRAY_SIZE(unique_name) <= ret) {
> + DRM_WARN("Truncated vkms card driver name '%s-%s' to '%s'\n",
Nit: The DRM_* print macros are deprecated in favor of the drm_* print functions. You probably also want to use a categorized message here instead of just warn, perhaps drm_dbg() which defaults to DRIVER category.
You might consider converting the driver to drm_* before this patchset so we're working from a consistent base.
> + DRIVER_NAME, name, unique_name);
> + }
> +
> + pdev = platform_device_register_simple(unique_name, -1, NULL, 0);
> if (IS_ERR(pdev))
> - return PTR_ERR(pdev);
> + return ERR_PTR(PTR_ERR(pdev));
return pdev;
>
> - if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> - ret = -ENOMEM;
> - goto out_unregister;
> + grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
> + if (!grp) {
> + ret = ENOMEM;
> + goto out_platform_device;
> }
>
> - 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;
> + card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
> + drm);
> + if (IS_ERR(card)) {
> + ret = PTR_ERR(card);
> + goto out_release_group;
> }
> -
> - vkms_device->platform = pdev;
> - vkms_device->config.cursor = enable_cursor;
> - vkms_device->config.writeback = enable_writeback;
> - vkms_device->config.overlay = enable_overlay;
>
> - ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
> - DMA_BIT_MASK(64));
> + strncpy(card->name_buf, unique_name, ARRAY_SIZE(card->name_buf) - 1);
> + card->platform = pdev;
> + card->drm.unique = card->name_buf;
> + card->vkms_device = &vkms_device;
> + card->is_default = is_default;
> + card->is_registered = false;
> + card->resource_group_id = grp;
>
> + ret = dma_coerce_mask_and_coherent(card->drm.dev, DMA_BIT_MASK(64));
> if (ret) {
> DRM_ERROR("Could not initialize DMA support\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = drm_vblank_init(&vkms_device->drm, 1);
> + ret = drm_vblank_init(&card->drm, 1);
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> - goto out_devres;
> + goto out_release_group;
> }
>
> - ret = vkms_modeset_init(vkms_device);
> - if (ret)
> - goto out_devres;
> -
> - ret = drm_dev_register(&vkms_device->drm, 0);
> - if (ret)
> - goto out_devres;
> + ret = vkms_modeset_init(card);
> + if (ret) {
> + DRM_ERROR("Unable to initialize modesetting");
> + goto out_release_group;
> + }
>
> - drm_fbdev_generic_setup(&vkms_device->drm, 0);
> + ret = drm_dev_register(&card->drm, 0);
> + if (ret) {
> + DRM_ERROR("Unable to register card");
> + return ERR_PTR(ret);
No cleanup required if this fails?
> + }
>
> - vkms_device->initialized = true;
> + drm_fbdev_generic_setup(&card->drm, 0);
> + card->is_registered = true;
Re-raising the race concern here.
>
> - return 0;
> + devres_close_group(&pdev->dev, grp);
> + list_add_tail(&card->node, &vkms_device.cards);
I am still unclear why we need to bookkeep the cards ourselves since we're
spawning a new platform_device for each drm_device. It feels like perhaps it
would be easier to spawn a new platform_driver for each new drm device with its
own .probe & .remove function which keeps track of its own instance. This would
avoid the necessity of vkms_card, each platform_driver would have its own
vkms_device containing a drm_device.
I'm assuming that rmmod would invoke .remove on all the sub platform_drivers,
but if not you could keep track of them in the platform device and unregister
each one on unload which would be a lot less statekeeping.
> +
Nit: Trailing whitespace here
> + return card;
>
> -out_devres:
> - devres_release_group(&pdev->dev, NULL);
> -out_unregister:
> +out_release_group:
> + devres_release_group(&pdev->dev, grp);
> +out_platform_device:
> platform_device_unregister(pdev);
> - return ret;
> + return ERR_PTR(ret);
> }
>
> -static int __init vkms_init(void)
> +void vkms_card_destroy(struct vkms_card *card)
> {
> - return vkms_create();
> + list_del(&card->node);
> + if (card->is_registered) {
Nit: You could save yourself some indentation here and exit early.
> + drm_dev_unregister(&card->drm);
> + drm_atomic_helper_shutdown(&card->drm);
> + devres_release_group(&card->platform->dev,
> + card->resource_group_id);
> + platform_device_unregister(card->platform);
> + }
> }
>
> -static void __exit vkms_exit(void)
> +static int __init vkms_init(void)
> {
> - struct platform_device *pdev;
> -
> - if (!vkms_device || !vkms_device->initialized) {
> - return;
> + struct vkms_card *card;
> +
> + vkms_device.config.cursor = enable_cursor;
> + vkms_device.config.writeback = enable_writeback;
> + vkms_device.config.overlay = enable_overlay;
> + INIT_LIST_HEAD(&vkms_device.cards);
> +
> + card = vkms_card_init("default", /* configfs */ NULL);
This passes NULL for the bool is_default argument. Presumably this should be
true, or perhaps you don't need the second arg at all right now.
> + if (IS_ERR(card)) {
> + DRM_ERROR("Unable to init default card");
> + vkms_device_destroy();
> + return PTR_ERR(card);
> }
>
> - pdev = vkms_device->platform;
> + return 0;
> +}
>
> - drm_dev_unregister(&vkms_device->drm);
> - drm_atomic_helper_shutdown(&vkms_device->drm);
> - devres_release_group(&pdev->dev, NULL);
> - platform_device_unregister(pdev);
> +static void __exit vkms_exit(void)
> +{
> + vkms_device_destroy();
> }
>
> module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c7ebc4ee6b14..1a98b81d6f22 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -4,6 +4,7 @@
> #define _VKMS_DRV_H_
>
> #include <linux/hrtimer.h>
> +#include <linux/kernel.h>
From kernel.h:
* This header has combined a lot of unrelated to each other stuff.
* The process of splitting its content is in progress while keeping
* backward compatibility. That's why it's highly recommended NOT to
* include this header inside another header file, especially under
* generic or architectural include/ directory.
>
> #include <drm/drm.h>
> #include <drm/drm_gem.h>
> @@ -101,19 +102,29 @@ struct vkms_config {
> bool overlay;
> };
>
> -struct vkms_device {
> - struct drm_device drm;
> +struct vkms_card {
> struct platform_device *platform;
> + struct drm_device drm;
> + struct list_head node;
Nit: node is pretty ambiguous
> struct vkms_output output;
> + struct vkms_device *vkms_device;
> + /* storage for the value of drm->unique, giving this dev a unique busid */
I didn't quite grok this comment, mind expanding?
> + char name_buf[64];
> + void *resource_group_id;
> + bool is_default;
> + bool is_registered;
> +};
> +
> +struct vkms_device {
> + struct list_head cards;
> struct vkms_config config;
> - bool initialized;
> };
>
> #define drm_crtc_to_vkms_output(target) \
> container_of(target, struct vkms_output, crtc)
>
> -#define drm_device_to_vkms_device(target) \
> - container_of(target, struct vkms_device, drm)
> +#define drm_device_to_vkms_card(target) \
> + container_of(target, struct vkms_card, drm)
>
> #define to_vkms_crtc_state(target)\
> container_of(target, struct vkms_crtc_state, base)
> @@ -121,13 +132,18 @@ struct vkms_device {
> #define to_vkms_plane_state(target)\
> container_of(target, struct vkms_plane_state, base.base)
>
> +/* Cards */
> +struct vkms_card *vkms_card_init(const char *name, bool is_default);
> +void vkms_card_destroy(struct vkms_card *card);
> +
> /* CRTC */
> int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane *cursor);
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index);
> +int vkms_output_init_default(struct vkms_card *card, int index);
> +int vkms_output_init(struct vkms_card *card, int index);
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index);
>
> /* CRC Support */
> @@ -142,6 +158,6 @@ void vkms_composer_worker(struct work_struct *work);
> void vkms_set_composer(struct vkms_output *out, bool enabled);
>
> /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> +int vkms_enable_writeback_connector(struct vkms_card *card);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index d0061c82003a..dafd47c0a54d 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,12 +32,12 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> +static int vkms_add_overlay_plane(struct vkms_card *card, int index,
> struct drm_crtc *crtc)
> {
> struct vkms_plane *overlay;
>
> - overlay = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY, index);
> + overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
> if (IS_ERR(overlay))
> return PTR_ERR(overlay);
>
> @@ -47,10 +47,11 @@ static int vkms_add_overlay_plane(struct vkms_device *vkmsdev, int index,
> return 0;
> }
>
> -int vkms_output_init(struct vkms_device *vkmsdev, int index)
> +int vkms_output_init_default(struct vkms_card *card, int index)
> {
> - struct vkms_output *output = &vkmsdev->output;
> - struct drm_device *dev = &vkmsdev->drm;
> + const struct vkms_config *config = &card->vkms_device->config;
> + struct vkms_output *output = &card->output;
> + struct drm_device *dev = &card->drm;
> struct drm_connector *connector = &output->connector;
> struct drm_encoder *encoder = &output->encoder;
> struct drm_crtc *crtc = &output->crtc;
> @@ -59,20 +60,20 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> int writeback;
> unsigned int n;
>
> - primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY, index);
> + primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> - if (vkmsdev->config.overlay) {
> + if (config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
> + ret = vkms_add_overlay_plane(card, index, crtc);
> if (ret)
> return ret;
> }
> }
>
> - if (vkmsdev->config.cursor) {
> - cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
> + if (config->cursor) {
> + cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
> if (IS_ERR(cursor))
> return PTR_ERR(cursor);
> }
> @@ -103,8 +104,8 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
> goto err_attach;
> }
>
> - if (vkmsdev->config.writeback) {
> - writeback = vkms_enable_writeback_connector(vkmsdev);
> + if (config->writeback) {
> + writeback = vkms_enable_writeback_connector(card);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index d8eb674b49a6..28abd61a0bb9 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -158,10 +158,10 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> };
>
> -struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> +struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type, int index)
> {
> - struct drm_device *dev = &vkmsdev->drm;
> + struct drm_device *dev = &card->drm;
> const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> const u32 *formats;
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index af1604dfbbaf..681e7267d688 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -94,15 +94,15 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> struct drm_writeback_job *job)
> {
> struct vkms_writeback_job *vkmsjob = job->priv;
> - struct vkms_device *vkmsdev;
> + struct vkms_card *card;
>
> if (!job->fb)
> return;
>
> drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>
> - vkmsdev = drm_device_to_vkms_device(job->fb->dev);
> - vkms_set_composer(&vkmsdev->output, false);
> + card = drm_device_to_vkms_card(job->fb->dev);
> + vkms_set_composer(&card->output, false);
> kfree(vkmsjob);
> }
>
> @@ -111,8 +111,8 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> {
> struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> conn);
> - struct vkms_device *vkmsdev = drm_device_to_vkms_device(conn->dev);
> - struct vkms_output *output = &vkmsdev->output;
> + struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
> + struct vkms_output *output = &card->output;
> struct drm_writeback_connector *wb_conn = &output->wb_connector;
> struct drm_connector_state *conn_state = wb_conn->base.state;
> struct vkms_crtc_state *crtc_state = output->composer_state;
> @@ -120,7 +120,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> if (!conn_state)
> return;
>
> - vkms_set_composer(&vkmsdev->output, true);
> + vkms_set_composer(&card->output, true);
>
> spin_lock_irq(&output->composer_lock);
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> @@ -136,14 +136,14 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> .atomic_commit = vkms_wb_atomic_commit,
> };
>
> -int vkms_enable_writeback_connector(struct vkms_device *vkmsdev)
> +int vkms_enable_writeback_connector(struct vkms_card *card)
> {
> - struct drm_writeback_connector *wb = &vkmsdev->output.wb_connector;
> + struct drm_writeback_connector *wb = &card->output.wb_connector;
>
> - vkmsdev->output.wb_connector.encoder.possible_crtcs = 1;
> + card->output.wb_connector.encoder.possible_crtcs = 1;
> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>
> - return drm_writeback_connector_init(&vkmsdev->drm, wb,
> + return drm_writeback_connector_init(&card->drm, wb,
> &vkms_wb_connector_funcs,
> &vkms_wb_encoder_helper_funcs,
> vkms_wb_formats,
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
2022-08-05 18:27 ` Sean Paul
@ 2022-08-05 18:34 ` Sean Paul
0 siblings, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-05 18:34 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Aug 05, 2022 at 06:27:08PM +0000, Sean Paul wrote:
> On Fri, Jul 22, 2022 at 05:32:10PM -0400, Jim Shargo wrote:
> > This is a small refactor to make ConfigFS support easier.
> >
> > We now store the vkms_device statically, and maintain a list of
> > "cards", each representing a different virtual DRM driver.
> >
> > We also make it clear when a card is "default", that is created at
> > initialization, and not. This is because, due to limitations on what we
> > can do with the configfs API, the default card won't be reflected in
> > configfs and is initialized in a unique way.
> >
> > Since we're only managing a single card, this should be a no-op.
> >
> > Signed-off-by: Jim Shargo <jshargo@chromium.org>
/snip
What a mess, I replied to the wrong patch. The review here is targeting PATCH
2/5 despite the title and reply-to.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card
2022-07-22 21:32 ` [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card Jim Shargo
2022-08-05 18:27 ` Sean Paul
@ 2022-08-05 20:03 ` Sean Paul
1 sibling, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-05 20:03 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Jul 22, 2022 at 05:32:11PM -0400, Jim Shargo wrote:
> This change supports multiple CRTCs, encoders, connectors instead of one
> of each per card.
>
> Since ConfigFS-based devices will support multiple crtcs, it's useful to
> move all of the writeback/composition data from being a per-output thing
> to being a per-CRTC thing.
>
> Since there's still only ever one CRTC, this should be a no-op refactor.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
Actual PATCH 3/5 review below :-)
> drivers/gpu/drm/vkms/vkms_composer.c | 28 +++----
> drivers/gpu/drm/vkms/vkms_crtc.c | 91 +++++++++++----------
> drivers/gpu/drm/vkms/vkms_drv.c | 3 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 66 ++++++++++------
> drivers/gpu/drm/vkms/vkms_output.c | 110 ++++++++++++++++++--------
> drivers/gpu/drm/vkms/vkms_plane.c | 39 ++++++---
> drivers/gpu/drm/vkms/vkms_writeback.c | 25 +++---
> 7 files changed, 225 insertions(+), 137 deletions(-)
>
/snip
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index c1b632952532..d93678d984ae 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -11,35 +11,34 @@
>
> static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> {
> - struct vkms_output *output = container_of(timer, struct vkms_output,
> - vblank_hrtimer);
> - struct drm_crtc *crtc = &output->crtc;
> + struct vkms_crtc *vkms_crtc = timer_to_vkms_crtc(timer);
> + struct drm_crtc *crtc = &vkms_crtc->base;
> struct vkms_crtc_state *state;
> u64 ret_overrun;
> bool ret, fence_cookie;
>
> fence_cookie = dma_fence_begin_signalling();
>
> - ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
> - output->period_ns);
> + ret_overrun = hrtimer_forward_now(&vkms_crtc->vblank_hrtimer,
> + vkms_crtc->period_ns);
> if (ret_overrun != 1)
> pr_warn("%s: vblank timer overrun\n", __func__);
>
> - spin_lock(&output->lock);
> + spin_lock(&vkms_crtc->lock);
> ret = drm_crtc_handle_vblank(crtc);
> if (!ret)
> DRM_ERROR("vkms failure on handling vblank");
>
> - state = output->composer_state;
> - spin_unlock(&output->lock);
> + state = vkms_crtc->composer_state;
> + spin_unlock(&vkms_crtc->lock);
>
> - if (state && output->composer_enabled) {
> + if (state && vkms_crtc->composer_enabled) {
> u64 frame = drm_crtc_accurate_vblank_count(crtc);
>
> /* update frame_start only if a queued vkms_composer_worker()
> * has read the data
> */
> - spin_lock(&output->composer_lock);
> + spin_lock(&vkms_crtc->composer_lock);
> if (!state->crc_pending)
> state->frame_start = frame;
> else
> @@ -47,9 +46,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> state->frame_start, frame);
> state->frame_end = frame;
> state->crc_pending = true;
> - spin_unlock(&output->composer_lock);
> + spin_unlock(&vkms_crtc->composer_lock);
>
> - ret = queue_work(output->composer_workq, &state->composer_work);
> + ret = queue_work(vkms_crtc->composer_workq, &state->composer_work);
> if (!ret)
> DRM_DEBUG_DRIVER("Composer worker already queued\n");
> }
> @@ -62,28 +61,24 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
> static int vkms_enable_vblank(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = &card->output;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
> unsigned int pipe = drm_crtc_index(crtc);
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> drm_calc_timestamping_constants(crtc, &crtc->mode);
>
> - hrtimer_init(&out->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> - out->vblank_hrtimer.function = &vkms_vblank_simulate;
> - out->period_ns = ktime_set(0, vblank->framedur_ns);
> - hrtimer_start(&out->vblank_hrtimer, out->period_ns, HRTIMER_MODE_REL);
> + hrtimer_init(&vkms_crtc->vblank_hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + vkms_crtc->vblank_hrtimer.function = &vkms_vblank_simulate;
> + vkms_crtc->period_ns = ktime_set(0, vblank->framedur_ns);
> + hrtimer_start(&vkms_crtc->vblank_hrtimer, vkms_crtc->period_ns, HRTIMER_MODE_REL);
>
> return 0;
> }
>
> static void vkms_disable_vblank(struct drm_crtc *crtc)
> {
> - struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *out = &card->output;
> -
> - hrtimer_cancel(&out->vblank_hrtimer);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
Nit: blank line here
> + hrtimer_cancel(&vkms_crtc->vblank_hrtimer);
> }
>
> static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> @@ -91,8 +86,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> bool in_vblank_irq)
> {
> struct drm_device *dev = crtc->dev;
> - struct vkms_card *card = drm_device_to_vkms_card(dev);
> - struct vkms_output *output = &card->output;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
> unsigned int pipe = crtc->index;
> struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>
> @@ -101,7 +95,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> return true;
> }
>
> - *vblank_time = READ_ONCE(output->vblank_hrtimer.node.expires);
> + *vblank_time = READ_ONCE(vkms_crtc->vblank_hrtimer.node.expires);
>
> if (WARN_ON(*vblank_time == vblank->time))
> return true;
> @@ -113,7 +107,7 @@ static bool vkms_get_vblank_timestamp(struct drm_crtc *crtc,
> * the vblank core expects. Therefore we need to always correct the
> * timestampe by one frame.
> */
> - *vblank_time -= output->period_ns;
> + *vblank_time -= vkms_crtc->period_ns;
>
> return true;
> }
> @@ -240,18 +234,18 @@ static void vkms_crtc_atomic_disable(struct drm_crtc *crtc,
> static void vkms_crtc_atomic_begin(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> - struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
>
> /* This lock is held across the atomic commit to block vblank timer
> * from scheduling vkms_composer_worker until the composer is updated
> */
> - spin_lock_irq(&vkms_output->lock);
> + spin_lock_irq(&vkms_crtc->lock);
> }
>
> static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> - struct vkms_output *vkms_output = drm_crtc_to_vkms_output(crtc);
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(crtc);
>
> if (crtc->state->event) {
> spin_lock(&crtc->dev->event_lock);
> @@ -266,9 +260,9 @@ static void vkms_crtc_atomic_flush(struct drm_crtc *crtc,
> crtc->state->event = NULL;
> }
>
> - vkms_output->composer_state = to_vkms_crtc_state(crtc->state);
> + vkms_crtc->composer_state = to_vkms_crtc_state(crtc->state);
>
> - spin_unlock_irq(&vkms_output->lock);
> + spin_unlock_irq(&vkms_crtc->lock);
> }
>
> static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> @@ -279,27 +273,38 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> .atomic_disable = vkms_crtc_atomic_disable,
> };
>
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> +struct vkms_crtc *vkms_crtc_init(struct vkms_card *card,
> struct drm_plane *primary, struct drm_plane *cursor)
> {
> - struct vkms_output *vkms_out = drm_crtc_to_vkms_output(crtc);
> + struct drm_device *dev = &card->drm;
> + struct vkms_crtc *vkms_crtc;
> int ret;
>
> - ret = drm_crtc_init_with_planes(dev, crtc, primary, cursor,
> + if (card->output.num_crtcs >= VKMS_MAX_OUTPUT_OBJECTS) {
> + return ERR_PTR(ENOMEM);
> + }
Nit: Remove braces
> + vkms_crtc = &card->output.crtcs[card->output.num_crtcs++];
Instead of incrementing here and decrementing in the error handling code, just
increment right before the return when we're in the clear.
While I'm here, do you need any locking around the output members?
> +
> + ret = drm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
> &vkms_crtc_funcs, NULL);
> if (ret) {
> DRM_ERROR("Failed to init CRTC\n");
> - return ret;
> + goto out_error;
> }
>
> - drm_crtc_helper_add(crtc, &vkms_crtc_helper_funcs);
> + drm_crtc_helper_add(&vkms_crtc->base, &vkms_crtc_helper_funcs);
>
> - spin_lock_init(&vkms_out->lock);
> - spin_lock_init(&vkms_out->composer_lock);
> + spin_lock_init(&vkms_crtc->lock);
> + spin_lock_init(&vkms_crtc->composer_lock);
>
> - vkms_out->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> - if (!vkms_out->composer_workq)
> - return -ENOMEM;
> + vkms_crtc->composer_workq = alloc_ordered_workqueue("vkms_composer", 0);
> + if (!vkms_crtc->composer_workq)
> + goto out_error;
> +
> + return vkms_crtc;
>
> - return ret;
> +out_error:
> + memset(vkms_crtc, 0, sizeof(*vkms_crtc));
> + card->output.num_crtcs -= 1;
> + return ERR_PTR(ret);
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 92fbade2199b..266c9f643f70 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -57,7 +57,8 @@ static void vkms_release(struct drm_device *dev)
> {
> struct vkms_card *card = drm_device_to_vkms_card(dev);
>
> - destroy_workqueue(card->output.composer_workq);
> + for (int i = 0; i < card->output.num_crtcs; i++)
> + destroy_workqueue(card->output.crtcs[i].composer_workq);
> }
>
> static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 1a98b81d6f22..30ae06d7af71 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -23,6 +23,8 @@
>
> #define NUM_OVERLAY_PLANES 8
>
> +#define VKMS_MAX_OUTPUT_OBJECTS 16
> +
> struct vkms_writeback_job {
> struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> @@ -51,6 +53,27 @@ struct vkms_plane {
> struct drm_plane base;
> };
>
> +struct vkms_output;
> +
> +struct vkms_crtc {
> + struct drm_crtc base;
> +
> + struct drm_writeback_connector wb_connector;
> + struct hrtimer vblank_hrtimer;
> + ktime_t period_ns;
> + struct drm_pending_vblank_event *event;
> + /* ordered wq for composer_work */
> + struct workqueue_struct *composer_workq;
> + /* protects concurrent access to composer */
> + spinlock_t lock;
> +
> + /* protected by @lock */
> + bool composer_enabled;
> + struct vkms_crtc_state *composer_state;
> +
> + spinlock_t composer_lock;
> +};
> +
> /**
> * vkms_crtc_state - Driver specific CRTC state
> * @base: base CRTC state
> @@ -75,23 +98,14 @@ struct vkms_crtc_state {
> };
>
> struct vkms_output {
> - struct drm_crtc crtc;
> - struct drm_encoder encoder;
> - struct drm_connector connector;
> - struct drm_writeback_connector wb_connector;
> - struct hrtimer vblank_hrtimer;
> - ktime_t period_ns;
> - struct drm_pending_vblank_event *event;
> - /* ordered wq for composer_work */
> - struct workqueue_struct *composer_workq;
> - /* protects concurrent access to composer */
> - spinlock_t lock;
> -
> - /* protected by @lock */
> - bool composer_enabled;
> - struct vkms_crtc_state *composer_state;
> -
> - spinlock_t composer_lock;
> + int num_crtcs;
> + struct vkms_crtc crtcs[VKMS_MAX_OUTPUT_OBJECTS];
> + int num_encoders;
> + struct drm_encoder encoders[VKMS_MAX_OUTPUT_OBJECTS];
> + int num_connectors;
> + struct drm_connector connectors[VKMS_MAX_OUTPUT_OBJECTS];
Nit: Trailing whitespace
> + int num_planes;
> + struct vkms_plane planes[VKMS_MAX_OUTPUT_OBJECTS];
We probably want some number more planes than CRTCs, so perhaps make
VKMS_MAX_PLANES a multiple of VKMS_MAX_OUTPUT_OBJECTS?
> };
>
> struct vkms_device;
> @@ -120,12 +134,15 @@ struct vkms_device {
> struct vkms_config config;
> };
>
> -#define drm_crtc_to_vkms_output(target) \
> - container_of(target, struct vkms_output, crtc)
> +#define drm_crtc_to_vkms_crtc(crtc)\
> + container_of(crtc, struct vkms_crtc, base)
>
> -#define drm_device_to_vkms_card(target) \
> +#define drm_device_to_vkms_card(target) \
> container_of(target, struct vkms_card, drm)
>
> +#define timer_to_vkms_crtc(timer)\
> + container_of(timer, struct vkms_crtc, vblank_hrtimer)
> +
> #define to_vkms_crtc_state(target)\
> container_of(target, struct vkms_crtc_state, base)
>
> @@ -137,14 +154,13 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default);
> void vkms_card_destroy(struct vkms_card *card);
>
> /* CRTC */
> -int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> - struct drm_plane *primary, struct drm_plane *cursor);
> +struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
Nit: Please split this line
>
> int vkms_output_init_default(struct vkms_card *card, int index);
> int vkms_output_init(struct vkms_card *card, int index);
>
> struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> - enum drm_plane_type type, int index);
> + enum drm_plane_type type);
>
> /* CRC Support */
> const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> @@ -155,9 +171,9 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>
> /* Composer Support */
> void vkms_composer_worker(struct work_struct *work);
> -void vkms_set_composer(struct vkms_output *out, bool enabled);
> +void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
>
> /* Writeback */
> -int vkms_enable_writeback_connector(struct vkms_card *card);
> +int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
>
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index dafd47c0a54d..2b72d8e763a8 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -32,19 +32,44 @@ static const struct drm_connector_helper_funcs vkms_conn_helper_funcs = {
> .get_modes = vkms_conn_get_modes,
> };
>
> -static int vkms_add_overlay_plane(struct vkms_card *card, int index,
> - struct drm_crtc *crtc)
> +static struct drm_connector *vkms_connector_init(struct vkms_card *card)
> {
> - struct vkms_plane *overlay;
> + struct drm_connector *connector;
> + int ret;
>
> - overlay = vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY, index);
> - if (IS_ERR(overlay))
> - return PTR_ERR(overlay);
> + if (card->output.num_connectors >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
>
> - if (!overlay->base.possible_crtcs)
> - overlay->base.possible_crtcs = drm_crtc_mask(crtc);
> + connector = &card->output.connectors[card->output.num_connectors++];
> + ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
> + DRM_MODE_CONNECTOR_VIRTUAL);
> + if (ret) {
> + memset(connector, 0, sizeof(*connector));
> + card->output.num_connectors -= 1;
> + return ERR_PTR(ret);
> + }
>
> - return 0;
> + drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> +
> + return connector;
> +}
> +
> +static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
> +{
> + struct drm_encoder *encoder;
> + int ret;
> +
> + if (card->output.num_encoders >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
> +
> + encoder = &card->output.encoders[card->output.num_encoders++];
> + ret = drm_simple_encoder_init(&card->drm, encoder, DRM_MODE_ENCODER_VIRTUAL);
> + if (ret) {
> + memset(encoder, 0, sizeof(*encoder));
> + card->output.num_encoders -= 1;
> + return ERR_PTR(ret);
> + }
> + return encoder;
> }
>
> int vkms_output_init_default(struct vkms_card *card, int index)
> @@ -52,51 +77,65 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> const struct vkms_config *config = &card->vkms_device->config;
> struct vkms_output *output = &card->output;
> struct drm_device *dev = &card->drm;
> - struct drm_connector *connector = &output->connector;
> - struct drm_encoder *encoder = &output->encoder;
> - struct drm_crtc *crtc = &output->crtc;
> + struct drm_connector *connector;
> + struct drm_encoder *encoder;
> + struct vkms_crtc *vkms_crtc;
> struct vkms_plane *primary, *cursor = NULL;
> int ret;
> int writeback;
> unsigned int n;
>
> - primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY, index);
> + primary = vkms_plane_init(card, DRM_PLANE_TYPE_PRIMARY);
> if (IS_ERR(primary))
> return PTR_ERR(primary);
>
> if (config->overlay) {
> for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> - ret = vkms_add_overlay_plane(card, index, crtc);
> - if (ret)
> - return ret;
> + struct vkms_plane *overlay =
> + vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
> + if (IS_ERR(overlay)) {
> + ret = PTR_ERR(overlay);
> + goto err_planes;
> + }
> }
> }
>
> if (config->cursor) {
> - cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR, index);
> - if (IS_ERR(cursor))
> - return PTR_ERR(cursor);
> + cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
> + if (IS_ERR(cursor)) {
> + ret = PTR_ERR(cursor);
> + goto err_planes;
> + }
> }
>
> - ret = vkms_crtc_init(dev, crtc, &primary->base, &cursor->base);
> - if (ret)
> - return ret;
> + vkms_crtc = vkms_crtc_init(card, &primary->base,
> + cursor ? &cursor->base : NULL);
Looks like we had a NULL deref previously if there was no cursor.. Whoops!
> + if (IS_ERR(vkms_crtc)) {
> + DRM_ERROR("Failed to init crtc\n");
> + ret = PTR_ERR(vkms_crtc);
> + goto err_planes;
> + }
>
> - ret = drm_connector_init(dev, connector, &vkms_connector_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL);
> - if (ret) {
> + for (int i = 0; i < card->output.num_planes; i++) {
> + card->output.planes[i].base.possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> +
> + connector = vkms_connector_init(card);
> + if (IS_ERR(connector)) {
> DRM_ERROR("Failed to init connector\n");
> + ret = PTR_ERR(connector);
> goto err_connector;
> }
>
> - drm_connector_helper_add(connector, &vkms_conn_helper_funcs);
> -
> - ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_VIRTUAL);
> - if (ret) {
> + encoder = vkms_encoder_init(card);
> + if (IS_ERR(encoder)) {
> DRM_ERROR("Failed to init encoder\n");
> + ret = PTR_ERR(encoder);
> goto err_encoder;
> }
> - encoder->possible_crtcs = 1;
> + encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
> +
>
> ret = drm_connector_attach_encoder(connector, encoder);
> if (ret) {
> @@ -105,7 +144,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> }
>
> if (config->writeback) {
> - writeback = vkms_enable_writeback_connector(card);
> + writeback = vkms_enable_writeback_connector(card, vkms_crtc);
> if (writeback)
> DRM_ERROR("Failed to init writeback connector\n");
> }
> @@ -121,7 +160,14 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> drm_connector_cleanup(connector);
>
> err_connector:
> - drm_crtc_cleanup(crtc);
> + drm_crtc_cleanup(&vkms_crtc->base);
> +
> +err_planes:
> + for (int i = 0; i < output->num_planes; i++) {
> + drm_plane_cleanup(&output->planes[i].base);
> + }
Nit: Remove braces
> +
> + memset(output, 0, sizeof(struct vkms_output));
Is this strictly necessary? I tend to worry about blanket clears like this in
error handling since they could cause future issues if people extend
functionality and forget to update the memset.
IMO it would be better to define vkms_output_destroy() which just does all of
the above, clears out the num_* members explicitly and doesn't worry about the
array contents.
>
> return ret;
> }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 28abd61a0bb9..5f49257fe16e 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -7,6 +7,7 @@
> #include <drm/drm_fourcc.h>
> #include <drm/drm_gem_atomic_helper.h>
> #include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_plane_helper.h>
>
> #include "vkms_drv.h"
> @@ -65,6 +66,19 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
> kfree(vkms_state);
> }
>
> +static void vkms_plane_destroy(struct drm_plane *plane)
I think this could be replaced by drm_plane_helper_destroy() instead of rolling
your own destroy function.
> +{
> + struct vkms_plane *vkms_plane =
> + container_of(plane, struct vkms_plane, base);
> +
> + if (plane->state) {
> + vkms_plane_destroy_state(plane, plane->state);
> + plane->state = NULL;
> + }
> +
> + memset(vkms_plane, 0, sizeof(struct vkms_plane));
> +}
> +
> static void vkms_plane_reset(struct drm_plane *plane)
> {
> struct vkms_plane_state *vkms_state;
> @@ -86,8 +100,9 @@ static void vkms_plane_reset(struct drm_plane *plane)
> static const struct drm_plane_funcs vkms_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> - .reset = vkms_plane_reset,
> - .atomic_duplicate_state = vkms_plane_duplicate_state,
> + .destroy = vkms_plane_destroy,
> + .reset = vkms_plane_reset,
Nit: Remove space before tab after .destroy and .reset. I think they're also not
properly aligned.
> + .atomic_duplicate_state = vkms_plane_duplicate_state,
> .atomic_destroy_state = vkms_plane_destroy_state,
> };
>
> @@ -159,13 +174,20 @@ static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
> };
>
> struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> - enum drm_plane_type type, int index)
> + enum drm_plane_type type)
> {
> struct drm_device *dev = &card->drm;
> + struct vkms_output *output = &card->output;
> const struct drm_plane_helper_funcs *funcs;
> struct vkms_plane *plane;
> const u32 *formats;
> int nformats;
> + int ret;
> +
> + if (output->num_planes >= VKMS_MAX_OUTPUT_OBJECTS)
> + return ERR_PTR(ENOMEM);
> +
> + plane = &output->planes[output->num_planes++];
Hmm, I'm guessing this is why the destroy() hook is being hand rolled? Perhaps
changing the output arrays to arrays of pointers would allow you to leverage the
helpers and drmm_*
>
> switch (type) {
> case DRM_PLANE_TYPE_PRIMARY:
> @@ -186,12 +208,11 @@ struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> break;
> }
>
> - plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 1 << index,
> - &vkms_plane_funcs,
> - formats, nformats,
> - NULL, type, NULL);
> - if (IS_ERR(plane))
> - return plane;
> + ret = drm_universal_plane_init(dev, &plane->base, 0,
> + &vkms_plane_funcs, formats, nformats,
> + NULL, type, NULL);
> + if (ret)
> + return ERR_PTR(ret);
>
> drm_plane_helper_add(&plane->base, funcs);
>
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index 681e7267d688..68c7a77ab93f 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> #include <linux/iosys-map.h>
> +#include <linux/kernel.h>
Same kernel.h nit as before.
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_fourcc.h>
> @@ -94,15 +95,14 @@ static void vkms_wb_cleanup_job(struct drm_writeback_connector *connector,
> struct drm_writeback_job *job)
> {
> struct vkms_writeback_job *vkmsjob = job->priv;
> - struct vkms_card *card;
> + struct vkms_crtc *vkms_crtc = container_of(connector, struct vkms_crtc, wb_connector);
>
> if (!job->fb)
> return;
>
> drm_gem_fb_vunmap(job->fb, vkmsjob->map);
>
> - card = drm_device_to_vkms_card(job->fb->dev);
> - vkms_set_composer(&card->output, false);
> + vkms_set_composer(vkms_crtc, false);
> kfree(vkmsjob);
> }
>
> @@ -111,21 +111,20 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> {
> struct drm_connector_state *connector_state = drm_atomic_get_new_connector_state(state,
> conn);
> - struct vkms_card *card = drm_device_to_vkms_card(conn->dev);
> - struct vkms_output *output = &card->output;
> - struct drm_writeback_connector *wb_conn = &output->wb_connector;
> + struct vkms_crtc *vkms_crtc = drm_crtc_to_vkms_crtc(connector_state->crtc);
> + struct drm_writeback_connector *wb_conn = &vkms_crtc->wb_connector;
> struct drm_connector_state *conn_state = wb_conn->base.state;
> - struct vkms_crtc_state *crtc_state = output->composer_state;
> + struct vkms_crtc_state *crtc_state = vkms_crtc->composer_state;
>
> if (!conn_state)
> return;
>
> - vkms_set_composer(&card->output, true);
> + vkms_set_composer(vkms_crtc, true);
>
> - spin_lock_irq(&output->composer_lock);
> + spin_lock_irq(&vkms_crtc->composer_lock);
> crtc_state->active_writeback = conn_state->writeback_job->priv;
> crtc_state->wb_pending = true;
> - spin_unlock_irq(&output->composer_lock);
> + spin_unlock_irq(&vkms_crtc->composer_lock);
> drm_writeback_queue_job(wb_conn, connector_state);
> }
>
> @@ -136,11 +135,11 @@ static const struct drm_connector_helper_funcs vkms_wb_conn_helper_funcs = {
> .atomic_commit = vkms_wb_atomic_commit,
> };
>
> -int vkms_enable_writeback_connector(struct vkms_card *card)
> +int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc)
> {
> - struct drm_writeback_connector *wb = &card->output.wb_connector;
> + struct drm_writeback_connector *wb = &vkms_crtc->wb_connector;
>
> - card->output.wb_connector.encoder.possible_crtcs = 1;
> + vkms_crtc->wb_connector.encoder.possible_crtcs = 1;
> drm_connector_helper_add(&wb->base, &vkms_wb_conn_helper_funcs);
>
> return drm_writeback_connector_init(&card->drm, wb,
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 4/5] drm/vkms: Add ConfigFS scaffolding to VKMS
[not found] <20220722213214.1377835-1-jshargo@chromium.org>
` (2 preceding siblings ...)
2022-07-22 21:32 ` [PATCH 3/5] drm/vkms: Support multiple objects (crtcs, etc.) per card Jim Shargo
@ 2022-07-22 21:32 ` Jim Shargo
2022-08-05 21:23 ` Sean Paul
2022-07-22 21:32 ` [PATCH 5/5] drm/vkms: Support registering configfs devices Jim Shargo
4 siblings, 1 reply; 12+ messages in thread
From: Jim Shargo @ 2022-07-22 21:32 UTC (permalink / raw)
To: jshargo, Rodrigo Siqueira, Melissa Wen, Haneen Mohammed,
Daniel Vetter, David Airlie, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Jonathan Corbet
Cc: Jim Shargo, linux-kernel, dri-devel, linux-doc
This change adds the basic scaffolding for ConfigFS, including setting
up the default directories. It does not allow for the registration of
configfs-backed devices, which is complex and provided in a follow-up
commit.
This CL includes docs about using ConfigFS with VKMS, but I'll summarize
in brief here as well (assuming ConfigFS is mounted at /config/):
To create a new configuration, you can do so via `mkdir
/config/vkms/my-device`.
This will create a number of directories and files automatically:
/config
`-- vkms
`-- my-device
|-- connectors
|-- crtcs
|-- encoders
|-- planes
`-- is_registered
You can then configure objects by mkdir'ing in each of the directories.
When you're satisfied, you can `echo 1 > /config/vkms/my-device/is_registered`.
This will create a new device according to your configuration.
For now, this will fail, but the next change will add support for it.
Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
Documentation/gpu/vkms.rst | 76 ++++
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/vkms/Makefile | 1 +
drivers/gpu/drm/vkms/vkms_configfs.c | 527 +++++++++++++++++++++++++++
drivers/gpu/drm/vkms/vkms_drv.c | 28 +-
drivers/gpu/drm/vkms/vkms_drv.h | 75 +++-
drivers/gpu/drm/vkms/vkms_output.c | 11 +-
7 files changed, 711 insertions(+), 8 deletions(-)
create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 9c873c3912cc..f5f8ea0de5bc 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -51,6 +51,82 @@ To disable the driver, use ::
sudo modprobe -r vkms
+Configuration With ConfigFS
+===========================
+
+VKMS is instrumented with support for configuration via `ConfigFS`. With VKMS
+installed, you can mount ConfigFS at /config/ like so::
+
+ mkdir -p /config/
+ sudo mount -t configfs none /config
+
+This allows you to configure multiple virtual cards in addition to an immutable
+"default" card created by the driver at initialization time.
+
+To set up a new card, create a new directory under the vkms configfs
+directory::
+
+ $ mkdir /config/vkms/test
+
+With your card created you'll find an new directory ready to be configured::
+
+ /config
+ `-- vkms
+ |-- default
+ | `-- is_registered
+ `-- test
+ |-- connectors
+ |-- crtcs
+ |-- encoders
+ |-- planes
+ `-- is_registered
+
+Each directory you add within the connectors, crtcs, encoders, and planes
+directories will let you configure a new object of that type. Adding new
+objects will automatically create a set of default files and folders you can
+use to configure that object.
+
+For instance, we can set up a very basic driver like so::
+
+ DRM_PLANE_TYPE_PRIMARY=1
+ DRM_PLANE_TYPE_CURSOR=2
+ DRM_PLANE_TYPE_OVERLAY=0
+
+ mkdir /config/vkms/test/planes/primary
+ echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/primary/type
+
+ mkdir /config/vkms/test/planes/other_primary
+ echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/other_primary/type
+
+ mkdir /config/vkms/test/planes/cursor
+ echo $DRM_PLANE_TYPE_CURSOR > /config/vkms/test/planes/cursor/type
+
+ mkdir /config/vkms/test/planes/overlay
+ echo $DRM_PLANE_TYPE_OVERLAY > /config/vkms/test/planes/overlay/type
+
+ mkdir /config/vkms/test/crtcs/crtc
+ mkdir /config/vkms/test/crtcs/crtc_other
+ mkdir /config/vkms/test/encoders/encoder
+ mkdir /config/vkms/test/connectors/connector
+
+ ln -s /config/vkms/test/encoders/encoder /config/vkms/test/connectors/connector/possible_encoders
+ ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/encoders/encoder/possible_crtcs/
+ ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/primary/possible_crtcs/
+ ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/cursor/possible_crtcs/
+ ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/overlay/possible_crtcs/
+ ln -s /config/vkms/test/crtcs/crtc_other /config/vkms/test/planes/overlay/possible_crtcs/
+
+You can see that specific attributes, such as `.../<plane>/type`, can be
+configured by writing into them. Associating objects together can be done via
+symlinks.
+
+Finally, to register your newly configured objects with DRM so you can use your
+virtual device, all you need to do is write `1` into the special file
+`/config/vkms/test/is_registered``, which will return non-zero on error.
+
+When you're done with the virtual card, all you need to do is `rmdir` the root
+directory.
+
Testing With IGT
================
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f1422bee3dcc..5c90c82fab6d 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -307,6 +307,7 @@ config DRM_VKMS
depends on DRM && MMU
select DRM_KMS_HELPER
select DRM_GEM_SHMEM_HELPER
+ select CONFIGFS_FS
select CRC32
default n
help
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 72f779cbfedd..3efb7b0f5319 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -3,6 +3,7 @@ vkms-y := \
vkms_drv.o \
vkms_plane.o \
vkms_output.o \
+ vkms_configfs.o \
vkms_crtc.o \
vkms_composer.o \
vkms_writeback.o
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
new file mode 100644
index 000000000000..fa0d8700258e
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -0,0 +1,527 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/configfs.h>
+#include <linux/kernel.h>
+#include <linux/mutex.h>
+
+#include <drm/drm_plane.h>
+#include <drm/drm_print.h>
+
+#include "vkms_drv.h"
+
+/**
+ * DOC: ConfigFS Support for VKMS
+ *
+ * When ConfigFS is mounted, say at /config/, we add a /config/vkms/
+ * directory. Each folder in this directory represents a different
+ * virtual driver. Drivers can be "registered", meaning that they're
+ * available to use as a DRM driver. Initially, this just contains a
+ * registered "default" device.
+ *
+ * Users can mkdir in this the VKMS directory to begin the process of
+ * configuring a new device. Users can mkdir in the automatically
+ * created "connectors", "crtcs", "encoders", and "planes" folders to
+ * set up those objects.
+ *
+ * In order to create associations between objects, users can symlink
+ * the object's directory. For instance, to make a plane work with a
+ * CRTC, the user will add a symlink like so:
+ * /config/vkms/card/planes/<plane>/possible_crtcs/<crtc_symlink>
+ *
+ * Objects may have files (in ConfigFS-speak, attributes) that allow
+ * for additional configuration. For instance, a user can set the
+ * plane type by writing the appropriate value to the "type" property
+ * in the plane's directory.
+ *
+ * Once the driver is configured, the user can write a "1" to the
+ * "is_registered" file/attribute in the driver's directory. Until a
+ * virtual driver is registered, nothing DRM-related is set up.
+ */
+
+static struct config_item_type card_group_type;
+static struct config_item_type connectors_group_type;
+static struct config_item_type crtcs_group_type;
+static struct config_item_type encoders_group_type;
+static struct config_item_type planes_group_type;
+
+static void vkms_configfs_setup_default_groups(struct vkms_configfs *configfs,
+ const char *name)
+{
+ config_group_init_type_name(&configfs->card_group, name,
+ &card_group_type);
+
+ config_group_init_type_name(&configfs->connectors_group, "connectors",
+ &connectors_group_type);
+ configfs_add_default_group(&configfs->connectors_group,
+ &configfs->card_group);
+
+ config_group_init_type_name(&configfs->crtcs_group, "crtcs",
+ &crtcs_group_type);
+ configfs_add_default_group(&configfs->crtcs_group,
+ &configfs->card_group);
+
+ config_group_init_type_name(&configfs->encoders_group, "encoders",
+ &encoders_group_type);
+ configfs_add_default_group(&configfs->encoders_group,
+ &configfs->card_group);
+
+ config_group_init_type_name(&configfs->planes_group, "planes",
+ &planes_group_type);
+ configfs_add_default_group(&configfs->planes_group,
+ &configfs->card_group);
+}
+
+/* Root directory group and subsystem, e.g. /config/vkms/ */
+
+static struct config_group *make_root_group(struct config_group *group,
+ const char *name)
+{
+ struct vkms_configfs *configfs =
+ kzalloc(sizeof(struct vkms_configfs), GFP_KERNEL);
+
+ if (!configfs)
+ return ERR_PTR(ENOMEM);
+
+ vkms_configfs_setup_default_groups(configfs, name);
+
+ return &configfs->card_group;
+}
+
+static void drop_root_group(struct config_group *group,
+ struct config_item *item)
+{
+ struct vkms_configfs *configfs = item_to_configfs(item);
+
+ if (configfs->card)
+ vkms_card_destroy(configfs->card);
+
+ kfree(configfs);
+}
+
+static struct configfs_group_operations root_group_ops = {
+ .make_group = &make_root_group,
+ .drop_item = &drop_root_group,
+};
+
+static struct config_item_type vkms_type = {
+ .ct_group_ops = &root_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem vkms_subsys = {
+ .su_group = {
+ .cg_item = {
+ .ci_name = "vkms",
+ .ci_type = &vkms_type,
+ },
+ },
+};
+
+int vkms_init_configfs(void)
+{
+ config_group_init(&vkms_subsys.su_group);
+ mutex_init(&vkms_subsys.su_mutex);
+ return configfs_register_subsystem(&vkms_subsys);
+}
+
+void vkms_unregister_configfs(void)
+{
+ configfs_unregister_subsystem(&vkms_subsys);
+}
+
+/*
+ * Common helpers (common sub-groups)
+ */
+
+/* Possible CRTCs, e.g. /config/vkms/card/<object>/possible_crtcs/<symlink> */
+
+static struct config_item_type crtc_type;
+
+static int possible_crtcs_allow_link(struct config_item *src,
+ struct config_item *target)
+{
+ struct vkms_config_links *links = item_to_config_links(src);
+ struct vkms_config_crtc *crtc;
+
+ if (target->ci_type != &crtc_type) {
+ DRM_ERROR("Unable to link non-CRTCs.");
+ return -EINVAL;
+ }
+
+ crtc = item_to_config_crtc(target);
+
+ if (links->linked_object_bitmap & (1 << crtc->crtc_config_idx)) {
+ DRM_ERROR(
+ "Tried to add two symlinks to the same CRTC from the same object");
+ return -EINVAL;
+ }
+
+ links->linked_object_bitmap |= 1 << crtc->crtc_config_idx;
+
+ return 0;
+}
+
+static void possible_crtcs_drop_link(struct config_item *src,
+ struct config_item *target)
+{
+ struct vkms_config_links *links = item_to_config_links(src);
+ struct vkms_config_crtc *crtc = item_to_config_crtc(target);
+
+ links->linked_object_bitmap &= ~(1 << crtc->crtc_config_idx);
+}
+
+static struct configfs_item_operations possible_crtcs_item_ops = {
+ .allow_link = &possible_crtcs_allow_link,
+ .drop_link = &possible_crtcs_drop_link,
+};
+
+static struct config_item_type possible_crtcs_group_type = {
+ .ct_item_ops = &possible_crtcs_item_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+static void add_possible_crtcs(struct config_group *parent,
+ struct config_group *possible_crtcs)
+{
+ config_group_init_type_name(possible_crtcs, "possible_crtcs",
+ &possible_crtcs_group_type);
+ configfs_add_default_group(possible_crtcs, parent);
+}
+
+/* Possible encoders, e.g. /config/vkms/card/connector/possible_encoders/<symlink> */
+
+static struct config_item_type encoder_type;
+
+static int possible_encoders_allow_link(struct config_item *src,
+ struct config_item *target)
+{
+ struct vkms_config_links *links = item_to_config_links(src);
+ struct vkms_config_encoder *encoder;
+
+ if (target->ci_type != &encoder_type) {
+ DRM_ERROR("Unable to link non-encoders.");
+ return -EINVAL;
+ }
+
+ encoder = item_to_config_encoder(target);
+
+ if (links->linked_object_bitmap & (1 << encoder->encoder_config_idx)) {
+ DRM_ERROR(
+ "Tried to add two symlinks to the same encoder from the same object");
+ return -EINVAL;
+ }
+
+ links->linked_object_bitmap |= 1 << encoder->encoder_config_idx;
+
+ return 0;
+}
+
+static void possible_encoders_drop_link(struct config_item *src,
+ struct config_item *target)
+{
+ struct vkms_config_links *links = item_to_config_links(src);
+ struct vkms_config_encoder *encoder = item_to_config_encoder(target);
+
+ links->linked_object_bitmap &= ~(1 << encoder->encoder_config_idx);
+}
+
+static struct configfs_item_operations possible_encoders_item_ops = {
+ .allow_link = &possible_encoders_allow_link,
+ .drop_link = &possible_encoders_drop_link,
+};
+
+static struct config_item_type possible_encoders_group_type = {
+ .ct_item_ops = &possible_encoders_item_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+static void add_possible_encoders(struct config_group *parent,
+ struct config_group *possible_encoders)
+{
+ config_group_init_type_name(possible_encoders, "possible_encoders",
+ &possible_encoders_group_type);
+ configfs_add_default_group(possible_encoders, parent);
+}
+
+/*
+ * Individual objects (connectors, crtcs, encoders, planes):
+ */
+
+/* Connector item, e.g. /config/vkms/card/connectors/ID */
+
+static struct config_item_type connector_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+/* Crtc item, e.g. /config/vkms/card/crtcs/ID */
+
+static int crtc_allow_link(struct config_item *src, struct config_item *target)
+{
+ // TODO delete?
+ return 0;
+}
+
+static struct configfs_item_operations crtc_item_ops = {
+ .allow_link = &crtc_allow_link,
+};
+
+static struct config_item_type crtc_type = {
+ .ct_item_ops = &crtc_item_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+/* Encoder item, e.g. /config/vkms/card/encoder/ID */
+
+static struct config_item_type encoder_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+/* Plane item, e.g. /config/vkms/card/planes/ID */
+
+static ssize_t plane_type_show(struct config_item *item, char *buf)
+{
+ struct vkms_config_plane *plane = item_to_config_plane(item);
+ return sprintf(buf, "%u", plane->type);
+}
+
+static ssize_t plane_type_store(struct config_item *item, const char *buf,
+ size_t len)
+{
+ struct vkms_config_plane *plane = item_to_config_plane(item);
+ int val, ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret) {
+ return -ret;
+ }
+
+ if (val != DRM_PLANE_TYPE_PRIMARY && val != DRM_PLANE_TYPE_CURSOR &&
+ val != DRM_PLANE_TYPE_OVERLAY) {
+ return -EINVAL;
+ }
+
+ plane->type = val;
+
+ return len;
+}
+
+CONFIGFS_ATTR(plane_, type);
+
+static struct configfs_attribute *plane_attrs[] = {
+ &plane_attr_type,
+ NULL,
+};
+
+static struct config_item_type plane_type = {
+ .ct_attrs = plane_attrs,
+ .ct_owner = THIS_MODULE,
+};
+
+/*
+ * Directory groups, e.g. /config/vkms/card/{planes, crtcs, ...}
+ */
+
+/* Connectors group: /config/vkms/card/connectors/ */
+
+static struct config_group *connectors_group_make(struct config_group *group,
+ const char *name)
+{
+ struct vkms_config_connector *connector =
+ kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
+ if (!connector)
+ return ERR_PTR(ENOMEM);
+
+ config_group_init_type_name(&connector->config_group, name,
+ &connector_type);
+ add_possible_encoders(&connector->config_group,
+ &connector->possible_encoders.group);
+
+ return &connector->config_group;
+}
+
+static void connectors_group_drop(struct config_group *group,
+ struct config_item *item)
+{
+ struct vkms_config_connector *connector =
+ item_to_config_connector(item);
+ kfree(connector);
+}
+
+static struct configfs_group_operations connectors_group_ops = {
+ .make_group = &connectors_group_make,
+ .drop_item = &connectors_group_drop,
+};
+
+static struct config_item_type connectors_group_type = {
+ .ct_group_ops = &connectors_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+/* CRTCs group: /config/vkms/card/crtcs/ */
+
+static struct config_group *crtcs_group_make(struct config_group *group,
+ const char *name)
+{
+ struct vkms_configfs *configfs =
+ container_of(group, struct vkms_configfs, crtcs_group);
+ unsigned long next_idx = find_first_zero_bit(&configfs->allocated_crtcs,
+ VKMS_MAX_OUTPUT_OBJECTS);
+ struct vkms_config_crtc *crtc;
+
+ if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
+ DRM_ERROR("Unable to allocate another CRTC.");
+ return ERR_PTR(ENOMEM);
+ }
+
+ crtc = kzalloc(sizeof(struct vkms_config_crtc), GFP_KERNEL);
+ if (!crtc)
+ return ERR_PTR(ENOMEM);
+
+ config_group_init_type_name(&crtc->config_group, name, &crtc_type);
+ crtc->crtc_config_idx = next_idx;
+
+ set_bit(next_idx, &configfs->allocated_crtcs);
+
+ return &crtc->config_group;
+}
+
+static void crtcs_group_drop(struct config_group *group,
+ struct config_item *item)
+{
+ struct vkms_config_crtc *crtc = item_to_config_crtc(item);
+ kfree(crtc);
+}
+
+static struct configfs_group_operations crtcs_group_ops = {
+ .make_group = &crtcs_group_make,
+ .drop_item = &crtcs_group_drop,
+};
+
+static struct config_item_type crtcs_group_type = {
+ .ct_group_ops = &crtcs_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+/* Encoders group: /config/vkms/card/encoders/ */
+
+static struct config_group *encoders_group_make(struct config_group *group,
+ const char *name)
+{
+ struct vkms_configfs *configfs =
+ container_of(group, struct vkms_configfs, encoders_group);
+ unsigned long next_idx = find_first_zero_bit(
+ &configfs->allocated_encoders, VKMS_MAX_OUTPUT_OBJECTS);
+ struct vkms_config_encoder *encoder;
+
+ if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
+ DRM_ERROR("Unable to allocate another encoder.");
+ return ERR_PTR(ENOMEM);
+ }
+
+ encoder = kzalloc(sizeof(struct vkms_config_encoder), GFP_KERNEL);
+ if (!encoder)
+ return ERR_PTR(ENOMEM);
+
+ config_group_init_type_name(&encoder->config_group, name,
+ &encoder_type);
+ add_possible_crtcs(&encoder->config_group,
+ &encoder->possible_crtcs.group);
+ encoder->encoder_config_idx = next_idx;
+ set_bit(next_idx, &configfs->allocated_encoders);
+
+ return &encoder->config_group;
+}
+
+static void encoders_group_drop(struct config_group *group,
+ struct config_item *item)
+{
+ struct vkms_config_encoder *encoder = item_to_config_encoder(item);
+ kfree(encoder);
+}
+
+static struct configfs_group_operations encoders_group_ops = {
+ .make_group = &encoders_group_make,
+ .drop_item = &encoders_group_drop,
+};
+
+static struct config_item_type encoders_group_type = {
+ .ct_group_ops = &encoders_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+/* Planes group: /config/vkms/card/planes/ */
+
+static struct config_group *make_plane_group(struct config_group *group,
+ const char *name)
+{
+ struct vkms_config_plane *plane =
+ kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
+ if (!plane)
+ return ERR_PTR(ENOMEM);
+
+ config_group_init_type_name(&plane->config_group, name, &plane_type);
+ add_possible_crtcs(&plane->config_group, &plane->possible_crtcs.group);
+
+ return &plane->config_group;
+}
+
+static void drop_plane_group(struct config_group *group,
+ struct config_item *item)
+{
+ struct vkms_config_plane *plane = item_to_config_plane(item);
+ kfree(plane);
+}
+
+static struct configfs_group_operations plane_group_ops = {
+ .make_group = &make_plane_group,
+ .drop_item = &drop_plane_group,
+};
+
+static struct config_item_type planes_group_type = {
+ .ct_group_ops = &plane_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+/* Root directory group, e.g. /config/vkms/card */
+
+static ssize_t card_is_registered_show(struct config_item *item, char *buf)
+{
+ struct vkms_configfs *configfs = item_to_configfs(item);
+ return sprintf(buf, "%d", configfs->card != NULL);
+}
+
+static ssize_t card_is_registered_store(struct config_item *item,
+ const char *buf, size_t len)
+{
+ struct vkms_configfs *configfs = item_to_configfs(item);
+ struct vkms_card *card;
+ int value, ret;
+
+ ret = kstrtoint(buf, 0, &value);
+ if (ret)
+ return ret;
+
+ if (value != 1)
+ return -EINVAL;
+
+ if (configfs->card)
+ return len;
+
+ card = vkms_card_init(item->ci_name, configfs);
+ if (IS_ERR(card))
+ return -PTR_ERR(card);
+
+ return len;
+}
+
+CONFIGFS_ATTR(card_, is_registered);
+
+static struct configfs_attribute *card_group_attrs[] = {
+ &card_attr_is_registered,
+ NULL,
+};
+
+static struct config_item_type card_group_type = {
+ .ct_attrs = card_group_attrs,
+ .ct_owner = THIS_MODULE,
+};
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 266c9f643f70..37b95ca28672 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -9,6 +9,9 @@
* the GPU in DRM API tests.
*/
+#include <linux/configfs.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/platform_device.h>
@@ -152,7 +155,7 @@ static int vkms_modeset_init(struct vkms_card *card)
dev->mode_config.preferred_depth = 0;
dev->mode_config.helper_private = &vkms_mode_config_helpers;
- return card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
+ return card->is_default ? vkms_output_init_default(card, 0) : vkms_output_init(card, 0);
}
static void vkms_device_destroy(void)
@@ -166,7 +169,7 @@ static void vkms_device_destroy(void)
memset(&vkms_device, 0, sizeof(struct vkms_device));
}
-struct vkms_card *vkms_card_init(const char *name, bool is_default)
+struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configfs)
{
char unique_name[64] = { 0 };
struct platform_device *pdev;
@@ -190,7 +193,7 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
ret = ENOMEM;
goto out_platform_device;
}
-
+
card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
drm);
if (IS_ERR(card)) {
@@ -202,7 +205,8 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
card->platform = pdev;
card->drm.unique = card->name_buf;
card->vkms_device = &vkms_device;
- card->is_default = is_default;
+ card->configfs = configfs;
+ card->is_default = configfs == NULL;
card->is_registered = false;
card->resource_group_id = grp;
@@ -235,7 +239,10 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
devres_close_group(&pdev->dev, grp);
list_add_tail(&card->node, &vkms_device.cards);
-
+
+ if (configfs)
+ configfs->card = card;
+
return card;
out_release_group:
@@ -260,16 +267,24 @@ void vkms_card_destroy(struct vkms_card *card)
static int __init vkms_init(void)
{
struct vkms_card *card;
+ int ret;
vkms_device.config.cursor = enable_cursor;
vkms_device.config.writeback = enable_writeback;
vkms_device.config.overlay = enable_overlay;
INIT_LIST_HEAD(&vkms_device.cards);
+
+ ret = vkms_init_configfs();
+ if (ret) {
+ DRM_ERROR("Unable to initialize configfs\n");
+ return ret;
+ }
card = vkms_card_init("default", /* configfs */ NULL);
if (IS_ERR(card)) {
- DRM_ERROR("Unable to init default card");
+ DRM_ERROR("Unable to init default card\n");
vkms_device_destroy();
+ vkms_unregister_configfs();
return PTR_ERR(card);
}
@@ -279,6 +294,7 @@ static int __init vkms_init(void)
static void __exit vkms_exit(void)
{
vkms_device_destroy();
+ vkms_unregister_configfs();
}
module_init(vkms_init);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 30ae06d7af71..f43e4c563863 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -3,6 +3,7 @@
#ifndef _VKMS_DRV_H_
#define _VKMS_DRV_H_
+#include <linux/configfs.h>
#include <linux/hrtimer.h>
#include <linux/kernel.h>
@@ -10,6 +11,7 @@
#include <drm/drm_gem.h>
#include <drm/drm_gem_atomic_helper.h>
#include <drm/drm_encoder.h>
+#include <drm/drm_plane.h>
#include <drm/drm_writeback.h>
#define XRES_MIN 20
@@ -116,11 +118,59 @@ struct vkms_config {
bool overlay;
};
+struct vkms_config_links {
+ struct config_group group;
+ unsigned long linked_object_bitmap;
+};
+
+struct vkms_config_connector {
+ struct config_group config_group;
+ struct vkms_config_links possible_encoders;
+};
+
+struct vkms_config_crtc {
+ struct config_group config_group;
+ unsigned long crtc_config_idx;
+};
+
+struct vkms_config_encoder {
+ struct config_group config_group;
+ struct vkms_config_links possible_crtcs;
+ unsigned long encoder_config_idx;
+};
+
+struct vkms_config_plane {
+ struct config_group config_group;
+ struct vkms_config_links possible_crtcs;
+ enum drm_plane_type type;
+};
+
+struct vkms_card;
+
+struct vkms_configfs {
+ /* Directory group containing connector configs, e.g. /config/vkms/card/ */
+ struct config_group card_group;
+ /* Directory group containing connector configs, e.g. /config/vkms/card/connectors/ */
+ struct config_group connectors_group;
+ /* Directory group containing CRTC configs, e.g. /config/vkms/card/crtcs/ */
+ struct config_group crtcs_group;
+ /* Directory group containing encoder configs, e.g. /config/vkms/card/encoders/ */
+ struct config_group encoders_group;
+ /* Directory group containing plane configs, e.g. /config/vkms/card/planes/ */
+ struct config_group planes_group;
+
+ unsigned long allocated_crtcs;
+ unsigned long allocated_encoders;
+
+ struct vkms_card *card;
+};
+
struct vkms_card {
struct platform_device *platform;
struct drm_device drm;
struct list_head node;
struct vkms_output output;
+ struct vkms_configfs *configfs;
struct vkms_device *vkms_device;
/* storage for the value of drm->unique, giving this dev a unique busid */
char name_buf[64];
@@ -149,8 +199,27 @@ struct vkms_device {
#define to_vkms_plane_state(target)\
container_of(target, struct vkms_plane_state, base.base)
+#define item_to_configfs(item)\
+ container_of(to_config_group(item), struct vkms_configfs, card_group)
+
+#define item_to_config_connector(item)\
+ container_of(to_config_group(item), struct vkms_config_connector, config_group)
+
+#define item_to_config_crtc(item) \
+ container_of(to_config_group(item), struct vkms_config_crtc, config_group)
+
+#define item_to_config_encoder(item) \
+ container_of(to_config_group(item), struct vkms_config_encoder, config_group)
+
+#define item_to_config_plane(item) \
+ container_of(to_config_group(item), struct vkms_config_plane, config_group)
+
+#define item_to_config_links(item) \
+ container_of(to_config_group(item), struct vkms_config_links, group)
+
/* Cards */
-struct vkms_card *vkms_card_init(const char *name, bool is_default);
+struct vkms_card *vkms_card_init(const char *name,
+ struct vkms_configfs *configfs);
void vkms_card_destroy(struct vkms_card *card);
/* CRTC */
@@ -176,4 +245,8 @@ void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
/* Writeback */
int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
+/* ConfigFS Support */
+int vkms_init_configfs(void);
+void vkms_unregister_configfs(void);
+
#endif /* _VKMS_DRV_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 2b72d8e763a8..e343a9c1f311 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -1,10 +1,14 @@
// SPDX-License-Identifier: GPL-2.0+
-#include "vkms_drv.h"
+#include <linux/configfs.h>
+#include <linux/kernel.h>
+
#include <drm/drm_atomic_helper.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
+#include "vkms_drv.h"
+
static void vkms_connector_destroy(struct drm_connector *connector)
{
drm_connector_cleanup(connector);
@@ -171,3 +175,8 @@ int vkms_output_init_default(struct vkms_card *card, int index)
return ret;
}
+
+int vkms_output_init(struct vkms_card *card, int index)
+{
+ return -ENOTSUPP;
+}
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/5] drm/vkms: Add ConfigFS scaffolding to VKMS
2022-07-22 21:32 ` [PATCH 4/5] drm/vkms: Add ConfigFS scaffolding to VKMS Jim Shargo
@ 2022-08-05 21:23 ` Sean Paul
0 siblings, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-05 21:23 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Thomas Zimmermann, Rodrigo Siqueira,
Jonathan Corbet, David Airlie, linux-doc, linux-kernel,
Melissa Wen, jshargo, dri-devel
On Fri, Jul 22, 2022 at 05:32:12PM -0400, Jim Shargo wrote:
> This change adds the basic scaffolding for ConfigFS, including setting
> up the default directories. It does not allow for the registration of
> configfs-backed devices, which is complex and provided in a follow-up
> commit.
>
> This CL includes docs about using ConfigFS with VKMS, but I'll summarize
> in brief here as well (assuming ConfigFS is mounted at /config/):
>
> To create a new configuration, you can do so via `mkdir
> /config/vkms/my-device`.
>
> This will create a number of directories and files automatically:
>
> /config
> `-- vkms
> `-- my-device
> |-- connectors
> |-- crtcs
> |-- encoders
> |-- planes
> `-- is_registered
>
> You can then configure objects by mkdir'ing in each of the directories.
>
> When you're satisfied, you can `echo 1 > /config/vkms/my-device/is_registered`.
> This will create a new device according to your configuration.
>
> For now, this will fail, but the next change will add support for it.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
> Documentation/gpu/vkms.rst | 76 ++++
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/vkms/Makefile | 1 +
> drivers/gpu/drm/vkms/vkms_configfs.c | 527 +++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_drv.c | 28 +-
> drivers/gpu/drm/vkms/vkms_drv.h | 75 +++-
> drivers/gpu/drm/vkms/vkms_output.c | 11 +-
> 7 files changed, 711 insertions(+), 8 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
>
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index 9c873c3912cc..f5f8ea0de5bc 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -51,6 +51,82 @@ To disable the driver, use ::
>
> sudo modprobe -r vkms
>
> +Configuration With ConfigFS
> +===========================
> +
> +VKMS is instrumented with support for configuration via `ConfigFS`. With VKMS
> +installed, you can mount ConfigFS at /config/ like so::
> +
> + mkdir -p /config/
> + sudo mount -t configfs none /config
> +
> +This allows you to configure multiple virtual cards in addition to an immutable
> +"default" card created by the driver at initialization time.
Is the default for backwards compatibility?
> +
> +To set up a new card, create a new directory under the vkms configfs
> +directory::
> +
> + $ mkdir /config/vkms/test
> +
> +With your card created you'll find an new directory ready to be configured::
> +
> + /config
> + `-- vkms
> + |-- default
> + | `-- is_registered
> + `-- test
> + |-- connectors
> + |-- crtcs
> + |-- encoders
> + |-- planes
> + `-- is_registered
> +
> +Each directory you add within the connectors, crtcs, encoders, and planes
> +directories will let you configure a new object of that type. Adding new
> +objects will automatically create a set of default files and folders you can
> +use to configure that object.
> +
> +For instance, we can set up a very basic driver like so::
> +
> + DRM_PLANE_TYPE_PRIMARY=1
> + DRM_PLANE_TYPE_CURSOR=2
> + DRM_PLANE_TYPE_OVERLAY=0
> +
> + mkdir /config/vkms/test/planes/primary
> + echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/primary/type
> +
> + mkdir /config/vkms/test/planes/other_primary
> + echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/other_primary/type
> +
> + mkdir /config/vkms/test/planes/cursor
> + echo $DRM_PLANE_TYPE_CURSOR > /config/vkms/test/planes/cursor/type
> +
> + mkdir /config/vkms/test/planes/overlay
> + echo $DRM_PLANE_TYPE_OVERLAY > /config/vkms/test/planes/overlay/type
> +
> + mkdir /config/vkms/test/crtcs/crtc
> + mkdir /config/vkms/test/crtcs/crtc_other
> + mkdir /config/vkms/test/encoders/encoder
> + mkdir /config/vkms/test/connectors/connector
> +
> + ln -s /config/vkms/test/encoders/encoder /config/vkms/test/connectors/connector/possible_encoders
> + ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/encoders/encoder/possible_crtcs/
> + ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/primary/possible_crtcs/
> + ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/cursor/possible_crtcs/
> + ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/overlay/possible_crtcs/
> + ln -s /config/vkms/test/crtcs/crtc_other /config/vkms/test/planes/overlay/possible_crtcs/
> +
> +You can see that specific attributes, such as `.../<plane>/type`, can be
> +configured by writing into them. Associating objects together can be done via
> +symlinks.
> +
> +Finally, to register your newly configured objects with DRM so you can use your
> +virtual device, all you need to do is write `1` into the special file
> +`/config/vkms/test/is_registered``, which will return non-zero on error.
> +
> +When you're done with the virtual card, all you need to do is `rmdir` the root
> +directory.
There seems to be some amount of overlap between this blurb and the one in
vkms_configfs.c. IMO it'd be better to unify these in the source and link it
here using:
.. kernel-doc:: drivers/gpu/drm/vkms/vkms_configfs.c
:doc: ConfigFS Support for VKMS
> +
> Testing With IGT
> ================
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index f1422bee3dcc..5c90c82fab6d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -307,6 +307,7 @@ config DRM_VKMS
> depends on DRM && MMU
> select DRM_KMS_HELPER
> select DRM_GEM_SHMEM_HELPER
> + select CONFIGFS_FS
> select CRC32
> default n
> help
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index 72f779cbfedd..3efb7b0f5319 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -3,6 +3,7 @@ vkms-y := \
> vkms_drv.o \
> vkms_plane.o \
> vkms_output.o \
> + vkms_configfs.o \
> vkms_crtc.o \
> vkms_composer.o \
> vkms_writeback.o
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> new file mode 100644
> index 000000000000..fa0d8700258e
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -0,0 +1,527 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/configfs.h>
> +#include <linux/kernel.h>
Same comment re: kernel.h
> +#include <linux/mutex.h>
> +
> +#include <drm/drm_plane.h>
> +#include <drm/drm_print.h>
> +
> +#include "vkms_drv.h"
> +
> +/**
> + * DOC: ConfigFS Support for VKMS
> + *
> + * When ConfigFS is mounted, say at /config/, we add a /config/vkms/
> + * directory. Each folder in this directory represents a different
> + * virtual driver. Drivers can be "registered", meaning that they're
> + * available to use as a DRM driver. Initially, this just contains a
> + * registered "default" device.
Nit: s/driver/device/ since the term drm device is much better specified than
drm driver.
> + *
> + * Users can mkdir in this the VKMS directory to begin the process of
> + * configuring a new device. Users can mkdir in the automatically
> + * created "connectors", "crtcs", "encoders", and "planes" folders to
> + * set up those objects.
> + *
> + * In order to create associations between objects, users can symlink
> + * the object's directory. For instance, to make a plane work with a
> + * CRTC, the user will add a symlink like so:
> + * /config/vkms/card/planes/<plane>/possible_crtcs/<crtc_symlink>
> + *
> + * Objects may have files (in ConfigFS-speak, attributes) that allow
> + * for additional configuration. For instance, a user can set the
> + * plane type by writing the appropriate value to the "type" property
> + * in the plane's directory.
> + *
> + * Once the driver is configured, the user can write a "1" to the
> + * "is_registered" file/attribute in the driver's directory. Until a
> + * virtual driver is registered, nothing DRM-related is set up.
> + */
> +
> +static struct config_item_type card_group_type;
> +static struct config_item_type connectors_group_type;
> +static struct config_item_type crtcs_group_type;
> +static struct config_item_type encoders_group_type;
> +static struct config_item_type planes_group_type;
Could you reorder things to avoid these icky forward declarations?
> +
> +static void vkms_configfs_setup_default_groups(struct vkms_configfs *configfs,
> + const char *name)
> +{
> + config_group_init_type_name(&configfs->card_group, name,
> + &card_group_type);
> +
> + config_group_init_type_name(&configfs->connectors_group, "connectors",
> + &connectors_group_type);
> + configfs_add_default_group(&configfs->connectors_group,
> + &configfs->card_group);
> +
> + config_group_init_type_name(&configfs->crtcs_group, "crtcs",
> + &crtcs_group_type);
> + configfs_add_default_group(&configfs->crtcs_group,
> + &configfs->card_group);
> +
> + config_group_init_type_name(&configfs->encoders_group, "encoders",
> + &encoders_group_type);
> + configfs_add_default_group(&configfs->encoders_group,
> + &configfs->card_group);
> +
> + config_group_init_type_name(&configfs->planes_group, "planes",
> + &planes_group_type);
> + configfs_add_default_group(&configfs->planes_group,
> + &configfs->card_group);
> +}
> +
> +/* Root directory group and subsystem, e.g. /config/vkms/ */
> +
> +static struct config_group *make_root_group(struct config_group *group,
> + const char *name)
> +{
> + struct vkms_configfs *configfs =
> + kzalloc(sizeof(struct vkms_configfs), GFP_KERNEL);
Nit: sizeof(*configfs) would be more compact. You could probably use
devm_kzalloc if you passed in device to avoid the kfree in drop_root_group().
> +
> + if (!configfs)
> + return ERR_PTR(ENOMEM);
> +
> + vkms_configfs_setup_default_groups(configfs, name);
> +
> + return &configfs->card_group;
> +}
> +
> +static void drop_root_group(struct config_group *group,
> + struct config_item *item)
> +{
> + struct vkms_configfs *configfs = item_to_configfs(item);
> +
> + if (configfs->card)
> + vkms_card_destroy(configfs->card);
> +
> + kfree(configfs);
> +}
> +
> +static struct configfs_group_operations root_group_ops = {
> + .make_group = &make_root_group,
> + .drop_item = &drop_root_group,
> +};
> +
> +static struct config_item_type vkms_type = {
> + .ct_group_ops = &root_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem vkms_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_name = "vkms",
> + .ci_type = &vkms_type,
> + },
> + },
If you do the following you can remove the mutex_init() below:
.su_mutex = __MUTEX_INITIALIZER(vkms_subsys.su_mutex),
> +};
> +
> +int vkms_init_configfs(void)
> +{
> + config_group_init(&vkms_subsys.su_group);
> + mutex_init(&vkms_subsys.su_mutex);
> + return configfs_register_subsystem(&vkms_subsys);
> +}
> +
> +void vkms_unregister_configfs(void)
> +{
> + configfs_unregister_subsystem(&vkms_subsys);
> +}
> +
> +/*
> + * Common helpers (common sub-groups)
> + */
> +
> +/* Possible CRTCs, e.g. /config/vkms/card/<object>/possible_crtcs/<symlink> */
> +
> +static struct config_item_type crtc_type;
Nit: Same comment re: forward declaration (and below too, I'll stop mentioning
it)
> +
> +static int possible_crtcs_allow_link(struct config_item *src,
> + struct config_item *target)
> +{
> + struct vkms_config_links *links = item_to_config_links(src);
> + struct vkms_config_crtc *crtc;
> +
> + if (target->ci_type != &crtc_type) {
> + DRM_ERROR("Unable to link non-CRTCs.");
> + return -EINVAL;
> + }
> +
> + crtc = item_to_config_crtc(target);
> +
> + if (links->linked_object_bitmap & (1 << crtc->crtc_config_idx)) {
> + DRM_ERROR(
> + "Tried to add two symlinks to the same CRTC from the same object");
> + return -EINVAL;
> + }
> +
> + links->linked_object_bitmap |= 1 << crtc->crtc_config_idx;
Nit: you can replace the shift with BIT(crtc->crtc_config_idx). Same comment
elsewhere in the file.
> +
> + return 0;
> +}
> +
> +static void possible_crtcs_drop_link(struct config_item *src,
> + struct config_item *target)
> +{
> + struct vkms_config_links *links = item_to_config_links(src);
> + struct vkms_config_crtc *crtc = item_to_config_crtc(target);
> +
> + links->linked_object_bitmap &= ~(1 << crtc->crtc_config_idx);
> +}
> +
> +static struct configfs_item_operations possible_crtcs_item_ops = {
> + .allow_link = &possible_crtcs_allow_link,
> + .drop_link = &possible_crtcs_drop_link,
> +};
> +
> +static struct config_item_type possible_crtcs_group_type = {
> + .ct_item_ops = &possible_crtcs_item_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static void add_possible_crtcs(struct config_group *parent,
> + struct config_group *possible_crtcs)
> +{
> + config_group_init_type_name(possible_crtcs, "possible_crtcs",
> + &possible_crtcs_group_type);
> + configfs_add_default_group(possible_crtcs, parent);
> +}
> +
> +/* Possible encoders, e.g. /config/vkms/card/connector/possible_encoders/<symlink> */
> +
> +static struct config_item_type encoder_type;
> +
> +static int possible_encoders_allow_link(struct config_item *src,
> + struct config_item *target)
> +{
> + struct vkms_config_links *links = item_to_config_links(src);
> + struct vkms_config_encoder *encoder;
> +
> + if (target->ci_type != &encoder_type) {
> + DRM_ERROR("Unable to link non-encoders.");
> + return -EINVAL;
> + }
> +
> + encoder = item_to_config_encoder(target);
> +
> + if (links->linked_object_bitmap & (1 << encoder->encoder_config_idx)) {
> + DRM_ERROR(
> + "Tried to add two symlinks to the same encoder from the same object");
> + return -EINVAL;
> + }
> +
> + links->linked_object_bitmap |= 1 << encoder->encoder_config_idx;
> +
> + return 0;
> +}
> +
> +static void possible_encoders_drop_link(struct config_item *src,
> + struct config_item *target)
> +{
> + struct vkms_config_links *links = item_to_config_links(src);
> + struct vkms_config_encoder *encoder = item_to_config_encoder(target);
> +
> + links->linked_object_bitmap &= ~(1 << encoder->encoder_config_idx);
> +}
> +
> +static struct configfs_item_operations possible_encoders_item_ops = {
> + .allow_link = &possible_encoders_allow_link,
> + .drop_link = &possible_encoders_drop_link,
> +};
> +
> +static struct config_item_type possible_encoders_group_type = {
> + .ct_item_ops = &possible_encoders_item_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static void add_possible_encoders(struct config_group *parent,
> + struct config_group *possible_encoders)
> +{
> + config_group_init_type_name(possible_encoders, "possible_encoders",
> + &possible_encoders_group_type);
> + configfs_add_default_group(possible_encoders, parent);
> +}
> +
> +/*
> + * Individual objects (connectors, crtcs, encoders, planes):
> + */
> +
> +/* Connector item, e.g. /config/vkms/card/connectors/ID */
> +
> +static struct config_item_type connector_type = {
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Crtc item, e.g. /config/vkms/card/crtcs/ID */
> +
> +static int crtc_allow_link(struct config_item *src, struct config_item *target)
> +{
> + // TODO delete?
Could you expand on this TODO?
> + return 0;
> +}
> +
> +static struct configfs_item_operations crtc_item_ops = {
> + .allow_link = &crtc_allow_link,
> +};
> +
> +static struct config_item_type crtc_type = {
> + .ct_item_ops = &crtc_item_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Encoder item, e.g. /config/vkms/card/encoder/ID */
> +
> +static struct config_item_type encoder_type = {
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Plane item, e.g. /config/vkms/card/planes/ID */
> +
> +static ssize_t plane_type_show(struct config_item *item, char *buf)
> +{
> + struct vkms_config_plane *plane = item_to_config_plane(item);
> + return sprintf(buf, "%u", plane->type);
> +}
> +
> +static ssize_t plane_type_store(struct config_item *item, const char *buf,
> + size_t len)
> +{
> + struct vkms_config_plane *plane = item_to_config_plane(item);
> + int val, ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret) {
> + return -ret;
I think kstrtouint already returns negative errno.
> + }
Nit: Remove braces
> +
> + if (val != DRM_PLANE_TYPE_PRIMARY && val != DRM_PLANE_TYPE_CURSOR &&
> + val != DRM_PLANE_TYPE_OVERLAY) {
> + return -EINVAL;
> + }
Nit: Here too
> +
> + plane->type = val;
> +
> + return len;
> +}
> +
> +CONFIGFS_ATTR(plane_, type);
> +
> +static struct configfs_attribute *plane_attrs[] = {
> + &plane_attr_type,
> + NULL,
> +};
> +
> +static struct config_item_type plane_type = {
> + .ct_attrs = plane_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/*
> + * Directory groups, e.g. /config/vkms/card/{planes, crtcs, ...}
> + */
> +
> +/* Connectors group: /config/vkms/card/connectors/ */
> +
> +static struct config_group *connectors_group_make(struct config_group *group,
> + const char *name)
> +{
> + struct vkms_config_connector *connector =
> + kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
Nit: Add blank line here, use sizeof(*connector) and consider switching to
devm_kzalloc. There might be other places I missed with this feedback.
> + if (!connector)
> + return ERR_PTR(ENOMEM);
Can you please return negative errno? There are a few other places with positive
ERR_PTR values which should be converted.
> +
> + config_group_init_type_name(&connector->config_group, name,
> + &connector_type);
> + add_possible_encoders(&connector->config_group,
> + &connector->possible_encoders.group);
> +
> + return &connector->config_group;
> +}
> +
> +static void connectors_group_drop(struct config_group *group,
> + struct config_item *item)
> +{
> + struct vkms_config_connector *connector =
> + item_to_config_connector(item);
Nit: Here too.
> + kfree(connector);
> +}
> +
> +static struct configfs_group_operations connectors_group_ops = {
> + .make_group = &connectors_group_make,
> + .drop_item = &connectors_group_drop,
> +};
> +
> +static struct config_item_type connectors_group_type = {
> + .ct_group_ops = &connectors_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* CRTCs group: /config/vkms/card/crtcs/ */
> +
> +static struct config_group *crtcs_group_make(struct config_group *group,
> + const char *name)
> +{
> + struct vkms_configfs *configfs =
> + container_of(group, struct vkms_configfs, crtcs_group);
> + unsigned long next_idx = find_first_zero_bit(&configfs->allocated_crtcs,
> + VKMS_MAX_OUTPUT_OBJECTS);
> + struct vkms_config_crtc *crtc;
> +
> + if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
> + DRM_ERROR("Unable to allocate another CRTC.");
> + return ERR_PTR(ENOMEM);
> + }
> +
> + crtc = kzalloc(sizeof(struct vkms_config_crtc), GFP_KERNEL);
> + if (!crtc)
> + return ERR_PTR(ENOMEM);
> +
> + config_group_init_type_name(&crtc->config_group, name, &crtc_type);
> + crtc->crtc_config_idx = next_idx;
> +
> + set_bit(next_idx, &configfs->allocated_crtcs);
> +
> + return &crtc->config_group;
> +}
> +
> +static void crtcs_group_drop(struct config_group *group,
> + struct config_item *item)
> +{
> + struct vkms_config_crtc *crtc = item_to_config_crtc(item);
> + kfree(crtc);
> +}
> +
> +static struct configfs_group_operations crtcs_group_ops = {
> + .make_group = &crtcs_group_make,
> + .drop_item = &crtcs_group_drop,
> +};
> +
> +static struct config_item_type crtcs_group_type = {
> + .ct_group_ops = &crtcs_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Encoders group: /config/vkms/card/encoders/ */
> +
> +static struct config_group *encoders_group_make(struct config_group *group,
> + const char *name)
> +{
> + struct vkms_configfs *configfs =
> + container_of(group, struct vkms_configfs, encoders_group);
> + unsigned long next_idx = find_first_zero_bit(
> + &configfs->allocated_encoders, VKMS_MAX_OUTPUT_OBJECTS);
> + struct vkms_config_encoder *encoder;
> +
> + if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
> + DRM_ERROR("Unable to allocate another encoder.");
> + return ERR_PTR(ENOMEM);
> + }
> +
> + encoder = kzalloc(sizeof(struct vkms_config_encoder), GFP_KERNEL);
> + if (!encoder)
> + return ERR_PTR(ENOMEM);
> +
> + config_group_init_type_name(&encoder->config_group, name,
> + &encoder_type);
> + add_possible_crtcs(&encoder->config_group,
> + &encoder->possible_crtcs.group);
> + encoder->encoder_config_idx = next_idx;
> + set_bit(next_idx, &configfs->allocated_encoders);
> +
> + return &encoder->config_group;
> +}
> +
> +static void encoders_group_drop(struct config_group *group,
> + struct config_item *item)
> +{
> + struct vkms_config_encoder *encoder = item_to_config_encoder(item);
> + kfree(encoder);
> +}
> +
> +static struct configfs_group_operations encoders_group_ops = {
> + .make_group = &encoders_group_make,
> + .drop_item = &encoders_group_drop,
> +};
> +
> +static struct config_item_type encoders_group_type = {
> + .ct_group_ops = &encoders_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Planes group: /config/vkms/card/planes/ */
> +
> +static struct config_group *make_plane_group(struct config_group *group,
> + const char *name)
> +{
> + struct vkms_config_plane *plane =
> + kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
> + if (!plane)
> + return ERR_PTR(ENOMEM);
> +
> + config_group_init_type_name(&plane->config_group, name, &plane_type);
> + add_possible_crtcs(&plane->config_group, &plane->possible_crtcs.group);
> +
> + return &plane->config_group;
> +}
> +
> +static void drop_plane_group(struct config_group *group,
> + struct config_item *item)
> +{
> + struct vkms_config_plane *plane = item_to_config_plane(item);
> + kfree(plane);
> +}
> +
> +static struct configfs_group_operations plane_group_ops = {
> + .make_group = &make_plane_group,
> + .drop_item = &drop_plane_group,
> +};
> +
> +static struct config_item_type planes_group_type = {
> + .ct_group_ops = &plane_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +/* Root directory group, e.g. /config/vkms/card */
> +
> +static ssize_t card_is_registered_show(struct config_item *item, char *buf)
> +{
> + struct vkms_configfs *configfs = item_to_configfs(item);
> + return sprintf(buf, "%d", configfs->card != NULL);
> +}
> +
> +static ssize_t card_is_registered_store(struct config_item *item,
> + const char *buf, size_t len)
> +{
> + struct vkms_configfs *configfs = item_to_configfs(item);
> + struct vkms_card *card;
> + int value, ret;
> +
> + ret = kstrtoint(buf, 0, &value);
> + if (ret)
> + return ret;
> +
> + if (value != 1)
> + return -EINVAL;
> +
> + if (configfs->card)
> + return len;
> +
> + card = vkms_card_init(item->ci_name, configfs);
> + if (IS_ERR(card))
> + return -PTR_ERR(card);
I just realized that vkms_card_init() returns a combination of positive and
negative errnos (encoded as PTR). We'll probably want to convert all of those to
negative errnos and return PTR_ERR(card) here.
Generally speaking, everything should return negative errno on error.
> +
> + return len;
> +}
> +
> +CONFIGFS_ATTR(card_, is_registered);
> +
> +static struct configfs_attribute *card_group_attrs[] = {
> + &card_attr_is_registered,
> + NULL,
> +};
> +
> +static struct config_item_type card_group_type = {
> + .ct_attrs = card_group_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 266c9f643f70..37b95ca28672 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,6 +9,9 @@
> * the GPU in DRM API tests.
> */
>
> +#include <linux/configfs.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> @@ -152,7 +155,7 @@ static int vkms_modeset_init(struct vkms_card *card)
> dev->mode_config.preferred_depth = 0;
> dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
> - return card->is_default ? vkms_output_init_default(card, 0) : -ENOTSUPP;
> + return card->is_default ? vkms_output_init_default(card, 0) : vkms_output_init(card, 0);
> }
>
> static void vkms_device_destroy(void)
> @@ -166,7 +169,7 @@ static void vkms_device_destroy(void)
> memset(&vkms_device, 0, sizeof(struct vkms_device));
> }
>
> -struct vkms_card *vkms_card_init(const char *name, bool is_default)
> +struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configfs)
> {
> char unique_name[64] = { 0 };
> struct platform_device *pdev;
> @@ -190,7 +193,7 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
> ret = ENOMEM;
> goto out_platform_device;
> }
> -
> +
Nit: Trailing whitespace introduction?
> card = devm_drm_dev_alloc(&pdev->dev, &vkms_driver, struct vkms_card,
> drm);
> if (IS_ERR(card)) {
> @@ -202,7 +205,8 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
> card->platform = pdev;
> card->drm.unique = card->name_buf;
> card->vkms_device = &vkms_device;
> - card->is_default = is_default;
> + card->configfs = configfs;
> + card->is_default = configfs == NULL;
IMO we should get rid of is_default and just check !configfs if they're
equivalent.
> card->is_registered = false;
> card->resource_group_id = grp;
>
> @@ -235,7 +239,10 @@ struct vkms_card *vkms_card_init(const char *name, bool is_default)
>
> devres_close_group(&pdev->dev, grp);
> list_add_tail(&card->node, &vkms_device.cards);
> -
> +
> + if (configfs)
> + configfs->card = card;
> +
> return card;
>
> out_release_group:
> @@ -260,16 +267,24 @@ void vkms_card_destroy(struct vkms_card *card)
> static int __init vkms_init(void)
> {
> struct vkms_card *card;
> + int ret;
>
> vkms_device.config.cursor = enable_cursor;
> vkms_device.config.writeback = enable_writeback;
> vkms_device.config.overlay = enable_overlay;
> INIT_LIST_HEAD(&vkms_device.cards);
> +
> + ret = vkms_init_configfs();
> + if (ret) {
> + DRM_ERROR("Unable to initialize configfs\n");
> + return ret;
> + }
>
> card = vkms_card_init("default", /* configfs */ NULL);
> if (IS_ERR(card)) {
> - DRM_ERROR("Unable to init default card");
> + DRM_ERROR("Unable to init default card\n");
Does this belong in an earlier change?
> vkms_device_destroy();
> + vkms_unregister_configfs();
vkms_unregister_configfs() always directly follows vkms_device_destroy(), seems
like it could just be called from destroy?
> return PTR_ERR(card);
> }
>
> @@ -279,6 +294,7 @@ static int __init vkms_init(void)
> static void __exit vkms_exit(void)
> {
> vkms_device_destroy();
> + vkms_unregister_configfs();
> }
>
> module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 30ae06d7af71..f43e4c563863 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -3,6 +3,7 @@
> #ifndef _VKMS_DRV_H_
> #define _VKMS_DRV_H_
>
> +#include <linux/configfs.h>
> #include <linux/hrtimer.h>
> #include <linux/kernel.h>
>
> @@ -10,6 +11,7 @@
> #include <drm/drm_gem.h>
> #include <drm/drm_gem_atomic_helper.h>
> #include <drm/drm_encoder.h>
> +#include <drm/drm_plane.h>
> #include <drm/drm_writeback.h>
>
> #define XRES_MIN 20
> @@ -116,11 +118,59 @@ struct vkms_config {
> bool overlay;
> };
>
> +struct vkms_config_links {
> + struct config_group group;
> + unsigned long linked_object_bitmap;
> +};
> +
> +struct vkms_config_connector {
> + struct config_group config_group;
> + struct vkms_config_links possible_encoders;
> +};
> +
> +struct vkms_config_crtc {
> + struct config_group config_group;
> + unsigned long crtc_config_idx;
> +};
> +
> +struct vkms_config_encoder {
> + struct config_group config_group;
> + struct vkms_config_links possible_crtcs;
> + unsigned long encoder_config_idx;
> +};
> +
> +struct vkms_config_plane {
> + struct config_group config_group;
> + struct vkms_config_links possible_crtcs;
> + enum drm_plane_type type;
> +};
> +
> +struct vkms_card;
> +
> +struct vkms_configfs {
> + /* Directory group containing connector configs, e.g. /config/vkms/card/ */
> + struct config_group card_group;
> + /* Directory group containing connector configs, e.g. /config/vkms/card/connectors/ */
> + struct config_group connectors_group;
> + /* Directory group containing CRTC configs, e.g. /config/vkms/card/crtcs/ */
> + struct config_group crtcs_group;
> + /* Directory group containing encoder configs, e.g. /config/vkms/card/encoders/ */
> + struct config_group encoders_group;
> + /* Directory group containing plane configs, e.g. /config/vkms/card/planes/ */
> + struct config_group planes_group;
> +
> + unsigned long allocated_crtcs;
> + unsigned long allocated_encoders;
For bitmasks you probably want to explicitly specify their length by using
u32/u64 types. This probably applies elsewhere too.
> +
> + struct vkms_card *card;
> +};
> +
> struct vkms_card {
> struct platform_device *platform;
> struct drm_device drm;
> struct list_head node;
> struct vkms_output output;
> + struct vkms_configfs *configfs;
> struct vkms_device *vkms_device;
> /* storage for the value of drm->unique, giving this dev a unique busid */
> char name_buf[64];
> @@ -149,8 +199,27 @@ struct vkms_device {
> #define to_vkms_plane_state(target)\
> container_of(target, struct vkms_plane_state, base.base)
>
> +#define item_to_configfs(item)\
> + container_of(to_config_group(item), struct vkms_configfs, card_group)
> +
> +#define item_to_config_connector(item)\
> + container_of(to_config_group(item), struct vkms_config_connector, config_group)
> +
> +#define item_to_config_crtc(item) \
> + container_of(to_config_group(item), struct vkms_config_crtc, config_group)
> +
> +#define item_to_config_encoder(item) \
> + container_of(to_config_group(item), struct vkms_config_encoder, config_group)
> +
> +#define item_to_config_plane(item) \
> + container_of(to_config_group(item), struct vkms_config_plane, config_group)
> +
> +#define item_to_config_links(item) \
> + container_of(to_config_group(item), struct vkms_config_links, group)
> +
> /* Cards */
> -struct vkms_card *vkms_card_init(const char *name, bool is_default);
> +struct vkms_card *vkms_card_init(const char *name,
> + struct vkms_configfs *configfs);
> void vkms_card_destroy(struct vkms_card *card);
>
> /* CRTC */
> @@ -176,4 +245,8 @@ void vkms_set_composer(struct vkms_crtc *vkms_crtc, bool enabled);
> /* Writeback */
> int vkms_enable_writeback_connector(struct vkms_card *card, struct vkms_crtc *vkms_crtc);
>
> +/* ConfigFS Support */
> +int vkms_init_configfs(void);
> +void vkms_unregister_configfs(void);
> +
> #endif /* _VKMS_DRV_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 2b72d8e763a8..e343a9c1f311 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -1,10 +1,14 @@
> // SPDX-License-Identifier: GPL-2.0+
>
> -#include "vkms_drv.h"
> +#include <linux/configfs.h>
> +#include <linux/kernel.h>
> +
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
>
> +#include "vkms_drv.h"
> +
> static void vkms_connector_destroy(struct drm_connector *connector)
> {
> drm_connector_cleanup(connector);
> @@ -171,3 +175,8 @@ int vkms_output_init_default(struct vkms_card *card, int index)
>
> return ret;
> }
> +
> +int vkms_output_init(struct vkms_card *card, int index)
> +{
> + return -ENOTSUPP;
> +}
I think you can drop this from the patch and introduce it when it's useful.
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] drm/vkms: Support registering configfs devices
[not found] <20220722213214.1377835-1-jshargo@chromium.org>
` (3 preceding siblings ...)
2022-07-22 21:32 ` [PATCH 4/5] drm/vkms: Add ConfigFS scaffolding to VKMS Jim Shargo
@ 2022-07-22 21:32 ` Jim Shargo
2022-08-08 15:15 ` Sean Paul
4 siblings, 1 reply; 12+ messages in thread
From: Jim Shargo @ 2022-07-22 21:32 UTC (permalink / raw)
To: jshargo, Rodrigo Siqueira, Melissa Wen, Haneen Mohammed,
Daniel Vetter, David Airlie
Cc: Jim Shargo, linux-kernel, dri-devel
VKMS now supports creating virtual cards and registering them to create
real drm devices.
In addition to the registration logic, this commit also prevents users
from adding new objects once a card is registered.
Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
drivers/gpu/drm/vkms/vkms_configfs.c | 27 +++-
drivers/gpu/drm/vkms/vkms_drv.c | 21 ++-
drivers/gpu/drm/vkms/vkms_drv.h | 3 +
drivers/gpu/drm/vkms/vkms_output.c | 227 ++++++++++++++++++++++++---
4 files changed, 251 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index fa0d8700258e..6f0f4e39864a 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -326,8 +326,15 @@ static struct config_item_type plane_type = {
static struct config_group *connectors_group_make(struct config_group *group,
const char *name)
{
- struct vkms_config_connector *connector =
- kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
+ struct vkms_configfs *configfs =
+ container_of(group, struct vkms_configfs, connectors_group);
+ struct vkms_config_connector *connector;
+
+ if (configfs->card) {
+ return ERR_PTR(EBUSY);
+ }
+
+ connector = kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
if (!connector)
return ERR_PTR(ENOMEM);
@@ -368,6 +375,9 @@ static struct config_group *crtcs_group_make(struct config_group *group,
VKMS_MAX_OUTPUT_OBJECTS);
struct vkms_config_crtc *crtc;
+ if (configfs->card)
+ return ERR_PTR(EBUSY);
+
if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
DRM_ERROR("Unable to allocate another CRTC.");
return ERR_PTR(ENOMEM);
@@ -413,6 +423,9 @@ static struct config_group *encoders_group_make(struct config_group *group,
&configfs->allocated_encoders, VKMS_MAX_OUTPUT_OBJECTS);
struct vkms_config_encoder *encoder;
+ if (configfs->card)
+ return ERR_PTR(EBUSY);
+
if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
DRM_ERROR("Unable to allocate another encoder.");
return ERR_PTR(ENOMEM);
@@ -454,8 +467,14 @@ static struct config_item_type encoders_group_type = {
static struct config_group *make_plane_group(struct config_group *group,
const char *name)
{
- struct vkms_config_plane *plane =
- kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
+ struct vkms_configfs *configfs =
+ container_of(group, struct vkms_configfs, planes_group);
+ struct vkms_config_plane *plane;
+
+ if (configfs->card)
+ return ERR_PTR(EBUSY);
+
+ plane = kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
if (!plane)
return ERR_PTR(ENOMEM);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 37b95ca28672..4e00f3b0de7d 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -216,7 +216,7 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
goto out_release_group;
}
- ret = drm_vblank_init(&card->drm, 1);
+ ret = drm_vblank_init(&card->drm, vkms_card_crtc_count(card));
if (ret) {
DRM_ERROR("Failed to vblank\n");
goto out_release_group;
@@ -231,7 +231,7 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
ret = drm_dev_register(&card->drm, 0);
if (ret) {
DRM_ERROR("Unable to register card");
- return ERR_PTR(ret);
+ goto out_modeset;
}
drm_fbdev_generic_setup(&card->drm, 0);
@@ -245,6 +245,8 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
return card;
+out_modeset:
+ vkms_output_clear(card);
out_release_group:
devres_release_group(&pdev->dev, grp);
out_platform_device:
@@ -264,6 +266,21 @@ void vkms_card_destroy(struct vkms_card *card)
}
}
+int vkms_card_crtc_count(struct vkms_card *card)
+{
+ struct list_head *item;
+ int count = 0;
+
+ if (card->is_default)
+ return 1;
+
+ BUG_ON(!card->configfs);
+ list_for_each(item, &card->configfs->crtcs_group.cg_children) {
+ count += 1;
+ }
+ return count;
+}
+
static int __init vkms_init(void)
{
struct vkms_card *card;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index f43e4c563863..2e6bfed890f9 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -222,11 +222,14 @@ struct vkms_card *vkms_card_init(const char *name,
struct vkms_configfs *configfs);
void vkms_card_destroy(struct vkms_card *card);
+int vkms_card_crtc_count(struct vkms_card *card);
+
/* CRTC */
struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
int vkms_output_init_default(struct vkms_card *card, int index);
int vkms_output_init(struct vkms_card *card, int index);
+void vkms_output_clear(struct vkms_card *card);
struct vkms_plane *vkms_plane_init(struct vkms_card *card,
enum drm_plane_type type);
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index e343a9c1f311..857cd8593dce 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -4,6 +4,10 @@
#include <linux/kernel.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_plane.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
@@ -46,7 +50,7 @@ static struct drm_connector *vkms_connector_init(struct vkms_card *card)
connector = &card->output.connectors[card->output.num_connectors++];
ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
- DRM_MODE_CONNECTOR_VIRTUAL);
+ DRM_MODE_CONNECTOR_VIRTUAL);
if (ret) {
memset(connector, 0, sizeof(*connector));
card->output.num_connectors -= 1;
@@ -79,7 +83,6 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
int vkms_output_init_default(struct vkms_card *card, int index)
{
const struct vkms_config *config = &card->vkms_device->config;
- struct vkms_output *output = &card->output;
struct drm_device *dev = &card->drm;
struct drm_connector *connector;
struct drm_encoder *encoder;
@@ -99,7 +102,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
if (IS_ERR(overlay)) {
ret = PTR_ERR(overlay);
- goto err_planes;
+ goto cleanup_output;
}
}
}
@@ -108,7 +111,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
if (IS_ERR(cursor)) {
ret = PTR_ERR(cursor);
- goto err_planes;
+ goto cleanup_output;
}
}
@@ -117,7 +120,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
if (IS_ERR(vkms_crtc)) {
DRM_ERROR("Failed to init crtc\n");
ret = PTR_ERR(vkms_crtc);
- goto err_planes;
+ goto cleanup_output;
}
for (int i = 0; i < card->output.num_planes; i++) {
@@ -129,22 +132,21 @@ int vkms_output_init_default(struct vkms_card *card, int index)
if (IS_ERR(connector)) {
DRM_ERROR("Failed to init connector\n");
ret = PTR_ERR(connector);
- goto err_connector;
+ goto cleanup_output;
}
encoder = vkms_encoder_init(card);
if (IS_ERR(encoder)) {
DRM_ERROR("Failed to init encoder\n");
ret = PTR_ERR(encoder);
- goto err_encoder;
+ goto cleanup_output;
}
encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
-
ret = drm_connector_attach_encoder(connector, encoder);
if (ret) {
DRM_ERROR("Failed to attach connector to encoder\n");
- goto err_attach;
+ goto cleanup_output;
}
if (config->writeback) {
@@ -157,26 +159,209 @@ int vkms_output_init_default(struct vkms_card *card, int index)
return 0;
-err_attach:
- drm_encoder_cleanup(encoder);
+cleanup_output:
+ vkms_output_clear(card);
+ return ret;
+}
-err_encoder:
- drm_connector_cleanup(connector);
+static bool is_object_linked(struct vkms_config_links *links, unsigned long idx)
+{
+ return links->linked_object_bitmap & (1 << idx);
+}
-err_connector:
- drm_crtc_cleanup(&vkms_crtc->base);
+int vkms_output_init(struct vkms_card *card, int index)
+{
+ struct drm_device *dev = &card->drm;
+ struct vkms_configfs *configfs = card->configfs;
+ struct vkms_output *output = &card->output;
+ struct plane_map {
+ struct vkms_config_plane *config_plane;
+ struct vkms_plane *plane;
+ } plane_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
+ struct encoder_map {
+ struct vkms_config_encoder *config_encoder;
+ struct drm_encoder *encoder;
+ } encoder_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
+ struct config_item *item;
+ int i, ret;
+
+ i = 0;
+ list_for_each_entry (item, &configfs->planes_group.cg_children,
+ ci_entry) {
+ struct vkms_config_plane *config_plane =
+ item_to_config_plane(item);
+ struct vkms_plane *plane =
+ vkms_plane_init(card, config_plane->type);
+
+ if (IS_ERR(plane)) {
+ DRM_ERROR("Unable to init plane from config: %s",
+ item->ci_name);
+ ret = PTR_ERR(plane);
+ goto cleanup_output;
+ }
-err_planes:
- for (int i = 0; i < output->num_planes; i++) {
- drm_plane_cleanup(&output->planes[i].base);
+ plane_map[i].config_plane = config_plane;
+ plane_map[i].plane = plane;
+ i += 1;
+ }
+
+ i = 0;
+ list_for_each_entry (item, &configfs->encoders_group.cg_children,
+ ci_entry) {
+ struct vkms_config_encoder *config_encoder =
+ item_to_config_encoder(item);
+ struct drm_encoder *encoder = vkms_encoder_init(card);
+
+ if (IS_ERR(encoder)) {
+ DRM_ERROR("Failed to init config encoder: %s",
+ item->ci_name);
+ ret = PTR_ERR(encoder);
+ goto cleanup_output;
+ }
+ encoder_map[i].config_encoder = config_encoder;
+ encoder_map[i].encoder = encoder;
+ i += 1;
+ }
+
+ list_for_each_entry (item, &configfs->connectors_group.cg_children,
+ ci_entry) {
+ struct vkms_config_connector *config_connector =
+ item_to_config_connector(item);
+ struct drm_connector *connector = vkms_connector_init(card);
+
+ if (IS_ERR(connector)) {
+ DRM_ERROR("Failed to init connector from config: %s",
+ item->ci_name);
+ ret = PTR_ERR(connector);
+ goto cleanup_output;
+ }
+
+ for (int j = 0; j < output->num_connectors; j++) {
+ struct encoder_map *encoder = &encoder_map[j];
+
+ if (is_object_linked(
+ &config_connector->possible_encoders,
+ encoder->config_encoder
+ ->encoder_config_idx)) {
+ drm_connector_attach_encoder(connector,
+ encoder->encoder);
+ }
+ }
+ }
+
+ list_for_each_entry (item, &configfs->crtcs_group.cg_children,
+ ci_entry) {
+ struct vkms_config_crtc *config_crtc =
+ item_to_config_crtc(item);
+ struct vkms_crtc *vkms_crtc;
+ struct drm_plane *primary = NULL, *cursor = NULL;
+
+ for (int j = 0; j < output->num_planes; j++) {
+ struct plane_map *plane_entry = &plane_map[j];
+ struct drm_plane *plane = &plane_entry->plane->base;
+
+ if (!is_object_linked(
+ &plane_entry->config_plane->possible_crtcs,
+ config_crtc->crtc_config_idx)) {
+ continue;
+ }
+
+ if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+ if (primary) {
+ DRM_WARN(
+ "Too many primary planes found for crtc %s.",
+ item->ci_name);
+ ret = EINVAL;
+ goto cleanup_output;
+ }
+ primary = plane;
+ } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+ if (cursor) {
+ DRM_WARN(
+ "Too many cursor planes found for crtc %s.",
+ item->ci_name);
+ ret = EINVAL;
+ goto cleanup_output;
+ }
+ cursor = plane;
+ }
+ }
+
+ if (!primary) {
+ DRM_WARN("No primary plane configured for crtc %s", item->ci_name);
+ ret = EINVAL;
+ goto cleanup_output;
+ }
+
+ // TODO add names to helper
+ vkms_crtc = vkms_crtc_init(card, primary, cursor);
+ if (IS_ERR(vkms_crtc)) {
+ DRM_WARN("Unable to init crtc from config: %s",
+ item->ci_name);
+ ret = PTR_ERR(vkms_crtc);
+ goto cleanup_output;
+ }
+
+ for (int j = 0; j < output->num_planes; j++) {
+ struct plane_map *plane_entry = &plane_map[j];
+
+ if (!plane_entry->plane)
+ break;
+
+ if (is_object_linked(
+ &plane_entry->config_plane->possible_crtcs,
+ config_crtc->crtc_config_idx)) {
+ plane_entry->plane->base.possible_crtcs |=
+ drm_crtc_mask(&vkms_crtc->base);
+ }
+ }
+
+ for (int j = 0; j < output->num_encoders; j++) {
+ struct encoder_map *encoder_entry = &encoder_map[j];
+
+ if (is_object_linked(&encoder_entry->config_encoder
+ ->possible_crtcs,
+ config_crtc->crtc_config_idx)) {
+ encoder_entry->encoder->possible_crtcs |=
+ drm_crtc_mask(&vkms_crtc->base);
+ }
+ }
+
+ if (card->vkms_device->config.writeback) {
+ ret = vkms_enable_writeback_connector(card, vkms_crtc);
+ if (ret)
+ DRM_WARN(
+ "Failed to init writeback connector for config crtc: %s",
+ item->ci_name);
+ }
}
- memset(output, 0, sizeof(struct vkms_output));
+ drm_mode_config_reset(dev);
+
+ return 0;
+cleanup_output:
+ vkms_output_clear(card);
+ resume_device_irqs(); // REMOVE
return ret;
}
-int vkms_output_init(struct vkms_card *card, int index)
+void vkms_output_clear(struct vkms_card *card)
{
- return -ENOTSUPP;
+ struct vkms_output *output = &card->output;
+
+ for (int i = 0; i < output->num_crtcs; i++) {
+ drm_crtc_cleanup(&output->crtcs[i].base);
+ }
+ for (int i = 0; i < output->num_encoders; i++) {
+ drm_encoder_cleanup(&output->encoders[i]);
+ }
+ for (int i = 0; i < output->num_connectors; i++) {
+ drm_connector_cleanup(&output->connectors[i]);
+ }
+ for (int i = 0; i < output->num_planes; i++) {
+ drm_plane_cleanup(&output->planes[i].base);
+ }
+
+ memset(output, 0, sizeof(*output));
}
--
2.37.1.359.gd136c6c3e2-goog
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 5/5] drm/vkms: Support registering configfs devices
2022-07-22 21:32 ` [PATCH 5/5] drm/vkms: Support registering configfs devices Jim Shargo
@ 2022-08-08 15:15 ` Sean Paul
0 siblings, 0 replies; 12+ messages in thread
From: Sean Paul @ 2022-08-08 15:15 UTC (permalink / raw)
To: Jim Shargo
Cc: Haneen Mohammed, Rodrigo Siqueira, David Airlie, linux-kernel,
dri-devel, Melissa Wen, jshargo
On Fri, Jul 22, 2022 at 05:32:13PM -0400, Jim Shargo wrote:
> VKMS now supports creating virtual cards and registering them to create
> real drm devices.
>
> In addition to the registration logic, this commit also prevents users
> from adding new objects once a card is registered.
>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
> drivers/gpu/drm/vkms/vkms_configfs.c | 27 +++-
> drivers/gpu/drm/vkms/vkms_drv.c | 21 ++-
> drivers/gpu/drm/vkms/vkms_drv.h | 3 +
> drivers/gpu/drm/vkms/vkms_output.c | 227 ++++++++++++++++++++++++---
> 4 files changed, 251 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index fa0d8700258e..6f0f4e39864a 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -326,8 +326,15 @@ static struct config_item_type plane_type = {
> static struct config_group *connectors_group_make(struct config_group *group,
> const char *name)
> {
> - struct vkms_config_connector *connector =
> - kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
> + struct vkms_configfs *configfs =
> + container_of(group, struct vkms_configfs, connectors_group);
> + struct vkms_config_connector *connector;
> +
> + if (configfs->card) {
I think it would be better to assign configfs->card in
card_is_registered_store() after successful call to vkms_card_init(). My
reasoning is that the functions in this file care about configfs->card, and not
vkms_drv.c. As a bonus, you can also eliminate the if (configfs) check before
the assignment which is necessary in vkms_card_init() but not
card_is_registered_store().
> + return ERR_PTR(EBUSY);
Nit: Negative errno please (applies elsewhere through the patch as well).
> + }
Nit: remove braces
> +
> + connector = kzalloc(sizeof(struct vkms_config_connector), GFP_KERNEL);
Nit: sizeof(*connector) could be handy here and elsewhere below.
It's a bit awkward to make use of devres elsewhere and not here. I guess the
devres group doesn't exist at this point and perhaps card destruction and
config_group destruction aren't strictly ordered. ¯\_(ツ)_/¯
> if (!connector)
> return ERR_PTR(ENOMEM);
>
> @@ -368,6 +375,9 @@ static struct config_group *crtcs_group_make(struct config_group *group,
> VKMS_MAX_OUTPUT_OBJECTS);
> struct vkms_config_crtc *crtc;
>
> + if (configfs->card)
It really feels like we should have some locking protecting us from races
between adding new components and registration.
> + return ERR_PTR(EBUSY);
> +
> if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
> DRM_ERROR("Unable to allocate another CRTC.");
> return ERR_PTR(ENOMEM);
> @@ -413,6 +423,9 @@ static struct config_group *encoders_group_make(struct config_group *group,
> &configfs->allocated_encoders, VKMS_MAX_OUTPUT_OBJECTS);
> struct vkms_config_encoder *encoder;
>
> + if (configfs->card)
> + return ERR_PTR(EBUSY);
> +
> if (next_idx == VKMS_MAX_OUTPUT_OBJECTS) {
> DRM_ERROR("Unable to allocate another encoder.");
> return ERR_PTR(ENOMEM);
> @@ -454,8 +467,14 @@ static struct config_item_type encoders_group_type = {
> static struct config_group *make_plane_group(struct config_group *group,
> const char *name)
> {
> - struct vkms_config_plane *plane =
> - kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
> + struct vkms_configfs *configfs =
> + container_of(group, struct vkms_configfs, planes_group);
> + struct vkms_config_plane *plane;
> +
> + if (configfs->card)
> + return ERR_PTR(EBUSY);
> +
> + plane = kzalloc(sizeof(struct vkms_config_plane), GFP_KERNEL);
> if (!plane)
> return ERR_PTR(ENOMEM);
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 37b95ca28672..4e00f3b0de7d 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -216,7 +216,7 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
> goto out_release_group;
> }
>
> - ret = drm_vblank_init(&card->drm, 1);
> + ret = drm_vblank_init(&card->drm, vkms_card_crtc_count(card));
I wonder if you could re-order this below vkms_modeset_init() and use
card->output.num_crtcs instead of the new function?
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> goto out_release_group;
> @@ -231,7 +231,7 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
> ret = drm_dev_register(&card->drm, 0);
> if (ret) {
> DRM_ERROR("Unable to register card");
> - return ERR_PTR(ret);
> + goto out_modeset;
Not sure if this is the one I commented on in the previous patch, but at the
risk of duplicate comments, this cleanup feels like it belongs in an earlier patch.
> }
>
> drm_fbdev_generic_setup(&card->drm, 0);
> @@ -245,6 +245,8 @@ struct vkms_card *vkms_card_init(const char *name, struct vkms_configfs *configf
>
> return card;
>
> +out_modeset:
> + vkms_output_clear(card);
> out_release_group:
> devres_release_group(&pdev->dev, grp);
> out_platform_device:
> @@ -264,6 +266,21 @@ void vkms_card_destroy(struct vkms_card *card)
I feel like vkms_card_destroy() should also be calling vkms_output_clear() or it
should have the same cleanup procedure as the error handling code in
vkms_card_init().
> }
> }
>
> +int vkms_card_crtc_count(struct vkms_card *card)
AFAICT this isn't used outside vkms_drv.c so it should be static.
> +{
> + struct list_head *item;
> + int count = 0;
> +
> + if (card->is_default)
> + return 1;
> +
> + BUG_ON(!card->configfs);
> + list_for_each(item, &card->configfs->crtcs_group.cg_children) {
Do you need any locking here?
> + count += 1;
> + }
> + return count;
> +}
> +
> static int __init vkms_init(void)
> {
> struct vkms_card *card;
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index f43e4c563863..2e6bfed890f9 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -222,11 +222,14 @@ struct vkms_card *vkms_card_init(const char *name,
> struct vkms_configfs *configfs);
> void vkms_card_destroy(struct vkms_card *card);
>
> +int vkms_card_crtc_count(struct vkms_card *card);
> +
> /* CRTC */
> struct vkms_crtc *vkms_crtc_init(struct vkms_card *card, struct drm_plane *primary, struct drm_plane *cursor);
>
> int vkms_output_init_default(struct vkms_card *card, int index);
> int vkms_output_init(struct vkms_card *card, int index);
> +void vkms_output_clear(struct vkms_card *card);
>
> struct vkms_plane *vkms_plane_init(struct vkms_card *card,
> enum drm_plane_type type);
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index e343a9c1f311..857cd8593dce 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -4,6 +4,10 @@
> #include <linux/kernel.h>
>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_plane.h>
It makes sense that we would need these, but I'm not sure why it's now and not
earlier in the patchset.
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_simple_kms_helper.h>
>
> @@ -46,7 +50,7 @@ static struct drm_connector *vkms_connector_init(struct vkms_card *card)
>
> connector = &card->output.connectors[card->output.num_connectors++];
> ret = drm_connector_init(&card->drm, connector, &vkms_connector_funcs,
> - DRM_MODE_CONNECTOR_VIRTUAL);
> + DRM_MODE_CONNECTOR_VIRTUAL);
Nit: This indent was introduced in patch 3, if you fix it there it'll drop out of here.
> if (ret) {
> memset(connector, 0, sizeof(*connector));
> card->output.num_connectors -= 1;
> @@ -79,7 +83,6 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_card *card)
> int vkms_output_init_default(struct vkms_card *card, int index)
How feasible would it be to initialize the default device in configfs with the
module config and then call vkms_output_init() to initialize it instead of
duplicating all of this init? As a bonus, we could probably turf some of the
if(default) logic sprinkled around.
> {
> const struct vkms_config *config = &card->vkms_device->config;
> - struct vkms_output *output = &card->output;
> struct drm_device *dev = &card->drm;
> struct drm_connector *connector;
> struct drm_encoder *encoder;
> @@ -99,7 +102,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> vkms_plane_init(card, DRM_PLANE_TYPE_OVERLAY);
> if (IS_ERR(overlay)) {
> ret = PTR_ERR(overlay);
> - goto err_planes;
> + goto cleanup_output;
> }
> }
> }
> @@ -108,7 +111,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> cursor = vkms_plane_init(card, DRM_PLANE_TYPE_CURSOR);
> if (IS_ERR(cursor)) {
> ret = PTR_ERR(cursor);
> - goto err_planes;
> + goto cleanup_output;
> }
> }
>
> @@ -117,7 +120,7 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> if (IS_ERR(vkms_crtc)) {
> DRM_ERROR("Failed to init crtc\n");
> ret = PTR_ERR(vkms_crtc);
> - goto err_planes;
> + goto cleanup_output;
> }
>
> for (int i = 0; i < card->output.num_planes; i++) {
> @@ -129,22 +132,21 @@ int vkms_output_init_default(struct vkms_card *card, int index)
> if (IS_ERR(connector)) {
> DRM_ERROR("Failed to init connector\n");
> ret = PTR_ERR(connector);
> - goto err_connector;
> + goto cleanup_output;
> }
>
> encoder = vkms_encoder_init(card);
> if (IS_ERR(encoder)) {
> DRM_ERROR("Failed to init encoder\n");
> ret = PTR_ERR(encoder);
> - goto err_encoder;
> + goto cleanup_output;
> }
> encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
>
> -
Nit: This blank line was introduced in patch 3, perhaps remove it from there and
it'll drop out here.
> ret = drm_connector_attach_encoder(connector, encoder);
> if (ret) {
> DRM_ERROR("Failed to attach connector to encoder\n");
> - goto err_attach;
> + goto cleanup_output;
> }
>
> if (config->writeback) {
> @@ -157,26 +159,209 @@ int vkms_output_init_default(struct vkms_card *card, int index)
>
> return 0;
>
> -err_attach:
> - drm_encoder_cleanup(encoder);
> +cleanup_output:
> + vkms_output_clear(card);
> + return ret;
> +}
>
> -err_encoder:
> - drm_connector_cleanup(connector);
> +static bool is_object_linked(struct vkms_config_links *links, unsigned long idx)
> +{
> + return links->linked_object_bitmap & (1 << idx);
Nit: You could use some of the bit helpers available (and avoid the implicit
cast):
return FIELD_GET(BIT(idx), links->linked_object_bitmap) != 0;
> +}
>
> -err_connector:
> - drm_crtc_cleanup(&vkms_crtc->base);
> +int vkms_output_init(struct vkms_card *card, int index)
> +{
> + struct drm_device *dev = &card->drm;
> + struct vkms_configfs *configfs = card->configfs;
> + struct vkms_output *output = &card->output;
> + struct plane_map {
> + struct vkms_config_plane *config_plane;
> + struct vkms_plane *plane;
> + } plane_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
> + struct encoder_map {
> + struct vkms_config_encoder *config_encoder;
> + struct drm_encoder *encoder;
> + } encoder_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
> + struct config_item *item;
> + int i, ret;
> +
> + i = 0;
> + list_for_each_entry (item, &configfs->planes_group.cg_children,
> + ci_entry) {
> + struct vkms_config_plane *config_plane =
> + item_to_config_plane(item);
> + struct vkms_plane *plane =
> + vkms_plane_init(card, config_plane->type);
> +
> + if (IS_ERR(plane)) {
> + DRM_ERROR("Unable to init plane from config: %s",
> + item->ci_name);
It'd be nice to print the return value here.
> + ret = PTR_ERR(plane);
> + goto cleanup_output;
> + }
>
> -err_planes:
> - for (int i = 0; i < output->num_planes; i++) {
> - drm_plane_cleanup(&output->planes[i].base);
> + plane_map[i].config_plane = config_plane;
> + plane_map[i].plane = plane;
> + i += 1;
Instead of tracking with i, you could use "output->num_planes - 1"? Same comment
below for different "num_blah"
> + }
> +
> + i = 0;
> + list_for_each_entry (item, &configfs->encoders_group.cg_children,
> + ci_entry) {
> + struct vkms_config_encoder *config_encoder =
> + item_to_config_encoder(item);
> + struct drm_encoder *encoder = vkms_encoder_init(card);
> +
> + if (IS_ERR(encoder)) {
> + DRM_ERROR("Failed to init config encoder: %s",
> + item->ci_name);
> + ret = PTR_ERR(encoder);
> + goto cleanup_output;
> + }
> + encoder_map[i].config_encoder = config_encoder;
> + encoder_map[i].encoder = encoder;
> + i += 1;
> + }
> +
> + list_for_each_entry (item, &configfs->connectors_group.cg_children,
> + ci_entry) {
> + struct vkms_config_connector *config_connector =
> + item_to_config_connector(item);
> + struct drm_connector *connector = vkms_connector_init(card);
> +
> + if (IS_ERR(connector)) {
> + DRM_ERROR("Failed to init connector from config: %s",
> + item->ci_name);
> + ret = PTR_ERR(connector);
> + goto cleanup_output;
> + }
> +
> + for (int j = 0; j < output->num_connectors; j++) {
> + struct encoder_map *encoder = &encoder_map[j];
> +
> + if (is_object_linked(
> + &config_connector->possible_encoders,
> + encoder->config_encoder
> + ->encoder_config_idx)) {
> + drm_connector_attach_encoder(connector,
> + encoder->encoder);
> + }
> + }
> + }
> +
> + list_for_each_entry (item, &configfs->crtcs_group.cg_children,
> + ci_entry) {
> + struct vkms_config_crtc *config_crtc =
> + item_to_config_crtc(item);
> + struct vkms_crtc *vkms_crtc;
> + struct drm_plane *primary = NULL, *cursor = NULL;
> +
> + for (int j = 0; j < output->num_planes; j++) {
> + struct plane_map *plane_entry = &plane_map[j];
> + struct drm_plane *plane = &plane_entry->plane->base;
> +
> + if (!is_object_linked(
> + &plane_entry->config_plane->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + continue;
> + }
Nit: Braces
> +
> + if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
> + if (primary) {
> + DRM_WARN(
> + "Too many primary planes found for crtc %s.",
> + item->ci_name);
Nit: drm_dbg()
> + ret = EINVAL;
> + goto cleanup_output;
> + }
> + primary = plane;
> + } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> + if (cursor) {
> + DRM_WARN(
> + "Too many cursor planes found for crtc %s.",
> + item->ci_name);
> + ret = EINVAL;
> + goto cleanup_output;
> + }
> + cursor = plane;
> + }
> + }
> +
> + if (!primary) {
> + DRM_WARN("No primary plane configured for crtc %s", item->ci_name);
> + ret = EINVAL;
> + goto cleanup_output;
> + }
> +
> + // TODO add names to helper
Mind expanding on this?
> + vkms_crtc = vkms_crtc_init(card, primary, cursor);
> + if (IS_ERR(vkms_crtc)) {
> + DRM_WARN("Unable to init crtc from config: %s",
> + item->ci_name);
> + ret = PTR_ERR(vkms_crtc);
> + goto cleanup_output;
> + }
> +
> + for (int j = 0; j < output->num_planes; j++) {
> + struct plane_map *plane_entry = &plane_map[j];
> +
> + if (!plane_entry->plane)
> + break;
> +
> + if (is_object_linked(
> + &plane_entry->config_plane->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + plane_entry->plane->base.possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> + }
> +
> + for (int j = 0; j < output->num_encoders; j++) {
> + struct encoder_map *encoder_entry = &encoder_map[j];
> +
> + if (is_object_linked(&encoder_entry->config_encoder
> + ->possible_crtcs,
> + config_crtc->crtc_config_idx)) {
> + encoder_entry->encoder->possible_crtcs |=
> + drm_crtc_mask(&vkms_crtc->base);
> + }
> + }
> +
> + if (card->vkms_device->config.writeback) {
> + ret = vkms_enable_writeback_connector(card, vkms_crtc);
> + if (ret)
> + DRM_WARN(
> + "Failed to init writeback connector for config crtc: %s",
> + item->ci_name);
Nit: drm_dbg and please print the error message.
> + }
> }
>
> - memset(output, 0, sizeof(struct vkms_output));
> + drm_mode_config_reset(dev);
> +
> + return 0;
>
> +cleanup_output:
> + vkms_output_clear(card);
> + resume_device_irqs(); // REMOVE
REMOVE?
> return ret;
> }
>
> -int vkms_output_init(struct vkms_card *card, int index)
> +void vkms_output_clear(struct vkms_card *card)
> {
> - return -ENOTSUPP;
> + struct vkms_output *output = &card->output;
> +
> + for (int i = 0; i < output->num_crtcs; i++) {
> + drm_crtc_cleanup(&output->crtcs[i].base);
> + }
> + for (int i = 0; i < output->num_encoders; i++) {
> + drm_encoder_cleanup(&output->encoders[i]);
> + }
> + for (int i = 0; i < output->num_connectors; i++) {
> + drm_connector_cleanup(&output->connectors[i]);
> + }
> + for (int i = 0; i < output->num_planes; i++) {
> + drm_plane_cleanup(&output->planes[i].base);
> + }
Nit: braces for all of these
> +
> + memset(output, 0, sizeof(*output));
Same concerns about memset as before.
> }
> --
> 2.37.1.359.gd136c6c3e2-goog
>
--
Sean Paul, Software Engineer, Google / Chromium OS
^ permalink raw reply [flat|nested] 12+ messages in thread