Re: [PATCH 12/14] media: medkatek: vcodec: set secure mode to decoder driver
From: Yunfei Dong (董云飞)
Date: Mon Sep 11 2023 - 23:07:44 EST
Hi Nicolas,
Thanks for your advice.
On Mon, 2023-09-11 at 11:54 -0400, Nicolas Dufresne wrote:
> Hi,
>
> Le lundi 11 septembre 2023 à 20:59 +0800, Yunfei Dong a écrit :
> > Setting secure mode flag to kernel when trying to play secure
> > video,
> > then decoder driver will initialize tee related interface to
> > support
> > svp.
>
>
> This is not what the patch is doing, please rework. This patch is an
> vendor API
> addition introducing V4L2_CID_MPEG_MTK_SET_SECURE_MODE. I should not
> have to
> read your patch to understand this.
>
I don't know your meaning clearly. Whether I need to add one new patch
to add the definition of V4L2_CID_MPEG_MTK_SET_SECURE_MODE? Than the
driver calling it to set secure mode?
Best Regards,
Yunfei Dong
> >
> > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > ---
> > .../vcodec/decoder/mtk_vcodec_dec_stateless.c | 15
> > ++++++++++++++-
> > 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_sta
> > teless.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > index d2b09ce9f1cf..a981178c25d9 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_sta
> > teless.c
> > @@ -535,6 +535,17 @@ static int mtk_vdec_s_ctrl(struct v4l2_ctrl
> > *ctrl)
> > 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);
> > break;
> > + case V4L2_CID_MPEG_MTK_SET_SECURE_MODE:
>
> Stepping back a little and focusing on the API, what makes your
> driver so
> special that it should be the only one having a "secure mode" ? We
> are touching
> in gap in the media pipeline in Linux, and this should come with
> consideration
> of the global API.
>
> Why is this API better then let's say Google Android one, were they
> expose 2
> device nodes in their fork of the MFC driver (a secure and a non
> secure one) ?
>
> regards,
> Nicolas
>
> p.s. you forgot to document your control in the RST doc, please do in
> following
> release.
>
> > + 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);
> > + }
> > + break;
> > default:
> > mtk_v4l2_vdec_dbg(3, ctx, "Not supported to set ctrl
> > id: 0x%x\n", hdr_ctrl->id);
> > return ret;
> > @@ -573,7 +584,7 @@ static int mtk_vcodec_dec_ctrls_setup(struct
> > mtk_vcodec_dec_ctx *ctx)
> > 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)
> >
> > ctrl = v4l2_ctrl_new_std(&ctx->ctrl_hdl,
> > &mtk_vcodec_dec_ctrl_ops,
> > V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE,
> > 0, 65535, 1, 0);
> > + ctrl = v4l2_ctrl_new_std(&ctx->ctrl_hdl,
> > &mtk_vcodec_dec_ctrl_ops,
> > + 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
> > 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";
> > case V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR: ret
> > urn "Prepend SPS and PPS to IDR";
> > case V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE: return
> > "MediaTek Decoder get secure handle";
> > + case V4L2_CID_MPEG_MTK_SET_SECURE_MODE: ret
> > urn "MediaTek Decoder set secure mode";
> >
> > /* AV1 controls */
> > case V4L2_CID_MPEG_VIDEO_AV1_PROFILE: ret
> > urn "AV1 Profile";
> > @@ -1442,6 +1443,10 @@ void v4l2_ctrl_fill(u32 id, const char
> > **name, enum v4l2_ctrl_type *type,
> > *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
> > 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 {
> > /* MPEG-class control IDs specific to the MediaTek Decoder driver
> > as defined by V4L2 */
> > #define V4L2_CID_MPEG_MTK_BASE (V4L2_CTRL_CLAS
> > S_CODEC | 0x2000)
> > #define V4L2_CID_MPEG_MTK_GET_SECURE_HANDLE (V4L2_CID_MPEG_
> > MTK_BASE+8)
> > +#define V4L2_CID_MPEG_MTK_SET_SECURE_MODE (V4L2_CID_MPEG_MTK_BASE
> > +9)
> >
> > /* Camera class control IDs */
> >
>
>