| Message ID | 20240516122539.30787-3-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use V4L2 mbus config for conveying MEI CSI link frequency | expand |
On Thu, May 16, 2024 at 03:25:37PM GMT, Sakari Ailus wrote: > Add link_freq field to struct v4l2_mbus_config in order to pass the link > frequency to the receiving sub-device. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > include/media/v4l2-mediabus.h | 2 ++ > 2 files changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index 01650aed7c30..ff859a340c5d 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -506,13 +506,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > unsigned int div) > { > + struct v4l2_mbus_config mbus_config = {}; > struct v4l2_subdev *sd; > + int ret; > > sd = media_entity_to_v4l2_subdev(pad->entity); > - if (!sd) > - return -ENODEV; > + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index, > + &mbus_config); > + if (ret < 0 && ret != -ENOIOCTLCMD) > + return ret; Should we do like what we did with endpoint matching ? (let alone the fact we then backtracked on that.. :) WARN to encourage tx drivers to implement get_mbus_config and advertise the link freq through it ? > > - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > + return mbus_config.link_freq ?: > + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > } > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > #endif /* CONFIG_MEDIA_CONTROLLER */ > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > index 5bce6e423e94..cc5f776dc662 100644 > --- a/include/media/v4l2-mediabus.h > +++ b/include/media/v4l2-mediabus.h > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > /** > * struct v4l2_mbus_config - media bus configuration > * @type: interface type > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > * @bus: bus configuration data structure > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > * Used if the bus is parallel or BT.656. > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > */ > struct v4l2_mbus_config { > enum v4l2_mbus_type type; > + u64 link_freq; I will retaliate that link_freq has different meaning for serial and parallel busses, and it would feel more natural having something like mipi_csi2.link_freq parallel.pixel_clock or do you think it's an overkill ? > union { > struct v4l2_mbus_config_parallel parallel; > struct v4l2_mbus_config_mipi_csi1 mipi_csi1; > -- > 2.39.2 >
Hi Jacopo, On Fri, May 17, 2024 at 12:31:43PM +0200, Jacopo Mondi wrote: > On Thu, May 16, 2024 at 03:25:37PM GMT, Sakari Ailus wrote: > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > frequency to the receiving sub-device. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > include/media/v4l2-mediabus.h | 2 ++ > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > index 01650aed7c30..ff859a340c5d 100644 > > --- a/drivers/media/v4l2-core/v4l2-common.c > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > @@ -506,13 +506,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > unsigned int div) > > { > > + struct v4l2_mbus_config mbus_config = {}; > > struct v4l2_subdev *sd; > > + int ret; > > > > sd = media_entity_to_v4l2_subdev(pad->entity); > > - if (!sd) > > - return -ENODEV; > > + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index, > > + &mbus_config); > > + if (ret < 0 && ret != -ENOIOCTLCMD) > > + return ret; > > Should we do like what we did with endpoint matching ? (let alone the > fact we then backtracked on that.. :) Hmm. What exactly are you suggesting? > > WARN to encourage tx drivers to implement get_mbus_config and > advertise the link freq through it ? Why? If the value is conveyed by the control, there's no reason to copy it here, is it? > > > > > - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > + return mbus_config.link_freq ?: > > + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > } > > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > > index 5bce6e423e94..cc5f776dc662 100644 > > --- a/include/media/v4l2-mediabus.h > > +++ b/include/media/v4l2-mediabus.h > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > > /** > > * struct v4l2_mbus_config - media bus configuration > > * @type: interface type > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > > * @bus: bus configuration data structure > > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > > * Used if the bus is parallel or BT.656. > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > > */ > > struct v4l2_mbus_config { > > enum v4l2_mbus_type type; > > + u64 link_freq; > > I will retaliate that link_freq has different meaning for serial and > parallel busses, and it would feel more natural having something like > > mipi_csi2.link_freq > parallel.pixel_clock > > or do you think it's an overkill ? How is the meaning different? The value reflects the frequency on both buses. > > > union { > > struct v4l2_mbus_config_parallel parallel; > > struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
Hi Sakari On Tue, May 28, 2024 at 06:09:12AM GMT, Sakari Ailus wrote: > Hi Jacopo, > > On Fri, May 17, 2024 at 12:31:43PM +0200, Jacopo Mondi wrote: > > On Thu, May 16, 2024 at 03:25:37PM GMT, Sakari Ailus wrote: > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > > frequency to the receiving sub-device. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > > include/media/v4l2-mediabus.h | 2 ++ > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > index 01650aed7c30..ff859a340c5d 100644 > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > @@ -506,13 +506,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > > unsigned int div) > > > { > > > + struct v4l2_mbus_config mbus_config = {}; > > > struct v4l2_subdev *sd; > > > + int ret; > > > > > > sd = media_entity_to_v4l2_subdev(pad->entity); > > > - if (!sd) > > > - return -ENODEV; > > > + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index, > > > + &mbus_config); > > > + if (ret < 0 && ret != -ENOIOCTLCMD) > > > + return ret; > > > > Should we do like what we did with endpoint matching ? (let alone the > > fact we then backtracked on that.. :) > > Hmm. What exactly are you suggesting? > See below > > > > WARN to encourage tx drivers to implement get_mbus_config and > > advertise the link freq through it ? > > Why? If the value is conveyed by the control, there's no reason to copy it > here, is it? > My understanding is that using get_mbus_config() is preferred, but yes, if the control is in place the same value should be propagated through both path, which is probably a biy silly, yeah. Ok, ignore the suggestion then > > > > > > > > - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > + return mbus_config.link_freq ?: > > > + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > } > > > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > > > index 5bce6e423e94..cc5f776dc662 100644 > > > --- a/include/media/v4l2-mediabus.h > > > +++ b/include/media/v4l2-mediabus.h > > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > > > /** > > > * struct v4l2_mbus_config - media bus configuration > > > * @type: interface type > > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > > > * @bus: bus configuration data structure > > > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > > > * Used if the bus is parallel or BT.656. > > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > > > */ > > > struct v4l2_mbus_config { > > > enum v4l2_mbus_type type; > > > + u64 link_freq; > > > > I will retaliate that link_freq has different meaning for serial and > > parallel busses, and it would feel more natural having something like > > > > mipi_csi2.link_freq > > parallel.pixel_clock > > > > or do you think it's an overkill ? > > How is the meaning different? The value reflects the frequency on both > buses. The meaning is slightly different. For a parallel bus the "link_freq" is actually the pixel clock (and thus "link_freq" is an ill-defined concept for parallel busses ?). For serial busses there actually is a "link frequency" which corresponds to the clock lane frequency. In general, however we define it, the link_freq is a bus property and feels better placed inside the per-bus structures to me. Up to you > > > > > > union { > > > struct v4l2_mbus_config_parallel parallel; > > > struct v4l2_mbus_config_mipi_csi1 mipi_csi1; > > -- > Kind regards, > > Sakari Ailus
Hi Jacopo, On Tue, May 28, 2024 at 09:16:06AM +0200, Jacopo Mondi wrote: > Hi Sakari > > On Tue, May 28, 2024 at 06:09:12AM GMT, Sakari Ailus wrote: > > Hi Jacopo, > > > > On Fri, May 17, 2024 at 12:31:43PM +0200, Jacopo Mondi wrote: > > > On Thu, May 16, 2024 at 03:25:37PM GMT, Sakari Ailus wrote: > > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link > > > > frequency to the receiving sub-device. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- > > > > include/media/v4l2-mediabus.h | 2 ++ > > > > 2 files changed, 10 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > > > > index 01650aed7c30..ff859a340c5d 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-common.c > > > > +++ b/drivers/media/v4l2-core/v4l2-common.c > > > > @@ -506,13 +506,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); > > > > s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, > > > > unsigned int div) > > > > { > > > > + struct v4l2_mbus_config mbus_config = {}; > > > > struct v4l2_subdev *sd; > > > > + int ret; > > > > > > > > sd = media_entity_to_v4l2_subdev(pad->entity); > > > > - if (!sd) > > > > - return -ENODEV; > > > > + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index, > > > > + &mbus_config); > > > > + if (ret < 0 && ret != -ENOIOCTLCMD) > > > > + return ret; > > > > > > Should we do like what we did with endpoint matching ? (let alone the > > > fact we then backtracked on that.. :) > > > > Hmm. What exactly are you suggesting? > > > > See below > > > > > > > WARN to encourage tx drivers to implement get_mbus_config and > > > advertise the link freq through it ? > > > > Why? If the value is conveyed by the control, there's no reason to copy it > > here, is it? > > > > My understanding is that using get_mbus_config() is preferred, but > yes, if the control is in place the same value should be propagated > through both path, which is probably a biy silly, yeah. The problem is that few drivers implement get_mbus_config() and they actually shouldn't if the configuration is fixed. I've actually thought of adding a helper to obtain the information in struct v4l2_mbus_config from the V4L2 fwnode endpoint but it's not implemented. > > Ok, ignore the suggestion then > > > > > > > > > > > > - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > > + return mbus_config.link_freq ?: > > > > + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); > > > > } > > > > EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); > > > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > > > > index 5bce6e423e94..cc5f776dc662 100644 > > > > --- a/include/media/v4l2-mediabus.h > > > > +++ b/include/media/v4l2-mediabus.h > > > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type { > > > > /** > > > > * struct v4l2_mbus_config - media bus configuration > > > > * @type: interface type > > > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. > > > > * @bus: bus configuration data structure > > > > * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. > > > > * Used if the bus is parallel or BT.656. > > > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type { > > > > */ > > > > struct v4l2_mbus_config { > > > > enum v4l2_mbus_type type; > > > > + u64 link_freq; > > > > > > I will retaliate that link_freq has different meaning for serial and > > > parallel busses, and it would feel more natural having something like > > > > > > mipi_csi2.link_freq > > > parallel.pixel_clock > > > > > > or do you think it's an overkill ? > > > > How is the meaning different? The value reflects the frequency on both > > buses. > > The meaning is slightly different. For a parallel bus the "link_freq" > is actually the pixel clock (and thus "link_freq" is an ill-defined concept > for parallel busses ?). For serial busses there actually is a "link > frequency" which corresponds to the clock lane frequency. It's not different on parallel buses: there's a clock signal, too, and the data lines do use the same frequency. The frequency of that clock is the link frequency. The pixel clock can be different still as multiple samples on the bus may be required to obtain a single pixel. > > In general, however we define it, the link_freq is a bus property and > feels better placed inside the per-bus structures to me. > > Up to you > > > > > > > > > > union { > > > > struct v4l2_mbus_config_parallel parallel; > > > > struct v4l2_mbus_config_mipi_csi1 mipi_csi1; > >
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 01650aed7c30..ff859a340c5d 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -506,13 +506,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl); s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul, unsigned int div) { + struct v4l2_mbus_config mbus_config = {}; struct v4l2_subdev *sd; + int ret; sd = media_entity_to_v4l2_subdev(pad->entity); - if (!sd) - return -ENODEV; + ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index, + &mbus_config); + if (ret < 0 && ret != -ENOIOCTLCMD) + return ret; - return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); + return mbus_config.link_freq ?: + __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div); } EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad); #endif /* CONFIG_MEDIA_CONTROLLER */ diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h index 5bce6e423e94..cc5f776dc662 100644 --- a/include/media/v4l2-mediabus.h +++ b/include/media/v4l2-mediabus.h @@ -148,6 +148,7 @@ enum v4l2_mbus_type { /** * struct v4l2_mbus_config - media bus configuration * @type: interface type + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control. * @bus: bus configuration data structure * @bus.parallel: embedded &struct v4l2_mbus_config_parallel. * Used if the bus is parallel or BT.656. @@ -162,6 +163,7 @@ enum v4l2_mbus_type { */ struct v4l2_mbus_config { enum v4l2_mbus_type type; + u64 link_freq; union { struct v4l2_mbus_config_parallel parallel; struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
Add link_freq field to struct v4l2_mbus_config in order to pass the link frequency to the receiving sub-device. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++--- include/media/v4l2-mediabus.h | 2 ++ 2 files changed, 10 insertions(+), 3 deletions(-)