Hans, Thanks for doing the 64-bit test. I will incorporate the below comments along with the other comments and will post v3 of the patch with you included in the sign off. Regards, Murali > >I did some quick 64-bit tests and discovered that we need to add the packed >attribute to struct v4l2_bt_timings and struct v4l2_dv_timings in order to >prevent nasty 32-bit to 64-bit conversions in v4l2-compat-ioctl32.c. > >See below: > >> >> +/* >> >> + * D V B T T I M I N G S >> >> + */ >> >> + >> >> +/* BT.656/BT.1120 timing data */ >> >> +struct v4l2_bt_timings { >> >> + __u32 width; /* width in pixels */ >> >> + __u32 height; /* height in lines */ >> >> + __u32 interlaced; /* Interlaced or progressive */ >> >> + __u32 polarities; /* Positive or negative polarity */ >> >> + __u64 pixelclock; /* Pixel clock in HZ. Ex. 74.25MHz- >>74250000 */ >> >> + __u32 hfrontporch; /* Horizpontal front porch in pixels */ >> >> + __u32 hsync; /* Horizontal Sync length in pixels */ >> >> + __u32 hbackporch; /* Horizontal back porch in pixels */ >> >> + __u32 vfrontporch; /* Vertical front porch in pixels */ >> >> + __u32 vsync; /* Vertical Sync length in lines */ >> >> + __u32 vbackporch; /* Vertical back porch in lines */ >> >> + __u32 il_vfrontporch; /* Vertical front porch for bottom field >of >> >> + * interlaced field formats >> >> + */ >> >> + __u32 il_vsync; /* Vertical sync length for bottom field >of >> >> + * interlaced field formats >> >> + */ >> >> + __u32 il_vbackporch; /* Vertical back porch for bottom field >of >> >> + * interlaced field formats >> >> + */ >> >> + __u32 reserved[16]; >> >> +}; > >End with: > >} __attribute__ ((packed)); > >> >> + >> >> +/* Interlaced or progressive format */ >> >> +#define V4L2_DV_PROGRESSIVE 0 >> >> +#define V4L2_DV_INTERLACED 1 >> >> + >> >> +/* Polarities. If bit is not set, it is assumed to be negative >polarity >> >*/ >> >> +#define V4L2_DV_VSYNC_POS_POL 0x00000001 >> >> +#define V4L2_DV_HSYNC_POS_POL 0x00000002 >> >> + >> >> +/* BT.656/1120 timing type */ >> >> +enum v4l2_dv_timings_type { >> >> + V4L2_DV_BT_656_1120, >> >> +}; >> > >> >I forgot something: we shouldn't use enums as that can give problems on >> >some >> >architectures (ARM being one of them, I believe). So this should become >a >> >define and the type field below a __u32. >> > >> >> + >> >> +/* DV timings */ >> >> +struct v4l2_dv_timings { >> >> + enum v4l2_dv_timings_type type; >> >> + union { >> >> + struct v4l2_bt_timings bt; >> >> + __u32 reserved[32]; >> >> + }; >> >> +}; > >Ditto. > >I also attached a small diff for the v4l2-apps/test/ioctl-test.c source >which >I used to test this. > >The patch is Signed-off-by: Hans Verkuil <hverkuil@xxxxxxxxx> > >You can include this when you post the v4l2-apps patches. > >Regards, > > Hans > >-- >Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html