Em Tue, 8 Sep 2015 01:18:30 +0300
Sakari Ailus <sakari.ailus@xxxxxx> escreveu:
> Hi Mauro,
>
> A few comments below.
Thanks for review!
>
> On Sun, Sep 06, 2015 at 09:03:09AM -0300, Mauro Carvalho Chehab wrote:
> > Add support for the new MEDIA_IOC_G_TOPOLOGY ioctl, according
> > with the RFC for the MC next generation.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 5b2c9f7fcd45..96a476eeb16e 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -232,6 +232,156 @@ static long media_device_setup_link(struct media_device *mdev,
> > return ret;
> > }
> >
> > +static long __media_device_get_topology(struct media_device *mdev,
> > + struct media_v2_topology *topo)
> > +{
> > + struct media_entity *entity;
> > + struct media_interface *intf;
> > + struct media_pad *pad;
> > + struct media_link *link;
> > + struct media_v2_entity uentity;
> > + struct media_v2_interface uintf;
> > + struct media_v2_pad upad;
> > + struct media_v2_link ulink;
> > + int ret = 0, i;
>
> I think i wants to be unsigned.
Yes, "i" can be unsigned. I'll change that.
>
> > +
> > + topo->topology_version = mdev->topology_version;
> > +
> > + /* Get entities and number of entities */
> > + i = 0;
> > + media_device_for_each_entity(entity, mdev) {
> > + i++;
> > +
> > + if (ret || !topo->entities)
> > + continue;
> > +
> > + if (i > topo->num_entities) {
> > + ret = -ENOSPC;
> > + continue;
> > + }
> > +
> > + /* Copy fields to userspace struct if not error */
> > + memset(&uentity, 0, sizeof(uentity));
> > + uentity.id = entity->graph_obj.id;
> > + strncpy(uentity.name, entity->name,
> > + sizeof(uentity.name));
> > +
> > + if (copy_to_user(&topo->entities[i - 1], &uentity, sizeof(uentity)))
> > + ret = -EFAULT;
> > + }
> > + topo->num_entities = i;
> > +
> > + /* Get interfaces and number of interfaces */
> > + i = 0;
> > + media_device_for_each_intf(intf, mdev) {
> > + i++;
> > +
> > + if (ret || !topo->interfaces)
> > + continue;
> > +
> > + if (i > topo->num_interfaces) {
> > + ret = -ENOSPC;
> > + continue;
> > + }
> > +
> > + memset(&uintf, 0, sizeof(uintf));
> > +
> > + /* Copy intf fields to userspace struct */
> > + uintf.id = intf->graph_obj.id;
> > + uintf.intf_type = intf->type;
> > + uintf.flags = intf->flags;
> > +
> > + if (media_type(&intf->graph_obj) == MEDIA_GRAPH_INTF_DEVNODE) {
> > + struct media_intf_devnode *devnode;
> > +
> > + devnode = intf_to_devnode(intf);
> > +
> > + uintf.devnode.major = devnode->major;
> > + uintf.devnode.minor = devnode->minor;
> > + }
> > +
> > + if (copy_to_user(&topo->interfaces[i - 1], &uintf, sizeof(uintf)))
> > + ret = -EFAULT;
> > + }
> > + topo->num_interfaces = i;
> > +
> > + /* Get pads and number of pads */
> > + i = 0;
> > + media_device_for_each_pad(pad, mdev) {
> > + i++;
> > +
> > + if (ret || !topo->pads)
> > + continue;
> > +
> > + if (i > topo->num_pads) {
> > + ret = -ENOSPC;
> > + continue;
> > + }
> > +
> > + memset(&upad, 0, sizeof(upad));
> > +
> > + /* Copy pad fields to userspace struct */
> > + upad.id = pad->graph_obj.id;
>
> How about the pad index? Shouldn't that be also passed to the user space for
> every pad?
We've agreed to not pass the pad index to userspace at the MC workshop.
There are two aspects here to consider:
a) to properly represent the topology (e. g. TOPOLOGY)
Writing the userspace code to support it, I didn't find any need to
pass it to userspace, as the data links are connected via the PAD object
ID.
The PAD index for userspace is just a number that it uses when
generating the dot graph. See:
https://mchehab.fedorapeople.org/mc-next-gen/au0828.png
This was generated by this tool:
http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/contrib/test/mc_nextgen_test.c
And no pad index was required at all. What the tool does is that it
generates the pad numbers just when the --dot option is used,
at the media_show_graphviz() function.
Also, please notice that having a pad index makes harder to support
dynamic PAD addition/removal, as the pad index numberspace will
be discontinued.
b) using the PAD index to setup a link.
As I argued with Hans on one of his reviews, I don't think that
an index is always the best way to refer to a PAD.
See for example, the entity_1 on the au0828 (ATV decoder). It
has 3 PADs:
- pad 0 - sink - receives an ATV signal [1]
- pad 1 - source - it outputs a Video stream
- pad 2 - source - it outputs a VBI stream
Pads 1 and 2 are not interchangeable, as they carry different
types of data.
[1] Actually, to be honest, pad 0 there is also wrong. In a matter
of fact, Composite, S-Video and Tuner interfaces are actually
supported by 3 different PADs on the hardware. On some hardware,
it is just a software configuration, but on others, different
pins are used for each different input type. We just don't need
to have all those details ATM on the Media Controller data flow,
as the V4L2 input selection API at the /dev/video0 devnode hides
those dirty details. Note however that, if we end by adding support
for ATV decoder subdevice nodes, such level of detail may be
required.
However some other ATV decoder could have mapped it on some
different order, like:
- pad 0 - sink - receives an ATV signal
- pad 1 - source - it outputs a VBI stream
- pad 2 - source - it outputs a Video stream
Any (generic) application would need to check the properties of
pads 1 and 2 in order to identify what of those pads has video
and what pad has VBI. The pad index is meaningless.
Ok, there are cases where the pad index are meaningful. I guess
that the best is to use properties to properly identify the
pad, or add some strings to them.
Anyway, I guess this should be covered via the properties API.
>
> > + upad.entity_id = pad->entity->graph_obj.id;
> > + upad.flags = pad->flags;
> > +
> > + if (copy_to_user(&topo->pads[i - 1], &upad, sizeof(upad)))
> > + ret = -EFAULT;
> > + }
> > + topo->num_pads = i;
> > +
> > + /* Get links and number of links */
> > + i = 0;
> > + media_device_for_each_link(link, mdev) {
> > + i++;
> > +
> > + if (ret || !topo->links)
> > + continue;
> > +
> > + if (i > topo->num_links) {
> > + ret = -ENOSPC;
> > + continue;
> > + }
> > +
> > + memset(&ulink, 0, sizeof(ulink));
> > +
> > + /* Copy link fields to userspace struct */
> > + ulink.id = link->graph_obj.id;
> > + ulink.source_id = link->gobj0->id;
> > + ulink.sink_id = link->gobj1->id;
> > + ulink.flags = link->flags;
> > +
> > + if (media_type(link->gobj0) != MEDIA_GRAPH_PAD)
> > + ulink.flags |= MEDIA_LNK_FL_INTERFACE_LINK;
> > +
> > + if (copy_to_user(&topo->links[i - 1], &ulink, sizeof(ulink)))
> > + ret = -EFAULT;
> > + }
> > + topo->num_links = i;
> > +
> > + return ret;
> > +}
> > +
> > +static long media_device_get_topology(struct media_device *mdev,
> > + struct media_v2_topology __user *utopo)
> > +{
> > + struct media_v2_topology ktopo;
> > + int ret;
> > +
> > + ret = copy_from_user(&ktopo, utopo, sizeof(ktopo));
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = __media_device_get_topology(mdev, &ktopo);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = copy_to_user(utopo, &ktopo, sizeof(*utopo));
>
> You can return the result from copy_to_user() without assigning it to ret in
> between.
True. Will change on the next version.
> > +
> > + return ret;
> > +}
> > +
> > static long media_device_ioctl(struct file *filp, unsigned int cmd,
> > unsigned long arg)
> > {
> > @@ -264,6 +414,13 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> > mutex_unlock(&dev->graph_mutex);
> > break;
> >
> > + case MEDIA_IOC_G_TOPOLOGY:
> > + mutex_lock(&dev->graph_mutex);
> > + ret = media_device_get_topology(dev,
> > + (struct media_v2_topology __user *)arg);
> > + mutex_unlock(&dev->graph_mutex);
> > + break;
> > +
> > default:
> > ret = -ENOIOCTLCMD;
> > }
> > @@ -312,6 +469,7 @@ static long media_device_compat_ioctl(struct file *filp, unsigned int cmd,
> > case MEDIA_IOC_DEVICE_INFO:
> > case MEDIA_IOC_ENUM_ENTITIES:
> > case MEDIA_IOC_SETUP_LINK:
> > + case MEDIA_IOC_G_TOPOLOGY:
> > return media_device_ioctl(filp, cmd, arg);
> >
> > case MEDIA_IOC_ENUM_LINKS32:
>
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
