Re: [PATCH v8 1/3] media: qcom: camss: Add common TPG support

[Vladimir Zapolskiy]

On 1/15/26 03:06, Bryan O'Donoghue wrote:
> On 14/01/2026 22:07, Vladimir Zapolskiy wrote:
> > Hi Wenmeng.
> >
> > On 1/14/26 14:18, Wenmeng Liu wrote:
> > > Hi Vladimir,
> > >
> > >
> > > On 1/14/2026 1:05 PM, Vladimir Zapolskiy wrote:
> > > > Hi Wenmeng.
> > > >
> > > > On 1/14/26 05:04, Wenmeng Liu wrote:
> > > > > Hi Vladimir,
> > > > >
> > > > > On 1/14/2026 12:27 AM, Vladimir Zapolskiy wrote:
> > > > > > Hello Wenmeng.
> > > > > >
> > > > > > On 1/13/26 11:03, Wenmeng Liu wrote:
> > > > > > > Introduce a new common Test Pattern Generator (TPG) implementation
> > > > > > > for
> > > > > > > Qualcomm CAMSS. This module provides a generic interface for pattern
> > > > > > > generation that can be reused by multiple platforms.
> > > > > > >
> > > > > > > Unlike CSID-integrated TPG, this TPG acts as a standalone block
> > > > > > > that emulates both CSIPHY and sensor behavior, enabling flexible test
> > > > > > > patterns without external hardware.
> > > > > > >
> > > > > > > Signed-off-by: Wenmeng Liu <wenmeng.liu@xxxxxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >      drivers/media/platform/qcom/camss/Makefile    |   1 +
> > > > > > >      drivers/media/platform/qcom/camss/camss-tpg.c | 710 ++++++++++
> > > > > > > ++++++
> > > > > > > ++++++++++
> > > > > > >      drivers/media/platform/qcom/camss/camss-tpg.h | 127 +++++
> > > > > > >      drivers/media/platform/qcom/camss/camss.h     |   5 +
> > > > > > >      4 files changed, 843 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/
> > > > > > > media/platform/qcom/camss/Makefile
> > > > > > > index
> > > > > > > 5e349b4915130c71dbff90e73102e46dfede1520..d355e67c25700ac061b878543c32ed8defc03ad0 100644
> > > > > > > --- a/drivers/media/platform/qcom/camss/Makefile
> > > > > > > +++ b/drivers/media/platform/qcom/camss/Makefile
> > > > > > > @@ -27,5 +27,6 @@ qcom-camss-objs += \
> > > > > > >              camss-vfe.o \
> > > > > > >              camss-video.o \
> > > > > > >              camss-format.o \
> > > > > > > +        camss-tpg.o \
> > > > > >
> > > > > > While you're here, please sort and keep the lines in alphabetical
> > > > > > order.
> > > > > ACK.
> > > > >
> > > > > > >      obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom-camss.o
> > > > > > > diff --git a/drivers/media/platform/qcom/camss/camss-tpg.c b/drivers/
> > > > > > > media/platform/qcom/camss/camss-tpg.c
> > > > > > > new file mode 100644
> > > > > > > index
> > > > > > > 0000000000000000000000000000000000000000..f4c015aafa202e5b64fafa3c543128fda6440b11
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/media/platform/qcom/camss/camss-tpg.c
> > > > > > > @@ -0,0 +1,710 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/*
> > > > > > > + *
> > > > > > > + * Qualcomm MSM Camera Subsystem - TPG Module
> > > > > > > + *
> > > > > > > + * Copyright (c) Qualcomm Technologies, Inc. and/or its
> > > > > > > subsidiaries.
> > > > > > > + */
> > > > > > > +#include <linux/clk.h>
> > > > > > > +#include <linux/delay.h>
> > > > > > > +#include <linux/io.h>
> > > > > > > +#include <linux/kernel.h>
> > > > > > > +#include <linux/of.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > +#include <media/media-entity.h>
> > > > > > > +#include <media/v4l2-device.h>
> > > > > > > +#include <media/v4l2-subdev.h>
> > > > > > > +
> > > > > > > +#include "camss-tpg.h"
> > > > > > > +#include "camss.h"
> > > > > > > +
> > > > > > > +const char * const testgen_payload_modes[] = {
> > > > > > > +    "Disabled",
> > > > > > > +    "Incrementing",
> > > > > > > +    "Alternating 0x55/0xAA",
> > > > > > > +    "Reserved",
> > > > > > > +    "Reserved",
> > > > > > > +    "Pseudo-random Data",
> > > > > > > +    "User Specified",
> > > > > > > +    "Reserved",
> > > > > > > +    "Reserved",
> > > > > > > +    "Color bars",
> > > > > > > +    "Reserved"
> > > > > >
> > > > > > It makes little sense to mention the unsupported values, and then
> > > > > > introduce enum tpg_testgen_mode to list the supported ones.
> > > > > This is for ctrl menu, will do as follow:
> > > > > static const char * const testgen_payload_modes[] = {
> > > > >         [TPG_PAYLOAD_MODE_DISABLED]          = "Disabled",
> > > > >         [TPG_PAYLOAD_MODE_INCREMENTING]      = "Incrementing",
> > > > >         [TPG_PAYLOAD_MODE_ALTERNATING_55_AA]       = "Alternating
> > > > > 0x55/0xAA",
> > > > >         [TPG_PAYLOAD_MODE_RANDOM]      = "Pseudo-random Data",
> > > > >         [TPG_PAYLOAD_MODE_USER_SPECIFIED]    = "User Specified",
> > > > >         [TPG_PAYLOAD_MODE_COLOR_BARS]        = "Color bars",
> > > > > };
> > > >
> > > > This is also not perfect, still userspace is misinformed about a number
> > > > of possible TPG modes vs. a number of actually supported TPG modes.
> > > 0x0: INCREMENTING
> > > 0x1: ALTERNATING_55_AA
> > > 0x4: RANDOM
> > > 0x5: USER_SPECIFIED
> > > 0x8: COLOR_BARS
> > >
> > > These values come from the register configuration, these pattern values
> > > are consistent with the CSID TPG.
> >
> > Userspace should not be aware of such low level details as register values,
> > there are many abstraction layers in-between to hide this type of
> > information.
> >
> > Writing proper values to registers should be a concern on the driver level,
> > it sounds improper to push this simple task and responsibility to
> > userspace.
>
> I think we should stick to the same format as is already upstream for
> the CSID version of this - which is the same data.

It is not the same and it will not be the same, if the currently presented
version is taken. If TPG modes in CSID are continuous, here they are not,
so it makes a big difference for userspace, and better it should be removed.

--
Best wishes,
Vladimir