Message-ID: <ZpU_9WnG_6xfTaTL@fedora>
Date: Mon, 15 Jul 2024 17:27:49 +0200
From: José Expósito <jose.exposito89@...il.com>
To: Marius Vlad <marius.vlad@...labora.com>,
	Jim Shargo <jshargo@...gle.com>, daniel@...ll.ch,
	brpol@...omium.org, corbet@....net, dri-devel@...ts.freedesktop.org,
	hamohammed.sa@...il.com, hirono@...omium.org, jshargo@...omium.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	mairacanal@...eup.net, mduggan@...omium.org, melissa.srw@...il.com,
	mripard@...nel.org, rodrigosiqueiramelo@...il.com,
	tzimmermann@...e.de
Subject: 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&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

Powered by blists - more mailing lists