Message-ID: <Z74EXo_qL1bZ2uNo@fedora>
Date: Tue, 25 Feb 2025 18:56:46 +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/16] drm/vkms: Add configfs support

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

Powered by blists - more mailing lists