[PATCH 12/14] media: medkatek: vcodec: set secure mode to decoder driver
Hans Verkuil
hverkuil-cisco at xs4all.nl
Wed Sep 20 11:25:59 PDT 2023
On 20/09/2023 20:13, Jeffrey Kardatzke wrote:
> On Wed, Sep 20, 2023 at 12:10 AM Hans Verkuil <hverkuil-cisco at xs4all.nl> wrote:
>>
>> On 19/09/2023 21:49, Jeffrey Kardatzke wrote:
>>> On Tue, Sep 19, 2023 at 11:51 AM Nicolas Dufresne
>>> <nicolas.dufresne at collabora.com> wrote:
>>>>
>>>> Le mardi 19 septembre 2023 à 10:53 +0200, Hans Verkuil a écrit :
>>>>> On 18/09/2023 22:57, Jeffrey Kardatzke wrote:
>>>>>> On Fri, Sep 15, 2023 at 1:56 AM Hans Verkuil <hverkuil-cisco at xs4all.nl> wrote:
>>>>>>>
>>>>>>> On 15/09/2023 10:25, Yunfei Dong (董云飞) wrote:
>>>>>>>> Hi Hans & Nicolas,
>>>>>>>>
>>>>>>>> Thanks for your advice.
>>>>>>>>
>>>>>>>> On Tue, 2023-09-12 at 11:30 +0200, Hans Verkuil wrote:
>>>>>>>>>
>>>>>>>>> External email : Please do not click links or open attachments until
>>>>>>>>> you have verified the sender or the content.
>>>>>>>>> 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 at 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_state
>>>>>>>>> less.c
>>>>>>>>> b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state
>>>>>>>>> less.c
>>>>>>>>>>> index d2b09ce9f1cf..a981178c25d9 100644
>>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state
>>>>>>>>> less.c
>>>>>>>>>>> +++
>>>>>>>>>
>>>>>>>>> b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_state
>>>>>>>>> less.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.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Whether your have any advice about how to do a more generic driver to
>>>>>>>> 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
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> User driver will tell the kernel driver with CID control whether the
>>>>>>>> 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?
>>>>>>
>>>>>> The only user space usage for getting the 'secure handle' from an fd
>>>>>> is when that memory is written to. This is done when the TEE decrypts
>>>>>> the video contents. User space sends the encrypted video + 'secure
>>>>>> handle' to the TEE, and the TEE decrypts the contents to the memory
>>>>>> associated with the 'secure handle'. Then the 'secure handle' is
>>>>>> passed into the TEE again with the v4l2 driver to use as the source
>>>>>> for video decoding (but w/ v4l2, user space is passing in fds).
>>>>>
>>>>> I think I need some more background. This series is to support a 'Secure Video
>>>>> Processor' (at least, that's what svp stands for I believe, something that
>>>>> is not mentioned anywhere in this series, BTW) which is used to decode an
>>>>> encrypted h264 stream.
>>>>>
>>>>> First question: how is that stream encrypted? Is that according to some standard?
>>>>> Nothing is mentioned about that.
>>>>>
>>>>> I gather that the encrypted stream is fed to the codec as usual (i.e. just put it
>>>>> in the output buffer and queue it to the codec), nothing special is needed for that.
>>>>> Except, how does the hardware know it is encrypted? I guess that's where the
>>>>> control comes in, you have to turn on SVP mode first.
>>>>
>>>> Decryption takes place before the decoder. I suspect there is no dedicated
>>>> driver for that, the TEE driver API is similar to smart card API and fits well
>>>> this task. So the decrytor consume normal memory that is encrypted and is only
>>>> allowed to decrypt into secure memory. All this is happening before the decoder,
>>>> so is out of scope for this patchset.
>>>>
>>>> Just a correction :-D.
>>>>
>>>>>
>>>>> For the capture buffers you need to provide buffers from secure/trusted memory.
>>>>> That's a dmabuf fd, but where does that come from?
>>>>>
>>>>> I saw this message:
>>>>>
>>>>> https://lore.kernel.org/linux-media/CAPj87rOHctwHJM-7HiQpt8Q0b09x0WWw_T4XsL0qT=dS+XzyZQ@mail.gmail.com/T/#u
>>>>>
>>>>> so I expect that's where it comes from. But I agree that getting this from dma-heaps
>>>>> seems more natural.
>>>>>
>>>>> I assume that those capture buffers are inaccessible from the CPU? (Hence 'secure')
>>>>>
>>>>> For actually displaying these secure buffers you would use drm, and I assume that
>>>>> the hardware would mix in the contents of the secure buffer into the video output
>>>>> pipeline? I.e., the actual contents remain inaccessible. And that the video output
>>>>> (HDMI or DisplayPort) is using HDCP?
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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?
>>>>
>>>> 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 ?
>>>>
>>>> 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.
>>>>
>>>> Nicolas
>>>
>>>
>>> Here's some links to the current userspace implementation built on top
>>> of the MTK patches (and yeah, this'll end up changing based on what
>>> happens upstream).
>>>
>>> 1. This is where we are decrypting the video to a secure buffer, it's
>>> invoking IPC into a closed source component to do that:
>>> https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/decoder_buffer_transcryptor.cc;l=87
>>
>> So the encrypted compressed stream (contained in a regular non-secure buffer)
>> is decrypted here into secure buffers. Correct?
> Correct
>>
>> The hardware codec will just operate on those secure buffers, both for the
>> output and capture queues, right? And no decryption/encryption takes place,
>> it is all operating on unencrypted secure buffers, right?
> Correct
>>
>> Or is the plan to include the decryption step in the driver?
> No, the driver will never be doing the decryption.
>>
>> But who encrypted the compressed stream? Is it encrypted according to
>> some standard? Or it is mediatek specific?
> It's encrypted using CENC (Common Encryption). The method for
> acquiring the keys to perform the decryption is Widevine specific
> (Widevine is the Digital Rights Management system we are using...but
> nothing in the kernel patches dictates which Digital Rights Management
> system is used, but the encryption technique is a standard).
Ah, that's the missing piece for me. Now I understand the context of these
patches.
Thank you, this was very helpful.
Regards,
Hans
>>
>> Regards,
>>
>> Hans
>>
>>> 2. This is where we aren enabling secure mode:
>>> https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/v4l2_video_decoder.cc;l=412
>>> 3. This is where we are resolving secure buffers to secure handles:
>>> https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/v4l2_video_decoder.cc;l=535
>>> (the allocation of the secure buffers is done in closed source CDM
>>> code, but it's just opening the dma-buf heap and issuing the ioctl to
>>> allocate it)
>>> 4. This is where we submit the secure buffers to the output queue:
>>> https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/v4l2/v4l2_queue.cc;l=816
>>> (this is nothing special, since it's just passing in the fd)
>>> 5. For the capture queue, there's zero changes in Chrome V4L2 code for
>>> that...it's all transparent to user space that it's a secure surface
>>> that's being rendered to. We do allocate them w/ different flags via
>>> minigbm which happens here:
>>> https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/platform_video_frame_pool.cc;l=37
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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 at lists.infradead.org
>>>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>>
>>>>
>>
More information about the Linux-mediatek
mailing list