Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

2 participants
3208 discussions

by Thomas Hellström

Hi! I'm seeing the below lockdep splat 1) with the xe driver in an imported dma-buf object destruction path. It's not because we hold the dma_resv lock at that point, but rather because we hold *another* dma_resv lock at that point, and the dma_resv detach happens when the object is idle, in this case it was idle at the final put(), and dma_buf_detach() is called in the putting process. Holding another dma-buf lock might happen as part of drm_exec_unlock_all, or simply if the wider vm dma_resv was held at object put time, so it's not an uncommon pattern, even if the drm_exec instance can be fixed by putting all bos after unlocking them all. Two solutions coming to mind here: 1) Provide a dma_buf_detach_locked() 2) Have TTM always take the delayed delete path for imported dma-buf objects. I'd prefer 1) since I think the correct place to call this is in the TTM callback delete_mem_notify() where the bo is already locked, and I figure non-TTM gem backends may come to suffer from the same problem. Opinions, suggestions? [1] [ 99.136161] ============================================ [ 99.136162] WARNING: possible recursive locking detected [ 99.136163] 6.10.0-rc2+ #6 Tainted: G U [ 99.136165] -------------------------------------------- [ 99.136166] glxgears:sh0/4675 is trying to acquire lock: [ 99.136167] ffff9967dcdd91a8 (reservation_ww_class_mutex){+.+.}- {3:3}, at: dma_buf_detach+0x3b/0xf0 [ 99.136184] but task is already holding lock: [ 99.136186] ffff9967d8c145a8 (reservation_ww_class_mutex){+.+.}- {3:3}, at: drm_exec_lock_obj+0x49/0x2b0 [drm_exec] [ 99.136191] other info that might help us debug this: [ 99.136192] Possible unsafe locking scenario: [ 99.136194] CPU0 [ 99.136194] ---- [ 99.136195] lock(reservation_ww_class_mutex); [ 99.136197] lock(reservation_ww_class_mutex); [ 99.136199] *** DEADLOCK *** [ 99.136199] May be due to missing lock nesting notation [ 99.136200] 5 locks held by glxgears:sh0/4675: [ 99.136202] #0: ffff9967d8c104c8 (&xef->vm.lock){+.+.}-{3:3}, at: xe_file_close+0xde/0x1c0 [xe] [ 99.136272] #1: ffff9967d5bb7480 (&vm->lock){++++}-{3:3}, at: xe_vm_close_and_put+0x161/0x9b0 [xe] [ 99.136350] #2: ffff9967ef88a970 (&val->lock){.+.+}-{3:3}, at: xe_validation_ctx_init+0x6d/0x70 [xe] [ 99.136440] #3: ffffbd6a085577b8 (reservation_ww_class_acquire){+.+.}-{0:0}, at: xe_vma_destroy_unlocked+0x7f/0xe0 [xe] [ 99.136546] #4: ffff9967d8c145a8 (reservation_ww_class_mutex){+.+.}-{3:3}, at: drm_exec_lock_obj+0x49/0x2b0 [drm_exec] [ 99.136552] stack backtrace: [ 99.136553] CPU: 10 PID: 4675 Comm: glxgears:sh0 Tainted: G U 6.10.0-rc2+ #6 [ 99.136555] Hardware name: ASUS System Product Name/PRIME B560M-A AC, BIOS 2001 02/01/2023 [ 99.136557] Call Trace: [ 99.136558] <TASK> [ 99.136560] dump_stack_lvl+0x77/0xb0 [ 99.136564] __lock_acquire+0x1232/0x2160 [ 99.136569] lock_acquire+0xcb/0x2d0 [ 99.136570] ? dma_buf_detach+0x3b/0xf0 [ 99.136574] ? __lock_acquire+0x417/0x2160 [ 99.136577] __ww_mutex_lock.constprop.0+0xd0/0x13b0 [ 99.136580] ? dma_buf_detach+0x3b/0xf0 [ 99.136584] ? dma_buf_detach+0x3b/0xf0 [ 99.136588] ? ww_mutex_lock+0x2b/0x90 [ 99.136590] ww_mutex_lock+0x2b/0x90 [ 99.136592] dma_buf_detach+0x3b/0xf0 [ 99.136595] drm_prime_gem_destroy+0x2f/0x40 [drm] [ 99.136638] xe_ttm_bo_destroy+0x32/0x220 [xe] [ 99.136734] ? __mutex_unlock_slowpath+0x3a/0x290 [ 99.136738] drm_exec_unlock_all+0xa1/0xd0 [drm_exec] [ 99.136741] drm_exec_fini+0x12/0xb0 [drm_exec] [ 99.136743] xe_validation_ctx_fini+0x15/0x40 [xe] [ 99.136848] xe_vma_destroy_unlocked+0xb1/0xe0 [xe] [ 99.136954] xe_vm_close_and_put+0x41a/0x9b0 [xe] [ 99.137056] ? xa_find+0xe3/0x1e0 [ 99.137060] xe_file_close+0x10a/0x1c0 [xe] [ 99.137157] drm_file_free+0x22a/0x280 [drm] [ 99.137193] drm_release_noglobal+0x22/0x70 [drm] [ 99.137227] __fput+0xf1/0x2d0 [ 99.137231] task_work_run+0x59/0x90 [ 99.137235] do_exit+0x330/0xb40 [ 99.137238] do_group_exit+0x36/0xa0 [ 99.137241] get_signal+0xbd2/0xbe0 [ 99.137245] arch_do_signal_or_restart+0x3e/0x240 [ 99.137249] syscall_exit_to_user_mode+0x1e7/0x290 [ 99.137252] do_syscall_64+0xa1/0x180 [ 99.137255] ? _raw_spin_unlock+0x23/0x40 [ 99.137257] ? look_up_lock_class+0x6f/0x120 [ 99.137261] ? __lock_acquire+0x417/0x2160 [ 99.137264] ? lock_acquire+0xcb/0x2d0 [ 99.137266] ? __set_task_comm+0x28/0x1e0 [ 99.137268] ? find_held_lock+0x2b/0x80 [ 99.137271] ? __set_task_comm+0xe1/0x1e0 [ 99.137273] ? lock_release+0xca/0x290 [ 99.137277] ? __do_sys_prctl+0x245/0xab0 [ 99.137279] ? lockdep_hardirqs_on_prepare+0xde/0x190 [ 99.137281] ? syscall_exit_to_user_mode+0xb0/0x290 [ 99.137284] ? do_syscall_64+0xa1/0x180 [ 99.137286] ? cpuset_cpus_allowed+0x36/0x140 [ 99.137289] ? find_held_lock+0x2b/0x80 [ 99.137291] ? find_held_lock+0x2b/0x80 [ 99.137294] ? __sched_setaffinity+0x78/0x240 [ 99.137297] ? kfree+0xe2/0x310 [ 99.137301] ? kfree+0x202/0x310 [ 99.137303] ? __sched_setaffinity+0x78/0x240 [ 99.137305] ? __x64_sys_sched_setaffinity+0x69/0xb0 [ 99.137307] ? kfree+0xe2/0x310 [ 99.137310] ? lockdep_hardirqs_on_prepare+0xde/0x190 [ 99.137312] ? syscall_exit_to_user_mode+0xb0/0x290 [ 99.137315] ? do_syscall_64+0xa1/0x180 [ 99.137317] ? trace_hardirqs_off+0x4b/0xc0 [ 99.137321] ? clear_bhb_loop+0x45/0xa0 [ 99.137325] ? clear_bhb_loop+0x45/0xa0 [ 99.137327] ? clear_bhb_loop+0x45/0xa0 [ 99.137330] entry_SYSCALL_64_after_hwframe+0x76/0x7e [ 99.137333] RIP: 0033:0x7fda70ee6169 [ 99.137351] Code: Unable to access opcode bytes at 0x7fda70ee613f. [ 99.137352] RSP: 002b:00007fda5fdffc80 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca [ 99.137354] RAX: fffffffffffffe00 RBX: 0000000000000000 RCX: 00007fda70ee6169 [ 99.137356] RDX: 0000000000000000 RSI: 0000000000000189 RDI: 0000564a96f45b30 [ 99.137358] RBP: 00007fda5fdffcb0 R08: 0000000000000000 R09: 00000000ffffffff [ 99.137359] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000 [ 99.137360] R13: 0000000000000000 R14: 0000000000000000 R15: 0000564a96f45b30 [ 99.137365] </TASK>

1 year, 5 months

[PATCH v12 0/7] iio: new DMABUF based API v12

by Paul Cercueil

Hi Jonathan, Here's the v12 of my patchset that introduces DMABUF support to IIO. Apart from a small documentation fix, it reverts to using mutex_lock/mutex_unlock in one particular place, which used cleanup GOTOs (which don't play well with scope-managed cleanups). Changelog: - [3/7]: - Revert to mutex_lock/mutex_unlock in iio_buffer_attach_dmabuf(), as it uses cleanup GOTOs - [6/7]: - "obtained using..." -> "which can be obtained using..." This is based on next-20240619. Cheers, -Paul Paul Cercueil (7): dmaengine: Add API function dmaengine_prep_peripheral_dma_vec() dmaengine: dma-axi-dmac: Implement device_prep_peripheral_dma_vec iio: core: Add new DMABUF interface infrastructure iio: buffer-dma: Enable support for DMABUFs iio: buffer-dmaengine: Support new DMABUF based userspace API Documentation: iio: Document high-speed DMABUF based API Documentation: dmaengine: Document new dma_vec API Documentation/driver-api/dmaengine/client.rst | 9 + .../driver-api/dmaengine/provider.rst | 10 + Documentation/iio/iio_dmabuf_api.rst | 54 +++ Documentation/iio/index.rst | 1 + drivers/dma/dma-axi-dmac.c | 40 ++ drivers/iio/Kconfig | 1 + drivers/iio/buffer/industrialio-buffer-dma.c | 178 ++++++- .../buffer/industrialio-buffer-dmaengine.c | 62 ++- drivers/iio/industrialio-buffer.c | 459 ++++++++++++++++++ include/linux/dmaengine.h | 33 ++ include/linux/iio/buffer-dma.h | 31 ++ include/linux/iio/buffer_impl.h | 33 ++ include/uapi/linux/iio/buffer.h | 22 + 13 files changed, 913 insertions(+), 20 deletions(-) create mode 100644 Documentation/iio/iio_dmabuf_api.rst -- 2.43.0

1 year, 5 months

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 27.06.24 um 05:17 schrieb Jason-JH Lin (林睿祥): > > On Wed, 2024-06-26 at 12:49 +0200, Christian König wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > > > "restricted_mtk_cma", but there is no way to pass the > > > secure flag > > > > > or > > > > > private data from userspace to the import interface in DRM > > > driver. > > > > > > > > Why do you need to verify that? > > > > > > I need to know the imported buffer is allocated from restricted cma > > > and > > > mark it as a secure buffer in mediatek-drm driver. Then, I will add > > > some configuration to the hardware if the buffer is secure buffer, > > > so > > > that it can get the permission to access the secure buffer. > > > > Yeah so far that makes sense. This is basically what other drivers do > > with secure buffers as well. > > > > But why do you want the kernel to transport that information? Usually > > drivers get the information from userspace what to do with a buffer. > > > > In other words the format, stride, tilling and also if it's a secure > > buffer or not comes from userspace. > > > > Thanks for your clear explanation. > I think this is what I want, but I can't find any DRM interface to pass > the secure flag from userspace. Well stuff like that is usually device driver specific. So you should probably use something device specific which tells the driver that this buffer is encrypted. > > What the hardware usually handles internally is things like > > encryption keys, but you eventually get the information where to look > > for the key from userspace as well. > > > > Handling inside the kernel would only be necessary if userspace could > > for example crash the system with invalid parameters. But for > > encryption that is usually not the case. > > > > Yes, that's true. > > > > > > > > > > So I can only verify it like this now: > > > > > 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; > > > > > } > > > > > > > > Complete NAK from my side to that approach. Importing of a DMA- > > > buf > > > > should be independent of the exporter. > > > > > > > > What you could do is to provide the secure buffer from a device > > > and > > > > not a device heap. > > > > > > > > > > You mean I should allocate buffer in mediate-drm driver not > > > userspace? > > > > Well that depends. The question is if you have multiple drivers which > > needs to work with this secure buffer? > > > > If yes then you should have a general allocation heap for it. If no > > then the buffers could as well be allocated from the driver interface > > directly. > > > > Yes, this buffer needs work with GPU and DRM drivers, so this general > "restricted_mtk_cma" will allocated in userspace, then being passed to > GPU and DRM. Well do you really need a separate heap for that? In other words is only a certain part of the system memory capable of being encrypted? Or would the "normal" CMA heap do as well and you only need to setup your hardware properly for encryption? Additional to that in most other drivers buffer sharing and encryption are two separate things. In other words other drivers do something like this: 1. Allocate the buffer. 2. Import the buffer using DRM_IOCTL_PRIME_FD_TO_HANDLE. 3. Set additional buffer properties, e.g. format, stride, tilling, if it's secure, which encryption key to use, where to map etc... So as far as I can see the problem you are facing is that you try to mangle everything into DRM_IOCTL_PRIME_FD_TO_HANDLE. Why not make that a separate IOCTL? I mean we intentionally don't provide things like format, stride, tilling etc.. to DRM_IOCTL_PRIME_FD_TO_HANDLE. Encryption is just another of those parameters. Regards, Christian. > > > > I just have modified this to userspace by the comment here: > > > > > > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > > > > > I think it would be better to have the user configurable > > > private > > > > > information in dma-buf, so all the drivers who have the same > > > > > requirement can get their private information from dma-buf > > > directly > > > > > and > > > > > no need to change or add the interface. > > > > > > > > > > What's your opinion in this point? > > > > > > > > Well of hand I don't see the need for that. > > > > > > > > What happens if you get a non-secure buffer imported in your > > > secure > > > > device? > > > > > > We use the same mediatek-drm driver for secure and non-secure > > > buffer. > > > If non-secure buffer imported to mediatek-drm driver, it's go to > > > the > > > normal flow with normal hardware settings. > > > > > > We use different configurations to make hardware have different > > > permission to access the buffer it should access. > > > > > > So if we can't get the information of "the buffer is allocated from > > > restricted_mtk_cma" when importing the buffer into the driver, we > > > won't > > > be able to configure the hardware correctly. > > > > Why can't you get this information from userspace? > > As I mentioned here: > > https://patchwork.kernel.org/project/linux-mediatek/cover/20240525232928.55… > > I tried some DRM interfaces using buffer FD and arg->flag as > parameters, but it didn't work. So I ask for your help here. > > But I think I should find DRM maintainer to add the secure flag to DRM > interface now. > > Regards, > Jason-JH.Lin > > ************* 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!

1 year, 5 months

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 26.06.24 um 10:05 schrieb Jason-JH Lin (林睿祥): > > > > > > In the step 3), we need to verify the dma-buf is allocated from > > > "restricted_mtk_cma", but there is no way to pass the secure flag > > > or > > > private data from userspace to the import interface in DRM driver. > > > > Why do you need to verify that? > > I need to know the imported buffer is allocated from restricted cma and > mark it as a secure buffer in mediatek-drm driver. Then, I will add > some configuration to the hardware if the buffer is secure buffer, so > that it can get the permission to access the secure buffer. Yeah so far that makes sense. This is basically what other drivers do with secure buffers as well. But why do you want the kernel to transport that information? Usually drivers get the information from userspace what to do with a buffer. In other words the format, stride, tilling and also if it's a secure buffer or not comes from userspace. What the hardware usually handles internally is things like encryption keys, but you eventually get the information where to look for the key from userspace as well. Handling inside the kernel would only be necessary if userspace could for example crash the system with invalid parameters. But for encryption that is usually not the case. > > > > > So I can only verify it like this now: > > > 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; > > > } > > > > Complete NAK from my side to that approach. Importing of a DMA-buf > > should be independent of the exporter. > > > > What you could do is to provide the secure buffer from a device and > > not a device heap. > > > > You mean I should allocate buffer in mediate-drm driver not userspace? Well that depends. The question is if you have multiple drivers which needs to work with this secure buffer? If yes then you should have a general allocation heap for it. If no then the buffers could as well be allocated from the driver interface directly. > I just have modified this to userspace by the comment here: > > https://patchwork.kernel.org/project/linux-mediatek/patch/20240403102701.36… > > > > I think I have the same problem as the ECC_FLAG mention in: > > > > > > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > > > > > I think it would be better to have the user configurable private > > > information in dma-buf, so all the drivers who have the same > > > requirement can get their private information from dma-buf directly > > > and > > > no need to change or add the interface. > > > > > > What's your opinion in this point? > > > > Well of hand I don't see the need for that. > > > > What happens if you get a non-secure buffer imported in your secure > > device? > > We use the same mediatek-drm driver for secure and non-secure buffer. > If non-secure buffer imported to mediatek-drm driver, it's go to the > normal flow with normal hardware settings. > > We use different configurations to make hardware have different > permission to access the buffer it should access. > > So if we can't get the information of "the buffer is allocated from > restricted_mtk_cma" when importing the buffer into the driver, we won't > be able to configure the hardware correctly. Why can't you get this information from userspace? Regards, Christian. > > Regards, > Jason-JH.Lin > > > > > Regards, > > Christian. > > > > > Regards, > > > Jason-JH.Lin > > > > > > > Regards, > > > > Christian. > > > > > > ************* 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! > > > > ************* 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!

1 year, 5 months

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 19/06/2024 16:46, Alexandre Mergnat wrote: > Add the audio codec sub-device. This sub-device is used to set the > optional voltage values according to the hardware. > The properties are: > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Also, add the audio power supply property which is dedicated for > the audio codec sub-device. > > Signed-off-by: Alexandre Mergnat <amergnat(a)baylibre.com> > --- > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..d95307393e75 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,32 @@ properties: > "#interrupt-cells": > const: 2 > > + vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec > + > + audio-codec: > + type: object Still not much improved. You do not have any resources there, so these should go to the parent node. Best regards, Krzysztof

1 year, 5 months

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 26/06/2024 10:30, Alexandre Mergnat wrote: > > > On 25/06/2024 15:44, Krzysztof Kozlowski wrote: >> On 25/06/2024 11:23, Alexandre Mergnat wrote: >>> >>> >>> On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >>>> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>>>> Add the audio codec sub-device. This sub-device is used to set the >>>>> optional voltage values according to the hardware. >>>>> The properties are: >>>>> - Setup of microphone bias voltage. >>>>> - Setup of the speaker pin pull-down. >>>>> >>>>> Also, add the audio power supply property which is dedicated for >>>>> the audio codec sub-device. >>>>> >>>>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>>>> --- >>>>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>>>> 1 file changed, 33 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> index 37423c2e0fdf..d95307393e75 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> @@ -37,6 +37,32 @@ properties: >>>>> "#interrupt-cells": >>>>> const: 2 >>>>> >>>>> + vaud28-supply: >>>>> + description: 2.8 volt supply phandle for the audio codec >>>>> + >>>>> + audio-codec: >>>>> + type: object >>>> Still not much improved. You do not have any resources there, so these >>>> should go to the parent node. >>> >>> Hi Krzysztof, >>> >>> vaud28-supply seems to be a mistake that I forward port. >>> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" >>> sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as >>> power-supply. >>> >>> pwrap { >>> pmic { >>> ... >>> audio-codec { >>> ... >>> >>> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of >>> PMIC-audio-codec subsystem. Then: >>> - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a >>> power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) >>> - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to >>> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec >>> substystem, beside mediatek,micbias0-microvolt >> >> I don't understand why do we talk again about supply. My comment was not >> under the supply. > > Because your word are: > " > And now you should see how odd it looks. Supplies are part of entire > chip, not subblock, even if they supply dedicated domain within that chip. > > That's why I asked to put it in the parent node. > " > > My bad, I forgot to link you the old message in my previous answer [1] > > [1] https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b0201b@kernel.org… And you implemented this, so why do we talk again about it? It is already solved, isn't it? Since previous version? Best regards, Krzysztof

1 year, 5 months

Re: [PATCH v6 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC

by Krzysztof Kozlowski

On 25/06/2024 11:23, Alexandre Mergnat wrote: > > > On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>> Add the audio codec sub-device. This sub-device is used to set the >>> optional voltage values according to the hardware. >>> The properties are: >>> - Setup of microphone bias voltage. >>> - Setup of the speaker pin pull-down. >>> >>> Also, add the audio power supply property which is dedicated for >>> the audio codec sub-device. >>> >>> Signed-off-by: Alexandre Mergnat<amergnat(a)baylibre.com> >>> --- >>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> index 37423c2e0fdf..d95307393e75 100644 >>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> @@ -37,6 +37,32 @@ properties: >>> "#interrupt-cells": >>> const: 2 >>> >>> + vaud28-supply: >>> + description: 2.8 volt supply phandle for the audio codec >>> + >>> + audio-codec: >>> + type: object >> Still not much improved. You do not have any resources there, so these >> should go to the parent node. > > Hi Krzysztof, > > vaud28-supply seems to be a mistake that I forward port. > In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" > sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as > power-supply. > > pwrap { > pmic { > ... > audio-codec { > ... > > Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of > PMIC-audio-codec subsystem. Then: > - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a > power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) > - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec > substystem, beside mediatek,micbias0-microvolt I don't understand why do we talk again about supply. My comment was not under the supply. Best regards, Krzysztof

1 year, 5 months

Re: [PATCH v5 2/9] scatterlist: Add a flag for the restricted memory

by Christian König

Am 25.06.24 um 13:02 schrieb Jason-JH Lin (林睿祥): > > Hi Christian, > > On Tue, 2024-05-21 at 20:36 +0200, Christian König wrote: > > Am 20.05.24 um 09:58 schrieb Yong Wu (吴勇): > > > On Thu, 2024-05-16 at 10:17 +0200, Christian König wrote: > > > > > > > > External email : Please do not click links or open attachments > > > > until > > > > you have verified the sender or the content. > > > > Am 15.05.24 um 13:23 schrieb Yong Wu: > > > > > Introduce a FLAG for the restricted memory which means the > > > > > memory > > > > > > > > is > > > > > protected by TEE or hypervisor, then it's inaccessiable for > > > > > kernel. > > > > > > > > > > Currently we don't use sg_dma_unmark_restricted, thus this > > > > > > > > interface > > > > > has not been added. > > > > > > > > Why should that be part of the scatterlist? It doesn't seem to > > > > affect > > > > any of it's functionality. > > > > > > > > As far as I can see the scatterlist shouldn't be the transport of > > > > this > > > > kind of information. > > > > > > Thanks for the review. I will remove this. > > > > > > In our user scenario, DRM will import these buffers and check if > > > this > > > is a restricted buffer. If yes, it will use secure GCE takes over. > > > > > > If this judgment is not suitable to be placed in scatterlist. I > > > don't > > > know if it is ok to limit this inside dma-buf. Adding such an > > > interface: > > > > > > static bool dma_buf_is_restricted(struct dma_buf *dmabuf) > > > { > > > return !strncmp(dmabuf->exp_name, "restricted", 10); > > > } > > > > No, usually stuff like that doesn't belong into DMA buf either. > > > > Question here really is who controls the security status of the > > memory > > backing the buffer? > > > > In other words who tells the exporter that it should allocate and > > fill a > > buffer with encrypted data? > > > > If that is userspace then that is part of the format information and > > it > > is also userspace who should tell the importer that it needs to work > > with encrypted data. > > > > The kernel is intentionally not involved in stuff like that. > > > > Here is the expected protected content buffer flow in DRM: > 1) userspace allocates a dma-buf FD from the "restricted_mtk_cma" by > DMA_HEAP_IOCTL_ALLOC. > 2) userspace imports that dma-buf into the device using prime for the > drm_file. > 3) userspace uses the already implemented driver import code for the > special cases of protected content buffer. What is so special on that case? > > In the step 3), we need to verify the dma-buf is allocated from > "restricted_mtk_cma", but there is no way to pass the secure flag or > private data from userspace to the import interface in DRM driver. Why do you need to verify that? > > So I can only verify it like this now: > 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; > } Complete NAK from my side to that approach. Importing of a DMA-buf should be independent of the exporter. What you could do is to provide the secure buffer from a device and not a device heap. > I think I have the same problem as the ECC_FLAG mention in: > > https://lore.kernel.org/linux-media/20240515-dma-buf-ecc-heap-v1-0-54cbbd04… > > I think it would be better to have the user configurable private > information in dma-buf, so all the drivers who have the same > requirement can get their private information from dma-buf directly and > no need to change or add the interface. > > What's your opinion in this point? Well of hand I don't see the need for that. What happens if you get a non-secure buffer imported in your secure device? Regards, Christian. > > Regards, > Jason-JH.Lin > > > Regards, > > Christian. > > ************* 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!

1 year, 5 months

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > From: Nicolas Belin <nbelin(a)baylibre.com> > > Add the support of MT6357 PMIC audio codec. This breaks an x86 allmodconfig build: /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt_delay_250_event’: /build/stage/linux/sound/soc/codecs/mt6357.c:993:29: error: unused variable ‘pri v’ [-Werror=unused-variable] 993 | struct mt6357_priv *priv = snd_soc_component_get_drvdata(cmpnt); | ^~~~ /build/stage/linux/sound/soc/codecs/mt6357.c: In function ‘mt6357_platform_drive r_probe’: /build/stage/linux/sound/soc/codecs/mt6357.c:1867:55: error: too many arguments for format [-Werror=format-extra-args] 1867 | return dev_err_probe(&pdev->dev, ret, "Failed to parse d ts\n", __func__); | ^~~~~~~~~~~~~~~~~~ ~~~~~ cc1: all warnings being treated as errors

1 year, 5 months

Re: [PATCH v6 12/16] ASoC: codecs: add MT6357 support

by Mark Brown

On Wed, Jun 19, 2024 at 04:46:48PM +0200, amergnat(a)baylibre.com wrote: > + /* gain default values*/ > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON0, MT6357_AUDPREAMPLGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPLGAIN_SFT); > + regmap_update_bits(priv->regmap, MT6357_AUDENC_ANA_CON1, MT6357_AUDPREAMPRGAIN_MASK, > + UL_GAIN_0DB << MT6357_AUDPREAMPRGAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON1, > + MT6357_AUD_LOL_GAIN_MASK | > + MT6357_AUD_LOR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_LOL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_LOR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON2, > + MT6357_AUD_HPL_GAIN_MASK | > + MT6357_AUD_HPR_GAIN_MASK, > + DL_GAIN_0DB << MT6357_AUD_HPL_GAIN_SFT | > + DL_GAIN_0DB << MT6357_AUD_HPR_GAIN_SFT); > + > + regmap_update_bits(priv->regmap, MT6357_ZCD_CON3, > + MT6357_AUD_HS_GAIN_MASK, DL_GAIN_0DB); We generally leave everything at chip defaults, why is this different? > +static int mt6357_codec_probe(struct snd_soc_component *codec) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + mt6357_codec_init(priv); > + return 0; > +} Why not just inline mt6357_codec_init() into the one user? > +static unsigned int mt6357_read(struct snd_soc_component *codec, unsigned int reg) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + unsigned int val; > + > + regmap_read(priv->regmap, reg, &val); > + return val; > +} > + > +static int mt6357_write(struct snd_soc_component *codec, unsigned int reg, unsigned int value) > +{ > + struct mt6357_priv *priv = snd_soc_component_get_drvdata(codec); > + > + return regmap_update_bits(priv->regmap, reg, 0xffff, value); > +} Why open code these, the core has standard adaptors for regmap? > +static const u32 micbias_values[MT6357_MICBIAS_ARRAY_SIZE] = { > + 1700000, 1800000, 1900000, 2000000, > + 2100000, 2500000, 2600000, 2700000 > +}; Just use ARRAY_SIZE() for the size then the number can't be out of sync.

1 year, 5 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig