| Message ID | 20200415105004.2497356-2-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | v4l2-subdev: Introduce get_mbus_format pad op | expand |
Hi Jacopo, Thank you for the patch. On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 69 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index a4848de59852..fc16af578471 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > unsigned short num_entries; > }; > > +/** > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > + * @hsync_active: hsync active state: 1 for high, 0 for low > + * @vsync_active: vsync active state: 1 for high, 0 for low > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling Is this for the driving side or the sampling side ? > + * @data_active: data lines active state: 1 for high, 0 for low I wonder, is there any system with active low data lines ? > + */ > +struct v4l2_mbus_parallel_config { Is this intended to cover BT.656 too ? Either way I think it should be documented. > + unsigned int hsync_active : 1; > + unsigned int vsync_active : 1; > + unsigned int pclk_rising : 1; > + unsigned int data_active : 1; Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is getting deprecated, it doesn't mean we can reuse the same flags if they make sense. Otherwise we'll have to translate between v4l2_fwnode_bus_parallel.flags and the fields here. Ideally v4l2_fwnode_bus_parallel should be replaced with v4l2_mbus_parallel_config (once we add additional fields). > +}; > + > +/** > + * struct v4l2_mbus_csi2_dphy_config - MIPI CSI-2 DPHY mbus configuration s/DPHY/D-PHY/ (same below) > + * @data_lanes: number of data lanes in use > + * @clock_noncontinuous: non continuous clock enable flag: 1 for non > + * continuous clock, 0 for continuous clock. > + */ > +struct v4l2_mbus_csi2_dphy_config { > + unsigned int data_lanes : 3; > + unsigned int clock_noncontinuous : 1; > +}; > + > +/** > + * struct v4l2_mbus_csi2_cphy_config - MIPI CSI-2 CPHY mbus configuration > + * > + * TODO > + */ > +struct v4l2_mbus_csi2_cphy_config { > + /* TODO */ > +}; How about leaving this one out for now as it's empty ? > + > +/** > + * 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. I'd focus here on what the structure contains rather than how it's used, usage belongs to the documentation of the .get_mbus_config() operation. I think the documentation should specify clearly that this is about the physical bus configuration (as it excludes virtual channels on CSI-2 for instance), and should also explain that this is about usage of the physical bus, not just its hardware configuration on the PCB (as the intent is to report the number of CSI-2 D-PHY data lanes actually used, not the number of data lanes present on the board for instance). > + * > + * 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. What is this TODO about ? There's a '_pad_' in the name of this structure. > + * > + * @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 +735,8 @@ 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 I was going to say that this is a bit too short, but then saw patch 3/6 :-) > */ > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > @@ -710,6 +777,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 Laurent, On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 69 insertions(+) > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index a4848de59852..fc16af578471 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > > unsigned short num_entries; > > }; > > > > +/** > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > > + * @hsync_active: hsync active state: 1 for high, 0 for low > > + * @vsync_active: vsync active state: 1 for high, 0 for low > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling > > Is this for the driving side or the sampling side ? Is there a difference? I'd expect the configuration needs to be the same on both sides. > > > + * @data_active: data lines active state: 1 for high, 0 for low > > I wonder, is there any system with active low data lines ? > > > + */ > > +struct v4l2_mbus_parallel_config { > > Is this intended to cover BT.656 too ? Either way I think it should be > documented. I think we should replace what directly refers to Bt.601 with something that refers to that, and not "parallel". Both are parallel after all. I think the naming is in line with that, assuming this covers both. > > > + unsigned int hsync_active : 1; > > + unsigned int vsync_active : 1; > > + unsigned int pclk_rising : 1; > > + unsigned int data_active : 1; > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is I'd think it's easier to work with fields in drivers than the flags. This isn't uAPI so we don't need to think the ABI. The change could also be done to struct v4l2_fwnode_bus_parallel. > getting deprecated, it doesn't mean we can reuse the same flags if they > make sense. Otherwise we'll have to translate between > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally > v4l2_fwnode_bus_parallel should be replaced with > v4l2_mbus_parallel_config (once we add additional fields). ...
Hi Sakari, Laurent, On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote: > Hi Laurent, > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 69 insertions(+) > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > index a4848de59852..fc16af578471 100644 > > > --- a/include/media/v4l2-subdev.h > > > +++ b/include/media/v4l2-subdev.h > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > > > unsigned short num_entries; > > > }; > > > > > > +/** > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > > > + * @hsync_active: hsync active state: 1 for high, 0 for low > > > + * @vsync_active: vsync active state: 1 for high, 0 for low > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling > > > > Is this for the driving side or the sampling side ? > > Is there a difference? I'd expect the configuration needs to be the same on > both sides. I was puzzled as well by this question, mostly because I never found anything like this in the existing documentation, but actually yes, the driving side clocks out data on one edge, sampling side samples on the opposite one ? Is this what you meant Laurent ? To me this has always been about sampling side though, and the setting should match on both endpoints of course. > > > > > > + * @data_active: data lines active state: 1 for high, 0 for low > > > > I wonder, is there any system with active low data lines ? As this is part of the standard DT properties for video interfaces, I added it here. > > > > > + */ > > > +struct v4l2_mbus_parallel_config { > > > > Is this intended to cover BT.656 too ? Either way I think it should be > > documented. > > I think we should replace what directly refers to Bt.601 with something > that refers to that, and not "parallel". Both are parallel after all. I > think the naming is in line with that, assuming this covers both. > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal synch and field flags are specified. This implies the following DT properties are shared between BT.601 and BT.656: - pclk-sample - data-active - slave-mode - bus-width - data-shift - sync-on-green-active - data-enable-active with - hsync-active - vsync-active - field-even-active being BT.601 only. We could do here do the same and mention which flags apply to 601 only, or more clearly split the config structure by keeping a generic 'parallel' flag structure, with a sub-structure which is 601 specific. I'm not sure it's worth the extra layer of indirection though. > > > > > + unsigned int hsync_active : 1; > > > + unsigned int vsync_active : 1; > > > + unsigned int pclk_rising : 1; > > > + unsigned int data_active : 1; > > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is > > I'd think it's easier to work with fields in drivers than the flags. This I find it easier and less error prone to work with fields as well, provided the space occupation is the same as working with flags. > isn't uAPI so we don't need to think the ABI. The change could also be done > to struct v4l2_fwnode_bus_parallel. > > > getting deprecated, it doesn't mean we can reuse the same flags if they > > make sense. Otherwise we'll have to translate between > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally Right, I agree this is not desirable. Every driver should inspect the fwnode properties and translate to this new config when queryed through get_mbus_format. > > v4l2_fwnode_bus_parallel should be replaced with > > v4l2_mbus_parallel_config (once we add additional fields). I like this idea We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel struct v4l2_fwnode_bus_parallel { unsigned int flags; unsigned char bus_width; unsigned char data_shift; }; but then we should replace the whole structure. All in all, working with the same set of flags is the less disruptive change and would not require translations in drivers... I'm not a fan, but currently seems the easiest way forward... What do you think ? > > ... > > -- > Kind regards, > > Sakari Ailus
Hi Jacopo, On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote: > Hi Sakari, Laurent, > > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote: > > Hi Laurent, > > > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 69 insertions(+) > > > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > > index a4848de59852..fc16af578471 100644 > > > > --- a/include/media/v4l2-subdev.h > > > > +++ b/include/media/v4l2-subdev.h > > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > > > > unsigned short num_entries; > > > > }; > > > > > > > > +/** > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > > > > + * @hsync_active: hsync active state: 1 for high, 0 for low > > > > + * @vsync_active: vsync active state: 1 for high, 0 for low > > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling > > > > > > Is this for the driving side or the sampling side ? > > > > Is there a difference? I'd expect the configuration needs to be the same on > > both sides. > > I was puzzled as well by this question, mostly because I never found > anything like this in the existing documentation, but actually yes, > the driving side clocks out data on one edge, sampling side samples on > the opposite one ? Is this what you meant Laurent ? > > To me this has always been about sampling side though, and the setting > should match on both endpoints of course. > > > > > > > > > > + * @data_active: data lines active state: 1 for high, 0 for low > > > > > > I wonder, is there any system with active low data lines ? > > As this is part of the standard DT properties for video interfaces, I > added it here. > > > > > > > > + */ > > > > +struct v4l2_mbus_parallel_config { > > > > > > Is this intended to cover BT.656 too ? Either way I think it should be > > > documented. > > > > I think we should replace what directly refers to Bt.601 with something > > that refers to that, and not "parallel". Both are parallel after all. I > > think the naming is in line with that, assuming this covers both. > > > > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal > synch and field flags are specified. This implies the following DT > properties are shared between BT.601 and BT.656: > - pclk-sample > - data-active > - slave-mode > - bus-width > - data-shift > - sync-on-green-active > - data-enable-active > > with > - hsync-active > - vsync-active > - field-even-active > being BT.601 only. > > We could do here do the same and mention which flags apply to 601 > only, or more clearly split the config structure by keeping a generic > 'parallel' flag structure, with a sub-structure which is 601 specific. > I'm not sure it's worth the extra layer of indirection though. > > > > > > > > > + unsigned int hsync_active : 1; > > > > + unsigned int vsync_active : 1; > > > > + unsigned int pclk_rising : 1; > > > > + unsigned int data_active : 1; > > > > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used > > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is > > > > I'd think it's easier to work with fields in drivers than the flags. This > > I find it easier and less error prone to work with fields as well, > provided the space occupation is the same as working with flags. > > > isn't uAPI so we don't need to think the ABI. The change could also be done > > to struct v4l2_fwnode_bus_parallel. > > > > > getting deprecated, it doesn't mean we can reuse the same flags if they > > > make sense. Otherwise we'll have to translate between > > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally > > Right, I agree this is not desirable. Every driver should inspect the > fwnode properties and translate to this new config when queryed > through get_mbus_format. > > > > v4l2_fwnode_bus_parallel should be replaced with > > > v4l2_mbus_parallel_config (once we add additional fields). > > I like this idea > > We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel > > struct v4l2_fwnode_bus_parallel { > unsigned int flags; > unsigned char bus_width; > unsigned char data_shift; > }; > > but then we should replace the whole structure. > > All in all, working with the same set of flags is the less disruptive > change and would not require translations in drivers... I'm not a fan, > but currently seems the easiest way forward... > > What do you think ? Could we use a struct instead, say: struct v4l2_parallel_flags { unsigned int hsync_active:1; /* and so on */ }; And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No defines would be needed this way, and it'd be slightly safer because you get types checked by the compilter. I don't have strong opinion either way. Both would work just fine.
Hello, On Mon, May 11, 2020 at 11:03:54PM +0300, Sakari Ailus wrote: > On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote: > > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote: > > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote: > > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 69 insertions(+) > > > > > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > > > index a4848de59852..fc16af578471 100644 > > > > > --- a/include/media/v4l2-subdev.h > > > > > +++ b/include/media/v4l2-subdev.h > > > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > > > > > unsigned short num_entries; > > > > > }; > > > > > > > > > > +/** > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > > > > > + * @hsync_active: hsync active state: 1 for high, 0 for low > > > > > + * @vsync_active: vsync active state: 1 for high, 0 for low > > > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling > > > > > > > > Is this for the driving side or the sampling side ? > > > > > > Is there a difference? I'd expect the configuration needs to be the same on > > > both sides. > > > > I was puzzled as well by this question, mostly because I never found > > anything like this in the existing documentation, but actually yes, > > the driving side clocks out data on one edge, sampling side samples on > > the opposite one ? Is this what you meant Laurent ? Yes, that's what I meant, sorry for the confusion. > > To me this has always been about sampling side though, and the setting > > should match on both endpoints of course. Can we make it explicit ? How about naming the field pclk_sample_edge, and adding macros name *_RISING and *_FALLING ? See include/drm/drm_connector.h for an example (DRM_BUS_FLAG_PIXDATA_*). > > > > > + * @data_active: data lines active state: 1 for high, 0 for low > > > > > > > > I wonder, is there any system with active low data lines ? > > > > As this is part of the standard DT properties for video interfaces, I > > added it here. > > > > > > > + */ > > > > > +struct v4l2_mbus_parallel_config { > > > > > > > > Is this intended to cover BT.656 too ? Either way I think it should be > > > > documented. > > > > > > I think we should replace what directly refers to Bt.601 with something > > > that refers to that, and not "parallel". Both are parallel after all. I > > > think the naming is in line with that, assuming this covers both. > > > > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal > > synch and field flags are specified. This implies the following DT > > properties are shared between BT.601 and BT.656: > > - pclk-sample > > - data-active > > - slave-mode > > - bus-width > > - data-shift > > - sync-on-green-active Isn't this a property of analog signals ? > > - data-enable-active Does BT.656 have a data enable signal ? > > > > with > > - hsync-active > > - vsync-active > > - field-even-active > > being BT.601 only. > > > > We could do here do the same and mention which flags apply to 601 > > only, or more clearly split the config structure by keeping a generic > > 'parallel' flag structure, with a sub-structure which is 601 specific. > > I'm not sure it's worth the extra layer of indirection though. Possibly not, I would be fine with just documenting the structure and fields in details. > > > > > + unsigned int hsync_active : 1; > > > > > + unsigned int vsync_active : 1; > > > > > + unsigned int pclk_rising : 1; > > > > > + unsigned int data_active : 1; > > > > > > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used > > > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is > > > > > > I'd think it's easier to work with fields in drivers than the flags. This > > > > I find it easier and less error prone to work with fields as well, > > provided the space occupation is the same as working with flags. I'm probably influenced by DRM_BUS_FLAG_* here :-) Especially for the signal polarity, being able to say ->flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; in the driver the transmitter driver, and if (->flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE) ... in the receiver driver is very nice. I won't push for it though. > > > isn't uAPI so we don't need to think the ABI. The change could also be done > > > to struct v4l2_fwnode_bus_parallel. > > > > > > > getting deprecated, it doesn't mean we can reuse the same flags if they > > > > make sense. Otherwise we'll have to translate between > > > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally > > > > Right, I agree this is not desirable. Every driver should inspect the > > fwnode properties and translate to this new config when queryed > > through get_mbus_format. Do you mean with a helper function ? > > > > v4l2_fwnode_bus_parallel should be replaced with > > > > v4l2_mbus_parallel_config (once we add additional fields). > > > > I like this idea > > > > We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel > > > > struct v4l2_fwnode_bus_parallel { > > unsigned int flags; > > unsigned char bus_width; > > unsigned char data_shift; > > }; > > > > but then we should replace the whole structure. > > > > All in all, working with the same set of flags is the less disruptive > > change and would not require translations in drivers... I'm not a fan, > > but currently seems the easiest way forward... > > > > What do you think ? > > Could we use a struct instead, say: > > struct v4l2_parallel_flags { > unsigned int hsync_active:1; > /* and so on */ > }; > > And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No > defines would be needed this way, and it'd be slightly safer because you > get types checked by the compilter. > > I don't have strong opinion either way. Both would work just fine. That's fine with me. As I wrote above I think flags can increase readability in some cases, but I won't insist.
Hi Laurent, Sakari, On Mon, May 11, 2020 at 11:21:26PM +0300, Laurent Pinchart wrote: > Hello, > > On Mon, May 11, 2020 at 11:03:54PM +0300, Sakari Ailus wrote: > > On Mon, May 11, 2020 at 01:32:39PM +0200, Jacopo Mondi wrote: > > > On Wed, Apr 29, 2020 at 04:54:30PM +0300, Sakari Ailus wrote: > > > > On Mon, Apr 20, 2020 at 04:44:57AM +0300, Laurent Pinchart wrote: > > > > > On Wed, Apr 15, 2020 at 12:49:58PM +0200, 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 | 69 +++++++++++++++++++++++++++++++++++++ > > > > > > 1 file changed, 69 insertions(+) > > > > > > > > > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > > > > index a4848de59852..fc16af578471 100644 > > > > > > --- a/include/media/v4l2-subdev.h > > > > > > +++ b/include/media/v4l2-subdev.h > > > > > > @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { > > > > > > unsigned short num_entries; > > > > > > }; > > > > > > > > > > > > +/** > > > > > > + * struct v4l2_mbus_parallel_config - parallel mbus configuration > > > > > > + * @hsync_active: hsync active state: 1 for high, 0 for low > > > > > > + * @vsync_active: vsync active state: 1 for high, 0 for low > > > > > > + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling > > > > > > > > > > Is this for the driving side or the sampling side ? > > > > > > > > Is there a difference? I'd expect the configuration needs to be the same on > > > > both sides. > > > > > > I was puzzled as well by this question, mostly because I never found > > > anything like this in the existing documentation, but actually yes, > > > the driving side clocks out data on one edge, sampling side samples on > > > the opposite one ? Is this what you meant Laurent ? > > Yes, that's what I meant, sorry for the confusion. > > > > To me this has always been about sampling side though, and the setting > > > should match on both endpoints of course. > > Can we make it explicit ? How about naming the field pclk_sample_edge, > and adding macros name *_RISING and *_FALLING ? See > include/drm/drm_connector.h for an example (DRM_BUS_FLAG_PIXDATA_*). > Please note that if we want to reuse V4L2_MBUS_* flags, there are already V4L2_MBUS_PCLK_SAMPLE_RISING and V4L2_MBUS_PCLK_SAMPLE_FALLING And yes, there's "SAMPLE" in the name :) > > > > > > + * @data_active: data lines active state: 1 for high, 0 for low > > > > > > > > > > I wonder, is there any system with active low data lines ? > > > > > > As this is part of the standard DT properties for video interfaces, I > > > added it here. > > > > > > > > > + */ > > > > > > +struct v4l2_mbus_parallel_config { > > > > > > > > > > Is this intended to cover BT.656 too ? Either way I think it should be > > > > > documented. > > > > > > > > I think we should replace what directly refers to Bt.601 with something > > > > that refers to that, and not "parallel". Both are parallel after all. I > > > > think the naming is in line with that, assuming this covers both. > > > > > > Currently in v4l2-fwnode BT.656 is selected if no vertical/horizontal > > > synch and field flags are specified. This implies the following DT > > > properties are shared between BT.601 and BT.656: > > > - pclk-sample > > > - data-active > > > - slave-mode > > > - bus-width > > > - data-shift > > > - sync-on-green-active > > Isn't this a property of analog signals ? > > > > - data-enable-active > > Does BT.656 have a data enable signal ? > I don't think so, but onle the following three items are listed as "BT.601" in v4l2-mediabus.h. I'll update it > > > > > > with > > > - hsync-active > > > - vsync-active > > > - field-even-active > > > being BT.601 only. > > > > > > We could do here do the same and mention which flags apply to 601 > > > only, or more clearly split the config structure by keeping a generic > > > 'parallel' flag structure, with a sub-structure which is 601 specific. > > > I'm not sure it's worth the extra layer of indirection though. > > Possibly not, I would be fine with just documenting the structure and > fields in details. > I think it depends on the last point here below > > > > > > + unsigned int hsync_active : 1; > > > > > > + unsigned int vsync_active : 1; > > > > > > + unsigned int pclk_rising : 1; > > > > > > + unsigned int data_active : 1; > > > > > > > > > > Shouldn't we reuse the V4L2_MBUS_* flags, given that they're also used > > > > > in v4l2_fwnode_bus_parallel ? While the v4l2_mbus_config structure is > > > > > > > > I'd think it's easier to work with fields in drivers than the flags. This > > > > > > I find it easier and less error prone to work with fields as well, > > > provided the space occupation is the same as working with flags. > > I'm probably influenced by DRM_BUS_FLAG_* here :-) Especially for the > signal polarity, being able to say > > ->flags = DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; > > in the driver the transmitter driver, and > > if (->flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE) > ... > > in the receiver driver is very nice. I won't push for it though. > > > > > isn't uAPI so we don't need to think the ABI. The change could also be done > > > > to struct v4l2_fwnode_bus_parallel. > > > > > > > > > getting deprecated, it doesn't mean we can reuse the same flags if they > > > > > make sense. Otherwise we'll have to translate between > > > > > v4l2_fwnode_bus_parallel.flags and the fields here. Ideally > > > > > > Right, I agree this is not desirable. Every driver should inspect the > > > fwnode properties and translate to this new config when queryed > > > through get_mbus_format. > > Do you mean with a helper function ? > > > > > > v4l2_fwnode_bus_parallel should be replaced with > > > > > v4l2_mbus_parallel_config (once we add additional fields). > > > > > > I like this idea > > > > > > We could indeed expand the .flags structure of v4l2_fwnode_bus_parallel > > > > > > struct v4l2_fwnode_bus_parallel { > > > unsigned int flags; > > > unsigned char bus_width; > > > unsigned char data_shift; > > > }; > > > > > > but then we should replace the whole structure. > > > > > > All in all, working with the same set of flags is the less disruptive > > > change and would not require translations in drivers... I'm not a fan, > > > but currently seems the easiest way forward... > > > > > > What do you think ? > > > > Could we use a struct instead, say: > > > > struct v4l2_parallel_flags { > > unsigned int hsync_active:1; > > /* and so on */ > > }; > > > > And then you'd add this to struct v4l2_fwnode_bus_parallel as a field. No > > defines would be needed this way, and it'd be slightly safer because you > > get types checked by the compilter. > > > > I don't have strong opinion either way. Both would work just fine. > > That's fine with me. As I wrote above I think flags can increase > readability in some cases, but I won't insist. I'm really debated. Fields are easier to work with for someone and safer. Flags are easier to work with for some else, and we have a ton of code that uses flags, including fwnode parsing, from which I expect most of the information reported by get_mbus_config() come from. As suggested by Laurent we could provide an helper to translate from flags collected in "struct v4l2_fwnode_bus_parallel.flags" to "struct v4l2_parallel_flags" but I'm not sure it is worth the effort... All in all, I prefer flags, but the fact flags are used basically everywhere makes me lean towards re-using V4L2_MBUS_* everywhere I could... v3 on its way Thanks j > > -- > Regards, > > Laurent Pinchart
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index a4848de59852..fc16af578471 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -350,6 +350,71 @@ struct v4l2_mbus_frame_desc { unsigned short num_entries; }; +/** + * struct v4l2_mbus_parallel_config - parallel mbus configuration + * @hsync_active: hsync active state: 1 for high, 0 for low + * @vsync_active: vsync active state: 1 for high, 0 for low + * @pclk_rising: pixel clock active edge: 1 for rising, 0 for falling + * @data_active: data lines active state: 1 for high, 0 for low + */ +struct v4l2_mbus_parallel_config { + unsigned int hsync_active : 1; + unsigned int vsync_active : 1; + unsigned int pclk_rising : 1; + unsigned int 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: 1 for non + * continuous clock, 0 for continuous clock. + */ +struct v4l2_mbus_csi2_dphy_config { + unsigned int data_lanes : 3; + unsigned int 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 +735,8 @@ 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 +777,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 | 69 +++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)