Em Mon, 08 Feb 2016 15:42:15 -0700
Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote:
> > Em Wed, 03 Feb 2016 21:03:52 -0700
> > Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> >
> >> Add enable_source and disable_source handlers.
> >> The enable source handler is called from
> >> v4l2-core, dvb-core, and ALSA drivers to check
> >> if the shared media source is free. The disable
> >> source handler is called to release the shared
> >> media source.
> >>
> >> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/media/usb/au0828/au0828-core.c | 149 +++++++++++++++++++++++++++++++++
> >> drivers/media/usb/au0828/au0828.h | 3 +
> >> 2 files changed, 152 insertions(+)
> >>
> >> diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c
> >> index 4c90f28..fd2265c 100644
> >> --- a/drivers/media/usb/au0828/au0828-core.c
> >> +++ b/drivers/media/usb/au0828/au0828-core.c
> >> @@ -282,6 +282,7 @@ static int au0828_create_media_graph(struct au0828_dev *dev)
> >> return -EINVAL;
> >>
> >> if (tuner) {
> >> + dev->tuner = tuner;
> >> /* create tuner to decoder link in deactivated state */
> >> ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT,
> >> decoder, 0, 0);
> >> @@ -373,6 +374,150 @@ void au0828_media_graph_notify(struct media_entity *new, void *notify_data)
> >> #endif
> >> }
> >>
> >> +static int au0828_enable_source(struct media_entity *entity,
> >> + struct media_pipeline *pipe)
> >> +{
> >> +#ifdef CONFIG_MEDIA_CONTROLLER
> >> + struct media_entity *source;
> >> + struct media_entity *sink;
> >> + struct media_link *link, *found_link = NULL;
> >> + int ret = 0;
> >> + struct media_device *mdev = entity->graph_obj.mdev;
> >> + struct au0828_dev *dev;
> >> +
> >> + if (!mdev)
> >> + return -ENODEV;
> >> +
> >> + /* for Audio and Video entities, source is the decoder */
> >> + mutex_lock(&mdev->graph_mutex);
> >> +
> >> + dev = mdev->source_priv;
> >> + if (!dev->tuner || !dev->decoder) {
> >> + ret = -ENODEV;
> >> + goto end;
> >> + }
> >
> > This is wrong. There are devices without tuner (capture devices) and
> > without analog decoder (pure DVB devices).
>
> Removed linux-api from the list.
>
> Yes this logic is making an assumption that both
> decoder and tuner are present. Based on your comment
> here, is the following check for decoder in
> au0828_create_media_graph() incorrect? When decoder
> is null, au0828_usb_probe() bails out. Please see
> au0828_create_media_graph() return handling in
> au0828_usb_probe()?
It seems so. at its current state, au0828 always register a V4L2
node, except if !CONFIG_VIDEO_AU0828_V4L2. So, it is missing a
#ifdef CONFIG_VIDEO_AU0828_V4L2
inside it. I'll write such fixup.
>
>
> /* Something bad happened! */
> if (!decoder)
> return -EINVAL;
>
> >
> > In the case of pure DVB devices (e. g. no dev->decoder), it should
> > just enable the DVB path.
> >
> > In the case of devices without tuner, it should use the same logic
> > needed to handle the S-Video/Composite connector inputs.
> >
> > Btw, I'm not seeing how this logic would do the right thing if the user
> > selects either S-Video or Composite connectors.
> >
> >> +
> >> + /*
> >> + * For Audio and V4L2 entity, find the link to which decoder
> >> + * is the sink. Look for an active link between decoder and
> >> + * tuner, if one exists, nothing to do. If not, look for any
> >> + * active links between tuner and any other entity. If one
> >> + * exists, tuner is busy. If tuner is free, setup link and
> >> + * start pipeline from source (tuner).
> >> + * For DVB FE entity, the source for the link is the tuner.
> >> + * Check if tuner is available and setup link and start
> >> + * pipeline.
> >> + */
> >> + if (entity->function != MEDIA_ENT_F_DTV_DEMOD)
> >> + sink = dev->decoder;
> >> + else
> >> + sink = entity;
> >> +
> >> + /* Is an active link between sink and tuner */
> >> + if (dev->active_link) {
> >> + if (dev->active_link->sink->entity == sink &&
> >> + dev->active_link->source->entity == dev->tuner) {
> >> + ret = 0;
> >> + goto end;
> >> + } else {
> >> + ret = -EBUSY;
> >> + goto end;
> >> + }
> >> + }
> >> +
> >> + list_for_each_entry(link, &sink->links, list) {
> >> + /* Check sink, and source */
> >> + if (link->sink->entity == sink &&
> >> + link->source->entity == dev->tuner) {
> >> + found_link = link;
> >> + break;
> >> + }
> >> + }
> >> +
> >> + if (!found_link) {
> >> + ret = -ENODEV;
> >> + goto end;
> >> + }
> >> +
> >> + /* activate link between source and sink and start pipeline */
> >> + source = found_link->source->entity;
> >> + ret = __media_entity_setup_link(found_link, MEDIA_LNK_FL_ENABLED);
> >> + if (ret) {
> >> + pr_err(
> >> + "Activate tuner link %s->%s. Error %d\n",
> >> + source->name, sink->name, ret);
> >> + goto end;
> >> + }
> >> +
> >> + ret = __media_entity_pipeline_start(entity, pipe);
> >> + if (ret) {
> >> + pr_err("Start Pipeline: %s->%s Error %d\n",
> >> + source->name, entity->name, ret);
> >> + ret = __media_entity_setup_link(found_link, 0);
> >> + pr_err("Deactive link Error %d\n", ret);
> >> + goto end;
> >> + }
> >
> > Hmm... isn't it to early to activate the pipeline here? My original
> > guess is that, on the analog side, this should happen only at the stream
> > on code. Wouldn't this break apps like mythTV?
> >
> >> + /*
> >> + * save active link and active link owner to avoid audio
> >> + * deactivating video owned link from disable_source and
> >> + * vice versa
> >> + */
> >> + dev->active_link = found_link;
> >> + dev->active_link_owner = entity;
> >> +end:
> >> + mutex_unlock(&mdev->graph_mutex);
> >> + pr_debug("au0828_enable_source() end %s %d %d\n",
> >> + entity->name, entity->function, ret);
> >> + return ret;
> >> +#endif
> >> + return 0;
> >> +}
> >> +
> >> +static void au0828_disable_source(struct media_entity *entity)
> >> +{
> >> +#ifdef CONFIG_MEDIA_CONTROLLER
> >> + struct media_entity *sink;
> >> + int ret = 0;
> >> + struct media_device *mdev = entity->graph_obj.mdev;
> >> + struct au0828_dev *dev;
> >> +
> >> + if (!mdev)
> >> + return;
> >> +
> >> + mutex_lock(&mdev->graph_mutex);
> >> + dev = mdev->source_priv;
> >> + if (!dev->tuner || !dev->decoder || !dev->active_link) {
> >> + ret = -ENODEV;
> >> + goto end;
> >> + }
> >
> > Same note as before.
>
> Same comment as before here about au0828_create_media_graph()
> and au0828_usb_probe() handling.
>
> >
> >> +
> >> + if (entity->function != MEDIA_ENT_F_DTV_DEMOD)
> >> + sink = dev->decoder;
> >> + else
> >> + sink = entity;
> >> +
> >> + /* link is active - stop pipeline from source (tuner) */
> >> + if (dev->active_link && dev->active_link->sink->entity == sink &&
> >> + dev->active_link->source->entity == dev->tuner) {
> >> + /*
> >> + * prevent video from deactivating link when audio
> >> + * has active pipeline
> >> + */
> >> + if (dev->active_link_owner != entity)
> >> + goto end;
> >> + __media_entity_pipeline_stop(entity);
> >> + ret = __media_entity_setup_link(dev->active_link, 0);
> >> + if (ret)
> >> + pr_err("Deactive link Error %d\n", ret);
> >> + dev->active_link = NULL;
> >> + dev->active_link_owner = NULL;
> >> + }
> >
> > Most code here looks like the one at au0828_enable_source(). Wouldn't
> > be simpler to merge those code and add a "bool enable" to the function
> > parameters?
>
> I would rather keep these separate. A very short
> section is common really.
>
> thanks,
> -- Shuah
>
> >
> >> +
> >> +end:
> >> + mutex_unlock(&mdev->graph_mutex);
> >> +#endif
> >> +}
> >> +
> >> static int au0828_media_device_register(struct au0828_dev *dev,
> >> struct usb_device *udev)
> >> {
> >> @@ -403,6 +548,10 @@ static int au0828_media_device_register(struct au0828_dev *dev,
> >> ret);
> >> return ret;
> >> }
> >> + /* set enable_source */
> >> + dev->media_dev->source_priv = (void *) dev;
> >> + dev->media_dev->enable_source = au0828_enable_source;
> >> + dev->media_dev->disable_source = au0828_disable_source;
> >> #endif
> >> return 0;
> >> }
> >> diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h
> >> index 54379ec..a7c88a1 100644
> >> --- a/drivers/media/usb/au0828/au0828.h
> >> +++ b/drivers/media/usb/au0828/au0828.h
> >> @@ -284,6 +284,9 @@ struct au0828_dev {
> >> struct media_entity input_ent[AU0828_MAX_INPUT];
> >> struct media_pad input_pad[AU0828_MAX_INPUT];
> >> struct media_entity_notify entity_notify;
> >> + struct media_entity *tuner;
> >> + struct media_link *active_link;
> >> + struct media_entity *active_link_owner;
> >> #endif
> >> };
> >>
>
>
--
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
