<pre>
Hi CK,

Thanks for the reviews.

On Tue, 2023-10-24 at 07:42 +0000, CK Hu (胡俊光) wrote:
> Hi, Jason:
>
> On Mon, 2023-10-23 at 12:45 +0800, Jason-JH.Lin wrote:
> > To add secure flow support for mediatek-drm, each crtc have to
> > create a secure cmdq mailbox channel. Then cmdq packets with
> > display HW configuration will be sent to secure cmdq mailbox
> > channel
> > and configured in the secure world.
> >
> > Each crtc have to use secure cmdq interface to configure some
> > secure
> > settings for display HW before sending cmdq packets to secure cmdq
> > mailbox channel.
> >
> > If any of fb get from current drm_atomic_state is secure, then crtc
> > will switch to the secure flow to configure display HW.
> > If all fbs are not secure in current drm_atomic_state, then crtc
> > will
> > switch to the normal flow.
> >
> > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 272
> > ++++++++++++++++++++++-
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.h | 1 +
> > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 7 +
> > 3 files changed, 269 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index b6fa4ad2f94d..6c2cf339b923 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -56,6 +56,11 @@ struct mtk_drm_crtc {
> > u32cmdq_event;
> > u32cmdq_vblank_cnt;
> > wait_queue_head_tcb_blocking_queue;
> > +
> > +struct cmdq_clientsec_cmdq_client;
> > +struct cmdq_pktsec_cmdq_handle;
> > +boolsec_cmdq_working;
> > +wait_queue_head_tsec_cb_blocking_queue;
> > #endif
> >
> > struct device*mmsys_dev;
> > @@ -67,6 +72,7 @@ struct mtk_drm_crtc {
> > /* lock for display hardware access */
> > struct mutexhw_lock;
> > boolconfig_updating;
> > +boolsec_on;
> > };
> >
> > struct mtk_crtc_state {
> > @@ -109,6 +115,154 @@ static void mtk_drm_finish_page_flip(struct
> > mtk_drm_crtc *mtk_crtc)
> > }
> > }
> >
> > +void mtk_crtc_disable_secure_state(struct drm_crtc *crtc)
> > +{
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +enum cmdq_sec_scenario sec_scn = CMDQ_MAX_SEC_COUNT;
> > +int i;
> > +struct mtk_ddp_comp *ddp_first_comp;
> > +struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
> > +u64 sec_engine = 0; /* for hw engine write output secure fb */
> > +u64 sec_port = 0; /* for larb port read input secure fb */
> > +
> > +mutex_lock(&mtk_crtc->hw_lock);
> > +
> > +if (!mtk_crtc->sec_cmdq_client.chan) {
> > +pr_err("crtc-%d secure mbox channel is NULL\n",
> > drm_crtc_index(crtc));
> > +goto err;
> > +}
> > +
> > +if (!mtk_crtc->sec_on) {
> > +pr_debug("crtc-%d is already disabled!\n",
> > drm_crtc_index(crtc));
> > +goto err;
> > +}
> > +
> > +mbox_flush(mtk_crtc->sec_cmdq_client.chan, 0);
> > +mtk_crtc->sec_cmdq_handle.cmd_buf_size = 0;
> > +
> > +if (mtk_crtc->sec_cmdq_handle.sec_data) {
> > +struct cmdq_sec_data *sec_data;
> > +
> > +sec_data = mtk_crtc->sec_cmdq_handle.sec_data;
> > +sec_data->addrMetadataCount = 0;
> > +sec_data->addrMetadatas = (uintptr_t)NULL;
> > +}
> > +
> > +/*
> > + * Secure path only support DL mode, so we just wait
> > + * the first path frame done here
> > + */
> > +cmdq_pkt_wfe(&mtk_crtc->sec_cmdq_handle, mtk_crtc->cmdq_event,
> > false);
> > +
> > +ddp_first_comp = mtk_crtc->ddp_comp[0];
> > +for (i = 0; i < mtk_crtc->layer_nr; i++) {
> > +struct drm_plane *plane = &mtk_crtc->planes[i];
> > +
> > +sec_port |=
> > mtk_ddp_comp_layer_get_sec_port(ddp_first_comp, i);
> > +
> > +/* make sure secure layer off before switching secure
> > state */
> > +if (!mtk_plane_fb_is_secure(plane->state->fb)) {
> > +struct mtk_plane_state *plane_state =
> > to_mtk_plane_state(plane->state);
> > +
> > +plane_state->pending.enable = false;
> > +mtk_ddp_comp_layer_config(ddp_first_comp, i,
> > plane_state,
> > + &mtk_crtc-
> > > sec_cmdq_handle);
>
> You disable layer here and disable secure path in
> cmdq_sec_pkt_set_data() later. But this is real world and could be
> hacked by hacker. If hacker do not disable layer here but disable
> secure path in cmdq_sec_pkt_set_data() later, the hardware would keep
> reading secure buffer and path is not secure? That means video could
> output to HDMI without HDCP?

Disabling secure path by cmdq_sec_pkt_set_data() will also switch the
larb used by OVL to non-secure identity. So even if the secure layer is
enabled, OVL can not access secure DRAM with non-secure larb.

And it will cause a IOMMU translation fault when non-secure larb access
the address of secure DRAM.

Regards,
Jason-JH.Lin

>
> Regards,
> CK
>

</pre><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any 
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be 
conveyed only to the designated recipient(s). Any use, dissemination, 
distribution, printing, retaining or copying of this e-mail (including its 
attachments) by unintended recipient(s) is strictly prohibited and may 
be unlawful. If you are not an intended recipient of this e-mail, or believe 
that you have received this e-mail in error, please notify the sender 
immediately (by replying to this e-mail), delete any and all copies of 
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->