Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[niklas soderlund <niklas.soderlund@ragnatech.se>]

Hello,

On 2023-02-24 16:48:55 +0100, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > Add routing and stream_config support to CAL driver.
> >
> > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > separate streams at the same time.
> >
> > Add 8 video device nodes, each representing a single dma-engine, and set
> > the number of source pads on camerarx to 8. Each video node can be
> > connected to any of the source pads on either of the camerarx instances
> > using media links. Camerarx internal routing is used to route the
> > incoming CSI-2 streams to one of the 8 source pads.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> >  drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
> >  drivers/media/platform/ti/cal/cal.c          |  65 ++++--
> >  drivers/media/platform/ti/cal/cal.h          |   4 +-
> >  4 files changed, 342 insertions(+), 106 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > index faafbd0e9240..49ae29065cd1 100644
> > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> >  {
> >  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> >  	u32 num_lanes = mipi_csi2->num_data_lanes;
> > -	const struct cal_format_info *fmtinfo;
> >  	struct v4l2_subdev_state *state;
> > -	struct v4l2_mbus_framefmt *fmt;
> >  	u32 bpp;
> >  	s64 freq;
> >
> > -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > +	/*
> > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > +	 *
> > +	 * With multistream input there is no single pixel rate, and thus we
> > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > +	 * V4L2_CID_PIXEL_RATE.
> > +	 */
> 
> To recap a bit of our offline discussion:
> - max9286 GMSL deserializer (as a comparison for a multiplexed
>   transmitter) use PIXEL_RATE to report the cumulative pixel rate of
>   enabled transmitters. This is because the R-Car CSI-2 receiver on
>   which use PIXEL_RATE to compute the link freq [1]
> 
> - according to [2]
>   pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> 
>   from which:
>   link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> 
>   This works as long the reported pixel rate includes visible and
>   blankings, something I'm not sure how many transmitters handle
>   correctly as PIXEL_RATE control is meant to report the visible pixel
>   sampling rate on the pixel array.
> 
> I guess we should go towards mandating LINK_FREQ for transmitters.
> 
> cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()

Thanks for the ping.

The choice to use the PIXEL_RATE instead of the LINK_FREQ control for 
the R-Car CSI-2 was originally because the ADV748x which was the first 
CSI-2 transmitter used during development.

AFIK the ADV748x adjusts the CSI-2 TX link frequency to match the pixel 
clock. This results in quiet a big range of possible values that need to 
be communicated between the two sub devices. The V4L2_CID_LINK_FREQ 
control is a V4L2_CTRL_TYPE_INTEGER_MENU which do not render itself to 
report the large range of values needed.

When we added MAX9286 and friends to the mix, we built on-top of this by 
reporting the total pixel rate of all streams being transmitted on the 
CSI-2 link. IMHO the v4l2_get_link_freq() was an OK middle ground on how 
to align the two use-cases.

I agree that situation is not ideal. And in a perfect world a control 
other then PIXEL_RATE would be used for the R-Car CSI-2 driver, but no 
such control exists. And chancing the control type of LINK_FREQ is not a 
good idea as that is usually specified in as a list in DT.

Adding a new control DYNAMIC_LINK_FREQ and wire that into 
v4l2_get_link_freq() ?

> 
> [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> 

-- 
Kind Regards,
Niklas Söderlund