BUG: BW requirement is not updated when VI output format is changed (with patch proposal)

Hello,

when trying to fix https://forums.developer.nvidia.com/t/bandwidth-problem-when-converting-mipi-raw12-to-t-r16-i-or-t-r16-on-tx2/279468/8 , I noticed that the bw field of cdev does not follow the output format of VI, and actually never changes, regardless of the number of bytes per pixel.

Here is a fix proposal, in a dirty patch to keep it as small as possible.

Please comment.

From ec0c13d25308701ce01f0e6321ce8ade26d2deaa Mon Sep 17 00:00:00 2001
From: Philippe De Muyter <philippe.demuyter@macq.eu>
Date: Thu, 25 Jan 2024 16:22:23 +0100
Subject: [PATCH] drivers: media: camera: channel: update cdev->bw when needed.

Up to now, cdev->bw was only initialized once, with the settings
matching the first compatible entry in vi4_video_formats (on TX2's),
vi2_video_formats (on TX1's and Nano's) or vi5_video_formats.
In particular, bw did not change when switching between a 2 bytes per
pixel mode and a 1 byte per pixel mode.
Fix that.  This needs scattered changes that could be split into
preparation patches.

- add a 'struct tegra_camera_dev_info *cdev' field to 'struct tegra_channel'.
- let 'tegra_camera_device_register' return the address of the created
  'struct tegra_camera_dev_info'.  In order to keep this patch small,
  I create a similar 'tegra_camera_device_register_ptr' function, which
  returns the address, and make the existing 'tegra_camera_device_register'
  be a wrapper around it.
- in 'tegra_channel_init_subdevices', call 'tegra_camera_device_register_ptr'
  instead of 'tegra_camera_device_register', and store the returned address
  into chan->cdev.
- add a forward declaration for 'tegra_channel_populate_dev_info',
  because we want to use it in a function which is currently above
  in the same file.
- in 'tegra_channel_update_format', call 'tegra_channel_populate_dev_info'
  if chan->cdev is set.

Signed-off-by: Philippe De Muyter <phdm@macqel.be>
---
 .../media/platform/tegra/camera/vi/channel.c  | 13 +++++++--
 .../tegra/camera/tegra_camera_platform.c      | 28 +++++++++++++------
 include/media/mc_common.h                     |  2 ++
 include/media/tegra_camera_platform.h         |  1 +
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/tegra/camera/vi/channel.c b/drivers/media/platform/tegra/camera/vi/channel.c
index 52e82a4c8..0ea535354 100644
--- a/drivers/media/platform/tegra/camera/vi/channel.c
+++ b/drivers/media/platform/tegra/camera/vi/channel.c
@@ -255,6 +255,9 @@ void tegra_channel_set_interlace_mode(struct tegra_channel *chan)
        }
 }

+static void tegra_channel_populate_dev_info(struct tegra_camera_dev_info *cdev,
+                       struct tegra_channel *chan);
+
 static void tegra_channel_update_format(struct tegra_channel *chan,
                u32 width, u32 height, u32 fourcc,
                const struct tegra_frac *bpp,
@@ -295,6 +298,8 @@ static void tegra_channel_update_format(struct tegra_channel *chan,

        if (fourcc == V4L2_PIX_FMT_NV16)
                chan->format.sizeimage *= 2;
+       if (chan->cdev)
+               tegra_channel_populate_dev_info(chan->cdev, chan);
 }

 static void tegra_channel_fmts_bitmap_init(struct tegra_channel *chan)
@@ -1804,6 +1809,7 @@ int tegra_channel_init_subdevices(struct tegra_channel *chan)
        int index = 0;
        int num_sd = 0;
        struct tegra_camera_dev_info camdev_info;
+       struct tegra_camera_dev_info *cdev;
        int grp_id = chan->pg_mode ? (TPG_CSI_GROUP_ID + chan->port[0] + 1)
                : chan->port[0] + 1;

@@ -1896,9 +1902,12 @@ int tegra_channel_init_subdevices(struct tegra_channel *chan)
        }

        tegra_channel_populate_dev_info(&camdev_info, chan);
-       ret = tegra_camera_device_register(&camdev_info, chan);
+       cdev = tegra_camera_device_register_ptr(&camdev_info, chan);
+       if (IS_ERR_OR_NULL(cdev))
+               return PTR_ERR(cdev);
+       chan->cdev = cdev;

-       return ret;
+       return 0;
 fail:
        tegra_channel_free_sensor_properties(chan->subdev_on_csi);
        return ret;
diff --git a/drivers/video/tegra/camera/tegra_camera_platform.c b/drivers/video/tegra/camera/tegra_camera_platform.c
index c63d9d299..f7b6e4e6d 100644
--- a/drivers/video/tegra/camera/tegra_camera_platform.c
+++ b/drivers/video/tegra/camera/tegra_camera_platform.c
@@ -708,8 +708,7 @@ static int remove_nvhost_client(struct tegra_camera_dev_info *cdev)
        return 0;
 }

-int tegra_camera_device_register(struct tegra_camera_dev_info *cdev_info,
-                                       void *priv)
+struct tegra_camera_dev_info *tegra_camera_device_register_ptr(struct tegra_camera_dev_info *cdev_info, void *priv)
 {
        int err = 0;
        struct tegra_camera_dev_info *cdev;
@@ -722,19 +721,19 @@ int tegra_camera_device_register(struct tegra_camera_dev_info *cdev_info,
         */
        if (tegra_camera_misc.parent == NULL) {
                pr_info("driver not enabled, cannot register any devices\n");
-               return 0;
+               return NULL;
        }

        if (!cdev_info || !priv)
-               return -EINVAL;
+               return ERR_PTR(-EINVAL);

        info = dev_get_drvdata(tegra_camera_misc.parent);
        if (!info)
-               return -EINVAL;
+               return ERR_PTR(-EINVAL);

        cdev = kzalloc(sizeof(struct tegra_camera_dev_info), GFP_KERNEL);
        if (!cdev)
-               return -ENOMEM;
+               return ERR_PTR(-ENOMEM);

        *cdev = *cdev_info;

@@ -748,12 +747,25 @@ int tegra_camera_device_register(struct tegra_camera_dev_info *cdev_info,
                mutex_unlock(&info->device_list_mutex);
                dev_err(info->dev, "%s could not add %d to nvhost\n",
                                __func__, cdev->hw_type);
-               return err;
+               return ERR_PTR(err);
        }
        update_platform_data(cdev, info, true);
        mutex_unlock(&info->device_list_mutex);

-       return err;
+       return cdev;
+}
+EXPORT_SYMBOL(tegra_camera_device_register_ptr);
+
+int tegra_camera_device_register(struct tegra_camera_dev_info *cdev_info,
+                                       void *priv)
+{
+       struct tegra_camera_dev_info *cdev;
+
+       cdev = tegra_camera_device_register_ptr(cdev_info, priv);
+
+       if (IS_ERR(cdev))
+               return PTR_ERR(cdev);
+       return 0;
 }
 EXPORT_SYMBOL(tegra_camera_device_register);

diff --git a/include/media/mc_common.h b/include/media/mc_common.h
index 85888dbde..615c8e63f 100644
--- a/include/media/mc_common.h
+++ b/include/media/mc_common.h
@@ -264,6 +264,8 @@ struct tegra_channel {

        atomic_t syncpt_depth;
        struct rw_semaphore reset_lock;
+
+       struct tegra_camera_dev_info *cdev;
 };

 #define to_tegra_channel(vdev) \
diff --git a/include/media/tegra_camera_platform.h b/include/media/tegra_camera_platform.h
index f58cd8457..ae8866605 100644
--- a/include/media/tegra_camera_platform.h
+++ b/include/media/tegra_camera_platform.h
@@ -95,6 +95,7 @@ struct tegra_camera_dev_info {
 int tegra_camera_update_isobw(void);
 int tegra_camera_emc_clk_enable(void);
 int tegra_camera_emc_clk_disable(void);
+struct tegra_camera_dev_info *tegra_camera_device_register_ptr(struct tegra_camera_dev_info *cdev_info, void *priv);
 int tegra_camera_device_register(struct tegra_camera_dev_info *cdev_info,
                                        void *priv);
 int tegra_camera_device_unregister(void *priv);
--
2.31.1
1 Like

thanks for sharing, let me arrange resources to have code-review internally.

hello phdm,

BTW,
may I also know what’s the test-case, or what scenario you’ve change the BW requirement.
please share the test steps for us checking this locally on the developer kits.

Hello JerryChang,

my sensor produces 12-bits-per-pixel monochrome pixels (aka Y12_1X12). This the media bus format produced by my driver.

Here are the relevant entries in vi4_video_formats

static const struct tegra_video_format vi4_video_formats[] = {

        ...
        /* RAW 12 */
        TEGRA_VIDEO_FORMAT(RAW12, 12, Y12_1X12, 2, 1, T_R16,
                                RAW12, Y16, "GRAY16_LE"),
        TEGRA_VIDEO_FORMAT(RAW12, 12, Y12_1X12, 1, 1, T_L8,
                                RAW12, GREY, "GRAY8"),

To debug the problem described in Bandwidth problem when converting mipi RAW12 to T_R16_I or T_R16 on TX2, I had also added printk’s in file drivers/video/tegra/camera/tegra_camera_platform.c in function ‘tegra_camera_update_clknbw’

diff --git a/drivers/video/tegra/camera/tegra_camera_platform.c b/drivers/video/
tegra/camera/tegra_camera_platform.c
index aa221d10c..3fcb05341 100644
--- a/drivers/video/tegra/camera/tegra_camera_platform.c
+++ b/drivers/video/tegra/camera/tegra_camera_platform.c
@@ -985,6 +990,7 @@ int tegra_camera_update_clknbw(void *priv, bool stream_on)
        if (!info)
                return -EINVAL;

+printk("tegra_camera_update_clknbw: active_iso_bw = %llu\n", info->active_iso_bw);
        mutex_lock(&info->device_list_mutex);
        /* Need to traverse the list twice, first to make sure that
         * stream on is set for the active stream and then to
@@ -997,11 +1003,13 @@ int tegra_camera_update_clknbw(void *priv, bool stream_on)
                        cdev->stream_on = stream_on;
                        if (stream_on) {
                                info->active_pixel_rate += cdev->pixel_rate;
+printk("tegra_camera_update_clknbw: adding %llu\n", cdev->bw);
                                info->active_iso_bw += cdev->bw;
                                info->num_active_streams++;
                        } else {
                                info->active_pixel_rate -= cdev->pixel_rate;
                                info->active_iso_bw -= cdev->bw;
+printk("tegra_camera_update_clknbw: removing %llu\n", cdev->bw);
                                info->num_active_streams--;
                        }
                        break;
@@ -1017,6 +1025,7 @@ int tegra_camera_update_clknbw(void *priv, bool stream_on)
                }
        }
        mutex_unlock(&info->device_list_mutex);
+printk("tegra_camera_update_clknbw: active_iso_bw = %llu\n", info->active_iso_bw);

        /* set BW */
        tegra_camera_update_isobw();

and here are my testcases :

v4l2-ctl -d /dev/video0 --stream-mmap=3 --stream-count=256 '--set-fmt-video=width=2464,height=2056,pixelformat=GREY'
v4l2-ctl -d /dev/video0 --stream-mmap=3 --stream-count=256 '--set-fmt-video=width=2464,height=2056,pixelformat=Y16 '

I noticed that the ‘bw’ values used in tegra_camera_update_clknbw were identical in both cases, while it seems to me that the bw requirements for 1-byte-per-pixel images should be the half of the bw requirements for 2-bytes-per-pixel images.

FYI, we don’t have sensor with different format types for using T_R16 and T_L8.
and… VI driver internally has called tegra_channel_get_sensor_peak_vals() to populate dev_info. so, all those different sensor modes were used the same value for BW allocation.

anyways, this patch looks reasonable, let me arrange resources for code-review.

Hello JerryChang,

did you get feedback about that patch ?

hello phdm,

we had some discussion, and, this change is not approved yet.
FYI, we’ve internally setting the BW with its max value across modes.
for instance,
Mode0 → BW x Kbps
Mode1 → BW y Kbps
BW being set = max(x,y)

hence,
may I know what’s happening without this update?
for instance, what happens if we don’t take this change for having same BW across two different modes.

Hello JerryChang,

Well it’s actually a follow-up to the patch in Bandwidth problem when converting mipi RAW12 to T_R16_I or T_R16 on TX2 - #9 by phdm which fixes a real bug (the required bandwidth was underestimated), but fixing that bug multiplies the bandwidth requirement by 8.

Also overestimating the actual bandwidth requirement can lead to many problems like overheating or denying of service for that or other bandwidth clients.