Hi,
On 9/11/23 17:54, 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.
>
>>
>> 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_stateless.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>> index d2b09ce9f1cf..a981178c25d9 100644
>> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.c
>> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_stateless.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) ?
Perhaps it is a good idea to first post an RFC with an uAPI proposal 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 it is
really great that you started work on this, but you need to look at this from
a more generic point-of-view, and not mediatek-specific.
Regards,
Hans
>
> 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: return "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: return "MediaTek Decoder set secure mode";
>>
>> /* 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,
>> *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_CLASS_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 */
>>
>
