On 11/10/2021 12:00, Dillon Min wrote:

Hi Hans

Thanks for the quick reply.

On Mon, 11 Oct 2021 at 17:40, Hans Verkuil [off-list ref] wrote:

quoted

On 08/10/2021 12:30, dillon.minfei@gmail.com wrote:

quoted

From: Dillon Min <redacted>

- add V4L2_COLORFX_SET_ARGB color effects control.
- add V4L2_CID_COLORFX_ARGB for ARGB color setting.

Signed-off-by: Dillon Min <redacted>
---
v3: according to Hans's suggestion, thanks.
- remove old stm32 private R2M ioctl
- add V4L2_CID_COLORFX_ARGB
- add V4L2_COLORFX_SET_ARGB

 Documentation/userspace-api/media/v4l/control.rst | 8 ++++++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c         | 2 ++
 include/uapi/linux/v4l2-controls.h                | 4 +++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/v4l/control.rst b/Documentation/userspace-api/media/v4l/control.rst
index f8d0b923da20..319606a6288f 100644
--- a/Documentation/userspace-api/media/v4l/control.rst
+++ b/Documentation/userspace-api/media/v4l/control.rst

@@ -242,8 +242,16 @@ Control IDs
     * - ``V4L2_COLORFX_SET_CBCR``
       - The Cb and Cr chroma components are replaced by fixed coefficients
      determined by ``V4L2_CID_COLORFX_CBCR`` control.
+    * - ``V4L2_COLORFX_SET_ARGB``
+      - ARGB colors.

How about:

        - The ARGB components are replaced by the fixed ARGB components
        determined by ``V4L2_CID_COLORFX_ARGB`` control.

Sure, will be addressed by v4.

quoted

I also wonder if it makes sense to include the alpha channel here.

Looking at the driver code it appears to me (I might be wrong) that the alpha
channel is never touched (DMA2D_ALPHA_MODE_NO_MODIF), and setting the alpha
channel as part of a color effects control is rather odd as well.

Indeed, Alpha channel is not used in current code. I'll remove this item in v4.
how about change the code like below:

    * - ``V4L2_COLORFX_SET_RGB``
       - The RGB components are replaced by the fixed RGB components
         determined by ``V4L2_CID_COLORFX_RGB`` control.

``V4L2_CID_COLORFX_RGB`` ``(integer)``
    Determines the Red, Green, and Blue coefficients for
    ``V4L2_COLORFX_SET_RGB`` color effect.
    Bits [7:0] of the supplied 32 bit value are interpreted as Blue component,
    bits [15:8] as Green component, bits [23:16] as Red component, and
    bits [31:24] must be zero.

Yes, that looks OK to me.

Regards,

	Hans

quoted

Alpha channel manipulation really is separate from the color and - if needed - should
be done with a separate control.

OK, Will use a separate control when adding blend features.

Best Regards,
Dillon

quoted

Regards,

        Hans

quoted

+``V4L2_CID_COLORFX_ARGB`` ``(integer)``
+    Determines the Alpha, Red, Green, and Blue coefficients for
+    ``V4L2_COLORFX_SET_ARGB`` color effect.
+    Bits [7:0] of the supplied 32 bit value are interpreted as Blue component,
+    bits [15:8] as Green component, bits [23:16] as Red component, and
+    bits [31:24] as Alpha component.

 ``V4L2_CID_COLORFX_CBCR`` ``(integer)``
     Determines the Cb and Cr coefficients for ``V4L2_COLORFX_SET_CBCR``

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 421300e13a41..53be6aadb289 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c

@@ -785,6 +785,7 @@ const char *v4l2_ctrl_get_name(u32 id)
      case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:   return "Min Number of Output Buffers";
      case V4L2_CID_ALPHA_COMPONENT:          return "Alpha Component";
      case V4L2_CID_COLORFX_CBCR:             return "Color Effects, CbCr";
+     case V4L2_CID_COLORFX_ARGB:             return "Color Effects, ARGB";

      /*
       * Codec controls

@@ -1392,6 +1393,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
              *min = *max = *step = *def = 0;
              break;
      case V4L2_CID_BG_COLOR:
+     case V4L2_CID_COLORFX_ARGB:
              *type = V4L2_CTRL_TYPE_INTEGER;
              *step = 1;
              *min = 0;

diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 5532b5f68493..2876c2282a68 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h

@@ -128,6 +128,7 @@ enum v4l2_colorfx {
      V4L2_COLORFX_SOLARIZATION               = 13,
      V4L2_COLORFX_ANTIQUE                    = 14,
      V4L2_COLORFX_SET_CBCR                   = 15,
+     V4L2_COLORFX_SET_ARGB                   = 16,
 };
 #define V4L2_CID_AUTOBRIGHTNESS                      (V4L2_CID_BASE+32)
 #define V4L2_CID_BAND_STOP_FILTER            (V4L2_CID_BASE+33)

@@ -145,9 +146,10 @@ enum v4l2_colorfx {

 #define V4L2_CID_ALPHA_COMPONENT             (V4L2_CID_BASE+41)
 #define V4L2_CID_COLORFX_CBCR                        (V4L2_CID_BASE+42)
+#define V4L2_CID_COLORFX_ARGB                        (V4L2_CID_BASE+43)

 /* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+44)

 /* USER-class private control IDs */