| Message ID | 1450341626-6695-27-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Laurent, On 12/17/2015 09:40 AM, Laurent Pinchart wrote: > Let userspace specify a request ID when getting or setting formats. The > support is limited to the multi-planar API at the moment, extending it > to the single-planar API is possible if needed. > > From a userspace point of view the API change is also minimized and > doesn't require any new ioctl. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > include/uapi/linux/videodev2.h | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 5af1d2d87558..5b2a8bc80eb2 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1973,6 +1973,7 @@ struct v4l2_plane_pix_format { > * @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding > * @quantization: enum v4l2_quantization, colorspace quantization > * @xfer_func: enum v4l2_xfer_func, colorspace transfer function > + * @request: request ID > */ > struct v4l2_pix_format_mplane { > __u32 width; > @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane { > __u8 ycbcr_enc; > __u8 quantization; > __u8 xfer_func; > - __u8 reserved[7]; > + __u8 reserved[3]; > + __u32 request; I think I mentioned this before: I feel uncomfortable using 4 bytes of the reserved fields when the request ID is limited to 16 bits anyway. I would prefer a __u16 here. Also put the request field *before* the reserved array, not after. Regards, Hans > } __attribute__ ((packed)); > > /** > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thank you for the review. On Friday 18 December 2015 12:18:26 Hans Verkuil wrote: > On 12/17/2015 09:40 AM, Laurent Pinchart wrote: > > Let userspace specify a request ID when getting or setting formats. The > > support is limited to the multi-planar API at the moment, extending it > > to the single-planar API is possible if needed. > > > > From a userspace point of view the API change is also minimized and > > doesn't require any new ioctl. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > include/uapi/linux/videodev2.h | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/include/uapi/linux/videodev2.h > > b/include/uapi/linux/videodev2.h index 5af1d2d87558..5b2a8bc80eb2 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h > > @@ -1973,6 +1973,7 @@ struct v4l2_plane_pix_format { > > > > * @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding > > * @quantization: enum v4l2_quantization, colorspace quantization > > * @xfer_func: enum v4l2_xfer_func, colorspace transfer function > > > > + * @request: request ID > > > > */ > > > > struct v4l2_pix_format_mplane { > > > > __u32 width; > > > > @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane { > > > > __u8 ycbcr_enc; > > __u8 quantization; > > __u8 xfer_func; > > > > - __u8 reserved[7]; > > + __u8 reserved[3]; > > + __u32 request; > > I think I mentioned this before: I feel uncomfortable using 4 bytes of the > reserved fields when the request ID is limited to 16 bits anyway. I'm still unsure whether request IDs should be 16 or 32 bits long. If we go for 16 bits then I'll of course make this field a __u16. > I would prefer a __u16 here. Also put the request field *before* the > reserved array, not after. The reserved array isn't aligned to a 32 bit (or even 16 bit) boundary. I can put the request field before it, with a 8 bit hole before the field. > > } __attribute__ ((packed)); > > > > /**
Hi Laurent, On Fri, Dec 18, 2015 at 6:16 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane { >> > >> > __u8 ycbcr_enc; >> > __u8 quantization; >> > __u8 xfer_func; >> > >> > - __u8 reserved[7]; >> > + __u8 reserved[3]; >> > + __u32 request; >> >> I think I mentioned this before: I feel uncomfortable using 4 bytes of the >> reserved fields when the request ID is limited to 16 bits anyway. > > I'm still unsure whether request IDs should be 16 or 32 bits long. If we go > for 16 bits then I'll of course make this field a __u16. > >> I would prefer a __u16 here. Also put the request field *before* the >> reserved array, not after. > > The reserved array isn't aligned to a 32 bit (or even 16 bit) boundary. I can > put the request field before it, with a 8 bit hole before the field. There's no alignment at all due to: >> > } __attribute__ ((packed)); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Friday 18 December 2015 18:37:51 Geert Uytterhoeven wrote: > On Fri, Dec 18, 2015 at 6:16 PM, Laurent Pinchart wrote: > >> > @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane { > >> > __u8 ycbcr_enc; > >> > __u8 quantization; > >> > __u8 xfer_func; > >> > - __u8 reserved[7]; > >> > + __u8 reserved[3]; > >> > + __u32 request; > >> > >> I think I mentioned this before: I feel uncomfortable using 4 bytes of > >> the reserved fields when the request ID is limited to 16 bits anyway. > > > > I'm still unsure whether request IDs should be 16 or 32 bits long. If we > > go for 16 bits then I'll of course make this field a __u16. > > > >> I would prefer a __u16 here. Also put the request field *before* the > >> reserved array, not after. > > > > The reserved array isn't aligned to a 32 bit (or even 16 bit) boundary. I > > can put the request field before it, with a 8 bit hole before the field. > > There's no alignment at all due to: > > >> > } __attribute__ ((packed)); Oops, indeed. Still, isn't it better to keep 16-bit or 32-bit values aligned to 16-bit or 32-bit boundaries ?
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 5af1d2d87558..5b2a8bc80eb2 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -1973,6 +1973,7 @@ struct v4l2_plane_pix_format { * @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding * @quantization: enum v4l2_quantization, colorspace quantization * @xfer_func: enum v4l2_xfer_func, colorspace transfer function + * @request: request ID */ struct v4l2_pix_format_mplane { __u32 width; @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane { __u8 ycbcr_enc; __u8 quantization; __u8 xfer_func; - __u8 reserved[7]; + __u8 reserved[3]; + __u32 request; } __attribute__ ((packed)); /**
Let userspace specify a request ID when getting or setting formats. The support is limited to the multi-planar API at the moment, extending it to the single-planar API is possible if needed. From a userspace point of view the API change is also minimized and doesn't require any new ioctl. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- include/uapi/linux/videodev2.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)