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@xxxxxxxxxxx/ > > [2] https://patchwork.kernel.org/project/dri-devel/cover/20240813105134.17439-1-jose.exposito89@xxxxxxxxx/ > > [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 > >
