Re: [PATCH v9 07/28] rcar-vin: change name of video device

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Niklas,

On Monday, 8 January 2018 18:42:05 EET Niklas Söderlund wrote:
> On 2018-01-08 18:35:23 +0200, Laurent Pinchart wrote:
> > On Wednesday, 20 December 2017 17:20:55 EET Niklas Söderlund wrote:
> >> On 2017-12-14 17:50:24 +0200, Laurent Pinchart wrote:
> >>> On Thursday, 14 December 2017 16:25:00 EET Sakari Ailus wrote:
> >>>> On Fri, Dec 08, 2017 at 10:17:36AM +0200, Laurent Pinchart wrote:
> >>>>> On Friday, 8 December 2017 03:08:21 EET Niklas Söderlund wrote:
> >>>>>> The rcar-vin driver needs to be part of a media controller to
> >>>>>> support Gen3. Give each VIN instance a unique name so it can be
> >>>>>> referenced from userspace.
> >>>>>> 
> >>>>>> Signed-off-by: Niklas Söderlund
> >>>>>> <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >>>>>> Reviewed-by: Kieran Bingham
> >>>>>> <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> >>>>>> Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> >>>>>> ---
> >>>>>> 
> >>>>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 3 ++-
> >>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>>>>> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> >>>>>> 59ec6d3d119590aa..19de99133f048960 100644
> >>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>>>>> @@ -876,7 +876,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
> >>>>>>  	vdev->fops = &rvin_fops;
> >>>>>>  	vdev->v4l2_dev = &vin->v4l2_dev;
> >>>>>>  	vdev->queue = &vin->queue;
> >>>>>> -	strlcpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> >>>>>> +	snprintf(vdev->name, sizeof(vdev->name), "%s %s", KBUILD_MODNAME,
> >>>>>> +		 dev_name(vin->dev));
> >>>>> 
> >>>>> Do we need the module name here ? How about calling them "%s
> >>>>> output", dev_name(vin->dev) to emphasize the fact that this is a video
> >>>>> node and not a VIN subdev ? This is what the omap3isp and vsp1 drivers
> >>>>> do.
> >>>>> 
> >>>>> We're suffering a bit from the fact that V4L2 has never standardized
> >>>>> a naming scheme for the devices. It wouldn't be fair to ask you to fix
> >>>>> that as a prerequisite to get the VIN driver merged, but we clearly
> >>>>> have to work on that at some point.
> >>>> 
> >>>> Agreed, this needs to be stable and I think aligning to what omap3isp
> >>>> or vsp1 do would be a good fix here.
> >>> 
> >>> Even omap3isp and vsp1 are not fully aligned, so I think we need to
> >>> design a naming policy and document it.
> >> 
> >> I agree that align this is a good idea. And for this reason I chosen to
> >> update this patch as such:
> >> 
> >> "%s output", dev_name(vin->dev)
> > 
> > Wouldn't it be easier for userspace to use
> > 
> > 	"VIN%u output", index
> > 
> > where index is the VIN index as specified in DT ?
> 
> I would be OK whit this, but do this agree with the idea above that we
> should try to align this name across drivers?

Do you mean does this align with the naming policy that we haven't documented, 
or even designed, yet ? :-)

If you want to start designing that policy, here are a few ideas (that may or 
may not reflect general consensus).

First of all, a few general considerations about the entity name. Entity names 
shall be unique within the context of a media graph. There have been 
discussions in the past to move to a single media graph that would span the 
whole system but no concrete proposal has been submitted. Names thus don't 
have to be unique system-wide at the moment but this constraint should still 
be kept in mind. One option would be to simply prepend all entity names with 
the name of the media device (as defined today) if we later move to a single 
media graph.

Another important point is that entity names should be consumable by both 
machines and humans (at least by developers). They should thus describe the 
entity to some extend. A case could be made to make them parsable by machines 
(for instance using the "input" or "output" suffix to infer both the nature of 
the entity and its direction) but I don't think that's a good idea. A more 
formal description of the entity through structured properties would be a 
better API in my opinion.

It could be argued that an "input" or "output" suffix would duplicate 
information conveyed through those structured properties. While I can't deny 
that, the need to have unique names readable by developers is met by adding 
such suffixes.

Another point to consider is the prefix. Using the device name (as in 
dev_name(dev)) is an option but makes reading the media graph by humans more 
difficult. Using a more ad-hoc naming scheme (that would then be driver-
specific to some extend, as is "VIN%u", index for the VIN driver) would create 
names that are easier to read and interpret, and that would also vary less 
between SoCs of the same family.

I believe there's a balance to consider between readability and avoidance of 
duplicated information. One could argue that using "VIN" in the name would 
duplicate information conveyed at the media device level: only the VIN has 
video nodes, so there's need to make that explicit. Similarly the "OMAP3 ISP" 
prefix in the OMAP3 ISP media graph could be argued to be redundant, and is to 
some extend.

If I had to redesign the OMAP3 ISP entity naming scheme today I would consider 
dropping the "OMAP3 ISP" prefix. All of the ISP entities have a name of their 
own (CCP2, CSI2, CCDC, preview, resizer) that is unique already. The media 
device reports the device name through the info model field as "TI OMAP3 ISP" 
so I could argue that the prefix is not needed.

However, the graph can also contain external entities (mostly camera sensors 
in this case). Keeping the "OMAP3 ISP" prefix for all ISP entities would make 
it clear what the entities belong to, and would also help avoiding duplicate 
names. In the R-Car case we face a similar situation, as the graph contains, 
in addition to external camera sensors and video decoders, CSI-2 receivers 
that are in the SoC but not part of the VIN as such.

One possible way to solve this would be to report an entity hierarchy, or at 
least make it possible to group entities in named groups. The name of the 
group wouldn't need to be duplicated as a prefix in every entity.

> >> I hope this is a step in the correct direction. If not please let me
> >> know as soon as possible so I can minimize the trouble for the other
> >> developers doing stuff on-top of this series and there test scripts :-)

-- 
Regards,

Laurent Pinchart





[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux