Re: [PATCH v6 0/7] Add mediate-drm secure flow for SVP

[Jason-JH Lin (林睿祥)]

Hi Maxime,

[snip]

> > > > > ---
> > > > > TODO:
> > > > > 1) Drop MTK_DRM_IOCTL_GEM_CREATE and use DMA_HEAP_IOCTL_ALLOC
> > > > > in
> > > > > userspace
> > > > > 2) DRM driver use secure mailbox channel to handle normal and
> > > > > secure flow
> > > > > 3) Implement setting mmsys routing table in the secure world
> > > > > series
> > > > 
> > > > I'm not sure what you mean here. Why are you trying to upstream
> > > > something that still needs to be removed from your patch
> > > > series?
> > > > 
> > > 
> > > Because their is too much patches need to be fixed in this
> > > series,
> > > so I
> > > list down the remaining TODO items and send to review for the
> > > other
> > > patches.
> > > 
> > > Sorry for the bothering, I'll drop this at the next version.
> > 
> > If you don't intend to use it, we just shouldn't add it. Removing
> > the
> > TODO item doesn't make sense, even more so if heaps should be the
> > way
> > you handle this.
> > 
> 
> Sorry for this misunderstanding.
> 
> I mean I'll remove the DRM_IOCTL_GEM_CREATE patch and then change
> user
> space calling DMA_HEAP_IOCTL_ALLOC to allocate buffer from secure
> heap.
> 

I have changed user space to use DMA_HEAP_IOCTL_ALLOC to allocate
secure buffer, but I still encounter the problem of determining whether
the buffer is secure in mediatek-drm driver to add some secure
configure for hardware.


As the comment in [1], dma driver won't provide API for use.
[1]: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20240515112308.10171-3-yong.wu@xxxxxxxxxxxx/#25857255


So I use name checking at [PATCH v6 3/7] like this currently:

struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device
*dev,
            struct dma_buf_attachment *attach, struct sg_table *sg)
{
    struct mtk_gem_obj *mtk_gem;

    /* check if the entries in the sg_table are contiguous */
    if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {
        DRM_ERROR("sg_table is not contiguous");
        return ERR_PTR(-EINVAL);
    }

    mtk_gem = mtk_gem_init(dev, attach->dmabuf->size);
    if (IS_ERR(mtk_gem))
        return ERR_CAST(mtk_gem);

+   mtk_gem->secure = (!strncmp(attach->dmabuf->exp_name, "restricted",
10));
    mtk_gem->dma_addr = sg_dma_address(sg->sgl);
+   mtk_gem->size = attach->dmabuf->size;
    mtk_gem->sg = sg;

    return &mtk_gem->base;
}

But I want to change this name checking to the information brought from
user space.
I tried to use arg->flags to append the secure flag in user space and
call drmPrimeHandleToFD() to pass it to DRM driver, but it will be
blocked by at the beginning of the drm_prime_handle_to_fd_ioctl().

I can't find any other existing ioctl to pass such private information.
Do you have any idea? Or should we open a new ioctl for that?

Regards,
Jason-JH.Lin

> > > > Also, I made some comments on the previous version that have
> > > > been
> > > > entirely ignored and still apply on this version:
> > > > 
> > > 
> > > 
> 
> 
https://lore.kernel.org/dri-devel/20240415-guppy-of-perpetual-current-3a7974@houat/
> > > > 
> > > 
> > > I lost that mail in my mailbox, so I didn't reply at that time.
> > > I have imported that mail and replied to you. Hope you don't mind
> > > :)
> > 
> > I haven't received that answer
> 
> I don't know why it doesn't show up at your link.
> 
> Could you see it here?
> 
> 
https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.369-3-shawn.sung@xxxxxxxxxxxx/
> 
> 
> 
> Regards,
> Jason-JH.Lin
> > 
> > Maxime