| Message ID | 1393629426-31341-2-git-send-email-james.hogan@imgtec.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 28, 2014 at 11:17:02PM +0000, James Hogan wrote: >Add generic scancode filtering of RC input events, and fall back to >permitting any RC_FILTER_NORMAL scancode filter to be set if no s_filter >callback exists. This allows raw IR decoder events to be filtered, and >potentially allows hardware decoders to set looser filters and rely on >generic code to filter out the corner cases. Hi James, What's the purpose of providing the sw scancode filtering in the case where there's no hardware filtering support at all? (sorry that I'm replying so late...busy schedule :)) > >Signed-off-by: James Hogan <james.hogan@imgtec.com> >Cc: Mauro Carvalho Chehab <m.chehab@samsung.com> >Cc: Antti Seppälä <a.seppala@gmail.com> >Cc: linux-media@vger.kernel.org >--- > drivers/media/rc/rc-main.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > >diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c >index 6448128..0a4f680 100644 >--- a/drivers/media/rc/rc-main.c >+++ b/drivers/media/rc/rc-main.c >@@ -633,6 +633,7 @@ EXPORT_SYMBOL_GPL(rc_repeat); > static void ir_do_keydown(struct rc_dev *dev, int scancode, > u32 keycode, u8 toggle) > { >+ struct rc_scancode_filter *filter; > bool new_event = !dev->keypressed || > dev->last_scancode != scancode || > dev->last_toggle != toggle; >@@ -640,6 +641,11 @@ static void ir_do_keydown(struct rc_dev *dev, int scancode, > if (new_event && dev->keypressed) > ir_do_keyup(dev, false); > >+ /* Generic scancode filtering */ >+ filter = &dev->scancode_filters[RC_FILTER_NORMAL]; >+ if (filter->mask && ((scancode ^ filter->data) & filter->mask)) >+ return; >+ > input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode); > > if (new_event && keycode != KEY_RESERVED) { >@@ -1019,9 +1025,7 @@ static ssize_t show_filter(struct device *device, > return -EINVAL; > > mutex_lock(&dev->lock); >- if (!dev->s_filter) >- val = 0; >- else if (fattr->mask) >+ if (fattr->mask) > val = dev->scancode_filters[fattr->type].mask; > else > val = dev->scancode_filters[fattr->type].data; >@@ -1069,7 +1073,7 @@ static ssize_t store_filter(struct device *device, > return ret; > > /* Scancode filter not supported (but still accept 0) */ >- if (!dev->s_filter) >+ if (!dev->s_filter && fattr->type != RC_FILTER_NORMAL) > return val ? -EINVAL : count; > > mutex_lock(&dev->lock); >@@ -1081,9 +1085,11 @@ static ssize_t store_filter(struct device *device, > local_filter.mask = val; > else > local_filter.data = val; >- ret = dev->s_filter(dev, fattr->type, &local_filter); >- if (ret < 0) >- goto unlock; >+ if (dev->s_filter) { >+ ret = dev->s_filter(dev, fattr->type, &local_filter); >+ if (ret < 0) >+ goto unlock; >+ } > > /* Success, commit the new filter */ > *filter = local_filter; >-- >1.8.3.2 > >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Tuesday 25 March 2014 00:51:46 David Härdeman wrote: > On Fri, Feb 28, 2014 at 11:17:02PM +0000, James Hogan wrote: > >Add generic scancode filtering of RC input events, and fall back to > >permitting any RC_FILTER_NORMAL scancode filter to be set if no s_filter > >callback exists. This allows raw IR decoder events to be filtered, and > >potentially allows hardware decoders to set looser filters and rely on > >generic code to filter out the corner cases. > > Hi James, > > What's the purpose of providing the sw scancode filtering in the case where > there's no hardware filtering support at all? Consistency is probably the main reason, but I'll admit it's not perfectly consistent between generic/hardware filtering (mostly thanks to NEC scancode complexities), and I have no particular objection to dropping it if that isn't considered a good enough reason. Here's the original discussion: On Monday 10 February 2014 21:45:30 Antti Seppälä wrote: > On 10 February 2014 11:58, James Hogan <james.hogan@imgtec.com> wrote: > > On Saturday 08 February 2014 13:30:01 Antti Seppälä wrote: > > > Also adding the scancode filter to it would > > > demonstrate its usage. > > > > To actually add filtering support to loopback would require either: > > * raw-decoder/rc-core level scancode filtering for raw ir drivers > > * OR loopback driver to encode like nuvoton and fuzzy match the IR > > signals. > > Rc-core level scancode filtering shouldn't be too hard to do right? If > such would exist then it would provide a software fallback to other rc > devices where hardware filtering isn't available. I'd love to see the > sysfs filter and filter_mask files to have an effect on my nuvoton too > (sorry that I'm replying so late...busy schedule :)) No problem :) Cheers James > >Signed-off-by: James Hogan <james.hogan@imgtec.com> > >Cc: Mauro Carvalho Chehab <m.chehab@samsung.com> > >Cc: Antti Seppälä <a.seppala@gmail.com> > >Cc: linux-media@vger.kernel.org > >--- > > > > drivers/media/rc/rc-main.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > >diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c > >index 6448128..0a4f680 100644 > >--- a/drivers/media/rc/rc-main.c > >+++ b/drivers/media/rc/rc-main.c > >@@ -633,6 +633,7 @@ EXPORT_SYMBOL_GPL(rc_repeat); > > > > static void ir_do_keydown(struct rc_dev *dev, int scancode, > > > > u32 keycode, u8 toggle) > > > > { > > > >+ struct rc_scancode_filter *filter; > > > > bool new_event = !dev->keypressed || > > > > dev->last_scancode != scancode || > > dev->last_toggle != toggle; > > > >@@ -640,6 +641,11 @@ static void ir_do_keydown(struct rc_dev *dev, int > >scancode,> > > if (new_event && dev->keypressed) > > > > ir_do_keyup(dev, false); > > > >+ /* Generic scancode filtering */ > >+ filter = &dev->scancode_filters[RC_FILTER_NORMAL]; > >+ if (filter->mask && ((scancode ^ filter->data) & filter->mask)) > >+ return; > >+ > > > > input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode); > > > > if (new_event && keycode != KEY_RESERVED) { > > > >@@ -1019,9 +1025,7 @@ static ssize_t show_filter(struct device *device, > > > > return -EINVAL; > > > > mutex_lock(&dev->lock); > > > >- if (!dev->s_filter) > >- val = 0; > >- else if (fattr->mask) > >+ if (fattr->mask) > > > > val = dev->scancode_filters[fattr->type].mask; > > > > else > > > > val = dev->scancode_filters[fattr->type].data; > > > >@@ -1069,7 +1073,7 @@ static ssize_t store_filter(struct device *device, > > > > return ret; > > > > /* Scancode filter not supported (but still accept 0) */ > > > >- if (!dev->s_filter) > >+ if (!dev->s_filter && fattr->type != RC_FILTER_NORMAL) > > > > return val ? -EINVAL : count; > > > > mutex_lock(&dev->lock); > > > >@@ -1081,9 +1085,11 @@ static ssize_t store_filter(struct device *device, > > > > local_filter.mask = val; > > > > else > > > > local_filter.data = val; > > > >- ret = dev->s_filter(dev, fattr->type, &local_filter); > >- if (ret < 0) > >- goto unlock; > >+ if (dev->s_filter) { > >+ ret = dev->s_filter(dev, fattr->type, &local_filter); > >+ if (ret < 0) > >+ goto unlock; > >+ } > > > > /* Success, commit the new filter */ > > *filter = local_filter;
On Tue, Mar 25, 2014 at 09:12:11AM +0000, James Hogan wrote: >On Tuesday 25 March 2014 00:51:46 David Härdeman wrote: >> On Fri, Feb 28, 2014 at 11:17:02PM +0000, James Hogan wrote: >> >Add generic scancode filtering of RC input events, and fall back to >> >permitting any RC_FILTER_NORMAL scancode filter to be set if no s_filter >> >callback exists. This allows raw IR decoder events to be filtered, and >> >potentially allows hardware decoders to set looser filters and rely on >> >generic code to filter out the corner cases. >> >> Hi James, >> >> What's the purpose of providing the sw scancode filtering in the case where >> there's no hardware filtering support at all? > >Consistency is probably the main reason, but I'll admit it's not perfectly >consistent between generic/hardware filtering (mostly thanks to NEC scancode >complexities), and I have no particular objection to dropping it if that isn't >considered a good enough reason. I'm kind of sceptical...and given how difficult it is to remove functionality that is in a released kernel...I think that particular part (i.e. the software filtering) should be removed until it has had further discussion... >Here's the original discussion: >On Monday 10 February 2014 21:45:30 Antti Seppälä wrote: >> On 10 February 2014 11:58, James Hogan <james.hogan@imgtec.com> wrote: >> > On Saturday 08 February 2014 13:30:01 Antti Seppälä wrote: >> > > Also adding the scancode filter to it would >> > > demonstrate its usage. >> > >> > To actually add filtering support to loopback would require either: >> > * raw-decoder/rc-core level scancode filtering for raw ir drivers >> > * OR loopback driver to encode like nuvoton and fuzzy match the IR >> > signals. >> >> Rc-core level scancode filtering shouldn't be too hard to do right? If >> such would exist then it would provide a software fallback to other rc >> devices where hardware filtering isn't available. I'd love to see the >> sysfs filter and filter_mask files to have an effect on my nuvoton too I don't understand. What's the purpose of a "software fallback" for scancode filtering? Antti?
On 26 March 2014 01:21, David Härdeman <david@hardeman.nu> wrote: > On Tue, Mar 25, 2014 at 09:12:11AM +0000, James Hogan wrote: >>On Tuesday 25 March 2014 00:51:46 David Härdeman wrote: >>> On Fri, Feb 28, 2014 at 11:17:02PM +0000, James Hogan wrote: >>> >Add generic scancode filtering of RC input events, and fall back to >>> >permitting any RC_FILTER_NORMAL scancode filter to be set if no s_filter >>> >callback exists. This allows raw IR decoder events to be filtered, and >>> >potentially allows hardware decoders to set looser filters and rely on >>> >generic code to filter out the corner cases. >>> >>> Hi James, >>> >>> What's the purpose of providing the sw scancode filtering in the case where >>> there's no hardware filtering support at all? >> >>Consistency is probably the main reason, but I'll admit it's not perfectly >>consistent between generic/hardware filtering (mostly thanks to NEC scancode >>complexities), and I have no particular objection to dropping it if that isn't >>considered a good enough reason. > > I'm kind of sceptical...and given how difficult it is to remove > functionality that is in a released kernel...I think that particular > part (i.e. the software filtering) should be removed until it has had > further discussion... > >>Here's the original discussion: >>On Monday 10 February 2014 21:45:30 Antti Seppälä wrote: >>> On 10 February 2014 11:58, James Hogan <james.hogan@imgtec.com> wrote: >>> > On Saturday 08 February 2014 13:30:01 Antti Seppälä wrote: >>> > > Also adding the scancode filter to it would >>> > > demonstrate its usage. >>> > >>> > To actually add filtering support to loopback would require either: >>> > * raw-decoder/rc-core level scancode filtering for raw ir drivers >>> > * OR loopback driver to encode like nuvoton and fuzzy match the IR >>> > signals. >>> >>> Rc-core level scancode filtering shouldn't be too hard to do right? If >>> such would exist then it would provide a software fallback to other rc >>> devices where hardware filtering isn't available. I'd love to see the >>> sysfs filter and filter_mask files to have an effect on my nuvoton too > > I don't understand. What's the purpose of a "software fallback" for > scancode filtering? Antti? > Well since the ImgTec patches will create a new sysfs interface for the HW scancode filtering I figured that it would be nice for it to also function on devices which lack the hardware filtering capabilities. Especially since it's only three lines of code. :) Therefore I suggested the software fallback. At the time I had no clue that there might be added complexities with nec scancodes. So like James said it exists mainly for api consistency reasons. -Antti -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014-03-26 08:08, Antti Seppälä wrote: > On 26 March 2014 01:21, David Härdeman <david@hardeman.nu> wrote: >> On Tue, Mar 25, 2014 at 09:12:11AM +0000, James Hogan wrote: >>> On Tuesday 25 March 2014 00:51:46 David Härdeman wrote: >>>> What's the purpose of providing the sw scancode filtering in the >>>> case where >>>> there's no hardware filtering support at all? >>> >>> Consistency is probably the main reason, but I'll admit it's not >>> perfectly >>> consistent between generic/hardware filtering (mostly thanks to NEC >>> scancode >>> complexities), and I have no particular objection to dropping it if >>> that isn't >>> considered a good enough reason. >> >> I'm kind of sceptical...and given how difficult it is to remove >> functionality that is in a released kernel...I think that particular >> part (i.e. the software filtering) should be removed until it has had >> further discussion... ... >> I don't understand. What's the purpose of a "software fallback" for >> scancode filtering? Antti? >> > > Well since the ImgTec patches will create a new sysfs interface for > the HW scancode filtering I figured that it would be nice for it to > also function on devices which lack the hardware filtering > capabilities. Especially since it's only three lines of code. :) > > Therefore I suggested the software fallback. At the time I had no clue > that there might be added complexities with nec scancodes. It's not only NEC scancodes, the sw scancode filter is state that is changeable from user-space and which will require reader/writer synchronization during the RX path (which is the "hottest" path in rc-core). I've posted patches before which make the RX path lockless, this change makes complicates such changes. Additionally, the provision of the sw fallback means that userspace has no idea if there is an actual hardware filter present or not, meaning that a userspace program that is aware of the scancode filter will always enable it. So, I still think the SW part should be reverted, at least for now (i.e. the sysfs file should only be present if there is hardware support). Mauro? //David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26/03/14 13:44, David Härdeman wrote: > On 2014-03-26 08:08, Antti Seppälä wrote: >> On 26 March 2014 01:21, David Härdeman <david@hardeman.nu> wrote: >>> On Tue, Mar 25, 2014 at 09:12:11AM +0000, James Hogan wrote: >>>> On Tuesday 25 March 2014 00:51:46 David Härdeman wrote: >>>>> What's the purpose of providing the sw scancode filtering in the >>>>> case where >>>>> there's no hardware filtering support at all? >>>> >>>> Consistency is probably the main reason, but I'll admit it's not >>>> perfectly >>>> consistent between generic/hardware filtering (mostly thanks to NEC >>>> scancode >>>> complexities), and I have no particular objection to dropping it if >>>> that isn't >>>> considered a good enough reason. >>> >>> I'm kind of sceptical...and given how difficult it is to remove >>> functionality that is in a released kernel...I think that particular >>> part (i.e. the software filtering) should be removed until it has had >>> further discussion... > ... >>> I don't understand. What's the purpose of a "software fallback" for >>> scancode filtering? Antti? >>> >> >> Well since the ImgTec patches will create a new sysfs interface for >> the HW scancode filtering I figured that it would be nice for it to >> also function on devices which lack the hardware filtering >> capabilities. Especially since it's only three lines of code. :) >> >> Therefore I suggested the software fallback. At the time I had no clue >> that there might be added complexities with nec scancodes. > > It's not only NEC scancodes, the sw scancode filter is state that is > changeable from user-space and which will require reader/writer > synchronization during the RX path (which is the "hottest" path in > rc-core). I've posted patches before which make the RX path lockless, > this change makes complicates such changes. > > Additionally, the provision of the sw fallback means that userspace has > no idea if there is an actual hardware filter present or not, meaning > that a userspace program that is aware of the scancode filter will > always enable it. > > So, I still think the SW part should be reverted, at least for now (i.e. > the sysfs file should only be present if there is hardware support). I've prepared a revert patch (which also reverts a part of a later patch which takes SW filtering into account when updating the filter on a protocol change). I'll double check it works right later and submit. Note that this still leaves the sysfs nodes there though, but if !s_filter then they read as 0 and only accept the value 0 to be written (mask == 0 represents no filtering). Cheers James
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c index 6448128..0a4f680 100644 --- a/drivers/media/rc/rc-main.c +++ b/drivers/media/rc/rc-main.c @@ -633,6 +633,7 @@ EXPORT_SYMBOL_GPL(rc_repeat); static void ir_do_keydown(struct rc_dev *dev, int scancode, u32 keycode, u8 toggle) { + struct rc_scancode_filter *filter; bool new_event = !dev->keypressed || dev->last_scancode != scancode || dev->last_toggle != toggle; @@ -640,6 +641,11 @@ static void ir_do_keydown(struct rc_dev *dev, int scancode, if (new_event && dev->keypressed) ir_do_keyup(dev, false); + /* Generic scancode filtering */ + filter = &dev->scancode_filters[RC_FILTER_NORMAL]; + if (filter->mask && ((scancode ^ filter->data) & filter->mask)) + return; + input_event(dev->input_dev, EV_MSC, MSC_SCAN, scancode); if (new_event && keycode != KEY_RESERVED) { @@ -1019,9 +1025,7 @@ static ssize_t show_filter(struct device *device, return -EINVAL; mutex_lock(&dev->lock); - if (!dev->s_filter) - val = 0; - else if (fattr->mask) + if (fattr->mask) val = dev->scancode_filters[fattr->type].mask; else val = dev->scancode_filters[fattr->type].data; @@ -1069,7 +1073,7 @@ static ssize_t store_filter(struct device *device, return ret; /* Scancode filter not supported (but still accept 0) */ - if (!dev->s_filter) + if (!dev->s_filter && fattr->type != RC_FILTER_NORMAL) return val ? -EINVAL : count; mutex_lock(&dev->lock); @@ -1081,9 +1085,11 @@ static ssize_t store_filter(struct device *device, local_filter.mask = val; else local_filter.data = val; - ret = dev->s_filter(dev, fattr->type, &local_filter); - if (ret < 0) - goto unlock; + if (dev->s_filter) { + ret = dev->s_filter(dev, fattr->type, &local_filter); + if (ret < 0) + goto unlock; + } /* Success, commit the new filter */ *filter = local_filter;
Add generic scancode filtering of RC input events, and fall back to permitting any RC_FILTER_NORMAL scancode filter to be set if no s_filter callback exists. This allows raw IR decoder events to be filtered, and potentially allows hardware decoders to set looser filters and rely on generic code to filter out the corner cases. Signed-off-by: James Hogan <james.hogan@imgtec.com> Cc: Mauro Carvalho Chehab <m.chehab@samsung.com> Cc: Antti Seppälä <a.seppala@gmail.com> Cc: linux-media@vger.kernel.org --- drivers/media/rc/rc-main.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)