Hello,
On Wed, Apr 15, 2020 at 05:22:20AM +0300, Laurent Pinchart wrote:
> Hi Huyn,
>
> On Fri, Apr 10, 2020 at 12:26:26PM -0700, Hyun Kwon wrote:
> > On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@xxxxxxxxxxxx wrote:
> > > On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote:
> > > > On Thu, 2020-04-09 at 00:35:07 -0700, Jacopo Mondi wrote:
> > > > > Hi Niklas, Huyn,
> > > > > On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@xxxxxxxxxxxx wrote:
> > > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote:
> > > > > > > On Fri, 2020-03-13 at 07:40:33 -0700, Jacopo Mondi wrote:
> > > > > > > > Introduce a new pad operation to allow retrieving the media bus
> > > > > > > > configuration on a subdevice pad.
> > > > > > > >
> > > > > > > > The newly introduced operation reassembles the s/g_mbus_config video
> > > > > > > > operation, which have been on their way to be deprecated since a long
> > > > > > > > time.
> > > > > > > >
> > > > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++
> > > > > > > > 1 file changed, 67 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > > > > > > index 761aa83a3f3c..3a1afc00e094 100644
> > > > > > > > --- a/include/media/v4l2-subdev.h
> > > > > > > > +++ b/include/media/v4l2-subdev.h
> > > > > > > > @@ -350,6 +350,70 @@ struct v4l2_mbus_frame_desc {
> > > > > > > > unsigned short num_entries;
> > > > > > > > };
> > > > > > > >
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration
> > > > > > > > + * @hsync_active: hsync active state: true for high, false for low
> > > > > > > > + * @vsync_active: vsync active state: true for high, false for low
> > > > > > > > + * @pclk_rising: pixel clock active edge: true for rising, false for falling
> > > > > > > > + * @data_active: data lines active state: true for high, false for low
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_parallel_config {
> > > > > > > > + bool hsync_active : 1;
> > > > > > > > + bool vsync_active : 1;
> > > > > > > > + bool pclk_rising : 1;
> > > > > > > > + bool data_active : 1;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration
> > > > > > > > + * @data_lanes: number of data lanes in use
> > > > > > > > + * @clock_noncontinuous: non continuous clock enable flag
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_csi2_dphy_config {
> > > > > > > > + unsigned int data_lanes : 3;
> > > > > > > > + bool clock_noncontinuous : 1;
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration
> > > > > > > > + *
> > > > > > > > + * TODO
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_csi2_cphy_config {
> > > > > > > > + /* TODO */
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +/**
> > > > > > > > + * struct v4l2_mbus_pad_config - media bus configuration
> > > > > > > > + *
> > > > > > > > + * Report the subdevice media bus information to inform the caller of the
> > > > > > > > + * current bus configuration. The structure describes bus configuration
> > > > > > > > + * parameters that might change in-between streaming sessions, in order to allow
> > > > > > > > + * the caller to adjust its media bus configuration to match what is reported
> > > > > > > > + * here.
> > > > > > > > + *
> > > > > > > > + * TODO: add '_pad_' to the name to distinguish this from the structure
> > > > > > > > + * defined in v4l2_mediabus.h used for the same purpose by the g/s_mbus_config
> > > > > > > > + * video operations. Reuse the there defined enum v4l2_mbus_type to define
> > > > > > > > + * the bus type.
> > > > > > > > + *
> > > > > > > > + * @type: mbus type. See &enum v4l2_mbus_type
> > > > > > > > + * @parallel: parallel bus configuration parameters.
> > > > > > > > + * See &struct v4l2_mbus_parallel_config
> > > > > > > > + * @csi2_dphy: MIPI CSI-2 DPHY configuration parameters
> > > > > > > > + * See &struct v4l2_mbus_csi2_dphy_config
> > > > > > > > + * @csi2_cphy: MIPI CSI-2 CPHY configuration parameters
> > > > > > > > + * See &struct v4l2_mbus_csi2_cphy_config
> > > > > > > > + */
> > > > > > > > +struct v4l2_mbus_pad_config {
> > > > > > > > + enum v4l2_mbus_type type;
> > > > > > > > + union {
> > > > > > > > + struct v4l2_mbus_parallel_config parallel;
> > > > > > > > + struct v4l2_mbus_csi2_dphy_config csi2_dphy;
> > > > > > > > + struct v4l2_mbus_csi2_cphy_config csi2_cphy;
> > > > > > > > + };
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > /**
> > > > > > > > * struct v4l2_subdev_video_ops - Callbacks used when v4l device was opened
> > > > > > > > * in video mode.
> > > > > > > > @@ -670,6 +734,7 @@ struct v4l2_subdev_pad_config {
> > > > > > > > *
> > > > > > > > * @set_frame_desc: set the low level media bus frame parameters, @fd array
> > > > > > > > * may be adjusted by the subdev driver to device capabilities.
> > > > > > > > + * @get_mbus_config: get the current mbus configuration
> > > > > > > > */
> > > > > > > > struct v4l2_subdev_pad_ops {
> > > > > > > > int (*init_cfg)(struct v4l2_subdev *sd,
> > > > > > > > @@ -710,6 +775,8 @@ struct v4l2_subdev_pad_ops {
> > > > > > > > struct v4l2_mbus_frame_desc *fd);
> > > > > > > > int (*set_frame_desc)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > > struct v4l2_mbus_frame_desc *fd);
> > > > > > > > + int (*get_mbus_config)(struct v4l2_subdev *sd, unsigned int pad,
> > > > > > > > + struct v4l2_mbus_pad_config *config);
> > > > > > >
> > > > > > > Because this can be used in many different ways, there's more chance it can
> > > > > > > be misused. That means, drivers call this in different locations, ex probe,
> > > > > > > get format, start stream,,,, and on differnt pads, src or sink. So imagine
> > > > > > > one set of drivers call on sink pad, and the other set call on source pad.
> > > > > > > It works well only until those are mixed together.
> > > > >
> > > > > I don't think we can right now establish all possible use cases, or
> > > > > prevent people from shooting in their foot, moreover, the 'right'
> > > > > usage really depends on the bus in use, and I can't tell where this is
> > > > > will be used in the wild...
> > > > >
> > > > > > That subdevice operations can be called at both probe and s_stream() is
> > > > > > nothing new, I don't thin this is a new problem. But I agree maybe we
> > > > > > could limit get_mbus_config() in the core to only be valid four source
> > > > > > pads? Apart from this open question I think this patch looks good.
> > > > >
> > > > > I'm a bit skeptical on limiting this to source pads as, again, this
> > > > > really depends on the bus on which this operation is used. For my
> > > > > limited knowledge, yes, the use case is always the receiver quering
> > > > > the transmitter, but I don't feel like ruling out the opposite.
> > > > >
> > > > > > > So wouldn't it be better to put some restrictions? One is to document
> > > > > > > recommendations. I think this better be called in stream on because
> > > > > > > some bus config may change at runtime depending on other configuration.
> > > > > > > So any bus config prior to stream-on may be outdated. The other is to
> > > > > > > enforce in the code. Some, but maybe not all, can be handled in
> > > > > > > v4l2_subdev_call_pad_wrappers, for example allowing this call only on
> > > > > > > source pad.
> > > > >
> > > > > I hear your concern, but I think it really depends on the use cases
> > > > > and I would have an hard time to provide recommendations that
> > > > > address all use cases.
> > > > >
> > > > > Is your concern due to some mis-uses example you can describe ?
> > > >
> > > > Yeah, while trying this out, I was thinking how it should be used. I ended
> > > > up with a specific way: single direction, starting from stream on,
> > > >
> > > > (streamon) -> max9286 -> (g_mbus_conf) -> max96705 -> (g_mbus_conf) -> sensor1
> > > >
> > > > It's this way because max96705 doesn't have own vsync polarity, and it
> > > > should get it from the connected sensor. While someone may implement the same
> > > > in complete opposite direction for another set of drivers, starting from
> > > > sensor, or even in different entry point,
> > >
> > > I agree with the use-case above.
> > >
> > > >
> > > > (s_fmt) -> sensor2 -> (g_mbus_conf) -> some_ser -> (g_mbus_conf) -> some_des
> > > >
> > > > When the sensor2 driver is used with max96705 above, there could be a problem
> > > > such as circular calls or getting an outdated value. And it is harder to fix at
> > > > that point. So I thought enforcing the direction works for current use cases
> > > > (under my visilbity), and may help avoid such issue in future. Probably it may
> > > > be just me over-thinking, as it sounds like? :-)
> > >
> > > If a format is set on a subdevice we are operating on a subdevice that
> > > is part of a media device right? If so shall setting the format of the
> > > different entities of the graph involve cross entry calls? Shall not the
> > > entire pipeline format be validated at stream_start() and that is the
> > > time g_mbus_conf() would be involved like in the first case above. I
> > > might have misunderstood something if so I apologize.
> >
> > In this patch, it's fully up to driver implementation, so it's legitimate
> > if some driver decides to call that in subdev set format and call another
> > get_mbus_config() within it.
>
> I share some of the concern that was expressed on this topic. The V4L2
> subdev in-kernel API is full of operations that are specified as
> generic, and are called at different times and different ways by
> different drivers. The different subdevs then implement those operations
> differently, as they're only tested with one or a small subset of V4L2
> drivers, and we end up with subdevs that implement different and
> incompatible behaviours. I think the use cases need to be considered
> here, and we should specify the usage of this API in details.
>
I'm a bit skeptical on the fact we can rule out all usage cases and
properyl capture them with a comment, but if this is felt like a
pressing matter we could add a few hints. I wonder why this operation
is different than the othera kapi-only pad operations we have
already.
If you all agree this should be limited to fetching information from
source pads (which for all the use cases I know of is true, but I
don't know all the possible use cases) this can be captured, but I
would have an hard time imposing when this should be used, as each
bus/driver is different in requirements and implementation.
> --
> Regards,
>
> Laurent Pinchart
