Message-ID: <Zkd_C77S-ERpMiqu@localhost.localdomain>
Date: Fri, 17 May 2024 18:00:11 +0200
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: José Expósito <jose.exposito89@...il.com>
Cc: 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!

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

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists