On Mon, Jul 15, 2019 at 11:09:24AM +0000, Vasilev, Oleg wrote: > On Fri, 2019-07-12 at 00:07 -0300, Rodrigo Siqueira wrote: > > On 07/10, Daniel Vetter wrote: > > > On Mon, Jul 01, 2019 at 12:23:39AM -0300, Rodrigo Siqueira wrote: > > > > This patchset introduces the support for configfs in vkms by > > > > adding a > > > > primary structure for handling the vkms subsystem and exposing > > > > connectors as a use case. This series allows enabling/disabling > > > > virtual > > > > and writeback connectors on the fly. The first patch of this > > > > series > > > > reworks the initialization and cleanup code of each type of > > > > connector, > > > > with this change, the second patch adds the configfs support for > > > > vkms. > > > > It is important to highlight that this patchset depends on > > > > https://patchwork.freedesktop.org/series/61738/. > > > > > > > > After applying this series, the user can utilize these features > > > > with the > > > > following steps: > > > > > > > > 1. Load vkms without parameter > > > > > > > > modprobe vkms > > > > > > > > 2. Mount a configfs filesystem > > > > > > > > mount -t configfs none /mnt/ > > > > > > > > After that, the vkms subsystem will look like this: > > > > > > > > vkms/ > > > > |__connectors > > > > |__Virtual > > > > |__ enable > > > > > > > > The connectors directories have information related to > > > > connectors, and > > > > as can be seen, the virtual connector is enabled by default. > > > > Inside a > > > > connector directory (e.g., Virtual) has an attribute named > > > > ‘enable’ > > > > which is used to enable and disable the target connector. For > > > > example, > > > > the Virtual connector has the enable attribute set to 1. If the > > > > user > > > > wants to enable the writeback connector it is required to use the > > > > mkdir > > > > command, as follows: > > > > > > > > cd /mnt/vkms/connectors > > > > mkdir Writeback > > > > > > > > After the above command, the writeback connector will be enabled, > > > > and > > > > the user could see the following tree: > > > > > > > > vkms/ > > > > |__connectors > > > > |__Virtual > > > > | |__ enable > > > > |__Writeback > > > > |__ enable > > > > > > > > If the user wants to remove the writeback connector, it is > > > > required to > > > > use the command rmdir, for example > > > > > > > > rmdir Writeback > > > > > > > > Another way to enable and disable a connector it is by using the > > > > enable > > > > attribute, for example, we can disable the Virtual connector > > > > with: > > > > > > > > echo 0 > /mnt/vkms/connectors/Virtual/enable > > > > > > > > And enable it again with: > > > > > > > > echo 1 > /mnt/vkms/connectors/Virtual/enable > > > > > > > > It is important to highlight that configfs 'obey' the parameters > > > > used > > > > during the vkms load and does not allow users to remove a > > > > connector > > > > directory if it was load via module parameter. For example: > > > > > > > > modprobe vkms enable_writeback=1 > > > > > > > > vkms/ > > > > |__connectors > > > > |__Virtual > > > > | |__ enable > > > > |__Writeback > > > > |__ enable > > > > > > > > If the user tries to remove the Writeback connector with “rmdir > > > > Writeback”, the operation will be not permitted because the > > > > Writeback > > > > connector was loaded with the modules. However, the user may > > > > disable the > > > > writeback connector with: > > > > > > > > echo 0 > /mnt/vkms/connectors/Writeback/enable > > > > Thanks for detail this issue, I just have some few questions inline. > > > > > I guess I should have put a warning into that task that step one is > > > designing the interface. Here's the fundamental thoughts: > > > > > > - The _only_ thing we can hotplug after drm_dev_register() is a > > > drm_connector. That's an interesting use-case, but atm not really > > > supported by the vkms codebase. So we can't just enable/disable > > > writeback like this. We also can't change _anything_ else in the > > > drm > > > driver like this. > > > > In the first patch of this series, I tried to decouple enable/disable > > for virtual and writeback connectors; I tried to take advantage of > > drm_connector_[register/unregister] in each connector. Can we use the > > first patch or it doesn't make sense? No, because you also add the drm_encoder. That cannot be hotadded/removed right now. So you'd need to add a drm_encoder preemptively for all writeback connectors you might need, which doesn't make much sense. > > I did not understand why writeback connectors should not be > > registered > > and unregister by calling drm_connector_[register/unregister], is it > > a > > writeback or vkms limitation? Could you detail why we cannot change > > connectors as I did? > > Hi, > > I guess, some more stuff should happen during the hotplug, like > drm_kms_helper_hotplug_event()? writeback is generally a SoC feature. I don't think anyone expects that you can hotplug a writeback connector. > > > > > Additionally, below you said "enable going from 1 -> 0, needs to be > > treated like a physical hotunplug", do you mean that we first have to > > add support for drm_dev_plug and drm_dev_unplug in vkms? > > > > > - The other bit we want is support multiple vkms instances, to > > > simulate > > > multi-gpus and fun stuff like that. > > > > Do you mean something like this: > > > > configfs/vkms/instance1 > > > _enable_device > > > _more_stuff > > configfs/vkms/instance2 > > > _enable_device > > > _more_stuff > > configfs/vkms/instanceN > > > _enable_device > > > _more_stuff > > I think it would be a good idea. This way the creation of new device > could look like this: > > mkdir -p instanceN/connector/virtual0 > echo "virtual" > instanceN/connector/virtual0/type virtual should be a top-level property. I'm not aware of any real driver where vblank works on some connector/crtc, but not on another one. > echo 1 > instanceN/connector/virtual0/enable For the initial connectors those don't need separate enable properties, they get enabled with the global enable knob. > mkdir -p instanceN/crtc/crtc0 > ... > echo 1 > instanceN/enable > > Once the last command is executed, the whole instanceN/ becomes > immutable, except for > - instanceN/enable, so we can later disable it > - instanceN/connector, where we can create new connectors, it would be > treated as a hotplug. For hotplugged connectors we need a separate enable knob. But we're still _very_ far away from making that possible I think. > > Will each instance work like a totally independent device? What is > > the > > main benefit of this? I can think about some use case for testing > > prime, but I cannot see other things. > > We can test that userspace always select the right device to display. Also hotunplug behaviour. We can take out an entire drm_device, and make sure userspace and the kernel copes correctly. This is _really_ hard to get right on both sides. And it's a real thing with stuff like usb display link. > > > - Therefore vkms configs should be at the drm_device level, so a > > > directory under configfs/vkms/ represents an entire device. > > > > > > - We need a special config item to control > > > drm_dev_register/drm_dev_unregister. While a drm_device is > > > registers, > > > all other config items need to fail if userspace tries to change > > > them. > > > Maybe this should be a top-level "enable" property. > > > > > > - Every time enable goes from 0 -> 1 we need to create a completely > > > new > > > vkms instance. The old one might still be around, waiting for the > > > last > > > few references to disappear. > > > > > > - enable going from 1 -> 0 needs to be treated like a physical > > > hotunplug, > > > i.e. not drm_dev_unregister but drm_dev_unplug. We also need to > > > annotate > > > all the vkms code with drm_dev_enter/exit() as the kerneldoc of > > > drm_dev_unplug explains. > > > > > > - rmdir should be treated like enable going from 1 -> 0. Or maybe > > > we > > > should disable enable every going from 1 -> 0, would propably > > > simplify > > > everything. > > > > > > - The initial instance created at module load also neeeds to be > > > removable > > > like this. > > > > > > Once we have all this, then can we start to convert driver module > > > options > > > over to configs and add cool features. But lots of infrastructure > > > needed > > > first. > > > > > > Also, we probably want some nasty testcases which races an rmdir in > > > configfs against userspace still doing ioctl calls against vkms. > > > This is > > > ideal for validation the hotunplug infrastructure we have in drm. > > > > > > An alternative is adding connector hotplugging. But I think before > > > we do > > > that we need to have support for more crtc and more connectors as > > > static > > > module options. So maybe not a good starting point for configfs. > > > > So, probably the first set of tasks should be: > > > > 1. Enable multiple CRTC via module parameters. For example: > > modprobe vkms crtcs=13 > > 2. Enable multiple connectors via module parameters. For example: > > modprobe vkms crtcs=3 connector=3 // 3 virtual connectors per crtc > > But do we want to have those parameters as module options, if we then > plan to replace those with configfs? Imo we should phase out module options, or at least not add more. Then you can just have a small script which sets up the vkms device you want, and then run the tests. I'd go further even, and say we should have default_device=0 knob to start out with nothing when you load the driver. -Daniel > > > > > Thanks again, > > > > > The above text should probably be added to the vkms.rst todo item > > > ... > > > -Daniel > > > > > > > > > > > Rodrigo Siqueira (2): > > > > drm/vkms: Add enable/disable functions per connector > > > > drm/vkms: Introduce configfs for enabling/disabling connectors > > > > > > > > drivers/gpu/drm/vkms/Makefile | 3 +- > > > > drivers/gpu/drm/vkms/vkms_configfs.c | 229 > > > > ++++++++++++++++++++++++++ > > > > drivers/gpu/drm/vkms/vkms_drv.c | 6 + > > > > drivers/gpu/drm/vkms/vkms_drv.h | 17 ++ > > > > drivers/gpu/drm/vkms/vkms_output.c | 84 ++++++---- > > > > drivers/gpu/drm/vkms/vkms_writeback.c | 31 +++- > > > > 6 files changed, 332 insertions(+), 38 deletions(-) > > > > create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c > > > > > > > > -- > > > > 2.21.0 > > > > > > > > > > > > -- > > > > Rodrigo Siqueira > > > > https://siqueira.tech > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- > Oleg Vasilev <oleg.vasilev@xxxxxxxxx> > Intel Corporation > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
