| Message ID | 20230309144320.2937553-1-luca.ceresoli@bootlin.com |
|---|---|
| Headers | show |
| Series | Add Tegra20 parallel video input capture | expand |
Hello, On Thu, 9 Mar 2023 15:42:59 +0100 Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > TL;DR: Resending this series with Rob's review tag added. Now all DT > patches have at least a reviewed-by tag from Rob or Krzysztof, all the > driver patches have one from Dmitry. This is a gentle ping about this series. All patches have a reviewed-by, there are no major changes since v2, and it's touching a staging driver anyway. I don't think there is more I can do at the moment besides pinging, but should there be anything, I'd be glad to know! :-) Best regards, Luca
On 22/03/2023 10:14, Luca Ceresoli wrote: > Hello, > > On Thu, 9 Mar 2023 15:42:59 +0100 > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote: > >> TL;DR: Resending this series with Rob's review tag added. Now all DT >> patches have at least a reviewed-by tag from Rob or Krzysztof, all the >> driver patches have one from Dmitry. > > This is a gentle ping about this series. All patches have a > reviewed-by, there are no major changes since v2, and it's touching a > staging driver anyway. > > I don't think there is more I can do at the moment besides pinging, but > should there be anything, I'd be glad to know! :-) I actually started on this last week, but now I am abroad again with no hardware access. I hope I can resume work on it next week, because after that I am again abroad for 10 days or so. It's currently very high on my todo list, so I really hope I will be able to make progress on it next week. Regards, Hans
Hi Luca, I finally found the time to test this series. It looks OK, except for this patch. The list of supported formats really has to be the intersection of what the tegra supports and what the sensor supports. Otherwise you would advertise pixelformats that cannot be used, and the application would have no way of knowing that. This patch needs to be dropped. I'll run this series through my other checks, and I will let you know today if anything else needs to be changed. Regards, Hans On 09/03/2023 15:43, Luca Ceresoli wrote: > The .vidioc_enum_fmt_vid_cap (called tegra_channel_enum_format() here) > should return all the supported formats. Instead the current implementation > computes the intersection between the formats it supports and those > supported by the first subdev in the stream (typically the image sensor). > > Remove all the unnecessary logic that supports such algorithm. In order to > do this, also change the Tegra210 CSI TPG formats from the current > open-coded implementation in vi_tpg_fmts_bitmap_init() to a const array in > tegra210.c, just like the one that describes the regular formats. > > Fixes: 3d8a97eabef0 ("media: tegra-video: Add Tegra210 Video input driver") > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > Reviewed-by: Dmitry Osipenko <digetx@gmail.com> > > --- > > Changed in v4: > - Added review tags > > No changes in v3 > No changes in v2 > --- > drivers/staging/media/tegra-video/tegra210.c | 7 +- > drivers/staging/media/tegra-video/vi.c | 103 +------------------ > drivers/staging/media/tegra-video/vi.h | 4 - > 3 files changed, 9 insertions(+), 105 deletions(-) > > diff --git a/drivers/staging/media/tegra-video/tegra210.c b/drivers/staging/media/tegra-video/tegra210.c > index d58370a84737..eb19dd5107ce 100644 > --- a/drivers/staging/media/tegra-video/tegra210.c > +++ b/drivers/staging/media/tegra-video/tegra210.c > @@ -683,8 +683,12 @@ enum tegra210_image_format { > V4L2_PIX_FMT_##FOURCC, \ > } > > -/* Tegra210 supported video formats */ > static const struct tegra_video_format tegra210_video_formats[] = { > +#if IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG) > + /* VI only support 2 formats in TPG mode */ > + TEGRA210_VIDEO_FMT(RAW10, 10, SRGGB10_1X10, 2, T_R16_I, SRGGB10), > + TEGRA210_VIDEO_FMT(RGB888, 24, RGB888_1X32_PADHI, 4, T_A8B8G8R8, RGBX32), > +#else > /* RAW 8 */ > TEGRA210_VIDEO_FMT(RAW8, 8, SRGGB8_1X8, 1, T_L8, SRGGB8), > TEGRA210_VIDEO_FMT(RAW8, 8, SGRBG8_1X8, 1, T_L8, SGRBG8), > @@ -714,6 +718,7 @@ static const struct tegra_video_format tegra210_video_formats[] = { > TEGRA210_VIDEO_FMT(YUV422_8, 16, VYUY8_2X8, 2, T_V8_Y8__U8_Y8, YUYV), > TEGRA210_VIDEO_FMT(YUV422_8, 16, YUYV8_2X8, 2, T_Y8_U8__Y8_V8, VYUY), > TEGRA210_VIDEO_FMT(YUV422_8, 16, YVYU8_2X8, 2, T_Y8_V8__Y8_U8, UYVY), > +#endif > }; > > /* Tegra210 VI operations */ > diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c > index 11dd142c98c5..9dba6e97ebdd 100644 > --- a/drivers/staging/media/tegra-video/vi.c > +++ b/drivers/staging/media/tegra-video/vi.c > @@ -3,7 +3,6 @@ > * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved. > */ > > -#include <linux/bitmap.h> > #include <linux/clk.h> > #include <linux/delay.h> > #include <linux/host1x.h> > @@ -73,15 +72,6 @@ static int tegra_get_format_idx_by_code(struct tegra_vi *vi, > return -1; > } > > -static u32 tegra_get_format_fourcc_by_idx(struct tegra_vi *vi, > - unsigned int index) > -{ > - if (index >= vi->soc->nformats) > - return -EINVAL; > - > - return vi->soc->video_formats[index].fourcc; > -} > - > static const struct tegra_video_format * > tegra_get_format_by_fourcc(struct tegra_vi *vi, u32 fourcc) > { > @@ -430,19 +420,12 @@ static int tegra_channel_enum_format(struct file *file, void *fh, > struct v4l2_fmtdesc *f) > { > struct tegra_vi_channel *chan = video_drvdata(file); > - unsigned int index = 0, i; > - unsigned long *fmts_bitmap = chan->tpg_fmts_bitmap; > - > - if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) > - fmts_bitmap = chan->fmts_bitmap; > + const struct tegra_vi_soc *soc = chan->vi->soc; > > - if (f->index >= bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM)) > + if (f->index >= soc->nformats) > return -EINVAL; > > - for (i = 0; i < f->index + 1; i++, index++) > - index = find_next_bit(fmts_bitmap, MAX_FORMAT_NUM, index); > - > - f->pixelformat = tegra_get_format_fourcc_by_idx(chan->vi, index - 1); > + f->pixelformat = soc->video_formats[f->index].fourcc; > > return 0; > } > @@ -1059,78 +1042,6 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan) > return 0; > } > > -/* VI only support 2 formats in TPG mode */ > -static void vi_tpg_fmts_bitmap_init(struct tegra_vi_channel *chan) > -{ > - int index; > - > - bitmap_zero(chan->tpg_fmts_bitmap, MAX_FORMAT_NUM); > - > - index = tegra_get_format_idx_by_code(chan->vi, > - MEDIA_BUS_FMT_SRGGB10_1X10, 0); > - bitmap_set(chan->tpg_fmts_bitmap, index, 1); > - > - index = tegra_get_format_idx_by_code(chan->vi, > - MEDIA_BUS_FMT_RGB888_1X32_PADHI, > - 0); > - bitmap_set(chan->tpg_fmts_bitmap, index, 1); > -} > - > -static int vi_fmts_bitmap_init(struct tegra_vi_channel *chan) > -{ > - int index, ret, match_code = 0; > - struct v4l2_subdev *subdev; > - struct v4l2_subdev_mbus_code_enum code = { > - .which = V4L2_SUBDEV_FORMAT_ACTIVE, > - }; > - > - bitmap_zero(chan->fmts_bitmap, MAX_FORMAT_NUM); > - > - /* > - * Set the bitmap bits based on all the matched formats between the > - * available media bus formats of sub-device and the pre-defined Tegra > - * supported video formats. > - */ > - subdev = tegra_channel_get_remote_source_subdev(chan); > - while (1) { > - ret = v4l2_subdev_call(subdev, pad, enum_mbus_code, > - NULL, &code); > - if (ret < 0) > - break; > - > - index = tegra_get_format_idx_by_code(chan->vi, code.code, 0); > - while (index >= 0) { > - bitmap_set(chan->fmts_bitmap, index, 1); > - if (!match_code) > - match_code = code.code; > - /* look for other formats with same mbus code */ > - index = tegra_get_format_idx_by_code(chan->vi, > - code.code, > - index + 1); > - } > - > - code.index++; > - } > - > - /* > - * Set the bitmap bit corresponding to default tegra video format if > - * there are no matched formats. > - */ > - if (!match_code) { > - match_code = tegra_default_format.code; > - index = tegra_get_format_idx_by_code(chan->vi, match_code, 0); > - if (WARN_ON(index < 0)) > - return -EINVAL; > - > - bitmap_set(chan->fmts_bitmap, index, 1); > - } > - > - /* initialize channel format to the sub-device active format */ > - tegra_channel_set_subdev_active_fmt(chan); > - > - return 0; > -} > - > static void tegra_channel_host1x_syncpts_free(struct tegra_vi_channel *chan) > { > int i; > @@ -1501,7 +1412,6 @@ int tegra_v4l2_nodes_setup_tpg(struct tegra_video_device *vid) > goto cleanup; > > v4l2_set_subdev_hostdata(&csi_chan->subdev, vi_chan); > - vi_tpg_fmts_bitmap_init(vi_chan); > csi_chan = list_next_entry(csi_chan, list); > } > > @@ -1721,13 +1631,6 @@ static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier) > goto unregister_video; > } > > - ret = vi_fmts_bitmap_init(chan); > - if (ret < 0) { > - dev_err(vi->dev, > - "failed to initialize formats bitmap: %d\n", ret); > - goto unregister_video; > - } > - > subdev = tegra_channel_get_remote_csi_subdev(chan); > if (!subdev) { > ret = -ENODEV; > diff --git a/drivers/staging/media/tegra-video/vi.h b/drivers/staging/media/tegra-video/vi.h > index a68e2c02c7b0..183796c8a46a 100644 > --- a/drivers/staging/media/tegra-video/vi.h > +++ b/drivers/staging/media/tegra-video/vi.h > @@ -163,8 +163,6 @@ struct tegra_vi_graph_entity { > * > * @ctrl_handler: V4L2 control handler of this video channel > * @syncpt_timeout_retry: syncpt timeout retry count for the capture > - * @fmts_bitmap: a bitmap for supported formats matching v4l2 subdev formats > - * @tpg_fmts_bitmap: a bitmap for supported TPG formats > * @pg_mode: test pattern generator mode (disabled/direct/patch) > * @notifier: V4L2 asynchronous subdevs notifier > */ > @@ -205,8 +203,6 @@ struct tegra_vi_channel { > > struct v4l2_ctrl_handler ctrl_handler; > unsigned int syncpt_timeout_retry; > - DECLARE_BITMAP(fmts_bitmap, MAX_FORMAT_NUM); > - DECLARE_BITMAP(tpg_fmts_bitmap, MAX_FORMAT_NUM); > enum tegra_vi_pg_mode pg_mode; > > struct v4l2_async_notifier notifier;
Hi Luca, On 29/03/2023 13:16, Hans Verkuil wrote: > Hi Luca, > > I finally found the time to test this series. It looks OK, except for this patch. > The list of supported formats really has to be the intersection of what the tegra > supports and what the sensor supports. > > Otherwise you would advertise pixelformats that cannot be used, and the application > would have no way of knowing that. > > This patch needs to be dropped. > > I'll run this series through my other checks, and I will let you know today if > anything else needs to be changed. All other checks passed, so this is the only issue blocking this series from being merged. Regards, Hans > > Regards, > > Hans > > On 09/03/2023 15:43, Luca Ceresoli wrote: >> The .vidioc_enum_fmt_vid_cap (called tegra_channel_enum_format() here) >> should return all the supported formats. Instead the current implementation >> computes the intersection between the formats it supports and those >> supported by the first subdev in the stream (typically the image sensor). >> >> Remove all the unnecessary logic that supports such algorithm. In order to >> do this, also change the Tegra210 CSI TPG formats from the current >> open-coded implementation in vi_tpg_fmts_bitmap_init() to a const array in >> tegra210.c, just like the one that describes the regular formats. >> >> Fixes: 3d8a97eabef0 ("media: tegra-video: Add Tegra210 Video input driver") >> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> >> Reviewed-by: Dmitry Osipenko <digetx@gmail.com> >> >> --- >> >> Changed in v4: >> - Added review tags >> >> No changes in v3 >> No changes in v2 >> --- >> drivers/staging/media/tegra-video/tegra210.c | 7 +- >> drivers/staging/media/tegra-video/vi.c | 103 +------------------ >> drivers/staging/media/tegra-video/vi.h | 4 - >> 3 files changed, 9 insertions(+), 105 deletions(-) >> >> diff --git a/drivers/staging/media/tegra-video/tegra210.c b/drivers/staging/media/tegra-video/tegra210.c >> index d58370a84737..eb19dd5107ce 100644 >> --- a/drivers/staging/media/tegra-video/tegra210.c >> +++ b/drivers/staging/media/tegra-video/tegra210.c >> @@ -683,8 +683,12 @@ enum tegra210_image_format { >> V4L2_PIX_FMT_##FOURCC, \ >> } >> >> -/* Tegra210 supported video formats */ >> static const struct tegra_video_format tegra210_video_formats[] = { >> +#if IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG) >> + /* VI only support 2 formats in TPG mode */ >> + TEGRA210_VIDEO_FMT(RAW10, 10, SRGGB10_1X10, 2, T_R16_I, SRGGB10), >> + TEGRA210_VIDEO_FMT(RGB888, 24, RGB888_1X32_PADHI, 4, T_A8B8G8R8, RGBX32), >> +#else >> /* RAW 8 */ >> TEGRA210_VIDEO_FMT(RAW8, 8, SRGGB8_1X8, 1, T_L8, SRGGB8), >> TEGRA210_VIDEO_FMT(RAW8, 8, SGRBG8_1X8, 1, T_L8, SGRBG8), >> @@ -714,6 +718,7 @@ static const struct tegra_video_format tegra210_video_formats[] = { >> TEGRA210_VIDEO_FMT(YUV422_8, 16, VYUY8_2X8, 2, T_V8_Y8__U8_Y8, YUYV), >> TEGRA210_VIDEO_FMT(YUV422_8, 16, YUYV8_2X8, 2, T_Y8_U8__Y8_V8, VYUY), >> TEGRA210_VIDEO_FMT(YUV422_8, 16, YVYU8_2X8, 2, T_Y8_V8__Y8_U8, UYVY), >> +#endif >> }; >> >> /* Tegra210 VI operations */ >> diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c >> index 11dd142c98c5..9dba6e97ebdd 100644 >> --- a/drivers/staging/media/tegra-video/vi.c >> +++ b/drivers/staging/media/tegra-video/vi.c >> @@ -3,7 +3,6 @@ >> * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved. >> */ >> >> -#include <linux/bitmap.h> >> #include <linux/clk.h> >> #include <linux/delay.h> >> #include <linux/host1x.h> >> @@ -73,15 +72,6 @@ static int tegra_get_format_idx_by_code(struct tegra_vi *vi, >> return -1; >> } >> >> -static u32 tegra_get_format_fourcc_by_idx(struct tegra_vi *vi, >> - unsigned int index) >> -{ >> - if (index >= vi->soc->nformats) >> - return -EINVAL; >> - >> - return vi->soc->video_formats[index].fourcc; >> -} >> - >> static const struct tegra_video_format * >> tegra_get_format_by_fourcc(struct tegra_vi *vi, u32 fourcc) >> { >> @@ -430,19 +420,12 @@ static int tegra_channel_enum_format(struct file *file, void *fh, >> struct v4l2_fmtdesc *f) >> { >> struct tegra_vi_channel *chan = video_drvdata(file); >> - unsigned int index = 0, i; >> - unsigned long *fmts_bitmap = chan->tpg_fmts_bitmap; >> - >> - if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG)) >> - fmts_bitmap = chan->fmts_bitmap; >> + const struct tegra_vi_soc *soc = chan->vi->soc; >> >> - if (f->index >= bitmap_weight(fmts_bitmap, MAX_FORMAT_NUM)) >> + if (f->index >= soc->nformats) >> return -EINVAL; >> >> - for (i = 0; i < f->index + 1; i++, index++) >> - index = find_next_bit(fmts_bitmap, MAX_FORMAT_NUM, index); >> - >> - f->pixelformat = tegra_get_format_fourcc_by_idx(chan->vi, index - 1); >> + f->pixelformat = soc->video_formats[f->index].fourcc; >> >> return 0; >> } >> @@ -1059,78 +1042,6 @@ static int tegra_channel_setup_ctrl_handler(struct tegra_vi_channel *chan) >> return 0; >> } >> >> -/* VI only support 2 formats in TPG mode */ >> -static void vi_tpg_fmts_bitmap_init(struct tegra_vi_channel *chan) >> -{ >> - int index; >> - >> - bitmap_zero(chan->tpg_fmts_bitmap, MAX_FORMAT_NUM); >> - >> - index = tegra_get_format_idx_by_code(chan->vi, >> - MEDIA_BUS_FMT_SRGGB10_1X10, 0); >> - bitmap_set(chan->tpg_fmts_bitmap, index, 1); >> - >> - index = tegra_get_format_idx_by_code(chan->vi, >> - MEDIA_BUS_FMT_RGB888_1X32_PADHI, >> - 0); >> - bitmap_set(chan->tpg_fmts_bitmap, index, 1); >> -} >> - >> -static int vi_fmts_bitmap_init(struct tegra_vi_channel *chan) >> -{ >> - int index, ret, match_code = 0; >> - struct v4l2_subdev *subdev; >> - struct v4l2_subdev_mbus_code_enum code = { >> - .which = V4L2_SUBDEV_FORMAT_ACTIVE, >> - }; >> - >> - bitmap_zero(chan->fmts_bitmap, MAX_FORMAT_NUM); >> - >> - /* >> - * Set the bitmap bits based on all the matched formats between the >> - * available media bus formats of sub-device and the pre-defined Tegra >> - * supported video formats. >> - */ >> - subdev = tegra_channel_get_remote_source_subdev(chan); >> - while (1) { >> - ret = v4l2_subdev_call(subdev, pad, enum_mbus_code, >> - NULL, &code); >> - if (ret < 0) >> - break; >> - >> - index = tegra_get_format_idx_by_code(chan->vi, code.code, 0); >> - while (index >= 0) { >> - bitmap_set(chan->fmts_bitmap, index, 1); >> - if (!match_code) >> - match_code = code.code; >> - /* look for other formats with same mbus code */ >> - index = tegra_get_format_idx_by_code(chan->vi, >> - code.code, >> - index + 1); >> - } >> - >> - code.index++; >> - } >> - >> - /* >> - * Set the bitmap bit corresponding to default tegra video format if >> - * there are no matched formats. >> - */ >> - if (!match_code) { >> - match_code = tegra_default_format.code; >> - index = tegra_get_format_idx_by_code(chan->vi, match_code, 0); >> - if (WARN_ON(index < 0)) >> - return -EINVAL; >> - >> - bitmap_set(chan->fmts_bitmap, index, 1); >> - } >> - >> - /* initialize channel format to the sub-device active format */ >> - tegra_channel_set_subdev_active_fmt(chan); >> - >> - return 0; >> -} >> - >> static void tegra_channel_host1x_syncpts_free(struct tegra_vi_channel *chan) >> { >> int i; >> @@ -1501,7 +1412,6 @@ int tegra_v4l2_nodes_setup_tpg(struct tegra_video_device *vid) >> goto cleanup; >> >> v4l2_set_subdev_hostdata(&csi_chan->subdev, vi_chan); >> - vi_tpg_fmts_bitmap_init(vi_chan); >> csi_chan = list_next_entry(csi_chan, list); >> } >> >> @@ -1721,13 +1631,6 @@ static int tegra_vi_graph_notify_complete(struct v4l2_async_notifier *notifier) >> goto unregister_video; >> } >> >> - ret = vi_fmts_bitmap_init(chan); >> - if (ret < 0) { >> - dev_err(vi->dev, >> - "failed to initialize formats bitmap: %d\n", ret); >> - goto unregister_video; >> - } >> - >> subdev = tegra_channel_get_remote_csi_subdev(chan); >> if (!subdev) { >> ret = -ENODEV; >> diff --git a/drivers/staging/media/tegra-video/vi.h b/drivers/staging/media/tegra-video/vi.h >> index a68e2c02c7b0..183796c8a46a 100644 >> --- a/drivers/staging/media/tegra-video/vi.h >> +++ b/drivers/staging/media/tegra-video/vi.h >> @@ -163,8 +163,6 @@ struct tegra_vi_graph_entity { >> * >> * @ctrl_handler: V4L2 control handler of this video channel >> * @syncpt_timeout_retry: syncpt timeout retry count for the capture >> - * @fmts_bitmap: a bitmap for supported formats matching v4l2 subdev formats >> - * @tpg_fmts_bitmap: a bitmap for supported TPG formats >> * @pg_mode: test pattern generator mode (disabled/direct/patch) >> * @notifier: V4L2 asynchronous subdevs notifier >> */ >> @@ -205,8 +203,6 @@ struct tegra_vi_channel { >> >> struct v4l2_ctrl_handler ctrl_handler; >> unsigned int syncpt_timeout_retry; >> - DECLARE_BITMAP(fmts_bitmap, MAX_FORMAT_NUM); >> - DECLARE_BITMAP(tpg_fmts_bitmap, MAX_FORMAT_NUM); >> enum tegra_vi_pg_mode pg_mode; >> >> struct v4l2_async_notifier notifier; >
Hello Hans, On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > Hi Luca, > > I finally found the time to test this series. It looks OK, except for this patch. Thank you very much for taking the time! > The list of supported formats really has to be the intersection of what the tegra > supports and what the sensor supports. > > Otherwise you would advertise pixelformats that cannot be used, and the application > would have no way of knowing that. As far as I understand, I think we should rather make this driver fully behave as an MC-centric device. It is already using MC quite successfully after all. Do you think this is correct? If you do, then I think the plan would be: - Add the V4L2_CAP_IO_MC flag - As the mbus_code in get_format appropriately - Leave the changes in this patch unmodified otherwise Best regards, Luca
Hi Luca, On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote: > On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote: > > > Hi Luca, > > > > I finally found the time to test this series. It looks OK, except for this patch. > > Thank you very much for taking the time! > > > The list of supported formats really has to be the intersection of what the tegra > > supports and what the sensor supports. > > > > Otherwise you would advertise pixelformats that cannot be used, and the application > > would have no way of knowing that. > > As far as I understand, I think we should rather make this driver fully > behave as an MC-centric device. It is already using MC quite > successfully after all. > > Do you think this is correct? Given the use cases for this driver, I agree. > If you do, then I think the plan would be: > > - Add the V4L2_CAP_IO_MC flag > - As the mbus_code in get_format appropriately > - Leave the changes in this patch unmodified otherwise
Hi Laurent, On Wed, 5 Apr 2023 05:30:48 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Luca, > > On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote: > > On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote: > > > > > Hi Luca, > > > > > > I finally found the time to test this series. It looks OK, except for this patch. > > > > Thank you very much for taking the time! > > > > > The list of supported formats really has to be the intersection of what the tegra > > > supports and what the sensor supports. > > > > > > Otherwise you would advertise pixelformats that cannot be used, and the application > > > would have no way of knowing that. > > > > As far as I understand, I think we should rather make this driver fully > > behave as an MC-centric device. It is already using MC quite > > successfully after all. > > > > Do you think this is correct? > > Given the use cases for this driver, I agree. Ok, thanks for the feedback. I will send a v5 with this change. Best regards, Luca
On 05/04/2023 10:31, Luca Ceresoli wrote: > Hi Laurent, > > On Wed, 5 Apr 2023 05:30:48 +0300 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > >> Hi Luca, >> >> On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote: >>> On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote: >>> >>>> Hi Luca, >>>> >>>> I finally found the time to test this series. It looks OK, except for this patch. >>> >>> Thank you very much for taking the time! >>> >>>> The list of supported formats really has to be the intersection of what the tegra >>>> supports and what the sensor supports. >>>> >>>> Otherwise you would advertise pixelformats that cannot be used, and the application >>>> would have no way of knowing that. >>> >>> As far as I understand, I think we should rather make this driver fully >>> behave as an MC-centric device. It is already using MC quite >>> successfully after all. >>> >>> Do you think this is correct? >> >> Given the use cases for this driver, I agree. I disagree. This driver doesn't use the media controller for anything at the moment. The /dev/mediaX device just shows the internal topology (i.e. connected sensors), but otherwise it does nothing. While it would be great if we could unlock the ISP on the Tegra, the reality is that it is entirely closed source and can't be used in a linux driver, and that's not going to change, sadly. That leaves us with just a basic CSI capture driver. Rather than trying to change this driver to a full MC device with no benefits, just drop this change and get your code in. Note that this driver will stay in staging since it still fails when I try to capture from two sensors at the same time: syncpoint errors start appearing in that case. I think there are locking issues. I think I have someone to take a look at that, but first I want your series to get merged. In the very unlikely event that the ISP can be implemented in a linux driver, it will probably become a new driver. Regards, Hans > > Ok, thanks for the feedback. I will send a v5 with this change. > > Best regards, > Luca >
On Wed, Apr 05, 2023 at 10:50:37AM +0200, Hans Verkuil wrote: [...] > Note that this driver will stay in staging since it still fails when I try to > capture from two sensors at the same time: syncpoint errors start appearing > in that case. I think there are locking issues. I think I have someone to take > a look at that, but first I want your series to get merged. Mikko (added) is familiar with syncpoints, so he may be able to help with. Can you provide steps to reproduce these issues? That may make it easier for us to help figure this out. Unfortunately I don't have any device with an actual sensor on it, so I can only test with the test pattern generator, but syncpoint errors sound like they would happen with either setup. Thierry
Hi Hans, On Wed, Apr 05, 2023 at 10:50:37AM +0200, Hans Verkuil wrote: > On 05/04/2023 10:31, Luca Ceresoli wrote: > > On Wed, 5 Apr 2023 05:30:48 +0300 Laurent Pinchart wrote: > >> On Tue, Apr 04, 2023 at 04:12:51PM +0200, Luca Ceresoli wrote: > >>> On Wed, 29 Mar 2023 13:16:22 +0200 Hans Verkuil wrote: > >>> > >>>> Hi Luca, > >>>> > >>>> I finally found the time to test this series. It looks OK, except for this patch. > >>> > >>> Thank you very much for taking the time! > >>> > >>>> The list of supported formats really has to be the intersection of what the tegra > >>>> supports and what the sensor supports. > >>>> > >>>> Otherwise you would advertise pixelformats that cannot be used, and the application > >>>> would have no way of knowing that. > >>> > >>> As far as I understand, I think we should rather make this driver fully > >>> behave as an MC-centric device. It is already using MC quite > >>> successfully after all. > >>> > >>> Do you think this is correct? > >> > >> Given the use cases for this driver, I agree. > > I disagree. > > This driver doesn't use the media controller for anything at the moment. The > /dev/mediaX device just shows the internal topology (i.e. connected sensors), > but otherwise it does nothing. > > While it would be great if we could unlock the ISP on the Tegra, the reality > is that it is entirely closed source and can't be used in a linux driver, and > that's not going to change, sadly. Never say never :-) > That leaves us with just a basic CSI capture driver. Rather than trying to > change this driver to a full MC device with no benefits, just drop this change > and get your code in. Can't the hardware support capturing different virtual channels or data types from the same CSI-2 source ? That would require MC support, the stream API requires subdev device nodes. > Note that this driver will stay in staging since it still fails when I try to > capture from two sensors at the same time: syncpoint errors start appearing > in that case. I think there are locking issues. I think I have someone to take > a look at that, but first I want your series to get merged. > > In the very unlikely event that the ISP can be implemented in a linux driver, > it will probably become a new driver. > > Regards, > > > Ok, thanks for the feedback. I will send a v5 with this change.
TL;DR: Resending this series with Rob's review tag added. Now all DT patches have at least a reviewed-by tag from Rob or Krzysztof, all the driver patches have one from Dmitry. Full details follow. Tegra20 and other Tegra SoCs have a video input (VI) peripheral that can receive from either MIPI CSI-2 or parallel video (called respectively "CSI" and "VIP" in the documentation). The kernel currently has a staging driver for Tegra210 CSI capture. This patch set adds support for Tegra20 VIP capture. Unfortunately I had no real documentation available to base this work on. I only had a working downstream 3.1 kernel, so I started with the driver found there and heavily reworked it to fit into the mainline tegra-video driver structure. The existing code appears written with the intent of being modular and allow adding new input mechanisms and new SoCs while keeping a unique VI core module. However its modularity and extensibility was not enough to add Tegra20 VIP support, so I added some hooks to turn hard-coded behaviour into per-SoC or per-bus customizable code. There are also a fix, some generic cleanups and DT bindings. Quick tour of the patches: * Device tree bindings 01. dt-bindings: display: tegra: add Tegra20 VIP 02. dt-bindings: display: tegra: vi: add 'vip' property and example * A fix 03. staging: media: tegra-video: fix .vidioc_enum_fmt_vid_cap to return all formats * Minor improvements to logging, comments, cleanups 04. staging: media: tegra-video: improve documentation of tegra_video_format fields 05. staging: media: tegra-video: document tegra_channel_get_remote_source_subdev 06. staging: media: tegra-video: fix typos in comment 07. staging: media: tegra-video: improve error messages 08. staging: media: tegra-video: slightly simplify cleanup on errors 09. staging: media: tegra-video: move private struct declaration to C file 10. staging: media: tegra-video: move tegra210_csi_soc to C file 11. staging: media: tegra-video: remove unneeded include * Preparation to make the VI module generic enough to host Tegra20 and VIP 12. staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 13. staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op 14. staging: media: tegra-video: move default format to soc-specific data 15. staging: media: tegra-video: move MIPI calibration calls from VI to CSI 16. staging: media: tegra-video: add a per-soc enable/disable op 17. staging: media: tegra-video: move syncpt init/free to a per-soc op 18. staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi 19. staging: media: tegra-video: add hooks for planar YUV and H/V flip 20. staging: media: tegra-video: add H/V flip controls * Implementation of VIP and Tegra20 21. staging: media: tegra-video: add support for Tegra20 parallel input Enjoy! Changed in RESEND,v4: - add Rob's review tag on patch 2 Changed in v4: - fixed the leftovers after the removal of 'channel@0' in DT - added review tags by Dimtry Changed in v3: - removed the 'channel@0' node from the device tree representation of vip - squashed the last two patches (VIP + T20) into one - small cleanups - rebase on v6.2-rc1 Changed in v2: - improved dt-bindings patches based on reviews - removed patches 3 and 4 adding DT labels without a mainline user - two small fixes to the last patch [v4] https://lore.kernel.org/linux-tegra/20230130141603.323221-1-luca.ceresoli@bootlin.com/ [v3] https://lore.kernel.org/linux-media/20221229133205.981397-1-luca.ceresoli@bootlin.com/ [v2] https://lore.kernel.org/linux-tegra/20221222100328.6e341874@booty/T/#t [v1] https://lore.kernel.org/linux-tegra/20221124155634.5bc2a423@booty/T/#t Luca Luca Ceresoli (21): dt-bindings: display: tegra: add Tegra20 VIP dt-bindings: display: tegra: vi: add 'vip' property and example staging: media: tegra-video: fix .vidioc_enum_fmt_vid_cap to return all formats staging: media: tegra-video: improve documentation of tegra_video_format fields staging: media: tegra-video: document tegra_channel_get_remote_source_subdev staging: media: tegra-video: fix typos in comment staging: media: tegra-video: improve error messages staging: media: tegra-video: slightly simplify cleanup on errors staging: media: tegra-video: move private struct declaration to C file staging: media: tegra-video: move tegra210_csi_soc to C file staging: media: tegra-video: remove unneeded include staging: media: tegra-video: Kconfig: allow TPG only on Tegra210 staging: media: tegra-video: move tegra_channel_fmt_align to a per-soc op staging: media: tegra-video: move default format to soc-specific data staging: media: tegra-video: move MIPI calibration calls from VI to CSI staging: media: tegra-video: add a per-soc enable/disable op staging: media: tegra-video: move syncpt init/free to a per-soc op staging: media: tegra-video: add syncpts for Tegra20 to struct tegra_vi staging: media: tegra-video: add hooks for planar YUV and H/V flip staging: media: tegra-video: add H/V flip controls staging: media: tegra-video: add support for Tegra20 parallel input .../display/tegra/nvidia,tegra20-vi.yaml | 59 ++ .../display/tegra/nvidia,tegra20-vip.yaml | 41 ++ MAINTAINERS | 3 + drivers/staging/media/tegra-video/Kconfig | 1 + drivers/staging/media/tegra-video/Makefile | 2 + drivers/staging/media/tegra-video/csi.c | 48 ++ drivers/staging/media/tegra-video/csi.h | 4 - drivers/staging/media/tegra-video/tegra20.c | 661 ++++++++++++++++++ drivers/staging/media/tegra-video/tegra210.c | 97 ++- drivers/staging/media/tegra-video/vi.c | 321 ++------- drivers/staging/media/tegra-video/vi.h | 75 +- drivers/staging/media/tegra-video/video.c | 5 + drivers/staging/media/tegra-video/video.h | 2 +- drivers/staging/media/tegra-video/vip.c | 290 ++++++++ drivers/staging/media/tegra-video/vip.h | 68 ++ 15 files changed, 1387 insertions(+), 290 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-vip.yaml create mode 100644 drivers/staging/media/tegra-video/tegra20.c create mode 100644 drivers/staging/media/tegra-video/vip.c create mode 100644 drivers/staging/media/tegra-video/vip.h