[v8,48/55,media] media_device: add a topology version field
diff mbox

Message ID e8cb8de5ad8f2da3c32418d67340fe4bb663ce5c.1440902901.git.mchehab@osg.samsung.com
State New
Headers show

Commit Message

Mauro Carvalho Chehab Aug. 30, 2015, 3:06 a.m. UTC
Every time a graph object is added or removed, the version
of the topology changes. That's a requirement for the new
MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
that the topology has changed after a previous call to it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Comments

Hans Verkuil Aug. 31, 2015, 12:29 p.m. UTC | #1
On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> Every time a graph object is added or removed, the version
> of the topology changes. That's a requirement for the new
> MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> that the topology has changed after a previous call to it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

I think this should be postponed until we actually have dynamic reconfigurable
graphs.

I would also like to reserve version 0: if 0 is returned, then the graph is
static.

In G_TOPOLOGY we'd return always 0 for now.

Regards,

	Hans

> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index c89f51bc688d..c18f4af52771 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -185,6 +185,9 @@ void media_gobj_init(struct media_device *mdev,
>  		list_add_tail(&gobj->list, &mdev->interfaces);
>  		break;
>  	}
> +
> +	mdev->topology_version++;
> +
>  	dev_dbg_obj(__func__, gobj);
>  }
>  
> @@ -199,6 +202,8 @@ void media_gobj_remove(struct media_gobj *gobj)
>  {
>  	dev_dbg_obj(__func__, gobj);
>  
> +	gobj->mdev->topology_version++;
> +
>  	/* Remove the object from mdev list */
>  	list_del(&gobj->list);
>  }
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 0d1b9c687454..1b12774a9ab4 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -41,6 +41,8 @@ struct device;
>   * @bus_info:	Unique and stable device location identifier
>   * @hw_revision: Hardware device revision
>   * @driver_version: Device driver version
> + * @topology_version: Monotonic counter for storing the version of the graph
> + *		topology. Should be incremented each time the topology changes.
>   * @entity_id:	Unique ID used on the last entity registered
>   * @pad_id:	Unique ID used on the last pad registered
>   * @link_id:	Unique ID used on the last link registered
> @@ -74,6 +76,8 @@ struct media_device {
>  	u32 hw_revision;
>  	u32 driver_version;
>  
> +	u32 topology_version;
> +
>  	u32 entity_id;
>  	u32 pad_id;
>  	u32 link_id;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Aug. 31, 2015, 12:52 p.m. UTC | #2
Em Mon, 31 Aug 2015 14:29:28 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Every time a graph object is added or removed, the version
> > of the topology changes. That's a requirement for the new
> > MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> > that the topology has changed after a previous call to it.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> I think this should be postponed until we actually have dynamic reconfigurable
> graphs.

So far, we're using the term "dynamic" to mean partial graph object
removal.

But even today, MC does support "dynamic" in the sense of graph
object additions.

You should notice that having a topology_version is something that
IMHO, it is needed since the beginning, even without dynamic
reconfigurable graphs, because the graph may grow in runtime.

That will happen, for example, if usb-snd-audio is blacklisted
at /etc/modprobe*, and someone connects an au0828.

New entities/links will be created (after Shuah patches) if one would
modprobe latter snd-usb-audio.

> 
> I would also like to reserve version 0: if 0 is returned, then the graph is
> static.

Why? Implementing this would be really hard, as that would mean that
G_TOPOLOGY would need to be blocked until all drivers and subdevices
get probed.

In order to implement that, some logic would be needed at the drivers
to identify if everything was set and unlock G_TOPOLOGY.

What would be the gain for that? I fail to see any.

On the other hand, the patch below offers a simple way to detect if topology
changes, as, no matter if an object was added or removed, the topology
version will be increased.

Btw, I added a logic at the mc_nextgen_test program to identify if the
topology changes between the two calls:
	http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/contrib/test/mc_nextgen_test.c?h=mc-next-gen&id=fdc16ece9732c94cfa76eee86978158c5976c00a#n504

Regards,
Mauro

> 
> In G_TOPOLOGY we'd return always 0 for now.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index c89f51bc688d..c18f4af52771 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -185,6 +185,9 @@ void media_gobj_init(struct media_device *mdev,
> >  		list_add_tail(&gobj->list, &mdev->interfaces);
> >  		break;
> >  	}
> > +
> > +	mdev->topology_version++;
> > +
> >  	dev_dbg_obj(__func__, gobj);
> >  }
> >  
> > @@ -199,6 +202,8 @@ void media_gobj_remove(struct media_gobj *gobj)
> >  {
> >  	dev_dbg_obj(__func__, gobj);
> >  
> > +	gobj->mdev->topology_version++;
> > +
> >  	/* Remove the object from mdev list */
> >  	list_del(&gobj->list);
> >  }
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 0d1b9c687454..1b12774a9ab4 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -41,6 +41,8 @@ struct device;
> >   * @bus_info:	Unique and stable device location identifier
> >   * @hw_revision: Hardware device revision
> >   * @driver_version: Device driver version
> > + * @topology_version: Monotonic counter for storing the version of the graph
> > + *		topology. Should be incremented each time the topology changes.
> >   * @entity_id:	Unique ID used on the last entity registered
> >   * @pad_id:	Unique ID used on the last pad registered
> >   * @link_id:	Unique ID used on the last link registered
> > @@ -74,6 +76,8 @@ struct media_device {
> >  	u32 hw_revision;
> >  	u32 driver_version;
> >  
> > +	u32 topology_version;
> > +
> >  	u32 entity_id;
> >  	u32 pad_id;
> >  	u32 link_id;
> > 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil Aug. 31, 2015, 1:35 p.m. UTC | #3
On 08/31/2015 02:52 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 31 Aug 2015 14:29:28 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
>>> Every time a graph object is added or removed, the version
>>> of the topology changes. That's a requirement for the new
>>> MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
>>> that the topology has changed after a previous call to it.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>
>> I think this should be postponed until we actually have dynamic reconfigurable
>> graphs.
> 
> So far, we're using the term "dynamic" to mean partial graph object
> removal.
> 
> But even today, MC does support "dynamic" in the sense of graph
> object additions.
> 
> You should notice that having a topology_version is something that
> IMHO, it is needed since the beginning, even without dynamic
> reconfigurable graphs, because the graph may grow in runtime.
> 
> That will happen, for example, if usb-snd-audio is blacklisted
> at /etc/modprobe*, and someone connects an au0828.
> 
> New entities/links will be created (after Shuah patches) if one would
> modprobe latter snd-usb-audio.

latter -> later :-)

You are right, this would trigger a topology change. I hadn't thought about
that.

> 
>>
>> I would also like to reserve version 0: if 0 is returned, then the graph is
>> static.
> 
> Why? Implementing this would be really hard, as that would mean that
> G_TOPOLOGY would need to be blocked until all drivers and subdevices
> get probed.
> 
> In order to implement that, some logic would be needed at the drivers
> to identify if everything was set and unlock G_TOPOLOGY.

That wouldn't be needed if the media device node was created last. Which
I think is a good idea anyway.

> 
> What would be the gain for that? I fail to see any.

It would tell userspace that it doesn't have to cope with dynamically
changing graphs.

Even though with the au0828 example you can expect to see cases like that,
I can pretty much guarantee that no generic v4l2 applications will ever
support dynamic changes. Those that will support it will be custom-made.

Being able to see that graphs can change dynamically would allow such apps
to either refuse to use the device, or warn the user.

Regards,

	Hans

> 
> On the other hand, the patch below offers a simple way to detect if topology
> changes, as, no matter if an object was added or removed, the topology
> version will be increased.
> 
> Btw, I added a logic at the mc_nextgen_test program to identify if the
> topology changes between the two calls:
> 	http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/contrib/test/mc_nextgen_test.c?h=mc-next-gen&id=fdc16ece9732c94cfa76eee86978158c5976c00a#n504
> 
> Regards,
> Mauro
> 
>>
>> In G_TOPOLOGY we'd return always 0 for now.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>> index c89f51bc688d..c18f4af52771 100644
>>> --- a/drivers/media/media-entity.c
>>> +++ b/drivers/media/media-entity.c
>>> @@ -185,6 +185,9 @@ void media_gobj_init(struct media_device *mdev,
>>>  		list_add_tail(&gobj->list, &mdev->interfaces);
>>>  		break;
>>>  	}
>>> +
>>> +	mdev->topology_version++;
>>> +
>>>  	dev_dbg_obj(__func__, gobj);
>>>  }
>>>  
>>> @@ -199,6 +202,8 @@ void media_gobj_remove(struct media_gobj *gobj)
>>>  {
>>>  	dev_dbg_obj(__func__, gobj);
>>>  
>>> +	gobj->mdev->topology_version++;
>>> +
>>>  	/* Remove the object from mdev list */
>>>  	list_del(&gobj->list);
>>>  }
>>> diff --git a/include/media/media-device.h b/include/media/media-device.h
>>> index 0d1b9c687454..1b12774a9ab4 100644
>>> --- a/include/media/media-device.h
>>> +++ b/include/media/media-device.h
>>> @@ -41,6 +41,8 @@ struct device;
>>>   * @bus_info:	Unique and stable device location identifier
>>>   * @hw_revision: Hardware device revision
>>>   * @driver_version: Device driver version
>>> + * @topology_version: Monotonic counter for storing the version of the graph
>>> + *		topology. Should be incremented each time the topology changes.
>>>   * @entity_id:	Unique ID used on the last entity registered
>>>   * @pad_id:	Unique ID used on the last pad registered
>>>   * @link_id:	Unique ID used on the last link registered
>>> @@ -74,6 +76,8 @@ struct media_device {
>>>  	u32 hw_revision;
>>>  	u32 driver_version;
>>>  
>>> +	u32 topology_version;
>>> +
>>>  	u32 entity_id;
>>>  	u32 pad_id;
>>>  	u32 link_id;
>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab Sept. 4, 2015, 5:08 p.m. UTC | #4
Em Mon, 31 Aug 2015 15:35:38 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 08/31/2015 02:52 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 31 Aug 2015 14:29:28 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > 
> >> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> >>> Every time a graph object is added or removed, the version
> >>> of the topology changes. That's a requirement for the new
> >>> MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> >>> that the topology has changed after a previous call to it.
> >>>
> >>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>
> >> I think this should be postponed until we actually have dynamic reconfigurable
> >> graphs.
> > 
> > So far, we're using the term "dynamic" to mean partial graph object
> > removal.
> > 
> > But even today, MC does support "dynamic" in the sense of graph
> > object additions.
> > 
> > You should notice that having a topology_version is something that
> > IMHO, it is needed since the beginning, even without dynamic
> > reconfigurable graphs, because the graph may grow in runtime.
> > 
> > That will happen, for example, if usb-snd-audio is blacklisted
> > at /etc/modprobe*, and someone connects an au0828.
> > 
> > New entities/links will be created (after Shuah patches) if one would
> > modprobe latter snd-usb-audio.
> 
> latter -> later :-)
> 
> You are right, this would trigger a topology change. I hadn't thought about
> that.
> 
> > 
> >>
> >> I would also like to reserve version 0: if 0 is returned, then the graph is
> >> static.
> > 
> > Why? Implementing this would be really hard, as that would mean that
> > G_TOPOLOGY would need to be blocked until all drivers and subdevices
> > get probed.
> > 
> > In order to implement that, some logic would be needed at the drivers
> > to identify if everything was set and unlock G_TOPOLOGY.
> 
> That wouldn't be needed if the media device node was created last. Which
> I think is a good idea anyway.

Creating the media device node last won't work. It should be the first
thing to be created, as all objects should be added to media_device
linked lists. 

Also, the numberspace should be local to a given media_device, as the
graph traversal algorithm relies on having the number of entities <= 64,
currently, in order to be able to detect loops. We should increase that
number, but removing an "as low as possible" entity number limit is not
trivial.

> 
> > 
> > What would be the gain for that? I fail to see any.
> 
> It would tell userspace that it doesn't have to cope with dynamically
> changing graphs.
> 
> Even though with the au0828 example you can expect to see cases like that,
> I can pretty much guarantee that no generic v4l2 applications will ever
> support dynamic changes.

Well, my test app supports it and it is generic ;) My plan is to use it
as a basis for the library to be used on userspace for generic apps,
extending it to be used by other tools like xawtv, qv4l2 and the dvbv5 apps.

I don't think it is hard to handle it on a generic app, and this should
be done anyway if we want dynamic support.

The logic seems actually be simple:

at G_TOPOLOGY, if the topology changes, reload the objects;

at SETUP_LINK_V2, the topology will be sent. if the driver detects that
topology changed, it returns an error.

The caller will then need to reload the topology and re-apply the
transaction to select the links, if the entities affected still
exists. In other words, if the user's intent were to change the pipeline
to receive the data at /dev/video2, e. g. something like:
	./yavta/yavta -f UYVY -s 720x312 -n 1 --capture=1 -F /dev/video2

What userspace would do is to reload everything, check if /dev/video2
still exists and then redo the function that it is equivalent to the
above command, failing otherwise. That doesn't sound hard to implement.

> Those that will support it will be custom-made.
> 
> Being able to see that graphs can change dynamically would allow such apps
> to either refuse to use the device, or warn the user.

The way I see is that applications that will assume that the graph
is static will be the custom-made ones. As they know the hardware,
they can just either ignore the topology_version or wait for it to
stabilize when the hardware is still being probed.

In any case, if we end by needing to have an explicit way for the
Kernelspace to tell userspace that a graph is static, that could
be done via an extra flag at MEDIA_INFO.

Enabling this flag could be as easy as waiting for all graph
elements to be created (where the topology is still dynamic),
and raise such flag after finishing the probe sequence.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Nov. 23, 2015, 10:18 p.m. UTC | #5
Hi Mauro,

On Friday 04 September 2015 14:08:27 Mauro Carvalho Chehab wrote:
> Em Mon, 31 Aug 2015 15:35:38 +0200 Hans Verkuil escreveu:
> > On 08/31/2015 02:52 PM, Mauro Carvalho Chehab wrote:
> >> Em Mon, 31 Aug 2015 14:29:28 +0200 Hans Verkuil escreveu:
> >>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> >>>> Every time a graph object is added or removed, the version
> >>>> of the topology changes. That's a requirement for the new
> >>>> MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> >>>> that the topology has changed after a previous call to it.
> >>>> 
> >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >>> 
> >>> I think this should be postponed until we actually have dynamic
> >>> reconfigurable graphs.
> >> 
> >> So far, we're using the term "dynamic" to mean partial graph object
> >> removal.
> >> 
> >> But even today, MC does support "dynamic" in the sense of graph object
> >> additions.
> >> 
> >> You should notice that having a topology_version is something that IMHO,
> >> it is needed since the beginning, even without dynamic reconfigurable
> >> graphs, because the graph may grow in runtime.
> >> 
> >> That will happen, for example, if usb-snd-audio is blacklisted at
> >> /etc/modprobe*, and someone connects an au0828.
> >> 
> >> New entities/links will be created (after Shuah patches) if one would
> >> modprobe latter snd-usb-audio.
> > 
> > latter -> later :-)
> > 
> > You are right, this would trigger a topology change. I hadn't thought
> > about that.

First of all it won't be very useful without a topology change notification, 
so until we have them it doesn't matter too much. Then, the code is full of 
race conditions when it comes to dynamic updates, and I'm afraid Shua's 
patches can't go in before we fix them.

> >>> I would also like to reserve version 0: if 0 is returned, then the graph
> >>> is static.
> >> 
> >> Why? Implementing this would be really hard, as that would mean that
> >> G_TOPOLOGY would need to be blocked until all drivers and subdevices get
> >> probed.
> >> 
> >> In order to implement that, some logic would be needed at the drivers to
> >> identify if everything was set and unlock G_TOPOLOGY.
> > 
> > That wouldn't be needed if the media device node was created last. Which I
> > think is a good idea anyway.
> 
> Creating the media device node last won't work. It should be the first thing
> to be created, as all objects should be added to media_device linked lists.

I disagree with that. The media_device needs to be initialized first, but can 
be registered with userspace last. We don't want to generate a topology update 
event for every new entity, link or pad during the device probe sequence. 
Drivers should be in control and tell when they're done with initialization.

> Also, the numberspace should be local to a given media_device, as the graph
> traversal algorithm relies on having the number of entities <= 64,
> currently, in order to be able to detect loops. We should increase that
> number, but removing an "as low as possible" entity number limit is not
> trivial.
> 
> > > What would be the gain for that? I fail to see any.
> > 
> > It would tell userspace that it doesn't have to cope with dynamically
> > changing graphs.
> > 
> > Even though with the au0828 example you can expect to see cases like that,
> > I can pretty much guarantee that no generic v4l2 applications will ever
> > support dynamic changes.
> 
> Well, my test app supports it and it is generic ;) My plan is to use it as a
> basis for the library to be used on userspace for generic apps, extending it
> to be used by other tools like xawtv, qv4l2 and the dvbv5 apps.
> 
> I don't think it is hard to handle it on a generic app,

I'll quote you later on that :-)

> and this should be done anyway if we want dynamic support.
>
> The logic seems actually be simple:
> 
> at G_TOPOLOGY, if the topology changes, reload the objects;

And update everything in userspace. That's a very hard task if you don't 
design your applications extremely carefully.

> at SETUP_LINK_V2, the topology will be sent. if the driver detects that
> topology changed, it returns an error.
> 
> The caller will then need to reload the topology and re-apply the
> transaction to select the links, if the entities affected still exists. In
> other words, if the user's intent were to change the pipeline to receive the
> data at /dev/video2, e. g. something like:
> 	./yavta/yavta -f UYVY -s 720x312 -n 1 --capture=1 -F /dev/video2
> 
> What userspace would do is to reload everything, check if /dev/video2 still
> exists and then redo the function that it is equivalent to the above
> command, failing otherwise. That doesn't sound hard to implement.
> 
> > Those that will support it will be custom-made.
> > 
> > Being able to see that graphs can change dynamically would allow such apps
> > to either refuse to use the device, or warn the user.
> 
> The way I see is that applications that will assume that the graph
> is static will be the custom-made ones.

Or (some of) the generic ones.

> As they know the hardware, they can just either ignore the topology_version
> or wait for it to stabilize when the hardware is still being probed.
> 
> In any case, if we end by needing to have an explicit way for the
> Kernelspace to tell userspace that a graph is static, that could be done via
> an extra flag at MEDIA_INFO.

Why ? I don't see anything wrong with reversing version 0 for that purpose, as 
Hans proposed.

> Enabling this flag could be as easy as waiting for all graph elements to be
> created (where the topology is still dynamic), and raise such flag after
> finishing the probe sequence.
Laurent Pinchart Nov. 23, 2015, 10:20 p.m. UTC | #6
Hi Mauro and Hans,

On Monday 31 August 2015 14:29:28 Hans Verkuil wrote:
> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Every time a graph object is added or removed, the version
> > of the topology changes. That's a requirement for the new
> > MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> > that the topology has changed after a previous call to it.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> I think this should be postponed until we actually have dynamic
> reconfigurable graphs.
> 
> I would also like to reserve version 0: if 0 is returned, then the graph is
> static.
> 
> In G_TOPOLOGY we'd return always 0 for now.

So would I. We need a way to group graph modifications to avoid incrementing 
the version number and generating an event for every entity, link or pad added 
or removed. As this patch doesn't address that I don't see a use for the 
version number now other than making our life more difficult when we'll 
implement dynamic updates by forcing us to consider backward compatibility 
with something that we know won't do the job.

> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index c89f51bc688d..c18f4af52771 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -185,6 +185,9 @@ void media_gobj_init(struct media_device *mdev,
> >  		list_add_tail(&gobj->list, &mdev->interfaces);
> >  		break;
> >  	}
> > +
> > +	mdev->topology_version++;
> > +
> >  	dev_dbg_obj(__func__, gobj);
> >  }
> > 
> > @@ -199,6 +202,8 @@ void media_gobj_remove(struct media_gobj *gobj)
> >  {
> >  	dev_dbg_obj(__func__, gobj);
> > 
> > +	gobj->mdev->topology_version++;
> > +
> >  	/* Remove the object from mdev list */
> >  	list_del(&gobj->list);
> >  }
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 0d1b9c687454..1b12774a9ab4 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -41,6 +41,8 @@ struct device;
> >   * @bus_info:	Unique and stable device location identifier
> >   * @hw_revision: Hardware device revision
> >   * @driver_version: Device driver version
> > + * @topology_version: Monotonic counter for storing the version of the
> > graph
> > + *		topology. Should be incremented each time the topology changes.
> >   * @entity_id:	Unique ID used on the last entity registered
> >   * @pad_id:	Unique ID used on the last pad registered
> >   * @link_id:	Unique ID used on the last link registered
> > @@ -74,6 +76,8 @@ struct media_device {
> >  	u32 hw_revision;
> >  	u32 driver_version;
> > 
> > +	u32 topology_version;
> > +
> >  	u32 entity_id;
> >  	u32 pad_id;
> >  	u32 link_id;
Mauro Carvalho Chehab Dec. 8, 2015, 8:05 p.m. UTC | #7
Em Tue, 24 Nov 2015 00:18:07 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Friday 04 September 2015 14:08:27 Mauro Carvalho Chehab wrote:
> > Em Mon, 31 Aug 2015 15:35:38 +0200 Hans Verkuil escreveu:
> > > On 08/31/2015 02:52 PM, Mauro Carvalho Chehab wrote:
> > >> Em Mon, 31 Aug 2015 14:29:28 +0200 Hans Verkuil escreveu:
> > >>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > >>>> Every time a graph object is added or removed, the version
> > >>>> of the topology changes. That's a requirement for the new
> > >>>> MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> > >>>> that the topology has changed after a previous call to it.
> > >>>> 
> > >>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > >>> 
> > >>> I think this should be postponed until we actually have dynamic
> > >>> reconfigurable graphs.
> > >> 
> > >> So far, we're using the term "dynamic" to mean partial graph object
> > >> removal.
> > >> 
> > >> But even today, MC does support "dynamic" in the sense of graph object
> > >> additions.
> > >> 
> > >> You should notice that having a topology_version is something that IMHO,
> > >> it is needed since the beginning, even without dynamic reconfigurable
> > >> graphs, because the graph may grow in runtime.
> > >> 
> > >> That will happen, for example, if usb-snd-audio is blacklisted at
> > >> /etc/modprobe*, and someone connects an au0828.
> > >> 
> > >> New entities/links will be created (after Shuah patches) if one would
> > >> modprobe latter snd-usb-audio.
> > > 
> > > latter -> later :-)
> > > 
> > > You are right, this would trigger a topology change. I hadn't thought
> > > about that.
> 
> First of all it won't be very useful without a topology change notification, 
> so until we have them it doesn't matter too much.

I'm not sure if we need an explicit topology change notification. The
way I see, we just need to add a topology_version at SET_LINK_V2. The
topology should be filled by userspace, based on the last topology from
G_TOPOLOGY.

If topology_version is different than the one at Kernelspace, Kernel
will return an error that would cause userspace to call G_TOPOLOGY
again.

Ok, a topology notification could be added in order to help userspace
to detect changes, but this is an unrelated.

> Then, the code is full of 
> race conditions when it comes to dynamic updates, and I'm afraid Shua's 
> patches can't go in before we fix them.

The race conditions are related to dynamic removal. Dynamic creation
should be OK.

We'll review the locks to be sure that this would work fine.

> 
> > >>> I would also like to reserve version 0: if 0 is returned, then the graph
> > >>> is static.
> > >> 
> > >> Why? Implementing this would be really hard, as that would mean that
> > >> G_TOPOLOGY would need to be blocked until all drivers and subdevices get
> > >> probed.
> > >> 
> > >> In order to implement that, some logic would be needed at the drivers to
> > >> identify if everything was set and unlock G_TOPOLOGY.
> > > 
> > > That wouldn't be needed if the media device node was created last. Which I
> > > think is a good idea anyway.
> > 
> > Creating the media device node last won't work. It should be the first thing
> > to be created, as all objects should be added to media_device linked lists.
> 
> I disagree with that. The media_device needs to be initialized first, but can 
> be registered with userspace last. We don't want to generate a topology update 
> event for every new entity, link or pad during the device probe sequence. 
> Drivers should be in control and tell when they're done with initialization.

This comment is outdated ;)

Javier wrote a patch series already addressing this: it basically splits
the internal registration from the userspace API registration. 

See:
	[PATCH v4 0/2] [media] Fix race between graph enumeration and entities registration


The idea is to postpone the latter to happen after having all 
entitites/links/subdevices created.

It is unrelated to the MC next gen, so he submitted in separate. I guess Sakari
already acked his patches.

I'll be merging his series after finishing the merge of MC next gen.

> 
> > Also, the numberspace should be local to a given media_device, as the graph
> > traversal algorithm relies on having the number of entities <= 64,
> > currently, in order to be able to detect loops. We should increase that
> > number, but removing an "as low as possible" entity number limit is not
> > trivial.
> > 
> > > > What would be the gain for that? I fail to see any.
> > > 
> > > It would tell userspace that it doesn't have to cope with dynamically
> > > changing graphs.
> > > 
> > > Even though with the au0828 example you can expect to see cases like that,
> > > I can pretty much guarantee that no generic v4l2 applications will ever
> > > support dynamic changes.
> > 
> > Well, my test app supports it and it is generic ;) My plan is to use it as a
> > basis for the library to be used on userspace for generic apps, extending it
> > to be used by other tools like xawtv, qv4l2 and the dvbv5 apps.
> > 
> > I don't think it is hard to handle it on a generic app,
> 
> I'll quote you later on that :-)
> 
> > and this should be done anyway if we want dynamic support.
> >
> > The logic seems actually be simple:
> > 
> > at G_TOPOLOGY, if the topology changes, reload the objects;
> 
> And update everything in userspace. That's a very hard task if you don't 
> design your applications extremely carefully.

True.

> 
> > at SETUP_LINK_V2, the topology will be sent. if the driver detects that
> > topology changed, it returns an error.
> > 
> > The caller will then need to reload the topology and re-apply the
> > transaction to select the links, if the entities affected still exists. In
> > other words, if the user's intent were to change the pipeline to receive the
> > data at /dev/video2, e. g. something like:
> > 	./yavta/yavta -f UYVY -s 720x312 -n 1 --capture=1 -F /dev/video2
> > 
> > What userspace would do is to reload everything, check if /dev/video2 still
> > exists and then redo the function that it is equivalent to the above
> > command, failing otherwise. That doesn't sound hard to implement.
> > 
> > > Those that will support it will be custom-made.
> > > 
> > > Being able to see that graphs can change dynamically would allow such apps
> > > to either refuse to use the device, or warn the user.
> > 
> > The way I see is that applications that will assume that the graph
> > is static will be the custom-made ones.
> 
> Or (some of) the generic ones.

If the app doesn't support dynamic changes, it is not generic, as it
will only support simple graphs that are provided by just a single driver.

> 
> > As they know the hardware, they can just either ignore the topology_version
> > or wait for it to stabilize when the hardware is still being probed.
> > 
> > In any case, if we end by needing to have an explicit way for the
> > Kernelspace to tell userspace that a graph is static, that could be done via
> > an extra flag at MEDIA_INFO.
> 
> Why ? I don't see anything wrong with reversing version 0 for that purpose, as 
> Hans proposed.

Well, we can do that: we can zero the topology_version at
media_device_register() (see Javier's patch "media-device: split media
initialization and registration")

This way, topology_version will be zero if the topology never changed
after registering the media controller userspace API.

That doesn't warrant that the topology will never change. Such change
will happen, for example, during driver (or sub-driver) removal
or unbind. Taking this into account, I doubt that we have any driver
that has a static topology, because the driver can be removed and/or
unbind from the hardware.

> 
> > Enabling this flag could be as easy as waiting for all graph elements to be
> > created (where the topology is still dynamic), and raise such flag after
> > finishing the probe sequence.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index c89f51bc688d..c18f4af52771 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -185,6 +185,9 @@  void media_gobj_init(struct media_device *mdev,
 		list_add_tail(&gobj->list, &mdev->interfaces);
 		break;
 	}
+
+	mdev->topology_version++;
+
 	dev_dbg_obj(__func__, gobj);
 }
 
@@ -199,6 +202,8 @@  void media_gobj_remove(struct media_gobj *gobj)
 {
 	dev_dbg_obj(__func__, gobj);
 
+	gobj->mdev->topology_version++;
+
 	/* Remove the object from mdev list */
 	list_del(&gobj->list);
 }
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 0d1b9c687454..1b12774a9ab4 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -41,6 +41,8 @@  struct device;
  * @bus_info:	Unique and stable device location identifier
  * @hw_revision: Hardware device revision
  * @driver_version: Device driver version
+ * @topology_version: Monotonic counter for storing the version of the graph
+ *		topology. Should be incremented each time the topology changes.
  * @entity_id:	Unique ID used on the last entity registered
  * @pad_id:	Unique ID used on the last pad registered
  * @link_id:	Unique ID used on the last link registered
@@ -74,6 +76,8 @@  struct media_device {
 	u32 hw_revision;
 	u32 driver_version;
 
+	u32 topology_version;
+
 	u32 entity_id;
 	u32 pad_id;
 	u32 link_id;