| Message ID | 20240503155127.105235-8-jacopo.mondi@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: renesas: rcar-csi2: Use the subdev active state | expand |
Hi Jacopo, Thank you for the patch. On Fri, May 03, 2024 at 05:51:22PM +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. > > 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. As indicated in the review if 06/11, I think this should be UYVY8 > + */ > + ret = adv748x_csi2_is_fmt_supported(tx, sdformat->format.code); > + if (ret) > + sdformat->format.code = MEDIA_BUS_FMT_YUYV8_1X16; > > mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > sdformat->which);
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@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. 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. > > > > > mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, > > sdformat->which); > > -- > > 2.44.0 > > > > -- > Kind Regards, > Niklas Söderlund
Hello Jacopo, On 2024-05-06 16:36:01 +0200, Jacopo Mondi wrote: > 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@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 ;-) > > >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 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); > > > > > -- > > > > > 2.44.0 > > > > > > > > > > > > > -- > > > > Kind Regards, > > > > Niklas Söderlund > > > > -- > > Kind Regards, > > Niklas Söderlund > >
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);
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; mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad, sdformat->which);
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. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- drivers/media/i2c/adv748x/adv748x-csi2.c | 27 +++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-)