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

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

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.

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.

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

I'm still trying to make them work, but I think they are what we need.

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

> Thanks again for this series,
> Louis Chauvet

I'll look with more time into your comments in the other patches next week.

Thanks,
Jose

> > Best wishes,
> > José Expósito
> > 
> > [1] https://patchwork.kernel.org/project/dri-devel/cover/20250121-google-remove-crtc-index-from-parameter-v3-0-cac00a3c3544@bootlin.com/
> > [2] https://patchwork.kernel.org/project/dri-devel/cover/20240813105134.17439-1-jose.exposito89@gmail.com/
> > [3] https://github.com/JoseExposito/linux/commits/patch-vkms-configfs/
> > [4] https://gitlab.freedesktop.org/jexposit/igt-gpu-tools/-/commits/vkms-configfs
> > 
> > José Expósito (12):
> >   drm/vkms: Extract vkms_connector header
> >   drm/vkms: Add KUnit test scaffolding
> >   drm/vkms: Extract vkms_config header
> >   drm/vkms: Move default_config creation to its own function
> >   drm/vkms: Set device name from vkms_config
> >   drm/vkms: Allow to configure multiple planes
> >   drm/vkms: Allow to configure multiple CRTCs
> >   drm/vkms: Allow to attach planes and CRTCs
> >   drm/vkms: Allow to configure multiple encoders
> >   drm/vkms: Allow to attach encoders and CRTCs
> >   drm/vkms: Allow to configure multiple connectors
> >   drm/vkms: Allow to attach connectors and encoders
> > 
> > Louis Chauvet (1):
> >   drm/vkms: Add a validation function for VKMS configuration
> > 
> >  drivers/gpu/drm/vkms/Kconfig                  |  15 +
> >  drivers/gpu/drm/vkms/Makefile                 |   5 +-
> >  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
> >  drivers/gpu/drm/vkms/tests/Makefile           |   3 +
> >  drivers/gpu/drm/vkms/tests/vkms_config_test.c | 782 +++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_config.c            | 784 ++++++++++++++++++
> >  drivers/gpu/drm/vkms/vkms_config.h            | 479 +++++++++++
> >  drivers/gpu/drm/vkms/vkms_connector.c         |  61 ++
> >  drivers/gpu/drm/vkms/vkms_connector.h         |  26 +
> >  drivers/gpu/drm/vkms/vkms_drv.c               |  45 +-
> >  drivers/gpu/drm/vkms/vkms_drv.h               |  17 +-
> >  drivers/gpu/drm/vkms/vkms_output.c            | 255 ++++--
> >  12 files changed, 2337 insertions(+), 139 deletions(-)
> >  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
> >  create mode 100644 drivers/gpu/drm/vkms/tests/Makefile
> >  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_config_test.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_config.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_config.h
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_connector.c
> >  create mode 100644 drivers/gpu/drm/vkms/vkms_connector.h
> > 
> > -- 
> > 2.48.1
> > 

Powered by blists - more mailing lists