From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([213.167.242.64]:52444 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727205AbeIZU56 (ORCPT ); Wed, 26 Sep 2018 16:57:58 -0400 From: Laurent Pinchart To: Mauro Carvalho Chehab Cc: Linux Media Mailing List , Mauro Carvalho Chehab , Pravin Shedge , Hans Verkuil , Shuah Khan Subject: Re: [PATCH 03/13] v4l2-mc: switch it to use the new approach to setup pipelines Date: Wed, 26 Sep 2018 17:44:53 +0300 Message-ID: <6034562.6tWgEtGTRM@avalon> In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: 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 > --- > 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