Hi Dafna, On 9/19/19 10:32 PM, Dafna Hirschfeld wrote: > This patchset introduces the usage of configfs in order to create vimc devices > with a configured topology. A patch introducing configfs usage was already sent by Helen Koike: > https://patchwork.linuxtv.org/patch/53397/ . The current patch is based on her patch but > suggests a new API for using configfs. > It uses symlinks to represent a link between two entities, an approach already used in the kernel > by usb gadgets composed with configfs to associate usb gadget's functions to its configurations. > For example, a topology of sensor->capture will be created with the following commands: > > CONFIGFS_ROOT=/sys/kernel/config > > mkdir ${CONFIGFS_ROOT}/vimc/mdev/ > mkdir ${CONFIGFS_ROOT}/vimc/mdev/vimc-sensor:sen > mkdir ${CONFIGFS_ROOT}/vimc/mdev/vimc-capture:cap > tree ${CONFIGFS_ROOT} > /configfs/ > `-- vimc > `-- mdev > |-- hotplug > |-- vimc-capture:cap > | `-- pad:sink:0 > `-- vimc-sensor:sen > `-- pad:source:0 > > mkdir ${CONFIGFS_ROOT}/vimc/mdev/vimc-sensor:sen/pad:source:0/to-cap > ln -s ${CONFIGFS_ROOT}/vimc/mdev/vimc-capture:cap/pad:sink:0 ${CONFIGFS_ROOT}/vimc/mdev/vimc-sensor:sen/pad:source:0/to-cap > tree ${CONFIGFS_ROOT} > /configfs/ > `-- vimc > `-- mdev > |-- hotplug > |-- vimc-capture:cap > | `-- pad:sink:0 > `-- vimc-sensor:sen > `-- pad:source:0 > `-- to-cap > |-- enabled > |-- immutable > `-- pad:sink:0 -> ../../../../../vimc/mdev/vimc-capture:cap/pad:sink:0 > > There are several reasons to prefer the symlink approach in order to represent links between entities. > The previous approach in which links are represented with directories of the form 'entity1:pad>-><entity2:pad' > requires userspace to parse the dirctories names in order to understand the topology, while in the symlink > approach userspace needs only to traverse the configfs tree. > Also, the usage of symlinks prevents userspace from creating links between entities that don't exist and also > an entity can't be removed if there is a symlink pointing to it or from it, while in the previous approach the Why can't you remove an entity if there is a symlink pointing to it? In the example above I can remove mdev/vimc-capture:cap just fine. Afterwards the pad:sink:0 symlink will point to a non-existing file, but that's valid symlink behavior. Or is vimc checking internally and prohibits the user from making invalid changes? In any case, this cover letter is a bit confusing and needs to address this in more detail. Regards, Hans > links were created by creating unrelated directories and care had to be taken to ensure consistency. This way > the topology configured from userspace is restricted to always be valid and represent the current topology of > the device. This results in less validation needed in kernel code when plugging the device and less possibility > for mistakes in the userspace side. Last, but not least, using symlinks is the natural way of associating things > in configfs. > > This patch is meant to demonstrate the suggested configfs api and get comments and acceptance/disagreement from > the community. It passes few tests that configure basic topology and streams the capture entities. > Here is the tests script: https://gitlab.collabora.com/dafna/scripts/blob/master/configfs/sym-unit-tests-simple-topo.sh > Further versions will go through more extensive debugging. > > The patchset is rebased on top of v5 of the patchset 'Collapse vimc into single monolithic driver' sent by Shuah Khan > https://lkml.org/lkml/2019/9/17/656 > > Patch 1, was sent by me before as a single patch and is needed for the configfs implementation. > > Patch 2, documents how to use the new configfs api in order to create and set devices topologies. > > Patch 3, only adds the new configfs api code but does not use it yet, so it still creates only the hardcoded device. > > Patch 4, removes the hardcoded device topology and creates devices with topologies configured with the configfs. > > Patch 5, implements indexing for the bus_info field since now there can be more than one vimc device. > > Dafna Hirschfeld (5): > media: vimc: upon streaming, check that the pipeline starts with a > source entity > docs: media: vimc: Documenting vimc topology configuration using > configfs > media: vimc: Add the implementation for the configfs api > media: vimc: use configfs instead of having hardcoded configuration > media: vimc: Add device index to the bus_info > > Documentation/media/v4l-drivers/vimc.dot | 28 +- > Documentation/media/v4l-drivers/vimc.rst | 240 ++++++- > drivers/media/platform/vimc/Kconfig | 9 +- > drivers/media/platform/vimc/Makefile | 2 +- > drivers/media/platform/vimc/vimc-capture.c | 50 +- > drivers/media/platform/vimc/vimc-common.h | 86 ++- > drivers/media/platform/vimc/vimc-configfs.c | 656 ++++++++++++++++++++ > drivers/media/platform/vimc/vimc-configfs.h | 41 ++ > drivers/media/platform/vimc/vimc-core.c | 350 +++++------ > drivers/media/platform/vimc/vimc-debayer.c | 35 +- > drivers/media/platform/vimc/vimc-scaler.c | 35 +- > drivers/media/platform/vimc/vimc-sensor.c | 33 +- > drivers/media/platform/vimc/vimc-streamer.c | 39 +- > 13 files changed, 1289 insertions(+), 315 deletions(-) > create mode 100644 drivers/media/platform/vimc/vimc-configfs.c > create mode 100644 drivers/media/platform/vimc/vimc-configfs.h >
