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@xxxxxxxxx>
> > > Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > > ---
> > > 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
