Hi Helen, Finally after way too long I found some time to review this. See my comments below. On 04/11/2017 12:53 AM, Helen Koike wrote: > > Hi, > > Continuing the discussion about the API of the vimc driver, I made some > changes > based on the previous comments, please see below and let me know your > opinion about it. > > Helen > > /*********************** > Configfs considerations: > ************************/ > Informal definitions: > subsystem: the root driver folder in user space (/configfs/vimc) > item: aka a folder in user space > attributes: aka files in the folder > group: aka a folder that can contain subfolders (parent and child relation) > default group: aka a subfolder that is created automatically when the > "parent" folder is created > it is not considered a child in terms of rmdir > > * Performing rmdir in a group will fail if it contain children that are > not default groups, i.e, if the > folder contain subfolders that are default group, then it can be removed > with rmdir, if the > subfolders were created with mkdir, then rmdir in the parent will fail. > > * Configfs has the notion of committable item but it is not implemented > yet. A committable item is an item > that can be in one of two parent folders called: live and pending. The > idea is to create and modify the item > in the pending directory and then to move the item through a rename to > the live directory where > it can't be modified. This seems to be a nice feature for vimc, but as > it is not available yet the > proposal below won't be based on this. > > * Groups can be dynamically created/destroyed by the driver whenever it > wants. Afaik attributes can only > be created when the group or item is created and symlinks can only be > create from user space, i.e, the > driver don't know how to create/destroy attributes or symlinks in anytime. > > /*********************** > The API: > ************************/ > > In short, a topology like this one: http://goo.gl/Y7eUfu > Would look like this filesystem tree: https://goo.gl/mEOmOf This mentions 'Yellow' lines, but since you dropped symlinks these no longer exist. You probably need to update the legend. > > v3 core changes: > - I removed the use of symlinks as I wans't able to see how to do it nicely. > - I use the names of the folders created by user space to retrieve > information at mkdir time > - hotplug file in each entity > - hotplug file in each device > - reset file in each device > > * The /configfs/vimc subsystem > empty when the driver is loaded I'm not sure about that. I think it would make sense that vimc when loaded would make one instance, unless otherwise told via a module option. Something like this (taken from vivid): parm: n_devs: number of driver instances to create (uint) By default this is 1, but can also be 0, 2, 3, etc. > > * Create a device > Userspace can create a new vimc device with: > > $ mkdir /configfs/vimc/any_name > Example: > $ mkdir /configfs/vimc/vimc0 > $ ls -l /configfs/vimc/vimc0 > hotplug > reset > entities/ > links/ > > entities/ and links/ folder are default groups, thus they don't prevent > rmdir vimc0/, but > rmdir will fail if it has any child inside entities/ or links/. > hotplug is used to plug and unplug the device, it can read "plugged" or > "unplugged" and user can > write "plug" or "unplug" to change its state. I would also support writing "plugged" and "unplugged". I.e. support both variants. > Changing hotplug state will never fail as the configfs tree will always > be in a valid state. > reset is used to easily destroy all the topology without the need to > walk through all the children > to perform rmdir, writing 1 to reset file will set hotplug to > "unplugged" and erase all folders > under entities/ and links/. > > * Create an entity > Userspace can create a new entity with: > > $ mkdir /configfs/vimc/vimc0/entities/<role>:<name> > Example: > $ mkdir /configfs/vimc/vimc0/entities/sensor:SensorA > $ ls -l /configfs/vimc/vimc0/entities/sensor:SensorA > hotplug > pad:source:0/ > > The name of the folder needs to be in the format <role>:<name> or it > will be rejected, this allows the > creation of the right pads according to its role at mkdir time, > eliminating the previously proposed role > and name files. > hotplug is used to plug and unplug the hw block, it can read "plugged" > or "unplugged" and user can > write "plug" or "unplug" to change its state. As we don't support this > yet in the media core, changing it > will only be allowed if /configfs/vimc/vimc0/hotplug is "unplugged". > hotplug file is "unplugged" by default. > Pads will be created as default groups with the name in the format > pad:<direction>:<pad_number> and it > will be an empty folder. > If the hw block supports different number of pads, we could expose two > files: > sinks > sources > where the user space can write the desired number of sink and source > pads and the driver will dynamically > create the folders pad:<direction>:<pad_number> > > * Create a link > User space can create a link between two entities with: > > $ mkdir > /configfs/vimc/vimc0/links/<entity_src_name>:<pad_n>-><entity_sink_name>:<pad_n> > Example: > $ mkdir /configfs/vimc/vimc0/links/DebayerA:1->Scaler:0 > $ ls -l /configfs/vimc/vimc0/links/DebayerA:1->Scaler:0 You need to escape '>' in the examples above. Or better, just put the whole argument in '...' quotes. > flags > > mkdir will be rejected if folder is not on the format > <entity_src_name>:<pad_n>-><entity_sink_name>:<pad_n>. > mkdir will be rejected if either <entity_src_name> or <entity_sink_name> > are not found in /configfs/vimc/vimc0/entities/ > The link will only be created if both entities are in "plugged" state. > When an entity is removed from /configfs/vimc/vimc0/entities/ with > rmdir, its corresponding link folders at > /configfs/vimc/vimc0/links will be automatically removed. > If one of the entities changes from "plugged" to "unplugged", the link > is only removed from the media > representation, the link folder won't be removed. > flags can be one of "", "enabled", "immutable", "dynamic", > "dynamic,enabled". > flags cannot be changed if the link was already created in the media > controller, to alter it unplug the > device through /configfs/vimc/vimc0/hotplug or unplug one of the source > or sink entities connected to the > link through its hotplug file. > flags are of type "" by default. > I'm a bit uncertain about the hotplug part. We don't really support this yet, so why add this to vimc now? Otherwise this seems a sane proposal to me. Regards, Hans
