[RFC PATCH 0/1] drm/vkms: ConfigFS support

[RFC PATCH 0/1] drm/vkms: ConfigFS support

[Jim Shargo]

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

-- 
2.36.0.512.ge40c2bad7a-goog

[RFC PATCH 1/1] drm/vkms: Add basic support for ConfigFS to VKMS

[Jim Shargo]

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;
 }
-- 
2.36.0.512.ge40c2bad7a-goog

Re: [RFC PATCH 0/1] drm/vkms: ConfigFS support

[Daniel Vetter]

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:
> 
> 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.

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH 0/1] drm/vkms: ConfigFS support

[Jim Shargo]

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:
> >
> > 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.
>
> 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).

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.

>
> 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.

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

Re: [RFC PATCH 0/1] drm/vkms: ConfigFS support

[Daniel Vetter]

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:
> > >
> > > 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.
> >
> > 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).
> 
> 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.

> > 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.
> 
> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC PATCH 1/1] drm/vkms: Add basic support for ConfigFS to VKMS

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
BCC: lkp(a)intel.com
In-Reply-To: <20220506001822.890772-2-jshargo@chromium.org>
References: <20220506001822.890772-2-jshargo@chromium.org>
TO: Jim Shargo <jshargo@chromium.org>

Hi Jim,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on drm/drm-next]
[also build test WARNING on v5.18-rc5 next-20220506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Jim-Shargo/drm-vkms-ConfigFS-support/20220506-182056
base:   git://anongit.freedesktop.org/drm/drm drm-next
:::::: branch date: 17 hours ago
:::::: commit date: 17 hours ago
compiler: mipsel-linux-gcc (GCC) 11.3.0
reproduce (cppcheck warning):
        # apt-get install cppcheck
        git checkout e32fa3dd59fa0c63d6c7b007bddbc78a53cb40bc
        cppcheck --quiet --enable=style,performance,portability --template=gcc FILE

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/vkms/vkms_configfs.c:15:2: warning: %d in format string (no. 1) requires 'int' but the argument type is 'unsigned int'. [invalidPrintfArgType_sint]
    snprintf(name, CONFIGFS_ITEM_NAME_LEN, "%d", id);
    ^

vim +15 drivers/gpu/drm/vkms/vkms_configfs.c

e32fa3dd59fa0c Jim Shargo 2022-05-06   8  
e32fa3dd59fa0c Jim Shargo 2022-05-06   9  static void setup_configfs_object(uint32_t id, struct config_group *object,
e32fa3dd59fa0c Jim Shargo 2022-05-06  10  				  struct config_group *parent,
e32fa3dd59fa0c Jim Shargo 2022-05-06  11  				  struct config_item_type *type)
e32fa3dd59fa0c Jim Shargo 2022-05-06  12  {
e32fa3dd59fa0c Jim Shargo 2022-05-06  13  	char name[CONFIGFS_ITEM_NAME_LEN];
e32fa3dd59fa0c Jim Shargo 2022-05-06  14  
e32fa3dd59fa0c Jim Shargo 2022-05-06 @15  	snprintf(name, CONFIGFS_ITEM_NAME_LEN, "%d", id);
e32fa3dd59fa0c Jim Shargo 2022-05-06  16  	config_group_init_type_name(object, name, type);
e32fa3dd59fa0c Jim Shargo 2022-05-06  17  	configfs_add_default_group(object, parent);
e32fa3dd59fa0c Jim Shargo 2022-05-06  18  }
e32fa3dd59fa0c Jim Shargo 2022-05-06  19  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp