| Message ID | 20190316154801.20460-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: Implement negotiation of CSI-2 data lanes | expand |
On 03/16/2019 06:47 PM, Jacopo Mondi wrote: > Add PHY-specific parameters to MIPI CSI-2 frame descriptor. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > 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 [...] > @@ -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 The union members have csi2_ prefix in their names, no? > + * @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; > unsigned short num_entries; > }; >
Hi Jacopo, 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@jmondi.org> > --- > 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; Do you need more than the number of the data lanes? I'd expect few devices to be able to do more than that. The PHY type comes already from the firmware but I guess it's good to do the separation here as well. Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I think it'd be good to stick to a single definition. > +}; > + > +/** > + * 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; > unsigned short num_entries; > }; >
Hello, On Sat, Mar 16, 2019 at 07:20:28PM +0300, Sergei Shtylyov wrote: > On 03/16/2019 06:47 PM, Jacopo Mondi wrote: > > > Add PHY-specific parameters to MIPI CSI-2 frame descriptor. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > 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 > [...] > > @@ -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 > > The union members have csi2_ prefix in their names, no? > Correct! Thanks Sergei for noticing this! > > + * @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; > > unsigned short num_entries; > > }; > > >
Hi Sakari, +Maxime because of it's D-PHY work in the phy framework. On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote: > Hi Jacopo, > > 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@jmondi.org> > > --- > > 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; > > Do you need more than the number of the data lanes? I'd expect few devices > to be able to do more than that. The PHY type comes already from the > firmware but I guess it's good to do the separation here as well. Indeed lane reordering at run time seems like a quite unusual operation. I would say I could drop that, but then, a structure and a new field in v4l2_mbus_frame_desc for just an u8, isn't it an overkill (unless we know it could be expanded, with say, D-PHY timings as in Maxime's D-PHY phy support implementation. Again, not sure they should be run-time negotiated, but...) > > Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I > think it'd be good to stick to a single definition. > I initially moved and renamed that define, then went back and added a new one as I was not sure where to put this new global and D-PHY specific define. I'll look into unifying them. Thanks j > > +}; > > + > > +/** > > + * 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; > > unsigned short num_entries; > > }; > > > > -- > Regards, > > Sakari Ailus > sakari.ailus@linux.intel.com
Hi Jacopo. Sorry, for some reason linux-media messages aren't coming through to me at the moment. I'm interested mainly for tc358743 rather than adv748x, but they want the very similar functionality. I'll try to create patches for that source as well. On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Sakari, > +Maxime because of it's D-PHY work in the phy framework. > > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > 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@jmondi.org> > > > --- > > > 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; > > > > Do you need more than the number of the data lanes? I'd expect few devices > > to be able to do more than that. The PHY type comes already from the > > firmware but I guess it's good to do the separation here as well. > > Indeed lane reordering at run time seems like a quite unusual > operation. I would say I could drop that, but then, a structure and a > new field in v4l2_mbus_frame_desc for just an u8, isn't it an > overkill (unless we know it could be expanded, with say, D-PHY timings > as in Maxime's D-PHY phy support implementation. Again, not sure they > should be run-time negotiated, but...) If we're adding extra information, then can I suggest that the clock-noncontinuous flag is also added? If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial CSI2 switch), then there is nothing stopping one source wanting continuous clocks, and one not. Encoding it in the receiver's DT node therefore doesn't work for one of the sources. Duplication of that definition between source and receiver has always seemed a likely source of errors to me, but I'm the relative newcomer here. Cheers Dave [1] https://www.onsemi.com/PowerSolutions/product.do?id=FSA642 available on a board such as http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-v2-raspberry-pi-camera-module-v2-multiplexer-p-109 > > > > Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I > > think it'd be good to stick to a single definition. > > > > I initially moved and renamed that define, then went back and added a > new one as I was not sure where to put this new global and D-PHY > specific define. I'll look into unifying them. > > Thanks > j > > > > > +}; > > > + > > > +/** > > > + * 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; > > > unsigned short num_entries; > > > }; > > > > > > > -- > > Regards, > > > > Sakari Ailus > > sakari.ailus@linux.intel.com > -----BEGIN PGP SIGNATURE----- > > iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAlyPV1EACgkQcjQGjxah > VjxkUxAAkdhZTDN90GEkYfaiIIhthYaaz3Hd2ByI51aTqbNR4wq/6+WeqqmCUVJP > wSB4cD3NQp6ZfJLbw97+XZ1oIZj7n4IRNDD050a3z4MFmVkJiz7dJ/yetBZhaqvA > eutDDqk+OM6GJc0d2IUTOuiX69JSA9ToUXJrcMbkCp8TjzD5g7Kt7bwbQv4oaG44 > VBzTaMJpgGWzP1Lxv78mnWeOtH+WIuufw4vtjF5UHvRO8EC6f3kdqilem76+Ffn2 > N+n3ajhvbPyHk398wyxmcNhm29DZ6Y8CehqWw3AlzMHVbCeEEeEqsQ2MmRxrBlYx > +U8R0Nw9JHJ1BqsrjkWWWMVCrTNzoeAGIu4Dbd/81JpxAG+FowNaVDI0Xlm0r9wG > psLhQr6ZFaEcxE07VU7E8jfoGvjbRfmZkrtdxr34tUoBAE/YXfZ6rVXVCjTqABIf > dcGNVMtVDV7cMvqjqiJa+9uHx467b0QZEaYn3CwW2V7qoa4D7iLf4WchsW9gvjq4 > mUEnzQFbd63qFuBaTrE4paq9hkYcoSAjrrDtL12l3FHop4wTjJLCJXLSO7khd97w > qMQOKnWKI8edPrakz3nBx8lexgOWcoe/tMRYDF4dgYJLMm+ZNS0R90Xu9x5DMOvG > cYBKeQk72lr0znYNTowGdc+xZcuO2BjyU83SnyZuRsmAo6kg3nA= > =BYda > -----END PGP SIGNATURE-----
Hi Dave, thanks for the feedback! On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote: > Hi Jacopo. > > Sorry, for some reason linux-media messages aren't coming through to > me at the moment. > > I'm interested mainly for tc358743 rather than adv748x, but they want > the very similar functionality. > I'll try to create patches for that source as well. > > On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Sakari, > > +Maxime because of it's D-PHY work in the phy framework. > > > > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote: > > > Hi Jacopo, > > > > > > 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@jmondi.org> > > > > --- > > > > 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; > > > > > > Do you need more than the number of the data lanes? I'd expect few devices > > > to be able to do more than that. The PHY type comes already from the > > > firmware but I guess it's good to do the separation here as well. > > > > Indeed lane reordering at run time seems like a quite unusual > > operation. I would say I could drop that, but then, a structure and a > > new field in v4l2_mbus_frame_desc for just an u8, isn't it an > > overkill (unless we know it could be expanded, with say, D-PHY timings > > as in Maxime's D-PHY phy support implementation. Again, not sure they > > should be run-time negotiated, but...) > > If we're adding extra information, then can I suggest that the > clock-noncontinuous flag is also added? Great, this is a good point. I'll add this flag to the next version. > If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial > CSI2 switch), then there is nothing stopping one source wanting > continuous clocks, and one not. Encoding it in the receiver's DT node > therefore doesn't work for one of the sources. Duplication of that > definition between source and receiver has always seemed a likely > source of errors to me, but I'm the relative newcomer here. > The duplication of the bus configuration parameters seems to be a notable source of confusion :) Apart from this, if drivers move to support fetching some parameters from the frame descriptor, at the same time they should make sure they retain compatibility with "legacy" implementations, where these informations come from DT only. I don't think that's a big deal, as one should not exclude the other, but establishing a policy for this going forward might be beneficial. I would say, as long as your receiver does not mandate the source to provide bus parameters in the frame descriptor, DT has always precedence, as this would also make sure older DT still work as intended on your platform. Thanks j > Cheers > Dave > > [1] https://www.onsemi.com/PowerSolutions/product.do?id=FSA642 > available on a board such as > http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-v2-raspberry-pi-camera-module-v2-multiplexer-p-109 > > > > > > > Could you use V4L2_FWNODE_CSI2_MAX_DATA_LANES? Or we could rename it. But I > > > think it'd be good to stick to a single definition. > > > > > > > I initially moved and renamed that define, then went back and added a > > new one as I was not sure where to put this new global and D-PHY > > specific define. I'll look into unifying them. > > > > Thanks > > j > > > > > > > > +}; > > > > + > > > > +/** > > > > + * 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; > > > > unsigned short num_entries; > > > > }; > > > > > > > > > > -- > > > Regards, > > > > > > Sakari Ailus > > > sakari.ailus@linux.intel.com > > -----BEGIN PGP SIGNATURE----- > > > > iQIzBAABCAAdFiEEtcQ9SICaIIqPWDjAcjQGjxahVjwFAlyPV1EACgkQcjQGjxah > > VjxkUxAAkdhZTDN90GEkYfaiIIhthYaaz3Hd2ByI51aTqbNR4wq/6+WeqqmCUVJP > > wSB4cD3NQp6ZfJLbw97+XZ1oIZj7n4IRNDD050a3z4MFmVkJiz7dJ/yetBZhaqvA > > eutDDqk+OM6GJc0d2IUTOuiX69JSA9ToUXJrcMbkCp8TjzD5g7Kt7bwbQv4oaG44 > > VBzTaMJpgGWzP1Lxv78mnWeOtH+WIuufw4vtjF5UHvRO8EC6f3kdqilem76+Ffn2 > > N+n3ajhvbPyHk398wyxmcNhm29DZ6Y8CehqWw3AlzMHVbCeEEeEqsQ2MmRxrBlYx > > +U8R0Nw9JHJ1BqsrjkWWWMVCrTNzoeAGIu4Dbd/81JpxAG+FowNaVDI0Xlm0r9wG > > psLhQr6ZFaEcxE07VU7E8jfoGvjbRfmZkrtdxr34tUoBAE/YXfZ6rVXVCjTqABIf > > dcGNVMtVDV7cMvqjqiJa+9uHx467b0QZEaYn3CwW2V7qoa4D7iLf4WchsW9gvjq4 > > mUEnzQFbd63qFuBaTrE4paq9hkYcoSAjrrDtL12l3FHop4wTjJLCJXLSO7khd97w > > qMQOKnWKI8edPrakz3nBx8lexgOWcoe/tMRYDF4dgYJLMm+ZNS0R90Xu9x5DMOvG > > cYBKeQk72lr0znYNTowGdc+xZcuO2BjyU83SnyZuRsmAo6kg3nA= > > =BYda > > -----END PGP SIGNATURE-----
Hi Dave, On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote: > Hi Jacopo. > > Sorry, for some reason linux-media messages aren't coming through to > me at the moment. > > I'm interested mainly for tc358743 rather than adv748x, but they want > the very similar functionality. > I'll try to create patches for that source as well. > > On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Sakari, > > +Maxime because of it's D-PHY work in the phy framework. > > > > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote: > > > Hi Jacopo, > > > > > > 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@jmondi.org> > > > > --- > > > > 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; > > > > > > Do you need more than the number of the data lanes? I'd expect few devices > > > to be able to do more than that. The PHY type comes already from the > > > firmware but I guess it's good to do the separation here as well. > > > > Indeed lane reordering at run time seems like a quite unusual > > operation. I would say I could drop that, but then, a structure and a > > new field in v4l2_mbus_frame_desc for just an u8, isn't it an > > overkill (unless we know it could be expanded, with say, D-PHY timings > > as in Maxime's D-PHY phy support implementation. Again, not sure they > > should be run-time negotiated, but...) > > If we're adding extra information, then can I suggest that the > clock-noncontinuous flag is also added? > If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial > CSI2 switch), then there is nothing stopping one source wanting > continuous clocks, and one not. Encoding it in the receiver's DT node > therefore doesn't work for one of the sources. Duplication of that > definition between source and receiver has always seemed a likely > source of errors to me, but I'm the relative newcomer here. Do you have such a case somewhere? As this is typically up to a device configuration (for those that support it), it should be possible to use the same setting for both the sensors.
Hi Sakari On Thu, 4 Apr 2019 at 09:49, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Dave, > > On Mon, Mar 18, 2019 at 03:29:55PM +0000, Dave Stevenson wrote: > > Hi Jacopo. > > > > Sorry, for some reason linux-media messages aren't coming through to > > me at the moment. > > > > I'm interested mainly for tc358743 rather than adv748x, but they want > > the very similar functionality. > > I'll try to create patches for that source as well. > > > > On Mon, 18 Mar 2019 at 08:30, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Sakari, > > > +Maxime because of it's D-PHY work in the phy framework. > > > > > > On Sat, Mar 16, 2019 at 07:51:21PM +0200, Sakari Ailus wrote: > > > > Hi Jacopo, > > > > > > > > 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@jmondi.org> > > > > > --- > > > > > 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; > > > > > > > > Do you need more than the number of the data lanes? I'd expect few devices > > > > to be able to do more than that. The PHY type comes already from the > > > > firmware but I guess it's good to do the separation here as well. > > > > > > Indeed lane reordering at run time seems like a quite unusual > > > operation. I would say I could drop that, but then, a structure and a > > > new field in v4l2_mbus_frame_desc for just an u8, isn't it an > > > overkill (unless we know it could be expanded, with say, D-PHY timings > > > as in Maxime's D-PHY phy support implementation. Again, not sure they > > > should be run-time negotiated, but...) > > > > If we're adding extra information, then can I suggest that the > > clock-noncontinuous flag is also added? > > If you've got muxed CSI2 buses (eg via the FSA642 [1] as a trivial > > CSI2 switch), then there is nothing stopping one source wanting > > continuous clocks, and one not. Encoding it in the receiver's DT node > > therefore doesn't work for one of the sources. Duplication of that > > definition between source and receiver has always seemed a likely > > source of errors to me, but I'm the relative newcomer here. > > Do you have such a case somewhere? There are boards available for the Pi that use the FSA642 to switch between multiple camera modules [1]. They haven't written any clever drivers as their intended use is against the (closed) firmware, therefore they currently have to be identical sensors on each input. The use of those boards seems to fit pretty cleanly into this multiplexed CSI bus framework, and there needn't be the limitation of them being the same sensor. I acknowledge that the Pi CSI2 receiver driver isn't upstream yet. I put that on hold as the TC358743 was one of the main devices to be supported, and without the rest of this patch set passing the number of lanes in use through then that was pretty dead in the water. It was less effort to keep the driver in the Pi vendor tree using the g_mbus_config mechanism that was rejected, rather than holding patches on an upstreamed driver. Once this set is merged and I can therefore support TC358743 cleanly I'll be looking again at upstreaming it. [1] http://www.ivmech.com/magaza/en/development-modules-c-4/ivport-dual-raspberry-pi-camera-module-multiplexer-p-104 > As this is typically up to a device configuration (for those that support > it), it should be possible to use the same setting for both the sensors. Are you meaning CSI transmitter or receiver for device configuration? Most receivers support either, but often have to be told which mode to expect. TC358743 seems to be one of the few CSI drivers that actually supports both. None of the others appear to quote the use of clock-noncontinuous in their DT bindings, but that doesn't guarantee that that is the actual truth. So there's no issue at the moment, but there will be issues when you get the first driver that only supports clock-noncontinuous and someone wants to mux it. As I said, having the potential for the two ends to be set differently in device tree and no way to check at runtime seems like an unnecessary trap for the unwary. You're now adding a config path from source to receiver, therefore adding that information removes that confusion. Just a suggestion. Dave
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@jmondi.org> > --- > 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 >
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; unsigned short num_entries; };
Add PHY-specific parameters to MIPI CSI-2 frame descriptor. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- include/media/v4l2-subdev.h | 42 +++++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 6 deletions(-)