Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Simona Vetter <simona.vetter@ffwll.ch>]

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.

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

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

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

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

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.

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