[PATCH] media: rc: nuvoton: decrease size of raw event fifo

[PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Heiner Kallweit]

This chip has a 32 byte HW FIFO only. Therefore the default fifo size
of 512 raw events is not needed and can be significantly decreased.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/media/rc/nuvoton-cir.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
index 99b303b..e98c955 100644
--- a/drivers/media/rc/nuvoton-cir.c
+++ b/drivers/media/rc/nuvoton-cir.c
@@ -1186,6 +1186,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
 	rdev->priv = nvt;
 	rdev->driver_type = RC_DRIVER_IR_RAW;
 	rdev->allowed_protocols = RC_BIT_ALL;
+	rdev->raw_fifo_size = RX_BUF_LEN;
 	rdev->open = nvt_open;
 	rdev->close = nvt_close;
 	rdev->tx_ir = nvt_tx_ir;
-- 
2.8.2

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Sean Young]

On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
> of 512 raw events is not needed and can be significantly decreased.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

The 32 byte hardware queue is read from an interrupt handler and added
to the kfifo. The kfifo is read by the decoders in a seperate kthread
(in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
66 edges) and the kthread is not scheduled in time (e.g. high load), will
we not end up with an overflow in the kfifo and unable to decode it?


Sean

> ---
>  drivers/media/rc/nuvoton-cir.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
> index 99b303b..e98c955 100644
> --- a/drivers/media/rc/nuvoton-cir.c
> +++ b/drivers/media/rc/nuvoton-cir.c
> @@ -1186,6 +1186,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
>  	rdev->priv = nvt;
>  	rdev->driver_type = RC_DRIVER_IR_RAW;
>  	rdev->allowed_protocols = RC_BIT_ALL;
> +	rdev->raw_fifo_size = RX_BUF_LEN;
>  	rdev->open = nvt_open;
>  	rdev->close = nvt_close;
>  	rdev->tx_ir = nvt_tx_ir;
> -- 
> 2.8.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Heiner Kallweit]

Am 04.07.2016 um 22:13 schrieb Sean Young:
> On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
>> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
>> of 512 raw events is not needed and can be significantly decreased.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> The 32 byte hardware queue is read from an interrupt handler and added
> to the kfifo. The kfifo is read by the decoders in a seperate kthread
> (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
> 66 edges) and the kthread is not scheduled in time (e.g. high load), will
> we not end up with an overflow in the kfifo and unable to decode it?
> 
The interrupt handler is triggered latest when 24 bytes have been read.
(at least that's how the chip gets configured at the moment)
This gives the decoder thread at least 8 bytes time to process the
kfifo. This should be sufficient even under high load.

If somebody configures the driver the generate an interrupt after 32 bytes
only then there are possible issues under high load or with longer code
pieces with interrupts disabled anyway:
Then the next byte might arrive (and make the chip fifo overrun) before
the interrupt handler can read the chip fifo.

Heiner

> 
> Sean
> 
>> ---
>>  drivers/media/rc/nuvoton-cir.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c
>> index 99b303b..e98c955 100644
>> --- a/drivers/media/rc/nuvoton-cir.c
>> +++ b/drivers/media/rc/nuvoton-cir.c
>> @@ -1186,6 +1186,7 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id)
>>  	rdev->priv = nvt;
>>  	rdev->driver_type = RC_DRIVER_IR_RAW;
>>  	rdev->allowed_protocols = RC_BIT_ALL;
>> +	rdev->raw_fifo_size = RX_BUF_LEN;
>>  	rdev->open = nvt_open;
>>  	rdev->close = nvt_close;
>>  	rdev->tx_ir = nvt_tx_ir;
>> -- 
>> 2.8.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Sean Young]

On Mon, Jul 04, 2016 at 10:51:50PM +0200, Heiner Kallweit wrote:
> Am 04.07.2016 um 22:13 schrieb Sean Young:
> > On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
> >> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
> >> of 512 raw events is not needed and can be significantly decreased.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > 
> > The 32 byte hardware queue is read from an interrupt handler and added
> > to the kfifo. The kfifo is read by the decoders in a seperate kthread
> > (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
> > 66 edges) and the kthread is not scheduled in time (e.g. high load), will
> > we not end up with an overflow in the kfifo and unable to decode it?
> > 
> The interrupt handler is triggered latest when 24 bytes have been read.
> (at least that's how the chip gets configured at the moment)
> This gives the decoder thread at least 8 bytes time to process the
> kfifo. This should be sufficient even under high load.

No, it gives the interrupt handler at least 8 bytes time to read the
hardware fifo (and add it to the kfifo). There are no guarantees about
when the decoder kthread runs (which reads the kfifo).

To put it another way, in the nuvoton interrupt handler, you call 
ir_raw_event_handle() which does a wake_up_process(). That puts the
decoder process (it has a pid) in a runnable state and it will run at
some future time.


Sean

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Heiner Kallweit]

Am 04.07.2016 um 23:06 schrieb Sean Young:
> On Mon, Jul 04, 2016 at 10:51:50PM +0200, Heiner Kallweit wrote:
>> Am 04.07.2016 um 22:13 schrieb Sean Young:
>>> On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
>>>> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
>>>> of 512 raw events is not needed and can be significantly decreased.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>
>>> The 32 byte hardware queue is read from an interrupt handler and added
>>> to the kfifo. The kfifo is read by the decoders in a seperate kthread
>>> (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
>>> 66 edges) and the kthread is not scheduled in time (e.g. high load), will
>>> we not end up with an overflow in the kfifo and unable to decode it?
>>>
>> The interrupt handler is triggered latest when 24 bytes have been read.
>> (at least that's how the chip gets configured at the moment)
>> This gives the decoder thread at least 8 bytes time to process the
>> kfifo. This should be sufficient even under high load.
> 
> No, it gives the interrupt handler at least 8 bytes time to read the
> hardware fifo (and add it to the kfifo). There are no guarantees about
> when the decoder kthread runs (which reads the kfifo).
> 
> To put it another way, in the nuvoton interrupt handler, you call 
> ir_raw_event_handle() which does a wake_up_process(). That puts the
> decoder process (it has a pid) in a runnable state and it will run at
> some future time.
> 
You're right, that's the more precise description.
These 8 bytes time give the decoder process few msec's to start.
(Not sure wheter there's any protocol resulting in much shorter time.)
At least during my tests this was always sufficient.

However if you think 32 bytes might be too small as kfifo size I'd
also be fine with increasing it. The result should still be better
than the default size of 512.

Heiner

> 
> Sean
> 

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Sean Young]

On Tue, Jul 05, 2016 at 08:05:16AM +0200, Heiner Kallweit wrote:
> Am 04.07.2016 um 23:06 schrieb Sean Young:
> > On Mon, Jul 04, 2016 at 10:51:50PM +0200, Heiner Kallweit wrote:
> >> Am 04.07.2016 um 22:13 schrieb Sean Young:
> >>> On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
> >>>> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
> >>>> of 512 raw events is not needed and can be significantly decreased.
> >>>>
> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>
> >>> The 32 byte hardware queue is read from an interrupt handler and added
> >>> to the kfifo. The kfifo is read by the decoders in a seperate kthread
> >>> (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
> >>> 66 edges) and the kthread is not scheduled in time (e.g. high load), will
> >>> we not end up with an overflow in the kfifo and unable to decode it?
> >>>
> >> The interrupt handler is triggered latest when 24 bytes have been read.
> >> (at least that's how the chip gets configured at the moment)
> >> This gives the decoder thread at least 8 bytes time to process the
> >> kfifo. This should be sufficient even under high load.
> > 
> > No, it gives the interrupt handler at least 8 bytes time to read the
> > hardware fifo (and add it to the kfifo). There are no guarantees about
> > when the decoder kthread runs (which reads the kfifo).
> > 
> > To put it another way, in the nuvoton interrupt handler, you call 
> > ir_raw_event_handle() which does a wake_up_process(). That puts the
> > decoder process (it has a pid) in a runnable state and it will run at
> > some future time.
> > 
> You're right, that's the more precise description.
> These 8 bytes time give the decoder process few msec's to start.
> (Not sure wheter there's any protocol resulting in much shorter time.)
> At least during my tests this was always sufficient.

So worst case scenario with NEC IR (I have not checked all ir protocols).
1. 32 bytes of IR read from hardware fifo.
2. IR in the middle of series of 1s (two edges, 1.125ms each)
3. After 13.5ms interrupt is triggered again as 12 new bits generated 24 edges
4. Decoder thread has not run and emptied the fifo.
5. kfifo overflow and IR cannot be decoded; key lost

> However if you think 32 bytes might be too small as kfifo size I'd
> also be fine with increasing it. The result should still be better
> than the default size of 512.

So this depends on how many edges an remote could possible generate and
how often the kthread will be scheduled to read it. For example I have
a remote which repeats the entire nec code after 20ms if you hold a
button down. That's about 800 edges per second, so with a kfifo of 512
the kthread would have to be scheduled twice a second.

I think we should stick to the default of 512.


Sean

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Heiner Kallweit]

Am 05.07.2016 um 11:17 schrieb Sean Young:
> On Tue, Jul 05, 2016 at 08:05:16AM +0200, Heiner Kallweit wrote:
>> Am 04.07.2016 um 23:06 schrieb Sean Young:
>>> On Mon, Jul 04, 2016 at 10:51:50PM +0200, Heiner Kallweit wrote:
>>>> Am 04.07.2016 um 22:13 schrieb Sean Young:
>>>>> On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
>>>>>> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
>>>>>> of 512 raw events is not needed and can be significantly decreased.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>
>>>>> The 32 byte hardware queue is read from an interrupt handler and added
>>>>> to the kfifo. The kfifo is read by the decoders in a seperate kthread
>>>>> (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
>>>>> 66 edges) and the kthread is not scheduled in time (e.g. high load), will
>>>>> we not end up with an overflow in the kfifo and unable to decode it?
>>>>>
>>>> The interrupt handler is triggered latest when 24 bytes have been read.
>>>> (at least that's how the chip gets configured at the moment)
>>>> This gives the decoder thread at least 8 bytes time to process the
>>>> kfifo. This should be sufficient even under high load.
>>>
>>> No, it gives the interrupt handler at least 8 bytes time to read the
>>> hardware fifo (and add it to the kfifo). There are no guarantees about
>>> when the decoder kthread runs (which reads the kfifo).
>>>
>>> To put it another way, in the nuvoton interrupt handler, you call 
>>> ir_raw_event_handle() which does a wake_up_process(). That puts the
>>> decoder process (it has a pid) in a runnable state and it will run at
>>> some future time.
>>>
>> You're right, that's the more precise description.
>> These 8 bytes time give the decoder process few msec's to start.
>> (Not sure wheter there's any protocol resulting in much shorter time.)
>> At least during my tests this was always sufficient.
> 
> So worst case scenario with NEC IR (I have not checked all ir protocols).
> 1. 32 bytes of IR read from hardware fifo.
> 2. IR in the middle of series of 1s (two edges, 1.125ms each)
> 3. After 13.5ms interrupt is triggered again as 12 new bits generated 24 edges
> 4. Decoder thread has not run and emptied the fifo.
> 5. kfifo overflow and IR cannot be decoded; key lost
> 
At least in the nuvoton driver the decoder thread is started whenever
something was read from the hw fifo and written to kfifo.
-> call to ir_raw_event_handle() at the end of nvt_process_rx_ir_data
It doesn't wait for the end of an IR packet.
Means in your example the decoder thread would be started after step 1
and whenever the hw fifo reaches the threshold to generate an irq.

>> However if you think 32 bytes might be too small as kfifo size I'd
>> also be fine with increasing it. The result should still be better
>> than the default size of 512.
> 
> So this depends on how many edges an remote could possible generate and
> how often the kthread will be scheduled to read it. For example I have
> a remote which repeats the entire nec code after 20ms if you hold a
> button down. That's about 800 edges per second, so with a kfifo of 512
> the kthread would have to be scheduled twice a second.
> 
> I think we should stick to the default of 512.
> 
> 
> Sean
> 

Re: [PATCH] media: rc: nuvoton: decrease size of raw event fifo

[Sean Young]

On Tue, Jul 05, 2016 at 07:48:34PM +0200, Heiner Kallweit wrote:
> Am 05.07.2016 um 11:17 schrieb Sean Young:
> > On Tue, Jul 05, 2016 at 08:05:16AM +0200, Heiner Kallweit wrote:
> >> Am 04.07.2016 um 23:06 schrieb Sean Young:
> >>> On Mon, Jul 04, 2016 at 10:51:50PM +0200, Heiner Kallweit wrote:
> >>>> Am 04.07.2016 um 22:13 schrieb Sean Young:
> >>>>> On Wed, May 18, 2016 at 10:29:41PM +0200, Heiner Kallweit wrote:
> >>>>>> This chip has a 32 byte HW FIFO only. Therefore the default fifo size
> >>>>>> of 512 raw events is not needed and can be significantly decreased.
> >>>>>>
> >>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >>>>>
> >>>>> The 32 byte hardware queue is read from an interrupt handler and added
> >>>>> to the kfifo. The kfifo is read by the decoders in a seperate kthread
> >>>>> (in ir_raw_event_thread). If we have a long IR (e.g. nec which has 
> >>>>> 66 edges) and the kthread is not scheduled in time (e.g. high load), will
> >>>>> we not end up with an overflow in the kfifo and unable to decode it?
> >>>>>
> >>>> The interrupt handler is triggered latest when 24 bytes have been read.
> >>>> (at least that's how the chip gets configured at the moment)
> >>>> This gives the decoder thread at least 8 bytes time to process the
> >>>> kfifo. This should be sufficient even under high load.
> >>>
> >>> No, it gives the interrupt handler at least 8 bytes time to read the
> >>> hardware fifo (and add it to the kfifo). There are no guarantees about
> >>> when the decoder kthread runs (which reads the kfifo).
> >>>
> >>> To put it another way, in the nuvoton interrupt handler, you call 
> >>> ir_raw_event_handle() which does a wake_up_process(). That puts the
> >>> decoder process (it has a pid) in a runnable state and it will run at
> >>> some future time.
> >>>
> >> You're right, that's the more precise description.
> >> These 8 bytes time give the decoder process few msec's to start.
> >> (Not sure wheter there's any protocol resulting in much shorter time.)
> >> At least during my tests this was always sufficient.
> > 
> > So worst case scenario with NEC IR (I have not checked all ir protocols).
> > 1. 32 bytes of IR read from hardware fifo.
> > 2. IR in the middle of series of 1s (two edges, 1.125ms each)
> > 3. After 13.5ms interrupt is triggered again as 12 new bits generated 24 edges
> > 4. Decoder thread has not run and emptied the fifo.
> > 5. kfifo overflow and IR cannot be decoded; key lost
> > 
> At least in the nuvoton driver the decoder thread is started whenever
> something was read from the hw fifo and written to kfifo.
> -> call to ir_raw_event_handle() at the end of nvt_process_rx_ir_data
> It doesn't wait for the end of an IR packet.
> Means in your example the decoder thread would be started after step 1
> and whenever the hw fifo reaches the threshold to generate an irq.

No the IR decoder thread is put in a runnable state in the interrupt handler 
(i.e. in step 1).  It doesn't mean it will run in time (so within 13.5ms)
necessarily. That depends on the scheduler.

Now finding some hard numbers for scheduling latency (under high load
of course) is proving to be difficult. Does anyone know what sort of spikes
we should cater for?


Sean