Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Am Dienstag, dem 20.04.2021 um 13:47 + schrieb Robin Gong: > On 2021/04/19 17:46 Lucas Stach wrote: > > Am Montag, dem 19.04.2021 um 07:17 + schrieb Robin Gong: > > > Hi Lucas, > > > > > > On 2021/04/14 Lucas Stach wrote: > > > > Hi Robin, > > > > > > > > Am Mittwoch, dem 14.04.2021 um 14:33 + schrieb Robin Gong: > > > > > On 2020/05/20 17:43 Lucas Stach wrote: > > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > > > > > > > Hi > > > > > > > > > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach > > > > > > > > > > > > > wrote: > > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > > > > > > > There are two requirements that we need to move the > > > > > > > > > request of dma channel from probe to open. > > > > > > > > > > > > > > > > How do you handle -EPROBE_DEFER return code from the channel > > > > > > > > request if you don't do it in probe? > > > > > > > > > > > > > > I use the dma_request_slave_channel or dma_request_channel > > > > > > > instead of dmaengine_pcm_request_chan_of. so there should be > > > > > > > not -EPROBE_DEFER return code. > > > > > > > > > > > > This is a pretty weak argument. The dmaengine device might probe > > > > > > after you try to get the channel. Using a function to request > > > > > > the channel that doesn't allow you to handle probe deferral is > > > > > > IMHO a bug and should be fixed, instead of building even more > > > > > > assumptions on top > > > > of it. > > > > > > > > > > > > > > > - When dma device binds with power-domains, the power will > > > > > > > > > be enabled when we request dma channel. If the request of > > > > > > > > > dma channel happen on probe, then the power-domains will > > > > > > > > > be always enabled after kernel boot up, which is not good > > > > > > > > > for power saving, so we need to move the request of dma > > > > > > > > > channel to .open(); > > > > > > > > > > > > > > > > This is certainly something which could be fixed in the > > > > > > > > dmaengine driver. > > > > > > > > > > > > > > Dma driver always call the pm_runtime_get_sync in > > > > > > > device_alloc_chan_resources, the device_alloc_chan_resources > > > > > > > is called when channel is requested. so power is enabled on > > > > > > > channel > > > > request. > > > > > > > > > > > > So why can't you fix the dmaengine driver to do that RPM call at > > > > > > a later time when the channel is actually going to be used? This > > > > > > will allow further power savings with other slave devices than the > > > > > > audio > > PCM. > > > > > Hi Lucas, > > > > > Thanks for your suggestion. I have tried to implement runtime > > > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 > > > > > sec) for this feature as below (or you can refer to > > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/ > > > > > pm_runtime_put_autosuspend in all dmaengine driver interface like > > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_c > > > > > ycli > > > > > c/ > > > > > device_tx_status... > > > > > > > > > > > > > > > pm_runtime_use_autosuspend(fsl_chan->dev); > > > > > pm_runtime_set_autosuspend_delay(fsl_chan-> > > dev, > > > > 2000); > > > > > > > > > > That could resolve this audio case since the autosuspend could > > > > > suspend runtime after > > > > > 2 seconds if there is no further dma transfer but only channel > > > > request(device_alloc_chan_resources). > > > > > But unfortunately, it cause another issue. As you know, on our > > > > > i.mx8qm/qxp, power domain do
Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Am Montag, dem 19.04.2021 um 07:17 + schrieb Robin Gong:
> Hi Lucas,
>
> On 2021/04/14 Lucas Stach wrote:
> > Hi Robin,
> >
> > Am Mittwoch, dem 14.04.2021 um 14:33 + schrieb Robin Gong:
> > > On 2020/05/20 17:43 Lucas Stach wrote:
> > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > > > Hi
> > > > >
> > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > >
> > > > wrote:
> > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > > > There are two requirements that we need to move the request of
> > > > > > > dma channel from probe to open.
> > > > > >
> > > > > > How do you handle -EPROBE_DEFER return code from the channel
> > > > > > request if you don't do it in probe?
> > > > >
> > > > > I use the dma_request_slave_channel or dma_request_channel instead
> > > > > of dmaengine_pcm_request_chan_of. so there should be not
> > > > > -EPROBE_DEFER return code.
> > > >
> > > > This is a pretty weak argument. The dmaengine device might probe
> > > > after you try to get the channel. Using a function to request the
> > > > channel that doesn't allow you to handle probe deferral is IMHO a
> > > > bug and should be fixed, instead of building even more assumptions on
> > > > top
> > of it.
> > > >
> > > > > > > - When dma device binds with power-domains, the power will be
> > > > > > > enabled when we request dma channel. If the request of dma
> > > > > > > channel happen on probe, then the power-domains will be always
> > > > > > > enabled after kernel boot up, which is not good for power
> > > > > > > saving, so we need to move the request of dma channel to
> > > > > > > .open();
> > > > > >
> > > > > > This is certainly something which could be fixed in the
> > > > > > dmaengine driver.
> > > > >
> > > > > Dma driver always call the pm_runtime_get_sync in
> > > > > device_alloc_chan_resources, the device_alloc_chan_resources is
> > > > > called when channel is requested. so power is enabled on channel
> > request.
> > > >
> > > > So why can't you fix the dmaengine driver to do that RPM call at a
> > > > later time when the channel is actually going to be used? This will
> > > > allow further power savings with other slave devices than the audio PCM.
> > > Hi Lucas,
> > > Thanks for your suggestion. I have tried to implement runtime
> > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2 sec)
> > > for this feature as below (or you can refer to
> > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > > pm_runtime_put_autosuspend in all dmaengine driver interface like
> > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cycli
> > > c/
> > > device_tx_status...
> > >
> > >
> > > pm_runtime_use_autosuspend(fsl_chan->dev);
> > > pm_runtime_set_autosuspend_delay(fsl_chan->dev,
> > 2000);
> > >
> > > That could resolve this audio case since the autosuspend could suspend
> > > runtime after
> > > 2 seconds if there is no further dma transfer but only channel
> > request(device_alloc_chan_resources).
> > > But unfortunately, it cause another issue. As you know, on our
> > > i.mx8qm/qxp, power domain done by scfw (drivers/firmware/imx/scu-pd.c)
> > over mailbox:
> > > imx_sc_pd_power()->imx_scu_call_rpc()->
> > > imx_scu_ipc_write()->mbox_send_message()
> > > which means have to 'waits for completion', meanwhile, some driver
> > > like tty will call dmaengine interfaces in non-atomic case as below,
> > >
> > > static int uart_write(struct tty_struct *tty, const unsigned char
> > > *buf, int count) {
> > > ...
> > > port = uart_port_lock(state, flags);
> > > ..
> > > __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg...
> > > uart_port_unlock(port, flags);
> > > return ret;
> > > }
> > >
> > > Thus dma runtime resume may happen in that timing window and cause
> > kernel alarm.
> > >
Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register
Am Freitag, dem 16.04.2021 um 15:08 +0200 schrieb Benjamin Gaignard:
> Le 16/04/2021 à 12:54, Lucas Stach a écrit :
> > Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> > > In order to be able to share the control hardware block between
> > > VPUs use a syscon instead a ioremap it in the driver.
> > > To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> > > phandle is not found look at 'ctrl' reg-name.
> > > With the method it becomes useless to provide a list of register
> > > names so remove it.
> > Sorry for putting a spoke in the wheel after many iterations of the
> > series.
> >
> > We just discussed a way forward on how to handle the clocks and resets
> > provided by the blkctl block on i.MX8MM and later and it seems there is
> > a consensus on trying to provide virtual power domains from a blkctl
> > driver, controlling clocks and resets for the devices in the power
> > domain. I would like to avoid introducing yet another way of handling
> > the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
> > what we are planning to do on the later chip generations.
> >
> > CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
> > virtual power domain thing a shot.
>
> That could replace the 3 first patches and Dt patche of this series
> but that will not impact the hevc part, so I wonder if pure hevc patches
> could be merged anyway ?
> They are reviewed and don't depend of how the ctrl block is managed.
I'm not really in a position to give any informed opinion about that
hvec patches, as I only skimmed them, but I don't see any reason to
delay patches 04-11 from this series until the i.MX8M platform issues
are sorted. AFAICS those things are totally orthogonal.
Regards,
Lucas
> >
> > Regards,
> > Lucas
> >
> > > Signed-off-by: Benjamin Gaignard
> > > Reviewed-by: Philipp Zabel
> > > ---
> > > version 9:
> > > - Corrections in commit message
> > >
> > > version 7:
> > > - Add Philipp reviewed-by tag.
> > > - Change syscon phandle name.
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > version 5:
> > > - use syscon instead of VPU reset driver.
> > > - if DT doesn't provide syscon keep backward compatibilty by using
> > > 'ctrl' reg-name.
> > >
> > > drivers/staging/media/hantro/hantro.h | 5 +-
> > > drivers/staging/media/hantro/imx8m_vpu_hw.c | 52 -
> > > 2 files changed, 34 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro.h
> > > b/drivers/staging/media/hantro/hantro.h
> > > index 6c1b888abe75..37b9ce04bd4e 100644
> > > --- a/drivers/staging/media/hantro/hantro.h
> > > +++ b/drivers/staging/media/hantro/hantro.h
> > > @@ -13,6 +13,7 @@
> > > #define HANTRO_H_
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -167,7 +168,7 @@ hantro_vdev_to_func(struct video_device *vdev)
> > > * @reg_bases: Mapped addresses of VPU registers.
> > > * @enc_base: Mapped address of VPU encoder register for
> > > convenience.
> > > * @dec_base: Mapped address of VPU decoder register for
> > > convenience.
> > > - * @ctrl_base: Mapped address of VPU control block.
> > > + * @ctrl_base: Regmap of VPU control block.
> > > * @vpu_mutex: Mutex to synchronize V4L2 calls.
> > > * @irqlock:Spinlock to synchronize access to data
> > > structures
> > > * shared with interrupt handlers.
> > > @@ -186,7 +187,7 @@ struct hantro_dev {
> > > void __iomem **reg_bases;
> > > void __iomem *enc_base;
> > > void __iomem *dec_base;
> > > - void __iomem *ctrl_base;
> > > + struct regmap *ctrl_base;
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > struct mutex vpu_mutex; /* video_device lock */
> > > spinlock_t irqlock;
> > > diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> > > index c222de075ef4..8d0c3425234
Re: [PATCH v9 03/13] media: hantro: Use syscon instead of 'ctrl' register
Am Mittwoch, dem 07.04.2021 um 09:35 +0200 schrieb Benjamin Gaignard:
> In order to be able to share the control hardware block between
> VPUs use a syscon instead a ioremap it in the driver.
> To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> phandle is not found look at 'ctrl' reg-name.
> With the method it becomes useless to provide a list of register
> names so remove it.
Sorry for putting a spoke in the wheel after many iterations of the
series.
We just discussed a way forward on how to handle the clocks and resets
provided by the blkctl block on i.MX8MM and later and it seems there is
a consensus on trying to provide virtual power domains from a blkctl
driver, controlling clocks and resets for the devices in the power
domain. I would like to avoid introducing yet another way of handling
the blkctl and thus would like to align the i.MX8MQ VPU blkctl with
what we are planning to do on the later chip generations.
CC'ing Jacky Bai and Peng Fan from NXP, as they were going to give this
virtual power domain thing a shot.
Regards,
Lucas
> Signed-off-by: Benjamin Gaignard
> Reviewed-by: Philipp Zabel
> ---
> version 9:
> - Corrections in commit message
>
> version 7:
> - Add Philipp reviewed-by tag.
> - Change syscon phandle name.
>
>
>
>
> version 5:
> - use syscon instead of VPU reset driver.
> - if DT doesn't provide syscon keep backward compatibilty by using
> 'ctrl' reg-name.
>
> drivers/staging/media/hantro/hantro.h | 5 +-
> drivers/staging/media/hantro/imx8m_vpu_hw.c | 52 -
> 2 files changed, 34 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/media/hantro/hantro.h
> b/drivers/staging/media/hantro/hantro.h
> index 6c1b888abe75..37b9ce04bd4e 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -13,6 +13,7 @@
> #define HANTRO_H_
>
>
>
>
> #include
> +#include
> #include
> #include
> #include
> @@ -167,7 +168,7 @@ hantro_vdev_to_func(struct video_device *vdev)
> * @reg_bases: Mapped addresses of VPU registers.
> * @enc_base:Mapped address of VPU encoder register for
> convenience.
> * @dec_base:Mapped address of VPU decoder register for
> convenience.
> - * @ctrl_base: Mapped address of VPU control block.
> + * @ctrl_base: Regmap of VPU control block.
> * @vpu_mutex: Mutex to synchronize V4L2 calls.
> * @irqlock: Spinlock to synchronize access to data structures
> * shared with interrupt handlers.
> @@ -186,7 +187,7 @@ struct hantro_dev {
> void __iomem **reg_bases;
> void __iomem *enc_base;
> void __iomem *dec_base;
> - void __iomem *ctrl_base;
> + struct regmap *ctrl_base;
>
>
>
>
> struct mutex vpu_mutex; /* video_device lock */
> spinlock_t irqlock;
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index c222de075ef4..8d0c3425234b 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -7,6 +7,7 @@
>
>
>
>
> #include
> #include
> +#include
>
>
>
>
> #include "hantro.h"
> #include "hantro_jpeg.h"
> @@ -24,30 +25,28 @@
> #define CTRL_G1_PP_FUSE 0x0c
> #define CTRL_G2_DEC_FUSE 0x10
>
>
>
>
> +static const struct regmap_config ctrl_regmap_ctrl = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 0x14,
> +};
> +
> static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> {
> - u32 val;
> -
> /* Assert */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val &= ~reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, reset_bits, 0);
>
>
>
>
> udelay(2);
>
>
>
>
> /* Release */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val |= reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET,
> +reset_bits, reset_bits);
> }
>
>
>
>
> static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> {
> - u32 val;
> -
> - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> - val |= clock_bits;
> - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> + regmap_update_bits(vpu->ctrl_base, CTRL_CLOCK_ENABLE,
> +clock_bits, clock_bits);
> }
>
>
>
>
> static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> @@ -64,9 +63,9 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
>
>
>
>
> /* Set values of the fuse registers */
> - writel(0x, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> - writel(0x, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> -
Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Hi Robin,
Am Mittwoch, dem 14.04.2021 um 14:33 + schrieb Robin Gong:
> On 2020/05/20 17:43 Lucas Stach wrote:
> > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > Hi
> > >
> > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > wrote:
> > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > There are two requirements that we need to move the request of dma
> > > > > channel from probe to open.
> > > >
> > > > How do you handle -EPROBE_DEFER return code from the channel request
> > > > if you don't do it in probe?
> > >
> > > I use the dma_request_slave_channel or dma_request_channel instead of
> > > dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER
> > > return code.
> >
> > This is a pretty weak argument. The dmaengine device might probe after you
> > try to get the channel. Using a function to request the channel that doesn't
> > allow you to handle probe deferral is IMHO a bug and should be fixed,
> > instead
> > of building even more assumptions on top of it.
> >
> > > > > - When dma device binds with power-domains, the power will be
> > > > > enabled when we request dma channel. If the request of dma channel
> > > > > happen on probe, then the power-domains will be always enabled
> > > > > after kernel boot up, which is not good for power saving, so we
> > > > > need to move the request of dma channel to .open();
> > > >
> > > > This is certainly something which could be fixed in the dmaengine
> > > > driver.
> > >
> > > Dma driver always call the pm_runtime_get_sync in
> > > device_alloc_chan_resources, the device_alloc_chan_resources is called
> > > when channel is requested. so power is enabled on channel request.
> >
> > So why can't you fix the dmaengine driver to do that RPM call at a later
> > time
> > when the channel is actually going to be used? This will allow further power
> > savings with other slave devices than the audio PCM.
> Hi Lucas,
> Thanks for your suggestion. I have tried to implement runtime autosuspend in
> fsl-edma driver on i.mx8qm/qxp with delay time (2 sec) for this feature as
> below
> (or you can refer to drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> pm_runtime_put_autosuspend in all dmaengine driver interface like
> device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_cyclic/
> device_tx_status...
>
>
> pm_runtime_use_autosuspend(fsl_chan->dev);
> pm_runtime_set_autosuspend_delay(fsl_chan->dev, 2000);
>
> That could resolve this audio case since the autosuspend could suspend
> runtime after
> 2 seconds if there is no further dma transfer but only channel
> request(device_alloc_chan_resources).
> But unfortunately, it cause another issue. As you know, on our i.mx8qm/qxp,
> power domain done by scfw (drivers/firmware/imx/scu-pd.c) over mailbox:
> imx_sc_pd_power()->imx_scu_call_rpc()->
> imx_scu_ipc_write()->mbox_send_message()
> which means have to 'waits for completion', meanwhile, some driver like tty
> will call dmaengine
> interfaces in non-atomic case as below,
>
> static int uart_write(struct tty_struct *tty, const unsigned char *buf, int
> count)
> {
> ...
> port = uart_port_lock(state, flags);
> ..
> __uart_start(tty); //call start_tx()->dmaengine_prep_slave_sg...
> uart_port_unlock(port, flags);
> return ret;
> }
>
> Thus dma runtime resume may happen in that timing window and cause kernel
> alarm.
> I'm not sure whether there are similar limitations on other driver subsystem.
> But for me,
> It looks like the only way to resolve the contradiction between tty and
> scu-pd (hardware
> limitation on i.mx8qm/qxp) is to give up autosuspend and keep
> pm_runtime_get_sync
> only in device_alloc_chan_resources because request channel is a safe
> non-atomic phase.
> Do you have any idea? Thanks in advance.
If you look closely at the driver you used as an example (hidma.c) it
looks like there is already something in there, which looks very much
like what you need here:
In hidma_issue_pending() the driver tries to get the device to runtime
resume. If this doesn't work, maybe due to the power domain code not
being able to be called in atomic context, the actual work of waking up
the dma hardware and issuing the descriptor is shunted to a tasklet.
If I'm reading this right, this is exactly what you need here to be
able to call the dma
Re: [PATCH v2 0/7] remove different PHY fixups
Hi Russell, sorry for the noise of this arriving in your inbox twice. Apparently I messed up and replied in private in my last mail. Am Dienstag, dem 13.04.2021 um 11:51 +0100 schrieb Russell King - ARM Linux admin: > On Tue, Apr 13, 2021 at 12:00:45PM +0200, Lucas Stach wrote: > > I agree with the opinion that those PHY fixups introduce more harm than > > good. Essentially they are pushing board specific configuration values > > into the PHY, without any checks that the fixup is even running on the > > specific board it was targeted at. > > Yes and no. The problem is, that's an easy statement to make when one > doesn't understand what they're all doing. > > Some are "board specific" in that the normal setup for e.g. iMX6 would > be to enable clock output from the AR8035 PHY and feed that into the > iMX6 - as far as I'm aware, that's the only working configuration for > that SoC and PHY. However, it's also true that this fixup should not > be applied unconditionally. > > Then there's SmartEEE - it has been found that the PHY defaults for > this lead to link drops independent of the board and SoC that it is > connected to. It seems that the PHY is essentially broken - it powers > up with SmartEEE enabled, and when connected to another SmartEEE > supporting device, it seems guaranteed that it will result in link > drops in its default configuration. > > Freescale's approach has apparently been to unconditionally disable > SmartEEE for all their platforms because of this. With a bit of > research however (as has been done by Jon and myself) we've found > that increasing the Tw parameter for 1G connections results in a > much more stable link. > > So, just saying that these are bad without actually understanding what > they are doing is _also_ bad. I'm not saying the fixups are bad per se. What I'm saying is that they are inherently board specific and the right way to apply them is either via DT properties, or if absolutely necessary via a fixup that at least checks that it is running on the specific board it was targeted at. While SmartEEE disabling will cause no big harm, aside from a bit more power consumption, a wrong clock configuration can cause major confusion. Especially if the configuration in DT and values put into the PHY via fixups differ from each other. Regards, Lucas
Re: [PATCH v2 0/7] remove different PHY fixups
Hi all, Am Dienstag, dem 09.03.2021 um 12:26 +0100 schrieb Oleksij Rempel: > changes v2: > - rebase against latest kernel > - fix networking on RIoTBoard > > This patch series tries to remove most of the imx6 and imx7 board > specific PHY configuration via fixup, as this breaks the PHYs when > connected to switch chips or USB Ethernet MACs. > > Each patch has the possibility to break boards, but contains a > recommendation to fix the problem in a more portable and future-proof > way. I agree with the opinion that those PHY fixups introduce more harm than good. Essentially they are pushing board specific configuration values into the PHY, without any checks that the fixup is even running on the specific board it was targeted at. While there is a real chance to break some out of tree boards or incomplete configs I think that's something we can accept here. If someone makes a case why they can absolutely not fixup their DT or kernel config we could even bring back some of those fixups with a proper board compatible check to avoid mashing things up for other boards with the same PHY. I guess the only realistic way to learn if someone can make such a case is to apply this series and look for any fallout. So for what it is worth: Acked-by: Lucas Stach > regards, > Oleksij > > Oleksij Rempel (7): > ARM: imx6q: remove PHY fixup for KSZ9031 > ARM: imx6q: remove TX clock delay of ar8031_phy_fixup() > ARM: imx6q: remove hand crafted PHY power up in ar8035_phy_fixup() > ARM: imx6q: remove clk-out fixup for the Atheros AR8031 and AR8035 > PHYs > ARM: imx6q: remove Atheros AR8035 SmartEEE fixup > ARM: imx6sx: remove Atheros AR8031 PHY fixup > ARM: imx7d: remove Atheros AR8031 PHY fixup > > arch/arm/boot/dts/imx6dl-riotboard.dts | 2 + > arch/arm/boot/dts/imx6q-dmo-edmqmx6.dts | 2 +- > arch/arm/mach-imx/mach-imx6q.c | 85 - > arch/arm/mach-imx/mach-imx6sx.c | 26 > arch/arm/mach-imx/mach-imx7d.c | 22 --- > 5 files changed, 3 insertions(+), 134 deletions(-) >
Re: [PATCH v3] drivers: introduce and use WANT_DMA_CMA for soft dependencies on DMA_CMA
Am Freitag, dem 09.04.2021 um 13:20 +0200 schrieb David Hildenbrand:
> Random drivers should not override a user configuration of core knobs
> (e.g., CONFIG_DMA_CMA=n). Applicable drivers would like to use DMA_CMA,
> which depends on CMA, if possible; however, these drivers also have to
> tolerate if DMA_CMA is not available/functioning, for example, if no CMA
> area for DMA_CMA use has been setup via "cma=X". In the worst case, the
> driver cannot do it's job properly in some configurations.
>
> For example, commit 63f5677544b3 ("drm/etnaviv: select CMA and DMA_CMA if
> available") documents
> While this is no build dependency, etnaviv will only work correctly
> on most systems if CMA and DMA_CMA are enabled. Select both options
> if available to avoid users ending up with a non-working GPU due to
> a lacking kernel config.
> So etnaviv really wants to have DMA_CMA, however, can deal with some cases
> where it is not available.
>
> Let's introduce WANT_DMA_CMA and use it in most cases where drivers
> select CMA/DMA_CMA, or depend on DMA_CMA (in a wrong way via CMA because
> of recursive dependency issues).
>
> We'll assume that any driver that selects DRM_GEM_CMA_HELPER or
> DRM_KMS_CMA_HELPER would like to use DMA_CMA if possible.
>
> With this change, distributions can disable CONFIG_CMA or
> CONFIG_DMA_CMA, without it silently getting enabled again by random
> drivers. Also, we'll now automatically try to enabled both, CONFIG_CMA
> and CONFIG_DMA_CMA if they are unspecified and any driver is around that
> selects WANT_DMA_CMA -- also implicitly via DRM_GEM_CMA_HELPER or
> DRM_KMS_CMA_HELPER.
>
> For example, if any driver selects WANT_DMA_CMA and we do a
> "make olddefconfig":
>
> 1. With "# CONFIG_CMA is not set" and no specification of
> "CONFIG_DMA_CMA"
>
> -> CONFIG_DMA_CMA won't be part of .config
>
> 2. With no specification of CONFIG_CMA or CONFIG_DMA_CMA
>
> Contiguous Memory Allocator (CMA) [Y/n/?] (NEW)
> DMA Contiguous Memory Allocator (DMA_CMA) [Y/n/?] (NEW)
>
> 3. With "# CONFIG_CMA is not set" and "# CONFIG_DMA_CMA is not set"
>
> -> CONFIG_DMA_CMA will be removed from .config
>
> Note: drivers/remoteproc seems to be special; commit c51e882cd711
> ("remoteproc/davinci: Update Kconfig to depend on DMA_CMA") explains that
> there is a real dependency to DMA_CMA for it to work; leave that dependency
> in place and don't convert it to a soft dependency.
Hm, to me this sounds much like the reasoning for the etnaviv
dependency. There is no actual build dependency, as the allocations are
done through the DMA API, but for the allocations to succeed you most
likely want CMA to be enabled. But that's just an observation from the
outside, I have no real clue about the remoteproc drivers.
As far as the etnaviv changes are concerned:
Acked-by: Lucas Stach
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Joel Stanley
> Cc: Andrew Jeffery
> Cc: Lucas Stach
> Cc: Russell King
> Cc: Christian Gmeiner
> Cc: Paul Cercueil
> Cc: Linus Walleij
> Cc: Christoph Hellwig
> Cc: Marek Szyprowski
> Cc: Robin Murphy
> Cc: Andrew Morton
> Cc: Mike Rapoport
> Cc: Arnd Bergmann
> Cc: Bartlomiej Zolnierkiewicz
> Cc: Eric Anholt
> Cc: Michal Simek
> Cc: Masahiro Yamada
> Cc: "Alexander A. Klimov"
> Cc: Peter Collingbourne
> Cc: Suman Anna
> Cc: Jason Gunthorpe
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-asp...@lists.ozlabs.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: etna...@lists.freedesktop.org
> Cc: linux-m...@vger.kernel.org
> Cc: linux-fb...@vger.kernel.org
> Cc: io...@lists.linux-foundation.org
> Signed-off-by: David Hildenbrand
> ---
>
> Let's see if this approach is better for soft dependencies (and if we
> actually have some hard dependencies in there). This is the follow-up
> of
> https://lkml.kernel.org/r/20210408092011.52763-1-da...@redhat.com
> https://lkml.kernel.org/r/20210408100523.63356-1-da...@redhat.com
>
> I was wondering if it would make sense in some drivers to warn if either
> CONFIG_DMA_CMA is not available or if DRM_CMA has not been configured
> properly - just to give people a heads up that something might more likely
> go wrong; that would, however, be future work.
>
> v2 -> v3:
> - Don't use "imply" but instead use a new WANT_DMA_CMA and make the default
> of CMA and DMA_CMA depend on it.
> - Also adjust ingenic, mcde, tve200; these sound like soft dependencies as
> well (although DMA_CMA is really desired)
>
> v1 -&g
Re: [PATCH v1 0/7] imx-gpcv2 improvements
Hi Adrien, I feel like I already mentioned to you some time ago that there is already a much more complete patch series to add this functionality on the list [1]. If you want this functionality to go upstream, please help test and extend this patch series. Regards, Lucas [1] https://lore.kernel.org/linux-arm-kernel/20201105174434.1817539-1-l.st...@pengutronix.de/ Am Mittwoch, dem 07.04.2021 um 23:21 +0200 schrieb Adrien Grassein: > Hi, > > This patch set aims is to add the support of the i.MX8 MM power domains > on the mainline kernel. > > To achieve this, I do several patches > - Check errors when reading or writing registers (concerns i.MX8M base > implementation); > - Fix power up/down sequence. Handshake was not checked and it was > not called at the appropriate time (concerns i.MX8M base > implementaions); > - Allow domains without power sequence control like the HSIOMIX of the > i.MX8MM. > - Add some i.MX8MM domains (HSIO and OTGS); > - Introduce quirks. For example, i.MX8MM OTG domains should not be > powered off (seen n the source code of th i.MX ATF). Quirks are > easily upgrable for other cases. > - Finally I defined power domains into the imx8mm.dtb file. > > I know that this kind of patch is rejected by NXP ut the other way > (callin ATF directly) was also rejected. > > I also know that NXP is concerned abou adding hundred lines of codes for > each new SOC but it' the way it works on Linux. And the "added code" > mainly consist of adding structures, defines and generic methods for > regmap. > > If it's a real problem, maybe we can introduc a new "gpcv3" driver for > i.MX8MM, i.MX8MN and i.MX8MP. > > Thanks, > > Adrien Grassein (7): > soc: imx: gpcv2: check for errors when r/w registers > soc: imx: gpcv2: Fix power up/down sequence > soc: imx: gpcv2: allow domains without power sequence control > dt-bindings: power: fsl,imx-gpcv2: add definitions for i.MX8MM > soc: imx: gpcv2: add HSIOMIX and USB domains for i.MX8MM > soc: imx: gpcv2: add quirks to domains > arm64: dts: imx8mm: add power-domains > > .../bindings/power/fsl,imx-gpcv2.yaml | 7 +- > arch/arm64/boot/dts/freescale/imx8mm.dtsi | 35 ++ > drivers/soc/imx/gpcv2.c | 336 ++ > include/dt-bindings/power/imx8mm-power.h | 21 ++ > 4 files changed, 333 insertions(+), 66 deletions(-) > create mode 100644 include/dt-bindings/power/imx8mm-power.h >
Re: [PATCH v4 1/2] dt-bindings: imx6q-pcie: add one regulator used to power up pcie phy
Am Montag, dem 29.03.2021 um 16:03 +0800 schrieb Richard Zhu: > Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY. > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, > the VREG_BYPASS bits of GPR registers should be cleared from default > value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be > turned on. > > Signed-off-by: Richard Zhu Reviewed-by: Lucas Stach > --- > Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index de4b2baf91e8..d8971ab99274 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -38,6 +38,9 @@ Optional properties: > The regulator will be enabled when initializing the PCIe host and > disabled either as part of the init process or when shutting down the > host. > +- vph-supply: Should specify the regulator in charge of VPH one of the three > + PCIe PHY powers. This regulator can be supplied by both 1.8v and 3.3v > voltage > + supplies. > > > > > Additional required properties for imx6sx-pcie: > - clock names: Must include the following additional entries:
Re: [PATCH v3 3/3] PCI: imx: clear vreg bypass when pcie vph voltage is 3v3
Am Donnerstag, dem 25.03.2021 um 16:44 +0800 schrieb Richard Zhu:
> Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY.
> In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data
> sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design,
> the VREG_BYPASS bits of GPR registers should be cleared from default
> value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be
> turned on.
>
> Signed-off-by: Richard Zhu
Reviewed-by: Lucas Stach
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 20
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 853ea8e82952..d9d534f0840f 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -37,6 +37,7 @@
> #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> +#define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
> #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
> #define IMX8MQ_PCIE2_BASE_ADDR 0x33c0
>
>
>
>
> @@ -80,6 +81,7 @@ struct imx6_pcie {
> u32 tx_swing_full;
> u32 tx_swing_low;
> struct regulator*vpcie;
> + struct regulator*vph;
> void __iomem*phy_base;
>
>
>
>
> /* power domain for pcie */
> @@ -621,6 +623,17 @@ static void imx6_pcie_init_phy(struct imx6_pcie
> *imx6_pcie)
> imx6_pcie_grp_offset(imx6_pcie),
> IMX8MQ_GPR_PCIE_REF_USE_PAD,
> IMX8MQ_GPR_PCIE_REF_USE_PAD);
> + /*
> + * Regarding to the datasheet, the PCIE_VPH is suggested
> + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> + * VREG_BYPASS should be cleared to zero.
> + */
> + if (imx6_pcie->vph &&
> + regulator_get_voltage(imx6_pcie->vph) > 300)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> +imx6_pcie_grp_offset(imx6_pcie),
> +IMX8MQ_GPR_PCIE_VREG_BYPASS,
> +0);
> break;
> case IMX7D:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> @@ -1130,6 +1143,13 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> imx6_pcie->vpcie = NULL;
> }
>
>
>
>
> + imx6_pcie->vph = devm_regulator_get_optional(>dev, "vph");
> + if (IS_ERR(imx6_pcie->vph)) {
> + if (PTR_ERR(imx6_pcie->vph) != -ENODEV)
> + return PTR_ERR(imx6_pcie->vph);
> + imx6_pcie->vph = NULL;
> + }
> +
> platform_set_drvdata(pdev, imx6_pcie);
>
>
>
>
> ret = imx6_pcie_attach_pd(dev);
Re: [PATCH v3 2/3] arm64: dts: imx8mq-evk: add one regulator used to power up pcie phy
Am Donnerstag, dem 25.03.2021 um 16:44 +0800 schrieb Richard Zhu: > Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY. > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, > the VREG_BYPASS bits of GPR registers should be cleared from default > value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be > turned on. > > Signed-off-by: Richard Zhu Reviewed-by: Lucas Stach I guess you need to split this patch out of the series and post it for Shawn to pick up into the imx DT tree, after the other two patches of the series have been accepted into the PCIe tree. Regards, Lucas > --- > arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts > b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts > index 85b045253a0e..4d2035e3dd7c 100644 > --- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts > +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts > @@ -318,6 +318,7 @@ > < IMX8MQ_CLK_PCIE1_PHY>, > <_refclk>; > clock-names = "pcie", "pcie_aux", "pcie_phy", "pcie_bus"; > + vph-supply = <_reg>; > status = "okay"; > }; > > > >
Re: [PATCH v3 1/3] dt-bindings: imx6q-pcie: add one regulator used to power up pcie phy
Am Donnerstag, dem 25.03.2021 um 16:44 +0800 schrieb Richard Zhu: > Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY. > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, > the VREG_BYPASS bits of GPR registers should be cleared from default > value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be > turned on. > > Signed-off-by: Richard Zhu > --- > Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index de4b2baf91e8..e6d1886144ce 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -38,6 +38,9 @@ Optional properties: > The regulator will be enabled when initializing the PCIe host and > disabled either as part of the init process or when shutting down the > host. > +- vph-supply: Should specify the regulator in charge of VPH one of the three > + PCIe PHY powers. This regulator can be supplied by both 1.8v and 3.3v > voltage > + supplies. Might be used to distinguish different HW board designs. Please just get rid of the last sentence. All DT properties are used in one way or the other to distinguish different HW designs, so no need to mention this. Regards, Lucas
Re: [PATCH v2 3/3] PCI: imx: clear vreg bypass when pcie vph voltage is 3v3
Am Mittwoch, dem 24.03.2021 um 13:34 +0800 schrieb Richard Zhu:
> Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY.
> In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data
> sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design,
> the VREG_BYPASS bits of GPR registers should be cleared from default
> value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be
> turned on.
>
> Signed-off-by: Richard Zhu
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 853ea8e82952..beca085a9300 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -37,6 +37,7 @@
> #define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> #define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> +#define IMX8MQ_GPR_PCIE_VREG_BYPASS BIT(12)
> #define IMX8MQ_GPR12_PCIE2_CTRL_DEVICE_TYPE GENMASK(11, 8)
> #define IMX8MQ_PCIE2_BASE_ADDR 0x33c0
>
>
>
>
> @@ -80,6 +81,7 @@ struct imx6_pcie {
> u32 tx_swing_full;
> u32 tx_swing_low;
> struct regulator*vpcie;
> + struct regulator*vph;
> void __iomem*phy_base;
>
>
>
>
> /* power domain for pcie */
> @@ -611,6 +613,8 @@ static void imx6_pcie_configure_type(struct imx6_pcie
> *imx6_pcie)
>
>
>
>
> static void imx6_pcie_init_phy(struct imx6_pcie *imx6_pcie)
> {
> + int phy_uv;
> +
No need for this variable...
> switch (imx6_pcie->drvdata->variant) {
> case IMX8MQ:
> /*
> @@ -621,6 +625,18 @@ static void imx6_pcie_init_phy(struct imx6_pcie
> *imx6_pcie)
> imx6_pcie_grp_offset(imx6_pcie),
> IMX8MQ_GPR_PCIE_REF_USE_PAD,
> IMX8MQ_GPR_PCIE_REF_USE_PAD);
> + /*
> + * Regarding to the datasheet, the PCIE_VPH is suggested
> + * to be 1.8V. If the PCIE_VPH is supplied by 3.3V, the
> + * VREG_BYPASS should be cleared to zero.
> + */
> + if (imx6_pcie->vph)
> + phy_uv = regulator_get_voltage(imx6_pcie->vph);
> + if (phy_uv > 300)
> + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> +imx6_pcie_grp_offset(imx6_pcie),
> +IMX8MQ_GPR_PCIE_VREG_BYPASS,
> +0);
...if you just fold this into a single condition. Right now phy_uv
might be used uninitialized when the vph-supply is not specified in the
DT. Better write this as:
if (imx6_pcie->vph && regulator_get_voltage(imx6_pcie->vph) > 300)
Regards,
Lucas
> break;
> case IMX7D:
> regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> @@ -1130,6 +1146,13 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> imx6_pcie->vpcie = NULL;
> }
>
>
>
>
>
>
>
>
> + imx6_pcie->vph = devm_regulator_get_optional(>dev, "vph");
> + if (IS_ERR(imx6_pcie->vph)) {
> + if (PTR_ERR(imx6_pcie->vph) != -ENODEV)
> + return PTR_ERR(imx6_pcie->vph);
> + imx6_pcie->vph = NULL;
> + }
> +
> platform_set_drvdata(pdev, imx6_pcie);
>
>
>
>
>
>
>
>
> ret = imx6_pcie_attach_pd(dev);
Re: [PATCH v2 1/3] dt-bindings: imx6q-pcie: add one regulator used to power up pcie phy
Hi Richard, Am Mittwoch, dem 24.03.2021 um 13:34 +0800 schrieb Richard Zhu: > Both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe PHY. > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, > the VREG_BYPASS bits of GPR registers should be cleared from default > value 1b'1 to 1b'0. Thus, the internal 3v3 to 1v8 translator would be > turned on. > > Signed-off-by: Richard Zhu > --- > Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index de4b2baf91e8..3248b7192ced 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -38,6 +38,12 @@ Optional properties: > The regulator will be enabled when initializing the PCIe host and > disabled either as part of the init process or when shutting down the > host. > +- vph-supply: Should specify the regulator in charge of PCIe PHY power. > + On i.MX8MQ, both 1.8v and 3.3v power supplies can be used by i.MX8MQ PCIe > + PHY. In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > + sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, the > + VREG_BYPASS bits of GPR registers should be cleared from default value 1b'1 > + to 1b'0. This description of the internal driver behavior does not belong into a DT binding description. Instead the binding should describe the function of the regulator exactly. From the datasheet I can see that there are actually 3 supplies (VPH, VP, VPTX) going into the PCIe PHY, so "regulator in charge of PCIe PHY power" doesn't seem like a very accurate description. Regards, Lucas
Re: Re: [PATCH 1/3] dt-bindings: imx6q-pcie: specify the imx8mq pcie phy voltage
Hi Richard, Am Montag, dem 22.03.2021 um 09:06 + schrieb Richard Zhu: > > -Original Message- > > From: Lucas Stach > > Sent: Friday, March 19, 2021 5:49 PM > > To: Richard Zhu ; andrew.smir...@gmail.com; > > shawn...@kernel.org; k...@linux.com; bhelg...@google.com; > > ste...@agner.ch; lorenzo.pieral...@arm.com > > Cc: linux-...@vger.kernel.org; dl-linux-imx ; > > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; > > ker...@pengutronix.de > > Subject: [EXT] Re: [PATCH 1/3] dt-bindings: imx6q-pcie: specify the > > imx8mq > > pcie phy voltage > > Am Freitag, dem 19.03.2021 um 16:24 +0800 schrieb Richard Zhu: > > > Both 1.8v and 3.3v power supplies can be feeded to i.MX8MQ PCIe > > > PHY. > > > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to > > > data > > > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic > > > design, > > > the VREG_BYPASS bits of GPR registers should be cleared from > > > default > > > value 1b'1 to 1b'0. > > > > > > Signed-off-by: Richard Zhu > > > --- > > > Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 > > > > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q- > > > pcie.txt > > > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > > index de4b2baf91e8..23efbad9e804 100644 > > > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > > > @@ -59,6 +59,10 @@ Additional required properties for imx7d-pcie > > > and > > imx8mq-pcie: > > > Additional required properties for imx8mq-pcie: > > > - clock-names: Must include the following additional entries: > > > - "pcie_aux" > > > +- pcie-vph-3v3: If present then PCIE_VPH is feeded by 3.3v in > > > the HW > > > + schematic design. The PCIE_VPH is suggested to be 1.8v refer > > > to the > > > + data sheet. If the PCIE_VPH is supplied by 3.3V, the > > > VREG_BYPASS > > > + should be cleared to zero accordingly. > > > > Uhm, no. Please don't add boolean DT properties for random parts of > > the > > board design. > > > > If we need to know the voltage of PCIE_VPH, we should really add > > the VPH > > regulator as a supply to the PCIe controller node, then work out > > the voltage > > the usual way by using the Linux regulator API. > > > [Richard Zhu] Hi Lucas: > Thanks for your comments. Since the vgen5_reg is used to power up > PCIe PHY on i.MX8MQ EVK board, > and it's set to be "regulator-always-on;". > Did only the regulator_get_voltage or combined with > regulator_enable/regulator_disable can be used in the driver? The regulator API doesn't care, you can call enable/disable in the driver as normal. If the regulator is marked as always-on it will just stay enabled even if the use-count drops to 0. The other question however is if it's even allowed by the SoC design to disable this supply outside of deep power down. A quick look into the reference manual and datasheet didn't yield any information about this. Regards, Lucas
Re: [PATCH 1/3] dt-bindings: imx6q-pcie: specify the imx8mq pcie phy voltage
Am Freitag, dem 19.03.2021 um 16:24 +0800 schrieb Richard Zhu: > Both 1.8v and 3.3v power supplies can be feeded to i.MX8MQ PCIe PHY. > In default, the PCIE_VPH voltage is suggested to be 1.8v refer to data > sheet. When PCIE_VPH is supplied by 3.3v in the HW schematic design, > the VREG_BYPASS bits of GPR registers should be cleared from default > value 1b'1 to 1b'0. > > Signed-off-by: Richard Zhu > --- > Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt | 4 > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > index de4b2baf91e8..23efbad9e804 100644 > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt > @@ -59,6 +59,10 @@ Additional required properties for imx7d-pcie and > imx8mq-pcie: > Additional required properties for imx8mq-pcie: > - clock-names: Must include the following additional entries: > - "pcie_aux" > +- pcie-vph-3v3: If present then PCIE_VPH is feeded by 3.3v in the HW > + schematic design. The PCIE_VPH is suggested to be 1.8v refer to the > + data sheet. If the PCIE_VPH is supplied by 3.3V, the VREG_BYPASS > + should be cleared to zero accordingly. Uhm, no. Please don't add boolean DT properties for random parts of the board design. If we need to know the voltage of PCIE_VPH, we should really add the VPH regulator as a supply to the PCIe controller node, then work out the voltage the usual way by using the Linux regulator API. Regards, Lucas
Re: [PATCH v1 03/18] arm64: dts: imx8mq-evk: add reserve memory node for CMA region
Am Mittwoch, dem 17.02.2021 um 16:39 -0300 schrieb Ezequiel Garcia:
> Hi Benjamin,
>
> On Wed, 2021-02-17 at 09:02 +0100, Benjamin Gaignard wrote:
> > Define allocation range for the default CMA region.
> >
> > Signed-off-by: Benjamin Gaignard
> > Signed-off-by: Ezequiel Garcia
>
> Despite it seems like I signed-off this one...
>
> > Signed-off-by: Adrian Ratiu
> > ---
> > arch/arm64/boot/dts/freescale/imx8mq-evk.dts | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> > b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> > index 85b045253a0e..047dfd4a1ffd 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq-evk.dts
> > @@ -21,6 +21,21 @@ memory@4000 {
> > reg = <0x 0x4000 0 0xc000>;
> > };
> >
> >
> > + resmem: reserved-memory {
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + /* global autoconfigured region for contiguous allocations
> > */
> > + linux,cma {
> > + compatible = "shared-dma-pool";
> > + reusable;
> > + size = <0 0x3c00>;
> > + alloc-ranges = <0 0x4000 0 0x4000>;
> > + linux,cma-default;
> > + };
>
> ... I'm not a fan of the change :)
>
> Hopefully someone from NXP can provide some insight here?
>
> If it's absolutely needed for the VPU, then I guess it should
> be 1) very well documented and 2) moved to the top-lovel dtsi.
>
> But if we can drop it, that'd be nicer.
What's the justification for this CMA area?
I could only imagine the DMA addressing restrictions on the platform.
DMA masters on the i.MX8MQ can not access memory beyond the 4GB mark
and 1GB of address space is reserved for MMIO, so if you have 4GB
installed the upper 1GB of DRAM is only accessible to the CPU. But this
restriction is already properly communicated to the Linux DMA framework
by the dma-ranges in the top level SoC bus node in the DT, so I don't
think this CMA setup is necessary.
Regards,
Lucas
Re: [PATCH] PCI: imx6: Limit DBI register length for imx6qp pcie
Am Donnerstag, dem 18.02.2021 um 13:09 +0800 schrieb Richard Zhu:
> Refer to commit 075af61c19cd ("PCI: imx6: Limit DBI register length"),
> i.MX6QP PCIe has the similar issue.
> Define the length of the DBI registers and limit config space to its
> length for i.MX6QP PCIe too.
>
> Signed-off-by: Richard Zhu
Reviewed-by: Lucas Stach
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 0cf1333c0440..853ea8e82952 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1175,6 +1175,7 @@ static const struct imx6_pcie_drvdata drvdata[] = {
> .variant = IMX6QP,
> .flags = IMX6_PCIE_FLAG_IMX6_PHY |
> IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE,
> + .dbi_length = 0x200,
> },
> [IMX7D] = {
> .variant = IMX7D,
Re: [BUG REPORT] media: coda: mpeg4 decode corruption on i.MX6qp only
Am Montag, dem 15.02.2021 um 10:54 -0500 schrieb Sven Van Asbroeck: > Hi Lucas, > > On Mon, Feb 15, 2021 at 5:15 AM Lucas Stach wrote: > > > > The straight forward way to fix this would be to just disable the PRE > > when the stride is getting too large, which might not work well with > > all userspace requirements, as it effectively disables the ability to > > scan GPU tiled surfaces when the stride is getting too large. > > Thank you for your very knowledgeable input, really appreciate it. > > I am wondering why I am the first to notice this particular corner > case. Is this perhaps because X+armada plugin allocate a huge bitmap > that fits all displays, and other software frameworks do not? Are > people on i.MX6 mostly using X or Wayland? If Wayland allocates a > separate bitmap for each display, this PRE bug will of course never > show up... Yep, I really doubt that there are a lot i.MX6QP, multi-display, X.Org based devices out there. While it's not anywhere in a protocol or similar fixed API, Wayland compositors mostly opted to have a separate surface per display. The weston reference compositor started out this way (as it makes surface repaint easier) and other followed the lead, so Wayland based stacks won't hit this case. Regards, Lucas
Re: DMA-buf and uncached system memory
Am Montag, dem 15.02.2021 um 13:04 +0100 schrieb Christian König: > Am 15.02.21 um 12:53 schrieb Lucas Stach: > > Am Montag, dem 15.02.2021 um 10:34 +0100 schrieb Christian König: > > > Am 15.02.21 um 10:06 schrieb Simon Ser: > > > > On Monday, February 15th, 2021 at 9:58 AM, Christian König > > > > wrote: > > > > > > > > > we are currently working an Freesync and direct scan out from system > > > > > memory on AMD APUs in A+A laptops. > > > > > > > > > > On problem we stumbled over is that our display hardware needs to scan > > > > > out from uncached system memory and we currently don't have a way to > > > > > communicate that through DMA-buf. > > > > > > > > > > For our specific use case at hand we are going to implement something > > > > > driver specific, but the question is should we have something more > > > > > generic for this? > > > > > > > > > > After all the system memory access pattern is a PCIe extension and as > > > > > such something generic. > > > > Intel also needs uncached system memory if I'm not mistaken? > > > No idea, that's why I'm asking. Could be that this is also interesting > > > for I+A systems. > > > > > > > Where are the buffers allocated? If GBM, then it needs to allocate > > > > memory that > > > > can be scanned out if the USE_SCANOUT flag is set or if a > > > > scanout-capable > > > > modifier is picked. > > > > > > > > If this is about communicating buffer constraints between different > > > > components > > > > of the stack, there were a few proposals about it. The most recent one > > > > is [1]. > > > Well the problem here is on a different level of the stack. > > > > > > See resolution, pitch etc:.. can easily communicated in userspace > > > without involvement of the kernel. The worst thing which can happen is > > > that you draw garbage into your own application window. > > > > > > But if you get the caching attributes in the page tables (both CPU as > > > well as IOMMU, device etc...) wrong then ARM for example has the > > > tendency to just spontaneously reboot > > > > > > X86 is fortunately a bit more gracefully and you only end up with random > > > data corruption, but that is only marginally better. > > > > > > So to sum it up that is not something which we can leave in the hands of > > > userspace. > > > > > > I think that exporters in the DMA-buf framework should have the ability > > > to tell importers if the system memory snooping is necessary or not. > > There is already a coarse-grained way to do so: the dma_coherent > > property in struct device, which you can check at dmabuf attach time. > > > > However it may not be enough for the requirements of a GPU where the > > engines could differ in their dma coherency requirements. For that you > > need to either have fake struct devices for the individual engines or > > come up with a more fine-grained way to communicate those requirements. > > Yeah, that won't work. We need this on a per buffer level. > > > > Userspace components can then of course tell the exporter what the > > > importer needs, but validation if that stuff is correct and doesn't > > > crash the system must happen in the kernel. > > What exactly do you mean by "scanout requires non-coherent memory"? > > Does the scanout requestor always set the no-snoop PCI flag, so you get > > garbage if some writes to memory are still stuck in the caches, or is > > it some other requirement? > > Snooping the CPU caches introduces some extra latency, so what can > happen is that the response to the PCIe read comes to late for the > scanout. The result is an underflow and flickering whenever something is > in the cache which needs to be flushed first. Okay, that confirms my theory on why this is needed. So things don't totally explode if you don't do it, but to in order to guarantee access latency you need to take the no-snoop path, which means your device effectively gets dma-noncoherent. > On the other hand when the don't snoop the CPU caches we at least get > garbage/stale data on the screen. That wouldn't be that worse, but the > big problem is that we have also seen machine check exceptions when > don't snoop and the cache is dirty. If you attach to the dma-buf with a struct device which is non-coherent it's the exporters job
Re: DMA-buf and uncached system memory
Am Montag, dem 15.02.2021 um 10:34 +0100 schrieb Christian König: > > Am 15.02.21 um 10:06 schrieb Simon Ser: > > On Monday, February 15th, 2021 at 9:58 AM, Christian König > > wrote: > > > > > we are currently working an Freesync and direct scan out from system > > > memory on AMD APUs in A+A laptops. > > > > > > On problem we stumbled over is that our display hardware needs to scan > > > out from uncached system memory and we currently don't have a way to > > > communicate that through DMA-buf. > > > > > > For our specific use case at hand we are going to implement something > > > driver specific, but the question is should we have something more > > > generic for this? > > > > > > After all the system memory access pattern is a PCIe extension and as > > > such something generic. > > Intel also needs uncached system memory if I'm not mistaken? > > No idea, that's why I'm asking. Could be that this is also interesting > for I+A systems. > > > Where are the buffers allocated? If GBM, then it needs to allocate memory > > that > > can be scanned out if the USE_SCANOUT flag is set or if a scanout-capable > > modifier is picked. > > > > If this is about communicating buffer constraints between different > > components > > of the stack, there were a few proposals about it. The most recent one is > > [1]. > > Well the problem here is on a different level of the stack. > > See resolution, pitch etc:.. can easily communicated in userspace > without involvement of the kernel. The worst thing which can happen is > that you draw garbage into your own application window. > > But if you get the caching attributes in the page tables (both CPU as > well as IOMMU, device etc...) wrong then ARM for example has the > tendency to just spontaneously reboot > > X86 is fortunately a bit more gracefully and you only end up with random > data corruption, but that is only marginally better. > > So to sum it up that is not something which we can leave in the hands of > userspace. > > I think that exporters in the DMA-buf framework should have the ability > to tell importers if the system memory snooping is necessary or not. There is already a coarse-grained way to do so: the dma_coherent property in struct device, which you can check at dmabuf attach time. However it may not be enough for the requirements of a GPU where the engines could differ in their dma coherency requirements. For that you need to either have fake struct devices for the individual engines or come up with a more fine-grained way to communicate those requirements. > Userspace components can then of course tell the exporter what the > importer needs, but validation if that stuff is correct and doesn't > crash the system must happen in the kernel. What exactly do you mean by "scanout requires non-coherent memory"? Does the scanout requestor always set the no-snoop PCI flag, so you get garbage if some writes to memory are still stuck in the caches, or is it some other requirement? Regards, Lucas
Re: [PATCH] kernel: Expose SYS_kcmp by default
Am Samstag, dem 13.02.2021 um 18:40 +0100 schrieb Pavel Machek: > Hi! > > > Userspace has discovered the functionality offered by SYS_kcmp and has > > started to depend upon it. In particular, Mesa uses SYS_kcmp for > > os_same_file_description() in order to identify when two fd (e.g. device > > or dmabuf) point to the same struct file. Since they depend on it for > > core functionality, lift SYS_kcmp out of the non-default > > CONFIG_CHECKPOINT_RESTORE into the selectable syscall category. > > Is it good idea to enable everything because Mesa uses it for file > descriptors? > > This is really interesting syscall... As Debian/Ubuntu and Fedora are already shipping with CONFIG_CHECKPOINT_RESTORE=y in their kernel configs, I don't really see the need to add further restrictions here. Or this discussion should have happened a while ago... Regards, Lucas
Re: [BUG REPORT] media: coda: mpeg4 decode corruption on i.MX6qp only
Hi Sven, Am Freitag, dem 12.02.2021 um 18:52 -0500 schrieb Sven Van Asbroeck: > Philipp, Fabio, > > I was able to verify that the PREs do indeed overrun their allocated ocram > area. > > Section 38.5.1 of the iMX6QuadPlus manual indicates the ocram size > required: width(pixels) x 8 lines x 4 bytes. For 2048 pixels max, this > comes to 64K. This is what the PRE driver allocates. So far, so good. > > The trouble starts when we're displaying a section of a much wider > bitmap. This happens in X when using two displays. e.g.: > HDMI 1920x1088 > LVDS 1280x800 > X bitmap 3200x1088, left side displayed on HDMI, right side on LVDS. > > In such a case, the stride will be much larger than the width of a > display scanline. Urgh, bad tested corner case. > This is where things start to go very wrong. > > I found that the ocram area used by the PREs increases with the > stride. I experimentally found a formula: > ocam_used = display_widthx8x4 + (bitmap_width-display_width)x7x4 > > As the stride increases, the PRE eventually overruns the ocram and... > ends up in the "ocram aliased" area, where it overwrites the ocram in > use by the vpu/coda ! > > I could not find any PRE register setting that changes the used ocram area. There is no such setting. The PRE always prefetches a doublebuffer of 2x4 scanlines and the scanline size is defined by the store engine pitch. The straight forward way to fix this would be to just disable the PRE when the stride is getting too large, which might not work well with all userspace requirements, as it effectively disables the ability to scan GPU tiled surfaces when the stride is getting too large. I'm not sure if this works in practice, as the PRG address rewriting might make this harder than it seems, but on could probably try to rewrite the prefetch start address, input pitch, input width/height and store pitch of the PRE settings to cover only the area used by the the CRTC to reduce OCRAM requirements. Regards, Lucas
Re: [PATCH] kernel: Expose SYS_kcmp by default
Am Freitag, dem 05.02.2021 um 16:37 + schrieb Chris Wilson: > Userspace has discovered the functionality offered by SYS_kcmp and has > started to depend upon it. In particular, Mesa uses SYS_kcmp for > os_same_file_description() in order to identify when two fd (e.g. device > or dmabuf) point to the same struct file. Since they depend on it for > core functionality, lift SYS_kcmp out of the non-default > CONFIG_CHECKPOINT_RESTORE into the selectable syscall category. > > Signed-off-by: Chris Wilson > Cc: Kees Cook > Cc: Andy Lutomirski > Cc: Will Drewry > Cc: Andrew Morton > Cc: Dave Airlie > Cc: Daniel Vetter > Cc: Lucas Stach > --- > init/Kconfig | 11 +++ > kernel/Makefile | 2 +- > tools/testing/selftests/seccomp/seccomp_bpf.c | 2 +- > 3 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..f62fca13ac5b 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -1194,6 +1194,7 @@ endif # NAMESPACES > config CHECKPOINT_RESTORE > bool "Checkpoint/restore support" > select PROC_CHILDREN > + select KCMP > default n > help > Enables additional kernel features in a sake of checkpoint/restore. > @@ -1737,6 +1738,16 @@ config ARCH_HAS_MEMBARRIER_CALLBACKS > config ARCH_HAS_MEMBARRIER_SYNC_CORE > bool > > > > > +config KCMP > + bool "Enable kcmp() system call" if EXPERT > + default y > + help > + Enable the file descriptor comparison system call. It provides > + user-space with the ability to compare two fd to see if they > + point to the same file, and check other attributes. This description undersells the abilities of kcmp, while fd compare is the only thing used by the graphics stack, kcmp can compare a handful of other system resources, see man 2 kcmp. I think the helptext should at least try to cover this fact somewhat. Regards, Lucas > + > + If unsure, say Y. > + > config RSEQ > bool "Enable rseq() system call" if EXPERT > default y > diff --git a/kernel/Makefile b/kernel/Makefile > index aa7368c7eabf..320f1f3941b7 100644 > --- a/kernel/Makefile > +++ b/kernel/Makefile > @@ -51,7 +51,7 @@ obj-y += livepatch/ > obj-y += dma/ > obj-y += entry/ > > > > > -obj-$(CONFIG_CHECKPOINT_RESTORE) += kcmp.o > +obj-$(CONFIG_KCMP) += kcmp.o > obj-$(CONFIG_FREEZER) += freezer.o > obj-$(CONFIG_PROFILING) += profile.o > obj-$(CONFIG_STACKTRACE) += stacktrace.o > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c > b/tools/testing/selftests/seccomp/seccomp_bpf.c > index 26c72f2b61b1..1b6c7d33c4ff 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -315,7 +315,7 @@ TEST(kcmp) > ret = __filecmp(getpid(), getpid(), 1, 1); > EXPECT_EQ(ret, 0); > if (ret != 0 && errno == ENOSYS) > - SKIP(return, "Kernel does not support kcmp() (missing > CONFIG_CHECKPOINT_RESTORE?)"); > + SKIP(return, "Kernel does not support kcmp() (missing > CONFIG_KCMP?)"); > } > > > > > TEST(mode_strict_support)
Re: [PATCH] drm/etnaviv: provide more ID values via GET_PARAM ioctl.
Am Mittwoch, dem 16.12.2020 um 12:42 +0100 schrieb Christian Gmeiner:
> Make it possible for the user space to access these ID values.
Thanks, I've added this patch to my etnaviv/next branch.
Regards,
Lucas
> Signed-off-by: Christian Gmeiner
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 12
> include/uapi/drm/etnaviv_drm.h| 3 +++
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index c6404b8d067f..ec16991ba8b6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -156,6 +156,18 @@ int etnaviv_gpu_get_param(struct etnaviv_gpu *gpu, u32
> param, u64 *value)
> *value = ~0ULL;
> break;
>
>
>
>
> + case ETNAVIV_PARAM_GPU_PRODUCT_ID:
> + *value = gpu->identity.product_id;
> + break;
> +
> + case ETNAVIV_PARAM_GPU_CUSTOMER_ID:
> + *value = gpu->identity.customer_id;
> + break;
> +
> + case ETNAVIV_PARAM_GPU_ECO_ID:
> + *value = gpu->identity.eco_id;
> + break;
> +
> default:
> DBG("%s: invalid param: %u", dev_name(gpu->dev), param);
> return -EINVAL;
> diff --git a/include/uapi/drm/etnaviv_drm.h b/include/uapi/drm/etnaviv_drm.h
> index 09d0df8b71c5..af024d90453d 100644
> --- a/include/uapi/drm/etnaviv_drm.h
> +++ b/include/uapi/drm/etnaviv_drm.h
> @@ -74,6 +74,9 @@ struct drm_etnaviv_timespec {
> #define ETNAVIV_PARAM_GPU_NUM_CONSTANTS 0x19
> #define ETNAVIV_PARAM_GPU_NUM_VARYINGS 0x1a
> #define ETNAVIV_PARAM_SOFTPIN_START_ADDR0x1b
> +#define ETNAVIV_PARAM_GPU_PRODUCT_ID0x1c
> +#define ETNAVIV_PARAM_GPU_CUSTOMER_ID 0x1d
> +#define ETNAVIV_PARAM_GPU_ECO_ID0x1e
>
>
>
>
> #define ETNA_MAX_PIPES 4
>
>
>
>
Re: [PATCH v2 1/1] phy: fsl-imx8-mipi-dphy: Hook into runtime pm
Hi Guido,
this time hopefully with less broken quoting. My mailer is driving me
mad right now...
Am Mittwoch, dem 16.12.2020 um 12:27 +0100 schrieb Guido Günther:
> This allows us to shut down the mipi power domain on the imx8. The
> alternative would be to drop the dphy from the mipi power domain in the
> SOCs device tree and only have the DSI host controller visible there but
> since the PD is mostly about the PHY that would defeat it's purpose.
Adding RPM support is exactly the right course of action.
> This allows to shut off the power domain hen blanking the LCD panel:
>
> pm_genpd_summary before:
>
> domain status slaves
> /device runtime status
> --
> mipion
> /devices/platform/soc@0/soc@0:bus@3080/30a00300.dphy unsupported
> /devices/platform/soc@0/soc@0:bus@3080/30a0.mipi_dsi suspended
>
> after:
>
> mipioff-0
> /devices/platform/soc@0/soc@0:bus@3080/30a00300.dphy suspended
> /devices/platform/soc@0/soc@0:bus@3080/30a0.mipi_dsi suspended
>
> Signed-off-by: Guido Günther
> ---
> .../phy/freescale/phy-fsl-imx8-mipi-dphy.c| 22 ++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> index a95572b397ca..34e2d801e520 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
> @@ -14,6 +14,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> /* DPHY registers */
> @@ -93,6 +94,7 @@ struct mixel_dphy_cfg {
> };
>
> struct mixel_dphy_priv {
> + struct device *dev;
> struct mixel_dphy_cfg cfg;
> struct regmap *regmap;
> struct clk *phy_ref_clk;
> @@ -382,6 +384,7 @@ static int mixel_dphy_power_on(struct phy *phy)
> ret = clk_prepare_enable(priv->phy_ref_clk);
> if (ret < 0)
> return ret;
> + pm_runtime_get_sync(priv->dev);
>
This call can fail and will leave you with an elevated rpm refcount.
Better use the new pm_runtime_resume_and_get to avoid this issue?
Nitpick: I would add a blank line before the call.
Regards,
Lucas
Re: [PATCH v2 1/1] phy: fsl-imx8-mipi-dphy: Hook into runtime pm
Hi Guido,
Am Mittwoch, dem 16.12.2020 um 12:27 +0100 schrieb Guido Günther:
This allows us to shut down the mipi power domain on the imx8. The
alternative would be to drop the dphy from the mipi power domain in the
SOCs device tree and only have the DSI host controller visible there but
since the PD is mostly about the PHY that would defeat it's purpose.
Adding RPM support is exactly the right course of action.
This allows to shut off the power domain hen blanking the LCD panel:
pm_genpd_summary before:
domain status slaves
/device runtime status
--
mipion
/devices/platform/soc@0/soc@0:bus@3080/30a00300.dphy unsupported
/devices/platform/soc@0/soc@0:bus@3080/30a0.mipi_dsi suspended
after:
mipioff-0
/devices/platform/soc@0/soc@0:bus@3080/30a00300.dphy suspended
/devices/platform/soc@0/soc@0:bus@3080/30a0.mipi_dsi suspended
Signed-off-by: Guido Günther
---
.../phy/freescale/phy-fsl-imx8-mipi-dphy.c| 22 ++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
index a95572b397ca..34e2d801e520 100644
--- a/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
+++ b/drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
@@ -14,6 +14,7 @@
#include
#include
#include
+#include
#include
/* DPHY registers */
@@ -93,6 +94,7 @@ struct mixel_dphy_cfg {
};
struct mixel_dphy_priv {
+ struct device *dev;
struct mixel_dphy_cfg cfg;
struct regmap *regmap;
struct clk *phy_ref_clk;
@@ -382,6 +384,7 @@ static int mixel_dphy_power_on(struct phy *phy)
ret = clk_prepare_enable(priv->phy_ref_clk);
if (ret < 0)
return ret;
+ pm_runtime_get_sync(priv->dev);
This call can fail and will leave you with an elevated rpm refcount.
Better use the new pm_runtime_resume_and_get to avoid this issue?
Nitpick: I would add a blank line before the call.
Regards,
Lucas
phy_write(phy, PWR_ON, DPHY_PD_PLL);
ret = regmap_read_poll_timeout(priv->regmap, DPHY_LOCK, locked,
@@ -395,6 +398,7 @@ static int mixel_dphy_power_on(struct phy *phy)
return 0;
clock_disable:
+ pm_runtime_put(priv->dev);
clk_disable_unprepare(priv->phy_ref_clk);
return ret;
}
@@ -406,6 +410,7 @@ static int mixel_dphy_power_off(struct phy *phy)
phy_write(phy, PWR_OFF, DPHY_PD_PLL);
phy_write(phy, PWR_OFF, DPHY_PD_DPHY);
+ pm_runtime_put(priv->dev);
clk_disable_unprepare(priv->phy_ref_clk);
return 0;
@@ -467,6 +472,7 @@ static int mixel_dphy_probe(struct platform_device *pdev)
dev_dbg(dev, "phy_ref clock rate: %lu\n",
clk_get_rate(priv->phy_ref_clk));
+ priv->dev = dev;
dev_set_drvdata(dev, priv);
phy = devm_phy_create(dev, np, _dphy_phy_ops);
@@ -477,12 +483,26 @@ static int mixel_dphy_probe(struct platform_device *pdev)
phy_set_drvdata(phy, priv);
phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+ if (IS_ERR(phy_provider))
+ return PTR_ERR(phy_provider);
- return PTR_ERR_OR_ZERO(phy_provider);
+ pm_runtime_enable(dev);
+
+ return 0;
+}
+
+static int mixel_dphy_remove(struct platform_device *pdev)
+{
+ struct mixel_dphy_priv *priv = platform_get_drvdata(pdev);
+
+ pm_runtime_disable(priv->dev);
+
+ return 0;
}
static struct platform_driver mixel_dphy_driver = {
.probe = mixel_dphy_probe,
+ .remove = mixel_dphy_remove,
.driver = {
.name = "mixel-mipi-dphy",
.of_match_table = mixel_dphy_of_match,
Re: tracer_init_tracefs really slow
Hi Steven,
Am Montag, den 07.12.2020, 14:47 -0500 schrieb Steven Rostedt:
On Mon, 07 Dec 2020 17:24:58 +0100
Lucas Stach wrote:
> I would be happy to test a patch on our whimpy machines, though. :)
Let me know if this helps:
-- Steve
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 06134189e9a7..48aa5b6a7d15 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9062,7 +9062,10 @@ int tracing_init_dentry(void)
extern struct trace_eval_map *__start_ftrace_eval_maps[];
extern struct trace_eval_map *__stop_ftrace_eval_maps[];
-static void __init trace_eval_init(void)
+static struct workqueue_struct *eval_map_wq __initdata;
+struct work_struct eval_map_work __initdata;
+
+static void __init eval_map_work_func(struct work_struct *work)
{
int len;
@@ -9070,6 +9073,34 @@ static void __init trace_eval_init(void)
trace_insert_eval_map(NULL, __start_ftrace_eval_maps, len);
}
+static int __init trace_eval_init(void)
+{
+ INIT_WORK(_map_work, eval_map_work_func);
+
+ eval_map_wq = alloc_workqueue("eval_map_wq", WQ_UNBOUND, 0);
+ if (!eval_map_wq) {
+ pr_err("Unable to allocate eval_map_wq\n");
+ /* Do work here */
+ eval_map_work_func(_map_work);
+ return -ENOMEM;
+ }
+
+ queue_work(eval_map_wq, _map_work);
+ return 0;
+}
+
+static int __init trace_eval_sync(void)
+{
+ /* Make sure the eval map updates are finished */
+ if (eval_map_wq)
+ destroy_workqueue(eval_map_wq);
+ return 0;
+}
+
+subsys_initcall(trace_eval_init);
You don't want to do this...
+late_initcall_sync(trace_eval_sync);
+
+
#ifdef CONFIG_MODULES
static void trace_module_add_evals(struct module *mod)
{
@@ -9171,8 +9202,6 @@ static __init int tracer_init_tracefs(void)
trace_create_file("saved_tgids", 0444, NULL,
NULL, _saved_tgids_fops);
- trace_eval_init();
-
... and this. Moving the trace_eval_init into its own initcall means it
gets started before tracer_init_tracefs. As it holds the
trace_event_sem while updating the eval maps, event_trace_init() then
blocks further initcall execution when it tries to grab this semaphore
a bit down the line, killing the parallelism we are trying to unlock
here.
With those 2 lines dropped, the change seems to work as intended and
shaves ~830ms from the kernel boot time on this system.
Regards,
Lucas
trace_create_eval_file(NULL);
#ifdef CONFIG_MODULES
Re: tracer_init_tracefs really slow
Hi Steven, Am Donnerstag, den 03.12.2020, 21:18 -0500 schrieb Steven Rostedt: > Sorry for the really late reply, but I received this while I was on > vacation, and my backlog was so big when I got back that I left most of > it unread. :-/ And to make matters worse, my out-of-office script > wasn't working, to let people know I was on vacation. No problem, I already figured that this might have fallen through the cracks. It's also not really a high prio issue for us. > On Mon, 07 Sep 2020 18:16:52 +0200 > Lucas Stach wrote: > > > Hi all, > > > > one of my colleagues has taken a look at device boot times and stumbled > > across a pretty big amount of kernel boot time being spent in > > tracer_init_tracefs(). On this particular i.MX6Q based device the > > kernel spends more than 1 second in this function, which is a > > significant amount of the overall kernel inititalization time. While > > this machine is no rocket with its Cortex A9 @ 800MHz, the amount of > > CPU time being used there is pretty irritating. > > > > Specifically the issue lies within trace_event_eval_update where ~1100 > > trace_event_calls get updated with ~500 trace_eval_maps. I haven't had > > a chance yet to dig any deeper or try to understand more of what's > > going on there, but I wanted to get the issue out there in case anyone > > has some cycles to spare to help us along. > > OK, that makes sense. The macro TRACE_DEFINE_ENUM() will make a mapping > of enums into their values. This is needed because if an enum is used > in tp_printk() of a TRACE_EVENT(), the name of the ENUM is passed to > user space. The enum name is useless to user space, so this function > will scan the strings that are exported to user space and convert the > enum name to the enum values. > > > > > The obvious questions for now are: > > 1. Why is this function so damn expensive (at least on this whimpy ARM > > machine)? and > > Well, it's doing a string substitution for thousands of events. > > > > 2. Could any of this be done asynchronously, to not block the kernel in > > early init? > > Yes :-) > > We could make a thread that does this, that the init wakes up and runs, > letting the kernel to move forward. Would you like to make that patch > or shall I? I guess you are much more likely to come up with a correct patch, as I'm not really clear yet on when we would need to synchronize this thread, to make sure things are available before they get used by something. I likely won't have time in the near future to read enough code in this particular spot of the kernel. I would be happy to test a patch on our whimpy machines, though. :) Regards, Lucas
Re: [PATCH v1 1/1] drm/imx/dcss: Add interconnect support
Am Dienstag, den 01.12.2020, 12:34 +0100 schrieb Guido Günther:
> This allows us to raise DRAM bandiwth to a high enough value for a
> stable picture on i.mx8mq. We pick a bandwidth that should be sufficient
> for 4k@60Hz.
>
> Modelled like mdp5_kms.
>
> Signed-off-by: Guido Günther
> ---
> drivers/gpu/drm/imx/dcss/dcss-dev.c | 47 +++--
> drivers/gpu/drm/imx/dcss/dcss-dev.h | 3 ++
> 2 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.c
> b/drivers/gpu/drm/imx/dcss/dcss-dev.c
> index c849533ca83e..e336f03448d6 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-dev.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-dev.c
> @@ -15,6 +15,9 @@
> #include "dcss-dev.h"
> #include "dcss-kms.h"
>
> +/* sufficient for 4K at 60 Hz */
> +#define DCSS_BW_MAX GBps_to_icc(2)
> +
Same comment as for the similar change in mxsfb: this should not be a
static value, but should be scaled proportional to the current mode and
plane settings. With a single static value you keep the clocks way too
high for most use-cases with smaller displays, but fail to account for
the additional bandwidth requirement when more than one plane is
active.
Regards,
Lucas
> static void dcss_clocks_enable(struct dcss_dev *dcss)
> {
> clk_prepare_enable(dcss->axi_clk);
> @@ -162,6 +165,31 @@ static void dcss_clks_release(struct dcss_dev *dcss)
> devm_clk_put(dcss->dev, dcss->apb_clk);
> }
>
>
>
>
> +static int dcss_init_icc(struct dcss_dev *dcss)
> +{
> + int ret;
> + struct icc_path *icc_path;
> +
> + /* Optional interconnect request */
> + icc_path = of_icc_get(dcss->dev, NULL);
> + if (IS_ERR(icc_path)) {
> + ret = PTR_ERR(icc_path);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + /* no interconnect support is not necessarily a fatal
> + * condition, the platform may simply not have an
> + * interconnect driver yet. But warn about it in case
> + * bootloader didn't setup bus clocks high enough for
> + * scanout.
> + */
> + dev_warn(dcss->dev, "No interconnect support may cause display
> underflows!\n");
> + return 0;
> + }
> + dcss->icc_path = icc_path;
> + dcss->icc_peak_bw = DCSS_BW_MAX;
> + return 0;
> +}
> +
> struct dcss_dev *dcss_dev_create(struct device *dev, bool hdmi_output)
> {
> struct platform_device *pdev = to_platform_device(dev);
> @@ -190,10 +218,14 @@ struct dcss_dev *dcss_dev_create(struct device *dev,
> bool hdmi_output)
> dcss->devtype = devtype;
> dcss->hdmi_output = hdmi_output;
>
>
>
>
> + ret = dcss_init_icc(dcss);
> + if (ret < 0)
> + goto err;
> +
> ret = dcss_clks_init(dcss);
> if (ret) {
> dev_err(dev, "clocks initialization failed\n");
> - goto err;
> + goto icc_err;
> }
>
>
>
>
> dcss->of_port = of_graph_get_port_by_id(dev->of_node, 0);
> @@ -223,7 +255,8 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool
> hdmi_output)
>
>
>
>
> clks_err:
> dcss_clks_release(dcss);
> -
> +icc_err:
> + icc_put(dcss->icc_path);
> err:
> kfree(dcss);
>
>
>
>
> @@ -243,6 +276,8 @@ void dcss_dev_destroy(struct dcss_dev *dcss)
>
>
>
>
> dcss_clks_release(dcss);
>
>
>
>
> + icc_put(dcss->icc_path);
> +
> kfree(dcss);
> }
>
>
>
>
> @@ -267,6 +302,8 @@ int dcss_dev_suspend(struct device *dev)
>
>
>
>
> dcss_clocks_disable(dcss);
>
>
>
>
> + icc_set_bw(dcss->icc_path, 0, 0);
> +
> return 0;
> }
>
>
>
>
> @@ -281,6 +318,8 @@ int dcss_dev_resume(struct device *dev)
> return 0;
> }
>
>
>
>
> + icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
> +
> dcss_clocks_enable(dcss);
>
>
>
>
> dcss_blkctl_cfg(dcss->blkctl);
> @@ -307,6 +346,8 @@ int dcss_dev_runtime_suspend(struct device *dev)
>
>
>
>
> dcss_clocks_disable(dcss);
>
>
>
>
> + icc_set_bw(dcss->icc_path, 0, 0);
> +
> return 0;
> }
>
>
>
>
> @@ -314,6 +355,8 @@ int dcss_dev_runtime_resume(struct device *dev)
> {
> struct dcss_dev *dcss = dcss_drv_dev_to_dcss(dev);
>
>
>
>
> + icc_set_bw(dcss->icc_path, 0, dcss->icc_peak_bw);
> +
> dcss_clocks_enable(dcss);
>
>
>
>
> dcss_blkctl_cfg(dcss->blkctl);
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.h
> b/drivers/gpu/drm/imx/dcss/dcss-dev.h
> index c642ae17837f..1b35a6f0d0d4 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-dev.h
> +++ b/drivers/gpu/drm/imx/dcss/dcss-dev.h
> @@ -8,6 +8,7 @@
>
>
>
>
> #include
> #include
> +#include
> #include
>
>
>
>
> #define SET 0x04
> @@ -85,6 +86,8 @@ struct dcss_dev {
> struct clk *pll_phy_ref_clk;
>
>
>
>
> bool hdmi_output;
> + struct icc_path *icc_path;
> +
Re: [PATCH 5/6] arm64: defconfig: Enable interconnect for imx8mq
Am Dienstag, den 01.12.2020, 11:01 +0100 schrieb Martin Kepplinger: > Enable INTERCONNECT_IMX8MQ in order to make interconnect more widely > available for testing. > > Signed-off-by: Martin Kepplinger > --- > arch/arm64/configs/defconfig | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig > index 867cc4a5f00f..57cf2f50b5f7 100644 > --- a/arch/arm64/configs/defconfig > +++ b/arch/arm64/configs/defconfig > @@ -1051,10 +1051,11 @@ CONFIG_OF_FPGA_REGION=m > CONFIG_TEE=y > CONFIG_OPTEE=y > CONFIG_SLIMBUS=m > +CONFIG_MUX_MMIO=y This change should be part of the other patch to regenerate the config without functional changes. Also order of the patches should be the other way around: first regenerate without changes, then stack the INTERCONNECT_IMX8MQ enable on top. Regards, Lucas > CONFIG_SLIM_QCOM_CTRL=m > CONFIG_SLIM_QCOM_NGD_CTRL=m > -CONFIG_MUX_MMIO=y > -CONFIG_INTERCONNECT=y > +CONFIG_INTERCONNECT_IMX=m > +CONFIG_INTERCONNECT_IMX8MQ=m > CONFIG_INTERCONNECT_QCOM=y > CONFIG_INTERCONNECT_QCOM_MSM8916=m > CONFIG_INTERCONNECT_QCOM_OSM_L3=m
Re: [PATCH 3/6] arm64: dts: imx8mq: Add interconnect for lcdif
Am Dienstag, den 01.12.2020, 11:01 +0100 schrieb Martin Kepplinger:
> Add interconnect ports for lcdif to set bus capabilities.
>
> Signed-off-by: Martin Kepplinger
> ---
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 244e28e54b35..7384a69ed36c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -11,6 +11,7 @@
> #include "dt-bindings/input/input.h"
> #include
> #include
> +#include
> #include "imx8mq-pinfunc.h"
>
>
>
>
> / {
> @@ -522,6 +523,8 @@
> < IMX8MQ_VIDEO_PLL1>,
> < IMX8MQ_VIDEO_PLL1_OUT>;
> assigned-clock-rates = <0>, <0>, <0>,
> <59400>;
> + interconnects = < IMX8MQ_ICM_LCDIF
> IMX8MQ_ICS_DRAM>;
> + interconnect-names = "lcdif-dram";
This needs to be documented in the binding. Also I would argue that you
can drop the lcdif prefix in the name, as it's part of the lcdif DT
node.
Regards,
Lucas
> status = "disabled";
>
>
>
>
> port@0 {
Re: [PATCH] drm: mxsfb: Add interconnect path request
Am Dienstag, den 01.12.2020, 11:37 +0100 schrieb Martin Kepplinger:
> Add interconnect support to mxsfb so that it is able to request enough
> bandwidth to DDR for display output to work.
>
> Signed-off-by: Martin Kepplinger
> ---
> drivers/gpu/drm/mxsfb/mxsfb_drv.c | 33 +++
> drivers/gpu/drm/mxsfb/mxsfb_drv.h | 2 ++
> drivers/gpu/drm/mxsfb/mxsfb_kms.c | 13
> 3 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> index 6faf17b6408d..b05e8e5f1e38 100644
> --- a/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> +++ b/drivers/gpu/drm/mxsfb/mxsfb_drv.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
>
>
>
> #include
> #include
> @@ -311,6 +312,34 @@ static const struct of_device_id mxsfb_dt_ids[] = {
> };
> MODULE_DEVICE_TABLE(of, mxsfb_dt_ids);
>
> +
> +static int mxsfb_init_icc(struct platform_device *pdev)
> +{
> + struct drm_device *drm = platform_get_drvdata(pdev);
> + struct mxsfb_drm_private *mxsfb = drm->dev_private;
> + int ret;
> +
> + /* Optional interconnect request */
> + mxsfb->icc_path = devm_of_icc_get(>dev, "lcdif-dram");
> + if (IS_ERR(mxsfb->icc_path)) {
> + ret = PTR_ERR(mxsfb->icc_path);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> +
> + mxsfb->icc_path = NULL;
> + dev_dbg(drm->dev,
> + "No interconnect may cause display underflows!\n");
> + }
> +
> + ret = icc_set_bw(mxsfb->icc_path, 0, MBps_to_icc(700));
Shouldn't this be proportional to the current mode, instead of a fixed
value? 700MB looks like 1080p@60Hz@32bpp with a bit of headroom, but
there are many valid use-cases where significantly smaller displays are
connected to the eLCDIF. Also it doesn't cover the case where an
overlay is active, which needs additional bandwidth.
> + if (ret) {
> + dev_err(drm->dev, "%s: icc_set_bw failed: %d\n", __func__, ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
[...]
> #include
> #include
> @@ -310,6 +311,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc
> *crtc,
> struct mxsfb_drm_private *mxsfb = to_mxsfb_drm_private(crtc->dev);
> struct drm_device *drm = mxsfb->drm;
> dma_addr_t paddr;
> + int ret;
> +
> + ret = icc_enable(mxsfb->icc_path);
> + if (ret)
> + dev_err_ratelimited(drm->dev, "%s: icc_enable failed: %d\n",
> + __func__, ret);
Why ratelimited? I wouldn't expect atomic enable/disable to be called
often enough for this to make any difference.
Regards,
Lucas
>
> pm_runtime_get_sync(drm->dev);
> mxsfb_enable_axi_clk(mxsfb);
Re: [PATCH] drm/imx/dcss: allow using nearest neighbor interpolation scaling
On Do, 2020-11-05 at 16:50 +0200, Laurentiu Palcu wrote:
> This patch adds support for using NN interpolation scaling by setting the
> SCALING_FILTER plane property to 1. Otherwise, the default method is used.
>
> Signed-off-by: Laurentiu Palcu
Reviewed and pushed into drm-misc-next.
Regards,
Lucas
> ---
> I had no retro pixel art games to test this with, so I used modetest to see
> the
> results:
>
> To test, I used a 240x135 buffer, upscaled 8 times to 1920x1080:
> * default scaling method using gaussian filter:
> /usr/bin/modetest -M imx-dcss -w 33:SCALING_FILTER:0 -P 33@38:240x135*8@XR24
> * NN interpolation method:
> /usr/bin/modetest -M imx-dcss -w 33:SCALING_FILTER:1 -P 33@38:240x135*8@XR24
>
> Thanks,
> laurentiu
>
> drivers/gpu/drm/imx/dcss/dcss-dev.h| 3 ++
> drivers/gpu/drm/imx/dcss/dcss-plane.c | 10 +-
> drivers/gpu/drm/imx/dcss/dcss-scaler.c | 47 +-
> 3 files changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.h
> b/drivers/gpu/drm/imx/dcss/dcss-dev.h
> index c642ae17837f..1e582270c6ea 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-dev.h
> +++ b/drivers/gpu/drm/imx/dcss/dcss-dev.h
> @@ -7,6 +7,7 @@
> #define __DCSS_PRV_H__
>
> #include
> +#include
> #include
> #include
>
> @@ -165,6 +166,8 @@ void dcss_ss_sync_set(struct dcss_ss *ss, struct
> videomode *vm,
> /* SCALER */
> int dcss_scaler_init(struct dcss_dev *dcss, unsigned long scaler_base);
> void dcss_scaler_exit(struct dcss_scaler *scl);
> +void dcss_scaler_set_filter(struct dcss_scaler *scl, int ch_num,
> + enum drm_scaling_filter scaling_filter);
> void dcss_scaler_setup(struct dcss_scaler *scl, int ch_num,
> const struct drm_format_info *format,
> int src_xres, int src_yres, int dst_xres, int dst_yres,
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-plane.c
> b/drivers/gpu/drm/imx/dcss/dcss-plane.c
> index 5db093aada2f..03ba88f7f995 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-plane.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-plane.c
> @@ -257,7 +257,8 @@ static bool dcss_plane_needs_setup(struct drm_plane_state
> *state,
> state->src_h != old_state->src_h ||
> fb->format->format != old_fb->format->format ||
> fb->modifier != old_fb->modifier ||
> -state->rotation != old_state->rotation;
> +state->rotation != old_state->rotation ||
> +state->scaling_filter != old_state->scaling_filter;
> }
>
> static void dcss_plane_atomic_update(struct drm_plane *plane,
> @@ -313,6 +314,9 @@ static void dcss_plane_atomic_update(struct drm_plane
> *plane,
> is_rotation_90_or_270 = state->rotation & (DRM_MODE_ROTATE_90 |
> DRM_MODE_ROTATE_270);
>
> + dcss_scaler_set_filter(dcss->scaler, dcss_plane->ch_num,
> +state->scaling_filter);
> +
> dcss_scaler_setup(dcss->scaler, dcss_plane->ch_num,
> state->fb->format,
> is_rotation_90_or_270 ? src_h : src_w,
> @@ -394,6 +398,10 @@ struct dcss_plane *dcss_plane_init(struct drm_device
> *drm,
> if (ret)
> return ERR_PTR(ret);
>
> + drm_plane_create_scaling_filter_property(_plane->base,
> + BIT(DRM_SCALING_FILTER_DEFAULT) |
> +
> BIT(DRM_SCALING_FILTER_NEAREST_NEIGHBOR));
> +
> drm_plane_create_rotation_property(_plane->base,
> DRM_MODE_ROTATE_0,
> DRM_MODE_ROTATE_0 |
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-scaler.c
> b/drivers/gpu/drm/imx/dcss/dcss-scaler.c
> index cd21905de580..47852b9dd5ea 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-scaler.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-scaler.c
> @@ -77,6 +77,8 @@ struct dcss_scaler_ch {
>
> u32 c_vstart;
> u32 c_hstart;
> +
> + bool use_nn_interpolation;
> };
>
> struct dcss_scaler {
> @@ -243,6 +245,17 @@ static void dcss_scaler_gaussian_filter(int fc_q, bool
> use_5_taps,
> }
> }
>
> +static void dcss_scaler_nearest_neighbor_filter(bool use_5_taps,
> + int coef[][PSC_NUM_TAPS])
> +{
> + int i, j;
> +
> + for (i = 0; i < PSC_STORED_PHASES; i++)
> + for (j = 0; j < PSC_NUM_TAPS; j++)
> + coef[i][j] = j == PSC_NUM_TAPS >> 1 ?
> + (1 << PSC_COEFF_PRECISION) : 0;
> +}
> +
> /**
> * dcss_scaler_filter_design() - Compute filter coefficients using
> *Gaussian filter.
> @@ -253,7 +266,8 @@ static void dcss_scaler_gaussian_filter(int fc_q, bool
> use_5_taps,
> */
> static void dcss_scaler_filter_design(int src_length, int dst_length,
> bool use_5_taps, bool
Re: [PATCH 0/2] drm/imx/dcss: a couple of fixes
On Do, 2020-11-05 at 16:01 +0200, Laurentiu Palcu wrote: > Hi, > > This patchset fixes 90/270 rotations for Vivante tiled and super-tiled > formats and a Coccinelle warning. Thanks, looks good. I've pushed them into drm-misc-next. Regards, Lucas
Re: [PATCH] arm64: dts: imx8mq: add DCSS node
On Mi, 2020-11-25 at 12:39 +0200, Laurentiu Palcu wrote:
> This patch adds the node for iMX8MQ Display Controller Subsystem.
>
> Signed-off-by: Laurentiu Palcu
Reviewed-by: Lucas Stach
> ---
> Hi,
>
> This is, actually, a resend of the patch because we decided to drop it
> from the main DCSS patchset until the driver gets merged.
>
> Now that the driver is in mainline, we can finally add DCSS node in DTS.
>
> Thanks,
> laurentiu
>
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 23 +++
> 1 file changed, 23 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 5e0e7d0f1bc4..5a617f9ed8b5 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1103,6 +1103,29 @@
> #size-cells = <1>;
> ranges = <0x32c0 0x32c0 0x40>;
>
> + dcss: display-controller@32e0 {
> + compatible = "nxp,imx8mq-dcss";
> + reg = <0x32e0 0x2d000>, <0x32e2f000 0x1000>;
> + interrupts = <6>, <8>, <9>;
> + interrupt-names = "ctxld", "ctxld_kick",
> "vblank";
> + interrupt-parent = <>;
> + clocks = < IMX8MQ_CLK_DISP_APB_ROOT>,
> + < IMX8MQ_CLK_DISP_AXI_ROOT>,
> + < IMX8MQ_CLK_DISP_RTRM_ROOT>,
> + < IMX8MQ_VIDEO2_PLL_OUT>,
> + < IMX8MQ_CLK_DISP_DTRC>;
> + clock-names = "apb", "axi", "rtrm", "pix",
> "dtrc";
> + assigned-clocks = < IMX8MQ_CLK_DISP_AXI>,
> + < IMX8MQ_CLK_DISP_RTRM>,
> + <
> IMX8MQ_VIDEO2_PLL1_REF_SEL>;
> + assigned-clock-parents = <
> IMX8MQ_SYS1_PLL_800M>,
> + <
> IMX8MQ_SYS1_PLL_800M>,
> + < IMX8MQ_CLK_27M>;
> + assigned-clock-rates = <8>,
> +<4>;
> + status = "disabled";
> + };
> +
> irqsteer: interrupt-controller@32e2d000 {
> compatible = "fsl,imx8m-irqsteer",
> "fsl,imx-irqsteer";
> reg = <0x32e2d000 0x1000>;
Re: dts: add MNT Reform 2.0
Hi Lukas,
just some general comments, not a full review.
Am Dienstag, den 24.11.2020, 18:27 +0100 schrieb Lukas F. Hartmann:
> This is the device tree for the MNT Reform 2.0 open hardware laptop
> which is based on NXP i.MX8MQ. It is designed around the Boundary Devices
> Nitrogen8M SoM.
Since this is based on a module the DT should be split in a module dtsi
and a baseboard dts.
> Technical details: https://mntre.com/reform
>
> Signed-off-by: Lukas F. Hartmann
> ---
> .../boot/dts/freescale/imx8mq-mnt-reform2.dts | 783 ++
> 1 file changed, 783 insertions(+)
> create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-mnt-reform2.dts
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-mnt-reform2.dts
> b/arch/arm64/boot/dts/freescale/imx8mq-mnt-reform2.dts
> new file mode 100644
> index 0..c95a22a1e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-mnt-reform2.dts
> @@ -0,1 +1,782 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2018 Boundary Devices
> + * Copyright 2019-2020 MNT Research GmbH
> + */
> +
> +/dts-v1/;
> +
> +#include "dt-bindings/input/input.h"
> +#include "dt-bindings/pwm/pwm.h"
> +#include "dt-bindings/usb/pd.h"
> +#include "dt-bindings/gpio/gpio.h"
> +#include "imx8mq.dtsi"
> +
> +/ {
> + model = "MNT Reform 2";
> + compatible = "fsl,imx8mq";
This needs a more specific compatible.
> +
> + chosen {
> + stdout-path = "serial0:115200n8";
> + };
> +
> + // 4GB of RAM
> + memory@4000 {
> + device_type = "memory";
> + reg = <0x 0x4000 0 0xc000>;
> + };
Do you need this node? Most bootloaders should be able to patch in the
correct amount of memory before passing control to the kernel.
> +
> + reg_vref_0v9: regulator-vref-0v9 {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-0v9";
> + regulator-min-microvolt = <90>;
> + regulator-max-microvolt = <90>;
> + regulator-always-on;
> + };
> +
> + reg_vref_1v2: regulator-vref-1v2 {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-1v2";
> + regulator-min-microvolt = <120>;
> + regulator-max-microvolt = <120>;
> + regulator-always-on;
> + };
> +
> + reg_vref_1v8: regulator-vref-1v8 {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-1v8";
> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> + regulator-always-on;
> + };
> +
> + reg_vref_2v5: regulator-vref-2v5 {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-2v5";
> + regulator-min-microvolt = <250>;
> + regulator-max-microvolt = <250>;
> + regulator-always-on;
> + };
> +
> + reg_vref_3v3: regulator-vref-3v3 {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-3v3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + regulator-always-on;
> + };
> +
> + reg_vref_5v: regulator-vref-5v {
> + compatible = "regulator-fixed";
> + regulator-name = "vref-5v";
> + regulator-min-microvolt = <500>;
> + regulator-max-microvolt = <500>;
> + regulator-always-on;
> + };
> +
> + backlight: backlight {
> + compatible = "pwm-backlight";
> + pwms = < 0 1>;
> + enable-gpios = < 10 GPIO_ACTIVE_HIGH>;
> + brightness-levels = <0 32 64 128 160 200 255>;
> + default-brightness-level = <6>;
> + };
> +
> + panel {
> + compatible = "innolux,n125hce-gn1";
> + power = <_vref_3v3>;
> + backlight = <>;
> + no-hpd;
> +
> + port@0 {
> + panel_in: endpoint {
> + remote-endpoint = <_bridge_out>;
> + };
> + };
> + };
> +
> + sound {
> + compatible = "simple-audio-card";
> + simple-audio-card,name = "wm8960-audio";
> + simple-audio-card,format = "i2s";
> + simple-audio-card,frame-master = <_cpu_master>;
> + simple-audio-card,bitclock-master = <_cpu_master>;
> +
> + simple-audio-card,widgets =
> + "Microphone", "Mic Jack",
> + "Line", "Line In",
> + "Line", "Line Out",
> + "Speaker", "Speaker",
> + "Headphone", "Headphone Jack";
> + simple-audio-card,routing =
> + "Headphone Jack", "HP_L",
> + "Headphone Jack", "HP_R",
> + "Speaker", "SPK_LP",
> + "Speaker", "SPK_LN",
> + "Speaker", "SPK_RP",
> +
Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN
Am Donnerstag, den 29.10.2020, 07:18 -0500 schrieb Adam Ford:
> On Thu, Oct 29, 2020 at 6:55 AM Lucas Stach
> wrote:
> > Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford:
> > > On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach <
> > > l.st...@pengutronix.de> wrote:
> > > > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford:
> > > > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa
> > > > > wrote:
> > > > > > On 20-10-25 11:05:32, Adam Ford wrote:
> > > > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut <
> > > > > > > ma...@denx.de> wrote:
> > > > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote:
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > > Together, both the GPC and the clk-blk driver
> > > > > > > > > > should be able to pull
> > > > > > > > > > the multimedia block out of reset. Currently, the
> > > > > > > > > > GPC can handle the
> > > > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI
> > > > > > > > > > appear to be gated by
> > > > > > > > > > the clock block
> > > > > > > > > >
> > > > > > > > > > My original patch RFC didn't include the imx8mn
> > > > > > > > > > node, because it
> > > > > > > > > > hangs, but the node I added looks like:
> > > > > > > > > >
> > > > > > > > > > media_blk_ctl: clock-controller@32e28000 {
> > > > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl",
> > > > > > > > > > "syscon";
> > > > > > > > > > reg = <0x32e28000 0x1000>;
> > > > > > > > > > #clock-cells = <1>;
> > > > > > > > > > #reset-cells = <1>;
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > I was hoping you might have some feedback on the
> > > > > > > > > > 8mn clk-blk driver
> > > > > > > > > > since you did the 8mp clk-blk drive and they appear
> > > > > > > > > > to be very
> > > > > > > > > > similar.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I'll do you one better still. I'll apply the patch in
> > > > > > > > > my tree and give it
> > > > > > > > > a test tomorrow morning.
> > > > > > >
> > > > > > > I do have some more updates on how to get the system to
> > > > > > > not hang, and
> > > > > > > to enumerate more clocks.
> > > > > > > Looking at Marek's work on enabling clocks in the 8MM, he
> > > > > > > added a
> > > > > > > power-domain in dispmix_blk_ctl pointing to the dispmix
> > > > > > > in the GPC.
> > > > > > > By forcing the GPC driver to write 0x1fff to 32e28004,
> > > > > > > 0x7f to
> > > > > > > 32e28000 and 0x3 to 32e28008, the i.MX8MM can bring
> > > > > > > the display
> > > > > > > clocks out of reset.
> > > > > > >
> > > > > >
> > > > > > Yeah, that makes sense. Basically, it was trying to disable
> > > > > > unused clocks
> > > > > > (see clk_disable_unused) but in order to disable the clocks
> > > > > > from the
> > > > > > media BLK_CTL (which I think should be renamed in display
> > > > > > BLK_CTL) the
> > > > > > PD need to be on. Since you initially didn't give it any
> > > > > > PD, it was trying
> > > > > > to blindly write/read the gate bit and therefore freeze.
> > > > > >
> > > > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to
> > > > > > > both
> > > > > > > 32e28000 and 32e28004, and the values written for the 8MM
> > > > > >
Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN
Am Montag, den 26.10.2020, 11:23 -0500 schrieb Adam Ford:
> On Mon, Oct 26, 2020 at 10:44 AM Lucas Stach wrote:
> > Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford:
> > > On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa wrote:
> > > > On 20-10-25 11:05:32, Adam Ford wrote:
> > > > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut wrote:
> > > > > > On 10/25/20 1:05 PM, Abel Vesa wrote:
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > Together, both the GPC and the clk-blk driver should be able to
> > > > > > > > pull
> > > > > > > > the multimedia block out of reset. Currently, the GPC can
> > > > > > > > handle the
> > > > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be
> > > > > > > > gated by
> > > > > > > > the clock block
> > > > > > > >
> > > > > > > > My original patch RFC didn't include the imx8mn node, because it
> > > > > > > > hangs, but the node I added looks like:
> > > > > > > >
> > > > > > > > media_blk_ctl: clock-controller@32e28000 {
> > > > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon";
> > > > > > > > reg = <0x32e28000 0x1000>;
> > > > > > > > #clock-cells = <1>;
> > > > > > > > #reset-cells = <1>;
> > > > > > > > };
> > > > > > > >
> > > > > > > > I was hoping you might have some feedback on the 8mn clk-blk
> > > > > > > > driver
> > > > > > > > since you did the 8mp clk-blk drive and they appear to be very
> > > > > > > > similar.
> > > > > > > >
> > > > > > >
> > > > > > > I'll do you one better still. I'll apply the patch in my tree and
> > > > > > > give it
> > > > > > > a test tomorrow morning.
> > > > >
> > > > > I do have some more updates on how to get the system to not hang, and
> > > > > to enumerate more clocks.
> > > > > Looking at Marek's work on enabling clocks in the 8MM, he added a
> > > > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC.
> > > > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to
> > > > > 32e28000 and 0x3 to 32e28008, the i.MX8MM can bring the display
> > > > > clocks out of reset.
> > > > >
> > > >
> > > > Yeah, that makes sense. Basically, it was trying to disable unused
> > > > clocks
> > > > (see clk_disable_unused) but in order to disable the clocks from the
> > > > media BLK_CTL (which I think should be renamed in display BLK_CTL) the
> > > > PD need to be on. Since you initially didn't give it any PD, it was
> > > > trying
> > > > to blindly write/read the gate bit and therefore freeze.
> > > >
> > > > > Unfortunately, the i.MX8MN needs to have 0x100 written to both
> > > > > 32e28000 and 32e28004, and the values written for the 8MM are not
> > > > > compatible.
> > > > > By forcing the GPC to write those values, I can get lcdif_pixel_clk
> > > > > and the mipi_dsi_clkref appearing on the Nano.
> > > >
> > > > I'm trying to make a branch with all the patches for all i.MX8M so I
> > > > can keep track of it all. On this branch I've also applied the
> > > > following patchset from Lucas Stach:
> > > > https://www.spinics.net/lists/arm-kernel/msg843007.html
> > > > but I'm getting the folowing errors:
> > > >
> > > > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> > > > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> > > > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> > > >
> > > > Lucas, any thoughts?
> > > >
> > > > Maybe it's something related to 8MN.
> > > >
> > > I will go back and double check this now that we have both the
> > > blt_crl->power-domain and the power-domain->blk_ctl.
> > >
> > > > Will dig further, see what pops out.
&g
Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN
Am Montag, den 26.10.2020, 16:55 +0200 schrieb Abel Vesa:
> On 20-10-25 11:05:32, Adam Ford wrote:
> > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut wrote:
> > > On 10/25/20 1:05 PM, Abel Vesa wrote:
> > >
> > > [...]
> > >
> > > > > Together, both the GPC and the clk-blk driver should be able to pull
> > > > > the multimedia block out of reset. Currently, the GPC can handle the
> > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated by
> > > > > the clock block
> > > > >
> > > > > My original patch RFC didn't include the imx8mn node, because it
> > > > > hangs, but the node I added looks like:
> > > > >
> > > > > media_blk_ctl: clock-controller@32e28000 {
> > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon";
> > > > > reg = <0x32e28000 0x1000>;
> > > > > #clock-cells = <1>;
> > > > > #reset-cells = <1>;
> > > > > };
> > > > >
> > > > > I was hoping you might have some feedback on the 8mn clk-blk driver
> > > > > since you did the 8mp clk-blk drive and they appear to be very
> > > > > similar.
> > > > >
> > > >
> > > > I'll do you one better still. I'll apply the patch in my tree and give
> > > > it
> > > > a test tomorrow morning.
> >
> > I do have some more updates on how to get the system to not hang, and
> > to enumerate more clocks.
> > Looking at Marek's work on enabling clocks in the 8MM, he added a
> > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC.
> > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to
> > 32e28000 and 0x3 to 32e28008, the i.MX8MM can bring the display
> > clocks out of reset.
> >
>
> Yeah, that makes sense. Basically, it was trying to disable unused clocks
> (see clk_disable_unused) but in order to disable the clocks from the
> media BLK_CTL (which I think should be renamed in display BLK_CTL) the
> PD need to be on. Since you initially didn't give it any PD, it was trying
> to blindly write/read the gate bit and therefore freeze.
>
> > Unfortunately, the i.MX8MN needs to have 0x100 written to both
> > 32e28000 and 32e28004, and the values written for the 8MM are not
> > compatible.
> > By forcing the GPC to write those values, I can get lcdif_pixel_clk
> > and the mipi_dsi_clkref appearing on the Nano.
>
> I'm trying to make a branch with all the patches for all i.MX8M so I
> can keep track of it all. On this branch I've also applied the
> following patchset from Lucas Stach:
> https://www.spinics.net/lists/arm-kernel/msg843007.html
> but I'm getting the folowing errors:
>
> [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400
>
> Lucas, any thoughts?
>
> Maybe it's something related to 8MN.
The ADB is apparently clocked by one of the BLK_CTL clocks, so the ADB
handshake ack will only work when the BLK_CTL clocks are enabled. So I
guess the GPC driver should enable those clocks and assert the resets
at the right time in the power-up sequencing. Unfortunately this means
we can't properly put the BLK_CTL driver in the power-domain without
having a cyclic dependency in the DT. I'm still thinking about how to
solve this properly.
Regards,
Lucas
Re: [RFC 0/3] clk: imx: Implement blk-ctl driver for i.MX8MN
Am Montag, den 26.10.2020, 10:12 -0500 schrieb Adam Ford:
> On Mon, Oct 26, 2020 at 9:55 AM Abel Vesa wrote:
> > On 20-10-25 11:05:32, Adam Ford wrote:
> > > On Sun, Oct 25, 2020 at 7:19 AM Marek Vasut wrote:
> > > > On 10/25/20 1:05 PM, Abel Vesa wrote:
> > > >
> > > > [...]
> > > >
> > > > > > Together, both the GPC and the clk-blk driver should be able to pull
> > > > > > the multimedia block out of reset. Currently, the GPC can handle
> > > > > > the
> > > > > > USB OTG and the GPU, but the LCDIF and MIPI DSI appear to be gated
> > > > > > by
> > > > > > the clock block
> > > > > >
> > > > > > My original patch RFC didn't include the imx8mn node, because it
> > > > > > hangs, but the node I added looks like:
> > > > > >
> > > > > > media_blk_ctl: clock-controller@32e28000 {
> > > > > > compatible = "fsl,imx8mn-media-blk-ctl", "syscon";
> > > > > > reg = <0x32e28000 0x1000>;
> > > > > > #clock-cells = <1>;
> > > > > > #reset-cells = <1>;
> > > > > > };
> > > > > >
> > > > > > I was hoping you might have some feedback on the 8mn clk-blk driver
> > > > > > since you did the 8mp clk-blk drive and they appear to be very
> > > > > > similar.
> > > > > >
> > > > >
> > > > > I'll do you one better still. I'll apply the patch in my tree and
> > > > > give it
> > > > > a test tomorrow morning.
> > >
> > > I do have some more updates on how to get the system to not hang, and
> > > to enumerate more clocks.
> > > Looking at Marek's work on enabling clocks in the 8MM, he added a
> > > power-domain in dispmix_blk_ctl pointing to the dispmix in the GPC.
> > > By forcing the GPC driver to write 0x1fff to 32e28004, 0x7f to
> > > 32e28000 and 0x3 to 32e28008, the i.MX8MM can bring the display
> > > clocks out of reset.
> > >
> >
> > Yeah, that makes sense. Basically, it was trying to disable unused clocks
> > (see clk_disable_unused) but in order to disable the clocks from the
> > media BLK_CTL (which I think should be renamed in display BLK_CTL) the
> > PD need to be on. Since you initially didn't give it any PD, it was trying
> > to blindly write/read the gate bit and therefore freeze.
> >
> > > Unfortunately, the i.MX8MN needs to have 0x100 written to both
> > > 32e28000 and 32e28004, and the values written for the 8MM are not
> > > compatible.
> > > By forcing the GPC to write those values, I can get lcdif_pixel_clk
> > > and the mipi_dsi_clkref appearing on the Nano.
> >
> > I'm trying to make a branch with all the patches for all i.MX8M so I
> > can keep track of it all. On this branch I've also applied the
> > following patchset from Lucas Stach:
> > https://www.spinics.net/lists/arm-kernel/msg843007.html
> > but I'm getting the folowing errors:
> >
> > [ 16.690885] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> > [ 16.716839] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> > [ 16.730500] imx-pgc imx-pgc-domain.3: failed to power up ADB400
> >
> > Lucas, any thoughts?
> >
> > Maybe it's something related to 8MN.
> >
> I will go back and double check this now that we have both the
> blt_crl->power-domain and the power-domain->blk_ctl.
>
> > Will dig further, see what pops out.
>
> I wasn't sure which direction to go with the name. I can rename the
> media_blk_ctl driver to display_blk_ctl. I used Media based on the
> imx8mp naming convention and the fact that it's controlling both the
> display and the camera interface, however it's depending on the
> dispmix GPC.
>
> I'll submit a RFC V2 with the cross referencing to the GPC based on
> Marek's Mini patch set, but we'll still have an issue where the Mini
> and Nano have different syscon values to enable the clocks, and
> Marek's branch has it card-coded, so my patch would effectively break
> the Mini in order to make the Nano operate until we find a better
> solution.
The GPC should not write into the BLK_CTL region via syscon, but
instead use the clocks and resets as exposed by the BLK_CTL driver.
Doing it via syscon is a hack to get things going. The clocks and
resets should properly be hooked up to the GPC domains via the clocks
and resets DT properties.
For the clocks there is one complication: if the clocks are controlled
via BLK_CTL we can only enable them once the domain is powered up,
however the earlier designs using the GPCv2 assert resets as part of
the power up sequence, which needs the clocks to be running for the
reset to propagate. So depending on whether we have a power domain with
a BLK_CTL or not we need to enable the clocks before or after powering
up the domain. I guess we need a new DT property to specify which way
the domain needs to handled.
Regards,
Lucas
Re: [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single ring
On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> From: Rob Clark
>
> If there is only a single ring (no-preemption), everything is FIFO order
> and there is no need to implicit-sync.
>
> Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as behavior
> is undefined when fences are not used to synchronize buffer usage across
> contexts (which is the only case where multiple different priority rings
> could come into play).
Really, doesn't this break cross-device implicit sync? Okay, you may
not have many peripherals that rely on implicit sync on devices where
Adreno is usually found, but it seems rather heavy-handed.
Wouldn't it be better to only ignore fences from your own ring context
in the implicit sync, like we do in the common DRM scheduler
(drm_sched_dependency_optimized)?
Regards,
Lucas
> Signed-off-by: Rob Clark
> Reviewed-by: Kristian H. Kristensen
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index d04c349d8112..b6babc7f9bb8 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -283,7 +283,7 @@ static int submit_lock_objects(struct msm_gem_submit
> *submit)
> return ret;
> }
>
> -static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
> +static int submit_fence_sync(struct msm_gem_submit *submit, bool
> implicit_sync)
> {
> int i, ret = 0;
>
> @@ -303,7 +303,7 @@ static int submit_fence_sync(struct msm_gem_submit
> *submit, bool no_implicit)
> return ret;
> }
>
> - if (no_implicit)
> + if (!implicit_sync)
> continue;
>
> ret = msm_gem_sync_object(_obj->base, submit->ring->fctx,
> @@ -774,7 +774,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void
> *data,
> if (ret)
> goto out;
>
> - ret = submit_fence_sync(submit, !!(args->flags &
> MSM_SUBMIT_NO_IMPLICIT));
> + ret = submit_fence_sync(submit, (gpu->nr_rings > 1) &&
> + !(args->flags & MSM_SUBMIT_NO_IMPLICIT));
> if (ret)
> goto out;
>
Re: [PATCH] arm64: dts: imx8mm: Add GPU node
On Do, 2020-10-22 at 13:31 -0500, Adam Ford wrote: > On Thu, Oct 22, 2020 at 1:17 PM Marek Vasut wrote: > > On 10/22/20 7:16 PM, Adam Ford wrote: > > > According to the documentation from NXP, the i.MX8M Nano has a > > > Vivante GC7000 Ultra Lite as its GPU core. > > > > > > With this patch, the Etnaviv driver presents the GPU as: > > >etnaviv-gpu 3800.gpu: model: GC7000, revision: 6203 > > > > > > It uses the GPCV2 controller to enable the power domain for the > > > GPU. > > > > Subject should say 8mn , not 8mm . > > ugh.. My mistake. I'll submit a V2 once other have had a chance to > give some feedback. > > Maybe NXP can comment on the dialog below. > > > Are the assigned-clock-rates correct ? > > I used the assigned clock rates from the vendor kernel, with the > exception of running at 400MHz instead of 600MHz. According to the > datasheet, the GPU clock needs to be 400MHZ to run at 0.85V. The > 600MHz operating point for the GPU requires a 0.95V operating point. > Since the default operating point for the Nano shows 0.85V, I left > the > GPU clock lower to match the normal operating speed. This varies a > bit from the vendor kernel, but their kernel is also showing a 0.95V > operating point, so I think that's why they are specifying a 600MHz > operating point. > > On the Beacon embedded board, we're driving the LPDDR to 800MHz which > requires the ARM to run at .95V. I was able to override the > assigned-clock rates for my board to run at 600MHz, and change the > ARM > operating point to .95V to meet the spec. > > My intent was to use the defaults and let the board files override > them. If you want, I can try to look through the board files to see > what operating point their using and propose updates to those > respective device trees to address the clocks on those boards. I think this is fine as-is with this explanation. At least we have a precedent in the i.MX8MQ DT where the assigned clocks are for the base (non overdrive) operating point and boards can choose to override it if they want to use the overdrive OPP. At least until we add proper frequency scaling in etnaviv, which should obsolete those fixed clock rates. Regards, Lucas
Re: [PATCH] tee: optee: replace might_sleep with cond_resched
On Fr, 2020-09-25 at 08:59 +0200, Jens Wiklander wrote:
> On Fri, Sep 18, 2020 at 7:45 PM Rouven Czerwinski
> wrote:
> > On Kernels with CONFIG_PREEMPT_NONE might_sleep() is not enough to force
> > rescheduling, replace it with a resched check and cond_resched. Fixes
> > the following stall:
> >
> > [ 572.945146] rcu: INFO: rcu_sched self-detected stall on CPU
> > [ 572.949275] rcu: 0-: (2099 ticks this GP)
> > idle=572/1/0x4002 softirq=7412/7412 fqs=974
> > [ 572.957964] (t=2100 jiffies g=10393 q=21)
> > [ 572.962054] NMI backtrace for cpu 0
> > [ 572.965540] CPU: 0 PID: 165 Comm: xtest Not tainted 5.8.7 #1
> > [ 572.971188] Hardware name: STM32 (Device Tree Support)
> > [ 572.976354] [] (unwind_backtrace) from []
> > (show_stack+0x10/0x14)
> > [ 572.984080] [] (show_stack) from []
> > (dump_stack+0xc4/0xd8)
> > [ 572.991300] [] (dump_stack) from []
> > (nmi_cpu_backtrace+0x90/0xc4)
> > [ 572.999130] [] (nmi_cpu_backtrace) from []
> > (nmi_trigger_cpumask_backtrace+0xec/0x130)
> > [ 573.008706] [] (nmi_trigger_cpumask_backtrace) from
> > [] (rcu_dump_cpu_stacks+0xe8/0x110)
> > [ 573.018453] [] (rcu_dump_cpu_stacks) from []
> > (rcu_sched_clock_irq+0x7fc/0xa88)
> > [ 573.027416] [] (rcu_sched_clock_irq) from []
> > (update_process_times+0x30/0x8c)
> > [ 573.036291] [] (update_process_times) from []
> > (tick_sched_timer+0x4c/0xa8)
> > [ 573.044905] [] (tick_sched_timer) from []
> > (__hrtimer_run_queues+0x174/0x358)
> > [ 573.053696] [] (__hrtimer_run_queues) from []
> > (hrtimer_interrupt+0x118/0x2bc)
> > [ 573.062573] [] (hrtimer_interrupt) from []
> > (arch_timer_handler_virt+0x28/0x30)
> > [ 573.071536] [] (arch_timer_handler_virt) from []
> > (handle_percpu_devid_irq+0x8c/0x240)
> > [ 573.081109] [] (handle_percpu_devid_irq) from []
> > (generic_handle_irq+0x34/0x44)
> > [ 573.090156] [] (generic_handle_irq) from []
> > (__handle_domain_irq+0x5c/0xb0)
> > [ 573.098857] [] (__handle_domain_irq) from []
> > (gic_handle_irq+0x4c/0x90)
> > [ 573.107209] [] (gic_handle_irq) from []
> > (__irq_svc+0x6c/0x90)
> > [ 573.114682] Exception stack(0xd90dfcf8 to 0xd90dfd40)
> > [ 573.119732] fce0:
> > 0004
> > [ 573.127917] fd00:
> > d93493cc
> > [ 573.136098] fd20: d2bc39c0 be926998 d90dfd58 d90dfd48 c09f3384
> > c01151f0 400d0013
> > [ 573.144281] [] (__irq_svc) from []
> > (__arm_smccc_smc+0x10/0x20)
> > [ 573.151854] [] (__arm_smccc_smc) from []
> > (optee_smccc_smc+0x3c/0x44)
> > [ 573.159948] [] (optee_smccc_smc) from []
> > (optee_do_call_with_arg+0xb8/0x154)
> > [ 573.168735] [] (optee_do_call_with_arg) from []
> > (optee_invoke_func+0x110/0x190)
> > [ 573.177786] [] (optee_invoke_func) from []
> > (tee_ioctl+0x10b8/0x11c0)
> > [ 573.185879] [] (tee_ioctl) from []
> > (ksys_ioctl+0xe0/0xa4c)
> > [ 573.193101] [] (ksys_ioctl) from []
> > (ret_fast_syscall+0x0/0x54)
> > [ 573.200750] Exception stack(0xd90dffa8 to 0xd90dfff0)
> > [ 573.205803] ffa0: be926bf4 be926a78 0003
> > 8010a403 be926908 004e3cf8
> > [ 573.213987] ffc0: be926bf4 be926a78 0036 be926908
> > be926918 be9269b0 bffdf0f8
> > [ 573.222162] ffe0: b6d76fb0 be9268fc b6d66621 b6c7e0d8
> >
> > seen on STM32 DK2.
> >
> > Signed-off-by: Rouven Czerwinski
> > ---
> > drivers/tee/optee/call.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tee/optee/call.c b/drivers/tee/optee/call.c
> > index 20b6fd7383c5..83b73b1d52f0 100644
> > --- a/drivers/tee/optee/call.c
> > +++ b/drivers/tee/optee/call.c
> > @@ -8,6 +8,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -148,7 +149,8 @@ u32 optee_do_call_with_arg(struct tee_context *ctx,
> > phys_addr_t parg)
> > */
> > optee_cq_wait_for_completion(>call_queue,
> > );
> > } else if (OPTEE_SMC_RETURN_IS_RPC(res.a0)) {
> > - might_sleep();
> > + if(need_resched())
Missing space after if.
> > + cond_resched();
>
> This looks OK to me. But I'd prefer if someone else could confirm this too.
The previous might_sleep() is wrong, as it is a debug annotation to
more easily find sleep while atomic bugs and will only ever have any
effect when CONFIG_DEBUG_ATOMIC_SLEEP is enabled. So I guess the commit
message here is also not fully right, as the now added cond_resched()
is the proper way to yield the CPU for both PREEMPT_VOLUNTARY and
PREEMPT_NONE configs.
Regards,
Lucas
[tip: sched/core] sched/deadline: Fix stale throttling on de-/boosted tasks
The following commit has been merged into the sched/core branch of tip:
Commit-ID: 46fcc4b00c3cca8adb9b7c9afdd499f64e427135
Gitweb:
https://git.kernel.org/tip/46fcc4b00c3cca8adb9b7c9afdd499f64e427135
Author:Lucas Stach
AuthorDate:Mon, 31 Aug 2020 13:07:19 +02:00
Committer: Peter Zijlstra
CommitterDate: Fri, 25 Sep 2020 14:23:24 +02:00
sched/deadline: Fix stale throttling on de-/boosted tasks
When a boosted task gets throttled, what normally happens is that it's
immediately enqueued again with ENQUEUE_REPLENISH, which replenishes the
runtime and clears the dl_throttled flag. There is a special case however:
if the throttling happened on sched-out and the task has been deboosted in
the meantime, the replenish is skipped as the task will return to its
normal scheduling class. This leaves the task with the dl_throttled flag
set.
Now if the task gets boosted up to the deadline scheduling class again
while it is sleeping, it's still in the throttled state. The normal wakeup
however will enqueue the task with ENQUEUE_REPLENISH not set, so we don't
actually place it on the rq. Thus we end up with a task that is runnable,
but not actually on the rq and neither a immediate replenishment happens,
nor is the replenishment timer set up, so the task is stuck in
forever-throttled limbo.
Clear the dl_throttled flag before dropping back to the normal scheduling
class to fix this issue.
Signed-off-by: Lucas Stach
Signed-off-by: Peter Zijlstra (Intel)
Acked-by: Juri Lelli
Link: https://lkml.kernel.org/r/20200831110719.2126930-1-l.st...@pengutronix.de
---
kernel/sched/deadline.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 3862a28..c19c188 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1527,12 +1527,15 @@ static void enqueue_task_dl(struct rq *rq, struct
task_struct *p, int flags)
pi_se = _task->dl;
} else if (!dl_prio(p->normal_prio)) {
/*
-* Special case in which we have a !SCHED_DEADLINE task
-* that is going to be deboosted, but exceeds its
-* runtime while doing so. No point in replenishing
-* it, as it's going to return back to its original
-* scheduling class after this.
+* Special case in which we have a !SCHED_DEADLINE task that is
going
+* to be deboosted, but exceeds its runtime while doing so. No
point in
+* replenishing it, as it's going to return back to its original
+* scheduling class after this. If it has been throttled, we
need to
+* clear the flag, otherwise the task may wake up as throttled
after
+* being boosted again with no means to replenish the runtime
and clear
+* the throttle.
*/
+ p->dl.dl_throttled = 0;
BUG_ON(!p->dl.dl_boosted || flags != ENQUEUE_REPLENISH);
return;
}
Re: [PATCH 3/4] drm/etnaviv: add total hi bandwidth perfcounter
On Fr, 2020-08-14 at 11:05 +0200, Christian Gmeiner wrote:
> These two perf counters represent the total read and write
> GPU bandwidth in terms of 64bits.
>
> The used sequence was taken from Vivante kernel driver.
>
> Signed-off-by: Christian Gmeiner
> ---
> drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 35 ++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> index 782732e6ce72..b37459f022d7 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> @@ -69,6 +69,29 @@ static u32 pipe_perf_reg_read(struct etnaviv_gpu *gpu,
> return value;
> }
>
> +static u32 pipe_reg_read(struct etnaviv_gpu *gpu,
> + const struct etnaviv_pm_domain *domain,
> + const struct etnaviv_pm_signal *signal)
> +{
> + u32 clock = gpu_read(gpu, VIVS_HI_CLOCK_CONTROL);
> + u32 value = 0;
> + unsigned i;
> +
> + for (i = 0; i < gpu->identity.pixel_pipes; i++) {
> + clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
> + clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(i);
> + gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
> + value += gpu_read(gpu, signal->data);
> + }
> +
> + /* switch back to pixel pipe 0 to prevent GPU hang */
> + clock &= ~(VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE__MASK);
> + clock |= VIVS_HI_CLOCK_CONTROL_DEBUG_PIXEL_PIPE(0);
> + gpu_write(gpu, VIVS_HI_CLOCK_CONTROL, clock);
> +
> + return value;
> +}
> +
> static u32 hi_total_cycle_read(struct etnaviv_gpu *gpu,
> const struct etnaviv_pm_domain *domain,
> const struct etnaviv_pm_signal *signal)
> @@ -102,8 +125,18 @@ static const struct etnaviv_pm_domain doms_3d[] = {
> .name = "HI",
> .profile_read = VIVS_MC_PROFILE_HI_READ,
> .profile_config = VIVS_MC_PROFILE_CONFIG2,
> - .nr_signals = 5,
> + .nr_signals = 7,
I've tripped across this part. It's something I don't particularly
like, as this value has a risk of getting inconsistent with the actual
array. Maybe we could split out signal array from this initialization,
so we could then use the ARRAY_SIZE macro to initialize this value?
But that's not really related to this patch and can be done in a
follow-up cleanup.
Regards,
Lucas
> .signal = (const struct etnaviv_pm_signal[]) {
> + {
> + "TOTAL_READ_BYTES8",
> + VIVS_HI_PROFILE_READ_BYTES8,
> + _reg_read,
> + },
> + {
> + "TOTAL_WRITE_BYTES8",
> + VIVS_HI_PROFILE_WRITE_BYTES8,
> + _reg_read,
> + },
> {
> "TOTAL_CYCLES",
> 0,
Re: [PATCH 0/4] drm/etnaviv: add total hi bandwidth perf counters
On Fr, 2020-08-14 at 11:05 +0200, Christian Gmeiner wrote: > This little patch set adds support for the total bandwidth used by HI. The > basic hi bandwidth read-out is quite simple but I needed to add some little > clean-ups to make it nice looking. > > Christian Gmeiner (4): > drm/etnaviv: rename pipe_reg_read(..) > drm/etnaviv: call perf_reg_read(..) > drm/etnaviv: add total hi bandwidth perfcounter > drm/etnaviv: add pipe_select(..) helper > > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 78 --- > 1 file changed, 55 insertions(+), 23 deletions(-) Thanks, I've applied the whole series to my etnaviv/next branch. regards, Lucas
Re: [PATCH v1 1/2] dt-bindings: arm: fsl: add Protonic WD3 board
On Mi, 2020-09-23 at 11:45 +0200, Oleksij Rempel wrote: > Add Protonic Holland WD3 iMX6qp based board > > Signed-off-by: Oleksij Rempel > --- > Documentation/devicetree/bindings/arm/fsl.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml > b/Documentation/devicetree/bindings/arm/fsl.yaml > index e94a455eeab9..cf949c460426 100644 > --- a/Documentation/devicetree/bindings/arm/fsl.yaml > +++ b/Documentation/devicetree/bindings/arm/fsl.yaml > @@ -161,6 +161,7 @@ properties: >- enum: >- fsl,imx6qp-sabreauto # i.MX6 Quad Plus SABRE Automotive > Board >- fsl,imx6qp-sabresd# i.MX6 Quad Plus SABRE Smart > Device Board > + - prt,prtwd3# Protonic WD2 board Comment says WD2, but compatible is WD3. Regards, Lucas
Re: [PATCH] drm/imx/dcss: fix compilation issue on 32bit
On Do, 2020-09-10 at 13:21 +0300, Laurentiu Palcu wrote: > On Thu, Sep 10, 2020 at 11:57:10AM +0200, Daniel Vetter wrote: > > On Thu, Sep 10, 2020 at 11:53 AM Laurentiu Palcu > > wrote: > > > When compiling for 32bit platforms, the compilation fails with: > > > > > > ERROR: modpost: "__aeabi_ldivmod" > > > [drivers/gpu/drm/imx/dcss/imx-dcss.ko] undefined! > > > ERROR: modpost: "__aeabi_uldivmod" > > > [drivers/gpu/drm/imx/dcss/imx-dcss.ko] undefined! > > > > > > This patch adds a dependency on ARM64 since no 32bit SoCs have DCSS, so > > > far. > > > > Usual way to fix this correctly is using the right division macros, > > not limiting the driver to 64bit. But this works for now, would be > > good to fix this properly for compile-testing and all that. > > I didn't see the point in using the macros since this is running only on > 64bit. Though I will probably revisit it and fix it properly in a > subsequent patch. > > > > Signed-off-by: Laurentiu Palcu > > > Reported-by: Daniel Vetter > > > > Reviewed-by: Daniel Vetter > > > > Please push to drm-misc-next. > > About that, I may need some help pushing it. I've already pushed a few minutes ago, so we don't disturb the build tests any longer. Regards, Lucas > Apparently, my request for > a legacy SSH account for contributing to drm-misc got stalled: > > https://gitlab.freedesktop.org/freedesktop/freedesktop/-/issues/289 > > Thanks, > laurentiu > > > -Daniel > > > > > > > > --- > > > drivers/gpu/drm/imx/dcss/Kconfig | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/imx/dcss/Kconfig > > > b/drivers/gpu/drm/imx/dcss/Kconfig > > > index 69860de8861f..2b17a964ff05 100644 > > > --- a/drivers/gpu/drm/imx/dcss/Kconfig > > > +++ b/drivers/gpu/drm/imx/dcss/Kconfig > > > @@ -3,7 +3,7 @@ config DRM_IMX_DCSS > > > select IMX_IRQSTEER > > > select DRM_KMS_CMA_HELPER > > > select VIDEOMODE_HELPERS > > > - depends on DRM && ARCH_MXC > > > + depends on DRM && ARCH_MXC && ARM64 > > > help > > > Choose this if you have a NXP i.MX8MQ based system and want to > > > use the > > > Display Controller Subsystem. This option enables DCSS support. > > > -- > > > 2.17.1 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
Re: [PATCH v9 0/5] Add support for iMX8MQ Display Controller Subsystem
Hi Laurentiu, On Mo, 2020-08-31 at 14:24 +0300, Laurentiu Palcu wrote: > Hi Lucas, Sam, > > On Mon, Aug 31, 2020 at 12:37:23PM +0200, Lucas Stach wrote: > > Hi Laurentiu, > > > > On Fr, 2020-08-28 at 11:36 +0300, Laurentiu Palcu wrote: > > > Hi Lucas, > > > > > > I was wondering about the plans to merge this series. Since not many > > > people can test it properly due to lack of DCSS support in the upstream > > > NWL driver (which I heard it's coming soon) and a completely nonexistent > > > HDP driver, are we going to take a leap of faith and merge it as is? Or > > > should we wait? > > > > I think even if the bridges aren't ready yet, the driver is in a good > > enough shape to merge it. There's no point in letting it accumulate > > bitrot while waiting for the bridges to land. Also I guess it will make > > bridge driver development a lot easier when the DCSS driver is in-tree. > > OK, fair enough. > > > > As to who is going to do the actual merge, I know we had a brief > > > discussion about it some time ago and I was supposed to apply for > > > drm-misc rights, however it feels a little awkward to merge my own > > > code... :) Though, I might not even qualify for drm-misc rights anyway, > > > considering I haven't been very active in this area... :/ > > > > Please consider applying for drm-misc merge rights. Even if you don't > > do the initial merge I think it will come in handy in the long run. > > OK, I'll apply for drm-misc rights. Sam also encouraged me to do this in > a previous email. Since this series will be followed by a bunch of other > patches, enabling support for video planes, among other things, I guess > I'll need the rights anyway. > > > > On that note, I will probably need help with the merging, provided it's > > > still happenning. Will you be able to help me out with this? > > > > Sure, I'm having some time available this week. I'll give this v9 > > another spin on my boards and will do the merge after that. > > Thanks Lucas. As a matter of fact, after Sam's email, I was preparing a > v10 after rebasing to latest drm-next. However, no conflicts were encountered > and the v9 series applied cleanly. Let me know if you encounter any > issues and I can resend it. I've tested this together with a hacked up eDP bridge driver stack on my boards. The testing hasn't been really in-depth, but things seem to work fine. I have now pushed the whole series into drm-misc-next. Regards, Lucas > Thanks, > Laurentiu > > > Regards, > > Lucas > > > > > Thanks, > > > laurentiu > > > > > > On Fri, Jul 31, 2020 at 11:18:28AM +0300, Laurentiu Palcu wrote: > > > > From: Laurentiu Palcu > > > > > > > > Hi, > > > > > > > > This patchset adds initial DCSS support for iMX8MQ chip. Initial support > > > > includes only graphics plane support (no video planes), no HDR10 > > > > capabilities, > > > > no graphics decompression (only linear, tiled and super-tiled buffers > > > > allowed). > > > > > > > > Support for the rest of the features will be added incrementally, in > > > > subsequent > > > > patches. > > > > > > > > The patchset was tested with both HDP driver (in the downstream tree) > > > > and the upstream > > > > MIPI-DSI driver (with a couple of patches on top, to make it work > > > > correctly with DCSS). > > > > > > > > Thanks, > > > > Laurentiu > > > > > > > > Changes in v9: > > > > * Fixed a compilation issue found by Guido in his setup: 'select > > > >VIDEOMODE_HELPERS' was missing from Kconfig; > > > > * Use imx8mq-clock.h in the bindings file so one can understand what > > > >those clock values mean; > > > > * no other changes done. Couldn't address the hang Guido reported as > > > >it's not happening in my setup. However, in my tree, there are some > > > >extra NWL and ADV patches applied on top of upstream ones... Also, > > > >removing them and testing only with upstream, even if there's no > > > >image out, does not produce a hang... :/ > > > > > > > > Changes in v8: > > > > * Removed 'select RESET_CONTROLLER" from Kconfig as Philipp pointed > > > >out. SRC is not used in DCSS driver; > > > > * Nothing else changed; > > > > > > > > Changes in v7
Re: [PATCH] sched/deadline: Fix stale throttling on de-/boosted tasks
On Mi, 2020-09-02 at 11:43 +0200, pet...@infradead.org wrote: > On Wed, Sep 02, 2020 at 08:00:24AM +0200, Juri Lelli wrote: > > On 31/08/20 13:07, Lucas Stach wrote: > > > When a boosted task gets throttled, what normally happens is that it's > > > immediately enqueued again with ENQUEUE_REPLENISH, which replenishes the > > > runtime and clears the dl_throttled flag. There is a special case however: > > > if the throttling happened on sched-out and the task has been deboosted in > > > the meantime, the replenish is skipped as the task will return to its > > > normal scheduling class. This leaves the task with the dl_throttled flag > > > set. > > > > > > Now if the task gets boosted up to the deadline scheduling class again > > > while it is sleeping, it's still in the throttled state. The normal wakeup > > > however will enqueue the task with ENQUEUE_REPLENISH not set, so we don't > > > actually place it on the rq. Thus we end up with a task that is runnable, > > > but not actually on the rq and neither a immediate replenishment happens, > > > nor is the replenishment timer set up, so the task is stuck in > > > forever-throttled limbo. > > > > > > Clear the dl_throttled flag before dropping back to the normal scheduling > > > class to fix this issue. > > > > > > Signed-off-by: Lucas Stach > > Acked-by: Juri Lelli > > Thanks! Does this mean the patch will get picked up as-is, or are there any changes required? Regards, Lucas
tracer_init_tracefs really slow
Hi all, one of my colleagues has taken a look at device boot times and stumbled across a pretty big amount of kernel boot time being spent in tracer_init_tracefs(). On this particular i.MX6Q based device the kernel spends more than 1 second in this function, which is a significant amount of the overall kernel inititalization time. While this machine is no rocket with its Cortex A9 @ 800MHz, the amount of CPU time being used there is pretty irritating. Specifically the issue lies within trace_event_eval_update where ~1100 trace_event_calls get updated with ~500 trace_eval_maps. I haven't had a chance yet to dig any deeper or try to understand more of what's going on there, but I wanted to get the issue out there in case anyone has some cycles to spare to help us along. The obvious questions for now are: 1. Why is this function so damn expensive (at least on this whimpy ARM machine)? and 2. Could any of this be done asynchronously, to not block the kernel in early init? Regards, Lucas
Re: [PATCH 10/13] arm64: dts: imx8mq: Add missing interrupts to GPC
On Fr, 2020-09-04 at 16:53 +0200, Krzysztof Kozlowski wrote:
> The i.MX General Power Controller v2 device node was missing interrupts
> property necessary to route its interrupt to GIC. This also fixes the
> dbts_check warnings like:
>
> arch/arm64/boot/dts/freescale/imx8mq-evk.dt.yaml: gpc@303a:
> {'compatible': ... '$nodename': ['gpc@303a']} is not valid under any
> of the given schemas
> arch/arm64/boot/dts/freescale/imx8mq-evk.dt.yaml: gpc@303a:
> 'interrupts' is a required property
>
> Fixes: fdbcc04da246 ("arm64: dts: imx8mq: add GPC power domains")
> Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Lucas Stach
> ---
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index aad3b9f2f35c..5e0e7d0f1bc4 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -623,6 +623,7 @@
> gpc: gpc@303a {
> compatible = "fsl,imx8mq-gpc";
> reg = <0x303a 0x1>;
> + interrupts = ;
> interrupt-parent = <>;
> interrupt-controller;
> #interrupt-cells = <3>;
Re: [PATCH 09/13] arm64: dts: imx8mp-evk: Align pin configuration group names with schema
On Fr, 2020-09-04 at 16:53 +0200, Krzysztof Kozlowski wrote:
> Device tree schema expects pin configuration groups to end with 'grp'
> suffix, otherwise dtbs_check complain with a warning like:
>
> ... 'usdhc3grp-100mhz', 'usdhc3grp-200mhz' do not match any of the regexes:
> 'grp$', 'pinctrl-[0-9]+'
>
> Signed-off-by: Krzysztof Kozlowski
Reviewed-by: Lucas Stach
> ---
> arch/arm64/boot/dts/freescale/imx8mp-evk.dts | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> index 3d535f1b3440..ad66f1286d95 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
> @@ -157,7 +157,7 @@
> >;
> };
>
> - pinctrl_reg_usdhc2_vmmc: regusdhc2vmmc {
> + pinctrl_reg_usdhc2_vmmc: regusdhc2vmmcgrp {
> fsl,pins = <
> MX8MP_IOMUXC_SD2_RESET_B__GPIO2_IO190x41
> >;
> @@ -182,7 +182,7 @@
> >;
> };
>
> - pinctrl_usdhc2_100mhz: usdhc2grp-100mhz {
> + pinctrl_usdhc2_100mhz: usdhc2-100mhzgrp {
> fsl,pins = <
> MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK0x194
> MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD0x1d4
> @@ -194,7 +194,7 @@
> >;
> };
>
> - pinctrl_usdhc2_200mhz: usdhc2grp-200mhz {
> + pinctrl_usdhc2_200mhz: usdhc2-200mhzgrp {
> fsl,pins = <
> MX8MP_IOMUXC_SD2_CLK__USDHC2_CLK0x196
> MX8MP_IOMUXC_SD2_CMD__USDHC2_CMD0x1d6
> @@ -206,7 +206,7 @@
> >;
> };
>
> - pinctrl_usdhc2_gpio: usdhc2grp-gpio {
> + pinctrl_usdhc2_gpio: usdhc2gpiogrp {
> fsl,pins = <
> MX8MP_IOMUXC_SD2_CD_B__GPIO2_IO12 0x1c4
> >;
> @@ -228,7 +228,7 @@
> >;
> };
>
> - pinctrl_usdhc3_100mhz: usdhc3grp-100mhz {
> + pinctrl_usdhc3_100mhz: usdhc3-100mhzgrp {
> fsl,pins = <
> MX8MP_IOMUXC_NAND_WE_B__USDHC3_CLK 0x194
> MX8MP_IOMUXC_NAND_WP_B__USDHC3_CMD 0x1d4
> @@ -244,7 +244,7 @@
> >;
> };
>
> - pinctrl_usdhc3_200mhz: usdhc3grp-200mhz {
> + pinctrl_usdhc3_200mhz: usdhc3-200mhzgrp {
> fsl,pins = <
> MX8MP_IOMUXC_NAND_WE_B__USDHC3_CLK 0x196
> MX8MP_IOMUXC_NAND_WP_B__USDHC3_CMD 0x1d6
Re: [PATCH 05/13] dt-bindings: gpu: vivante,gc: Remove trailing whitespace
On Fr, 2020-09-04 at 16:53 +0200, Krzysztof Kozlowski wrote: > Remove whitespace at the end of line. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Lucas Stach > --- > Documentation/devicetree/bindings/gpu/vivante,gc.yaml | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > index 67b71cad7b3b..3ed172629974 100644 > --- a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > +++ b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > @@ -33,7 +33,7 @@ properties: >- description: AXI/master interface clock >- description: GPU core clock >- description: Shader clock (only required if GPU has feature PIPE_3D) > - - description: AHB/slave interface clock (only required if GPU can > gate > + - description: AHB/slave interface clock (only required if GPU can gate >slave interface independently) > minItems: 1 > maxItems: 4
Re: [PATCH 01/13] dt-bindings: power: fsl,imx-gpcv2: Document interrupt controller properties
On Fr, 2020-09-04 at 16:53 +0200, Krzysztof Kozlowski wrote: > The i.MX General Power Controller v2 is also an interrupt controller so > document additional properties to fix dtbs_check warnings like: > > arch/arm64/boot/dts/freescale/imx8mq-evk.dt.yaml: gpc@303a: > '#interrupt-cells', 'interrupt-controller' do not match any of the > regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Lucas Stach > --- > Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml | 4 > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > index bde09a0b2da3..a96e6dbf1858 100644 > --- a/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > +++ b/Documentation/devicetree/bindings/power/fsl,imx-gpcv2.yaml > @@ -33,6 +33,10 @@ properties: >interrupts: > maxItems: 1 > > + interrupt-controller: true > + '#interrupt-cells': > +const: 3 > + >pgc: > type: object > description: list of power domains provided by this controller.
Re: [PATCH 04/13] dt-bindings: gpu: vivante,gc: Add common properties
On Fr, 2020-09-04 at 16:53 +0200, Krzysztof Kozlowski wrote: > Add common properties appearing in DTSes (cooling-cells, assigned-clocks > and others) to fix dtbs_check warnings like: > > arch/arm64/boot/dts/freescale/imx8mq-evk.dt.yaml: gpu@3800: > '#cooling-cells', 'assigned-clock-parents', 'assigned-clock-rates', > 'assigned-clocks' do not match any of the regexes: 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Lucas Stach > --- > Documentation/devicetree/bindings/gpu/vivante,gc.yaml | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > index 4843df1ddbb6..67b71cad7b3b 100644 > --- a/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > +++ b/Documentation/devicetree/bindings/gpu/vivante,gc.yaml > @@ -21,6 +21,13 @@ properties: >interrupts: > maxItems: 1 > > + '#cooling-cells': > +const: 2 > + > + assigned-clock-parents: true > + assigned-clock-rates: true > + assigned-clocks: true > + >clocks: > items: >- description: AXI/master interface clock
Re: [PATCH v10 05/30] drm: etnaviv: fix common struct sg_table related issues
On Fr, 2020-09-04 at 15:16 +0200, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski
> Reviewed-by: Robin Murphy
Acked-by: Lucas Stach
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 +---
> drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 15 ---
> 2 files changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index f06e19e7be04..eaf1949bc2e4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -27,7 +27,7 @@ static void etnaviv_gem_scatter_map(struct
> etnaviv_gem_object *etnaviv_obj)
>* because display controller, GPU, etc. are not coherent.
>*/
> if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)
> - dma_map_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
> + dma_map_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
> }
>
> static void etnaviv_gem_scatterlist_unmap(struct etnaviv_gem_object
> *etnaviv_obj)
> @@ -51,7 +51,7 @@ static void etnaviv_gem_scatterlist_unmap(struct
> etnaviv_gem_object *etnaviv_obj
>* discard those writes.
>*/
> if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)
> - dma_unmap_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
> }
>
> /* called with etnaviv_obj->lock held */
> @@ -404,9 +404,8 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32
> op,
> }
>
> if (etnaviv_obj->flags & ETNA_BO_CACHED) {
> - dma_sync_sg_for_cpu(dev->dev, etnaviv_obj->sgt->sgl,
> - etnaviv_obj->sgt->nents,
> - etnaviv_op_to_dma_dir(op));
> + dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
> + etnaviv_op_to_dma_dir(op));
> etnaviv_obj->last_cpu_prep_op = op;
> }
>
> @@ -421,8 +420,7 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj)
> if (etnaviv_obj->flags & ETNA_BO_CACHED) {
> /* fini without a prep is almost certainly a userspace error */
> WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);
> - dma_sync_sg_for_device(dev->dev, etnaviv_obj->sgt->sgl,
> - etnaviv_obj->sgt->nents,
> + dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
> etnaviv_op_to_dma_dir(etnaviv_obj->last_cpu_prep_op));
> etnaviv_obj->last_cpu_prep_op = 0;
> }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> index 3607d348c298..15d9fa3879e5 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c
> @@ -73,13 +73,13 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context
> *context, u32 iova,
>struct sg_table *sgt, unsigned len, int prot)
> {struct scatterlist *sg;
> unsigned int da = iova;
> - unsigned int i, j;
> + unsigned int i;
> int ret;
>
> if (!context || !sgt)
> return -EINVAL;
>
> - for_each_sg(sgt->sgl, sg, sgt->nents, i) {
> + for_each_sgtable_dma_sg(sgt, sg, i) {
> u32 pa = sg_dma_address(sg) - sg->offse
[PATCH] sched/deadline: Fix stale throttling on de-/boosted tasks
When a boosted task gets throttled, what normally happens is that it's
immediately enqueued again with ENQUEUE_REPLENISH, which replenishes the
runtime and clears the dl_throttled flag. There is a special case however:
if the throttling happened on sched-out and the task has been deboosted in
the meantime, the replenish is skipped as the task will return to its
normal scheduling class. This leaves the task with the dl_throttled flag
set.
Now if the task gets boosted up to the deadline scheduling class again
while it is sleeping, it's still in the throttled state. The normal wakeup
however will enqueue the task with ENQUEUE_REPLENISH not set, so we don't
actually place it on the rq. Thus we end up with a task that is runnable,
but not actually on the rq and neither a immediate replenishment happens,
nor is the replenishment timer set up, so the task is stuck in
forever-throttled limbo.
Clear the dl_throttled flag before dropping back to the normal scheduling
class to fix this issue.
Signed-off-by: Lucas Stach
---
This is the root cause and fix of the issue described at [1]. After working
on other stuff for the last few months, I finally was able to circle back
to this issue and gather the required data to pinpoint the failure mode.
[1] https://lkml.org/lkml/2020/3/20/765
---
kernel/sched/deadline.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 3862a28cd05d..c19c1883d695 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1527,12 +1527,15 @@ static void enqueue_task_dl(struct rq *rq, struct
task_struct *p, int flags)
pi_se = _task->dl;
} else if (!dl_prio(p->normal_prio)) {
/*
-* Special case in which we have a !SCHED_DEADLINE task
-* that is going to be deboosted, but exceeds its
-* runtime while doing so. No point in replenishing
-* it, as it's going to return back to its original
-* scheduling class after this.
+* Special case in which we have a !SCHED_DEADLINE task that is
going
+* to be deboosted, but exceeds its runtime while doing so. No
point in
+* replenishing it, as it's going to return back to its original
+* scheduling class after this. If it has been throttled, we
need to
+* clear the flag, otherwise the task may wake up as throttled
after
+* being boosted again with no means to replenish the runtime
and clear
+* the throttle.
*/
+ p->dl.dl_throttled = 0;
BUG_ON(!p->dl.dl_boosted || flags != ENQUEUE_REPLENISH);
return;
}
--
2.20.1
Re: [PATCH v9 0/5] Add support for iMX8MQ Display Controller Subsystem
Hi Laurentiu, On Fr, 2020-08-28 at 11:36 +0300, Laurentiu Palcu wrote: > Hi Lucas, > > I was wondering about the plans to merge this series. Since not many > people can test it properly due to lack of DCSS support in the upstream > NWL driver (which I heard it's coming soon) and a completely nonexistent > HDP driver, are we going to take a leap of faith and merge it as is? Or > should we wait? I think even if the bridges aren't ready yet, the driver is in a good enough shape to merge it. There's no point in letting it accumulate bitrot while waiting for the bridges to land. Also I guess it will make bridge driver development a lot easier when the DCSS driver is in-tree. > As to who is going to do the actual merge, I know we had a brief > discussion about it some time ago and I was supposed to apply for > drm-misc rights, however it feels a little awkward to merge my own > code... :) Though, I might not even qualify for drm-misc rights anyway, > considering I haven't been very active in this area... :/ Please consider applying for drm-misc merge rights. Even if you don't do the initial merge I think it will come in handy in the long run. > On that note, I will probably need help with the merging, provided it's > still happenning. Will you be able to help me out with this? Sure, I'm having some time available this week. I'll give this v9 another spin on my boards and will do the merge after that. Regards, Lucas > Thanks, > laurentiu > > On Fri, Jul 31, 2020 at 11:18:28AM +0300, Laurentiu Palcu wrote: > > From: Laurentiu Palcu > > > > Hi, > > > > This patchset adds initial DCSS support for iMX8MQ chip. Initial support > > includes only graphics plane support (no video planes), no HDR10 > > capabilities, > > no graphics decompression (only linear, tiled and super-tiled buffers > > allowed). > > > > Support for the rest of the features will be added incrementally, in > > subsequent > > patches. > > > > The patchset was tested with both HDP driver (in the downstream tree) and > > the upstream > > MIPI-DSI driver (with a couple of patches on top, to make it work correctly > > with DCSS). > > > > Thanks, > > Laurentiu > > > > Changes in v9: > > * Fixed a compilation issue found by Guido in his setup: 'select > >VIDEOMODE_HELPERS' was missing from Kconfig; > > * Use imx8mq-clock.h in the bindings file so one can understand what > >those clock values mean; > > * no other changes done. Couldn't address the hang Guido reported as > >it's not happening in my setup. However, in my tree, there are some > >extra NWL and ADV patches applied on top of upstream ones... Also, > >removing them and testing only with upstream, even if there's no > >image out, does not produce a hang... :/ > > > > Changes in v8: > > * Removed 'select RESET_CONTROLLER" from Kconfig as Philipp pointed > >out. SRC is not used in DCSS driver; > > * Nothing else changed; > > > > Changes in v7: > > * Added a patch to initialize the connector using the drm_bridge_connector > >API as Sam suggested. Tested it using NWL_DSI and ADV7535 with > >Guido's patch [1] applied and one fix for ADV [2]. Also, some extra > >patches for ADV and NWL were needed, from our downstream tree, which > >will be upstreamed soon by their author; > > * Rest of the patches are untouched; > > > > [1] https://lists.freedesktop.org/archives/dri-devel/2020-July/273025.html > > [2] https://lists.freedesktop.org/archives/dri-devel/2020-July/273132.html > > > > Changes in v6: > > * Addressed Rob's comment and added "additionalProperties: false" at > >the end of the bindings' properties. However, this change surfaced > >an issue with the assigned-clock* properties not being documented in > >the properties section. Added the descriptions and the bindings patch > >will need another review; > > * Added an entry for DCSS driver in the MAINTAINERS file; > > * Removed the component framework patch altogether; > > > > Changes in v5: > > * Rebased to latest; > > * Took out component framework support and made it a separate patch so > >that people can still test with HDP driver, which makes use of it. > >But the idea is to get rid of it once HDP driver's next versions > >will remove component framework as well; > > * Slight improvement to modesetting: avoid cutting off the pixel clock > >if the new mode and the old one are equal. Also, in this case, is > >not necessary to wait for DTG to shut off. This would allow to switch > >from 8b RGB to 12b YUV422, for example, with no interruptions (at least > >from DCSS point of view); > > * Do not fire off CTXLD when going to suspend, unless it still has > >entries that need to be committed to DCSS; > > * Addressed Rob's comments on bindings; > > > > Changes in v4: > > * Addressed Lucas and Philipp's comments: > >* Added DRM_KMS_CMA_HELPER dependency in Kconfig; > >* Removed usage of devm_ functions since I'm
Re: [PATCH] drm/etnaviv: fix external abort seen on GC600 rev 0x19
Hi Russell, Am Sonntag, den 23.08.2020, 20:19 +0100 schrieb Russell King - ARM Linux admin: > On Sun, Aug 23, 2020 at 09:10:25PM +0200, Christian Gmeiner wrote: > > Hi > > > > > I have formally tested the patch with 5.7.10 - and it doesn't resolve > > > the issue - sadly :( > > > > > > From my testing, the reads on > > > VIVS_HI_CHIP_PRODUCT_ID > > > VIVS_HI_CHIP_ECO_ID > > > need to be conditional - while > > > VIVS_HI_CHIP_CUSTOMER_ID > > > seems to be okay. > > > > > > > Uhh.. okay.. just send a V2 - thanks for testing :) > > There is also something else going on with the GC600 - 5.4 worked fine, > 5.8 doesn't - my 2D Xorg driver gets stuck waiting on a BO after just > a couple of minutes. Looking in debugfs, there's a whole load of BOs > that are listed as "active", yet the GPU is idle: > >0002: A 0 ( 7) 8294400 >0001: I 0 ( 1) 4096 >0001: I 0 ( 1) 4096 >0001: I 0 ( 1) 327680 >0001: A 0 ( 7) 8388608 >0001: I 0 ( 1) 8388608 >0001: I 0 ( 1) 8388608 >0001: A 0 ( 7) 8388608 >0001: A 0 ( 3) 8388608 >0001: A 0 ( 4) 8388608 >0001: A 0 ( 3) 8388608 >0001: A 0 ( 3) 8388608 >0001: A 0 ( 3) 8388608 > >0001: A 0 ( 3) 8388608 > Total 38 objects, 293842944 bytes > > My guess is there's something up with the way a job completes that's > causing the BOs not to be marked inactive. I haven't yet been able > to debug any further. The patch I just sent out should fix this issue. The DRM scheduler is doing some funny business which breaks our job done signalling if the GPU timeout has been hit, even if our timeout handler is just extending the timeout as the GPU is still working normally. Regards, Lucas
Re: [PATCH v2] drm/etnaviv: fix external abort seen on GC600 rev 0x19
Am Sonntag, den 23.08.2020, 21:09 +0200 schrieb Christian Gmeiner:
> It looks like that this GPU core triggers an abort when
> reading VIVS_HI_CHIP_PRODUCT_ID and/or VIVS_HI_CHIP_ECO_ID.
>
> I looked at different versions of Vivante's kernel driver and did
> not found anything about this issue or what feature flag can be
> used. So go the simplest route and do not read these two registers
> on the affected GPU core.
>
> Signed-off-by: Christian Gmeiner
> Reported-by: Josua Mayer
> Fixes: 815e45bbd4d3 ("drm/etnaviv: determine product, customer and eco id")
> Cc: sta...@vger.kernel.org
> ---
> Changelog:
>
> V2:
> - use correct register for conditional reads.
Thanks, I applied this patch to my etnaviv/fixes branch.
Regards,
Lucas
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index d5a4cd85a0f6..c6404b8d067f 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -337,9 +337,16 @@ static void etnaviv_hw_identify(struct etnaviv_gpu *gpu)
>
> gpu->identity.model = gpu_read(gpu, VIVS_HI_CHIP_MODEL);
> gpu->identity.revision = gpu_read(gpu, VIVS_HI_CHIP_REV);
> - gpu->identity.product_id = gpu_read(gpu,
> VIVS_HI_CHIP_PRODUCT_ID);
> gpu->identity.customer_id = gpu_read(gpu,
> VIVS_HI_CHIP_CUSTOMER_ID);
> - gpu->identity.eco_id = gpu_read(gpu, VIVS_HI_CHIP_ECO_ID);
> +
> + /*
> + * Reading these two registers on GC600 rev 0x19 result in a
> + * unhandled fault: external abort on non-linefetch
> + */
> + if (!etnaviv_is_model_rev(gpu, GC600, 0x19)) {
> + gpu->identity.product_id = gpu_read(gpu,
> VIVS_HI_CHIP_PRODUCT_ID);
> + gpu->identity.eco_id = gpu_read(gpu,
> VIVS_HI_CHIP_ECO_ID);
> + }
>
> /*
>* HACK ALERT
Re: [PATCH V2] PCI: imx6: Do not output error message when devm_clk_get() failed with -EPROBE_DEFER
Am Dienstag, den 11.08.2020, 09:29 +0800 schrieb Anson Huang:
> When devm_clk_get() returns -EPROBE_DEFER, i.MX6 PCI driver should
> NOT print error message, use dev_err_probe() to handle it.
>
> Signed-off-by: Anson Huang
Reviewed-by: Lucas Stach
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 35
> +++
> 1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 90df28c..e6d6116 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1073,38 +1073,33 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
>
> /* Fetch clocks */
> imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> - if (IS_ERR(imx6_pcie->pcie_phy)) {
> - dev_err(dev, "pcie_phy clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_phy);
> - }
> + if (IS_ERR(imx6_pcie->pcie_phy))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_phy),
> + "pcie_phy clock source missing or
> invalid\n");
>
> imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> - if (IS_ERR(imx6_pcie->pcie_bus)) {
> - dev_err(dev, "pcie_bus clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_bus);
> - }
> + if (IS_ERR(imx6_pcie->pcie_bus))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_bus),
> + "pcie_bus clock source missing or
> invalid\n");
>
> imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> - if (IS_ERR(imx6_pcie->pcie)) {
> - dev_err(dev, "pcie clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie);
> - }
> + if (IS_ERR(imx6_pcie->pcie))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie),
> + "pcie clock source missing or invalid\n");
>
> switch (imx6_pcie->drvdata->variant) {
> case IMX6SX:
> imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> "pcie_inbound_axi");
> - if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> - dev_err(dev, "pcie_inbound_axi clock missing or
> invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_inbound_axi);
> - }
> + if (IS_ERR(imx6_pcie->pcie_inbound_axi))
> + return dev_err_probe(dev,
> PTR_ERR(imx6_pcie->pcie_inbound_axi),
> + "pcie_inbound_axi clock missing or
> invalid\n");
> break;
> case IMX8MQ:
> imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> - if (IS_ERR(imx6_pcie->pcie_aux)) {
> - dev_err(dev, "pcie_aux clock source missing or
> invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_aux);
> - }
> + if (IS_ERR(imx6_pcie->pcie_aux))
> + return dev_err_probe(dev, PTR_ERR(imx6_pcie->pcie_aux),
> + "pcie_aux clock source missing or
> invalid\n");
> /* fall through */
> case IMX7D:
> if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR)
Re: [PATCH] PCI: imx6: Do not output error message when devm_clk_get() failed with -EPROBE_DEFER
Am Dienstag, den 04.08.2020, 13:38 +0800 schrieb Anson Huang:
> When devm_clk_get() returns -EPROBE_DEFER, i.MX6 PCI driver should
> NOT print error message, just return -EPROBE_DEFER is enough.
The reasoning behind this change is fine, but I think we should use the
recently merged dev_err_probe() helper to achieve the same goal.
Regards,
Lucas
> Signed-off-by: Anson Huang
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 30 --
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 4e5c379..ee75d35 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -1076,20 +1076,26 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> /* Fetch clocks */
> imx6_pcie->pcie_phy = devm_clk_get(dev, "pcie_phy");
> if (IS_ERR(imx6_pcie->pcie_phy)) {
> - dev_err(dev, "pcie_phy clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_phy);
> + ret = PTR_ERR(imx6_pcie->pcie_phy);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "pcie_phy clock source missing or
> invalid\n");
> + return ret;
> }
>
> imx6_pcie->pcie_bus = devm_clk_get(dev, "pcie_bus");
> if (IS_ERR(imx6_pcie->pcie_bus)) {
> - dev_err(dev, "pcie_bus clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_bus);
> + ret = PTR_ERR(imx6_pcie->pcie_bus);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "pcie_bus clock source missing or
> invalid\n");
> + return ret;
> }
>
> imx6_pcie->pcie = devm_clk_get(dev, "pcie");
> if (IS_ERR(imx6_pcie->pcie)) {
> - dev_err(dev, "pcie clock source missing or invalid\n");
> - return PTR_ERR(imx6_pcie->pcie);
> + ret = PTR_ERR(imx6_pcie->pcie);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "pcie clock source missing or invalid\n");
> + return ret;
> }
>
> switch (imx6_pcie->drvdata->variant) {
> @@ -1097,15 +1103,19 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> imx6_pcie->pcie_inbound_axi = devm_clk_get(dev,
> "pcie_inbound_axi");
> if (IS_ERR(imx6_pcie->pcie_inbound_axi)) {
> - dev_err(dev, "pcie_inbound_axi clock missing or
> invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_inbound_axi);
> + ret = PTR_ERR(imx6_pcie->pcie_inbound_axi);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "pcie_inbound_axi clock missing or
> invalid\n");
> + return ret;
> }
> break;
> case IMX8MQ:
> imx6_pcie->pcie_aux = devm_clk_get(dev, "pcie_aux");
> if (IS_ERR(imx6_pcie->pcie_aux)) {
> - dev_err(dev, "pcie_aux clock source missing or
> invalid\n");
> - return PTR_ERR(imx6_pcie->pcie_aux);
> + ret = PTR_ERR(imx6_pcie->pcie_aux);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "pcie_aux clock source missing or
> invalid\n");
> + return ret;
> }
> /* fall through */
> case IMX7D:
Re: [PATCH 1/2] ARM: dts: imx6qp-sabresd: enable pcie
Am Dienstag, den 21.07.2020, 15:44 +0800 schrieb Richard Zhu:
> Add one regulator, used to power up the external oscillator,
> and enable PCIe on iMX6QP SABRESD board.
That's not the right thing to do. If there is an external oscillator,
which requires a power supply then the oscillator should have its own
clock DT node (it's a separate device after all) and this node needs to
control the regulator.
This has nothing to do with the PCIe controller, which only cares about
the clock being provided.
Regards,
Lucas
> Signed-off-by: Richard Zhu
> ---
> arch/arm/boot/dts/imx6qp-sabresd.dts | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/imx6qp-sabresd.dts
> b/arch/arm/boot/dts/imx6qp-sabresd.dts
> index 480e73183f6b..cd8a1f610427 100644
> --- a/arch/arm/boot/dts/imx6qp-sabresd.dts
> +++ b/arch/arm/boot/dts/imx6qp-sabresd.dts
> @@ -51,7 +51,8 @@
> };
>
> {
> - status = "disabled";
> + vepdev-supply = <_reg>;
> + status = "okay";
> };
>
> {
Re: [PATCH v5 0/4] Add support for iMX8MQ Display Controller Subsystem
Am Freitag, den 17.07.2020, 12:45 +0300 schrieb Laurentiu Palcu: > Hi Lukas and Daniel, > > On Fri, Jul 17, 2020 at 11:27:58AM +0200, Daniel Vetter wrote: > > On Fri, Jul 17, 2020 at 11:12:39AM +0200, Lucas Stach wrote: > > > Am Freitag, den 17.07.2020, 10:59 +0200 schrieb Daniel Vetter: > > > > On Fri, Jul 17, 2020 at 10:18 AM Lucas Stach > > > > wrote: > > > > > Hi Laurentiu, > > > > > > > > > > Am Donnerstag, den 09.07.2020, 19:47 +0300 schrieb Laurentiu Palcu: > > > > > > From: Laurentiu Palcu > > > > > > > > > > > > Hi, > > > > > > > > > > > > This patchset adds initial DCSS support for iMX8MQ chip. Initial > > > > > > support > > > > > > includes only graphics plane support (no video planes), no HDR10 > > > > > > capabilities, > > > > > > no graphics decompression (only linear, tiled and super-tiled > > > > > > buffers allowed). > > > > > > > > > > > > Support for the rest of the features will be added incrementally, > > > > > > in subsequent > > > > > > patches. > > > > > > > > > > > > The patchset was tested with both HDP driver (in the downstream > > > > > > tree) and the upstream > > > > > > MIPI-DSI driver (with a couple of patches on top, to make it work > > > > > > correctly with DCSS). > > > > > > > > > > I think the series (minus 3/5 and minor correction to the DT binding) > > > > > is fine to go in now. So just some formal questions: are you going to > > > > > maintain this driver in upstream? If so we should add a MAINTAINERS > > > > > entry to that effect. I can offer to act as a reviewer in this case. > > I can maintain the DCSS driver, sure, and the more reviewers the better. > Thanks for helping out with this. Should I send a v6 then with a patch > for MAINTAINERS? > > > > > > How do you intend to merge this? IMO pushing this through drm-misc > > > > > seems like the right thing to do. If you agree I can help you get this > > > > > applied. If you are going to maintain the driver on your own, I think > > > > > you should then apply for commit rights to drm-misc. > > > > > > > > drm/imx isn't listed yet as under the drm-misc umbrella, maybe we > > > > should put the entire collective of imx drivers under drm-misc? Or > > > > maybe it's just an oversight that the git repo isn't specified in the > > > > MAINTAINERS entry. Also maybe we should add the pengutronix kernel > > > > team alias there too? > > > > > > drm/imx was exclusively the IPUv3 up until now, which is in fact > > > maintained outside of drm-misc in its own git tree. This has worked > > > quite well in the past so even though IPUv3 doesn't see a lot of churn > > > these days the motivation to change anything to this workflow is quite > > > low. And yes, the git tree is missing from the MAINTAINERS entry. > > > > > > For the DCSS driver, if it's going to be maintained by NXP, I figured > > > it might be easier for Laurentiu to push things into drm-misc than set > > > up a separate public git tree. But IMHO that's fully up to him to > > > decide. > > > > /me puts on maintainer hat > > > > Much prefer drm-misc over random people playing maintainer and fumbling > > it. I think the reasonable options are either in the current imx tree, or > > drm-misc. Standalone tree for these small drivers just doesn't make much > > sense. > > I don't have anything against either method, though I have to agree I > like things to be simple. Going through drm-misc sounds simple enough to me. > :) > However, since there is going to be more activity in the DRM IMX area in > the future, reviving the drm/imx tree, and push all IMX related stuff > through drm/imx, could make sense as well. I think drm-misc is the right place then. Please send a v6 with the following changes: - drop the component framework patch - drop the i.MX8MQ DT patch, this should go through Shawn's imx tree after the driver and binding has landed in drm-misc - you can add my Reviewed-by to the whole series or I can add it when applying - add a MAINTAINERS entry, please add me as a reviewer if you don't mind I can push this initial series into drm-misc until you've got your own commit rights. Regards, Lucas
Re: [PATCH v5 0/4] Add support for iMX8MQ Display Controller Subsystem
Am Freitag, den 17.07.2020, 10:59 +0200 schrieb Daniel Vetter: > On Fri, Jul 17, 2020 at 10:18 AM Lucas Stach wrote: > > Hi Laurentiu, > > > > Am Donnerstag, den 09.07.2020, 19:47 +0300 schrieb Laurentiu Palcu: > > > From: Laurentiu Palcu > > > > > > Hi, > > > > > > This patchset adds initial DCSS support for iMX8MQ chip. Initial support > > > includes only graphics plane support (no video planes), no HDR10 > > > capabilities, > > > no graphics decompression (only linear, tiled and super-tiled buffers > > > allowed). > > > > > > Support for the rest of the features will be added incrementally, in > > > subsequent > > > patches. > > > > > > The patchset was tested with both HDP driver (in the downstream tree) and > > > the upstream > > > MIPI-DSI driver (with a couple of patches on top, to make it work > > > correctly with DCSS). > > > > I think the series (minus 3/5 and minor correction to the DT binding) > > is fine to go in now. So just some formal questions: are you going to > > maintain this driver in upstream? If so we should add a MAINTAINERS > > entry to that effect. I can offer to act as a reviewer in this case. > > > > How do you intend to merge this? IMO pushing this through drm-misc > > seems like the right thing to do. If you agree I can help you get this > > applied. If you are going to maintain the driver on your own, I think > > you should then apply for commit rights to drm-misc. > > drm/imx isn't listed yet as under the drm-misc umbrella, maybe we > should put the entire collective of imx drivers under drm-misc? Or > maybe it's just an oversight that the git repo isn't specified in the > MAINTAINERS entry. Also maybe we should add the pengutronix kernel > team alias there too? drm/imx was exclusively the IPUv3 up until now, which is in fact maintained outside of drm-misc in its own git tree. This has worked quite well in the past so even though IPUv3 doesn't see a lot of churn these days the motivation to change anything to this workflow is quite low. And yes, the git tree is missing from the MAINTAINERS entry. For the DCSS driver, if it's going to be maintained by NXP, I figured it might be easier for Laurentiu to push things into drm-misc than set up a separate public git tree. But IMHO that's fully up to him to decide. Regards, Lucas > -Daniel > > > > Regards, > > Lucas > > > > > Thanks, > > > Laurentiu > > > > > > Changes in v5: > > > * Rebased to latest; > > > * Took out component framework support and made it a separate patch so > > >that people can still test with HDP driver, which makes use of it. > > >But the idea is to get rid of it once HDP driver's next versions > > >will remove component framework as well; > > > * Slight improvement to modesetting: avoid cutting off the pixel clock > > >if the new mode and the old one are equal. Also, in this case, is > > >not necessary to wait for DTG to shut off. This would allow to switch > > >from 8b RGB to 12b YUV422, for example, with no interruptions (at least > > >from DCSS point of view); > > > * Do not fire off CTXLD when going to suspend, unless it still has > > >entries that need to be committed to DCSS; > > > * Addressed Rob's comments on bindings; > > > > > > Changes in v4: > > > * Addressed Lucas and Philipp's comments: > > >* Added DRM_KMS_CMA_HELPER dependency in Kconfig; > > >* Removed usage of devm_ functions since I'm already doing all the > > > clean-up in the submodules_deinit(); > > >* Moved the drm_crtc_arm_vblank_event() in dcss_crtc_atomic_flush(); > > >* Removed en_completion variable from dcss_crtc since this was > > > introduced mainly to avoid vblank timeout warnings which were fixed > > > by arming the vblank event in flush() instead of begin(); > > >* Removed clks_on and irq_enabled flags since all the calls to > > > enabling/disabling clocks and interrupts were balanced; > > >* Removed the custom atomic_commit callback and used the DRM core > > > helper and, in the process, got rid of a workqueue that wasn't > > > necessary anymore; > > >* Fixed some minor DT binding issues flagged by Philipp; > > >* Some other minor changes suggested by Lucas; > > > * Removed YUV formats from the supported formats as these cannot work > > >without the HDR10 module CSCs and LUTs. Will add them back
Re: [PATCH v5 0/4] Add support for iMX8MQ Display Controller Subsystem
Hi Laurentiu, Am Donnerstag, den 09.07.2020, 19:47 +0300 schrieb Laurentiu Palcu: > From: Laurentiu Palcu > > Hi, > > This patchset adds initial DCSS support for iMX8MQ chip. Initial support > includes only graphics plane support (no video planes), no HDR10 capabilities, > no graphics decompression (only linear, tiled and super-tiled buffers > allowed). > > Support for the rest of the features will be added incrementally, in > subsequent > patches. > > The patchset was tested with both HDP driver (in the downstream tree) and the > upstream > MIPI-DSI driver (with a couple of patches on top, to make it work correctly > with DCSS). I think the series (minus 3/5 and minor correction to the DT binding) is fine to go in now. So just some formal questions: are you going to maintain this driver in upstream? If so we should add a MAINTAINERS entry to that effect. I can offer to act as a reviewer in this case. How do you intend to merge this? IMO pushing this through drm-misc seems like the right thing to do. If you agree I can help you get this applied. If you are going to maintain the driver on your own, I think you should then apply for commit rights to drm-misc. Regards, Lucas > Thanks, > Laurentiu > > Changes in v5: > * Rebased to latest; > * Took out component framework support and made it a separate patch so >that people can still test with HDP driver, which makes use of it. >But the idea is to get rid of it once HDP driver's next versions >will remove component framework as well; > * Slight improvement to modesetting: avoid cutting off the pixel clock >if the new mode and the old one are equal. Also, in this case, is >not necessary to wait for DTG to shut off. This would allow to switch >from 8b RGB to 12b YUV422, for example, with no interruptions (at least >from DCSS point of view); > * Do not fire off CTXLD when going to suspend, unless it still has >entries that need to be committed to DCSS; > * Addressed Rob's comments on bindings; > > Changes in v4: > * Addressed Lucas and Philipp's comments: >* Added DRM_KMS_CMA_HELPER dependency in Kconfig; >* Removed usage of devm_ functions since I'm already doing all the > clean-up in the submodules_deinit(); >* Moved the drm_crtc_arm_vblank_event() in dcss_crtc_atomic_flush(); >* Removed en_completion variable from dcss_crtc since this was > introduced mainly to avoid vblank timeout warnings which were fixed > by arming the vblank event in flush() instead of begin(); >* Removed clks_on and irq_enabled flags since all the calls to > enabling/disabling clocks and interrupts were balanced; >* Removed the custom atomic_commit callback and used the DRM core > helper and, in the process, got rid of a workqueue that wasn't > necessary anymore; >* Fixed some minor DT binding issues flagged by Philipp; >* Some other minor changes suggested by Lucas; > * Removed YUV formats from the supported formats as these cannot work >without the HDR10 module CSCs and LUTs. Will add them back when I >will add support for video planes; > > Changes in v3: > * rebased to latest linux-next and made it compile as drmP.h was >removed; > * removed the patch adding the VIDEO2_PLL clock. It's already applied; > * removed an unnecessary 50ms sleep in the dcss_dtg_sync_set(); > * fixed a a spurious hang reported by Lukas Hartmann and encountered >by me several times; > * mask DPR and DTG interrupts by default, as they may come enabled from >U-boot; > > Changes in v2: > * Removed '0x' in node's unit-address both in DT and yaml; > * Made the address region size lowercase, to be consistent; > * Removed some left-over references to P010; > * Added a Kconfig dependency of DRM && ARCH_MXC. This will also silence > compilation >issues reported by kbuild for other architectures; > > Laurentiu Palcu (5): > drm/imx: compile imx directory by default > drm/imx: Add initial support for DCSS on iMX8MQ > drm/imx/dcss: add component framework functionality > dt-bindings: display: imx: add bindings for DCSS > arm64: dts: imx8mq: add DCSS node > > .../bindings/display/imx/nxp,imx8mq-dcss.yaml | 84 ++ > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 23 + > drivers/gpu/drm/Makefile | 2 +- > drivers/gpu/drm/imx/Kconfig | 2 + > drivers/gpu/drm/imx/Makefile | 1 + > drivers/gpu/drm/imx/dcss/Kconfig | 9 + > drivers/gpu/drm/imx/dcss/Makefile | 6 + > drivers/gpu/drm/imx/dcss/dcss-blkctl.c| 70 ++ > drivers/gpu/drm/imx/dcss/dcss-crtc.c | 219 + > drivers/gpu/drm/imx/dcss/dcss-ctxld.c | 424 + > drivers/gpu/drm/imx/dcss/dcss-dev.c | 314 +++ > drivers/gpu/drm/imx/dcss/dcss-dev.h | 177 > drivers/gpu/drm/imx/dcss/dcss-dpr.c | 562 > drivers/gpu/drm/imx/dcss/dcss-drv.c
Re: [PATCH 0/4] Add support for GPU load values
Hi Christian, Am Freitag, den 10.07.2020, 09:41 +0200 schrieb Christian Gmeiner: > This patch series add support for loadavg values for GPU > sub-components. I am adding a SMA algorithm as I was not > really sure if EWMA would be a good fit for this use case. 1 second is a pretty long window in GPU time. Why do you feel that a simple moving average is more appropriate than a exponentially weighted one here? Note that I haven't given this any thought myself and haven't made up my mind yet, so this is a honest question to understand the reasoning behind your choice. Regards, Lucas > Christian Gmeiner (4): > drm/etnaviv: add simple moving average (SMA) > drm/etnaviv: add loadavg accounting > drm/etnaviv: show loadavg in debugfs > drm/etnaviv: export loadavg via perfmon > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 14 > drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 44 - > drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 29 + > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 79 +++ > drivers/gpu/drm/etnaviv/etnaviv_sma.h | 53 +++ > 5 files changed, 218 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_sma.h >
Re: [PATCH 2/4] drm/etnaviv: add loadavg accounting
Hi Christian,
Am Freitag, den 10.07.2020, 09:41 +0200 schrieb Christian Gmeiner:
> The GPU has an idle state register where each bit represents the idle
> state of a sub-GPU component like FE or TX. Sample this register
> every 10ms and calculate a simple moving average over the sub-GPU
> component idle states with a total observation time frame of 1s.
>
> This provides us with a percentage based load of each sub-GPU
> component.
>
> Signed-off-by: Christian Gmeiner
> ---
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 14
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 32 +++
> drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 29
> 3 files changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index f9afe11c50f0..b31920241c86 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -46,6 +46,19 @@ static void load_gpu(struct drm_device *dev)
> }
> }
>
> +static void unload_gpu(struct drm_device *dev)
> +{
> + struct etnaviv_drm_private *priv = dev->dev_private;
> + unsigned int i;
> +
> + for (i = 0; i < ETNA_MAX_PIPES; i++) {
> + struct etnaviv_gpu *g = priv->gpu[i];
> +
> + if (g)
> + etnaviv_gpu_shutdown(g);
> + }
> +}
> +
> static int etnaviv_open(struct drm_device *dev, struct drm_file *file)
> {
> struct etnaviv_drm_private *priv = dev->dev_private;
> @@ -581,6 +594,7 @@ static void etnaviv_unbind(struct device *dev)
> struct drm_device *drm = dev_get_drvdata(dev);
> struct etnaviv_drm_private *priv = drm->dev_private;
>
> + unload_gpu(drm);
> drm_dev_unregister(drm);
>
> component_unbind_all(dev, drm);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index a31eeff2b297..1f0eb7e00657 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -714,6 +714,28 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
> gpu_write(gpu, VIVS_HI_INTR_ENBL, ~0U);
> }
>
> +static void etnaviv_loadavg_function(struct timer_list *t)
> +{
> + struct etnaviv_gpu *gpu = from_timer(gpu, t, loadavg_timer);
> + const u32 idle = gpu_read(gpu, VIVS_HI_IDLE_STATE);
This isn't guaranteed to work on a clock/power gated GPU. Also we
surely don't want to wake a idle system every 10ms just to sample a "no
load" value, so this needs some integration with runtime PM, to disable
the sampling when the GPU is powered down and enable when powered up.
The loadavg must be able to adapt to jumps in the sampling interval
while idle.
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(etna_idle_module_names); i++)
> + if ((idle & etna_idle_module_names[i].bit))
> + sma_loadavg_add(>loadavg_value[i], 0);
> + else
> + sma_loadavg_add(>loadavg_value[i], 100);
> +
> + spin_lock_bh(>loadavg_spinlock);
> +
> + for (i = 0; i < ARRAY_SIZE(etna_idle_module_names); i++)
> + gpu->loadavg_percentage[i] =
> sma_loadavg_read(>loadavg_value[i]);
> +
> + spin_unlock_bh(>loadavg_spinlock);
> +
> + mod_timer(t, jiffies + msecs_to_jiffies(10));
A jiffies based timer is much too coarse for a regular 10ms sampling.
On a typical 100Hz system 10ms is a single jiffy, so your timer will
fire anywhere in the range of ~0ms...~20ms. This won't get us a usable
measurement.
Regards,
Lucas
> +}
> +
> int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> {
> struct etnaviv_drm_private *priv = gpu->drm->dev_private;
> @@ -804,6 +826,10 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> for (i = 0; i < ARRAY_SIZE(gpu->event); i++)
> complete(>event_free);
>
> + /* Setup loadavg timer */
> + timer_setup(>loadavg_timer, etnaviv_loadavg_function, 0);
> + mod_timer(>loadavg_timer, jiffies + msecs_to_jiffies(10));
> +
> /* Now program the hardware */
> mutex_lock(>lock);
> etnaviv_gpu_hw_init(gpu);
> @@ -824,6 +850,11 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> return ret;
> }
>
> +void etnaviv_gpu_shutdown(struct etnaviv_gpu *gpu)
> +{
> + del_timer(>loadavg_timer);
> +}
> +
> #ifdef CONFIG_DEBUG_FS
> struct dma_debug {
> u32 address[2];
> @@ -1762,6 +1793,7 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> gpu->dev = >dev;
> mutex_init(>lock);
> mutex_init(>fence_lock);
> + spin_lock_init(>loadavg_spinlock);
>
> /* Map registers: */
> gpu->mmio = devm_platform_ioremap_resource(pdev, 0);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 8ea48697d132..a5b9c89c6744 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -10,6 +10,8 @@
> #include
Re: [RESEND PATCH v2 0/4] drm/etnaviv: Tidy up clocks handling
Am Dienstag, den 16.06.2020, 23:21 +0200 schrieb Lubomir Rintel: > Hi, > > please consider applying patches that are chained to this message. Thanks, I've applied all of them to etnaviv/next. Regards, Lucas > They make getting/enabling the clocks in the etnaviv driver slightly nicer, > first two also fix potential problems. > > Compared to v1, patch 2/4 was fixed and patch 3/4 was added. > > As it was pointed out in response to v1, the clocks documented as > mandatory by the binding document are different from what the driver > enforces. Moreover, there is no agreement on which clocks must be > present in the device tree, so I'm leaving the binding document until > it's cleared up. > > In any case, the "core" clock is always present so it's safe to make it > mandatory and regardless of what ends up happening to the binding > documentation, the other clocks can't be enforced without regressions. > At most a comment or a warning could be added. I'm leaving it as it is. > > Thank you > Lubo > > > ___ > etnaviv mailing list > etna...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/etnaviv
Re: [PATCH v2] drm/etnaviv: convert get_user_pages() --> pin_user_pages()
Am Montag, den 25.05.2020, 16:49 -0700 schrieb John Hubbard:
> This code was using get_user_pages*(), in a "Case 2" scenario
> (DMA/RDMA), using the categorization from [1]. That means that it's
> time to convert the get_user_pages*() + put_page() calls to
> pin_user_pages*() + unpin_user_pages() calls.
>
> There is some helpful background in [2]: basically, this is a small
> part of fixing a long-standing disconnect between pinning pages, and
> file systems' use of those pages.
>
> [1] Documentation/core-api/pin_user_pages.rst
>
> [2] "Explicit pinning of user-space pages":
> https://lwn.net/Articles/807108/
>
> Signed-off-by: John Hubbard
Thanks, I've applied this to etnaviv/next.
Regards,
Lucas
> ---
>
> Hi,
>
> Changes since v1:
>
> * Rebased onto Linux 5.7-rc7
>
> * Added: Lucas Stach
>
> thanks
> John Hubbard
> NVIDIA
>
>
> drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index dc9ef302f517..0f4578dc169d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -675,10 +675,10 @@ static int etnaviv_gem_userptr_get_pages(struct
> etnaviv_gem_object *etnaviv_obj)
> uint64_t ptr = userptr->ptr + pinned * PAGE_SIZE;
> struct page **pages = pvec + pinned;
>
> - ret = get_user_pages_fast(ptr, num_pages,
> + ret = pin_user_pages_fast(ptr, num_pages,
> !userptr->ro ? FOLL_WRITE : 0, pages);
> if (ret < 0) {
> - release_pages(pvec, pinned);
> + unpin_user_pages(pvec, pinned);
> kvfree(pvec);
> return ret;
> }
> @@ -702,7 +702,7 @@ static void etnaviv_gem_userptr_release(struct
> etnaviv_gem_object *etnaviv_obj)
> if (etnaviv_obj->pages) {
> int npages = etnaviv_obj->base.size >> PAGE_SHIFT;
>
> - release_pages(etnaviv_obj->pages, npages);
> + unpin_user_pages(etnaviv_obj->pages, npages);
> kvfree(etnaviv_obj->pages);
> }
> }
Re: [PATCH v2] drm/etnaviv: fix ref count leak via pm_runtime_get_sync
Hi Navid,
Am Montag, den 15.06.2020, 01:12 -0500 schrieb Navid Emamdoost:
> in etnaviv_gpu_submit, etnaviv_gpu_recover_hang, etnaviv_gpu_debugfs,
> and etnaviv_gpu_init the call to pm_runtime_get_sync increments the
> counter even in case of failure, leading to incorrect ref count.
> In case of failure, decrement the ref count before returning.
While that change is correct with the current API, may I ask the
question why the way this API works is considered reasonable? A API
call that fails, but still changes internal state and expects the
caller to clean up the mess it not really what I would consider fool-
proof API design. Is there a specific reason why it is done this way
and not handled internally?
Regards,
Lucas
> Signed-off-by: Navid Emamdoost
> ---
> Changes in v2:
> - replace pm_runtime_put with pm_runtime_put_noidle
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 14 ++
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index a31eeff2b297..7c9f3f9ba123 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -722,7 +722,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> ret = pm_runtime_get_sync(gpu->dev);
> if (ret < 0) {
> dev_err(gpu->dev, "Failed to enable GPU power domain\n");
> - return ret;
> + goto pm_put;
> }
>
> etnaviv_hw_identify(gpu);
> @@ -819,6 +819,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
>
> fail:
> pm_runtime_mark_last_busy(gpu->dev);
> +pm_put:
> pm_runtime_put_autosuspend(gpu->dev);
>
> return ret;
> @@ -859,7 +860,7 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct
> seq_file *m)
>
> ret = pm_runtime_get_sync(gpu->dev);
> if (ret < 0)
> - return ret;
> + goto pm_put;
>
> dma_lo = gpu_read(gpu, VIVS_FE_DMA_LOW);
> dma_hi = gpu_read(gpu, VIVS_FE_DMA_HIGH);
> @@ -1003,6 +1004,7 @@ int etnaviv_gpu_debugfs(struct etnaviv_gpu *gpu, struct
> seq_file *m)
> ret = 0;
>
> pm_runtime_mark_last_busy(gpu->dev);
> +pm_put:
> pm_runtime_put_autosuspend(gpu->dev);
>
> return ret;
> @@ -1016,7 +1018,7 @@ void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu)
> dev_err(gpu->dev, "recover hung GPU!\n");
>
> if (pm_runtime_get_sync(gpu->dev) < 0)
> - return;
> + goto pm_put;
>
> mutex_lock(>lock);
>
> @@ -1035,6 +1037,7 @@ void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu)
>
> mutex_unlock(>lock);
> pm_runtime_mark_last_busy(gpu->dev);
> +pm_put:
> pm_runtime_put_autosuspend(gpu->dev);
> }
>
> @@ -1308,8 +1311,10 @@ struct dma_fence *etnaviv_gpu_submit(struct
> etnaviv_gem_submit *submit)
>
> if (!submit->runtime_resumed) {
> ret = pm_runtime_get_sync(gpu->dev);
> - if (ret < 0)
> + if (ret < 0) {
> + pm_runtime_put_noidle(gpu->dev);
> return NULL;
> + }
> submit->runtime_resumed = true;
> }
>
> @@ -1326,6 +1331,7 @@ struct dma_fence *etnaviv_gpu_submit(struct
> etnaviv_gem_submit *submit)
> ret = event_alloc(gpu, nr_events, event);
> if (ret) {
> DRM_ERROR("no free events\n");
> + pm_runtime_put_noidle(gpu->dev);
> return NULL;
> }
>
Re: [PATCH v5 0/2] mfd: Add ENE KB3930 Embedded Controller driver
Hi Lubomir, Am Mittwoch, den 17.06.2020, 00:44 +0200 schrieb Lubomir Rintel: > Hi, > > please consider applying the patches chained to this message. It's the > fifth version of the driver for the ENE KB3930 Embedded Controller. > > This version is essentially a resend of v4. The only actual change is the > addition of the Rob's Reviewed-by tag which I failed to do previously. > Detailed change logs are in the individual patch descriptions. I don't think you wanted this to go to the etnaviv list and maintainers, right? Regards, Lucas
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Mittwoch, den 20.05.2020, 15:38 +0200 schrieb Lubomir Rintel: > On Thu, May 14, 2020 at 09:53:08AM +0100, Russell King - ARM Linux admin > wrote: > > On Thu, May 14, 2020 at 10:40:58AM +0200, Lucas Stach wrote: > > > Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM > > > Linux admin: > > > > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > > > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam > > > > > > wrote: > > > > > > > > > > > > > The binding doc > > > > > > > Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > > > > says that only the 'reg' clock could be optional, the others are > > > > > > > required. > > > > > > > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > > > > > > > Maybe the binding needs to be updated and it seems that using > > > > > > devm_clk_get_optional() like you propose is safe. > > > > > > > > > > The binding is correct as-is. We want to require those clocks to be > > > > > present, but the dove DT was added before the binding was finalized, > > > > > so > > > > > the driver still treats the clocks as optional to not break > > > > > compatibility with old DTs. Maybe this warrants a comment in the > > > > > code... > > > > > > > > The binding doc in mainline says: > > > > > > > > clocks: > > > > items: > > > > - description: AXI/master interface clock > > > > - description: GPU core clock > > > > - description: Shader clock (only required if GPU has feature > > > > PIPE_3D) > > > > - description: AHB/slave interface clock (only required if GPU > > > > can gate slave interface independently) > > > > minItems: 1 > > > > maxItems: 4 > > > > > > > > clock-names: > > > > items: > > > > enum: [ bus, core, shader, reg ] > > > > minItems: 1 > > > > maxItems: 4 > > > > > > > > which looks correct to me - and means that Dove is compliant with that. > > > > > > The YAML binding actually did loose something in translation here, > > > which I didn't notice. Previously all those clocks were listed under > > > "Required properties", with the exceptions listed in parenthesis. So > > > the Dove GPU, which is a combined 2D/3D core should have axi, core and > > > shader clocks specified. > > > > That may be your desire, but that is impossible without knowing that > > (a) it has the clocks > > (b) what those clocks are connected to > > > > I guess we could "make something up" but as DT is supposed to describe > > hardware, I don't see how we can satisfy that and your requirement. > > > > The only thing that is known from the documentation is that there is > > one clock for the GPU on Dove. > > Yes. This means that in fact "core" is the only required clock for all > implementations of vivante,gc and the common binding needs to be updated > to reflect that. I'll follow with a patch that does that, unless there > are strong objections. > > If there are implementations that require different clock inputs, then they > need to use additional compatible string for the particular flavor and the > binding should have conditionals for them. Something like this: > > if: > properties: > compatible: > contains: > const: fsl,imx6sx-gpu > then: > properties: > clocks: > minItems: 4 The DT binding of a device should describe the hardware of the device, not the specific integration into a SoC. Now it's a bit hard to make any definite statements about the Vivante GC GPU module itself, as most of the information we have is from reverse engineering. It's pretty clear though that the GPU module has at least 2 clock inputs: axi and core, as there is a feature bit that tells us if it's okay to gate the axi clock independently from core. I'm not 100% sure about the older cores as found in Dove, but all the more recent cores allow to clock the shader partition independently of the core partition, so that's another clock input. Now when it comes to a SoC integration, it's totally fine to have all those GPU module clock inputs fed from the same clock source and behind a shared gate maybe. But that doesn't change the clock inputs from the device perspective, it's still 3 independent clock inputs, which then just point to the same clock source in the DT. imx6sx.dtsi is even a precedent of such a setup: all module clock inputs are fed by a common clock and share a single gate. Regards, Lucas
Re: [PATCH] drm/etnaviv: fix memory leak when mapping prime imported buffers
Hi Martin,
Am Mittwoch, den 20.05.2020, 12:10 +0200 schrieb Martin Fuzzey:
> When using mmap() on a prime imported buffer allocated by a
> different driver (such as imx-drm) the later munmap() does
> not correctly decrement the refcount of the original enaviv_gem_object,
> leading to a leak.
>
> Signed-off-by: Martin Fuzzey
> Cc: sta...@vger.kernel.org
What's the use-case where you did hit this issue? mmap'ing of imported
buffers through the etnaviv DRM device is currently not well defined
and I was pondering the idea of forbidding it completely by not
returning a mmap offset for those objects.
Regards,
Lucas
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> index f24dd21..28a01b8 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
> @@ -93,7 +93,25 @@ static void *etnaviv_gem_prime_vmap_impl(struct
> etnaviv_gem_object *etnaviv_obj)
> static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
> struct vm_area_struct *vma)
> {
> - return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
> + int ret;
> +
> + ret = dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
> +
> + /* drm_gem_mmap_obj() has already been called before this function
> + * and has incremented our refcount, expecting it to be decremented
> + * on unmap() via drm_gem_vm_close().
> + * However dma_buf_mmap() invokes drm_gem_cma_prime_mmap()
> + * that ends up updating the vma->vma_private_data to point to the
> + * dma_buf's gem object.
> + * Hence our gem object here will not have its refcount decremented
> + * when userspace does unmap().
> + * So decrement the refcount here to avoid a memory leak if the dma
> + * buf mapping was successful.
> + */
> + if (!ret)
> + drm_gem_object_put_unlocked(_obj->base);
> +
> + return ret;
> }
>
> static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang: > Hi > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach wrote: > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang: > > > There are two requirements that we need to move the request > > > of dma channel from probe to open. > > > > How do you handle -EPROBE_DEFER return code from the channel request if > > you don't do it in probe? > > I use the dma_request_slave_channel or dma_request_channel instead > of dmaengine_pcm_request_chan_of. so there should be not -EPROBE_DEFER > return code. This is a pretty weak argument. The dmaengine device might probe after you try to get the channel. Using a function to request the channel that doesn't allow you to handle probe deferral is IMHO a bug and should be fixed, instead of building even more assumptions on top of it. > > > - When dma device binds with power-domains, the power will > > > be enabled when we request dma channel. If the request of dma > > > channel happen on probe, then the power-domains will be always > > > enabled after kernel boot up, which is not good for power > > > saving, so we need to move the request of dma channel to .open(); > > > > This is certainly something which could be fixed in the dmaengine > > driver. > > Dma driver always call the pm_runtime_get_sync in > device_alloc_chan_resources, the device_alloc_chan_resources is > called when channel is requested. so power is enabled on channel > request. So why can't you fix the dmaengine driver to do that RPM call at a later time when the channel is actually going to be used? This will allow further power savings with other slave devices than the audio PCM. Regards, Lucas > > > - With FE-BE case, if the dma channel is requested in probe, > > > then there will be below issue, which is caused by that the > > > dma channel will be requested duplicately > > > > Why is this requested a second time? Is this just some missing cleanup > > on a deferred probe path? > > Not relate with deferred probe. With DMA1->ASRC->DMA2->ESAI case, > the DMA1->ASRC->DMA2 is in FE, ESAI is in BE. When ESAI drvier > probe, DMA3 channel is created with ESAI's "dma:tx" (DMA3 channel > is not used in this FE-BE case).When FE-BE startup, DMA2 > channel is created, it needs the ESAI's "dma:tx", so below warning > comes out. > > > Regards, > > Lucas > > > > > [ 638.906268] sysfs: cannot create duplicate filename > > > '/devices/soc0/soc/200.bus/200.spba-bus/2024000.esai/dma:tx' > > > [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted > > > 5.7.0-rc1-12956-gfc64b2585593 #287 > > > [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) > > > [ 638.933690] [] (unwind_backtrace) from [] > > > (show_stack+0x10/0x14) > > > [ 638.941464] [] (show_stack) from [] > > > (dump_stack+0xe4/0x118) > > > [ 638.948808] [] (dump_stack) from [] > > > (sysfs_warn_dup+0x50/0x64) > > > [ 638.956406] [] (sysfs_warn_dup) from [] > > > (sysfs_do_create_link_sd+0xc8/0xd4) > > > [ 638.965134] [] (sysfs_do_create_link_sd) from [] > > > (dma_request_chan+0xb0/0x210) > > > [ 638.974120] [] (dma_request_chan) from [] > > > (dma_request_slave_channel+0x8/0x14) > > > [ 638.983111] [] (dma_request_slave_channel) from [] > > > (fsl_asrc_dma_hw_params+0x1e0/0x438) > > > [ 638.992881] [] (fsl_asrc_dma_hw_params) from [] > > > (soc_pcm_hw_params+0x4a0/0x6a8) > > > [ 639.001952] [] (soc_pcm_hw_params) from [] > > > (dpcm_fe_dai_hw_params+0x70/0xe4) > > > [ 639.010765] [] (dpcm_fe_dai_hw_params) from [] > > > (snd_pcm_hw_params+0x158/0x418) > > > [ 639.019750] [] (snd_pcm_hw_params) from [] > > > (snd_pcm_ioctl+0x734/0x183c) > > > [ 639.028129] [] (snd_pcm_ioctl) from [] > > > (ksys_ioctl+0x2ac/0xb98) > > > [ 639.035812] [] (ksys_ioctl) from [] > > > (ret_fast_syscall+0x0/0x28) > > > [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0) > > > [ 639.048565] 9fa0: bee84650 01321870 0004 > > > c25c4111 bee84650 0002000f > > > [ 639.056766] 9fc0: bee84650 01321870 01321820 0036 1f40 > > > 0002c2f8 0003 > > > [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8 > > > [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink > > > > > > Signed-off-by: Shengjiu Wang > > > --- > > > sound/soc/fs
Re: [PATCH v2] drm/etnaviv: fix perfmon domain interation
Am Dienstag, den 19.05.2020, 07:30 +0200 schrieb Christian Gmeiner:
> The GC860 has one GPU device which has a 2d and 3d core. In this case
> we want to expose perfmon information for both cores.
>
> The driver has one array which contains all possible perfmon domains
> with some meta data - doms_meta. Here we can see that for the GC860
> two elements of that array are relevant:
>
> doms_3d: is at index 0 in the doms_meta array with 8 perfmon domains
> doms_2d: is at index 1 in the doms_meta array with 1 perfmon domain
>
> The userspace driver wants to get a list of all perfmon domains and
> their perfmon signals. This is done by iterating over all domains and
> their signals. If the userspace driver wants to access the domain with
> id 8 the kernel driver fails and returns invalid data from doms_3d with
> and invalid offset.
>
> This results in:
> Unable to handle kernel paging request at virtual address
>
> On such a device it is not possible to use the userspace driver at all.
>
> The fix for this off-by-one error is quite simple.
>
> Reported-by: Paul Cercueil
> Tested-by: Paul Cercueil
> Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query infrastructure")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Christian Gmeiner
Thanks, applied to etnaviv/fixes.
Regards,
Lucas
> ---
> drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> index e6795bafcbb9..75f9db8f7bec 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain *pm_domain(const
> struct etnaviv_gpu *gpu,
> if (!(gpu->identity.features & meta->feature))
> continue;
>
> - if (meta->nr_domains < (index - offset)) {
> + if (index - offset >= meta->nr_domains) {
> offset += meta->nr_domains;
> continue;
> }
Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> There are two requirements that we need to move the request
> of dma channel from probe to open.
How do you handle -EPROBE_DEFER return code from the channel request if
you don't do it in probe?
> - When dma device binds with power-domains, the power will
> be enabled when we request dma channel. If the request of dma
> channel happen on probe, then the power-domains will be always
> enabled after kernel boot up, which is not good for power
> saving, so we need to move the request of dma channel to .open();
This is certainly something which could be fixed in the dmaengine
driver.
> - With FE-BE case, if the dma channel is requested in probe,
> then there will be below issue, which is caused by that the
> dma channel will be requested duplicately
Why is this requested a second time? Is this just some missing cleanup
on a deferred probe path?
Regards,
Lucas
> [ 638.906268] sysfs: cannot create duplicate filename
> '/devices/soc0/soc/200.bus/200.spba-bus/2024000.esai/dma:tx'
> [ 638.919061] CPU: 1 PID: 673 Comm: aplay Not tainted
> 5.7.0-rc1-12956-gfc64b2585593 #287
> [ 638.927113] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [ 638.933690] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [ 638.941464] [] (show_stack) from []
> (dump_stack+0xe4/0x118)
> [ 638.948808] [] (dump_stack) from []
> (sysfs_warn_dup+0x50/0x64)
> [ 638.956406] [] (sysfs_warn_dup) from []
> (sysfs_do_create_link_sd+0xc8/0xd4)
> [ 638.965134] [] (sysfs_do_create_link_sd) from []
> (dma_request_chan+0xb0/0x210)
> [ 638.974120] [] (dma_request_chan) from []
> (dma_request_slave_channel+0x8/0x14)
> [ 638.983111] [] (dma_request_slave_channel) from []
> (fsl_asrc_dma_hw_params+0x1e0/0x438)
> [ 638.992881] [] (fsl_asrc_dma_hw_params) from []
> (soc_pcm_hw_params+0x4a0/0x6a8)
> [ 639.001952] [] (soc_pcm_hw_params) from []
> (dpcm_fe_dai_hw_params+0x70/0xe4)
> [ 639.010765] [] (dpcm_fe_dai_hw_params) from []
> (snd_pcm_hw_params+0x158/0x418)
> [ 639.019750] [] (snd_pcm_hw_params) from []
> (snd_pcm_ioctl+0x734/0x183c)
> [ 639.028129] [] (snd_pcm_ioctl) from []
> (ksys_ioctl+0x2ac/0xb98)
> [ 639.035812] [] (ksys_ioctl) from []
> (ret_fast_syscall+0x0/0x28)
> [ 639.043490] Exception stack(0xec529fa8 to 0xec529ff0)
> [ 639.048565] 9fa0: bee84650 01321870 0004 c25c4111
> bee84650 0002000f
> [ 639.056766] 9fc0: bee84650 01321870 01321820 0036 1f40
> 0002c2f8 0003
> [ 639.064964] 9fe0: b6f483fc bee8451c b6ee2655 b6e1dcf8
> [ 639.070339] fsl-esai-dai 2024000.esai: Cannot create DMA dma:tx symlink
>
> Signed-off-by: Shengjiu Wang
> ---
> sound/soc/fsl/imx-pcm-dma.c | 173 +---
> 1 file changed, 159 insertions(+), 14 deletions(-)
>
> diff --git a/sound/soc/fsl/imx-pcm-dma.c b/sound/soc/fsl/imx-pcm-dma.c
> index 04a9bc749016..dae53b384df4 100644
> --- a/sound/soc/fsl/imx-pcm-dma.c
> +++ b/sound/soc/fsl/imx-pcm-dma.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -29,24 +30,168 @@ static bool filter(struct dma_chan *chan, void *param)
> return true;
> }
>
> -static const struct snd_dmaengine_pcm_config imx_dmaengine_pcm_config = {
> - .prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
> - .compat_filter_fn = filter,
> -};
> +static int imx_pcm_hw_params(struct snd_soc_component *component,
> + struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> + struct snd_dmaengine_dai_dma_data *dma_data;
> + struct dma_slave_config config;
> + struct dma_chan *chan;
> + int ret = 0;
>
> -int imx_pcm_dma_init(struct platform_device *pdev, size_t size)
> + snd_pcm_set_runtime_buffer(substream, >dma_buffer);
> + runtime->dma_bytes = params_buffer_bytes(params);
> +
> + chan = snd_dmaengine_pcm_get_chan(substream);
> + if (!chan)
> + return -EINVAL;
> +
> + ret = snd_hwparams_to_dma_slave_config(substream, params, );
> + if (ret)
> + return ret;
> +
> + dma_data = snd_soc_dai_get_dma_data(cpu_dai, substream);
> + if (!dma_data)
> + return -EINVAL;
> +
> + snd_dmaengine_pcm_set_config_from_dai_data(substream,
> +dma_data,
> +);
> + return dmaengine_slave_config(chan, );
> +}
> +
> +static int imx_pcm_hw_free(struct snd_soc_component *component,
> +struct snd_pcm_substream *substream)
> {
> - struct snd_dmaengine_pcm_config *config;
> + snd_pcm_set_runtime_buffer(substream, NULL);
Re: [PATCH] drm/etnaviv: fix perfmon domain interation
Hi Christian,
Am Montag, den 11.05.2020, 14:37 +0200 schrieb Christian Gmeiner:
> The GC860 has one GPU device which has a 2d and 3d core. In this case
> we want to expose perfmon information for both cores.
>
> The driver has one array which contains all possible perfmon domains
> with some meta data - doms_meta. Here we can see that for the GC860
> two elements of that array are relevant:
>
> doms_3d: is at index 0 in the doms_meta array with 8 perfmon domains
> doms_2d: is at index 1 in the doms_meta array with 1 perfmon domain
>
> The userspace driver wants to get a list of all perfmon domains and
> their perfmon signals. This is done by iterating over all domains and
> their signals. If the userspace driver wants to access the domain with
> id 8 the kernel driver fails and returns invalid data from doms_3d with
> and invalid offset.
>
> This results in:
> Unable to handle kernel paging request at virtual address
>
> On such a device it is not possible to use the userspace driver at all.
>
> The fix for this off-by-one error is quite simple.
>
> Reported-by: Paul Cercueil
> Tested-by: Paul Cercueil
> Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query infrastructure")
> Cc: sta...@vger.kernel.or
Missing last letter of the TLD.
> Signed-off-by: Christian Gmeiner
> ---
> drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> index e6795bafcbb9..35f7171e779a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain *pm_domain(const
> struct etnaviv_gpu *gpu,
> if (!(gpu->identity.features & meta->feature))
> continue;
>
> - if (meta->nr_domains < (index - offset)) {
> + if ((meta->nr_domains - 1) < (index - offset)) {
While the logic is correct, I find this quite hard to read. A more
idiomatic way to write this (which is much easier to grok when reading
the code IMHO) would be:
if (index - offset >= meta->nr_domains)
If you agree, please send a v2 of this patch.
Regards,
Lucas
> offset += meta->nr_domains;
> continue;
> }
Re: [PATCH] drm/etnaviv: fix perfmon domain interation
Am Freitag, den 15.05.2020, 12:27 +0200 schrieb Christian Gmeiner:
> Am Fr., 15. Mai 2020 um 12:24 Uhr schrieb Lucas Stach
> :
> > Am Freitag, den 15.05.2020, 12:12 +0200 schrieb Paul Cercueil:
> > > Hi Christian,
> > >
> > > Le ven. 15 mai 2020 à 12:09, Christian Gmeiner
> > > a écrit :
> > > > Am Mo., 11. Mai 2020 um 14:38 Uhr schrieb Christian Gmeiner
> > > > :
> > > > > The GC860 has one GPU device which has a 2d and 3d core. In this
> > > > > case
> > > > > we want to expose perfmon information for both cores.
> > > > >
> > > > > The driver has one array which contains all possible perfmon domains
> > > > > with some meta data - doms_meta. Here we can see that for the GC860
> > > > > two elements of that array are relevant:
> > > > >
> > > > >doms_3d: is at index 0 in the doms_meta array with 8 perfmon
> > > > > domains
> > > > >doms_2d: is at index 1 in the doms_meta array with 1 perfmon
> > > > > domain
> > > > >
> > > > > The userspace driver wants to get a list of all perfmon domains and
> > > > > their perfmon signals. This is done by iterating over all domains
> > > > > and
> > > > > their signals. If the userspace driver wants to access the domain
> > > > > with
> > > > > id 8 the kernel driver fails and returns invalid data from doms_3d
> > > > > with
> > > > > and invalid offset.
> > > > >
> > > > > This results in:
> > > > >Unable to handle kernel paging request at virtual address
> > > > >
> > > > > On such a device it is not possible to use the userspace driver at
> > > > > all.
> > > > >
> > > > > The fix for this off-by-one error is quite simple.
> > > > >
> > > > > Reported-by: Paul Cercueil
> > > > > Tested-by: Paul Cercueil
> > > > > Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query
> > > > > infrastructure")
> > > > > Cc: sta...@vger.kernel.org
> > > > > Signed-off-by: Christian Gmeiner
> > > > > ---
> > > > > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > > > b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > > > index e6795bafcbb9..35f7171e779a 100644
> > > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > > > @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain
> > > > > *pm_domain(const struct etnaviv_gpu *gpu,
> > > > > if (!(gpu->identity.features & meta->feature))
> > > > > continue;
> > > > >
> > > > > - if (meta->nr_domains < (index - offset)) {
> > > > > + if ((meta->nr_domains - 1) < (index - offset)) {
> > > > > offset += meta->nr_domains;
> > > > > continue;
> > > > > }
> > > > > --
> > > > > 2.26.2
> > > > >
> > > >
> > > > ping
> > >
> > > I'll merge it tomorrow if there's no further feedback.
> >
> > Huh? Etnaviv patches are going through the etnaviv tree.
> >
> > We now have two different solutions to the same issue. I first want to
> > dig into the code to see why two developers can get confused enough by
> > the code to come up with totally different fixes.
> >
>
> You will see that the solutions are not totally different. I really hoped to
> get this fixed in the 5.7 release.. but I think its now too late.
I didn't have time to look at the full picture, yet. We still have at
least a week until the final 5.7 release, why would it be too late to
get a fix upstream?
Regards,
Lucas
Re: [PATCH] drm/etnaviv: fix perfmon domain interation
Am Freitag, den 15.05.2020, 12:12 +0200 schrieb Paul Cercueil:
> Hi Christian,
>
> Le ven. 15 mai 2020 à 12:09, Christian Gmeiner
> a écrit :
> > Am Mo., 11. Mai 2020 um 14:38 Uhr schrieb Christian Gmeiner
> > :
> > > The GC860 has one GPU device which has a 2d and 3d core. In this
> > > case
> > > we want to expose perfmon information for both cores.
> > >
> > > The driver has one array which contains all possible perfmon domains
> > > with some meta data - doms_meta. Here we can see that for the GC860
> > > two elements of that array are relevant:
> > >
> > >doms_3d: is at index 0 in the doms_meta array with 8 perfmon
> > > domains
> > >doms_2d: is at index 1 in the doms_meta array with 1 perfmon
> > > domain
> > >
> > > The userspace driver wants to get a list of all perfmon domains and
> > > their perfmon signals. This is done by iterating over all domains
> > > and
> > > their signals. If the userspace driver wants to access the domain
> > > with
> > > id 8 the kernel driver fails and returns invalid data from doms_3d
> > > with
> > > and invalid offset.
> > >
> > > This results in:
> > >Unable to handle kernel paging request at virtual address
> > >
> > > On such a device it is not possible to use the userspace driver at
> > > all.
> > >
> > > The fix for this off-by-one error is quite simple.
> > >
> > > Reported-by: Paul Cercueil
> > > Tested-by: Paul Cercueil
> > > Fixes: ed1dd899baa3 ("drm/etnaviv: rework perfmon query
> > > infrastructure")
> > > Cc: sta...@vger.kernel.org
> > > Signed-off-by: Christian Gmeiner
> > > ---
> > > drivers/gpu/drm/etnaviv/etnaviv_perfmon.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > index e6795bafcbb9..35f7171e779a 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_perfmon.c
> > > @@ -453,7 +453,7 @@ static const struct etnaviv_pm_domain
> > > *pm_domain(const struct etnaviv_gpu *gpu,
> > > if (!(gpu->identity.features & meta->feature))
> > > continue;
> > >
> > > - if (meta->nr_domains < (index - offset)) {
> > > + if ((meta->nr_domains - 1) < (index - offset)) {
> > > offset += meta->nr_domains;
> > > continue;
> > > }
> > > --
> > > 2.26.2
> > >
> >
> > ping
>
> I'll merge it tomorrow if there's no further feedback.
Huh? Etnaviv patches are going through the etnaviv tree.
We now have two different solutions to the same issue. I first want to
dig into the code to see why two developers can get confused enough by
the code to come up with totally different fixes.
Regards,
Lucas
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Donnerstag, den 14.05.2020, 09:27 +0100 schrieb Russell King - ARM Linux admin: > On Thu, May 14, 2020 at 10:18:02AM +0200, Lucas Stach wrote: > > Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > > > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > > > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > > > says that only the 'reg' clock could be optional, the others are > > > > required. > > > > > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > > > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > > > > > Maybe the binding needs to be updated and it seems that using > > > devm_clk_get_optional() like you propose is safe. > > > > The binding is correct as-is. We want to require those clocks to be > > present, but the dove DT was added before the binding was finalized, so > > the driver still treats the clocks as optional to not break > > compatibility with old DTs. Maybe this warrants a comment in the > > code... > > The binding doc in mainline says: > > clocks: > items: > - description: AXI/master interface clock > - description: GPU core clock > - description: Shader clock (only required if GPU has feature PIPE_3D) > - description: AHB/slave interface clock (only required if GPU can gate > slave interface independently) > minItems: 1 > maxItems: 4 > > clock-names: > items: > enum: [ bus, core, shader, reg ] > minItems: 1 > maxItems: 4 > > which looks correct to me - and means that Dove is compliant with that. The YAML binding actually did loose something in translation here, which I didn't notice. Previously all those clocks were listed under "Required properties", with the exceptions listed in parenthesis. So the Dove GPU, which is a combined 2D/3D core should have axi, core and shader clocks specified. Regards, Lucas
Re: [PATCH 2/3] drm/etnaviv: Don't ignore errors on getting clocks
Am Mittwoch, den 13.05.2020, 23:41 -0300 schrieb Fabio Estevam: > On Wed, May 13, 2020 at 2:09 PM Fabio Estevam wrote: > > > The binding doc Documentation/devicetree/bindings/gpu/vivante,gc.yaml > > says that only the 'reg' clock could be optional, the others are > > required. > > arch/arm/boot/dts/dove.dtsi only uses the 'core' clock. > arch/arm/boot/dts/stm32mp157.dtsi uses 'bus' and 'core' > > Maybe the binding needs to be updated and it seems that using > devm_clk_get_optional() like you propose is safe. The binding is correct as-is. We want to require those clocks to be present, but the dove DT was added before the binding was finalized, so the driver still treats the clocks as optional to not break compatibility with old DTs. Maybe this warrants a comment in the code... Regards, Lucas
Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m family
Am Montag, den 27.04.2020, 15:37 + schrieb Jacky Bai: > > -Original Message- > > From: Lucas Stach > > Sent: Monday, April 27, 2020 11:11 PM > > To: Abel Vesa ; Jacky Bai ; Shawn > > Guo ; Sascha Hauer ; Liam > > Girdwood ; Mark Brown > > Cc: Aisheng Dong ; dl-linux-imx > > ; linux-arm-ker...@lists.infradead.org; Linux Kernel > > Mailing List > > Subject: Re: [PATCH] soc: imx: Add power domain driver support for i.mx8m > > family > > > > Am Montag, den 27.04.2020, 17:58 +0300 schrieb Abel Vesa: > > > From: Jacky Bai > > > > > > The i.MX8M family is a set of NXP product focus on delivering the > > > latest and greatest video and audio experience combining > > > state-of-the-art media-specific features with high-performance > > > processing while optimized for lowest power consumption. > > > > > > i.MX8MQ, i.MX8MM, i.MX8MN, even the furture i.MX8MP are all belong to > > > this family. A GPC module is used to manage all the PU power domain > > > on/off. But the situation is that the number of power domains & the > > > power up sequence has significate difference on those SoCs. Even on > > > the same SoC. The power up sequence still has big difference. It makes > > > us hard to reuse the GPCv2 driver to cover the whole i.MX8M family. > > > Each time a new SoC is supported in the mainline kernel, we need to > > > modify the GPCv2 driver to support it. We need to add or modify > > > hundred lines of code in worst case. > > > It is a bad practice for the driver maintainability. > > > > > > This driver add a more generic power domain driver that the actual > > > power on/off is done by TF-A code. the abstraction give us the > > > possibility that using one driver to cover the whole i.MX8M family in > > > kernel side. > > > > > > > Again: what does this driver bring to the table, other than moving a > > fraction of > > the power domain functionality into the firmware? > > > > The discussions on the last submissions of this driver already established > > that > > we can't move all functionality for the power domains into the firmware, as > > controlling regulators is probably not easy to do from this context. Also > > the > > TF-A side implementation of this driver is "interesting" IMHO, it does > > stuff like > > accessing the clock controller registers without any locking or other means > > of > > mutual exclusion with the Linux kernel clock controller driver. > > > > The clock handling is in kernel side through CCF, not in ATF. See the patch > below. > > > Why can't we just extend the existing GPCv2 driver with support for the > > other > > i.MX8M family members? > > > > The reason that why I don’t like to extend the GPCv2 is that when doing > domain on/off, > We need to access some special control register in each domain & do some > special flow, > These control register(mediamix block control, vpumix block control) is not > in GPC > module's address range. No benefit to reuse the GPCv2. Only bring complexity > to the > GPCv2 driver each time a new SoC is added. > > Yes, the i.MX8M power domain support has been pending for a while. ARM guys > rejected this patchset > because they suggest us to use SCMI rather than SiP. But SCMI is only partial > suitable for our > case. Can you please point me to the most resent version of the TF-A side implementation of this? The i.MX8MM implementation in the imx_5.4.3_2.0.0 branch in the codeaurora imx-atf repo still contains writes to the clock controller register range. Also I would love to learn why the GPC needs to access Mediamix and VPUmix domain registers. If you are talking about the NOC configuration I would strongly suggest that those should be handled by a Linux side interconnect driver, this has nothing to do with the power domain sequencing, it just happens to lose state over the power down and needs to be reprogrammed after power on. The NOC configuration though is use- case dependent, so this should be properly handled in a rich OS driver. Sure we needs to extend the Linux side GPC driver for each new SoC generation, but that's no different from any other hardware driver in Linux. Drivers are the abstraction around the hardware, there is no need to invent another one if there are no clear benefits. Regards, Lucas
Re: [RFC PATCH 4/4] arm64: dts: imx8mm: Add GPU nodes for 2D and 3D core using Etnaviv
Am Sonntag, den 03.05.2020, 09:49 -0500 schrieb Adam Ford: > On Thu, Apr 30, 2020 at 7:46 AM Schrempf Frieder > wrote: > > From: Frieder Schrempf > > > > According to the documents, the i.MX8M-Mini features a GC320 and a > > GCNanoUltra GPU core. Etnaviv detects them as: > > > > etnaviv-gpu 3800.gpu: model: GC600, revision: 4653 > > etnaviv-gpu 38008000.gpu: model: GC520, revision: 5341 > > > > This seems to work fine more or less without any changes to the HWDB, > > which still might be needed in the future to correct some features, > > etc. > > > > Signed-off-by: Frieder Schrempf > > --- > Since not everyone uses the 3D or 2D, would it make sense to mark them > as disabled by default and let people who need the 3D and 2D enable > them at their respective board files? No, devices on the SoC with no external dependencies should be always enabled. The board has much less influence over whether the GPU is being used than the specific use-case. While the board designer may not even think about using the GPUs (because no display connector present or something like that) people using the board may still find uses for the GPU, like doing video pipeline color space conversions or something lie that. Regards, Lucas
Re: [RFC PATCH 3/4] drm/etnaviv: Change order of enabling clocks to fix boot on i.MX8MM
Am Donnerstag, den 30.04.2020, 12:46 + schrieb Schrempf Frieder:
> From: Frieder Schrempf
>
> On some i.MX8MM devices the boot hangs when enabling the GPU clocks.
> Changing the order of clock initalization to
>
> core -> shader -> bus -> reg
>
> fixes the issue. This is the same order used in the imx platform code
> of the downstream GPU driver in the NXP kernel [1]. For the sake of
> consistency we also adjust the order of disabling the clocks to the
> reverse.
>
> [1]
> https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/mxc/gpu-viv/hal/os/linux/kernel/platform/freescale/gc_hal_kernel_platform_imx.c?h=imx_5.4.3_2.0.0#n1538
I don't see why the order of the clocks is important. Is this really a
GPU issue? As in: does a GPU access hang when enabling the clocks in
the wrong order? Or is this a clock driver issue with a clock access
hanging due to an upstream clock still being disabled?
Regards,
Lucas
> Signed-off-by: Frieder Schrempf
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 42 +--
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 7b138d4dd068..424b2e5951f0 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1487,55 +1487,55 @@ static int etnaviv_gpu_clk_enable(struct etnaviv_gpu
> *gpu)
> {
> int ret;
>
> - if (gpu->clk_reg) {
> - ret = clk_prepare_enable(gpu->clk_reg);
> + if (gpu->clk_core) {
> + ret = clk_prepare_enable(gpu->clk_core);
> if (ret)
> return ret;
> }
>
> - if (gpu->clk_bus) {
> - ret = clk_prepare_enable(gpu->clk_bus);
> + if (gpu->clk_shader) {
> + ret = clk_prepare_enable(gpu->clk_shader);
> if (ret)
> - goto disable_clk_reg;
> + goto disable_clk_core;
> }
>
> - if (gpu->clk_core) {
> - ret = clk_prepare_enable(gpu->clk_core);
> + if (gpu->clk_bus) {
> + ret = clk_prepare_enable(gpu->clk_bus);
> if (ret)
> - goto disable_clk_bus;
> + goto disable_clk_shader;
> }
>
> - if (gpu->clk_shader) {
> - ret = clk_prepare_enable(gpu->clk_shader);
> + if (gpu->clk_reg) {
> + ret = clk_prepare_enable(gpu->clk_reg);
> if (ret)
> - goto disable_clk_core;
> + goto disable_clk_bus;
> }
>
> return 0;
>
> -disable_clk_core:
> - if (gpu->clk_core)
> - clk_disable_unprepare(gpu->clk_core);
> disable_clk_bus:
> if (gpu->clk_bus)
> clk_disable_unprepare(gpu->clk_bus);
> -disable_clk_reg:
> - if (gpu->clk_reg)
> - clk_disable_unprepare(gpu->clk_reg);
> +disable_clk_shader:
> + if (gpu->clk_shader)
> + clk_disable_unprepare(gpu->clk_shader);
> +disable_clk_core:
> + if (gpu->clk_core)
> + clk_disable_unprepare(gpu->clk_core);
>
> return ret;
> }
>
> static int etnaviv_gpu_clk_disable(struct etnaviv_gpu *gpu)
> {
> + if (gpu->clk_reg)
> + clk_disable_unprepare(gpu->clk_reg);
> + if (gpu->clk_bus)
> + clk_disable_unprepare(gpu->clk_bus);
> if (gpu->clk_shader)
> clk_disable_unprepare(gpu->clk_shader);
> if (gpu->clk_core)
> clk_disable_unprepare(gpu->clk_core);
> - if (gpu->clk_bus)
> - clk_disable_unprepare(gpu->clk_bus);
> - if (gpu->clk_reg)
> - clk_disable_unprepare(gpu->clk_reg);
>
> return 0;
> }
Re: [RFC PATCH 1/4] drm/etnaviv: Prevent IRQ triggering at probe time on i.MX8MM
Hi Frieder,
Am Donnerstag, den 30.04.2020, 12:46 + schrieb Schrempf Frieder:
> From: Frieder Schrempf
>
> On i.MX8MM there is an interrupt getting triggered immediately after
> requesting the IRQ, which leads to a stall as the handler accesses
> the GPU registers whithout the clock being enabled.
>
> Enabling the clocks briefly seems to clear the IRQ state, so we do
> this before requesting the IRQ.
This is most likely caused by improper power-up sequencing. Normally
the GPC will trigger a hardware reset of the modules inside a power
domain when the domain is powered on. This requires the clocks to be
running at this point, as those resets are synchronous, so need clock
pulses to propagate through the hardware.
>From what I see the i.MX8MM is still missing the power domain
controller integration, but I'm pretty confident that this problem
should be solved in the power domain code, instead of the GPU driver.
Regards,
Lucas
> Signed-off-by: Frieder Schrempf
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 29 -
> --
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index a31eeff2b297..23877c1f150a 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -1775,13 +1775,6 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> return gpu->irq;
> }
>
> - err = devm_request_irq(>dev, gpu->irq, irq_handler, 0,
> -dev_name(gpu->dev), gpu);
> - if (err) {
> - dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq,
> err);
> - return err;
> - }
> -
> /* Get Clocks: */
> gpu->clk_reg = devm_clk_get(>dev, "reg");
> DBG("clk_reg: %p", gpu->clk_reg);
> @@ -1805,6 +1798,28 @@ static int etnaviv_gpu_platform_probe(struct
> platform_device *pdev)
> gpu->clk_shader = NULL;
> gpu->base_rate_shader = clk_get_rate(gpu->clk_shader);
>
> + /*
> + * On i.MX8MM there is an interrupt getting triggered
> immediately
> + * after requesting the IRQ, which leads to a stall as the
> handler
> + * accesses the GPU registers whithout the clock being enabled.
> + * Enabling the clocks briefly seems to clear the IRQ state, so
> we do
> + * this here before requesting the IRQ.
> + */
> + err = etnaviv_gpu_clk_enable(gpu);
> + if (err)
> + return err;
> +
> + err = etnaviv_gpu_clk_disable(gpu);
> + if (err)
> + return err;
> +
> + err = devm_request_irq(>dev, gpu->irq, irq_handler, 0,
> +dev_name(gpu->dev), gpu);
> + if (err) {
> + dev_err(dev, "failed to request IRQ%u: %d\n", gpu->irq,
> err);
> + return err;
> + }
> +
> /* TODO: figure out max mapped size */
> dev_set_drvdata(dev, gpu);
>
Re: [PATCH 2/3] ARM: dts: imx6qdl-zii-rdu2: Fix accelerometer interrupt-names
Am Montag, den 21.10.2019, 21:04 -0700 schrieb Andrey Smirnov:
> According to Documentation/devicetree/bindings/iio/accel/mma8452.txt,
> the correct interrupt-names are "INT1" and "INT2", so fix them
> accordingly.
>
> While at it, modify the node to only specify "INT2" since providing
> two interrupts is not necessary or useful (the driver will only use
> one).
>
> Signed-off-by: Fabio Estevam
> [andrew.smir...@gmail.com modified the patch to drop INT1]
> Signed-off-by: Andrey Smirnov
Reviewed-by: Lucas Stach
> Cc: Fabio Estevam
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Shawn Guo
> Cc: linux-arm-ker...@lists.infradead.org,
> Cc: linux-kernel@vger.kernel.org
> ---
>
> Original patch from Fabio can be seen here:
>
> https://lore.kernel.org/linux-arm-kernel/20191010125300.2822-1-feste...@gmail.com/
>
> arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> index 8d46f7b2722b..a8c86e621b49 100644
> --- a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> @@ -358,8 +358,8 @@
> compatible = "fsl,mma8451";
> reg = <0x1c>;
> interrupt-parent = <>;
> - interrupt-names = "int1", "int2";
> - interrupts = <18 IRQ_TYPE_LEVEL_LOW>, <20 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "INT2";
> + interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> };
>
> hpa2: amp@60 {
> @@ -849,7 +849,6 @@
> {
> pinctrl_accel: accelgrp {
> fsl,pins = <
> - MX6QDL_PAD_SD1_CMD__GPIO1_IO18 0x4001b000
> MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x4001b000
> >;
> };
Re: [PATCH 3/3] ARM: dts: imx6qdl-zii-rdu2: Specify supplies for accelerometer
Am Montag, den 21.10.2019, 21:05 -0700 schrieb Andrey Smirnov:
> Specify 'vdd' and 'vddio' supplies for accelerometer to avoid warnings
> during boot.
>
> Signed-off-by: Andrey Smirnov
Reviewed-by: Lucas Stach
> Cc: Fabio Estevam
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: Shawn Guo
> Cc: linux-arm-ker...@lists.infradead.org,
> Cc: linux-kernel@vger.kernel.org
> ---
> arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> index a8c86e621b49..42c0a728216d 100644
> --- a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi
> @@ -360,6 +360,8 @@
> interrupt-parent = <>;
> interrupt-names = "INT2";
> interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
> + vdd-supply = <_3p3v>;
> + vddio-supply = <_3p3v>;
> };
>
> hpa2: amp@60 {
Re: [PATCH 1/3] ARM: dts: imx6qdl-zii-rdu2: Drop GPIO_ACTIVE_LOW form reg_5p0v_user_usb
Am Montag, den 21.10.2019, 21:04 -0700 schrieb Andrey Smirnov: > Drop GPIO_ACTIVE_LOW form reg_5p0v_user_usb since it is ignored by the > gpiolib and results in a warning. NACK. This is consistent with the regulator binding behavior and a fix to gpiolib has been accepted to not print a warning in this case. Regards, Lucas > Signed-off-by: Andrey Smirnov > Cc: Fabio Estevam > Cc: Chris Healy > Cc: Lucas Stach > Cc: Shawn Guo > Cc: linux-arm-ker...@lists.infradead.org, > Cc: linux-kernel@vger.kernel.org > --- > arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi > b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi > index 93be00a60c88..8d46f7b2722b 100644 > --- a/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi > @@ -68,7 +68,7 @@ > regulator-name = "5V_USER_USB"; > regulator-min-microvolt = <500>; > regulator-max-microvolt = <500>; > - gpio = < 22 GPIO_ACTIVE_LOW>; > + gpio = < 22 0>; > startup-delay-us = <1000>; > }; >
Re: [PATCH v2 2/2] dt-bindings: etnaviv: Add #cooling-cells
On Fr, 2019-10-18 at 15:50 +0200, Guido Günther wrote: > Hi, > On Wed, Sep 11, 2019 at 07:40:36PM -0700, Guido Günther wrote: > > Add #cooling-cells for when the gpu acts as a cooling device. > > > > Signed-off-by: Guido Günther > > --- > > .../devicetree/bindings/display/etnaviv/etnaviv-drm.txt | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git > > a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt > > b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt > > index 8def11b16a24..640592e8ab2e 100644 > > --- a/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt > > +++ b/Documentation/devicetree/bindings/display/etnaviv/etnaviv-drm.txt > > @@ -21,6 +21,7 @@ Required properties: > > Optional properties: > > - power-domains: a power domain consumer specifier according to > >Documentation/devicetree/bindings/power/power_domain.txt > > +- #cooling-cells: : If used as a cooling device, must be <2> > > The other patch of the series made it into linux-next already but this > documentation fixup didn't. Anything i can do to get this applied as > well so documentation stays in sync? I've applied and pushed this to my etnaviv/next branch just now, so it should show up in linux-next pretty soon. Regards, Lucas
Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
On Fr, 2019-08-16 at 22:59 +0200, Heiner Kallweit wrote: > On 15.08.2019 17:32, Christian Herber wrote: > > This patch adds basic support for BASE-T1 PHYs in the framework. > > BASE-T1 PHYs main area of application are automotive and industrial. > > BASE-T1 is standardized in IEEE 802.3, namely > > - IEEE 802.3bw: 100BASE-T1 > > - IEEE 802.3bp 1000BASE-T1 > > - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S > > > > There are no products which contain BASE-T1 and consumer type PHYs like > > 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1 > > PHYs with auto-negotiation. > > Is this meant in a way that *currently* there are no PHY's combining Base-T1 > with normal Base-T modes? Or are there reasons why this isn't possible in > general? I'm asking because we have PHY's combining copper and fiber, and e.g. > the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2. There are PHYs combining both Base-T1 and other Base-T capabilities. E.g. the Broadcom BCM54811 support both Base-T1, as well as 1000BASE-T and 100BASE-TX. Regards, Lucas
Re: [PATCH] soc: imx: gpc: fix initialiser format
On Di, 2019-10-15 at 15:09 +0100, Ben Dooks wrote:
> Make the initialiers in imx_gpc_domains C99 format to fix the
> following sparse warnings:
>
> drivers/soc/imx/gpc.c:252:30: warning: obsolete array initializer, use C99
> syntax
> drivers/soc/imx/gpc.c:258:29: warning: obsolete array initializer, use C99
> syntax
> drivers/soc/imx/gpc.c:269:34: warning: obsolete array initializer, use C99
> syntax
> drivers/soc/imx/gpc.c:278:30: warning: obsolete array initializer, use C99
> syntax
>
> Signed-off-by: Ben Dooks
Reviewed-by: Lucas Stach
> ---
> Cc: Shawn Guo
> Cc: Sascha Hauer
> Cc: Pengutronix Kernel Team
> Cc: Fabio Estevam
> Cc: NXP Linux Team
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/soc/imx/gpc.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/soc/imx/gpc.c b/drivers/soc/imx/gpc.c
> index d9231bd3c691..98b9d9a902ae 100644
> --- a/drivers/soc/imx/gpc.c
> +++ b/drivers/soc/imx/gpc.c
> @@ -249,13 +249,13 @@ static struct genpd_power_state imx6_pm_domain_pu_state
> = {
> };
>
> static struct imx_pm_domain imx_gpc_domains[] = {
> - [GPC_PGC_DOMAIN_ARM] {
> + [GPC_PGC_DOMAIN_ARM] = {
> .base = {
> .name = "ARM",
> .flags = GENPD_FLAG_ALWAYS_ON,
> },
> },
> - [GPC_PGC_DOMAIN_PU] {
> + [GPC_PGC_DOMAIN_PU] = {
> .base = {
> .name = "PU",
> .power_off = imx6_pm_domain_power_off,
> @@ -266,7 +266,7 @@ static struct imx_pm_domain imx_gpc_domains[] = {
> .reg_offs = 0x260,
> .cntr_pdn_bit = 0,
> },
> - [GPC_PGC_DOMAIN_DISPLAY] {
> + [GPC_PGC_DOMAIN_DISPLAY] = {
> .base = {
> .name = "DISPLAY",
> .power_off = imx6_pm_domain_power_off,
> @@ -275,7 +275,7 @@ static struct imx_pm_domain imx_gpc_domains[] = {
> .reg_offs = 0x240,
> .cntr_pdn_bit = 4,
> },
> - [GPC_PGC_DOMAIN_PCI] {
> + [GPC_PGC_DOMAIN_PCI] = {
> .base = {
> .name = "PCI",
> .power_off = imx6_pm_domain_power_off,
Re: [PATCH v2 00/21] Refine memblock API
Am Freitag, den 04.10.2019, 10:27 +0100 schrieb Russell King - ARM
Linux admin:
> On Thu, Oct 03, 2019 at 02:30:10PM +0300, Mike Rapoport wrote:
> > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux
> > admin wrote:
> > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote:
> > > > (trimmed the CC)
> > > >
> > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote:
> > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport <
> > > > > r...@linux.ibm.com> wrote:
> > > > >
> > > > > Before the patch:
> > > > >
> > > > > # cat /sys/kernel/debug/memblock/memory
> > > > >0: 0x1000..0x8fff
> > > > > # cat /sys/kernel/debug/memblock/reserved
> > > > >0: 0x10004000..0x10007fff
> > > > > 34: 0x2f88..0x3fff
> > > > >
> > > > >
> > > > > After the patch:
> > > > > # cat /sys/kernel/debug/memblock/memory
> > > > >0: 0x1000..0x8fff
> > > > > # cat /sys/kernel/debug/memblock/reserved
> > > > >0: 0x10004000..0x10007fff
> > > > > 36: 0x8000..0x8fff
> > > >
> > > > I'm still not convinced that the memblock refactoring didn't
> > > > uncovered an
> > > > issue in etnaviv driver.
> > > >
> > > > Why moving the CMA area from 0x8000 to 0x3000 makes it
> > > > fail?
> > >
> > > I think you have that the wrong way round.
> >
> > I'm relying on Adam's reports of working and non-working versions.
> > According to that etnaviv works when CMA area is at 0x8000 and
> > does not
> > work when it is at 0x3000.
> >
> > He also sent logs a few days ago [1], they also confirm that.
> >
> > [1]
> > https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w...@mail.gmail.com/
>
> Sorry, yes, you're right. Still, I've reported this same regression
> a while back, and it's never gone away.
>
> > > > BTW, the code that complained about "command buffer outside
> > > > valid memory
> > > > window" has been removed by the commit 17e4660ae3d7
> > > > ("drm/etnaviv:
> > > > implement per-process address spaces on MMUv2").
> > > >
> > > > Could be that recent changes to MMU management of etnaviv
> > > > resolve the
> > > > issue?
> > >
> > > The iMX6 does not have MMUv2 hardware, it has MMUv1. With MMUv1
> > > hardware requires command buffers within the first 2GiB of
> > > physical
> > > RAM.
> >
> > I've mentioned that patch because it removed the check for cmdbuf
> > address
> > for MMUv1:
> >
> > @@ -785,15 +768,7 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu)
> > PAGE_SIZE);
> > if (ret) {
> > dev_err(gpu->dev, "could not create command
> > buffer\n");
> > - goto unmap_suballoc;
> > - }
> > -
> > - if (!(gpu->identity.minor_features1 &
> > chipMinorFeatures1_MMU_VERSION) &&
> > - etnaviv_cmdbuf_get_va(>buffer,
> > >cmdbuf_mapping) > 0x8000) {
> > - ret = -EINVAL;
> > - dev_err(gpu->dev,
> > - "command buffer outside valid memory
> > window\n");
> > - goto free_buffer;
> > + goto fail;
> > }
> >
> > /* Setup event management */
> >
> >
> > I really don't know how etnaviv works, so I hoped that people who
> > understand it would help.
>
> From what I can see, removing that check is a completely insane thing
> to do, and I note that these changes are _not_ described in the
> commit
> message. The problem was known about _before_ (June 22) the patch
> was
> created (July 5).
>
> Lucas, please can you explain why removing the above check, which is
> well known to correctly trigger on various platforms to prevent
> incorrect GPU behaviour, is safe?
It isn't. It's a pretty big oversight in this commit to remove this
check. It can't be done at the same spot in the code anymore, as we
don't have a mapping context at this time anymore, but it should have
moved into etnaviv_iommu_context_init(). I'll send a patch to fix this
up.
Regards,
Lucas
Re: [PATCH v2 00/21] Refine memblock API
Am Donnerstag, den 03.10.2019, 14:30 +0300 schrieb Mike Rapoport: > On Thu, Oct 03, 2019 at 09:49:14AM +0100, Russell King - ARM Linux admin > wrote: > > On Thu, Oct 03, 2019 at 08:34:52AM +0300, Mike Rapoport wrote: > > > (trimmed the CC) > > > > > > On Wed, Oct 02, 2019 at 06:14:11AM -0500, Adam Ford wrote: > > > > On Wed, Oct 2, 2019 at 2:36 AM Mike Rapoport wrote: > > > > > > > > Before the patch: > > > > > > > > # cat /sys/kernel/debug/memblock/memory > > > >0: 0x1000..0x8fff > > > > # cat /sys/kernel/debug/memblock/reserved > > > >0: 0x10004000..0x10007fff > > > > 34: 0x2f88..0x3fff > > > > > > > > > > > > After the patch: > > > > # cat /sys/kernel/debug/memblock/memory > > > >0: 0x1000..0x8fff > > > > # cat /sys/kernel/debug/memblock/reserved > > > >0: 0x10004000..0x10007fff > > > > 36: 0x8000..0x8fff > > > > > > I'm still not convinced that the memblock refactoring didn't uncovered an > > > issue in etnaviv driver. > > > > > > Why moving the CMA area from 0x8000 to 0x3000 makes it fail? > > > > I think you have that the wrong way round. > > I'm relying on Adam's reports of working and non-working versions. > According to that etnaviv works when CMA area is at 0x8000 and does not > work when it is at 0x3000. > > He also sent logs a few days ago [1], they also confirm that. > > [1] > https://lore.kernel.org/linux-mm/CAHCN7xJEvS2Si=M+BYtz+kY0M4NxmqDjiX9Nwq6_3GGBh3yg=w...@mail.gmail.com/ To clarify: Etnaviv needs to know where the CMA area is in order to move a aperture window to cover the CMA area so the command buffers allocated in contig memory can be mapped through this aperture. Now the issue is that there is currently there is no good API for a driver to know where the CMA area is located, so we are trying to infer this from dma_get_required_mask. Unfortunately this can overshoot the real DRAM area by a bit, so combined with the fixed 2GB size of the GPU aperture this means we are no longer able to map the command buffers through the required aperture if the CMA area moves too far down in the physical memory. It's really a bad interaction between etnaviv and CMA area placement, due to insufficient APIs to communicate some crucial information. There is nothing in the etnaviv driver or the hardware which requires the CMA area to be at a certain place, we just need to know where it is located exactly. So my try at fixing this [1] was by adding a API to get the required information, but the first attempt was shot down and I hadn't had time to follow up on this yet. Regards, Lucas [1] https://patchwork.kernel.org/patch/10966767/
Re: [PATCH 5/5] arm64: dts: imx8mq: add DCSS node
Am Montag, den 23.09.2019, 13:12 -0300 schrieb Fabio Estevam:
> Hi Laurentiu,
>
> On Mon, Sep 23, 2019 at 11:14 AM Laurentiu Palcu
> wrote:
>
> > +
> > + dcss: dcss@0x32e0 {
>
> Node names should be generic, so:
>
> dcss: display-controller@32e0
>
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + compatible = "nxp,imx8mq-dcss";
> > + reg = <0x32e0 0x2D000>,
> > <0x32e2f000 0x1000>;
>
> 0x2d000 for consistency.
>
> > + interrupts = <6>, <8>, <9>;
>
> The interrupts are passed in the
> format.
No, they are not. Those are imx-irqsteer IRQs, this controller has 0
irq cells, so the description in this patch is correct.
Regards,
Lucas
Re: [PATCH v4 2/3] dmaengine: imx-sdma: fix dma freezes
On Fr, 2019-09-20 at 10:53 +0200, Philipp Puschmann wrote:
> Hi Jan,
>
> Am 19.09.19 um 17:19 schrieb Jan Lübbe:
> > Hi Philipp,
> >
> > see below...
> >
> > On Thu, 2019-09-19 at 16:29 +0200, Philipp Puschmann wrote:
> > > For some years and since many kernel versions there are reports that the
> > > RX UART SDMA channel stops working at some point. The workaround was to
> > > disable DMA for RX. This commit tries to fix the problem itself.
> > >
> > > Due to its license i wasn't able to debug the sdma script itself but it
> > > somehow leads to blocking the scheduling of the channel script when a
> > > running sdma script does not find any free descriptor in the ring to put
> > > its data into.
> > >
> > > If we detect such a potential case we manually restart the channel.
> > >
> > > As sdmac->desc is constant we can move desc out of the loop.
> > >
> > > Fixes: 1ec1e82f2510 ("dmaengine: Add Freescale i.MX SDMA support")
> > > Signed-off-by: Philipp Puschmann
> > > Reviewed-by: Lucas Stach
> > > ---
> > >
> > > Changelog v4:
> > > - fixed the fixes tag
> > >
> > > Changelog v3:
> > > - use correct dma_wmb() instead of dma_wb()
> > > - add fixes tag
> > >
> > > Changelog v2:
> > > - clarify comment and commit description
> > >
> > > drivers/dma/imx-sdma.c | 21 +
> > > 1 file changed, 17 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > > index e029a2443cfc..a32b5962630e 100644
> > > --- a/drivers/dma/imx-sdma.c
> > > +++ b/drivers/dma/imx-sdma.c
> > > @@ -775,21 +775,23 @@ static void sdma_start_desc(struct sdma_channel
> > > *sdmac)
> > > static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> > > {
> > > struct sdma_buffer_descriptor *bd;
> > > - int error = 0;
> > > - enum dma_status old_status = sdmac->status;
> > > + struct sdma_desc *desc = sdmac->desc;
> > > + int error = 0, cnt = 0;
> > > + enum dma_status old_status = sdmac->status;
> > >
> > > /*
> > >* loop mode. Iterate over descriptors, re-setup them and
> > >* call callback function.
> > >*/
> > > - while (sdmac->desc) {
> > > - struct sdma_desc *desc = sdmac->desc;
> > > + while (desc) {
> > >
> > > bd = >bd[desc->buf_tail];
> > >
> > > if (bd->mode.status & BD_DONE)
> > > break;
> > >
> > > + cnt++;
> > > +
> > > if (bd->mode.status & BD_RROR) {
> > > bd->mode.status &= ~BD_RROR;
> > > sdmac->status = DMA_ERROR;
> > > @@ -822,6 +824,17 @@ static void sdma_update_channel_loop(struct
> > > sdma_channel *sdmac)
> > > if (error)
> > > sdmac->status = old_status;
> > > }
> > > +
> > > + /* In some situations it may happen that the sdma does not found any
> > ^ hasn't
> > > + * usable descriptor in the ring to put data into. The channel is
> > > + * stopped then. While there is no specific error condition we can
> > > + * check for, a necessary condition is that all available buffers for
> > > + * the current channel have been written to by the sdma script. In
> > > + * this case and after we have made the buffers available again,
> > > + * we restart the channel.
> > > + */
> >
> > Are you sure we can't miss cases where we only had to make some buffers
> > available again, but the SDMA already ran out of buffers before?
> Think so, yes.
> > A while ago, I was debugging a similar issue triggered by receiving
> > data with a wrong baud rate, which leads to all descriptors being
> > marked with the error flag very quickly (and the SDMA stalling).
> > I noticed that you can check if the channel is still running by
> > checking the SDMA_H_STATSTOP register & BIT(sdmac->channel).
>
> I think checking for this register is the better approach. Then i could drop
> the
> cnt variable. And by droppting cnt i would propose to move the check and
> reenabling
> to the end of the while loop to reenable the channel after freeing first
> buffer.
You certain
Re: [PATCH v2 1/3] dmaengine: imx-sdma: fix buffer ownership
Hi Philipp, On Do, 2019-09-19 at 12:23 +0200, Philipp Puschmann wrote: > BD_DONE flag marks ownership of the buffer. When 1 SDMA owns the > buffer, when 0 ARM owns it. When processing the buffers in > sdma_update_channel_loop the ownership of the currently processed > buffer was set to SDMA again before running the callback function of > the buffer and while the sdma script may be running in parallel. So > there was the possibility to get the buffer overwritten by SDMA > before > it has been processed by kernel leading to kind of random errors in > the > upper layers, e.g. bluetooth. > > Signed-off-by: Philipp Puschmann > > --- > > Changelog v2: > - add dma_wb() > > drivers/dma/imx-sdma.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c > index 9ba74ab7e912..e029a2443cfc 100644 > --- a/drivers/dma/imx-sdma.c > +++ b/drivers/dma/imx-sdma.c > @@ -802,7 +802,6 @@ static void sdma_update_channel_loop(struct > sdma_channel *sdmac) > */ > > desc->chn_real_count = bd->mode.count; > - bd->mode.status |= BD_DONE; > bd->mode.count = desc->period_len; > desc->buf_ptail = desc->buf_tail; > desc->buf_tail = (desc->buf_tail + 1) % desc->num_bd; > @@ -817,6 +816,9 @@ static void sdma_update_channel_loop(struct > sdma_channel *sdmac) > dmaengine_desc_get_callback_invoke(>vd.tx, NULL); > spin_lock(>vc.lock); > > + dma_wb(); Has this change been tested? The function you want here is called dma_wmb(). Regards, Lucas > + bd->mode.status |= BD_DONE; > + > if (error) > sdmac->status = old_status; > }
