| Message ID | 1ebb3d41fa42581f8741e493f3109357ad1a0b3c.1454557589.git.shuahkh@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Wed, 03 Feb 2016 21:03:52 -0700 Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com> > --- > 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). 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. > + > + 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? > + > +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 > }; >
On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote: > Em Wed, 03 Feb 2016 21:03:52 -0700 > Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com> >> --- >> 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()? /* 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 >> }; >>
On 02/08/2016 03:42 PM, Shuah Khan wrote: > On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote: >> Em Wed, 03 Feb 2016 21:03:52 -0700 >> Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com> >>> --- >>> 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()? > > > /* 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? On analog side, there are a few ioctls that change the configuration on the tuner way before stream on step. Is there a reason to separate the setup_link() and pipeline_start() steps? I can separate these two steps, but I am not really seeing the reason for that. >> >>> + /* >>> + * 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 >>> }; >>> > >
Em Mon, 08 Feb 2016 15:42:15 -0700 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote: > > Em Wed, 03 Feb 2016 21:03:52 -0700 > > Shuah Khan <shuahkh@osg.samsung.com> 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@osg.samsung.com> > >> --- > >> 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 > >> }; > >> > >
Em Mon, 08 Feb 2016 16:00:26 -0700 Shuah Khan <shuahkh@osg.samsung.com> escreveu: > >>> + 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? > > On analog side, there are a few ioctls that > change the configuration on the tuner way > before stream on step. Is there a reason to > separate the setup_link() and pipeline_start() > steps? I can separate these two steps, but I > am not really seeing the reason for that. I'm actually ok with that, provided that it won't break existing apps. Did you test it with MythTV? Regards, Mauro
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; + } + + /* + * 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; + } + /* + * 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; + } + + 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; + } + +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 };
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@osg.samsung.com> --- drivers/media/usb/au0828/au0828-core.c | 149 +++++++++++++++++++++++++++++++++ drivers/media/usb/au0828/au0828.h | 3 + 2 files changed, 152 insertions(+)