On 18/09/2023 22:57, Jeffrey Kardatzke wrote:
> On Fri, Sep 15, 2023 at 1:56 AM Hans Verkuil <[email protected]> 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 <[email protected]>
>>>>>> ---
>>>>>> .../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.
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.
>
>>
>> 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
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <[email protected]> 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 <[email protected]>
> > > > > > > ---
> > > > > > > .../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
>
> >
> > >
> > > 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
> > > [email protected]
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Tue, Sep 19, 2023 at 1:53 AM Hans Verkuil <[email protected]> wrote:
>
> On 18/09/2023 22:57, Jeffrey Kardatzke wrote:
> > On Fri, Sep 15, 2023 at 1:56 AM Hans Verkuil <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> .../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.
SVP = Secure Video Path, which is explained in what you linked to below.
>
> First question: how is that stream encrypted? Is that according to some standard?
> Nothing is mentioned about that.
The TEE does the decryption of the incoming stream to the secure
buffer...and that is then fed into the output queue of v4l2. So
there's no handling of encrypted content in the kernel patches...it's
just about handling secure buffers and secure surfaces.
>
> 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.
Yeah, the driver does need to know if the FDs are from secure or
non-secure memory...which is what the SVP control was about.
>
> For the capture buffers you need to provide buffers from secure/trusted memory.
> That's a dmabuf fd, but where does that come from?
That fd is allocated from the secure dma-buf heap which is being
proposed in another patchset.
https://lore.kernel.org/linux-mediatek/[email protected]/
>
> 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')
Correct
>
> 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?
Correct
>
> >
> >>
> >> 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.
Ahh ok, good point about the control being able to be toggled at any
time. So adding a new V4L2_MEMORY_ type would make sense relating to
that.
>
> 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.
>
> >
> >>
> >> 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
> >> [email protected]
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Tue, Sep 19, 2023 at 11:51 AM Nicolas Dufresne
<[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > ---
> > > > > > > > .../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
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
> > > > [email protected]
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
On 19/09/2023 21:49, Jeffrey Kardatzke wrote:
> On Tue, Sep 19, 2023 at 11:51 AM Nicolas Dufresne
> <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../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?
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?
Or is the plan to include the decryption step in the driver?
But who encrypted the compressed stream? Is it encrypted according to
some standard? Or it is mediatek specific?
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
>>>>> [email protected]
>>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
On 19/09/2023 20:51, Nicolas Dufresne 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 <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> .../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?
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.
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
>>>> [email protected]
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
On Wed, Sep 20, 2023 at 12:10 AM Hans Verkuil <[email protected]> wrote:
>
> On 19/09/2023 21:49, Jeffrey Kardatzke wrote:
> > On Tue, Sep 19, 2023 at 11:51 AM Nicolas Dufresne
> > <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>>>>> ---
> >>>>>>>>> .../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).
>
> 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
> >>>>> [email protected]
> >>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>>
> >>
>
On Wed, Sep 20, 2023 at 12:21 AM Hans Verkuil <[email protected]> wrote:
>
> On 19/09/2023 20:51, Nicolas Dufresne 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 <[email protected]> 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 <[email protected]>
> >>>>>>>> ---
> >>>>>>>> .../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?
>
> 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.
>
> 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.
>
OK, sounds good. I'll work with Mediatek to get the patches updated for that.
> > 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.
>
Already talked to Mediatek about this and they are working on updating
the dma-buf patches for this.
> >
> > 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.
>
It would be possible to use this with clearkey w/ some additional work
on our end. If this is then part of the public ChromiumOS build, would
that be satisfactory? (the TEE would have some binary blob components
like firmware does though)
> 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
> >>>> [email protected]
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
>
Le mercredi 20 septembre 2023 à 11:20 -0700, Jeffrey Kardatzke a écrit :
> > >
> > > 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.
> >
>
> It would be possible to use this with clearkey w/ some additional work
> on our end. If this is then part of the public ChromiumOS build, would
> that be satisfactory? (the TEE would have some binary blob components
> like firmware does though)
From my point of view, this would fully cover my concern. To clarify this
concern, the decryption into secure memory currently only ever take place in
proprietary code that implements the protection (Widewine CDM). With clear key,
we can have an open source CDM (made for testing purpose) so that we don't have
to have hidden code to test the entire pipeline. So appart from the TEE
firmware, which is just a firmware like all the others, we could have open
source tests in kernelCI and other CI, and we could extend these test to
eventually support other vendors.
Note that currently, with other proposal, one could allocate and fill a normal
buffer, and "secure" that buffer to test the CODECs and display, but on this
specific architecture, with the limitation on the number of secure regions, this
feature isn't available.
Alternatives to this end-to-end solution, we could consider a TA (Trusted
Application) that simply copy data from a untrusted chunk of memory into a
trusted chunk of memory. That seems like a cross-platform solution. It would be
even better if this get standardized in TEEs for course (or at least required
with all secure memory implementation). Then copying from untrusted to trusted
could easily become an ioctl generic to all TEE drivers. That to me would be
equally acceptable, and perhaps easier to use.
Nicolas
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
> >>>> [email protected]
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
>
On Thu, Sep 21, 2023 at 8:46 AM Nicolas Dufresne
<[email protected]> wrote:
>
> Le mercredi 20 septembre 2023 à 11:20 -0700, Jeffrey Kardatzke a écrit :
> > > >
> > > > 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.
> > >
> >
> > It would be possible to use this with clearkey w/ some additional work
> > on our end. If this is then part of the public ChromiumOS build, would
> > that be satisfactory? (the TEE would have some binary blob components
> > like firmware does though)
>
> From my point of view, this would fully cover my concern. To clarify this
> concern, the decryption into secure memory currently only ever take place in
> proprietary code that implements the protection (Widewine CDM). With clear key,
> we can have an open source CDM (made for testing purpose) so that we don't have
> to have hidden code to test the entire pipeline. So appart from the TEE
> firmware, which is just a firmware like all the others, we could have open
> source tests in kernelCI and other CI, and we could extend these test to
> eventually support other vendors.
>
> Note that currently, with other proposal, one could allocate and fill a normal
> buffer, and "secure" that buffer to test the CODECs and display, but on this
> specific architecture, with the limitation on the number of secure regions, this
> feature isn't available.
>
> Alternatives to this end-to-end solution, we could consider a TA (Trusted
> Application) that simply copy data from a untrusted chunk of memory into a
> trusted chunk of memory. That seems like a cross-platform solution. It would be
> even better if this get standardized in TEEs for course (or at least required
> with all secure memory implementation). Then copying from untrusted to trusted
> could easily become an ioctl generic to all TEE drivers. That to me would be
> equally acceptable, and perhaps easier to use.
>
> Nicolas
It's very likely for the clearkey implementation that I would just
have it copying the data from a non-secure to secure buffer in a TA.
We would never do that in production of course, but for testing images
that would suffice.