Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

18 participants
2944 discussions

Re: [Linaro-mm-sig] [PATCH][next] dma-buf: heaps: remove redundant assignment to variable ret

by John Stultz

On Wed, Oct 30, 2019 at 8:45 AM Andrew F. Davis <afd(a)ti.com> wrote: > > On 10/30/19 11:02 AM, Colin King wrote: > > From: Colin Ian King <colin.king(a)canonical.com> > > > > The variable ret is being assigned with a value that is never > > read, it is being re-assigned the same value on the err0 exit > > path. The assignment is redundant and hence can be removed. > > > > Addresses-Coverity: ("Unused value") > > Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") > > Signed-off-by: Colin Ian King <colin.king(a)canonical.com> > > --- > > > The root of the issue is that ret is not used in the error path, it > should be, I suggest this fix: > > > --- a/drivers/dma-buf/heaps/system_heap.c > > +++ b/drivers/dma-buf/heaps/system_heap.c > > @@ -98,7 +98,7 @@ static int system_heap_allocate(struct dma_heap *heap, > > err0: > > kfree(helper_buffer); > > > > - return -ENOMEM; > > + return ret; > > } Sounds good! If its ok I'll generate a commit crediting Colin for reporting the issue and Andrew for the fix and submit it to Sumit. Many thanks to you both! -john

5 years, 8 months

[PATCH] dma-buf: heaps: Fix redundant assignment to variable ret in system_heap.c

by John Stultz

Colin King reported a coverity error: The variable ret is being assigned with a value that is never read, it is being re-assigned the same value on the err0 exit path. The assignment is redundant and hence can be removed. He had a fix, but Andrew Davis suggested a better solution (actually returning ret), so this patch implements that fix. Cc: Colin King <colin.king(a)canonical.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Benjamin Gaignard <benjamin.gaignard(a)linaro.org> Cc: Liam Mark <lmark(a)codeaurora.org> Cc: Laura Abbott <labbott(a)redhat.com> Cc: Brian Starkey <brian.starkey(a)arm.com> Cc: Andrew F. Davis <afd(a)ti.com> Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: kernel-janitors(a)vger.kernel.org Addresses-Coverity: ("Unused value") Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") Reported-by: Colin Ian King <colin.king(a)canonical.com> Suggested-by: Andrew F. Davis <afd(a)ti.com> Signed-off-by: John Stultz <john.stultz(a)linaro.org> --- drivers/dma-buf/heaps/system_heap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 455782efbb32..9a56393e40b4 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -98,7 +98,7 @@ static int system_heap_allocate(struct dma_heap *heap, err0: kfree(helper_buffer); - return -ENOMEM; + return ret; } static const struct dma_heap_ops system_heap_ops = { -- 2.17.1

5 years, 8 months

[PATCH] dma-buf: Fix a warning message in dma_heap_buffer_destroy()

by Dan Carpenter

The first argument of WARN() is a condition so this will just print the function name instead of the whole warning message. Fixes: 7b87ea704fd9 ("dma-buf: heaps: Add heap helpers") Signed-off-by: Dan Carpenter <dan.carpenter(a)oracle.com> --- drivers/dma-buf/heaps/heap-helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dma-buf/heaps/heap-helpers.c b/drivers/dma-buf/heaps/heap-helpers.c index 750bef4e902d..a31684c0d5b2 100644 --- a/drivers/dma-buf/heaps/heap-helpers.c +++ b/drivers/dma-buf/heaps/heap-helpers.c @@ -52,7 +52,7 @@ static void *dma_heap_map_kernel(struct heap_helper_buffer *buffer) static void dma_heap_buffer_destroy(struct heap_helper_buffer *buffer) { if (buffer->vmap_cnt > 0) { - WARN("%s: buffer still mapped in the kernel\n", __func__); + WARN(1, "%s: buffer still mapped in the kernel\n", __func__); vunmap(buffer->vaddr); } -- 2.20.1

5 years, 8 months

[PATCH][next] dma-buf: heaps: remove redundant assignment to variable ret

by Colin King

From: Colin Ian King <colin.king(a)canonical.com> The variable ret is being assigned with a value that is never read, it is being re-assigned the same value on the err0 exit path. The assignment is redundant and hence can be removed. Addresses-Coverity: ("Unused value") Fixes: 47a32f9c1226 ("dma-buf: heaps: Add system heap to dmabuf heaps") Signed-off-by: Colin Ian King <colin.king(a)canonical.com> --- drivers/dma-buf/heaps/system_heap.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c index 455782efbb32..817a1667bd57 100644 --- a/drivers/dma-buf/heaps/system_heap.c +++ b/drivers/dma-buf/heaps/system_heap.c @@ -55,10 +55,8 @@ static int system_heap_allocate(struct dma_heap *heap, helper_buffer->pages = kmalloc_array(helper_buffer->pagecount, sizeof(*helper_buffer->pages), GFP_KERNEL); - if (!helper_buffer->pages) { - ret = -ENOMEM; + if (!helper_buffer->pages) goto err0; - } for (pg = 0; pg < helper_buffer->pagecount; pg++) { /* -- 2.20.1

5 years, 8 months

[PATCH 1/4] dma-buf: change DMA-buf locking convention v2

by Christian König

This patch is a stripped down version of the locking changes necessary to support dynamic DMA-buf handling. It adds a dynamic flag for both importers as well as exporters so that drivers can choose if they want the reservation object locked or unlocked during mapping of attachments. For compatibility between drivers we cache the DMA-buf mapping during attaching an importer as soon as exporter/importer disagree on the dynamic handling. This change has gone through a lengthy discussion on dri-devel and other mailing lists with at least 3-4 different attempts and dead-ends until we settled on this solution. Please refer to the mailing lists archives for full background on the history of this change. v2: cleanup set_name merge, improve kerneldoc Signed-off-by: Christian König <christian.koenig(a)amd.com> --- drivers/dma-buf/dma-buf.c | 102 +++++++++++++++++++++++++++++++++----- include/linux/dma-buf.h | 57 +++++++++++++++++++-- 2 files changed, 143 insertions(+), 16 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 433d91d710e4..753be84b5fd6 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -45,10 +45,10 @@ static char *dmabuffs_dname(struct dentry *dentry, char *buffer, int buflen) size_t ret = 0; dmabuf = dentry->d_fsdata; - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (dmabuf->name) ret = strlcpy(name, dmabuf->name, DMA_BUF_NAME_LEN); - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); return dynamic_dname(dentry, buffer, buflen, "/%s:%s", dentry->d_name.name, ret > 0 ? name : ""); @@ -334,7 +334,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) if (IS_ERR(name)) return PTR_ERR(name); - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (!list_empty(&dmabuf->attachments)) { ret = -EBUSY; kfree(name); @@ -344,7 +344,7 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const char __user *buf) dmabuf->name = name; out_unlock: - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); return ret; } @@ -403,10 +403,10 @@ static void dma_buf_show_fdinfo(struct seq_file *m, struct file *file) /* Don't count the temporary reference taken inside procfs seq_show */ seq_printf(m, "count:\t%ld\n", file_count(dmabuf->file) - 1); seq_printf(m, "exp_name:\t%s\n", dmabuf->exp_name); - mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); if (dmabuf->name) seq_printf(m, "name:\t%s\n", dmabuf->name); - mutex_unlock(&dmabuf->lock); + dma_resv_unlock(dmabuf->resv); } static const struct file_operations dma_buf_fops = { @@ -525,6 +525,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) return ERR_PTR(-EINVAL); } + if (WARN_ON(exp_info->ops->cache_sgt_mapping && + exp_info->ops->dynamic_mapping)) + return ERR_PTR(-EINVAL); + if (!try_module_get(exp_info->owner)) return ERR_PTR(-ENOENT); @@ -645,10 +649,11 @@ void dma_buf_put(struct dma_buf *dmabuf) EXPORT_SYMBOL_GPL(dma_buf_put); /** - * dma_buf_attach - Add the device to dma_buf's attachments list; optionally, + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list; optionally, * calls attach() of dma_buf_ops to allow device-specific attach functionality - * @dmabuf: [in] buffer to attach device to. - * @dev: [in] device to be attached. + * @dmabuf: [in] buffer to attach device to. + * @dev: [in] device to be attached. + * @dynamic_mapping: [in] calling convention for map/unmap * * Returns struct dma_buf_attachment pointer for this attachment. Attachments * must be cleaned up by calling dma_buf_detach(). @@ -662,8 +667,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * accessible to @dev, and cannot be moved to a more suitable place. This is * indicated with the error code -EBUSY. */ -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev) +struct dma_buf_attachment * +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, + bool dynamic_mapping) { struct dma_buf_attachment *attach; int ret; @@ -677,6 +683,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, attach->dev = dev; attach->dmabuf = dmabuf; + attach->dynamic_mapping = dynamic_mapping; mutex_lock(&dmabuf->lock); @@ -685,16 +692,64 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, if (ret) goto err_attach; } + dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); + dma_resv_unlock(dmabuf->resv); mutex_unlock(&dmabuf->lock); + /* When either the importer or the exporter can't handle dynamic + * mappings we cache the mapping here to avoid issues with the + * reservation object lock. + */ + if (dma_buf_attachment_is_dynamic(attach) != + dma_buf_is_dynamic(dmabuf)) { + struct sg_table *sgt; + + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_lock(attach->dmabuf->resv, NULL); + + sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + if (!sgt) + sgt = ERR_PTR(-ENOMEM); + if (IS_ERR(sgt)) { + ret = PTR_ERR(sgt); + goto err_unlock; + } + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + attach->sgt = sgt; + attach->dir = DMA_BIDIRECTIONAL; + } + return attach; err_attach: kfree(attach); mutex_unlock(&dmabuf->lock); return ERR_PTR(ret); + +err_unlock: + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + + dma_buf_detach(dmabuf, attach); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach); + +/** + * dma_buf_attach - Wrapper for dma_buf_dynamic_attach + * @dmabuf: [in] buffer to attach device to. + * @dev: [in] device to be attached. + * + * Wrapper to call dma_buf_dynamic_attach() for drivers which still use a static + * mapping. + */ +struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, + struct device *dev) +{ + return dma_buf_dynamic_attach(dmabuf, dev, false); } EXPORT_SYMBOL_GPL(dma_buf_attach); @@ -711,11 +766,20 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (WARN_ON(!dmabuf || !attach)) return; - if (attach->sgt) + if (attach->sgt) { + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_lock(attach->dmabuf->resv, NULL); + dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_unlock(attach->dmabuf->resv); + } + mutex_lock(&dmabuf->lock); + dma_resv_lock(dmabuf->resv, NULL); list_del(&attach->node); + dma_resv_unlock(dmabuf->resv); if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); @@ -749,6 +813,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf)) return ERR_PTR(-EINVAL); + if (dma_buf_attachment_is_dynamic(attach)) + dma_resv_assert_held(attach->dmabuf->resv); + if (attach->sgt) { /* * Two mappings with different directions for the same @@ -761,6 +828,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, return attach->sgt; } + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_assert_held(attach->dmabuf->resv); + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -793,9 +863,15 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (WARN_ON(!attach || !attach->dmabuf || !sg_table)) return; + if (dma_buf_attachment_is_dynamic(attach)) + dma_resv_assert_held(attach->dmabuf->resv); + if (attach->sgt == sg_table) return; + if (dma_buf_is_dynamic(attach->dmabuf)) + dma_resv_assert_held(attach->dmabuf->resv); + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); @@ -1219,10 +1295,12 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) seq_puts(s, "\tAttached Devices:\n"); attach_count = 0; + dma_resv_lock(buf_obj->resv, NULL); list_for_each_entry(attach_obj, &buf_obj->attachments, node) { seq_printf(s, "\t%s\n", dev_name(attach_obj->dev)); attach_count++; } + dma_resv_unlock(buf_obj->resv); seq_printf(s, "Total %d devices attached\n\n", attach_count); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index ec212cb27fdc..bcc0f4d0b678 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -42,6 +42,18 @@ struct dma_buf_ops { */ bool cache_sgt_mapping; + /** + * @dynamic_mapping: + * + * If true the framework makes sure that the map/unmap_dma_buf + * callbacks are always called with the dma_resv object locked. + * + * If false the framework makes ure that the map/unmap_dma_buf + * callbacks are always called without the dma_resv object locked. + * Mutual exclusive with @cache_sgt_mapping. + */ + bool dynamic_mapping; + /** * @attach: * @@ -109,6 +121,9 @@ struct dma_buf_ops { * any other kind of sharing that the exporter might wish to make * available to buffer-users. * + * This is always called with the dmabuf->resv object locked when + * the dynamic_mapping flag is true. + * * Returns: * * A &sg_table scatter list of or the backing storage of the DMA buffer, @@ -267,7 +282,8 @@ struct dma_buf_ops { * struct dma_buf - shared buffer object * @size: size of the buffer * @file: file pointer used for sharing buffers across, and for refcounting. - * @attachments: list of dma_buf_attachment that denotes all devices attached. + * @attachments: list of dma_buf_attachment that denotes all devices attached, + * protected by dma_resv lock. * @ops: dma_buf_ops associated with this buffer object. * @lock: used internally to serialize list manipulation, attach/detach and * vmap/unmap, and accesses to name @@ -323,10 +339,12 @@ struct dma_buf { * struct dma_buf_attachment - holds device-buffer attachment data * @dmabuf: buffer for this attachment. * @dev: device attached to the buffer. - * @node: list of dma_buf_attachment. + * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf. * @sgt: cached mapping. * @dir: direction of cached mapping. * @priv: exporter specific attachment data. + * @dynamic_mapping: true if dma_buf_map/unmap_attachment() is called with the + * dma_resv lock held. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -343,6 +361,7 @@ struct dma_buf_attachment { struct list_head node; struct sg_table *sgt; enum dma_data_direction dir; + bool dynamic_mapping; void *priv; }; @@ -394,10 +413,39 @@ static inline void get_dma_buf(struct dma_buf *dmabuf) get_file(dmabuf->file); } +/** + * dma_buf_is_dynamic - check if a DMA-buf uses dynamic mappings. + * @dmabuf: the DMA-buf to check + * + * Returns true if a DMA-buf exporter wants to be called with the dma_resv + * locked, false if it doesn't wants to be called with the lock held. + */ +static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) +{ + return dmabuf->ops->dynamic_mapping; +} + +/** + * dma_buf_attachment_is_dynamic - check if a DMA-buf attachment uses dynamic + * mappinsg + * @attach: the DMA-buf attachment to check + * + * Returns true if a DMA-buf importer wants to call the map/unmap functions with + * the dma_resv lock held. + */ +static inline bool +dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach) +{ + return attach->dynamic_mapping; +} + struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, - struct device *dev); + struct device *dev); +struct dma_buf_attachment * +dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, + bool dynamic_mapping); void dma_buf_detach(struct dma_buf *dmabuf, - struct dma_buf_attachment *dmabuf_attach); + struct dma_buf_attachment *attach); struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info); @@ -409,6 +457,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *, enum dma_data_direction); void dma_buf_unmap_attachment(struct dma_buf_attachment *, struct sg_table *, enum dma_data_direction); +void dma_buf_move_notify(struct dma_buf *dma_buf); int dma_buf_begin_cpu_access(struct dma_buf *dma_buf, enum dma_data_direction dir); int dma_buf_end_cpu_access(struct dma_buf *dma_buf, -- 2.17.1

5 years, 8 months

Dynamic DMA-buf locking changes

by Christian König

Hi everyone, since upstreaming the full dynamic DMA-buf changes turned out more problematic than previously thought I've reverted back to individual patches and separated out only the locking changes. So this patch does NOT contain any new callbacks for pinning/unpinning and move notification, but only the locking changes necessary. As previously discussed when the framework detects that the locking semantics between exporter and importer are different it just falls back to using a cached sgt created during attach time. While separating the patch set I've most likely stumbled over the problem why this previously raised some lockdep warning with i915, it turned out to be just a might_lock() at the wrong place. Please review and/or comment, Christian.

5 years, 8 months

Re: [Linaro-mm-sig] [PATCH] dma-buf/resv: fix exclusive fence get

by Koenig, Christian

Hi Qiang, oh, good point. Yes it certainly should. Looks like I accidentally pushed it to the wrong branch. Thanks, Christian. Am 10.10.19 um 16:27 schrieb Qiang Yu: > Hi Chris, > > This fix has been pushed to drm-misc-next for a while. But Linux > 5.4-rc kernels still does not have this fix. > Should it be also pushed to drm-misc-fixes? > > Thanks, > Qiang > > > On Sun, Sep 22, 2019 at 8:50 PM Chris Wilson <chris(a)chris-wilson.co.uk> wrote: >> Quoting Chris Wilson (2019-09-22 13:17:19) >>> Quoting Qiang Yu (2019-09-22 08:49:00) >>>> This causes kernel crash when testing lima driver. >>>> >>>> Cc: Christian König <christian.koenig(a)amd.com> >>>> Fixes: b8c036dfc66f ("dma-buf: simplify reservation_object_get_fences_rcu a bit") >>>> Signed-off-by: Qiang Yu <yuq825(a)gmail.com> >>>> --- >>>> drivers/dma-buf/dma-resv.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c >>>> index 42a8f3f11681..709002515550 100644 >>>> --- a/drivers/dma-buf/dma-resv.c >>>> +++ b/drivers/dma-buf/dma-resv.c >>>> @@ -471,7 +471,7 @@ int dma_resv_get_fences_rcu(struct dma_resv *obj, >>>> if (pfence_excl) >>>> *pfence_excl = fence_excl; >>>> else if (fence_excl) >>>> - shared[++shared_count] = fence_excl; >>>> + shared[shared_count++] = fence_excl; >>> Oops. >>> >>> Reviewed-by: Chris Wilson <chris(a)chris-wilson.co.uk> >> Applied, thanks for the fix. >> -Chris

5 years, 9 months

Re: [Linaro-mm-sig] [PATCH] dma-buf/resv: fix exclusive fence get

by Daniel Vetter

On Sun, Sep 22, 2019 at 2:08 PM Qiang Yu <yuq825(a)gmail.com> wrote: > > This causes kernel crash when testing lima driver. > > Cc: Christian König <christian.koenig(a)amd.com> > Fixes: b8c036dfc66f ("dma-buf: simplify reservation_object_get_fences_rcu a bit") > Signed-off-by: Qiang Yu <yuq825(a)gmail.com> Selftest for this would be lovely, now that the basic infrastructure is in place ... -Daniel > --- > drivers/dma-buf/dma-resv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 42a8f3f11681..709002515550 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -471,7 +471,7 @@ int dma_resv_get_fences_rcu(struct dma_resv *obj, > if (pfence_excl) > *pfence_excl = fence_excl; > else if (fence_excl) > - shared[++shared_count] = fence_excl; > + shared[shared_count++] = fence_excl; > > if (!shared_count) { > kfree(shared); > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years, 9 months

Re: [Linaro-mm-sig] [RFC PATCH] drm:- Add a modifier to denote 'protected' framebuffer

by Alex Deucher

On Mon, Sep 30, 2019 at 8:57 AM Ayan Halder <Ayan.Halder(a)arm.com> wrote: > > On Mon, Sep 30, 2019 at 09:51:35AM +0000, Brian Starkey wrote: > > Hi, > > > > On Tue, Sep 17, 2019 at 07:36:45PM +0200, Daniel Vetter wrote: > > > On Tue, Sep 17, 2019 at 6:15 PM Neil Armstrong <narmstrong(a)baylibre.com> wrote: > > > > > > > > Hi, > > > > > > > > On 17/09/2019 18:07, Liviu Dudau wrote: > > > > > On Tue, Sep 17, 2019 at 02:53:01PM +0200, Daniel Vetter wrote: > > > > >> On Mon, Sep 09, 2019 at 01:42:53PM +0000, Ayan Halder wrote: > > > > >>> Add a modifier 'DRM_FORMAT_MOD_ARM_PROTECTED' which denotes that the framebuffer > > > > >>> is allocated in a protected system memory. > > > > >>> Essentially, we want to support EGL_EXT_protected_content in our komeda driver. > > > > >>> > > > > >>> Signed-off-by: Ayan Kumar Halder <ayan.halder(a)arm.com> > > > > >>> > > > > >>> /-- Note to reviewer > > > > >>> Komeda driver is capable of rendering DRM (Digital Rights Management) protected > > > > >>> content. The DRM content is stored in a framebuffer allocated in system memory > > > > >>> (which needs some special hardware signals for access). > > > > >>> > > > > >>> Let us ignore how the protected system memory is allocated and for the scope of > > > > >>> this discussion, we want to figure out the best way possible for the userspace > > > > >>> to communicate to the drm driver to turn the protected mode on (for accessing the > > > > >>> framebuffer with the DRM content) or off. > > > > >>> > > > > >>> The possible ways by which the userspace could achieve this is via:- > > > > >>> > > > > >>> 1. Modifiers :- This looks to me the best way by which the userspace can > > > > >>> communicate to the kernel to turn the protected mode on for the komeda driver > > > > >>> as it is going to access one of the protected framebuffers. The only problem is > > > > >>> that the current modifiers describe the tiling/compression format. However, it > > > > >>> does not hurt to extend the meaning of modifiers to denote other attributes of > > > > >>> the framebuffer as well. > > > > >>> > > > > >>> The other reason is that on Android, we get an info from Gralloc > > > > >>> (GRALLOC_USAGE_PROTECTED) which tells us that the buffer is protected. This can > > > > >>> be used to set up the modifier/s (AddFB2) during framebuffer creation. > > > > >> > > > > >> How does this mesh with other modifiers, like AFBC? That's where I see the > > > > >> issue here. > > > > > > > > > > AFBC modifiers are currently under Arm's namespace, the thought behind the DRM > > > > > modifiers would be to have it as a "generic" modifier. > > > > > > But if it's a generic flag, how do you combine that with other > > > modifiers? Like if you have a tiled buffer, but also encrypted? Or > > > afbc compressed, or whatever else. I'd expect for your hw encryption > > > is orthogonal to the buffer/tiling/compression format used? > > > > This bit doesn't overlap with any of the other AFBC modifiers, so as > > you say it'd be orthogonal, and could be set on AFBC buffers (if we > > went that route). > > > > > > > > > >>> 2. Framebuffer flags :- As of today, this can be one of the two values > > > > >>> ie (DRM_MODE_FB_INTERLACED/DRM_MODE_FB_MODIFIERS). Unlike modifiers, the drm > > > > >>> framebuffer flags are generic to the drm subsystem and ideally we should not > > > > >>> introduce any driver specific constraint/feature. > > > > >>> > > > > >>> 3. Connector property:- I could see the following properties used for DRM > > > > >>> protected content:- > > > > >>> DRM_MODE_CONTENT_PROTECTION_DESIRED / ENABLED :- "This property is used by > > > > >>> userspace to request the kernel protect future content communicated over > > > > >>> the link". Clearly, we are not concerned with the protection attributes of the > > > > >>> transmitter. So, we cannot use this property for our case. > > > > >>> > > > > >>> 4. DRM plane property:- Again, we want to communicate that the framebuffer(which > > > > >>> can be attached to any plane) is protected. So introducing a new plane property > > > > >>> does not help. > > > > >>> > > > > >>> 5. DRM crtc property:- For the same reason as above, introducing a new crtc > > > > >>> property does not help. > > > > >> > > > > >> 6. Just track this as part of buffer allocation, i.e. I think it does > > > > >> matter how you allocate these protected buffers. We could add a "is > > > > >> protected buffer" flag at the dma_buf level for this. > > > > I also like this approach. The protected-ness is a property of the > > allocation, so makes sense to store it with the allocation IMO. > > > > > > >> > > > > >> So yeah for this stuff here I think we do want the full userspace side, > > > > >> from allocator to rendering something into this protected buffers (no need > > > > >> to also have the entire "decode a protected bitstream part" imo, since > > > > >> that will freak people out). Unfortunately, in my experience, that kills > > > > >> it for upstream :-/ But also in my experience of looking into this for > > > > >> other gpu's, we really need to have the full picture here to make sure > > > > >> we're not screwing this up. > > > > > > > > > > Maybe Ayan could've been a bit clearer in his message, but the ask here is for ideas > > > > > on how userspace "communicates" (stores?) the fact that the buffers are protected to > > > > > the kernel driver. In our display processor we need to the the hardware that the > > > > > buffers are protected before it tries to fetch them so that it can 1) enable the > > > > > additional hardware signaling that sets the protection around the stream; and 2) read > > > > > the protected buffers in a special mode where there the magic happens. > > > > > > That was clear, but for the full picture we also need to know how > > > these buffers are produced and where they are allocated. One approach > > > would be to have a dma-buf heap that gives you encrypted buffers back. > > > With that we need to make sure that only encryption-aware drivers > > > allow such buffers to be imported, and the entire problem becomes a > > > kernel-internal one - aside from allocating the right kind of buffer > > > at the right place. > > > > > > > In our case, we'd be supporting a system like TZMP-1, there's a > > Linaro connect presentation on it here: > > https://connect.linaro.org/resources/hkg18/hkg18-408/ > > > > The simplest way to implement this is for firmware to set up a > > carveout which it tells linux is secure. A linux allocator (ion, gem, > > vb2, whatever) can allocate from this carveout, and tag the buffer as > > secure. > > > > In this kind of system, linux doesn't necessarily need to know > > anything about how buffers are protected, or what HW is capable of - > > it only needs to carry around the "is_protected" flag. > > > > Here, the TEE is ultimately responsible for deciding which HW gets > > access to a buffer. I don't see a benefit of having linux decide which > > drivers can or cannot import a buffer, because that decision should be > > handled by the TEE. > > > > For proving out the pipeline, IMO it doesn't matter whether the > > buffers are protected or not. For our DPU, all that matters is that if > > the buffer claims to be protected, we have to set our protected > > control bit. Nothing more. AFAIK it should work the same for other > > TZMP-1 implementations. > > > > > > > So yeah, we know we do want full userspace support, we're prodding the community on > > > > > answers on how to best let the kernel side know what userspace has done. > > > > > > > > Actually this is interesting for other multimedia SoCs implementing secure video decode > > > > paths where video buffers are allocated and managed by a trusted app. > > > > > > Yeah I expect there's more than just arm wanting this. I also wonder > > > how that interacts with the secure memory allocator that was bobbing > > > around on dri-devel for a while, but seems to not have gone anywhere. > > > That thing implemented my idea of "secure memory is only allocated by > > > a special entity". > > > -Daniel > > > > Like I said, for us all we need is a way to carry around a 1-bit > > "is_protected" flag with a buffer. Could other folks share what's > > needed for their systems so we can reason about something that works > > for all? > > To make things a bit more specific, we are thinking of the following > patch :- > > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index ec212cb27fdc..36f0813073a2 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -279,6 +279,7 @@ struct dma_buf_ops { > * kernel module. > * @list_node: node for dma_buf accounting and debugging. > * @priv: exporter specific private data for this buffer object. > + * @is_protected: denotes that the buffer is > secure/protected/encrypted/trusted. > * @resv: reservation object linked to this dma-buf > * @poll: for userspace poll support > * @cb_excl: for userspace poll support > @@ -306,6 +307,7 @@ struct dma_buf { > struct module *owner; > struct list_head list_node; > void *priv; > + bool is_protected; > struct dma_resv *resv; > > /* poll support */ > > @all, @amdgpu-folks :- Is this something you can use of to denote > secure/protected/encrypted/trusted buffers ? I suppose. At the moment, we don't really have a need for it since we only our IPs support our encryption scheme and if we share buffers between we can get to the secure status when we look up the amdgpu buffer object internally in the kernel side. Still might be useful for cases where secure buffers get shared across drivers so we have a generic check for secure status. Alex > > The way 'is_protected' flag gets used to allocate > secure/protected/encrypted buffers will be vendor specific. > > Please comment to let us know if it looks useful to non Arm folks. > > > > Thanks! > > -Brian > > > > > > > > > > > > > Neil > > > > > > > > > > > > > > Best regards, > > > > > Liviu > > > > > > > > > > > > > > >> -Daniel > > > > >> > > > > >>> > > > > >>> --/ > > > > >>> > > > > >>> --- > > > > >>> include/uapi/drm/drm_fourcc.h | 9 +++++++++ > > > > >>> 1 file changed, 9 insertions(+) > > > > >>> > > > > >>> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > > > > >>> index 3feeaa3f987a..38e5e81d11fe 100644 > > > > >>> --- a/include/uapi/drm/drm_fourcc.h > > > > >>> +++ b/include/uapi/drm/drm_fourcc.h > > > > >>> @@ -742,6 +742,15 @@ extern "C" { > > > > >>> */ > > > > >>> #define AFBC_FORMAT_MOD_BCH (1ULL << 11) > > > > >>> > > > > >>> +/* > > > > >>> + * Protected framebuffer > > > > >>> + * > > > > >>> + * The framebuffer is allocated in a protected system memory which can be accessed > > > > >>> + * via some special hardware signals from the dpu. This is used to support > > > > >>> + * 'GRALLOC_USAGE_PROTECTED' in our framebuffer for EGL_EXT_protected_content. > > > > >>> + */ > > > > >>> +#define DRM_FORMAT_MOD_ARM_PROTECTED fourcc_mod_code(ARM, (1ULL << 55)) > > > > >>> + > > > > >>> /* > > > > >>> * Allwinner tiled modifier > > > > >>> * > > > > >>> -- > > > > >>> 2.23.0 > > > > >>> > > > > >> > > > > >> -- > > > > >> Daniel Vetter > > > > >> Software Engineer, Intel Corporation > > > > >> http://blog.ffwll.ch > > > > > > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel(a)lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

5 years, 9 months

Re: [Linaro-mm-sig] [PATCH] ion_system_heap: support X86 archtecture

by Laura Abbott

On 9/29/19 3:28 AM, jun.zhang(a)intel.com wrote: > From: zhang jun <jun.zhang(a)intel.com> > > we see tons of warning like: > [ 45.846872] x86/PAT: NDK MediaCodec_:3753 map pfn RAM range req > write-combining for [mem 0x1e7a80000-0x1e7a87fff], got write-back > [ 45.848827] x86/PAT: .vorbis.decoder:4088 map pfn RAM range req > write-combining for [mem 0x1e7a58000-0x1e7a58fff], got write-back > [ 45.848875] x86/PAT: NDK MediaCodec_:3753 map pfn RAM range req > write-combining for [mem 0x1e7a48000-0x1e7a4ffff], got write-back > [ 45.849403] x86/PAT: .vorbis.decoder:4088 map pfn RAM range > req write-combining for [mem 0x1e7a70000-0x1e7a70fff], got write-back > > check the kernel Documentation/x86/pat.txt, it says: > A. Exporting pages to users with remap_pfn_range, io_remap_pfn_range, > vm_insert_pfn > Drivers wanting to export some pages to userspace do it by using > mmap interface and a combination of > 1) pgprot_noncached() > 2) io_remap_pfn_range() or remap_pfn_range() or vm_insert_pfn() > With PAT support, a new API pgprot_writecombine is being added. > So, drivers can continue to use the above sequence, with either > pgprot_noncached() or pgprot_writecombine() in step 1, followed by step 2. > > In addition, step 2 internally tracks the region as UC or WC in > memtype list in order to ensure no conflicting mapping. > > Note that this set of APIs only works with IO (non RAM) regions. > If driver ants to export a RAM region, it has to do set_memory_uc() or > set_memory_wc() as step 0 above and also track the usage of those pages > and use set_memory_wb() before the page is freed to free pool. > > the fix follow the pat document, do set_memory_wc() as step 0 and > use the set_memory_wb() before the page is freed. > All this work needs to be done on the new dma-buf heap rework and I don't think it makes sense to put it on the staging version https://lore.kernel.org/lkml/20190906184712.91980-1-john.stultz@linaro.org/ (I also continue to question the value of uncached buffers, especially on x86) > Signed-off-by: he, bo <bo.he(a)intel.com> > Signed-off-by: zhang jun <jun.zhang(a)intel.com> > Signed-off-by: Bai, Jie A <jie.a.bai(a)intel.com> > --- > drivers/staging/android/ion/ion_system_heap.c | 28 ++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c > index b83a1d16bd89..d298b8194820 100644 > --- a/drivers/staging/android/ion/ion_system_heap.c > +++ b/drivers/staging/android/ion/ion_system_heap.c > @@ -13,6 +13,7 @@ > #include <linux/scatterlist.h> > #include <linux/slab.h> > #include <linux/vmalloc.h> > +#include <asm/set_memory.h> > > #include "ion.h" > > @@ -134,6 +135,13 @@ static int ion_system_heap_allocate(struct ion_heap *heap, > sg = table->sgl; > list_for_each_entry_safe(page, tmp_page, &pages, lru) { > sg_set_page(sg, page, page_size(page), 0); > + > +#ifdef CONFIG_X86 > + if (!(buffer->flags & ION_FLAG_CACHED)) > + set_memory_wc((unsigned long)page_address(sg_page(sg)), > + PAGE_ALIGN(sg->length) >> PAGE_SHIFT); > +#endif > + > sg = sg_next(sg); > list_del(&page->lru); > } > @@ -162,8 +170,15 @@ static void ion_system_heap_free(struct ion_buffer *buffer) > if (!(buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE)) > ion_heap_buffer_zero(buffer); > > - for_each_sg(table->sgl, sg, table->nents, i) > + for_each_sg(table->sgl, sg, table->nents, i) { > +#ifdef CONFIG_X86 > + if (!(buffer->flags & ION_FLAG_CACHED)) > + set_memory_wb((unsigned long)page_address(sg_page(sg)), > + PAGE_ALIGN(sg->length) >> PAGE_SHIFT); > +#endif > + > free_buffer_page(sys_heap, buffer, sg_page(sg)); > + } > sg_free_table(table); > kfree(table); > } > @@ -316,6 +331,12 @@ static int ion_system_contig_heap_allocate(struct ion_heap *heap, > > buffer->sg_table = table; > > +#ifdef CONFIG_X86 > + if (!(buffer->flags & ION_FLAG_CACHED)) > + set_memory_wc((unsigned long)page_address(page), > + PAGE_ALIGN(len) >> PAGE_SHIFT); > +#endif > + > return 0; > > free_table: > @@ -334,6 +355,11 @@ static void ion_system_contig_heap_free(struct ion_buffer *buffer) > unsigned long pages = PAGE_ALIGN(buffer->size) >> PAGE_SHIFT; > unsigned long i; > > +#ifdef CONFIG_X86 > + if (!(buffer->flags & ION_FLAG_CACHED)) > + set_memory_wb((unsigned long)page_address(page), pages); > +#endif > + > for (i = 0; i < pages; i++) > __free_page(page + i); > sg_free_table(table); >

5 years, 9 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig