On 9/20/19 5:07 PM, Dafna Hirschfeld wrote:
> Hi,
>
> On Fri, 2019-09-20 at 15:17 +0200, Hans Verkuil wrote:
>> 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.
>
> Hmm, this should not be allowed, maybe you did something wrong?
> I get:
> # rmdir /sys/kernel/config/vimc/mdev/vimc-capture:cap
> rmdir: failed to remove '/sys/kernel/config/vimc/mdev/vimc-capture:cap':
> Device or resource busy
>
>
>> Or is vimc checking internally and prohibits the user from making invalid
>> changes?
>
> No, this is not internal to vimc, it is part of configfs, it is also written
> in the docs:
>
> "A config_item cannot be removed while it links to any other item, nor
> can it be removed while an item links to it. Dangling symlinks are not
> allowed in configfs."
Ah, I was not aware of that. Just ignore my comments about this, then :-)
Nice, I learned something today.
Regards,
Hans
>
> Regards,
> Dafna
>
>> 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
>>>
>
