Re: [PATCH v2 11/22] media: dvb-frontend invoke enable/disable_source handlers

[Shuah Khan <shuahkh@osg.samsung.com>]

On 02/04/2016 02:35 AM, Mauro Carvalho Chehab wrote:
> Em Wed, 03 Feb 2016 21:03:43 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
>> Change dvb frontend to check if tuner is free when
>> device opened in RW mode. Call to enable_source
>> handler either returns with an active pipeline to
>> tuner or error if tuner is busy. Tuner is released
>> when frontend is released calling the disable_source
>> handler.
> 
> This patch seems too early in the series, as I'm not seeing any patch
> providing a replacement for the removed code yet.

Removed linux-api from cc:

Yes you are right - I will move this after au0828 patch
that adds the handler.

> 
>>
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>  drivers/media/dvb-core/dvb_frontend.c | 139 +++++-----------------------------
>>  drivers/media/dvb-core/dvb_frontend.h |   3 +
>>  2 files changed, 24 insertions(+), 118 deletions(-)
>>
>> diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c
>> index 03cc508..2b17e8b 100644
>> --- a/drivers/media/dvb-core/dvb_frontend.c
>> +++ b/drivers/media/dvb-core/dvb_frontend.c
>> @@ -131,11 +131,6 @@ struct dvb_frontend_private {
>>  	int quality;
>>  	unsigned int check_wrapped;
>>  	enum dvbfe_search algo_status;
>> -
>> -#if defined(CONFIG_MEDIA_CONTROLLER_DVB)
>> -	struct media_pipeline pipe;
>> -	struct media_entity *pipe_start_entity;
>> -#endif
>>  };
>>  
>>  static void dvb_frontend_wakeup(struct dvb_frontend *fe);
>> @@ -596,104 +591,12 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe)
>>  	wake_up_interruptible(&fepriv->wait_queue);
>>  }
>>  
>> -/**
>> - * dvb_enable_media_tuner() - tries to enable the DVB tuner
>> - *
>> - * @fe:		struct dvb_frontend pointer
>> - *
>> - * This function ensures that just one media tuner is enabled for a given
>> - * frontend. It has two different behaviors:
>> - * - For trivial devices with just one tuner:
>> - *   it just enables the existing tuner->fe link
>> - * - For devices with more than one tuner:
>> - *   It is up to the driver to implement the logic that will enable one tuner
>> - *   and disable the other ones. However, if more than one tuner is enabled for
>> - *   the same frontend, it will print an error message and return -EINVAL.
>> - *
>> - * At return, it will return the error code returned by media_entity_setup_link,
>> - * or 0 if everything is OK, if no tuner is linked to the frontend or if the
>> - * mdev is NULL.
>> - */
>> -#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -static int dvb_enable_media_tuner(struct dvb_frontend *fe)
>> -{
>> -	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>> -	struct dvb_adapter *adapter = fe->dvb;
>> -	struct media_device *mdev = adapter->mdev;
>> -	struct media_entity  *entity, *source;
>> -	struct media_link *link, *found_link = NULL;
>> -	int ret, n_links = 0, active_links = 0;
>> -
>> -	fepriv->pipe_start_entity = NULL;
>> -
>> -	if (!mdev)
>> -		return 0;
>> -
>> -	entity = fepriv->dvbdev->entity;
>> -	fepriv->pipe_start_entity = entity;
>> -
>> -	list_for_each_entry(link, &entity->links, list) {
>> -		if (link->sink->entity == entity) {
>> -			found_link = link;
>> -			n_links++;
>> -			if (link->flags & MEDIA_LNK_FL_ENABLED)
>> -				active_links++;
>> -		}
>> -	}
>> -
>> -	if (!n_links || active_links == 1 || !found_link)
>> -		return 0;
>> -
>> -	/*
>> -	 * If a frontend has more than one tuner linked, it is up to the driver
>> -	 * to select with one will be the active one, as the frontend core can't
>> -	 * guess. If the driver doesn't do that, it is a bug.
>> -	 */
>> -	if (n_links > 1 && active_links != 1) {
>> -		dev_err(fe->dvb->device,
>> -			"WARNING: there are %d active links among %d tuners. This is a driver's bug!\n",
>> -			active_links, n_links);
>> -		return -EINVAL;
>> -	}
>> -
>> -	source = found_link->source->entity;
>> -	fepriv->pipe_start_entity = source;
>> -	list_for_each_entry(link, &source->links, list) {
>> -		struct media_entity *sink;
>> -		int flags = 0;
>> -
>> -		sink = link->sink->entity;
>> -		if (sink == entity)
>> -			flags = MEDIA_LNK_FL_ENABLED;
>> -
>> -		ret = media_entity_setup_link(link, flags);
>> -		if (ret) {
>> -			dev_err(fe->dvb->device,
>> -				"Couldn't change link %s->%s to %s. Error %d\n",
>> -				source->name, sink->name,
>> -				flags ? "enabled" : "disabled",
>> -				ret);
>> -			return ret;
>> -		} else
>> -			dev_dbg(fe->dvb->device,
>> -				"link %s->%s was %s\n",
>> -				source->name, sink->name,
>> -				flags ? "ENABLED" : "disabled");
>> -	}
>> -	return 0;
>> -}
>> -#endif
>> -
>>  static int dvb_frontend_thread(void *data)
>>  {
>>  	struct dvb_frontend *fe = data;
>>  	struct dvb_frontend_private *fepriv = fe->frontend_priv;
>>  	enum fe_status s;
>>  	enum dvbfe_algo algo;
>> -#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -	int ret;
>> -#endif
>> -
>>  	bool re_tune = false;
>>  	bool semheld = false;
>>  
>> @@ -706,20 +609,6 @@ static int dvb_frontend_thread(void *data)
>>  	fepriv->wakeup = 0;
>>  	fepriv->reinitialise = 0;
>>  
>> -#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -	ret = dvb_enable_media_tuner(fe);
>> -	if (ret) {
>> -		/* FIXME: return an error if it fails */
>> -		dev_info(fe->dvb->device,
>> -			"proceeding with FE task\n");
>> -	} else if (fepriv->pipe_start_entity) {
>> -		ret = media_entity_pipeline_start(fepriv->pipe_start_entity,
>> -						  &fepriv->pipe);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -#endif
>> -
>>  	dvb_frontend_init(fe);
>>  
>>  	set_freezable();
>> @@ -829,12 +718,6 @@ restart:
>>  		}
>>  	}
>>  
>> -#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> -	if (fepriv->pipe_start_entity)
>> -		media_entity_pipeline_stop(fepriv->pipe_start_entity);
>> -	fepriv->pipe_start_entity = NULL;
>> -#endif
>> -
>>  	if (dvb_powerdown_on_sleep) {
>>  		if (fe->ops.set_voltage)
>>  			fe->ops.set_voltage(fe, SEC_VOLTAGE_OFF);
>> @@ -2612,9 +2495,20 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>>  		fepriv->tone = -1;
>>  		fepriv->voltage = -1;
>>  
>> +#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> +		if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
>> +			ret = fe->dvb->mdev->enable_source(dvbdev->entity,
>> +							   &fe->pipe);
>> +			if (ret) {
>> +				dev_err(fe->dvb->device,
>> +					"Tuner is busy. Error %d\n", ret);
>> +				goto err2;
>> +			}
>> +		}
>> +#endif
>>  		ret = dvb_frontend_start (fe);
>>  		if (ret)
>> -			goto err2;
>> +			goto err3;
>>  
>>  		/*  empty event queue */
>>  		fepriv->events.eventr = fepriv->events.eventw = 0;
>> @@ -2624,7 +2518,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file)
>>  		mutex_unlock (&adapter->mfe_lock);
>>  	return ret;
>>  
>> +err3:
>> +#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> +	if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
>> +		fe->dvb->mdev->disable_source(dvbdev->entity);
>>  err2:
>> +#endif
>>  	dvb_generic_release(inode, file);
>>  err1:
>>  	if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl)
>> @@ -2653,6 +2552,10 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)
>>  
>>  	if (dvbdev->users == -1) {
>>  		wake_up(&fepriv->wait_queue);
>> +#ifdef CONFIG_MEDIA_CONTROLLER_DVB
>> +		if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
>> +			fe->dvb->mdev->disable_source(dvbdev->entity);
>> +#endif
>>  		if (fe->exit != DVB_FE_NO_EXIT)
>>  			wake_up(&dvbdev->wait_queue);
>>  		if (fe->ops.ts_bus_ctrl)
>> diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h
>> index 458bcce..9466906 100644
>> --- a/drivers/media/dvb-core/dvb_frontend.h
>> +++ b/drivers/media/dvb-core/dvb_frontend.h
>> @@ -686,6 +686,9 @@ struct dvb_frontend {
>>  	int (*callback)(void *adapter_priv, int component, int cmd, int arg);
>>  	int id;
>>  	unsigned int exit;
>> +#if defined(CONFIG_MEDIA_CONTROLLER_DVB)
>> +	struct media_pipeline pipe;
>> +#endif
> 
> Why moving this to the non-private part of the data?
> 

There was a reason for this move and I don't recall
at the moment. It has been several months since I
had to rework. I will make sure this change is
necessary and include it in the changelog.

thanks,
-- Shuah


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