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@xxxxxxxx> 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
