mbox series

[v6,0/7] Adds support for ConfigFS to VKMS!

Message ID 20230829053201.423261-1-brpol@chromium.org (mailing list archive)
Headers show
Series Adds support for ConfigFS to VKMS! | expand

Message

Brandon Ross Pollack Aug. 29, 2023, 5:30 a.m. UTC
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.

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

Comments

Marius Vlad Sept. 1, 2023, 9:32 a.m. UTC | #1
Hi Brandon,

You can now add https://lists.freedesktop.org/archives/igt-dev/2023-September/060717.html
as part of this series.

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.
> 
> 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 April 30, 2024, 8:36 a.m. UTC | #2
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
>
José Expósito May 8, 2024, 6:17 p.m. UTC | #3
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
Jim Shargo May 9, 2024, 10:18 p.m. UTC | #4
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 <jose.exposito89@gmail.com> 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
Louis Chauvet May 10, 2024, 4:19 p.m. UTC | #5
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 <jose.exposito89@gmail.com> 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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/

Thanks,
Louis Chauvet
José Expósito May 13, 2024, 8:08 a.m. UTC | #6
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 <jose.exposito89@gmail.com> 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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/
> 
> Thanks,
> Louis Chauvet
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Marius Vlad May 13, 2024, 9:03 a.m. UTC | #7
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 <jose.exposito89@gmail.com> 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/20230901092819.16924-1-marius.vlad@collabora.com/

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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/
> > 
> > Thanks,
> > Louis Chauvet
> > 
> > -- 
> > Louis Chauvet, Bootlin
> > Embedded Linux and Kernel engineering
> > https://bootlin.com
José Expósito May 13, 2024, 4:23 p.m. UTC | #8
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 <jose.exposito89@gmail.com> 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/20230901092819.16924-1-marius.vlad@collabora.com/
> 
> 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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/
> > > 
> > > Thanks,
> > > Louis Chauvet
> > > 
> > > -- 
> > > Louis Chauvet, Bootlin
> > > Embedded Linux and Kernel engineering
> > > https://bootlin.com
Louis Chauvet May 17, 2024, 4 p.m. UTC | #9
> > > 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/20230901092819.16924-1-marius.vlad@collabora.com/
> > 
> > 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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/
> > > > 
> > > > Thanks,
> > > > Louis Chauvet
> > > > 
> > > > -- 
> > > > Louis Chauvet, Bootlin
> > > > Embedded Linux and Kernel engineering
> > > > https://bootlin.com
> 
>
Daniel Vetter May 21, 2024, 12:25 p.m. UTC | #10
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 <jose.exposito89@gmail.com> 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
José Expósito July 15, 2024, 3:27 p.m. UTC | #11
On Fri, May 17, 2024 at 06:00:11PM +0200, Louis Chauvet wrote:
> > > > 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

Thanks for all your work Louis!!

I reviewed the first 9 patches and added a few comments on your
GitHub fork.

I think that this first batch of patches is independent of the
VKMS ConfigFS work and, if you want, you could send it as it is.

To try to make your work a bit easier, I applied the suggested
fixes and rebased on top of "upstream-drm-misc-next" your patches.

You can find them here:
https://github.com/JoseExposito/linux/commits/patch-vkms-header-refactor/

> 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

I'm still testing/understanding these patches.
I'll keep adding review comments :)

Best wishes,
José Expósito
 
> 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/20230901092819.16924-1-marius.vlad@collabora.com/
> > > 
> > > 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/20240409-yuv-v6-0-de1c5728fd70@bootlin.com/
> > > > > 
> > > > > 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