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

["Yunfei Dong (董云飞)" <Yunfei.Dong@mediatek.com>]

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@mediatek.com>
> > ---
> >  .../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 */
> >  
> 
>