Hi Tomi
On Wed, Mar 01, 2023 at 12:05:45PM +0200, Tomi Valkeinen wrote:
> On 01/03/2023 11:40, Jacopo Mondi wrote:
> > Hi Tomi
> >
> > On Tue, Feb 28, 2023 at 07:16:19PM +0200, Tomi Valkeinen wrote:
> > > CAL uses get_frame_desc to get the VC and DT for the incoming CSI-2
> > > stream, but does it in a bit hacky way. Clean this up by implementing
> > > .get_frame_desc to camera-rx, and calling that from cal.c.
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/media/platform/ti/cal/cal-camerarx.c | 62 +++++++++++---------
> > > drivers/media/platform/ti/cal/cal.c | 30 ++++------
> > > drivers/media/platform/ti/cal/cal.h | 2 -
> > > 3 files changed, 47 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > index 3dfcb276d367..95e0ad59a39b 100644
> > > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > @@ -589,33 +589,6 @@ static int cal_camerarx_parse_dt(struct cal_camerarx *phy)
> > > return ret;
> > > }
> > >
> > > -int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy,
> > > - struct v4l2_mbus_frame_desc *desc)
> > > -{
> > > - struct media_pad *pad;
> > > - int ret;
> > > -
> > > - if (!phy->source)
> > > - return -EPIPE;
> > > -
> > > - pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]);
> > > - if (!pad)
> > > - return -EPIPE;
> > > -
> > > - ret = v4l2_subdev_call(phy->source, pad, get_frame_desc, pad->index,
> > > - desc);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > > - dev_err(phy->cal->dev,
> > > - "Frame descriptor does not describe CSI-2 link");
> > > - return -EINVAL;
> > > - }
> > > -
> > > - return 0;
> > > -}
> > > -
> > > /* ------------------------------------------------------------------
> > > * V4L2 Subdev Operations
> > > * ------------------------------------------------------------------
> > > @@ -772,6 +745,40 @@ static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
> > > return cal_camerarx_sd_set_fmt(sd, state, &format);
> > > }
> > >
> > > +static int cal_camerarx_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > > + struct v4l2_mbus_frame_desc *fd)
> > > +{
> > > + struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > + struct v4l2_mbus_frame_desc remote_desc;
> > > + const struct media_pad *remote_pad;
> > > + int ret;
> > > +
> > > + remote_pad = media_pad_remote_pad_first(&phy->pads[CAL_CAMERARX_PAD_SINK]);
> > > + if (!remote_pad)
> > > + return -EPIPE;
> > > +
> > > + ret = v4l2_subdev_call(phy->source, pad, get_frame_desc,
> > > + remote_pad->index, &remote_desc);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (remote_desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > > + dev_err(phy->cal->dev,
> > > + "Frame descriptor does not describe CSI-2 link");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (remote_desc.num_entries > 1)
> > > + dev_dbg(phy->cal->dev,
> > > + "Multiple streams not supported in remote frame descriptor, using the first one\n");
> >
> > Maybe WARN_ONCE, but doesn't really matter as I presume this will go
> > away in next patch
> >
> > > +
> > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> >
> > Is the bus between CAL and camera-rx a CSI-2 bus ??
>
> No, it's a custom internal bus.
>
> > As this mostly serves to transport to CAL the DT and VC, I guess it's
> > fine
>
> Yes, we use it to transport CSI-2 data, and mainly just the DT and VC.
>
> The SW architecture doesn't really match the HW, but I think it works quite
> fine. E.g. there's really just a single output from each of the PHYs, which
> go to the CAL block (so CAL would have two inputs), and CAL contains a
> processing pipeline, and at the end it has the DMAs and memory connection.
>
> I don't know if trying to design the SW like that would help any, and
> besides, CAL is an old driver so the legacy drag is there. The current
> design is an extension to the older designs.
>
> Here, we could of course drop the .get_frame_desc from camera-rx, and
> implement (more or less identical) function which the cal.c could call. In
> some ways that would be better, but then again, I think the .get_frame_desc
> works here quite nicely, as cal.c can ask the frame desc for a single output
> pad (i.e. the stream which the context in question is interested in).
>
Yeah I was thinking about some helper that explicitly fetch the DT and
VC transported on the camera-rx pad connected to this CAL instance.
But I agree the current design works, and if you're fine with that I
am as well :)
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
>
> Thanks!
>
> Tomi
>
