Re: [PATCH 03/13] v4l2-mc: switch it to use the new approach to setup pipelines

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mauro,

Thank you for the patch.

On Wednesday, 1 August 2018 18:55:05 EEST Mauro Carvalho Chehab wrote:
> Instead of relying on a static map for pids, use the new sig_type
> "taint" type to setup the pipelines with the same tipe between

s/tipe/type/

> different entities.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
> ---
>  drivers/media/media-entity.c      | 26 +++++++++++
>  drivers/media/v4l2-core/v4l2-mc.c | 73 ++++++++++++++++++++++++-------
>  include/media/media-entity.h      | 19 ++++++++
>  3 files changed, 101 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 3498551e618e..0b1cb3559140 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -662,6 +662,32 @@ static void __media_entity_remove_link(struct
> media_entity *entity, kfree(link);
>  }
> 
> +int media_get_pad_index(struct media_entity *entity, bool is_sink,
> +			enum media_pad_signal_type sig_type)
> +{
> +	int i;

is is never negative, please use an unsigned int.

> +	bool pad_is_sink;
> +
> +	if (!entity)
> +		return -EINVAL;
> +
> +	for (i = 0; i < entity->num_pads; i++) {
> +		if (entity->pads[i].flags == MEDIA_PAD_FL_SINK)
> +			pad_is_sink = true;
> +		else if (entity->pads[i].flags == MEDIA_PAD_FL_SOURCE)
> +			pad_is_sink = false;
> +		else
> +			continue;	/* This is an error! */
> +
> +		if (pad_is_sink != is_sink)
> +			continue;
> +		if (entity->pads[i].sig_type == sig_type)
> +			return i;
> +	}
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(media_get_pad_index);
> +
>  int
>  media_create_pad_link(struct media_entity *source, u16 source_pad,
>  			 struct media_entity *sink, u16 sink_pad, u32 flags)
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c
> b/drivers/media/v4l2-core/v4l2-mc.c index 982bab3530f6..1925e1a3b861 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -28,7 +28,7 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
>  	struct media_entity *io_v4l = NULL, *io_vbi = NULL, *io_swradio = NULL;
>  	bool is_webcam = false;
>  	u32 flags;
> -	int ret;
> +	int ret, pad_sink, pad_source;
> 
>  	if (!mdev)
>  		return 0;
> @@ -97,29 +97,52 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev) /* Link the tuner and IF video output pads */
>  	if (tuner) {
>  		if (if_vid) {
> -			ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> -						    if_vid,
> -						    IF_VID_DEC_PAD_IF_INPUT,
> +			pad_source = media_get_pad_index(tuner, false,
> +							 PAD_SIGNAL_ANALOG);
> +			pad_sink = media_get_pad_index(if_vid, true,
> +						       PAD_SIGNAL_ANALOG);
> +			if (pad_source < 0 || pad_sink < 0)
> +				return -EINVAL;
> +			ret = media_create_pad_link(tuner, pad_source,
> +						    if_vid, pad_sink,
>  						    MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
> -			ret = media_create_pad_link(if_vid, IF_VID_DEC_PAD_OUT,
> -						decoder, DEMOD_PAD_IF_INPUT,
> +
> +			pad_source = media_get_pad_index(if_vid, false,
> +							 PAD_SIGNAL_DV);
> +			pad_sink = media_get_pad_index(decoder, true,
> +						       PAD_SIGNAL_DV);
> +			if (pad_source < 0 || pad_sink < 0)
> +				return -EINVAL;
> +			ret = media_create_pad_link(if_vid, pad_source,
> +						decoder, pad_sink,
>  						MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
>  		} else {
> -			ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> -						decoder, DEMOD_PAD_IF_INPUT,
> +			pad_source = media_get_pad_index(tuner, false,
> +							 PAD_SIGNAL_ANALOG);
> +			pad_sink = media_get_pad_index(decoder, true,
> +						       PAD_SIGNAL_ANALOG);
> +			if (pad_source < 0 || pad_sink < 0)
> +				return -EINVAL;
> +			ret = media_create_pad_link(tuner, pad_source,
> +						decoder, pad_sink,
>  						MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
>  		}
> 
>  		if (if_aud) {
> -			ret = media_create_pad_link(tuner, TUNER_PAD_AUD_OUT,
> -						    if_aud,
> -						    IF_AUD_DEC_PAD_IF_INPUT,
> +			pad_source = media_get_pad_index(tuner, false,
> +							 PAD_SIGNAL_AUDIO);
> +			pad_sink = media_get_pad_index(decoder, true,
> +						       PAD_SIGNAL_AUDIO);
> +			if (pad_source < 0 || pad_sink < 0)
> +				return -EINVAL;
> +			ret = media_create_pad_link(tuner, pad_source,
> +						    if_aud, pad_sink,
>  						    MEDIA_LNK_FL_ENABLED);
>  			if (ret)
>  				return ret;
> @@ -131,7 +154,10 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev)
> 
>  	/* Create demod to V4L, VBI and SDR radio links */
>  	if (io_v4l) {
> -		ret = media_create_pad_link(decoder, DEMOD_PAD_VID_OUT,
> +		pad_source = media_get_pad_index(decoder, false, PAD_SIGNAL_DV);
> +		if (pad_source < 0)
> +			return -EINVAL;
> +		ret = media_create_pad_link(decoder, pad_source,
>  					io_v4l, 0,
>  					MEDIA_LNK_FL_ENABLED);
>  		if (ret)
> @@ -139,7 +165,10 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev) }
> 
>  	if (io_swradio) {
> -		ret = media_create_pad_link(decoder, DEMOD_PAD_VID_OUT,
> +		pad_source = media_get_pad_index(decoder, false, PAD_SIGNAL_DV);
> +		if (pad_source < 0)
> +			return -EINVAL;
> +		ret = media_create_pad_link(decoder, pad_source,
>  					io_swradio, 0,
>  					MEDIA_LNK_FL_ENABLED);
>  		if (ret)
> @@ -147,7 +176,10 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev) }
> 
>  	if (io_vbi) {
> -		ret = media_create_pad_link(decoder, DEMOD_PAD_VID_OUT,
> +		pad_source = media_get_pad_index(decoder, false, PAD_SIGNAL_DV);
> +		if (pad_source < 0)
> +			return -EINVAL;
> +		ret = media_create_pad_link(decoder, pad_source,
>  					    io_vbi, 0,
>  					    MEDIA_LNK_FL_ENABLED);
>  		if (ret)
> @@ -161,15 +193,22 @@ int v4l2_mc_create_media_graph(struct media_device
> *mdev) case MEDIA_ENT_F_CONN_RF:
>  			if (!tuner)
>  				continue;
> -
> +			pad_source = media_get_pad_index(tuner, false,
> +							 PAD_SIGNAL_ANALOG);
> +			if (pad_source < 0)
> +				return -EINVAL;
>  			ret = media_create_pad_link(entity, 0, tuner,
> -						    TUNER_PAD_RF_INPUT,
> +						    pad_source,
>  						    flags);
>  			break;
>  		case MEDIA_ENT_F_CONN_SVIDEO:
>  		case MEDIA_ENT_F_CONN_COMPOSITE:
> +			pad_sink = media_get_pad_index(decoder, true,
> +						       PAD_SIGNAL_ANALOG);
> +			if (pad_sink < 0)
> +				return -EINVAL;
>  			ret = media_create_pad_link(entity, 0, decoder,
> -						    DEMOD_PAD_IF_INPUT,
> +						    pad_sink,
>  						    flags);
>  			break;
>  		default:
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 8bfbe6b59fa9..ac8b93e46167 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -675,6 +675,25 @@ static inline void media_entity_cleanup(struct
> media_entity *entity) {} #define media_entity_cleanup(entity) do { } while
> (false)
>  #endif
> 
> +

Extra blank line.

> +/**
> + * media_get_pad_index() - retrieves a pad index from an entity

I think a better name would be media_entity_find_pad(), similarly to 
media_entity_find_link(), as the function searches for a pad given a direction 
and signal type. A *_get_*() function name hints of reference counting.

> + *
> + * @entity:	entity where the pads belong
> + * @is_sink:	true if the pad is a sink, false if it is a source

Could we use pad flags instead ? It's easier to read

	pad = media_get_pad_index(entity, MEDIA_PAD_FL_SINK, ...);

than

	pad = media_get_pad_index(entity, true, ...);

As an added bonus that would allow the caller to search for any pad with a 
given signal type by specifying MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_SOURCE.

> + * @sig_type:	type of signal of the pad to be search
> + *
> + * This helper function finds the first pad index inside an entity that
> + * satisfies both @is_sink and @sig_type conditions.
> + *
> + * Return:
> + *
> + * On success, return the pad number. If the pad was not found or the media
> + * entity is a NULL pointer, return -EINVAL.
> + */
> +int media_get_pad_index(struct media_entity *entity, bool is_sink,
> +			enum media_pad_signal_type sig_type);
> +
>  /**
>   * media_create_pad_link() - creates a link between two entities.
>   *

-- 
Regards,

Laurent Pinchart