Message-ID: <Z5zJ1rEZyBEgd7DN@louis-chauvet-laptop>
Date: Fri, 31 Jan 2025 14:02:14 +0100
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: José Expósito <jose.exposito89@...il.com>
Cc: 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 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
> 

[...]

Powered by blists - more mailing lists