- Linaro-mm-sig - lists.linaro.org

[PATCH AUTOSEL 5.4 39/42] i2c: sh_mobile: implement atomic transfers

by Sasha Levin

From: Ulrich Hecht <uli+renesas(a)fpond.eu> [ Upstream commit a49cc1fe9d64a2dc4e19b599204f403e5d25f44b ] Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and similar boards. Signed-off-by: Ulrich Hecht <uli+renesas(a)fpond.eu> Tested-by: Wolfram Sang <wsa+renesas(a)sang-engineering.com> Tested-by: Geert Uytterhoeven <geert+renesas(a)glider.be> [wsa: some whitespace fixing] Signed-off-by: Wolfram Sang <wsa(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/i2c/busses/i2c-sh_mobile.c | 86 +++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index 8777af4c695e9..d5dd58c27ce5f 100644 --- a/drivers/i2c/busses/i2c-sh_mobile.c +++ b/drivers/i2c/busses/i2c-sh_mobile.c @@ -129,6 +129,7 @@ struct sh_mobile_i2c_data { int sr; bool send_stop; bool stop_after_dma; + bool atomic_xfer; struct resource *res; struct dma_chan *dma_tx; @@ -333,13 +334,15 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op ret = iic_rd(pd, ICDR); break; case OP_RX_STOP: /* enable DTE interrupt, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); ret = iic_rd(pd, ICDR); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; @@ -435,7 +438,8 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id) if (wakeup) { pd->sr |= SW_DONE; - wake_up(&pd->wait); + if (!pd->atomic_xfer) + wake_up(&pd->wait); } /* defeat write posting to avoid spurious WAIT interrupts */ @@ -587,6 +591,9 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, pd->pos = -1; pd->sr = 0; + if (pd->atomic_xfer) + return; + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); if (pd->dma_buf) sh_mobile_i2c_xfer_dma(pd); @@ -643,15 +650,13 @@ static int poll_busy(struct sh_mobile_i2c_data *pd) return i ? 0 : -ETIMEDOUT; } -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, - struct i2c_msg *msgs, - int num) +static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd, + struct i2c_msg *msgs, int num) { - struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); struct i2c_msg *msg; int err = 0; int i; - long timeout; + long time_left; /* Wake up device and enable clock */ pm_runtime_get_sync(pd->dev); @@ -668,15 +673,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, if (do_start) i2c_op(pd, OP_START); - /* The interrupt handler takes care of the rest... */ - timeout = wait_event_timeout(pd->wait, - pd->sr & (ICSR_TACK | SW_DONE), - adapter->timeout); - - /* 'stop_after_dma' tells if DMA transfer was complete */ - i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma); + if (pd->atomic_xfer) { + unsigned long j = jiffies + pd->adap.timeout; + + time_left = time_before_eq(jiffies, j); + while (time_left && + !(pd->sr & (ICSR_TACK | SW_DONE))) { + unsigned char sr = iic_rd(pd, ICSR); + + if (sr & (ICSR_AL | ICSR_TACK | + ICSR_WAIT | ICSR_DTE)) { + sh_mobile_i2c_isr(0, pd); + udelay(150); + } else { + cpu_relax(); + } + time_left = time_before_eq(jiffies, j); + } + } else { + /* The interrupt handler takes care of the rest... */ + time_left = wait_event_timeout(pd->wait, + pd->sr & (ICSR_TACK | SW_DONE), + pd->adap.timeout); + + /* 'stop_after_dma' tells if DMA xfer was complete */ + i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, + pd->stop_after_dma); + } - if (!timeout) { + if (!time_left) { dev_err(pd->dev, "Transfer request timed out\n"); if (pd->dma_direction != DMA_NONE) sh_mobile_i2c_cleanup_dma(pd); @@ -702,14 +727,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, return err ?: num; } +static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = false; + return sh_mobile_xfer(pd, msgs, num); +} + +static int sh_mobile_i2c_xfer_atomic(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = true; + return sh_mobile_xfer(pd, msgs, num); +} + static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING; } static const struct i2c_algorithm sh_mobile_i2c_algorithm = { - .functionality = sh_mobile_i2c_func, - .master_xfer = sh_mobile_i2c_xfer, + .functionality = sh_mobile_i2c_func, + .master_xfer = sh_mobile_i2c_xfer, + .master_xfer_atomic = sh_mobile_i2c_xfer_atomic, }; static const struct i2c_adapter_quirks sh_mobile_i2c_quirks = { -- 2.27.0

5 years, 1 month

1
0
0 0

[PATCH AUTOSEL 5.9 49/55] i2c: sh_mobile: implement atomic transfers

by Sasha Levin

From: Ulrich Hecht <uli+renesas(a)fpond.eu> [ Upstream commit a49cc1fe9d64a2dc4e19b599204f403e5d25f44b ] Implements atomic transfers to fix reboot/shutdown on r8a7790 Lager and similar boards. Signed-off-by: Ulrich Hecht <uli+renesas(a)fpond.eu> Tested-by: Wolfram Sang <wsa+renesas(a)sang-engineering.com> Tested-by: Geert Uytterhoeven <geert+renesas(a)glider.be> [wsa: some whitespace fixing] Signed-off-by: Wolfram Sang <wsa(a)kernel.org> Signed-off-by: Sasha Levin <sashal(a)kernel.org> --- drivers/i2c/busses/i2c-sh_mobile.c | 86 +++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/drivers/i2c/busses/i2c-sh_mobile.c b/drivers/i2c/busses/i2c-sh_mobile.c index cab7255599991..bdd60770779ad 100644 --- a/drivers/i2c/busses/i2c-sh_mobile.c +++ b/drivers/i2c/busses/i2c-sh_mobile.c @@ -129,6 +129,7 @@ struct sh_mobile_i2c_data { int sr; bool send_stop; bool stop_after_dma; + bool atomic_xfer; struct resource *res; struct dma_chan *dma_tx; @@ -330,13 +331,15 @@ static unsigned char i2c_op(struct sh_mobile_i2c_data *pd, enum sh_mobile_i2c_op ret = iic_rd(pd, ICDR); break; case OP_RX_STOP: /* enable DTE interrupt, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; case OP_RX_STOP_DATA: /* enable DTE interrupt, read data, issue stop */ - iic_wr(pd, ICIC, - ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); + if (!pd->atomic_xfer) + iic_wr(pd, ICIC, + ICIC_DTEE | ICIC_WAITE | ICIC_ALE | ICIC_TACKE); ret = iic_rd(pd, ICDR); iic_wr(pd, ICCR, ICCR_ICE | ICCR_RACK); break; @@ -429,7 +432,8 @@ static irqreturn_t sh_mobile_i2c_isr(int irq, void *dev_id) if (wakeup) { pd->sr |= SW_DONE; - wake_up(&pd->wait); + if (!pd->atomic_xfer) + wake_up(&pd->wait); } /* defeat write posting to avoid spurious WAIT interrupts */ @@ -581,6 +585,9 @@ static void start_ch(struct sh_mobile_i2c_data *pd, struct i2c_msg *usr_msg, pd->pos = -1; pd->sr = 0; + if (pd->atomic_xfer) + return; + pd->dma_buf = i2c_get_dma_safe_msg_buf(pd->msg, 8); if (pd->dma_buf) sh_mobile_i2c_xfer_dma(pd); @@ -637,15 +644,13 @@ static int poll_busy(struct sh_mobile_i2c_data *pd) return i ? 0 : -ETIMEDOUT; } -static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, - struct i2c_msg *msgs, - int num) +static int sh_mobile_xfer(struct sh_mobile_i2c_data *pd, + struct i2c_msg *msgs, int num) { - struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); struct i2c_msg *msg; int err = 0; int i; - long timeout; + long time_left; /* Wake up device and enable clock */ pm_runtime_get_sync(pd->dev); @@ -662,15 +667,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, if (do_start) i2c_op(pd, OP_START); - /* The interrupt handler takes care of the rest... */ - timeout = wait_event_timeout(pd->wait, - pd->sr & (ICSR_TACK | SW_DONE), - adapter->timeout); - - /* 'stop_after_dma' tells if DMA transfer was complete */ - i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, pd->stop_after_dma); + if (pd->atomic_xfer) { + unsigned long j = jiffies + pd->adap.timeout; + + time_left = time_before_eq(jiffies, j); + while (time_left && + !(pd->sr & (ICSR_TACK | SW_DONE))) { + unsigned char sr = iic_rd(pd, ICSR); + + if (sr & (ICSR_AL | ICSR_TACK | + ICSR_WAIT | ICSR_DTE)) { + sh_mobile_i2c_isr(0, pd); + udelay(150); + } else { + cpu_relax(); + } + time_left = time_before_eq(jiffies, j); + } + } else { + /* The interrupt handler takes care of the rest... */ + time_left = wait_event_timeout(pd->wait, + pd->sr & (ICSR_TACK | SW_DONE), + pd->adap.timeout); + + /* 'stop_after_dma' tells if DMA xfer was complete */ + i2c_put_dma_safe_msg_buf(pd->dma_buf, pd->msg, + pd->stop_after_dma); + } - if (!timeout) { + if (!time_left) { dev_err(pd->dev, "Transfer request timed out\n"); if (pd->dma_direction != DMA_NONE) sh_mobile_i2c_cleanup_dma(pd); @@ -696,14 +721,35 @@ static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, return err ?: num; } +static int sh_mobile_i2c_xfer(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = false; + return sh_mobile_xfer(pd, msgs, num); +} + +static int sh_mobile_i2c_xfer_atomic(struct i2c_adapter *adapter, + struct i2c_msg *msgs, + int num) +{ + struct sh_mobile_i2c_data *pd = i2c_get_adapdata(adapter); + + pd->atomic_xfer = true; + return sh_mobile_xfer(pd, msgs, num); +} + static u32 sh_mobile_i2c_func(struct i2c_adapter *adapter) { return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING; } static const struct i2c_algorithm sh_mobile_i2c_algorithm = { - .functionality = sh_mobile_i2c_func, - .master_xfer = sh_mobile_i2c_xfer, + .functionality = sh_mobile_i2c_func, + .master_xfer = sh_mobile_i2c_xfer, + .master_xfer_atomic = sh_mobile_i2c_xfer_atomic, }; static const struct i2c_adapter_quirks sh_mobile_i2c_quirks = { -- 2.27.0

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/20] [Set 3] Rid W=1 warnings from GPU

by Alex Deucher

On Mon, Nov 9, 2020 at 4:19 PM Lee Jones <lee.jones(a)linaro.org> wrote: > > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > This set takes the running (decreasing) total from 5000 before > [Set 1] down to 2300. > > Lee Jones (20): > drm/radeon/radeon_ttm: Place declaration of 'rdev' in same clause as > its use > drm/radeon: Move radeon_ttm{init,fini} to shared location > drm/radeon/radeon_fence: Demote some non-conformant kernel-doc headers > and fix another > drm/radeon: Move 'radeon_add_legacy_encoder' prototype to shared > header > drm/radeon: Move 'radeon_add_legacy_encoder's prototype to shared > location > drm/radeon/radeon_connectors: Strip out set but unused variable 'ret' > drm/radeon/radeon_display: Remove unused variable 'mod' > drm/radeon/radeon_i2c: Remove pointless function header > drm/radeon/radeon_irq_kms: Demote non-conformant kernel-doc fix > another > drm/radeon/radeon_ring: Add missing function parameters 'rdev' and > 'data' > drm/radeon/r600: Strip out set but unused 'tmp' variables > drm/radeon/radeon_cs: Fix a bunch of doc-rot issues > drm/radeon/evergreen: Move 'rv770_set_clk_bypass_mode' prototype to > shared location > drm/radeon: Move 'radeon_pm_acpi_event_handler' prototype into shared > header > drm/radeon/r600d: Move 'rc600_*' prototypes into shared header > drm/radeon/radeon_audio: Move 'dce3_2_*' prototypes to shared location > drm/radeon/evergreend: Move 'evergreen_*' and 'sumo_*' prototypes out > to shared location > drm/radeon/radeon_display: Fix function doc formatting and missing > param issues > drm/radeon/r600: Fix a misnamed parameter description and a formatting > issue > drm/radeon/cik: Fix a bunch of function parameter descriptions > > drivers/gpu/drm/radeon/cik.c | 10 +++---- > drivers/gpu/drm/radeon/evergreen.c | 1 - > drivers/gpu/drm/radeon/ni.c | 16 ---------- > drivers/gpu/drm/radeon/r600.c | 11 +++---- > drivers/gpu/drm/radeon/r600d.h | 14 +++++++++ > drivers/gpu/drm/radeon/radeon.h | 34 ++++++++++++++++++++++ > drivers/gpu/drm/radeon/radeon_acpi.c | 2 -- > drivers/gpu/drm/radeon/radeon_atombios.c | 5 ---- > drivers/gpu/drm/radeon/radeon_audio.c | 23 +-------------- > drivers/gpu/drm/radeon/radeon_audio.h | 12 ++++++++ > drivers/gpu/drm/radeon/radeon_combios.c | 5 ---- > drivers/gpu/drm/radeon/radeon_connectors.c | 4 +-- > drivers/gpu/drm/radeon/radeon_cs.c | 12 ++++---- > drivers/gpu/drm/radeon/radeon_display.c | 17 +++++------ > drivers/gpu/drm/radeon/radeon_encoders.c | 3 -- > drivers/gpu/drm/radeon/radeon_fence.c | 6 ++-- > drivers/gpu/drm/radeon/radeon_i2c.c | 4 --- > drivers/gpu/drm/radeon/radeon_irq_kms.c | 6 ++-- > drivers/gpu/drm/radeon/radeon_object.c | 2 -- > drivers/gpu/drm/radeon/radeon_ring.c | 3 ++ > drivers/gpu/drm/radeon/radeon_ttm.c | 8 ++--- > drivers/gpu/drm/radeon/si.c | 6 ---- > 22 files changed, 96 insertions(+), 108 deletions(-) Instead of just dumping everything in radeon.h, I think it would be cleaner to add new headers that match the .c files that define them. E.g., add evergreen.h and put all the stuff from evergreen.c in there. Add rv770.h and add all the stuff defined in rv770.c, etc. Alex > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: David Airlie <airlied(a)linux.ie> > Cc: dri-devel(a)lists.freedesktop.org > Cc: Jerome Glisse <glisse(a)freedesktop.org> > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: linux-media(a)vger.kernel.org > Cc: report to <xorg-driver-ati(a)lists.x.org> > Cc: Slava Grigorev <slava.grigorev(a)amd.com> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > -- > 2.25.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drivers: amdgpu: amdgpu_display: Fixed the spelling of falg to flag

by Alex Deucher

On Mon, Nov 9, 2020 at 4:16 PM Bhaskar Chowdhury <unixbhaskar(a)gmail.com> wrote: > > s/falg/flag/p > > Signed-off-by: Bhaskar Chowdhury <unixbhaskar(a)gmail.com> Applied. Thanks! Alex > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index 2e8a8b57639f..9223502c1e5b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -509,7 +509,7 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev, > * to avoid hang caused by placement of scanout BO in GTT on certain > * APUs. So force the BO placement to VRAM in case this architecture > * will not allow USWC mappings. > - * Also, don't allow GTT domain if the BO doens't have USWC falg set. > + * Also, don't allow GTT domain if the BO doens't have USWC flag set. > */ > if ((bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC) && > amdgpu_bo_support_uswc(bo_flags) && > -- > 2.26.2 > > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 14/19] drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr' params

by Alex Deucher

On Fri, Nov 6, 2020 at 4:50 PM Lee Jones <lee.jones(a)linaro.org> wrote: > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:594: warning: Function parameter or member 'reg_addr' not described in 'amdgpu_device_indirect_rreg' > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:624: warning: Function parameter or member 'reg_addr' not described in 'amdgpu_device_indirect_rreg64' > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: David Airlie <airlied(a)linux.ie> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: dri-devel(a)lists.freedesktop.org > Cc: linux-media(a)vger.kernel.org > Cc: linaro-mm-sig(a)lists.linaro.org > Signed-off-by: Lee Jones <lee.jones(a)linaro.org> Applied. Thanks! Alex > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 300fcade4a2b1..63374d12e00fe 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -585,6 +585,7 @@ void amdgpu_mm_wdoorbell64(struct amdgpu_device *adev, u32 index, u64 v) > * @adev: amdgpu_device pointer > * @pcie_index: mmio register offset > * @pcie_data: mmio register offset > + * @reg_addr: indirect register address to read from > * > * Returns the value of indirect register @reg_addr > */ > @@ -615,6 +616,7 @@ u32 amdgpu_device_indirect_rreg(struct amdgpu_device *adev, > * @adev: amdgpu_device pointer > * @pcie_index: mmio register offset > * @pcie_data: mmio register offset > + * @reg_addr: indirect register address to read from > * > * Returns the value of indirect register @reg_addr > */ > -- > 2.25.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drivers: amdgpu: amdgpu_display.c: Fix a spelling doens\'t to doesn\'t

by Alex Deucher

On Mon, Nov 9, 2020 at 3:52 AM Bhaskar Chowdhury <unixbhaskar(a)gmail.com> wrote: > > s/doens't/doesn't/p > > Signed-off-by: Bhaskar Chowdhury <unixbhaskar(a)gmail.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > index 7cc7af2a6822..a92cb137293a 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c > @@ -512,7 +512,7 @@ uint32_t amdgpu_display_supported_domains(struct amdgpu_device *adev, > * to avoid hang caused by placement of scanout BO in GTT on certain > * APUs. So force the BO placement to VRAM in case this architecture > * will not allow USWC mappings. > - * Also, don't allow GTT domain if the BO doens't have USWC falg set. > + * Also, don't allow GTT domain if the BO doesn't have USWC falg set. Applied. Care to send a patch to also fix the spelling of "flag"? Thanks, Alex > */ > if ((bo_flags & AMDGPU_GEM_CREATE_CPU_GTT_USWC) && > amdgpu_bo_support_uswc(bo_flags) && > -- > 2.26.2 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 10/19] drm/radeon/radeon: Move prototype into shared header

by Sam Ravnborg

Hi Lee, > > > > Other public functions in radeon_device.c have their prototype in > > radeon.h - for example radeon_is_px() > > > > Add radeon_device_is_virtual() there so we avoiid this new header. > > Oh yes, I remember why this wasn't a suitable solution now: > > The macro "radeon_init" in radeon.h clashes with the init function of > the same name in radeon_drv.c: > > In file included from drivers/gpu/drm/radeon/radeon_drv.c:53: > drivers/gpu/drm/radeon/radeon_drv.c:620:31: error: expected identifier or ‘(’ before ‘void’ > 620 | static int __init radeon_init(void) ... > > How would you like me to move forward? Fix the thousand of warnings in other places :-) I will take a look at radeon and post a new series based on your work with all W=1 warnings fixed. Sam

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Sam Ravnborg

On Sat, Nov 07, 2020 at 06:41:38PM +0000, Lee Jones wrote: > On Sat, 07 Nov 2020, Sam Ravnborg wrote: > > > Hi Christian. > > > > > I'm not sure if we want to do some of the suggested changes to radeon. > > > > All patches for radeon looks good to me except "drm/radeon/radeon: Move > > prototype into shared header". > > Was that the one where the prototype needs moving to radeon.h? Yes, Sam

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Christian König

Well that's quite a patch set. First of all can you separate this a bit more by driver? I'm assuming we maintainers are supposed to pick that up and apply it. radeon and amdgpu can stick together since that is mostly Alex and me, but I'm not sure if we want to do some of the suggested changes to radeon. Going to pick up the single TTM change for upstreaming. Thanks, Christian. Am 06.11.20 um 22:49 schrieb Lee Jones: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > There are 5000 warnings to work through. It will take a couple more > sets. Although, ("drm/amd/display/dc/basics/fixpt31_32: Move > variables to where they're used") does take care of 2000 of them! > > Lee Jones (19): > drm/ttm/ttm_range_manager: Demote non-conformant kernel-doc header > drm/r128/ati_pcigart: Source file headers are not good candidates for > kernel-doc > drm/selftests/test-drm_dp_mst_helper: Move > 'sideband_msg_req_encode_decode' onto the heap > drm/mga/mga_dma: Demote kernel-doc abusers to standard comment blocks > drm/mga/mga_state: Remove unused variable 'buf_priv' > drm/radeon/atom: Move prototype into shared location > drm/radeon/radeon_kms: Include header containing our own prototypes > drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions > drm/omapdrm/omap_dmm_tiler: Demote abusive use of kernel-doc format > drm/radeon/radeon: Move prototype into shared header > drm/radeon/radeon_drv: Source file headers are not good candidates for > kernel-doc > drm/amd/display/dc/basics/fixpt31_32: Move variables to where they're > used > drm/radeon/radeon_drv: Move prototypes to a shared headerfile > drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr' > params > drm/radeon: Move prototypes to shared header > drm/amd/amdgpu/amdgpu_kms: Remove 'struct drm_amdgpu_info_device > dev_info' from the stack > drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param > drm/radeon/atombios_crtc: Remove description of non-existent function > param 'encoder' > drm/v3d/v3d_drv: Remove unused static variable 'v3d_v3d_pm_ops' > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 + > drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 104 +++++++++--------- > .../drm/amd/display/dc/basics/fixpt31_32.c | 5 + > .../gpu/drm/amd/display/include/fixed31_32.h | 6 - > drivers/gpu/drm/mga/mga_dma.c | 10 +- > drivers/gpu/drm/mga/mga_state.c | 2 - > drivers/gpu/drm/omapdrm/omap_dmm_tiler.c | 6 +- > drivers/gpu/drm/omapdrm/omap_gem.c | 3 +- > drivers/gpu/drm/r128/ati_pcigart.c | 2 +- > drivers/gpu/drm/radeon/atom.h | 6 + > drivers/gpu/drm/radeon/atombios_crtc.c | 1 - > drivers/gpu/drm/radeon/atombios_encoders.c | 4 - > drivers/gpu/drm/radeon/radeon.h | 6 + > drivers/gpu/drm/radeon/radeon_device.c | 1 + > drivers/gpu/drm/radeon/radeon_device.h | 32 ++++++ > drivers/gpu/drm/radeon/radeon_display.c | 4 - > drivers/gpu/drm/radeon/radeon_drv.c | 11 +- > drivers/gpu/drm/radeon/radeon_drv.h | 7 ++ > drivers/gpu/drm/radeon/radeon_kms.c | 3 +- > .../drm/selftests/test-drm_dp_mst_helper.c | 11 +- > drivers/gpu/drm/ttm/ttm_range_manager.c | 2 +- > drivers/gpu/drm/v3d/v3d_drv.c | 36 ------ > 22 files changed, 138 insertions(+), 126 deletions(-) > create mode 100644 drivers/gpu/drm/radeon/radeon_device.h > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: Andy Gross <andy.gross(a)ti.com> > Cc: by <jhartmann(a)precisioninsight.com> > Cc: Christian Koenig <christian.koenig(a)amd.com> > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: David Airlie <airlied(a)linux.ie> > Cc: dri-devel(a)lists.freedesktop.org > Cc: Eric Anholt <eric(a)anholt.net> > Cc: Faith <faith(a)valinux.com> > Cc: Gareth Hughes <gareth(a)valinux.com> > Cc: Harry Wentland <harry.wentland(a)amd.com> > Cc: Huang Rui <ray.huang(a)amd.com> > Cc: Jeff Hartmann <jhartmann(a)valinux.com> > Cc: Keith Whitwell <keith(a)tungstengraphics.com> > Cc: Leo Li <sunpeng.li(a)amd.com> > Cc: linaro-mm-sig(a)lists.linaro.org > Cc: linux-media(a)vger.kernel.org > Cc: Philipp Zabel <p.zabel(a)pengutronix.de> > Cc: Rob Clark <rob.clark(a)linaro.org> > Cc: Rob Clark <rob(a)ti.com> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: Tomi Valkeinen <tomi.valkeinen(a)ti.com>

5 years, 1 month

2
1
0 0

Re: [Linaro-mm-sig] [PATCH 10/19] drm/radeon/radeon: Move prototype into shared header

by Sam Ravnborg

Hi Lee, On Fri, Nov 06, 2020 at 09:49:40PM +0000, Lee Jones wrote: > Unfortunately, a suitable one didn't already exist. > > Fixes the following W=1 kernel build warning(s): > > drivers/gpu/drm/radeon/radeon_device.c:637:6: warning: no previous prototype for ‘radeon_device_is_virtual’ [-Wmissing-prototypes] > 637 | bool radeon_device_is_virtual(void) > | ^~~~~~~~~~~~~~~~~~~~~~~~ > > Cc: Alex Deucher <alexander.deucher(a)amd.com> > Cc: "Christian König" <christian.koenig(a)amd.com> > Cc: David Airlie <airlied(a)linux.ie> > Cc: Daniel Vetter <daniel(a)ffwll.ch> > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > Cc: amd-gfx(a)lists.freedesktop.org > Cc: dri-devel(a)lists.freedesktop.org > Cc: linux-media(a)vger.kernel.org > Cc: linaro-mm-sig(a)lists.linaro.org > Signed-off-by: Lee Jones <lee.jones(a)linaro.org> > --- > drivers/gpu/drm/radeon/radeon_device.c | 1 + > drivers/gpu/drm/radeon/radeon_device.h | 32 ++++++++++++++++++++++++++ > drivers/gpu/drm/radeon/radeon_drv.c | 3 +-- > 3 files changed, 34 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/radeon/radeon_device.h Other public functions in radeon_device.c have their prototype in radeon.h - for example radeon_is_px() Add radeon_device_is_virtual() there so we avoiid this new header. Sam

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 00/19] [Set 2] Rid W=1 warnings from GPU

by Sam Ravnborg

Hi Lee and DRM folks. On Fri, Nov 06, 2020 at 09:49:30PM +0000, Lee Jones wrote: > This set is part of a larger effort attempting to clean-up W=1 > kernel builds, which are currently overwhelmingly riddled with > niggly little warnings. > > There are 5000 warnings to work through. It will take a couple more > sets. Although, ("drm/amd/display/dc/basics/fixpt31_32: Move > variables to where they're used") does take care of 2000 of them! > > Lee Jones (19): > drm/ttm/ttm_range_manager: Demote non-conformant kernel-doc header > drm/r128/ati_pcigart: Source file headers are not good candidates for > kernel-doc Applied > drm/selftests/test-drm_dp_mst_helper: Move > 'sideband_msg_req_encode_decode' onto the heap > drm/mga/mga_dma: Demote kernel-doc abusers to standard comment blocks > drm/mga/mga_state: Remove unused variable 'buf_priv' Applied x2 > drm/radeon/atom: Move prototype into shared location > drm/radeon/radeon_kms: Include header containing our own prototypes > drm/omapdrm/omap_gem: Fix misnamed and missing parameter descriptions > drm/omapdrm/omap_dmm_tiler: Demote abusive use of kernel-doc format > drm/radeon/radeon: Move prototype into shared header > drm/radeon/radeon_drv: Source file headers are not good candidates for > kernel-doc > drm/amd/display/dc/basics/fixpt31_32: Move variables to where they're > used > drm/radeon/radeon_drv: Move prototypes to a shared headerfile > drm/amd/amdgpu/amdgpu_device: Provide documentation for 'reg_addr' > params > drm/radeon: Move prototypes to shared header > drm/amd/amdgpu/amdgpu_kms: Remove 'struct drm_amdgpu_info_device > dev_info' from the stack > drm/radeon/radeon_kms: Fix misnaming of 'radeon_info_ioctl's dev param > drm/radeon/atombios_crtc: Remove description of non-existent function > param 'encoder' > drm/v3d/v3d_drv: Remove unused static variable 'v3d_v3d_pm_ops' I have applied the three patches that has no obvious maintainer as indicated above. I assume the respective maintaines to pick radeon, omapdrm, ttm, amd, v3d and selftest patches. Sam

5 years, 1 month

1
0
0 0

[PATCH 1/2] mm: mmap: fix fput in error path v2

by Christian König

Patch "495c10cc1c0c CHROMIUM: dma-buf: restore args..." adds a workaround for a bug in mmap_region. As the comment states ->mmap() callback can change vma->vm_file and so we might call fput() on the wrong file. Revert the workaround and proper fix this in mmap_region. v2: drop the extra if in dma_buf_mmap as well Signed-off-by: Christian König <christian.koenig(a)amd.com> Reviewed-by: Jason Gunthorpe <jgg(a)nvidia.com> --- drivers/dma-buf/dma-buf.c | 20 +++----------------- mm/mmap.c | 2 +- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a6ba4d598f0e..08630d057cf2 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1143,9 +1143,6 @@ EXPORT_SYMBOL_GPL(dma_buf_end_cpu_access); int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, unsigned long pgoff) { - struct file *oldfile; - int ret; - if (WARN_ON(!dmabuf || !vma)) return -EINVAL; @@ -1163,22 +1160,11 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, return -EINVAL; /* readjust the vma */ - get_file(dmabuf->file); - oldfile = vma->vm_file; - vma->vm_file = dmabuf->file; + fput(vma->vm_file); + vma->vm_file = get_file(dmabuf->file); vma->vm_pgoff = pgoff; - ret = dmabuf->ops->mmap(dmabuf, vma); - if (ret) { - /* restore old parameters on failure */ - vma->vm_file = oldfile; - fput(dmabuf->file); - } else { - if (oldfile) - fput(oldfile); - } - return ret; - + return dmabuf->ops->mmap(dmabuf, vma); } EXPORT_SYMBOL_GPL(dma_buf_mmap); diff --git a/mm/mmap.c b/mm/mmap.c index 40248d84ad5f..3a2670d73355 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1852,8 +1852,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, return addr; unmap_and_free_vma: + fput(vma->vm_file); vma->vm_file = NULL; - fput(file); /* Undo any partial mapping done by a device driver. */ unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end); -- 2.17.1

5 years, 1 month

2
4
0 0

[PATCH 5.9 267/391] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Greg Kroah-Hartman

From: Daniel Vetter <daniel.vetter(a)ffwll.ch> commit f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a upstream. When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Acked-by: Christian König <christian.koenig(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20201027214922.3566743-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1085,6 +1085,8 @@ int drm_gem_mmap_obj(struct drm_gem_obje */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1107,8 +1109,6 @@ int drm_gem_mmap_obj(struct drm_gem_obje vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -594,8 +594,13 @@ int drm_gem_shmem_mmap(struct drm_gem_ob /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj);

5 years, 1 month

1
0
0 0

Patch "drm/shme-helpers: Fix dma_buf_mmap forwarding bug" has been added to the 5.9-stable tree

by gregkh＠linuxfoundation.org

This is a note to let you know that I've just added the patch titled drm/shme-helpers: Fix dma_buf_mmap forwarding bug to the 5.9-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=sum… The filename of the patch is: drm-shme-helpers-fix-dma_buf_mmap-forwarding-bug.patch and it can be found in the queue-5.9 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable(a)vger.kernel.org> know about it. >From f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a Mon Sep 17 00:00:00 2001 From: Daniel Vetter <daniel.vetter(a)ffwll.ch> Date: Tue, 27 Oct 2020 22:49:22 +0100 Subject: drm/shme-helpers: Fix dma_buf_mmap forwarding bug MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From: Daniel Vetter <daniel.vetter(a)ffwll.ch> commit f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a upstream. When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Acked-by: Christian König <christian.koenig(a)amd.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20201027214922.3566743-1-dani… Signed-off-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1085,6 +1085,8 @@ int drm_gem_mmap_obj(struct drm_gem_obje */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1107,8 +1109,6 @@ int drm_gem_mmap_obj(struct drm_gem_obje vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -594,8 +594,13 @@ int drm_gem_shmem_mmap(struct drm_gem_ob /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj); Patches currently in stable-queue which might be from daniel.vetter(a)ffwll.ch are queue-5.9/drm-ast-separate-drm-driver-from-pci-code.patch queue-5.9/drm-shme-helpers-fix-dma_buf_mmap-forwarding-bug.patch

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] WARNING in dma_map_page_attrs

by hch＠lst.de

On Tue, Oct 27, 2020 at 12:52:30PM +0000, Parav Pandit wrote: > > > From: hch(a)lst.de <hch(a)lst.de> > > Sent: Tuesday, October 27, 2020 1:41 PM > > > > On Mon, Oct 26, 2020 at 05:23:48AM +0000, Parav Pandit wrote: > > > Hi Christoph, > > > > > > > From: Jakub Kicinski <kuba(a)kernel.org> > > > > Sent: Saturday, October 24, 2020 11:45 PM > > > > > > > > CC: rdma, looks like rdma from the stack trace > > > > > > > > On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote: > > > > > syzbot has found a reproducer for the following issue on: > > > > > > > > > > HEAD commit: 3cb12d27 Merge tag 'net-5.10-rc1' of > > git://git.kernel.org/.. > > > > > > In [1] you mentioned that dma_mask should not be set for dma_virt_ops. > > > So patch [2] removed it. > > > > > > But check to validate the dma mask for all dma_ops was added in [3]. > > > > > > What is the right way? Did I misunderstood your comment about > > dma_mask in [1]? > > > > No, I did not say we don't need the mask. I said copying over the various > > dma-related fields from the parent is bogus. > > > > I think rxe (and ther other drivers/infiniband/sw drivers) need a simple > > dma_coerce_mask_and_coherent and nothing else. > > I see. Does below fix make sense? > Is DMA_MASK_NONE correct? DMA_MASK_NONE is gone in 5.10. I think you want DMA_BIT_MASK(64). That isn't actually correct for 32-bit platforms, but good enough.

5 years, 1 month

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Daniel Vetter

On Wed, Oct 28, 2020 at 09:44:15AM +0100, Boris Brezillon wrote: > On Tue, 27 Oct 2020 22:49:22 +0100 > Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > When we forward an mmap to the dma_buf exporter, they get to own > > everything. Unfortunately drm_gem_mmap_obj() overwrote > > vma->vm_private_data after the driver callback, wreaking the > > exporter complete. This was noticed because vb2_common_vm_close blew > > up on mali gpu with panfrost after commit 26d3ac3cb04d > > ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). > > > > Unfortunately drm_gem_mmap_obj also acquires a surplus reference that > > we need to drop in shmem helpers, which is a bit of a mislayer > > situation. Maybe the entire dma_buf_mmap forwarding should be pulled > > into core gem code. > > > > Note that the only two other drivers which forward mmap in their own > > code (etnaviv and exynos) get this somewhat right by overwriting the > > gem mmap code. But they seem to still have the leak. This might be a > > good excuse to move these drivers over to shmem helpers completely. > > > > Note to stable team: There's a trivial context conflict with > > d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from > > struct drm_driver"). I'm assuming it's clear where to put the first > > hunk, otherwise I can send a 5.9 version of this. > > > > Cc: Christian König <christian.koenig(a)amd.com> > > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > > Cc: Lucas Stach <l.stach(a)pengutronix.de> > > Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> > > Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> > > Cc: Inki Dae <inki.dae(a)samsung.com> > > Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> > > Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> > > Cc: Kyungmin Park <kyungmin.park(a)samsung.com> > > Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") > > Cc: Boris Brezillon <boris.brezillon(a)collabora.com> > > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Patch pushed to drm-misc-fixes, thanks for taking a look. -Daniel > > > Cc: Thomas Zimmermann <tzimmermann(a)suse.de> > > Cc: Gerd Hoffmann <kraxel(a)redhat.com> > > Cc: Rob Herring <robh(a)kernel.org> > > Cc: dri-devel(a)lists.freedesktop.org > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > Cc: <stable(a)vger.kernel.org> # v5.9+ > > Reported-and-tested-by: piotr.oniszczuk(a)gmail.com > > Cc: piotr.oniszczuk(a)gmail.com > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > --- > > drivers/gpu/drm/drm_gem.c | 4 ++-- > > drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > > index 1da67d34e55d..d586068f5509 100644 > > --- a/drivers/gpu/drm/drm_gem.c > > +++ b/drivers/gpu/drm/drm_gem.c > > @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > */ > > drm_gem_object_get(obj); > > > > + vma->vm_private_data = obj; > > + > > if (obj->funcs->mmap) { > > ret = obj->funcs->mmap(obj, vma); > > if (ret) { > > @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, > > vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > > } > > > > - vma->vm_private_data = obj; > > - > > return 0; > > } > > EXPORT_SYMBOL(drm_gem_mmap_obj); > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > > index fb11df7aced5..8233bda4692f 100644 > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > > @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > /* Remove the fake offset */ > > vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); > > > > - if (obj->import_attach) > > + if (obj->import_attach) { > > + /* Drop the reference drm_gem_mmap_obj() acquired.*/ > > + drm_gem_object_put(obj); > > + vma->vm_private_data = NULL; > > + > > return dma_buf_mmap(obj->dma_buf, vma, 0); > > + } > > > > shmem = to_drm_gem_shmem_obj(obj); > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

5 years, 1 month

1
0
0 0

[PATCH] drm/shme-helpers: Fix dma_buf_mmap forwarding bug

by Daniel Vetter

When we forward an mmap to the dma_buf exporter, they get to own everything. Unfortunately drm_gem_mmap_obj() overwrote vma->vm_private_data after the driver callback, wreaking the exporter complete. This was noticed because vb2_common_vm_close blew up on mali gpu with panfrost after commit 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf"). Unfortunately drm_gem_mmap_obj also acquires a surplus reference that we need to drop in shmem helpers, which is a bit of a mislayer situation. Maybe the entire dma_buf_mmap forwarding should be pulled into core gem code. Note that the only two other drivers which forward mmap in their own code (etnaviv and exynos) get this somewhat right by overwriting the gem mmap code. But they seem to still have the leak. This might be a good excuse to move these drivers over to shmem helpers completely. Note to stable team: There's a trivial context conflict with d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver"). I'm assuming it's clear where to put the first hunk, otherwise I can send a 5.9 version of this. Cc: Christian König <christian.koenig(a)amd.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Russell King <linux+etnaviv(a)armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf") Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Rob Herring <robh(a)kernel.org> Cc: dri-devel(a)lists.freedesktop.org Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> # v5.9+ Reported-and-tested-by: piotr.oniszczuk(a)gmail.com Cc: piotr.oniszczuk(a)gmail.com Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/drm_gem.c | 4 ++-- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 1da67d34e55d..d586068f5509 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, */ drm_gem_object_get(obj); + vma->vm_private_data = obj; + if (obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) { @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); } - vma->vm_private_data = obj; - return 0; } EXPORT_SYMBOL(drm_gem_mmap_obj); diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index fb11df7aced5..8233bda4692f 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) /* Remove the fake offset */ vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - if (obj->import_attach) + if (obj->import_attach) { + /* Drop the reference drm_gem_mmap_obj() acquired.*/ + drm_gem_object_put(obj); + vma->vm_private_data = NULL; + return dma_buf_mmap(obj->dma_buf, vma, 0); + } shmem = to_drm_gem_shmem_obj(obj); -- 2.28.0

5 years, 1 month

2
1
0 0

Re: [Linaro-mm-sig] WARNING in dma_map_page_attrs

by hch＠lst.de

On Mon, Oct 26, 2020 at 05:23:48AM +0000, Parav Pandit wrote: > Hi Christoph, > > > From: Jakub Kicinski <kuba(a)kernel.org> > > Sent: Saturday, October 24, 2020 11:45 PM > > > > CC: rdma, looks like rdma from the stack trace > > > > On Fri, 23 Oct 2020 20:07:17 -0700 syzbot wrote: > > > syzbot has found a reproducer for the following issue on: > > > > > > HEAD commit: 3cb12d27 Merge tag 'net-5.10-rc1' of git://git.kernel.org/.. > > In [1] you mentioned that dma_mask should not be set for dma_virt_ops. > So patch [2] removed it. > > But check to validate the dma mask for all dma_ops was added in [3]. > > What is the right way? Did I misunderstood your comment about dma_mask in [1]? No, I did not say we don't need the mask. I said copying over the various dma-related fields from the parent is bogus. I think rxe (and ther other drivers/infiniband/sw drivers) need a simple dma_coerce_mask_and_coherent and nothing else.

5 years, 1 month

1
0
0 0

[PATCH v4 00/23] drm/msm: de-struct_mutex-ification

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> This doesn't remove *all* the struct_mutex, but it covers the worst of it, ie. shrinker/madvise/free/retire. The submit path still uses struct_mutex, but it still needs *something* serialize a portion of the submit path, and lock_stat mostly just shows the lock contention there being with other submits. And there are a few other bits of struct_mutex usage in less critical paths (debugfs, etc). But this seems like a reasonable step in the right direction. v2: teach lockdep about shrinker locking patters (danvet) and convert to obj->resv locking (danvet) v3: fix get_vaddr locking for legacy userspace (relocs), devcoredump, and rd/hangrd v4: couple minor review comments (krh), fix deadlock with imported dma-buf's (ie. from v4l2, etc) Rob Clark (23): drm/msm: Fix a couple incorrect usages of get_vaddr_active() drm/msm/gem: Add obj->lock wrappers drm/msm/gem: Rename internal get_iova_locked helper drm/msm/gem: Move prototypes to msm_gem.h drm/msm/gem: Add some _locked() helpers drm/msm/gem: Move locking in shrinker path drm/msm/submit: Move copy_from_user ahead of locking bos drm/msm: Do rpm get sooner in the submit path drm/msm/gem: Switch over to obj->resv for locking drm/msm: Use correct drm_gem_object_put() in fail case drm/msm: Drop chatty trace drm/msm: Move update_fences() drm/msm: Add priv->mm_lock to protect active/inactive lists drm/msm: Document and rename preempt_lock drm/msm: Protect ring->submits with it's own lock drm/msm: Refcount submits drm/msm: Remove obj->gpu drm/msm: Drop struct_mutex from the retire path drm/msm: Drop struct_mutex in free_object() path drm/msm: Remove msm_gem_free_work drm/msm: Drop struct_mutex in madvise path drm/msm: Drop struct_mutex in shrinker path drm/msm: Don't implicit-sync if only a single ring drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 6 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c | 1 + drivers/gpu/drm/msm/msm_debugfs.c | 7 + drivers/gpu/drm/msm/msm_drv.c | 21 +- drivers/gpu/drm/msm/msm_drv.h | 73 +----- drivers/gpu/drm/msm/msm_fbdev.c | 1 + drivers/gpu/drm/msm/msm_gem.c | 271 +++++++++++----------- drivers/gpu/drm/msm/msm_gem.h | 133 +++++++++-- drivers/gpu/drm/msm/msm_gem_shrinker.c | 81 ++----- drivers/gpu/drm/msm/msm_gem_submit.c | 164 ++++++++----- drivers/gpu/drm/msm/msm_gpu.c | 110 +++++---- drivers/gpu/drm/msm/msm_gpu.h | 5 +- drivers/gpu/drm/msm/msm_rd.c | 2 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +- drivers/gpu/drm/msm/msm_ringbuffer.h | 13 +- 19 files changed, 506 insertions(+), 405 deletions(-) -- 2.26.2

5 years, 1 month

1
1
0 0

[PATCH 23/65] drm/i915: Annotate dma_fence_work

by Daniel Vetter

i915 does tons of allocations from this worker, which lockdep catches. Also generic infrastructure like this with big potential for how dma_fence or other cross driver contracts work, really should be reviewed on dri-devel. Implementing custom wheels for everything within the driver is a classic case of "platform problem" [1]. Which in upstream we really shouldn't have. Since there's no quick way to solve these splats (dma_fence_work is used a bunch in basic buffer management and command submission) like for amdgpu, I'm giving up at this point here. Annotating i915 scheduler and gpu reset could would be interesting, but since lockdep is one-shot we can't see what surprises would lurk there. 1: https://lwn.net/Articles/443531/ Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/i915/i915_sw_fence_work.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_sw_fence_work.c b/drivers/gpu/drm/i915/i915_sw_fence_work.c index a3a81bb8f2c3..5b74acadaef5 100644 --- a/drivers/gpu/drm/i915/i915_sw_fence_work.c +++ b/drivers/gpu/drm/i915/i915_sw_fence_work.c @@ -17,12 +17,15 @@ static void fence_work(struct work_struct *work) { struct dma_fence_work *f = container_of(work, typeof(*f), work); int err; + bool fence_cookie; + fence_cookie = dma_fence_begin_signalling(); err = f->ops->work(f); if (err) dma_fence_set_error(&f->dma, err); fence_complete(f); + dma_fence_end_signalling(fence_cookie); dma_fence_put(&f->dma); } -- 2.28.0

5 years, 1 month

1
0
0 0

[PATCH 22/65] Revert "drm/amdgpu: add fbdev suspend/resume on gpu reset"

by Daniel Vetter

This is one from the department of "maybe play lottery if you hit this, karma compensation might work". Or at least lockdep ftw! This reverts commit 565d1941557756a584ac357d945bc374d5fcd1d0. It's not quite as low-risk as the commit message claims, because this grabs console_lock, which might be held when we allocate memory, which might never happen because the dma_fence_wait() is stuck waiting on our gpu reset: [ 136.763714] ====================================================== [ 136.763714] WARNING: possible circular locking dependency detected [ 136.763715] 5.7.0-rc3+ #346 Tainted: G W [ 136.763716] ------------------------------------------------------ [ 136.763716] kworker/2:3/682 is trying to acquire lock: [ 136.763716] ffffffff8226f140 (console_lock){+.+.}-{0:0}, at: drm_fb_helper_set_suspend_unlocked+0x7b/0xa0 [drm_kms_helper] [ 136.763723] but task is already holding lock: [ 136.763724] ffffffff82318c80 (dma_fence_map){++++}-{0:0}, at: drm_sched_job_timedout+0x25/0xf0 [gpu_sched] [ 136.763726] which lock already depends on the new lock. [ 136.763726] the existing dependency chain (in reverse order) is: [ 136.763727] -> #2 (dma_fence_map){++++}-{0:0}: [ 136.763730] __dma_fence_might_wait+0x41/0xb0 [ 136.763732] dma_resv_lockdep+0x171/0x202 [ 136.763734] do_one_initcall+0x5d/0x2f0 [ 136.763736] kernel_init_freeable+0x20d/0x26d [ 136.763738] kernel_init+0xa/0xfb [ 136.763740] ret_from_fork+0x27/0x50 [ 136.763740] -> #1 (fs_reclaim){+.+.}-{0:0}: [ 136.763743] fs_reclaim_acquire.part.0+0x25/0x30 [ 136.763745] kmem_cache_alloc_trace+0x2e/0x6e0 [ 136.763747] device_create_groups_vargs+0x52/0xf0 [ 136.763747] device_create+0x49/0x60 [ 136.763749] fb_console_init+0x25/0x145 [ 136.763750] fbmem_init+0xcc/0xe2 [ 136.763750] do_one_initcall+0x5d/0x2f0 [ 136.763751] kernel_init_freeable+0x20d/0x26d [ 136.763752] kernel_init+0xa/0xfb [ 136.763753] ret_from_fork+0x27/0x50 [ 136.763753] -> #0 (console_lock){+.+.}-{0:0}: [ 136.763755] __lock_acquire+0x1241/0x23f0 [ 136.763756] lock_acquire+0xad/0x370 [ 136.763757] console_lock+0x47/0x70 [ 136.763761] drm_fb_helper_set_suspend_unlocked+0x7b/0xa0 [drm_kms_helper] [ 136.763809] amdgpu_device_gpu_recover.cold+0x21e/0xe7b [amdgpu] [ 136.763850] amdgpu_job_timedout+0xfb/0x150 [amdgpu] [ 136.763851] drm_sched_job_timedout+0x8a/0xf0 [gpu_sched] [ 136.763852] process_one_work+0x23c/0x580 [ 136.763853] worker_thread+0x50/0x3b0 [ 136.763854] kthread+0x12e/0x150 [ 136.763855] ret_from_fork+0x27/0x50 [ 136.763855] other info that might help us debug this: [ 136.763856] Chain exists of: console_lock --> fs_reclaim --> dma_fence_map [ 136.763857] Possible unsafe locking scenario: [ 136.763857] CPU0 CPU1 [ 136.763857] ---- ---- [ 136.763857] lock(dma_fence_map); [ 136.763858] lock(fs_reclaim); [ 136.763858] lock(dma_fence_map); [ 136.763858] lock(console_lock); [ 136.763859] *** DEADLOCK *** [ 136.763860] 4 locks held by kworker/2:3/682: [ 136.763860] #0: ffff8887fb81c938 ((wq_completion)events){+.+.}-{0:0}, at: process_one_work+0x1bc/0x580 [ 136.763862] #1: ffffc90000cafe58 ((work_completion)(&(&sched->work_tdr)->work)){+.+.}-{0:0}, at: process_one_work+0x1bc/0x580 [ 136.763863] #2: ffffffff82318c80 (dma_fence_map){++++}-{0:0}, at: drm_sched_job_timedout+0x25/0xf0 [gpu_sched] [ 136.763865] #3: ffff8887ab621748 (&adev->lock_reset){+.+.}-{3:3}, at: amdgpu_device_gpu_recover.cold+0x5ab/0xe7b [amdgpu] [ 136.763914] stack backtrace: [ 136.763915] CPU: 2 PID: 682 Comm: kworker/2:3 Tainted: G W 5.7.0-rc3+ #346 [ 136.763916] Hardware name: System manufacturer System Product Name/PRIME X370-PRO, BIOS 4011 04/19/2018 [ 136.763918] Workqueue: events drm_sched_job_timedout [gpu_sched] [ 136.763919] Call Trace: [ 136.763922] dump_stack+0x8f/0xd0 [ 136.763924] check_noncircular+0x162/0x180 [ 136.763926] __lock_acquire+0x1241/0x23f0 [ 136.763927] lock_acquire+0xad/0x370 [ 136.763932] ? drm_fb_helper_set_suspend_unlocked+0x7b/0xa0 [drm_kms_helper] [ 136.763933] ? mark_held_locks+0x2d/0x80 [ 136.763934] ? _raw_spin_unlock_irqrestore+0x46/0x60 [ 136.763936] console_lock+0x47/0x70 [ 136.763940] ? drm_fb_helper_set_suspend_unlocked+0x7b/0xa0 [drm_kms_helper] [ 136.763944] drm_fb_helper_set_suspend_unlocked+0x7b/0xa0 [drm_kms_helper] [ 136.763993] amdgpu_device_gpu_recover.cold+0x21e/0xe7b [amdgpu] [ 136.764036] amdgpu_job_timedout+0xfb/0x150 [amdgpu] [ 136.764038] drm_sched_job_timedout+0x8a/0xf0 [gpu_sched] [ 136.764040] process_one_work+0x23c/0x580 [ 136.764041] worker_thread+0x50/0x3b0 [ 136.764042] ? process_one_work+0x580/0x580 [ 136.764044] kthread+0x12e/0x150 [ 136.764045] ? kthread_create_worker_on_cpu+0x70/0x70 [ 136.764046] ret_from_fork+0x27/0x50 Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index 029a026ecfa9..935116614884 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4330,8 +4330,6 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive, if (r) goto out; - amdgpu_fbdev_set_suspend(tmp_adev, 0); - /* * The GPU enters bad state once faulty pages * by ECC has reached the threshold, and ras @@ -4590,8 +4588,6 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, */ amdgpu_unregister_gpu_instance(tmp_adev); - amdgpu_fbdev_set_suspend(tmp_adev, 1); - /* disable ras on ALL IPs */ if (!need_emergency_restart && amdgpu_device_ip_need_full_reset(tmp_adev)) -- 2.28.0

5 years, 1 month

1
0
0 0

[PATCH 21/65] drm/amdgpu: use dma-fence annotations for gpu reset code

by Daniel Vetter

To improve coverage also annotate the gpu reset code itself, since that's called from other places than drm/scheduler (which is already annotated). Annotations nests, so this doesn't break anything, and allows easier testing. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> --- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index e8b41756c9f9..029a026ecfa9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -4496,6 +4496,9 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, int i, r = 0; bool need_emergency_restart = false; bool audio_suspended = false; + bool fence_cookie; + + fence_cookie = dma_fence_begin_signalling(); /** * Special case: RAS triggered and full reset isn't supported @@ -4529,6 +4532,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, DRM_INFO("Bailing on TDR for s_job:%llx, hive: %llx as another already in progress", job ? job->base.id : -1, hive->hive_id); amdgpu_put_xgmi_hive(hive); + dma_fence_end_signalling(fence_cookie); return 0; } mutex_lock(&hive->hive_lock); @@ -4541,8 +4545,10 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, */ INIT_LIST_HEAD(&device_list); if (adev->gmc.xgmi.num_physical_nodes > 1) { - if (!hive) + if (!hive) { + dma_fence_end_signalling(fence_cookie); return -ENODEV; + } if (!list_is_first(&adev->gmc.xgmi.head, &hive->device_list)) list_rotate_to_front(&adev->gmc.xgmi.head, &hive->device_list); device_list_handle = &hive->device_list; @@ -4556,8 +4562,6 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, if (!amdgpu_device_lock_adev(tmp_adev, hive)) { dev_info(tmp_adev->dev, "Bailing on TDR for s_job:%llx, as another already in progress", job ? job->base.id : -1); - r = 0; - goto skip_recovery; } /* @@ -4699,6 +4703,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, if (r) dev_info(adev->dev, "GPU reset end with ret = %d\n", r); + dma_fence_end_signalling(fence_cookie); return r; } -- 2.28.0

5 years, 1 month

1
0
0 0

[PATCH 20/65] drm/scheduler: use dma-fence annotations in tdr work

by Daniel Vetter

In the face of unpriviledged userspace being able to submit bogus gpu workloads the kernel needs gpu timeout and reset (tdr) to guarantee that dma_fences actually complete. Annotate this worker to make sure we don't have any accidental locking inversions or other problems lurking. Originally this was part of the overall scheduler annotation patch. But amdgpu has some glorious inversions here: - grabs console_lock - does a full modeset, which grabs all kinds of locks (drm_modeset_lock, dma_resv_lock) which can deadlock with dma_fence_wait held inside them. - almost minor at that point, but the modeset code also allocates memory These all look like they'll be very hard to fix properly, the hardware seems to require a full display reset with any gpu recovery. Hence split out as a seperate patch. Since amdgpu isn't the only hardware driver that needs to reset the display (at least gen2/3 on intel have the same problem) we need a generic solution for this. There's two tricks we could still from drm/i915 and lift to dma-fence: - The big whack, aka force-complete all fences. i915 does this for all pending jobs if the reset is somehow stuck. Trouble is we'd need to do this for all fences in the entire system, and just the book-keeping for that will be fun. Plus lots of drivers use fences for all kinds of internal stuff like memory management, so unconditionally resetting all of them doesn't work. I'm also hoping that with these fence annotations we could enlist lockdep in finding the last offenders causing deadlocks, and we could remove this get-out-of-jail trick. - The more feasible approach (across drivers at least as part of the dma_fence contract) is what drm/i915 does for gen2/3: When we need to reset the display we wake up all dma_fence_wait_interruptible calls, or well at least the equivalent of those in i915 internally. Relying on ioctl restart we force all other threads to release their locks, which means the tdr thread is guaranteed to be able to get them. I think we could implement this at the dma_fence level, including proper lockdep annotations. dma_fence_begin_tdr(): - must be nested within a dma_fence_begin/end_signalling section - will wake up all interruptible (but not the non-interruptible) dma_fence_wait() calls and force them to complete with a -ERESTARTSYS errno code. All new interrupitble calls to dma_fence_wait() will immeidately fail with the same error code. dma_fence_end_trdr(): - this will convert dma_fence_wait() calls back to normal. Of course interrupting dma_fence_wait is only ok if the caller specified that, which means we need to split the annotations into interruptible and non-interruptible version. If we then make sure that we only use interruptible dma_fence_wait() calls while holding drm_modeset_lock we can grab them in tdr code, and allow display resets. Doing the same for dma_resv_lock might be a lot harder, so buffer updates must be avoided. What's worse, we're not going to be able to make the dma_fence_wait calls in mmu-notifiers interruptible, that doesn't work. So allocating memory still wont' be allowed, even in tdr sections. Plus obviously we can use this trick only in tdr, it is rather intrusive. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/scheduler/sched_main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index f69abc4e70d3..ae0d5ceca49a 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -281,9 +281,12 @@ static void drm_sched_job_timedout(struct work_struct *work) { struct drm_gpu_scheduler *sched; struct drm_sched_job *job; + bool fence_cookie; sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); + fence_cookie = dma_fence_begin_signalling(); + /* Protects against concurrent deletion in drm_sched_get_cleanup_job */ spin_lock(&sched->job_list_lock); job = list_first_entry_or_null(&sched->ring_mirror_list, @@ -315,6 +318,8 @@ static void drm_sched_job_timedout(struct work_struct *work) spin_lock(&sched->job_list_lock); drm_sched_start_timeout(sched); spin_unlock(&sched->job_list_lock); + + dma_fence_end_signalling(fence_cookie); } /** -- 2.28.0

5 years, 1 month

1
0
0 0

[PATCH 19/65] drm/amdgpu: s/GFP_KERNEL/GFP_ATOMIC in scheduler code

by Daniel Vetter

My dma-fence lockdep annotations caught an inversion because we allocate memory where we really shouldn't: kmem_cache_alloc+0x2b/0x6d0 amdgpu_fence_emit+0x30/0x330 [amdgpu] amdgpu_ib_schedule+0x306/0x550 [amdgpu] amdgpu_job_run+0x10f/0x260 [amdgpu] drm_sched_main+0x1b9/0x490 [gpu_sched] kthread+0x12e/0x150 Trouble right now is that lockdep only validates against GFP_FS, which would be good enough for shrinkers. But for mmu_notifiers we actually need !GFP_ATOMIC, since they can be called from any page laundering, even if GFP_NOFS or GFP_NOIO are set. I guess we should improve the lockdep annotations for fs_reclaim_acquire/release. Ofc real fix is to properly preallocate this fence and stuff it into the amdgpu job structure. But GFP_ATOMIC gets the lockdep splat out of the way. v2: Two more allocations in scheduler paths. Frist one: __kmalloc+0x58/0x720 amdgpu_vmid_grab+0x100/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched] Second one: kmem_cache_alloc+0x2b/0x6d0 amdgpu_sync_fence+0x7e/0x110 [amdgpu] amdgpu_vmid_grab+0x86b/0xca0 [amdgpu] amdgpu_job_dependency+0xf9/0x120 [amdgpu] drm_sched_entity_pop_job+0x3f/0x440 [gpu_sched] drm_sched_main+0xf9/0x490 [gpu_sched] Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index fe2d495d08ab..09614b325b5f 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -143,7 +143,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f, uint32_t seq; int r; - fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_KERNEL); + fence = kmem_cache_alloc(amdgpu_fence_slab, GFP_ATOMIC); if (fence == NULL) return -ENOMEM; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c index 7521f4ab55de..2a4cde7cd746 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c @@ -208,7 +208,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_vm *vm, if (ring->vmid_wait && !dma_fence_is_signaled(ring->vmid_wait)) return amdgpu_sync_fence(sync, ring->vmid_wait); - fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_KERNEL); + fences = kmalloc_array(sizeof(void *), id_mgr->num_ids, GFP_ATOMIC); if (!fences) return -ENOMEM; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c index 8ea6c49529e7..af22b526cec9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c @@ -160,7 +160,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f) if (amdgpu_sync_add_later(sync, f)) return 0; - e = kmem_cache_alloc(amdgpu_sync_slab, GFP_KERNEL); + e = kmem_cache_alloc(amdgpu_sync_slab, GFP_ATOMIC); if (!e) return -ENOMEM; -- 2.28.0

5 years, 1 month

1
0
0 0

[PATCH 18/65] drm/amdgpu: use dma-fence annotations in cs_submit()

by Daniel Vetter

This is a bit tricky, since ->notifier_lock is held while calling dma_fence_wait we must ensure that also the read side (i.e. dma_fence_begin_signalling) is on the same side. If we mix this up lockdep complaints, and that's again why we want to have these annotations. A nice side effect of this is that because of the fs_reclaim priming for dma_fence_enable lockdep now automatically checks for us that nothing in here allocates memory, without even running any userptr workloads. Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: linux-rdma(a)vger.kernel.org Cc: amd-gfx(a)lists.freedesktop.org Cc: intel-gfx(a)lists.freedesktop.org Cc: Chris Wilson <chris(a)chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Christian König <christian.koenig(a)amd.com> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index d50b63a93d37..3b3999225e31 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1212,6 +1212,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, struct amdgpu_job *job; uint64_t seq; int r; + bool fence_cookie; job = p->job; p->job = NULL; @@ -1226,6 +1227,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, */ mutex_lock(&p->adev->notifier_lock); + fence_cookie = dma_fence_begin_signalling(); + /* If userptr are invalidated after amdgpu_cs_parser_bos(), return * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl. */ @@ -1262,12 +1265,14 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm); ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence); + dma_fence_end_signalling(fence_cookie); mutex_unlock(&p->adev->notifier_lock); return 0; error_abort: drm_sched_job_cleanup(&job->base); + dma_fence_end_signalling(fence_cookie); mutex_unlock(&p->adev->notifier_lock); error_unlock: -- 2.28.0

5 years, 1 month

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig