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