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@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx/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,
--
Sakari Ailus
sakari.ailus@xxxxxx
--
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
