* [PATCH 0/2] Fix rtl28xxu nec/rc5 receiver @ 2022-02-12 16:32 Sean Young 2022-02-12 16:32 ` [PATCH 1/2] media: rc-core: split IR timeout into rawir timeout and keyup delay Sean Young 2022-02-12 16:32 ` [PATCH 2/2] media: rtl28xxu: improve IR receiver Sean Young 0 siblings, 2 replies; 17+ messages in thread From: Sean Young @ 2022-02-12 16:32 UTC (permalink / raw) To: linux-media; +Cc: Marko Mäkelä Since forever rc-core has used the timeout for two separate purposes: the raw IR timeout and the key up delay. This problem has now shown itself in issues with the rtl28xxu driver, so these two patches split the timeout and then fix the rtl28xxu driver, which is now very responsive. Sean Young (2): media: rc-core: split IR timeout into rawir timeout and keyup delay media: rtl28xxu: improve IR receiver drivers/hid/hid-picolcd_cir.c | 3 ++- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/cec/platform/seco/seco-cec.c | 2 +- drivers/media/pci/cx88/cx88-input.c | 3 ++- drivers/media/pci/saa7134/saa7134-input.c | 3 ++- drivers/media/pci/smipcie/smipcie-ir.c | 3 ++- drivers/media/rc/ene_ir.c | 11 ++++---- drivers/media/rc/fintek-cir.c | 3 ++- drivers/media/rc/gpio-ir-recv.c | 3 ++- drivers/media/rc/igorplugusb.c | 5 ++-- drivers/media/rc/iguanair.c | 2 +- drivers/media/rc/ir-hix5hd2.c | 3 ++- drivers/media/rc/ir-mce_kbd-decoder.c | 2 +- drivers/media/rc/ir_toy.c | 3 ++- drivers/media/rc/ite-cir.c | 3 ++- drivers/media/rc/lirc_dev.c | 6 ++--- drivers/media/rc/mceusb.c | 9 ++++--- drivers/media/rc/meson-ir.c | 3 ++- drivers/media/rc/mtk-cir.c | 3 ++- drivers/media/rc/nuvoton-cir.c | 3 ++- drivers/media/rc/rc-ir-raw.c | 10 ++++---- drivers/media/rc/rc-loopback.c | 5 ++-- drivers/media/rc/rc-main.c | 8 +++--- drivers/media/rc/redrat3.c | 5 ++-- drivers/media/rc/serial_ir.c | 7 +++--- drivers/media/rc/st_rc.c | 8 ++++-- drivers/media/rc/streamzap.c | 5 ++-- drivers/media/rc/sunxi-cir.c | 7 +++--- drivers/media/rc/ttusbir.c | 3 ++- drivers/media/rc/winbond-cir.c | 3 ++- drivers/media/rc/xbox_remote.c | 2 +- drivers/media/usb/dvb-usb-v2/dvb_usb.h | 3 ++- drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 3 ++- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 28 +++++++++++++++++++-- include/media/rc-core.h | 11 +++++--- 35 files changed, 120 insertions(+), 63 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] media: rc-core: split IR timeout into rawir timeout and keyup delay 2022-02-12 16:32 [PATCH 0/2] Fix rtl28xxu nec/rc5 receiver Sean Young @ 2022-02-12 16:32 ` Sean Young 2022-02-12 16:32 ` [PATCH 2/2] media: rtl28xxu: improve IR receiver Sean Young 1 sibling, 0 replies; 17+ messages in thread From: Sean Young @ 2022-02-12 16:32 UTC (permalink / raw) To: linux-media; +Cc: Marko Mäkelä There are two timeouts relevant for IR processing: - The rawir_timeout specifies how long an IR space or gap has to be before it is converted to an IR timeout. This signals the end of an message. - The timeout for a key up event. When an keypress has been decoded a key down event is generated. After some time a key up event is sent, unless the original IR is repeated (if the user holds the button down), and then the key up is event is delayed. This delay should be what possible delay the driver or device has for decoding, for example is the device is polled. Signed-off-by: Sean Young <sean@mess.org> --- drivers/hid/hid-picolcd_cir.c | 3 ++- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/cec/platform/seco/seco-cec.c | 2 +- drivers/media/pci/cx88/cx88-input.c | 3 ++- drivers/media/pci/saa7134/saa7134-input.c | 3 ++- drivers/media/pci/smipcie/smipcie-ir.c | 3 ++- drivers/media/rc/ene_ir.c | 11 ++++++----- drivers/media/rc/fintek-cir.c | 3 ++- drivers/media/rc/gpio-ir-recv.c | 3 ++- drivers/media/rc/igorplugusb.c | 5 +++-- drivers/media/rc/iguanair.c | 2 +- drivers/media/rc/ir-hix5hd2.c | 3 ++- drivers/media/rc/ir-mce_kbd-decoder.c | 2 +- drivers/media/rc/ir_toy.c | 3 ++- drivers/media/rc/ite-cir.c | 3 ++- drivers/media/rc/lirc_dev.c | 6 +++--- drivers/media/rc/mceusb.c | 9 +++++---- drivers/media/rc/meson-ir.c | 3 ++- drivers/media/rc/mtk-cir.c | 3 ++- drivers/media/rc/nuvoton-cir.c | 3 ++- drivers/media/rc/rc-ir-raw.c | 10 +++++----- drivers/media/rc/rc-loopback.c | 5 +++-- drivers/media/rc/rc-main.c | 8 +++++--- drivers/media/rc/redrat3.c | 5 +++-- drivers/media/rc/serial_ir.c | 7 ++++--- drivers/media/rc/st_rc.c | 8 ++++++-- drivers/media/rc/streamzap.c | 5 +++-- drivers/media/rc/sunxi-cir.c | 7 ++++--- drivers/media/rc/ttusbir.c | 3 ++- drivers/media/rc/winbond-cir.c | 3 ++- drivers/media/rc/xbox_remote.c | 2 +- drivers/media/usb/dvb-usb-v2/dvb_usb.h | 3 ++- drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 3 ++- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 3 ++- include/media/rc-core.h | 11 +++++++---- 35 files changed, 96 insertions(+), 62 deletions(-) diff --git a/drivers/hid/hid-picolcd_cir.c b/drivers/hid/hid-picolcd_cir.c index d6faa0e00f95a..d8181c890e6ab 100644 --- a/drivers/hid/hid-picolcd_cir.c +++ b/drivers/hid/hid-picolcd_cir.c @@ -114,7 +114,8 @@ int picolcd_init_cir(struct picolcd_data *data, struct hid_report *report) rdev->dev.parent = &data->hdev->dev; rdev->driver_name = PICOLCD_NAME; rdev->map_name = RC_MAP_RC6_MCE; - rdev->timeout = MS_TO_US(100); + rdev->rawir_timeout = MS_TO_US(100); + rdev->keyup_delay = MS_TO_US(100); rdev->rx_resolution = 1; ret = rc_register_device(rdev); diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c index a3ab6a43fb14a..0247bbe4c97e1 100644 --- a/drivers/media/cec/core/cec-core.c +++ b/drivers/media/cec/core/cec-core.c @@ -312,7 +312,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, adap->rc->allowed_protocols = RC_PROTO_BIT_CEC; adap->rc->priv = adap; adap->rc->map_name = RC_MAP_CEC; - adap->rc->timeout = MS_TO_US(550); + adap->rc->keyup_delay = MS_TO_US(550); #endif return adap; } diff --git a/drivers/media/cec/platform/seco/seco-cec.c b/drivers/media/cec/platform/seco/seco-cec.c index ae138cc253fde..e1176f67f89a5 100644 --- a/drivers/media/cec/platform/seco/seco-cec.c +++ b/drivers/media/cec/platform/seco/seco-cec.c @@ -369,7 +369,7 @@ static int secocec_ir_probe(void *priv) cec->ir->allowed_protocols = RC_PROTO_BIT_RC5; cec->ir->priv = cec; cec->ir->map_name = RC_MAP_HAUPPAUGE; - cec->ir->timeout = MS_TO_US(100); + cec->ir->keyup_delay = MS_TO_US(100); /* Clear the status register */ status = smb_rd16(SECOCEC_STATUS_REG_1, &val); diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c index ce0ef0b8186f5..8325865149623 100644 --- a/drivers/media/pci/cx88/cx88-input.c +++ b/drivers/media/pci/cx88/cx88-input.c @@ -479,7 +479,8 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci) dev->scancode_mask = hardware_mask; if (ir->sampling) { - dev->timeout = MS_TO_US(10); /* 10 ms */ + dev->rawir_timeout = MS_TO_US(10); /* 10 ms */ + dev->keyup_delay = MS_TO_US(10); /* 10 ms */ } else { dev->driver_type = RC_DRIVER_SCANCODE; dev->allowed_protocols = rc_proto; diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c index 8610eb473b39e..cda6be9633110 100644 --- a/drivers/media/pci/saa7134/saa7134-input.c +++ b/drivers/media/pci/saa7134/saa7134-input.c @@ -812,7 +812,8 @@ int saa7134_input_init1(struct saa7134_dev *dev) rc->map_name = ir_codes; rc->driver_name = MODULE_NAME; rc->min_timeout = 1; - rc->timeout = IR_DEFAULT_TIMEOUT; + rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + rc->keyup_delay = IR_DEFAULT_TIMEOUT; rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; err = rc_register_device(rc); diff --git a/drivers/media/pci/smipcie/smipcie-ir.c b/drivers/media/pci/smipcie/smipcie-ir.c index c0604d9c70119..c50b8fc727294 100644 --- a/drivers/media/pci/smipcie/smipcie-ir.c +++ b/drivers/media/pci/smipcie/smipcie-ir.c @@ -156,7 +156,8 @@ int smi_ir_init(struct smi_dev *dev) rc_dev->dev.parent = &dev->pci_dev->dev; rc_dev->map_name = dev->info->rc_map; - rc_dev->timeout = SMI_SAMPLE_PERIOD * SMI_SAMPLE_IDLEMIN; + rc_dev->rawir_timeout = SMI_SAMPLE_PERIOD * SMI_SAMPLE_IDLEMIN; + rc_dev->keyup_delay = SMI_SAMPLE_PERIOD * SMI_SAMPLE_IDLEMIN; rc_dev->rx_resolution = SMI_SAMPLE_PERIOD; ir->rc_dev = rc_dev; diff --git a/drivers/media/rc/ene_ir.c b/drivers/media/rc/ene_ir.c index e09270916fbca..dd0ef832ca9d7 100644 --- a/drivers/media/rc/ene_ir.c +++ b/drivers/media/rc/ene_ir.c @@ -454,10 +454,10 @@ static void ene_rx_setup(struct ene_device *dev) if (dev->hw_learning_and_tx_capable) dev->rdev->tx_resolution = sample_period; - if (dev->rdev->timeout > dev->rdev->max_timeout) - dev->rdev->timeout = dev->rdev->max_timeout; - if (dev->rdev->timeout < dev->rdev->min_timeout) - dev->rdev->timeout = dev->rdev->min_timeout; + if (dev->rdev->rawir_timeout > dev->rdev->max_timeout) + dev->rdev->rawir_timeout = dev->rdev->max_timeout; + if (dev->rdev->rawir_timeout < dev->rdev->min_timeout) + dev->rdev->rawir_timeout = dev->rdev->min_timeout; } /* Enable the device for receive */ @@ -818,7 +818,8 @@ static void ene_setup_default_settings(struct ene_device *dev) dev->learning_mode_enabled = learning_mode_force; /* Set reasonable default timeout */ - dev->rdev->timeout = MS_TO_US(150); + dev->rdev->rawir_timeout = MS_TO_US(150); + dev->rdev->keyup_delay = MS_TO_US(150); } /* Upload all hardware settings at once. Used at load and resume time */ diff --git a/drivers/media/rc/fintek-cir.c b/drivers/media/rc/fintek-cir.c index 3fb0968efd57d..e9f18a68ef457 100644 --- a/drivers/media/rc/fintek-cir.c +++ b/drivers/media/rc/fintek-cir.c @@ -524,7 +524,8 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id rdev->dev.parent = &pdev->dev; rdev->driver_name = FINTEK_DRIVER_NAME; rdev->map_name = RC_MAP_RC6_MCE; - rdev->timeout = 1000; + rdev->rawir_timeout = 1000; + rdev->keyup_delay = 1000; /* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */ rdev->rx_resolution = CIR_SAMPLE_PERIOD; diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c index 22e524b69806a..9474fd21b11bb 100644 --- a/drivers/media/rc/gpio-ir-recv.c +++ b/drivers/media/rc/gpio-ir-recv.c @@ -99,7 +99,8 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) rcdev->dev.parent = dev; rcdev->driver_name = KBUILD_MODNAME; rcdev->min_timeout = 1; - rcdev->timeout = IR_DEFAULT_TIMEOUT; + rcdev->rawir_timeout = IR_DEFAULT_TIMEOUT; + rcdev->keyup_delay = MS_TO_US(40); rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; rcdev->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; rcdev->map_name = of_get_property(np, "linux,rc-map-name", NULL); diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c index b40dbf5001869..d42ea8ed75a28 100644 --- a/drivers/media/rc/igorplugusb.c +++ b/drivers/media/rc/igorplugusb.c @@ -81,7 +81,7 @@ static void igorplugusb_irdata(struct igorplugusb *ir, unsigned len) } while (i != start); /* add a trailing space */ - rawir.duration = ir->rc->timeout; + rawir.duration = ir->rc->rawir_timeout; rawir.pulse = false; ir_raw_event_store_with_filter(ir->rc, &rawir); @@ -204,7 +204,8 @@ static int igorplugusb_probe(struct usb_interface *intf, rc->priv = ir; rc->driver_name = DRIVER_NAME; rc->map_name = RC_MAP_HAUPPAUGE; - rc->timeout = MS_TO_US(100); + rc->rawir_timeout = MS_TO_US(100); + rc->keyup_delay = MS_TO_US(100); rc->rx_resolution = 85; ir->rc = rc; diff --git a/drivers/media/rc/iguanair.c b/drivers/media/rc/iguanair.c index c9cb8277723f4..f434e8037035e 100644 --- a/drivers/media/rc/iguanair.c +++ b/drivers/media/rc/iguanair.c @@ -461,7 +461,7 @@ static int iguanair_probe(struct usb_interface *intf, rc->driver_name = KBUILD_MODNAME; rc->map_name = RC_MAP_RC6_MCE; rc->min_timeout = 1; - rc->timeout = IR_DEFAULT_TIMEOUT; + rc->rawir_timeout = IR_DEFAULT_TIMEOUT; rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; rc->rx_resolution = RX_RESOLUTION; diff --git a/drivers/media/rc/ir-hix5hd2.c b/drivers/media/rc/ir-hix5hd2.c index 4ff954b11dc77..a499f98d2163f 100644 --- a/drivers/media/rc/ir-hix5hd2.c +++ b/drivers/media/rc/ir-hix5hd2.c @@ -310,7 +310,8 @@ static int hix5hd2_ir_probe(struct platform_device *pdev) rdev->input_id.product = 0x0001; rdev->input_id.version = 0x0100; rdev->rx_resolution = 10; - rdev->timeout = IR_CFG_SYMBOL_MAXWIDTH * 10; + rdev->rawir_timeout = IR_CFG_SYMBOL_MAXWIDTH * 10; + rdev->keyup_delay = IR_CFG_SYMBOL_MAXWIDTH * 10; ret = rc_register_device(rdev); if (ret < 0) diff --git a/drivers/media/rc/ir-mce_kbd-decoder.c b/drivers/media/rc/ir-mce_kbd-decoder.c index 66e8feb9a569a..0f93960021004 100644 --- a/drivers/media/rc/ir-mce_kbd-decoder.c +++ b/drivers/media/rc/ir-mce_kbd-decoder.c @@ -320,7 +320,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev) data->body); spin_lock(&data->keylock); if (scancode) { - delay = usecs_to_jiffies(dev->timeout) + + delay = usecs_to_jiffies(dev->keyup_delay) + msecs_to_jiffies(100); mod_timer(&data->rx_timeout, jiffies + delay); } else { diff --git a/drivers/media/rc/ir_toy.c b/drivers/media/rc/ir_toy.c index 1968067092594..5407db98a9626 100644 --- a/drivers/media/rc/ir_toy.c +++ b/drivers/media/rc/ir_toy.c @@ -489,7 +489,8 @@ static int irtoy_probe(struct usb_interface *intf, rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; rc->map_name = RC_MAP_RC6_MCE; rc->rx_resolution = UNIT_US; - rc->timeout = IR_DEFAULT_TIMEOUT; + rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + rc->keyup_delay = IR_DEFAULT_TIMEOUT; /* * end of transmission is detected by absence of a usb packet diff --git a/drivers/media/rc/ite-cir.c b/drivers/media/rc/ite-cir.c index fcfadd7ea31cf..dc2a66781f47e 100644 --- a/drivers/media/rc/ite-cir.c +++ b/drivers/media/rc/ite-cir.c @@ -1377,7 +1377,8 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id /* FIFO threshold is 17 bytes, so 17 * 8 samples minimum */ rdev->min_timeout = 17 * 8 * ITE_BAUDRATE_DIVISOR * sample_period / 1000; - rdev->timeout = IR_DEFAULT_TIMEOUT; + rdev->rawir_timeout = IR_DEFAULT_TIMEOUT; + rdev->keyup_delay = IR_DEFAULT_TIMEOUT; rdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; rdev->rx_resolution = ITE_BAUDRATE_DIVISOR * sample_period / 1000; rdev->tx_resolution = ITE_BAUDRATE_DIVISOR * sample_period / 1000; diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c index 765375bda0c6e..e7eff7eef687f 100644 --- a/drivers/media/rc/lirc_dev.c +++ b/drivers/media/rc/lirc_dev.c @@ -545,15 +545,15 @@ static long lirc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) else if (dev->s_timeout) ret = dev->s_timeout(dev, val); else - dev->timeout = val; + dev->rawir_timeout = val; } break; case LIRC_GET_REC_TIMEOUT: - if (!dev->timeout) + if (!dev->rawir_timeout) ret = -ENOTTY; else - val = dev->timeout; + val = dev->rawir_timeout; break; case LIRC_SET_REC_TIMEOUT_REPORTS: diff --git a/drivers/media/rc/mceusb.c b/drivers/media/rc/mceusb.c index 2dc810f5a73f7..9adfc5856ff11 100644 --- a/drivers/media/rc/mceusb.c +++ b/drivers/media/rc/mceusb.c @@ -1203,7 +1203,7 @@ static void mceusb_handle_command(struct mceusb_dev *ir, u8 *buf_in) switch (subcmd) { /* 2-byte return value commands */ case MCE_RSP_EQIRTIMEOUT: - ir->rc->timeout = (*hi << 8 | *lo) * MCE_TIME_UNIT; + ir->rc->rawir_timeout = (*hi << 8 | *lo) * MCE_TIME_UNIT; break; case MCE_RSP_EQIRNUMPORTS: ir->num_txports = *hi; @@ -1335,7 +1335,7 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len) } else { struct ir_raw_event ev = { .timeout = 1, - .duration = ir->rc->timeout + .duration = ir->rc->rawir_timeout }; if (ir_raw_event_store_with_filter(ir->rc, @@ -1615,7 +1615,8 @@ static struct rc_dev *mceusb_init_rc_dev(struct mceusb_dev *ir) rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; rc->rx_resolution = MCE_TIME_UNIT; rc->min_timeout = MCE_TIME_UNIT; - rc->timeout = MS_TO_US(100); + rc->rawir_timeout = MS_TO_US(100); + rc->keyup_delay = MS_TO_US(100); if (!mceusb_model[ir->model].broken_irtimeout) { rc->s_timeout = mceusb_set_timeout; rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; @@ -1624,7 +1625,7 @@ static struct rc_dev *mceusb_init_rc_dev(struct mceusb_dev *ir) * If we can't set the timeout using CMD_SETIRTIMEOUT, we can * rely on software timeouts for timeouts < 100ms. */ - rc->max_timeout = rc->timeout; + rc->max_timeout = rc->rawir_timeout; } if (!ir->flags.no_tx) { rc->s_tx_mask = mceusb_set_tx_mask; diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c index 4b769111f78e3..9f02b94909956 100644 --- a/drivers/media/rc/meson-ir.c +++ b/drivers/media/rc/meson-ir.c @@ -133,7 +133,8 @@ static int meson_ir_probe(struct platform_device *pdev) ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; ir->rc->rx_resolution = MESON_TRATE; ir->rc->min_timeout = 1; - ir->rc->timeout = IR_DEFAULT_TIMEOUT; + ir->rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + ir->rc->keyup_delay = IR_DEFAULT_TIMEOUT; ir->rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; ir->rc->driver_name = DRIVER_NAME; diff --git a/drivers/media/rc/mtk-cir.c b/drivers/media/rc/mtk-cir.c index 27b7412d02a56..df42092fa7bd3 100644 --- a/drivers/media/rc/mtk-cir.c +++ b/drivers/media/rc/mtk-cir.c @@ -343,7 +343,8 @@ static int mtk_ir_probe(struct platform_device *pdev) ir->rc->driver_name = MTK_IR_DEV; ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; ir->rc->rx_resolution = MTK_IR_SAMPLE; - ir->rc->timeout = MTK_MAX_SAMPLES * (MTK_IR_SAMPLE + 1); + ir->rc->rawir_timeout = MTK_MAX_SAMPLES * (MTK_IR_SAMPLE + 1); + ir->rc->keyup_delay = MTK_MAX_SAMPLES * (MTK_IR_SAMPLE + 1); ret = devm_rc_register_device(dev, ir->rc); if (ret) { diff --git a/drivers/media/rc/nuvoton-cir.c b/drivers/media/rc/nuvoton-cir.c index 2214d41ef5794..29147a084407c 100644 --- a/drivers/media/rc/nuvoton-cir.c +++ b/drivers/media/rc/nuvoton-cir.c @@ -998,7 +998,8 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id) rdev->input_id.version = nvt->chip_minor; rdev->driver_name = NVT_DRIVER_NAME; rdev->map_name = RC_MAP_RC6_MCE; - rdev->timeout = MS_TO_US(100); + rdev->rawir_timeout = MS_TO_US(100); + rdev->keyup_delay = MS_TO_US(100); /* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */ rdev->rx_resolution = CIR_SAMPLE_PERIOD; #if 0 diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c index 16e33d7eaaa2d..b10f575d4ba2f 100644 --- a/drivers/media/rc/rc-ir-raw.c +++ b/drivers/media/rc/rc-ir-raw.c @@ -185,8 +185,8 @@ int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev) } /* Enter idle mode if necessary */ - if (!ev->pulse && dev->timeout && - dev->raw->this_ev.duration >= dev->timeout) + if (!ev->pulse && dev->rawir_timeout && + dev->raw->this_ev.duration >= dev->rawir_timeout) ir_raw_event_set_idle(dev, true); return 1; @@ -283,7 +283,7 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_proto) if (dev->s_timeout) dev->s_timeout(dev, timeout); else - dev->timeout = timeout; + dev->rawir_timeout = timeout; return 0; } @@ -559,7 +559,7 @@ static void ir_raw_edge_handle(struct timer_list *t) spin_lock_irqsave(&dev->raw->edge_spinlock, flags); interval = ktime_sub(ktime_get(), dev->raw->last_event); - if (ktime_to_us(interval) >= dev->timeout) { + if (ktime_to_us(interval) >= dev->rawir_timeout) { struct ir_raw_event ev = { .timeout = true, .duration = ktime_to_us(interval) @@ -568,7 +568,7 @@ static void ir_raw_edge_handle(struct timer_list *t) ir_raw_event_store(dev, &ev); } else { mod_timer(&dev->raw->edge_handle, - jiffies + usecs_to_jiffies(dev->timeout - + jiffies + usecs_to_jiffies(dev->rawir_timeout - ktime_to_us(interval))); } spin_unlock_irqrestore(&dev->raw->edge_spinlock, flags); diff --git a/drivers/media/rc/rc-loopback.c b/drivers/media/rc/rc-loopback.c index b356041c5c00e..b8812350ce49c 100644 --- a/drivers/media/rc/rc-loopback.c +++ b/drivers/media/rc/rc-loopback.c @@ -129,7 +129,7 @@ static int loop_tx_ir(struct rc_dev *dev, unsigned *txbuf, unsigned count) /* Fake a silence long enough to cause us to go idle */ rawir.pulse = false; - rawir.duration = dev->timeout; + rawir.duration = dev->rawir_timeout; ir_raw_event_store_with_filter(dev, &rawir); ir_raw_event_handle(dev); @@ -226,7 +226,8 @@ static int __init loop_init(void) rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; rc->allowed_wakeup_protocols = RC_PROTO_BIT_ALL_IR_ENCODER; rc->encode_wakeup = true; - rc->timeout = IR_DEFAULT_TIMEOUT; + rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + rc->keyup_delay = IR_DEFAULT_TIMEOUT; rc->min_timeout = 1; rc->max_timeout = IR_MAX_TIMEOUT; rc->rx_resolution = 1; diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index b90438a71c800..f27f6b383816c 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -737,7 +737,7 @@ static unsigned int repeat_period(int protocol) void rc_repeat(struct rc_dev *dev) { unsigned long flags; - unsigned int timeout = usecs_to_jiffies(dev->timeout) + + unsigned int timeout = usecs_to_jiffies(dev->keyup_delay) + msecs_to_jiffies(repeat_period(dev->last_protocol)); struct lirc_scancode sc = { .scancode = dev->last_scancode, .rc_proto = dev->last_protocol, @@ -855,7 +855,8 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, ir_do_keydown(dev, protocol, scancode, keycode, toggle); if (dev->keypressed) { - dev->keyup_jiffies = jiffies + usecs_to_jiffies(dev->timeout) + + dev->keyup_jiffies = jiffies + + usecs_to_jiffies(dev->keyup_delay) + msecs_to_jiffies(repeat_period(protocol)); mod_timer(&dev->timer_keyup, dev->keyup_jiffies); } @@ -1715,7 +1716,8 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type) dev->input_dev->setkeycode = ir_setkeycode; input_set_drvdata(dev->input_dev, dev); - dev->timeout = IR_DEFAULT_TIMEOUT; + dev->rawir_timeout = IR_DEFAULT_TIMEOUT; + dev->keyup_delay = IR_DEFAULT_TIMEOUT; timer_setup(&dev->timer_keyup, ir_timer_keyup, 0); timer_setup(&dev->timer_repeat, ir_timer_repeat, 0); diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c index cb22316b3f002..51f9fce1df569 100644 --- a/drivers/media/rc/redrat3.c +++ b/drivers/media/rc/redrat3.c @@ -381,7 +381,7 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3) /* add a trailing space */ rawir.pulse = false; rawir.timeout = true; - rawir.duration = rr3->rc->timeout; + rawir.duration = rr3->rc->rawir_timeout; dev_dbg(dev, "storing trailing timeout with duration %d\n", rawir.duration); ir_raw_event_store_with_filter(rr3->rc, &rawir); @@ -948,7 +948,8 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3) rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; rc->min_timeout = MS_TO_US(RR3_RX_MIN_TIMEOUT); rc->max_timeout = MS_TO_US(RR3_RX_MAX_TIMEOUT); - rc->timeout = redrat3_get_timeout(rr3); + rc->rawir_timeout = redrat3_get_timeout(rr3); + rc->keyup_delay = rc->rawir_timeout; rc->s_timeout = redrat3_set_timeout; rc->tx_ir = redrat3_transmit_ir; rc->s_tx_carrier = redrat3_set_tx_carrier; diff --git a/drivers/media/rc/serial_ir.c b/drivers/media/rc/serial_ir.c index 96ae0294ac102..cd211bc14b33a 100644 --- a/drivers/media/rc/serial_ir.c +++ b/drivers/media/rc/serial_ir.c @@ -385,7 +385,7 @@ static irqreturn_t serial_ir_irq_handler(int i, void *blah) } while (!(sinp(UART_IIR) & UART_IIR_NO_INT)); /* still pending ? */ mod_timer(&serial_ir.timeout_timer, - jiffies + usecs_to_jiffies(serial_ir.rcdev->timeout)); + jiffies + usecs_to_jiffies(serial_ir.rcdev->rawir_timeout)); ir_raw_event_handle(serial_ir.rcdev); @@ -466,7 +466,7 @@ static void serial_ir_timeout(struct timer_list *unused) { struct ir_raw_event ev = { .timeout = true, - .duration = serial_ir.rcdev->timeout + .duration = serial_ir.rcdev->rawir_timeout }; ir_raw_event_store_with_filter(serial_ir.rcdev, &ev); ir_raw_event_handle(serial_ir.rcdev); @@ -526,7 +526,8 @@ static int serial_ir_probe(struct platform_device *dev) rcdev->driver_name = KBUILD_MODNAME; rcdev->map_name = RC_MAP_RC6_MCE; rcdev->min_timeout = 1; - rcdev->timeout = IR_DEFAULT_TIMEOUT; + rcdev->rawir_timeout = IR_DEFAULT_TIMEOUT; + rcdev->keyup_delay = IR_DEFAULT_TIMEOUT; rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; rcdev->rx_resolution = 250; diff --git a/drivers/media/rc/st_rc.c b/drivers/media/rc/st_rc.c index 19e987a048ccb..041024191cb9c 100644 --- a/drivers/media/rc/st_rc.c +++ b/drivers/media/rc/st_rc.c @@ -63,7 +63,10 @@ struct st_rc_device { static void st_rc_send_lirc_timeout(struct rc_dev *rdev) { - struct ir_raw_event ev = { .timeout = true, .duration = rdev->timeout }; + struct ir_raw_event ev = { + .timeout = true, + .duration = rdev->rawir_timeout + }; ir_raw_event_store(rdev, &ev); } @@ -299,7 +302,8 @@ static int st_rc_probe(struct platform_device *pdev) rdev->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; /* rx sampling rate is 10Mhz */ rdev->rx_resolution = 100; - rdev->timeout = MAX_SYMB_TIME; + rdev->rawir_timeout = MAX_SYMB_TIME; + rdev->keyup_delay = MAX_SYMB_TIME; rdev->priv = rc_dev; rdev->open = st_rc_open; rdev->close = st_rc_close; diff --git a/drivers/media/rc/streamzap.c b/drivers/media/rc/streamzap.c index 16ba85d7c090c..563e49e0f1415 100644 --- a/drivers/media/rc/streamzap.c +++ b/drivers/media/rc/streamzap.c @@ -198,7 +198,7 @@ static void streamzap_callback(struct urb *urb) if (sz->buf_in[i] == SZ_TIMEOUT) { struct ir_raw_event rawir = { .pulse = false, - .duration = sz->rdev->timeout + .duration = sz->rdev->rawir_timeout }; sz_push(sz, rawir); } else { @@ -334,7 +334,8 @@ static int streamzap_probe(struct usb_interface *intf, sz->decoder_state = PulseSpace; /* FIXME: don't yet have a way to set this */ - sz->rdev->timeout = SZ_TIMEOUT * SZ_RESOLUTION; + sz->rdev->rawir_timeout = SZ_TIMEOUT * SZ_RESOLUTION; + sz->rdev->keyup_delay = SZ_TIMEOUT * SZ_RESOLUTION; #if 0 /* not yet supported, depends on patches from maxim */ /* see also: LIRC_GET_REC_RESOLUTION and LIRC_SET_REC_TIMEOUT */ diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c index b631a81e58bb1..14503bf50b020 100644 --- a/drivers/media/rc/sunxi-cir.c +++ b/drivers/media/rc/sunxi-cir.c @@ -164,7 +164,7 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout) writel(REG_CIR_NTHR(SUNXI_IR_RXNOISE) | REG_CIR_ITHR(ithr), ir->base + SUNXI_IR_CIR_REG); - rc_dev->timeout = sunxi_ithr_to_usec(base_clk, ithr); + rc_dev->rawir_timeout = sunxi_ithr_to_usec(base_clk, ithr); return 0; } @@ -195,7 +195,7 @@ static int sunxi_ir_hw_init(struct device *dev) writel(REG_CTL_MD, ir->base + SUNXI_IR_CTL_REG); /* Set noise threshold and idle threshold */ - sunxi_ir_set_timeout(ir->rc, ir->rc->timeout); + sunxi_ir_set_timeout(ir->rc, ir->rc->rawir_timeout); /* Invert Input Signal */ writel(REG_RXCTL_RPPI, ir->base + SUNXI_IR_RXCTL_REG); @@ -324,7 +324,8 @@ static int sunxi_ir_probe(struct platform_device *pdev) ir->rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; /* Frequency after IR internal divider with sample period in us */ ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64)); - ir->rc->timeout = IR_DEFAULT_TIMEOUT; + ir->rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + ir->rc->keyup_delay = IR_DEFAULT_TIMEOUT; ir->rc->min_timeout = sunxi_ithr_to_usec(b_clk_freq, 0); ir->rc->max_timeout = sunxi_ithr_to_usec(b_clk_freq, 255); ir->rc->s_timeout = sunxi_ir_set_timeout; diff --git a/drivers/media/rc/ttusbir.c b/drivers/media/rc/ttusbir.c index 629787d53ee1c..d7e37e11fba04 100644 --- a/drivers/media/rc/ttusbir.c +++ b/drivers/media/rc/ttusbir.c @@ -307,7 +307,8 @@ static int ttusbir_probe(struct usb_interface *intf, rc->driver_name = DRIVER_NAME; rc->map_name = RC_MAP_TT_1500; rc->min_timeout = 1; - rc->timeout = IR_DEFAULT_TIMEOUT; + rc->rawir_timeout = IR_DEFAULT_TIMEOUT; + rc->keyup_delay = IR_DEFAULT_TIMEOUT; rc->max_timeout = 10 * IR_DEFAULT_TIMEOUT; /* diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c index 25884a79985c8..718d711fc75a4 100644 --- a/drivers/media/rc/winbond-cir.c +++ b/drivers/media/rc/winbond-cir.c @@ -1071,7 +1071,8 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id) data->dev->priv = data; data->dev->dev.parent = &device->dev; data->dev->min_timeout = 1; - data->dev->timeout = IR_DEFAULT_TIMEOUT; + data->dev->rawir_timeout = IR_DEFAULT_TIMEOUT; + data->dev->keyup_delay = IR_DEFAULT_TIMEOUT; data->dev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; data->dev->rx_resolution = 2; data->dev->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER; diff --git a/drivers/media/rc/xbox_remote.c b/drivers/media/rc/xbox_remote.c index 98d0b43608ad6..9dba3ff0dc2e0 100644 --- a/drivers/media/rc/xbox_remote.c +++ b/drivers/media/rc/xbox_remote.c @@ -157,7 +157,7 @@ static void xbox_remote_rc_init(struct xbox_remote *xbox_remote) rdev->device_name = xbox_remote->rc_name; rdev->input_phys = xbox_remote->rc_phys; - rdev->timeout = MS_TO_US(10); + rdev->keyup_delay = MS_TO_US(10); usb_to_input_id(xbox_remote->udev, &rdev->input_id); rdev->dev.parent = &xbox_remote->interface->dev; diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb.h b/drivers/media/usb/dvb-usb-v2/dvb_usb.h index 288c15a7d72be..b69ab5e6ab421 100644 --- a/drivers/media/usb/dvb-usb-v2/dvb_usb.h +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb.h @@ -132,7 +132,8 @@ struct dvb_usb_rc { unsigned int interval; enum rc_driver_type driver_type; bool bulk_mode; - int timeout; + u32 rawir_timeout; + u32 keyup_delay; }; /** diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c index f1c79f351ec8d..c3be2b8eaef73 100644 --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c @@ -150,7 +150,8 @@ static int dvb_usbv2_remote_init(struct dvb_usb_device *d) dev->map_name = d->rc.map_name; dev->allowed_protocols = d->rc.allowed_protos; dev->change_protocol = d->rc.change_protocol; - dev->timeout = d->rc.timeout; + dev->rawir_timeout = d->rc.rawir_timeout; + dev->keyup_delay = d->rc.keyup_delay; dev->priv = d; ret = rc_register_device(dev); diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index 795a012d40200..60e5153fcb24c 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1809,7 +1809,8 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d, rc->query = rtl2832u_rc_query; rc->interval = 200; /* we program idle len to 0xc0, set timeout to one less */ - rc->timeout = 0xbf * 51; + rc->rawir_timeout = 0xbf * 51; + rc->keyup_delay = MS_TO_US(210); return 0; } diff --git a/include/media/rc-core.h b/include/media/rc-core.h index 803349599c272..e7316891a7ac3 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -123,9 +123,11 @@ struct lirc_fh { * @last_protocol: protocol of last keypress * @last_scancode: scancode of last keypress * @last_toggle: toggle value of last command - * @timeout: optional time after which device stops sending data - * @min_timeout: minimum timeout supported by device - * @max_timeout: maximum timeout supported by device + * @rawir_timeout: length space to convert into IR timeout + * @keyup_delay: timeout before keyup event. This should be the maximum + * delay between reading IR events. + * @min_timeout: minimum rawir timeout supported by device + * @max_timeout: maximum rawir timeout supported by device * @rx_resolution : resolution (in us) of input sampler * @tx_resolution: resolution (in us) of output sampler * @lirc_dev: lirc device @@ -190,7 +192,8 @@ struct rc_dev { enum rc_proto last_protocol; u64 last_scancode; u8 last_toggle; - u32 timeout; + u32 rawir_timeout; + u32 keyup_delay; u32 min_timeout; u32 max_timeout; u32 rx_resolution; -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-02-12 16:32 [PATCH 0/2] Fix rtl28xxu nec/rc5 receiver Sean Young 2022-02-12 16:32 ` [PATCH 1/2] media: rc-core: split IR timeout into rawir timeout and keyup delay Sean Young @ 2022-02-12 16:32 ` Sean Young 2022-06-26 12:33 ` Marko Mäkelä 1 sibling, 1 reply; 17+ messages in thread From: Sean Young @ 2022-02-12 16:32 UTC (permalink / raw) To: linux-media; +Cc: Marko Mäkelä This device presents an IR buffer, which can be read and cleared. Clearing the buffer is racey with receiving IR, so wait until the IR message is finished before clearing it. This should minimize the chance of the buffer clear happening while IR is being received, although we cannot avoid this completely. Signed-off-by: Sean Young <sean@mess.org> --- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 27 +++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index 60e5153fcb24c..a83b1107fc7fd 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1720,6 +1720,7 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) {IR_RX_BUF_CTRL, 0x80, 0xff}, {IR_RX_CTRL, 0x80, 0xff}, }; + u32 idle_length; /* init remote controller */ if (!dev->rc_active) { @@ -1770,6 +1771,28 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) if (ret) goto err; + dev_dbg(&d->intf->dev, "IR_RX_BUF=%*ph\n", len, buf); + + /* if the receiver is not idle yet, do not process */ + idle_length = 0; + if (len > 2) { + if (!(buf[len - 1] & 0x80)) + idle_length += buf[len - 1]; + if (!(buf[len - 2] & 0x80)) + idle_length += buf[len - 2]; + } + + if (idle_length < 0xbf) { + /* + * If the IR does not end with a space equal to the idle + * length, then the IR is not complete yet and more is to + * arrive shortly. If we process it and flush the buffer now, + * we end up missing IR. + */ + dev_dbg(&d->intf->dev, "ignoring idle=%x\n", idle_length); + return 0; + } + /* let hw receive new code */ for (i = 0; i < ARRAY_SIZE(refresh_tab); i++) { ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg, @@ -1807,10 +1830,10 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d, rc->allowed_protos = RC_PROTO_BIT_ALL_IR_DECODER; rc->driver_type = RC_DRIVER_IR_RAW; rc->query = rtl2832u_rc_query; - rc->interval = 200; + rc->interval = 100; /* we program idle len to 0xc0, set timeout to one less */ rc->rawir_timeout = 0xbf * 51; - rc->keyup_delay = MS_TO_US(210); + rc->keyup_delay = MS_TO_US(110); return 0; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-02-12 16:32 ` [PATCH 2/2] media: rtl28xxu: improve IR receiver Sean Young @ 2022-06-26 12:33 ` Marko Mäkelä 2022-06-27 5:00 ` Marko Mäkelä 2022-06-27 10:53 ` Sean Young 0 siblings, 2 replies; 17+ messages in thread From: Marko Mäkelä @ 2022-06-26 12:33 UTC (permalink / raw) To: Sean Young; +Cc: linux-media Hi Sean, I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will handle the repeat. For lirc_scancode_event() it will never set the LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug. Sat, Feb 12, 2022 at 04:32:19PM +0000, Sean Young wrote: >This device presents an IR buffer, which can be read and cleared. >Clearing the buffer is racey with receiving IR, so wait until the IR >message is finished before clearing it. > >This should minimize the chance of the buffer clear happening while >IR is being received, although we cannot avoid this completely. I just realized that this limitation of the interface may be causing exactly what I was observing when I was testing this. If a constant stream of data is being received because a button is being held down, a buffer overflow or wrap-around glitch is inevitable, maybe expect if the wrap-around occurs exactly at the 128-byte boundary. How about the following improvement? If IR_RX_BC is a simple cursor to the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes at IR_RX_BUF, and process everything between the previous position reported by IR_RX_BC and the current position reported by IR_RX_BC, and treat buf[] as a ring buffer. Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel. Best regards, Marko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-06-26 12:33 ` Marko Mäkelä @ 2022-06-27 5:00 ` Marko Mäkelä 2022-07-02 8:17 ` Sean Young 2022-06-27 10:53 ` Sean Young 1 sibling, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-06-27 5:00 UTC (permalink / raw) To: Sean Young; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 1138 bytes --] Sun, Jun 26, 2022 at 03:33:48PM +0300, Marko Mäkelä wrote: >How about the following improvement? If IR_RX_BC is a simple cursor to >the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending >refresh_tab[] but simply remember where the previous call left off. We >could always read the 128 bytes at IR_RX_BUF, and process everything >between the previous position reported by IR_RX_BC and the current >position reported by IR_RX_BC, and treat buf[] as a ring buffer. I experimented with this on the 5.19.0-rc3 kernel. With the attached patch applied on top of this patch series, "ir-keytables -t" reported only one RC5 encoded key-down event. I had to unplug and plug in the adapter in order to receive another RC5 event. The refresh command seems to be necessary for the device to store and forward further IR data. >Last time I tested it, the patch was a significant improvement. I think >that "perfect" is the enemy of "good enough", and the patch should be >included in the kernel. The remaining problem definitely is a limitation of the interface. There is little that can be done to work around it. Marko [-- Attachment #2: rc_bc_ringbuffer.patch --] [-- Type: text/x-diff, Size: 2783 bytes --] diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index a83b1107fc7f..04670cec727c 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1711,16 +1711,10 @@ static int rtl2831u_get_rc_config(struct dvb_usb_device *d, static int rtl2832u_rc_query(struct dvb_usb_device *d) { - int ret, i, len; + int ret, i, end; struct rtl28xxu_dev *dev = d->priv; struct ir_raw_event ev = {}; u8 buf[128]; - static const struct rtl28xxu_reg_val_mask refresh_tab[] = { - {IR_RX_IF, 0x03, 0xff}, - {IR_RX_BUF_CTRL, 0x80, 0xff}, - {IR_RX_CTRL, 0x80, 0xff}, - }; - u32 idle_length; /* init remote controller */ if (!dev->rc_active) { @@ -1761,48 +1755,22 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) goto exit; ret = rtl28xxu_rd_reg(d, IR_RX_BC, &buf[0]); - if (ret || buf[0] > sizeof(buf)) + if (ret) goto err; - len = buf[0]; + i = dev->rc_bc; + end = dev->rc_bc = buf[0] & 0x7f; /* read raw code from hw */ - ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, len); + ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, sizeof buf); if (ret) goto err; - dev_dbg(&d->intf->dev, "IR_RX_BUF=%*ph\n", len, buf); - - /* if the receiver is not idle yet, do not process */ - idle_length = 0; - if (len > 2) { - if (!(buf[len - 1] & 0x80)) - idle_length += buf[len - 1]; - if (!(buf[len - 2] & 0x80)) - idle_length += buf[len - 2]; - } - - if (idle_length < 0xbf) { - /* - * If the IR does not end with a space equal to the idle - * length, then the IR is not complete yet and more is to - * arrive shortly. If we process it and flush the buffer now, - * we end up missing IR. - */ - dev_dbg(&d->intf->dev, "ignoring idle=%x\n", idle_length); - return 0; - } - - /* let hw receive new code */ - for (i = 0; i < ARRAY_SIZE(refresh_tab); i++) { - ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg, - refresh_tab[i].val, refresh_tab[i].mask); - if (ret) - goto err; - } + dev_dbg(&d->intf->dev, "IR_RX_BUF=%d,%*ph\n", end, + (int) sizeof buf, buf); /* pass data to Kernel IR decoder */ - for (i = 0; i < len; i++) { + for (; i != end; i++, i &= 0x7f) { ev.pulse = buf[i] >> 7; ev.duration = 51 * (buf[i] & 0x7f); ir_raw_event_store_with_filter(d->rc_dev, &ev); diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h index d5e207baa05d..b1abd73a3020 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h @@ -64,6 +64,7 @@ struct rtl28xxu_dev { u8 tuner; char *tuner_name; u8 page; /* integrated demod active register page */ + u8 rc_bc; struct i2c_adapter *demod_i2c_adapter; bool rc_active; bool new_i2c_write; ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-06-27 5:00 ` Marko Mäkelä @ 2022-07-02 8:17 ` Sean Young 0 siblings, 0 replies; 17+ messages in thread From: Sean Young @ 2022-07-02 8:17 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media On Mon, Jun 27, 2022 at 08:00:45AM +0300, Marko Mäkelä wrote: > Sun, Jun 26, 2022 at 03:33:48PM +0300, Marko Mäkelä wrote: > > How about the following improvement? If IR_RX_BC is a simple cursor to > > the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending > > refresh_tab[] but simply remember where the previous call left off. We > > could always read the 128 bytes at IR_RX_BUF, and process everything > > between the previous position reported by IR_RX_BC and the current > > position reported by IR_RX_BC, and treat buf[] as a ring buffer. > > I experimented with this on the 5.19.0-rc3 kernel. With the attached patch > applied on top of this patch series, "ir-keytables -t" reported only one RC5 > encoded key-down event. I had to unplug and plug in the adapter in order to > receive another RC5 event. The refresh command seems to be necessary for the > device to store and forward further IR data. > > > Last time I tested it, the patch was a significant improvement. I think > > that "perfect" is the enemy of "good enough", and the patch should be > > included in the kernel. > > The remaining problem definitely is a limitation of the interface. There is > little that can be done to work around it. This particular device is really badly designed. Unless we come up with a better solution to work around its limitations, it's just broken. Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-06-26 12:33 ` Marko Mäkelä 2022-06-27 5:00 ` Marko Mäkelä @ 2022-06-27 10:53 ` Sean Young 2022-06-28 6:27 ` Marko Mäkelä 1 sibling, 1 reply; 17+ messages in thread From: Sean Young @ 2022-06-27 10:53 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media Hi Marko, On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote: > I finally took the time to get a deeper understanding of the infrared remote > control subsystem. I think that I now understand the translation into > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will > handle the repeat. For lirc_scancode_event() it will never set the > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does > support the toggle bit. That might qualify as a bug. You are right, this was missed. Patches welcome. > Sat, Feb 12, 2022 at 04:32:19PM +0000, Sean Young wrote: > > This device presents an IR buffer, which can be read and cleared. > > Clearing the buffer is racey with receiving IR, so wait until the IR > > message is finished before clearing it. > > > > This should minimize the chance of the buffer clear happening while > > IR is being received, although we cannot avoid this completely. > > I just realized that this limitation of the interface may be causing exactly > what I was observing when I was testing this. If a constant stream of data > is being received because a button is being held down, a buffer overflow or > wrap-around glitch is inevitable, maybe expect if the wrap-around occurs > exactly at the 128-byte boundary. > > How about the following improvement? If IR_RX_BC is a simple cursor to the > 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending > refresh_tab[] but simply remember where the previous call left off. We could > always read the 128 bytes at IR_RX_BUF, and process everything between the > previous position reported by IR_RX_BC and the current position reported by > IR_RX_BC, and treat buf[] as a ring buffer. This is a great idea. Very original. > Last time I tested it, the patch was a significant improvement. I think that > "perfect" is the enemy of "good enough", and the patch should be included in > the kernel. The idea sounds really good. I'll review/test the patch and get back to you. Thanks, Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-06-27 10:53 ` Sean Young @ 2022-06-28 6:27 ` Marko Mäkelä 2022-07-02 8:14 ` Sean Young 0 siblings, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-06-28 6:27 UTC (permalink / raw) To: Sean Young; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 1977 bytes --] Hi Sean, Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote: >Hi Marko, > >On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote: >> I finally took the time to get a deeper understanding of the infrared remote >> control subsystem. I think that I now understand the translation into >> key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() >> will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will >> handle the repeat. For lirc_scancode_event() it will never set the >> LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does >> support the toggle bit. That might qualify as a bug. > >You are right, this was missed. Patches welcome. Attached (for 5.19.0-rc3, on top of the two commits of this patch series). I thought that it would be the least amount of trouble to slightly change the interpretation of the "toggle" parameter of rc_keydown(). My intention was to use the values 1 and 2 when the toggle flag is present. Any nonzero values would work. I am not that familiar with updating the modules, and I suspect that instead of actually testing this code, I was testing the "ring buffer" patch that I posted yesterday. I could not use the rmmod/insmod approach for testing this change, because the rc_core module was in use by the display driver. So, I can only say that the patch compiled for me. A few FIXME places are indicated where I am not sure that a correct (nonzero) toggle value would be computed. An alternative approach would be to use the value toggle=1 for the case when the toggle bit is set, and toggle>1 for the case when it is not set. Basically, change things like 1+!!x to 1+!x in the callers, and change the condition toggle > 1 to toggle == 1 in rc-main.c. In that way, any old driver that would use the toggle values 0 and 1 would still generate LIRC_SCANCODE_FLAG_TOGGLE. But then, the repeat_event logic would only work half the time (when toggle!=0). Marko [-- Attachment #2: 0001-rc_keydown-Report-LIRC_SCANCODE_FLAG_REPEAT-based-on.patch --] [-- Type: text/x-diff, Size: 17174 bytes --] From 29a5c2a00653f49c854109b2f2c8f99b4431430f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@iki.fi> Date: Tue, 28 Jun 2022 07:59:17 +0300 Subject: [PATCH] rc_keydown(): Report LIRC_SCANCODE_FLAG_REPEAT based on toggle bit Drivers that will not call rc_repeat() will let rc_keydown() create repeat events. For the LIRC interface, the repeat flag would only be set by rc_repeat(), never by rc_keydown(). We change the meaning of the toggle parameter: Drivers that invoke rc_repeat() by themselves should always pass toggle=0, while protocols that include a toggle bit should pass toggle>0, with the value 1 meaning that the toggle bit is clear. This is largely untested code. See FIXME comments. Also, an interface change for bpf_rc_keydown() might have to be documented. --- drivers/media/cec/platform/seco/seco-cec.c | 3 +- drivers/media/i2c/ir-kbd-i2c.c | 4 +- drivers/media/pci/bt8xx/bttv-input.c | 2 +- drivers/media/pci/ttpci/budget-ci.c | 4 +- drivers/media/rc/bpf-lirc.c | 2 +- drivers/media/rc/img-ir/img-ir-rc5.c | 2 +- drivers/media/rc/img-ir/img-ir-rc6.c | 2 +- drivers/media/rc/imon.c | 2 +- drivers/media/rc/ir-jvc-decoder.c | 3 +- drivers/media/rc/ir-rc5-decoder.c | 2 +- drivers/media/rc/ir-rc6-decoder.c | 4 +- drivers/media/rc/ir-rcmm-decoder.c | 2 +- drivers/media/rc/rc-main.c | 9 ++-- drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- drivers/media/usb/dvb-usb-v2/dvbsky.c | 2 +- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 48 ++++----------------- drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 1 + drivers/media/usb/dvb-usb/dib0700_core.c | 2 +- drivers/media/usb/dvb-usb/dib0700_devices.c | 2 +- drivers/media/usb/dvb-usb/ttusb2.c | 3 +- drivers/media/usb/em28xx/em28xx-input.c | 4 +- drivers/staging/media/av7110/av7110_ir.c | 2 +- 22 files changed, 40 insertions(+), 67 deletions(-) diff --git a/drivers/media/cec/platform/seco/seco-cec.c b/drivers/media/cec/platform/seco/seco-cec.c index a056412883b9..6aa889add090 100644 --- a/drivers/media/cec/platform/seco/seco-cec.c +++ b/drivers/media/cec/platform/seco/seco-cec.c @@ -416,7 +416,8 @@ static int secocec_ir_rx(struct secocec_data *priv) addr = (val & SECOCEC_IR_ADDRESS_MASK) >> SECOCEC_IR_ADDRESS_SHL; toggle = (val & SECOCEC_IR_TOGGLE_MASK) >> SECOCEC_IR_TOGGLE_SHL; - rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle); + rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), + 1 + toggle); dev_dbg(dev, "IR key pressed: 0x%02x addr 0x%02x toggle 0x%02x\n", key, addr, toggle); diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c index 56674173524f..28dbce16dc19 100644 --- a/drivers/media/i2c/ir-kbd-i2c.c +++ b/drivers/media/i2c/ir-kbd-i2c.c @@ -99,7 +99,7 @@ static int get_key_haup_common(struct IR_i2c *ir, enum rc_proto *protocol, *protocol = RC_PROTO_RC5; *scancode = RC_SCANCODE_RC5(dev, code); - *ptoggle = toggle; + *ptoggle = toggle + 1; return 1; } else if (size == 6 && (buf[0] & 0x40)) { @@ -108,7 +108,7 @@ static int get_key_haup_common(struct IR_i2c *ir, enum rc_proto *protocol, vendor = get_unaligned_be16(buf + 1); if (vendor == 0x800f) { - *ptoggle = (dev & 0x80) != 0; + *ptoggle = 1 + ((dev & 0x80) != 0); *protocol = RC_PROTO_RC6_MCE; dev &= 0x7f; dev_dbg(&ir->rc->dev, diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c index 41226f1d0e5b..a82e40b13078 100644 --- a/drivers/media/pci/bt8xx/bttv-input.c +++ b/drivers/media/pci/bt8xx/bttv-input.c @@ -228,7 +228,7 @@ static void bttv_rc5_timer_end(struct timer_list *t) } scancode = RC_SCANCODE_RC5(system, command); - rc_keydown(ir->dev, RC_PROTO_RC5, scancode, toggle); + rc_keydown(ir->dev, RC_PROTO_RC5, scancode, toggle + 1); dprintk("scancode %x, toggle %x\n", scancode, toggle); } diff --git a/drivers/media/pci/ttpci/budget-ci.c b/drivers/media/pci/ttpci/budget-ci.c index d59d18647371..50c9f5672cc0 100644 --- a/drivers/media/pci/ttpci/budget-ci.c +++ b/drivers/media/pci/ttpci/budget-ci.c @@ -147,13 +147,13 @@ static void msp430_ir_interrupt(struct tasklet_struct *t) if (budget_ci->ir.full_rc5) { rc_keydown(dev, RC_PROTO_RC5, RC_SCANCODE_RC5(budget_ci->ir.rc5_device, budget_ci->ir.ir_key), - !!(command & 0x20)); + 1 + !!(command & 0x20)); return; } /* FIXME: We should generate complete scancodes for all devices */ rc_keydown(dev, RC_PROTO_UNKNOWN, budget_ci->ir.ir_key, - !!(command & 0x20)); + 1 + !!(command & 0x20)); } static int msp430_ir_init(struct budget_ci *budget_ci) diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c index fe17c7f98e81..829302c6d891 100644 --- a/drivers/media/rc/bpf-lirc.c +++ b/drivers/media/rc/bpf-lirc.c @@ -42,7 +42,7 @@ BPF_CALL_4(bpf_rc_keydown, u32*, sample, u32, protocol, u64, scancode, ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample); - rc_keydown(ctrl->dev, protocol, scancode, toggle != 0); + rc_keydown(ctrl->dev, protocol, scancode, (u8) toggle); return 0; } diff --git a/drivers/media/rc/img-ir/img-ir-rc5.c b/drivers/media/rc/img-ir/img-ir-rc5.c index 23c8e2397ba7..386be464bfa7 100644 --- a/drivers/media/rc/img-ir/img-ir-rc5.c +++ b/drivers/media/rc/img-ir/img-ir-rc5.c @@ -31,7 +31,7 @@ static int img_ir_rc5_scancode(int len, u64 raw, u64 enabled_protocols, request->protocol = RC_PROTO_RC5; request->scancode = addr << 8 | cmd; - request->toggle = tgl; + request->toggle = 1 + tgl; return IMG_IR_SCANCODE; } diff --git a/drivers/media/rc/img-ir/img-ir-rc6.c b/drivers/media/rc/img-ir/img-ir-rc6.c index b2bf46886420..5baa74a67d72 100644 --- a/drivers/media/rc/img-ir/img-ir-rc6.c +++ b/drivers/media/rc/img-ir/img-ir-rc6.c @@ -52,7 +52,7 @@ static int img_ir_rc6_scancode(int len, u64 raw, u64 enabled_protocols, request->protocol = RC_PROTO_RC6_0; request->scancode = addr << 8 | cmd; - request->toggle = trl2; + request->toggle = 1 + trl2; return IMG_IR_SCANCODE; } diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 735b925da998..0fd0e41fd1d5 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -1552,7 +1552,7 @@ static int imon_parse_press_type(struct imon_context *ictx, /* mce-specific button handling, no keyup events */ else if (ktype == IMON_KEY_MCE) { - ictx->rc_toggle = buf[2]; + ictx->rc_toggle = 1 + buf[2]; /* FIXME: test this */ press_type = 1; /* incoherent or irrelevant data */ diff --git a/drivers/media/rc/ir-jvc-decoder.c b/drivers/media/rc/ir-jvc-decoder.c index 8b10954d2b6b..c72aa1c2f15a 100644 --- a/drivers/media/rc/ir-jvc-decoder.c +++ b/drivers/media/rc/ir-jvc-decoder.c @@ -129,7 +129,8 @@ static int ir_jvc_decode(struct rc_dev *dev, struct ir_raw_event ev) scancode = (bitrev8((data->bits >> 8) & 0xff) << 8) | (bitrev8((data->bits >> 0) & 0xff) << 0); dev_dbg(&dev->dev, "JVC scancode 0x%04x\n", scancode); - rc_keydown(dev, RC_PROTO_JVC, scancode, data->toggle); + rc_keydown(dev, RC_PROTO_JVC, scancode, + 1 + data->toggle); data->first = false; data->old_bits = data->bits; } else if (data->bits == data->old_bits) { diff --git a/drivers/media/rc/ir-rc5-decoder.c b/drivers/media/rc/ir-rc5-decoder.c index 82d7f6ad2338..9a1071418d61 100644 --- a/drivers/media/rc/ir-rc5-decoder.c +++ b/drivers/media/rc/ir-rc5-decoder.c @@ -157,7 +157,7 @@ static int ir_rc5_decode(struct rc_dev *dev, struct ir_raw_event ev) dev_dbg(&dev->dev, "RC5(x/sz) scancode 0x%06x (p: %u, t: %u)\n", scancode, protocol, toggle); - rc_keydown(dev, protocol, scancode, toggle); + rc_keydown(dev, protocol, scancode, 1 + toggle); data->state = STATE_INACTIVE; return 0; } diff --git a/drivers/media/rc/ir-rc6-decoder.c b/drivers/media/rc/ir-rc6-decoder.c index 3b2c8bab3e73..74dfc270e7f7 100644 --- a/drivers/media/rc/ir-rc6-decoder.c +++ b/drivers/media/rc/ir-rc6-decoder.c @@ -212,7 +212,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev) switch (rc6_mode(data)) { case RC6_MODE_0: scancode = data->body; - toggle = data->toggle; + toggle = 1 + data->toggle; protocol = RC_PROTO_RC6_0; dev_dbg(&dev->dev, "RC6(0) scancode 0x%04x (toggle: %u)\n", scancode, toggle); @@ -241,7 +241,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev) case RC6_6A_KATHREIN_CC: case RC6_6A_ZOTAC_CC: protocol = RC_PROTO_RC6_MCE; - toggle = !!(scancode & RC6_6A_MCE_TOGGLE_MASK); + toggle = 1 + !!(scancode & RC6_6A_MCE_TOGGLE_MASK); scancode &= ~RC6_6A_MCE_TOGGLE_MASK; break; default: diff --git a/drivers/media/rc/ir-rcmm-decoder.c b/drivers/media/rc/ir-rcmm-decoder.c index a8a34436fe85..7c43bc22f580 100644 --- a/drivers/media/rc/ir-rcmm-decoder.c +++ b/drivers/media/rc/ir-rcmm-decoder.c @@ -148,7 +148,7 @@ static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev) break; if (rcmm_mode(data)) { - toggle = !!(0x8000 & data->bits); + toggle = 1 + !!(0x8000 & data->bits); scancode = data->bits & ~0x8000; } else { toggle = 0; diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index f27f6b383816..dbea6eda83ee 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, { bool new_event = (!dev->keypressed || dev->last_protocol != protocol || - dev->last_scancode != scancode || - dev->last_toggle != toggle); + dev->last_scancode != scancode); + bool repeat_event = !new_event && toggle && dev->last_toggle == toggle; struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0, + .flags = (toggle > 1 ? LIRC_SCANCODE_FLAG_TOGGLE : 0) | + (repeat_event ? LIRC_SCANCODE_FLAG_REPEAT : 0), .keycode = keycode }; if (dev->allowed_protocols != RC_PROTO_BIT_CEC) lirc_scancode_event(dev, &sc); - if (new_event && dev->keypressed) + if (!repeat_event && dev->keypressed) ir_do_keyup(dev, false); if (scancode <= U32_MAX) diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c index 62ee09f28a0b..cc218c0d71a8 100644 --- a/drivers/media/usb/dvb-usb-v2/az6007.c +++ b/drivers/media/usb/dvb-usb-v2/az6007.c @@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) proto = RC_PROTO_NEC32; } - rc_keydown(d->rc_dev, proto, code, st->data[5]); + rc_keydown(d->rc_dev, proto, code, 1 + st->data[5]); /* FIXME */ return 0; } diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 1221c924312a..e264c774c64f 100644 --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c @@ -192,7 +192,7 @@ static int dvbsky_rc_query(struct dvb_usb_device *d) rc5_system = (code & 0x7C0) >> 6; toggle = (code & 0x800) ? 1 : 0; scancode = rc5_system << 8 | rc5_command; - rc_keydown(d->rc_dev, RC_PROTO_RC5, scancode, toggle); + rc_keydown(d->rc_dev, RC_PROTO_RC5, scancode, 1 + toggle); } return 0; } diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c index a83b1107fc7f..04670cec727c 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c @@ -1711,16 +1711,10 @@ static int rtl2831u_get_rc_config(struct dvb_usb_device *d, static int rtl2832u_rc_query(struct dvb_usb_device *d) { - int ret, i, len; + int ret, i, end; struct rtl28xxu_dev *dev = d->priv; struct ir_raw_event ev = {}; u8 buf[128]; - static const struct rtl28xxu_reg_val_mask refresh_tab[] = { - {IR_RX_IF, 0x03, 0xff}, - {IR_RX_BUF_CTRL, 0x80, 0xff}, - {IR_RX_CTRL, 0x80, 0xff}, - }; - u32 idle_length; /* init remote controller */ if (!dev->rc_active) { @@ -1761,48 +1755,22 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) goto exit; ret = rtl28xxu_rd_reg(d, IR_RX_BC, &buf[0]); - if (ret || buf[0] > sizeof(buf)) + if (ret) goto err; - len = buf[0]; + i = dev->rc_bc; + end = dev->rc_bc = buf[0] & 0x7f; /* read raw code from hw */ - ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, len); + ret = rtl28xxu_rd_regs(d, IR_RX_BUF, buf, sizeof buf); if (ret) goto err; - dev_dbg(&d->intf->dev, "IR_RX_BUF=%*ph\n", len, buf); - - /* if the receiver is not idle yet, do not process */ - idle_length = 0; - if (len > 2) { - if (!(buf[len - 1] & 0x80)) - idle_length += buf[len - 1]; - if (!(buf[len - 2] & 0x80)) - idle_length += buf[len - 2]; - } - - if (idle_length < 0xbf) { - /* - * If the IR does not end with a space equal to the idle - * length, then the IR is not complete yet and more is to - * arrive shortly. If we process it and flush the buffer now, - * we end up missing IR. - */ - dev_dbg(&d->intf->dev, "ignoring idle=%x\n", idle_length); - return 0; - } - - /* let hw receive new code */ - for (i = 0; i < ARRAY_SIZE(refresh_tab); i++) { - ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg, - refresh_tab[i].val, refresh_tab[i].mask); - if (ret) - goto err; - } + dev_dbg(&d->intf->dev, "IR_RX_BUF=%d,%*ph\n", end, + (int) sizeof buf, buf); /* pass data to Kernel IR decoder */ - for (i = 0; i < len; i++) { + for (; i != end; i++, i &= 0x7f) { ev.pulse = buf[i] >> 7; ev.duration = 51 * (buf[i] & 0x7f); ir_raw_event_store_with_filter(d->rc_dev, &ev); diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h index d5e207baa05d..b1abd73a3020 100644 --- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.h +++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.h @@ -64,6 +64,7 @@ struct rtl28xxu_dev { u8 tuner; char *tuner_name; u8 page; /* integrated demod active register page */ + u8 rc_bc; struct i2c_adapter *demod_i2c_adapter; bool rc_active; bool new_i2c_write; diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c index 1caabb51ea47..02bb7349857f 100644 --- a/drivers/media/usb/dvb-usb/dib0700_core.c +++ b/drivers/media/usb/dvb-usb/dib0700_core.c @@ -780,7 +780,7 @@ static void dib0700_rc_urb_completion(struct urb *purb) default: deb_data("RC5 protocol\n"); protocol = RC_PROTO_RC5; - toggle = poll_reply->report_id; + toggle = 1 + poll_reply->report_id; /* FIXME */ keycode = RC_SCANCODE_RC5(poll_reply->rc5.system, poll_reply->rc5.data); if ((poll_reply->rc5.data ^ poll_reply->rc5.not_data) != 0xff) { diff --git a/drivers/media/usb/dvb-usb/dib0700_devices.c b/drivers/media/usb/dvb-usb/dib0700_devices.c index 7f8bebfa3e8e..7cbc1a251f47 100644 --- a/drivers/media/usb/dvb-usb/dib0700_devices.c +++ b/drivers/media/usb/dvb-usb/dib0700_devices.c @@ -565,7 +565,7 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) /* RC-5 protocol changes toggle bit on new keypress */ protocol = RC_PROTO_RC5; scancode = RC_SCANCODE_RC5(st->buf[3 - 2], st->buf[3 - 3]); - toggle = st->buf[3 - 1]; + toggle = 1 + st->buf[3 - 1]; /* FIXME: test this */ break; } diff --git a/drivers/media/usb/dvb-usb/ttusb2.c b/drivers/media/usb/dvb-usb/ttusb2.c index 373ffa7f641e..c649ae5415a3 100644 --- a/drivers/media/usb/dvb-usb/ttusb2.c +++ b/drivers/media/usb/dvb-usb/ttusb2.c @@ -456,7 +456,8 @@ static int tt3650_rc_query(struct dvb_usb_device *d) /* got a "press" event */ st->last_rc_key = RC_SCANCODE_RC5(rx[3], rx[2]); deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]); - rc_keydown(d->rc_dev, RC_PROTO_RC5, st->last_rc_key, rx[1]); + rc_keydown(d->rc_dev, RC_PROTO_RC5, st->last_rc_key, + 1 + rx[1]); /* FIXME: test this */ } else if (st->last_rc_key) { rc_keyup(d->rc_dev); st->last_rc_key = 0; diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c index 0b6d77c3bec8..79f7c2540c02 100644 --- a/drivers/media/usb/em28xx/em28xx-input.c +++ b/drivers/media/usb/em28xx/em28xx-input.c @@ -340,12 +340,12 @@ static void em28xx_ir_handle_key(struct em28xx_IR *ir) rc_keydown(ir->rc, poll_result.protocol, poll_result.scancode, - poll_result.toggle_bit); + 1 + poll_result.toggle_bit); else rc_keydown(ir->rc, RC_PROTO_UNKNOWN, poll_result.scancode & 0xff, - poll_result.toggle_bit); + 1 + poll_result.toggle_bit); if (ir->dev->chip_id == CHIP_ID_EM2874 || ir->dev->chip_id == CHIP_ID_EM2884) diff --git a/drivers/staging/media/av7110/av7110_ir.c b/drivers/staging/media/av7110/av7110_ir.c index a851ba328e4a..ec33a4be3cd5 100644 --- a/drivers/staging/media/av7110/av7110_ir.c +++ b/drivers/staging/media/av7110/av7110_ir.c @@ -64,7 +64,7 @@ void av7110_ir_handler(struct av7110 *av7110, u32 ircom) return; } - rc_keydown(rcdev, proto, scancode, toggle != 0); + rc_keydown(rcdev, proto, scancode, 1 + !!toggle); } } -- 2.36.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-06-28 6:27 ` Marko Mäkelä @ 2022-07-02 8:14 ` Sean Young 2022-07-03 17:02 ` Marko Mäkelä 0 siblings, 1 reply; 17+ messages in thread From: Sean Young @ 2022-07-02 8:14 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media Hi, On Tue, Jun 28, 2022 at 09:27:26AM +0300, Marko Mäkelä wrote: > Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote: > > Hi Marko, > > > > On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote: > > > I finally took the time to get a deeper understanding of the infrared remote > > > control subsystem. I think that I now understand the translation into > > > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() > > > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will > > > handle the repeat. For lirc_scancode_event() it will never set the > > > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does > > > support the toggle bit. That might qualify as a bug. > > > > You are right, this was missed. Patches welcome. > > Attached (for 5.19.0-rc3, on top of the two commits of this patch series). > > I thought that it would be the least amount of trouble to slightly change > the interpretation of the "toggle" parameter of > rc_keydown(). My intention was to use the values 1 and 2 when the toggle > flag is present. Any nonzero values would work. I don't understand why this is needed. > I am not that familiar with updating the modules, and I suspect that instead > of actually testing this code, I was testing the "ring buffer" patch that I > posted yesterday. I could not use the rmmod/insmod approach for testing this > change, because the rc_core module was in use by the display driver. So, I > can only say that the patch compiled for me. A few FIXME places are > indicated where I am not sure that a correct (nonzero) toggle value would be > computed. A patch needs to be tested. Just rebuild the entire kernel and boot from that. > An alternative approach would be to use the value toggle=1 for the case when > the toggle bit is set, and toggle>1 for the case when it is not set. > Basically, change things like 1+!!x to 1+!x in the callers, and change the > condition toggle > 1 to toggle == 1 in rc-main.c. In that way, any old > driver that would use the toggle values 0 and 1 would still generate > LIRC_SCANCODE_FLAG_TOGGLE. But then, the repeat_event logic would only work > half the time (when toggle!=0). > > Marko > >From 29a5c2a00653f49c854109b2f2c8f99b4431430f Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@iki.fi> > Date: Tue, 28 Jun 2022 07:59:17 +0300 > Subject: [PATCH] rc_keydown(): Report LIRC_SCANCODE_FLAG_REPEAT based on > toggle bit > > Drivers that will not call rc_repeat() will let rc_keydown() > create repeat events. For the LIRC interface, the repeat flag > would only be set by rc_repeat(), never by rc_keydown(). > > We change the meaning of the toggle parameter: Drivers that > invoke rc_repeat() by themselves should always pass toggle=0, > while protocols that include a toggle bit should pass toggle>0, > with the value 1 meaning that the toggle bit is clear. > > This is largely untested code. See FIXME comments. > Also, an interface change for bpf_rc_keydown() might have to be > documented. > --- > drivers/media/cec/platform/seco/seco-cec.c | 3 +- > drivers/media/i2c/ir-kbd-i2c.c | 4 +- > drivers/media/pci/bt8xx/bttv-input.c | 2 +- > drivers/media/pci/ttpci/budget-ci.c | 4 +- > drivers/media/rc/bpf-lirc.c | 2 +- > drivers/media/rc/img-ir/img-ir-rc5.c | 2 +- > drivers/media/rc/img-ir/img-ir-rc6.c | 2 +- > drivers/media/rc/imon.c | 2 +- > drivers/media/rc/ir-jvc-decoder.c | 3 +- > drivers/media/rc/ir-rc5-decoder.c | 2 +- > drivers/media/rc/ir-rc6-decoder.c | 4 +- > drivers/media/rc/ir-rcmm-decoder.c | 2 +- > drivers/media/rc/rc-main.c | 9 ++-- > drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- > drivers/media/usb/dvb-usb-v2/dvbsky.c | 2 +- > drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 48 ++++----------------- > drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 1 + > drivers/media/usb/dvb-usb/dib0700_core.c | 2 +- > drivers/media/usb/dvb-usb/dib0700_devices.c | 2 +- > drivers/media/usb/dvb-usb/ttusb2.c | 3 +- > drivers/media/usb/em28xx/em28xx-input.c | 4 +- > drivers/staging/media/av7110/av7110_ir.c | 2 +- > 22 files changed, 40 insertions(+), 67 deletions(-) > > diff --git a/drivers/media/cec/platform/seco/seco-cec.c b/drivers/media/cec/platform/seco/seco-cec.c > index a056412883b9..6aa889add090 100644 > --- a/drivers/media/cec/platform/seco/seco-cec.c > +++ b/drivers/media/cec/platform/seco/seco-cec.c > @@ -416,7 +416,8 @@ static int secocec_ir_rx(struct secocec_data *priv) > addr = (val & SECOCEC_IR_ADDRESS_MASK) >> SECOCEC_IR_ADDRESS_SHL; > toggle = (val & SECOCEC_IR_TOGGLE_MASK) >> SECOCEC_IR_TOGGLE_SHL; > > - rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle); > + rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), > + 1 + toggle); You can't change the toggle value because you want a repeat flag. This makes no sense. -snip- > --- a/drivers/media/rc/rc-main.c > +++ b/drivers/media/rc/rc-main.c > @@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, > { > bool new_event = (!dev->keypressed || > dev->last_protocol != protocol || > - dev->last_scancode != scancode || > - dev->last_toggle != toggle); > + dev->last_scancode != scancode); > + bool repeat_event = !new_event && toggle && dev->last_toggle == toggle; Why this change? > struct lirc_scancode sc = { > .scancode = scancode, .rc_proto = protocol, > - .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0, > + .flags = (toggle > 1 ? LIRC_SCANCODE_FLAG_TOGGLE : 0) | > + (repeat_event ? LIRC_SCANCODE_FLAG_REPEAT : 0), Why not simply (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0) and be done with it? > .keycode = keycode > }; > > if (dev->allowed_protocols != RC_PROTO_BIT_CEC) > lirc_scancode_event(dev, &sc); > > - if (new_event && dev->keypressed) > + if (!repeat_event && dev->keypressed) > ir_do_keyup(dev, false); > > if (scancode <= U32_MAX) > diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c > index 62ee09f28a0b..cc218c0d71a8 100644 > --- a/drivers/media/usb/dvb-usb-v2/az6007.c > +++ b/drivers/media/usb/dvb-usb-v2/az6007.c > @@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) > proto = RC_PROTO_NEC32; > } > > - rc_keydown(d->rc_dev, proto, code, st->data[5]); > + rc_keydown(d->rc_dev, proto, code, 1 + st->data[5]); /* FIXME */ I still have no idea what you are trying to achieve with this. Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-02 8:14 ` Sean Young @ 2022-07-03 17:02 ` Marko Mäkelä 2022-07-04 7:21 ` Sean Young 0 siblings, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-07-03 17:02 UTC (permalink / raw) To: Sean Young; +Cc: linux-media Sat, Jul 02, 2022 at 09:14:59AM +0100, Sean Young wrote: >Hi, > >On Tue, Jun 28, 2022 at 09:27:26AM +0300, Marko Mäkelä wrote: >> Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote: >> > Hi Marko, >> > >> > On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote: >> > > I finally took the time to get a deeper understanding of the infrared remote >> > > control subsystem. I think that I now understand the translation into >> > > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() >> > > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will >> > > handle the repeat. For lirc_scancode_event() it will never set the >> > > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does >> > > support the toggle bit. That might qualify as a bug. >> > >> > You are right, this was missed. Patches welcome. >> >> Attached (for 5.19.0-rc3, on top of the two commits of this patch series). >> >> I thought that it would be the least amount of trouble to slightly change >> the interpretation of the "toggle" parameter of >> rc_keydown(). My intention was to use the values 1 and 2 when the toggle >> flag is present. Any nonzero values would work. > >I don't understand why this is needed. For protocols that do not use a toggle bit, the last parameter of rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() will be issued when needed. For those protocols, I thought that we would not want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any circumstances. >A patch needs to be tested. Just rebuild the entire kernel and boot >from that. Yes, I will do that for revising my patch. >> - rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle); >> + rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), >> + 1 + toggle); > >You can't change the toggle value because you want a repeat flag. This makes >no sense. [snip] >> --- a/drivers/media/rc/rc-main.c >> +++ b/drivers/media/rc/rc-main.c >> @@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, >> { >> bool new_event = (!dev->keypressed || >> dev->last_protocol != protocol || >> - dev->last_scancode != scancode || >> - dev->last_toggle != toggle); >> + dev->last_scancode != scancode); >> + bool repeat_event = !new_event && toggle && dev->last_toggle == toggle; > >Why this change? > >> struct lirc_scancode sc = { >> .scancode = scancode, .rc_proto = protocol, >> - .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0, >> + .flags = (toggle > 1 ? LIRC_SCANCODE_FLAG_TOGGLE : 0) | >> + (repeat_event ? LIRC_SCANCODE_FLAG_REPEAT : 0), > >Why not simply (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0) and be done with it? Drivers that invoke rc_repeat() do not want rc_keydown() to ever set LIRC_SCANCODE_FLAG_REPEAT. The patch slightly changed the meaning of toggle: it *must* be 0 if and only if the protocol does not implement a toggle bit. If it does, the values must alternate between 1 and some greater-than-1 value. A cleaner alternative could be to retain the interface of rc_keydown() as is, and add a new function, say, rc_keydown_or_repeat(), which would generate key-repeat events from the toggle bit. Best regards, Marko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-03 17:02 ` Marko Mäkelä @ 2022-07-04 7:21 ` Sean Young 2022-07-04 9:20 ` Marko Mäkelä 0 siblings, 1 reply; 17+ messages in thread From: Sean Young @ 2022-07-04 7:21 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: > Sat, Jul 02, 2022 at 09:14:59AM +0100, Sean Young wrote: > > Hi, > > > > On Tue, Jun 28, 2022 at 09:27:26AM +0300, Marko Mäkelä wrote: > > > Mon, Jun 27, 2022 at 11:53:52AM +0100, Sean Young wrote: > > > > Hi Marko, > > > > > > > > On Sun, Jun 26, 2022 at 03:33:47PM +0300, Marko Mäkelä wrote: > > > > > I finally took the time to get a deeper understanding of the infrared remote > > > > > control subsystem. I think that I now understand the translation into > > > > > key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() > > > > > will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will > > > > > handle the repeat. For lirc_scancode_event() it will never set the > > > > > LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does > > > > > support the toggle bit. That might qualify as a bug. > > > > > > > > You are right, this was missed. Patches welcome. > > > > > > Attached (for 5.19.0-rc3, on top of the two commits of this patch series). > > > > > > I thought that it would be the least amount of trouble to slightly change > > > the interpretation of the "toggle" parameter of > > > rc_keydown(). My intention was to use the values 1 and 2 when the toggle > > > flag is present. Any nonzero values would work. > > > > I don't understand why this is needed. > > For protocols that do not use a toggle bit, the last parameter of > rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() > will be issued when needed. For those protocols, I thought that we would not > want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any > circumstances. Toggle and repeat are distinct concepts. rc_repeat() is for protocols which have a special repeat message, which carry no information other that "repeat the last message". However, all protocols repeat. Whether they use a special repeat message or not. It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT is set. Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-04 7:21 ` Sean Young @ 2022-07-04 9:20 ` Marko Mäkelä 2022-07-04 10:00 ` Sean Young 0 siblings, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-07-04 9:20 UTC (permalink / raw) To: Sean Young; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 3781 bytes --] Hi Sean, Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: >On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: >> For protocols that do not use a toggle bit, the last parameter of >> rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() >> will be issued when needed. For those protocols, I thought that we would not >> want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any >> circumstances. > >Toggle and repeat are distinct concepts. > >rc_repeat() is for protocols which have a special repeat message, which >carry no information other that "repeat the last message". However, >all protocols repeat. Whether they use a special repeat message or not. > >It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT >is set. Is it right to set the flag when a message is being repeated due to user effort (repeatedly pressing and releasing a button, instead of holding the button down)? If it is, is it consistent to avoid setting the flag when a protocol uses a toggle bit (say, RC5)? In that case, the toggle bit would change its value each time the button is pressed, and your suggested change to rc_keydown() would not set the repeat flag. As far as I understand, the change that you suggested would set the LIRC_SCANCODE_FLAG_REPEAT if I repeatedly press a button on the NEC protocol remote control, but not on an RC5 remote control. I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core. By the way, the patch that I sent earlier accidentally included my futile attempt at avoiding buffer overrun on the rtl28xxu. That is probably why my previous test failed. With the NEC protocol of the bundled remote of the Astrometa DVB-T2 adapter, the "repeat" flag was occasionally set when I repeated keypresses too quickly. I think that this was because the key matrix scanning algorithm on the remote control did not get a reading of "no key pressed" between two key presses. When I used an RC5 based remote (that of Hauppauge Nova-T PCI 90002), I only saw the "repeat" flag in the output of "ir-keytable -t" when holding a button down. If I repeated keypresses manually, the toggle bit was changing between them. In other words, for these two remote controls, the patch works exactly like I intended. One might think that it is not necessary to make difference between long button presses (which should generate repeat events) and short button presses that are quickly repeated by the user. I can think of a user-space application that would intentionally ignore repeat events for some buttons where it would make little sense. For example, when the number button 1 is pressed for a long time, the application might choose not to repeat the keypress, but "demand" multiple separate button presses by the user, if the channel should really be switched to 11, 111, or 1111. The intention of ignoring "repeat" events would be to avoid "punishing" users who are pressing a button longer, possibly compensating for unreliable IR signal reception. If the user wants to quickly switch to channel 111 by quickly pressing the button three times, it should not be misreported as an auto-repeated event, but reported as 3 LIRC events without the "repeat" flag, and as 3 pairs of keydown and keyup events. On the other hand, there should be no reason for an application to not honor repeat events for a volume button. That is of course up to the application to decide, not the kernel. If you agree that this patch is on the right track, an interface for the new function rc_keydown_or_repeat() may have to be exposed to the BPF interface as well. Marko [-- Attachment #2: 0001-media-rc-Report-LIRC_SCANCODE_FLAG_REPEAT-when-not-u.patch --] [-- Type: text/x-diff, Size: 15781 bytes --] From 1a1825e6ae737d840e2330db454e0426f7658d4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@iki.fi> Date: Mon, 4 Jul 2022 12:07:26 +0300 Subject: [PATCH] media: rc: Report LIRC_SCANCODE_FLAG_REPEAT when not using rc_repeat() The flag LIRC_SCANCODE_FLAG_REPEAT is never set on rc_keydown(). Previously it was only set by rc_repeat(). The new function rc_keydown_or_repeat() is like rc_keydown(), but it will automatically set the LIRC_SCANCODE_FLAG_REPEAT flag for repeated events. For protocols for which rc_repeat() will not be invoked, calls to the new function rc_keydown_or_repeat() will replace calls to rc_keydown(). Protocols that rely on rc_repeat() will continue to use rc_keydown(), so that the LIRC_SCANCODE_FLAG_REPEAT flag will only be set for auto-generated repeat events when a button is being held down, and not when the user is repeatedly pressing a button. TODO: Introduce bpf_rc_keydown_or_repeat() --- drivers/media/cec/platform/seco/seco-cec.c | 3 +- drivers/media/i2c/ir-kbd-i2c.c | 2 +- drivers/media/pci/bt8xx/bttv-input.c | 2 +- drivers/media/pci/ttpci/budget-ci.c | 7 ++-- drivers/media/rc/imon.c | 5 +-- drivers/media/rc/ir-rc5-decoder.c | 2 +- drivers/media/rc/ir-rc6-decoder.c | 2 +- drivers/media/rc/ir-rcmm-decoder.c | 4 ++- drivers/media/rc/rc-main.c | 40 ++++++++++++++++++--- drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- drivers/media/usb/dvb-usb-v2/dvbsky.c | 3 +- drivers/media/usb/dvb-usb/dib0700_core.c | 10 +++--- drivers/media/usb/dvb-usb/dib0700_devices.c | 7 ++-- drivers/media/usb/dvb-usb/ttusb2.c | 3 +- drivers/media/usb/em28xx/em28xx-input.c | 8 ++--- drivers/staging/media/av7110/av7110_ir.c | 2 +- include/media/rc-core.h | 2 ++ 17 files changed, 71 insertions(+), 33 deletions(-) diff --git a/drivers/media/cec/platform/seco/seco-cec.c b/drivers/media/cec/platform/seco/seco-cec.c index e1176f67f89a..c552303c77df 100644 --- a/drivers/media/cec/platform/seco/seco-cec.c +++ b/drivers/media/cec/platform/seco/seco-cec.c @@ -429,7 +429,8 @@ static int secocec_ir_rx(struct secocec_data *priv) addr = (val & SECOCEC_IR_ADDRESS_MASK) >> SECOCEC_IR_ADDRESS_SHL; toggle = (val & SECOCEC_IR_TOGGLE_MASK) >> SECOCEC_IR_TOGGLE_SHL; - rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle); + rc_keydown_or_repeat(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), + toggle); dev_dbg(dev, "IR key pressed: 0x%02x addr 0x%02x toggle 0x%02x", key, addr, toggle); diff --git a/drivers/media/i2c/ir-kbd-i2c.c b/drivers/media/i2c/ir-kbd-i2c.c index 56674173524f..e80a8bdf18e1 100644 --- a/drivers/media/i2c/ir-kbd-i2c.c +++ b/drivers/media/i2c/ir-kbd-i2c.c @@ -299,7 +299,7 @@ static int ir_key_poll(struct IR_i2c *ir) if (rc) { dev_dbg(&ir->rc->dev, "%s: proto = 0x%04x, scancode = 0x%08x\n", __func__, protocol, scancode); - rc_keydown(ir->rc, protocol, scancode, toggle); + rc_keydown_or_repeat(ir->rc, protocol, scancode, toggle); } return 0; } diff --git a/drivers/media/pci/bt8xx/bttv-input.c b/drivers/media/pci/bt8xx/bttv-input.c index 41226f1d0e5b..8b93424299b6 100644 --- a/drivers/media/pci/bt8xx/bttv-input.c +++ b/drivers/media/pci/bt8xx/bttv-input.c @@ -228,7 +228,7 @@ static void bttv_rc5_timer_end(struct timer_list *t) } scancode = RC_SCANCODE_RC5(system, command); - rc_keydown(ir->dev, RC_PROTO_RC5, scancode, toggle); + rc_keydown_or_repeat(ir->dev, RC_PROTO_RC5, scancode, toggle); dprintk("scancode %x, toggle %x\n", scancode, toggle); } diff --git a/drivers/media/pci/ttpci/budget-ci.c b/drivers/media/pci/ttpci/budget-ci.c index d59d18647371..20f974b462e8 100644 --- a/drivers/media/pci/ttpci/budget-ci.c +++ b/drivers/media/pci/ttpci/budget-ci.c @@ -145,9 +145,10 @@ static void msp430_ir_interrupt(struct tasklet_struct *t) return; if (budget_ci->ir.full_rc5) { - rc_keydown(dev, RC_PROTO_RC5, - RC_SCANCODE_RC5(budget_ci->ir.rc5_device, budget_ci->ir.ir_key), - !!(command & 0x20)); + rc_keydown_or_repeat(dev, RC_PROTO_RC5, + RC_SCANCODE_RC5(budget_ci->ir.rc5_device, + budget_ci->ir.ir_key), + !!(command & 0x20)); return; } diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 54da6f60079b..32371f81612c 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -1664,8 +1664,9 @@ static void imon_incoming_packet(struct imon_context *ictx, else return; - rc_keydown(ictx->rdev, proto, ictx->rc_scancode, - ictx->rc_toggle); + rc_keydown_or_repeat(ictx->rdev, proto, + ictx->rc_scancode, + ictx->rc_toggle); spin_lock_irqsave(&ictx->kc_lock, flags); ictx->last_keycode = ictx->kc; diff --git a/drivers/media/rc/ir-rc5-decoder.c b/drivers/media/rc/ir-rc5-decoder.c index d58b6226afeb..2594e3aba4d9 100644 --- a/drivers/media/rc/ir-rc5-decoder.c +++ b/drivers/media/rc/ir-rc5-decoder.c @@ -157,7 +157,7 @@ static int ir_rc5_decode(struct rc_dev *dev, struct ir_raw_event ev) dev_dbg(&dev->dev, "RC5(x/sz) scancode 0x%06x (p: %u, t: %u)\n", scancode, protocol, toggle); - rc_keydown(dev, protocol, scancode, toggle); + rc_keydown_or_repeat(dev, protocol, scancode, toggle); data->state = STATE_INACTIVE; return 0; } diff --git a/drivers/media/rc/ir-rc6-decoder.c b/drivers/media/rc/ir-rc6-decoder.c index 0657ad5eef48..1d8b9e42e1f5 100644 --- a/drivers/media/rc/ir-rc6-decoder.c +++ b/drivers/media/rc/ir-rc6-decoder.c @@ -263,7 +263,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev) goto out; } - rc_keydown(dev, protocol, scancode, toggle); + rc_keydown_or_repeat(dev, protocol, scancode, toggle); data->state = STATE_INACTIVE; return 0; } diff --git a/drivers/media/rc/ir-rcmm-decoder.c b/drivers/media/rc/ir-rcmm-decoder.c index fd9ec69a3718..4c32d5448df3 100644 --- a/drivers/media/rc/ir-rcmm-decoder.c +++ b/drivers/media/rc/ir-rcmm-decoder.c @@ -156,7 +156,9 @@ static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev) } if (dev->enabled_protocols & RC_PROTO_BIT_RCMM32) { - rc_keydown(dev, RC_PROTO_RCMM32, scancode, toggle); + rc_keydown_or_repeat(dev, RC_PROTO_RCMM32, + data->bits & ~0x8000, + !!(0x8000 & data->bits)); data->state = STATE_INACTIVE; return 0; } diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index f27f6b383816..301a3c2e9bd6 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -773,12 +773,13 @@ EXPORT_SYMBOL_GPL(rc_repeat); * @scancode: the scancode of the keypress * @keycode: the keycode of the keypress * @toggle: the toggle value of the keypress + * @do_repeat: whether to set the LIRC_SCANCODE_FLAG_REPEAT when needed * * This function is used internally to register a keypress, it must be * called with keylock held. */ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, - u64 scancode, u32 keycode, u8 toggle) + u64 scancode, u32 keycode, u8 toggle, bool do_repeat) { bool new_event = (!dev->keypressed || dev->last_protocol != protocol || @@ -786,7 +787,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, dev->last_toggle != toggle); struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0, + .flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) | + (do_repeat && !new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0), .keycode = keycode }; @@ -852,7 +854,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, u32 keycode = rc_g_keycode_from_table(dev, scancode); spin_lock_irqsave(&dev->keylock, flags); - ir_do_keydown(dev, protocol, scancode, keycode, toggle); + ir_do_keydown(dev, protocol, scancode, keycode, toggle, false); if (dev->keypressed) { dev->keyup_jiffies = jiffies + @@ -864,6 +866,36 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, } EXPORT_SYMBOL_GPL(rc_keydown); +/** + * rc_keydown_or_repeat() - generates input event for a key press + * @dev: the struct rc_dev descriptor of the device + * @protocol: the protocol for the keypress + * @scancode: the scancode for the keypress + * @toggle: the toggle value (protocol dependent, if the protocol doesn't + * support toggle values, this should be set to zero) + * + * This routine is used to signal that a key has been pressed or is + * being held down on the remote control. + */ +void rc_keydown_or_repeat(struct rc_dev *dev, enum rc_proto protocol, + u64 scancode, u8 toggle) +{ + unsigned long flags; + u32 keycode = rc_g_keycode_from_table(dev, scancode); + + spin_lock_irqsave(&dev->keylock, flags); + ir_do_keydown(dev, protocol, scancode, keycode, toggle, true); + + if (dev->keypressed) { + dev->keyup_jiffies = jiffies + + usecs_to_jiffies(dev->keyup_delay) + + msecs_to_jiffies(repeat_period(protocol)); + mod_timer(&dev->timer_keyup, dev->keyup_jiffies); + } + spin_unlock_irqrestore(&dev->keylock, flags); +} +EXPORT_SYMBOL_GPL(rc_keydown_or_repeat); + /** * rc_keydown_notimeout() - generates input event for a key press without * an automatic keyup event at a later time @@ -883,7 +915,7 @@ void rc_keydown_notimeout(struct rc_dev *dev, enum rc_proto protocol, u32 keycode = rc_g_keycode_from_table(dev, scancode); spin_lock_irqsave(&dev->keylock, flags); - ir_do_keydown(dev, protocol, scancode, keycode, toggle); + ir_do_keydown(dev, protocol, scancode, keycode, toggle, false); spin_unlock_irqrestore(&dev->keylock, flags); } EXPORT_SYMBOL_GPL(rc_keydown_notimeout); diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c index 62ee09f28a0b..7ff10eb7d5b2 100644 --- a/drivers/media/usb/dvb-usb-v2/az6007.c +++ b/drivers/media/usb/dvb-usb-v2/az6007.c @@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) proto = RC_PROTO_NEC32; } - rc_keydown(d->rc_dev, proto, code, st->data[5]); + rc_keydown_or_repeat(d->rc_dev, proto, code, st->data[5]); return 0; } diff --git a/drivers/media/usb/dvb-usb-v2/dvbsky.c b/drivers/media/usb/dvb-usb-v2/dvbsky.c index 1221c924312a..d8f398179760 100644 --- a/drivers/media/usb/dvb-usb-v2/dvbsky.c +++ b/drivers/media/usb/dvb-usb-v2/dvbsky.c @@ -192,7 +192,8 @@ static int dvbsky_rc_query(struct dvb_usb_device *d) rc5_system = (code & 0x7C0) >> 6; toggle = (code & 0x800) ? 1 : 0; scancode = rc5_system << 8 | rc5_command; - rc_keydown(d->rc_dev, RC_PROTO_RC5, scancode, toggle); + rc_keydown_or_repeat(d->rc_dev, RC_PROTO_RC5, scancode, + toggle); } return 0; } diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c index 1caabb51ea47..ff97106bb7eb 100644 --- a/drivers/media/usb/dvb-usb/dib0700_core.c +++ b/drivers/media/usb/dvb-usb/dib0700_core.c @@ -711,7 +711,6 @@ static void dib0700_rc_urb_completion(struct urb *purb) struct dib0700_rc_response *poll_reply; enum rc_proto protocol; u32 keycode; - u8 toggle; deb_info("%s()\n", __func__); if (d->rc_dev == NULL) { @@ -743,8 +742,6 @@ static void dib0700_rc_urb_completion(struct urb *purb) switch (d->props.rc.core.protocol) { case RC_PROTO_BIT_NEC: - toggle = 0; - /* NEC protocol sends repeat code as 0 0 0 FF */ if (poll_reply->nec.system == 0x00 && poll_reply->nec.not_system == 0x00 && @@ -780,7 +777,6 @@ static void dib0700_rc_urb_completion(struct urb *purb) default: deb_data("RC5 protocol\n"); protocol = RC_PROTO_RC5; - toggle = poll_reply->report_id; keycode = RC_SCANCODE_RC5(poll_reply->rc5.system, poll_reply->rc5.data); if ((poll_reply->rc5.data ^ poll_reply->rc5.not_data) != 0xff) { @@ -791,10 +787,12 @@ static void dib0700_rc_urb_completion(struct urb *purb) goto resubmit; } - break; + rc_keydown_or_repeat(d->rc_dev, protocol, keycode, + poll_reply->report_id); + goto resubmit; } - rc_keydown(d->rc_dev, protocol, keycode, toggle); + rc_keydown(d->rc_dev, protocol, keycode, 0); resubmit: /* Clean the buffer before we requeue */ diff --git a/drivers/media/usb/dvb-usb/dib0700_devices.c b/drivers/media/usb/dvb-usb/dib0700_devices.c index 710c1afe3e85..31126e69290f 100644 --- a/drivers/media/usb/dvb-usb/dib0700_devices.c +++ b/drivers/media/usb/dvb-usb/dib0700_devices.c @@ -517,7 +517,6 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) { enum rc_proto protocol; u32 scancode; - u8 toggle; int i; struct dib0700_state *st = d->priv; @@ -558,18 +557,18 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) protocol = RC_PROTO_NEC; scancode = RC_SCANCODE_NEC(st->buf[3 - 2], st->buf[3 - 3]); - toggle = 0; + rc_keydown(d->rc_dev, protocol, scancode, 0); break; default: /* RC-5 protocol changes toggle bit on new keypress */ protocol = RC_PROTO_RC5; scancode = RC_SCANCODE_RC5(st->buf[3 - 2], st->buf[3 - 3]); - toggle = st->buf[3 - 1]; + rc_keydown_or_repeat(d->rc_dev, protocol, scancode, + st->buf[3 - 1]); break; } - rc_keydown(d->rc_dev, protocol, scancode, toggle); return 0; } diff --git a/drivers/media/usb/dvb-usb/ttusb2.c b/drivers/media/usb/dvb-usb/ttusb2.c index 294274fd8f55..94b722b87a72 100644 --- a/drivers/media/usb/dvb-usb/ttusb2.c +++ b/drivers/media/usb/dvb-usb/ttusb2.c @@ -456,7 +456,8 @@ static int tt3650_rc_query(struct dvb_usb_device *d) /* got a "press" event */ st->last_rc_key = RC_SCANCODE_RC5(rx[3], rx[2]); deb_info("%s: cmd=0x%02x sys=0x%02x\n", __func__, rx[2], rx[3]); - rc_keydown(d->rc_dev, RC_PROTO_RC5, st->last_rc_key, rx[1]); + rc_keydown_or_repeat(d->rc_dev, RC_PROTO_RC5, st->last_rc_key, + rx[1]); } else if (st->last_rc_key) { rc_keyup(d->rc_dev); st->last_rc_key = 0; diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c index 0b6d77c3bec8..7034707d5138 100644 --- a/drivers/media/usb/em28xx/em28xx-input.c +++ b/drivers/media/usb/em28xx/em28xx-input.c @@ -337,10 +337,10 @@ static void em28xx_ir_handle_key(struct em28xx_IR *ir) poll_result.toggle_bit, poll_result.read_count, poll_result.scancode); if (ir->full_code) - rc_keydown(ir->rc, - poll_result.protocol, - poll_result.scancode, - poll_result.toggle_bit); + rc_keydown_or_repeat(ir->rc, + poll_result.protocol, + poll_result.scancode, + poll_result.toggle_bit); else rc_keydown(ir->rc, RC_PROTO_UNKNOWN, diff --git a/drivers/staging/media/av7110/av7110_ir.c b/drivers/staging/media/av7110/av7110_ir.c index a851ba328e4a..6691e4322654 100644 --- a/drivers/staging/media/av7110/av7110_ir.c +++ b/drivers/staging/media/av7110/av7110_ir.c @@ -64,7 +64,7 @@ void av7110_ir_handler(struct av7110 *av7110, u32 ircom) return; } - rc_keydown(rcdev, proto, scancode, toggle != 0); + rc_keydown_or_repeat(rcdev, proto, scancode, !!toggle); } } diff --git a/include/media/rc-core.h b/include/media/rc-core.h index b688304959bc..4f49b5a9b1c6 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -287,6 +287,8 @@ void rc_unregister_device(struct rc_dev *dev); void rc_repeat(struct rc_dev *dev); void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, u8 toggle); +void rc_keydown_or_repeat(struct rc_dev *dev, enum rc_proto protocol, + u64 scancode, u8 toggle); void rc_keydown_notimeout(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, u8 toggle); void rc_keyup(struct rc_dev *dev); -- 2.36.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-04 9:20 ` Marko Mäkelä @ 2022-07-04 10:00 ` Sean Young 2022-07-04 19:04 ` Marko Mäkelä 0 siblings, 1 reply; 17+ messages in thread From: Sean Young @ 2022-07-04 10:00 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media Hi Marko, On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote: > Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: > > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: > > > For protocols that do not use a toggle bit, the last parameter of > > > rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() > > > will be issued when needed. For those protocols, I thought that we would not > > > want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any > > > circumstances. > > > > Toggle and repeat are distinct concepts. > > > > rc_repeat() is for protocols which have a special repeat message, which > > carry no information other that "repeat the last message". However, > > all protocols repeat. Whether they use a special repeat message or not. > > > > It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT > > is set. > > Is it right to set the flag when a message is being repeated due to user > effort (repeatedly pressing and releasing a button, instead of holding the > button down)? The problem here is that the nec repeat is used by some remotes, but not others. Some nec remotes repeat the entire code every time. Our generic nec decoder cannot distinguish between the two. So, our nec decoder interprets both a nec repeat and a repeated code as "button being held down". rc5 is a much nicer protocol and explicitly uses a toggle bit to specify the button has been released/pressed. Some protocols use more than one bit for toggle, in case a toggle was lost due to packet loss. > If it is, is it consistent to avoid setting the flag when a > protocol uses a toggle bit (say, RC5)? No, RC5 repeats the same message if a button is being held down with the same toggle value. We should get a LIRC_SCANCODE_FLAG_REPEAT in this case. > In that case, the toggle bit would > change its value each time the button is pressed, and your suggested change > to rc_keydown() would not set the repeat flag. If we can distinguish between press/release vs hold then it is not a repeat. If it is being held down. then it is a repeat. > As far as I understand, the change that you suggested would set the > LIRC_SCANCODE_FLAG_REPEAT if I repeatedly press a button on the NEC protocol > remote control, but not on an RC5 remote control. RC5 too. > I tested the attached patch (which was created on 5.19-rc5, which failed to > boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes > to rtl28xxu and rc-core. You'll need to fix this. > One might think that it is not necessary to make difference between long > button presses (which should generate repeat events) and short button > presses that are quickly repeated by the user. I can think of a user-space > application that would intentionally ignore repeat events for some buttons > where it would make little sense. For example, when the number button 1 is > pressed for a long time, the application might choose not to repeat the > keypress, but "demand" multiple separate button presses by the user, if the > channel should really be switched to 11, 111, or 1111. The intention of > ignoring "repeat" events would be to avoid "punishing" users who are > pressing a button longer, possibly compensating for unreliable IR signal > reception. The input layer create autorepeat key events for keys that are being held down. > If the user wants to quickly switch to channel 111 by quickly pressing the > button three times, it should not be misreported as an auto-repeated event, > but reported as 3 LIRC events without the "repeat" flag, and as 3 pairs of > keydown and keyup events. Ideally yes, if we can distinguish between the two. FWIW I'm (slowly) working on new tooling that allows you specify the IR protocol in IRP format. This would allow you say for NEC: {38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m)* [D:0..255,S:0..255=255-D,F:0..255] For remotes that repeat the entire code each time, and {38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m,(16,-4,1,^108m)*) [D:0..255,S:0..255=255-D,F:0..255]]] For remotes that send nec repeats. This would be compiled down BPF. I'm still working on the decoder and I haven't started on the BPF compilation side yet (the encoder is fully working). See https://github.com/seanyoung/cir/ > On the other hand, there should be no reason for an application to not honor > repeat events for a volume button. That is of course up to the application > to decide, not the kernel. Well, that's not the way things work. Keys have autorepeats which are generated kernel-side. I think libinput wants to change this to user space but certainly not application side. > If you agree that this patch is on the right track, an interface for the new > function rc_keydown_or_repeat() may have to be exposed to the BPF interface > as well. I'm not sure why that is needed. Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-04 10:00 ` Sean Young @ 2022-07-04 19:04 ` Marko Mäkelä 2022-07-05 7:25 ` Sean Young 0 siblings, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-07-04 19:04 UTC (permalink / raw) To: Sean Young; +Cc: linux-media [-- Attachment #1: Type: text/plain, Size: 4931 bytes --] Hi Sean, Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: >Hi Marko, > >On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote: >> Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: >> > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: >> > > For protocols that do not use a toggle bit, the last parameter of >> > > rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() >> > > will be issued when needed. For those protocols, I thought that we would not >> > > want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any >> > > circumstances. >> > >> > Toggle and repeat are distinct concepts. >> > >> > rc_repeat() is for protocols which have a special repeat message, which >> > carry no information other that "repeat the last message". However, >> > all protocols repeat. Whether they use a special repeat message or not. >> > >> > It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT >> > is set. >> >> Is it right to set the flag when a message is being repeated due to user >> effort (repeatedly pressing and releasing a button, instead of holding the >> button down)? > >The problem here is that the nec repeat is used by some remotes, but >not others. Some nec remotes repeat the entire code every time. Our >generic nec decoder cannot distinguish between the two. So, our nec >decoder interprets both a nec repeat and a repeated code as "button >being held down". I see. My experience of remote control protocols is mostly limited to RC5. >> As far as I understand, the change that you suggested would set the >> LIRC_SCANCODE_FLAG_REPEAT if I repeatedly press a button on the NEC protocol >> remote control, but not on an RC5 remote control. > >RC5 too. I think that it is depends on the implementation of the firmware that runs on the remote control unit. If the button is released and quickly pressed again so that no keyboard matrix scan was scheduled to run in between, then the firmware could report it as a repeat event. Alternatively, every second IR message could be lost, so that the receiver would observe the same toggle bit value for every IR message that it receives. >>I tested the attached patch (which was created on 5.19-rc5, which >>failed to boot on my system for unrelated reasons) on Linux 5.17, on >>top of your fixes to rtl28xxu and rc-core. > >You'll need to fix this. The 5.19-rc5 boot failure could have been related to LUKS setup on that machine, because a kernel panic message was displayed before I was being prompted for an encryption key. The modules would not have been loaded at that point, so I do not think that it is related to my modifications. When compiled for the v5.17 kernel release tag on another computer, the patch that implements rc_keydown_or_repeat() worked for me. It does not look like there are many changes in drivers/media/rc between 5.17 and 5.19. >>If the user wants to quickly switch to channel 111 by quickly pressing >>the button three times, it should not be misreported as an >>auto-repeated event, but reported as 3 LIRC events without the >>"repeat" flag, and as 3 pairs of keydown and keyup events. > >Ideally yes, if we can distinguish between the two. Okay, I understand that we indeed cannot always do that. >See https://github.com/seanyoung/cir/ This could open up many possibilities. Would the decoded events also be available via some low-level interface to user-space programs, in addition to the input event driver? >>On the other hand, there should be no reason for an application to not >>honor repeat events for a volume button. That is of course up to the >>application to decide, not the kernel. > >Well, that's not the way things work. Keys have autorepeats which are >generated kernel-side. I think libinput wants to change this to user >space but certainly not application side. It is how https://tvdr.de works. I have been using it via two interfaces: a built-in LIRC interface, and vdr-plugin-remote for the Linux input device driver. In http://git.tvdr.de/vdr.git you can find that in some cases, normal key-presses are being distinguished from repeat events (git grep -w k_Repeat). This is the reason why I brought up the missing LIRC_SCANCODE_FLAG_REPEAT in the RC5 protocol decoder: VDR with the LIRC interface would observe that the user is repeatedly pressing a button even when the button is being held down. >> If you agree that this patch is on the right track, an interface for the new >> function rc_keydown_or_repeat() may have to be exposed to the BPF interface >> as well. > >I'm not sure why that is needed. I am attaching a minimal version of the patch, just one hunk, like you suggested earlier. I did not observe any bad effects with either remote control unit I tested it with: RC5 and NEC, again, on the rtl28xxu and with your two commits, on the v5.17 tag. Best regards, Marko [-- Attachment #2: 0001-media-rc-Always-report-LIRC-repeat-flag.patch --] [-- Type: text/x-diff, Size: 1064 bytes --] From 7bdda9eccd704076297a0d0802c8638b4562c703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <marko.makela@mariadb.com> Date: Mon, 4 Jul 2022 21:56:08 +0300 Subject: [PATCH] media: rc: Always report LIRC repeat flag The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown(). Previously it was only set by rc_repeat(), but not all protocol decoders invoke that function. --- drivers/media/rc/rc-main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index f27f6b383816..d875d84ea221 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, dev->last_toggle != toggle); struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0, + .flags = (toggle ? LIRC_SCANCODE_FLAG_TOGGLE : 0) | + (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0), .keycode = keycode }; -- 2.36.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-04 19:04 ` Marko Mäkelä @ 2022-07-05 7:25 ` Sean Young 2022-07-05 8:48 ` Marko Mäkelä 0 siblings, 1 reply; 17+ messages in thread From: Sean Young @ 2022-07-05 7:25 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media Hi Marko, On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: > Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: > > On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote: > > > Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: > > > > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: > > > I tested the attached patch (which was created on 5.19-rc5, which > > > failed to boot on my system for unrelated reasons) on Linux 5.17, on > > > top of your fixes to rtl28xxu and rc-core. > > > > You'll need to fix this. > > The 5.19-rc5 boot failure could have been related to LUKS setup on that > machine, because a kernel panic message was displayed before I was being > prompted for an encryption key. The modules would not have been loaded at > that point, so I do not think that it is related to my modifications. > > When compiled for the v5.17 kernel release tag on another computer, the > patch that implements rc_keydown_or_repeat() worked for me. > > It does not look like there are many changes in drivers/media/rc between > 5.17 and 5.19. Your patch needs a `Signed-off-by` and it should not be attached, it should be inline in your email. https://www.kernel.org/doc/html/latest/process/submitting-patches.html#no-mime-no-links-no-compression-no-attachments-just-plain-text > > See https://github.com/seanyoung/cir/ > > This could open up many possibilities. Would the decoded events also be > available via some low-level interface to user-space programs, in addition > to the input event driver? The plan was for it to run once, generate an eBPF program, attach that an exit. The eBPF program sends the decoded stuff to the lirc chardev in this struct: https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. Will that be sufficient for your needs? Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-05 7:25 ` Sean Young @ 2022-07-05 8:48 ` Marko Mäkelä 2022-07-05 9:26 ` Sean Young 0 siblings, 1 reply; 17+ messages in thread From: Marko Mäkelä @ 2022-07-05 8:48 UTC (permalink / raw) To: Sean Young; +Cc: linux-media Hi Sean, Tue, Jul 05, 2022 at 08:25:48AM +0100, Sean Young wrote: >On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: >> Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: >> > On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote: >> > > Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: >> > > > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: >> > > I tested the attached patch (which was created on 5.19-rc5, which >> > > failed to boot on my system for unrelated reasons) on Linux 5.17, on >> > > top of your fixes to rtl28xxu and rc-core. >> > >> > You'll need to fix this. >> >> The 5.19-rc5 boot failure could have been related to LUKS setup on that >> machine, because a kernel panic message was displayed before I was being >> prompted for an encryption key. The modules would not have been loaded at >> that point, so I do not think that it is related to my modifications. >> >> When compiled for the v5.17 kernel release tag on another computer, the >> patch that implements rc_keydown_or_repeat() worked for me. >> >> It does not look like there are many changes in drivers/media/rc between >> 5.17 and 5.19. > >Your patch needs a `Signed-off-by` and it should not be attached, it should >be inline in your email. Thank you for your patience. I hope that I got it right. It would be my very first patch submission to the Linux kernel. I did not see it appear on this list archive yet. You are Cc'd. >> > See https://github.com/seanyoung/cir/ >> >> This could open up many possibilities. Would the decoded events also be >> available via some low-level interface to user-space programs, in addition >> to the input event driver? > >The plan was for it to run once, generate an eBPF program, attach that an >exit. The eBPF program sends the decoded stuff to the lirc chardev in >this struct: > >https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode > >This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. > >Will that be sufficient for your needs? I think that it should cover the most common types of remote control units. I can name an example of a complex IR remote control, which I think would be challenging to repurpose for controlling anything else than the original type of device. But, I would think that something Bluetooth or WLAN based on a touchscreen device will replace IR in such applications. The remote control of my air conditioner presents all settings on a local LCD. On every change, maybe after a short timeout of inactivity, it will send a long IR message with all the settings. The 32 bits of keycode or 64 bits of scancode would not be sufficient for that. Marko ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver 2022-07-05 8:48 ` Marko Mäkelä @ 2022-07-05 9:26 ` Sean Young 0 siblings, 0 replies; 17+ messages in thread From: Sean Young @ 2022-07-05 9:26 UTC (permalink / raw) To: Marko Mäkelä; +Cc: linux-media Hi Marko, On Tue, Jul 05, 2022 at 11:48:50AM +0300, Marko Mäkelä wrote: > Tue, Jul 05, 2022 at 08:25:48AM +0100, Sean Young wrote: > > On Mon, Jul 04, 2022 at 10:04:38PM +0300, Marko Mäkelä wrote: > > > Mon, Jul 04, 2022 at 11:00:38AM +0100, Sean Young wrote: > > > > On Mon, Jul 04, 2022 at 12:20:01PM +0300, Marko Mäkelä wrote: > > > > > Mon, Jul 04, 2022 at 08:21:39AM +0100, Sean Young wrote: > > > > > > On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote: > > > > > I tested the attached patch (which was created on 5.19-rc5, which > > > > > failed to boot on my system for unrelated reasons) on Linux 5.17, on > > > > > top of your fixes to rtl28xxu and rc-core. > > > > > > > > You'll need to fix this. > > > > > > The 5.19-rc5 boot failure could have been related to LUKS setup on that > > > machine, because a kernel panic message was displayed before I was being > > > prompted for an encryption key. The modules would not have been loaded at > > > that point, so I do not think that it is related to my modifications. > > > > > > When compiled for the v5.17 kernel release tag on another computer, the > > > patch that implements rc_keydown_or_repeat() worked for me. > > > > > > It does not look like there are many changes in drivers/media/rc between > > > 5.17 and 5.19. > > > > Your patch needs a `Signed-off-by` and it should not be attached, it should > > be inline in your email. > > Thank you for your patience. I hope that I got it right. It would be my very > first patch submission to the Linux kernel. I did not see it appear on this > list archive yet. You are Cc'd. Thanks, I'll have a look. > > > > See https://github.com/seanyoung/cir/ > > > > > > This could open up many possibilities. Would the decoded events also be > > > available via some low-level interface to user-space programs, in addition > > > to the input event driver? > > > > The plan was for it to run once, generate an eBPF program, attach that an > > exit. The eBPF program sends the decoded stuff to the lirc chardev in > > this struct: > > > > https://www.kernel.org/doc/html/latest/userspace-api/media/rc/lirc-dev-intro.html#data-types-used-by-lirc-mode-scancode > > > > This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT. > > > > Will that be sufficient for your needs? > > I think that it should cover the most common types of remote control units. That is the hope. > I can name an example of a complex IR remote control, which I think would be > challenging to repurpose for controlling anything else than the original > type of device. But, I would think that something Bluetooth or WLAN based on > a touchscreen device will replace IR in such applications. The remote > control of my air conditioner presents all settings on a local LCD. On every > change, maybe after a short timeout of inactivity, it will send a long IR > message with all the settings. The 32 bits of keycode or 64 bits of scancode > would not be sufficient for that. There certainly can be an eBPF decoder for this type of IR, but I'm not sure what the use case is, because it's not key information. Maybe there is something else in eBPF which is more suitable. BTW I'm sure it's possible to control an air conditioner with the cir tool above, using the IRP language. Would be great to see something like that. Transmission is already working, so just requires reverse engineering the protocol and writing some IRP for it. :) Sean ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-05 9:27 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-02-12 16:32 [PATCH 0/2] Fix rtl28xxu nec/rc5 receiver Sean Young 2022-02-12 16:32 ` [PATCH 1/2] media: rc-core: split IR timeout into rawir timeout and keyup delay Sean Young 2022-02-12 16:32 ` [PATCH 2/2] media: rtl28xxu: improve IR receiver Sean Young 2022-06-26 12:33 ` Marko Mäkelä 2022-06-27 5:00 ` Marko Mäkelä 2022-07-02 8:17 ` Sean Young 2022-06-27 10:53 ` Sean Young 2022-06-28 6:27 ` Marko Mäkelä 2022-07-02 8:14 ` Sean Young 2022-07-03 17:02 ` Marko Mäkelä 2022-07-04 7:21 ` Sean Young 2022-07-04 9:20 ` Marko Mäkelä 2022-07-04 10:00 ` Sean Young 2022-07-04 19:04 ` Marko Mäkelä 2022-07-05 7:25 ` Sean Young 2022-07-05 8:48 ` Marko Mäkelä 2022-07-05 9:26 ` Sean Young
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).