| Message ID | 20240813105134.17439-1-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | VKMS: Add configfs support | expand |
Le 13/08/24 - 12:44, José Expósito a écrit : > Hi everyone, Hi José, > This RFC implements support to configure VKMS using configfs. > It allows to: > > - Create multiple devices > - Configure multiple overlay planes, CRTCs, encoders and > connectors > - Enable or disable cursor plane and writeback connector for > each CRTC > - Hot-plug/unplug connectors after device creation > - Disable the creation of the default VKMS instance to be > able to use only the configfs ones > > This work is based on a previous attempt to implement configfs > support by Jim Shargo and Brandon Pollack [1]. > I tried to keep the changes as minimal and simple as possible > and addressed Sima's comments on [1]. > > Currently, there is another RFC by Louis Chauvet [2]. As I > mentioned on his RFC, I'm not trying to push my implementation. > Instead, I think that having 2 implementations will make code > review way easier and I don't mind which implementation is used > as long as we get the feature implemented :) I will send few series tomorrow, don't panic, there will be 9 series and a total of ~50 commits (I have many conflict to rebase only the configFS part, and even if it was easy, I plan to submit all of my work, not everything will be RFC). > I'm looking forward to analyzing Louis's implementation, seeing > what the differences are and finding a common solution. There are four main differences: - I complelty splitted vkms_config and vkms_configfs structures - I splitted my work in many different series - I created a real platform device driver - I did not manage index by hand, I let drm core doing it - I used list to link crtc/planes/encoders and not bitfield (because of the previous point) - The primary and cursor planes are fully configurable The first two points are personnal preferences, so I am open to discussion. The third point was already discussed before, I don't know if it is a good solution or not. I think it should be easy to remove it. But for the index managment, I really think that for our usage in ConfigFS, bitfields are not a good solution and as shown in this series, very error-prone. If you have a better solution than what I did, let me know, I am not very happy with mine too. The last point is also important, we don't want to break uAPI once this series is merged, so having "default hidden planes" that can't be configured is annoying as we will have to manage them with a special case. > What's missing? > > - DebugFS only works for the default VKMS instance. > If we want to support it on instances created with configfs > I'll need to implement it. Same on my side, I forgot to reimplement this :-). It will not be in my RFC, but on the v1 for sure! > Known bugs: > > - When a CRTC is added and removed before device creation, there > is a vblank warning. > The issue is caused because vblanks are referenced using the > CRTC index but, because one of the CRTCs is removed, the > indices are not consecutives and drm_crtc_vblank_crtc() tries to > access and invalid index > I'm not sure if CRTC's indices *must* start at 0 and be > consecutives or if this is a bug in the drm_crtc_vblank_crtc() > implementation. Very nice work, but you hurted many issue I had too, and I attempted to solve them as nicely as I can. Overall there is one main issues for me: the crtc index managment is not correct and the configfs behavior is very easily broken because of this. This is an issue for two reason I think: - We are trying to implement a new index allocation mecanism, but it is not very difficult to let drm manage this part on device creation, so maybe just dont store indexes in config - The usage of a simple index++ is not suitable for configFS usecase, crating 32 crtcs and deleting 1 should be possible: mkdir {1..32};rmdir 1;mkdir 1 but the index of 1 is now 33, which is forbidden by drm, so you have to do a "complex" algorithim "find_first_value_not_used_bellow_32". Thanks for all your work! You were right, while reviewing your work, I found issues in mine :-) Have a nice day, Louis Chauvet > > Best wishes, > José Expósito > > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=780110&archive=both > [2] https://lore.kernel.org/dri-devel/ZrZZFQW5RiG12ApN@louis-chauvet-laptop/T/#u > > José Expósito (17): > 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 CRTCs > drm/vkms: Use managed memory to create encoders > drm/vkms: Allow to configure multiple encoders > drm/vkms: Use managed memory to create connectors > drm/vkms: Allow to configure multiple connectors > drm/vkms: Allow to configure multiple overlay planes > drm/vkms: Allow to change connector status > drm/vkms: Add and remove VKMS instances via configfs > drm/vkms: Allow to configure multiple CRTCs via configfs > drm/vkms: Allow to configure multiple encoders via configfs > drm/vkms: Allow to configure multiple encoders > drm/vkms: Allow to configure multiple planes via configfs > drm/vkms: Allow to configure the default device creation > drm/vkms: Remove completed task from the TODO list > > Documentation/gpu/vkms.rst | 102 +++- > drivers/gpu/drm/vkms/Kconfig | 1 + > drivers/gpu/drm/vkms/Makefile | 4 +- > drivers/gpu/drm/vkms/vkms_composer.c | 30 +- > drivers/gpu/drm/vkms/vkms_config.c | 265 ++++++++++ > drivers/gpu/drm/vkms/vkms_config.h | 101 ++++ > drivers/gpu/drm/vkms/vkms_configfs.c | 721 ++++++++++++++++++++++++++ > drivers/gpu/drm/vkms/vkms_configfs.h | 9 + > drivers/gpu/drm/vkms/vkms_crtc.c | 99 ++-- > drivers/gpu/drm/vkms/vkms_drv.c | 75 ++- > drivers/gpu/drm/vkms/vkms_drv.h | 52 +- > drivers/gpu/drm/vkms/vkms_output.c | 187 ++++--- > drivers/gpu/drm/vkms/vkms_plane.c | 6 +- > drivers/gpu/drm/vkms/vkms_writeback.c | 27 +- > 14 files changed, 1464 insertions(+), 215 deletions(-) > 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_configfs.c > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h > > -- > 2.46.0 >
Hi José, On Tue, 13 Aug 2024 at 11:51, José Expósito <jose.exposito89@gmail.com> wrote: > - When a CRTC is added and removed before device creation, there > is a vblank warning. > The issue is caused because vblanks are referenced using the > CRTC index but, because one of the CRTCs is removed, the > indices are not consecutives and drm_crtc_vblank_crtc() tries to > access and invalid index > I'm not sure if CRTC's indices *must* start at 0 and be > consecutives or if this is a bug in the drm_crtc_vblank_crtc() > implementation. CRTCs and planes are not hotpluggable. I recommend you just create a lot of each of them statically at startup, and hotplug only connectors. Cheers, Daniel
Hi Louis, Thanks a lot for the review and for your comments! On Tue, Aug 13, 2024 at 07:58:55PM +0200, Louis Chauvet wrote: > Le 13/08/24 - 12:44, José Expósito a écrit : > > Hi everyone, > > Hi José, > > > This RFC implements support to configure VKMS using configfs. > > It allows to: > > > > - Create multiple devices > > - Configure multiple overlay planes, CRTCs, encoders and > > connectors > > - Enable or disable cursor plane and writeback connector for > > each CRTC > > - Hot-plug/unplug connectors after device creation > > - Disable the creation of the default VKMS instance to be > > able to use only the configfs ones > > > > This work is based on a previous attempt to implement configfs > > support by Jim Shargo and Brandon Pollack [1]. > > I tried to keep the changes as minimal and simple as possible > > and addressed Sima's comments on [1]. > > > > Currently, there is another RFC by Louis Chauvet [2]. As I > > mentioned on his RFC, I'm not trying to push my implementation. > > Instead, I think that having 2 implementations will make code > > review way easier and I don't mind which implementation is used > > as long as we get the feature implemented :) > > I will send few series tomorrow, don't panic, there will be 9 series and a > total of ~50 commits (I have many conflict to rebase only the configFS > part, and even if it was easy, I plan to submit all of my work, not > everything will be RFC). Nice work! I already reviewed "drm/vkms: Miscelanious clarifications", "drm/vkms: Completly split headers" and "drm/vkms: Switch all vkms object to DRM managed objects". I'll have a look to the vkms_config and configfs as soon as possible, but I think they'll have to wait until next week. > > I'm looking forward to analyzing Louis's implementation, seeing > > what the differences are and finding a common solution. > > There are four main differences: > - I complelty splitted vkms_config and vkms_configfs structures I considered this and I didn't do it because it duplicates a lot of fields, but your implementation does the right think. I need to split both structures. > - I splitted my work in many different series Which is really nice, I think that the 3 series I reviewed can be easily rebased on drm-misc-next and get merged. I'd be able to drop at least 4 patches if your code is merged. > - I created a real platform device driver > - I did not manage index by hand, I let drm core doing it > - I used list to link crtc/planes/encoders and not bitfield (because of > the previous point) I need to look about your solution for the indices. It is the root cause for the problems I'm having with vblanks not being referenced by the correct index. > - The primary and cursor planes are fully configurable I think this is the right approach. However, there are some restrictions on primary planes and a user will be able to create some invalid setups. The DRM subsystem complains if you create a CRTC without a primary plane, but I'm not sure if we want to do the validation in VKMS as well. There are other restrictions handled by DRM (for example, only 32 encoders and CRTCs are allowed) and I don't know if re-implement all validation rules in VKMS is the best idea. > The first two points are personnal preferences, so I am open to > discussion. > > The third point was already discussed before, I don't know if it is a good > solution or not. I think it should be easy to remove it. > > But for the index managment, I really think that for our usage > in ConfigFS, bitfields are not a good solution and as shown in this > series, very error-prone. If you have a better solution than what I did, > let me know, I am not very happy with mine too. > > The last point is also important, we don't want to break uAPI once this > series is merged, so having "default hidden planes" that can't be > configured is annoying as we will have to manage them with a special case. > > > What's missing? > > > > - DebugFS only works for the default VKMS instance. > > If we want to support it on instances created with configfs > > I'll need to implement it. > > Same on my side, I forgot to reimplement this :-). It will not be in my > RFC, but on the v1 for sure! > > > Known bugs: > > > > - When a CRTC is added and removed before device creation, there > > is a vblank warning. > > The issue is caused because vblanks are referenced using the > > CRTC index but, because one of the CRTCs is removed, the > > indices are not consecutives and drm_crtc_vblank_crtc() tries to > > access and invalid index > > I'm not sure if CRTC's indices *must* start at 0 and be > > consecutives or if this is a bug in the drm_crtc_vblank_crtc() > > implementation. > > Very nice work, but you hurted many issue I had too, and I attempted to > solve them as nicely as I can. Overall there is one main issues for me: > the crtc index managment is not correct and the configfs behavior is very > easily broken because of this. > > This is an issue for two reason I think: > - We are trying to implement a new index allocation mecanism, but it is > not very difficult to let drm manage this part on device creation, so > maybe just dont store indexes in config > - The usage of a simple index++ is not suitable for configFS usecase, > crating 32 crtcs and deleting 1 should be possible: > mkdir {1..32};rmdir 1;mkdir 1 > but the index of 1 is now 33, which is forbidden by drm, so you have to > do a "complex" algorithim "find_first_value_not_used_bellow_32". > > Thanks for all your work! You were right, while reviewing your work, I > found issues in mine :-) > > Have a nice day, > Louis Chauvet I ran out of time this week to work on VKMS, but I'll try to solve the issues you pointed out. Thanks again for your review, Jose > > > > Best wishes, > > José Expósito > > > > [1] https://patchwork.kernel.org/project/dri-devel/list/?series=780110&archive=both > > [2] https://lore.kernel.org/dri-devel/ZrZZFQW5RiG12ApN@louis-chauvet-laptop/T/#u > > > > José Expósito (17): > > 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 CRTCs > > drm/vkms: Use managed memory to create encoders > > drm/vkms: Allow to configure multiple encoders > > drm/vkms: Use managed memory to create connectors > > drm/vkms: Allow to configure multiple connectors > > drm/vkms: Allow to configure multiple overlay planes > > drm/vkms: Allow to change connector status > > drm/vkms: Add and remove VKMS instances via configfs > > drm/vkms: Allow to configure multiple CRTCs via configfs > > drm/vkms: Allow to configure multiple encoders via configfs > > drm/vkms: Allow to configure multiple encoders > > drm/vkms: Allow to configure multiple planes via configfs > > drm/vkms: Allow to configure the default device creation > > drm/vkms: Remove completed task from the TODO list > > > > Documentation/gpu/vkms.rst | 102 +++- > > drivers/gpu/drm/vkms/Kconfig | 1 + > > drivers/gpu/drm/vkms/Makefile | 4 +- > > drivers/gpu/drm/vkms/vkms_composer.c | 30 +- > > drivers/gpu/drm/vkms/vkms_config.c | 265 ++++++++++ > > drivers/gpu/drm/vkms/vkms_config.h | 101 ++++ > > drivers/gpu/drm/vkms/vkms_configfs.c | 721 ++++++++++++++++++++++++++ > > drivers/gpu/drm/vkms/vkms_configfs.h | 9 + > > drivers/gpu/drm/vkms/vkms_crtc.c | 99 ++-- > > drivers/gpu/drm/vkms/vkms_drv.c | 75 ++- > > drivers/gpu/drm/vkms/vkms_drv.h | 52 +- > > drivers/gpu/drm/vkms/vkms_output.c | 187 ++++--- > > drivers/gpu/drm/vkms/vkms_plane.c | 6 +- > > drivers/gpu/drm/vkms/vkms_writeback.c | 27 +- > > 14 files changed, 1464 insertions(+), 215 deletions(-) > > 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_configfs.c > > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h > > > > -- > > 2.46.0 > > > > -- > Louis Chauvet, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hi Daniel, Thanks a lot for looking into this. On Wed, Aug 14, 2024 at 10:10:39AM +0100, Daniel Stone wrote: > Hi José, > > On Tue, 13 Aug 2024 at 11:51, José Expósito <jose.exposito89@gmail.com> wrote: > > - When a CRTC is added and removed before device creation, there > > is a vblank warning. > > The issue is caused because vblanks are referenced using the > > CRTC index but, because one of the CRTCs is removed, the > > indices are not consecutives and drm_crtc_vblank_crtc() tries to > > access and invalid index > > I'm not sure if CRTC's indices *must* start at 0 and be > > consecutives or if this is a bug in the drm_crtc_vblank_crtc() > > implementation. > > CRTCs and planes are not hotpluggable. I recommend you just create a > lot of each of them statically at startup, and hotplug only > connectors. Yes, it is an issue creating them before the device is active. Once the VKMS device is active, it is not possible to delete them. Because of how the CRTC index is handled, it is possible create 3 CRTCs (indices 0, 1 and 2), delete the second one and end up with 2 CRTCs: The first one with index 0 and the second one with index 2. This is handled nicelly in the possible_crtcs bitmask, but drm_crtc_vblank_crtc() tries to access index 2 of an array of size 2. This case is not possible with actual HW, so I need to fix it on the VKMS side and make indices start at 0 and be consecutive. A check on the drm_crtc_vblank_crtc() side won't hurt us though. For extra context, see Louis message on the topic. It looks like we are having similar issues: https://lore.kernel.org/dri-devel/ZsS7x2y_HKgqGUFR@fedora/T/#mccf9a9748ae67a07a7e6ad694c42afc2ccd3c7f1 Best wishes, Jose > Cheers, > Daniel