Message-ID: <Z50EnT31Fm8EWBX3@fedora>
Date: Fri, 31 Jan 2025 18:13:01 +0100
From: José Expósito <jose.exposito89@...il.com>
To: hamohammed.sa@...il.com, simona@...ll.ch, melissa.srw@...il.com,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 00/13] drm/vkms: Allow to configure device

On Fri, Jan 31, 2025 at 02:02:14PM +0100, Louis Chauvet wrote:
> On 31/01/25 - 10:31, José Expósito wrote:
> > On Thu, Jan 30, 2025 at 02:48:10PM +0100, Louis Chauvet wrote:
> > > On 29/01/25 - 12:00, José Expósito wrote:
> > > > Hi everyone,
> > > > 
> > > > In preparation for ConfigFS support, a flexible way to configure VKMS device(s)
> > > > is required.
> > > > This series adds the required APIs to create a configuration, the code changes
> > > > required to apply it and KUnit test validating the changes.
> > > 
> > > Hi José,
> > 
> > Hi Louis,
> > 
> > Thanks a lot for the quick review!
> > 
> > > Thanks a lot!
> > > 
> > > This series is amazing and better than mine on many points. I have few 
> > > comments:
> > > - a "strange" naming pair: add/destroy (I expect add/remove or 
> > >   create/destroy like other function in DRM)
> > 
> > I used "add" because the function creates and adds a display pipeline
> > items and "destroy" because the opposite function removes it and frees
> > its memory, so I wanted to emphasize that the action was destructive.
> > 
> > However, I don't have a strong preference about the naming. If you
> > prefer another pair of verbs, I'll be happy to change the function
> > names.
> 
> So, I think create/destroy is a bit better: `create` with the 
> `vkms_config` parameter is enough to tell "it allocates stuff in 
> `vkms_config`". 

ACK, I'll change in v2.

> > > - usage of "complex" list accessors, can't we just create iterators?
> > 
> > Yes, on the first iteration, I used the underlying structure: list
> > iterators for planes/CRTCs/encoders/connectors and xa_for_each for
> > the possible_* items.
> > 
> > However, I found 2 main issues that made me rewrite this code:
> > 
> > The first one is that, if in the future, we change the internal data
> > type, we'll have to change all the code using it. On this way, like
> > I did with all the other vkms_config_*_get_*() functions, the data is
> > encapsulated.
> 
> In one of my comment I proposed a macro to help on this point. I think 
> this is sufficient to hide internals. (see patch 7/13)

That's a very good point, I like the vkms_config_for_each_plane()
macro you suggested. Also, it matches really well with similar macros
in drm, for example drm_connector_for_each_possible_encoder().

I'll add them in v2.

> > The second one is vkms_config_get_connectors(). Unlike the other
> > functions, this one filters by connector enabled status. If we let the
> > caller do the filtering, we'll duplicate that logic.
> 
> That something I missed, and a very good point.
> 
> I will try to create a macro that do the filtered iteration, if I succeed 
> and you agree on the previous point, I think it does not worth it to have 
> those huge amount of code just to iterate over a list.
> 
> > Because of these two reasons, I decided to add a getter for lists.
> > 
> > > - should we use pr_err in vkms_config_valid?
> > 
> > I think it is great to show to the user a reason why their device couldn't
> > be enabled in dmesg... But I'm not sure if there is a better way to do it.
> 
> I was not clear: I agree we want some logs, but pr_err is too 
> agressive (see patch 8/13).

Agreed, we can lower to warn.

> > > > Louis Chauvet and I are working on ConfigFS support. In this series I tried to
> > > > merge his changes [1] with mine [2].
> > > > I kept his Signed-off-by to reflect that, even if I show up as the author of
> > > > some/most of the patches, this was a joint effort.
> > > 
> > > To avoid confusion, you should add the Co-developped-by tag, so it will be 
> > > clear that we worked together on this.
> > 
> > Good point, I'll change it.
> > 
> > > > I'm still polishing the ConfigFS code [3] and its IGT tests [4] (connector
> > > > hot-add/remove bugs) but the IGT tests also exercise this series and can be used
> > > > for additional test coverage.
> > > 
> > > I will take a look at those series. For the connector hot-add/remove, do 
> > > you have any example of usage in the kernel? I did not found anything in 
> > > the documentation explaining they are hot-addable.
> > 
> > I pushed a couple of WIP commits to the kernel and IGT so you can see/test
> > the crashes and hopefully share some ideas.
> > 
> > About the documentation: I didn't find much information other than a few
> > mentions to hot-add/remove. However, in one of my rebases, two new functions, 
> > drm_connector_dynamic_init() and drm_connector_dynamic_register(), were added:
> > https://patchwork.freedesktop.org/patch/628418/
> 
> Ho! This is exactly one issue I had when developping IGT tests, sometimes 
> you fetch the connector list, and when querying info about a specific 
> connector you get nothing!

I'm not sure if it is the same issue, but when it happened to me, I was
missing a call to drm_mode_config_reset(). Credit to Maxime for helping
me with this error :)
 
> > I'm still trying to make them work, but I think they are what we need.
> 
> After reading the patch "https://patchwork.freedesktop.org/patch/628418/", 
> I don't think we really need to support "dynamic connector creation" right 
> now:
> - None of the existing driver do it (except MST, but MST need a lot of 
>   stuff to works)
> - If we want to support it later, just create a "useless" 
>   /configfs/vkms/DEV/connectors/CON/enable that you must write 1 before 
>   the device initialization to make the connector working. This way we 
>   will not have to break the UAPI (the file is already there, disabled by 
>   default)

Fair enough, we can add it in the future. To keep it backwars compatible
though, we'll need to enable by default "static" connectors so users don't
need to change the way they create devices.

For "dynamic" connectors (connectors created once the device is enabled) we
would need to set them disabled on creation, but this would be backwards
compatible, as we won't allow creating new connectors in the first version.

I'll remove the connector hot-add/remove on v2... Meaning that configfs + IGT
will be ready for review :)

> > Part of the crashes happen on the cleanup of drm_client_setup(). Adding a
> > connector adds modes in the DRM client, but removing the connector doesn't
> > remove them and, on cleanup, I get a NULL pointer.
> >
> > I'm a bit stuck, so help or tips are very welcome :)
> 
> I will look at it next week (same repo/branch?).

Yes, same repo.

Enjoy your weekend!
Jose

> Have a nice weekend,
> Louis Chauvet
> 
> > 
> > > Thanks again for this series,
> > > Louis Chauvet
> > 
> > I'll look with more time into your comments in the other patches next week.
> > 
> > Thanks,
> > Jose
> > 
> 
> [...]

Powered by blists - more mailing lists