| Message ID | 20250129110059.12199-1-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/vkms: Allow to configure device | expand |
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é, 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) - usage of "complex" list accessors, can't we just create iterators? - should we use pr_err in vkms_config_valid? > 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. > 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. Thanks again for this series, Louis Chauvet > 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 > > 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 >
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@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 > > > > 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 > >
On 31/01/25 - 10:31, José Expósito wrote: > 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. So, I think create/destroy is a bit better: `create` with the `vkms_config` parameter is enough to tell "it allocates stuff in `vkms_config`". > > - 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. In one of my comment I proposed a macro to help on this point. I think this is sufficient to hide internals. (see patch 7/13) > 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. That something I missed, and a very good point. I will try to create a macro that do the filtered iteration, if I succeed and you agree on the previous point, I think it does not worth it to have those huge amount of code just to iterate over a list. > 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. I was not clear: I agree we want some logs, but pr_err is too agressive (see patch 8/13). > > > 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/ Ho! This is exactly one issue I had when developping IGT tests, sometimes you fetch the connector list, and when querying info about a specific connector you get nothing! > I'm still trying to make them work, but I think they are what we need. After reading the patch "https://patchwork.freedesktop.org/patch/628418/", I don't think we really need to support "dynamic connector creation" right now: - None of the existing driver do it (except MST, but MST need a lot of stuff to works) - If we want to support it later, just create a "useless" /configfs/vkms/DEV/connectors/CON/enable that you must write 1 before the device initialization to make the connector working. This way we will not have to break the UAPI (the file is already there, disabled by default) > 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 :) I will look at it next week (same repo/branch?). Have a nice weekend, Louis Chauvet > > > Thanks again for this series, > > Louis Chauvet > > I'll look with more time into your comments in the other patches next week. > > Thanks, > Jose > [...]
On Fri, Jan 31, 2025 at 02:02:14PM +0100, Louis Chauvet wrote: > On 31/01/25 - 10:31, José Expósito wrote: > > 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. > > So, I think create/destroy is a bit better: `create` with the > `vkms_config` parameter is enough to tell "it allocates stuff in > `vkms_config`". ACK, I'll change in v2. > > > - 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. > > In one of my comment I proposed a macro to help on this point. I think > this is sufficient to hide internals. (see patch 7/13) That's a very good point, I like the vkms_config_for_each_plane() macro you suggested. Also, it matches really well with similar macros in drm, for example drm_connector_for_each_possible_encoder(). I'll add them in v2. > > 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. > > That something I missed, and a very good point. > > I will try to create a macro that do the filtered iteration, if I succeed > and you agree on the previous point, I think it does not worth it to have > those huge amount of code just to iterate over a list. > > > 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. > > I was not clear: I agree we want some logs, but pr_err is too > agressive (see patch 8/13). Agreed, we can lower to warn. > > > > 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/ > > Ho! This is exactly one issue I had when developping IGT tests, sometimes > you fetch the connector list, and when querying info about a specific > connector you get nothing! I'm not sure if it is the same issue, but when it happened to me, I was missing a call to drm_mode_config_reset(). Credit to Maxime for helping me with this error :) > > I'm still trying to make them work, but I think they are what we need. > > After reading the patch "https://patchwork.freedesktop.org/patch/628418/", > I don't think we really need to support "dynamic connector creation" right > now: > - None of the existing driver do it (except MST, but MST need a lot of > stuff to works) > - If we want to support it later, just create a "useless" > /configfs/vkms/DEV/connectors/CON/enable that you must write 1 before > the device initialization to make the connector working. This way we > will not have to break the UAPI (the file is already there, disabled by > default) Fair enough, we can add it in the future. To keep it backwars compatible though, we'll need to enable by default "static" connectors so users don't need to change the way they create devices. For "dynamic" connectors (connectors created once the device is enabled) we would need to set them disabled on creation, but this would be backwards compatible, as we won't allow creating new connectors in the first version. I'll remove the connector hot-add/remove on v2... Meaning that configfs + IGT will be ready for review :) > > 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 :) > > I will look at it next week (same repo/branch?). Yes, same repo. Enjoy your weekend! Jose > Have a nice weekend, > Louis Chauvet > > > > > > Thanks again for this series, > > > Louis Chauvet > > > > I'll look with more time into your comments in the other patches next week. > > > > Thanks, > > Jose > > > > [...]