Date:   Wed, 27 Sep 2017 15:53:04 -0300
From:   Helen Koike <helen.koike@...labora.com>
To:     Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@....samsung.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Jeremy Gebben <jgebben@...eaurora.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [media] vimc: API proposal, configuring the topology from user
 space

Hi Hans,

Thanks for your review.

On 2017-07-28 10:39 AM, Hans Verkuil wrote:
> 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.
> 

Yes, thanks for noticing, I'll update when including it in the docs of
the API.

>>
>> 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.

I don't think it really matters, the instance would be in unplugged
state anyway and it would need to be configured, but sure, we can add a
n_devs param as you proposed, no problem.

> 
> 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.
> 

So it matches the values that you can read from the file, thanks for
this suggestion.

>> 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?

The idea is to make the testing workflow easier without the need to
remove folders and re-adding them again, lets say you want to test the
device with entity E and without it.

Then if you want to test the device without entity E and without doing
rmdir, we could do:

$ echo unplug > /configfs/vimc/vimc0/hotplug
$ echo unplug >
/configfs/vimc/vimc0/configfs/vimc/vimc0/entities/sensor:E/hotplug
$ echo plug > /configfs/vimc/vimc0/hotplug

And to test it with E present in the device:
$ echo unplug > /configfs/vimc/vimc0/hotplug
$ echo plug >
/configfs/vimc/vimc0/configfs/vimc/vimc0/entities/sensor:E/hotplug
$ echo plug > /configfs/vimc/vimc0/hotplug

So we don't loose our previous configuration, otherwise we would need to
do and rmdir and then a mkdir+configurations_and_links when alternating
between the two stages.

As we don't support hotplug yet, we reject when the user tries to change
the value of hotplug in a specific entity while the
/configfs/vimc/vimc0/hotplug reads 'plugged'.

The file
/configfs/vimc/vimc0/configfs/vimc/vimc0/entities/sensor:E/hotplug just
means: if 'unplugged', then pretend that entity E and all its associated
links don't exist.

Please, let me know if you see any other potential problem not mentioned
above.

Helen

Powered by blists - more mailing lists