Hi!
I wanted to send this patch out early to get some feedback on the layout of the code and new ConfigFS directory. I intend to follow this up with a more complete patch set that uses this to, for instance, add more connectors and toggle feature support.
A few questions I had as someone new to kernel dev:
1. Would you prefer laying out all the objects right now or add them as-needed? IMO it’s nice to lay things out now to make future work that much easier.
2. Is the layout of /config/vkms/<type>s/<id>/<attributes> OK? If VKMS would eventually want to support installing multiple devices, we could do something like /config/vkms/card<N>/<type>s/<id>/<attributes>.
3. Should I split out the documention into a separate change?
4. Any other style / design thoughts?
Thanks! I am excited to be reaching out and contributing.
Jim Shargo (1): drm/vkms: Add basic support for ConfigFS to VKMS
Documentation/gpu/vkms.rst | 23 +++++ drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 129 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 10 +++ drivers/gpu/drm/vkms/vkms_drv.h | 25 ++++++ drivers/gpu/drm/vkms/vkms_output.c | 2 + drivers/gpu/drm/vkms/vkms_plane.c | 2 + 8 files changed, 193 insertions(+) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
This change adds boilerplate setup for ConfigFS, resulting in a config directory that looks like this (assuming it's mounted under /config/):
/config `-- vkms |-- connectors | `-- 52 |-- crtcs | `-- 51 |-- encoders | `-- 53 `-- planes |-- 31 |-- 33 |-- 35 |-- 37 |-- 39 |-- 41 |-- 43 |-- 45 |-- 47 `-- 49
Notes on the ConfigFS setup:
- Each `vkms/<type>s/<id>/` is a config group/item based directory that can have readable and writable attributes set within them. - Each `vkms/<type>s` directory can have ConfigFS groupops set on them to respond to mkdir calls, allowing for the creation of new objects within vkms (such as hotplugging a new connector). --- Documentation/gpu/vkms.rst | 23 +++++ drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 129 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 10 +++ drivers/gpu/drm/vkms/vkms_drv.h | 25 ++++++ drivers/gpu/drm/vkms/vkms_output.c | 2 + drivers/gpu/drm/vkms/vkms_plane.c | 2 + 8 files changed, 193 insertions(+) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst index 9c873c3912cc..13233fc87b53 100644 --- a/Documentation/gpu/vkms.rst +++ b/Documentation/gpu/vkms.rst @@ -51,6 +51,29 @@ 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 will create a directory tree that looks something like this:: + + - /config/vkms/ + - connectors/ -- a list of all the planes available in the VKMS driver + - N/ -- the connector with ID=N + - crtc/ -- a list of all the crtcs available in the VKMS driver + - N/ -- the crtc with ID=N + - encoders/ -- a list of all the encoders available in the VKMS driver + - N/ -- the encoder with ID=N + - planes/ -- a list of all the planes available in the VKMS driver + - N/ -- the plane with ID=N. + +Settings for each object will appear in the ``/config/vkms/<type>/<N>/`` 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..edaa041a830c --- /dev/null +++ b/drivers/gpu/drm/vkms/vkms_configfs.c @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: GPL-2.0+ + +#include <linux/configfs.h> +#include <linux/export.h> +#include <linux/mutex.h> + +#include "vkms_drv.h" + +static void setup_configfs_object(uint32_t id, struct config_group *object, + struct config_group *parent, + struct config_item_type *type) +{ + char name[CONFIGFS_ITEM_NAME_LEN]; + + snprintf(name, CONFIGFS_ITEM_NAME_LEN, "%d", id); + config_group_init_type_name(object, name, type); + configfs_add_default_group(object, parent); +} + +/* connector item, e.g. /config/vkms/connectors/ID */ + +static struct config_item_type connector_type = { + .ct_owner = THIS_MODULE, +}; + +/* crtc item, e.g. /config/vkms/crtc/ID */ + +static struct config_item_type crtc_type = { + .ct_owner = THIS_MODULE, +}; + +/* encoder item, e.g. /config/vkms/encoder/ID */ + +static struct config_item_type encoder_type = { + .ct_owner = THIS_MODULE, +}; + +void vkms_init_output_configfs(struct vkms_device *vkmsdev, + struct vkms_output *output) +{ + setup_configfs_object(output->connector.base.id, + &output->connector_config_group, + &vkmsdev->configfs.connectors_group, + &connector_type); + + setup_configfs_object(output->crtc.base.id, &output->crtc_config_group, + &vkmsdev->configfs.crtcs_group, &crtc_type); + + setup_configfs_object(output->encoder.base.id, + &output->encoder_config_group, + &vkmsdev->configfs.encoders_group, &encoder_type); +} + +/* Plane item, e.g. /config/vkms/planes/ID */ + +static struct config_item_type plane_type = { + .ct_owner = THIS_MODULE, +}; + +void vkms_init_plane_configfs(struct vkms_device *vkmsdev, + struct vkms_plane *plane) +{ + setup_configfs_object(plane->base.base.id, &plane->config_group, + &vkmsdev->configfs.planes_group, &plane_type); +} + +/* Directory groups, e.g. /config/vkms/planes */ + +static struct config_item_type connectors_group_type = { + .ct_owner = THIS_MODULE, +}; + +static struct config_item_type crtcs_group_type = { + .ct_owner = THIS_MODULE, +}; + +static struct config_item_type encoders_group_type = { + .ct_owner = THIS_MODULE, +}; + +static struct config_item_type planes_group_type = { + .ct_owner = THIS_MODULE, +}; + +/* Root directory group, e.g. /config/vkms/ */ + +static struct config_item_type vkms_type = { + .ct_owner = THIS_MODULE, +}; + +static struct configfs_subsystem vkms_subsys = { + .su_group = { + .cg_item = { + .ci_name = "vkms", + .ci_type = &vkms_type, + }, + }, +}; + +void vkms_init_configfs(struct vkms_device *vkmsdev) +{ + config_group_init(&vkms_subsys.su_group); + mutex_init(&vkms_subsys.su_mutex); + + config_group_init_type_name(&vkmsdev->configfs.connectors_group, + "connectors", &connectors_group_type); + configfs_add_default_group(&vkmsdev->configfs.connectors_group, + &vkms_subsys.su_group); + + config_group_init_type_name(&vkmsdev->configfs.crtcs_group, "crtcs", + &crtcs_group_type); + configfs_add_default_group(&vkmsdev->configfs.crtcs_group, + &vkms_subsys.su_group); + + config_group_init_type_name(&vkmsdev->configfs.encoders_group, + "encoders", &encoders_group_type); + configfs_add_default_group(&vkmsdev->configfs.encoders_group, + &vkms_subsys.su_group); + + config_group_init_type_name(&vkmsdev->configfs.planes_group, "planes", + &planes_group_type); + configfs_add_default_group(&vkmsdev->configfs.planes_group, + &vkms_subsys.su_group); +} + +int vkms_register_configfs(void) +{ + return configfs_register_subsystem(&vkms_subsys); +} diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index 0ffe5f0e33f7..92ca8cf2d104 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -9,6 +9,7 @@ * the GPU in DRM API tests. */
+#include <linux/configfs.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/dma-mapping.h> @@ -191,6 +192,8 @@ static int vkms_create(struct vkms_config *config) goto out_devres; }
+ vkms_init_configfs(vkms_device); + ret = drm_vblank_init(&vkms_device->drm, 1); if (ret) { DRM_ERROR("Failed to vblank\n"); @@ -207,8 +210,15 @@ static int vkms_create(struct vkms_config *config)
drm_fbdev_generic_setup(&vkms_device->drm, 0);
+ ret = vkms_register_configfs(); + if (ret) + goto out_drmres; + + return 0;
+out_drmres: + drm_dev_unregister(&vkms_device->drm); out_devres: devres_release_group(&pdev->dev, NULL); out_unregister: diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 91e63b12f60f..873b91f63309 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 <drm/drm.h> @@ -48,6 +49,7 @@ struct vkms_plane_state {
struct vkms_plane { struct drm_plane base; + struct config_group config_group; };
/** @@ -86,6 +88,10 @@ struct vkms_output { /* protects concurrent access to composer */ spinlock_t lock;
+ struct config_group crtc_config_group; + struct config_group encoder_config_group; + struct config_group connector_config_group; + /* protected by @lock */ bool composer_enabled; struct vkms_crtc_state *composer_state; @@ -103,10 +109,22 @@ struct vkms_config { struct vkms_device *dev; };
+struct vkms_configfs { + /* Directory group containing connector configs, e.g. /config/vkms/connectors/ */ + struct config_group connectors_group; + /* Directory group containing CRTC configs, e.g. /config/vkms/crtcs/ */ + struct config_group crtcs_group; + /* Directory group containing encoder configs, e.g. /config/vkms/encoders/ */ + struct config_group encoders_group; + /* Directory group containing plane configs, e.g. /config/vkms/planes/ */ + struct config_group planes_group; +}; + struct vkms_device { struct drm_device drm; struct platform_device *platform; struct vkms_output output; + struct vkms_configfs configfs; const struct vkms_config *config; };
@@ -145,4 +163,11 @@ void vkms_set_composer(struct vkms_output *out, bool enabled); /* Writeback */ int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
+/* ConfigFS Support */ +void vkms_init_configfs(struct vkms_device *vkmsdev); +int vkms_register_configfs(void); + +void vkms_init_plane_configfs(struct vkms_device *vkmsdev, struct vkms_plane *plane); +void vkms_init_output_configfs(struct vkms_device *vkmsdev, struct vkms_output *output); + #endif /* _VKMS_DRV_H_ */ diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c index ba0e82ae549a..fa80d27118c8 100644 --- a/drivers/gpu/drm/vkms/vkms_output.c +++ b/drivers/gpu/drm/vkms/vkms_output.c @@ -111,6 +111,8 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
drm_mode_config_reset(dev);
+ vkms_init_output_configfs(vkmsdev, output); + return 0;
err_attach: diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c index d8eb674b49a6..4b93cfa1bb19 100644 --- a/drivers/gpu/drm/vkms/vkms_plane.c +++ b/drivers/gpu/drm/vkms/vkms_plane.c @@ -195,5 +195,7 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
drm_plane_helper_add(&plane->base, funcs);
+ vkms_init_plane_configfs(vkmsdev, plane); + return plane; }
On Fri, May 06, 2022 at 12:18:20AM +0000, Jim Shargo wrote:
Hi!
I wanted to send this patch out early to get some feedback on the layout of the code and new ConfigFS directory. I intend to follow this up with a more complete patch set that uses this to, for instance, add more connectors and toggle feature support.
A few questions I had as someone new to kernel dev:
- Would you prefer laying out all the objects right now or add them
as-needed? IMO it’s nice to lay things out now to make future work that much easier.
- Is the layout of /config/vkms/<type>s/<id>/<attributes> OK? If VKMS
would eventually want to support installing multiple devices, we could do something like /config/vkms/card<N>/<type>s/<id>/<attributes>.
Should I split out the documention into a separate change?
Any other style / design thoughts?
Thanks! I am excited to be reaching out and contributing.
So the overall idea here is that a lot of these things cannot be changed once the vkms_device instance is created, due to how drm works. This is why struct vmks_config has been extracted. The rough flow would be:
1. you create a new directory in the vkms configfs directory when creating a new instance. This then gets populated with all the properties from vkms_config
2. user sets these properts through configfs
3. There is a special property called "registered" or similar, which starts out as 0/false. When set to 1/true the vkms_device will be registered with the vkms_config that's been prepared. After that point further changes to vkms_config are not allowed anymore at all (this might change later on for connector hotplug, which really is the only thing a drm_device can change at runtime).
4. removal of the vkms_device could perhaps be done simply be deleting the entire directory? I think that would be a nice clean interface.
So in other words, basing the configfs interface on drm objects doesn't work, because once the drm objects exist you cannot change the configuration anymore.
Cheers, Daniel
Jim Shargo (1): drm/vkms: Add basic support for ConfigFS to VKMS
Documentation/gpu/vkms.rst | 23 +++++ drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 129 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 10 +++ drivers/gpu/drm/vkms/vkms_drv.h | 25 ++++++ drivers/gpu/drm/vkms/vkms_output.c | 2 + drivers/gpu/drm/vkms/vkms_plane.c | 2 + 8 files changed, 193 insertions(+) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
-- 2.36.0.512.ge40c2bad7a-goog
Thanks for taking a look! Sorry for the delay in response, I've been moving house and prototyping locally.
On Mon, May 9, 2022 at 11:10 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 06, 2022 at 12:18:20AM +0000, Jim Shargo wrote:
Hi!
I wanted to send this patch out early to get some feedback on the layout of the code and new ConfigFS directory. I intend to follow this up with a more complete patch set that uses this to, for instance, add more connectors and toggle feature support.
A few questions I had as someone new to kernel dev:
- Would you prefer laying out all the objects right now or add them
as-needed? IMO it’s nice to lay things out now to make future work that much easier.
- Is the layout of /config/vkms/<type>s/<id>/<attributes> OK? If VKMS
would eventually want to support installing multiple devices, we could do something like /config/vkms/card<N>/<type>s/<id>/<attributes>.
Should I split out the documention into a separate change?
Any other style / design thoughts?
Thanks! I am excited to be reaching out and contributing.
So the overall idea here is that a lot of these things cannot be changed once the vkms_device instance is created, due to how drm works. This is why struct vmks_config has been extracted. The rough flow would be:
- you create a new directory in the vkms configfs directory when creating
a new instance. This then gets populated with all the properties from vkms_config
user sets these properts through configfs
There is a special property called "registered" or similar, which
starts out as 0/false. When set to 1/true the vkms_device will be registered with the vkms_config that's been prepared. After that point further changes to vkms_config are not allowed anymore at all (this might change later on for connector hotplug, which really is the only thing a drm_device can change at runtime).
I think this allows for a slightly easier initialization, too, where we don't keep a half-built drm device around or need to manage ids in any special way.
I'll get things working and send out a new patch set.
I'm also thinking that, to make life easier, we'll create the regular default device during init and register it automatically, so unless someone starts actively adding virtual devices things behavior remains the same.
- removal of the vkms_device could perhaps be done simply be deleting the
entire directory? I think that would be a nice clean interface.
Yep! I just got that wired up locally.
So in other words, basing the configfs interface on drm objects doesn't work, because once the drm objects exist you cannot change the configuration anymore.
I wasn't 100% sure how much trouble we'd get into if we tried to set DRM device properties at run time, but with this confirmation I think that this staging/registration approach is the best.
Cheers, Daniel
Jim Shargo (1): drm/vkms: Add basic support for ConfigFS to VKMS
Documentation/gpu/vkms.rst | 23 +++++ drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 129 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 10 +++ drivers/gpu/drm/vkms/vkms_drv.h | 25 ++++++ drivers/gpu/drm/vkms/vkms_output.c | 2 + drivers/gpu/drm/vkms/vkms_plane.c | 2 + 8 files changed, 193 insertions(+) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
-- 2.36.0.512.ge40c2bad7a-goog
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, Jun 06, 2022 at 05:59:37PM -0400, Jim Shargo wrote:
Thanks for taking a look! Sorry for the delay in response, I've been moving house and prototyping locally.
On Mon, May 9, 2022 at 11:10 AM Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 06, 2022 at 12:18:20AM +0000, Jim Shargo wrote:
Hi!
I wanted to send this patch out early to get some feedback on the layout of the code and new ConfigFS directory. I intend to follow this up with a more complete patch set that uses this to, for instance, add more connectors and toggle feature support.
A few questions I had as someone new to kernel dev:
- Would you prefer laying out all the objects right now or add them
as-needed? IMO it’s nice to lay things out now to make future work that much easier.
- Is the layout of /config/vkms/<type>s/<id>/<attributes> OK? If VKMS
would eventually want to support installing multiple devices, we could do something like /config/vkms/card<N>/<type>s/<id>/<attributes>.
Should I split out the documention into a separate change?
Any other style / design thoughts?
Thanks! I am excited to be reaching out and contributing.
So the overall idea here is that a lot of these things cannot be changed once the vkms_device instance is created, due to how drm works. This is why struct vmks_config has been extracted. The rough flow would be:
- you create a new directory in the vkms configfs directory when creating
a new instance. This then gets populated with all the properties from vkms_config
user sets these properts through configfs
There is a special property called "registered" or similar, which
starts out as 0/false. When set to 1/true the vkms_device will be registered with the vkms_config that's been prepared. After that point further changes to vkms_config are not allowed anymore at all (this might change later on for connector hotplug, which really is the only thing a drm_device can change at runtime).
I think this allows for a slightly easier initialization, too, where we don't keep a half-built drm device around or need to manage ids in any special way.
I'll get things working and send out a new patch set.
I'm also thinking that, to make life easier, we'll create the regular default device during init and register it automatically, so unless someone starts actively adding virtual devices things behavior remains the same.
Yup, I think keeping the regular default device is a good idea.
Also I think initializing a new instance's vkms_config with the module options we have would probably also make sense. Of course for new complex features (like special plane features or attaching ebpf to implement atomic_check restrictions) are best done only through configfs, so that we can slowly deprecate the module options.
But for the handful of existing knobs I think it's fine to just keep it all as-is.
- removal of the vkms_device could perhaps be done simply be deleting the
entire directory? I think that would be a nice clean interface.
Yep! I just got that wired up locally.
So in other words, basing the configfs interface on drm objects doesn't work, because once the drm objects exist you cannot change the configuration anymore.
I wasn't 100% sure how much trouble we'd get into if we tried to set DRM device properties at run time, but with this confirmation I think that this staging/registration approach is the best.
Looking forward to your next round!
Cheers, Daniel
Cheers, Daniel
Jim Shargo (1): drm/vkms: Add basic support for ConfigFS to VKMS
Documentation/gpu/vkms.rst | 23 +++++ drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/vkms/Makefile | 1 + drivers/gpu/drm/vkms/vkms_configfs.c | 129 +++++++++++++++++++++++++++ drivers/gpu/drm/vkms/vkms_drv.c | 10 +++ drivers/gpu/drm/vkms/vkms_drv.h | 25 ++++++ drivers/gpu/drm/vkms/vkms_output.c | 2 + drivers/gpu/drm/vkms/vkms_plane.c | 2 + 8 files changed, 193 insertions(+) create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
-- 2.36.0.512.ge40c2bad7a-goog
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jim Shargo -
Jim Shargo