| Message ID | 20200313144035.401430-3-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | v4l2-subdev: Introduce get_mbus_format pad op | expand |
Hi Jacopo, Thanks for the patch. 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@jmondi.org> > --- > 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. 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. Thanks, -hyun > }; > > /** > -- > 2.25.1 >
Hi Hyun and Jacopo, On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote: > Hi Jacopo, > > Thanks for the patch. > > 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@jmondi.org> > > --- > > 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. 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. > > 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. > > Thanks, > -hyun > > > }; > > > > /** > > -- > > 2.25.1 > >
Hi Jacopo, Thanks for the patchset. "subdev" in the subject. On Fri, Mar 13, 2020 at 03:40:33PM +0100, 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. How is this expected to work with existing drivers that just get their configuration from DT/ACPI? Update to use this API driver by driver as needed basis, or something else? Have you thought about setting the configuration as well? Where is this expected to be implemented? Transmitters only, and to be called by receiver drivers? I think ideally the g_mbus_config video op should go with this patchset, and drivers using it converted. Given the likely small intersection of the two (drivers usign the old video op), this might be possible to do later on as well. But in that case g_mbus_config should be deprecated in documentation. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > 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; unsigned int, please. > +}; > + > +/** > + * 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; This should be unsigned int as well. > +}; > + > +/** > + * 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); > }; > > /**
Hi Niklas, Huyn, On Wed, Apr 08, 2020 at 12:22:55AM +0200, niklas.soderlund+renesas@ragnatech.se wrote: > Hi Hyun and Jacopo, > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote: > > Hi Jacopo, > > > > Thanks for the patch. > > > > 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@jmondi.org> > > > --- > > > 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 ? Thanks j > > Thanks, > > -hyun > > > > > }; > > > > > > /** > > > -- > > > 2.25.1 > > > > > -- > Regards, > Niklas Söderlund
Hi Sakari, On Wed, Apr 08, 2020 at 11:47:04AM +0300, Sakari Ailus wrote: > Hi Jacopo, > > Thanks for the patchset. > > "subdev" in the subject. > > On Fri, Mar 13, 2020 at 03:40:33PM +0100, 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. > > How is this expected to work with existing drivers that just get their > configuration from DT/ACPI? Update to use this API driver by driver as > needed basis, or something else? I think so. The firmware specifies a somehow immutable value depending on the hardware configuration (like the number of CSI-2 data lanes actually wired between a receiver and a transmitter), this operation allow to specify how many of them are actually in use depending on the currently configured mode of operation, if ever required, otherwise the firmware property provided value is used. Dave's use case is slightly different, as clock continuous or non-continuous would be specified in the receiver as a default, but given the ability to switch between to transmitter at run-time, it has to be refreshed by querying the current configuration using this op, similar to the "two parallel sensors sharing the same data lines" I tried to describe in the series' cover letter. > > Have you thought about setting the configuration as well? > Didn't get it, which configuration ? > Where is this expected to be implemented? Transmitters only, and to be > called by receiver drivers? For the use cases at end at the moment, yes. Hyun had the same concern in his reply to this series, but I don't feel like ruling out other usages, as I can't tell at the moment on which busses this could prove useful. > > I think ideally the g_mbus_config video op should go with this patchset, Agreed > and drivers using it converted. Given the likely small intersection of the > two (drivers usign the old video op), this might be possible to do later on > as well. But in that case g_mbus_config should be deprecated in > documentation. I'm fine deprecating the other operation in documentation, but porting old drivers using the video ops should be done on top, otherwise it will take some time and I could only compile test them. Thanks j > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > 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; > > unsigned int, please. > > > +}; > > + > > +/** > > + * 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; > > This should be unsigned int as well. > > > +}; > > + > > +/** > > + * 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); > > }; > > > > /** > > -- > Kind regards, > > Sakari Ailus
Hi Niklas and Jacop, 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@ragnatech.se wrote: > > Hi Hyun and Jacopo, > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote: > > > Hi Jacopo, > > > > > > Thanks for the patch. > > > > > > 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@jmondi.org> > > > > --- > > > > 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, (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? :-) Thanks, -hyun > > Thanks > j > > > > Thanks, > > > -hyun > > > > > > > }; > > > > > > > > /** > > > > -- > > > > 2.25.1 > > > > > > > > -- > > Regards, > > Niklas Söderlund
Hi Huyn, On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote: > Hi Niklas and Jacop, > > 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@ragnatech.se wrote: > > > Hi Hyun and Jacopo, > > > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote: > > > > Hi Jacopo, > > > > > > > > Thanks for the patch. > > > > > > > > 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@jmondi.org> > > > > > --- > > > > > 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. > > Thanks, > -hyun > > > > > Thanks > > j > > > > > > Thanks, > > > > -hyun > > > > > > > > > }; > > > > > > > > > > /** > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > -- > > > Regards, > > > Niklas Söderlund
Hi Niklas! On Fri, 2020-04-10 at 03:24:06 -0700, niklas.soderlund+renesas@ragnatech.se wrote: > Hi Huyn, > > On 2020-04-09 17:30:28 -0700, Hyun Kwon wrote: > > Hi Niklas and Jacop, > > > > 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@ragnatech.se wrote: > > > > Hi Hyun and Jacopo, > > > > > > > > On 2020-04-01 15:30:38 -0700, Hyun Kwon wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thanks for the patch. > > > > > > > > > > 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@jmondi.org> > > > > > > --- > > > > > > 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. Thanks, -hyun
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@ragnatech.se 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@ragnatech.se 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@jmondi.org> > > > > > > > --- > > > > > > > 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.
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@ragnatech.se 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@ragnatech.se 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@jmondi.org> > > > > > > > > --- > > > > > > > > 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
Hi Jacopo, On Wed, Apr 15, 2020 at 09:43:31AM +0200, Jacopo Mondi wrote: > On Wed, Apr 15, 2020 at 05:22:20AM +0300, Laurent Pinchart wrote: > > 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@ragnatech.se 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@ragnatech.se 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@jmondi.org> > >>>>>>>> --- > >>>>>>>> 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. It shouldn't be different, all operations should have their usage properly documented :-) > 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. I don't mind either way, I won't push against this series just because of that, as far as I'm concerned, it's the whole subdev API that's problematic, not documenting use cases here won't make anything worse.
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); }; /**
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@jmondi.org> --- include/media/v4l2-subdev.h | 67 +++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+)