From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Kieran Bingham" <kieran.bingham+renesas@ideasonboard.com>,
"Tomi Valkeinen" <tomi.valkeinen@ideasonboard.com>,
linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 07/11] media: adv748x-csi2: Validate the image format
Date: Mon, 6 May 2024 22:02:04 +0300 [thread overview]
Message-ID: <20240506190204.GD26689@pendragon.ideasonboard.com> (raw)
In-Reply-To: <volz34u4ooq3rnw3pws4lulbvpdz2efuzgbphpf46uebdqftfg@b2ftppacqein>
On Mon, May 06, 2024 at 05:04:52PM +0200, Jacopo Mondi wrote:
> On Mon, May 06, 2024 at 04:58:30PM GMT, Niklas Söderlund wrote:
> > On 2024-05-06 16:36:01 +0200, Jacopo Mondi wrote:
> > > On Mon, May 06, 2024 at 04:12:01PM GMT, Niklas Söderlund wrote:
> > > > On 2024-05-06 15:21:30 +0200, Jacopo Mondi wrote:
> > > > > On Mon, May 06, 2024 at 01:37:30PM GMT, Niklas Söderlund wrote:
> > > > > > 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@ideasonboard.com>
> > > > > > > ---
> > > > > > > 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"
> >
> > Thanks I found it now, maybe add that to the commit message of that
> > patch? I was hunting for register values in the register control manual
> > and found nothing ;-)
>
> ok..
>
> > > > 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.
> >
> > Ahh! I think we found the root of the issue we talked about the other
> > day in the VIN format handling about 1X16 vs 2x8 and CSI-2 ;-) That
> > likely came from this setting.
> >
> > Yes, with the information from the datahseet I do think we should change
> > adv748x_afe_enum_mbus_code() to report MEDIA_BUS_FMT_YUYV8_1X16 instead
>
> nit: MEDIA_BUS_FMT_UYVY8_1X16
A general note about internal formats: in the absence of information
telling the exact format used in internal buses, and actually even when
that information is available but the exact format doesn't affect
anything, you're free to pick whatever seems logical enough and makes
life the easiest for userspace and kernel drivers. In this specific
case, MEDIA_BUS_FMT_YUYV8_1X16 would work, but I think
MEDIA_BUS_FMT_UYVY8_1X16 is better as it will match the CSI-2 TX output,
saving us from writing conversion code in the CSI-2 TX subdev.
> > of MEDIA_BUS_FMT_UYVY8_2X8.
> >
> > > > Why is the formats supported different between TXA and TXB ?
> > >
> > > You should ask the chip producer :)
> >
> > If only we could. Imagine how much time we save if we could talk to them
> > and have datasheets instead of guessing half the time ;-)
>
> :')
>
> > > > > > >
> > > > > > > mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> > > > > > > sdformat->which);
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2024-05-06 19:02 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-03 15:51 [PATCH 00/11] media: renesas: rcar-csi2: Use the subdev active state Jacopo Mondi
2024-05-03 15:51 ` [PATCH 01/11] media: rcar-vin: Fix YUYV8_1X16 handling for CSI-2 Jacopo Mondi
2024-05-05 20:50 ` Laurent Pinchart
2024-05-06 15:45 ` Niklas Söderlund
2024-05-03 15:51 ` [PATCH 02/11] media: rcar-csi2: Disable runtime_pm in probe error Jacopo Mondi
2024-05-05 20:52 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 03/11] media: rcar-csi2: Cleanup subdevice in remove() Jacopo Mondi
2024-05-05 20:53 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 04/11] media: rcar-csi2: Use the subdev active state Jacopo Mondi
2024-05-03 18:03 ` Niklas Söderlund
2024-05-05 21:52 ` Laurent Pinchart
2024-05-06 11:18 ` Niklas Söderlund
2024-05-06 7:25 ` Jacopo Mondi
2024-05-06 8:10 ` Geert Uytterhoeven
2024-05-06 11:26 ` Niklas Söderlund
2024-05-05 21:54 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 05/11] media: adv748x-csi2: Initialize subdev format Jacopo Mondi
2024-05-05 21:01 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 06/11] media: adv748x-csi2: Implement enum_mbus_codes Jacopo Mondi
2024-05-05 21:07 ` Laurent Pinchart
2024-05-06 8:10 ` Jacopo Mondi
2024-05-06 8:38 ` Laurent Pinchart
2024-05-06 8:42 ` Jacopo Mondi
2024-05-06 11:30 ` Niklas Söderlund
2024-05-03 15:51 ` [PATCH 07/11] media: adv748x-csi2: Validate the image format Jacopo Mondi
2024-05-05 21:09 ` Laurent Pinchart
2024-05-06 11:37 ` Niklas Söderlund
2024-05-06 13:21 ` Jacopo Mondi
2024-05-06 14:12 ` Niklas Söderlund
2024-05-06 14:36 ` Jacopo Mondi
2024-05-06 14:58 ` Niklas Söderlund
2024-05-06 15:04 ` Jacopo Mondi
2024-05-06 19:02 ` Laurent Pinchart [this message]
2024-05-03 15:51 ` [PATCH 08/11] media: adv748x-csi2: Use the subdev active state Jacopo Mondi
2024-05-05 21:15 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 09/11] media: max9286: Fix enum_mbus_code Jacopo Mondi
2024-05-05 21:18 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 10/11] media: max9286: Use the subdev active state Jacopo Mondi
2024-05-05 21:26 ` Laurent Pinchart
2024-05-03 15:51 ` [PATCH 11/11] media: max9286: Use frame interval from subdev state Jacopo Mondi
2024-05-05 21:36 ` Laurent Pinchart
2024-05-06 9:37 ` Jacopo Mondi
2024-05-06 10:32 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240506190204.GD26689@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=jacopo.mondi@ideasonboard.com \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).