Linaro-mm-sig March 2024

linaro-mm-sig@lists.linaro.org

19 participants
34 discussions

Re: [PATCH v3 07/14] drm/mediatek: Rename files "mtk_drm_crtc.c" to "mtk_crtc.c"

by AngeloGioacchino Del Regno

Il 20/03/24 03:42, Shawn Sung ha scritto: > From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com> > > Rename files mtk_drm_crtc.c to mtk_crtc.c and > modify the Makefile accordingly. > > Reviewed-by: CK Hu <ck.hu(a)mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

9 months, 1 week

[PATCH] dma-buf: Fix NULL pointer dereference in sanitycheck()

by Pavel Sakharov

If due to a memory allocation failure mock_chain() returns NULL, it is passed to dma_fence_enable_sw_signaling() resulting in NULL pointer dereference there. Call dma_fence_enable_sw_signaling() only if mock_chain() succeeds. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: d62c43a953ce ("dma-buf: Enable signaling on fence for selftests") Signed-off-by: Pavel Sakharov <p.sakharov(a)ispras.ru> --- drivers/dma-buf/st-dma-fence-chain.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/dma-buf/st-dma-fence-chain.c b/drivers/dma-buf/st-dma-fence-chain.c index 9c2a0c082a76..ed4b323886e4 100644 --- a/drivers/dma-buf/st-dma-fence-chain.c +++ b/drivers/dma-buf/st-dma-fence-chain.c @@ -84,11 +84,11 @@ static int sanitycheck(void *arg) return -ENOMEM; chain = mock_chain(NULL, f, 1); - if (!chain) + if (chain) + dma_fence_enable_sw_signaling(chain); + else err = -ENOMEM; - dma_fence_enable_sw_signaling(chain); - dma_fence_signal(f); dma_fence_put(f); -- 2.44.0

9 months, 1 week

Re: [PATCH v2 02/14] drm/mediatek: Rename "mtk_drm_ddp_comp" to "mtk_ddp_comp"

by AngeloGioacchino Del Regno

Il 19/03/24 08:02, Shawn Sung ha scritto: > From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com> > > Rename all "mtk_drm_ddp_comp" to "mtk_ddp_comp": > - To align the naming rule > - To reduce the code size > > Reviewed-by: AngeloGiaocchino Del Regno <angelogioacchino.delregno(a)collabora.com> Shawn, I don't know if I typoed my own name (which is actually possible, since I write the tags by hand), or what actually happened to my Reviewed-by tags on the entire series. Can you please fix the typo in the tag? Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com> Use this one, please. Thanks, Angelo > Reviewed-by: CK Hu <ck.hu(a)mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com>

9 months, 1 week

Re: [PATCH v2 11/14] drm/mediatek: Rename files "mtk_drm_plane.c" to "mtk_plane.c"

by AngeloGioacchino Del Regno

Il 19/03/24 08:02, Shawn Sung ha scritto: > From: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com> > > Rename files mtk_drm_plane.c to mtk_plane.c and > modify the Makefile accordingly. > > Signed-off-by: Hsiao Chien Sung <shawn.sung(a)mediatek.corp-partner.google.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno(a)collabora.com>

9 months, 1 week

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Fri, Mar 15, 2024 at 06:36:19PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 16:15, Mark Brown wrote: > > On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > > > > In the register. You only need to reset the gain to -40dB at the start > > > > of the ramp. > > > Sorry but I don't understand your logic, I'm not able to implement it... > > > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > > > register the value will be -40dB. > > After we've done the ramp and turned the amplifier off we can just > > restore the desired value? The hardware is not going to care what the > > volume is while it's not enabled. > If you do that, HP will be enabled at the saved gain, and after that you > will do the ramp. To avoid pop, the driver should be rewrite to: So reset the volume to -40dB prior to turning the amplifier on... > Read gain in the reg and save it locally > Set -40dB in the reg > Enable HP > Do ramp ...as you yourself suggest? > > > AFAII from the comment in the code, it's done to avoid the "pop noises". > > Yes, that's the usual reason to ramp gains. Though if you've just > > copied the code without checking that it's needed it's possible that > > this is something that's been fixed in current hardware. > I did the test at 24dB with and without the "pop filter". Isn't big but I > ear the pop at the start of the record without the "pop filter". OK, it probably is still doing something then. > To be clear, the algo/behavior of this code is an implementation based on > the 6k+ lines downstream code for this specific audio codec. But the > shape/style is based on upstreamed drivers like mt6358.c. The Mediatek code has a bunch of issues, I wouldn't read too much into something being present in the code.

9 months, 2 weeks

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Fri, Mar 15, 2024 at 04:05:21PM +0100, Alexandre Mergnat wrote: > On 15/03/2024 15:30, Mark Brown wrote: > > > Let me know, when you change de gain to do a ramp down (start from user gain > > > to gain=-40db), next time for the ramp up, how/where do you find the user > > > gain ? > > In the register. You only need to reset the gain to -40dB at the start > > of the ramp. > Sorry but I don't understand your logic, I'm not able to implement it... > If I'm at -10dB and doing a ramp to reach -40dB, next time I will read the > register the value will be -40dB. After we've done the ramp and turned the amplifier off we can just restore the desired value? The hardware is not going to care what the volume is while it's not enabled. > This implementation is also done in other MTK audio codec drivers. Perhaps they should be updated too? > > > When microphone isn't capturing, the gain read back from the register is > > > 0dB. I've put some logs in my code and do capture to show how it works: > > Is this a property of the hardware or a property of your driver? > At the end of the capture, the gain is set to 0dB by the driver. > At the start of the capture, the gain is set to the setup gain. So that's a property of the driver then? > AFAII from the comment in the code, it's done to avoid the "pop noises". Yes, that's the usual reason to ramp gains. Though if you've just copied the code without checking that it's needed it's possible that this is something that's been fixed in current hardware.

9 months, 2 weeks

Re: [PATCH 00/18] Add audio support for the MediaTek Genio 350-evk board

by Mark Brown

On Tue, Mar 12, 2024 at 09:58:05AM +0100, Alexandre Mergnat wrote: > I'm a bit lost for mixer-test and pcm-test. > Currently, I cross-compile the alsa lib project to be able to build the > tests and put it on my board. > I can execute it, but I still have 2 issues: > 1) I've a lot of missing module in my environment (Encode.so, Encode.pm, > Symbol.pm, IO/Handle.pm, ...). AFAII, I've to cross compile the missing perl > modules and install them in the rootfs These tests are both simple C programs... > 2) I don't know how to configure pcm-test.conf & > Lenovo_ThinkPad_P1_Gen2.conf (or new file to match with my board). The configuration is optional. > My test cmd: > ./run_kselftest.sh -c alsa Just run the programs directly. I'm only asking for the output from two of them anyway.

9 months, 2 weeks

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Fri, Mar 15, 2024 at 12:01:12PM +0100, Alexandre Mergnat wrote: > On 13/03/2024 18:23, Mark Brown wrote: > > On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: > > > Actually you must save the values because the gain selected by the user will > > > be override to do a ramp => volume_ramp(.....): > > > - When you switch on the HP, you start from gain=-40db to final_gain > > > (selected by user). > > > - When you switch off the HP, you start from final_gain (selected by user) > > > to gain=-40db. > > You can just read the value back when you need to do a ramp? > You can't. Because you will read -40db when HP isn't playing sound. That is > why the gain is saved into the struct. > Let me know, when you change de gain to do a ramp down (start from user gain > to gain=-40db), next time for the ramp up, how/where do you find the user > gain ? In the register. You only need to reset the gain to -40dB at the start of the ramp. > > > Also, the microphone's gain change when it's enabled/disabled. > > I don't understand what this means? > When microphone isn't capturing, the gain read back from the register is > 0dB. I've put some logs in my code and do capture to show how it works: Is this a property of the hardware or a property of your driver? > > > > > + /* ul channel swap */ > > > > > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > > > > On/off controls should end in Switch. > > > Sorry, I don't understand your comment. Can you reword it please ? > > See control-names.rst. Run mixer-test on a card with this driver and > > fix all the issues it reports. > Ok the name is the issue for you AFAII. > This control isn't for on/off but swap Left and Right. > From the codec documentation: > "Swaps audio UL L/R channel before UL SRC" > This control is overkill, I will remove it This is turning the swapping on and off.

9 months, 2 weeks

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Wed, Mar 13, 2024 at 06:11:50PM +0100, Alexandre Mergnat wrote: > On 26/02/2024 17:09, Mark Brown wrote: > > > index 000000000000..13e95c227114 > > > --- /dev/null > > > +++ b/sound/soc/codecs/mt6357.c > > > @@ -0,0 +1,1805 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * MT6357 ALSA SoC audio codec driver > > Please use a C++ comment for the whole comment to make it clearer that > > this is intentional. > If I do that, the checkpatch raise a warning: > WARNING: Improper SPDX comment style for > 'sound/soc/mediatek/mt8365/mt8365-afe-clk.c', please use '//' instead > #22: FILE: sound/soc/mediatek/mt8365/mt8365-afe-clk.c:1: > +/* SPDX-License-Identifier: GPL-2.0 That's not a C++ comment so checkpatch is correctly warning?

9 months, 2 weeks

Re: [PATCH 12/18] ASoC: codecs: mt6357: add MT6357 codec

by Mark Brown

On Tue, Mar 12, 2024 at 07:03:25PM +0100, Alexandre Mergnat wrote: > On 26/02/2024 17:09, Mark Brown wrote: > > > + case MT6357_ZCD_CON2: > > > + regmap_read(priv->regmap, MT6357_ZCD_CON2, &reg); > > > + priv->ana_gain[ANALOG_VOLUME_HPOUTL] = > > > + (reg & AUD_HPL_GAIN_MASK) >> AUD_HPL_GAIN_SFT; > > > + priv->ana_gain[ANALOG_VOLUME_HPOUTR] = > > > + (reg & AUD_HPR_GAIN_MASK) >> AUD_HPR_GAIN_SFT; > > > + break; > > It would probably be less code and would definitely be clearer and > > simpler to just read the values when we need them rather than constatly > > keeping a cache separate to the register cache. > Actually you must save the values because the gain selected by the user will > be override to do a ramp => volume_ramp(.....): > - When you switch on the HP, you start from gain=-40db to final_gain > (selected by user). > - When you switch off the HP, you start from final_gain (selected by user) > to gain=-40db. You can just read the value back when you need to do a ramp? > Also, the microphone's gain change when it's enabled/disabled. I don't understand what this means? > > > + /* ul channel swap */ > > > + SOC_SINGLE("UL LR Swap", MT6357_AFE_UL_DL_CON0, AFE_UL_LR_SWAP_SFT, 1, 0), > > On/off controls should end in Switch. > Sorry, I don't understand your comment. Can you reword it please ? See control-names.rst. Run mixer-test on a card with this driver and fix all the issues it reports. > > > +static int hslo_mux_map_value[] = { > > > + 0x0, 0x1, 0x2, 0x3, > > > +}; > > Why not just use a normal mux here, there's no missing values or > > reordering? Similarly for other muxes. > I've dug into some other codecs and it's done like that, but I've probably > misunderstood something. > The only bad thing I see is enum is missing currently: > > enum { > PGA_MUX_OPEN = 0, > PGA_MUX_DACR, > PGA_MUX_PB, > PGA_MUX_TM, > PGA_MUX_MASK = 0x3, > }; The whole thing with explicitly specfying the mapping is just completely redundant, you may as well remove it.

9 months, 2 weeks

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig March 2024