Re: [PATCH 12/14] media: medkatek: vcodec: set secure mode to decoder driver

Hi Hans,

Thanks for your advice.
On Fri, 2023-09-15 at 10:54 +0200, Hans Verkuil wrote:

 	 
External email : Please do not click links or open attachments until
you have verified the sender or the content.
 On 15/09/2023 10:25, Yunfei Dong (董云飞) wrote:

quoted

Hi Hans & Nicolas,

Thanks for your advice.

On Tue, 2023-09-12 at 11:30 +0200, Hans Verkuil wrote:

quoted

  
External email : Please do not click links or open attachments

until

quoted

quoted

you have verified the sender or the content.
 Hi,

On 9/11/23 17:54, Nicolas Dufresne wrote:

quoted

Hi,

Le lundi 11 septembre 2023 à 20:59 +0800, Yunfei Dong a écrit :

quoted

Setting secure mode flag to kernel when trying to play secure

video,

quoted

quoted

then decoder driver will initialize tee related interface to

support

quoted

quoted

svp.

This is not what the patch is doing, please rework. This patch is

an vendor API

quoted

addition introducing V4L2_CID_MPEG_MTK_SET_SECURE_MODE. I should

not have to

quoted

read your patch to understand this.

quoted

Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
---
 .../vcodec/decoder/mtk_vcodec_dec_stateless.c     | 15

++++++++++++++-

quoted

quoted

 drivers/media/v4l2-core/v4l2-ctrls-defs.c         |  5 +++++
 include/uapi/linux/v4l2-controls.h                |  1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git

a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state

quoted

quoted

less.c

b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state

quoted

quoted

less.c

quoted

quoted

index d2b09ce9f1cf..a981178c25d9 100644
---

a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state

quoted

quoted

less.c

quoted

quoted

+++

b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state

quoted

quoted

less.c

quoted

quoted

@@ -535,6 +535,17 @@ static int mtk_vdec_s_ctrl(struct v4l2_ctrl

*ctrl)

quoted

quoted

 ctrl->val = mtk_dma_contig_get_secure_handle(ctx, ctrl->val);
 mtk_v4l2_vdec_dbg(3, ctx, "get secure handle: %d => 0x%x",

sec_fd, ctrl->val);

quoted

quoted

 break;
+case V4L2_CID_MPEG_MTK_SET_SECURE_MODE:

Stepping back a little and focusing on the API, what makes your

driver so

quoted

special that it should be the only one having a "secure mode" ?

We

quoted

quoted

are touching

quoted

in gap in the media pipeline in Linux, and this should come with

consideration

quoted

of the global API.

Why is this API better then let's say Google Android one, were

they

quoted

quoted

expose 2

quoted

device nodes in their fork of the MFC driver (a secure and a non

secure one) ?

Perhaps it is a good idea to first post an RFC with an uAPI

proposal

quoted

quoted

on how to
handle secure video. I suspect this isn't mediatek specific, other
SoCs with
tee support could use this as well.

As Nicolas said, it's long known to be a gap in our media support,

so

quoted

quoted

it is
really great that you started work on this, but you need to look

at

quoted

quoted

this from
a more generic point-of-view, and not mediatek-specific.

Whether your have any advice about how to do a more generic driver

to

quoted

handle secure video playback?

There are several kind of buffer: output queue buffer/capture queue
buffer/working buffer.

output and capture queue buffer: user space will call tee related
interface to allocate secure handle. Will convert to secure handle

with

quoted

v4l2 framework, then send secure handle to optee-os.

working buffer: calling dma_heap and dma_buf to get secure memory
handle, then covert secure iova in optee-os.

Using the same kernel driver for svp and non-svp playback, just the
buffer type are different. Normal is iova and secure is secure

handle.

quoted

User driver will tell the kernel driver with CID control whether

the

quoted

current playback is svp or non-svp.

My understanding is that when you switch to secure mode, the driver
makes
some optee calls to set everything up. And userspace needs a way
convert a
dmabuf fd to a 'secure handle', which appears to be the DMA address
of the
buffer. Who uses that handle?

User space need to get/set information for some secure memory with dma
secure handle, such as input buffer/frame buffer/tmp buffer, etc.
Secure fd will convert to secure handle in kernel space then in optee-
os in user space.

In any case, using a control to switch to secure mode and using a
control
to convert a dmabuf fd to a secure handle seems a poor choice to me.

I was wondering if it wouldn't be better to create a new V4L2_MEMORY_
type,
e.g. V4L2_MEMORY_DMABUF_SECURE (or perhaps _DMABUF_OPTEE). That
ensures that
once you create buffers for the first time, the driver can switch
into secure
mode, and until all buffers are released again you know that the
driver will
stay in secure mode.

It's no need to add V4L2_MEMORY_DMABUF_SECURE, v4l2 framewrok can
convert secure fd to secure handle directly for VB2_MEMROY_DMABUF.
User space will queue capture and output buffer, then calling
vb2_dma_contig_plane_dma_addr to get secure handle for secure mode,
getting iova for normal playback.

If the driver want to use memory type MMAP, maybe need to add
VB2_MEMROY_MMAP_SECURE.

For converting the dmabuf fd into a secure handle: a new ioctl
similar to
VIDIOC_EXPBUF might be more suited for that.

Note that I am the first to admit that I have no experience with
secure
video pipelines or optee-os, so I am looking at this purely from an
uAPI
perspective.

It's a good choice to convert secure fd to secure handle with a new
ioctl callback, will call the ioctl many times for there are many
buffers. If the driver in secure fd to secure handler callback, the
driver regard the playback is secure mode?

Best Regards,
Yunfei Dong

Regards,

Hans

quoted

Best Regards,
Yunfei Dong

quoted

Regards,

Hans

quoted

regards,
Nicolas

p.s. you forgot to document your control in the RST doc, please

do

quoted

quoted

in following

quoted

release.

quoted

+ctx->is_svp_mode = ctrl->val;
+
+if (ctx->is_svp_mode) {
+ret = mtk_vcodec_dec_optee_open(ctx->dev->optee_private);
+if (ret)
+mtk_v4l2_vdec_err(ctx, "open secure mode failed.");
+else
+mtk_v4l2_vdec_dbg(3, ctx, "decoder in secure mode: %d", ctrl-

val);

quoted

+}
+break;
 default:
 mtk_v4l2_vdec_dbg(3, ctx, "Not supported to set ctrl id:
0x%x\n",

hdr_ctrl->id);

quoted

quoted

 return ret;

@@ -573,7 +584,7 @@ static int mtk_vcodec_dec_ctrls_setup(struct

mtk_vcodec_dec_ctx *ctx)

quoted

quoted

 unsigned int i;
 struct v4l2_ctrl *ctrl;
 
-v4l2_ctrl_handler_init(&ctx->ctrl_hdl, NUM_CTRLS + 1);
+v4l2_ctrl_handler_init(&ctx->ctrl_hdl, NUM_CTRLS + 2);
 if (ctx->ctrl_hdl.error) {
 mtk_v4l2_vdec_err(ctx, "v4l2_ctrl_handler_init failed\n");
 return ctx->ctrl_hdl.error;

@@ -592,6 +603,8 @@ static int mtk_vcodec_dec_ctrls_setup(struct

mtk_vcodec_dec_ctx *ctx)

quoted

quoted

 
 ctrl = v4l2_ctrl_new_std(&ctx->ctrl_hdl,

&mtk_vcodec_dec_ctrl_ops,

quoted

quoted

  V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE, 0, 65535, 1, 0);
+ctrl = v4l2_ctrl_new_std(&ctx->ctrl_hdl,

&mtk_vcodec_dec_ctrl_ops,

quoted

quoted

+ V4L2_CID_MPEG_MTK_SET_SECURE_MODE, 0, 65535, 1, 0);
 
 v4l2_ctrl_handler_setup(&ctx->ctrl_hdl);
 

diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c

b/drivers/media/v4l2-core/v4l2-ctrls-defs.c

quoted

quoted

index d8cf01f76aab..a507045a3f30 100644

--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c

@@ -1042,6 +1042,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 case V4L2_CID_MPEG_VIDEO_REF_NUMBER_FOR_PFRAMES:return

"Reference

Frames for a P-Frame";

quoted

quoted

 case V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR:return "Prepend

SPS and PPS to IDR";

quoted

quoted

 case V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE:return "MediaTek
Decoder

get secure handle";

quoted

quoted

+case V4L2_CID_MPEG_MTK_SET_SECURE_MODE:return "MediaTek Decoder

set secure mode";

quoted

quoted

 
 /* AV1 controls */
 case V4L2_CID_MPEG_VIDEO_AV1_PROFILE:return "AV1 Profile";

@@ -1442,6 +1443,10 @@ void v4l2_ctrl_fill(u32 id, const char

**name, enum v4l2_ctrl_type *type,

quoted

quoted

 *type = V4L2_CTRL_TYPE_INTEGER;
 *flags |= V4L2_CTRL_FLAG_WRITE_ONLY;
 break;
+case V4L2_CID_MPEG_MTK_SET_SECURE_MODE:
+*type = V4L2_CTRL_TYPE_INTEGER;
+*flags |= V4L2_CTRL_FLAG_WRITE_ONLY;
+break;
 case V4L2_CID_USER_CLASS:
 case V4L2_CID_CAMERA_CLASS:
 case V4L2_CID_CODEC_CLASS:

diff --git a/include/uapi/linux/v4l2-controls.h

b/include/uapi/linux/v4l2-controls.h

quoted

quoted

index 7b3694985366..88e90d943e38 100644

--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h

@@ -957,6 +957,7 @@ enum v4l2_mpeg_mfc51_video_force_frame_type

{

quoted

quoted

quoted

quoted

 /*  MPEG-class control IDs specific to the MediaTek Decoder

driver as defined by V4L2 */

quoted

quoted

 #define V4L2_CID_MPEG_MTK_BASE(V4L2_CTRL_CLASS_CODEC | 0x2000)
 #define

V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE(V4L2_CID_MPEG_MTK_BASE+8)

quoted

quoted

+#define

V4L2_CID_MPEG_MTK_SET_SECURE_MODE(V4L2_CID_MPEG_MTK_BASE+9)

quoted

quoted

 
 /*  Camera class control IDs */