| Message ID | 20220506001822.890772-1-jshargo@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/vkms: ConfigFS support | expand |
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 >
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
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