Message-ID: <Z7b5OcewxCEsDGo9@phenom.ffwll.local>
Date: Thu, 20 Feb 2025 10:43:21 +0100
From: Simona Vetter <simona.vetter@...ll.ch>
To: Louis Chauvet <louis.chauvet@...tlin.com>
Cc: Maxime Ripard <mripard@...nel.org>,
	Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
	Melissa Wen <melissa.srw@...il.com>,
	Maíra Canal <mairacanal@...eup.net>,
	Haneen Mohammed <hamohammed.sa@...il.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	jose.exposito89@...il.com, dri-devel@...ts.freedesktop.org,
	arthurgrillo@...eup.net, linux-kernel@...r.kernel.org,
	jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com,
	thomas.petazzoni@...tlin.com, seanpaul@...gle.com,
	nicolejadeyee@...gle.com
Subject: Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

On Wed, Feb 19, 2025 at 05:28:37PM +0100, Louis Chauvet wrote:
> Le 19/02/2025 à 14:15, Simona Vetter a écrit :
> > On Wed, Feb 19, 2025 at 10:28:56AM +0100, Maxime Ripard wrote:
> > > On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
> > > > Hi,
> > > > 
> > > > > > Hi all,
> > > > > > 
> > > > > > I am also currently working on MST emulation for VKMS. If someone can read
> > > > > > what I already did and at tell me if my implementation seems on the right
> > > > > > track it could be nice.
> > > > > > 
> > > > > > The current status is not very advanced: I can emulate a mst HUB, but not
> > > > > > a screen. I am currently working on properly emulating the HUB by using an
> > > > > > other hub.
> > > > > > 
> > > > > > You can find the branch for this work here:
> > > > > > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
> > 
> > Can't look at this because it's private.
> 
> Hi Maxime, Sima,
> 
> Normally, those should work:
> 
> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/vkms-mst
> https://github.com/Fomys/linux/tree/vkms-mst
> 
> I just re-tested, this is broken, probably because I never had the time to
> properly finish my last idea: simplifying vkms_connector by creating
> vkms_mst_emulator_root. With the rest of the code (i.e.
> vkms_mst_hub/display_emulator + vkms_connector), I was able to make this
> config working:
> 
> HUB1 -> HUB2 -> DISPLAY1
>      |       -> DISPLAY2
>      -> DISPLAY3
> 
> (working == it was listed properly by modetest + did not report any issue
> when using a connector with modetest -s)
> 
> Few things to note: no ConfigFS support, no eBPF support, poorly tested
> (there are probably multithread/recursion issues)
> 
> I had to stop working on it because I don't have anymore time, I plan to at
> least rebase + send an RFC once the VKMS+ConfigFS work is done.
> 
> > > > > I think this is exactly the kind of things where we'll want eBPF I
> > > > > think. There's no way you'll be able to model each possible test
> > > > > scenarios for MST through configfs, even more so with a stable
> > > > > interface.
> > > > 
> > > > I just spent some time to think about it. I agree that eBPF seems
> > > > applicable: we want to allows userspace to emulate any MST device, and we
> > > > don't want to create huge uAPI + whole emulation in the kernel.
> > > > 
> > > > As most of the protocol is similar accros device kind, I currently built
> > > > my code around the struct vkms_mst_sideband_helpers to specify only the
> > > > changed behavior (this way the "write to registers" "parse command"... is
> > > > only done in one place). The choice of function is not definitive at all
> > > > (it was just practical at this moment).
> > > > 
> > > > With eBPF, I know three different way to attach programs to the kernel:
> > > >   - Using a kprobe/attaching to a function: I can change the behavior of
> > > >     all the device, there is no way "attach prog1 to hub" and "attach prog2
> > > >     to screen1", it will be "attach prog1 to the function
> > > >     `vkms_mst_emulator_handle_transfer_write`, and all the device will be
> > > >     affected. This should be very easy to implement (maybe it already
> > > >     works without modification?), but very limited / make userspace stuff
> > > >     very ugly => Not ideal at all
> > > >   - Creating a whole architecture to attach eBPF programs in vkms: VKMS
> > > >     manage the list of attached eBPF programs, call them when it needs...
> > > >     This is probably the "most flexible" option (in the sense "VKMS can do
> > > >     whatever we need to fit our usecase"). This seems not trivial to
> > > >     implement (drm complexity + MST complexity + BPF complexity in the same
> > > >     driver seems a bad idea, espicially because VKMS don't have a lot of
> > > >     maintainance and I don't feel confident introducing more complexity)
> > > >     => Can work, require some work, but may bring more complexity in VKMS
> > > >   - Using BPF struct_ops: I can "simply" create/complete a struct ops for
> > > >     diverse mst callbacks (see the proposition bellow). I looked at some
> > > >     example, this seems to be "easy" to do, and the work in VKMS should be
> > > >     limited.
> > > >     => Can work, a bit less flexible than the previous solution, but avoid
> > > >     a lot of complexity
> > > > 
> > > > I don't know if I will be able to finish the implementation but I imagine
> > > > something like that may be a nice interface (may be not possible "as is"):
> > > > 
> > > > // vkms_mst.c struct_ops that can be filled by userspace with custom
> > > > // functions
> > > > // Known limitation: maximum 64 functions in this table
> > > > struct vkms_mst_ops {
> > > > 	// Completly overide the transfer function, so the userspace can
> > > > 	// do whatever he wants (even emulating a complex MST tree
> > > > 	// without using stuff in vkms)
> > > > 	handle_transfer(drm_dp_aux_msg);
> > > > 
> > > > 	// If default transfer function is used, those are the callback
> > > > 	// you can use (again, they will come with default
> > > > 	// implementation)
> > > > 	clear_payload_id_table(..);
> > > > 	link_address(..);
> > > > 	enum_path_ressources(..);
> > > > 	[...]
> > > > 
> > > > 	// Used to identify this kind of device, in my example the
> > > > 	// userspace could register "LG_screen", "dell dock", "HP screen",
> > > > 	// and then configure each mst device with the correct kind
> > > > 	name = "<unique name for this device kind>",
> > > > 
> > > > 	// If you want to use the default "hub" implementation, but only
> > > > 	// tweak one specific behavior, you can use this
> > > > 	base = "<name of the other structops>"
> > > > }
> > > > 
> > > > 
> > > > // Needed to implement eBPF struct_ops, called when userspace registers a
> > > > // struct_ops of type vkms_mst_ops
> > > > void register_struct_ops(new_ops...) {
> > > > 	vkms_registered_ops.append(new_ops).
> > > > }
> > > > 
> > > > // In vkms_connector.c
> > > > // Callback called by drm core to do transfer on the connector
> > > > void vkms_mst_transfer(aux, msg) {
> > > > 	mst_emulator = get_mst_emulator(aux);
> > > > 
> > > > 	ops = vkms_registered_ops.search_for(mst_emulator.name);
> > > > 	ops->handle_transfer(msg);
> > > > }
> > > > 
> > > > // mst_ebpf.c In the BPF program, userspace side
> > > > void handle_transfer(...) {
> > > > 	[...]
> > > > }
> > > > struct vkms_mst_ops {
> > > > 	handle_transfer = handle_transfer;
> > > > 	name = "lg-screen";
> > > > 	base = "default-screen"
> > > > }
> > > 
> > > I don't think MST is the right abstraction there. We should have MST
> > > related helper functions available to eBPF programs, but we should load
> > > them at the connector level, ie, implement a full blown connector
> > > atomic_check for example. It's more flexible and will allow to implement
> > > other use-cases people have been interested in (like panels).
> > 
> > So since I can't look at the code I'll just drop my thoughts here.
> > 
> > - I think validating the MST helpers implementation should be done with
> >    kunit tests. So these are out of scope for vkms testing I think
> >    entirely.
> 
> Yes, I agree with this, and it joins your last comment: the full dp-aux
> emulation does not belong only to VKMS. I had this idea only because my
> solution only use the normal core MST implementation (no special handling in
> VKMS, just pure dp-aux emulation), so technically you could also stress-test
> drm core with it.
> 
> > - Next up are the driver implementations. There we might want to be able
> >    to inject fake mst devices to stress-test driver corner cases. But I
> >    don't think that's a job for vkms either.
> 
> I agree, VKMS is not here to test other drivers.
> 
> > - Now for vkms itself, I think the interesting case here is being able to
> >    test compositors against funny mst corner-cases, but for that we don't
> >    really need an mst operation at all. So no dp-aux or anything like that,
> >    we just hotplug-create connectors with names and PATH property and MST
> >    type, without any of the kernel-internal presentations for hubs/branch
> >    points and all that stuff. Because userspace doesn't ever see that.
> 
> I knew that user space don't really see the MST information (apart from
> PATH), but I did not think about this solution. This may work well to test
> user space, I agree!
> 
> I think we are on the good track with José, he is trying to implement
> connector hot-creation through ConfigFS [1]. To add "MST emulation", we can
> "simply" add the PATH property through ConfigFS.

Yeah, I think that's the way to go. Well maybe with a change to always
include the PATH property, because currently that's true for all
hotpluggable connectors. And we probably want to keep that if we extend it
to hotpluggable bridges or similar.

> [1]: Few discussions here
> https://lore.kernel.org/all/Z5zJ1rEZyBEgd7DN@louis-chauvet-laptop/

Agreeing with you, connector hotplug is something we need to tackle as an
extension of the basic configfs support, since it's quite complex.

> > - Next up is expressing all the funny constraints that can result in,
> >    across different drivers. For that I think we want ebpf to implement the
> >    various atomic_check hooks, so that you can implement all the funny
> >    constraints of mst link bw limitations, but also host-side display
> >    limitations. And I concur with Maxime that this ebpf support should be
> >    entirely agnostic and just allow you to attach atomic_check
> >    implementations to connectors, planes and crtcs. And then maybe one for
> >    the overall commit, so that global constraints are easier to implement.
> 
> If I understand correctly, this has nothing to do with VKMS + MST?
> I don't clearly understand the use case: why do we want the user space to
> express hardware limitations? If you have already discussed this on the
> mailing list, can you share the discussion?

Not sure this was discussed in-depth, but when you get into more complex
output configurations, there's all kinds of funny hw constraints that pop
up. Examples:
- Multiple mst outputs on the same physical port share the overall
  bandwidth. So individually you might be able to light up each connector
  at max resolution, but if you try to light up all of them, there's a
  limitation. This is the mst specific case.
- There's also lots of display hw constraints, like a limited amount of
  clocks (fewer than crtc), or memory bw constraints for scanout, and
  similar things.

The idea is to express these constraints with ebpf programs, so that you
can validate that a compositor correctly handles these cases and doesn't
try an invalide configuration and then just fails instead of trying to
fall back to something that works.

So it's a much bigger issue, but multi-connector mst is a fairly important
case here.

Cheers, Sima

> > So summary: MST testing in vkms only needs to look like MST to userspace.
> > Internally we'll just hand-roll the entire connector hotplug and leave out
> > everything else. Because testing driver dp mst implementations and the
> > helpers is better covered through different means.
> >
> > Eventually we might want to implement fake i2c and dp-aux implementations
> > for additional features like TV remote control and fun stuff like that (I
> > forgot the CEA/HDMI name for these). But I think that's waaaayyyyyy down
> > the road.
> 
> I think I am not that far from a full dp-aux emulation, it is precisely what
> I tried to do in VKMS. I don't have the time to transform it to kunit tests.
> 
> Thanks a lot for your feedback!
> Louis Chauvet
> 
> > Cheers, Sima
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists