[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 at 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