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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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;
> >
--
Kind regards,
Sakari Ailus
