Hi Sakari, Laurent, all..
The more I look at this patch, the less I like it, to be honest...
On Sat, Mar 16, 2019 at 04:47:57PM +0100, Jacopo Mondi wrote:
> Add PHY-specific parameters to MIPI CSI-2 frame descriptor.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> ---
> include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------
> 1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 6311f670de3c..eca9633c83bf 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -317,11 +317,33 @@ struct v4l2_subdev_audio_ops {
> int (*s_stream)(struct v4l2_subdev *sd, int enable);
> };
>
> +#define V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES 4
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_dphy - MIPI D-PHY bus configuration
> + *
> + * @clock_lane: physical lane index of the clock lane
> + * @data_lanes: an array of physical data lane indexes
> + * @num_data_lanes: number of data lanes
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_dphy {
> + u8 clock_lane;
> + u8 data_lanes[V4L2_FRAME_DESC_ENTRY_DPHY_DATA_LANES];
> + u8 num_data_lanes;
> +};
> +
> +/**
> + * struct v4l2_mbus_frame_desc_entry_csi2_cphy - MIPI C-PHY bus configuration
> + */
> +struct v4l2_mbus_frame_desc_entry_csi2_cphy {
> + /* TODO */
> +};
> +
> /**
> * struct v4l2_mbus_frame_desc_entry_csi2
> *
> - * @channel: CSI-2 virtual channel
> - * @data_type: CSI-2 data type ID
> + * @channel: CSI-2 virtual channel
> + * @data_type: CSI-2 data type ID
> */
> struct v4l2_mbus_frame_desc_entry_csi2 {
> u8 channel;
> @@ -371,18 +393,26 @@ enum v4l2_mbus_frame_desc_type {
> V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM,
> V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL,
> V4L2_MBUS_FRAME_DESC_TYPE_CCP2,
> - V4L2_MBUS_FRAME_DESC_TYPE_CSI2,
> + V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY,
> + V4L2_MBUS_FRAME_DESC_TYPE_CSI2_CPHY,
> };
>
> /**
> * struct v4l2_mbus_frame_desc - media bus data frame description
> - * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> + * @entry: frame descriptors array
> + * @phy: PHY specific parameters
> + * @phy.dphy: MIPI D-PHY specific bus configurations
> + * @phy.cphy: MIPI C-PHY specific bus configurations
> + * @num_entries: number of entries in @entry array
> */
> struct v4l2_mbus_frame_desc {
> enum v4l2_mbus_frame_desc_type type;
> struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> + union {
> + struct v4l2_mbus_frame_desc_entry_csi2_dphy csi2_dphy;
> + struct v4l2_mbus_frame_desc_entry_csi2_cphy csi2_cphy;
> + } phy;
I had a brief look at how v4l2_mbus_frame_desc evolved, and how Sakari
has extended it with stream and CSI-2 specific information in the
v4l-multiplexed series[1] before I've thrown the phy configuration part
in the mix with this patch, and my feeling is that we're maybe bending
the original v4l2_mbus_frame_desc purpose to our needs a bit...
What I'm questioning is if stream and media bus configuration actually
belong here. Sure we could bend this to match our purposes, but
'struct v4l2_mbus_frame_desc' is actually only used in mainline to
transport informations about binary streams, and it fits the purpose,
which is, by the way, also intended in the structure name.
What I think would be more appropriate would be a 'streaming session'
descriptor, that contains entries for up to 4 streams and one phy
configuration part. I would even say that, as this changes are intended
to make frame_desc usable to negotiate multiplexed streams parameters
that are only available on CSI-2 (VC and data type at the protocol level,
and since this patch, a few D-PHY parameters at the phy level) this
could actually be (at least initially) a new CSI-2 specific
structure with a new set of operations associated, so that we can leave
frame_desc alone to be used for binary blob description?
What do you think? Are you comfortable with this change?
Thanks
j
[1]
https://patchwork.kernel.org/patch/10875871/
https://patchwork.kernel.org/patch/10875873/
https://patchwork.kernel.org/patch/10875879/
From the same series, here it is how frame_desc is now used:
https://patchwork.kernel.org/patch/10875911/
https://patchwork.kernel.org/patch/10875911/
And from this RFC series how it would be used to expose phy
parameters:
https://patchwork.kernel.org/patch/10855937/
> unsigned short num_entries;
> };
>
> --
> 2.21.0
>
Attachment:
signature.asc
Description: PGP signature
