Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Mon, Dec 16, 2024 at 01:16:45PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote:
> > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote:
> > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > > > frequency to the receiving sub-device.
> > > 
> > > The documentation of v4l2_get_link_freq() should be expanded to explain
> > > the new mode of operation. It should document which of the supported
> > > methods are recommended for new drivers.
> > > 
> > > > 
> > > > 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 9fe74c7e064f..88fbd5608f00 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > > @@ -508,13 +508,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 = {};
> > > 
> > > Isn't mbus_config fully populated by the .get_mbus_config() operation ?
> > > That should be documented in the .get_mbus_config() operation
> > > documentation.
> > 
> > It's a good practice to zero the memory before drivers get to work on it. I
> > presume drivers will set the fields that are relevant for them and ignore
> > the rest.
> > 
> > I can add a note on get_mbus_config() the drivers should set all struct
> > fields to known values.
> 
> Thanks.
> 
> > > >  	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);
> > > 
> > > 	if (mbus_config.link_freq)
> > > 		return mbus_config.link_freq;
> > > 
> > > 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > 
> > Whether this would be cleaner is debatable at least. I can switch if you
> > insist though.
> 
> I think it's nicer. You could even write
> 
>  	if (mbus_config.link_freq)
>  		return mbus_config.link_freq;
> 
> 	/*
> 	 * Fall back to using the link frequency control if the media bus config
> 	 * doesn't provide a link frequency.
> 	 */
>  	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

I can use this.

> 
> > > I wonder if we should also add a message in case link_freq is 0, to get
> > > drivers to convert to reporting the link frequency through
> > > .get_mbus_config() if they already implement the operation.
> > 
> > I'm not sure this will be useful: in most cases the LINK_FREQ control is
> > used and for a reason: it's user-configurable. Drivers should only populate
> > the field if the link frequency is determined by the driver. For the
> > receiver drivers it does not matter: they use v4l2_get_link_freq().
> 
> I think this depends on whether or not driver that have a configurable
> link frequency should report the current value through
> .get_mbus_config(). I think that drivers that implement
> .get_mbus_config() should, for consistency.

We should have a helper that obtains information using get_mbus_config() as
well as the fwnode endpoint. I've proposed that a few times over the years.
I'm hoping for someone who needs dynamic configuration e.g. for lane
numbers to implement it. :-)

I wouldn't try to add more burden for drivers on this. Beyond that, it's
common that if you have multiple implementations of something, one of them
(the unused one) eventually breaks. What we really need is to obtain the
information from the sub-device API, using the method the driver uses to
report it.

-- 
Regards,

Sakari Ailus