Re: The problem "ring gfx timeout" are experienced yet another AMD GPU Vega 8 user
On Wed, 27 Feb 2019 at 00:57, Marek Olšák wrote: > > Sadly, the logs don't contain any clue as to why it hangs. > > It would be helpful to check if the hang can be reproduced on Vega 56 or 64 > as well. > > Marek > Hi, Marek. I'm sorry to trouble you. But today the user of described above Vega 8 graphic sended me fresh logs. Actual versions: kernel 5.1.15 / DRM 3.30.0 / Mesa 19.0. / LLVM 8.0.0 I uploaded all logs to mega cloud storage. Can you look this logs please? https://mega.nz/#F!Mt5mhKiI!8Sv2T5a6yTxBqVknhH1NjA -- Best Regards, Mike Gavrilov. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/2] drm/amdgpu/gfx10: use reset default for PA_SC_FIFO_SIZE
Reviewed-by: Xiaojie Yuan BR, Xiaojie From: amd-gfx on behalf of Alex Deucher Sent: Monday, July 1, 2019 10:00 PM To: amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Subject: [PATCH 2/2] drm/amdgpu/gfx10: use reset default for PA_SC_FIFO_SIZE Recommended by the hw team. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 18 -- 1 file changed, 18 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 2932ade7dbd0..ccd5a4968a60 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -1544,24 +1544,6 @@ static void gfx_v10_0_constants_init(struct amdgpu_device *adev) gfx_v10_0_init_compute_vmid(adev); - mutex_lock(>grbm_idx_mutex); - /* -* making sure that the following register writes will be broadcasted -* to all the shaders -*/ - gfx_v10_0_select_se_sh(adev, 0x, 0x, 0x); - - tmp = REG_SET_FIELD(0, PA_SC_FIFO_SIZE, SC_FRONTEND_PRIM_FIFO_SIZE, - adev->gfx.config.sc_prim_fifo_size_frontend); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_BACKEND_PRIM_FIFO_SIZE, - adev->gfx.config.sc_prim_fifo_size_backend); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_HIZ_TILE_FIFO_SIZE, - adev->gfx.config.sc_hiz_tile_fifo_size); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_EARLYZ_TILE_FIFO_SIZE, - adev->gfx.config.sc_earlyz_tile_fifo_size); - WREG32_SOC15(GC, 0, mmPA_SC_FIFO_SIZE, tmp); - - mutex_unlock(>grbm_idx_mutex); } static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device *adev, -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 1/2] drm/amdgpu/gfx9: use reset default for PA_SC_FIFO_SIZE
Reviewed-by: Xiaojie Yuan BR, Xiaojie From: amd-gfx on behalf of Alex Deucher Sent: Monday, July 1, 2019 10:00 PM To: amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Subject: [PATCH 1/2] drm/amdgpu/gfx9: use reset default for PA_SC_FIFO_SIZE Recommended by the hw team. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 19 --- 1 file changed, 19 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c index 9b413f6fa588..5bdd7c42b00e 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c @@ -1960,25 +1960,6 @@ static void gfx_v9_0_constants_init(struct amdgpu_device *adev) mutex_unlock(>srbm_mutex); gfx_v9_0_init_compute_vmid(adev); - - mutex_lock(>grbm_idx_mutex); - /* -* making sure that the following register writes will be broadcasted -* to all the shaders -*/ - gfx_v9_0_select_se_sh(adev, 0x, 0x, 0x); - - WREG32_SOC15_RLC(GC, 0, mmPA_SC_FIFO_SIZE, - (adev->gfx.config.sc_prim_fifo_size_frontend << - PA_SC_FIFO_SIZE__SC_FRONTEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_prim_fifo_size_backend << - PA_SC_FIFO_SIZE__SC_BACKEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_hiz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_HIZ_TILE_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_earlyz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_EARLYZ_TILE_FIFO_SIZE__SHIFT)); - mutex_unlock(>grbm_idx_mutex); - } static void gfx_v9_0_wait_for_rlc_serdes(struct amdgpu_device *adev) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On Thu, 2019-06-27 at 22:21 +, Li, Sun peng (Leo) wrote:
> Sorry for the late response! just jumping back on this now.
>
> On 2019-05-16 5:40 p.m., Lyude Paul wrote:
> > [CAUTION: External Email]
> >
> > So a couple of things:
> >
> > On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote:
> > > From: Ville Syrjälä
> > >
> > > All available downstream ports - physical and logical - are exposed for
> > > each MST device. They are listed in /dev/, following the same naming
> > > scheme as SST devices by appending an incremental ID.
> > >
> > > Although all downstream ports are exposed, only some will work as
> > > expected. Consider the following topology:
> > >
> > > +-+
> > > | ASIC |
> > > +-+
> > >Conn-0|
> > > |
> > > +v+
> > >+| MST HUB |+
> > >|+-+|
> > >| |
> > >|Port-1 Port-2|
> > > +-v-+ +-v-+
> > > | MST | | SST |
> > > | Display | | Display |
> > > +---+ +---+
> > >|Port-1
> > >x
> > >
> > > MST Path | MST Device
> > > --+--
> > > sst:0 | MST Hub
> > > mst:0-1 | MST Display
> > > mst:0-1-1 | MST Display's disconnected DP out
> > > mst:0-1-8 | MST Display's internal sink
> > > mst:0-2 | SST Display
> > >
> > > On certain MST displays, the upstream physical port will ACK DPCD reads.
> > > However, reads on the local logical port to the internal sink will
> > > *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
> > >
> > > There may also be duplicates. Some displays will return the same GUID
> > > when reading DPCD from both mst:0-1 and mst:0-1-8.
> > >
> > > There are some device-dependent behavior as well. The MST hub used
> > > during testing will actually *ACK* read requests on a disconnected
> > > physical port, whereas the MST displays will NAK.
> > >
> > > In light of these discrepancies, it's simpler to expose all downstream
> > > ports - both physical and logical - and let the user decide what to use.
> > >
> > > Cc: Lyude Paul
> > > Signed-off-by: Ville Syrjälä
> > > Signed-off-by: Leo Li
> > > ---
> > > drivers/gpu/drm/drm_dp_aux_dev.c | 14 -
> > > drivers/gpu/drm/drm_dp_mst_topology.c | 103
> > > +
> > > -
> > > include/drm/drm_dp_helper.h | 4 ++
> > > include/drm/drm_dp_mst_helper.h | 6 ++
> > > 4 files changed, 112 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > index 6d84611..01c02b9 100644
> > > --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> > > @@ -34,6 +34,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > >
> > > @@ -114,6 +115,7 @@ static ssize_t name_show(struct device *dev,
> > >
> > >return res;
> > > }
> > > +
> > > static DEVICE_ATTR_RO(name);
> > >
> > > static struct attribute *drm_dp_aux_attrs[] = {
> > > @@ -160,7 +162,11 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
> > > struct iov_iter *to)
> > >break;
> > >}
> > >
> > > - res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
> > > + if (aux_dev->aux->is_remote)
> > > + res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
> > > todo);
> > > + else
> > > + res = drm_dp_dpcd_read(aux_dev->aux, pos, buf,
> > > todo);
> > > +
> >
> > It's still not at all clear to me why we're trying to avoid specifying
> > actual
> > callbacks for the aux device. We should remove this part entirely, remove
> > the
> > is_remote entry from struct drm_dp_aux, and then just specify our own hook
> > in
> > struct drm_dp_aux->transfer().
> >
>
> I'm not sure if this would work well. The existing policy does retries
> around the ->transfer() call. Using the same hook will cause a nested
> retry - once when calling remote_aux->transfer, and another when calling
> real_aux->transfer. The difference is the scope of the retry. The former
> replays the entire aux transaction, while the latter replays just the
> failed sideband packet. I think having the retry at the packet level
> makes more sense. Either way, it shouldn't happen in a nested manner.
>
> In general, we need a way to determine whether a message needs to be
> sent via sideband. I'm not sure if there's a better way than setting a
> 'is_remote' flag?
oh-this is a very good point actually! I suppose we would certainly want to be
able to retry on a packet level instead. Since that's the case, I think going
with the current is_remote solution should be fine then.
XDC 2019: Final week to submit your talks!
Hi! It's the final week to submit your talks, workshops or demos for #XDC2019!! CfP ends this coming Sunday, July 7! Have some new developments to share? Facing some challenges with you projects? If it's related to open source graphics, please send it in! http://xdc2019.x.org Best, Mark ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH 1/2] drm/amdgpu/gfx9: use reset default for PA_SC_FIFO_SIZE
Series are Reviewed-and-Tested-by: Huang Rui -Original Message- From: amd-gfx On Behalf Of Alex Deucher Sent: Monday, July 1, 2019 9:00 AM To: amd-gfx@lists.freedesktop.org Cc: Deucher, Alexander Subject: [PATCH 1/2] drm/amdgpu/gfx9: use reset default for PA_SC_FIFO_SIZE Recommended by the hw team. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 19 --- 1 file changed, 19 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c index 9b413f6fa588..5bdd7c42b00e 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c @@ -1960,25 +1960,6 @@ static void gfx_v9_0_constants_init(struct amdgpu_device *adev) mutex_unlock(>srbm_mutex); gfx_v9_0_init_compute_vmid(adev); - - mutex_lock(>grbm_idx_mutex); - /* -* making sure that the following register writes will be broadcasted -* to all the shaders -*/ - gfx_v9_0_select_se_sh(adev, 0x, 0x, 0x); - - WREG32_SOC15_RLC(GC, 0, mmPA_SC_FIFO_SIZE, - (adev->gfx.config.sc_prim_fifo_size_frontend << - PA_SC_FIFO_SIZE__SC_FRONTEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_prim_fifo_size_backend << - PA_SC_FIFO_SIZE__SC_BACKEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_hiz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_HIZ_TILE_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_earlyz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_EARLYZ_TILE_FIFO_SIZE__SHIFT)); - mutex_unlock(>grbm_idx_mutex); - } static void gfx_v9_0_wait_for_rlc_serdes(struct amdgpu_device *adev) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct drm_connector
Hi
Am 01.07.19 um 15:27 schrieb Andrzej Pietrasiewicz:
> Hi Thomas,
>
> Thank you for your comments. Please see inline.
>
> W dniu 30.06.2019 o 10:12, Thomas Zimmermann pisze:
>> Hi
>>
>> I like the idea, but would prefer a more structured approach.
>>
>> Setting connector->ddc before calling drm_sysfs_connector_add() seems
>> error prone. The dependency is not really clear from the code or
>> interfaces.
>>
>> The other thing is that drivers I mostly work on, ast and mgag200, have
>> code like this:
>>
>> struct ast_i2c_chan {
>> struct i2c_adapter adapter;
>> struct drm_device *dev;
>> struct i2c_algo_bit_data bit;
>> };
>>
>> struct ast_connector {
>> struct drm_connector base;
>> struct ast_i2c_chan *i2c;
>> };
>>
>> It already encodes the connection between connector and ddc adapter.
>>
>> I suggest to introduce struct drm_i2c_adapter
>>
>> struct drm_i2c_adapter {
>> struct i2c_adapter base;
>> struct drm_connector *connector;
>> };
>>
>> and convert drivers over to it. Ast would look like this:
>>
>> struct ast_i2c_chan {
>> struct drm_i2c_adapter adapter;
>> struct i2c_algo_bit_data bit;
>> };
>>
>
> There are few flavors of these drivers. In some of them
> the i2c_adapter for ddc is allocated and initialized by
> the driver, which must provide a place in memory to hold
> the adapter. ast is an example of this approach.
>
> But there are others, such as for example exynos_hdmi.c.
> It gets its ddc adapter with of_find_i2c_adapter_by_node()
> and merely stores a pointer to it, while not managing the
> memory needed to hold the i2c_adapter.
I see.
> Do you have any idea how to accommodate these various
> flavors of drivers in the scheme you propose?
Something like
struct drm_i2c_adapter {
struct i2c_adapter *adapter;
struct drm_connector *connector;
};
with adapter either being allocated dynamically or acquired via
of_find_i2c_adapter_by_node(); with separate init and finish functions
for each variant. But it looks like over-abstraction to me. Without
prototyping the idea, I cannot say if it's worth the effort.
For something more simple, maybe just have a function to attach an i2c
adapter to a connector:
drm_connector_attach_i2c_adapter(connector, i2c_adapter)
which sets the connector's ddc pointer and creates the sysfs entry if
the connector's entry is already present.
Best regards
Thomas
> Andrzej
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v3 01/22] drm: Include ddc adapter pointer in struct drm_connector
Hi Thomas,
Thank you for your comments. Please see inline.
W dniu 30.06.2019 o 10:12, Thomas Zimmermann pisze:
Hi
I like the idea, but would prefer a more structured approach.
Setting connector->ddc before calling drm_sysfs_connector_add() seems
error prone. The dependency is not really clear from the code or interfaces.
The other thing is that drivers I mostly work on, ast and mgag200, have
code like this:
struct ast_i2c_chan {
struct i2c_adapter adapter;
struct drm_device *dev;
struct i2c_algo_bit_data bit;
};
struct ast_connector {
struct drm_connector base;
struct ast_i2c_chan *i2c;
};
It already encodes the connection between connector and ddc adapter.
I suggest to introduce struct drm_i2c_adapter
struct drm_i2c_adapter {
struct i2c_adapter base;
struct drm_connector *connector;
};
and convert drivers over to it. Ast would look like this:
struct ast_i2c_chan {
struct drm_i2c_adapter adapter;
struct i2c_algo_bit_data bit;
};
There are few flavors of these drivers. In some of them
the i2c_adapter for ddc is allocated and initialized by
the driver, which must provide a place in memory to hold
the adapter. ast is an example of this approach.
But there are others, such as for example exynos_hdmi.c.
It gets its ddc adapter with of_find_i2c_adapter_by_node()
and merely stores a pointer to it, while not managing the
memory needed to hold the i2c_adapter.
Do you have any idea how to accommodate these various
flavors of drivers in the scheme you propose?
Andrzej
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 1/2] drm/amdgpu/gfx9: use reset default for PA_SC_FIFO_SIZE
Recommended by the hw team. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 19 --- 1 file changed, 19 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c index 9b413f6fa588..5bdd7c42b00e 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c @@ -1960,25 +1960,6 @@ static void gfx_v9_0_constants_init(struct amdgpu_device *adev) mutex_unlock(>srbm_mutex); gfx_v9_0_init_compute_vmid(adev); - - mutex_lock(>grbm_idx_mutex); - /* -* making sure that the following register writes will be broadcasted -* to all the shaders -*/ - gfx_v9_0_select_se_sh(adev, 0x, 0x, 0x); - - WREG32_SOC15_RLC(GC, 0, mmPA_SC_FIFO_SIZE, - (adev->gfx.config.sc_prim_fifo_size_frontend << - PA_SC_FIFO_SIZE__SC_FRONTEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_prim_fifo_size_backend << - PA_SC_FIFO_SIZE__SC_BACKEND_PRIM_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_hiz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_HIZ_TILE_FIFO_SIZE__SHIFT) | - (adev->gfx.config.sc_earlyz_tile_fifo_size << - PA_SC_FIFO_SIZE__SC_EARLYZ_TILE_FIFO_SIZE__SHIFT)); - mutex_unlock(>grbm_idx_mutex); - } static void gfx_v9_0_wait_for_rlc_serdes(struct amdgpu_device *adev) -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/2] drm/amdgpu/gfx10: use reset default for PA_SC_FIFO_SIZE
Recommended by the hw team. Signed-off-by: Alex Deucher --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 18 -- 1 file changed, 18 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 2932ade7dbd0..ccd5a4968a60 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -1544,24 +1544,6 @@ static void gfx_v10_0_constants_init(struct amdgpu_device *adev) gfx_v10_0_init_compute_vmid(adev); - mutex_lock(>grbm_idx_mutex); - /* -* making sure that the following register writes will be broadcasted -* to all the shaders -*/ - gfx_v10_0_select_se_sh(adev, 0x, 0x, 0x); - - tmp = REG_SET_FIELD(0, PA_SC_FIFO_SIZE, SC_FRONTEND_PRIM_FIFO_SIZE, - adev->gfx.config.sc_prim_fifo_size_frontend); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_BACKEND_PRIM_FIFO_SIZE, - adev->gfx.config.sc_prim_fifo_size_backend); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_HIZ_TILE_FIFO_SIZE, - adev->gfx.config.sc_hiz_tile_fifo_size); - tmp = REG_SET_FIELD(tmp, PA_SC_FIFO_SIZE, SC_EARLYZ_TILE_FIFO_SIZE, - adev->gfx.config.sc_earlyz_tile_fifo_size); - WREG32_SOC15(GC, 0, mmPA_SC_FIFO_SIZE, tmp); - - mutex_unlock(>grbm_idx_mutex); } static void gfx_v10_0_enable_gui_idle_interrupt(struct amdgpu_device *adev, -- 2.20.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 07/27] gpu: drm: remove memset after zalloc
Hi Fuqian, On Fri, 28 Jun 2019 at 09:30, Fuqian Huang wrote: > > zalloc has already zeroed the memory. > so memset is unneeded. > > Signed-off-by: Fuqian Huang > --- > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 2 -- > drivers/gpu/drm/amd/powerplay/hwmgr/process_pptables_v1_0.c | 2 -- > drivers/gpu/drm/amd/powerplay/smumgr/ci_smumgr.c| 2 -- > drivers/gpu/drm/amd/powerplay/smumgr/iceland_smumgr.c | 2 -- > drivers/gpu/drm/amd/powerplay/smumgr/tonga_smumgr.c | 2 -- > 5 files changed, 10 deletions(-) > Thanks for fixing these. One nit pick: the commit message should start with "drm/amdgpu:" as you can see from git log. With that: Reviewed-by: Emil Velikov -Emil ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v4 hmm 00/12]
On Mon, Jun 24, 2019 at 06:00:58PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe > > This patch series arised out of discussions with Jerome when looking at the > ODP changes, particularly informed by use after free races we have already > found and fixed in the ODP code (thanks to syzkaller) working with mmu > notifiers, and the discussion with Ralph on how to resolve the lifetime model. > > Overall this brings in a simplified locking scheme and easy to explain > lifetime model: > > If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the mm > is allocated memory. > > If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, etc) > then the mmget must be obtained via mmget_not_zero(). > > The use of unlocked reads on 'hmm->dead' are also eliminated in favour of > using standard mmget() locking to prevent the mm from being released. Many of > the debugging checks of !range->hmm and !hmm->mm are dropped in favour of > poison - which is much clearer as to the lifetime intent. > > The trailing patches are just some random cleanups I noticed when reviewing > this code. > > I'll apply this in the next few days - the only patch that doesn't have enough > Reviewed-bys is 'mm/hmm: Remove confusing comment and logic from hmm_release', > which had alot of questions, I still think it is good. If people really don't > like it I'll drop it. > > Thanks to everyone who took time to look at this! > > Jason Gunthorpe (12): > mm/hmm: fix use after free with struct hmm in the mmu notifiers > mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register > mm/hmm: Hold a mmgrab from hmm to mm > mm/hmm: Simplify hmm_get_or_create and make it reliable > mm/hmm: Remove duplicate condition test before wait_event_timeout > mm/hmm: Do not use list*_rcu() for hmm->ranges > mm/hmm: Hold on to the mmget for the lifetime of the range > mm/hmm: Use lockdep instead of comments > mm/hmm: Remove racy protection against double-unregistration > mm/hmm: Poison hmm_range during unregister > mm/hmm: Remove confusing comment and logic from hmm_release > mm/hmm: Fix error flows in hmm_invalidate_range_start I think we are done now, so applied to hmm.git, thank you to everyone. I expect some conflicts in linux-next with the AMD DRM driver, we need to decide how to handle them. Jason ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
