Hi Helen,
Thanks for the patch!
On Tue, Sep 13, 2016 at 05:09:58PM -0300, Helen Koike wrote:
> The format was only being propagated to the subdevices directly
> connected to the node being changed.
> Continue propagating the format to all the subdevices in the video pipe.
>
> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
> ---
>
> Only one level of propagation was not that useful for me so I made it to completely
> propagate the format through the topology, I hope this patch to be useful to others.
I'd say most of the time a single sub-device is configured at a time --- the
current implementation does set the format set by the user on a source pad
on the sink pad at the other end of an enabled link as well, but that's just
for the convenience.
Your patch changes this to do a lot more than that. I'm not saying that the
functionality isn't needed, but it would have to be behind a specific flag
as it only caters for the needs of a somewhat special case: you wish to
propagate the same format all the way to the end of the pipeline.
Additionally, just changing the behaviour does break the existing users of
media-ctl.
Sub-devices propagate the cropping and composing (i.e. scaling)
configuration downstream inside a single sub-device. This (very likely)
consequently changes the format on the source pads as well, and should be
similarly propagated downstream.
I think how this could be added to media-ctl is by adding a new command line
option, and amending the functionality with propagating the selections as
well.
The propagation should only be performed after full configuration of a
sub-device is done: a single command line may well first change the format
on a sub-device sink pad, then cropping and finally composition
configuration, all of which are internally propagated in the sub-device
before the following configuration step is taken. I don't think we wish to
propagate each step over the full pipeline downstream.
As per where would I put this --- I'm not fully certain libv4l2subdev is the
right place for the functionality, at least it'd widen the scope for the
library somewhat. I started writing (but unfortunately not being able to
finish it) a pipeline configuration library with the intent of providing
functionality of a regular V4L2 device node on a device the full
configuration (links, sub-device formats etc.) is up to the user. The
library was called libautopipe, I think this could be a beginning of such a
library.
I wonder what Laurent thinks.
>
> utils/media-ctl/libv4l2subdev.c | 43 +++++++++++++++++++++++------------------
> 1 file changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index 3dcf943..8333557 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -644,6 +644,28 @@ static int set_frame_interval(struct media_entity *entity,
> return 0;
> }
>
> +static void propagate_set_fmt(struct media_entity *entity)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < entity->num_links; ++i) {
> + struct media_link *link = &entity->links[i];
> + struct v4l2_mbus_framefmt format;
> +
> + if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> + continue;
> +
> + /* If we found a source pad, propagate it's format to the remote sink */
> + if (link->source->entity == entity &&
> + link->sink->entity->info.type == MEDIA_ENT_T_V4L2_SUBDEV) {
> +
> + v4l2_subdev_get_format(entity, &format, link->source->index,
> + V4L2_SUBDEV_FORMAT_ACTIVE);
> + set_format(link->sink, &format);
> + propagate_set_fmt(link->sink->entity);
> + }
> + }
> +}
>
> static int v4l2_subdev_parse_setup_format(struct media_device *media,
> const char *p, char **endp)
> @@ -653,7 +675,6 @@ static int v4l2_subdev_parse_setup_format(struct media_device *media,
> struct v4l2_rect crop = { -1, -1, -1, -1 };
> struct v4l2_rect compose = crop;
> struct v4l2_fract interval = { 0, 0 };
> - unsigned int i;
> char *end;
> int ret;
>
> @@ -690,24 +711,8 @@ static int v4l2_subdev_parse_setup_format(struct media_device *media,
> return ret;
>
>
> - /* If the pad is an output pad, automatically set the same format on
> - * the remote subdev input pads, if any.
> - */
> - if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> - for (i = 0; i < pad->entity->num_links; ++i) {
> - struct media_link *link = &pad->entity->links[i];
> - struct v4l2_mbus_framefmt remote_format;
> -
> - if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> - continue;
> -
> - if (link->source == pad &&
> - link->sink->entity->info.type == MEDIA_ENT_T_V4L2_SUBDEV) {
> - remote_format = format;
> - set_format(link->sink, &remote_format);
> - }
> - }
> - }
> + /* Automaticaly propagate formats through the video pipe */
> + propagate_set_fmt(pad->entity);
>
> *endp = end;
> return 0;
> --
> 1.9.1
>
> --
> 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
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
