Hi Sakari,
On Thu, Mar 17, 2022 at 06:06:21PM +0200, Sakari Ailus wrote:
> On Thu, Mar 17, 2022 at 04:36:59PM +0200, Laurent Pinchart wrote:
> > The helper functions that test validity of colorspace-related fields
> > use the last value of the corresponding enums. This isn't very
> > future-proof, as there's a high chance someone adding a new value may
> > forget to update the helpers. Add new "LAST" entries to the enumerations
> > to improve this, and keep them private to the kernel.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > include/media/v4l2-common.h | 10 +++++-----
> > include/uapi/linux/videodev2.h | 29 +++++++++++++++++++++++++++++
> > 2 files changed, 34 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 3eb202259e8c..b686124e2ccf 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -563,19 +563,19 @@ static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf,
> > static inline bool v4l2_is_colorspace_valid(__u32 colorspace)
> > {
> > return colorspace > V4L2_COLORSPACE_DEFAULT &&
> > - colorspace <= V4L2_COLORSPACE_DCI_P3;
> > + colorspace <= V4L2_COLORSPACE_LAST;
> > }
> >
> > static inline bool v4l2_is_xfer_func_valid(__u32 xfer_func)
> > {
> > return xfer_func > V4L2_XFER_FUNC_DEFAULT &&
> > - xfer_func <= V4L2_XFER_FUNC_SMPTE2084;
> > + xfer_func <= V4L2_XFER_FUNC_LAST;
> > }
> >
> > static inline bool v4l2_is_ycbcr_enc_valid(__u8 ycbcr_enc)
> > {
> > return ycbcr_enc > V4L2_YCBCR_ENC_DEFAULT &&
> > - ycbcr_enc <= V4L2_YCBCR_ENC_SMPTE240M;
> > + ycbcr_enc <= V4L2_YCBCR_ENC_LAST;
> > }
> >
> > static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
> > @@ -585,8 +585,8 @@ static inline bool v4l2_is_hsv_enc_valid(__u8 hsv_enc)
> >
> > static inline bool v4l2_is_quant_valid(__u8 quantization)
> > {
> > - return quantization == V4L2_QUANTIZATION_FULL_RANGE ||
> > - quantization == V4L2_QUANTIZATION_LIM_RANGE;
> > + return quantization > V4L2_QUANTIZATION_DEFAULT &&
> > + quantization <= V4L2_QUANTIZATION_LAST;
> > }
> >
> > #endif /* V4L2_COMMON_H_ */
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index 16dcd9dd1a50..099da1576db6 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -245,6 +245,14 @@ enum v4l2_colorspace {
> >
> > /* DCI-P3 colorspace, used by cinema projectors */
> > V4L2_COLORSPACE_DCI_P3 = 12,
> > +
> > +#ifdef __KERNEL__
> > + /*
> > + * Largest supported colorspace value, used by the framework to check
> > + * for invalid values.
> > + */
> > + V4L2_COLORSPACE_LAST = 12,
>
> I might just add the enum there, it is more obvious it needs updating if
> it's right next to the previous one.
I'm not sure to understand what you mean by "add the enum there".
> Or rely on the compiler assigning the
> value, and update the code. Up to you.
Good idea. Hans, what do you think ?
> For both:
>
> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> > +#endif
> > };
> >
> > /*
> > @@ -283,6 +291,13 @@ enum v4l2_xfer_func {
> > V4L2_XFER_FUNC_NONE = 5,
> > V4L2_XFER_FUNC_DCI_P3 = 6,
> > V4L2_XFER_FUNC_SMPTE2084 = 7,
> > +#ifdef __KERNEL__
> > + /*
> > + * Largest supported transfer function value, used by the framework to
> > + * check for invalid values.
> > + */
> > + V4L2_XFER_FUNC_LAST = 7,
> > +#endif
> > };
> >
> > /*
> > @@ -343,6 +358,13 @@ enum v4l2_ycbcr_encoding {
> >
> > /* SMPTE 240M -- Obsolete HDTV */
> > V4L2_YCBCR_ENC_SMPTE240M = 8,
> > +#ifdef __KERNEL__
> > + /*
> > + * Largest supported encoding value, used by the framework to check for
> > + * invalid values.
> > + */
> > + V4L2_YCBCR_ENC_LAST = 8,
> > +#endif
> > };
> >
> > /*
> > @@ -378,6 +400,13 @@ enum v4l2_quantization {
> > V4L2_QUANTIZATION_DEFAULT = 0,
> > V4L2_QUANTIZATION_FULL_RANGE = 1,
> > V4L2_QUANTIZATION_LIM_RANGE = 2,
> > +#ifdef __KERNEL__
> > + /*
> > + * Largest supported quantization value, used by the framework to check
> > + * for invalid values.
> > + */
> > + V4L2_QUANTIZATION_LAST = 2,
> > +#endif
> > };
> >
> > /*
--
Regards,
Laurent Pinchart
