Hi Louis, Thanks a lot for the super fast review, you are the best! On Tue, Feb 25, 2025 at 12:01:58PM +0100, Louis Chauvet wrote: > On 18/02/25 - 18:07, José Expósito wrote: > > Hi everyone, > > > > This series, to be applied on top of [1], allow to configure one or more VKMS > > instances without having to reload the driver using configfs. > > > > The series is structured in 3 blocks: > > > > - Patches 1..11: Basic device configuration. For simplicity, I kept the > > available options as minimal as possible. > > Thanks for this series, it is really nice! > > I did some review, that can be sumarized in two point: > - Do we want to use scoped_guard? Since all mutex locks were unlock at the end of the file, I replaced mutex_lock/unlock with guard(mutex)(...). The resulting code is now much cleaner. Thanks for pointing me to cleanup.h, my Linux kernel books are too old to include these helpers and I wasn't aware of them. > - -EPERM, -EINVAL or -EBUSY when attempting to do something while the > device is enabled I replaced all the cases with EBUSY, thanks! > > - Patches 12, 13 and 14: Allow to hot-plug and unplug connectors. This is not > > part of the minimal set of options, but I included in this series so it can > > be used as a template/example of how new configurations can be added. > > > > - Patches 15 and 16: New option to skip the default device creation and to-do > > cleanup. > > For the next iteration, can you move those patch before 12, 13, 14, so > 1..11 could be merged before 12..14 (I need to think about the connector > API to check if it will works with dynamic connector)? Sure, I moved them to the end in v2. I experimented with dynamic connectors a little bit and I think that it is possible to make it backwards compatible: - We could add a "enabled" file for connectors - At the moment, connectors can only be created while the device is disabled. To keep compatibility, if the device is disabled, we need to set connector->enabled to 1 by default. - If the device is enabled, we'd need to set connector->enabled to 0. This would allow the user to configure the connector before it is hot-added. - We'd need to store if the connector is static or dynamic to allow hot-remove only dynamic connectors. I believe that, if we implemented it like this, we'd be able to keep compatibility. > > The process of configuring a VKMS device is documented in "vkms.rst". > > This is a really good documentation! > > > Finally, the code is thoroughly tested by a collection of IGT tests [2]. > > I quickly looked on them, it seems nice! Thank you for this huge > improvment! If you could comment on that mailing list, I'm sure that a LGTM from the maintainer will help :) Thanks a lot for your work Louis. Sending v2, Jose > Louis Chauvet > > > Best wishes, > > José Expósito > > > > [1] https://lore.kernel.org/all/20250218101214.5790-1-jose.exposito89@xxxxxxxxx/ > > [2] It is not in patchwork yet, but it'll appear here eventually: > > https://patchwork.freedesktop.org/project/igt/patches/?submitter=19782&state=*&q=&archive=both&delegate= > > > > José Expósito (16): > > drm/vkms: Expose device creation and destruction > > drm/vkms: Add and remove VKMS instances via configfs > > drm/vkms: Allow to configure multiple planes via configfs > > drm/vkms: Allow to configure the plane type via configfs > > drm/vkms: Allow to configure multiple CRTCs via configfs > > drm/vkms: Allow to configure CRTC writeback support via configfs > > drm/vkms: Allow to attach planes and CRTCs via configfs > > drm/vkms: Allow to configure multiple encoders via configfs > > drm/vkms: Allow to attach encoders and CRTCs via configfs > > drm/vkms: Allow to configure multiple connectors via configfs > > drm/vkms: Allow to attach connectors and encoders via configfs > > drm/vkms: Allow to configure connector status > > drm/vkms: Allow to update the connector status > > drm/vkms: Allow to configure connector status via configfs > > drm/vkms: Allow to configure the default device creation > > drm/vkms: Remove completed task from the TODO list > > > > Documentation/gpu/vkms.rst | 98 +- > > drivers/gpu/drm/vkms/Kconfig | 1 + > > drivers/gpu/drm/vkms/Makefile | 3 +- > > drivers/gpu/drm/vkms/tests/vkms_config_test.c | 24 + > > drivers/gpu/drm/vkms/vkms_config.c | 8 +- > > drivers/gpu/drm/vkms/vkms_config.h | 26 + > > drivers/gpu/drm/vkms/vkms_configfs.c | 918 ++++++++++++++++++ > > drivers/gpu/drm/vkms/vkms_configfs.h | 8 + > > drivers/gpu/drm/vkms/vkms_connector.c | 26 +- > > drivers/gpu/drm/vkms/vkms_connector.h | 18 +- > > drivers/gpu/drm/vkms/vkms_drv.c | 18 +- > > drivers/gpu/drm/vkms/vkms_drv.h | 4 + > > drivers/gpu/drm/vkms/vkms_output.c | 2 +- > > 13 files changed, 1138 insertions(+), 16 deletions(-) > > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c > > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h > > > > > > base-commit: 9b6c03cb96b9e19bce2c2764d2c6dd4ccbd06c5d > > prerequisite-patch-id: 1bff7bbc4ef0e29266265ac3dc009011c046f745 > > prerequisite-patch-id: 74a284d40a426a0038a7054068192238f7658187 > > prerequisite-patch-id: c3e34e88ad6a0acf7d9ded0cdb4745a87cf6fd82 > > prerequisite-patch-id: 9cd0dfaf8e21a811edbe5a2da7185b6f9055d42d > > prerequisite-patch-id: f50c41578b639370a5d610af6f25c2077321a886 > > prerequisite-patch-id: 5a7219a51e42de002b8dbf94ec8af96320043489 > > prerequisite-patch-id: 67ea5d4e21b4ce4acbd6fc3ce83017f55811c49b > > prerequisite-patch-id: 37a7fab113a32581f053c09f45efb137afd75a1b > > prerequisite-patch-id: 475bcdc6267f4b02fb1bb2379145529c33684e4f > > prerequisite-patch-id: d3114f0b3da3d8b5ad64692df761f1cf42fbdf12 > > prerequisite-patch-id: d1d9280fb056130df2050a09b7ea7e7ddde007c5 > > prerequisite-patch-id: 2c370f3de6d227fa8881212207978cce7bbb18ba > > prerequisite-patch-id: 938b8fe5437e5f7bc22bffc55ae249a27d399d66 > > prerequisite-patch-id: ab0a510994fbe9985dc46a3d35e6d0574ddbb633 > > -- > > 2.48.1 > >
