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

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

Hi Hans,

Thanks for your help to give some good advice.
On Wed, 2023-09-20 at 09:20 +0200, Hans Verkuil wrote:
>  	 
> >>>> 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.
> >>>
> >>> Why do you think the control for setting secure mode is a poor
> choice?
> >>> There's various places in the driver code where functionality
> changes
> >>> based on being secure/non-secure mode, so this is very much a
> 'global'
> >>> setting for the driver. It could be inferred based off a new
> memory
> >>> type for the queues...which then sets that flag in the driver;
> but
> >>> that seems like it would be more fragile and would require
> checking
> >>> for incompatible output/capture memory types. I'm not against
> another
> >>> way of doing this; but didn't see why you think the proposed
> method is
> >>> a poor choice.
> >>
> >> I assume you are either decoding to secure memory all the time, or
> not
> >> at all. That's something you would want to select the moment you
> allocate
> >> the first buffer. Using the V4L2_MEMORY_ value would be the
> natural place
> >> for that. A control can typically be toggled at any time, and it
> makes
> >> no sense to do that for secure streaming.
> >>
> >> Related to that: if you pass a dmabuf fd you will need to check
> somewhere
> >> if the fd points to secure memory or not. You don't want to mix
> the two
> >> but you want to check that at VIDIOC_QBUF time.
> >>
> >> Note that the V4L2_MEMORY_ value is already checked in the v4l2
> core,
> >> drivers do not need to do that.
> > 
> > Just to clarify a bit, and make sure I understand this too. You are
> proposing to
> > introduce something like:
> > 
> >    V4L2_MEMORY_SECURE_DMABUF
> > 
> > Which like V4L2_MEMORY_DMABUF is meant to import dmabuf, while
> telling the
> > driver that the memory is secure according to the definition of
> "secure" for the
> > platform its running on.
> > 
> > This drivers also allocate secure SHM (a standard tee concept) and
> have internal
> > allocation for reconstruction buffer and some hw specific reference
> metadata. So
> > the idea would be that it would keep allocation using the dmabuf
> heap internal
> > APIs ? And decide which type of memory based on the memory type
> found in the
> > queue?
> 
> Yes. Once you request the first buffer you basically tell the driver
> whether it
> will operate in secure or non-secure mode, and that stays that way
> until all
> buffers are freed. I think that makes sense.
> 

According to iommu's information, the dma operation for secure and non-
secure are the same, whether just need to add one memory type in v4l2
framework the same as V4L2_MEMORY_DMABUF? The dma operation in
videobuf2-dma-contig.c can use the same functions.

Best Regards,
Yunfei Dong

> If there is a need in the future to have V4L2 allocate the secure
> buffers, then
> a similar V4L2_MEMORY_MMAP_SECURE type can be added. I think using
> v4l2_memory
> to select secure or non-secure mode is logical and fits well with the
> V4L2 API.
> > Stepping back a little, why can't we have a way for drivers to
> detect that
> > dmabuf are secure ? I'm wondering if its actually useful to impose
> to all
> > userspace component to know that a dmabuf is secure ?
> 
> I was wondering the same thing: there should be a simple way for
> drivers and
> userspace to check if a dmabuf fd is secure or not. That will
> certainly help
> the vb2 framework verify that you don't mix secure and non-secure
> dmabuf fds.
> 
> > 
> > Also, regarding MTK, these are stateless decoders. I think it would
> be nice to
> > show use example code that can properly parse the un-encrypted
> header, pass the
> > data to the decryptor and decode. There is a bit of mechanic in
> there that lacks
> > clarification, a reference implementation would clearly help.
> Finally, does this
> > platform offers some clearkey implementation (or other alternative)
> so we can do
> > validation and regression testing? It would be very unfortunate to
> add feature
> > upstream that can only be tested by proprietary CDM software.
> 
> Good points.
> 
> Hans
> 
> > 
> > Nicolas
> > 
> >>
> >>>
> >>>>
> >>>> For converting the dmabuf fd into a secure handle: a new ioctl
> similar to
> >>>> VIDIOC_EXPBUF might be more suited for that.
> >>>
> >>> I actually think the best way for converting the dmabuf fd into a
> >>> secure handle would be another ioctl in the dma-heap
> driver...since
> >>> that's where the memory is actually allocated from. But this
> really
> >>> depends on upstream maintainers and what they are comfortable
> with.
> >>
> >> That feels like a more natural place of doing this.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>>>
> >>>> 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.
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>> Best Regards,
> >>>>> Yunfei Dong
> >>>>>> 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 */
> >>>>>>>>
> >>>>
> >>>>
> >>>> _______________________________________________
> >>>> linux-arm-kernel mailing list
> >>>> linux-arm-kernel@lists.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> > 
>