linux-kernel - Re: [PATCH v3 6/8] media: v4l2-ctrls: Add ARGB color effects control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <290d78b5-b6d4-a115-9556-f2f909f573da@xs4all.nl>
Date:   Mon, 11 Oct 2021 11:40:09 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     dillon.minfei@...il.com, mchehab@...nel.org,
        mchehab+huawei@...nel.org, ezequiel@...labora.com,
        gnurou@...il.com, pihsun@...omium.org, mcoquelin.stm32@...il.com,
        alexandre.torgue@...s.st.com, mturquette@...libre.com,
        sboyd@...nel.org, robh+dt@...nel.org, gabriel.fernandez@...com,
        gabriel.fernandez@...s.st.com
Cc:     patrice.chotard@...s.st.com, hugues.fruchet@...s.st.com,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v3 6/8] media: v4l2-ctrls: Add ARGB color effects control

On 08/10/2021 12:30, dillon.minfei@...il.com wrote:
> From: Dillon Min <dillon.minfei@...il.com>
> 
> - add V4L2_COLORFX_SET_ARGB color effects control.
> - add V4L2_CID_COLORFX_ARGB for ARGB color setting.
> 
> Signed-off-by: Dillon Min <dillon.minfei@...il.com>
> ---
> 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.

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.

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

Regards,

	Hans

>  
>  
> +``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 */
>  
>