Re: [PATCH/RFC 26/48] videodev2.h: Add request field to v4l2_pix_format_mplane

2015-12-20 Thread Laurent Pinchart 
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 {
> >> > __u8ycbcr_enc;
> >> > __u8quantization;
> >> > __u8xfer_func;
> >> > -   __u8reserved[7];
> >> > +   __u8reserved[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 ?

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 26/48] videodev2.h: Add request field to v4l2_pix_format_mplane

2015-12-18 Thread Hans Verkuil 
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 
> ---
>  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 {
>   __u8ycbcr_enc;
>   __u8quantization;
>   __u8xfer_func;
> - __u8reserved[7];
> + __u8reserved[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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 26/48] videodev2.h: Add request field to v4l2_pix_format_mplane

2015-12-18 Thread Laurent Pinchart 
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
> > 
> > ---
> > 
> >  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 {
> > 
> > __u8ycbcr_enc;
> > __u8quantization;
> > __u8xfer_func;
> > 
> > -   __u8reserved[7];
> > +   __u8reserved[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));
> >  
> >  /**

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 26/48] videodev2.h: Add request field to v4l2_pix_format_mplane

2015-12-18 Thread Geert Uytterhoeven 
Hi Laurent,

On Fri, Dec 18, 2015 at 6:16 PM, Laurent Pinchart
 wrote:
>> > @@ -1987,7 +1988,8 @@ struct v4l2_pix_format_mplane {
>> >
>> > __u8ycbcr_enc;
>> > __u8quantization;
>> > __u8xfer_func;
>> >
>> > -   __u8reserved[7];
>> > +   __u8reserved[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 -- ge...@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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH/RFC 26/48] videodev2.h: Add request field to v4l2_pix_format_mplane

2015-12-17 Thread Laurent Pinchart 
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 
---
 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 {
__u8ycbcr_enc;
__u8quantization;
__u8xfer_func;
-   __u8reserved[7];
+   __u8reserved[3];
+   __u32   request;
 } __attribute__ ((packed));
 
 /**
-- 
2.4.10

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html