diff mbox series

[v5,22/24] v4l: subdev: add v4l2_subdev_get_format_dir()

Message ID 20210415130450.421168-23-tomi.valkeinen@ideasonboard.com (mailing list archive)
State New
Headers show
Series v4l: subdev internal routing | expand

Commit Message

Tomi Valkeinen April 15, 2021, 1:04 p.m. UTC
Add v4l2_subdev_get_format_dir() which can be used to find subdev format
for a specific stream on a multiplexed pad. The function will follow the
routes and links until it finds a non-multiplexed pad.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
 include/media/v4l2-subdev.h           | 26 ++++++++
 2 files changed, 122 insertions(+)

Comments

Laurent Pinchart April 18, 2021, 7:04 p.m. UTC | #1
Hi Tomi,

Thank you for the patch.

On Thu, Apr 15, 2021 at 04:04:48PM +0300, Tomi Valkeinen wrote:
> Add v4l2_subdev_get_format_dir() which can be used to find subdev format
> for a specific stream on a multiplexed pad. The function will follow the
> routes and links until it finds a non-multiplexed pad.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           | 26 ++++++++
>  2 files changed, 122 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 7a4f71d8c6c3..430dbdaab080 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -998,6 +998,102 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
>  
> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
> +			       enum v4l2_direction dir,
> +			       struct v4l2_subdev_format *fmt)
> +{
> +	struct device *dev = pad->entity->graph_obj.mdev->dev;
> +	int ret;
> +	int i;
> +
> +	dev_dbg(dev, "%s '%s':%u:%u %s\n", __func__,
> +		pad->entity->name, pad->index, stream,
> +		dir == V4L2_DIR_SOURCEWARD ? "sourceward" : "sinkward");
> +
> +	while (true) {
> +		struct v4l2_subdev_krouting routing;
> +		struct v4l2_subdev_route *route;
> +
> +		if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
> +			return -EINVAL;
> +
> +		ret = v4l2_subdev_link_validate_get_format(pad, fmt);
> +		if (ret == 0)
> +			return 0;
> +		else if (ret != -ENOIOCTLCMD)
> +			return ret;
> +
> +		if (pad->flags &
> +		    (dir == V4L2_DIR_SINKWARD ? MEDIA_PAD_FL_SOURCE :
> +						MEDIA_PAD_FL_SINK)) {
> +			pad = media_entity_remote_pad(pad);
> +
> +			if (!pad)
> +				return -EINVAL;
> +
> +			if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
> +				return -EINVAL;
> +
> +			ret = v4l2_subdev_link_validate_get_format(pad, fmt);
> +			if (ret == 0)
> +				return 0;
> +			else if (ret != -ENOIOCTLCMD)
> +				return ret;
> +		}
> +
> +		ret = v4l2_subdev_get_krouting(media_entity_to_v4l2_subdev(pad->entity), &routing);
> +		if (ret)
> +			return ret;
> +
> +		route = NULL;
> +		for (i = 0; i < routing.num_routes; ++i) {
> +			u16 near_pad = dir == V4L2_DIR_SINKWARD ?
> +					       routing.routes[i].sink_pad :
> +					       routing.routes[i].source_pad;
> +			u16 near_stream = dir == V4L2_DIR_SINKWARD ?
> +						  routing.routes[i].sink_stream :
> +						  routing.routes[i].source_stream;
> +
> +			if (!(routing.routes[i].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> +				continue;
> +
> +			if (near_pad != pad->index)
> +				continue;
> +
> +			if (near_stream != stream)
> +				continue;
> +
> +			if (route) {
> +				dev_err(dev,
> +					"%s: '%s' has multiple active routes for stream %u\n",
> +					__func__, pad->entity->name, stream);
> +				v4l2_subdev_free_routing(&routing);
> +				return -EINVAL;
> +			}
> +
> +			route = &routing.routes[i];
> +		}
> +
> +		if (!route) {
> +			dev_err(dev, "%s: no route found in '%s' for stream %u\n",
> +				__func__, pad->entity->name, stream);
> +			v4l2_subdev_free_routing(&routing);
> +			return -EINVAL;
> +		}
> +
> +		if (dir == V4L2_DIR_SINKWARD) {
> +			pad = &pad->entity->pads[route->source_pad];
> +			stream = route->source_stream;
> +		} else {
> +			pad = &pad->entity->pads[route->sink_pad];
> +			stream = route->sink_stream;
> +		}
> +
> +		v4l2_subdev_free_routing(&routing);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_format_dir);
> +
>  int v4l2_subdev_link_validate(struct media_link *link)
>  {
>  	struct v4l2_subdev *sink;
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1843b77dd843..730631f9a091 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1239,4 +1239,30 @@ void v4l2_subdev_cpy_routing(struct v4l2_subdev_krouting *dst,
>  bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>  			   unsigned int pad0, unsigned int pad1);
>  
> +/**
> + * enum v4l2_direction - Direction either towards the source or the sink
> + *
> + * @V4L2_DIR_SOURCEWARD: Direction towards the source.
> + * @V4L2_DIR_SINKWARD: Direction towards the sink.
> + */
> +enum v4l2_direction {
> +	V4L2_DIR_SOURCEWARD,
> +	V4L2_DIR_SINKWARD,
> +};
> +
> +/**
> + * v4l2_subdev_get_format_dir() - Find format by following streams

The name is a bit cryptic, and the usage pattern error-prone. Can we do
better ?  In particular, if we could limit the usage of this function to
be called on a non-multiplexed pad, we could drop the stream argument.
Deducing the direction argument from the type of pad would also make the
API simpler.

> + * @pad: The pad from which to start the search
> + * @stream: The stream for which we want to find the format
> + * @dir: The direction of the search
> + * @fmt: Pointer to &struct v4l2_subdev_format where the found format is stored
> + *
> + * This function attempts to find v4l2_subdev_format for a specific stream on a
> + * multiplexed pad by following the stream using routes and links to the specified
> + * direction, until a non-multiplexed pad is found.
> + */
> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
> +			       enum v4l2_direction dir,
> +			       struct v4l2_subdev_format *fmt);
> +
>  #endif
Tomi Valkeinen April 21, 2021, 1:04 p.m. UTC | #2
Hi Laurent,

On 18/04/2021 22:04, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Thu, Apr 15, 2021 at 04:04:48PM +0300, Tomi Valkeinen wrote:
>> Add v4l2_subdev_get_format_dir() which can be used to find subdev format
>> for a specific stream on a multiplexed pad. The function will follow the
>> routes and links until it finds a non-multiplexed pad.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
>>   include/media/v4l2-subdev.h           | 26 ++++++++
>>   2 files changed, 122 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 7a4f71d8c6c3..430dbdaab080 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -998,6 +998,102 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
>>   
>> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
>> +			       enum v4l2_direction dir,
>> +			       struct v4l2_subdev_format *fmt)
>> +{
>> +	struct device *dev = pad->entity->graph_obj.mdev->dev;
>> +	int ret;
>> +	int i;
>> +
>> +	dev_dbg(dev, "%s '%s':%u:%u %s\n", __func__,
>> +		pad->entity->name, pad->index, stream,
>> +		dir == V4L2_DIR_SOURCEWARD ? "sourceward" : "sinkward");
>> +
>> +	while (true) {
>> +		struct v4l2_subdev_krouting routing;
>> +		struct v4l2_subdev_route *route;
>> +
>> +		if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
>> +			return -EINVAL;
>> +
>> +		ret = v4l2_subdev_link_validate_get_format(pad, fmt);
>> +		if (ret == 0)
>> +			return 0;
>> +		else if (ret != -ENOIOCTLCMD)
>> +			return ret;
>> +
>> +		if (pad->flags &
>> +		    (dir == V4L2_DIR_SINKWARD ? MEDIA_PAD_FL_SOURCE :
>> +						MEDIA_PAD_FL_SINK)) {
>> +			pad = media_entity_remote_pad(pad);
>> +
>> +			if (!pad)
>> +				return -EINVAL;
>> +
>> +			if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
>> +				return -EINVAL;
>> +
>> +			ret = v4l2_subdev_link_validate_get_format(pad, fmt);
>> +			if (ret == 0)
>> +				return 0;
>> +			else if (ret != -ENOIOCTLCMD)
>> +				return ret;
>> +		}
>> +
>> +		ret = v4l2_subdev_get_krouting(media_entity_to_v4l2_subdev(pad->entity), &routing);
>> +		if (ret)
>> +			return ret;
>> +
>> +		route = NULL;
>> +		for (i = 0; i < routing.num_routes; ++i) {
>> +			u16 near_pad = dir == V4L2_DIR_SINKWARD ?
>> +					       routing.routes[i].sink_pad :
>> +					       routing.routes[i].source_pad;
>> +			u16 near_stream = dir == V4L2_DIR_SINKWARD ?
>> +						  routing.routes[i].sink_stream :
>> +						  routing.routes[i].source_stream;
>> +
>> +			if (!(routing.routes[i].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
>> +				continue;
>> +
>> +			if (near_pad != pad->index)
>> +				continue;
>> +
>> +			if (near_stream != stream)
>> +				continue;
>> +
>> +			if (route) {
>> +				dev_err(dev,
>> +					"%s: '%s' has multiple active routes for stream %u\n",
>> +					__func__, pad->entity->name, stream);
>> +				v4l2_subdev_free_routing(&routing);
>> +				return -EINVAL;
>> +			}
>> +
>> +			route = &routing.routes[i];
>> +		}
>> +
>> +		if (!route) {
>> +			dev_err(dev, "%s: no route found in '%s' for stream %u\n",
>> +				__func__, pad->entity->name, stream);
>> +			v4l2_subdev_free_routing(&routing);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (dir == V4L2_DIR_SINKWARD) {
>> +			pad = &pad->entity->pads[route->source_pad];
>> +			stream = route->source_stream;
>> +		} else {
>> +			pad = &pad->entity->pads[route->sink_pad];
>> +			stream = route->sink_stream;
>> +		}
>> +
>> +		v4l2_subdev_free_routing(&routing);
>> +	}
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_format_dir);
>> +
>>   int v4l2_subdev_link_validate(struct media_link *link)
>>   {
>>   	struct v4l2_subdev *sink;
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 1843b77dd843..730631f9a091 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -1239,4 +1239,30 @@ void v4l2_subdev_cpy_routing(struct v4l2_subdev_krouting *dst,
>>   bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>>   			   unsigned int pad0, unsigned int pad1);
>>   
>> +/**
>> + * enum v4l2_direction - Direction either towards the source or the sink
>> + *
>> + * @V4L2_DIR_SOURCEWARD: Direction towards the source.
>> + * @V4L2_DIR_SINKWARD: Direction towards the sink.
>> + */
>> +enum v4l2_direction {
>> +	V4L2_DIR_SOURCEWARD,
>> +	V4L2_DIR_SINKWARD,
>> +};
>> +
>> +/**
>> + * v4l2_subdev_get_format_dir() - Find format by following streams
> 
> The name is a bit cryptic, and the usage pattern error-prone. Can we do
> better ?  In particular, if we could limit the usage of this function to
> be called on a non-multiplexed pad, we could drop the stream argument.
> Deducing the direction argument from the type of pad would also make the
> API simpler.

Hmm, but that's not what the function does. It follows a specific 
stream, from a multiplexed pad, so it has to get the stream as a parameter.

We can't deduct the direction from the type of the pad. We can of course 
define that given a source pad this function goes sourceward. But if 
that's not what the caller wants, then the caller needs to first follow 
the stream either direction to get a sink pad, and then call this 
function, which doesn't make sense.

>> + * @pad: The pad from which to start the search
>> + * @stream: The stream for which we want to find the format
>> + * @dir: The direction of the search
>> + * @fmt: Pointer to &struct v4l2_subdev_format where the found format is stored
>> + *
>> + * This function attempts to find v4l2_subdev_format for a specific stream on a
>> + * multiplexed pad by following the stream using routes and links to the specified
>> + * direction, until a non-multiplexed pad is found.
>> + */
>> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
>> +			       enum v4l2_direction dir,
>> +			       struct v4l2_subdev_format *fmt);
>> +
>>   #endif
>
Laurent Pinchart April 29, 2021, 1:43 a.m. UTC | #3
Hi Tomi,

On Wed, Apr 21, 2021 at 04:04:22PM +0300, Tomi Valkeinen wrote:
> On 18/04/2021 22:04, Laurent Pinchart wrote:
> > On Thu, Apr 15, 2021 at 04:04:48PM +0300, Tomi Valkeinen wrote:
> >> Add v4l2_subdev_get_format_dir() which can be used to find subdev format
> >> for a specific stream on a multiplexed pad. The function will follow the
> >> routes and links until it finds a non-multiplexed pad.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
> >>   include/media/v4l2-subdev.h           | 26 ++++++++
> >>   2 files changed, 122 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 7a4f71d8c6c3..430dbdaab080 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -998,6 +998,102 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
> >>   }
> >>   EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
> >>   
> >> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
> >> +			       enum v4l2_direction dir,
> >> +			       struct v4l2_subdev_format *fmt)
> >> +{
> >> +	struct device *dev = pad->entity->graph_obj.mdev->dev;
> >> +	int ret;
> >> +	int i;
> >> +
> >> +	dev_dbg(dev, "%s '%s':%u:%u %s\n", __func__,
> >> +		pad->entity->name, pad->index, stream,
> >> +		dir == V4L2_DIR_SOURCEWARD ? "sourceward" : "sinkward");
> >> +
> >> +	while (true) {
> >> +		struct v4l2_subdev_krouting routing;
> >> +		struct v4l2_subdev_route *route;
> >> +
> >> +		if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
> >> +			return -EINVAL;
> >> +
> >> +		ret = v4l2_subdev_link_validate_get_format(pad, fmt);
> >> +		if (ret == 0)
> >> +			return 0;
> >> +		else if (ret != -ENOIOCTLCMD)
> >> +			return ret;
> >> +
> >> +		if (pad->flags &
> >> +		    (dir == V4L2_DIR_SINKWARD ? MEDIA_PAD_FL_SOURCE :
> >> +						MEDIA_PAD_FL_SINK)) {
> >> +			pad = media_entity_remote_pad(pad);
> >> +
> >> +			if (!pad)
> >> +				return -EINVAL;
> >> +
> >> +			if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
> >> +				return -EINVAL;
> >> +
> >> +			ret = v4l2_subdev_link_validate_get_format(pad, fmt);
> >> +			if (ret == 0)
> >> +				return 0;
> >> +			else if (ret != -ENOIOCTLCMD)
> >> +				return ret;
> >> +		}
> >> +
> >> +		ret = v4l2_subdev_get_krouting(media_entity_to_v4l2_subdev(pad->entity), &routing);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		route = NULL;
> >> +		for (i = 0; i < routing.num_routes; ++i) {
> >> +			u16 near_pad = dir == V4L2_DIR_SINKWARD ?
> >> +					       routing.routes[i].sink_pad :
> >> +					       routing.routes[i].source_pad;
> >> +			u16 near_stream = dir == V4L2_DIR_SINKWARD ?
> >> +						  routing.routes[i].sink_stream :
> >> +						  routing.routes[i].source_stream;
> >> +
> >> +			if (!(routing.routes[i].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
> >> +				continue;
> >> +
> >> +			if (near_pad != pad->index)
> >> +				continue;
> >> +
> >> +			if (near_stream != stream)
> >> +				continue;
> >> +
> >> +			if (route) {
> >> +				dev_err(dev,
> >> +					"%s: '%s' has multiple active routes for stream %u\n",
> >> +					__func__, pad->entity->name, stream);
> >> +				v4l2_subdev_free_routing(&routing);
> >> +				return -EINVAL;
> >> +			}
> >> +
> >> +			route = &routing.routes[i];
> >> +		}
> >> +
> >> +		if (!route) {
> >> +			dev_err(dev, "%s: no route found in '%s' for stream %u\n",
> >> +				__func__, pad->entity->name, stream);
> >> +			v4l2_subdev_free_routing(&routing);
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +		if (dir == V4L2_DIR_SINKWARD) {
> >> +			pad = &pad->entity->pads[route->source_pad];
> >> +			stream = route->source_stream;
> >> +		} else {
> >> +			pad = &pad->entity->pads[route->sink_pad];
> >> +			stream = route->sink_stream;
> >> +		}
> >> +
> >> +		v4l2_subdev_free_routing(&routing);
> >> +	}
> >> +}
> >> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_format_dir);
> >> +
> >>   int v4l2_subdev_link_validate(struct media_link *link)
> >>   {
> >>   	struct v4l2_subdev *sink;
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index 1843b77dd843..730631f9a091 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -1239,4 +1239,30 @@ void v4l2_subdev_cpy_routing(struct v4l2_subdev_krouting *dst,
> >>   bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
> >>   			   unsigned int pad0, unsigned int pad1);
> >>   
> >> +/**
> >> + * enum v4l2_direction - Direction either towards the source or the sink
> >> + *
> >> + * @V4L2_DIR_SOURCEWARD: Direction towards the source.
> >> + * @V4L2_DIR_SINKWARD: Direction towards the sink.
> >> + */
> >> +enum v4l2_direction {
> >> +	V4L2_DIR_SOURCEWARD,
> >> +	V4L2_DIR_SINKWARD,
> >> +};
> >> +
> >> +/**
> >> + * v4l2_subdev_get_format_dir() - Find format by following streams
> > 
> > The name is a bit cryptic, and the usage pattern error-prone. Can we do
> > better ?  In particular, if we could limit the usage of this function to
> > be called on a non-multiplexed pad, we could drop the stream argument.
> > Deducing the direction argument from the type of pad would also make the
> > API simpler.
> 
> Hmm, but that's not what the function does. It follows a specific 
> stream, from a multiplexed pad, so it has to get the stream as a parameter.
> 
> We can't deduct the direction from the type of the pad. We can of course 
> define that given a source pad this function goes sourceward. But if 
> that's not what the caller wants, then the caller needs to first follow 
> the stream either direction to get a sink pad, and then call this 
> function, which doesn't make sense.

What do the current callers need ? We don't have to implement something
that is more generic or featureful than our needs dictate, as this is a
very ad hoc function anyway. If we really need the full behaviour
implemented here, we should at the very least rename the function, but I
think it should be possible to do better overall, perhaps splitting the
operation in the caller into cleaner functions.

> >> + * @pad: The pad from which to start the search
> >> + * @stream: The stream for which we want to find the format
> >> + * @dir: The direction of the search
> >> + * @fmt: Pointer to &struct v4l2_subdev_format where the found format is stored
> >> + *
> >> + * This function attempts to find v4l2_subdev_format for a specific stream on a
> >> + * multiplexed pad by following the stream using routes and links to the specified
> >> + * direction, until a non-multiplexed pad is found.
> >> + */
> >> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
> >> +			       enum v4l2_direction dir,
> >> +			       struct v4l2_subdev_format *fmt);
> >> +
> >>   #endif
Tomi Valkeinen May 4, 2021, 6:49 a.m. UTC | #4
On 29/04/2021 04:43, Laurent Pinchart wrote:
> Hi Tomi,
> 
> On Wed, Apr 21, 2021 at 04:04:22PM +0300, Tomi Valkeinen wrote:
>> On 18/04/2021 22:04, Laurent Pinchart wrote:
>>> On Thu, Apr 15, 2021 at 04:04:48PM +0300, Tomi Valkeinen wrote:
>>>> Add v4l2_subdev_get_format_dir() which can be used to find subdev format
>>>> for a specific stream on a multiplexed pad. The function will follow the
>>>> routes and links until it finds a non-multiplexed pad.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>    drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
>>>>    include/media/v4l2-subdev.h           | 26 ++++++++
>>>>    2 files changed, 122 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index 7a4f71d8c6c3..430dbdaab080 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -998,6 +998,102 @@ bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
>>>>    
>>>> +int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
>>>> +			       enum v4l2_direction dir,
>>>> +			       struct v4l2_subdev_format *fmt)
>>>> +{
>>>> +	struct device *dev = pad->entity->graph_obj.mdev->dev;
>>>> +	int ret;
>>>> +	int i;
>>>> +
>>>> +	dev_dbg(dev, "%s '%s':%u:%u %s\n", __func__,
>>>> +		pad->entity->name, pad->index, stream,
>>>> +		dir == V4L2_DIR_SOURCEWARD ? "sourceward" : "sinkward");
>>>> +
>>>> +	while (true) {
>>>> +		struct v4l2_subdev_krouting routing;
>>>> +		struct v4l2_subdev_route *route;
>>>> +
>>>> +		if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
>>>> +			return -EINVAL;
>>>> +
>>>> +		ret = v4l2_subdev_link_validate_get_format(pad, fmt);
>>>> +		if (ret == 0)
>>>> +			return 0;
>>>> +		else if (ret != -ENOIOCTLCMD)
>>>> +			return ret;
>>>> +
>>>> +		if (pad->flags &
>>>> +		    (dir == V4L2_DIR_SINKWARD ? MEDIA_PAD_FL_SOURCE :
>>>> +						MEDIA_PAD_FL_SINK)) {
>>>> +			pad = media_entity_remote_pad(pad);
>>>> +
>>>> +			if (!pad)
>>>> +				return -EINVAL;
>>>> +
>>>> +			if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
>>>> +				return -EINVAL;
>>>> +
>>>> +			ret = v4l2_subdev_link_validate_get_format(pad, fmt);
>>>> +			if (ret == 0)
>>>> +				return 0;
>>>> +			else if (ret != -ENOIOCTLCMD)
>>>> +				return ret;
>>>> +		}
>>>> +
>>>> +		ret = v4l2_subdev_get_krouting(media_entity_to_v4l2_subdev(pad->entity), &routing);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +
>>>> +		route = NULL;
>>>> +		for (i = 0; i < routing.num_routes; ++i) {
>>>> +			u16 near_pad = dir == V4L2_DIR_SINKWARD ?
>>>> +					       routing.routes[i].sink_pad :
>>>> +					       routing.routes[i].source_pad;
>>>> +			u16 near_stream = dir == V4L2_DIR_SINKWARD ?
>>>> +						  routing.routes[i].sink_stream :
>>>> +						  routing.routes[i].source_stream;
>>>> +
>>>> +			if (!(routing.routes[i].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
>>>> +				continue;
>>>> +
>>>> +			if (near_pad != pad->index)
>>>> +				continue;
>>>> +
>>>> +			if (near_stream != stream)
>>>> +				continue;
>>>> +
>>>> +			if (route) {
>>>> +				dev_err(dev,
>>>> +					"%s: '%s' has multiple active routes for stream %u\n",
>>>> +					__func__, pad->entity->name, stream);
>>>> +				v4l2_subdev_free_routing(&routing);
>>>> +				return -EINVAL;
>>>> +			}
>>>> +
>>>> +			route = &routing.routes[i];
>>>> +		}
>>>> +
>>>> +		if (!route) {
>>>> +			dev_err(dev, "%s: no route found in '%s' for stream %u\n",
>>>> +				__func__, pad->entity->name, stream);
>>>> +			v4l2_subdev_free_routing(&routing);
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +		if (dir == V4L2_DIR_SINKWARD) {
>>>> +			pad = &pad->entity->pads[route->source_pad];
>>>> +			stream = route->source_stream;
>>>> +		} else {
>>>> +			pad = &pad->entity->pads[route->sink_pad];
>>>> +			stream = route->sink_stream;
>>>> +		}
>>>> +
>>>> +		v4l2_subdev_free_routing(&routing);
>>>> +	}
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_format_dir);
>>>> +
>>>>    int v4l2_subdev_link_validate(struct media_link *link)
>>>>    {
>>>>    	struct v4l2_subdev *sink;
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index 1843b77dd843..730631f9a091 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -1239,4 +1239,30 @@ void v4l2_subdev_cpy_routing(struct v4l2_subdev_krouting *dst,
>>>>    bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
>>>>    			   unsigned int pad0, unsigned int pad1);
>>>>    
>>>> +/**
>>>> + * enum v4l2_direction - Direction either towards the source or the sink
>>>> + *
>>>> + * @V4L2_DIR_SOURCEWARD: Direction towards the source.
>>>> + * @V4L2_DIR_SINKWARD: Direction towards the sink.
>>>> + */
>>>> +enum v4l2_direction {
>>>> +	V4L2_DIR_SOURCEWARD,
>>>> +	V4L2_DIR_SINKWARD,
>>>> +};
>>>> +
>>>> +/**
>>>> + * v4l2_subdev_get_format_dir() - Find format by following streams
>>>
>>> The name is a bit cryptic, and the usage pattern error-prone. Can we do
>>> better ?  In particular, if we could limit the usage of this function to
>>> be called on a non-multiplexed pad, we could drop the stream argument.
>>> Deducing the direction argument from the type of pad would also make the
>>> API simpler.
>>
>> Hmm, but that's not what the function does. It follows a specific
>> stream, from a multiplexed pad, so it has to get the stream as a parameter.
>>
>> We can't deduct the direction from the type of the pad. We can of course
>> define that given a source pad this function goes sourceward. But if
>> that's not what the caller wants, then the caller needs to first follow
>> the stream either direction to get a sink pad, and then call this
>> function, which doesn't make sense.
> 
> What do the current callers need ? We don't have to implement something
> that is more generic or featureful than our needs dictate, as this is a
> very ad hoc function anyway. If we really need the full behaviour
> implemented here, we should at the very least rename the function, but I
> think it should be possible to do better overall, perhaps splitting the
> operation in the caller into cleaner functions.

The link validation will call the function with both V4L2_DIR_SOURCEWARD 
and V4L2_DIR_SINKWARD. DS90UB960 driver also uses the function, with 
V4L2_DIR_SOURCEWARD.

The name may not be the most clear one, but it's not easy to invent a 
name for something like this =). Why do you think the usage pattern is 
error prone?

  Tomi
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 7a4f71d8c6c3..430dbdaab080 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -998,6 +998,102 @@  bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_has_route);
 
+int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
+			       enum v4l2_direction dir,
+			       struct v4l2_subdev_format *fmt)
+{
+	struct device *dev = pad->entity->graph_obj.mdev->dev;
+	int ret;
+	int i;
+
+	dev_dbg(dev, "%s '%s':%u:%u %s\n", __func__,
+		pad->entity->name, pad->index, stream,
+		dir == V4L2_DIR_SOURCEWARD ? "sourceward" : "sinkward");
+
+	while (true) {
+		struct v4l2_subdev_krouting routing;
+		struct v4l2_subdev_route *route;
+
+		if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
+			return -EINVAL;
+
+		ret = v4l2_subdev_link_validate_get_format(pad, fmt);
+		if (ret == 0)
+			return 0;
+		else if (ret != -ENOIOCTLCMD)
+			return ret;
+
+		if (pad->flags &
+		    (dir == V4L2_DIR_SINKWARD ? MEDIA_PAD_FL_SOURCE :
+						MEDIA_PAD_FL_SINK)) {
+			pad = media_entity_remote_pad(pad);
+
+			if (!pad)
+				return -EINVAL;
+
+			if (pad->entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV)
+				return -EINVAL;
+
+			ret = v4l2_subdev_link_validate_get_format(pad, fmt);
+			if (ret == 0)
+				return 0;
+			else if (ret != -ENOIOCTLCMD)
+				return ret;
+		}
+
+		ret = v4l2_subdev_get_krouting(media_entity_to_v4l2_subdev(pad->entity), &routing);
+		if (ret)
+			return ret;
+
+		route = NULL;
+		for (i = 0; i < routing.num_routes; ++i) {
+			u16 near_pad = dir == V4L2_DIR_SINKWARD ?
+					       routing.routes[i].sink_pad :
+					       routing.routes[i].source_pad;
+			u16 near_stream = dir == V4L2_DIR_SINKWARD ?
+						  routing.routes[i].sink_stream :
+						  routing.routes[i].source_stream;
+
+			if (!(routing.routes[i].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
+				continue;
+
+			if (near_pad != pad->index)
+				continue;
+
+			if (near_stream != stream)
+				continue;
+
+			if (route) {
+				dev_err(dev,
+					"%s: '%s' has multiple active routes for stream %u\n",
+					__func__, pad->entity->name, stream);
+				v4l2_subdev_free_routing(&routing);
+				return -EINVAL;
+			}
+
+			route = &routing.routes[i];
+		}
+
+		if (!route) {
+			dev_err(dev, "%s: no route found in '%s' for stream %u\n",
+				__func__, pad->entity->name, stream);
+			v4l2_subdev_free_routing(&routing);
+			return -EINVAL;
+		}
+
+		if (dir == V4L2_DIR_SINKWARD) {
+			pad = &pad->entity->pads[route->source_pad];
+			stream = route->source_stream;
+		} else {
+			pad = &pad->entity->pads[route->sink_pad];
+			stream = route->sink_stream;
+		}
+
+		v4l2_subdev_free_routing(&routing);
+	}
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_get_format_dir);
+
 int v4l2_subdev_link_validate(struct media_link *link)
 {
 	struct v4l2_subdev *sink;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1843b77dd843..730631f9a091 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1239,4 +1239,30 @@  void v4l2_subdev_cpy_routing(struct v4l2_subdev_krouting *dst,
 bool v4l2_subdev_has_route(struct v4l2_subdev_krouting *routing,
 			   unsigned int pad0, unsigned int pad1);
 
+/**
+ * enum v4l2_direction - Direction either towards the source or the sink
+ *
+ * @V4L2_DIR_SOURCEWARD: Direction towards the source.
+ * @V4L2_DIR_SINKWARD: Direction towards the sink.
+ */
+enum v4l2_direction {
+	V4L2_DIR_SOURCEWARD,
+	V4L2_DIR_SINKWARD,
+};
+
+/**
+ * v4l2_subdev_get_format_dir() - Find format by following streams
+ * @pad: The pad from which to start the search
+ * @stream: The stream for which we want to find the format
+ * @dir: The direction of the search
+ * @fmt: Pointer to &struct v4l2_subdev_format where the found format is stored
+ *
+ * This function attempts to find v4l2_subdev_format for a specific stream on a
+ * multiplexed pad by following the stream using routes and links to the specified
+ * direction, until a non-multiplexed pad is found.
+ */
+int v4l2_subdev_get_format_dir(struct media_pad *pad, u16 stream,
+			       enum v4l2_direction dir,
+			       struct v4l2_subdev_format *fmt);
+
 #endif