[PATCH v2 0/4] media: cedrus: hevc: Add support for scaling matrix and multi-slice frames

[PATCH v2 0/4] media: cedrus: hevc: Add support for scaling matrix and multi-slice frames

[Jernej Skrabec]

Currently there is no control for HEVC scaling matrix and segment address
field is also missing. This series adds those and implements support for
them in Cedrus driver.

Following videos were used for testing:
- scaling matrix:
http://jernej.libreelec.tv/videos/h265/slist_test.mp4
- multi-slice:
http://jernej.libreelec.tv/videos/h265/Dredd%20%E2%80%93%20DTS%20Sound%20Check%20DTS-HD%20MA%207.1.m2ts

Best regards,
Jernej

Changes from v1:
- always set CTB_ADDR_X and CTB_ADDR_Y (patch 4)

Jernej Skrabec (4):
  media: uapi: hevc: Add scaling matrix control
  media: cedrus: hevc: Add support for scaling matrix
  media: uapi: hevc: Add segment address field
  media: cedrus: hevc: Add support for multiple slices

 .../media/uapi/v4l/ext-ctrls-codec.rst        | 46 ++++++++-
 .../media/uapi/v4l/pixfmt-compressed.rst      |  1 +
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 96 ++++++++++++++++---
 .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
 .../staging/media/sunxi/cedrus/cedrus_video.c |  1 +
 include/media/hevc-ctrls.h                    | 16 +++-
 10 files changed, 169 insertions(+), 13 deletions(-)

-- 
2.24.0

[PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Jernej Skrabec]

HEVC has a scaling matrix concept. Add support for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../media/uapi/v4l/ext-ctrls-codec.rst        | 41 +++++++++++++++++++
 .../media/uapi/v4l/pixfmt-compressed.rst      |  1 +
 drivers/media/v4l2-core/v4l2-ctrls.c          | 10 +++++
 include/media/hevc-ctrls.h                    | 11 +++++
 4 files changed, 63 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index 28313c0f4e7c..aab1451e54d4 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -4180,6 +4180,47 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
       - ``padding[6]``
       - Applications and drivers must set this to zero.
 
+``V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX (struct)``
+    Specifies the scaling matrix (as extracted from the bitstream) for
+    the associated HEVC slice data. The bitstream parameters are
+    defined according to :ref:`hevc`, section 7.4.5 "Scaling list
+    data semantics". For further documentation, refer to the above
+    specification, unless there is an explicit comment stating
+    otherwise.
+
+    .. note::
+
+       This compound control is not yet part of the public kernel API and
+       it is expected to change.
+
+.. c:type:: v4l2_ctrl_hevc_scaling_matrix
+
+.. cssclass:: longtable
+
+.. flat-table:: struct v4l2_ctrl_hevc_scaling_matrix
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u8
+      - ``scaling_list_4x4[6][16]``
+      -
+    * - __u8
+      - ``scaling_list_8x8[6][64]``
+      -
+    * - __u8
+      - ``scaling_list_16x16[6][64]``
+      -
+    * - __u8
+      - ``scaling_list_32x32[2][64]``
+      -
+    * - __u8
+      - ``scaling_list_dc_coef_16x16[6]``
+      -
+    * - __u8
+      - ``scaling_list_dc_coef_32x32[2]``
+      -
+
 ``V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE (enum)``
     Specifies the decoding mode to use. Currently exposes slice-based and
     frame-based decoding but new modes might be added later on.
diff --git a/Documentation/media/uapi/v4l/pixfmt-compressed.rst b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
index 561bda112809..3aabc322daa4 100644
--- a/Documentation/media/uapi/v4l/pixfmt-compressed.rst
+++ b/Documentation/media/uapi/v4l/pixfmt-compressed.rst
@@ -207,6 +207,7 @@ Compressed Formats
         * ``V4L2_CID_MPEG_VIDEO_HEVC_SPS``
         * ``V4L2_CID_MPEG_VIDEO_HEVC_PPS``
         * ``V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS``
+        * ``V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX``
 	See the :ref:`associated Codec Control IDs <v4l2-mpeg-hevc>`.
 	Buffers associated with this pixel format must contain the appropriate
 	number of macroblocks to decode a full corresponding frame.
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 2928c5e0a73d..8cc5ef33b8fd 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -975,6 +975,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_MPEG_VIDEO_HEVC_SPS:			return "HEVC Sequence Parameter Set";
 	case V4L2_CID_MPEG_VIDEO_HEVC_PPS:			return "HEVC Picture Parameter Set";
 	case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:		return "HEVC Slice Parameters";
+	case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX:		return "HEVC Scaling Matrix";
 	case V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE:		return "HEVC Decode Mode";
 	case V4L2_CID_MPEG_VIDEO_HEVC_START_CODE:		return "HEVC Start Code";
 
@@ -1407,6 +1408,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS:
 		*type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS;
 		break;
+	case V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX:
+		*type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX;
+		break;
 	case V4L2_CID_UNIT_CELL_SIZE:
 		*type = V4L2_CTRL_TYPE_AREA;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
@@ -1857,6 +1861,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		zero_padding(*p_hevc_slice_params);
 		break;
 
+	case V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX:
+		break;
+
 	case V4L2_CTRL_TYPE_AREA:
 		area = p;
 		if (!area->width || !area->height)
@@ -2546,6 +2553,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS:
 		elem_size = sizeof(struct v4l2_ctrl_hevc_slice_params);
 		break;
+	case V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX:
+		elem_size = sizeof(struct v4l2_ctrl_hevc_scaling_matrix);
+		break;
 	case V4L2_CTRL_TYPE_AREA:
 		elem_size = sizeof(struct v4l2_area);
 		break;
diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index 1009cf0891cc..1592e52c3614 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -19,6 +19,7 @@
 #define V4L2_CID_MPEG_VIDEO_HEVC_SPS		(V4L2_CID_MPEG_BASE + 1008)
 #define V4L2_CID_MPEG_VIDEO_HEVC_PPS		(V4L2_CID_MPEG_BASE + 1009)
 #define V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS	(V4L2_CID_MPEG_BASE + 1010)
+#define V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX	(V4L2_CID_MPEG_BASE + 1011)
 #define V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE	(V4L2_CID_MPEG_BASE + 1015)
 #define V4L2_CID_MPEG_VIDEO_HEVC_START_CODE	(V4L2_CID_MPEG_BASE + 1016)
 
@@ -26,6 +27,7 @@
 #define V4L2_CTRL_TYPE_HEVC_SPS 0x0120
 #define V4L2_CTRL_TYPE_HEVC_PPS 0x0121
 #define V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS 0x0122
+#define V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX 0x0123
 
 enum v4l2_mpeg_video_hevc_decode_mode {
 	V4L2_MPEG_VIDEO_HEVC_DECODE_MODE_SLICE_BASED,
@@ -209,4 +211,13 @@ struct v4l2_ctrl_hevc_slice_params {
 	__u64	flags;
 };
 
+struct v4l2_ctrl_hevc_scaling_matrix {
+	__u8	scaling_list_4x4[6][16];
+	__u8	scaling_list_8x8[6][64];
+	__u8	scaling_list_16x16[6][64];
+	__u8	scaling_list_32x32[2][64];
+	__u8	scaling_list_dc_coef_16x16[6];
+	__u8	scaling_list_dc_coef_32x32[2];
+};
+
 #endif
-- 
2.24.0

Re: [PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Mauro Carvalho Chehab]

Em Fri, 13 Dec 2019 17:04:25 +0100
Jernej Skrabec <jernej.skrabec@siol.net> escreveu:

> HEVC has a scaling matrix concept. Add support for it.

> +struct v4l2_ctrl_hevc_scaling_matrix {
> +	__u8	scaling_list_4x4[6][16];
> +	__u8	scaling_list_8x8[6][64];
> +	__u8	scaling_list_16x16[6][64];
> +	__u8	scaling_list_32x32[2][64];
> +	__u8	scaling_list_dc_coef_16x16[6];
> +	__u8	scaling_list_dc_coef_32x32[2];
> +};

I never looked at HEVC spec, but the above seems really weird.

Please correct me if I am wrong, but each of the above matrixes
is independent, and the driver will use just one of the above on
any specific time (for a given video output node), right?

If so, why would userspace be forced to update lots of matrixes, if would
likely use just one at a given time?

IMO, the proper way would be, instead, to use an uAPI like:

/*
 * Actually, as this is uAPI, we will use a fixed size integer type, like
 *  unsigned int
 */
enum hevc_scaling_matrix_type {
	HEVC_SCALING_MATRIX_4x4,
	HEVC_SCALING_MATRIX_8x8,
...
	HEVC_SCALING_MATRIX_DC_COEF_32x32,
};

struct v4l2_ctrl_hevc_scaling_matrix {
	__u32	scaling_type 		/* as defined by enum hevc_scaling_matrix_type */

	union {
		__u8	scaling_list_4x4[6][16];
		__u8	scaling_list_8x8[6][64];
		__u8	scaling_list_16x16[6][64];
		__u8	scaling_list_32x32[2][64];
		__u8	scaling_list_dc_coef_16x16[6];
		__u8	scaling_list_dc_coef_32x32[2];
	};
};

And let the core use a default for each scaling matrix, if userspace doesn't
set it.



Cheers,
Mauro

Re: [PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2216 bytes --]

Hi Mauro,

On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote:
> Em Fri, 13 Dec 2019 17:04:25 +0100
> Jernej Skrabec <jernej.skrabec@siol.net> escreveu:
> 
> > HEVC has a scaling matrix concept. Add support for it.
> 
> > +struct v4l2_ctrl_hevc_scaling_matrix {
> > +	__u8	scaling_list_4x4[6][16];
> > +	__u8	scaling_list_8x8[6][64];
> > +	__u8	scaling_list_16x16[6][64];
> > +	__u8	scaling_list_32x32[2][64];
> > +	__u8	scaling_list_dc_coef_16x16[6];
> > +	__u8	scaling_list_dc_coef_32x32[2];
> > +};
> 
> I never looked at HEVC spec, but the above seems really weird.
> 
> Please correct me if I am wrong, but each of the above matrixes
> is independent, and the driver will use just one of the above on
> any specific time (for a given video output node), right?

I am not too sure about what the specification really entails, but it is my
understanding that HEVC allows simultaneous block sizes between 4x4 and 32x32
to exist within the same coding tree and slice. That suggests that it makes
sense to have specific coefficients for each case.

Note that the hardware also has distinct registers for each scaling list.

Cheers,

Paul

> If so, why would userspace be forced to update lots of matrixes, if would
> likely use just one at a given time?
> 
> IMO, the proper way would be, instead, to use an uAPI like:
> 
> /*
>  * Actually, as this is uAPI, we will use a fixed size integer type, like
>  *  unsigned int
>  */
> enum hevc_scaling_matrix_type {
> 	HEVC_SCALING_MATRIX_4x4,
> 	HEVC_SCALING_MATRIX_8x8,
> ...
> 	HEVC_SCALING_MATRIX_DC_COEF_32x32,
> };
> 
> struct v4l2_ctrl_hevc_scaling_matrix {
> 	__u32	scaling_type 		/* as defined by enum hevc_scaling_matrix_type */
> 
> 	union {
> 		__u8	scaling_list_4x4[6][16];
> 		__u8	scaling_list_8x8[6][64];
> 		__u8	scaling_list_16x16[6][64];
> 		__u8	scaling_list_32x32[2][64];
> 		__u8	scaling_list_dc_coef_16x16[6];
> 		__u8	scaling_list_dc_coef_32x32[2];
> 	};
> };
> 
> And let the core use a default for each scaling matrix, if userspace doesn't
> set it.
> 
> 
> 
> Cheers,
> Mauro

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Jernej Škrabec]

Hi!

Dne sreda, 08. januar 2020 ob 15:43:36 CET je Paul Kocialkowski napisal(a):
> Hi Mauro,
> 
> On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote:
> > Em Fri, 13 Dec 2019 17:04:25 +0100
> > 
> > Jernej Skrabec <jernej.skrabec@siol.net> escreveu:
> > > HEVC has a scaling matrix concept. Add support for it.
> > > 
> > > +struct v4l2_ctrl_hevc_scaling_matrix {
> > > +	__u8	scaling_list_4x4[6][16];
> > > +	__u8	scaling_list_8x8[6][64];
> > > +	__u8	scaling_list_16x16[6][64];
> > > +	__u8	scaling_list_32x32[2][64];
> > > +	__u8	scaling_list_dc_coef_16x16[6];
> > > +	__u8	scaling_list_dc_coef_32x32[2];
> > > +};
> > 
> > I never looked at HEVC spec, but the above seems really weird.
> > 
> > Please correct me if I am wrong, but each of the above matrixes
> > is independent, and the driver will use just one of the above on
> > any specific time (for a given video output node), right?
> 
> I am not too sure about what the specification really entails, but it is my
> understanding that HEVC allows simultaneous block sizes between 4x4 and
> 32x32 to exist within the same coding tree and slice. That suggests that it
> makes sense to have specific coefficients for each case.

Specs ITU-T REC. H.265 (06/2019), chapter 7.3.4 shows that multiple different 
matrices can be present at the same time. If they are not, default values 
should be used instead. But in general, more than one can be needed at the 
same time.

Only real question is if default values should be also provided by userspace 
or by kernel. Since place has to be reserved for all different scaling lists 
anyway, we won't save any space by providing default values in kernel. Cedrus 
VPU has only bit switch for using default values for all matrices at the same 
time or all custom.

Note that this control contains slightly processed data. Frame has stored 
these matrices in form of deltas. But because this is the only driver that use 
this structure I have no idea what is the most proper form of this data (raw 
values or deltas). That's why this will stay in staging using private headers 
until we figure this out.

Best regards,
Jernej

> 
> Note that the hardware also has distinct registers for each scaling list.
> 
> Cheers,
> 
> Paul
> 
> > If so, why would userspace be forced to update lots of matrixes, if would
> > likely use just one at a given time?
> > 
> > IMO, the proper way would be, instead, to use an uAPI like:
> > 
> > /*
> > 
> >  * Actually, as this is uAPI, we will use a fixed size integer type, like
> >  *  unsigned int
> >  */
> > 
> > enum hevc_scaling_matrix_type {
> > 
> > 	HEVC_SCALING_MATRIX_4x4,
> > 	HEVC_SCALING_MATRIX_8x8,
> > 
> > ...
> > 
> > 	HEVC_SCALING_MATRIX_DC_COEF_32x32,
> > 
> > };
> > 
> > struct v4l2_ctrl_hevc_scaling_matrix {
> > 
> > 	__u32	scaling_type 		/* as defined by enum 
hevc_scaling_matrix_type */
> > 	
> > 	union {
> > 	
> > 		__u8	scaling_list_4x4[6][16];
> > 		__u8	scaling_list_8x8[6][64];
> > 		__u8	scaling_list_16x16[6][64];
> > 		__u8	scaling_list_32x32[2][64];
> > 		__u8	scaling_list_dc_coef_16x16[6];
> > 		__u8	scaling_list_dc_coef_32x32[2];
> > 	
> > 	};
> > 
> > };
> > 
> > And let the core use a default for each scaling matrix, if userspace
> > doesn't set it.
> > 
> > 
> > 
> > Cheers,
> > Mauro

Re: [PATCH v2 1/4] media: uapi: hevc: Add scaling matrix control

[Hans Verkuil]

On 1/9/20 4:17 PM, Jernej Škrabec wrote:
> Hi!
> 
> Dne sreda, 08. januar 2020 ob 15:43:36 CET je Paul Kocialkowski napisal(a):
>> Hi Mauro,
>>
>> On Wed 08 Jan 20, 15:11, Mauro Carvalho Chehab wrote:
>>> Em Fri, 13 Dec 2019 17:04:25 +0100
>>>
>>> Jernej Skrabec <jernej.skrabec@siol.net> escreveu:
>>>> HEVC has a scaling matrix concept. Add support for it.
>>>>
>>>> +struct v4l2_ctrl_hevc_scaling_matrix {
>>>> +	__u8	scaling_list_4x4[6][16];
>>>> +	__u8	scaling_list_8x8[6][64];
>>>> +	__u8	scaling_list_16x16[6][64];
>>>> +	__u8	scaling_list_32x32[2][64];
>>>> +	__u8	scaling_list_dc_coef_16x16[6];
>>>> +	__u8	scaling_list_dc_coef_32x32[2];
>>>> +};
>>>
>>> I never looked at HEVC spec, but the above seems really weird.
>>>
>>> Please correct me if I am wrong, but each of the above matrixes
>>> is independent, and the driver will use just one of the above on
>>> any specific time (for a given video output node), right?
>>
>> I am not too sure about what the specification really entails, but it is my
>> understanding that HEVC allows simultaneous block sizes between 4x4 and
>> 32x32 to exist within the same coding tree and slice. That suggests that it
>> makes sense to have specific coefficients for each case.
> 
> Specs ITU-T REC. H.265 (06/2019), chapter 7.3.4 shows that multiple different 
> matrices can be present at the same time. If they are not, default values 
> should be used instead. But in general, more than one can be needed at the 
> same time.
> 
> Only real question is if default values should be also provided by userspace 
> or by kernel. Since place has to be reserved for all different scaling lists 
> anyway, we won't save any space by providing default values in kernel. Cedrus 
> VPU has only bit switch for using default values for all matrices at the same 
> time or all custom.
> 
> Note that this control contains slightly processed data. Frame has stored 
> these matrices in form of deltas. But because this is the only driver that use 
> this structure I have no idea what is the most proper form of this data (raw 
> values or deltas). That's why this will stay in staging using private headers 
> until we figure this out.

This definitely needs to be documented! Otherwise this will be forgotten.

Regards,

	Hans

> 
> Best regards,
> Jernej
> 
>>
>> Note that the hardware also has distinct registers for each scaling list.
>>
>> Cheers,
>>
>> Paul
>>
>>> If so, why would userspace be forced to update lots of matrixes, if would
>>> likely use just one at a given time?
>>>
>>> IMO, the proper way would be, instead, to use an uAPI like:
>>>
>>> /*
>>>
>>>  * Actually, as this is uAPI, we will use a fixed size integer type, like
>>>  *  unsigned int
>>>  */
>>>
>>> enum hevc_scaling_matrix_type {
>>>
>>> 	HEVC_SCALING_MATRIX_4x4,
>>> 	HEVC_SCALING_MATRIX_8x8,
>>>
>>> ...
>>>
>>> 	HEVC_SCALING_MATRIX_DC_COEF_32x32,
>>>
>>> };
>>>
>>> struct v4l2_ctrl_hevc_scaling_matrix {
>>>
>>> 	__u32	scaling_type 		/* as defined by enum 
> hevc_scaling_matrix_type */
>>> 	
>>> 	union {
>>> 	
>>> 		__u8	scaling_list_4x4[6][16];
>>> 		__u8	scaling_list_8x8[6][64];
>>> 		__u8	scaling_list_16x16[6][64];
>>> 		__u8	scaling_list_32x32[2][64];
>>> 		__u8	scaling_list_dc_coef_16x16[6];
>>> 		__u8	scaling_list_dc_coef_32x32[2];
>>> 	
>>> 	};
>>>
>>> };
>>>
>>> And let the core use a default for each scaling matrix, if userspace
>>> doesn't set it.
>>>
>>>
>>>
>>> Cheers,
>>> Mauro
> 
> 
> 
> 

[PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Jernej Skrabec]

HEVC frames may use scaling list feature. Add support for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
 .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
 5 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
index c6ddd46eff82..bf68bc6b20c8 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
@@ -116,6 +116,13 @@ static const struct cedrus_control cedrus_controls[] = {
 		.codec		= CEDRUS_CODEC_H265,
 		.required	= true,
 	},
+	{
+		.cfg = {
+			.id	= V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
+		},
+		.codec		= CEDRUS_CODEC_H265,
+		.required	= true,
+	},
 	{
 		.cfg = {
 			.id	= V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 96765555ab8a..d945f4f0ff2d 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -73,6 +73,7 @@ struct cedrus_h265_run {
 	const struct v4l2_ctrl_hevc_sps			*sps;
 	const struct v4l2_ctrl_hevc_pps			*pps;
 	const struct v4l2_ctrl_hevc_slice_params	*slice_params;
+	const struct v4l2_ctrl_hevc_scaling_matrix	*scaling_matrix;
 };
 
 struct cedrus_run {
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index 4a2fc33a1d79..327ed6c264dc 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -66,6 +66,8 @@ void cedrus_device_run(void *priv)
 			V4L2_CID_MPEG_VIDEO_HEVC_PPS);
 		run.h265.slice_params = cedrus_find_control_data(ctx,
 			V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
+		run.h265.scaling_matrix = cedrus_find_control_data(ctx,
+			V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
 		break;
 
 	default:
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 6945dc74e1d7..888bfd5ca224 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -220,6 +220,69 @@ static void cedrus_h265_pred_weight_write(struct cedrus_dev *dev,
 	}
 }
 
+static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
+					   struct cedrus_run *run)
+{
+	const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
+	struct cedrus_dev *dev = ctx->dev;
+	u32 i, j, k, val;
+
+	scaling = run->h265.scaling_matrix;
+
+	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF0,
+		     (scaling->scaling_list_dc_coef_32x32[1] << 24) |
+		     (scaling->scaling_list_dc_coef_32x32[0] << 16) |
+		     (scaling->scaling_list_dc_coef_16x16[1] << 8) |
+		     (scaling->scaling_list_dc_coef_16x16[0] << 0));
+
+	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF1,
+		     (scaling->scaling_list_dc_coef_16x16[5] << 24) |
+		     (scaling->scaling_list_dc_coef_16x16[4] << 16) |
+		     (scaling->scaling_list_dc_coef_16x16[3] << 8) |
+		     (scaling->scaling_list_dc_coef_16x16[2] << 0));
+
+	cedrus_h265_sram_write_offset(dev, VE_DEC_H265_SRAM_OFFSET_SCALING_LISTS);
+
+	for (i = 0; i < 6; i++)
+		for (j = 0; j < 8; j++)
+			for (k = 0; k < 8; k += 4) {
+				val = ((u32)scaling->scaling_list_8x8[i][j + (k + 3) * 8] << 24) |
+				      ((u32)scaling->scaling_list_8x8[i][j + (k + 2) * 8] << 16) |
+				      ((u32)scaling->scaling_list_8x8[i][j + (k + 1) * 8] << 8) |
+				      scaling->scaling_list_8x8[i][j + k * 8];
+				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
+			}
+
+	for (i = 0; i < 2; i++)
+		for (j = 0; j < 8; j++)
+			for (k = 0; k < 8; k += 4) {
+				val = ((u32)scaling->scaling_list_32x32[i][j + (k + 3) * 8] << 24) |
+				      ((u32)scaling->scaling_list_32x32[i][j + (k + 2) * 8] << 16) |
+				      ((u32)scaling->scaling_list_32x32[i][j + (k + 1) * 8] << 8) |
+				      scaling->scaling_list_32x32[i][j + k * 8];
+				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
+			}
+
+	for (i = 0; i < 6; i++)
+		for (j = 0; j < 8; j++)
+			for (k = 0; k < 8; k += 4) {
+				val = ((u32)scaling->scaling_list_16x16[i][j + (k + 3) * 8] << 24) |
+				      ((u32)scaling->scaling_list_16x16[i][j + (k + 2) * 8] << 16) |
+				      ((u32)scaling->scaling_list_16x16[i][j + (k + 1) * 8] << 8) |
+				      scaling->scaling_list_16x16[i][j + k * 8];
+				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
+			}
+
+	for (i = 0; i < 6; i++)
+		for (j = 0; j < 4; j++) {
+			val = ((u32)scaling->scaling_list_4x4[i][j + 12] << 24) |
+			      ((u32)scaling->scaling_list_4x4[i][j + 8] << 16) |
+			      ((u32)scaling->scaling_list_4x4[i][j + 4] << 8) |
+			      scaling->scaling_list_4x4[i][j];
+			cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
+		}
+}
+
 static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 			      struct cedrus_run *run)
 {
@@ -499,7 +562,12 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 
 	/* Scaling list. */
 
-	reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
+	if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) {
+		cedrus_h265_write_scaling_list(ctx, run);
+		reg = VE_DEC_H265_SCALING_LIST_CTRL0_FLAG_ENABLED;
+	} else {
+		reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
+	}
 	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_CTRL0, reg);
 
 	/* Neightbor information address. */
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
index 7beb03d3bb39..0d9449fe2b28 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
@@ -492,6 +492,8 @@
 #define VE_DEC_H265_ENTRY_POINT_OFFSET_ADDR	(VE_ENGINE_DEC_H265 + 0x64)
 #define VE_DEC_H265_TILE_START_CTB		(VE_ENGINE_DEC_H265 + 0x68)
 #define VE_DEC_H265_TILE_END_CTB		(VE_ENGINE_DEC_H265 + 0x6c)
+#define VE_DEC_H265_SCALING_LIST_DC_COEF0	(VE_ENGINE_DEC_H265 + 0x78)
+#define VE_DEC_H265_SCALING_LIST_DC_COEF1	(VE_ENGINE_DEC_H265 + 0x7c)
 
 #define VE_DEC_H265_LOW_ADDR			(VE_ENGINE_DEC_H265 + 0x80)
 
-- 
2.24.0

Re: [PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Hans Verkuil]

On 12/13/19 5:04 PM, Jernej Skrabec wrote:
> HEVC frames may use scaling list feature. Add support for it.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
>  5 files changed, 81 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/media/sunxi/cedrus/cedrus.c
> index c6ddd46eff82..bf68bc6b20c8 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> @@ -116,6 +116,13 @@ static const struct cedrus_control cedrus_controls[] = {
>  		.codec		= CEDRUS_CODEC_H265,
>  		.required	= true,
>  	},
> +	{
> +		.cfg = {
> +			.id	= V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
> +		},
> +		.codec		= CEDRUS_CODEC_H265,
> +		.required	= true,

Should this be true? This means that existing applications are now
suddenly required to always pass the scaling matrix for every buffer.

Especially since the commit log says: 'HEVC frames *may* use scaling list
feature', indicating that this is an optional feature.

Regards,

	Hans

> +	},
>  	{
>  		.cfg = {
>  			.id	= V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
> index 96765555ab8a..d945f4f0ff2d 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -73,6 +73,7 @@ struct cedrus_h265_run {
>  	const struct v4l2_ctrl_hevc_sps			*sps;
>  	const struct v4l2_ctrl_hevc_pps			*pps;
>  	const struct v4l2_ctrl_hevc_slice_params	*slice_params;
> +	const struct v4l2_ctrl_hevc_scaling_matrix	*scaling_matrix;
>  };
>  
>  struct cedrus_run {
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> index 4a2fc33a1d79..327ed6c264dc 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -66,6 +66,8 @@ void cedrus_device_run(void *priv)
>  			V4L2_CID_MPEG_VIDEO_HEVC_PPS);
>  		run.h265.slice_params = cedrus_find_control_data(ctx,
>  			V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
> +		run.h265.scaling_matrix = cedrus_find_control_data(ctx,
> +			V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
>  		break;
>  
>  	default:
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> index 6945dc74e1d7..888bfd5ca224 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> @@ -220,6 +220,69 @@ static void cedrus_h265_pred_weight_write(struct cedrus_dev *dev,
>  	}
>  }
>  
> +static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
> +					   struct cedrus_run *run)
> +{
> +	const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
> +	struct cedrus_dev *dev = ctx->dev;
> +	u32 i, j, k, val;
> +
> +	scaling = run->h265.scaling_matrix;
> +
> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF0,
> +		     (scaling->scaling_list_dc_coef_32x32[1] << 24) |
> +		     (scaling->scaling_list_dc_coef_32x32[0] << 16) |
> +		     (scaling->scaling_list_dc_coef_16x16[1] << 8) |
> +		     (scaling->scaling_list_dc_coef_16x16[0] << 0));
> +
> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF1,
> +		     (scaling->scaling_list_dc_coef_16x16[5] << 24) |
> +		     (scaling->scaling_list_dc_coef_16x16[4] << 16) |
> +		     (scaling->scaling_list_dc_coef_16x16[3] << 8) |
> +		     (scaling->scaling_list_dc_coef_16x16[2] << 0));
> +
> +	cedrus_h265_sram_write_offset(dev, VE_DEC_H265_SRAM_OFFSET_SCALING_LISTS);
> +
> +	for (i = 0; i < 6; i++)
> +		for (j = 0; j < 8; j++)
> +			for (k = 0; k < 8; k += 4) {
> +				val = ((u32)scaling->scaling_list_8x8[i][j + (k + 3) * 8] << 24) |
> +				      ((u32)scaling->scaling_list_8x8[i][j + (k + 2) * 8] << 16) |
> +				      ((u32)scaling->scaling_list_8x8[i][j + (k + 1) * 8] << 8) |
> +				      scaling->scaling_list_8x8[i][j + k * 8];
> +				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
> +			}
> +
> +	for (i = 0; i < 2; i++)
> +		for (j = 0; j < 8; j++)
> +			for (k = 0; k < 8; k += 4) {
> +				val = ((u32)scaling->scaling_list_32x32[i][j + (k + 3) * 8] << 24) |
> +				      ((u32)scaling->scaling_list_32x32[i][j + (k + 2) * 8] << 16) |
> +				      ((u32)scaling->scaling_list_32x32[i][j + (k + 1) * 8] << 8) |
> +				      scaling->scaling_list_32x32[i][j + k * 8];
> +				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
> +			}
> +
> +	for (i = 0; i < 6; i++)
> +		for (j = 0; j < 8; j++)
> +			for (k = 0; k < 8; k += 4) {
> +				val = ((u32)scaling->scaling_list_16x16[i][j + (k + 3) * 8] << 24) |
> +				      ((u32)scaling->scaling_list_16x16[i][j + (k + 2) * 8] << 16) |
> +				      ((u32)scaling->scaling_list_16x16[i][j + (k + 1) * 8] << 8) |
> +				      scaling->scaling_list_16x16[i][j + k * 8];
> +				cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
> +			}
> +
> +	for (i = 0; i < 6; i++)
> +		for (j = 0; j < 4; j++) {
> +			val = ((u32)scaling->scaling_list_4x4[i][j + 12] << 24) |
> +			      ((u32)scaling->scaling_list_4x4[i][j + 8] << 16) |
> +			      ((u32)scaling->scaling_list_4x4[i][j + 4] << 8) |
> +			      scaling->scaling_list_4x4[i][j];
> +			cedrus_write(dev, VE_DEC_H265_SRAM_DATA, val);
> +		}
> +}
> +
>  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  			      struct cedrus_run *run)
>  {
> @@ -499,7 +562,12 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>  
>  	/* Scaling list. */
>  
> -	reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> +	if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) {
> +		cedrus_h265_write_scaling_list(ctx, run);
> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_FLAG_ENABLED;
> +	} else {
> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> +	}
>  	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_CTRL0, reg);
>  
>  	/* Neightbor information address. */
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> index 7beb03d3bb39..0d9449fe2b28 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> @@ -492,6 +492,8 @@
>  #define VE_DEC_H265_ENTRY_POINT_OFFSET_ADDR	(VE_ENGINE_DEC_H265 + 0x64)
>  #define VE_DEC_H265_TILE_START_CTB		(VE_ENGINE_DEC_H265 + 0x68)
>  #define VE_DEC_H265_TILE_END_CTB		(VE_ENGINE_DEC_H265 + 0x6c)
> +#define VE_DEC_H265_SCALING_LIST_DC_COEF0	(VE_ENGINE_DEC_H265 + 0x78)
> +#define VE_DEC_H265_SCALING_LIST_DC_COEF1	(VE_ENGINE_DEC_H265 + 0x7c)
>  
>  #define VE_DEC_H265_LOW_ADDR			(VE_ENGINE_DEC_H265 + 0x80)
>  
> 

Re: [PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Jernej Škrabec]

Hi!

Dne torek, 07. januar 2020 ob 16:01:16 CET je Hans Verkuil napisal(a):
> On 12/13/19 5:04 PM, Jernej Skrabec wrote:
> > HEVC frames may use scaling list feature. Add support for it.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
> >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
> >  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
> >  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
> >  5 files changed, 81 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> > c6ddd46eff82..bf68bc6b20c8 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> > @@ -116,6 +116,13 @@ static const struct cedrus_control cedrus_controls[]
> > = {> 
> >  		.codec		= CEDRUS_CODEC_H265,
> >  		.required	= true,
> >  	
> >  	},
> > 
> > +	{
> > +		.cfg = {
> > +			.id	= 
V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
> > +		},
> > +		.codec		= CEDRUS_CODEC_H265,
> > +		.required	= true,
> 
> Should this be true? This means that existing applications are now
> suddenly required to always pass the scaling matrix for every buffer.
> 
> Especially since the commit log says: 'HEVC frames *may* use scaling list
> feature', indicating that this is an optional feature.

True. Can you fix this when applying if this is the only issue?

Best regards,
Jernej

> 
> Regards,
> 
> 	Hans
> 
> > +	},
> > 
> >  	{
> >  	
> >  		.cfg = {
> >  		
> >  			.id	= 
V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> > 96765555ab8a..d945f4f0ff2d 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> > @@ -73,6 +73,7 @@ struct cedrus_h265_run {
> > 
> >  	const struct v4l2_ctrl_hevc_sps			*sps;
> >  	const struct v4l2_ctrl_hevc_pps			*pps;
> >  	const struct v4l2_ctrl_hevc_slice_params	*slice_params;
> > 
> > +	const struct v4l2_ctrl_hevc_scaling_matrix	
*scaling_matrix;
> > 
> >  };
> >  
> >  struct cedrus_run {
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index
> > 4a2fc33a1d79..327ed6c264dc 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> > @@ -66,6 +66,8 @@ void cedrus_device_run(void *priv)
> > 
> >  			V4L2_CID_MPEG_VIDEO_HEVC_PPS);
> >  		
> >  		run.h265.slice_params = cedrus_find_control_data(ctx,
> >  		
> >  			V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
> > 
> > +		run.h265.scaling_matrix = cedrus_find_control_data(ctx,
> > +			V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
> > 
> >  		break;
> >  	
> >  	default:
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> > 6945dc74e1d7..888bfd5ca224 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> > @@ -220,6 +220,69 @@ static void cedrus_h265_pred_weight_write(struct
> > cedrus_dev *dev,> 
> >  	}
> >  
> >  }
> > 
> > +static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
> > +					   struct cedrus_run 
*run)
> > +{
> > +	const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
> > +	struct cedrus_dev *dev = ctx->dev;
> > +	u32 i, j, k, val;
> > +
> > +	scaling = run->h265.scaling_matrix;
> > +
> > +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF0,
> > +		     (scaling->scaling_list_dc_coef_32x32[1] << 24) |
> > +		     (scaling->scaling_list_dc_coef_32x32[0] << 16) |
> > +		     (scaling->scaling_list_dc_coef_16x16[1] << 8) |
> > +		     (scaling->scaling_list_dc_coef_16x16[0] << 0));
> > +
> > +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF1,
> > +		     (scaling->scaling_list_dc_coef_16x16[5] << 24) |
> > +		     (scaling->scaling_list_dc_coef_16x16[4] << 16) |
> > +		     (scaling->scaling_list_dc_coef_16x16[3] << 8) |
> > +		     (scaling->scaling_list_dc_coef_16x16[2] << 0));
> > +
> > +	cedrus_h265_sram_write_offset(dev,
> > VE_DEC_H265_SRAM_OFFSET_SCALING_LISTS); +
> > +	for (i = 0; i < 6; i++)
> > +		for (j = 0; j < 8; j++)
> > +			for (k = 0; k < 8; k += 4) {
> > +				val = ((u32)scaling-
>scaling_list_8x8[i][j + (k + 3) * 8] << 24) |
> > +				      ((u32)scaling-
>scaling_list_8x8[i][j + (k + 2) * 8] << 16) |
> > +				      ((u32)scaling-
>scaling_list_8x8[i][j + (k + 1) * 8] << 8) |
> > +				      scaling-
>scaling_list_8x8[i][j + k * 8];
> > +				cedrus_write(dev, 
VE_DEC_H265_SRAM_DATA, val);
> > +			}
> > +
> > +	for (i = 0; i < 2; i++)
> > +		for (j = 0; j < 8; j++)
> > +			for (k = 0; k < 8; k += 4) {
> > +				val = ((u32)scaling-
>scaling_list_32x32[i][j + (k + 3) * 8] << 24) |
> > +				      ((u32)scaling-
>scaling_list_32x32[i][j + (k + 2) * 8] << 16) |
> > +				      ((u32)scaling-
>scaling_list_32x32[i][j + (k + 1) * 8] << 8) |
> > +				      scaling-
>scaling_list_32x32[i][j + k * 8];
> > +				cedrus_write(dev, 
VE_DEC_H265_SRAM_DATA, val);
> > +			}
> > +
> > +	for (i = 0; i < 6; i++)
> > +		for (j = 0; j < 8; j++)
> > +			for (k = 0; k < 8; k += 4) {
> > +				val = ((u32)scaling-
>scaling_list_16x16[i][j + (k + 3) * 8] << 24) |
> > +				      ((u32)scaling-
>scaling_list_16x16[i][j + (k + 2) * 8] << 16) |
> > +				      ((u32)scaling-
>scaling_list_16x16[i][j + (k + 1) * 8] << 8) |
> > +				      scaling-
>scaling_list_16x16[i][j + k * 8];
> > +				cedrus_write(dev, 
VE_DEC_H265_SRAM_DATA, val);
> > +			}
> > +
> > +	for (i = 0; i < 6; i++)
> > +		for (j = 0; j < 4; j++) {
> > +			val = ((u32)scaling->scaling_list_4x4[i][j + 
12] << 24) |
> > +			      ((u32)scaling->scaling_list_4x4[i][j + 
8] << 16) |
> > +			      ((u32)scaling->scaling_list_4x4[i][j + 
4] << 8) |
> > +			      scaling->scaling_list_4x4[i][j];
> > +			cedrus_write(dev, VE_DEC_H265_SRAM_DATA, 
val);
> > +		}
> > +}
> > +
> > 
> >  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> >  
> >  			      struct cedrus_run *run)
> >  
> >  {
> > 
> > @@ -499,7 +562,12 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> > 
> >  	/* Scaling list. */
> > 
> > -	reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> > +	if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) {
> > +		cedrus_h265_write_scaling_list(ctx, run);
> > +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_FLAG_ENABLED;
> > +	} else {
> > +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> > +	}
> > 
> >  	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_CTRL0, reg);
> >  	
> >  	/* Neightbor information address. */
> > 
> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> > 7beb03d3bb39..0d9449fe2b28 100644
> > --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> > @@ -492,6 +492,8 @@
> > 
> >  #define VE_DEC_H265_ENTRY_POINT_OFFSET_ADDR	(VE_ENGINE_DEC_H265 + 
0x64)
> >  #define VE_DEC_H265_TILE_START_CTB		(VE_ENGINE_DEC_H265 + 
0x68)
> >  #define VE_DEC_H265_TILE_END_CTB		(VE_ENGINE_DEC_H265 + 
0x6c)
> > 
> > +#define VE_DEC_H265_SCALING_LIST_DC_COEF0	(VE_ENGINE_DEC_H265 + 
0x78)
> > +#define VE_DEC_H265_SCALING_LIST_DC_COEF1	(VE_ENGINE_DEC_H265 + 
0x7c)
> > 
> >  #define VE_DEC_H265_LOW_ADDR			(VE_ENGINE_DEC_H265 + 
0x80)

Re: [PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Hans Verkuil]

On 1/7/20 6:10 PM, Jernej Škrabec wrote:
> Hi!
> 
> Dne torek, 07. januar 2020 ob 16:01:16 CET je Hans Verkuil napisal(a):
>> On 12/13/19 5:04 PM, Jernej Skrabec wrote:
>>> HEVC frames may use scaling list feature. Add support for it.
>>>
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
>>> ---
>>>
>>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
>>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>>>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
>>>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
>>>  5 files changed, 81 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
>>> c6ddd46eff82..bf68bc6b20c8 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> @@ -116,6 +116,13 @@ static const struct cedrus_control cedrus_controls[]
>>> = {> 
>>>  		.codec		= CEDRUS_CODEC_H265,
>>>  		.required	= true,
>>>  	
>>>  	},
>>>
>>> +	{
>>> +		.cfg = {
>>> +			.id	= 
> V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
>>> +		},
>>> +		.codec		= CEDRUS_CODEC_H265,
>>> +		.required	= true,
>>
>> Should this be true? This means that existing applications are now
>> suddenly required to always pass the scaling matrix for every buffer.
>>
>> Especially since the commit log says: 'HEVC frames *may* use scaling list
>> feature', indicating that this is an optional feature.
> 
> True. Can you fix this when applying if this is the only issue?

I'll do that.

Regards,

	Hans

Re: [PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Hans Verkuil]

On 1/7/20 6:10 PM, Jernej Škrabec wrote:
> Hi!
> 
> Dne torek, 07. januar 2020 ob 16:01:16 CET je Hans Verkuil napisal(a):
>> On 12/13/19 5:04 PM, Jernej Skrabec wrote:
>>> HEVC frames may use scaling list feature. Add support for it.
>>>
>>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
>>> ---
>>>
>>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
>>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>>>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
>>>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
>>>  5 files changed, 81 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
>>> c6ddd46eff82..bf68bc6b20c8 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
>>> @@ -116,6 +116,13 @@ static const struct cedrus_control cedrus_controls[]
>>> = {> 
>>>  		.codec		= CEDRUS_CODEC_H265,
>>>  		.required	= true,
>>>  	
>>>  	},
>>>
>>> +	{
>>> +		.cfg = {
>>> +			.id	= 
> V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
>>> +		},
>>> +		.codec		= CEDRUS_CODEC_H265,
>>> +		.required	= true,
>>
>> Should this be true? This means that existing applications are now
>> suddenly required to always pass the scaling matrix for every buffer.
>>
>> Especially since the commit log says: 'HEVC frames *may* use scaling list
>> feature', indicating that this is an optional feature.
> 
> True. Can you fix this when applying if this is the only issue?

I realized that after changing this to false, you also need to document
what happens if you do NOT set this control in the request.

Does it fall back to default values? It looks like the HEVC spec defines
some defaults (if I understand it correctly).

Regards,

	Hans

> 
> Best regards,
> Jernej
> 
>>
>> Regards,
>>
>> 	Hans
>>
>>> +	},
>>>
>>>  	{
>>>  	
>>>  		.cfg = {
>>>  		
>>>  			.id	= 
> V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> b/drivers/staging/media/sunxi/cedrus/cedrus.h index
>>> 96765555ab8a..d945f4f0ff2d 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
>>> @@ -73,6 +73,7 @@ struct cedrus_h265_run {
>>>
>>>  	const struct v4l2_ctrl_hevc_sps			*sps;
>>>  	const struct v4l2_ctrl_hevc_pps			*pps;
>>>  	const struct v4l2_ctrl_hevc_slice_params	*slice_params;
>>>
>>> +	const struct v4l2_ctrl_hevc_scaling_matrix	
> *scaling_matrix;
>>>
>>>  };
>>>  
>>>  struct cedrus_run {
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index
>>> 4a2fc33a1d79..327ed6c264dc 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>>> @@ -66,6 +66,8 @@ void cedrus_device_run(void *priv)
>>>
>>>  			V4L2_CID_MPEG_VIDEO_HEVC_PPS);
>>>  		
>>>  		run.h265.slice_params = cedrus_find_control_data(ctx,
>>>  		
>>>  			V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
>>>
>>> +		run.h265.scaling_matrix = cedrus_find_control_data(ctx,
>>> +			V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
>>>
>>>  		break;
>>>  	
>>>  	default:
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>>> b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
>>> 6945dc74e1d7..888bfd5ca224 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
>>> @@ -220,6 +220,69 @@ static void cedrus_h265_pred_weight_write(struct
>>> cedrus_dev *dev,> 
>>>  	}
>>>  
>>>  }
>>>
>>> +static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
>>> +					   struct cedrus_run 
> *run)
>>> +{
>>> +	const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
>>> +	struct cedrus_dev *dev = ctx->dev;
>>> +	u32 i, j, k, val;
>>> +
>>> +	scaling = run->h265.scaling_matrix;
>>> +
>>> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF0,
>>> +		     (scaling->scaling_list_dc_coef_32x32[1] << 24) |
>>> +		     (scaling->scaling_list_dc_coef_32x32[0] << 16) |
>>> +		     (scaling->scaling_list_dc_coef_16x16[1] << 8) |
>>> +		     (scaling->scaling_list_dc_coef_16x16[0] << 0));
>>> +
>>> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF1,
>>> +		     (scaling->scaling_list_dc_coef_16x16[5] << 24) |
>>> +		     (scaling->scaling_list_dc_coef_16x16[4] << 16) |
>>> +		     (scaling->scaling_list_dc_coef_16x16[3] << 8) |
>>> +		     (scaling->scaling_list_dc_coef_16x16[2] << 0));
>>> +
>>> +	cedrus_h265_sram_write_offset(dev,
>>> VE_DEC_H265_SRAM_OFFSET_SCALING_LISTS); +
>>> +	for (i = 0; i < 6; i++)
>>> +		for (j = 0; j < 8; j++)
>>> +			for (k = 0; k < 8; k += 4) {
>>> +				val = ((u32)scaling-
>> scaling_list_8x8[i][j + (k + 3) * 8] << 24) |
>>> +				      ((u32)scaling-
>> scaling_list_8x8[i][j + (k + 2) * 8] << 16) |
>>> +				      ((u32)scaling-
>> scaling_list_8x8[i][j + (k + 1) * 8] << 8) |
>>> +				      scaling-
>> scaling_list_8x8[i][j + k * 8];
>>> +				cedrus_write(dev, 
> VE_DEC_H265_SRAM_DATA, val);
>>> +			}
>>> +
>>> +	for (i = 0; i < 2; i++)
>>> +		for (j = 0; j < 8; j++)
>>> +			for (k = 0; k < 8; k += 4) {
>>> +				val = ((u32)scaling-
>> scaling_list_32x32[i][j + (k + 3) * 8] << 24) |
>>> +				      ((u32)scaling-
>> scaling_list_32x32[i][j + (k + 2) * 8] << 16) |
>>> +				      ((u32)scaling-
>> scaling_list_32x32[i][j + (k + 1) * 8] << 8) |
>>> +				      scaling-
>> scaling_list_32x32[i][j + k * 8];
>>> +				cedrus_write(dev, 
> VE_DEC_H265_SRAM_DATA, val);
>>> +			}
>>> +
>>> +	for (i = 0; i < 6; i++)
>>> +		for (j = 0; j < 8; j++)
>>> +			for (k = 0; k < 8; k += 4) {
>>> +				val = ((u32)scaling-
>> scaling_list_16x16[i][j + (k + 3) * 8] << 24) |
>>> +				      ((u32)scaling-
>> scaling_list_16x16[i][j + (k + 2) * 8] << 16) |
>>> +				      ((u32)scaling-
>> scaling_list_16x16[i][j + (k + 1) * 8] << 8) |
>>> +				      scaling-
>> scaling_list_16x16[i][j + k * 8];
>>> +				cedrus_write(dev, 
> VE_DEC_H265_SRAM_DATA, val);
>>> +			}
>>> +
>>> +	for (i = 0; i < 6; i++)
>>> +		for (j = 0; j < 4; j++) {
>>> +			val = ((u32)scaling->scaling_list_4x4[i][j + 
> 12] << 24) |
>>> +			      ((u32)scaling->scaling_list_4x4[i][j + 
> 8] << 16) |
>>> +			      ((u32)scaling->scaling_list_4x4[i][j + 
> 4] << 8) |
>>> +			      scaling->scaling_list_4x4[i][j];
>>> +			cedrus_write(dev, VE_DEC_H265_SRAM_DATA, 
> val);
>>> +		}
>>> +}
>>> +
>>>
>>>  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>>>  
>>>  			      struct cedrus_run *run)
>>>  
>>>  {
>>>
>>> @@ -499,7 +562,12 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
>>>
>>>  	/* Scaling list. */
>>>
>>> -	reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
>>> +	if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) {
>>> +		cedrus_h265_write_scaling_list(ctx, run);
>>> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_FLAG_ENABLED;
>>> +	} else {
>>> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
>>> +	}
>>>
>>>  	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_CTRL0, reg);
>>>  	
>>>  	/* Neightbor information address. */
>>>
>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
>>> b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
>>> 7beb03d3bb39..0d9449fe2b28 100644
>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
>>> @@ -492,6 +492,8 @@
>>>
>>>  #define VE_DEC_H265_ENTRY_POINT_OFFSET_ADDR	(VE_ENGINE_DEC_H265 + 
> 0x64)
>>>  #define VE_DEC_H265_TILE_START_CTB		(VE_ENGINE_DEC_H265 + 
> 0x68)
>>>  #define VE_DEC_H265_TILE_END_CTB		(VE_ENGINE_DEC_H265 + 
> 0x6c)
>>>
>>> +#define VE_DEC_H265_SCALING_LIST_DC_COEF0	(VE_ENGINE_DEC_H265 + 
> 0x78)
>>> +#define VE_DEC_H265_SCALING_LIST_DC_COEF1	(VE_ENGINE_DEC_H265 + 
> 0x7c)
>>>
>>>  #define VE_DEC_H265_LOW_ADDR			(VE_ENGINE_DEC_H265 + 
> 0x80)
> 
> 
> 
> 

Re: [PATCH v2 2/4] media: cedrus: hevc: Add support for scaling matrix

[Jernej Škrabec]

Dne sreda, 08. januar 2020 ob 15:46:50 CET je Hans Verkuil napisal(a):
> On 1/7/20 6:10 PM, Jernej Škrabec wrote:
> > Hi!
> > 
> > Dne torek, 07. januar 2020 ob 16:01:16 CET je Hans Verkuil napisal(a):
> >> On 12/13/19 5:04 PM, Jernej Skrabec wrote:
> >>> HEVC frames may use scaling list feature. Add support for it.
> >>> 
> >>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> >>> ---
> >>> 
> >>>  drivers/staging/media/sunxi/cedrus/cedrus.c   |  7 ++
> >>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  1 +
> >>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
> >>>  .../staging/media/sunxi/cedrus/cedrus_h265.c  | 70 ++++++++++++++++++-
> >>>  .../staging/media/sunxi/cedrus/cedrus_regs.h  |  2 +
> >>>  5 files changed, 81 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c
> >>> b/drivers/staging/media/sunxi/cedrus/cedrus.c index
> >>> c6ddd46eff82..bf68bc6b20c8 100644
> >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
> >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
> >>> @@ -116,6 +116,13 @@ static const struct cedrus_control
> >>> cedrus_controls[]
> >>> = {>
> >>> 
> >>>  		.codec		= CEDRUS_CODEC_H265,
> >>>  		.required	= true,
> >>>  	
> >>>  	},
> >>> 
> >>> +	{
> >>> +		.cfg = {
> >>> +			.id	=
> > 
> > V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX,
> > 
> >>> +		},
> >>> +		.codec		= CEDRUS_CODEC_H265,
> >>> +		.required	= true,
> >> 
> >> Should this be true? This means that existing applications are now
> >> suddenly required to always pass the scaling matrix for every buffer.
> >> 
> >> Especially since the commit log says: 'HEVC frames *may* use scaling list
> >> feature', indicating that this is an optional feature.
> > 
> > True. Can you fix this when applying if this is the only issue?
> 
> I realized that after changing this to false, you also need to document
> what happens if you do NOT set this control in the request.
> 
> Does it fall back to default values? It looks like the HEVC spec defines
> some defaults (if I understand it correctly).

This control is needed only when V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED is 
set. If flag is set but control is not provided, then it doesn't really matter 
if default values or old values of control are used, image will be incorrectly 
decoded anyway. I think it's more important that buffer is marked with 
VB2_BUF_STATE_ERROR in such case.

Best regards,
Jernej

> 
> Regards,
> 
> 	Hans
> 
> > Best regards,
> > Jernej
> > 
> >> Regards,
> >> 
> >> 	Hans
> >> 	
> >>> +	},
> >>> 
> >>>  	{
> >>>  	
> >>>  		.cfg = {
> >>>  		
> >>>  			.id	=
> > 
> > V4L2_CID_MPEG_VIDEO_HEVC_DECODE_MODE,
> > 
> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h
> >>> b/drivers/staging/media/sunxi/cedrus/cedrus.h index
> >>> 96765555ab8a..d945f4f0ff2d 100644
> >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> >>> @@ -73,6 +73,7 @@ struct cedrus_h265_run {
> >>> 
> >>>  	const struct v4l2_ctrl_hevc_sps			*sps;
> >>>  	const struct v4l2_ctrl_hevc_pps			*pps;
> >>>  	const struct v4l2_ctrl_hevc_slice_params	*slice_params;
> >>> 
> >>> +	const struct v4l2_ctrl_hevc_scaling_matrix
> > 
> > *scaling_matrix;
> > 
> >>>  };
> >>>  
> >>>  struct cedrus_run {
> >>> 
> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> >>> b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c index
> >>> 4a2fc33a1d79..327ed6c264dc 100644
> >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> >>> @@ -66,6 +66,8 @@ void cedrus_device_run(void *priv)
> >>> 
> >>>  			V4L2_CID_MPEG_VIDEO_HEVC_PPS);
> >>>  		
> >>>  		run.h265.slice_params = cedrus_find_control_data(ctx,
> >>>  		
> >>>  			V4L2_CID_MPEG_VIDEO_HEVC_SLICE_PARAMS);
> >>> 
> >>> +		run.h265.scaling_matrix = cedrus_find_control_data(ctx,
> >>> +			V4L2_CID_MPEG_VIDEO_HEVC_SCALING_MATRIX);
> >>> 
> >>>  		break;
> >>>  	
> >>>  	default:
> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> >>> b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c index
> >>> 6945dc74e1d7..888bfd5ca224 100644
> >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
> >>> @@ -220,6 +220,69 @@ static void cedrus_h265_pred_weight_write(struct
> >>> cedrus_dev *dev,>
> >>> 
> >>>  	}
> >>>  
> >>>  }
> >>> 
> >>> +static void cedrus_h265_write_scaling_list(struct cedrus_ctx *ctx,
> >>> +					   struct cedrus_run
> > 
> > *run)
> > 
> >>> +{
> >>> +	const struct v4l2_ctrl_hevc_scaling_matrix *scaling;
> >>> +	struct cedrus_dev *dev = ctx->dev;
> >>> +	u32 i, j, k, val;
> >>> +
> >>> +	scaling = run->h265.scaling_matrix;
> >>> +
> >>> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF0,
> >>> +		     (scaling->scaling_list_dc_coef_32x32[1] << 24) |
> >>> +		     (scaling->scaling_list_dc_coef_32x32[0] << 16) |
> >>> +		     (scaling->scaling_list_dc_coef_16x16[1] << 8) |
> >>> +		     (scaling->scaling_list_dc_coef_16x16[0] << 0));
> >>> +
> >>> +	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_DC_COEF1,
> >>> +		     (scaling->scaling_list_dc_coef_16x16[5] << 24) |
> >>> +		     (scaling->scaling_list_dc_coef_16x16[4] << 16) |
> >>> +		     (scaling->scaling_list_dc_coef_16x16[3] << 8) |
> >>> +		     (scaling->scaling_list_dc_coef_16x16[2] << 0));
> >>> +
> >>> +	cedrus_h265_sram_write_offset(dev,
> >>> VE_DEC_H265_SRAM_OFFSET_SCALING_LISTS); +
> >>> +	for (i = 0; i < 6; i++)
> >>> +		for (j = 0; j < 8; j++)
> >>> +			for (k = 0; k < 8; k += 4) {
> >>> +				val = ((u32)scaling-
> >> 
> >> scaling_list_8x8[i][j + (k + 3) * 8] << 24) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_8x8[i][j + (k + 2) * 8] << 16) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_8x8[i][j + (k + 1) * 8] << 8) |
> >> 
> >>> +				      scaling-
> >> 
> >> scaling_list_8x8[i][j + k * 8];
> >> 
> >>> +				cedrus_write(dev,
> > 
> > VE_DEC_H265_SRAM_DATA, val);
> > 
> >>> +			}
> >>> +
> >>> +	for (i = 0; i < 2; i++)
> >>> +		for (j = 0; j < 8; j++)
> >>> +			for (k = 0; k < 8; k += 4) {
> >>> +				val = ((u32)scaling-
> >> 
> >> scaling_list_32x32[i][j + (k + 3) * 8] << 24) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_32x32[i][j + (k + 2) * 8] << 16) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_32x32[i][j + (k + 1) * 8] << 8) |
> >> 
> >>> +				      scaling-
> >> 
> >> scaling_list_32x32[i][j + k * 8];
> >> 
> >>> +				cedrus_write(dev,
> > 
> > VE_DEC_H265_SRAM_DATA, val);
> > 
> >>> +			}
> >>> +
> >>> +	for (i = 0; i < 6; i++)
> >>> +		for (j = 0; j < 8; j++)
> >>> +			for (k = 0; k < 8; k += 4) {
> >>> +				val = ((u32)scaling-
> >> 
> >> scaling_list_16x16[i][j + (k + 3) * 8] << 24) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_16x16[i][j + (k + 2) * 8] << 16) |
> >> 
> >>> +				      ((u32)scaling-
> >> 
> >> scaling_list_16x16[i][j + (k + 1) * 8] << 8) |
> >> 
> >>> +				      scaling-
> >> 
> >> scaling_list_16x16[i][j + k * 8];
> >> 
> >>> +				cedrus_write(dev,
> > 
> > VE_DEC_H265_SRAM_DATA, val);
> > 
> >>> +			}
> >>> +
> >>> +	for (i = 0; i < 6; i++)
> >>> +		for (j = 0; j < 4; j++) {
> >>> +			val = ((u32)scaling->scaling_list_4x4[i][j +
> > 
> > 12] << 24) |
> > 
> >>> +			      ((u32)scaling->scaling_list_4x4[i][j +
> > 
> > 8] << 16) |
> > 
> >>> +			      ((u32)scaling->scaling_list_4x4[i][j +
> > 
> > 4] << 8) |
> > 
> >>> +			      scaling->scaling_list_4x4[i][j];
> >>> +			cedrus_write(dev, VE_DEC_H265_SRAM_DATA,
> > 
> > val);
> > 
> >>> +		}
> >>> +}
> >>> +
> >>> 
> >>>  static void cedrus_h265_setup(struct cedrus_ctx *ctx,
> >>>  
> >>>  			      struct cedrus_run *run)
> >>>  
> >>>  {
> >>> 
> >>> @@ -499,7 +562,12 @@ static void cedrus_h265_setup(struct cedrus_ctx
> >>> *ctx,
> >>> 
> >>>  	/* Scaling list. */
> >>> 
> >>> -	reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> >>> +	if (sps->flags & V4L2_HEVC_SPS_FLAG_SCALING_LIST_ENABLED) {
> >>> +		cedrus_h265_write_scaling_list(ctx, run);
> >>> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_FLAG_ENABLED;
> >>> +	} else {
> >>> +		reg = VE_DEC_H265_SCALING_LIST_CTRL0_DEFAULT;
> >>> +	}
> >>> 
> >>>  	cedrus_write(dev, VE_DEC_H265_SCALING_LIST_CTRL0, reg);
> >>>  	
> >>>  	/* Neightbor information address. */
> >>> 
> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> >>> b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h index
> >>> 7beb03d3bb39..0d9449fe2b28 100644
> >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_regs.h
> >>> @@ -492,6 +492,8 @@
> >>> 
> >>>  #define VE_DEC_H265_ENTRY_POINT_OFFSET_ADDR	(VE_ENGINE_DEC_H265 +
> > 
> > 0x64)
> > 
> >>>  #define VE_DEC_H265_TILE_START_CTB		(VE_ENGINE_DEC_H265 +
> > 
> > 0x68)
> > 
> >>>  #define VE_DEC_H265_TILE_END_CTB		(VE_ENGINE_DEC_H265 +
> > 
> > 0x6c)
> > 
> >>> +#define VE_DEC_H265_SCALING_LIST_DC_COEF0	(VE_ENGINE_DEC_H265 +
> > 
> > 0x78)
> > 
> >>> +#define VE_DEC_H265_SCALING_LIST_DC_COEF1	(VE_ENGINE_DEC_H265 +
> > 
> > 0x7c)
> > 
> >>>  #define VE_DEC_H265_LOW_ADDR			
(VE_ENGINE_DEC_H265 +
> > 
> > 0x80)

[PATCH v2 3/4] media: uapi: hevc: Add segment address field

[Jernej Skrabec]

If HEVC frame consists of multiple slices, segment address has to be
known in order to properly decode it.

Add segment address field to slice parameters.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 5 ++++-
 include/media/hevc-ctrls.h                       | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
index aab1451e54d4..5415d5babcc2 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
@@ -3975,6 +3975,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
     * - __u32
       - ``data_bit_offset``
       - Offset (in bits) to the video data in the current slice data.
+    * - __u32
+      - ``slice_segment_addr``
+      -
     * - __u8
       - ``nal_unit_type``
       -
@@ -4052,7 +4055,7 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
       - ``num_rps_poc_lt_curr``
       - The number of reference pictures in the long-term set.
     * - __u8
-      - ``padding[7]``
+      - ``padding[5]``
       - Applications and drivers must set this to zero.
     * - struct :c:type:`v4l2_hevc_dpb_entry`
       - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
index 1592e52c3614..3e2e32098312 100644
--- a/include/media/hevc-ctrls.h
+++ b/include/media/hevc-ctrls.h
@@ -167,6 +167,9 @@ struct v4l2_ctrl_hevc_slice_params {
 	__u32	bit_size;
 	__u32	data_bit_offset;
 
+	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
+	__u32	slice_segment_addr;
+
 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
 	__u8	nal_unit_type;
 	__u8	nuh_temporal_id_plus1;
@@ -200,7 +203,7 @@ struct v4l2_ctrl_hevc_slice_params {
 	__u8	num_rps_poc_st_curr_after;
 	__u8	num_rps_poc_lt_curr;
 
-	__u8	padding;
+	__u8	padding[5];
 
 	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
 	struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
-- 
2.24.0

Re: [PATCH v2 3/4] media: uapi: hevc: Add segment address field

[Mauro Carvalho Chehab]

Em Fri, 13 Dec 2019 17:04:27 +0100
Jernej Skrabec <jernej.skrabec@siol.net> escreveu:

> If HEVC frame consists of multiple slices, segment address has to be
> known in order to properly decode it.
> 
> Add segment address field to slice parameters.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 5 ++++-
>  include/media/hevc-ctrls.h                       | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> index aab1451e54d4..5415d5babcc2 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> @@ -3975,6 +3975,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>      * - __u32
>        - ``data_bit_offset``
>        - Offset (in bits) to the video data in the current slice data.
> +    * - __u32
> +      - ``slice_segment_addr``
> +      -
>      * - __u8
>        - ``nal_unit_type``
>        -
> @@ -4052,7 +4055,7 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>        - ``num_rps_poc_lt_curr``
>        - The number of reference pictures in the long-term set.
>      * - __u8
> -      - ``padding[7]``
> +      - ``padding[5]``
>        - Applications and drivers must set this to zero.
>      * - struct :c:type:`v4l2_hevc_dpb_entry`
>        - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> index 1592e52c3614..3e2e32098312 100644
> --- a/include/media/hevc-ctrls.h
> +++ b/include/media/hevc-ctrls.h
> @@ -167,6 +167,9 @@ struct v4l2_ctrl_hevc_slice_params {
>  	__u32	bit_size;
>  	__u32	data_bit_offset;
>  
> +	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
> +	__u32	slice_segment_addr;
> +

Why are you adding it in the middle of the data? This will break any 
existing userspace code that might be relying on it.

Ok, I know that this header is not yet under include/uapi,and there's a
warning on it for letting people know that it shouldn't be used anywhere.

Still, people might be using it.

>  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
>  	__u8	nal_unit_type;
>  	__u8	nuh_temporal_id_plus1;
> @@ -200,7 +203,7 @@ struct v4l2_ctrl_hevc_slice_params {
>  	__u8	num_rps_poc_st_curr_after;
>  	__u8	num_rps_poc_lt_curr;
>  
> -	__u8	padding;
> +	__u8	padding[5];
>  
>  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header */
>  	struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];




Cheers,
Mauro

Re: [PATCH v2 3/4] media: uapi: hevc: Add segment address field

[Jernej Škrabec]

Hi!

Dne sreda, 08. januar 2020 ob 15:31:26 CET je Mauro Carvalho Chehab 
napisal(a):
> Em Fri, 13 Dec 2019 17:04:27 +0100
> 
> Jernej Skrabec <jernej.skrabec@siol.net> escreveu:
> > If HEVC frame consists of multiple slices, segment address has to be
> > known in order to properly decode it.
> > 
> > Add segment address field to slice parameters.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  Documentation/media/uapi/v4l/ext-ctrls-codec.rst | 5 ++++-
> >  include/media/hevc-ctrls.h                       | 5 ++++-
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst index
> > aab1451e54d4..5415d5babcc2 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-codec.rst
> > @@ -3975,6 +3975,9 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > 
> >      * - __u32
> >      
> >        - ``data_bit_offset``
> >        - Offset (in bits) to the video data in the current slice data.
> > 
> > +    * - __u32
> > +      - ``slice_segment_addr``
> > +      -
> > 
> >      * - __u8
> >      
> >        - ``nal_unit_type``
> >        -
> > 
> > @@ -4052,7 +4055,7 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
> > 
> >        - ``num_rps_poc_lt_curr``
> >        - The number of reference pictures in the long-term set.
> >      
> >      * - __u8
> > 
> > -      - ``padding[7]``
> > +      - ``padding[5]``
> > 
> >        - Applications and drivers must set this to zero.
> >      
> >      * - struct :c:type:`v4l2_hevc_dpb_entry`
> >      
> >        - ``dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
> > 
> > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
> > index 1592e52c3614..3e2e32098312 100644
> > --- a/include/media/hevc-ctrls.h
> > +++ b/include/media/hevc-ctrls.h
> > @@ -167,6 +167,9 @@ struct v4l2_ctrl_hevc_slice_params {
> > 
> >  	__u32	bit_size;
> >  	__u32	data_bit_offset;
> > 
> > +	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header 
*/
> > +	__u32	slice_segment_addr;
> > +
> 
> Why are you adding it in the middle of the data? This will break any
> existing userspace code that might be relying on it.
> 
> Ok, I know that this header is not yet under include/uapi,and there's a
> warning on it for letting people know that it shouldn't be used anywhere.
> 
> Still, people might be using it.

I did it that way to keep fields aligned. Cedrus is currently the only 
mainlined driver to use this structure and userspace users are also very rare 
at this time.

Anyway, not all HEVC features are supported at this time, which means that 
this structure might grow. Once that happens, you'll lose binary compatibility 
anyway, but source compatibility should be fine.

Best regards,
Jernej

> 
> >  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: NAL unit header */
> >  	__u8	nal_unit_type;
> >  	__u8	nuh_temporal_id_plus1;
> > 
> > @@ -200,7 +203,7 @@ struct v4l2_ctrl_hevc_slice_params {
> > 
> >  	__u8	num_rps_poc_st_curr_after;
> >  	__u8	num_rps_poc_lt_curr;
> > 
> > -	__u8	padding;
> > +	__u8	padding[5];
> > 
> >  	/* ISO/IEC 23008-2, ITU-T Rec. H.265: General slice segment header 
*/
> >  	struct v4l2_hevc_dpb_entry dpb[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> 
> Cheers,
> Mauro

[PATCH v2 4/4] media: cedrus: hevc: Add support for multiple slices

[Jernej Skrabec]

Now that segment address is available, support for multi-slice frames
can be easily added.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 .../staging/media/sunxi/cedrus/cedrus_h265.c  | 26 ++++++++++++-------
 .../staging/media/sunxi/cedrus/cedrus_video.c |  1 +
 2 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
index 888bfd5ca224..109d3289418c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_h265.c
@@ -291,6 +291,8 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	const struct v4l2_ctrl_hevc_pps *pps;
 	const struct v4l2_ctrl_hevc_slice_params *slice_params;
 	const struct v4l2_hevc_pred_weight_table *pred_weight_table;
+	unsigned int width_in_ctb_luma, ctb_size_luma;
+	unsigned int log2_max_luma_coding_block_size;
 	dma_addr_t src_buf_addr;
 	dma_addr_t src_buf_end_addr;
 	u32 chroma_log2_weight_denom;
@@ -303,15 +305,17 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	slice_params = run->h265.slice_params;
 	pred_weight_table = &slice_params->pred_weight_table;
 
+	log2_max_luma_coding_block_size =
+		sps->log2_min_luma_coding_block_size_minus3 + 3 +
+		sps->log2_diff_max_min_luma_coding_block_size;
+	ctb_size_luma = 1UL << log2_max_luma_coding_block_size;
+	width_in_ctb_luma =
+		DIV_ROUND_UP(sps->pic_width_in_luma_samples, ctb_size_luma);
+
 	/* MV column buffer size and allocation. */
 	if (!ctx->codec.h265.mv_col_buf_size) {
 		unsigned int num_buffers =
 			run->dst->vb2_buf.vb2_queue->num_buffers;
-		unsigned int log2_max_luma_coding_block_size =
-			sps->log2_min_luma_coding_block_size_minus3 + 3 +
-			sps->log2_diff_max_min_luma_coding_block_size;
-		unsigned int ctb_size_luma =
-			1UL << log2_max_luma_coding_block_size;
 
 		/*
 		 * Each CTB requires a MV col buffer with a specific unit size.
@@ -366,15 +370,17 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 	reg = VE_DEC_H265_BITS_END_ADDR_BASE(src_buf_end_addr);
 	cedrus_write(dev, VE_DEC_H265_BITS_END_ADDR, reg);
 
-	/* Coding tree block address: start at the beginning. */
-	reg = VE_DEC_H265_DEC_CTB_ADDR_X(0) | VE_DEC_H265_DEC_CTB_ADDR_Y(0);
+	/* Coding tree block address */
+	reg = VE_DEC_H265_DEC_CTB_ADDR_X(slice_params->slice_segment_addr % width_in_ctb_luma);
+	reg |= VE_DEC_H265_DEC_CTB_ADDR_Y(slice_params->slice_segment_addr / width_in_ctb_luma);
 	cedrus_write(dev, VE_DEC_H265_DEC_CTB_ADDR, reg);
 
 	cedrus_write(dev, VE_DEC_H265_TILE_START_CTB, 0);
 	cedrus_write(dev, VE_DEC_H265_TILE_END_CTB, 0);
 
 	/* Clear the number of correctly-decoded coding tree blocks. */
-	cedrus_write(dev, VE_DEC_H265_DEC_CTB_NUM, 0);
+	if (ctx->fh.m2m_ctx->new_frame)
+		cedrus_write(dev, VE_DEC_H265_DEC_CTB_NUM, 0);
 
 	/* Initialize bitstream access. */
 	cedrus_write(dev, VE_DEC_H265_TRIGGER, VE_DEC_H265_TRIGGER_INIT_SWDEC);
@@ -523,8 +529,8 @@ static void cedrus_h265_setup(struct cedrus_ctx *ctx,
 				V4L2_HEVC_PPS_FLAG_DEPENDENT_SLICE_SEGMENT,
 				pps->flags);
 
-	/* FIXME: For multi-slice support. */
-	reg |= VE_DEC_H265_DEC_SLICE_HDR_INFO0_FLAG_FIRST_SLICE_SEGMENT_IN_PIC;
+	if (ctx->fh.m2m_ctx->new_frame)
+		reg |= VE_DEC_H265_DEC_SLICE_HDR_INFO0_FLAG_FIRST_SLICE_SEGMENT_IN_PIC;
 
 	cedrus_write(dev, VE_DEC_H265_DEC_SLICE_HDR_INFO0, reg);
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 15cf1f10221b..497b1199d3fe 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -311,6 +311,7 @@ static int cedrus_s_fmt_vid_out(struct file *file, void *priv,
 
 	switch (ctx->src_fmt.pixelformat) {
 	case V4L2_PIX_FMT_H264_SLICE:
+	case V4L2_PIX_FMT_HEVC_SLICE:
 		vq->subsystem_flags |=
 			VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
 		break;
-- 
2.24.0