On 02/08/2016 03:42 PM, Shuah Khan wrote:
On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote:
quoted
Em Wed, 03 Feb 2016 21:03:52 -0700
Shuah Khan [off-list ref] escreveu:
quoted
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 <redacted>
---
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;
quoted
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.
quoted
+
+ /*
+ * 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.
quoted
quoted
+ /*
+ * 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.
quoted
quoted
+
+ 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
quoted
quoted
+
+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
};
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978