All of lore.kernel.org
 help / color / mirror / Atom feed
* media: rc: make raw event fifo size a module parameter
@ 2016-03-18 17:43 Heiner Kallweit
  2016-05-07 13:56 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 2+ messages in thread
From: Heiner Kallweit @ 2016-03-18 17:43 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: linux-media

Currently the fifo size is 512 elements. After a recent patch the size
of struct ir_raw_event is down to 8 bytes, so the fifo still consumes
4KB. In most cases a much smaller fifo is sufficient, e.g. nuvoton-cir
triggers event processing after 24 events latest. However the needed
fifo size may also depend on system performance.

Therefore make the fifo size a module parameter with the current value
of 512 being the default.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/media/rc/rc-core-priv.h |  2 +-
 drivers/media/rc/rc-ir-raw.c    | 20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 585d5e5..ae6f81e 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -39,7 +39,7 @@ struct ir_raw_event_ctrl {
 	struct task_struct		*thread;
 	spinlock_t			lock;
 	/* fifo for the pulse/space durations */
-	DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
+	DECLARE_KFIFO_PTR(kfifo, struct ir_raw_event);
 	ktime_t				last_event;	/* when last event occurred */
 	enum raw_event_type		last_type;	/* last event type */
 	struct rc_dev			*dev;		/* pointer to the parent rc_dev */
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 144304c..620b036 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -18,6 +18,7 @@
 #include <linux/kmod.h>
 #include <linux/sched.h>
 #include <linux/freezer.h>
+#include <linux/module.h>
 #include "rc-core-priv.h"
 
 /* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
@@ -28,6 +29,9 @@ static DEFINE_MUTEX(ir_raw_handler_lock);
 static LIST_HEAD(ir_raw_handler_list);
 static u64 available_protocols;
 
+/* module param */
+static unsigned int ir_raw_event_fifo_size = MAX_IR_EVENT_SIZE;
+
 static int ir_raw_event_thread(void *data)
 {
 	struct ir_raw_event ev;
@@ -262,7 +266,7 @@ int ir_raw_event_register(struct rc_dev *dev)
 	int rc;
 	struct ir_raw_handler *handler;
 
-	if (!dev)
+	if (!dev || !ir_raw_event_fifo_size)
 		return -EINVAL;
 
 	dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
@@ -271,7 +275,10 @@ int ir_raw_event_register(struct rc_dev *dev)
 
 	dev->raw->dev = dev;
 	dev->change_protocol = change_protocol;
-	INIT_KFIFO(dev->raw->kfifo);
+
+	rc = kfifo_alloc(&dev->raw->kfifo, ir_raw_event_fifo_size, GFP_KERNEL);
+	if (rc)
+		goto out;
 
 	spin_lock_init(&dev->raw->lock);
 	dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
@@ -279,7 +286,7 @@ int ir_raw_event_register(struct rc_dev *dev)
 
 	if (IS_ERR(dev->raw->thread)) {
 		rc = PTR_ERR(dev->raw->thread);
-		goto out;
+		goto out_kfifo;
 	}
 
 	mutex_lock(&ir_raw_handler_lock);
@@ -291,6 +298,8 @@ int ir_raw_event_register(struct rc_dev *dev)
 
 	return 0;
 
+out_kfifo:
+	kfifo_free(&dev->raw->kfifo);
 out:
 	kfree(dev->raw);
 	dev->raw = NULL;
@@ -313,6 +322,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
 			handler->raw_unregister(dev);
 	mutex_unlock(&ir_raw_handler_lock);
 
+	kfifo_free(&dev->raw->kfifo);
 	kfree(dev->raw);
 	dev->raw = NULL;
 }
@@ -353,3 +363,7 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
 	mutex_unlock(&ir_raw_handler_lock);
 }
 EXPORT_SYMBOL(ir_raw_handler_unregister);
+
+module_param_named(fifo_size, ir_raw_event_fifo_size, uint, S_IRUGO);
+MODULE_PARM_DESC(fifo_size,
+	"raw event fifo size, will be rounded up to next power of 2");
-- 
2.7.3


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: media: rc: make raw event fifo size a module parameter
  2016-03-18 17:43 media: rc: make raw event fifo size a module parameter Heiner Kallweit
@ 2016-05-07 13:56 ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 2+ messages in thread
From: Mauro Carvalho Chehab @ 2016-05-07 13:56 UTC (permalink / raw)
  To: Heiner Kallweit; +Cc: linux-media

Hi Heiner,

Em Fri, 18 Mar 2016 18:43:49 +0100
Heiner Kallweit <hkallweit1@gmail.com> escreveu:

> Currently the fifo size is 512 elements. After a recent patch the size
> of struct ir_raw_event is down to 8 bytes, so the fifo still consumes
> 4KB. In most cases a much smaller fifo is sufficient, e.g. nuvoton-cir
> triggers event processing after 24 events latest. However the needed
> fifo size may also depend on system performance.
> 
> Therefore make the fifo size a module parameter with the current value
> of 512 being the default.

I didn't like this approach. If the user has more than one RC receiver
on his hardware, this approach would force all drivers to use the
same buffer size.

Instead, as this is actually a hardware limit, I guess the best is
to add a new field at struct rc_dev to allow the client drivers to
specify the desired buffer size. If zero, it would default to 512,
avoiding regressions.

That would, IMHO, give much more flexibility.

Btw, one of the things I missed on this patch is a logic that would
would prevent the user to specify crap values for buf size. 
If we let the user to specify it, some logic is need to prevent the
user to use an incredible small buffer and lose IR events (and complain
later that the driver is broken) or setup incredible big buffer, 
causing memory waste and probably causing other drivers to fail.

Regards,
Mauro.


> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/media/rc/rc-core-priv.h |  2 +-
>  drivers/media/rc/rc-ir-raw.c    | 20 +++++++++++++++++---
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 585d5e5..ae6f81e 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -39,7 +39,7 @@ struct ir_raw_event_ctrl {
>  	struct task_struct		*thread;
>  	spinlock_t			lock;
>  	/* fifo for the pulse/space durations */
> -	DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
> +	DECLARE_KFIFO_PTR(kfifo, struct ir_raw_event);
>  	ktime_t				last_event;	/* when last event occurred */
>  	enum raw_event_type		last_type;	/* last event type */
>  	struct rc_dev			*dev;		/* pointer to the parent rc_dev */
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 144304c..620b036 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -18,6 +18,7 @@
>  #include <linux/kmod.h>
>  #include <linux/sched.h>
>  #include <linux/freezer.h>
> +#include <linux/module.h>
>  #include "rc-core-priv.h"
>  
>  /* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
> @@ -28,6 +29,9 @@ static DEFINE_MUTEX(ir_raw_handler_lock);
>  static LIST_HEAD(ir_raw_handler_list);
>  static u64 available_protocols;
>  
> +/* module param */
> +static unsigned int ir_raw_event_fifo_size = MAX_IR_EVENT_SIZE;
> +
>  static int ir_raw_event_thread(void *data)
>  {
>  	struct ir_raw_event ev;
> @@ -262,7 +266,7 @@ int ir_raw_event_register(struct rc_dev *dev)
>  	int rc;
>  	struct ir_raw_handler *handler;
>  
> -	if (!dev)
> +	if (!dev || !ir_raw_event_fifo_size)
>  		return -EINVAL;
>  
>  	dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
> @@ -271,7 +275,10 @@ int ir_raw_event_register(struct rc_dev *dev)
>  
>  	dev->raw->dev = dev;
>  	dev->change_protocol = change_protocol;
> -	INIT_KFIFO(dev->raw->kfifo);
> +
> +	rc = kfifo_alloc(&dev->raw->kfifo, ir_raw_event_fifo_size, GFP_KERNEL);
> +	if (rc)
> +		goto out;
>  
>  	spin_lock_init(&dev->raw->lock);
>  	dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
> @@ -279,7 +286,7 @@ int ir_raw_event_register(struct rc_dev *dev)
>  
>  	if (IS_ERR(dev->raw->thread)) {
>  		rc = PTR_ERR(dev->raw->thread);
> -		goto out;
> +		goto out_kfifo;
>  	}
>  
>  	mutex_lock(&ir_raw_handler_lock);
> @@ -291,6 +298,8 @@ int ir_raw_event_register(struct rc_dev *dev)
>  
>  	return 0;
>  
> +out_kfifo:
> +	kfifo_free(&dev->raw->kfifo);
>  out:
>  	kfree(dev->raw);
>  	dev->raw = NULL;
> @@ -313,6 +322,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
>  			handler->raw_unregister(dev);
>  	mutex_unlock(&ir_raw_handler_lock);
>  
> +	kfifo_free(&dev->raw->kfifo);
>  	kfree(dev->raw);
>  	dev->raw = NULL;
>  }
> @@ -353,3 +363,7 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)
>  	mutex_unlock(&ir_raw_handler_lock);
>  }
>  EXPORT_SYMBOL(ir_raw_handler_unregister);
> +
> +module_param_named(fifo_size, ir_raw_event_fifo_size, uint, S_IRUGO);
> +MODULE_PARM_DESC(fifo_size,
> +	"raw event fifo size, will be rounded up to next power of 2");


-- 
Thanks,
Mauro

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-05-07 13:56 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-18 17:43 media: rc: make raw event fifo size a module parameter Heiner Kallweit
2016-05-07 13:56 ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.