On Tue, Aug 29, 2023 at 05:30:52AM +0000, Brandon Pollack wrote:
> Since Jim is busy with other work and I'm working on some things that
> rely on this, I've taken up the task of doing the iterations. I've
> addressed the comments as best I can (those replies are to each
> individual change) and here is the patch set to go with those.
>
> I added my own signoff to each commit, but I've left jshargo@ as the
> author of all the commits he wrote. I'm sure there is still more to
> address and the ICT tests that were writtein parallel to this may also
> need some additions, but I'm hoping we're in a good enough state to get
> this in and iterate from there soon.
>
> Since V6:
> ========
> rmdirs for documentation examples
> fix crtc mask for writebacks
>
> Since V5:
> ========
> Fixed some bad merge conflicts and locking behaviours as well as
> clarified some documentation, should be good to go now :)
>
> Since V4:
> ========
> Fixed up some documentation as suggested by Marius
> Fixed up some bad locking as suggested by Marius
> Small fixes here and there (most have email responses to previous chain
> emails)
>
> Since V3:
> ========
> I've added hotplug support in the latest patch. This has been reviewed some
> and the notes from that review are addressed here as well.
>
> Relevant/Utilizing work:
> =======================
> I've built a while test framework based on this as proof it functions (though
> I'm sure there may be lingering bugs!). You can check that out on
> crrev.com if you are interested and need to get started yourself (but be
> aware of any licensing that may differ from the kernel itself! Make
> sure you understand the license:
>
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/tast-tests/LICENSE
>
> That said, you can see the changes in review on the crrev gerrit:
>
> https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/4666669
>
> Outro:
> =====
> I really appreciate everyone's input and tolerance in getting these
> changes in. Jim's first patch series was this, and other than some
> small cleanups and documentation, taking over it is also mine.
Sorry for not having looked at this earlier. I think overall it's looking
good, mostly just a bunch of comments on lifetime/locking questions.
I'm also wondering a bit how much we want to go overboard with igt tests,
since the lifetime fun is quite big here. I think at least some basic
tests that trying to do nasty things like unbind the driver in sysfs and
then try to use configfs, or keeping the vkms_device alive with an open fd
and removing the configfs directory would be really good.
One thing that's a bit tricky is that configfs is considered uapi, so must
be stable forever. And I think that's actually the right thing for us,
since we want compositors and other projects to use this for their
testing. So unlike igt tests using special debugfs interfaces, which are
ok to be very tightly coupled to kernel releases
Cheers, Sima
>
> Thank you everyone :)
>
> Brandon Pollack (1):
> drm/vkms Add hotplug support via configfs to VKMS.
>
> Jim Shargo (6):
> drm/vkms: Back VKMS with DRM memory management instead of static
> objects
> drm/vkms: Support multiple DRM objects (crtcs, etc.) per VKMS device
> drm/vkms: Provide platform data when creating VKMS devices
> drm/vkms: Add ConfigFS scaffolding to VKMS
> drm/vkms: Support enabling ConfigFS devices
> drm/vkms: Add a module param to enable/disable the default device
>
> Documentation/gpu/vkms.rst | 20 +-
> drivers/gpu/drm/Kconfig | 1 +
> drivers/gpu/drm/vkms/Makefile | 1 +
> drivers/gpu/drm/vkms/vkms_composer.c | 30 +-
> drivers/gpu/drm/vkms/vkms_configfs.c | 723 ++++++++++++++++++++++++++
> drivers/gpu/drm/vkms/vkms_crtc.c | 102 ++--
> drivers/gpu/drm/vkms/vkms_drv.c | 206 +++++---
> drivers/gpu/drm/vkms/vkms_drv.h | 182 +++++--
> drivers/gpu/drm/vkms/vkms_output.c | 404 ++++++++++++--
> drivers/gpu/drm/vkms/vkms_plane.c | 44 +-
> drivers/gpu/drm/vkms/vkms_writeback.c | 42 +-
> 11 files changed, 1514 insertions(+), 241 deletions(-)
> create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
>
> --
> 2.42.0.rc2.253.gd59a3bf2b4-goog
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi everyone,
I wasn't aware of these patches, but I'm really glad they are getting
some attention, thanks a lot for your review Sima.
Given that it's been a while since the patches were emailed, I'm not
sure if the original authors of the patches could implement your
comments. If not, I can work on it. Please let me know.
I'm working on a Mutter feature that'd greatly benefit from this uapi
and I'm sure other compositors would find it useful.
I'll start working on a new version in a few days if nobody else is
already working on it.
Best wishes,
José Expósito
Sima--thanks SO MUCH for going through with everything leaving a
detailed review. I am excited to go through your feedback.
It makes me extremely happy to see these patches get people excited.
They've bounced between a few people, and I recently asked to take
them over again from the folks who were most recently looking at them
but haven't since had capacity to revisit them. I'd love to contribute
more but I am currently pretty swamped and I probably couldn't
realistically make too much headway before the middle of June.
José--if you've got capacity and interest, I'd love to see this work
get in! Thanks!! Please let me know your timeline and if you want to
split anything up or have any questions, I'd love to help if possible.
But most important to me is seeing the community benefit from the
feature.
And (in case it got lost in the shuffle of all these patches) the IGT
tests really make it much easier to develop this thing. Marius has
posted the most recent patches:
https://lore.kernel.org/igt-dev/?q=configfs
Thanks!
-- Jim
On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
>
> Hi everyone,
>
> I wasn't aware of these patches, but I'm really glad they are getting
> some attention, thanks a lot for your review Sima.
>
> Given that it's been a while since the patches were emailed, I'm not
> sure if the original authors of the patches could implement your
> comments. If not, I can work on it. Please let me know.
>
> I'm working on a Mutter feature that'd greatly benefit from this uapi
> and I'm sure other compositors would find it useful.
>
> I'll start working on a new version in a few days if nobody else is
> already working on it.
>
> Best wishes,
> José Expósito
Le 09/05/24 - 18:18, Jim Shargo a écrit :
> Sima--thanks SO MUCH for going through with everything leaving a
> detailed review. I am excited to go through your feedback.
>
> It makes me extremely happy to see these patches get people excited.
>
> They've bounced between a few people, and I recently asked to take
> them over again from the folks who were most recently looking at them
> but haven't since had capacity to revisit them. I'd love to contribute
> more but I am currently pretty swamped and I probably couldn't
> realistically make too much headway before the middle of June.
>
> José--if you've got capacity and interest, I'd love to see this work
> get in! Thanks!! Please let me know your timeline and if you want to
> split anything up or have any questions, I'd love to help if possible.
> But most important to me is seeing the community benefit from the
> feature.
>
> And (in case it got lost in the shuffle of all these patches) the IGT
> tests really make it much easier to develop this thing. Marius has
> posted the most recent patches:
> https://lore.kernel.org/igt-dev/?q=configfs
>
> Thanks!
> -- Jim
>
>
>
> On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
> >
> > Hi everyone,
> >
> > I wasn't aware of these patches, but I'm really glad they are getting
> > some attention, thanks a lot for your review Sima.
> >
> > Given that it's been a while since the patches were emailed, I'm not
> > sure if the original authors of the patches could implement your
> > comments. If not, I can work on it. Please let me know.
> >
> > I'm working on a Mutter feature that'd greatly benefit from this uapi
> > and I'm sure other compositors would find it useful.
> >
> > I'll start working on a new version in a few days if nobody else is
> > already working on it.
> >
> > Best wishes,
> > José Expósito
Hi all!
Very nice to see other people working on this subject. As the series
seemed inactive, I started two weeks ago to rebase it on top of [1]. I
also started some work to use drmm_* helpers instead of using lists in
vkms. I currently struggle with a deadlock during rmmod.
I need to clean my commits, but I can share a WIP version.
Maybe we can discuss a bit the comment from Daniel (split init between
default/configfs, use or not a real platform device...)
For the split, I think the first solution (struct vkms_config) can be
easier to understand and to implement, for two reasons:
- No need to distinguish between the "default" and the "configfs" devices
in the VKMS "core". All is managed with only one struct vkms_config.
- Most of the lifetime issue should be gone. The only thing to
synchronize is passing this vkms_config from ConfigFS to VKMS.
The drawback of this is that it can become difficult to do the "runtime"
configuration (today only hotplug, but I plan to add more complex stuff
like DP emulation, EDID selection, MST support...). Those configuration
must be done "at runtime" and will require a strong synchronization with
the vkms "core".
Maybe we can distinguish between the "creation" and the "runtime
configuration", in two different configFS directory? Once a device is
created, it is moved to the "enabled" directory and will have a different
set of attribute (connection status, current EDID...)
For the platform driver part, it seems logic to me to use a "real"
platform driver and a platform device for each pipeline, but I don't have
the experience to tell if this is a good idea or not.
[1]: https://lore.kernel.org/dri-devel/[email protected]/
Thanks,
Louis Chauvet
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Fri, May 10, 2024 at 06:19:45PM +0200, Louis Chauvet wrote:
> Le 09/05/24 - 18:18, Jim Shargo a écrit :
> > Sima--thanks SO MUCH for going through with everything leaving a
> > detailed review. I am excited to go through your feedback.
> >
> > It makes me extremely happy to see these patches get people excited.
> >
> > They've bounced between a few people, and I recently asked to take
> > them over again from the folks who were most recently looking at them
> > but haven't since had capacity to revisit them. I'd love to contribute
> > more but I am currently pretty swamped and I probably couldn't
> > realistically make too much headway before the middle of June.
> >
> > José--if you've got capacity and interest, I'd love to see this work
> > get in! Thanks!! Please let me know your timeline and if you want to
> > split anything up or have any questions, I'd love to help if possible.
> > But most important to me is seeing the community benefit from the
> > feature.
> >
> > And (in case it got lost in the shuffle of all these patches) the IGT
> > tests really make it much easier to develop this thing. Marius has
> > posted the most recent patches:
> > https://lore.kernel.org/igt-dev/?q=configfs
> >
> > Thanks!
> > -- Jim
> >
> >
> >
> > On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
> > >
> > > Hi everyone,
> > >
> > > I wasn't aware of these patches, but I'm really glad they are getting
> > > some attention, thanks a lot for your review Sima.
> > >
> > > Given that it's been a while since the patches were emailed, I'm not
> > > sure if the original authors of the patches could implement your
> > > comments. If not, I can work on it. Please let me know.
> > >
> > > I'm working on a Mutter feature that'd greatly benefit from this uapi
> > > and I'm sure other compositors would find it useful.
> > >
> > > I'll start working on a new version in a few days if nobody else is
> > > already working on it.
> > >
> > > Best wishes,
> > > José Expósito
>
> Hi all!
>
> Very nice to see other people working on this subject. As the series
> seemed inactive, I started two weeks ago to rebase it on top of [1]. I
> also started some work to use drmm_* helpers instead of using lists in
> vkms. I currently struggle with a deadlock during rmmod.
>
> I need to clean my commits, but I can share a WIP version.
Hi Louis,
If you could share a RFC/WIP series it would be awesome!
Since you are already working on the kernel patches (and I guess IGT?),
I'll start working on a libdrm high level API to interact with VKMS from
user-space on top of your patches. I'll share a link as soon as I have a
draft PR.
> Maybe we can discuss a bit the comment from Daniel (split init between
> default/configfs, use or not a real platform device...)
>
> For the split, I think the first solution (struct vkms_config) can be
> easier to understand and to implement, for two reasons:
> - No need to distinguish between the "default" and the "configfs" devices
> in the VKMS "core". All is managed with only one struct vkms_config.
> - Most of the lifetime issue should be gone. The only thing to
> synchronize is passing this vkms_config from ConfigFS to VKMS.
I agree, this seems like the easiest solution.
> The drawback of this is that it can become difficult to do the "runtime"
> configuration (today only hotplug, but I plan to add more complex stuff
> like DP emulation, EDID selection, MST support...). Those configuration
> must be done "at runtime" and will require a strong synchronization with
> the vkms "core".
>
> Maybe we can distinguish between the "creation" and the "runtime
> configuration", in two different configFS directory? Once a device is
> created, it is moved to the "enabled" directory and will have a different
> set of attribute (connection status, current EDID...)
Once the device is enabled (i.e, `echo 1 > /config/vkms/my-device/enabled`),
would it make sense to use sysfs instead of another configfs directory?
The advantage is that with sysfs the kernel controls the lifetime of the
objects and I think it *might* simplify the code, but I'll need to write a
proof of concept to see if this works.
> For the platform driver part, it seems logic to me to use a "real"
> platform driver and a platform device for each pipeline, but I don't have
> the experience to tell if this is a good idea or not.
I'm afraid I don't know which approach could work better. Trusting Sima and
Maíra on this one.
Jose
> [1]: https://lore.kernel.org/dri-devel/[email protected]/
>
> Thanks,
> Louis Chauvet
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Hi all,
On Mon, May 13, 2024 at 10:08:38AM +0200, José Expósito wrote:
> On Fri, May 10, 2024 at 06:19:45PM +0200, Louis Chauvet wrote:
> > Le 09/05/24 - 18:18, Jim Shargo a écrit :
> > > Sima--thanks SO MUCH for going through with everything leaving a
> > > detailed review. I am excited to go through your feedback.
> > >
> > > It makes me extremely happy to see these patches get people excited.
> > >
> > > They've bounced between a few people, and I recently asked to take
> > > them over again from the folks who were most recently looking at them
> > > but haven't since had capacity to revisit them. I'd love to contribute
> > > more but I am currently pretty swamped and I probably couldn't
> > > realistically make too much headway before the middle of June.
> > >
> > > José--if you've got capacity and interest, I'd love to see this work
> > > get in! Thanks!! Please let me know your timeline and if you want to
> > > split anything up or have any questions, I'd love to help if possible.
> > > But most important to me is seeing the community benefit from the
> > > feature.
> > >
> > > And (in case it got lost in the shuffle of all these patches) the IGT
> > > tests really make it much easier to develop this thing. Marius has
> > > posted the most recent patches:
> > > https://lore.kernel.org/igt-dev/?q=configfs
> > >
> > > Thanks!
> > > -- Jim
> > >
> > >
> > >
> > > On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
> > > >
> > > > Hi everyone,
> > > >
> > > > I wasn't aware of these patches, but I'm really glad they are getting
> > > > some attention, thanks a lot for your review Sima.
> > > >
> > > > Given that it's been a while since the patches were emailed, I'm not
> > > > sure if the original authors of the patches could implement your
> > > > comments. If not, I can work on it. Please let me know.
> > > >
> > > > I'm working on a Mutter feature that'd greatly benefit from this uapi
> > > > and I'm sure other compositors would find it useful.
> > > >
> > > > I'll start working on a new version in a few days if nobody else is
> > > > already working on it.
> > > >
> > > > Best wishes,
> > > > José Expósito
> >
> > Hi all!
> >
> > Very nice to see other people working on this subject. As the series
> > seemed inactive, I started two weeks ago to rebase it on top of [1]. I
> > also started some work to use drmm_* helpers instead of using lists in
> > vkms. I currently struggle with a deadlock during rmmod.
> >
> > I need to clean my commits, but I can share a WIP version.
>
> Hi Louis,
>
> If you could share a RFC/WIP series it would be awesome!
>
> Since you are already working on the kernel patches (and I guess IGT?),
> I'll start working on a libdrm high level API to interact with VKMS from
> user-space on top of your patches. I'll share a link as soon as I have a
> draft PR.
Just out of curiosity what API would that be? These should fairly
simple that they can be configured from a shell script
(mount/mkdir/rm/echo/umount). Believe should be easy enough to test stuff with
bunch scripts like that.
Perphas landing the I-G-T tests first (assuming we're settled
on how exactly this would work) might be of greated help to get a green lit
the kernel driver side? Skip if vkms/configfs/something else that tells
us VKMS doesn't have ConfigFS eneabled, and run it when that is on.
The lastest iteration was shared by Jim at
https://lore.kernel.org/igt-dev/[email protected]/
That way sub-sequent BAT CI would pick up issues, and can also used
independently by Louis. Should also divide the work-load evenly with
Louis focusing on the just the driver. Happy to review and test it.
>
> > Maybe we can discuss a bit the comment from Daniel (split init between
> > default/configfs, use or not a real platform device...)
> >
> > For the split, I think the first solution (struct vkms_config) can be
> > easier to understand and to implement, for two reasons:
> > - No need to distinguish between the "default" and the "configfs" devices
> > in the VKMS "core". All is managed with only one struct vkms_config.
> > - Most of the lifetime issue should be gone. The only thing to
> > synchronize is passing this vkms_config from ConfigFS to VKMS.
>
> I agree, this seems like the easiest solution.
>
> > The drawback of this is that it can become difficult to do the "runtime"
> > configuration (today only hotplug, but I plan to add more complex stuff
> > like DP emulation, EDID selection, MST support...). Those configuration
> > must be done "at runtime" and will require a strong synchronization with
> > the vkms "core".
> >
> > Maybe we can distinguish between the "creation" and the "runtime
> > configuration", in two different configFS directory? Once a device is
> > created, it is moved to the "enabled" directory and will have a different
> > set of attribute (connection status, current EDID...)
>
> Once the device is enabled (i.e, `echo 1 > /config/vkms/my-device/enabled`),
> would it make sense to use sysfs instead of another configfs directory?
> The advantage is that with sysfs the kernel controls the lifetime of the
> objects and I think it *might* simplify the code, but I'll need to write a
> proof of concept to see if this works.
Can indeed sysfs be used similar to ConfigFS? To me it sounds like sysfs is a
view into a kernel objects, mostly for viewing and slight modifications
but not manipulating, adding/removing, on the fly, various things. Sort
of see it the other way around, device enabled with sysfs but
configuration happens through ConfigFS. At least from a user-space pov.
>
> > For the platform driver part, it seems logic to me to use a "real"
> > platform driver and a platform device for each pipeline, but I don't have
> > the experience to tell if this is a good idea or not.
>
> I'm afraid I don't know which approach could work better. Trusting Sima and
> Maíra on this one.
>
> Jose
>
> > [1]: https://lore.kernel.org/dri-devel/[email protected]/
> >
> > Thanks,
> > Louis Chauvet
> >
> > --
> > Louis Chauvet, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
On Mon, May 13, 2024 at 12:03:07PM +0300, Marius Vlad wrote:
> Hi all,
> On Mon, May 13, 2024 at 10:08:38AM +0200, José Expósito wrote:
> > On Fri, May 10, 2024 at 06:19:45PM +0200, Louis Chauvet wrote:
> > > Le 09/05/24 - 18:18, Jim Shargo a écrit :
> > > > Sima--thanks SO MUCH for going through with everything leaving a
> > > > detailed review. I am excited to go through your feedback.
> > > >
> > > > It makes me extremely happy to see these patches get people excited.
> > > >
> > > > They've bounced between a few people, and I recently asked to take
> > > > them over again from the folks who were most recently looking at them
> > > > but haven't since had capacity to revisit them. I'd love to contribute
> > > > more but I am currently pretty swamped and I probably couldn't
> > > > realistically make too much headway before the middle of June.
> > > >
> > > > José--if you've got capacity and interest, I'd love to see this work
> > > > get in! Thanks!! Please let me know your timeline and if you want to
> > > > split anything up or have any questions, I'd love to help if possible.
> > > > But most important to me is seeing the community benefit from the
> > > > feature.
> > > >
> > > > And (in case it got lost in the shuffle of all these patches) the IGT
> > > > tests really make it much easier to develop this thing. Marius has
> > > > posted the most recent patches:
> > > > https://lore.kernel.org/igt-dev/?q=configfs
> > > >
> > > > Thanks!
> > > > -- Jim
> > > >
> > > >
> > > >
> > > > On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
> > > > >
> > > > > Hi everyone,
> > > > >
> > > > > I wasn't aware of these patches, but I'm really glad they are getting
> > > > > some attention, thanks a lot for your review Sima.
> > > > >
> > > > > Given that it's been a while since the patches were emailed, I'm not
> > > > > sure if the original authors of the patches could implement your
> > > > > comments. If not, I can work on it. Please let me know.
> > > > >
> > > > > I'm working on a Mutter feature that'd greatly benefit from this uapi
> > > > > and I'm sure other compositors would find it useful.
> > > > >
> > > > > I'll start working on a new version in a few days if nobody else is
> > > > > already working on it.
> > > > >
> > > > > Best wishes,
> > > > > José Expósito
> > >
> > > Hi all!
> > >
> > > Very nice to see other people working on this subject. As the series
> > > seemed inactive, I started two weeks ago to rebase it on top of [1]. I
> > > also started some work to use drmm_* helpers instead of using lists in
> > > vkms. I currently struggle with a deadlock during rmmod.
> > >
> > > I need to clean my commits, but I can share a WIP version.
> >
> > Hi Louis,
> >
> > If you could share a RFC/WIP series it would be awesome!
> >
> > Since you are already working on the kernel patches (and I guess IGT?),
> > I'll start working on a libdrm high level API to interact with VKMS from
> > user-space on top of your patches. I'll share a link as soon as I have a
> > draft PR.
>
> Just out of curiosity what API would that be? These should fairly
> simple that they can be configured from a shell script
> (mount/mkdir/rm/echo/umount). Believe should be easy enough to test stuff with
> bunch scripts like that.
My plan is to add a very thin C API around mkdir/rmdir/etc.
It is true that VKMS can be configure easily using a bash script; however,
compositors with test suites written in C (or with bindings to libdrm) would
have to write similar wrappers around the mkdir/rmdir/etc calls.
I think that it could be beneficial for them to have a shared wrapper available
in libdrm.
> Perphas landing the I-G-T tests first (assuming we're settled
> on how exactly this would work) might be of greated help to get a green lit
> the kernel driver side? Skip if vkms/configfs/something else that tells
> us VKMS doesn't have ConfigFS eneabled, and run it when that is on.
>
> The lastest iteration was shared by Jim at
> https://lore.kernel.org/igt-dev/[email protected]/
>
> That way sub-sequent BAT CI would pick up issues, and can also used
> independently by Louis. Should also divide the work-load evenly with
> Louis focusing on the just the driver. Happy to review and test it.
>
> >
> > > Maybe we can discuss a bit the comment from Daniel (split init between
> > > default/configfs, use or not a real platform device...)
> > >
> > > For the split, I think the first solution (struct vkms_config) can be
> > > easier to understand and to implement, for two reasons:
> > > - No need to distinguish between the "default" and the "configfs" devices
> > > in the VKMS "core". All is managed with only one struct vkms_config.
> > > - Most of the lifetime issue should be gone. The only thing to
> > > synchronize is passing this vkms_config from ConfigFS to VKMS.
> >
> > I agree, this seems like the easiest solution.
> >
> > > The drawback of this is that it can become difficult to do the "runtime"
> > > configuration (today only hotplug, but I plan to add more complex stuff
> > > like DP emulation, EDID selection, MST support...). Those configuration
> > > must be done "at runtime" and will require a strong synchronization with
> > > the vkms "core".
> > >
> > > Maybe we can distinguish between the "creation" and the "runtime
> > > configuration", in two different configFS directory? Once a device is
> > > created, it is moved to the "enabled" directory and will have a different
> > > set of attribute (connection status, current EDID...)
> >
> > Once the device is enabled (i.e, `echo 1 > /config/vkms/my-device/enabled`),
> > would it make sense to use sysfs instead of another configfs directory?
> > The advantage is that with sysfs the kernel controls the lifetime of the
> > objects and I think it *might* simplify the code, but I'll need to write a
> > proof of concept to see if this works.
> Can indeed sysfs be used similar to ConfigFS? To me it sounds like sysfs is a
> view into a kernel objects, mostly for viewing and slight modifications
> but not manipulating, adding/removing, on the fly, various things. Sort
> of see it the other way around, device enabled with sysfs but
> configuration happens through ConfigFS. At least from a user-space pov.
> >
> > > For the platform driver part, it seems logic to me to use a "real"
> > > platform driver and a platform device for each pipeline, but I don't have
> > > the experience to tell if this is a good idea or not.
> >
> > I'm afraid I don't know which approach could work better. Trusting Sima and
> > Maíra on this one.
> >
> > Jose
> >
> > > [1]: https://lore.kernel.org/dri-devel/[email protected]/
> > >
> > > Thanks,
> > > Louis Chauvet
> > >
> > > --
> > > Louis Chauvet, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
> > > Hi Louis,
> > >
> > > If you could share a RFC/WIP series it would be awesome!
Hi all!
I just uploaded my WIP series to github here [1]. Most of the work is
extracted from the current ConfigFS series, I just splitted and completed
what was done. I also tried to take in account the comments from Sima.
All commits should compile and `modprobe/rmmod/kms_plane` should not
crashing. The commits are not totaly clean, but it should be only cosmetic
stuff (formatting in the wrong commit for example). The commit messages
are not written yet, but the title should be sufficient to understand the
content of each commit.
This is how I plan to split this work in series: (hash may change over
time, I will force push to clean commits)
Some preparation stuff (no functionnal change):
256d7045ec70 drm/vkms: Formatting and typo fix
cc2de5004c42 drm/vkms: Rename index to possible_crtc
a74cefc87b9c drm/vkms: Add documentation
More preparation to split everything properly (no functionnal change):
ad2d0b07558f drm/vkms: Properly extract vkms_formats header
f9639cca2d43 drm/vkms: Extract vkms_writeback header
7edda8012b44 drm/vkms: Extract vkms_plane header
ced09ed9d0f7 drm/vkms: Rename macro to avoid confusion
9f00e4823529 drm/vkms: Extract vkms_crtc header
b510e480ed92 drm/vkms: Extract vkms_composer header
Switch all the vkms object to managed (this part need a careful review,
I am new with DRM, so I probably did some error):
ddef3c09ead6 drm/vkms: Switch to managed for connector
8859cad0e192 drm/vkms: Switch to managed for encoder
d2b8d93fb684 drm/vkms: Switch to managed for crtc
d1ad316b0f0d drm/vkms: Rename all vkms_crtc instance to be consistent
Temporaly remove debugfs entry, I plan to remove this commit:
079d875c015e drm/vkms: remove debugfs entry about the current vkms configuration
Clean up vkms_device and unlink vkms_config from vkms_device.
c782dbe9edc3 drm/vkms: Remove vkms_config from vkms_device
8a27c13634a3 drm/vkms: Remove (useles?) group
8fb24e1cdf88 drm/vkms: Introduce directly the default device as global/Remove default vkms config
More cleanup:
2572d90723ac drm/vkms: Remove possible crtc from parameters
Switching to platform driver (same thing, it is my first time, I probably
messed up things):
63be09e05760 drm/vkms: Use a real platform driver
5f4cf18b07d3 drm/vkms: Extract device driver in its own file
The configFS implementation itself. It only allows to create/enable/delete
a device:
b34651685f2e drm/vkms: Introduce configfs
Those commits were a POC to confirm that it works. They need to be
replaced by the "real" configuration (creation&link of crtc/connector/planes...)
dd55451ccef2 drm/vkms: Make overlay configurable with configfs
9dca357f1ee3 drm/vkms: Make cursor configurable with configfs
bd721f41fad9 drm/vkms: Make writeback configurable with configfs
Kind regards,
Louis Chauvet
[1]: https://github.com/Fomys/linux/tree/b4/new-configfs
> > > Since you are already working on the kernel patches (and I guess IGT?),
> > > I'll start working on a libdrm high level API to interact with VKMS from
> > > user-space on top of your patches. I'll share a link as soon as I have a
> > > draft PR.
> >
> > Just out of curiosity what API would that be? These should fairly
> > simple that they can be configured from a shell script
> > (mount/mkdir/rm/echo/umount). Believe should be easy enough to test stuff with
> > bunch scripts like that.
>
> My plan is to add a very thin C API around mkdir/rmdir/etc.
>
> It is true that VKMS can be configure easily using a bash script; however,
> compositors with test suites written in C (or with bindings to libdrm) would
> have to write similar wrappers around the mkdir/rmdir/etc calls.
> I think that it could be beneficial for them to have a shared wrapper available
> in libdrm.
>
> > Perphas landing the I-G-T tests first (assuming we're settled
> > on how exactly this would work) might be of greated help to get a green lit
> > the kernel driver side? Skip if vkms/configfs/something else that tells
> > us VKMS doesn't have ConfigFS eneabled, and run it when that is on.
> >
> > The lastest iteration was shared by Jim at
> > https://lore.kernel.org/igt-dev/[email protected]/
> >
> > That way sub-sequent BAT CI would pick up issues, and can also used
> > independently by Louis. Should also divide the work-load evenly with
> > Louis focusing on the just the driver. Happy to review and test it.
> >
> > >
> > > > Maybe we can discuss a bit the comment from Daniel (split init between
> > > > default/configfs, use or not a real platform device...)
> > > >
> > > > For the split, I think the first solution (struct vkms_config) can be
> > > > easier to understand and to implement, for two reasons:
> > > > - No need to distinguish between the "default" and the "configfs" devices
> > > > in the VKMS "core". All is managed with only one struct vkms_config.
> > > > - Most of the lifetime issue should be gone. The only thing to
> > > > synchronize is passing this vkms_config from ConfigFS to VKMS.
> > >
> > > I agree, this seems like the easiest solution.
> > >
> > > > The drawback of this is that it can become difficult to do the "runtime"
> > > > configuration (today only hotplug, but I plan to add more complex stuff
> > > > like DP emulation, EDID selection, MST support...). Those configuration
> > > > must be done "at runtime" and will require a strong synchronization with
> > > > the vkms "core".
> > > >
> > > > Maybe we can distinguish between the "creation" and the "runtime
> > > > configuration", in two different configFS directory? Once a device is
> > > > created, it is moved to the "enabled" directory and will have a different
> > > > set of attribute (connection status, current EDID...)
> > >
> > > Once the device is enabled (i.e, `echo 1 > /config/vkms/my-device/enabled`),
> > > would it make sense to use sysfs instead of another configfs directory?
> > > The advantage is that with sysfs the kernel controls the lifetime of the
> > > objects and I think it *might* simplify the code, but I'll need to write a
> > > proof of concept to see if this works.
> > Can indeed sysfs be used similar to ConfigFS? To me it sounds like sysfs is a
> > view into a kernel objects, mostly for viewing and slight modifications
> > but not manipulating, adding/removing, on the fly, various things. Sort
> > of see it the other way around, device enabled with sysfs but
> > configuration happens through ConfigFS. At least from a user-space pov.
> > >
> > > > For the platform driver part, it seems logic to me to use a "real"
> > > > platform driver and a platform device for each pipeline, but I don't have
> > > > the experience to tell if this is a good idea or not.
> > >
> > > I'm afraid I don't know which approach could work better. Trusting Sima and
> > > Ma?ra on this one.
> > >
> > > Jose
> > >
> > > > [1]: https://lore.kernel.org/dri-devel/[email protected]/
> > > >
> > > > Thanks,
> > > > Louis Chauvet
> > > >
> > > > --
> > > > Louis Chauvet, Bootlin
> > > > Embedded Linux and Kernel engineering
> > > > https://bootlin.com
>
>
--
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, May 13, 2024 at 10:08:38AM +0200, José Expósito wrote:
> On Fri, May 10, 2024 at 06:19:45PM +0200, Louis Chauvet wrote:
> > Le 09/05/24 - 18:18, Jim Shargo a écrit :
> > > Sima--thanks SO MUCH for going through with everything leaving a
> > > detailed review. I am excited to go through your feedback.
> > >
> > > It makes me extremely happy to see these patches get people excited.
> > >
> > > They've bounced between a few people, and I recently asked to take
> > > them over again from the folks who were most recently looking at them
> > > but haven't since had capacity to revisit them. I'd love to contribute
> > > more but I am currently pretty swamped and I probably couldn't
> > > realistically make too much headway before the middle of June.
> > >
> > > José--if you've got capacity and interest, I'd love to see this work
> > > get in! Thanks!! Please let me know your timeline and if you want to
> > > split anything up or have any questions, I'd love to help if possible.
> > > But most important to me is seeing the community benefit from the
> > > feature.
> > >
> > > And (in case it got lost in the shuffle of all these patches) the IGT
> > > tests really make it much easier to develop this thing. Marius has
> > > posted the most recent patches:
> > > https://lore.kernel.org/igt-dev/?q=configfs
> > >
> > > Thanks!
> > > -- Jim
> > >
> > >
> > >
> > > On Wed, May 8, 2024 at 2:17 PM José Expósito <[email protected]> wrote:
> > > >
> > > > Hi everyone,
> > > >
> > > > I wasn't aware of these patches, but I'm really glad they are getting
> > > > some attention, thanks a lot for your review Sima.
> > > >
> > > > Given that it's been a while since the patches were emailed, I'm not
> > > > sure if the original authors of the patches could implement your
> > > > comments. If not, I can work on it. Please let me know.
> > > >
> > > > I'm working on a Mutter feature that'd greatly benefit from this uapi
> > > > and I'm sure other compositors would find it useful.
> > > >
> > > > I'll start working on a new version in a few days if nobody else is
> > > > already working on it.
> > > >
> > > > Best wishes,
> > > > José Expósito
> >
> > Hi all!
> >
> > Very nice to see other people working on this subject. As the series
> > seemed inactive, I started two weeks ago to rebase it on top of [1]. I
> > also started some work to use drmm_* helpers instead of using lists in
> > vkms. I currently struggle with a deadlock during rmmod.
> >
> > I need to clean my commits, but I can share a WIP version.
>
> Hi Louis,
>
> If you could share a RFC/WIP series it would be awesome!
>
> Since you are already working on the kernel patches (and I guess IGT?),
> I'll start working on a libdrm high level API to interact with VKMS from
> user-space on top of your patches. I'll share a link as soon as I have a
> draft PR.
Great to see all the enthusiasm here, this is awesome.
Note that I'm out of office for two weeks next week, so if I miss any
patches please ping me again (sima in #dri-devel on oftc tends to work
best) when I'm back.
> > Maybe we can discuss a bit the comment from Daniel (split init between
> > default/configfs, use or not a real platform device...)
> >
> > For the split, I think the first solution (struct vkms_config) can be
> > easier to understand and to implement, for two reasons:
> > - No need to distinguish between the "default" and the "configfs" devices
> > in the VKMS "core". All is managed with only one struct vkms_config.
> > - Most of the lifetime issue should be gone. The only thing to
> > synchronize is passing this vkms_config from ConfigFS to VKMS.
>
> I agree, this seems like the easiest solution.
>
> > The drawback of this is that it can become difficult to do the "runtime"
> > configuration (today only hotplug, but I plan to add more complex stuff
> > like DP emulation, EDID selection, MST support...). Those configuration
> > must be done "at runtime" and will require a strong synchronization with
> > the vkms "core".
> >
> > Maybe we can distinguish between the "creation" and the "runtime
> > configuration", in two different configFS directory? Once a device is
> > created, it is moved to the "enabled" directory and will have a different
> > set of attribute (connection status, current EDID...)
>
> Once the device is enabled (i.e, `echo 1 > /config/vkms/my-device/enabled`),
> would it make sense to use sysfs instead of another configfs directory?
> The advantage is that with sysfs the kernel controls the lifetime of the
> objects and I think it *might* simplify the code, but I'll need to write a
> proof of concept to see if this works.
sysfs is very opinionated about lifetime, so we might actually make this
more complicated. Plus for the only thing we can hotplug (connectors) we
already have sysfs directories, so there could be a lifetime/name fight
between the sysfs interfaces to prepare a hotplugged connector, and the
connector sysfs files which are part of the existing uapi.
Also the second issue I'm seeing is that we're mixing up
testing/configuration apis with the generic uapi that should hold for
every kms driver. This could make the code in igt testcase or for driving
compositor end-to-end testcases a lot more confusing. I think separation
would be better.
The third point I'm seeing is that connectors can be created both before
we create the device, and at runtime. If we have two totally separate
interfaces for this, we might end up with needless code duplication.
But it's a complex topic, I think it does make sense to give sysfs some
serious thought. But maybe as part of the vkms driver directory, and not
in the drm_device chardev directories. So we could have some separation
that way maybe?
> > For the platform driver part, it seems logic to me to use a "real"
> > platform driver and a platform device for each pipeline, but I don't have
> > the experience to tell if this is a good idea or not.
>
> I'm afraid I don't know which approach could work better. Trusting Sima and
> Maíra on this one.
As I've said, I'm not opposed to a switch. I just think it's an orthogonal
issue to the configfs and should be separately justified.
We're trying hard to get away from kms userspace sneaking too much under
the hood of the driver, and have gone a long way from the o.g. drm days
where "everything is pci" was encoded into uapi. So from that pov I kinda
like the fact that vkms is special and fairly free-floating.
But maybe userspace does want to be able to test their device enumeration
more like a real device, so if vkms currently sticks out there that would
be a really good reason to change things and make it look more like a real
driver/device.
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch