Re: [PATCH v3 07/15] media: rcar-csi2: Optimize rcsi2_calc_mbps()

[Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>]

Hi,

On 06/06/2025 15:07, Niklas Söderlund wrote:
> Hi Tomi,
> 
> Thanks for your work.
> 
> On 2025-05-30 16:50:36 +0300, Tomi Valkeinen wrote:
>> With modern drivers supporting link-freq, we don't need to do any
>> calculations based on the bpp and number of lanes when figuring out the
>> link frequency. However, the code currently always runs code to get the
>> bpp and number of lanes.
>>
>> Optimize the rcsi2_calc_mbps() so that we only do that when needed, i.e.
>> when querying the link-freq is not supported by the upstream subdevice.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> I wonder if it make sens to add a dev_warn_once() for the old call path 
> so we won't forget to update all known users of this old way, and once 
> fixed we can remove the huristic method all together?

I think something like that should be done in the framework, rather in
each individual driver.

 Tomi

>> ---
>>  drivers/media/platform/renesas/rcar-csi2.c | 50 +++++++++++++++++-------------
>>  1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
>> index 90973f3cba38..e0a0fd96459b 100644
>> --- a/drivers/media/platform/renesas/rcar-csi2.c
>> +++ b/drivers/media/platform/renesas/rcar-csi2.c
>> @@ -1001,15 +1001,10 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
>>  static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>>  			   struct v4l2_subdev_state *state)
>>  {
>> -	const struct rcar_csi2_format *format;
>> -	struct v4l2_mbus_framefmt *fmt;
>>  	struct media_pad *remote_pad;
>>  	struct v4l2_subdev *source;
>> -	unsigned int lanes;
>> -	unsigned int bpp;
>>  	s64 freq;
>>  	u64 mbps;
>> -	int ret;
>>  
>>  	if (!priv->remote)
>>  		return -ENODEV;
>> @@ -1017,28 +1012,41 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>>  	source = priv->remote;
>>  	remote_pad = &source->entity.pads[priv->remote_pad];
>>  
>> -	ret = rcsi2_get_active_lanes(priv, &lanes);
>> -	if (ret)
>> -		return ret;
>> +	/*
>> +	 * First try to get the real link freq. If that fails, try the heuristic
>> +	 * method with bpp and lanes (but that only works for one route).
>> +	 */
>> +	freq = v4l2_get_link_freq(remote_pad, 0, 0);
>> +	if (freq < 0) {
>> +		const struct rcar_csi2_format *format;
>> +		struct v4l2_mbus_framefmt *fmt;
>> +		unsigned int lanes;
>> +		unsigned int bpp;
>> +		int ret;
>>  
>> -	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
>> -	if (!fmt)
>> -		return -EINVAL;
>> +		ret = rcsi2_get_active_lanes(priv, &lanes);
>> +		if (ret)
>> +			return ret;
>>  
>> -	format = rcsi2_code_to_fmt(fmt->code);
>> -	if (!format)
>> -		return -EINVAL;
>> +		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
>> +		if (!fmt)
>> +			return -EINVAL;
>>  
>> -	bpp = format->bpp;
>> +		format = rcsi2_code_to_fmt(fmt->code);
>> +		if (!format)
>> +			return -EINVAL;
>>  
>> -	freq = v4l2_get_link_freq(remote_pad, bpp, 2 * lanes);
>> -	if (freq < 0) {
>> -		int ret = (int)freq;
>> +		bpp = format->bpp;
>>  
>> -		dev_err(priv->dev, "failed to get link freq for %s: %d\n",
>> -			source->name, ret);
>> +		freq = v4l2_get_link_freq(remote_pad, bpp, 2 * lanes);
>> +		if (freq < 0) {
>> +			int ret = (int)freq;
>>  
>> -		return ret;
>> +			dev_err(priv->dev, "failed to get link freq for %s: %d\n",
>> +				source->name, ret);
>> +
>> +			return ret;
>> +		}
>>  	}
>>  
>>  	mbps = div_u64(freq * 2, MEGA);
>>
>> -- 
>> 2.43.0
>>
>