Re: [PATCH 00/13] drm/vkms: Allow to configure device

Louis Chauvet
2025-01-31T13:02:18Z

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

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

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

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

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

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