[PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Andres Salomon]

By default, we allocate DMA buffers when actually reading from the video
capture device.  On a system with 128MB or 256MB of ram, it's very easy
for that memory to quickly become fragmented.  We've had users report
having 30+MB of memory free, but the cafe_ccic driver is still unable to
allocate DMA buffers.

Our workaround has been to make use of the 'alloc_bufs_at_load' parameter
to allocate DMA buffers during device probing.  This patch makes DMA
buffer allocation happen during device probe by default, and changes
the parameter to 'alloc_bufs_at_read'.  The camera hardware is there,
if the cafe_ccic driver is enabled/loaded it should do its best to ensure
that the camera is actually usable; delaying DMA buffer allocation
saves an insignicant amount of memory, and causes the driver to be much
less useful.
---

 drivers/media/video/cafe_ccic.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
index ef53618..3588a59 100644
--- a/drivers/media/video/cafe_ccic.c
+++ b/drivers/media/video/cafe_ccic.c
@@ -63,13 +63,13 @@ MODULE_SUPPORTED_DEVICE("Video");
  */
 
 #define MAX_DMA_BUFS 3
-static int alloc_bufs_at_load = 0;
-module_param(alloc_bufs_at_load, bool, 0444);
-MODULE_PARM_DESC(alloc_bufs_at_load,
-		"Non-zero value causes DMA buffers to be allocated at module "
-		"load time.  This increases the chances of successfully getting "
-		"those buffers, but at the cost of nailing down the memory from "
-		"the outset.");
+static int alloc_bufs_at_read = 0;
+module_param(alloc_bufs_at_read, bool, 0444);
+MODULE_PARM_DESC(alloc_bufs_at_read,
+		"Non-zero value causes DMA buffers to be allocated when the "
+		"video capture device is read, rather than at module load "
+		"time.  This saves memory, but decreases the chances of "
+		"successfully getting those buffers.");
 
 static int n_dma_bufs = 3;
 module_param(n_dma_bufs, uint, 0644);
@@ -1503,7 +1503,7 @@ static int cafe_v4l_release(struct inode *inode, struct file *filp)
 	}
 	if (cam->users == 0) {
 		cafe_ctlr_power_down(cam);
-		if (! alloc_bufs_at_load)
+		if (alloc_bufs_at_read)
 			cafe_free_dma_bufs(cam);
 	}
 	mutex_unlock(&cam->s_mutex);
@@ -2162,7 +2162,7 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	/*
 	 * If so requested, try to get our DMA buffers now.
 	 */
-	if (alloc_bufs_at_load) {
+	if (!alloc_bufs_at_read) {
 		if (cafe_alloc_dma_bufs(cam, 1))
 			cam_warn(cam, "Unable to alloc DMA buffers at load"
 					" will try again later.");

Re: [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Jonathan Corbet]

Andres Salomon <dilinger@queued.net> wrote:

> This patch makes DMA buffer allocation happen during device probe by
> default, and changes the parameter to 'alloc_bufs_at_read'.  The
> camera hardware is there, if the cafe_ccic driver is enabled/loaded it
> should do its best to ensure that the camera is actually usable;
> delaying DMA buffer allocation saves an insignicant amount of memory,
> and causes the driver to be much less useful.

The amount of memory isn't quite "insignificant" (three 640x480x2
buffers), but the OLPC people clearly need things to work this way.
There are, as far as I know, no other systems out there using the CAFE
controller.  Making things work by default for the one user (with a fair
number of deployed systems!) makes sense to me.  So...

  Acked-by: Jonathan Corbet <corbet@lwn.net>

That said, I do prefer the original name for the parameter.

jon

Jonathan Corbet / LWN.net / corbet@lwn.net

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Trent Piepho]

On Wed, 19 Sep 2007, Jonathan Corbet wrote:
> Andres Salomon <dilinger@queued.net> wrote:
> > This patch makes DMA buffer allocation happen during device probe by
> > default, and changes the parameter to 'alloc_bufs_at_read'.  The
> > camera hardware is there, if the cafe_ccic driver is enabled/loaded it
> > should do its best to ensure that the camera is actually usable;
> > delaying DMA buffer allocation saves an insignicant amount of memory,
> > and causes the driver to be much less useful.
>
> The amount of memory isn't quite "insignificant" (three 640x480x2
> buffers), but the OLPC people clearly need things to work this way.
> There are, as far as I know, no other systems out there using the CAFE
> controller.  Making things work by default for the one user (with a fair
> number of deployed systems!) makes sense to me.  So...
>
>   Acked-by: Jonathan Corbet <corbet@lwn.net>
>
> That said, I do prefer the original name for the parameter.

Changing the parameter name will break the configuration of anyone who was
using the existing parameter.  It also means everyone who has multiple
kernels installed and uses this parameter will need to have one
configuration for old kernels and a different configuration for new
kernels.

In the default really needs to be changed (what's so hard about setting
alloc_bufs_at_load in /etc/modprobe{.d,.config}?) then just change the
default of the existing parameter.  That avoids the whole problem with
different configration files for different module versions.

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Andres Salomon]

On Wed, 19 Sep 2007 15:34:42 -0700 (PDT)
Trent Piepho <xyzzy@speakeasy.org> wrote:

> On Wed, 19 Sep 2007, Jonathan Corbet wrote:
> > Andres Salomon <dilinger@queued.net> wrote:
> > > This patch makes DMA buffer allocation happen during device probe by
> > > default, and changes the parameter to 'alloc_bufs_at_read'.  The
> > > camera hardware is there, if the cafe_ccic driver is enabled/loaded it
> > > should do its best to ensure that the camera is actually usable;
> > > delaying DMA buffer allocation saves an insignicant amount of memory,
> > > and causes the driver to be much less useful.
> >
> > The amount of memory isn't quite "insignificant" (three 640x480x2
> > buffers), but the OLPC people clearly need things to work this way.
> > There are, as far as I know, no other systems out there using the CAFE
> > controller.  Making things work by default for the one user (with a fair
> > number of deployed systems!) makes sense to me.  So...
> >
> >   Acked-by: Jonathan Corbet <corbet@lwn.net>
> >
> > That said, I do prefer the original name for the parameter.
> 
> Changing the parameter name will break the configuration of anyone who was
> using the existing parameter.  It also means everyone who has multiple

We (OLPC) are the only ones using this driver, as we are the only ones with
this hardware.  Marvell designed the cafe for us, and it is not available
on the market yet.  Backwards compatibility at this point is not a concern.

> kernels installed and uses this parameter will need to have one
> configuration for old kernels and a different configuration for new
> kernels.
> 
> In the default really needs to be changed (what's so hard about setting
> alloc_bufs_at_load in /etc/modprobe{.d,.config}?) then just change the
> default of the existing parameter.  That avoids the whole problem with
> different configration files for different module versions.

We're building the driver in statically; there's no point for us to make
it a module.  We could add a kernel argument, yes, but that quickly
becomes unwieldy as we require more and more.  We've actually been trying
to cut down the number of kernel args we use.


That said, I'm not opposed to keeping the parameter name the same while
making the default 1; I just thought that the name 'alloc_bufs_at_read' was
clearer.  Another option is to change it to 'no_alloc_bufs_at_load'.  Jon,
any preference there?


-- 
Andres Salomon <dilinger@queued.net>

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Trent Piepho]

On Wed, 19 Sep 2007, Andres Salomon wrote:
> On Wed, 19 Sep 2007 15:34:42 -0700 (PDT)
> Trent Piepho <xyzzy@speakeasy.org> wrote:
> > On Wed, 19 Sep 2007, Jonathan Corbet wrote:
> > > Andres Salomon <dilinger@queued.net> wrote:
> > > > This patch makes DMA buffer allocation happen during device probe by
> > > > default, and changes the parameter to 'alloc_bufs_at_read'.  The
> > > > camera hardware is there, if the cafe_ccic driver is enabled/loaded it
> > > > should do its best to ensure that the camera is actually usable;
> > > > delaying DMA buffer allocation saves an insignicant amount of memory,
> > > > and causes the driver to be much less useful.
> > >
> > Changing the parameter name will break the configuration of anyone who was
> > using the existing parameter.  It also means everyone who has multiple
>
> We (OLPC) are the only ones using this driver, as we are the only ones with
> this hardware.  Marvell designed the cafe for us, and it is not available
> on the market yet.  Backwards compatibility at this point is not a concern.
>
> > kernels installed and uses this parameter will need to have one
> > configuration for old kernels and a different configuration for new
> > kernels.
> >
> > In the default really needs to be changed (what's so hard about setting
> > alloc_bufs_at_load in /etc/modprobe{.d,.config}?) then just change the
> > default of the existing parameter.  That avoids the whole problem with
> > different configration files for different module versions.
>
> We're building the driver in statically; there's no point for us to make
> it a module.  We could add a kernel argument, yes, but that quickly
> becomes unwieldy as we require more and more.  We've actually been trying
> to cut down the number of kernel args we use.

Ok, that seems like a good reason to change the default.
>
>
> That said, I'm not opposed to keeping the parameter name the same while
> making the default 1; I just thought that the name 'alloc_bufs_at_read' was
> clearer.  Another option is to change it to 'no_alloc_bufs_at_load'.  Jon,
> any preference there?

It seems like the name 'alloc_bufs_at_read' isn't quite right, since the
buffers will be allocated when the format is set, from
cafe_vidioc_s_fmt_cap().

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Jonathan Corbet]

Andres Salomon <dilinger@queued.net> wrote:

> That said, I'm not opposed to keeping the parameter name the same while
> making the default 1; I just thought that the name 'alloc_bufs_at_read' was
> clearer.  Another option is to change it to 'no_alloc_bufs_at_load'.  Jon,
> any preference there?

I don't think that negating it by adding no_ at the front helps much.
In general, I prefer the name it has now, but it's not *that* big a
deal.  We've probably already expended more bandwidth than it's worth.

jon

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Mauro Carvalho Chehab]

Hi Andres,

Still missing on your patch is your SOB ;) From what I got from Jon, he
is ok with your patch.

Cheers,
Mauro.

Em Qua, 2007-09-19 às 01:44 -0400, Andres Salomon escreveu:
> By default, we allocate DMA buffers when actually reading from the video
> capture device.  On a system with 128MB or 256MB of ram, it's very easy
> for that memory to quickly become fragmented.  We've had users report
> having 30+MB of memory free, but the cafe_ccic driver is still unable to
> allocate DMA buffers.
> 
> Our workaround has been to make use of the 'alloc_bufs_at_load' parameter
> to allocate DMA buffers during device probing.  This patch makes DMA
> buffer allocation happen during device probe by default, and changes
> the parameter to 'alloc_bufs_at_read'.  The camera hardware is there,
> if the cafe_ccic driver is enabled/loaded it should do its best to ensure
> that the camera is actually usable; delaying DMA buffer allocation
> saves an insignicant amount of memory, and causes the driver to be much
> less useful.
> ---
> 
>  drivers/media/video/cafe_ccic.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
> index ef53618..3588a59 100644
> --- a/drivers/media/video/cafe_ccic.c
> +++ b/drivers/media/video/cafe_ccic.c
> @@ -63,13 +63,13 @@ MODULE_SUPPORTED_DEVICE("Video");
>   */
>  
>  #define MAX_DMA_BUFS 3
> -static int alloc_bufs_at_load = 0;
> -module_param(alloc_bufs_at_load, bool, 0444);
> -MODULE_PARM_DESC(alloc_bufs_at_load,
> -		"Non-zero value causes DMA buffers to be allocated at module "
> -		"load time.  This increases the chances of successfully getting "
> -		"those buffers, but at the cost of nailing down the memory from "
> -		"the outset.");
> +static int alloc_bufs_at_read = 0;
> +module_param(alloc_bufs_at_read, bool, 0444);
> +MODULE_PARM_DESC(alloc_bufs_at_read,
> +		"Non-zero value causes DMA buffers to be allocated when the "
> +		"video capture device is read, rather than at module load "
> +		"time.  This saves memory, but decreases the chances of "
> +		"successfully getting those buffers.");
>  
>  static int n_dma_bufs = 3;
>  module_param(n_dma_bufs, uint, 0644);
> @@ -1503,7 +1503,7 @@ static int cafe_v4l_release(struct inode *inode, struct file *filp)
>  	}
>  	if (cam->users == 0) {
>  		cafe_ctlr_power_down(cam);
> -		if (! alloc_bufs_at_load)
> +		if (alloc_bufs_at_read)
>  			cafe_free_dma_bufs(cam);
>  	}
>  	mutex_unlock(&cam->s_mutex);
> @@ -2162,7 +2162,7 @@ static int cafe_pci_probe(struct pci_dev *pdev,
>  	/*
>  	 * If so requested, try to get our DMA buffers now.
>  	 */
> -	if (alloc_bufs_at_load) {
> +	if (!alloc_bufs_at_read) {
>  		if (cafe_alloc_dma_bufs(cam, 1))
>  			cam_warn(cam, "Unable to alloc DMA buffers at load"
>  					" will try again later.");
> 
> _______________________________________________
> v4l-dvb-maintainer mailing list
> v4l-dvb-maintainer@linuxtv.org
> http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer
-- 
Cheers,
Mauro

Re: [v4l-dvb-maintainer] [PATCH] cafe_ccic: default to allocating DMA buffers at probe time

[Andres Salomon]

On Mon, 24 Sep 2007 12:04:17 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:

> Hi Andres,
> 
> Still missing on your patch is your SOB ;) From what I got from Jon, he
> is ok with your patch.
> 
> Cheers,
> Mauro.


Whoops, sorry about that!   well, here it is (for the patch below):

Signed-off-by: Andres Salomon <dilinger@debian.org>


> 
> Em Qua, 2007-09-19 às 01:44 -0400, Andres Salomon escreveu:
> > By default, we allocate DMA buffers when actually reading from the video
> > capture device.  On a system with 128MB or 256MB of ram, it's very easy
> > for that memory to quickly become fragmented.  We've had users report
> > having 30+MB of memory free, but the cafe_ccic driver is still unable to
> > allocate DMA buffers.
> > 
> > Our workaround has been to make use of the 'alloc_bufs_at_load' parameter
> > to allocate DMA buffers during device probing.  This patch makes DMA
> > buffer allocation happen during device probe by default, and changes
> > the parameter to 'alloc_bufs_at_read'.  The camera hardware is there,
> > if the cafe_ccic driver is enabled/loaded it should do its best to ensure
> > that the camera is actually usable; delaying DMA buffer allocation
> > saves an insignicant amount of memory, and causes the driver to be much
> > less useful.
> > ---
> > 
> >  drivers/media/video/cafe_ccic.c |   18 +++++++++---------
> >  1 files changed, 9 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/video/cafe_ccic.c b/drivers/media/video/cafe_ccic.c
> > index ef53618..3588a59 100644
> > --- a/drivers/media/video/cafe_ccic.c
> > +++ b/drivers/media/video/cafe_ccic.c
> > @@ -63,13 +63,13 @@ MODULE_SUPPORTED_DEVICE("Video");
> >   */
> >  
> >  #define MAX_DMA_BUFS 3
> > -static int alloc_bufs_at_load = 0;
> > -module_param(alloc_bufs_at_load, bool, 0444);
> > -MODULE_PARM_DESC(alloc_bufs_at_load,
> > -		"Non-zero value causes DMA buffers to be allocated at module "
> > -		"load time.  This increases the chances of successfully getting "
> > -		"those buffers, but at the cost of nailing down the memory from "
> > -		"the outset.");
> > +static int alloc_bufs_at_read = 0;
> > +module_param(alloc_bufs_at_read, bool, 0444);
> > +MODULE_PARM_DESC(alloc_bufs_at_read,
> > +		"Non-zero value causes DMA buffers to be allocated when the "
> > +		"video capture device is read, rather than at module load "
> > +		"time.  This saves memory, but decreases the chances of "
> > +		"successfully getting those buffers.");
> >  
> >  static int n_dma_bufs = 3;
> >  module_param(n_dma_bufs, uint, 0644);
> > @@ -1503,7 +1503,7 @@ static int cafe_v4l_release(struct inode *inode, struct file *filp)
> >  	}
> >  	if (cam->users == 0) {
> >  		cafe_ctlr_power_down(cam);
> > -		if (! alloc_bufs_at_load)
> > +		if (alloc_bufs_at_read)
> >  			cafe_free_dma_bufs(cam);
> >  	}
> >  	mutex_unlock(&cam->s_mutex);
> > @@ -2162,7 +2162,7 @@ static int cafe_pci_probe(struct pci_dev *pdev,
> >  	/*
> >  	 * If so requested, try to get our DMA buffers now.
> >  	 */
> > -	if (alloc_bufs_at_load) {
> > +	if (!alloc_bufs_at_read) {
> >  		if (cafe_alloc_dma_bufs(cam, 1))
> >  			cam_warn(cam, "Unable to alloc DMA buffers at load"
> >  					" will try again later.");
> > 
> > _______________________________________________
> > v4l-dvb-maintainer mailing list
> > v4l-dvb-maintainer@linuxtv.org
> > http://www.linuxtv.org/cgi-bin/mailman/listinfo/v4l-dvb-maintainer
> -- 
> Cheers,
> Mauro
> 


-- 
Andres Salomon <dilinger@queued.net>