Re: [PATCH v2 20/22] media: au0828 add enable, disable source handlers

[Shuah Khan]

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()?


       /* 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
>>  };
>>  


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978