* [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
@ 2012-10-26 19:46 Sakari Ailus
2012-11-13 14:24 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2012-10-26 19:46 UTC (permalink / raw)
To: dri-devel, linux-media, alsa-devel; +Cc: laurent.pinchart
If an entity has sink pads, at least one of them must be connected to
another pad with an enabled link. If a driver with multiple sink pads has
more strict requirements the check should be done in the driver itself.
Just requiring one sink pad is connected with an enabled link is enough
API-wise: entities with sink pads with only disabled links should not be
allowed to stream in the first place, but also in a different operation mode
a device might require only one of its pads connected with an active link.
If an entity has an ability to function as a source entity another logical
entity connected to the aforementioned one should be used for the purpose.
Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
drivers/media/media-entity.c | 16 +++++++++++++---
1 files changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e1cd132..8846ea7 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -227,6 +227,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
media_entity_graph_walk_start(&graph, entity);
while ((entity = media_entity_graph_walk_next(&graph))) {
+ bool has_sink = false, active_sink = false;
unsigned int i;
entity->stream_count++;
@@ -243,18 +244,27 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
for (i = 0; i < entity->num_links; i++) {
struct media_link *link = &entity->links[i];
+ /* Are we the sink or not? */
+ if (link->sink->entity != entity)
+ continue;
+
+ has_sink = true;
+
/* Is this pad part of an enabled link? */
if (!(link->flags & MEDIA_LNK_FL_ENABLED))
continue;
- /* Are we the sink or not? */
- if (link->sink->entity != entity)
- continue;
+ active_sink = true;
ret = entity->ops->link_validate(link);
if (ret < 0 && ret != -ENOIOCTLCMD)
goto error;
}
+
+ if (has_sink && !active_sink) {
+ ret = -EPIPE;
+ goto error;
+ }
}
mutex_unlock(&mdev->graph_mutex);
--
1.7.2.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-10-26 19:46 [PATCH 1/1] media: Entities with sink pads must have at least one enabled link Sakari Ailus
@ 2012-11-13 14:24 ` Sakari Ailus
2012-11-14 9:23 ` Sylwester Nawrocki
0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2012-11-13 14:24 UTC (permalink / raw)
To: dri-devel, linux-media, alsa-devel
Cc: laurent.pinchart, broonie, hverkuil, sylwester.nawrocki
Hi all,
Comments would be appreciated, either positive or negative. The omap3isp
driver does the same check itself currently, but I think this is more
generic than that.
Thanks.
On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
> If an entity has sink pads, at least one of them must be connected to
> another pad with an enabled link. If a driver with multiple sink pads has
> more strict requirements the check should be done in the driver itself.
>
> Just requiring one sink pad is connected with an enabled link is enough
> API-wise: entities with sink pads with only disabled links should not be
> allowed to stream in the first place, but also in a different operation mode
> a device might require only one of its pads connected with an active link.
>
> If an entity has an ability to function as a source entity another logical
> entity connected to the aforementioned one should be used for the purpose.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> drivers/media/media-entity.c | 16 +++++++++++++---
> 1 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e1cd132..8846ea7 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -227,6 +227,7 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> media_entity_graph_walk_start(&graph, entity);
>
> while ((entity = media_entity_graph_walk_next(&graph))) {
> + bool has_sink = false, active_sink = false;
> unsigned int i;
>
> entity->stream_count++;
> @@ -243,18 +244,27 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> for (i = 0; i < entity->num_links; i++) {
> struct media_link *link = &entity->links[i];
>
> + /* Are we the sink or not? */
> + if (link->sink->entity != entity)
> + continue;
> +
> + has_sink = true;
> +
> /* Is this pad part of an enabled link? */
> if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> continue;
>
> - /* Are we the sink or not? */
> - if (link->sink->entity != entity)
> - continue;
> + active_sink = true;
>
> ret = entity->ops->link_validate(link);
> if (ret < 0 && ret != -ENOIOCTLCMD)
> goto error;
> }
> +
> + if (has_sink && !active_sink) {
> + ret = -EPIPE;
> + goto error;
> + }
> }
>
> mutex_unlock(&mdev->graph_mutex);
> --
> 1.7.2.5
>
> --
> 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
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-13 14:24 ` Sakari Ailus
@ 2012-11-14 9:23 ` Sylwester Nawrocki
2012-11-14 10:58 ` Laurent Pinchart
2012-11-14 21:13 ` Sakari Ailus
0 siblings, 2 replies; 8+ messages in thread
From: Sylwester Nawrocki @ 2012-11-14 9:23 UTC (permalink / raw)
To: Sakari Ailus
Cc: dri-devel, linux-media, alsa-devel, laurent.pinchart, broonie, hverkuil
Hi Sakari,
On 11/13/2012 03:24 PM, Sakari Ailus wrote:
> Hi all,
>
> Comments would be appreciated, either positive or negative. The omap3isp
> driver does the same check itself currently, but I think this is more
> generic than that.
>
> Thanks.
>
> On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
>> If an entity has sink pads, at least one of them must be connected to
>> another pad with an enabled link. If a driver with multiple sink pads has
>> more strict requirements the check should be done in the driver itself.
>>
>> Just requiring one sink pad is connected with an enabled link is enough
>> API-wise: entities with sink pads with only disabled links should not be
>> allowed to stream in the first place, but also in a different operation mode
>> a device might require only one of its pads connected with an active link.
>>
>> If an entity has an ability to function as a source entity another logical
>> entity connected to the aforementioned one should be used for the purpose.
Why not leave it to individual drivers ? I'm not sure if it is a good idea
not to allow an entity with sink pads to be used as a source only. It might
be appropriate for most of the cases but likely not all. I'm inclined not to
add this requirement in the API. Just my opinion though.
--
Thanks,
Sylwester
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-14 9:23 ` Sylwester Nawrocki
@ 2012-11-14 10:58 ` Laurent Pinchart
2012-11-14 21:15 ` Sakari Ailus
2012-11-14 21:13 ` Sakari Ailus
1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2012-11-14 10:58 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: Sakari Ailus, dri-devel, linux-media, alsa-devel, broonie, hverkuil
Hi Sylwester,
On Wednesday 14 November 2012 10:23:19 Sylwester Nawrocki wrote:
> On 11/13/2012 03:24 PM, Sakari Ailus wrote:
> > Hi all,
> >
> > Comments would be appreciated, either positive or negative. The omap3isp
> > driver does the same check itself currently, but I think this is more
> > generic than that.
> >
> > Thanks.
> >
> > On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
> >> If an entity has sink pads, at least one of them must be connected to
> >> another pad with an enabled link. If a driver with multiple sink pads has
> >> more strict requirements the check should be done in the driver itself.
> >>
> >> Just requiring one sink pad is connected with an enabled link is enough
> >> API-wise: entities with sink pads with only disabled links should not be
> >> allowed to stream in the first place, but also in a different operation
> >> mode a device might require only one of its pads connected with an
> >> active link.
> >>
> >> If an entity has an ability to function as a source entity another
> >> logical entity connected to the aforementioned one should be used for the
> >> purpose.
>
> Why not leave it to individual drivers ? I'm not sure if it is a good idea
> not to allow an entity with sink pads to be used as a source only. It might
> be appropriate for most of the cases but likely not all. I'm inclined not to
> add this requirement in the API. Just my opinion though.
I have mixed feelings about this patch too, which is why I've asked Sakari to
cross-post it. It's pretty easy to add this check to the core now, but pushing
it back to drivers late if we realize it's too restrictive would be difficult.
I think my preference would go for a helper function that drivers can use,
possibly first waiting until a second driver requires this kind of checks
before implementing it.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-14 9:23 ` Sylwester Nawrocki
2012-11-14 10:58 ` Laurent Pinchart
@ 2012-11-14 21:13 ` Sakari Ailus
2012-11-15 0:46 ` Laurent Pinchart
1 sibling, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2012-11-14 21:13 UTC (permalink / raw)
To: Sylwester Nawrocki
Cc: dri-devel, linux-media, alsa-devel, laurent.pinchart, broonie, hverkuil
Hi Sylwester,
Thanks for the comments.
On Wed, Nov 14, 2012 at 10:23:19AM +0100, Sylwester Nawrocki wrote:
> On 11/13/2012 03:24 PM, Sakari Ailus wrote:
> > Hi all,
> >
> > Comments would be appreciated, either positive or negative. The omap3isp
> > driver does the same check itself currently, but I think this is more
> > generic than that.
> >
> > Thanks.
> >
> > On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
> >> If an entity has sink pads, at least one of them must be connected to
> >> another pad with an enabled link. If a driver with multiple sink pads has
> >> more strict requirements the check should be done in the driver itself.
> >>
> >> Just requiring one sink pad is connected with an enabled link is enough
> >> API-wise: entities with sink pads with only disabled links should not be
> >> allowed to stream in the first place, but also in a different operation mode
> >> a device might require only one of its pads connected with an active link.
> >>
> >> If an entity has an ability to function as a source entity another logical
> >> entity connected to the aforementioned one should be used for the purpose.
>
> Why not leave it to individual drivers ? I'm not sure if it is a good idea
> not to allow an entity with sink pads to be used as a source only. It might
> be appropriate for most of the cases but likely not all. I'm inclined not to
> add this requirement in the API. Just my opinion though.
I'm just wondering what would be the use case for that.
What comes closest is generating a test pattern, but even that should be a
separate subdev: the test pattern can be enabled by enabling the link from
the test pattern generator subdev.
As it seems not everyone is outright happy about the idea of making this
mandatory, then how about making it optional?
I'd hate having a link validate function for each subdev e.g. in the OMAP 3
ISP driver that just checks that its sink pad is actually connected with an
enabled link. That'd be lots of mostly useless code. If this is done in the
framework, the drivers will be spared from copying this code in a number of
places. Which was why I originally wrote this patch. The alternative is to
re-parse the whole graph in the driver which I'd also like to avoid.
One opion I can think of is to call link_validate op of struct
media_entity_operations also for disabled links on entities that are
connected through active links (on V4L2 to a video node right before
streaming, for example).
That'd make it easy to perform the check in the drivers.
What do you think?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-14 10:58 ` Laurent Pinchart
@ 2012-11-14 21:15 ` Sakari Ailus
0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2012-11-14 21:15 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sylwester Nawrocki, dri-devel, linux-media, alsa-devel, broonie,
hverkuil
On Wed, Nov 14, 2012 at 11:58:42AM +0100, Laurent Pinchart wrote:
> I think my preference would go for a helper function that drivers can use,
> possibly first waiting until a second driver requires this kind of checks
> before implementing it.
I'd like to see a driver that doesn't. Quite likelye either it has no
configurable links or it's broken. :-)
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-14 21:13 ` Sakari Ailus
@ 2012-11-15 0:46 ` Laurent Pinchart
2012-11-15 20:25 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2012-11-15 0:46 UTC (permalink / raw)
To: Sakari Ailus
Cc: Sylwester Nawrocki, dri-devel, linux-media, alsa-devel, broonie,
hverkuil
Hi Sakari,
On Wednesday 14 November 2012 23:13:45 Sakari Ailus wrote:
> On Wed, Nov 14, 2012 at 10:23:19AM +0100, Sylwester Nawrocki wrote:
> > On 11/13/2012 03:24 PM, Sakari Ailus wrote:
> > > Hi all,
> > >
> > > Comments would be appreciated, either positive or negative. The omap3isp
> > > driver does the same check itself currently, but I think this is more
> > > generic than that.
> > >
> > > Thanks.
> > >
> > > On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
> > >> If an entity has sink pads, at least one of them must be connected to
> > >> another pad with an enabled link. If a driver with multiple sink pads
> > >> has more strict requirements the check should be done in the driver
> > >> itself.
> > >>
> > >> Just requiring one sink pad is connected with an enabled link is enough
> > >> API-wise: entities with sink pads with only disabled links should not
> > >> be allowed to stream in the first place, but also in a different
> > >> operation mode a device might require only one of its pads connected
> > >> with an active link.
> > >>
> > >> If an entity has an ability to function as a source entity another
> > >> logical entity connected to the aforementioned one should be used for
> > >> the purpose.
> >
> > Why not leave it to individual drivers ? I'm not sure if it is a good idea
> > not to allow an entity with sink pads to be used as a source only. It
> > might be appropriate for most of the cases but likely not all. I'm
> > inclined not to add this requirement in the API. Just my opinion though.
>
> I'm just wondering what would be the use case for that.
>
> What comes closest is generating a test pattern, but even that should be a
> separate subdev: the test pattern can be enabled by enabling the link from
> the test pattern generator subdev.
That would force creating a separate subdev just to support test pattern
generation, I'm not sure if I want to push for that. There might also be other
use cases not related to V4L (thus the cross-posting).
> As it seems not everyone is outright happy about the idea of making this
> mandatory, then how about making it optional?
>
> I'd hate having a link validate function for each subdev e.g. in the OMAP 3
> ISP driver that just checks that its sink pad is actually connected with an
> enabled link. That'd be lots of mostly useless code.
Agreed.
> If this is done in the framework, the drivers will be spared from copying
> this code in a number of places. Which was why I originally wrote this
> patch. The alternative is to re-parse the whole graph in the driver which
> I'd also like to avoid.
I'd also prefer to avoid that *if*possible*, as we already walk the graph
during link validation.
> One opion I can think of is to call link_validate op of struct
> media_entity_operations also for disabled links on entities that are
> connected through active links (on V4L2 to a video node right before
> streaming, for example).
>
> That'd make it easy to perform the check in the drivers.
>
> What do you think?
I think that would be a bit too complex. Drivers (or the V4L core) would need
to gather data from multiple links in some state object to find out if the
complete pipeline is valid or not.
Another option would be to set a flag somewhere to indicate whether the check
should be performed by the media core or left to drivers. As different types
of drivers might need different types of checks, I think I would prefer for
now to walk the graph one more time in the OMAP3 ISP driver, as currently
done, and revisit this issue when we will have a couple of drivers
implementing pipeline validity checks. I'm just a bit uncomfortable adding
core code for a feature that has a single user at the moment without a clear
view regarding how it would scale.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] media: Entities with sink pads must have at least one enabled link
2012-11-15 0:46 ` Laurent Pinchart
@ 2012-11-15 20:25 ` Sakari Ailus
0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2012-11-15 20:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Sylwester Nawrocki, dri-devel, linux-media, alsa-devel, broonie,
hverkuil
Hi Laurent,
Thanks for the comments.
On Thu, Nov 15, 2012 at 01:46:23AM +0100, Laurent Pinchart wrote:
> On Wednesday 14 November 2012 23:13:45 Sakari Ailus wrote:
> > On Wed, Nov 14, 2012 at 10:23:19AM +0100, Sylwester Nawrocki wrote:
> > > On 11/13/2012 03:24 PM, Sakari Ailus wrote:
> > > > Hi all,
> > > >
> > > > Comments would be appreciated, either positive or negative. The omap3isp
> > > > driver does the same check itself currently, but I think this is more
> > > > generic than that.
> > > >
> > > > Thanks.
> > > >
> > > > On Fri, Oct 26, 2012 at 10:46:17PM +0300, Sakari Ailus wrote:
> > > >> If an entity has sink pads, at least one of them must be connected to
> > > >> another pad with an enabled link. If a driver with multiple sink pads
> > > >> has more strict requirements the check should be done in the driver
> > > >> itself.
> > > >>
> > > >> Just requiring one sink pad is connected with an enabled link is enough
> > > >> API-wise: entities with sink pads with only disabled links should not
> > > >> be allowed to stream in the first place, but also in a different
> > > >> operation mode a device might require only one of its pads connected
> > > >> with an active link.
> > > >>
> > > >> If an entity has an ability to function as a source entity another
> > > >> logical entity connected to the aforementioned one should be used for
> > > >> the purpose.
> > >
> > > Why not leave it to individual drivers ? I'm not sure if it is a good idea
> > > not to allow an entity with sink pads to be used as a source only. It
> > > might be appropriate for most of the cases but likely not all. I'm
> > > inclined not to add this requirement in the API. Just my opinion though.
> >
> > I'm just wondering what would be the use case for that.
> >
> > What comes closest is generating a test pattern, but even that should be a
> > separate subdev: the test pattern can be enabled by enabling the link from
> > the test pattern generator subdev.
>
> That would force creating a separate subdev just to support test pattern
> generation, I'm not sure if I want to push for that. There might also be other
The benefit of standard interfaces and uniform behaviour of those interfaces
is that you can configure test pattern generation using e.g. media-ctl and
yavta. Nothing else; not even private controls are needed.
> use cases not related to V4L (thus the cross-posting).
That's certainly possible.
...
> > One opion I can think of is to call link_validate op of struct
> > media_entity_operations also for disabled links on entities that are
> > connected through active links (on V4L2 to a video node right before
> > streaming, for example).
> >
> > That'd make it easy to perform the check in the drivers.
> >
> > What do you think?
>
> I think that would be a bit too complex. Drivers (or the V4L core) would need
> to gather data from multiple links in some state object to find out if the
> complete pipeline is valid or not.
>
> Another option would be to set a flag somewhere to indicate whether the check
> should be performed by the media core or left to drivers. As different types
> of drivers might need different types of checks, I think I would prefer for
> now to walk the graph one more time in the OMAP3 ISP driver, as currently
> done, and revisit this issue when we will have a couple of drivers
> implementing pipeline validity checks. I'm just a bit uncomfortable adding
> core code for a feature that has a single user at the moment without a clear
> view regarding how it would scale.
Honestly, I think that a driver that has configurable links but does not
require this check to be done is quite probably broken. I'm not really
convinced there is even a use case for a subdev operating its all sink pads
disconnected.
Btw. this "somewhere" could be struct media_pad. It already has a "flags"
field. We could call it MEDIA_PAD_FL_MUST_CONNECT.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi XMPP: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-11-15 20:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-26 19:46 [PATCH 1/1] media: Entities with sink pads must have at least one enabled link Sakari Ailus
2012-11-13 14:24 ` Sakari Ailus
2012-11-14 9:23 ` Sylwester Nawrocki
2012-11-14 10:58 ` Laurent Pinchart
2012-11-14 21:15 ` Sakari Ailus
2012-11-14 21:13 ` Sakari Ailus
2012-11-15 0:46 ` Laurent Pinchart
2012-11-15 20:25 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.