mbox series

[v4,00/14] drm/vkms: Allow to configure device

Message ID 20250218101214.5790-1-jose.exposito89@gmail.com (mailing list archive)
Headers show
Series drm/vkms: Allow to configure device | expand

Message

José Expósito Feb. 18, 2025, 10:12 a.m. UTC 
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.

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.

I'll send the ConfigFS code [3] and its IGT tests [4] code this week.
Meanwhile, the IGT tests also exercise this series and can be used for
additional test coverage.

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

Changes in v4:

  - Applied review comments by Louis Chauvet: (thanks!!)
    - Do not access "private" struct fields in tests
    - Reviewed-by in all patches
  - Rebased on top of drm-misc-next
  - Link to v3: https://lore.kernel.org/dri-devel/20250217100120.7620-1-jose.exposito89@gmail.com/T/

Changes in v3:

  - Applied review comments by Louis Chauvet:
    - Moved drm_vblank_init(..., vkms_config_get_num_crtcs()) to a different patch
    - Improved error handling in vkms_config_*_attach_*() functions
    - Fixed compilation errors when CONFIG_DRM_VKMS_KUNIT_TEST=m
    - Used for_each iterators in vkms_config_test_default_config(). A leftover
      from the removed connector hot-add code
    - Validated that vkms_config.dev wasn't NULL before calling drm_info()
    - Kept vkms_device.config as a const pointer (at the cost of casting in
      list_count_nodes())
  - Renamed "CONFIG_DRM_VKMS_KUNIT_TESTS" to "CONFIG_DRM_VKMS_KUNIT_TEST" for
    consistency with other options 
  - Rebased on top of drm-misc-next
  - Link to v2: https://lore.kernel.org/all/20250211110912.15409-1-jose.exposito89@gmail.com/

Changes in v2:

  - Applied review comments by Louis Chauvet:
    - Added Co-developped-by tags where required
    - Split the first patch in 2: vkms_connector.h/c creation and struct
      vkms_connector creation
    - Fixes a user after free/double free bug. The bug is already present
      in drm-misc-next and the fix could be merged independently
    - Implemented for_each macros for the config
    - Other naming fixes
  - Link to v1: https://lore.kernel.org/all/20250129110059.12199-1-jose.exposito89@gmail.com/

José Expósito (13):
  drm/vkms: Extract vkms_connector header
  drm/vkms: Create vkms_connector struct
  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

 .clang-format                                 |   7 +
 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 | 951 ++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_config.c            | 640 ++++++++++++
 drivers/gpu/drm/vkms/vkms_config.h            | 437 ++++++++
 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            | 176 ++--
 13 files changed, 2246 insertions(+), 141 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

Comments

Louis Chauvet March 4, 2025, 9:43 a.m. UTC | #1 
Le 18/02/2025 à 11:12, José Expósito a écrit :
> 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.
> 
> 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.
> 
> I'll send the ConfigFS code [3] and its IGT tests [4] code this week.
> Meanwhile, the IGT tests also exercise this series and can be used for
> additional test coverage.
> 
> Best wishes,
> José Expósito

Hi all,

I am trying to apply this series but I am encountering some issues. Most 
of the patches have the following trailers:

Co-developed-by: Louis Chauvet
Signed-off-by: Louis Chauvet
Signed-off-by: José Exposito

When I use dim b4-shazam, the result is:

Co-developed-by: Louis Chauvet
Signed-off-by: José Exposito
Signed-off-by: Louis Chauvet

However, checkpatch is not happy with this because it removes my 
Signed-off-by line immediately after Co-developed-by.

I then tried to add it myself:

Co-developed-by: Louis Chauvet
Signed-off-by: Louis Chauvet
Signed-off-by: José Exposito
Signed-off-by: Louis Chauvet

But this time, checkpatch complains that I have my Signed-off-by line twice.

And if I don't add the last Signed-off-by, checkpatch is indeed not 
happy too:

Co-developed-by: Louis Chauvet
Signed-off-by: Louis Chauvet
Signed-off-by: José Exposito

How should I proceed in this situation? Should I ignore some warnings? 
If so, what are the expected trailers?

Thanks!

> [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
> 
> Changes in v4:
> 
>    - Applied review comments by Louis Chauvet: (thanks!!)
>      - Do not access "private" struct fields in tests
>      - Reviewed-by in all patches
>    - Rebased on top of drm-misc-next
>    - Link to v3: https://lore.kernel.org/dri-devel/20250217100120.7620-1-jose.exposito89@gmail.com/T/
> 
> Changes in v3:
> 
>    - Applied review comments by Louis Chauvet:
>      - Moved drm_vblank_init(..., vkms_config_get_num_crtcs()) to a different patch
>      - Improved error handling in vkms_config_*_attach_*() functions
>      - Fixed compilation errors when CONFIG_DRM_VKMS_KUNIT_TEST=m
>      - Used for_each iterators in vkms_config_test_default_config(). A leftover
>        from the removed connector hot-add code
>      - Validated that vkms_config.dev wasn't NULL before calling drm_info()
>      - Kept vkms_device.config as a const pointer (at the cost of casting in
>        list_count_nodes())
>    - Renamed "CONFIG_DRM_VKMS_KUNIT_TESTS" to "CONFIG_DRM_VKMS_KUNIT_TEST" for
>      consistency with other options
>    - Rebased on top of drm-misc-next
>    - Link to v2: https://lore.kernel.org/all/20250211110912.15409-1-jose.exposito89@gmail.com/
> 
> Changes in v2:
> 
>    - Applied review comments by Louis Chauvet:
>      - Added Co-developped-by tags where required
>      - Split the first patch in 2: vkms_connector.h/c creation and struct
>        vkms_connector creation
>      - Fixes a user after free/double free bug. The bug is already present
>        in drm-misc-next and the fix could be merged independently
>      - Implemented for_each macros for the config
>      - Other naming fixes
>    - Link to v1: https://lore.kernel.org/all/20250129110059.12199-1-jose.exposito89@gmail.com/
> 
> José Expósito (13):
>    drm/vkms: Extract vkms_connector header
>    drm/vkms: Create vkms_connector struct
>    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
> 
>   .clang-format                                 |   7 +
>   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 | 951 ++++++++++++++++++
>   drivers/gpu/drm/vkms/vkms_config.c            | 640 ++++++++++++
>   drivers/gpu/drm/vkms/vkms_config.h            | 437 ++++++++
>   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            | 176 ++--
>   13 files changed, 2246 insertions(+), 141 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
>
Maxime Ripard March 7, 2025, 9:59 a.m. UTC | #2 
On Tue, Mar 04, 2025 at 10:43:12AM +0100, Louis Chauvet wrote:
> 
> 
> Le 18/02/2025 à 11:12, José Expósito a écrit :
> > 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.
> > 
> > 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.
> > 
> > I'll send the ConfigFS code [3] and its IGT tests [4] code this week.
> > Meanwhile, the IGT tests also exercise this series and can be used for
> > additional test coverage.
> > 
> > Best wishes,
> > José Expósito
> 
> Hi all,
> 
> I am trying to apply this series but I am encountering some issues. Most of
> the patches have the following trailers:
> 
> Co-developed-by: Louis Chauvet
> Signed-off-by: Louis Chauvet
> Signed-off-by: José Exposito
> 
> When I use dim b4-shazam, the result is:
> 
> Co-developed-by: Louis Chauvet
> Signed-off-by: José Exposito
> Signed-off-by: Louis Chauvet
> 
> However, checkpatch is not happy with this because it removes my
> Signed-off-by line immediately after Co-developed-by.
> 
> I then tried to add it myself:
> 
> Co-developed-by: Louis Chauvet
> Signed-off-by: Louis Chauvet
> Signed-off-by: José Exposito
> Signed-off-by: Louis Chauvet
> 
> But this time, checkpatch complains that I have my Signed-off-by line twice.
> 
> And if I don't add the last Signed-off-by, checkpatch is indeed not happy
> too:
> 
> Co-developed-by: Louis Chauvet
> Signed-off-by: Louis Chauvet
> Signed-off-by: José Exposito
> 
> How should I proceed in this situation? Should I ignore some warnings? If
> so, what are the expected trailers?

I assume Jose is the author and you're the committer? If so, I guess I'd
expect the second to be the most relevant, but it's going to suck
anyway.

I kind of side-stepped the problem and applied the series

Maxime