| Message ID | 56E9CDAE.2040200@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 16, 2016 at 10:18:38PM +0100, Heiner Kallweit wrote: > struct ir_raw_event currently has a size of 12 bytes on most (all?) > architectures. This can be reduced to 8 bytes whilst maintaining > full backwards compatibility. > This saves 2KB in size of struct ir_raw_event_ctrl (as element > kfifo is reduced by 512 * 4 bytes) and it allows to copy the > full struct ir_raw_event with a single 64 bit operation. > > Successfully tested with the Nuvoton driver and successfully > compile-tested with the ene_ir driver (as it uses the carrier / > duty_cycle elements). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > include/media/rc-core.h | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) > > diff --git a/include/media/rc-core.h b/include/media/rc-core.h > index 0f77b3d..b8f27c9 100644 > --- a/include/media/rc-core.h > +++ b/include/media/rc-core.h > @@ -214,27 +214,17 @@ enum raw_event_type { > > struct ir_raw_event { > union { > - u32 duration; > - > - struct { > - u32 carrier; > - u8 duty_cycle; > - }; > + u32 duration; > + u32 carrier; > }; > - > - unsigned pulse:1; > - unsigned reset:1; > - unsigned timeout:1; > - unsigned carrier_report:1; > + u8 duty_cycle; Moving duty_cycle does indeed reduce the structure size from 12 to 8. > + u8 pulse:1; > + u8 reset:1; > + u8 timeout:1; > + u8 carrier_report:1; Why are you changing the type of the bitfields? > }; > > -#define DEFINE_IR_RAW_EVENT(event) \ > - struct ir_raw_event event = { \ > - { .duration = 0 } , \ > - .pulse = 0, \ > - .reset = 0, \ > - .timeout = 0, \ > - .carrier_report = 0 } > +#define DEFINE_IR_RAW_EVENT(event) struct ir_raw_event event = {} Seems fine. I've always kinda wondered why this macro is needed. Sean -- 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
Am 16.03.2016 um 23:28 schrieb Sean Young: > On Wed, Mar 16, 2016 at 10:18:38PM +0100, Heiner Kallweit wrote: >> struct ir_raw_event currently has a size of 12 bytes on most (all?) >> architectures. This can be reduced to 8 bytes whilst maintaining >> full backwards compatibility. >> This saves 2KB in size of struct ir_raw_event_ctrl (as element >> kfifo is reduced by 512 * 4 bytes) and it allows to copy the >> full struct ir_raw_event with a single 64 bit operation. >> >> Successfully tested with the Nuvoton driver and successfully >> compile-tested with the ene_ir driver (as it uses the carrier / >> duty_cycle elements). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> include/media/rc-core.h | 26 ++++++++------------------ >> 1 file changed, 8 insertions(+), 18 deletions(-) >> >> diff --git a/include/media/rc-core.h b/include/media/rc-core.h >> index 0f77b3d..b8f27c9 100644 >> --- a/include/media/rc-core.h >> +++ b/include/media/rc-core.h >> @@ -214,27 +214,17 @@ enum raw_event_type { >> >> struct ir_raw_event { >> union { >> - u32 duration; >> - >> - struct { >> - u32 carrier; >> - u8 duty_cycle; >> - }; >> + u32 duration; >> + u32 carrier; >> }; >> - >> - unsigned pulse:1; >> - unsigned reset:1; >> - unsigned timeout:1; >> - unsigned carrier_report:1; >> + u8 duty_cycle; > > Moving duty_cycle does indeed reduce the structure size from 12 to 8. > >> + u8 pulse:1; >> + u8 reset:1; >> + u8 timeout:1; >> + u8 carrier_report:1; > > Why are you changing the type of the bitfields? > I did this to make sure that the compiler uses one byte for the bit field. When testing gcc also used just one byte when keeping the original "unsigned" type for the bit field members. Therefore it wouldn't be strictly neeeded. But I'm not sure whether it's guaranteed that the compiler packs a bit field to the smallest possible data type and we can rely on it. AFAIK C99 is a little more specific about this implementation detail of bit fields but C89/C90 is used for kernel compilation. Heiner >> }; >> >> -#define DEFINE_IR_RAW_EVENT(event) \ >> - struct ir_raw_event event = { \ >> - { .duration = 0 } , \ >> - .pulse = 0, \ >> - .reset = 0, \ >> - .timeout = 0, \ >> - .carrier_report = 0 } >> +#define DEFINE_IR_RAW_EVENT(event) struct ir_raw_event event = {} > > Seems fine. I've always kinda wondered why this macro is needed. > > > Sean > -- 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 Thu, Mar 17, 2016 at 07:40:59AM +0100, Heiner Kallweit wrote: > Am 16.03.2016 um 23:28 schrieb Sean Young: > > On Wed, Mar 16, 2016 at 10:18:38PM +0100, Heiner Kallweit wrote: > >> + u8 pulse:1; > >> + u8 reset:1; > >> + u8 timeout:1; > >> + u8 carrier_report:1; > > > > Why are you changing the type of the bitfields? > > > I did this to make sure that the compiler uses one byte for > the bit field. When testing gcc also used just one byte when > keeping the original "unsigned" type for the bit field members. > Therefore it wouldn't be strictly neeeded. > > But I'm not sure whether it's guaranteed that the compiler packs a > bit field to the smallest possible data type and we can rely on it. > AFAIK C99 is a little more specific about this implementation detail of > bit fields but C89/C90 is used for kernel compilation. It might be worth reading about structure packing rules rather than guessing. Sean -- 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
Am 17.03.2016 um 11:53 schrieb Sean Young: > On Thu, Mar 17, 2016 at 07:40:59AM +0100, Heiner Kallweit wrote: >> Am 16.03.2016 um 23:28 schrieb Sean Young: >>> On Wed, Mar 16, 2016 at 10:18:38PM +0100, Heiner Kallweit wrote: >>>> + u8 pulse:1; >>>> + u8 reset:1; >>>> + u8 timeout:1; >>>> + u8 carrier_report:1; >>> >>> Why are you changing the type of the bitfields? >>> >> I did this to make sure that the compiler uses one byte for >> the bit field. When testing gcc also used just one byte when >> keeping the original "unsigned" type for the bit field members. >> Therefore it wouldn't be strictly neeeded. >> >> But I'm not sure whether it's guaranteed that the compiler packs a >> bit field to the smallest possible data type and we can rely on it. >> AFAIK C99 is a little more specific about this implementation detail of >> bit fields but C89/C90 is used for kernel compilation. > > It might be worth reading about structure packing rules rather than > guessing. > Whenever it became interesting when reading the statement was: unspecified / implementation-dependent. But at least C90 clearly states that only signed / unsigned int are acceptable for bit fields. Therefore leave the bit field as it is. I will provide a v2. Heiner > > Sean > -- 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
diff --git a/include/media/rc-core.h b/include/media/rc-core.h index 0f77b3d..b8f27c9 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -214,27 +214,17 @@ enum raw_event_type { struct ir_raw_event { union { - u32 duration; - - struct { - u32 carrier; - u8 duty_cycle; - }; + u32 duration; + u32 carrier; }; - - unsigned pulse:1; - unsigned reset:1; - unsigned timeout:1; - unsigned carrier_report:1; + u8 duty_cycle; + u8 pulse:1; + u8 reset:1; + u8 timeout:1; + u8 carrier_report:1; }; -#define DEFINE_IR_RAW_EVENT(event) \ - struct ir_raw_event event = { \ - { .duration = 0 } , \ - .pulse = 0, \ - .reset = 0, \ - .timeout = 0, \ - .carrier_report = 0 } +#define DEFINE_IR_RAW_EVENT(event) struct ir_raw_event event = {} static inline void init_ir_raw_event(struct ir_raw_event *ev) {
struct ir_raw_event currently has a size of 12 bytes on most (all?) architectures. This can be reduced to 8 bytes whilst maintaining full backwards compatibility. This saves 2KB in size of struct ir_raw_event_ctrl (as element kfifo is reduced by 512 * 4 bytes) and it allows to copy the full struct ir_raw_event with a single 64 bit operation. Successfully tested with the Nuvoton driver and successfully compile-tested with the ene_ir driver (as it uses the carrier / duty_cycle elements). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- include/media/rc-core.h | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)