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 <[email protected]> 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 <[email protected]>
>>> ---
>>> 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
>>> };
>>>
>
>
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
[email protected] | (970) 217-8978
