Hi Niklas
On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > Hi Niklas
> >
> > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your work.
> > >
> > > On 2024-05-03 17:51:22 +0200, Jacopo Mondi wrote:
> > > > The adv748x-csi2 driver configures the CSI-2 transmitter to
> > > > automatically infer the image stream format from the connected
> > > > frontend (HDMI or AFE).
> > > >
> > > > Setting a new format on the subdevice hence does not actually control
> > > > the CSI-2 output format, but it's only there for the purpose of
> > > > pipeline validation.
> > > >
> > > > However, there is currently no validation that the supplied media bus
> > > > code is valid and supported by the device.
> > > >
> > > > With the introduction of enum_mbus_codes a list of supported format is
> > > > now available, use it to validate that the supplied format is correct
> > > > and use the default YUYV8 one if that's not the case.
> > >
> > > With the update tests for the change in patch 4 I hit multiple issues
> > > with this patch for CVBS capture.
> > >
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++-
> > > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > index 219417b319a6..1aae6467ca62 100644
> > > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > > @@ -215,6 +215,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> > > > return 0;
> > > > }
> > > >
> > > > +static int adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> > > > + unsigned int code)
> > > > +{
> > > > + const unsigned int *codes = is_txa(tx) ?
> > > > + adv748x_csi2_txa_fmts :
> > > > + adv748x_csi2_txb_fmts;
> > > > + size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> > > > + : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> > > > +
> > > > + for (unsigned int i = 0; i < num_fmts; i++) {
> > > > + if (codes[i] == code)
> > > > + return 0;
> > > > + }
> > > > +
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > struct v4l2_subdev_state *sd_state,
> > > > struct v4l2_subdev_format *sdformat)
> > > > @@ -222,7 +239,15 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
> > > > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > > struct adv748x_state *state = tx->state;
> > > > struct v4l2_mbus_framefmt *mbusformat;
> > > > - int ret = 0;
> > > > + int ret;
> > > > +
> > > > + /*
> > > > + * Make sure the format is supported, if not default it to
> > > > + * YUYV8 as it's supported by both TXes.
> > > > + */
> > > > + ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code);
> > > > + if (ret)
> > > > + sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16;
> > >
> > > If adv748x_csi2_is_fmt_supported() returns non-zero you default to
> > > MEDIA_BUS_FMT_YUYV8_1X16, which is fine. But the non-zero return code is
> > > propagated at the end of this function and to user-space falling the
> > > IOCTL.
> >
> > Ouch, I think in my tests the error message got ignored
> >
> > >
> > > Fixing that I hit another issue that kind of shows we need this format
> > > validation ;-)
> > >
> > > The TXB entity only supports MEDIA_BUS_FMT_YUYV8_1X16 formats, the AFE
> > > entity only outputs MEDIA_BUS_FMT_UYVY8_2X8... So with format validation
> > > in place it becomes impossible to connect AFE to TXB and breaking CBVS
> > > capture on Gen3. As a hack I added MEDIA_BUS_FMT_UYVY8_2X8 support to
> > > TXB and I can again capture CVBS with patch 1-8 applied.
> >
> > Thanks for testing analog capture, I don't have a setup to easily do
> > so.
>
> You can test it with the pattern generator on any Gen3 board.
>
ah well
> >
> > Should we make the AFE front-end produce 1X16 instead ? The format is
> > only used between the AFE and TXs after all, changing it shouldn't
> > have any implication on the interoperability with other devices.
>
> Not sure, the list of formats supported by each entity in the ADV748x is
> added by patch 'media: adv748x-csi2: Implement enum_mbus_codes' in this
> series.
> Where did the list come from?
>From the chip datasheet ?
Section 9.7 "MIPI Ouput format"
>What formats do the AFE support?
The AFE doesn't really "support" any format in my understanding. It
connects to one of the two TXes with an internal processing pipeline,
and the TX transmits the received video stream on the serial bus.
> Why is the formats supported different between TXA and TXB ?
You should ask the chip producer :)
>
> > >
> > > >
> > > > mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > > sdformat->which);
> > > > --
> > > > 2.44.0
> > > >
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
>
> --
> Kind Regards,
> Niklas Söderlund
>
