Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 > > > 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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
> > > 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 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.v...@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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 > > > > 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.v...@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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 > > > 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.v...@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 >
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 > > 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-de1c5728f...@bootlin.com/ > > Thanks, > Louis Chauvet > > -- > Louis Chauvet, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 > 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-de1c5728f...@bootlin.com/ Thanks, Louis Chauvet -- Louis Chauvet, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
On Tue, Aug 29, 2023 at 05:30:52AM +, 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/+/469 > > 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
Re: [PATCH v6 0/7] Adds support for ConfigFS to VKMS!
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 +, 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/+/469 > > 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 > signature.asc Description: PGP signature
