| Message ID | 20190924114955.13132-2-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add g_csi_active_lanes for dynamic active lanes | expand |
Hi Philipp, (CC'ing Sakari, Jacopo, Kieran and Niklas) Thank you for the patch. On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote: > Add a subdevice video operation that allows to query the number > of data lanes a MIPI CSI-2 TX is actively transmitting on. > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > New in v4. > --- > include/media/v4l2-subdev.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 71f1f2f0da53..bb71eedc38f6 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc { > * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev > * can adjust @size to a lower value and must not write more data to the > * buffer starting at @data than the original value of @size. > + * > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes. > */ > struct v4l2_subdev_video_ops { > int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config); > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops { > const struct v4l2_mbus_config *cfg); > int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf, > unsigned int *size); > + int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes); This shouldn't be a video operation but a pad operation, as a subdev could have multiple CSI-2 pads. Furthermore, you need to define the semantics of this operation more precisely. When can it be called, when is the information valid ? Can the subdev change the number of lanes it supports at runtime ? If so, how are race conditions avoided ? All this needs to be documented. Finally, the number of lanes is far from being the only information about a physical bus that could be interesting for a remote subdev. I would much prefer a more generic operation to retrieve bus information/configuration, with a data structure that we will be able to extend later. > }; > > /**
Hello, sorry for having missed this On Wed, Sep 25, 2019 at 04:41:13PM +0300, Laurent Pinchart wrote: > Hi Philipp, > > (CC'ing Sakari, Jacopo, Kieran and Niklas) > > Thank you for the patch. > > On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote: > > Add a subdevice video operation that allows to query the number > > of data lanes a MIPI CSI-2 TX is actively transmitting on. > > > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > New in v4. > > --- > > include/media/v4l2-subdev.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 71f1f2f0da53..bb71eedc38f6 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc { > > * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev > > * can adjust @size to a lower value and must not write more data to the > > * buffer starting at @data than the original value of @size. > > + * > > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes. > > */ > > struct v4l2_subdev_video_ops { > > int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config); > > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops { > > const struct v4l2_mbus_config *cfg); > > int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf, > > unsigned int *size); > > + int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes); > > This shouldn't be a video operation but a pad operation, as a subdev > could have multiple CSI-2 pads. > > Furthermore, you need to define the semantics of this operation more > precisely. When can it be called, when is the information valid ? Can > the subdev change the number of lanes it supports at runtime ? If so, > how are race conditions avoided ? All this needs to be documented. > > Finally, the number of lanes is far from being the only information > about a physical bus that could be interesting for a remote subdev. I > would much prefer a more generic operation to retrieve bus > information/configuration, with a data structure that we will be able to > extend later. > For the record we tried to get those information from the frame descriptor (the number of used data lanes and the clock continous/non-continous information to be precise) This is the RFC series I sent https://patchwork.kernel.org/project/linux-media/list/?series=92501 Which depends on Sakari's patches: https://patchwork.kernel.org/patch/10875871/ https://patchwork.kernel.org/patch/10875873/ The latest two were part of a much bigger series that tried to add support for multiplexed streams. Honestly, it now seems to be that part could be breakout without involving streams, and re-use that portion to negotiate the csi-2 bus configuration. I might be wrong though, and the two parts could not be separate easily (from a very quick look, after months, it does not seem so). In the past I spoke against this solution as I would have preferred leaving frame_desc alone and introduce a bus configuration operation. I tried a few times and I ended up implementing g_mbus_format but on pads and not video. Right now with Sakari's and Laurent's ack I would say re-using frame_desc might actually work and get use a feature which is needed by many (cc also Dave, as he had a similar issue with TC358743 iirc) Thanks j > > }; > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Wed, 2019-09-25 at 16:41 +0300, Laurent Pinchart wrote: > Hi Philipp, > > (CC'ing Sakari, Jacopo, Kieran and Niklas) > > Thank you for the patch. > > On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote: > > Add a subdevice video operation that allows to query the number > > of data lanes a MIPI CSI-2 TX is actively transmitting on. > > > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > --- > > New in v4. > > --- > > include/media/v4l2-subdev.h | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 71f1f2f0da53..bb71eedc38f6 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc { > > * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev > > * can adjust @size to a lower value and must not write more data to the > > * buffer starting at @data than the original value of @size. > > + * > > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes. > > */ > > struct v4l2_subdev_video_ops { > > int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config); > > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops { > > const struct v4l2_mbus_config *cfg); > > int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf, > > unsigned int *size); > > + int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes); > > This shouldn't be a video operation but a pad operation, as a subdev > could have multiple CSI-2 pads. Right this should be pad specific. > Furthermore, you need to define the semantics of this operation more > precisely. When can it be called, The downstream subdevice connected to this pad is expected to call this in its s_stream(enable=1) op, right before enabling the MIPI CSI-2 RX, and then calling s_stream(enable=1) on the same upstream subdevice. The returned value is a decision by the upstream subdevice driver based on external factors such as available link-frequencies and mbus frame format, so it can change whenever those are changed, but not by itself. > when is the information valid ? It is valid until the next time the pad's mbus frame format or link frequency are changed. Since the caller > Can the subdev change the number of lanes it supports at runtime ? At least for MIPI CSI-2, no. Are there any buses that can dynamically change bus width while active? > If so, how are race conditions avoided ? All this needs to be documented. I think no, so the only possible race conditions would be with reconfiguration, which should already be avoided by requiring this to be called from s_stream(),as the media pipeline is already started and all configuration locked in place at this point. > Finally, the number of lanes is far from being the only information > about a physical bus that could be interesting for a remote subdev. I > would much prefer a more generic operation to retrieve bus > information/configuration, with a data structure that we will be able to > extend later. This is specifically about configuration chosen by the transmitter, not physical bus properties, which can be specified in the device tree. The chosen link frequency (if more than one is specified in DT) could be one of those values. regards Philipp
Hi Jacopo, On Wed, 2019-09-25 at 16:51 +0200, Jacopo Mondi wrote: > Hello, > sorry for having missed this Thank you and Laurent for completing the list of interested parties, I had completely forgotten about the frame descriptors. > On Wed, Sep 25, 2019 at 04:41:13PM +0300, Laurent Pinchart wrote: > > Hi Philipp, > > > > (CC'ing Sakari, Jacopo, Kieran and Niklas) > > > > Thank you for the patch. > > > > On Tue, Sep 24, 2019 at 01:49:53PM +0200, Philipp Zabel wrote: > > > Add a subdevice video operation that allows to query the number > > > of data lanes a MIPI CSI-2 TX is actively transmitting on. > > > > > > Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> > > > --- > > > New in v4. > > > --- > > > include/media/v4l2-subdev.h | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > index 71f1f2f0da53..bb71eedc38f6 100644 > > > --- a/include/media/v4l2-subdev.h > > > +++ b/include/media/v4l2-subdev.h > > > @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc { > > > * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev > > > * can adjust @size to a lower value and must not write more data to the > > > * buffer starting at @data than the original value of @size. > > > + * > > > + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes. > > > */ > > > struct v4l2_subdev_video_ops { > > > int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config); > > > @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops { > > > const struct v4l2_mbus_config *cfg); > > > int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf, > > > unsigned int *size); > > > + int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes); > > > > This shouldn't be a video operation but a pad operation, as a subdev > > could have multiple CSI-2 pads. > > > > Furthermore, you need to define the semantics of this operation more > > precisely. When can it be called, when is the information valid ? Can > > the subdev change the number of lanes it supports at runtime ? If so, > > how are race conditions avoided ? All this needs to be documented. > > > > Finally, the number of lanes is far from being the only information > > about a physical bus that could be interesting for a remote subdev. I > > would much prefer a more generic operation to retrieve bus > > information/configuration, with a data structure that we will be able to > > extend later. > > > > For the record we tried to get those information from the frame > descriptor (the number of used data lanes and the clock > continous/non-continous information to be precise) > > This is the RFC series I sent > https://patchwork.kernel.org/project/linux-media/list/?series=92501 > > Which depends on Sakari's patches: > https://patchwork.kernel.org/patch/10875871/ > https://patchwork.kernel.org/patch/10875873/ > > The latest two were part of a much bigger series that tried to add > support for multiplexed streams. Honestly, it now seems to be that > part could be breakout without involving streams, and re-use that > portion to negotiate the csi-2 bus configuration. I might be wrong > though, and the two parts could not be separate easily (from a very > quick look, after months, it does not seem so). > > In the past I spoke against this solution as I would have preferred > leaving frame_desc alone and introduce a bus configuration operation. > I tried a few times and I ended up implementing g_mbus_format but on > pads and not video. Right now with Sakari's and Laurent's ack I would > say re-using frame_desc might actually work and get use a feature > which is needed by many (cc also Dave, as he had a similar issue with > TC358743 iirc) That looks like it should work just as well. If there is agreement to add the number of data lanes, (non-)continuous clock flag, and possibly the chosen link frequency v4l2_mbus_frame_desc, I'll happily dig up these patches and switch to .get_frame_desc(). regards Philipp
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 71f1f2f0da53..bb71eedc38f6 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -411,6 +411,8 @@ struct v4l2_mbus_frame_desc { * @s_rx_buffer: set a host allocated memory buffer for the subdev. The subdev * can adjust @size to a lower value and must not write more data to the * buffer starting at @data than the original value of @size. + * + * @g_csi_active_lanes: Get number of currently active MIPI CSI-2 data lanes. */ struct v4l2_subdev_video_ops { int (*s_routing)(struct v4l2_subdev *sd, u32 input, u32 output, u32 config); @@ -441,6 +443,7 @@ struct v4l2_subdev_video_ops { const struct v4l2_mbus_config *cfg); int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf, unsigned int *size); + int (*g_csi_active_lanes)(struct v4l2_subdev *sd, u32 *lanes); }; /**
Add a subdevice video operation that allows to query the number of data lanes a MIPI CSI-2 TX is actively transmitting on. Suggested-by: Hans Verkuil <hverkuil@xs4all.nl> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de> --- New in v4. --- include/media/v4l2-subdev.h | 3 +++ 1 file changed, 3 insertions(+)