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@gmail.com/
> > [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
> >