| Message ID | 1348495217-32715-1-git-send-email-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sylwester, Thanks for the patch. I noticed this is already in Mauro's tree and is there without my ack. I know it's partly my own fault since I haven't commented it earlier. Sylwester Nawrocki wrote: > Add subdev callbacks for setting up and retrieving parameters of the frame > on media bus that are not exposed to user space directly. > > Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > > Hi All, > > This patch is intended as an initial, mostly a stub, implementation of the > media bus frame format descriptors idea outlined in Sakari's RFC [1]. > I included in this patch only what is necessary for the s5p-fimc driver to > capture JPEG and interleaved JPEG/YUV image data from M-5MOLS and S5C73M3 > cameras. The union containing per media bus type structures describing > bus specific configuration is not included here, it likely needs much > discussions and I would like to get this patch merged for v3.7 if possible. > > To follow is a patch adding users of these new subdev operations. > > Comments are welcome. > > Thanks, > Sylwester > > [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg43530.html > --- > include/media/v4l2-subdev.h | 42 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 28067ed..f5d8441 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -21,6 +21,7 @@ > #ifndef _V4L2_SUBDEV_H > #define _V4L2_SUBDEV_H > > +#include <linux/types.h> What do you need types.h for? > #include <linux/v4l2-subdev.h> > #include <media/media-entity.h> > #include <media/v4l2-common.h> > @@ -45,6 +46,7 @@ struct v4l2_fh; > struct v4l2_subdev; > struct v4l2_subdev_fh; > struct tuner_setup; > +struct v4l2_mbus_frame_desc; > > /* decode_vbi_line */ > struct v4l2_decode_vbi_line { > @@ -226,6 +228,36 @@ struct v4l2_subdev_audio_ops { > int (*s_stream)(struct v4l2_subdev *sd, int enable); > }; > > +/* Indicates the @length field specifies maximum data length. */ > +#define V4L2_MBUS_FRAME_DESC_FL_LEN_MAX (1U << 0) > +/* Indicates user defined data format, i.e. non standard frame format. */ > +#define V4L2_MBUS_FRAME_DESC_FL_BLOB (1U << 1) > + > +/** > + * struct v4l2_mbus_frame_desc_entry - media bus frame description structure > + * @flags: V4L2_MBUS_FRAME_DESC_FL_* flags > + * @pixelcode: media bus pixel code, valid if FRAME_DESC_FL_BLOB is not set > + * @length: number of octets per frame, valid for compressed or unspecified > + * formats > + */ > +struct v4l2_mbus_frame_desc_entry { > + u16 flags; > + u32 pixelcode; > + u32 length; > +}; Do you think that the flags, pixelcode and length defines (a part of) the frame precisely enough? How about width and height; they are important for uncompressed formats? Also, as stated above, "lenght of octets for frame" is only meaningful for compressed formats and for those with 8 bits per pixel. However, I think that limiting the frame descriptors for compressed formats only is simply not enough. The main use case I had in mind originally, and I still see it that way, involves uncompressed formats, especially metadata. Currently I see that all this is serving is just one use case: providing the maximum frame length in octets for compressed formats to the CSI-2 receiver. There's nothing wrong in using frame descriptors for that --- I think it's valid use for them, but we also need to consider other use cases. > +#define V4L2_FRAME_DESC_ENTRY_MAX 4 > + > +/** > + * struct v4l2_mbus_frame_desc - media bus data frame description > + * @entry: frame descriptors array > + * @num_entries: number of entries in @entry array > + */ > +struct v4l2_mbus_frame_desc { > + struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX]; > + unsigned short num_entries; > +}; > + > /* > s_std_output: set v4l2_std_id for video OUTPUT devices. This is ignored by > video input devices. > @@ -461,6 +493,12 @@ struct v4l2_subdev_ir_ops { > struct v4l2_subdev_ir_parameters *params); > }; > > +/** > + * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations > + * @get_frame_desc: get the current low level media bus frame parameters. > + * @get_frame_desc: set the low level media bus frame parameters, @fd array > + * may be adjusted by the subdev driver to device capabilities. > + */ > struct v4l2_subdev_pad_ops { > int (*enum_mbus_code)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, > struct v4l2_subdev_mbus_code_enum *code); > @@ -489,6 +527,10 @@ struct v4l2_subdev_pad_ops { > struct v4l2_subdev_format *source_fmt, > struct v4l2_subdev_format *sink_fmt); > #endif /* CONFIG_MEDIA_CONTROLLER */ > + int (*get_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd); > + int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd); Is there a meaningful use case for setting the frame descriptor? I would assume that this is what the driver for the transmitting component (e.g. a sensor) defines pretty much independently and that's mostly hardware dependent and not freely changeable. At least I haven't seen such configurability, let alone to the extent it would make sense to express it with such a relatively generic interface. Considering the above, I think this is going to mainline too early. At the very least I suggest that any further use of frame descriptors only comes after more people have had their say over the topic and a rough concensus is reached. Kind regards,
Hi Sakari, Thanks for the review. On 10/08/2012 11:12 PM, Sakari Ailus wrote: > Hi Sylwester, > > Thanks for the patch. I noticed this is already in Mauro's tree and is there > without my ack. I know it's partly my own fault since I haven't commented it > earlier. > > Sylwester Nawrocki wrote: >> Add subdev callbacks for setting up and retrieving parameters of the frame >> on media bus that are not exposed to user space directly. >> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> --- >> >> Hi All, >> >> This patch is intended as an initial, mostly a stub, implementation of the >> media bus frame format descriptors idea outlined in Sakari's RFC [1]. >> I included in this patch only what is necessary for the s5p-fimc driver to >> capture JPEG and interleaved JPEG/YUV image data from M-5MOLS and S5C73M3 >> cameras. The union containing per media bus type structures describing >> bus specific configuration is not included here, it likely needs much >> discussions and I would like to get this patch merged for v3.7 if possible. >> >> To follow is a patch adding users of these new subdev operations. >> >> Comments are welcome. >> >> Thanks, >> Sylwester >> >> [1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg43530.html >> --- >> include/media/v4l2-subdev.h | 42 ++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index 28067ed..f5d8441 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -21,6 +21,7 @@ >> #ifndef _V4L2_SUBDEV_H >> #define _V4L2_SUBDEV_H >> >> +#include <linux/types.h> > > What do you need types.h for? I remember hitting a build error due to missing declarations of u32 and u16. But not sure how it happened, since linux/types.h is included in linux/v4l2-subdev.h... >> #include <linux/v4l2-subdev.h> >> #include <media/media-entity.h> >> #include <media/v4l2-common.h> >> @@ -45,6 +46,7 @@ struct v4l2_fh; >> struct v4l2_subdev; >> struct v4l2_subdev_fh; >> struct tuner_setup; >> +struct v4l2_mbus_frame_desc; >> >> /* decode_vbi_line */ >> struct v4l2_decode_vbi_line { >> @@ -226,6 +228,36 @@ struct v4l2_subdev_audio_ops { >> int (*s_stream)(struct v4l2_subdev *sd, int enable); >> }; >> >> +/* Indicates the @length field specifies maximum data length. */ >> +#define V4L2_MBUS_FRAME_DESC_FL_LEN_MAX (1U << 0) >> +/* Indicates user defined data format, i.e. non standard frame format. */ >> +#define V4L2_MBUS_FRAME_DESC_FL_BLOB (1U << 1) >> + >> +/** >> + * struct v4l2_mbus_frame_desc_entry - media bus frame description structure >> + * @flags: V4L2_MBUS_FRAME_DESC_FL_* flags >> + * @pixelcode: media bus pixel code, valid if FRAME_DESC_FL_BLOB is not set >> + * @length: number of octets per frame, valid for compressed or unspecified >> + * formats >> + */ >> +struct v4l2_mbus_frame_desc_entry { >> + u16 flags; >> + u32 pixelcode; >> + u32 length; >> +}; > > Do you think that the flags, pixelcode and length defines (a part of) the frame > precisely enough? How about width and height; they are important for > uncompressed formats? Obviously not, nor it supposed to be precise enough yet and to make everyone happy right away. You proposed an union for width and height [1], and I assume that could still be added. But there might be use cases where width, height and length would be needed at the same time. > Also, as stated above, "lenght of octets for frame" is only meaningful for > compressed formats and for those with 8 bits per pixel. However, I think that > limiting the frame descriptors for compressed formats only is simply not > enough. The main use case I had in mind originally, and I still see it that > way, involves uncompressed formats, especially metadata. It's not only limited to compressed formats. I can't see how this is related to 8-bits per pixel, number of bits per pixel is variable for compressed streams. > Currently I see that all this is serving is just one use case: providing the > maximum frame length in octets for compressed formats to the CSI-2 receiver. > There's nothing wrong in using frame descriptors for that --- I think it's > valid use for them, but we also need to consider other use cases. Mostly yes, we're also using it for retrieving from the sensor subdev driver the information required for V4L2_PIX_FMT_S5C_UYVY_JPG format data capture, i.e. the second entry in the array describes meta-data. >> +#define V4L2_FRAME_DESC_ENTRY_MAX 4 >> + >> +/** >> + * struct v4l2_mbus_frame_desc - media bus data frame description >> + * @entry: frame descriptors array >> + * @num_entries: number of entries in @entry array >> + */ >> +struct v4l2_mbus_frame_desc { >> + struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX]; >> + unsigned short num_entries; >> +}; >> + >> /* >> s_std_output: set v4l2_std_id for video OUTPUT devices. This is ignored by >> video input devices. >> @@ -461,6 +493,12 @@ struct v4l2_subdev_ir_ops { >> struct v4l2_subdev_ir_parameters *params); >> }; >> >> +/** >> + * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations >> + * @get_frame_desc: get the current low level media bus frame parameters. >> + * @get_frame_desc: set the low level media bus frame parameters, @fd array >> + * may be adjusted by the subdev driver to device >> capabilities. >> + */ >> struct v4l2_subdev_pad_ops { >> int (*enum_mbus_code)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, >> struct v4l2_subdev_mbus_code_enum *code); >> @@ -489,6 +527,10 @@ struct v4l2_subdev_pad_ops { >> struct v4l2_subdev_format *source_fmt, >> struct v4l2_subdev_format *sink_fmt); >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> + int (*get_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, >> + struct v4l2_mbus_frame_desc *fd); >> + int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, >> + struct v4l2_mbus_frame_desc *fd); > > Is there a meaningful use case for setting the frame descriptor? I would assume > that this is what the driver for the transmitting component (e.g. a sensor) > defines pretty much independently and that's mostly hardware dependent and not > freely changeable. At least I haven't seen such configurability, let alone to > the extent it would make sense to express it with such a relatively generic > interface. There might be parameters that one would want to modify, so it could be done with a get call, modifying required value(s) and a set call. > Considering the above, I think this is going to mainline too early. At the very > least I suggest that any further use of frame descriptors only comes after more > people have had their say over the topic and a rough concensus is reached. Yes, that was entirely my intention as well. If there is someone needing more features further extensions could be made, after proper discussions and making sure it satisfies everyone. -- Regards, Sylwester [1] http://www.spinics.net/lists/linux-media/msg44629.html -- 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/v4l2-subdev.h b/include/media/v4l2-subdev.h index 28067ed..f5d8441 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -21,6 +21,7 @@ #ifndef _V4L2_SUBDEV_H #define _V4L2_SUBDEV_H +#include <linux/types.h> #include <linux/v4l2-subdev.h> #include <media/media-entity.h> #include <media/v4l2-common.h> @@ -45,6 +46,7 @@ struct v4l2_fh; struct v4l2_subdev; struct v4l2_subdev_fh; struct tuner_setup; +struct v4l2_mbus_frame_desc; /* decode_vbi_line */ struct v4l2_decode_vbi_line { @@ -226,6 +228,36 @@ struct v4l2_subdev_audio_ops { int (*s_stream)(struct v4l2_subdev *sd, int enable); }; +/* Indicates the @length field specifies maximum data length. */ +#define V4L2_MBUS_FRAME_DESC_FL_LEN_MAX (1U << 0) +/* Indicates user defined data format, i.e. non standard frame format. */ +#define V4L2_MBUS_FRAME_DESC_FL_BLOB (1U << 1) + +/** + * struct v4l2_mbus_frame_desc_entry - media bus frame description structure + * @flags: V4L2_MBUS_FRAME_DESC_FL_* flags + * @pixelcode: media bus pixel code, valid if FRAME_DESC_FL_BLOB is not set + * @length: number of octets per frame, valid for compressed or unspecified + * formats + */ +struct v4l2_mbus_frame_desc_entry { + u16 flags; + u32 pixelcode; + u32 length; +}; + +#define V4L2_FRAME_DESC_ENTRY_MAX 4 + +/** + * struct v4l2_mbus_frame_desc - media bus data frame description + * @entry: frame descriptors array + * @num_entries: number of entries in @entry array + */ +struct v4l2_mbus_frame_desc { + struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX]; + unsigned short num_entries; +}; + /* s_std_output: set v4l2_std_id for video OUTPUT devices. This is ignored by video input devices. @@ -461,6 +493,12 @@ struct v4l2_subdev_ir_ops { struct v4l2_subdev_ir_parameters *params); }; +/** + * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations + * @get_frame_desc: get the current low level media bus frame parameters. + * @get_frame_desc: set the low level media bus frame parameters, @fd array + * may be adjusted by the subdev driver to device capabilities. + */ struct v4l2_subdev_pad_ops { int (*enum_mbus_code)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, struct v4l2_subdev_mbus_code_enum *code); @@ -489,6 +527,10 @@ struct v4l2_subdev_pad_ops { struct v4l2_subdev_format *source_fmt, struct v4l2_subdev_format *sink_fmt); #endif /* CONFIG_MEDIA_CONTROLLER */ + int (*get_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, + struct v4l2_mbus_frame_desc *fd); + int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad, + struct v4l2_mbus_frame_desc *fd); }; struct v4l2_subdev_ops {