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
>
