Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320817 > unrolled thread

Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source handlers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source  handlers Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-01-28 17:00 +0100
    Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source  handlers One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-01-28 17:10 +0100
      Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source  handlers Shuah Khan <shuahkh@osg.samsung.com> - 2016-01-28 18:00 +0100
        Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source  handlers Mauro Carvalho Chehab <mchehab@osg.samsung.com> - 2016-01-28 18:40 +0100

#1320817 — Re: [PATCH 15/31] media: dvb-frontend invoke enable/disable_source handlers

Em Wed,  6 Jan 2016 13:27:04 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> Checking for tuner availability from frontend thread start
> disrupts video stream. Change to check for tuner and start
> pipeline from frontend open instead and stop pipeline from
> frontend release.

That's wrong, as DVB can be opened on read-only mode, where
it won't be changing anything.

Also, I don't think POSIX allows to return an error like EBUSY
on open:
	http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

> In addition, make a change to invoke
> enable_source and disable_source handlers to check for
> tuner availability. The enable_source handler finds tuner
> entity connected to the decoder and check is it is available
> or busy. If tuner is available, link is activated and pipeline
> is started. The disable_source handler to deactivate and stop
> the pipeline. dvb_enable_media_tuner() is removed as it is no
> longer necessary with dvb invoking enable_source and
> disable_source handlers. pipe_start_entity field is removed
> and pipe field is moved to dvb_frontend from dvb_frontend_private.
> 
> 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 4008064..c15f3d8 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
>  };
>  
>  /**

[toc] | [next] | [standalone]

#1320830

On Thu, 28 Jan 2016 13:53:04 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:

> Em Wed,  6 Jan 2016 13:27:04 -0700
> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> 
> > Checking for tuner availability from frontend thread start
> > disrupts video stream. Change to check for tuner and start
> > pipeline from frontend open instead and stop pipeline from
> > frontend release.  
> 
> That's wrong, as DVB can be opened on read-only mode, where
> it won't be changing anything.
> 
> Also, I don't think POSIX allows to return an error like EBUSY
> on open:
> 	http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

It doesn't document all the errors you may return. Quite a lot of
kernel drivers return EBUSY when they are "single open" things.

POSIX documents certain cases that *must* error and what the error code
is. It documents certain possible failures and what their error code is.
Beyond that it's up to you.

Alan

[toc] | [prev] | [next] | [standalone]

#1320870

On 01/28/2016 09:07 AM, One Thousand Gnomes wrote:
> On Thu, 28 Jan 2016 13:53:04 -0200
> Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:
> 
>> Em Wed,  6 Jan 2016 13:27:04 -0700
>> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
>>
>>> Checking for tuner availability from frontend thread start
>>> disrupts video stream. Change to check for tuner and start
>>> pipeline from frontend open instead and stop pipeline from
>>> frontend release.  
>>
>> That's wrong, as DVB can be opened on read-only mode, where
>> it won't be changing anything.

Correct. Please check the code. Tuner availability check is
done when the device is opened in Write mode in the following
conditional.

       if ((file->f_flags & O_ACCMODE) != O_RDONLY) {

>>
>> Also, I don't think POSIX allows to return an error like EBUSY
>> on open:
>> 	http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
> 
> It doesn't document all the errors you may return. Quite a lot of
> kernel drivers return EBUSY when they are "single open" things.
> 
> POSIX documents certain cases that *must* error and what the error code
> is. It documents certain possible failures and what their error code is.
> Beyond that it's up to you.
> 

Returning EBUSY clearly indicates the reason why the device
can't be opened in this case.

thanks,
-- Shuah


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

[toc] | [prev] | [next] | [standalone]

#1320904

Em Thu, 28 Jan 2016 09:59:38 -0700
Shuah Khan <shuahkh@osg.samsung.com> escreveu:

> On 01/28/2016 09:07 AM, One Thousand Gnomes wrote:
> > On Thu, 28 Jan 2016 13:53:04 -0200
> > Mauro Carvalho Chehab <mchehab@osg.samsung.com> wrote:
> >   
> >> Em Wed,  6 Jan 2016 13:27:04 -0700
> >> Shuah Khan <shuahkh@osg.samsung.com> escreveu:
> >>  
> >>> Checking for tuner availability from frontend thread start
> >>> disrupts video stream. Change to check for tuner and start
> >>> pipeline from frontend open instead and stop pipeline from
> >>> frontend release.    
> >>
> >> That's wrong, as DVB can be opened on read-only mode, where
> >> it won't be changing anything.  
> 
> Correct. Please check the code. Tuner availability check is
> done when the device is opened in Write mode in the following
> conditional.
> 
>        if ((file->f_flags & O_ACCMODE) != O_RDONLY) {

OK.

Thanks!
Mauro
, I don't think POSIX allows to return an error like EBUSY
> >> on open:
> >> 	http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html  
> > 
> > It doesn't document all the errors you may return. Quite a lot of
> > kernel drivers return EBUSY when they are "single open" things.
> > 
> > POSIX documents certain cases that *must* error and what the error code
> > is. It documents certain possible failures and what their error code is.
> > Beyond that it's up to you.
> >   
> 
> Returning EBUSY clearly indicates the reason why the device
> can't be opened in this case.
> 
> thanks,
> -- Shuah
> 
> 
Thanks!
Mauro

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web