[PATCH v2 0/2] Add control for VP9 profile

[PATCH v2 0/2] Add control for VP9 profile

[Keiichi Watanabe]

This patch series adds a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE
for VP9 profile.
This control can be used to select a desired profile for VP9 encoder.
In addition, it is also used to query for supported profiles by an
encoder or a decoder.

Patch 1 adds this control.
By patch 2, this control is added to MediaTek decoder's driver, which
supports VP9 profile 0.


Version 2 changes:
- Support this control in MediaTek decoder's driver

Keiichi Watanabe (2):
  media: v4l2-ctrl: Add control for VP9 profile
  media: mtk-vcodec: Support VP9 profile in decoder

 .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
 .../platform/mtk-vcodec/mtk_vcodec_dec.c      |  6 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
 include/uapi/linux/v4l2-controls.h            |  8 ++++++
 4 files changed, 52 insertions(+)

--
2.17.0.921.gf22659ad46-goog

[PATCH v2 0/2] Add control for VP9 profile

[Keiichi Watanabe]

This patch series adds a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE
for VP9 profile.
This control can be used to select a desired profile for VP9 encoder.
In addition, it is also used to query for supported profiles by an
encoder or a decoder.

Patch 1 adds this control.
By patch 2, this control is added to MediaTek decoder's driver, which
supports VP9 profile 0.


Version 2 changes:
- Support this control in MediaTek decoder's driver

Keiichi Watanabe (2):
  media: v4l2-ctrl: Add control for VP9 profile
  media: mtk-vcodec: Support VP9 profile in decoder

 .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
 .../platform/mtk-vcodec/mtk_vcodec_dec.c      |  6 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
 include/uapi/linux/v4l2-controls.h            |  8 ++++++
 4 files changed, 52 insertions(+)

--
2.17.0.921.gf22659ad46-goog

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Keiichi Watanabe]

Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
profile for VP9 encoder and querying for supported profiles by VP9 encoder
or decoder.

An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
used for querying since it is not a menu control but an integer
control, which cannot return an arbitrary set of supported profiles.

The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
with controls for other codec profiles. (e.g. H264)

Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
 include/uapi/linux/v4l2-controls.h            |  8 ++++++
 3 files changed, 46 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 03931f9b1285..4f7f128a4998 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
     Select the desired profile for VPx encoder. Acceptable values are 0,
     1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.

+.. _v4l2-mpeg-video-vp9-profile:
+
+``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
+    (enum)
+
+enum v4l2_mpeg_video_vp9_profile -
+    This control allows to select the profile for VP9 encoder.
+    This is also used to enumerate supported profiles by VP9 encoder or decoder.
+    Possible values are:
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
+      - Profile 0
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
+      - Profile 1
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
+      - Profile 2
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
+      - Profile 3
+
+

 High Efficiency Video Coding (HEVC/H.265) Control Reference
 -----------------------------------------------------------
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index d29e45516eb7..401ce21c2e63 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Use Previous Specific Frame",
 		NULL,
 	};
+	static const char * const vp9_profile[] = {
+		"0",
+		"1",
+		"2",
+		"3",
+		NULL,
+	};

 	static const char * const flash_led_mode[] = {
 		"Off",
@@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return mpeg4_profile;
 	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
 		return vpx_golden_frame_sel;
+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
+		return vp9_profile;
 	case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
 		return jpeg_chroma_subsampling;
 	case V4L2_CID_DV_TX_MODE:
@@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:		return "VPX P-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:			return "VPX Profile";

+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:			return "VP9 Profile";
+
 	/* HEVC controls */
 	case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:		return "HEVC I-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:		return "HEVC P-Frame QP Value";
@@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DEINTERLACING_MODE:
 	case V4L2_CID_TUNE_DEEMPHASIS:
 	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
 	case V4L2_CID_DETECT_MD_MODE:
 	case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
 	case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 8d473c979b61..56203b7b715c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
 #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP		(V4L2_CID_MPEG_BASE+510)
 #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE			(V4L2_CID_MPEG_BASE+511)

+#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE			(V4L2_CID_MPEG_BASE+512)
+enum v4l2_mpeg_video_vp9_profile {
+	V4L2_MPEG_VIDEO_VP9_PROFILE_0				= 0,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_1				= 1,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_2				= 2,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_3				= 3,
+};
+
 /* CIDs for HEVC encoding. */

 #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP		(V4L2_CID_MPEG_BASE + 600)
--
2.17.0.921.gf22659ad46-goog

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Keiichi Watanabe]

Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
profile for VP9 encoder and querying for supported profiles by VP9 encoder
or decoder.

An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
used for querying since it is not a menu control but an integer
control, which cannot return an arbitrary set of supported profiles.

The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
with controls for other codec profiles. (e.g. H264)

Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
 include/uapi/linux/v4l2-controls.h            |  8 ++++++
 3 files changed, 46 insertions(+)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 03931f9b1285..4f7f128a4998 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
     Select the desired profile for VPx encoder. Acceptable values are 0,
     1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.

+.. _v4l2-mpeg-video-vp9-profile:
+
+``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
+    (enum)
+
+enum v4l2_mpeg_video_vp9_profile -
+    This control allows to select the profile for VP9 encoder.
+    This is also used to enumerate supported profiles by VP9 encoder or decoder.
+    Possible values are:
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
+      - Profile 0
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
+      - Profile 1
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
+      - Profile 2
+    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
+      - Profile 3
+
+

 High Efficiency Video Coding (HEVC/H.265) Control Reference
 -----------------------------------------------------------
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index d29e45516eb7..401ce21c2e63 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Use Previous Specific Frame",
 		NULL,
 	};
+	static const char * const vp9_profile[] = {
+		"0",
+		"1",
+		"2",
+		"3",
+		NULL,
+	};

 	static const char * const flash_led_mode[] = {
 		"Off",
@@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return mpeg4_profile;
 	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
 		return vpx_golden_frame_sel;
+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
+		return vp9_profile;
 	case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
 		return jpeg_chroma_subsampling;
 	case V4L2_CID_DV_TX_MODE:
@@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:		return "VPX P-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:			return "VPX Profile";

+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:			return "VP9 Profile";
+
 	/* HEVC controls */
 	case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:		return "HEVC I-Frame QP Value";
 	case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:		return "HEVC P-Frame QP Value";
@@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DEINTERLACING_MODE:
 	case V4L2_CID_TUNE_DEEMPHASIS:
 	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
+	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
 	case V4L2_CID_DETECT_MD_MODE:
 	case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
 	case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 8d473c979b61..56203b7b715c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
 #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP		(V4L2_CID_MPEG_BASE+510)
 #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE			(V4L2_CID_MPEG_BASE+511)

+#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE			(V4L2_CID_MPEG_BASE+512)
+enum v4l2_mpeg_video_vp9_profile {
+	V4L2_MPEG_VIDEO_VP9_PROFILE_0				= 0,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_1				= 1,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_2				= 2,
+	V4L2_MPEG_VIDEO_VP9_PROFILE_3				= 3,
+};
+
 /* CIDs for HEVC encoding. */

 #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP		(V4L2_CID_MPEG_BASE + 600)
--
2.17.0.921.gf22659ad46-goog

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> profile for VP9 encoder and querying for supported profiles by VP9 encoder
> or decoder.
> 
> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> used for querying since it is not a menu control but an integer
> control, which cannot return an arbitrary set of supported profiles.
> 
> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> with controls for other codec profiles. (e.g. H264)

Please ignore my reply to patch 2/2. I looked at this a bit more and what you
should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.

All other codec profile controls are all enums, so the fact that VPX_PROFILE
isn't is a bug. Changing the type should not cause any problems since the same
control value is used when you set the control.

Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
as an integer type control, so the s5p-mfc driver should not be affected by
changing the type of this control.

Stanimir: this will slightly change the venus driver, but since it is a very
recent driver I think we can get away with changing the core type of the
VPX_PROFILE control. I think that's better than ending up with two controls
that do the same thing.

Regards,

	Hans

> 
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> ---
>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index 03931f9b1285..4f7f128a4998 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
>      Select the desired profile for VPx encoder. Acceptable values are 0,
>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> 
> +.. _v4l2-mpeg-video-vp9-profile:
> +
> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> +    (enum)
> +
> +enum v4l2_mpeg_video_vp9_profile -
> +    This control allows to select the profile for VP9 encoder.
> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> +    Possible values are:
> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> +      - Profile 0
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> +      - Profile 1
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> +      - Profile 2
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> +      - Profile 3
> +
> +
> 
>  High Efficiency Video Coding (HEVC/H.265) Control Reference
>  -----------------------------------------------------------
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index d29e45516eb7..401ce21c2e63 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Use Previous Specific Frame",
>  		NULL,
>  	};
> +	static const char * const vp9_profile[] = {
> +		"0",
> +		"1",
> +		"2",
> +		"3",
> +		NULL,
> +	};
> 
>  	static const char * const flash_led_mode[] = {
>  		"Off",
> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return mpeg4_profile;
>  	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>  		return vpx_golden_frame_sel;
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> +		return vp9_profile;
>  	case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>  		return jpeg_chroma_subsampling;
>  	case V4L2_CID_DV_TX_MODE:
> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:		return "VPX P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:			return "VPX Profile";
> 
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:			return "VP9 Profile";
> +
>  	/* HEVC controls */
>  	case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:		return "HEVC I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:		return "HEVC P-Frame QP Value";
> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DEINTERLACING_MODE:
>  	case V4L2_CID_TUNE_DEEMPHASIS:
>  	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>  	case V4L2_CID_DETECT_MD_MODE:
>  	case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>  	case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 8d473c979b61..56203b7b715c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP		(V4L2_CID_MPEG_BASE+510)
>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE			(V4L2_CID_MPEG_BASE+511)
> 
> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE			(V4L2_CID_MPEG_BASE+512)
> +enum v4l2_mpeg_video_vp9_profile {
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_0				= 0,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_1				= 1,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_2				= 2,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_3				= 3,
> +};
> +
>  /* CIDs for HEVC encoding. */
> 
>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP		(V4L2_CID_MPEG_BASE + 600)
> --
> 2.17.0.921.gf22659ad46-goog
> 

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> profile for VP9 encoder and querying for supported profiles by VP9 encoder
> or decoder.
> 
> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> used for querying since it is not a menu control but an integer
> control, which cannot return an arbitrary set of supported profiles.
> 
> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> with controls for other codec profiles. (e.g. H264)

Please ignore my reply to patch 2/2. I looked at this a bit more and what you
should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.

All other codec profile controls are all enums, so the fact that VPX_PROFILE
isn't is a bug. Changing the type should not cause any problems since the same
control value is used when you set the control.

Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
as an integer type control, so the s5p-mfc driver should not be affected by
changing the type of this control.

Stanimir: this will slightly change the venus driver, but since it is a very
recent driver I think we can get away with changing the core type of the
VPX_PROFILE control. I think that's better than ending up with two controls
that do the same thing.

Regards,

	Hans

> 
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> ---
>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
>  3 files changed, 46 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index 03931f9b1285..4f7f128a4998 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
>      Select the desired profile for VPx encoder. Acceptable values are 0,
>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> 
> +.. _v4l2-mpeg-video-vp9-profile:
> +
> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> +    (enum)
> +
> +enum v4l2_mpeg_video_vp9_profile -
> +    This control allows to select the profile for VP9 encoder.
> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> +    Possible values are:
> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> +      - Profile 0
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> +      - Profile 1
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> +      - Profile 2
> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> +      - Profile 3
> +
> +
> 
>  High Efficiency Video Coding (HEVC/H.265) Control Reference
>  -----------------------------------------------------------
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index d29e45516eb7..401ce21c2e63 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"Use Previous Specific Frame",
>  		NULL,
>  	};
> +	static const char * const vp9_profile[] = {
> +		"0",
> +		"1",
> +		"2",
> +		"3",
> +		NULL,
> +	};
> 
>  	static const char * const flash_led_mode[] = {
>  		"Off",
> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		return mpeg4_profile;
>  	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>  		return vpx_golden_frame_sel;
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> +		return vp9_profile;
>  	case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>  		return jpeg_chroma_subsampling;
>  	case V4L2_CID_DV_TX_MODE:
> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:		return "VPX P-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:			return "VPX Profile";
> 
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:			return "VP9 Profile";
> +
>  	/* HEVC controls */
>  	case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:		return "HEVC I-Frame QP Value";
>  	case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:		return "HEVC P-Frame QP Value";
> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DEINTERLACING_MODE:
>  	case V4L2_CID_TUNE_DEEMPHASIS:
>  	case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> +	case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>  	case V4L2_CID_DETECT_MD_MODE:
>  	case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>  	case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 8d473c979b61..56203b7b715c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP		(V4L2_CID_MPEG_BASE+510)
>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE			(V4L2_CID_MPEG_BASE+511)
> 
> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE			(V4L2_CID_MPEG_BASE+512)
> +enum v4l2_mpeg_video_vp9_profile {
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_0				= 0,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_1				= 1,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_2				= 2,
> +	V4L2_MPEG_VIDEO_VP9_PROFILE_3				= 3,
> +};
> +
>  /* CIDs for HEVC encoding. */
> 
>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP		(V4L2_CID_MPEG_BASE + 600)
> --
> 2.17.0.921.gf22659ad46-goog
> 

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

Hi Hans,

On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> > Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> > profile for VP9 encoder and querying for supported profiles by VP9 encoder
> > or decoder.
> >
> > An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> > used for querying since it is not a menu control but an integer
> > control, which cannot return an arbitrary set of supported profiles.
> >
> > The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> > with controls for other codec profiles. (e.g. H264)
>
> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.

Note that we still need a way to query VP8 and VP9 separately, since
the supported profiles will differ. (Most of hardware we have today
support all 4 profiles of VP8 and only profile 0 of VP9.)

Best regards,
Tomasz

>
> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> isn't is a bug. Changing the type should not cause any problems since the same
> control value is used when you set the control.
>
> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> as an integer type control, so the s5p-mfc driver should not be affected by
> changing the type of this control.
>
> Stanimir: this will slightly change the venus driver, but since it is a very
> recent driver I think we can get away with changing the core type of the
> VPX_PROFILE control. I think that's better than ending up with two controls
> that do the same thing.
>
> Regards,
>
>         Hans
>
> >
> > Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> > ---
> >  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > index 03931f9b1285..4f7f128a4998 100644
> > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >      Select the desired profile for VPx encoder. Acceptable values are 0,
> >      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >
> > +.. _v4l2-mpeg-video-vp9-profile:
> > +
> > +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> > +    (enum)
> > +
> > +enum v4l2_mpeg_video_vp9_profile -
> > +    This control allows to select the profile for VP9 encoder.
> > +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> > +    Possible values are:
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> > +      - Profile 0
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> > +      - Profile 1
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> > +      - Profile 2
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> > +      - Profile 3
> > +
> > +
> >
> >  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >  -----------------------------------------------------------
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index d29e45516eb7..401ce21c2e63 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               "Use Previous Specific Frame",
> >               NULL,
> >       };
> > +     static const char * const vp9_profile[] = {
> > +             "0",
> > +             "1",
> > +             "2",
> > +             "3",
> > +             NULL,
> > +     };
> >
> >       static const char * const flash_led_mode[] = {
> >               "Off",
> > @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               return mpeg4_profile;
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >               return vpx_golden_frame_sel;
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> > +             return vp9_profile;
> >       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >               return jpeg_chroma_subsampling;
> >       case V4L2_CID_DV_TX_MODE:
> > @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> > +
> >       /* HEVC controls */
> >       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> > @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_DEINTERLACING_MODE:
> >       case V4L2_CID_TUNE_DEEMPHASIS:
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >       case V4L2_CID_DETECT_MD_MODE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 8d473c979b61..56203b7b715c 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >
> > +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> > +enum v4l2_mpeg_video_vp9_profile {
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> > +};
> > +
> >  /* CIDs for HEVC encoding. */
> >
> >  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> > --
> > 2.17.0.921.gf22659ad46-goog
> >
>

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

Hi Hans,

On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> > Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> > profile for VP9 encoder and querying for supported profiles by VP9 encoder
> > or decoder.
> >
> > An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> > used for querying since it is not a menu control but an integer
> > control, which cannot return an arbitrary set of supported profiles.
> >
> > The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> > with controls for other codec profiles. (e.g. H264)
>
> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.

Note that we still need a way to query VP8 and VP9 separately, since
the supported profiles will differ. (Most of hardware we have today
support all 4 profiles of VP8 and only profile 0 of VP9.)

Best regards,
Tomasz

>
> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> isn't is a bug. Changing the type should not cause any problems since the same
> control value is used when you set the control.
>
> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> as an integer type control, so the s5p-mfc driver should not be affected by
> changing the type of this control.
>
> Stanimir: this will slightly change the venus driver, but since it is a very
> recent driver I think we can get away with changing the core type of the
> VPX_PROFILE control. I think that's better than ending up with two controls
> that do the same thing.
>
> Regards,
>
>         Hans
>
> >
> > Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> > ---
> >  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > index 03931f9b1285..4f7f128a4998 100644
> > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >      Select the desired profile for VPx encoder. Acceptable values are 0,
> >      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >
> > +.. _v4l2-mpeg-video-vp9-profile:
> > +
> > +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> > +    (enum)
> > +
> > +enum v4l2_mpeg_video_vp9_profile -
> > +    This control allows to select the profile for VP9 encoder.
> > +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> > +    Possible values are:
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> > +      - Profile 0
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> > +      - Profile 1
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> > +      - Profile 2
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> > +      - Profile 3
> > +
> > +
> >
> >  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >  -----------------------------------------------------------
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index d29e45516eb7..401ce21c2e63 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               "Use Previous Specific Frame",
> >               NULL,
> >       };
> > +     static const char * const vp9_profile[] = {
> > +             "0",
> > +             "1",
> > +             "2",
> > +             "3",
> > +             NULL,
> > +     };
> >
> >       static const char * const flash_led_mode[] = {
> >               "Off",
> > @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               return mpeg4_profile;
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >               return vpx_golden_frame_sel;
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> > +             return vp9_profile;
> >       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >               return jpeg_chroma_subsampling;
> >       case V4L2_CID_DV_TX_MODE:
> > @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> > +
> >       /* HEVC controls */
> >       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> > @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_DEINTERLACING_MODE:
> >       case V4L2_CID_TUNE_DEEMPHASIS:
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >       case V4L2_CID_DETECT_MD_MODE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 8d473c979b61..56203b7b715c 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >
> > +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> > +enum v4l2_mpeg_video_vp9_profile {
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> > +};
> > +
> >  /* CIDs for HEVC encoding. */
> >
> >  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> > --
> > 2.17.0.921.gf22659ad46-goog
> >
>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

Hi Hans,

On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> > Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> > profile for VP9 encoder and querying for supported profiles by VP9 encoder
> > or decoder.
> >
> > An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> > used for querying since it is not a menu control but an integer
> > control, which cannot return an arbitrary set of supported profiles.
> >
> > The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> > with controls for other codec profiles. (e.g. H264)
>
> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.

Note that we still need a way to query VP8 and VP9 separately, since
the supported profiles will differ. (Most of hardware we have today
support all 4 profiles of VP8 and only profile 0 of VP9.)

Best regards,
Tomasz

>
> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> isn't is a bug. Changing the type should not cause any problems since the same
> control value is used when you set the control.
>
> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> as an integer type control, so the s5p-mfc driver should not be affected by
> changing the type of this control.
>
> Stanimir: this will slightly change the venus driver, but since it is a very
> recent driver I think we can get away with changing the core type of the
> VPX_PROFILE control. I think that's better than ending up with two controls
> that do the same thing.
>
> Regards,
>
>         Hans
>
> >
> > Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> > ---
> >  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >  3 files changed, 46 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > index 03931f9b1285..4f7f128a4998 100644
> > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >      Select the desired profile for VPx encoder. Acceptable values are 0,
> >      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >
> > +.. _v4l2-mpeg-video-vp9-profile:
> > +
> > +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> > +    (enum)
> > +
> > +enum v4l2_mpeg_video_vp9_profile -
> > +    This control allows to select the profile for VP9 encoder.
> > +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> > +    Possible values are:
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> > +      - Profile 0
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> > +      - Profile 1
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> > +      - Profile 2
> > +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> > +      - Profile 3
> > +
> > +
> >
> >  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >  -----------------------------------------------------------
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index d29e45516eb7..401ce21c2e63 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               "Use Previous Specific Frame",
> >               NULL,
> >       };
> > +     static const char * const vp9_profile[] = {
> > +             "0",
> > +             "1",
> > +             "2",
> > +             "3",
> > +             NULL,
> > +     };
> >
> >       static const char * const flash_led_mode[] = {
> >               "Off",
> > @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >               return mpeg4_profile;
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >               return vpx_golden_frame_sel;
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> > +             return vp9_profile;
> >       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >               return jpeg_chroma_subsampling;
> >       case V4L2_CID_DV_TX_MODE:
> > @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> > +
> >       /* HEVC controls */
> >       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> > @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >       case V4L2_CID_DEINTERLACING_MODE:
> >       case V4L2_CID_TUNE_DEEMPHASIS:
> >       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> > +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >       case V4L2_CID_DETECT_MD_MODE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 8d473c979b61..56203b7b715c 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >
> > +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> > +enum v4l2_mpeg_video_vp9_profile {
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> > +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> > +};
> > +
> >  /* CIDs for HEVC encoding. */
> >
> >  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> > --
> > 2.17.0.921.gf22659ad46-goog
> >
>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>>> or decoder.
>>>
>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>>> used for querying since it is not a menu control but an integer
>>> control, which cannot return an arbitrary set of supported profiles.
>>>
>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>>> with controls for other codec profiles. (e.g. H264)
>>
>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> 
> Note that we still need a way to query VP8 and VP9 separately, since
> the supported profiles will differ. (Most of hardware we have today
> support all 4 profiles of VP8 and only profile 0 of VP9.)

Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
to v4l2-controls.h:

#define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE

And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).

Would that work?

This standardizes on enums for all profile controls (except s5p-mfc, which
makes its own int control, up to samsung to decide whether or not to change
that), and you have VP8 and VP9 specific profiles.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>> All other codec profile controls are all enums, so the fact that VPX_PROFILE
>> isn't is a bug. Changing the type should not cause any problems since the same
>> control value is used when you set the control.
>>
>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>> as an integer type control, so the s5p-mfc driver should not be affected by
>> changing the type of this control.
>>
>> Stanimir: this will slightly change the venus driver, but since it is a very
>> recent driver I think we can get away with changing the core type of the
>> VPX_PROFILE control. I think that's better than ending up with two controls
>> that do the same thing.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
>>> ---
>>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
>>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
>>>  3 files changed, 46 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>> index 03931f9b1285..4f7f128a4998 100644
>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
>>>      Select the desired profile for VPx encoder. Acceptable values are 0,
>>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
>>>
>>> +.. _v4l2-mpeg-video-vp9-profile:
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
>>> +    (enum)
>>> +
>>> +enum v4l2_mpeg_video_vp9_profile -
>>> +    This control allows to select the profile for VP9 encoder.
>>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
>>> +    Possible values are:
>>> +
>>> +
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
>>> +      - Profile 0
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
>>> +      - Profile 1
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
>>> +      - Profile 2
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
>>> +      - Profile 3
>>> +
>>> +
>>>
>>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
>>>  -----------------------------------------------------------
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index d29e45516eb7..401ce21c2e63 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               "Use Previous Specific Frame",
>>>               NULL,
>>>       };
>>> +     static const char * const vp9_profile[] = {
>>> +             "0",
>>> +             "1",
>>> +             "2",
>>> +             "3",
>>> +             NULL,
>>> +     };
>>>
>>>       static const char * const flash_led_mode[] = {
>>>               "Off",
>>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               return mpeg4_profile;
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>>               return vpx_golden_frame_sel;
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>> +             return vp9_profile;
>>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>>>               return jpeg_chroma_subsampling;
>>>       case V4L2_CID_DV_TX_MODE:
>>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
>>>
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
>>> +
>>>       /* HEVC controls */
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
>>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>       case V4L2_CID_DEINTERLACING_MODE:
>>>       case V4L2_CID_TUNE_DEEMPHASIS:
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>>       case V4L2_CID_DETECT_MD_MODE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index 8d473c979b61..56203b7b715c 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
>>>
>>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
>>> +enum v4l2_mpeg_video_vp9_profile {
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
>>> +};
>>> +
>>>  /* CIDs for HEVC encoding. */
>>>
>>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
>>> --
>>> 2.17.0.921.gf22659ad46-goog
>>>
>>

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>>> or decoder.
>>>
>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>>> used for querying since it is not a menu control but an integer
>>> control, which cannot return an arbitrary set of supported profiles.
>>>
>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>>> with controls for other codec profiles. (e.g. H264)
>>
>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> 
> Note that we still need a way to query VP8 and VP9 separately, since
> the supported profiles will differ. (Most of hardware we have today
> support all 4 profiles of VP8 and only profile 0 of VP9.)

Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
to v4l2-controls.h:

#define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE

And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).

Would that work?

This standardizes on enums for all profile controls (except s5p-mfc, which
makes its own int control, up to samsung to decide whether or not to change
that), and you have VP8 and VP9 specific profiles.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>> All other codec profile controls are all enums, so the fact that VPX_PROFILE
>> isn't is a bug. Changing the type should not cause any problems since the same
>> control value is used when you set the control.
>>
>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>> as an integer type control, so the s5p-mfc driver should not be affected by
>> changing the type of this control.
>>
>> Stanimir: this will slightly change the venus driver, but since it is a very
>> recent driver I think we can get away with changing the core type of the
>> VPX_PROFILE control. I think that's better than ending up with two controls
>> that do the same thing.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
>>> ---
>>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
>>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
>>>  3 files changed, 46 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>> index 03931f9b1285..4f7f128a4998 100644
>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
>>>      Select the desired profile for VPx encoder. Acceptable values are 0,
>>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
>>>
>>> +.. _v4l2-mpeg-video-vp9-profile:
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
>>> +    (enum)
>>> +
>>> +enum v4l2_mpeg_video_vp9_profile -
>>> +    This control allows to select the profile for VP9 encoder.
>>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
>>> +    Possible values are:
>>> +
>>> +
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
>>> +      - Profile 0
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
>>> +      - Profile 1
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
>>> +      - Profile 2
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
>>> +      - Profile 3
>>> +
>>> +
>>>
>>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
>>>  -----------------------------------------------------------
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index d29e45516eb7..401ce21c2e63 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               "Use Previous Specific Frame",
>>>               NULL,
>>>       };
>>> +     static const char * const vp9_profile[] = {
>>> +             "0",
>>> +             "1",
>>> +             "2",
>>> +             "3",
>>> +             NULL,
>>> +     };
>>>
>>>       static const char * const flash_led_mode[] = {
>>>               "Off",
>>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               return mpeg4_profile;
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>>               return vpx_golden_frame_sel;
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>> +             return vp9_profile;
>>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>>>               return jpeg_chroma_subsampling;
>>>       case V4L2_CID_DV_TX_MODE:
>>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
>>>
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
>>> +
>>>       /* HEVC controls */
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
>>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>       case V4L2_CID_DEINTERLACING_MODE:
>>>       case V4L2_CID_TUNE_DEEMPHASIS:
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>>       case V4L2_CID_DETECT_MD_MODE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index 8d473c979b61..56203b7b715c 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
>>>
>>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
>>> +enum v4l2_mpeg_video_vp9_profile {
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
>>> +};
>>> +
>>>  /* CIDs for HEVC encoding. */
>>>
>>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
>>> --
>>> 2.17.0.921.gf22659ad46-goog
>>>
>>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>>> or decoder.
>>>
>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>>> used for querying since it is not a menu control but an integer
>>> control, which cannot return an arbitrary set of supported profiles.
>>>
>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>>> with controls for other codec profiles. (e.g. H264)
>>
>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> 
> Note that we still need a way to query VP8 and VP9 separately, since
> the supported profiles will differ. (Most of hardware we have today
> support all 4 profiles of VP8 and only profile 0 of VP9.)

Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
to v4l2-controls.h:

#define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE

And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).

Would that work?

This standardizes on enums for all profile controls (except s5p-mfc, which
makes its own int control, up to samsung to decide whether or not to change
that), and you have VP8 and VP9 specific profiles.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>>
>> All other codec profile controls are all enums, so the fact that VPX_PROFILE
>> isn't is a bug. Changing the type should not cause any problems since the same
>> control value is used when you set the control.
>>
>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>> as an integer type control, so the s5p-mfc driver should not be affected by
>> changing the type of this control.
>>
>> Stanimir: this will slightly change the venus driver, but since it is a very
>> recent driver I think we can get away with changing the core type of the
>> VPX_PROFILE control. I think that's better than ending up with two controls
>> that do the same thing.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
>>> ---
>>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
>>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
>>>  3 files changed, 46 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
>>> index 03931f9b1285..4f7f128a4998 100644
>>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
>>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
>>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
>>>      Select the desired profile for VPx encoder. Acceptable values are 0,
>>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
>>>
>>> +.. _v4l2-mpeg-video-vp9-profile:
>>> +
>>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
>>> +    (enum)
>>> +
>>> +enum v4l2_mpeg_video_vp9_profile -
>>> +    This control allows to select the profile for VP9 encoder.
>>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
>>> +    Possible values are:
>>> +
>>> +
>>> +
>>> +.. flat-table::
>>> +    :header-rows:  0
>>> +    :stub-columns: 0
>>> +
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
>>> +      - Profile 0
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
>>> +      - Profile 1
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
>>> +      - Profile 2
>>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
>>> +      - Profile 3
>>> +
>>> +
>>>
>>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
>>>  -----------------------------------------------------------
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index d29e45516eb7..401ce21c2e63 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               "Use Previous Specific Frame",
>>>               NULL,
>>>       };
>>> +     static const char * const vp9_profile[] = {
>>> +             "0",
>>> +             "1",
>>> +             "2",
>>> +             "3",
>>> +             NULL,
>>> +     };
>>>
>>>       static const char * const flash_led_mode[] = {
>>>               "Off",
>>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>>               return mpeg4_profile;
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>>               return vpx_golden_frame_sel;
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>> +             return vp9_profile;
>>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
>>>               return jpeg_chroma_subsampling;
>>>       case V4L2_CID_DV_TX_MODE:
>>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
>>>
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
>>> +
>>>       /* HEVC controls */
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
>>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>>>       case V4L2_CID_DEINTERLACING_MODE:
>>>       case V4L2_CID_TUNE_DEEMPHASIS:
>>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
>>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
>>>       case V4L2_CID_DETECT_MD_MODE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
>>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
>>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>>> index 8d473c979b61..56203b7b715c 100644
>>> --- a/include/uapi/linux/v4l2-controls.h
>>> +++ b/include/uapi/linux/v4l2-controls.h
>>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
>>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
>>>
>>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
>>> +enum v4l2_mpeg_video_vp9_profile {
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
>>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
>>> +};
>>> +
>>>  /* CIDs for HEVC encoding. */
>>>
>>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
>>> --
>>> 2.17.0.921.gf22659ad46-goog
>>>
>>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

On Fri, Jun 8, 2018 at 6:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> >>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> >>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
> >>> or decoder.
> >>>
> >>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> >>> used for querying since it is not a menu control but an integer
> >>> control, which cannot return an arbitrary set of supported profiles.
> >>>
> >>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> >>> with controls for other codec profiles. (e.g. H264)
> >>
> >> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> >> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> >
> > Note that we still need a way to query VP8 and VP9 separately, since
> > the supported profiles will differ. (Most of hardware we have today
> > support all 4 profiles of VP8 and only profile 0 of VP9.)
>
> Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

I don't know the background, but it's completely inconsistent to
similar controls we have for other codecs. (Also we don't have
V4L2_CID_MPEG_VIDEO_PROFILE, but rather 1 control for each codec).

>
> In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
> V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
> to v4l2-controls.h:
>
> #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE
>
> And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).
>
> Would that work?
>
> This standardizes on enums for all profile controls (except s5p-mfc, which
> makes its own int control, up to samsung to decide whether or not to change
> that), and you have VP8 and VP9 specific profiles.

Sounds good to me, thanks.

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> >> isn't is a bug. Changing the type should not cause any problems since the same
> >> control value is used when you set the control.
> >>
> >> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> >> as an integer type control, so the s5p-mfc driver should not be affected by
> >> changing the type of this control.
> >>
> >> Stanimir: this will slightly change the venus driver, but since it is a very
> >> recent driver I think we can get away with changing the core type of the
> >> VPX_PROFILE control. I think that's better than ending up with two controls
> >> that do the same thing.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> >>> ---
> >>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >>>  3 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> index 03931f9b1285..4f7f128a4998 100644
> >>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> >>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >>>      Select the desired profile for VPx encoder. Acceptable values are 0,
> >>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >>>
> >>> +.. _v4l2-mpeg-video-vp9-profile:
> >>> +
> >>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> >>> +    (enum)
> >>> +
> >>> +enum v4l2_mpeg_video_vp9_profile -
> >>> +    This control allows to select the profile for VP9 encoder.
> >>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> >>> +    Possible values are:
> >>> +
> >>> +
> >>> +
> >>> +.. flat-table::
> >>> +    :header-rows:  0
> >>> +    :stub-columns: 0
> >>> +
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> >>> +      - Profile 0
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> >>> +      - Profile 1
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> >>> +      - Profile 2
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> >>> +      - Profile 3
> >>> +
> >>> +
> >>>
> >>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >>>  -----------------------------------------------------------
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index d29e45516eb7..401ce21c2e63 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               "Use Previous Specific Frame",
> >>>               NULL,
> >>>       };
> >>> +     static const char * const vp9_profile[] = {
> >>> +             "0",
> >>> +             "1",
> >>> +             "2",
> >>> +             "3",
> >>> +             NULL,
> >>> +     };
> >>>
> >>>       static const char * const flash_led_mode[] = {
> >>>               "Off",
> >>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               return mpeg4_profile;
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>>               return vpx_golden_frame_sel;
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>> +             return vp9_profile;
> >>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >>>               return jpeg_chroma_subsampling;
> >>>       case V4L2_CID_DV_TX_MODE:
> >>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >>>
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> >>> +
> >>>       /* HEVC controls */
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> >>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>       case V4L2_CID_DEINTERLACING_MODE:
> >>>       case V4L2_CID_TUNE_DEEMPHASIS:
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>>       case V4L2_CID_DETECT_MD_MODE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index 8d473c979b61..56203b7b715c 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >>>
> >>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> >>> +enum v4l2_mpeg_video_vp9_profile {
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> >>> +};
> >>> +
> >>>  /* CIDs for HEVC encoding. */
> >>>
> >>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> >>> --
> >>> 2.17.0.921.gf22659ad46-goog
> >>>
> >>
>

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

On Fri, Jun 8, 2018 at 6:40 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> >>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> >>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
> >>> or decoder.
> >>>
> >>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> >>> used for querying since it is not a menu control but an integer
> >>> control, which cannot return an arbitrary set of supported profiles.
> >>>
> >>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> >>> with controls for other codec profiles. (e.g. H264)
> >>
> >> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> >> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> >
> > Note that we still need a way to query VP8 and VP9 separately, since
> > the supported profiles will differ. (Most of hardware we have today
> > support all 4 profiles of VP8 and only profile 0 of VP9.)
>
> Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

I don't know the background, but it's completely inconsistent to
similar controls we have for other codecs. (Also we don't have
V4L2_CID_MPEG_VIDEO_PROFILE, but rather 1 control for each codec).

>
> In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
> V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
> to v4l2-controls.h:
>
> #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE
>
> And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).
>
> Would that work?
>
> This standardizes on enums for all profile controls (except s5p-mfc, which
> makes its own int control, up to samsung to decide whether or not to change
> that), and you have VP8 and VP9 specific profiles.

Sounds good to me, thanks.

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> >> isn't is a bug. Changing the type should not cause any problems since the same
> >> control value is used when you set the control.
> >>
> >> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> >> as an integer type control, so the s5p-mfc driver should not be affected by
> >> changing the type of this control.
> >>
> >> Stanimir: this will slightly change the venus driver, but since it is a very
> >> recent driver I think we can get away with changing the core type of the
> >> VPX_PROFILE control. I think that's better than ending up with two controls
> >> that do the same thing.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> >>> ---
> >>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >>>  3 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> index 03931f9b1285..4f7f128a4998 100644
> >>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> >>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >>>      Select the desired profile for VPx encoder. Acceptable values are 0,
> >>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >>>
> >>> +.. _v4l2-mpeg-video-vp9-profile:
> >>> +
> >>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> >>> +    (enum)
> >>> +
> >>> +enum v4l2_mpeg_video_vp9_profile -
> >>> +    This control allows to select the profile for VP9 encoder.
> >>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> >>> +    Possible values are:
> >>> +
> >>> +
> >>> +
> >>> +.. flat-table::
> >>> +    :header-rows:  0
> >>> +    :stub-columns: 0
> >>> +
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> >>> +      - Profile 0
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> >>> +      - Profile 1
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> >>> +      - Profile 2
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> >>> +      - Profile 3
> >>> +
> >>> +
> >>>
> >>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >>>  -----------------------------------------------------------
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index d29e45516eb7..401ce21c2e63 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               "Use Previous Specific Frame",
> >>>               NULL,
> >>>       };
> >>> +     static const char * const vp9_profile[] = {
> >>> +             "0",
> >>> +             "1",
> >>> +             "2",
> >>> +             "3",
> >>> +             NULL,
> >>> +     };
> >>>
> >>>       static const char * const flash_led_mode[] = {
> >>>               "Off",
> >>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               return mpeg4_profile;
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>>               return vpx_golden_frame_sel;
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>> +             return vp9_profile;
> >>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >>>               return jpeg_chroma_subsampling;
> >>>       case V4L2_CID_DV_TX_MODE:
> >>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >>>
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> >>> +
> >>>       /* HEVC controls */
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> >>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>       case V4L2_CID_DEINTERLACING_MODE:
> >>>       case V4L2_CID_TUNE_DEEMPHASIS:
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>>       case V4L2_CID_DETECT_MD_MODE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index 8d473c979b61..56203b7b715c 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >>>
> >>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> >>> +enum v4l2_mpeg_video_vp9_profile {
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> >>> +};
> >>> +
> >>>  /* CIDs for HEVC encoding. */
> >>>
> >>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> >>> --
> >>> 2.17.0.921.gf22659ad46-goog
> >>>
> >>
>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Tomasz Figa]

On Fri, Jun 8, 2018 at 6:40 PM Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> wrote:
>
> On 06/08/2018 11:31 AM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Fri, Jun 8, 2018 at 6:29 PM Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> wrote:
> >>
> >> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> >>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
> >>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
> >>> or decoder.
> >>>
> >>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
> >>> used for querying since it is not a menu control but an integer
> >>> control, which cannot return an arbitrary set of supported profiles.
> >>>
> >>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
> >>> with controls for other codec profiles. (e.g. H264)
> >>
> >> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> >> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> >
> > Note that we still need a way to query VP8 and VP9 separately, since
> > the supported profiles will differ. (Most of hardware we have today
> > support all 4 profiles of VP8 and only profile 0 of VP9.)
>
> Urgh. So V4L2_CID_MPEG_VIDEO_VPX_PROFILE is really just for VP8?

I don't know the background, but it's completely inconsistent to
similar controls we have for other codecs. (Also we don't have
V4L2_CID_MPEG_VIDEO_PROFILE, but rather 1 control for each codec).

>
> In that case I would suggest that we rename V4L2_CID_MPEG_VIDEO_VPX_PROFILE to
> V4L2_CID_MPEG_VIDEO_VP8_PROFILE and change it to an enum. Also add this line
> to v4l2-controls.h:
>
> #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE V4L2_CID_MPEG_VIDEO_VP8_PROFILE
>
> And add a new V4L2_CID_MPEG_VIDEO_VP9_PROFILE (basically this patch).
>
> Would that work?
>
> This standardizes on enums for all profile controls (except s5p-mfc, which
> makes its own int control, up to samsung to decide whether or not to change
> that), and you have VP8 and VP9 specific profiles.

Sounds good to me, thanks.

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >>
> >> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> >> isn't is a bug. Changing the type should not cause any problems since the same
> >> control value is used when you set the control.
> >>
> >> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> >> as an integer type control, so the s5p-mfc driver should not be affected by
> >> changing the type of this control.
> >>
> >> Stanimir: this will slightly change the venus driver, but since it is a very
> >> recent driver I think we can get away with changing the core type of the
> >> VPX_PROFILE control. I think that's better than ending up with two controls
> >> that do the same thing.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>> Signed-off-by: Keiichi Watanabe <keiichiw-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>> ---
> >>>  .../media/uapi/v4l/extended-controls.rst      | 26 +++++++++++++++++++
> >>>  drivers/media/v4l2-core/v4l2-ctrls.c          | 12 +++++++++
> >>>  include/uapi/linux/v4l2-controls.h            |  8 ++++++
> >>>  3 files changed, 46 insertions(+)
> >>>
> >>> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> index 03931f9b1285..4f7f128a4998 100644
> >>> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> >>> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> >>> @@ -1959,6 +1959,32 @@ enum v4l2_vp8_golden_frame_sel -
> >>>      Select the desired profile for VPx encoder. Acceptable values are 0,
> >>>      1, 2 and 3 corresponding to encoder profiles 0, 1, 2 and 3.
> >>>
> >>> +.. _v4l2-mpeg-video-vp9-profile:
> >>> +
> >>> +``V4L2_CID_MPEG_VIDEO_VP9_PROFILE``
> >>> +    (enum)
> >>> +
> >>> +enum v4l2_mpeg_video_vp9_profile -
> >>> +    This control allows to select the profile for VP9 encoder.
> >>> +    This is also used to enumerate supported profiles by VP9 encoder or decoder.
> >>> +    Possible values are:
> >>> +
> >>> +
> >>> +
> >>> +.. flat-table::
> >>> +    :header-rows:  0
> >>> +    :stub-columns: 0
> >>> +
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_0``
> >>> +      - Profile 0
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_1``
> >>> +      - Profile 1
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_2``
> >>> +      - Profile 2
> >>> +    * - ``V4L2_MPEG_VIDEO_VP9_PROFILE_3``
> >>> +      - Profile 3
> >>> +
> >>> +
> >>>
> >>>  High Efficiency Video Coding (HEVC/H.265) Control Reference
> >>>  -----------------------------------------------------------
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index d29e45516eb7..401ce21c2e63 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -431,6 +431,13 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               "Use Previous Specific Frame",
> >>>               NULL,
> >>>       };
> >>> +     static const char * const vp9_profile[] = {
> >>> +             "0",
> >>> +             "1",
> >>> +             "2",
> >>> +             "3",
> >>> +             NULL,
> >>> +     };
> >>>
> >>>       static const char * const flash_led_mode[] = {
> >>>               "Off",
> >>> @@ -614,6 +621,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >>>               return mpeg4_profile;
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>>               return vpx_golden_frame_sel;
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>> +             return vp9_profile;
> >>>       case V4L2_CID_JPEG_CHROMA_SUBSAMPLING:
> >>>               return jpeg_chroma_subsampling;
> >>>       case V4L2_CID_DV_TX_MODE:
> >>> @@ -841,6 +850,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP:                return "VPX P-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_PROFILE:                   return "VPX Profile";
> >>>
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:                   return "VP9 Profile";
> >>> +
> >>>       /* HEVC controls */
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_I_FRAME_QP:               return "HEVC I-Frame QP Value";
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_P_FRAME_QP:               return "HEVC P-Frame QP Value";
> >>> @@ -1180,6 +1191,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >>>       case V4L2_CID_DEINTERLACING_MODE:
> >>>       case V4L2_CID_TUNE_DEEMPHASIS:
> >>>       case V4L2_CID_MPEG_VIDEO_VPX_GOLDEN_FRAME_SEL:
> >>> +     case V4L2_CID_MPEG_VIDEO_VP9_PROFILE:
> >>>       case V4L2_CID_DETECT_MD_MODE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_PROFILE:
> >>>       case V4L2_CID_MPEG_VIDEO_HEVC_LEVEL:
> >>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> >>> index 8d473c979b61..56203b7b715c 100644
> >>> --- a/include/uapi/linux/v4l2-controls.h
> >>> +++ b/include/uapi/linux/v4l2-controls.h
> >>> @@ -589,6 +589,14 @@ enum v4l2_vp8_golden_frame_sel {
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_P_FRAME_QP           (V4L2_CID_MPEG_BASE+510)
> >>>  #define V4L2_CID_MPEG_VIDEO_VPX_PROFILE                      (V4L2_CID_MPEG_BASE+511)
> >>>
> >>> +#define V4L2_CID_MPEG_VIDEO_VP9_PROFILE                      (V4L2_CID_MPEG_BASE+512)
> >>> +enum v4l2_mpeg_video_vp9_profile {
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_0                           = 0,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_1                           = 1,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_2                           = 2,
> >>> +     V4L2_MPEG_VIDEO_VP9_PROFILE_3                           = 3,
> >>> +};
> >>> +
> >>>  /* CIDs for HEVC encoding. */
> >>>
> >>>  #define V4L2_CID_MPEG_VIDEO_HEVC_MIN_QP              (V4L2_CID_MPEG_BASE + 600)
> >>> --
> >>> 2.17.0.921.gf22659ad46-goog
> >>>
> >>
>

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Stanimir Varbanov]

Hi Hans,

On 06/08/2018 12:29 PM, Hans Verkuil wrote:
> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>> or decoder.
>>
>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>> used for querying since it is not a menu control but an integer
>> control, which cannot return an arbitrary set of supported profiles.
>>
>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>> with controls for other codec profiles. (e.g. H264)
> 
> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> 
> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> isn't is a bug. Changing the type should not cause any problems since the same
> control value is used when you set the control.
> 
> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> as an integer type control, so the s5p-mfc driver should not be affected by
> changing the type of this control.
> 
> Stanimir: this will slightly change the venus driver, but since it is a very
> recent driver I think we can get away with changing the core type of the
> VPX_PROFILE control. I think that's better than ending up with two controls
> that do the same thing.

I agree. Actually the changes shouldn't be so much in venus driver.

-- 
regards,
Stan

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Stanimir Varbanov]

Hi Hans,

On 06/08/2018 12:29 PM, Hans Verkuil wrote:
> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>> or decoder.
>>
>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>> used for querying since it is not a menu control but an integer
>> control, which cannot return an arbitrary set of supported profiles.
>>
>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>> with controls for other codec profiles. (e.g. H264)
> 
> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
> 
> All other codec profile controls are all enums, so the fact that VPX_PROFILE
> isn't is a bug. Changing the type should not cause any problems since the same
> control value is used when you set the control.
> 
> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
> as an integer type control, so the s5p-mfc driver should not be affected by
> changing the type of this control.
> 
> Stanimir: this will slightly change the venus driver, but since it is a very
> recent driver I think we can get away with changing the core type of the
> VPX_PROFILE control. I think that's better than ending up with two controls
> that do the same thing.

I agree. Actually the changes shouldn't be so much in venus driver.

-- 
regards,
Stan

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Keiichi Watanabe]

Hi, Hans.
Thank you for the review.
Your idea sounds good.

However, I think that changing V4L2_CID_MPEG_VIDEO_VPX_PROFILE to an enum
breaks both of s5p-mfc and venus drivers.  This is because they call
'v4l2_ctrl_new_std' for it.  For menu controls,
'v4l2_ctrl_new_std_menu' must be used.

https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L2678
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/vdec_ctrls.c#L133

I can fix them within the commit for adding VP8_PROFILE control, but
cannot confirm that it works on real devices since I don't have them.
What should I do?

Best regards,
Keiichi
On Fri, Jun 8, 2018 at 10:00 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
>
>
> Le ven. 8 juin 2018 08:56, Stanimir Varbanov <stanimir.varbanov@linaro.org> a écrit :
>>
>> Hi Hans,
>>
>> On 06/08/2018 12:29 PM, Hans Verkuil wrote:
>> > On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>> >> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>> >> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>> >> or decoder.
>> >>
>> >> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>> >> used for querying since it is not a menu control but an integer
>> >> control, which cannot return an arbitrary set of supported profiles.
>> >>
>> >> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>> >> with controls for other codec profiles. (e.g. H264)
>> >
>> > Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>> > should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
>> >
>> > All other codec profile controls are all enums, so the fact that VPX_PROFILE
>> > isn't is a bug. Changing the type should not cause any problems since the same
>> > control value is used when you set the control.
>> >
>> > Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>> > as an integer type control, so the s5p-mfc driver should not be affected by
>> > changing the type of this control.
>> >
>> > Stanimir: this will slightly change the venus driver, but since it is a very
>> > recent driver I think we can get away with changing the core type of the
>> > VPX_PROFILE control. I think that's better than ending up with two controls
>> > that do the same thing.
>>
>> I agree. Actually the changes shouldn't be so much in venus driver.
>
>
> Also fine on userspace side, since profiles enumeration isn't implemented yet in FFMPEG, GStreamer, Chrome
>
>
>>
>> --
>> regards,
>> Stan

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Keiichi Watanabe]

Hi, Hans.
Thank you for the review.
Your idea sounds good.

However, I think that changing V4L2_CID_MPEG_VIDEO_VPX_PROFILE to an enum
breaks both of s5p-mfc and venus drivers.  This is because they call
'v4l2_ctrl_new_std' for it.  For menu controls,
'v4l2_ctrl_new_std_menu' must be used.

https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L2678
https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/vdec_ctrls.c#L133

I can fix them within the commit for adding VP8_PROFILE control, but
cannot confirm that it works on real devices since I don't have them.
What should I do?

Best regards,
Keiichi
On Fri, Jun 8, 2018 at 10:00 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
>
>
> Le ven. 8 juin 2018 08:56, Stanimir Varbanov <stanimir.varbanov@linaro.org> a ?crit :
>>
>> Hi Hans,
>>
>> On 06/08/2018 12:29 PM, Hans Verkuil wrote:
>> > On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>> >> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>> >> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>> >> or decoder.
>> >>
>> >> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>> >> used for querying since it is not a menu control but an integer
>> >> control, which cannot return an arbitrary set of supported profiles.
>> >>
>> >> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>> >> with controls for other codec profiles. (e.g. H264)
>> >
>> > Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>> > should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
>> >
>> > All other codec profile controls are all enums, so the fact that VPX_PROFILE
>> > isn't is a bug. Changing the type should not cause any problems since the same
>> > control value is used when you set the control.
>> >
>> > Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>> > as an integer type control, so the s5p-mfc driver should not be affected by
>> > changing the type of this control.
>> >
>> > Stanimir: this will slightly change the venus driver, but since it is a very
>> > recent driver I think we can get away with changing the core type of the
>> > VPX_PROFILE control. I think that's better than ending up with two controls
>> > that do the same thing.
>>
>> I agree. Actually the changes shouldn't be so much in venus driver.
>
>
> Also fine on userspace side, since profiles enumeration isn't implemented yet in FFMPEG, GStreamer, Chrome
>
>
>>
>> --
>> regards,
>> Stan

Re: [PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 11/06/18 08:44, Keiichi Watanabe wrote:
> Hi, Hans.
> Thank you for the review.
> Your idea sounds good.
> 
> However, I think that changing V4L2_CID_MPEG_VIDEO_VPX_PROFILE to an enum
> breaks both of s5p-mfc and venus drivers.  This is because they call
> 'v4l2_ctrl_new_std' for it.  For menu controls,
> 'v4l2_ctrl_new_std_menu' must be used.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L2678
> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/vdec_ctrls.c#L133
> 
> I can fix them within the commit for adding VP8_PROFILE control, but
> cannot confirm that it works on real devices since I don't have them.
> What should I do?

Fix it in both drivers with a Cc to stanimir.varbanov@linaro.org (venus) and
s.nawrocki@samsung.com (s5p) to let them test/ack the patch.

Venus is no problem, the s5p driver can decide to keep the old INT control
since it has been in use for such a long time, but that is up to Sylwester to
decide.

Regards,

	Hans

> 
> Best regards,
> Keiichi
> On Fri, Jun 8, 2018 at 10:00 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>>
>>
>> Le ven. 8 juin 2018 08:56, Stanimir Varbanov <stanimir.varbanov@linaro.org> a écrit :
>>>
>>> Hi Hans,
>>>
>>> On 06/08/2018 12:29 PM, Hans Verkuil wrote:
>>>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>>>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>>>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>>>>> or decoder.
>>>>>
>>>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>>>>> used for querying since it is not a menu control but an integer
>>>>> control, which cannot return an arbitrary set of supported profiles.
>>>>>
>>>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>>>>> with controls for other codec profiles. (e.g. H264)
>>>>
>>>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>>>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
>>>>
>>>> All other codec profile controls are all enums, so the fact that VPX_PROFILE
>>>> isn't is a bug. Changing the type should not cause any problems since the same
>>>> control value is used when you set the control.
>>>>
>>>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>>>> as an integer type control, so the s5p-mfc driver should not be affected by
>>>> changing the type of this control.
>>>>
>>>> Stanimir: this will slightly change the venus driver, but since it is a very
>>>> recent driver I think we can get away with changing the core type of the
>>>> VPX_PROFILE control. I think that's better than ending up with two controls
>>>> that do the same thing.
>>>
>>> I agree. Actually the changes shouldn't be so much in venus driver.
>>
>>
>> Also fine on userspace side, since profiles enumeration isn't implemented yet in FFMPEG, GStreamer, Chrome
>>
>>
>>>
>>> --
>>> regards,
>>> Stan

[PATCH v2 1/2] media: v4l2-ctrl: Add control for VP9 profile

[Hans Verkuil]

On 11/06/18 08:44, Keiichi Watanabe wrote:
> Hi, Hans.
> Thank you for the review.
> Your idea sounds good.
> 
> However, I think that changing V4L2_CID_MPEG_VIDEO_VPX_PROFILE to an enum
> breaks both of s5p-mfc and venus drivers.  This is because they call
> 'v4l2_ctrl_new_std' for it.  For menu controls,
> 'v4l2_ctrl_new_std_menu' must be used.
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L2678
> https://elixir.bootlin.com/linux/latest/source/drivers/media/platform/qcom/venus/vdec_ctrls.c#L133
> 
> I can fix them within the commit for adding VP8_PROFILE control, but
> cannot confirm that it works on real devices since I don't have them.
> What should I do?

Fix it in both drivers with a Cc to stanimir.varbanov at linaro.org (venus) and
s.nawrocki at samsung.com (s5p) to let them test/ack the patch.

Venus is no problem, the s5p driver can decide to keep the old INT control
since it has been in use for such a long time, but that is up to Sylwester to
decide.

Regards,

	Hans

> 
> Best regards,
> Keiichi
> On Fri, Jun 8, 2018 at 10:00 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>>
>>
>> Le ven. 8 juin 2018 08:56, Stanimir Varbanov <stanimir.varbanov@linaro.org> a ?crit :
>>>
>>> Hi Hans,
>>>
>>> On 06/08/2018 12:29 PM, Hans Verkuil wrote:
>>>> On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
>>>>> Add a new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE for selecting desired
>>>>> profile for VP9 encoder and querying for supported profiles by VP9 encoder
>>>>> or decoder.
>>>>>
>>>>> An existing control V4L2_CID_MPEG_VIDEO_VPX_PROFILE cannot be
>>>>> used for querying since it is not a menu control but an integer
>>>>> control, which cannot return an arbitrary set of supported profiles.
>>>>>
>>>>> The new control V4L2_CID_MPEG_VIDEO_VP9_PROFILE is a menu control as
>>>>> with controls for other codec profiles. (e.g. H264)
>>>>
>>>> Please ignore my reply to patch 2/2. I looked at this a bit more and what you
>>>> should do is to change the type of V4L2_CID_MPEG_VIDEO_VPX_PROFILE to enum.
>>>>
>>>> All other codec profile controls are all enums, so the fact that VPX_PROFILE
>>>> isn't is a bug. Changing the type should not cause any problems since the same
>>>> control value is used when you set the control.
>>>>
>>>> Sylwester: I see that s5p-mfc uses this control, but it is explicitly added
>>>> as an integer type control, so the s5p-mfc driver should not be affected by
>>>> changing the type of this control.
>>>>
>>>> Stanimir: this will slightly change the venus driver, but since it is a very
>>>> recent driver I think we can get away with changing the core type of the
>>>> VPX_PROFILE control. I think that's better than ending up with two controls
>>>> that do the same thing.
>>>
>>> I agree. Actually the changes shouldn't be so much in venus driver.
>>
>>
>> Also fine on userspace side, since profiles enumeration isn't implemented yet in FFMPEG, GStreamer, Chrome
>>
>>
>>>
>>> --
>>> regards,
>>> Stan

[PATCH v2 2/2] media: mtk-vcodec: Support VP9 profile in decoder

[Keiichi Watanabe]

Add V4L2_CID_MPEG_VIDEO_VP9_PROFILE control in MediaTek decoder's
driver.
MediaTek decoder only supports profile 0 for now.

Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index 86f0a7134365..f9393504356d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -1400,6 +1400,12 @@ int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_ctx *ctx)
 				V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
 				0, 32, 1, 1);
 	ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	v4l2_ctrl_new_std_menu(&ctx->ctrl_hdl,
+				&mtk_vcodec_dec_ctrl_ops,
+				V4L2_CID_MPEG_VIDEO_VP9_PROFILE,
+				V4L2_MPEG_VIDEO_VP9_PROFILE_3,
+				~(1U << V4L2_MPEG_VIDEO_VP9_PROFILE_0),
+				V4L2_MPEG_VIDEO_VP9_PROFILE_0);

 	if (ctx->ctrl_hdl.error) {
 		mtk_v4l2_err("Adding control failed %d",
--
2.17.0.921.gf22659ad46-goog

[PATCH v2 2/2] media: mtk-vcodec: Support VP9 profile in decoder

[Keiichi Watanabe]

Add V4L2_CID_MPEG_VIDEO_VP9_PROFILE control in MediaTek decoder's
driver.
MediaTek decoder only supports profile 0 for now.

Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
index 86f0a7134365..f9393504356d 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
@@ -1400,6 +1400,12 @@ int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_ctx *ctx)
 				V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
 				0, 32, 1, 1);
 	ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	v4l2_ctrl_new_std_menu(&ctx->ctrl_hdl,
+				&mtk_vcodec_dec_ctrl_ops,
+				V4L2_CID_MPEG_VIDEO_VP9_PROFILE,
+				V4L2_MPEG_VIDEO_VP9_PROFILE_3,
+				~(1U << V4L2_MPEG_VIDEO_VP9_PROFILE_0),
+				V4L2_MPEG_VIDEO_VP9_PROFILE_0);

 	if (ctx->ctrl_hdl.error) {
 		mtk_v4l2_err("Adding control failed %d",
--
2.17.0.921.gf22659ad46-goog

Re: [PATCH v2 2/2] media: mtk-vcodec: Support VP9 profile in decoder

[Hans Verkuil]

On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> Add V4L2_CID_MPEG_VIDEO_VP9_PROFILE control in MediaTek decoder's
> driver.
> MediaTek decoder only supports profile 0 for now.
> 
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> index 86f0a7134365..f9393504356d 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> @@ -1400,6 +1400,12 @@ int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_ctx *ctx)
>  				V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
>  				0, 32, 1, 1);
>  	ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> +	v4l2_ctrl_new_std_menu(&ctx->ctrl_hdl,
> +				&mtk_vcodec_dec_ctrl_ops,
> +				V4L2_CID_MPEG_VIDEO_VP9_PROFILE,
> +				V4L2_MPEG_VIDEO_VP9_PROFILE_3,

It makes no sense to set max to PROFILE_3 if PROFILE_0 is the only choice.
Just set max to PROFILE_0 as well.

> +				~(1U << V4L2_MPEG_VIDEO_VP9_PROFILE_0),
> +				V4L2_MPEG_VIDEO_VP9_PROFILE_0);
> 
>  	if (ctx->ctrl_hdl.error) {
>  		mtk_v4l2_err("Adding control failed %d",
> --
> 2.17.0.921.gf22659ad46-goog
> 

Regards,

	Hans

[PATCH v2 2/2] media: mtk-vcodec: Support VP9 profile in decoder

[Hans Verkuil]

On 05/30/2018 09:16 AM, Keiichi Watanabe wrote:
> Add V4L2_CID_MPEG_VIDEO_VP9_PROFILE control in MediaTek decoder's
> driver.
> MediaTek decoder only supports profile 0 for now.
> 
> Signed-off-by: Keiichi Watanabe <keiichiw@chromium.org>
> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> index 86f0a7134365..f9393504356d 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> @@ -1400,6 +1400,12 @@ int mtk_vcodec_dec_ctrls_setup(struct mtk_vcodec_ctx *ctx)
>  				V4L2_CID_MIN_BUFFERS_FOR_CAPTURE,
>  				0, 32, 1, 1);
>  	ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> +	v4l2_ctrl_new_std_menu(&ctx->ctrl_hdl,
> +				&mtk_vcodec_dec_ctrl_ops,
> +				V4L2_CID_MPEG_VIDEO_VP9_PROFILE,
> +				V4L2_MPEG_VIDEO_VP9_PROFILE_3,

It makes no sense to set max to PROFILE_3 if PROFILE_0 is the only choice.
Just set max to PROFILE_0 as well.

> +				~(1U << V4L2_MPEG_VIDEO_VP9_PROFILE_0),
> +				V4L2_MPEG_VIDEO_VP9_PROFILE_0);
> 
>  	if (ctx->ctrl_hdl.error) {
>  		mtk_v4l2_err("Adding control failed %d",
> --
> 2.17.0.921.gf22659ad46-goog
> 

Regards,

	Hans