Re: [Intel-gfx] [PATCH 4/4] i915: fix remap_io_sg to verify the pgprot
On 5/18/21 3:23 PM, Christoph Hellwig wrote: On Mon, May 17, 2021 at 11:46:35PM +0200, Thomas Hellström wrote: Apart from the caching aliasing Mattew brought up, doesn't the remap_pfn_range_xxx() family require the mmap_sem held in write mode since it modifies the vma structure? remap_io_sg() is called from the fault handler with the mmap_sem held in read mode only. Only for vma->vm_flags, and remap_sg already asserts all the interesting flags are set, although it does not assert VM_IO. We could move the assignment out of remap_pfn_range_notrack and into remap_pfn_range and just assert that the proper flags are set, though. That to me sounds like a way forward. It sound like in general a gpu prefaulting helper that in the long run also supports faulting huge ptes is desired also by TTM. Although it looks like that BUG_ON() I pointed out was hit anyway /Thomas
Re: [Intel-gfx] [PATCH 4/4] i915: fix remap_io_sg to verify the pgprot
On 5/18/21 5:00 PM, Matthew Auld wrote: On Tue, 18 May 2021 at 14:21, Christoph Hellwig wrote: On Mon, May 17, 2021 at 06:06:44PM +0100, Matthew Auld wrote: Looks like it is caused by the validation failure then. Which means the existing code is doing something wrong in its choice of the page protection bit. I really need help from the i915 maintainers here.. AFAIK there are two users of remap_io_sg, the first is our shmem objects(see i915_gem_shmem.c), and for these we support UC, WC, and WB mmap modes for userspace. The other user is device local-memory objects(VRAM), and for this one we have an actual io_mapping which is allocated as WC, and IIRC this should only be mapped as WC for the mmap mode, but normal userspace can't hit this path yet. The only caller in current mainline is vm_fault_cpu in i915_gem_mman.c. Is that device local? The vm_fault_cpu covers both device local and shmem objects. What do we need to do here? It sounds like shmem backed objects are allocated as WB for the pages underneath, but i915 allows mapping them as UC/WC which trips up this track_pfn thing? To me the warnings looks like system memory is mapped with the wrong permissions, yes. If you want to map it as UC/WC the right set_memory_* needs to be used on the kernel mapping as well to ensure that the attributes don't conflict. AFAIK mmap_offset also supports multiple active mmap modes for a given object, so set_memory_* should still work here? No, that won't work because there are active maps with conflicting caching attributes. I think the history here is that that was assumed to be OK for integrated graphics that ran only on Intel processors that promise to never write back unmodified cache lines resulting from prefetching, like some AMD processors did way back at least. These conflicting mappings can obviously not be supported for discrete graphics, but for integrated they are part of the uAPI. /Thomas ___ Intel-gfx mailing list intel-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 4/4] drm/mediatek: add dsi module reset driver
On Sat, 2021-04-24 at 07:50 +0800, Chun-Kuang Hu wrote:
> Hi, Jitao:
>
> Jitao Shi 於 2021年4月20日 週二 下午9:26寫道：
> >
> > Reset dsi HW to default when power on. Prevent the setting differet
> > between bootloader and kernel.
> >
> > Signed-off-by: Jitao Shi
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 36 +-
> > 1 file changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 455fe582c6b5..113438ddd4cc 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -7,10 +7,12 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -143,6 +145,8 @@
> > #define DATA_0 (0xff << 16)
> > #define DATA_1 (0xff << 24)
> >
> > +#define MMSYS_SW_RST_DSI_B BIT(25)
> > +
> > #define NS_TO_CYCLE(n, c)((n) / (c) + (((n) % (c)) ? 1 : 0))
> >
> > #define MTK_DSI_HOST_IS_READ(type) \
> > @@ -186,7 +190,8 @@ struct mtk_dsi {
> > struct drm_bridge *next_bridge;
> > struct drm_connector *connector;
> > struct phy *phy;
> > -
> > + struct regmap *mmsys_sw_rst_b;
> > + u32 sw_rst_b;
> > void __iomem *regs;
> >
> > struct clk *engine_clk;
> > @@ -272,6 +277,16 @@ static void mtk_dsi_disable(struct mtk_dsi *dsi)
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, 0);
> > }
> >
> > +static void mtk_dsi_reset_all(struct mtk_dsi *dsi)
> > +{
> > + regmap_update_bits(dsi->mmsys_sw_rst_b, dsi->sw_rst_b,
> > + MMSYS_SW_RST_DSI_B, 0);
> > + usleep_range(1000, 1100);
> > +
> > + regmap_update_bits(dsi->mmsys_sw_rst_b, dsi->sw_rst_b,
> > + MMSYS_SW_RST_DSI_B, MMSYS_SW_RST_DSI_B);
> > +}
> > +
> > static void mtk_dsi_reset_engine(struct mtk_dsi *dsi)
> > {
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_RESET, DSI_RESET);
> > @@ -985,6 +1000,8 @@ static int mtk_dsi_bind(struct device *dev, struct
> > device *master, void *data)
> >
> > ret = mtk_dsi_encoder_init(drm, dsi);
> >
> > + mtk_dsi_reset_all(dsi);
> > +
> > return ret;
> > }
> >
> > @@ -1007,6 +1024,7 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> > struct device *dev = >dev;
> > struct drm_panel *panel;
> > struct resource *regs;
> > + struct regmap *regmap;
> > int irq_num;
> > int ret;
> >
> > @@ -1022,6 +1040,22 @@ static int mtk_dsi_probe(struct platform_device
> > *pdev)
> > return ret;
> > }
> >
> > + regmap = syscon_regmap_lookup_by_phandle(dev->of_node,
> > +"mediatek,syscon-dsi");
> > + ret = of_property_read_u32_index(dev->of_node,
> > "mediatek,syscon-dsi", 1,
> > +>sw_rst_b);
> > +
> > + if (IS_ERR(regmap))
> > + ret = PTR_ERR(regmap);
> > +
> > + if (ret) {
> > + ret = PTR_ERR(regmap);
> > + dev_err(dev, "Failed to get mmsys registers: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + dsi->mmsys_sw_rst_b = regmap;
> > +
>
> It looks like that mtk-mmsys is the reset controller and mtk-dsi is
> reset consumer. Please refer to [1], [2] to implement.
>
> [1] https://www.kernel.org/doc/html/latest/driver-api/reset.html
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12-rc8
>
> Regards,
> Chun-Kuang.
>
Thanks, I'll fix next version.
> > ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > , >next_bridge);
> > if (ret)
> > --
> > 2.25.1
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
回复: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
[AMD Official Use Only]
I have reverted Chris' patch, still hit this failure.
Just see two lines in Chris' patch. Any BO in cpu domian would be swapout
first. That is why we hit this issue frequently now. But the bug is there long
time ago.
- for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
- list_for_each_entry(bo, >swap_lru[i], swap) {
[snip]
+ for (i = TTM_PL_SYSTEM; i < TTM_NUM_MEM_TYPES; ++i) {
+ for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
发件人: Pan, Xinhui
发送时间: 2021年5月19日 12:09
收件人: Kuehling, Felix; amd-...@lists.freedesktop.org
抄送: Deucher, Alexander; Koenig, Christian; dri-devel@lists.freedesktop.org;
dan...@ffwll.ch
主题: 回复: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and
swapin
yes, we really dont swapout SG BOs.
The problems is that before we validate a userptr BO, we create this BO in CPU
domain by default. So this BO has chance to swapout.
we set flag TTM_PAGE_FLAG_SG on userptr BO in popluate() which is too late.
I have not try to revert Chris' patch as I think it desnt help. Or I can have a
try later.
发件人: Kuehling, Felix
发送时间: 2021年5月19日 11:29
收件人: Pan, Xinhui; amd-...@lists.freedesktop.org
抄送: Deucher, Alexander; Koenig, Christian; dri-devel@lists.freedesktop.org;
dan...@ffwll.ch
主题: Re: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and
swapin
Swapping SG BOs makes no sense, because TTM doesn't own the pages of
this type of BO.
Last I checked, userptr BOs (and other SG BOs) were protected from
swapout by the fact that they would not be added to the swap-LRU. But it
looks like Christian just removed the swap-LRU. I guess this broke that
protection:
commit 2cb51d22d70b18eaf339abf9758bf0b7608da65c
Author: Christian König
Date: Tue Oct 6 16:30:09 2020 +0200
drm/ttm: remove swap LRU v3
Instead evict round robin from each devices SYSTEM and TT domain.
v2: reorder num_pages access reported by Dan's script
v3: fix rebase fallout, num_pages should be 32bit
Signed-off-by: Christian König
Tested-by: Nirmoy Das
Reviewed-by: Huang Rui
Reviewed-by: Matthew Auld
Link: https://patchwork.freedesktop.org/patch/424009/
Regards,
Felix
On 2021-05-18 10:28 p.m., xinhui pan wrote:
> cpu 1 cpu 2
> kfd alloc BO A(userptr) alloc BO B(GTT)
> ->init -> validate -> init -> validate ->
> populate
> init_user_pages-> swapout BO A //hit ttm
> pages limit
> -> get_user_pages (fill up ttm->pages)
>-> validate -> populate
>-> swapin BO A // Now hit the BUG
>
> We know that get_user_pages may race with swapout on same BO.
> Threre are some issues I have met.
> 1) memory corruption.
> This is because we do a swap before memory is setup. ttm_tt_swapout()
> just create a swap_storage with its content being 0x0. So when we setup
> memory after the swapout. The following swapin makes the memory
> corrupted.
>
> 2) panic
> When swapout happes with get_user_pages, they touch ttm->pages without
> anylock. It causes memory corruption too. But I hit page fault mostly.
>
> Signed-off-by: xinhui pan
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 928e8d57cd08..42460e4480f8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -835,6 +835,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> struct amdkfd_process_info *process_info = mem->process_info;
> struct amdgpu_bo *bo = mem->bo;
> struct ttm_operation_ctx ctx = { true, false };
> + struct page **pages;
> int ret = 0;
>
> mutex_lock(_info->lock);
> @@ -852,7 +853,13 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> goto out;
> }
>
> - ret = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> + pages = kvmalloc_array(bo->tbo.ttm->num_pages,
> + sizeof(struct page *),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!pages)
> + goto unregister_out;
> +
> + ret = amdgpu_ttm_tt_get_user_pages(bo, pages);
> if (ret) {
> pr_err("%s: Failed to get user pages: %d\n", __func__, ret);
> goto unregister_out;
> @@ -863,6 +870,12 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> pr_err("%s: Failed to reserve BO\n", __func__);
> goto release_out;
> }
> +
> + WARN_ON_ONCE(bo->tbo.ttm->page_flags & TTM_PAGE_FLAG_SWAPPED);
> +
> + memcpy(bo->tbo.ttm->pages,
> +
Re: [PATCH 3/4] drm/mediatek: fine tune the dsi panel's power sequence
On Sat, 2021-04-24 at 00:36 +0800, Chun-Kuang Hu wrote:
> Hi, Jitao:
>
> Jitao Shi 於 2021年4月20日 週二 下午9:26寫道：
> >
> > Add the drm_panel_prepare_power and drm_panel_unprepare_power control.
> > Turn on panel power(drm_panel_prepare_power) and control before dsi
> > enable. And then dsi enable, send dcs cmd in drm_panel_prepare, last
> > turn on backlight.
>
> Please describe WHY do you need this patch? Fix any bug?
Most panels, have five steps when poweron.
1. turn on dsi signal to LP11 --> dsi host's action
2. turn on the power supplies, --> panel's action
3. send the DCS cmd to panel --> panel's action
4. start send video stream --> dsi host's action
5. turn on backlight. --> panel's action
we put "turn on the power supplies" and "send the DCS cmd to panel" in
panel_prepare. And "turn on backlight" in panel_enable.
But some other panels has a special poweron sequence as the following.
1. turn on the power supplies, --> panel's action
2. turn on dsi signal to LP11 --> dsi host's action
3. send the DCS cmd to panel --> panel's action
4. start send video stream --> dsi host's action
5. turn on backlight. --> panel's action
panel's actions are divided into three parts.
So I add a new api "drm_panel_prepare_power/rm_panel_unprepare_power" to
control the sequence.
Best Regards
Jitao
>
> >
> > Signed-off-by: Jitao Shi
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index a1ff152ef468..455fe582c6b5 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -615,10 +615,13 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > dsi->data_rate = DIV_ROUND_UP_ULL(dsi->vm.pixelclock *
> > bit_per_pixel,
> > dsi->lanes);
> >
> > + if (panel_bridge_prepare_power(dsi->next_bridge))
>
> ret = panel_bridge_prepare_power(dsi->next_bridge);
> if (ret)
>
> > + DRM_INFO("can't prepare power the panel\n");
>
> I think you should goto err_refcount;
Thanks for your review. I'll fix it next patch.
>
> > +
> > ret = clk_set_rate(dsi->hs_clk, dsi->data_rate);
> > if (ret < 0) {
> > dev_err(dev, "Failed to set data rate: %d\n", ret);
> > - goto err_refcount;
> > + goto err_prepare_power;
> > }
> >
> > phy_power_on(dsi->phy);
> > @@ -661,7 +664,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > clk_disable_unprepare(dsi->engine_clk);
> > err_phy_power_off:
> > phy_power_off(dsi->phy);
> > -err_refcount:
> > +err_prepare_power:
> > + if (panel_bridge_unprepare_power(dsi->next_bridge))
>
> ret = panel_bridge_unprepare_power(dsi->next_bridge);
>
> > + DRM_INFO("Can't unprepare power the panel\n");
> > dsi->refcount--;
> > return ret;
> > }
> > @@ -694,6 +699,9 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
> > clk_disable_unprepare(dsi->digital_clk);
> >
> > phy_power_off(dsi->phy);
> > +
> > + if (panel_bridge_unprepare_power(dsi->next_bridge))
>
> ret = panel_bridge_unprepare_power(dsi->next_bridge);
>
> > + DRM_INFO("Can't unprepare power the panel\n");
> > }
> >
> > static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > --
> > 2.25.1
> > ___
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
回复: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
[AMD Official Use Only]
yes, we really dont swapout SG BOs.
The problems is that before we validate a userptr BO, we create this BO in CPU
domain by default. So this BO has chance to swapout.
we set flag TTM_PAGE_FLAG_SG on userptr BO in popluate() which is too late.
I have not try to revert Chris' patch as I think it desnt help. Or I can have a
try later.
发件人: Kuehling, Felix
发送时间: 2021年5月19日 11:29
收件人: Pan, Xinhui; amd-...@lists.freedesktop.org
抄送: Deucher, Alexander; Koenig, Christian; dri-devel@lists.freedesktop.org;
dan...@ffwll.ch
主题: Re: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and
swapin
Swapping SG BOs makes no sense, because TTM doesn't own the pages of
this type of BO.
Last I checked, userptr BOs (and other SG BOs) were protected from
swapout by the fact that they would not be added to the swap-LRU. But it
looks like Christian just removed the swap-LRU. I guess this broke that
protection:
commit 2cb51d22d70b18eaf339abf9758bf0b7608da65c
Author: Christian König
Date: Tue Oct 6 16:30:09 2020 +0200
drm/ttm: remove swap LRU v3
Instead evict round robin from each devices SYSTEM and TT domain.
v2: reorder num_pages access reported by Dan's script
v3: fix rebase fallout, num_pages should be 32bit
Signed-off-by: Christian König
Tested-by: Nirmoy Das
Reviewed-by: Huang Rui
Reviewed-by: Matthew Auld
Link: https://patchwork.freedesktop.org/patch/424009/
Regards,
Felix
On 2021-05-18 10:28 p.m., xinhui pan wrote:
> cpu 1 cpu 2
> kfd alloc BO A(userptr) alloc BO B(GTT)
> ->init -> validate -> init -> validate ->
> populate
> init_user_pages-> swapout BO A //hit ttm
> pages limit
> -> get_user_pages (fill up ttm->pages)
>-> validate -> populate
>-> swapin BO A // Now hit the BUG
>
> We know that get_user_pages may race with swapout on same BO.
> Threre are some issues I have met.
> 1) memory corruption.
> This is because we do a swap before memory is setup. ttm_tt_swapout()
> just create a swap_storage with its content being 0x0. So when we setup
> memory after the swapout. The following swapin makes the memory
> corrupted.
>
> 2) panic
> When swapout happes with get_user_pages, they touch ttm->pages without
> anylock. It causes memory corruption too. But I hit page fault mostly.
>
> Signed-off-by: xinhui pan
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 928e8d57cd08..42460e4480f8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -835,6 +835,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> struct amdkfd_process_info *process_info = mem->process_info;
> struct amdgpu_bo *bo = mem->bo;
> struct ttm_operation_ctx ctx = { true, false };
> + struct page **pages;
> int ret = 0;
>
> mutex_lock(_info->lock);
> @@ -852,7 +853,13 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> goto out;
> }
>
> - ret = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
> + pages = kvmalloc_array(bo->tbo.ttm->num_pages,
> + sizeof(struct page *),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!pages)
> + goto unregister_out;
> +
> + ret = amdgpu_ttm_tt_get_user_pages(bo, pages);
> if (ret) {
> pr_err("%s: Failed to get user pages: %d\n", __func__, ret);
> goto unregister_out;
> @@ -863,6 +870,12 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> pr_err("%s: Failed to reserve BO\n", __func__);
> goto release_out;
> }
> +
> + WARN_ON_ONCE(bo->tbo.ttm->page_flags & TTM_PAGE_FLAG_SWAPPED);
> +
> + memcpy(bo->tbo.ttm->pages,
> + pages,
> + sizeof(struct page*) * bo->tbo.ttm->num_pages);
> amdgpu_bo_placement_from_domain(bo, mem->domain);
> ret = ttm_bo_validate(>tbo, >placement, );
> if (ret)
> @@ -872,6 +885,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
> user_addr)
> release_out:
> amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
> unregister_out:
> + kvfree(pages);
> if (ret)
> amdgpu_mn_unregister(bo);
> out:
Re: [PATCH v4 1/3] drm/dp_mst: Add self-tests for up requests
On Wed, 19 May 2021 at 08:01, Lyude Paul wrote:
>
> Looks like these tests might still need to be fixed up a bit:
>
> [ 34.785042] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]]
> connection status reply parse length fail 2 1
> [ 34.785082] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]]
> resource status reply parse length fail 2 1
> [ 34.785114] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]] sink
> event notify parse length fail 2 1
> [ 34.785146] (null): [drm] *ERROR* Got unknown request 0x23
> (REMOTE_I2C_WRITE)
>
Those are expected parse failures - testing that parse rejects
messages that are too short or are unsupported. I'll set the mock
device name to make this clearer in the next version, producing
logging like:
[ 25.163682] [drm_dp_mst_helper] expected parse failure:
[drm:drm_dp_sideband_parse_req] connection status reply parse length
fail 2 1
[ 25.163706] [drm_dp_mst_helper] expected parse failure:
[drm:drm_dp_sideband_parse_req] resource status reply parse length
fail 2 1
[ 25.163719] [drm_dp_mst_helper] expected parse failure:
[drm:drm_dp_sideband_parse_req] sink event notify parse length fail 2
1
[ 25.163730] [drm_dp_mst_helper] expected parse failure: [drm]
*ERROR* Got unknown request 0x23 (REMOTE_I2C_WRITE)
>
> On Tue, 2021-05-18 at 22:35 +1000, Sam McNally wrote:
> Up requests are decoded by drm_dp_sideband_parse_req(), which operates
> on a drm_dp_sideband_msg_rx, unlike down requests. Expand the existing
> self-test helper sideband_msg_req_encode_decode() to copy the message
> contents and length from a drm_dp_sideband_msg_tx to
> drm_dp_sideband_msg_rx and use the parse function under test in place of
> decode.
>
> Add support for currently-supported up requests to
> drm_dp_dump_sideband_msg_req_body(); add support to
> drm_dp_encode_sideband_req() to allow encoding for the self-tests.
>
> Add self-tests for CONNECTION_STATUS_NOTIFY and RESOURCE_STATUS_NOTIFY.
>
> Signed-off-by: Sam McNally
> ---
>
> Changes in v4:
> - New in v4
>
> drivers/gpu/drm/drm_dp_mst_topology.c | 54 ++-
> .../gpu/drm/drm_dp_mst_topology_internal.h| 4 +
> .../drm/selftests/test-drm_dp_mst_helper.c| 147 --
> 3 files changed, 190 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index 54604633e65c..573f39a3dc16 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -442,6 +442,37 @@ drm_dp_encode_sideband_req(const struct
> drm_dp_sideband_msg_req_body *req,
> idx++;
> }
> break;
> + case DP_CONNECTION_STATUS_NOTIFY: {
> + const struct drm_dp_connection_status_notify *msg;
> +
> + msg = >u.conn_stat;
> + buf[idx] = (msg->port_number & 0xf) << 4;
> + idx++;
> + memcpy(>msg[idx], msg->guid, 16);
> + idx += 16;
> + raw->msg[idx] = 0;
> + raw->msg[idx] |= msg->legacy_device_plug_status ? BIT(6) : 0;
> + raw->msg[idx] |= msg->displayport_device_plug_status ? BIT(5)
> :
> 0;
> + raw->msg[idx] |= msg->message_capability_status ? BIT(4) : 0;
> + raw->msg[idx] |= msg->input_port ? BIT(3) : 0;
> + raw->msg[idx] |= FIELD_PREP(GENMASK(2, 0), msg-
> >peer_device_type);
> + idx++;
> + break;
> + }
> + case DP_RESOURCE_STATUS_NOTIFY: {
> + const struct drm_dp_resource_status_notify *msg;
> +
> + msg = >u.resource_stat;
> + buf[idx] = (msg->port_number & 0xf) << 4;
> + idx++;
> + memcpy(>msg[idx], msg->guid, 16);
> + idx += 16;
> + buf[idx] = (msg->available_pbn & 0xff00) >> 8;
> + idx++;
> + buf[idx] = (msg->available_pbn & 0xff);
> + idx++;
> + break;
> + }
> }
> raw->cur_len = idx;
> }
> @@ -672,6 +703,22 @@ drm_dp_dump_sideband_msg_req_body(const struct
> drm_dp_sideband_msg_req_body *req
> req->u.enc_status.stream_behavior,
> req->u.enc_status.valid_stream_behavior);
> break;
> + case DP_CONNECTION_STATUS_NOTIFY:
> + P("port=%d guid=%*ph legacy=%d displayport=%d messaging=%d
> input=%d peer_type=%d",
> + req->u.conn_stat.port_number,
> + (int)ARRAY_SIZE(req->u.conn_stat.guid),
> req->u.conn_stat.guid,
> + req->u.conn_stat.legacy_device_plug_status,
> + req->u.conn_stat.displayport_device_plug_status,
> + req->u.conn_stat.message_capability_status,
> + req->u.conn_stat.input_port,
> + req->u.conn_stat.peer_device_type);
> + break;
> + case
Re: [Freedreno] [PATCH 0/4] drm/msm/dp: Add support for SC8180x eDP controller
Hi Bjorn I had a quick glance on the series and before getting to other things wanted to know how you are initializing two different connectors for DP & EDP resp. The connector type for DP should be DRM_MODE_CONNECTOR_DisplayPort and eDP should be DRM_MODE_CONNECTOR_eDP. We need both to be created so that both EDP and DP can be supported concurrently. Will these changes work for concurrent eDP and DP case? Thanks Abhinav On 2021-05-10 21:20, Bjorn Andersson wrote: The first patch in the series is somewhat unrelated to the support, but simplifies reasoning and debugging of timing related issues. The second patch introduces support for dealing with different register block layouts, which is used in the forth patch to describe the hardware blocks found in the SC8180x eDP block. The third patch configures the INTF_CONFIG register, which carries the configuration for widebus handling. As with the DPU the bootloader enables widebus and we need to disable it, or implement support for adjusting the timing. Bjorn Andersson (4): drm/msm/dp: Simplify the mvid/nvid calculation drm/msm/dp: Store each subblock in the io region drm/msm/dp: Initialize the INTF_CONFIG register drm/msm/dp: Add support for SC8180x eDP drivers/gpu/drm/msm/dp/dp_catalog.c | 99 +++-- drivers/gpu/drm/msm/dp/dp_display.c | 1 + drivers/gpu/drm/msm/dp/dp_parser.c | 22 +++ drivers/gpu/drm/msm/dp/dp_parser.h | 8 +++ 4 files changed, 53 insertions(+), 77 deletions(-)
Re: [RFC PATCH 03/97] drm/i915/gt: Move CS interrupt handler to the backend
On Thu, May 06, 2021 at 12:13:17PM -0700, Matthew Brost wrote:
> From: Chris Wilson
>
> The different submission backends each have their own preferred
> behaviour and interrupt setup. Let each handle their own interrupts.
>
> This becomes more useful later as we to extract the use of auxiliary
> state in the interrupt handler that is backend specific.
>
> Signed-off-by: Matthew Brost
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
Reviewed-by: Matthew Brost
> ---
> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 7 ++
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 14 +---
> .../drm/i915/gt/intel_execlists_submission.c | 41 ++
> drivers/gpu/drm/i915/gt/intel_gt_irq.c| 82 ++-
> drivers/gpu/drm/i915/gt/intel_gt_irq.h| 23 ++
> .../gpu/drm/i915/gt/intel_ring_submission.c | 8 ++
> drivers/gpu/drm/i915/gt/intel_rps.c | 2 +-
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 11 ++-
> drivers/gpu/drm/i915/i915_irq.c | 10 ++-
> 9 files changed, 124 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 0618379b68ca..828e1669f92c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -255,6 +255,11 @@ static void intel_engine_sanitize_mmio(struct
> intel_engine_cs *engine)
> intel_engine_set_hwsp_writemask(engine, ~0u);
> }
>
> +static void nop_irq_handler(struct intel_engine_cs *engine, u16 iir)
> +{
> + GEM_DEBUG_WARN_ON(iir);
> +}
> +
> static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
> {
> const struct engine_info *info = _engines[id];
> @@ -292,6 +297,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum
> intel_engine_id id)
> engine->hw_id = info->hw_id;
> engine->guc_id = MAKE_GUC_ID(info->class, info->instance);
>
> + engine->irq_handler = nop_irq_handler;
> +
> engine->class = info->class;
> engine->instance = info->instance;
> __sprint_engine_name(engine);
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index 883bafc44902..9ef349cd5cea 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -402,6 +402,7 @@ struct intel_engine_cs {
> u32 irq_enable_mask; /* bitmask to enable ring interrupt */
> void(*irq_enable)(struct intel_engine_cs *engine);
> void(*irq_disable)(struct intel_engine_cs *engine);
> + void(*irq_handler)(struct intel_engine_cs *engine, u16 iir);
>
> void(*sanitize)(struct intel_engine_cs *engine);
> int (*resume)(struct intel_engine_cs *engine);
> @@ -481,10 +482,9 @@ struct intel_engine_cs {
> #define I915_ENGINE_HAS_PREEMPTION BIT(2)
> #define I915_ENGINE_HAS_SEMAPHORES BIT(3)
> #define I915_ENGINE_HAS_TIMESLICES BIT(4)
> -#define I915_ENGINE_NEEDS_BREADCRUMB_TASKLET BIT(5)
> -#define I915_ENGINE_IS_VIRTUAL BIT(6)
> -#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(7)
> -#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(8)
> +#define I915_ENGINE_IS_VIRTUAL BIT(5)
> +#define I915_ENGINE_HAS_RELATIVE_MMIO BIT(6)
> +#define I915_ENGINE_REQUIRES_CMD_PARSER BIT(7)
> unsigned int flags;
>
> /*
> @@ -593,12 +593,6 @@ intel_engine_has_timeslices(const struct intel_engine_cs
> *engine)
> return engine->flags & I915_ENGINE_HAS_TIMESLICES;
> }
>
> -static inline bool
> -intel_engine_needs_breadcrumb_tasklet(const struct intel_engine_cs *engine)
> -{
> - return engine->flags & I915_ENGINE_NEEDS_BREADCRUMB_TASKLET;
> -}
> -
> static inline bool
> intel_engine_is_virtual(const struct intel_engine_cs *engine)
> {
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 9d2da5ccaef6..8db200422950 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -118,6 +118,7 @@
> #include "intel_engine_stats.h"
> #include "intel_execlists_submission.h"
> #include "intel_gt.h"
> +#include "intel_gt_irq.h"
> #include "intel_gt_pm.h"
> #include "intel_gt_requests.h"
> #include "intel_lrc.h"
> @@ -2384,6 +2385,45 @@ static void execlists_submission_tasklet(struct
> tasklet_struct *t)
> rcu_read_unlock();
> }
>
> +static void execlists_irq_handler(struct intel_engine_cs *engine, u16 iir)
> +{
> + bool tasklet = false;
> +
> + if (unlikely(iir & GT_CS_MASTER_ERROR_INTERRUPT)) {
> + u32 eir;
> +
> + /* Upper 16b are the enabling mask, rsvd for internal errors */
> + eir = ENGINE_READ(engine, RING_EIR) & GENMASK(15, 0);
> + ENGINE_TRACE(engine, "CS error: %x\n", eir);
> +
> + /*
Re: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
Swapping SG BOs makes no sense, because TTM doesn't own the pages of
this type of BO.
Last I checked, userptr BOs (and other SG BOs) were protected from
swapout by the fact that they would not be added to the swap-LRU. But it
looks like Christian just removed the swap-LRU. I guess this broke that
protection:
commit 2cb51d22d70b18eaf339abf9758bf0b7608da65c
Author: Christian König
Date: Tue Oct 6 16:30:09 2020 +0200
drm/ttm: remove swap LRU v3
Instead evict round robin from each devices SYSTEM and TT domain.
v2: reorder num_pages access reported by Dan's script
v3: fix rebase fallout, num_pages should be 32bit
Signed-off-by: Christian König
Tested-by: Nirmoy Das
Reviewed-by: Huang Rui
Reviewed-by: Matthew Auld
Link: https://patchwork.freedesktop.org/patch/424009/
Regards,
Felix
On 2021-05-18 10:28 p.m., xinhui pan wrote:
cpu 1 cpu 2
kfd alloc BO A(userptr) alloc BO B(GTT)
->init -> validate -> init -> validate ->
populate
init_user_pages -> swapout BO A //hit ttm
pages limit
-> get_user_pages (fill up ttm->pages)
-> validate -> populate
-> swapin BO A // Now hit the BUG
We know that get_user_pages may race with swapout on same BO.
Threre are some issues I have met.
1) memory corruption.
This is because we do a swap before memory is setup. ttm_tt_swapout()
just create a swap_storage with its content being 0x0. So when we setup
memory after the swapout. The following swapin makes the memory
corrupted.
2) panic
When swapout happes with get_user_pages, they touch ttm->pages without
anylock. It causes memory corruption too. But I hit page fault mostly.
Signed-off-by: xinhui pan
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 928e8d57cd08..42460e4480f8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -835,6 +835,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
struct amdkfd_process_info *process_info = mem->process_info;
struct amdgpu_bo *bo = mem->bo;
struct ttm_operation_ctx ctx = { true, false };
+ struct page **pages;
int ret = 0;
mutex_lock(_info->lock);
@@ -852,7 +853,13 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
goto out;
}
- ret = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
+ pages = kvmalloc_array(bo->tbo.ttm->num_pages,
+ sizeof(struct page *),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!pages)
+ goto unregister_out;
+
+ ret = amdgpu_ttm_tt_get_user_pages(bo, pages);
if (ret) {
pr_err("%s: Failed to get user pages: %d\n", __func__, ret);
goto unregister_out;
@@ -863,6 +870,12 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
pr_err("%s: Failed to reserve BO\n", __func__);
goto release_out;
}
+
+ WARN_ON_ONCE(bo->tbo.ttm->page_flags & TTM_PAGE_FLAG_SWAPPED);
+
+ memcpy(bo->tbo.ttm->pages,
+ pages,
+ sizeof(struct page*) * bo->tbo.ttm->num_pages);
amdgpu_bo_placement_from_domain(bo, mem->domain);
ret = ttm_bo_validate(>tbo, >placement, );
if (ret)
@@ -872,6 +885,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
release_out:
amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
unregister_out:
+ kvfree(pages);
if (ret)
amdgpu_mn_unregister(bo);
out:
Re: [RFC PATCH 02/97] drm/i915/gt: Move submission_method into intel_gt
On Thu, May 06, 2021 at 12:13:16PM -0700, Matthew Brost wrote:
> From: Chris Wilson
>
> Since we setup the submission method for the engines once, it is easy to
> assign an enum and use that instead of probing into the backends.
>
> Signed-off-by: Matthew Brost
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
Reviewed-by: Matthew Brost
> ---
> drivers/gpu/drm/i915/gt/intel_engine.h | 8 +++-
> drivers/gpu/drm/i915/gt/intel_engine_cs.c| 12
> drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 8
> drivers/gpu/drm/i915/gt/intel_execlists_submission.h | 3 ---
> drivers/gpu/drm/i915/gt/intel_gt_types.h | 7 +++
> drivers/gpu/drm/i915/gt/intel_reset.c| 7 +++
> drivers/gpu/drm/i915/gt/selftest_execlists.c | 2 +-
> drivers/gpu/drm/i915/gt/selftest_ring_submission.c | 2 +-
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c| 5 -
> drivers/gpu/drm/i915/gt/uc/intel_guc_submission.h| 1 -
> drivers/gpu/drm/i915/i915_perf.c | 10 +-
> 11 files changed, 32 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h
> b/drivers/gpu/drm/i915/gt/intel_engine.h
> index 47ee8578e511..8d9184920c51 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -13,8 +13,9 @@
> #include "i915_reg.h"
> #include "i915_request.h"
> #include "i915_selftest.h"
> -#include "gt/intel_timeline.h"
> #include "intel_engine_types.h"
> +#include "intel_gt_types.h"
> +#include "intel_timeline.h"
> #include "intel_workarounds.h"
>
> struct drm_printer;
> @@ -262,6 +263,11 @@ void intel_engine_init_active(struct intel_engine_cs
> *engine,
> #define ENGINE_MOCK 1
> #define ENGINE_VIRTUAL 2
>
> +static inline bool intel_engine_uses_guc(const struct intel_engine_cs
> *engine)
> +{
> + return engine->gt->submission_method >= INTEL_SUBMISSION_GUC;
> +}
> +
> static inline bool
> intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
> {
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 6dbdbde00f14..0618379b68ca 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -909,12 +909,16 @@ int intel_engines_init(struct intel_gt *gt)
> enum intel_engine_id id;
> int err;
>
> - if (intel_uc_uses_guc_submission(>uc))
> + if (intel_uc_uses_guc_submission(>uc)) {
> + gt->submission_method = INTEL_SUBMISSION_GUC;
> setup = intel_guc_submission_setup;
> - else if (HAS_EXECLISTS(gt->i915))
> + } else if (HAS_EXECLISTS(gt->i915)) {
> + gt->submission_method = INTEL_SUBMISSION_ELSP;
> setup = intel_execlists_submission_setup;
> - else
> + } else {
> + gt->submission_method = INTEL_SUBMISSION_RING;
> setup = intel_ring_submission_setup;
> + }
>
> for_each_engine(engine, gt, id) {
> err = engine_setup_common(engine);
> @@ -1479,7 +1483,7 @@ static void intel_engine_print_registers(struct
> intel_engine_cs *engine,
> drm_printf(m, "\tIPEHR: 0x%08x\n", ENGINE_READ(engine, IPEHR));
> }
>
> - if (intel_engine_in_guc_submission_mode(engine)) {
> + if (intel_engine_uses_guc(engine)) {
> /* nothing to print yet */
> } else if (HAS_EXECLISTS(dev_priv)) {
> struct i915_request * const *port, *rq;
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index 1108c193ab65..9d2da5ccaef6 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -1768,7 +1768,6 @@ process_csb(struct intel_engine_cs *engine, struct
> i915_request **inactive)
>*/
> GEM_BUG_ON(!tasklet_is_locked(>tasklet) &&
> !reset_in_progress(execlists));
> - GEM_BUG_ON(!intel_engine_in_execlists_submission_mode(engine));
>
> /*
>* Note that csb_write, csb_status may be either in HWSP or mmio.
> @@ -3884,13 +3883,6 @@ void intel_execlists_show_requests(struct
> intel_engine_cs *engine,
> spin_unlock_irqrestore(>active.lock, flags);
> }
>
> -bool
> -intel_engine_in_execlists_submission_mode(const struct intel_engine_cs
> *engine)
> -{
> - return engine->set_default_submission ==
> -execlists_set_default_submission;
> -}
> -
> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> #include "selftest_execlists.c"
> #endif
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
> index fd61dae820e9..4ca9b475e252 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.h
> +++
回复: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
[AMD Official Use Only]
To observe the issue. I made one kfdtest case for debug.
It just alloc a userptr memory and detect if memory is corrupted.
I can hit this failure in 2 minutes. :(
diff --git a/tests/kfdtest/src/KFDMemoryTest.cpp
b/tests/kfdtest/src/KFDMemoryTest.cpp
index 70c8033..a72f53f 100644
--- a/tests/kfdtest/src/KFDMemoryTest.cpp
+++ b/tests/kfdtest/src/KFDMemoryTest.cpp
@@ -584,6 +584,32 @@ TEST_F(KFDMemoryTest, ZeroMemorySizeAlloc) {
TEST_END
}
+TEST_F(KFDMemoryTest, swap) {
+TEST_START(TESTPROFILE_RUNALL)
+
+unsigned int size = 128<<20;
+unsigned int*tmp = (unsigned int *)mmap(0,
+ size,
+ PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE,
+ -1,
+ 0);
+EXPECT_NE(tmp, MAP_FAILED);
+
+LOG() << "pls run this with KFDMemoryTest.LargestSysBufferTest" <<
std::endl;
+do {
+ memset(tmp, 0xcc, size);
+
+ HsaMemoryBuffer buf(tmp, size);
+ sleep(1);
+ EXPECT_EQ(tmp[0], 0x);
+} while (true);
+
+munmap(tmp, size);
+
+TEST_END
+}
+
// Basic test for hsaKmtAllocMemory
TEST_F(KFDMemoryTest, MemoryAlloc) {
TEST_START(TESTPROFILE_RUNALL)
--
2.25.1
发件人: Pan, Xinhui
发送时间: 2021年5月19日 10:28
收件人: amd-...@lists.freedesktop.org
抄送: Kuehling, Felix; Deucher, Alexander; Koenig, Christian;
dri-devel@lists.freedesktop.org; dan...@ffwll.ch; Pan, Xinhui
主题: [RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
cpu 1 cpu 2
kfd alloc BO A(userptr) alloc BO B(GTT)
->init -> validate -> init -> validate -> populate
init_user_pages -> swapout BO A //hit ttm
pages limit
-> get_user_pages (fill up ttm->pages)
-> validate -> populate
-> swapin BO A // Now hit the BUG
We know that get_user_pages may race with swapout on same BO.
Threre are some issues I have met.
1) memory corruption.
This is because we do a swap before memory is setup. ttm_tt_swapout()
just create a swap_storage with its content being 0x0. So when we setup
memory after the swapout. The following swapin makes the memory
corrupted.
2) panic
When swapout happes with get_user_pages, they touch ttm->pages without
anylock. It causes memory corruption too. But I hit page fault mostly.
Signed-off-by: xinhui pan
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 928e8d57cd08..42460e4480f8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -835,6 +835,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
struct amdkfd_process_info *process_info = mem->process_info;
struct amdgpu_bo *bo = mem->bo;
struct ttm_operation_ctx ctx = { true, false };
+ struct page **pages;
int ret = 0;
mutex_lock(_info->lock);
@@ -852,7 +853,13 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
goto out;
}
- ret = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
+ pages = kvmalloc_array(bo->tbo.ttm->num_pages,
+ sizeof(struct page *),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!pages)
+ goto unregister_out;
+
+ ret = amdgpu_ttm_tt_get_user_pages(bo, pages);
if (ret) {
pr_err("%s: Failed to get user pages: %d\n", __func__, ret);
goto unregister_out;
@@ -863,6 +870,12 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
pr_err("%s: Failed to reserve BO\n", __func__);
goto release_out;
}
+
+ WARN_ON_ONCE(bo->tbo.ttm->page_flags & TTM_PAGE_FLAG_SWAPPED);
+
+ memcpy(bo->tbo.ttm->pages,
+ pages,
+ sizeof(struct page*) * bo->tbo.ttm->num_pages);
amdgpu_bo_placement_from_domain(bo, mem->domain);
ret = ttm_bo_validate(>tbo, >placement, );
if (ret)
@@ -872,6 +885,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
release_out:
amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
unregister_out:
+ kvfree(pages);
if (ret)
amdgpu_mn_unregister(bo);
out:
--
2.25.1
[RFC PATCH 2/2] drm/ttm: skip swapout when ttm has no backend page.
Signed-off-by: xinhui pan --- drivers/gpu/drm/ttm/ttm_device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c index 510e3e001dab..a9772fcc8f9c 100644 --- a/drivers/gpu/drm/ttm/ttm_device.c +++ b/drivers/gpu/drm/ttm/ttm_device.c @@ -146,6 +146,7 @@ int ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx, uint32_t num_pages; if (!bo->ttm || + !bo->ttm->pages || !bo->ttm->pages[0] || bo->ttm->page_flags & TTM_PAGE_FLAG_SG || bo->ttm->page_flags & TTM_PAGE_FLAG_SWAPPED) continue; -- 2.25.1
[RFC PATCH 1/2] drm/amdgpu: Fix memory corruption due to swapout and swapin
cpu 1 cpu 2
kfd alloc BO A(userptr) alloc BO B(GTT)
->init -> validate -> init -> validate -> populate
init_user_pages -> swapout BO A //hit ttm
pages limit
-> get_user_pages (fill up ttm->pages)
-> validate -> populate
-> swapin BO A // Now hit the BUG
We know that get_user_pages may race with swapout on same BO.
Threre are some issues I have met.
1) memory corruption.
This is because we do a swap before memory is setup. ttm_tt_swapout()
just create a swap_storage with its content being 0x0. So when we setup
memory after the swapout. The following swapin makes the memory
corrupted.
2) panic
When swapout happes with get_user_pages, they touch ttm->pages without
anylock. It causes memory corruption too. But I hit page fault mostly.
Signed-off-by: xinhui pan
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 928e8d57cd08..42460e4480f8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -835,6 +835,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
struct amdkfd_process_info *process_info = mem->process_info;
struct amdgpu_bo *bo = mem->bo;
struct ttm_operation_ctx ctx = { true, false };
+ struct page **pages;
int ret = 0;
mutex_lock(_info->lock);
@@ -852,7 +853,13 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
goto out;
}
- ret = amdgpu_ttm_tt_get_user_pages(bo, bo->tbo.ttm->pages);
+ pages = kvmalloc_array(bo->tbo.ttm->num_pages,
+ sizeof(struct page *),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!pages)
+ goto unregister_out;
+
+ ret = amdgpu_ttm_tt_get_user_pages(bo, pages);
if (ret) {
pr_err("%s: Failed to get user pages: %d\n", __func__, ret);
goto unregister_out;
@@ -863,6 +870,12 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
pr_err("%s: Failed to reserve BO\n", __func__);
goto release_out;
}
+
+ WARN_ON_ONCE(bo->tbo.ttm->page_flags & TTM_PAGE_FLAG_SWAPPED);
+
+ memcpy(bo->tbo.ttm->pages,
+ pages,
+ sizeof(struct page*) * bo->tbo.ttm->num_pages);
amdgpu_bo_placement_from_domain(bo, mem->domain);
ret = ttm_bo_validate(>tbo, >placement, );
if (ret)
@@ -872,6 +885,7 @@ static int init_user_pages(struct kgd_mem *mem, uint64_t
user_addr)
release_out:
amdgpu_ttm_tt_get_user_pages_done(bo->tbo.ttm);
unregister_out:
+ kvfree(pages);
if (ret)
amdgpu_mn_unregister(bo);
out:
--
2.25.1
Re: [PATCH] drm/i915/gvt: remove local storage of debugfs file
On 2021.05.18 18:17:05 +0200, Greg Kroah-Hartman wrote:
> There is no need to keep the dentry around for the debugfs kvmgt cache
> file, as we can just look it up when we want to remove it later on.
> Simplify the structure by removing the dentry and relying on debugfs
> to find the dentry to remove when we want to.
>
> By doing this change, we remove the last in-kernel user that was storing
> the result of debugfs_create_long(), so that api can be cleaned up.
>
> Cc: Zhenyu Wang
> Cc: Zhi Wang
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: intel-gvt-...@lists.freedesktop.org
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman
> ---
> drivers/gpu/drm/i915/gvt/kvmgt.c | 11 +--
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c
> b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 65ff43cfc0f7..433c2a448f2d 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -88,6 +88,7 @@ struct kvmgt_pgfn {
> struct hlist_node hnode;
> };
>
> +#define KVMGT_DEBUGFS_FILENAME "kvmgt_nr_cache_entries"
> struct kvmgt_guest_info {
> struct kvm *kvm;
> struct intel_vgpu *vgpu;
> @@ -95,7 +96,6 @@ struct kvmgt_guest_info {
> #define NR_BKT (1 << 18)
> struct hlist_head ptable[NR_BKT];
> #undef NR_BKT
> - struct dentry *debugfs_cache_entries;
> };
>
> struct gvt_dma {
> @@ -1843,16 +1843,15 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
> info->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
> kvm_page_track_register_notifier(kvm, >track_node);
>
> - info->debugfs_cache_entries = debugfs_create_ulong(
> - "kvmgt_nr_cache_entries",
> - 0444, vgpu->debugfs,
> - >nr_cache_entries);
> + debugfs_create_ulong(KVMGT_DEBUGFS_FILENAME, 0444, vgpu->debugfs,
> + >nr_cache_entries);
> return 0;
> }
>
> static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
> {
> - debugfs_remove(info->debugfs_cache_entries);
> + debugfs_remove(debugfs_lookup(KVMGT_DEBUGFS_FILENAME,
> + info->vgpu->debugfs));
>
> kvm_page_track_unregister_notifier(info->kvm, >track_node);
> kvm_put_kvm(info->kvm);
> --
Reviewed-by: Zhenyu Wang
Thanks!
signature.asc
Description: PGP signature
Re: [RFC PATCH 01/97] drm/i915/gt: Move engine setup out of set_default_submission
On Thu, May 06, 2021 at 12:13:15PM -0700, Matthew Brost wrote:
> From: Chris Wilson
>
> Now that we no longer switch back and forth between guc and execlists,
> we no longer need to restore the backend's vfunc and can leave them set
> after initialisation. The only catch is that we lose the submission on
> wedging and still need to reset the submit_request vfunc on unwedging.
>
> Signed-off-by: Matthew Brost
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
Reviewed-by: Matthew Brost
> ---
> .../drm/i915/gt/intel_execlists_submission.c | 46 -
> .../gpu/drm/i915/gt/intel_ring_submission.c | 4 --
> .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 50 ---
> 3 files changed, 44 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de124870af44..1108c193ab65 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3076,29 +3076,6 @@ static void execlists_set_default_submission(struct
> intel_engine_cs *engine)
> engine->submit_request = execlists_submit_request;
> engine->schedule = i915_schedule;
> engine->execlists.tasklet.callback = execlists_submission_tasklet;
> -
> - engine->reset.prepare = execlists_reset_prepare;
> - engine->reset.rewind = execlists_reset_rewind;
> - engine->reset.cancel = execlists_reset_cancel;
> - engine->reset.finish = execlists_reset_finish;
> -
> - engine->park = execlists_park;
> - engine->unpark = NULL;
> -
> - engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> - if (!intel_vgpu_active(engine->i915)) {
> - engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
> - if (can_preempt(engine)) {
> - engine->flags |= I915_ENGINE_HAS_PREEMPTION;
> - if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> - engine->flags |= I915_ENGINE_HAS_TIMESLICES;
> - }
> - }
> -
> - if (intel_engine_has_preemption(engine))
> - engine->emit_bb_start = gen8_emit_bb_start;
> - else
> - engine->emit_bb_start = gen8_emit_bb_start_noarb;
> }
>
> static void execlists_shutdown(struct intel_engine_cs *engine)
> @@ -3129,6 +3106,14 @@ logical_ring_default_vfuncs(struct intel_engine_cs
> *engine)
> engine->cops = _context_ops;
> engine->request_alloc = execlists_request_alloc;
>
> + engine->reset.prepare = execlists_reset_prepare;
> + engine->reset.rewind = execlists_reset_rewind;
> + engine->reset.cancel = execlists_reset_cancel;
> + engine->reset.finish = execlists_reset_finish;
> +
> + engine->park = execlists_park;
> + engine->unpark = NULL;
> +
> engine->emit_flush = gen8_emit_flush_xcs;
> engine->emit_init_breadcrumb = gen8_emit_init_breadcrumb;
> engine->emit_fini_breadcrumb = gen8_emit_fini_breadcrumb_xcs;
> @@ -3149,6 +3134,21 @@ logical_ring_default_vfuncs(struct intel_engine_cs
> *engine)
>* until a more refined solution exists.
>*/
> }
> +
> + engine->flags |= I915_ENGINE_SUPPORTS_STATS;
> + if (!intel_vgpu_active(engine->i915)) {
> + engine->flags |= I915_ENGINE_HAS_SEMAPHORES;
> + if (can_preempt(engine)) {
> + engine->flags |= I915_ENGINE_HAS_PREEMPTION;
> + if (IS_ACTIVE(CONFIG_DRM_I915_TIMESLICE_DURATION))
> + engine->flags |= I915_ENGINE_HAS_TIMESLICES;
> + }
> + }
> +
> + if (intel_engine_has_preemption(engine))
> + engine->emit_bb_start = gen8_emit_bb_start;
> + else
> + engine->emit_bb_start = gen8_emit_bb_start_noarb;
> }
>
> static void logical_ring_default_irqs(struct intel_engine_cs *engine)
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> index 9585546556ee..5f4f7f1df48f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring_submission.c
> @@ -989,14 +989,10 @@ static void gen6_bsd_submit_request(struct i915_request
> *request)
> static void i9xx_set_default_submission(struct intel_engine_cs *engine)
> {
> engine->submit_request = i9xx_submit_request;
> -
> - engine->park = NULL;
> - engine->unpark = NULL;
> }
>
> static void gen6_bsd_set_default_submission(struct intel_engine_cs *engine)
> {
> - i9xx_set_default_submission(engine);
> engine->submit_request = gen6_bsd_submit_request;
> }
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 92688a9b6717..f72faa0b8339 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -608,35
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 08:03:27PM -0300, Jason Gunthorpe wrote: > Logically during fork all these device exclusive pages should be > reverted back to their CPU pages, write protected and the CPU page PTE > copied to the fork. > > We should not copy the device exclusive page PTE to the fork. I think > I pointed to this on an earlier rev.. Agreed. Though please see the question I posted in the other thread: now I am not very sure whether we'll be able to mark a page as device exclusive if that page has mapcount>1. > > We can optimize this into the various variants above, but logically > device exclusive stop existing during fork. Makes sense, I think that's indeed what this patch did at least for the COW case, so I think Alistair did address that comment. It's just that I think we need to drop the other !COW case (imho that should correspond to the changes in copy_nonpresent_pte()) in this patch to guarantee it. I also hope we don't make copy_pte_range() even more complicated just to do the lock_page() right, so we could fail the fork() if the lock is hard to take. -- Peter Xu
[RFC 2/2] drm/doc/rfc: i915 new parallel submission uAPI plan
Add entry fpr i915 new parallel submission uAPI plan.
v2:
(Daniel Vetter):
- Expand logical order explaination
- Add dummy header
- Only allow N BBs in execbuf IOCTL
- Configure parallel submission per slot not per gem context
Cc: Tvrtko Ursulin
Cc: Tony Ye
CC: Carl Zhang
Cc: Daniel Vetter
Cc: Jason Ekstrand
Signed-off-by: Matthew Brost
---
Documentation/gpu/rfc/i915_parallel_execbuf.h | 144 ++
Documentation/gpu/rfc/i915_scheduler.rst | 53 ++-
2 files changed, 196 insertions(+), 1 deletion(-)
create mode 100644 Documentation/gpu/rfc/i915_parallel_execbuf.h
diff --git a/Documentation/gpu/rfc/i915_parallel_execbuf.h
b/Documentation/gpu/rfc/i915_parallel_execbuf.h
new file mode 100644
index ..8c64b983ccad
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_parallel_execbuf.h
@@ -0,0 +1,144 @@
+#define I915_CONTEXT_ENGINES_EXT_PARALLEL_SUBMIT 2 /* see
i915_context_engines_parallel_submit */
+
+/*
+ * i915_context_engines_parallel_submit:
+ *
+ * Setup a slot to allow multiple BBs to be submitted in a single execbuf
IOCTL.
+ * Those BBs will then be scheduled to run on the GPU in parallel. Multiple
+ * hardware contexts are created internally in the i915 run these BBs. Once a
+ * slot is configured for N BBs only N BBs can be submitted in each execbuf
+ * IOCTL and this is implict behavior (e.g. the user doesn't tell the execbuf
+ * IOCTL there are N BBs, the execbuf IOCTL know how many BBs there are based
on
+ * the slots configuration).
+ *
+ * Their are two currently defined ways to control the placement of the
+ * hardware contexts on physical engines: default behavior (no flags) and
+ * I915_PARALLEL_IMPLICT_BONDS (a flag). More flags may be added the in the
+ * future as new hardware / use cases arise. Details of how to use this
+ * interface below above the flags.
+ *
+ * Returns -EINVAL if hardware context placement configuration invalid or if
the
+ * placement configuration isn't supported on the platform / submission
+ * interface.
+ * Returns -ENODEV if extension isn't supported on the platform / submission
+ * inteface.
+ */
+struct i915_context_engines_parallel_submit {
+ struct i915_user_extension base;
+
+ __u16 engine_index; /* slot for parallel engine */
+ __u16 width;/* number of contexts per parallel engine */
+ __u16 num_siblings; /* number of siblings per context */
+ __u16 mbz16;
+/*
+ * Default placement behvavior (currently unsupported):
+ *
+ * Rather than restricting parallel submission to a single class with a
+ * logically contiguous placement (I915_PARALLEL_IMPLICT_BONDS), add a mode
that
+ * enables parallel submission across multiple engine classes. In this case
each
+ * context's logical engine mask indicates where that context can placed. It is
+ * implied in this mode that all contexts have mutual exclusive placement (e.g.
+ * if one context is running CS0 no other contexts can run on CS0).
+ *
+ * Example 1 pseudo code:
+ * CSX[Y] = engine class X, logical instance Y
+ * INVALID = I915_ENGINE_CLASS_INVALID, I915_ENGINE_CLASS_INVALID_NONE
+ * set_engines(INVALID)
+ * set_parallel(engine_index=0, width=2, num_siblings=2,
+ * engines=CS0[0],CS0[1],CS1[0],CS1[1])
+ *
+ * Results in the following valid placements:
+ * CS0[0], CS1[0]
+ * CS0[0], CS1[1]
+ * CS0[1], CS1[0]
+ * CS0[1], CS1[1]
+ *
+ * This can also be though of as 2 virtual engines:
+ * VE[0] = CS0[0], CS0[1]
+ * VE[1] = CS1[0], CS1[1]
+ *
+ * Example 2 pseudo code:
+ * CS[X] = generic engine of same class, logical instance X
+ * INVALID = I915_ENGINE_CLASS_INVALID, I915_ENGINE_CLASS_INVALID_NONE
+ * set_engines(INVALID)
+ * set_parallel(engine_index=0, width=2, num_siblings=3,
+ * engines=CS[0],CS[1],CS[2],CS[0],CS[1],CS[2])
+ *
+ * Results in the following valid placements:
+ * CS[0], CS[1]
+ * CS[0], CS[2]
+ * CS[1], CS[0]
+ * CS[1], CS[2]
+ * CS[2], CS[0]
+ * CS[2], CS[1]
+ *
+ *
+ * This can also be though of as 2 virtual engines:
+ * VE[0] = CS[0], CS[1], CS[2]
+ * VE[1] = CS[0], CS[1], CS[2]
+
+ * This enables a use case where all engines are created equally, we don't care
+ * where they are scheduled, we just want a certain number of resources, for
+ * those resources to be scheduled in parallel, and possibly across multiple
+ * engine classes.
+ */
+
+/*
+ * I915_PARALLEL_IMPLICT_BONDS - Create implict bonds between each context.
+ * Each context must have the same number sibling and bonds are implictly
create
+ * of the siblings.
+ *
+ * All of the below examples are in logical space.
+ *
+ * Example 1 pseudo code:
+ * CS[X] = generic engine of same class, logical instance X
+ * INVALID = I915_ENGINE_CLASS_INVALID, I915_ENGINE_CLASS_INVALID_NONE
+ * set_engines(INVALID)
+ * set_parallel(engine_index=0, width=2, num_siblings=1,
+ * engines=CS[0],CS[1], flags=I915_PARALLEL_IMPLICT_BONDS)
+ *
+ * Results in the following valid placements:
+ * CS[0], CS[1]
+ *
+ *
[RFC 1/2] drm/doc/rfc: i915 GuC submission / DRM scheduler
Add entry for i915 GuC submission / DRM scheduler integration plan. Follow up patch with details of new parallel submission uAPI to come. v2: (Daniel Vetter) - Expand explaination of why bonding isn't supported for GuC submission - CC some of the DRM scheduler maintainers - Add priority inheritance / boosting use case - Add reasoning for removing in order assumptions (Daniel Stone) - Add links to priority spec Cc: Christian König Cc: Luben Tuikov Cc: Alex Deucher Cc: Steven Price Cc: Jon Bloomfield Cc: Jason Ekstrand Cc: Dave Airlie Cc: Daniel Vetter Cc: Jason Ekstrand Cc: dri-devel@lists.freedesktop.org Signed-off-by: Matthew Brost --- Documentation/gpu/rfc/i915_scheduler.rst | 85 Documentation/gpu/rfc/index.rst | 4 ++ 2 files changed, 89 insertions(+) create mode 100644 Documentation/gpu/rfc/i915_scheduler.rst diff --git a/Documentation/gpu/rfc/i915_scheduler.rst b/Documentation/gpu/rfc/i915_scheduler.rst new file mode 100644 index ..7faa46cde088 --- /dev/null +++ b/Documentation/gpu/rfc/i915_scheduler.rst @@ -0,0 +1,85 @@ += +I915 GuC Submission/DRM Scheduler Section += + +Upstream plan += +For upstream the overall plan for landing GuC submission and integrating the +i915 with the DRM scheduler is: + +* Merge basic GuC submission + * Basic submission support for all gen11+ platforms + * Not enabled by default on any current platforms but can be enabled via + modparam enable_guc + * Lots of rework will need to be done to integrate with DRM scheduler so + no need to nit pick everything in the code, it just should be + functional, no major coding style / layering errors, and not regress + execlists + * Update IGTs / selftests as needed to work with GuC submission + * Enable CI on supported platforms for a baseline + * Rework / get CI heathly for GuC submission in place as needed +* Merge new parallel submission uAPI + * Bonding uAPI completely incompatible with GuC submission, plus it has + severe design issues in general, which is why we want to retire it no + matter what + * New uAPI adds I915_CONTEXT_ENGINES_EXT_PARALLEL context setup step + which configures a slot with N contexts + * After I915_CONTEXT_ENGINES_EXT_PARALLEL a user can submit N batches to + a slot in a single execbuf IOCTL and the batches run on the GPU in + paralllel + * Initially only for GuC submission but execlists can be supported if + needed +* Convert the i915 to use the DRM scheduler + * GuC submission backend fully integrated with DRM scheduler + * All request queues removed from backend (e.g. all backpressure + handled in DRM scheduler) + * Resets / cancels hook in DRM scheduler + * Watchdog hooks into DRM scheduler + * Lots of complexity of the GuC backend can be pulled out once + integrated with DRM scheduler (e.g. state machine gets + simplier, locking gets simplier, etc...) + * Execlist backend will do the minimum required to hook in the DRM + scheduler so it can live next to the fully integrated GuC backend + * Legacy interface + * Features like timeslicing / preemption / virtual engines would + be difficult to integrate with the DRM scheduler and these + features are not required for GuC submission as the GuC does + these things for us + * ROI low on fully integrating into DRM scheduler + * Fully integrating would add lots of complexity to DRM + scheduler + * Port i915 priority inheritance / boosting feature in DRM scheduler + * Used for i915 page flip, may be useful to other DRM drivers as + well + * Will be an optional feature in the DRM scheduler + * Remove in-order completion assumptions from DRM scheduler + * Even when using the DRM scheduler the backends will handle + preemption, timeslicing, etc... so it is possible for jobs to + finish out of order + * Pull out i915 priority levels and use DRM priority levels + * Optimize DRM scheduler as needed + +New uAPI for basic GuC submission += +No major changes are required to the uAPI for basic GuC submission. The only +change is a new scheduler attribute: I915_SCHEDULER_CAP_STATIC_PRIORITY_MAP. +This attribute indicates the 2k i915 user priority levels are statically mapped +into 3 levels as follows: + +* -1k to -1 Low priority +* 0 Medium priority +* 1 to 1k High priority + +This is needed because the GuC only has 4 priority bands. The highest priority +band is reserved with the
[RFC 0/2] GuC submission / DRM scheduler integration plan + new uAPI
Subject and patches say it all. v2: Address comments, patches has details of changes Signed-off-by: Matthew Brost Matthew Brost (2): drm/doc/rfc: i915 GuC submission / DRM scheduler drm/doc/rfc: i915 new parallel submission uAPI plan Documentation/gpu/rfc/i915_parallel_execbuf.h | 144 ++ Documentation/gpu/rfc/i915_scheduler.rst | 136 + Documentation/gpu/rfc/index.rst | 4 + 3 files changed, 284 insertions(+) create mode 100644 Documentation/gpu/rfc/i915_parallel_execbuf.h create mode 100644 Documentation/gpu/rfc/i915_scheduler.rst -- 2.28.0
Re: [GIT PULL] drm/imx: fixes, dma-fence annotation, and color encoding/range plane properties
On Wed, 12 May 2021 at 23:33, Philipp Zabel wrote: > > Hi Dave, Daniel, > > The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5: > > Linux 5.13-rc1 (2021-05-09 14:17:44 -0700) > > are available in the Git repository at: > > git://git.pengutronix.de/git/pza/linux.git tags/imx-drm-next-2021-05-12 Is this for 5.14 or 5.13-rc3? Dave.
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 04:29:14PM -0400, Peter Xu wrote: > On Tue, May 18, 2021 at 04:45:09PM -0300, Jason Gunthorpe wrote: > > On Tue, May 18, 2021 at 02:01:36PM -0400, Peter Xu wrote: > > > > > Indeed it'll be odd for a COW page since for COW page then it means > > > > > after > > > > > parent/child writting to the page it'll clone into two, then it's a > > > > > mistery on > > > > > which one will be the one that "exclusived owned" by the device.. > > > > > > > > For COW pages it is like every other fork case.. We can't reliably > > > > write-protect the device_exclusive page during fork so we must copy it > > > > at fork time. > > > > > > > > Thus three reasonable choices: > > > > - Copy to a new CPU page > > > > - Migrate back to a CPU page and write protect it > > > > - Copy to a new device exclusive page > > > > > > IMHO the ownership question would really help us to answer this one.. > > > > I'm confused about what device ownership you are talking about > > My question was more about the user scenario rather than anything related to > the kernel code, nor does it related to page struct at all. > > Let me try to be a little bit more verbose... > > Firstly, I think one simple solution to handle fork() of device exclusive ptes > is to do just like device private ptes: if COW we convert writable ptes into > readable ptes. Then when CPU access happens (in either parent/child) page > restore triggers which will convert those readable ptes into read-only present > ptes (with the original page backing it). Then do_wp_page() will take care of > page copy. I suspect it doesn't work. This is much more like pinning than anything, the data in the page is still under active use by a device and if we cannot globally write write protect it, both from CPU and device access, then we cannot do COW. IIRC the mm can't trigger a full global write protect through the pgmap? > Then here comes the ownership question: If we still want to have the parent > process behave like before it fork()ed, IMHO we must make sure that original > page (that exclusively owned by the device once) still belongs to the parent > process not the child. That's why I think if that's the case we'd do early > cow > in fork(), because it guarantees that. Logically during fork all these device exclusive pages should be reverted back to their CPU pages, write protected and the CPU page PTE copied to the fork. We should not copy the device exclusive page PTE to the fork. I think I pointed to this on an earlier rev.. We can optimize this into the various variants above, but logically device exclusive stop existing during fork. Jason
Re: [RFC PATCH 0/3] A drm_plane API to support HDR planes
On 2021-05-18 16:19, Harry Wentland wrote: On 2021-05-18 3:56 a.m., Pekka Paalanen wrote: On Mon, 17 May 2021 15:39:03 -0400 Vitaly Prosyak wrote: On 2021-05-17 12:48 p.m., Sebastian Wick wrote: On 2021-05-17 10:57, Pekka Paalanen wrote: On Fri, 14 May 2021 17:05:11 -0400 Harry Wentland wrote: On 2021-04-27 10:50 a.m., Pekka Paalanen wrote: On Mon, 26 Apr 2021 13:38:49 -0400 Harry Wentland wrote: ... ## Mastering Luminances Now we are able to use the PQ 2084 EOTF to define the luminance of pixels in absolute terms. Unfortunately we're again presented with physical limitations of the display technologies on the market today. Here are a few examples of luminance ranges of displays. | Display | Luminance range in nits | | | --- | | Typical PC display | 0.3 - 200 | | Excellent LCD HDTV | 0.3 - 400 | | HDR LCD w/ local dimming | 0.05 - 1,500 | Since no display can currently show the full 0.0005 to 10,000 nits luminance range the display will need to tonemap the HDR content, i.e to fit the content within a display's capabilities. To assist with tonemapping HDR content is usually accompanied with a metadata that describes (among other things) the minimum and maximum mastering luminance, i.e. the maximum and minimum luminance of the display that was used to master the HDR content. The HDR metadata is currently defined on the drm_connector via the hdr_output_metadata blob property. It might be useful to define per-plane hdr metadata, as different planes might have been mastered differently. I don't think this would directly help with the dynamic range blending problem. You still need to establish the mapping between the optical values from two different EOTFs and dynamic ranges. Or can you know which optical values match the mastering display maximum and minimum luminances for not-PQ? My understanding of this is probably best illustrated by this example: Assume HDR was mastered on a display with a maximum white level of 500 nits and played back on a display that supports a max white level of 400 nits. If you know the mastering white level of 500 you know that this is the maximum value you need to compress down to 400 nits, allowing you to use the full extent of the 400 nits panel. Right, but in the kernel, where do you get these nits values from? hdr_output_metadata blob is infoframe data to the monitor. I think this should be independent of the metadata used for color transformations in the display pipeline before the monitor. EDID may tell us the monitor HDR metadata, but again what is used in the color transformations should be independent, because EDIDs lie, lighting environments change, and users have different preferences. What about black levels? Do you want to do black level adjustment? How exactly should the compression work? Where do you map the mid-tones? What if the end user wants something different? I suspect that this is not about tone mapping at all. The use cases listed always have the display in PQ mode and just assume that no content exceeds the PQ limitations. Then you can simply bring all content to the color space with a matrix multiplication and then map the linear light content somewhere into the PQ range. Tone mapping is performed in the display only. The use cases do use the word "desktop" though. Harry, could you expand on this, are you seeking a design that is good for generic desktop compositors too, or one that is more tailored to "embedded" video player systems taking the most advantage of (potentially fixed-function) hardware? The goal is to enable this on a generic desktop, such as generic Wayland implementations or ChromeOS. We're not looking for a custom solution for some embedded systems, though the solution we end up with should obviously not prevent an implementation on embedded video players. What matrix would one choose? Which render intent would it correspond to? If you need to adapt different dynamic ranges into the blending dynamic range, would a simple linear transformation really be enough? From a generic wayland compositor point of view this is uninteresting. It a compositor's decision to provide or not the metadata property to the kernel. The metadata can be available from one or multiple clients or most likely not available at all. Compositors may put a display in HDR10 ( when PQ 2084 INV EOTF and TM occurs in display ) or NATIVE mode and do not attach any metadata to the connector and do TM in compositor. It is all about user preference or compositor design, or a combination of both options. Indeed. The thing here is that you cannot just add KMS UAPI, you also need to have the FOSS userspace to go with it. So you need to have your audience defined, userspace patches written and reviewed and agreed to be a good idea. I'm afraid this particular UAPI design would be difficult to justify with
Re: [RFC] Add DMA_RESV_USAGE flags
On Tue, May 18, 2021 at 4:17 PM Daniel Vetter wrote: > > On Tue, May 18, 2021 at 7:40 PM Christian König > wrote: > > > > Am 18.05.21 um 18:48 schrieb Daniel Vetter: > > > On Tue, May 18, 2021 at 2:49 PM Christian König > > > wrote: > > >> Hi Jason & Daniel, > > >> > > >> Am 18.05.21 um 07:59 schrieb Daniel Vetter: > > >>> On Tue, May 18, 2021 at 12:49 AM Jason Ekstrand > > >>> wrote: > > On Mon, May 17, 2021 at 3:15 PM Daniel Vetter wrote: > > > On Mon, May 17, 2021 at 9:38 PM Christian König > > > wrote: > > >> Am 17.05.21 um 17:04 schrieb Daniel Vetter: > > >>> On Mon, May 17, 2021 at 04:11:18PM +0200, Christian König wrote: > > We had a long outstanding problem in amdgpu that buffers exported > > to > > user drivers by DMA-buf serialize all command submissions using > > them. > > > > In other words we can't compose the buffer with different engines > > and > > then send it to another driver for display further processing. > > > > This was added to work around the fact that i915 didn't wanted to > > wait > > for shared fences in the dma_resv objects before displaying a > > buffer. > > > > Since this problem is now causing issues with Vulkan we need to > > find a > > better solution for that. > > > > The patch set here tries to do this by adding an usage flag to the > > shared fences noting when and how they should participate in > > implicit > > synchronization. > > >>> So the way this is fixed in every other vulkan driver is that vulkan > > >>> userspace sets flags in the CS ioctl when it wants to synchronize > > >>> with > > >>> implicit sync. This gets you mostly there. Last time I checked > > >>> amdgpu > > >>> isn't doing this, and yes that's broken. > > >> And exactly that is a really bad approach as far as I can see. The > > >> Vulkan stack on top simply doesn't know when to set this flag during > > >> CS. > > > Adding Jason for the Vulkan side of things, because this isn't how I > > > understand this works. > > > > > > But purely form a kernel pov your patches are sketchy for two reasons: > > > > > > - we reinstate the amdgpu special case of not setting exclusive fences > > > > > > - you only fix the single special case of i915 display, nothing else > > > > > > That's not how a cross driver interface works. And if you'd do this > > > properly, you'd be back to all the same sync fun you've orignally had, > > > with all the same fallout. > > I think I'm starting to see what Christian is trying to do here and I > > think there likely is a real genuine problem here. I'm not convinced > > this is 100% of a solution but there might be something real. Let me > > see if I can convince you or if I just make a hash of things. :-) > > > > The problem, once again, comes down to memory fencing vs. execution > > fencing and the way that we've unfortunately tied them together in the > > kernel. With the current architecture, the only way to get proper > > write-fence semantics for implicit sync is to take an exclusive fence > > on the buffer. This implies two things: > > > > 1. You have to implicitly wait on EVERY fence on the buffer before > > you can start your write-fenced operation > > > > 2. No one else can start ANY operation which accesses that buffer > > until you're done. > > >> Yes, exactly that. You absolutely nailed it. > > >> > > >> I unfortunately also have a 3rd use case: > > >> > > >> 3. Operations which shouldn't participate in any syncing, but only > > >> affect the memory management. > > >> > > >> This is basically our heavyweight TLB flush after unmapping the BO from > > >> somebodies page tables. Nobody should ever be concerned about it for any > > >> form of synchronization, but memory managment is not allowed to reuse or > > >> move the buffer before the operation is completed. > > > Isn't that just another case of 2? Or I'm not getting it. > > > > The problem in this case is not starting a new CS, but synchronizing to > > the existing ones. > > > > See a heavy TLB flush is made completely out of sync. E.g. it doesn't > > want to wait for any previous operation. > > > > In other words imagine the following example: > > 1. Both process A and B have a BO mapped. > > 2. Process A is heavily using the BO and doing all kind of rendering. > > 3. Process B is unmapping the BO. > > > > Now that process B unmaps the BO needs to trigger page table updates and > > a heavy TLB flush, but since this can take really long we want to do it > > asynchronously on the hardware. > > > > With the current approach you basically can't do that because you can't > > note that a fence should not participate in
Re: [PATCH v4 1/3] drm/dp_mst: Add self-tests for up requests
Looks like these tests might still need to be fixed up a bit:
[ 34.785042] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]]
connection status reply parse length fail 2 1
[ 34.785082] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]]
resource status reply parse length fail 2 1
[ 34.785114] (null): [drm:drm_dp_sideband_parse_req [drm_kms_helper]] sink
event notify parse length fail 2 1
[ 34.785146] (null): [drm] *ERROR* Got unknown request 0x23
(REMOTE_I2C_WRITE)
On Tue, 2021-05-18 at 22:35 +1000, Sam McNally wrote:
Up requests are decoded by drm_dp_sideband_parse_req(), which operates
on a drm_dp_sideband_msg_rx, unlike down requests. Expand the existing
self-test helper sideband_msg_req_encode_decode() to copy the message
contents and length from a drm_dp_sideband_msg_tx to
drm_dp_sideband_msg_rx and use the parse function under test in place of
decode.
Add support for currently-supported up requests to
drm_dp_dump_sideband_msg_req_body(); add support to
drm_dp_encode_sideband_req() to allow encoding for the self-tests.
Add self-tests for CONNECTION_STATUS_NOTIFY and RESOURCE_STATUS_NOTIFY.
Signed-off-by: Sam McNally
---
Changes in v4:
- New in v4
drivers/gpu/drm/drm_dp_mst_topology.c | 54 ++-
.../gpu/drm/drm_dp_mst_topology_internal.h | 4 +
.../drm/selftests/test-drm_dp_mst_helper.c | 147 --
3 files changed, 190 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index 54604633e65c..573f39a3dc16 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -442,6 +442,37 @@ drm_dp_encode_sideband_req(const struct
drm_dp_sideband_msg_req_body *req,
idx++;
}
break;
+ case DP_CONNECTION_STATUS_NOTIFY: {
+ const struct drm_dp_connection_status_notify *msg;
+
+ msg = >u.conn_stat;
+ buf[idx] = (msg->port_number & 0xf) << 4;
+ idx++;
+ memcpy(>msg[idx], msg->guid, 16);
+ idx += 16;
+ raw->msg[idx] = 0;
+ raw->msg[idx] |= msg->legacy_device_plug_status ? BIT(6) : 0;
+ raw->msg[idx] |= msg->displayport_device_plug_status ? BIT(5) :
0;
+ raw->msg[idx] |= msg->message_capability_status ? BIT(4) : 0;
+ raw->msg[idx] |= msg->input_port ? BIT(3) : 0;
+ raw->msg[idx] |= FIELD_PREP(GENMASK(2, 0), msg-
>peer_device_type);
+ idx++;
+ break;
+ }
+ case DP_RESOURCE_STATUS_NOTIFY: {
+ const struct drm_dp_resource_status_notify *msg;
+
+ msg = >u.resource_stat;
+ buf[idx] = (msg->port_number & 0xf) << 4;
+ idx++;
+ memcpy(>msg[idx], msg->guid, 16);
+ idx += 16;
+ buf[idx] = (msg->available_pbn & 0xff00) >> 8;
+ idx++;
+ buf[idx] = (msg->available_pbn & 0xff);
+ idx++;
+ break;
+ }
}
raw->cur_len = idx;
}
@@ -672,6 +703,22 @@ drm_dp_dump_sideband_msg_req_body(const struct
drm_dp_sideband_msg_req_body *req
req->u.enc_status.stream_behavior,
req->u.enc_status.valid_stream_behavior);
break;
+ case DP_CONNECTION_STATUS_NOTIFY:
+ P("port=%d guid=%*ph legacy=%d displayport=%d messaging=%d
input=%d peer_type=%d",
+ req->u.conn_stat.port_number,
+ (int)ARRAY_SIZE(req->u.conn_stat.guid), req->u.conn_stat.guid,
+ req->u.conn_stat.legacy_device_plug_status,
+ req->u.conn_stat.displayport_device_plug_status,
+ req->u.conn_stat.message_capability_status,
+ req->u.conn_stat.input_port,
+ req->u.conn_stat.peer_device_type);
+ break;
+ case DP_RESOURCE_STATUS_NOTIFY:
+ P("port=%d guid=%*ph pbn=%d",
+ req->u.resource_stat.port_number,
+ (int)ARRAY_SIZE(req->u.resource_stat.guid), req-
>u.resource_stat.guid,
+ req->u.resource_stat.available_pbn);
+ break;
default:
P("???\n");
break;
@@ -1116,9 +1163,9 @@ static bool
drm_dp_sideband_parse_resource_status_notify(const struct drm_dp_mst
return false;
}
-static bool drm_dp_sideband_parse_req(const struct drm_dp_mst_topology_mgr
*mgr,
- struct drm_dp_sideband_msg_rx *raw,
- struct drm_dp_sideband_msg_req_body *msg)
+bool drm_dp_sideband_parse_req(const struct drm_dp_mst_topology_mgr *mgr,
+ struct drm_dp_sideband_msg_rx *raw,
+ struct drm_dp_sideband_msg_req_body *msg)
{
memset(msg, 0, sizeof(*msg));
msg->req_type
Re: [PATCH] dt-bindings: display: convert faraday,tve200 to YAML
On Tue, May 18, 2021 at 8:38 PM LABBE Corentin wrote: > The only solution is to remove "reg = <0>;" and calling the node "port". > Does it is acceptable ? It's what I've done in the past at least so looks like the right thing to me. It is only used in this device tree: arch/arm/boot/dts/gemini-dlink-dir-685.dts if you send me a patch to change port@0 to just port then I'll merge it pronto. (I can apply this patch to the DRM misc tree as well when Rob thing it's fine.) Yours, Linus Walleij
Re: [RFC] Add DMA_RESV_USAGE flags
> > We basically don't know during CS if a BO is shared or not. Who doesn't know? We should be able to track this quite easily, userspace either imports or exports buffers, it can surely keep track of these and flag them. Is this a userspace might lie to use worry or do you have some really broken userspace we don't know about? Dave.
[PATCH] drm: fix leaked dma handles after removing drm_pci_free
After removing drm_pci_alloc/free, some instances where drm_pci_free() would have kfreed the dma handle were skipped. Ensure these handles are freed properly. Signed-off-by: Joseph Kogut --- drivers/gpu/drm/drm_bufs.c | 1 + drivers/gpu/drm/r128/ati_pcigart.c | 2 ++ 2 files changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_bufs.c b/drivers/gpu/drm/drm_bufs.c index ea3ca81be9dd..7eb3baed9a70 100644 --- a/drivers/gpu/drm/drm_bufs.c +++ b/drivers/gpu/drm/drm_bufs.c @@ -685,6 +685,7 @@ static void drm_cleanup_buf_error(struct drm_device *dev, dmah->size, dmah->vaddr, dmah->busaddr); + kfree(dmah); } } kfree(entry->seglist); diff --git a/drivers/gpu/drm/r128/ati_pcigart.c b/drivers/gpu/drm/r128/ati_pcigart.c index fbb0cfd79758..04408f372f74 100644 --- a/drivers/gpu/drm/r128/ati_pcigart.c +++ b/drivers/gpu/drm/r128/ati_pcigart.c @@ -71,6 +71,8 @@ static void drm_ati_free_pcigart_table(struct drm_device *dev, drm_dma_handle_t *dmah = gart_info->table_handle; dma_free_coherent(dev->dev, dmah->size, dmah->vaddr, dmah->busaddr); + kfree(dmah); + gart_info->table_handle = NULL; } -- 2.31.1
Re: [RFC] Add DMA_RESV_USAGE flags
On Tue, May 18, 2021 at 7:40 PM Christian König wrote: > > Am 18.05.21 um 18:48 schrieb Daniel Vetter: > > On Tue, May 18, 2021 at 2:49 PM Christian König > > wrote: > >> Hi Jason & Daniel, > >> > >> Am 18.05.21 um 07:59 schrieb Daniel Vetter: > >>> On Tue, May 18, 2021 at 12:49 AM Jason Ekstrand > >>> wrote: > On Mon, May 17, 2021 at 3:15 PM Daniel Vetter wrote: > > On Mon, May 17, 2021 at 9:38 PM Christian König > > wrote: > >> Am 17.05.21 um 17:04 schrieb Daniel Vetter: > >>> On Mon, May 17, 2021 at 04:11:18PM +0200, Christian König wrote: > We had a long outstanding problem in amdgpu that buffers exported to > user drivers by DMA-buf serialize all command submissions using them. > > In other words we can't compose the buffer with different engines and > then send it to another driver for display further processing. > > This was added to work around the fact that i915 didn't wanted to > wait > for shared fences in the dma_resv objects before displaying a buffer. > > Since this problem is now causing issues with Vulkan we need to find > a > better solution for that. > > The patch set here tries to do this by adding an usage flag to the > shared fences noting when and how they should participate in implicit > synchronization. > >>> So the way this is fixed in every other vulkan driver is that vulkan > >>> userspace sets flags in the CS ioctl when it wants to synchronize with > >>> implicit sync. This gets you mostly there. Last time I checked amdgpu > >>> isn't doing this, and yes that's broken. > >> And exactly that is a really bad approach as far as I can see. The > >> Vulkan stack on top simply doesn't know when to set this flag during > >> CS. > > Adding Jason for the Vulkan side of things, because this isn't how I > > understand this works. > > > > But purely form a kernel pov your patches are sketchy for two reasons: > > > > - we reinstate the amdgpu special case of not setting exclusive fences > > > > - you only fix the single special case of i915 display, nothing else > > > > That's not how a cross driver interface works. And if you'd do this > > properly, you'd be back to all the same sync fun you've orignally had, > > with all the same fallout. > I think I'm starting to see what Christian is trying to do here and I > think there likely is a real genuine problem here. I'm not convinced > this is 100% of a solution but there might be something real. Let me > see if I can convince you or if I just make a hash of things. :-) > > The problem, once again, comes down to memory fencing vs. execution > fencing and the way that we've unfortunately tied them together in the > kernel. With the current architecture, the only way to get proper > write-fence semantics for implicit sync is to take an exclusive fence > on the buffer. This implies two things: > > 1. You have to implicitly wait on EVERY fence on the buffer before > you can start your write-fenced operation > > 2. No one else can start ANY operation which accesses that buffer > until you're done. > >> Yes, exactly that. You absolutely nailed it. > >> > >> I unfortunately also have a 3rd use case: > >> > >> 3. Operations which shouldn't participate in any syncing, but only > >> affect the memory management. > >> > >> This is basically our heavyweight TLB flush after unmapping the BO from > >> somebodies page tables. Nobody should ever be concerned about it for any > >> form of synchronization, but memory managment is not allowed to reuse or > >> move the buffer before the operation is completed. > > Isn't that just another case of 2? Or I'm not getting it. > > The problem in this case is not starting a new CS, but synchronizing to > the existing ones. > > See a heavy TLB flush is made completely out of sync. E.g. it doesn't > want to wait for any previous operation. > > In other words imagine the following example: > 1. Both process A and B have a BO mapped. > 2. Process A is heavily using the BO and doing all kind of rendering. > 3. Process B is unmapping the BO. > > Now that process B unmaps the BO needs to trigger page table updates and > a heavy TLB flush, but since this can take really long we want to do it > asynchronously on the hardware. > > With the current approach you basically can't do that because you can't > note that a fence should not participate in synchronization at all. > > E.g. we can't add a fence which doesn't wait for the exclusive one as > shared. Ok I think that's a real problem, and guess it's also related to all the ttm privatization tricks and all that. So essentially we'd need the opposite of ttm_bo->moving, as in you can't ignore it, but otherwise it completely ignores all
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Wed, Apr 07, 2021 at 06:42:35PM +1000, Alistair Popple wrote:
[...]
> +static bool try_to_protect(struct page *page, struct mm_struct *mm,
> +unsigned long address, void *arg)
> +{
> + struct ttp_args ttp = {
> + .mm = mm,
> + .address = address,
> + .arg = arg,
> + .valid = false,
> + };
> + struct rmap_walk_control rwc = {
> + .rmap_one = try_to_protect_one,
> + .done = page_not_mapped,
> + .anon_lock = page_lock_anon_vma_read,
> + .arg = ,
> + };
> +
> + /*
> + * Restrict to anonymous pages for now to avoid potential writeback
> + * issues.
> + */
> + if (!PageAnon(page))
> + return false;
> +
> + /*
> + * During exec, a temporary VMA is setup and later moved.
> + * The VMA is moved under the anon_vma lock but not the
> + * page tables leading to a race where migration cannot
> + * find the migration ptes. Rather than increasing the
> + * locking requirements of exec(), migration skips
> + * temporary VMAs until after exec() completes.
> + */
> + if (!PageKsm(page) && PageAnon(page))
> + rwc.invalid_vma = invalid_migration_vma;
> +
> + rmap_walk(page, );
> +
> + return ttp.valid && !page_mapcount(page);
> +}
I raised a question in the other thread regarding fork():
https://lore.kernel.org/lkml/YKQjmtMo+YQGx%2FwZ@t490s/
While I suddenly noticed that we may have similar issues even if we fork()
before creating the ptes.
In that case, we may see multiple read-only ptes pointing to the same page. We
will convert all of them into device exclusive read ptes in rmap_walk() above,
however how do we guarantee after all COW done in the parent and all the childs
processes, the device owned page will be returned to the parent?
E.g., if parent accesses the page earlier than the children processes
(actually, as long as not the last one), do_wp_page() will do COW for parent on
this page because refcount(page)>1, then the page seems to get lost to a random
child too..
To resolve all these complexity, not sure whether try_to_protect() could
enforce VM_DONTCOPY (needs madvise MADV_DONTFORK in the user app), meanwhile
make sure mapcount(page)==1 before granting the page to the device, so that
this will guarantee this mm owns this page forever, I think? It'll simplify
fork() too as a side effect, since VM_DONTCOPY vma go away when fork.
--
Peter Xu
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 2:48 p.m., Andrey Grodzovsky wrote:
On 2021-05-18 2:13 p.m., Christian König wrote:
Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
On 2021-05-18 2:02 p.m., Christian König wrote:
Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
On 2021-05-18 12:33 p.m., Christian König wrote:
Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated
over the dma_resv requirements and I came to the conclusion
that this approach here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and
exports it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the
rendering, for the the fence of device A is replaced with a
new operation.
3. device B is hot plugged and the new operation
canceled/newer scheduled.
The problem is now that we can't do this since the operation
of device A is still running and by signaling our fences we
run into the problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the
uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits
for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Exactly that, yes.
In other words when you have a dependency chain like A->B->C then
memory management only waits for C before freeing up the memory
for example.
When C now signaled because the device is hot-plugged before A or
B are finished they are essentially accessing freed up memory.
But didn't C imported the BO form B or A in this case ? Why would
he be
the one releasing that memory ? He would be just dropping his
reference
to the BO, no ?
Well freeing the memory was just an example. The BO could also move
back to VRAM because of the dropped reference.
Also in the general case,
drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
the one who signals the 'C' fence with error code are as far
as I looked called from when the user of that BO is stopping
the usage anyway (e.g. drm_driver.postclose callback for when use
process closes his device file) who would then access and corrupt
the exported memory on device A where the job hasn't completed yet ?
Key point is that memory management only waits for the last added
fence, that is the design of the dma_resv object. How that happens
is irrelevant.
Because of this we at least need to wait for all dependencies of the
job before signaling the fence, even if we cancel the job for some
reason.
Christian.
Would this be the right way to do it ?
Yes, it is at least a start. Question is if we can wait blocking here
or not.
We install a callback a bit lower to avoid blocking, so I'm pretty
sure that won't work as expected.
Christian.
I can't see why this would create problems, as long as the dependencies
complete or force competed if they are from same device (extracted) but
on a different ring then looks to me it should work. I will give it
a try.
Andrey
Well, i gave it a try with my usual tests like IGT hot unplug while
rapid command submissions and unplug the card while glxgears runs
with DRI_PRIME=1 and haven't seen issues.
Andrey
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 2e93e881b65f..10f784874b63 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct
drm_sched_entity *entity)
{
struct drm_sched_job *job;
int r;
+ struct dma_fence *f;
while ((job =
to_drm_sched_job(spsc_queue_pop(>job_queue {
struct drm_sched_fence *s_fence = job->s_fence;
+ while (f = sched->ops->dependency(sched_job, entity))
+ dma_fence_wait(f);
+
drm_sched_fence_scheduled(s_fence);
dma_fence_set_error(_fence->finished, -ESRCH);
Andrey
Andrey
Christian.
Andrey
I am not sure this problem you describe above is related to
this patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need
to cleanly wait for their dependencies before they can be
completed.
drm_sched_entity_kill_jobs() is also not handling that correctly
at the moment, we only wait for the last scheduled fence but not
for the dependencies of the job.
Were you addressing the patch from yesterday in which you
commented
that you found a problem with how we finish fences ? It was
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 04:45:09PM -0300, Jason Gunthorpe wrote: > On Tue, May 18, 2021 at 02:01:36PM -0400, Peter Xu wrote: > > > > Indeed it'll be odd for a COW page since for COW page then it means > > > > after > > > > parent/child writting to the page it'll clone into two, then it's a > > > > mistery on > > > > which one will be the one that "exclusived owned" by the device.. > > > > > > For COW pages it is like every other fork case.. We can't reliably > > > write-protect the device_exclusive page during fork so we must copy it > > > at fork time. > > > > > > Thus three reasonable choices: > > > - Copy to a new CPU page > > > - Migrate back to a CPU page and write protect it > > > - Copy to a new device exclusive page > > > > IMHO the ownership question would really help us to answer this one.. > > I'm confused about what device ownership you are talking about My question was more about the user scenario rather than anything related to the kernel code, nor does it related to page struct at all. Let me try to be a little bit more verbose... Firstly, I think one simple solution to handle fork() of device exclusive ptes is to do just like device private ptes: if COW we convert writable ptes into readable ptes. Then when CPU access happens (in either parent/child) page restore triggers which will convert those readable ptes into read-only present ptes (with the original page backing it). Then do_wp_page() will take care of page copy. However... if you see that also means parent/child have the equal opportunity to reuse that original page: who access first will do COW because refcount>1 for that page (note! it's possible that mapcount==1 here, as we drop mapcount when converting to device exclusive ptes; however with the most recent do_wp_page change from Linus where we'll also check page_count(), we'll still do COW just like when this page was GUPed by someone else). While that matters because the device is writting to that original page only, not the COWed one. Then here comes the ownership question: If we still want to have the parent process behave like before it fork()ed, IMHO we must make sure that original page (that exclusively owned by the device once) still belongs to the parent process not the child. That's why I think if that's the case we'd do early cow in fork(), because it guarantees that. I can't say I fully understand the whole picture, so sorry if I missed something important there. Thanks, -- Peter Xu
Re: [PATCH v8 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
* Alistair Popple [210407 04:43]: > The behaviour of try_to_unmap_one() is difficult to follow because it > performs different operations based on a fairly large set of flags used > in different combinations. > > TTU_MUNLOCK is one such flag. However it is exclusively used by > try_to_munlock() which specifies no other flags. Therefore rather than > overload try_to_unmap_one() with unrelated behaviour split this out into > it's own function and remove the flag. > > Signed-off-by: Alistair Popple > Reviewed-by: Ralph Campbell > Reviewed-by: Christoph Hellwig > > --- > > v8: > * Renamed try_to_munlock to page_mlock to better reflect what the > function actually does. > * Removed the TODO from the documentation that this patch addresses. > > v7: > * Added Christoph's Reviewed-by > > v4: > * Removed redundant check for VM_LOCKED > --- > Documentation/vm/unevictable-lru.rst | 33 --- > include/linux/rmap.h | 3 +- > mm/mlock.c | 10 +++--- > mm/rmap.c| 48 +--- > 4 files changed, 55 insertions(+), 39 deletions(-) > > diff --git a/Documentation/vm/unevictable-lru.rst > b/Documentation/vm/unevictable-lru.rst > index 0e1490524f53..eae3af17f2d9 100644 > --- a/Documentation/vm/unevictable-lru.rst > +++ b/Documentation/vm/unevictable-lru.rst > @@ -389,14 +389,14 @@ mlocked, munlock_vma_page() updates that zone > statistics for the number of > mlocked pages. Note, however, that at this point we haven't checked whether > the page is mapped by other VM_LOCKED VMAs. > > -We can't call try_to_munlock(), the function that walks the reverse map to > +We can't call page_mlock(), the function that walks the reverse map to > check for other VM_LOCKED VMAs, without first isolating the page from the > LRU. > -try_to_munlock() is a variant of try_to_unmap() and thus requires that the > page > +page_mlock() is a variant of try_to_unmap() and thus requires that the page > not be on an LRU list [more on these below]. However, the call to > -isolate_lru_page() could fail, in which case we couldn't try_to_munlock(). > So, > +isolate_lru_page() could fail, in which case we can't call page_mlock(). So, > we go ahead and clear PG_mlocked up front, as this might be the only chance > we > -have. If we can successfully isolate the page, we go ahead and > -try_to_munlock(), which will restore the PG_mlocked flag and update the zone > +have. If we can successfully isolate the page, we go ahead and call > +page_mlock(), which will restore the PG_mlocked flag and update the zone > page statistics if it finds another VMA holding the page mlocked. If we fail > to isolate the page, we'll have left a potentially mlocked page on the LRU. > This is fine, because we'll catch it later if and if vmscan tries to reclaim > @@ -545,31 +545,24 @@ munlock or munmap system calls, mm teardown > (munlock_vma_pages_all), reclaim, > holepunching, and truncation of file pages and their anonymous COWed pages. > > > -try_to_munlock() Reverse Map Scan > +page_mlock() Reverse Map Scan > - > > -.. warning:: > - [!] TODO/FIXME: a better name might be page_mlocked() - analogous to the > - page_referenced() reverse map walker. > - > When munlock_vma_page() [see section :ref:`munlock()/munlockall() System Call > Handling ` above] tries to munlock a > page, it needs to determine whether or not the page is mapped by any > VM_LOCKED VMA without actually attempting to unmap all PTEs from the > page. For this purpose, the unevictable/mlock infrastructure > -introduced a variant of try_to_unmap() called try_to_munlock(). > +introduced a variant of try_to_unmap() called page_mlock(). > > -try_to_munlock() calls the same functions as try_to_unmap() for anonymous and > -mapped file and KSM pages with a flag argument specifying unlock versus unmap > -processing. Again, these functions walk the respective reverse maps looking > -for VM_LOCKED VMAs. When such a VMA is found, as in the try_to_unmap() case, > -the functions mlock the page via mlock_vma_page() and return SWAP_MLOCK. > This > -undoes the pre-clearing of the page's PG_mlocked done by munlock_vma_page. > +page_mlock() walks the respective reverse maps looking for VM_LOCKED VMAs. > When > +such a VMA is found the page is mlocked via mlock_vma_page(). This undoes the > +pre-clearing of the page's PG_mlocked done by munlock_vma_page. > > -Note that try_to_munlock()'s reverse map walk must visit every VMA in a > page's > +Note that page_mlock()'s reverse map walk must visit every VMA in a page's > reverse map to determine that a page is NOT mapped into any VM_LOCKED VMA. > However, the scan can terminate when it encounters a VM_LOCKED VMA. > -Although try_to_munlock() might be called a great many times when munlocking > a > +Although page_mlock() might be called a great many times when munlocking a > large region or
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 02:01:36PM -0400, Peter Xu wrote: > > > Indeed it'll be odd for a COW page since for COW page then it means after > > > parent/child writting to the page it'll clone into two, then it's a > > > mistery on > > > which one will be the one that "exclusived owned" by the device.. > > > > For COW pages it is like every other fork case.. We can't reliably > > write-protect the device_exclusive page during fork so we must copy it > > at fork time. > > > > Thus three reasonable choices: > > - Copy to a new CPU page > > - Migrate back to a CPU page and write protect it > > - Copy to a new device exclusive page > > IMHO the ownership question would really help us to answer this one.. I'm confused about what device ownership you are talking about It is just a page and it is tied to some specific pgmap? If the thing providing the migration stuff goes away then all device_exclusive pages should revert back to CPU pages and destroy the pgmap? Jason
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #27 from James Zhu (jam...@amd.com) --- Hi Jeromec, thanks for your feedback, can you also add drm.debug=0x1ff modprobe? I need log: case 1 dmesg and /sys/kernel/debug/dri/0/amdgpu_fence_info (if you can). James. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 2:13 p.m., Christian König wrote:
Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
On 2021-05-18 2:02 p.m., Christian König wrote:
Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
On 2021-05-18 12:33 p.m., Christian König wrote:
Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over
the dma_resv requirements and I came to the conclusion that
this approach here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and
exports it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the
rendering, for the the fence of device A is replaced with a new
operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run
into the problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the
uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Exactly that, yes.
In other words when you have a dependency chain like A->B->C then
memory management only waits for C before freeing up the memory for
example.
When C now signaled because the device is hot-plugged before A or B
are finished they are essentially accessing freed up memory.
But didn't C imported the BO form B or A in this case ? Why would he be
the one releasing that memory ? He would be just dropping his reference
to the BO, no ?
Well freeing the memory was just an example. The BO could also move
back to VRAM because of the dropped reference.
Also in the general case,
drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
the one who signals the 'C' fence with error code are as far
as I looked called from when the user of that BO is stopping
the usage anyway (e.g. drm_driver.postclose callback for when use
process closes his device file) who would then access and corrupt
the exported memory on device A where the job hasn't completed yet ?
Key point is that memory management only waits for the last added
fence, that is the design of the dma_resv object. How that happens is
irrelevant.
Because of this we at least need to wait for all dependencies of the
job before signaling the fence, even if we cancel the job for some
reason.
Christian.
Would this be the right way to do it ?
Yes, it is at least a start. Question is if we can wait blocking here or
not.
We install a callback a bit lower to avoid blocking, so I'm pretty sure
that won't work as expected.
Christian.
I can't see why this would create problems, as long as the dependencies
complete or force competed if they are from same device (extracted) but
on a different ring then looks to me it should work. I will give it
a try.
Andrey
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 2e93e881b65f..10f784874b63 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct
drm_sched_entity *entity)
{
struct drm_sched_job *job;
int r;
+ struct dma_fence *f;
while ((job =
to_drm_sched_job(spsc_queue_pop(>job_queue {
struct drm_sched_fence *s_fence = job->s_fence;
+ while (f = sched->ops->dependency(sched_job, entity))
+ dma_fence_wait(f);
+
drm_sched_fence_scheduled(s_fence);
dma_fence_set_error(_fence->finished, -ESRCH);
Andrey
Andrey
Christian.
Andrey
I am not sure this problem you describe above is related to this
patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need
to cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly
at the moment, we only wait for the last scheduled fence but not
for the dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences
for the
extracted device B, we are not touching any other fences. In
fact as you
may remember, I dropped all new logic to
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #26 from Jerome C (m...@jeromec.com) --- (In reply to Jerome C from comment #25) > I forgot to mention... I'm on kernel 5.13.4 5.12.4 I mean -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH] dt-bindings: display: convert faraday,tve200 to YAML
Le Mon, May 17, 2021 at 07:26:24PM -0500, Rob Herring a écrit :
> On Tue, May 11, 2021 at 04:54:48PM +, Corentin Labbe wrote:
> > Converts display/faraday,tve200.txt to yaml.
> >
> > Signed-off-by: Corentin Labbe
> > ---
> > .../bindings/display/faraday,tve200.txt | 54 ---
> > .../bindings/display/faraday,tve200.yaml | 92 +++
> > 2 files changed, 92 insertions(+), 54 deletions(-)
> > delete mode 100644
> > Documentation/devicetree/bindings/display/faraday,tve200.txt
> > create mode 100644
> > Documentation/devicetree/bindings/display/faraday,tve200.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/display/faraday,tve200.txt
> > b/Documentation/devicetree/bindings/display/faraday,tve200.txt
> > deleted file mode 100644
> > index 82e3bc0b7485..
> > --- a/Documentation/devicetree/bindings/display/faraday,tve200.txt
> > +++ /dev/null
> > @@ -1,54 +0,0 @@
> > -* Faraday TV Encoder TVE200
> > -
> > -Required properties:
> > -
> > -- compatible: must be one of:
> > - "faraday,tve200"
> > - "cortina,gemini-tvc", "faraday,tve200"
> > -
> > -- reg: base address and size of the control registers block
> > -
> > -- interrupts: contains an interrupt specifier for the interrupt
> > - line from the TVE200
> > -
> > -- clock-names: should contain "PCLK" for the clock line clocking the
> > - silicon and "TVE" for the 27MHz clock to the video driver
> > -
> > -- clocks: contains phandle and clock specifier pairs for the entries
> > - in the clock-names property. See
> > - Documentation/devicetree/bindings/clock/clock-bindings.txt
> > -
> > -Optional properties:
> > -
> > -- resets: contains the reset line phandle for the block
> > -
> > -Required sub-nodes:
> > -
> > -- port: describes LCD panel signals, following the common binding
> > - for video transmitter interfaces; see
> > - Documentation/devicetree/bindings/media/video-interfaces.txt
> > - This port should have the properties:
> > - reg = <0>;
> > - It should have one endpoint connected to a remote endpoint where
> > - the display is connected.
> > -
> > -Example:
> > -
> > -display-controller@6a00 {
> > - #address-cells = <1>;
> > - #size-cells = <0>;
> > - compatible = "faraday,tve200";
> > - reg = <0x6a00 0x1000>;
> > - interrupts = <13 IRQ_TYPE_EDGE_RISING>;
> > - resets = < GEMINI_RESET_TVC>;
> > - clocks = < GEMINI_CLK_GATE_TVC>,
> > -< GEMINI_CLK_TVC>;
> > - clock-names = "PCLK", "TVE";
> > -
> > - port@0 {
> > - reg = <0>;
> > - display_out: endpoint {
> > - remote-endpoint = <_in>;
> > - };
> > - };
> > -};
> > diff --git a/Documentation/devicetree/bindings/display/faraday,tve200.yaml
> > b/Documentation/devicetree/bindings/display/faraday,tve200.yaml
> > new file mode 100644
> > index ..3ab51e7e72af
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/faraday,tve200.yaml
> > @@ -0,0 +1,92 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/faraday,tve200.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Faraday TV Encoder TVE200
> > +
> > +maintainers:
> > + - Linus Walleij
> > +
> > +properties:
> > + compatible:
> > +oneOf:
> > + - const: faraday,tve200
> > + - items:
> > + - const: cortina,gemini-tvc
> > + - const: faraday,tve200
> > +
> > + reg:
> > +minItems: 1
>
> maxItems: 1
>
> They evaluate the same, but maxItems seems a bit more logical.
>
> > +
> > + interrupts:
> > +minItems: 1
> > +
> > + clock-names:
> > +items:
> > + - const: PCLK
> > + - const: TVE
> > +
> > + clocks:
> > +minItems: 2
> > +
> > + resets:
> > +minItems: 1
> > +
> > + "#address-cells":
> > +const: 1
> > +
> > + "#size-cells":
> > +const: 0
> > +
> > +patternProperties:
> > + "^port@[0-9]+$":
>
> Should be just 'port' or 'port@0', but really the former is preferred
> when only 1.
>
> Use the graph binding:
>
> $ref: /schemas/graph.yaml#/properties/port
>
I have a problem:
I get the following warning:
/usr/src/linux-next/arch/arm/boot/dts/gemini.dtsi:410.31-423.5: Warning
(graph_child_address): /soc/display-controller@6a00: graph node has single
child node 'port@0', #address-cells/#size-cells are not necessary
also defined at
/usr/src/linux-next/arch/arm/boot/dts/gemini-dlink-dir-685.dts:492.31-501.5
But if I remove them!
/usr/src/linux-next/arch/arm/boot/dts/gemini-dlink-dir-685.dts:496.5-15:
Warning (reg_format): /soc/display-controller@6a00/port@0:reg: property has
invalid length (4 bytes) (#address-cells == 2, #size-cells == 1)
arch/arm/boot/dts/gemini-dlink-dir-685.dt.yaml: Warning (pci_device_reg):
Failed prerequisite 'reg_format'
arch/arm/boot/dts/gemini-dlink-dir-685.dt.yaml: Warning (pci_device_bus_num):
Failed prerequisite
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #25 from Jerome C (m...@jeromec.com) --- I forgot to mention... I'm on kernel 5.13.4 -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #24 from Jerome C (m...@jeromec.com) --- (In reply to James Zhu from comment #21) > Hi Jeromec, to isolate the cause, can you help run two experiments > separately? > 1. To run suspend/resume without launching Xorg, just on text mode. > 2. To disable video acceleration (VCN IP). I need you share me the whole > dmesg log after loading amdgpu driver. I think basically running modprobe > with ip_block_mask=0x0ff should disable vcn ip for VCN1.(you can find words > in dmesg to tell you if vcn ip is disabled or not). > > Thanks! > James 1) In text mode, VCN enabled, suspensions issues are still there 2) I see the message confirming that VCN is disabled, In text mode, VCN disabled, suspensions issues are gone, After starting Xorg, VCN disabled, suspensions issues are gone I'll gather the logs those soon ( tomorrow sometime ) -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
Am 18.05.21 um 20:09 schrieb Andrey Grodzovsky:
On 2021-05-18 2:02 p.m., Christian König wrote:
Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
On 2021-05-18 12:33 p.m., Christian König wrote:
Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over
the dma_resv requirements and I came to the conclusion that
this approach here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and
exports it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the
rendering, for the the fence of device A is replaced with a new
operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run
into the problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the
uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Exactly that, yes.
In other words when you have a dependency chain like A->B->C then
memory management only waits for C before freeing up the memory for
example.
When C now signaled because the device is hot-plugged before A or B
are finished they are essentially accessing freed up memory.
But didn't C imported the BO form B or A in this case ? Why would he be
the one releasing that memory ? He would be just dropping his reference
to the BO, no ?
Well freeing the memory was just an example. The BO could also move
back to VRAM because of the dropped reference.
Also in the general case,
drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
the one who signals the 'C' fence with error code are as far
as I looked called from when the user of that BO is stopping
the usage anyway (e.g. drm_driver.postclose callback for when use
process closes his device file) who would then access and corrupt
the exported memory on device A where the job hasn't completed yet ?
Key point is that memory management only waits for the last added
fence, that is the design of the dma_resv object. How that happens is
irrelevant.
Because of this we at least need to wait for all dependencies of the
job before signaling the fence, even if we cancel the job for some
reason.
Christian.
Would this be the right way to do it ?
Yes, it is at least a start. Question is if we can wait blocking here or
not.
We install a callback a bit lower to avoid blocking, so I'm pretty sure
that won't work as expected.
Christian.
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 2e93e881b65f..10f784874b63 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct
drm_sched_entity *entity)
{
struct drm_sched_job *job;
int r;
+ struct dma_fence *f;
while ((job =
to_drm_sched_job(spsc_queue_pop(>job_queue {
struct drm_sched_fence *s_fence = job->s_fence;
+ while (f = sched->ops->dependency(sched_job, entity))
+ dma_fence_wait(f);
+
drm_sched_fence_scheduled(s_fence);
dma_fence_set_error(_fence->finished, -ESRCH);
Andrey
Andrey
Christian.
Andrey
I am not sure this problem you describe above is related to this
patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need
to cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly
at the moment, we only wait for the last scheduled fence but not
for the dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences
for the
extracted device B, we are not touching any other fences. In
fact as you
may remember, I dropped all new logic to forcing fence
completion in
this patch series and only call
amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today
anyway.
Signaling hardware fences is unproblematic since they are emitted
when the software scheduling is already
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 19:32 schrieb Thomas Hellström: [SNIP] PTE handling is the domain of TTM, drivers should never mess with that directly. Hmm. May I humbly suggest a different view on this: I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, TTM has no business whatsoever with user-space PTEs. That's really why that bo type exists in the first place. But otoh one can of course argue that then i915 has no business calling the TTM fault helper for these bos. Well the real question is why does i915 wants to expose kernel BOs to userspace? As the name says only the kernel should be able to access them. I'd say "kernel" of course refers to how TTM handles them. Fair enough. So for discrete we can probably do the right thing with ttm_bo_type_device. What worries me a bit is when we get to older hardware support because whatever we do is by definition going to be ugly. At best we might be able to split the address space between i915's mmos, and hand the rest to TTM, modifying offsets as you suggest. That way a TTM call to unmap_mapping_range() would do the right thing, I think. Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, overlayfs etc. So adjusting the offset inside the kernel is already well supported and used. What I don't fully understand is your use case here? Can you briefly describe that? Do you use different bits of the offset to signal what caching behavior you have? And then based on this adjust the pgprot_t in the vma? Thanks, Christian. TBH I'm not completely sure about the history of this. I think the basic idea is that you want to support different vmas with different caching modes, so you never change the vma pgprot after mmap time, like TTM does. Needless to say this only works with Intel processors on integrated and the more I write about this the more I'm convinced that we should perhaps not concern TTM with that at all. Yeah, that might be a possibility. But KFD does something very similar IIRC. They stuff the routing information into the upper bits of the offset and adjust it then to match what TTM wants in the mmap() callback. Adjusting the caching behavior on the fly is really tricky and I'm pretty sure you should not do that outside of the integrated Intel processor anyway :) Cheers, Christian. /Thomas /Thomas Christian. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 2:02 p.m., Christian König wrote:
Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky:
On 2021-05-18 12:33 p.m., Christian König wrote:
Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over
the dma_resv requirements and I came to the conclusion that this
approach here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and
exports it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the
rendering, for the the fence of device A is replaced with a new
operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run into
the problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Exactly that, yes.
In other words when you have a dependency chain like A->B->C then
memory management only waits for C before freeing up the memory for
example.
When C now signaled because the device is hot-plugged before A or B
are finished they are essentially accessing freed up memory.
But didn't C imported the BO form B or A in this case ? Why would he be
the one releasing that memory ? He would be just dropping his reference
to the BO, no ?
Well freeing the memory was just an example. The BO could also move back
to VRAM because of the dropped reference.
Also in the general case,
drm_sched_entity_fini->drm_sched_entity_kill_jobs which is
the one who signals the 'C' fence with error code are as far
as I looked called from when the user of that BO is stopping
the usage anyway (e.g. drm_driver.postclose callback for when use
process closes his device file) who would then access and corrupt
the exported memory on device A where the job hasn't completed yet ?
Key point is that memory management only waits for the last added fence,
that is the design of the dma_resv object. How that happens is irrelevant.
Because of this we at least need to wait for all dependencies of the job
before signaling the fence, even if we cancel the job for some reason.
Christian.
Would this be the right way to do it ?
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 2e93e881b65f..10f784874b63 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -223,10 +223,14 @@ static void drm_sched_entity_kill_jobs(struct
drm_sched_entity *entity)
{
struct drm_sched_job *job;
int r;
+ struct dma_fence *f;
while ((job =
to_drm_sched_job(spsc_queue_pop(>job_queue {
struct drm_sched_fence *s_fence = job->s_fence;
+ while (f = sched->ops->dependency(sched_job, entity))
+ dma_fence_wait(f);
+
drm_sched_fence_scheduled(s_fence);
dma_fence_set_error(_fence->finished, -ESRCH);
Andrey
Andrey
Christian.
Andrey
I am not sure this problem you describe above is related to this
patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need to
cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly at
the moment, we only wait for the last scheduled fence but not for
the dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences
for the
extracted device B, we are not touching any other fences. In fact
as you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.
Signaling hardware fences is unproblematic since they are emitted
when the software scheduling is already completed.
Christian.
Andrey
Not sure how to handle that case. One possibility would be to
wait for all dependencies of unscheduled jobs before signaling
their fences as canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem:
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #23 from James Zhu (jam...@amd.com) --- Hi kolAflash, VCN IP is for video acceleration(for video playback), if vcn ip didn't handle suspend/resume process properly, we do observe other IP blocks be affected. For your case it is display IP(dm) related. ip_block_mask=0xff (in grub should be amdgpu.ip_block_mask=0x0ff) can disable VCN IP during amdgpu driver loading. so this experiment can tell if this dm error is caused by VCN IP or not. sometimes /sys/kernel/debug/dri/0/amdgpu_fence_info can provide some useful information if it has chance to be dumped. these experiments can help identified which IP cause the issue. So we can find expert in that area to continue to triage. Your current report is case 2, so it can be replaced with 2. amd-drm-next-5.14-2021-05-12* with ip_block_mask=0x0ff, with Xorg and without 3D acceleration in daily use. I suggest you to execute your test plan in order 4->3->2->1. Thanks! James -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
Am 18.05.21 um 19:43 schrieb Andrey Grodzovsky: On 2021-05-18 12:33 p.m., Christian König wrote: Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky: On 2021-05-18 11:15 a.m., Christian König wrote: Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky: On 2021-05-18 10:07 a.m., Christian König wrote: In a separate discussion with Daniel we once more iterated over the dma_resv requirements and I came to the conclusion that this approach here won't work reliable. The problem is as following: 1. device A schedules some rendering with into a buffer and exports it as DMA-buf. 2. device B imports the DMA-buf and wants to consume the rendering, for the the fence of device A is replaced with a new operation. 3. device B is hot plugged and the new operation canceled/newer scheduled. The problem is now that we can't do this since the operation of device A is still running and by signaling our fences we run into the problem of potential memory corruption. By signaling s_fence->finished of the canceled operation from the removed device B we in fact cause memory corruption for the uncompleted job still running on device A ? Because there is someone waiting to read write from the imported buffer on device B and he only waits for the s_fence->finished of device B we signaled in drm_sched_entity_kill_jobs ? Exactly that, yes. In other words when you have a dependency chain like A->B->C then memory management only waits for C before freeing up the memory for example. When C now signaled because the device is hot-plugged before A or B are finished they are essentially accessing freed up memory. But didn't C imported the BO form B or A in this case ? Why would he be the one releasing that memory ? He would be just dropping his reference to the BO, no ? Well freeing the memory was just an example. The BO could also move back to VRAM because of the dropped reference. Also in the general case, drm_sched_entity_fini->drm_sched_entity_kill_jobs which is the one who signals the 'C' fence with error code are as far as I looked called from when the user of that BO is stopping the usage anyway (e.g. drm_driver.postclose callback for when use process closes his device file) who would then access and corrupt the exported memory on device A where the job hasn't completed yet ? Key point is that memory management only waits for the last added fence, that is the design of the dma_resv object. How that happens is irrelevant. Because of this we at least need to wait for all dependencies of the job before signaling the fence, even if we cancel the job for some reason. Christian. Andrey Christian. Andrey I am not sure this problem you describe above is related to this patch. Well it is kind of related. Here we purely expand the criteria for when sched_entity is considered idle in order to prevent a hang on device remove. And exactly that is problematic. See the jobs on the entity need to cleanly wait for their dependencies before they can be completed. drm_sched_entity_kill_jobs() is also not handling that correctly at the moment, we only wait for the last scheduled fence but not for the dependencies of the job. Were you addressing the patch from yesterday in which you commented that you found a problem with how we finish fences ? It was '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.' Also, in the patch series as it is now we only signal HW fences for the extracted device B, we are not touching any other fences. In fact as you may remember, I dropped all new logic to forcing fence completion in this patch series and only call amdgpu_fence_driver_force_completion for the HW fences of the extracted device as it's done today anyway. Signaling hardware fences is unproblematic since they are emitted when the software scheduling is already completed. Christian. Andrey Not sure how to handle that case. One possibility would be to wait for all dependencies of unscheduled jobs before signaling their fences as canceled. Christian. Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky: Problem: If scheduler is already stopped by the time sched_entity is released and entity's job_queue not empty I encountred a hang in drm_sched_entity_flush. This is because drm_sched_entity_is_idle never becomes false. Fix: In drm_sched_fini detach all sched_entities from the scheduler's run queues. This will satisfy drm_sched_entity_is_idle. Also wakeup all those processes stuck in sched_entity flushing as the scheduler main thread which wakes them up is stopped by now. v2: Reverse order of drm_sched_rq_remove_entity and marking s_entity as stopped to prevent reinserion back to rq due to race. v3: Drop drm_sched_rq_remove_entity, only modify entity->stopped and check for it in drm_sched_entity_is_idle Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/scheduler/sched_entity.c | 3 ++- drivers/gpu/drm/scheduler/sched_main.c | 24
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 02:33:34PM -0300, Jason Gunthorpe wrote: > On Tue, May 18, 2021 at 01:27:42PM -0400, Peter Xu wrote: > > > I also have a pure and high level question regarding a process fork() when > > there're device exclusive ptes: would the two processes then own the device > > together? Is this a real usecase? > > If the pages are MAP_SHARED then yes. All VMAs should point at the > same device_exclusive page and all VMA should migrate back to CPU > pages together. Makes sense. If we keep the anonymous-only in this series (I think it's good to separate these), maybe we can drop the !COW case, plus some proper installed WARN_ON_ONCE()s. > > > Indeed it'll be odd for a COW page since for COW page then it means after > > parent/child writting to the page it'll clone into two, then it's a mistery > > on > > which one will be the one that "exclusived owned" by the device.. > > For COW pages it is like every other fork case.. We can't reliably > write-protect the device_exclusive page during fork so we must copy it > at fork time. > > Thus three reasonable choices: > - Copy to a new CPU page > - Migrate back to a CPU page and write protect it > - Copy to a new device exclusive page IMHO the ownership question would really help us to answer this one.. If the device ownership should be kept in parent IMHO option (1) is the best approach. To be explicit on page copy: we can do best-effort, even if the copy is during a device atomic operation, perhaps? If the ownership will be shared, seems option (3) will be easier as I don't see a strong reason to do immediate restorinng of ptes; as long as we're careful on the refcounting. Thanks, -- Peter Xu
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 12:33 p.m., Christian König wrote: Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky: On 2021-05-18 11:15 a.m., Christian König wrote: Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky: On 2021-05-18 10:07 a.m., Christian König wrote: In a separate discussion with Daniel we once more iterated over the dma_resv requirements and I came to the conclusion that this approach here won't work reliable. The problem is as following: 1. device A schedules some rendering with into a buffer and exports it as DMA-buf. 2. device B imports the DMA-buf and wants to consume the rendering, for the the fence of device A is replaced with a new operation. 3. device B is hot plugged and the new operation canceled/newer scheduled. The problem is now that we can't do this since the operation of device A is still running and by signaling our fences we run into the problem of potential memory corruption. By signaling s_fence->finished of the canceled operation from the removed device B we in fact cause memory corruption for the uncompleted job still running on device A ? Because there is someone waiting to read write from the imported buffer on device B and he only waits for the s_fence->finished of device B we signaled in drm_sched_entity_kill_jobs ? Exactly that, yes. In other words when you have a dependency chain like A->B->C then memory management only waits for C before freeing up the memory for example. When C now signaled because the device is hot-plugged before A or B are finished they are essentially accessing freed up memory. But didn't C imported the BO form B or A in this case ? Why would he be the one releasing that memory ? He would be just dropping his reference to the BO, no ? Also in the general case, drm_sched_entity_fini->drm_sched_entity_kill_jobs which is the one who signals the 'C' fence with error code are as far as I looked called from when the user of that BO is stopping the usage anyway (e.g. drm_driver.postclose callback for when use process closes his device file) who would then access and corrupt the exported memory on device A where the job hasn't completed yet ? Andrey Christian. Andrey I am not sure this problem you describe above is related to this patch. Well it is kind of related. Here we purely expand the criteria for when sched_entity is considered idle in order to prevent a hang on device remove. And exactly that is problematic. See the jobs on the entity need to cleanly wait for their dependencies before they can be completed. drm_sched_entity_kill_jobs() is also not handling that correctly at the moment, we only wait for the last scheduled fence but not for the dependencies of the job. Were you addressing the patch from yesterday in which you commented that you found a problem with how we finish fences ? It was '[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.' Also, in the patch series as it is now we only signal HW fences for the extracted device B, we are not touching any other fences. In fact as you may remember, I dropped all new logic to forcing fence completion in this patch series and only call amdgpu_fence_driver_force_completion for the HW fences of the extracted device as it's done today anyway. Signaling hardware fences is unproblematic since they are emitted when the software scheduling is already completed. Christian. Andrey Not sure how to handle that case. One possibility would be to wait for all dependencies of unscheduled jobs before signaling their fences as canceled. Christian. Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky: Problem: If scheduler is already stopped by the time sched_entity is released and entity's job_queue not empty I encountred a hang in drm_sched_entity_flush. This is because drm_sched_entity_is_idle never becomes false. Fix: In drm_sched_fini detach all sched_entities from the scheduler's run queues. This will satisfy drm_sched_entity_is_idle. Also wakeup all those processes stuck in sched_entity flushing as the scheduler main thread which wakes them up is stopped by now. v2: Reverse order of drm_sched_rq_remove_entity and marking s_entity as stopped to prevent reinserion back to rq due to race. v3: Drop drm_sched_rq_remove_entity, only modify entity->stopped and check for it in drm_sched_entity_is_idle Signed-off-by: Andrey Grodzovsky Reviewed-by: Christian König --- drivers/gpu/drm/scheduler/sched_entity.c | 3 ++- drivers/gpu/drm/scheduler/sched_main.c | 24 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 0249c7450188..2e93e881b65f 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) rmb(); /* for list_empty to work without lock */ if (list_empty(>list) || -
Re: [RFC] Add DMA_RESV_USAGE flags
Am 18.05.21 um 18:48 schrieb Daniel Vetter: On Tue, May 18, 2021 at 2:49 PM Christian König wrote: Hi Jason & Daniel, Am 18.05.21 um 07:59 schrieb Daniel Vetter: On Tue, May 18, 2021 at 12:49 AM Jason Ekstrand wrote: On Mon, May 17, 2021 at 3:15 PM Daniel Vetter wrote: On Mon, May 17, 2021 at 9:38 PM Christian König wrote: Am 17.05.21 um 17:04 schrieb Daniel Vetter: On Mon, May 17, 2021 at 04:11:18PM +0200, Christian König wrote: We had a long outstanding problem in amdgpu that buffers exported to user drivers by DMA-buf serialize all command submissions using them. In other words we can't compose the buffer with different engines and then send it to another driver for display further processing. This was added to work around the fact that i915 didn't wanted to wait for shared fences in the dma_resv objects before displaying a buffer. Since this problem is now causing issues with Vulkan we need to find a better solution for that. The patch set here tries to do this by adding an usage flag to the shared fences noting when and how they should participate in implicit synchronization. So the way this is fixed in every other vulkan driver is that vulkan userspace sets flags in the CS ioctl when it wants to synchronize with implicit sync. This gets you mostly there. Last time I checked amdgpu isn't doing this, and yes that's broken. And exactly that is a really bad approach as far as I can see. The Vulkan stack on top simply doesn't know when to set this flag during CS. Adding Jason for the Vulkan side of things, because this isn't how I understand this works. But purely form a kernel pov your patches are sketchy for two reasons: - we reinstate the amdgpu special case of not setting exclusive fences - you only fix the single special case of i915 display, nothing else That's not how a cross driver interface works. And if you'd do this properly, you'd be back to all the same sync fun you've orignally had, with all the same fallout. I think I'm starting to see what Christian is trying to do here and I think there likely is a real genuine problem here. I'm not convinced this is 100% of a solution but there might be something real. Let me see if I can convince you or if I just make a hash of things. :-) The problem, once again, comes down to memory fencing vs. execution fencing and the way that we've unfortunately tied them together in the kernel. With the current architecture, the only way to get proper write-fence semantics for implicit sync is to take an exclusive fence on the buffer. This implies two things: 1. You have to implicitly wait on EVERY fence on the buffer before you can start your write-fenced operation 2. No one else can start ANY operation which accesses that buffer until you're done. Yes, exactly that. You absolutely nailed it. I unfortunately also have a 3rd use case: 3. Operations which shouldn't participate in any syncing, but only affect the memory management. This is basically our heavyweight TLB flush after unmapping the BO from somebodies page tables. Nobody should ever be concerned about it for any form of synchronization, but memory managment is not allowed to reuse or move the buffer before the operation is completed. Isn't that just another case of 2? Or I'm not getting it. The problem in this case is not starting a new CS, but synchronizing to the existing ones. See a heavy TLB flush is made completely out of sync. E.g. it doesn't want to wait for any previous operation. In other words imagine the following example: 1. Both process A and B have a BO mapped. 2. Process A is heavily using the BO and doing all kind of rendering. 3. Process B is unmapping the BO. Now that process B unmaps the BO needs to trigger page table updates and a heavy TLB flush, but since this can take really long we want to do it asynchronously on the hardware. With the current approach you basically can't do that because you can't note that a fence should not participate in synchronization at all. E.g. we can't add a fence which doesn't wait for the exclusive one as shared. Let's say that you have a buffer which is shared between two drivers A and B and let's say driver A has thrown a fence on it just to ensure that the BO doesn't get swapped out to disk until it's at a good stopping point. Then driver B comes along and wants to throw a write-fence on it. Suddenly, your memory fence from driver A causes driver B to have to stall waiting for a "good" time to throw in a fence. It sounds like this is the sort of scenario that Christian is running into. And, yes, with certain Vulkan drivers being a bit sloppy about exactly when they throw in write fences, I could see it being a real problem. Yes this is a potential problem, and on the i915 side we need to do some shuffling here most likely. Especially due to discrete, but the problem is pre-existing. tbh I forgot about the implications here until I pondered this again yesterday evening. But afaiui
Re: [PATCH v8 5/8] mm: Device exclusive memory access
On Tue, May 18, 2021 at 01:27:42PM -0400, Peter Xu wrote: > I also have a pure and high level question regarding a process fork() when > there're device exclusive ptes: would the two processes then own the device > together? Is this a real usecase? If the pages are MAP_SHARED then yes. All VMAs should point at the same device_exclusive page and all VMA should migrate back to CPU pages together. > Indeed it'll be odd for a COW page since for COW page then it means after > parent/child writting to the page it'll clone into two, then it's a mistery on > which one will be the one that "exclusived owned" by the device.. For COW pages it is like every other fork case.. We can't reliably write-protect the device_exclusive page during fork so we must copy it at fork time. Thus three reasonable choices: - Copy to a new CPU page - Migrate back to a CPU page and write protect it - Copy to a new device exclusive page Jason
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
On 5/18/21 7:17 PM, Christian König wrote: Am 18.05.21 um 19:10 schrieb Thomas Hellström: On 5/18/21 5:29 PM, Christian König wrote: Am 18.05.21 um 17:25 schrieb Thomas Hellström: On 5/18/21 5:17 PM, Christian König wrote: Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? Will have to take a look. That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. You mean the unmap_mapping_range() stuff? That's not a problem since it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() takes care of killing the ptes. That design is a certain NAK from my side for upstreaming this. PTE handling is the domain of TTM, drivers should never mess with that directly. Hmm. May I humbly suggest a different view on this: I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, TTM has no business whatsoever with user-space PTEs. That's really why that bo type exists in the first place. But otoh one can of course argue that then i915 has no business calling the TTM fault helper for these bos. Well the real question is why does i915 wants to expose kernel BOs to userspace? As the name says only the kernel should be able to access them. I'd say "kernel" of course refers to how TTM handles them. So for discrete we can probably do the right thing with ttm_bo_type_device. What worries me a bit is when we get to older hardware support because whatever we do is by definition going to be ugly. At best we might be able to split the address space between i915's mmos, and hand the rest to TTM, modifying offsets as you suggest. That way a TTM call to unmap_mapping_range() would do the right thing, I think. Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, overlayfs etc. So adjusting the offset inside the kernel is already well supported and used. What I don't fully understand is your use case here? Can you briefly describe that? Do you use different bits of the offset to signal what caching behavior you have? And then based on this adjust the pgprot_t in the vma? Thanks, Christian. TBH I'm not completely sure about the history of this. I think the basic idea is that you want to support different vmas with different caching modes, so you never change the vma pgprot after mmap time, like TTM does. Needless to say this only works with Intel processors on integrated and the more I write about this the more I'm convinced that we should perhaps not concern TTM with that at all. /Thomas /Thomas Christian. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v8 5/8] mm: Device exclusive memory access
Alistair,
While I got one reply below to your previous email, I also looked at the rest
code (majorly restore and fork sides) today and added a few more comments.
On Tue, May 18, 2021 at 11:19:05PM +1000, Alistair Popple wrote:
[...]
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index 3a5705cfc891..556ff396f2e9 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -700,6 +700,84 @@ struct page *vm_normal_page_pmd(struct vm_area_struct
> > > *vma, unsigned long addr,>
> > > }
> > > #endif
> > >
> > > +static void restore_exclusive_pte(struct vm_area_struct *vma,
> > > + struct page *page, unsigned long address,
> > > + pte_t *ptep)
> > > +{
> > > + pte_t pte;
> > > + swp_entry_t entry;
> > > +
> > > + pte = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot)));
> > > + if (pte_swp_soft_dirty(*ptep))
> > > + pte = pte_mksoft_dirty(pte);
> > > +
> > > + entry = pte_to_swp_entry(*ptep);
> > > + if (pte_swp_uffd_wp(*ptep))
> > > + pte = pte_mkuffd_wp(pte);
> > > + else if (is_writable_device_exclusive_entry(entry))
> > > + pte = maybe_mkwrite(pte_mkdirty(pte), vma);
> > > +
> > > + set_pte_at(vma->vm_mm, address, ptep, pte);
> > > +
> > > + /*
> > > + * No need to take a page reference as one was already
> > > + * created when the swap entry was made.
> > > + */
> > > + if (PageAnon(page))
> > > + page_add_anon_rmap(page, vma, address, false);
> > > + else
> > > + page_add_file_rmap(page, false);
This seems to be another leftover; maybe WARN_ON_ONCE(!PageAnon(page))?
> > > +
> > > + if (vma->vm_flags & VM_LOCKED)
> > > + mlock_vma_page(page);
> > > +
> > > + /*
> > > + * No need to invalidate - it was non-present before. However
> > > + * secondary CPUs may have mappings that need invalidating.
> > > + */
> > > + update_mmu_cache(vma, address, ptep);
> > > +}
[...]
> > > /*
> > >
> > > * copy one vm_area from one task to the other. Assumes the page tables
> > > * already present in the new task to be cleared in the whole range
> > >
> > > @@ -781,6 +859,12 @@ copy_nonpresent_pte(struct mm_struct *dst_mm, struct
> > > mm_struct *src_mm,>
> > > pte = pte_swp_mkuffd_wp(pte);
> > >
> > > set_pte_at(src_mm, addr, src_pte, pte);
> > >
> > > }
> > >
> > > + } else if (is_device_exclusive_entry(entry)) {
> > > + /* COW mappings should be dealt with by removing the entry
> > > */
Here the comment says "removing the entry" however I think it didn't remove the
pte, instead it keeps it (as there's no "return", so set_pte_at() will be
called below), so I got a bit confused.
> > > + VM_BUG_ON(is_cow_mapping(vm_flags));
Also here, if PageAnon() is the only case to support so far, doesn't that
easily satisfy is_cow_mapping()? Maybe I missed something..
I also have a pure and high level question regarding a process fork() when
there're device exclusive ptes: would the two processes then own the device
together? Is this a real usecase?
Indeed it'll be odd for a COW page since for COW page then it means after
parent/child writting to the page it'll clone into two, then it's a mistery on
which one will be the one that "exclusived owned" by the device..
> > > + page = pfn_swap_entry_to_page(entry);
> > > + get_page(page);
> > > + rss[mm_counter(page)]++;
> > >
> > > }
> > > set_pte_at(dst_mm, addr, dst_pte, pte);
> > > return 0;
> > >
> > > @@ -947,6 +1031,7 @@ copy_pte_range(struct vm_area_struct *dst_vma, struct
> > > vm_area_struct *src_vma,>
> > > int rss[NR_MM_COUNTERS];
> > > swp_entry_t entry = (swp_entry_t){0};
> > > struct page *prealloc = NULL;
> > >
> > > + struct page *locked_page = NULL;
> > >
> > > again:
> > > progress = 0;
> > >
> > > @@ -980,13 +1065,36 @@ copy_pte_range(struct vm_area_struct *dst_vma,
> > > struct vm_area_struct *src_vma,>
> > > continue;
> > >
> > > }
> > > if (unlikely(!pte_present(*src_pte))) {
> > >
> > > - entry.val = copy_nonpresent_pte(dst_mm, src_mm,
> > > - dst_pte, src_pte,
> > > - src_vma, addr, rss);
> > > - if (entry.val)
> > > - break;
> > > - progress += 8;
> > > - continue;
> > > + swp_entry_t swp_entry = pte_to_swp_entry(*src_pte);
(Just a side note to all of us: this will be one more place that I'll need to
look after in my uffd-wp series if this series lands first, as after that
series we can only call
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #22 from kolAflash (kolafl...@kolahilft.de) --- @James What do you mean by video acceleration? Is this about 3D / DRI acceleration like in video games? Or do you mean just "video" playback (movie, mp4, webm, h264, vp8, ...) acceleration? And I don't completely understand what ip_block_mask=0x0ff is supposed to do. I just rebootet with that kernel parameter added and 3D acceleration (DRI) is still working. I'm planing to run these kernels in the next days: 1. Current Debian testing Linux-5.10.0-6 with ip_block_mask=0x0ff, Xorg and 3D acceleration in daily use. 2. amd-drm-next-5.14-2021-05-12* without ip_block_mask=0x0ff, with Xorg and with 3D acceleration in daily use. 3. amd-drm-next-5.14-2021-05-12* without ip_block_mask=0x0ff, with Xorg, but without 3D acceleration** in daily use. 4. amd-drm-next-5.14-2021-05-12* without ip_block_mask=0x0ff and without Xorg, doing some standby cycles for testing. If I encounter any crash I'll post the whole dmesg starting with the boot output. * amd-drm-next-5.14-2021-05-12 https://gitlab.freedesktop.org/agd5f/linux/-/tree/amd-drm-next-5.14-2021-05-12 ae30d41eb ** Is there something special I should do to turn off acceleration? Or should I just don't start any application doing 3D / DRI acceleration? (the latter one might be difficult - I got to keep an eye on every application like Firefox, Atom, VLC, KWin/KDE window manager, ... not to use DRI) -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 19:10 schrieb Thomas Hellström: On 5/18/21 5:29 PM, Christian König wrote: Am 18.05.21 um 17:25 schrieb Thomas Hellström: On 5/18/21 5:17 PM, Christian König wrote: Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? Will have to take a look. That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. You mean the unmap_mapping_range() stuff? That's not a problem since it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() takes care of killing the ptes. That design is a certain NAK from my side for upstreaming this. PTE handling is the domain of TTM, drivers should never mess with that directly. Hmm. May I humbly suggest a different view on this: I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, TTM has no business whatsoever with user-space PTEs. That's really why that bo type exists in the first place. But otoh one can of course argue that then i915 has no business calling the TTM fault helper for these bos. Well the real question is why does i915 wants to expose kernel BOs to userspace? As the name says only the kernel should be able to access them. So for discrete we can probably do the right thing with ttm_bo_type_device. What worries me a bit is when we get to older hardware support because whatever we do is by definition going to be ugly. At best we might be able to split the address space between i915's mmos, and hand the rest to TTM, modifying offsets as you suggest. That way a TTM call to unmap_mapping_range() would do the right thing, I think. Well we do all kind of nasty stuff with the offset in DMA-buf, KFD, overlayfs etc. So adjusting the offset inside the kernel is already well supported and used. What I don't fully understand is your use case here? Can you briefly describe that? Do you use different bits of the offset to signal what caching behavior you have? And then based on this adjust the pgprot_t in the vma? Thanks, Christian. /Thomas Christian. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
On 5/18/21 5:29 PM, Christian König wrote: Am 18.05.21 um 17:25 schrieb Thomas Hellström: On 5/18/21 5:17 PM, Christian König wrote: Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? Will have to take a look. That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. You mean the unmap_mapping_range() stuff? That's not a problem since it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() takes care of killing the ptes. That design is a certain NAK from my side for upstreaming this. PTE handling is the domain of TTM, drivers should never mess with that directly. Hmm. May I humbly suggest a different view on this: I agree fully for ttm_bo_type_device bos but for ttm_bo_type_kernel, TTM has no business whatsoever with user-space PTEs. That's really why that bo type exists in the first place. But otoh one can of course argue that then i915 has no business calling the TTM fault helper for these bos. So for discrete we can probably do the right thing with ttm_bo_type_device. What worries me a bit is when we get to older hardware support because whatever we do is by definition going to be ugly. At best we might be able to split the address space between i915's mmos, and hand the rest to TTM, modifying offsets as you suggest. That way a TTM call to unmap_mapping_range() would do the right thing, I think. /Thomas Christian. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 18:49 schrieb Maarten Lankhorst: Op 18-05-2021 om 17:07 schreef Christian König: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. i915 has its own mmap allocation handling to handle caching modes, and wouldn't use TTM's vma handling. But we do want to use the ttm fault handlers, which only need mmap_base + bo to work correctly. Yeah, but that is really a no-go. The VMA handling is a very essential part of TTM. If you want to use TTM we have to find a cleaner way to implement this. Christian. ~Maarten
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Op 18-05-2021 om 17:07 schreef Christian König: > Am 18.05.21 um 16:55 schrieb Thomas Hellström: >> From: Maarten Lankhorst >> >> This allows other drivers that may not setup the vma in the same way >> to use the ttm bo helpers. > > Uff can you please explain why exactly you need that? > > Providing the BO is not much of a problem, but having the BO at different VMA > offsets is really a no-go with TTM. i915 has its own mmap allocation handling to handle caching modes, and wouldn't use TTM's vma handling. But we do want to use the ttm fault handlers, which only need mmap_base + bo to work correctly. ~Maarten
Re: [RFC] Add DMA_RESV_USAGE flags
On Tue, May 18, 2021 at 2:49 PM Christian König wrote: > > Hi Jason & Daniel, > > Am 18.05.21 um 07:59 schrieb Daniel Vetter: > > On Tue, May 18, 2021 at 12:49 AM Jason Ekstrand > > wrote: > >> On Mon, May 17, 2021 at 3:15 PM Daniel Vetter wrote: > >>> On Mon, May 17, 2021 at 9:38 PM Christian König > >>> wrote: > Am 17.05.21 um 17:04 schrieb Daniel Vetter: > > On Mon, May 17, 2021 at 04:11:18PM +0200, Christian König wrote: > >> We had a long outstanding problem in amdgpu that buffers exported to > >> user drivers by DMA-buf serialize all command submissions using them. > >> > >> In other words we can't compose the buffer with different engines and > >> then send it to another driver for display further processing. > >> > >> This was added to work around the fact that i915 didn't wanted to wait > >> for shared fences in the dma_resv objects before displaying a buffer. > >> > >> Since this problem is now causing issues with Vulkan we need to find a > >> better solution for that. > >> > >> The patch set here tries to do this by adding an usage flag to the > >> shared fences noting when and how they should participate in implicit > >> synchronization. > > So the way this is fixed in every other vulkan driver is that vulkan > > userspace sets flags in the CS ioctl when it wants to synchronize with > > implicit sync. This gets you mostly there. Last time I checked amdgpu > > isn't doing this, and yes that's broken. > And exactly that is a really bad approach as far as I can see. The > Vulkan stack on top simply doesn't know when to set this flag during CS. > >>> Adding Jason for the Vulkan side of things, because this isn't how I > >>> understand this works. > >>> > >>> But purely form a kernel pov your patches are sketchy for two reasons: > >>> > >>> - we reinstate the amdgpu special case of not setting exclusive fences > >>> > >>> - you only fix the single special case of i915 display, nothing else > >>> > >>> That's not how a cross driver interface works. And if you'd do this > >>> properly, you'd be back to all the same sync fun you've orignally had, > >>> with all the same fallout. > >> I think I'm starting to see what Christian is trying to do here and I > >> think there likely is a real genuine problem here. I'm not convinced > >> this is 100% of a solution but there might be something real. Let me > >> see if I can convince you or if I just make a hash of things. :-) > >> > >> The problem, once again, comes down to memory fencing vs. execution > >> fencing and the way that we've unfortunately tied them together in the > >> kernel. With the current architecture, the only way to get proper > >> write-fence semantics for implicit sync is to take an exclusive fence > >> on the buffer. This implies two things: > >> > >> 1. You have to implicitly wait on EVERY fence on the buffer before > >> you can start your write-fenced operation > >> > >> 2. No one else can start ANY operation which accesses that buffer > >> until you're done. > > Yes, exactly that. You absolutely nailed it. > > I unfortunately also have a 3rd use case: > > 3. Operations which shouldn't participate in any syncing, but only > affect the memory management. > > This is basically our heavyweight TLB flush after unmapping the BO from > somebodies page tables. Nobody should ever be concerned about it for any > form of synchronization, but memory managment is not allowed to reuse or > move the buffer before the operation is completed. Isn't that just another case of 2? Or I'm not getting it. > >> Let's say that you have a buffer which is shared between two drivers A > >> and B and let's say driver A has thrown a fence on it just to ensure > >> that the BO doesn't get swapped out to disk until it's at a good > >> stopping point. Then driver B comes along and wants to throw a > >> write-fence on it. Suddenly, your memory fence from driver A causes > >> driver B to have to stall waiting for a "good" time to throw in a > >> fence. It sounds like this is the sort of scenario that Christian is > >> running into. And, yes, with certain Vulkan drivers being a bit > >> sloppy about exactly when they throw in write fences, I could see it > >> being a real problem. > > Yes this is a potential problem, and on the i915 side we need to do > > some shuffling here most likely. Especially due to discrete, but the > > problem is pre-existing. tbh I forgot about the implications here > > until I pondered this again yesterday evening. > > > > But afaiui the amdgpu code and winsys in mesa, this isn't (yet) the > > problem amd vk drivers have. The issue is that with amdgpu, all you > > supply are the following bits at CS time: > > - list of always mapped private buffers, which is implicit and O(1) in > > the kernel fastpath > > - additional list of shared buffers that are used by the current CS > > > > I didn't check how exactly that works wrt winsys
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #21 from James Zhu (jam...@amd.com) --- Hi Jeromec, to isolate the cause, can you help run two experiments separately? 1. To run suspend/resume without launching Xorg, just on text mode. 2. To disable video acceleration (VCN IP). I need you share me the whole dmesg log after loading amdgpu driver. I think basically running modprobe with ip_block_mask=0x0ff should disable vcn ip for VCN1.(you can find words in dmesg to tell you if vcn ip is disabled or not). Thanks! James -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
Am 18.05.21 um 18:17 schrieb Andrey Grodzovsky:
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over the
dma_resv requirements and I came to the conclusion that this
approach here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and exports
it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering,
for the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run into
the problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Exactly that, yes.
In other words when you have a dependency chain like A->B->C then memory
management only waits for C before freeing up the memory for example.
When C now signaled because the device is hot-plugged before A or B are
finished they are essentially accessing freed up memory.
Christian.
Andrey
I am not sure this problem you describe above is related to this patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need to
cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly at
the moment, we only wait for the last scheduled fence but not for the
dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences for the
extracted device B, we are not touching any other fences. In fact as
you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.
Signaling hardware fences is unproblematic since they are emitted
when the software scheduling is already completed.
Christian.
Andrey
Not sure how to handle that case. One possibility would be to wait
for all dependencies of unscheduled jobs before signaling their
fences as canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because
drm_sched_entity_is_idle
never becomes false.
Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.
v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.
v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
drivers/gpu/drm/scheduler/sched_main.c | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 0249c7450188..2e93e881b65f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
rmb(); /* for list_empty to work without lock */
if (list_empty(>list) ||
- spsc_queue_count(>job_queue) == 0)
+ spsc_queue_count(>job_queue) == 0 ||
+ entity->stopped)
return true;
return false;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 8d1211e87101..a2a953693b45 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
*/
void drm_sched_fini(struct drm_gpu_scheduler *sched)
{
+ struct drm_sched_entity *s_entity;
+ int i;
+
if (sched->thread)
kthread_stop(sched->thread);
+ for (i =
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
Am 18.05.21 um 18:07 schrieb Thomas Hellström: On 5/18/21 5:42 PM, Christian König wrote: Am 18.05.21 um 17:38 schrieb Thomas Hellström: On 5/18/21 5:28 PM, Christian König wrote: Am 18.05.21 um 17:20 schrieb Thomas Hellström: On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. That sounds like a design bug to me since you should never need to do this. When you want to destroy the backing store of a buffer during eviction you should just do this by returning an empty placement from the evict_flags callback. So this is for the functionality where the user has indicated that the contents is no longer of value, but the buffer itself is cached until evicted or swapped out for performance reasons. So the above would work for eviction, but what about swapout. Could we add some similar functionality there? Amdgpu has the same functionality and you don't need to handle swap at all. Just return from the evict_flags that you want to drop the backing store as soon as the BO leaves the GTT domain. Hmm, the pipeline_gutting function seems ok, but overly complex if the bo is already idle, Am I allowed to optimize it slightly for the latter case? Yeah, sure. We just never hat that use case so far. Christian. /Thomas Christian. /Thomas Regards, Christian. /Thomas Thanks, Thomas
Re: [PATCH] drm/i915/gvt: remove local storage of debugfs file
On Tue, May 18, 2021 at 06:17:05PM +0200, Greg Kroah-Hartman wrote: > There is no need to keep the dentry around for the debugfs kvmgt cache > file, as we can just look it up when we want to remove it later on. > Simplify the structure by removing the dentry and relying on debugfs > to find the dentry to remove when we want to. > > By doing this change, we remove the last in-kernel user that was storing > the result of debugfs_create_long(), so that api can be cleaned up. > > Cc: Zhenyu Wang > Cc: Zhi Wang > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: David Airlie > Cc: Daniel Vetter > Cc: intel-gvt-...@lists.freedesktop.org > Cc: intel-...@lists.freedesktop.org > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Greg Kroah-Hartman > --- > drivers/gpu/drm/i915/gvt/kvmgt.c | 11 +-- > 1 file changed, 5 insertions(+), 6 deletions(-) Note, I can take this through my debugfs tree if wanted, that way I can clean up the debugfs_create_long() api at the same time. Otherwise it's fine, I can wait until next -rc1 for that to happen. thanks, greg k-h
[PATCH] drm/msm: Init mm_list before accessing it for use_vram path
Fix NULL pointer dereference caused by update_inactive() trying to list_del() an uninitialized mm_list who's prev/next pointers are NULL. Signed-off-by: Alexey Minnekhanov --- drivers/gpu/drm/msm/msm_gem.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index b199942266a26..b8c873fc63a78 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -1227,6 +1227,13 @@ static struct drm_gem_object *_msm_gem_new(struct drm_device *dev, to_msm_bo(obj)->vram_node = >node; + /* Call chain get_pages() -> update_inactive() tries to +* access msm_obj->mm_list, but it is not initialized yet. +* To avoid NULL pointer dereference error, initialize +* mm_list to be empty. +*/ + INIT_LIST_HEAD(_obj->mm_list); + msm_gem_lock(obj); pages = get_pages(obj); msm_gem_unlock(obj); -- 2.26.3
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 11:15 a.m., Christian König wrote:
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over the
dma_resv requirements and I came to the conclusion that this approach
here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and exports
it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering,
for the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run into the
problem of potential memory corruption.
By signaling s_fence->finished of the canceled operation from the
removed device B we in fact cause memory corruption for the uncompleted
job still running on device A ? Because there is someone waiting to
read write from the imported buffer on device B and he only waits for
the s_fence->finished of device B we signaled
in drm_sched_entity_kill_jobs ?
Andrey
I am not sure this problem you describe above is related to this patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need to
cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly at the
moment, we only wait for the last scheduled fence but not for the
dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences for the
extracted device B, we are not touching any other fences. In fact as you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.
Signaling hardware fences is unproblematic since they are emitted when
the software scheduling is already completed.
Christian.
Andrey
Not sure how to handle that case. One possibility would be to wait
for all dependencies of unscheduled jobs before signaling their
fences as canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because
drm_sched_entity_is_idle
never becomes false.
Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.
v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.
v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
drivers/gpu/drm/scheduler/sched_main.c | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 0249c7450188..2e93e881b65f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
rmb(); /* for list_empty to work without lock */
if (list_empty(>list) ||
- spsc_queue_count(>job_queue) == 0)
+ spsc_queue_count(>job_queue) == 0 ||
+ entity->stopped)
return true;
return false;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 8d1211e87101..a2a953693b45 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
*/
void drm_sched_fini(struct drm_gpu_scheduler *sched)
{
+ struct drm_sched_entity *s_entity;
+ int i;
+
if (sched->thread)
kthread_stop(sched->thread);
+ for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >=
DRM_SCHED_PRIORITY_MIN; i--) {
+ struct drm_sched_rq *rq = >sched_rq[i];
+
+ if (!rq)
+ continue;
+
+ spin_lock(>lock);
+ list_for_each_entry(s_entity, >entities, list)
+ /*
+ * Prevents reinsertion and marks job_queue as idle,
+ * it will removed from
[PATCH] drm/i915/gvt: remove local storage of debugfs file
There is no need to keep the dentry around for the debugfs kvmgt cache
file, as we can just look it up when we want to remove it later on.
Simplify the structure by removing the dentry and relying on debugfs
to find the dentry to remove when we want to.
By doing this change, we remove the last in-kernel user that was storing
the result of debugfs_create_long(), so that api can be cleaned up.
Cc: Zhenyu Wang
Cc: Zhi Wang
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: David Airlie
Cc: Daniel Vetter
Cc: intel-gvt-...@lists.freedesktop.org
Cc: intel-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Greg Kroah-Hartman
---
drivers/gpu/drm/i915/gvt/kvmgt.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 65ff43cfc0f7..433c2a448f2d 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -88,6 +88,7 @@ struct kvmgt_pgfn {
struct hlist_node hnode;
};
+#define KVMGT_DEBUGFS_FILENAME "kvmgt_nr_cache_entries"
struct kvmgt_guest_info {
struct kvm *kvm;
struct intel_vgpu *vgpu;
@@ -95,7 +96,6 @@ struct kvmgt_guest_info {
#define NR_BKT (1 << 18)
struct hlist_head ptable[NR_BKT];
#undef NR_BKT
- struct dentry *debugfs_cache_entries;
};
struct gvt_dma {
@@ -1843,16 +1843,15 @@ static int kvmgt_guest_init(struct mdev_device *mdev)
info->track_node.track_flush_slot = kvmgt_page_track_flush_slot;
kvm_page_track_register_notifier(kvm, >track_node);
- info->debugfs_cache_entries = debugfs_create_ulong(
- "kvmgt_nr_cache_entries",
- 0444, vgpu->debugfs,
- >nr_cache_entries);
+ debugfs_create_ulong(KVMGT_DEBUGFS_FILENAME, 0444, vgpu->debugfs,
+>nr_cache_entries);
return 0;
}
static bool kvmgt_guest_exit(struct kvmgt_guest_info *info)
{
- debugfs_remove(info->debugfs_cache_entries);
+ debugfs_remove(debugfs_lookup(KVMGT_DEBUGFS_FILENAME,
+ info->vgpu->debugfs));
kvm_page_track_unregister_notifier(info->kvm, >track_node);
kvm_put_kvm(info->kvm);
--
2.31.1
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 --- Comment #20 from Jerome C (m...@jeromec.com) --- (In reply to James Zhu from comment #19) > Created attachment 296841 [details] > to fix suspend/resume hung issue > > Hi @kolAflash and @jeromec, Can you help check if this patch can fix the > issue? Since we can't reproduce at our side. Thanks! James no, this doesn't work for me. I'm curious to how your exactly to reproducing this I start Xorg using the command "startx" Xorg is running with LXQT I start "Konsole" a gui terminal and execute the following "for i in $(seq 1 150); do echo $i; sudo rtcwake -s 7 -m mem; done" -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
On 5/18/21 5:42 PM, Christian König wrote: Am 18.05.21 um 17:38 schrieb Thomas Hellström: On 5/18/21 5:28 PM, Christian König wrote: Am 18.05.21 um 17:20 schrieb Thomas Hellström: On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. That sounds like a design bug to me since you should never need to do this. When you want to destroy the backing store of a buffer during eviction you should just do this by returning an empty placement from the evict_flags callback. So this is for the functionality where the user has indicated that the contents is no longer of value, but the buffer itself is cached until evicted or swapped out for performance reasons. So the above would work for eviction, but what about swapout. Could we add some similar functionality there? Amdgpu has the same functionality and you don't need to handle swap at all. Just return from the evict_flags that you want to drop the backing store as soon as the BO leaves the GTT domain. Hmm, the pipeline_gutting function seems ok, but overly complex if the bo is already idle, Am I allowed to optimize it slightly for the latter case? /Thomas Christian. /Thomas Regards, Christian. /Thomas Thanks, Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
Am 18.05.21 um 17:38 schrieb Thomas Hellström: On 5/18/21 5:28 PM, Christian König wrote: Am 18.05.21 um 17:20 schrieb Thomas Hellström: On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. That sounds like a design bug to me since you should never need to do this. When you want to destroy the backing store of a buffer during eviction you should just do this by returning an empty placement from the evict_flags callback. So this is for the functionality where the user has indicated that the contents is no longer of value, but the buffer itself is cached until evicted or swapped out for performance reasons. So the above would work for eviction, but what about swapout. Could we add some similar functionality there? Amdgpu has the same functionality and you don't need to handle swap at all. Just return from the evict_flags that you want to drop the backing store as soon as the BO leaves the GTT domain. Christian. /Thomas Regards, Christian. /Thomas Thanks, Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
On 5/18/21 5:28 PM, Christian König wrote: Am 18.05.21 um 17:20 schrieb Thomas Hellström: On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. That sounds like a design bug to me since you should never need to do this. When you want to destroy the backing store of a buffer during eviction you should just do this by returning an empty placement from the evict_flags callback. So this is for the functionality where the user has indicated that the contents is no longer of value, but the buffer itself is cached until evicted or swapped out for performance reasons. So the above would work for eviction, but what about swapout. Could we add some similar functionality there? /Thomas Regards, Christian. /Thomas Thanks, Thomas
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 17:25 schrieb Thomas Hellström: On 5/18/21 5:17 PM, Christian König wrote: Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? Will have to take a look. That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. You mean the unmap_mapping_range() stuff? That's not a problem since it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() takes care of killing the ptes. That design is a certain NAK from my side for upstreaming this. PTE handling is the domain of TTM, drivers should never mess with that directly. Christian. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
Am 18.05.21 um 17:20 schrieb Thomas Hellström: On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. That sounds like a design bug to me since you should never need to do this. When you want to destroy the backing store of a buffer during eviction you should just do this by returning an empty placement from the evict_flags callback. It is TTMs job to deal with the buffer placement and drivers are no longer allowed to mess with that. Regards, Christian. /Thomas Thanks, Thomas
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
On 5/18/21 5:17 PM, Christian König wrote: Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? Will have to take a look. That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. You mean the unmap_mapping_range() stuff? That's not a problem since it's a NOP for kernel ttm buffers, and the i915 move() / swap_notify() takes care of killing the ptes. While we're in the process of killing that offset flexibility for discrete, we can't do so for older hardware unfortunately. /Thomas Christian. /Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
On 5/18/21 5:18 PM, Christian König wrote: Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. OK, so is that reentrant from the move() or swap_notify() callback. /Thomas Thanks, Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
Am 18.05.21 um 17:15 schrieb Thomas Hellström: On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? We already have that. Just call ttm_bo_validate() without any place to put the buffer. See how ttm_bo_pipeline_gutting() is used. Christian. Thanks, Thomas
[Bug 211277] sometimes crash at s2ram-wake (Ryzen 3500U): amdgpu, drm, commit_tail, amdgpu_dm_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=211277 jam...@amd.com (jam...@amd.com) changed: What|Removed |Added CC||jam...@amd.com --- Comment #19 from jam...@amd.com (jam...@amd.com) --- Created attachment 296841 --> https://bugzilla.kernel.org/attachment.cgi?id=296841=edit to fix suspend/resume hung issue Hi @kolAflash and @jeromec, Can you help check if this patch can fix the issue? Since we can't reproduce at our side. Thanks! James -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 17:11 schrieb Thomas Hellström: On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. Can you instead adjust the offset in the mmap callback like we do for dma-buf? That's really a no-go what you describe here because it will mess up reverse mapping lockup for buffer movement. Christian. /Thomas
Re: [PATCH v2 09/15] drm/ttm, drm/amdgpu: Allow the driver some control over swapping
On 5/18/21 10:26 AM, Thomas Hellström wrote: We are calling the eviction_valuable driver callback at eviction time to determine whether we actually can evict a buffer object. The upcoming i915 TTM backend needs the same functionality for swapout, and that might actually be beneficial to other drivers as well. Add an eviction_valuable call also in the swapout path. Try to keep the current behaviour for all drivers by returning true if the buffer object is already in the TTM_PL_SYSTEM placement. We change behaviour for the case where a buffer object is in a TT backed placement when swapped out, in which case the drivers normal eviction_valuable path is run. Finally export ttm_tt_unpopulate() and don't swap out bos that are not populated. This allows a driver to purge a bo at swapout time if its content is no longer valuable rather than to have TTM swap the contents out. Cc: Christian König Signed-off-by: Thomas Hellström Christian, Here we have a ttm_tt_unpopulate() export as well at the end. I figure you will push back on that one. What we really need is a functionality to just drop the bo contents and end up in system memory unpopulated. Should I perhaps add a utility function to do that instead? like ttm_bo_purge()? Thanks, Thomas
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
Am 18.05.21 um 17:03 schrieb Andrey Grodzovsky:
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over the
dma_resv requirements and I came to the conclusion that this approach
here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and exports
it as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering,
for the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer
scheduled.
The problem is now that we can't do this since the operation of
device A is still running and by signaling our fences we run into the
problem of potential memory corruption.
I am not sure this problem you describe above is related to this patch.
Well it is kind of related.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
And exactly that is problematic. See the jobs on the entity need to
cleanly wait for their dependencies before they can be completed.
drm_sched_entity_kill_jobs() is also not handling that correctly at the
moment, we only wait for the last scheduled fence but not for the
dependencies of the job.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences for the
extracted device B, we are not touching any other fences. In fact as you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.
Signaling hardware fences is unproblematic since they are emitted when
the software scheduling is already completed.
Christian.
Andrey
Not sure how to handle that case. One possibility would be to wait
for all dependencies of unscheduled jobs before signaling their
fences as canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because
drm_sched_entity_is_idle
never becomes false.
Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.
v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.
v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
drivers/gpu/drm/scheduler/sched_main.c | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 0249c7450188..2e93e881b65f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
rmb(); /* for list_empty to work without lock */
if (list_empty(>list) ||
- spsc_queue_count(>job_queue) == 0)
+ spsc_queue_count(>job_queue) == 0 ||
+ entity->stopped)
return true;
return false;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 8d1211e87101..a2a953693b45 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
*/
void drm_sched_fini(struct drm_gpu_scheduler *sched)
{
+ struct drm_sched_entity *s_entity;
+ int i;
+
if (sched->thread)
kthread_stop(sched->thread);
+ for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >=
DRM_SCHED_PRIORITY_MIN; i--) {
+ struct drm_sched_rq *rq = >sched_rq[i];
+
+ if (!rq)
+ continue;
+
+ spin_lock(>lock);
+ list_for_each_entry(s_entity, >entities, list)
+ /*
+ * Prevents reinsertion and marks job_queue as idle,
+ * it will removed from rq in drm_sched_entity_fini
+ * eventually
+ */
+ s_entity->stopped = true;
+ spin_unlock(>lock);
+
+ }
+
+ /* Wakeup everyone stuck in drm_sched_entity_flush for this
scheduler */
+ wake_up_all(>job_scheduled);
+
/* Confirm no work left behind accessing device structures */
cancel_delayed_work_sync(>work_tdr);
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
On 5/18/21 5:07 PM, Christian König wrote: Am 18.05.21 um 16:55 schrieb Thomas Hellström: From: Maarten Lankhorst This allows other drivers that may not setup the vma in the same way to use the ttm bo helpers. Uff can you please explain why exactly you need that? Providing the BO is not much of a problem, but having the BO at different VMA offsets is really a no-go with TTM. Christian. The current i915 uapi is using different offsets for different caching :/. We're currently working around that by using ttm_bo_type_kernel (no TTM vma offset at all) and i915's offset. /Thomas
Re: [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
Am 18.05.21 um 16:55 schrieb Thomas Hellström:
From: Maarten Lankhorst
This allows other drivers that may not setup the vma in the same way
to use the ttm bo helpers.
Uff can you please explain why exactly you need that?
Providing the BO is not much of a problem, but having the BO at
different VMA offsets is really a no-go with TTM.
Christian.
Also clarify the documentation a bit, especially related to VM_FAULT_RETRY.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4 +-
drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +-
drivers/gpu/drm/radeon/radeon_ttm.c| 4 +-
drivers/gpu/drm/ttm/ttm_bo_vm.c| 84 +-
drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 8 ++-
include/drm/ttm/ttm_bo_api.h | 9 ++-
6 files changed, 75 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index d5a9d7a88315..89dafe14f828 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1919,7 +1919,9 @@ static vm_fault_t amdgpu_ttm_fault(struct vm_fault *vmf)
if (ret)
goto unlock;
- ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ vmf->vma->vm_page_prot,
TTM_BO_VM_NUM_PREFAULT, 1);
if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index b81ae90b8449..555fb6d8be8b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -144,7 +144,9 @@ static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf)
nouveau_bo_del_io_reserve_lru(bo);
prot = vm_get_page_prot(vma->vm_flags);
- ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT, 1);
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ prot, TTM_BO_VM_NUM_PREFAULT, 1);
nouveau_bo_add_io_reserve_lru(bo);
if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
return ret;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 3361d11769a2..ba48a2acdef0 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -816,7 +816,9 @@ static vm_fault_t radeon_ttm_fault(struct vm_fault *vmf)
if (ret)
goto unlock_resv;
- ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ vmf->vma->vm_page_prot,
TTM_BO_VM_NUM_PREFAULT, 1);
if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT))
goto unlock_mclk;
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index b31b18058965..ed00ccf1376e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -42,7 +42,7 @@
#include
static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo,
- struct vm_fault *vmf)
+ struct vm_fault *vmf)
{
vm_fault_t ret = 0;
int err = 0;
@@ -122,7 +122,8 @@ static unsigned long ttm_bo_io_mem_pfn(struct
ttm_buffer_object *bo,
* Return:
*0 on success and the bo was reserved.
*VM_FAULT_RETRY if blocking wait.
- *VM_FAULT_NOPAGE if blocking wait and retrying was not allowed.
+ *VM_FAULT_NOPAGE if blocking wait and retrying was not allowed, or wait
interrupted.
+ *VM_FAULT_SIGBUS if wait on bo->moving failed for reason other than a
signal.
*/
vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
struct vm_fault *vmf)
@@ -254,7 +255,9 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault
*vmf,
/**
* ttm_bo_vm_fault_reserved - TTM fault helper
+ * @bo: The buffer object
* @vmf: The struct vm_fault given as argument to the fault callback
+ * @mmap_base: The base of the mmap, to which the @vmf fault is relative to.
* @prot: The page protection to be used for this memory area.
* @num_prefault: Maximum number of prefault pages. The caller may want to
* specify this based on madvice settings and the size of the GPU object
@@ -265,19 +268,28 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct vm_fault
*vmf,
* memory backing the buffer object, and then returns a return code
* instructing the caller to retry the page access.
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
On 2021-05-18 10:07 a.m., Christian König wrote:
In a separate discussion with Daniel we once more iterated over the
dma_resv requirements and I came to the conclusion that this approach
here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and exports it
as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering, for
the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer scheduled.
The problem is now that we can't do this since the operation of device A
is still running and by signaling our fences we run into the problem of
potential memory corruption.
I am not sure this problem you describe above is related to this patch.
Here we purely expand the criteria for when sched_entity is
considered idle in order to prevent a hang on device remove.
Were you addressing the patch from yesterday in which you commented
that you found a problem with how we finish fences ? It was
'[PATCH v7 12/16] drm/amdgpu: Fix hang on device removal.'
Also, in the patch series as it is now we only signal HW fences for the
extracted device B, we are not touching any other fences. In fact as you
may remember, I dropped all new logic to forcing fence completion in
this patch series and only call amdgpu_fence_driver_force_completion
for the HW fences of the extracted device as it's done today anyway.
Andrey
Not sure how to handle that case. One possibility would be to wait for
all dependencies of unscheduled jobs before signaling their fences as
canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because
drm_sched_entity_is_idle
never becomes false.
Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.
v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.
v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
drivers/gpu/drm/scheduler/sched_main.c | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 0249c7450188..2e93e881b65f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
rmb(); /* for list_empty to work without lock */
if (list_empty(>list) ||
- spsc_queue_count(>job_queue) == 0)
+ spsc_queue_count(>job_queue) == 0 ||
+ entity->stopped)
return true;
return false;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 8d1211e87101..a2a953693b45 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
*/
void drm_sched_fini(struct drm_gpu_scheduler *sched)
{
+ struct drm_sched_entity *s_entity;
+ int i;
+
if (sched->thread)
kthread_stop(sched->thread);
+ for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >=
DRM_SCHED_PRIORITY_MIN; i--) {
+ struct drm_sched_rq *rq = >sched_rq[i];
+
+ if (!rq)
+ continue;
+
+ spin_lock(>lock);
+ list_for_each_entry(s_entity, >entities, list)
+ /*
+ * Prevents reinsertion and marks job_queue as idle,
+ * it will removed from rq in drm_sched_entity_fini
+ * eventually
+ */
+ s_entity->stopped = true;
+ spin_unlock(>lock);
+
+ }
+
+ /* Wakeup everyone stuck in drm_sched_entity_flush for this
scheduler */
+ wake_up_all(>job_scheduled);
+
/* Confirm no work left behind accessing device structures */
cancel_delayed_work_sync(>work_tdr);
Re: [Intel-gfx] [PATCH 4/4] i915: fix remap_io_sg to verify the pgprot
On Tue, 18 May 2021 at 14:21, Christoph Hellwig wrote: > > On Mon, May 17, 2021 at 06:06:44PM +0100, Matthew Auld wrote: > > > Looks like it is caused by the validation failure then. Which means the > > > existing code is doing something wrong in its choice of the page > > > protection bit. I really need help from the i915 maintainers here.. > > > > AFAIK there are two users of remap_io_sg, the first is our shmem > > objects(see i915_gem_shmem.c), and for these we support UC, WC, and WB > > mmap modes for userspace. The other user is device local-memory > > objects(VRAM), and for this one we have an actual io_mapping which is > > allocated as WC, and IIRC this should only be mapped as WC for the > > mmap mode, but normal userspace can't hit this path yet. > > The only caller in current mainline is vm_fault_cpu in i915_gem_mman.c. > Is that device local? The vm_fault_cpu covers both device local and shmem objects. > > > What do we need to do here? It sounds like shmem backed objects are > > allocated as WB for the pages underneath, but i915 allows mapping them > > as UC/WC which trips up this track_pfn thing? > > To me the warnings looks like system memory is mapped with the wrong > permissions, yes. If you want to map it as UC/WC the right set_memory_* > needs to be used on the kernel mapping as well to ensure that the > attributes don't conflict. AFAIK mmap_offset also supports multiple active mmap modes for a given object, so set_memory_* should still work here?
Re: [Intel-gfx] [PATCH v2 13/15] drm/ttm: Add BO and offset arguments for vm_access and vm_fault ttm handlers.
On 5/18/21 1:59 PM, Christian König wrote:
Can you send me the patch directly and not just on CC?
Thanks,
Christian.
Original patch sent. Pls remember to CC lists on reply, though.
The reason we need this is because of i915's strange mmap functionality
which allows a bo to be mapped at multiple offsets and the vma->private
is not a bo...
Thanks,
Thomas
Am 18.05.21 um 10:59 schrieb Thomas Hellström:
+ Christian König
On 5/18/21 10:26 AM, Thomas Hellström wrote:
From: Maarten Lankhorst
This allows other drivers that may not setup the vma in the same way
to use the ttm bo helpers.
Also clarify the documentation a bit, especially related to
VM_FAULT_RETRY.
Signed-off-by: Maarten Lankhorst
Lgtm. Reviewed-by: Thomas Hellström
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +-
drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +-
drivers/gpu/drm/radeon/radeon_ttm.c | 4 +-
drivers/gpu/drm/ttm/ttm_bo_vm.c | 84
+-
drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 8 ++-
include/drm/ttm/ttm_bo_api.h | 9 ++-
6 files changed, 75 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index d5a9d7a88315..89dafe14f828 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1919,7 +1919,9 @@ static vm_fault_t amdgpu_ttm_fault(struct
vm_fault *vmf)
if (ret)
goto unlock;
- ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ vmf->vma->vm_page_prot,
TTM_BO_VM_NUM_PREFAULT, 1);
if (ret == VM_FAULT_RETRY && !(vmf->flags &
FAULT_FLAG_RETRY_NOWAIT))
return ret;
diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c
b/drivers/gpu/drm/nouveau/nouveau_ttm.c
index b81ae90b8449..555fb6d8be8b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_ttm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c
@@ -144,7 +144,9 @@ static vm_fault_t nouveau_ttm_fault(struct
vm_fault *vmf)
nouveau_bo_del_io_reserve_lru(bo);
prot = vm_get_page_prot(vma->vm_flags);
- ret = ttm_bo_vm_fault_reserved(vmf, prot,
TTM_BO_VM_NUM_PREFAULT, 1);
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ prot, TTM_BO_VM_NUM_PREFAULT, 1);
nouveau_bo_add_io_reserve_lru(bo);
if (ret == VM_FAULT_RETRY && !(vmf->flags &
FAULT_FLAG_RETRY_NOWAIT))
return ret;
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 3361d11769a2..ba48a2acdef0 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -816,7 +816,9 @@ static vm_fault_t radeon_ttm_fault(struct
vm_fault *vmf)
if (ret)
goto unlock_resv;
- ret = ttm_bo_vm_fault_reserved(vmf, vmf->vma->vm_page_prot,
+ ret = ttm_bo_vm_fault_reserved(bo, vmf,
+ drm_vma_node_start(>base.vma_node),
+ vmf->vma->vm_page_prot,
TTM_BO_VM_NUM_PREFAULT, 1);
if (ret == VM_FAULT_RETRY && !(vmf->flags &
FAULT_FLAG_RETRY_NOWAIT))
goto unlock_mclk;
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c
b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index b31b18058965..ed00ccf1376e 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -42,7 +42,7 @@
#include
static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object
*bo,
- struct vm_fault *vmf)
+ struct vm_fault *vmf)
{
vm_fault_t ret = 0;
int err = 0;
@@ -122,7 +122,8 @@ static unsigned long ttm_bo_io_mem_pfn(struct
ttm_buffer_object *bo,
* Return:
* 0 on success and the bo was reserved.
* VM_FAULT_RETRY if blocking wait.
- * VM_FAULT_NOPAGE if blocking wait and retrying was not allowed.
+ * VM_FAULT_NOPAGE if blocking wait and retrying was not
allowed, or wait interrupted.
+ * VM_FAULT_SIGBUS if wait on bo->moving failed for reason other
than a signal.
*/
vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
struct vm_fault *vmf)
@@ -254,7 +255,9 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct
vm_fault *vmf,
/**
* ttm_bo_vm_fault_reserved - TTM fault helper
+ * @bo: The buffer object
* @vmf: The struct vm_fault given as argument to the fault callback
+ * @mmap_base: The base of the mmap, to which the @vmf fault is
relative to.
* @prot: The page protection to be used for this memory area.
* @num_prefault: Maximum number of prefault pages. The caller may
want to
* specify this based on madvice settings and the size of the GPU
object
@@ -265,19 +268,28 @@ static vm_fault_t ttm_bo_vm_insert_huge(struct
vm_fault *vmf,
* memory backing the buffer object, and then returns a return code
* instructing
Re: [PATCH] dt-bindings: display: ssd1307fb: Convert to json-schema
Hi Maxime,
On Tue, May 18, 2021 at 4:33 PM Maxime Ripard wrote:
> On Tue, May 18, 2021 at 09:51:31AM +0200, Geert Uytterhoeven wrote:
> > Convert the Solomon SSD1307 Framebuffer Device Tree binding
> > documentation to json-schema.
> >
> > Fix the spelling of the "pwms" property.
> > Document default values.
> > Make properties with default values not required.
> >
> > Signed-off-by: Geert Uytterhoeven
> > ---
> > I have listed Maxime as the maintainer, as he wrote the original driver
> > and bindings. Maxime: Please scream if this is inappropriate ;-)
>
> Fine by me :)
Thanks!
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
> > + solomon,dclk-div:
> > +$ref: /schemas/types.yaml#/definitions/uint32
> > +minimum: 1
> > +maximum: 16
> > +description:
> > + Clock divisor. The default value is controller-dependent.
>
> I guess we could document the default using an if / else statement?
While clk-div has only two different defaults, dclk-frq has different
defaults for each of the 4 variants supported.
Do you think it's worthwhile doing that? All upstream DTS files lack
these properties, thus use the default values.
> Looks good otherwise :)
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 RESEND 1/2] dt-bindings: display: add google, cros-ec-anx7688.yaml
Series applied to drm-misc-next.
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=b67f7599c90ae36a5174826132f7690fa13d462c
On Tue, 18 May 2021 at 16:19, Dafna Hirschfeld
wrote:
>
> ChromeOS EC ANX7688 is a display bridge that converts HDMI 2.0 to
> DisplayPort 1.3 Ultra-HDi (4096x2160p60). It is an Analogix ANX7688 chip
> which is connected to and operated by the ChromeOS Embedded Controller
> (See google,cros-ec.yaml). It is accessed using I2C tunneling through
> the EC and therefore its node should be a child of an EC I2C tunnel node
> (See google,cros-ec-i2c-tunnel.yaml).
>
> ChromOS EC ANX7688 is found on Acer Chromebook R13 (elm)
>
> Signed-off-by: Dafna Hirschfeld
> Reviewed-by: Rob Herring
> ---
> .../bridge/google,cros-ec-anx7688.yaml| 82 +++
> 1 file changed, 82 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
>
> b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
> new file mode 100644
> index ..9f7cc6b757cb
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/display/bridge/google,cros-ec-anx7688.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ChromeOS EC ANX7688 HDMI to DP Converter through Type-C Port
> +
> +maintainers:
> + - Nicolas Boichat
> + - Enric Balletbo i Serra
> +
> +description: |
> + ChromeOS EC ANX7688 is a display bridge that converts HDMI 2.0 to
> + DisplayPort 1.3 Ultra-HDi (4096x2160p60). It is an Analogix ANX7688 chip
> + which is connected to and operated by the ChromeOS Embedded Controller
> + (See google,cros-ec.yaml). It is accessed using I2C tunneling through
> + the EC and therefore its node should be a child of an EC I2C tunnel node
> + (See google,cros-ec-i2c-tunnel.yaml).
> +
> +properties:
> + compatible:
> +const: google,cros-ec-anx7688
> +
> + reg:
> +maxItems: 1
> +description: I2C address of the device.
> +
> + ports:
> +$ref: /schemas/graph.yaml#/properties/ports
> +
> +properties:
> + port@0:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: Video port for HDMI input.
> +
> + port@1:
> +$ref: /schemas/graph.yaml#/properties/port
> +description: USB Type-c connector.
> +
> +required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - reg
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +i2c_tunnel_b: i2c-tunnel1 {
> +compatible = "google,cros-ec-i2c-tunnel";
> +google,remote-bus = <1>;
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +anx7688: anx7688@2c {
> +compatible = "google,cros-ec-anx7688";
> +reg = <0x2c>;
> +
> +ports {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +port@0 {
> +reg = <0>;
> +anx7688_in: endpoint {
> +remote-endpoint = <_out>;
> +};
> +};
> +port@1 {
> +reg = <1>;
> +anx7688_out: endpoint {
> +remote-endpoint = <_connector>;
> +};
> +};
> +};
> +};
> +};
> +
> --
> 2.17.1
>
Re: [PATCH] dt-bindings: display: ssd1307fb: Convert to json-schema
Hi On Tue, May 18, 2021 at 09:51:31AM +0200, Geert Uytterhoeven wrote: > Convert the Solomon SSD1307 Framebuffer Device Tree binding > documentation to json-schema. > > Fix the spelling of the "pwms" property. > Document default values. > Make properties with default values not required. > > Signed-off-by: Geert Uytterhoeven > --- > I have listed Maxime as the maintainer, as he wrote the original driver > and bindings. Maxime: Please scream if this is inappropriate ;-) Fine by me :) > --- > .../bindings/display/solomon,ssd1307fb.yaml | 166 ++ > .../devicetree/bindings/display/ssd1307fb.txt | 60 --- > 2 files changed, 166 insertions(+), 60 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml > delete mode 100644 Documentation/devicetree/bindings/display/ssd1307fb.txt > > diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml > b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml > new file mode 100644 > index ..bd632d86a4f814a0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml > @@ -0,0 +1,166 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/solomon,ssd1307fb.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Solomon SSD1307 OLED Controller Framebuffer > + > +maintainers: > + - Maxime Ripard > + > +properties: > + compatible: > +enum: > + - solomon,ssd1305fb-i2c > + - solomon,ssd1306fb-i2c > + - solomon,ssd1307fb-i2c > + - solomon,ssd1309fb-i2c > + > + reg: > +maxItems: 1 > + > + pwms: > +maxItems: 1 > + > + reset-gpios: > +maxItems: 1 > + > + vbat-supply: > +description: The supply for VBAT > + > + solomon,height: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 16 > +description: > + Height in pixel of the screen driven by the controller > + > + solomon,width: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 96 > +description: > + Width in pixel of the screen driven by the controller > + > + solomon,page-offset: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 1 > +description: > + Offset of pages (band of 8 pixels) that the screen is mapped to > + > + solomon,segment-no-remap: > +type: boolean > +description: > + Display needs normal (non-inverted) data column to segment mapping > + > + solomon,col-offset: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 0 > +description: > + Offset of columns (COL/SEG) that the screen is mapped to > + > + solomon,com-seq: > +type: boolean > +description: > + Display uses sequential COM pin configuration > + > + solomon,com-lrremap: > +type: boolean > +description: > + Display uses left-right COM pin remap > + > + solomon,com-invdir: > +type: boolean > +description: > + Display uses inverted COM pin scan direction > + > + solomon,com-offset: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 0 > +description: > + Number of the COM pin wired to the first display line > + > + solomon,prechargep1: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 2 > +description: > + Length of deselect period (phase 1) in clock cycles > + > + solomon,prechargep2: > +$ref: /schemas/types.yaml#/definitions/uint32 > +default: 2 > +description: > + Length of precharge period (phase 2) in clock cycles. This needs to be > + the higher, the higher the capacitance of the OLED's pixels is. > + > + solomon,dclk-div: > +$ref: /schemas/types.yaml#/definitions/uint32 > +minimum: 1 > +maximum: 16 > +description: > + Clock divisor. The default value is controller-dependent. I guess we could document the default using an if / else statement? Looks good otherwise :) Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 1/3] drm/color: Add RGB Color encodings
On 2021-05-17 4:34 a.m., Pekka Paalanen wrote: > On Fri, 14 May 2021 17:04:51 -0400 > Harry Wentland wrote: > >> On 2021-04-30 8:53 p.m., Sebastian Wick wrote: >>> On 2021-04-26 20:56, Harry Wentland wrote: > > ... > Another reason I'm proposing to define the color space (and gamma) of a plane is to make this explicit. Up until the color space and gamma of a plane or framebuffer are not well defined, which leads to drivers assuming the color space and gamma of a buffer (for blending and other purposes) and might lead to sub-optimal outcomes. >>> >>> Blending only is "correct" with linear light so that property of the >>> color space is important. However, why does the kernel have to be >>> involved here? As long as user space knows that for correct blending the >>> data must represent linear light and knows when in the pipeline blending >>> happens it can make sure that the data at that point in the pipeline >>> contains linear light. >>> >> >> The only reason the kernel needs to be involved is to take full advantage >> of the available HW without requiring KMS clients to be aware of >> the difference in display HW. > > Can you explain in more tangible examples, why you think so, please? > > Is it because hardware does not fit the KMS UAPI model of the abstract > pixel pipeline? > I'd wager no HW is designed to meet KMS UAPI, rather KMS UAPI is designed to abstract HW. > Or is it because you have fixed-function hardware elements that you can > only make use of when userspace uses an enum-based UAPI? > One example is our degamma on our latest generation HW, where we have fixed-function "degamma" (rather de-EOTF): https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_dpp.c#L166 > I would totally agree that the driver does not want to be analysing LUT > entries to decipher if it could use a fixed-function element or not. It > would introduce uncertainty in the UAPI. So fixed-function elements > would need their own properties, but I don't know if that is feasible > as generic UAPI or if it should be driver-specific (and so left unused > by generic userspace). > For the CRTC LUTs we actually do a linearity check to program the HW into bypass when the LUT is linear since the KMS LUT definition doesn't map well onto the LUT definition used by our HW and leads to rounding errors and failing IGT kms_color tests (if I remember this correctly). https://gitlab.freedesktop.org/agd5f/linux/-/blob/amd-staging-drm-next/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_color.c#L330 Hence the suggestion to define pre-defined TFs right at a KMS level for usecases where we can assume the display will tonemap the content. Harry > > Thanks, > pq >
Re: [PATCH v5 4/6] drm/sprd: add Unisoc's drm display controller driver
On Fri, May 14, 2021 at 09:18:00PM +0800, Kevin Tang wrote:
> Maxime Ripard 于2021年4月30日周五 下午5:22写道：
> > > + info = drm_format_info(fb->format->format);
> >
> > Here fb->format is the result of drm_format_info(fb->format->format)
>
> info->num_planes == 3? I will fix it on next version
It's not really what I meant. You don't need the call to drm_format_info
in the first place, the result is going to be fb->format.
So either you do info = fb->format and
> > > + if (fb->format->num_planes == 3) {
You use info here
> > > + /* UV pitch is 1/2 of Y pitch */
> > > + pitch = (fb->pitches[0] / info->cpp[0]) |
> > > + (fb->pitches[0] / info->cpp[0] << 15);
Or you can use fb->format->cpp here
Either way you should be consistent.
> > > +static struct sprd_plane *sprd_planes_init(struct drm_device *drm)
> > > +{
> > > + struct sprd_plane *plane, *primary;
> > > + enum drm_plane_type plane_type;
> > > + int i;
> > > +
> > > + for (i = 0; i < 6; i++) {
> > > + plane_type = (i == 0) ? DRM_PLANE_TYPE_PRIMARY :
> > > + DRM_PLANE_TYPE_OVERLAY;
> > > +
> > > + plane = drmm_universal_plane_alloc(drm, struct sprd_plane,
> > > base,
> > > +1, _plane_funcs,
> > > +layer_fmts,
> > > ARRAY_SIZE(layer_fmts),
> > > +NULL, plane_type, NULL);
> > > + if (IS_ERR(plane)) {
> > > + drm_err(drm, "failed to init drm plane: %d\n", i);
> > > + return plane;
> > > + }
> > > +
> > > + drm_plane_helper_add(>base,
> > > _plane_helper_funcs);
> > > +
> > > + sprd_plane_create_properties(plane, i);
> > > +
> > > + if (i == 0)
> > > + primary = plane;
> > > + }
> > > +
> > > + return primary;
> > > +}
> > > +
> > > +static void sprd_crtc_mode_set_nofb(struct drm_crtc *crtc)
> > > +{
> > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > + struct drm_display_mode *mode = >state->adjusted_mode;
> > > +
> > > + if (mode->type & DRM_MODE_TYPE_PREFERRED)
> > > + drm_display_mode_to_videomode(mode, >ctx.vm);
> >
> > What happens if the mode isn't a preferred mode?
>
> Have already replied on the dsi driver side
>
> > > +}
> > > +
> > > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,
> > > +struct drm_atomic_state *state)
> > > +{
> > > + struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > +
> > > + sprd_dpu_init(dpu);
> >
> > I guess that call would fail here or program a bogus value. We already
> > discussed this in the previous version, but I'm really not sure why you
> > need this in the first place. Just use the crtc_state->adjusted_mode and
> > program that.
>
> Is also the enable_irq issue about this comment?
Not really? This is about the preferred mode stuff.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v5 6/6] drm/sprd: add Unisoc's drm mipi dsi driver
On Wed, May 12, 2021 at 09:53:06PM +0800, Kevin Tang wrote:
> > > +struct dsi_reg {
> > > + union _0x00 {
> > > + u32 val;
> > > + struct _DSI_VERSION {
> > > + u32 dsi_version: 16;
> > > + u32 reserved: 16;
> > > + } bits;
> > > + } DSI_VERSION;
> >
> > Using unions and structures to define the register is really frowned
> > upon in favor of defines, like you rightfully did in the crtc driver.
>
> This workload is too big, this design has been used for many years,
> so I actually want to keep it the same, but if it really doesn’t meet
> the current design.
>
> I can change the design, but it may take a lot of time..
There's no rush :)
> > > +static int sprd_dsi_find_panel(struct sprd_dsi *dsi)
> > > +{
> > > + struct device *dev = dsi->host.dev;
> > > + struct device_node *child, *lcds_node;
> > > + struct drm_panel *panel;
> > > +
> > > + /* search /lcds child node first */
> > > + lcds_node = of_find_node_by_path("/lcds");
> > > + for_each_child_of_node(lcds_node, child) {
> > > + panel = of_drm_find_panel(child);
> > > + if (!IS_ERR(panel)) {
> > > + dsi->panel = panel;
> > > + return 0;
> > > + }
> > > + }
> >
> > That's not part of your binding
> Ok, i will remove it.
> >
> > > +
> > > + /*
> > > + * If /lcds child node search failed, we search
> > > + * the child of dsi host node.
> > > + */
> > > + for_each_child_of_node(dev->of_node, child) {
> > > + panel = of_drm_find_panel(child);
> > > + if (!IS_ERR(panel)) {
> > > + dsi->panel = panel;
> > > + return 0;
> > > + }
> > > + }
> >
> > And you don't need this either. You'll register a mipi_dsi_host,
> > that will in turn probe all the devices under that bus, and will
> > then call the .attach callback.
>
> This should be move to the .attach callback?
The panel pointer assignment can. The rest is useless.
> > > + drm_err(dsi->drm, "of_drm_find_panel() failed\n");
> > > + return -ENODEV;
> > > +}
> > > +
> > > +static int sprd_dsi_host_attach(struct mipi_dsi_host *host,
> > > +struct mipi_dsi_device *slave)
> > > +{
> > > + struct sprd_dsi *dsi = host_to_dsi(host);
> > > + struct dsi_context *ctx = >ctx;
> > > + int ret;
> > > +
> > > + dsi->slave = slave;
> > > + ctx->lanes = slave->lanes;
> > > + ctx->format = slave->format;
> > > + ctx->byte_clk = slave->hs_rate / 8;
> > > + ctx->esc_clk = slave->lp_rate;
> > > +
> > > + if (slave->mode_flags & MIPI_DSI_MODE_VIDEO)
> > > + ctx->work_mode = DSI_MODE_VIDEO;
> > > + else
> > > + ctx->work_mode = DSI_MODE_CMD;
> > > +
> > > + if (slave->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> > > + ctx->burst_mode = VIDEO_BURST_WITH_SYNC_PULSES;
> > > + else if (slave->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> > > + ctx->burst_mode = VIDEO_NON_BURST_WITH_SYNC_PULSES;
> > > + else
> > > + ctx->burst_mode = VIDEO_NON_BURST_WITH_SYNC_EVENTS;
> > > +
> > > + if (slave->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)
> > > + ctx->nc_clk_en = true;
> >
> > I'm not sure why you need to duplicate all this, can't you just
> > dereference the dsi->slave pointer when you need it?
>
> Sorry, can you help me with a demo?
In your sprd_dsi_encoder_enable function, you call sprd_dphy_init which
takes a pointer to struct dsi_context, and will call, say,
dsi_phy_datalane_en, using ctx->lanes.
Since you have access to the struct sprd_dsi in sprd_dsi_encoder_enable,
why not pass it and the mipi_dsi_device pointer to sprd_dphy_init, and
dereference slave->lanes in dsi_phy_datalane_en?
This will greatly reduce the size of the dsi_context structure.
> > > +static enum drm_mode_status
> > > +sprd_dsi_connector_mode_valid(struct drm_connector *connector,
> > > + struct drm_display_mode *mode)
> > > +{
> > > + struct sprd_dsi *dsi = connector_to_dsi(connector);
> > > +
> > > + drm_dbg(dsi->drm, "%s() mode: "DRM_MODE_FMT"\n", __func__,
> > > DRM_MODE_ARG(mode));
> > > +
> > > + if (mode->type & DRM_MODE_TYPE_PREFERRED) {
> > > + dsi->mode = mode;
> > > + drm_display_mode_to_videomode(dsi->mode, >ctx.vm);
> > > + }
> >
> > Again, what happens if the mode isn't the preferred one?
>
> We hope to restore the low-resolution image to the original resolution
> through the scaling algorithm while keeping the resolution unchanged.
> So whether it's dpu or dsi, must be keeping on preferred mode.
Is there any particular reason to do so? Why do you need to take the
preferred mode into account in the first place? Can't you just use
whatever drm_display_mode is being passed?
Maxime
signature.asc
Description: PGP signature
[PATCH v6 RESEND 0/2] Add support for ANX7688
(resending patchset with Rb tags) ANX7688 is a typec port controller that also converts HDMI to DP. It is found on Acer Chromebook R13 (elm) and on Pine64 PinePhone. On Acer Chromebook R13 (elm), the device is powered-up and controller by the Embedded Controller. Therefore its operation is transparent to the SoC. It is used in elm only as a display bridge driver. The bridge driver only reads some values using i2c and use them to implement the mode_fixup cb. On v5 we added the full dt-binding of the generic Analogix anx7688 device. The problem is that for elm, most of the fields are not needed since the anx7688 sits behind the EC. After a discussion on v5 (see [1]) we decided to go back to the original approach and send the dt binding as specific to the elm. So in this version we rename the device to cros_ec_anx7688 and use the compatible 'google,cros-ec-anx7688'. [1] https://patchwork.kernel.org/project/dri-devel/patch/20210305124351.15079-3-dafna.hirschf...@collabora.com/ Changes since v5: * treat the device as a specific combination of an ANX7688 behind the EC and call it 'cros-ec-anx7688' Changes since v4: In v4 of this set, the device was added as an 'mfd' device and an additional 'bridge' device for the HDMI-DP conversion, see [2]. [2] https://lkml.org/lkml/2020/3/18/64 Dafna Hirschfeld (1): dt-bindings: display: add google,cros-ec-anx7688.yaml Enric Balletbo i Serra (1): drm/bridge: Add ChromeOS EC ANX7688 bridge driver support .../bridge/google,cros-ec-anx7688.yaml| 82 drivers/gpu/drm/bridge/Kconfig| 12 ++ drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/cros-ec-anx7688.c | 191 ++ 4 files changed, 286 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml create mode 100644 drivers/gpu/drm/bridge/cros-ec-anx7688.c -- 2.17.1
[PATCH v6 RESEND 1/2] dt-bindings: display: add google, cros-ec-anx7688.yaml
ChromeOS EC ANX7688 is a display bridge that converts HDMI 2.0 to
DisplayPort 1.3 Ultra-HDi (4096x2160p60). It is an Analogix ANX7688 chip
which is connected to and operated by the ChromeOS Embedded Controller
(See google,cros-ec.yaml). It is accessed using I2C tunneling through
the EC and therefore its node should be a child of an EC I2C tunnel node
(See google,cros-ec-i2c-tunnel.yaml).
ChromOS EC ANX7688 is found on Acer Chromebook R13 (elm)
Signed-off-by: Dafna Hirschfeld
Reviewed-by: Rob Herring
---
.../bridge/google,cros-ec-anx7688.yaml| 82 +++
1 file changed, 82 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
diff --git
a/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
new file mode 100644
index ..9f7cc6b757cb
--- /dev/null
+++
b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/google,cros-ec-anx7688.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ChromeOS EC ANX7688 HDMI to DP Converter through Type-C Port
+
+maintainers:
+ - Nicolas Boichat
+ - Enric Balletbo i Serra
+
+description: |
+ ChromeOS EC ANX7688 is a display bridge that converts HDMI 2.0 to
+ DisplayPort 1.3 Ultra-HDi (4096x2160p60). It is an Analogix ANX7688 chip
+ which is connected to and operated by the ChromeOS Embedded Controller
+ (See google,cros-ec.yaml). It is accessed using I2C tunneling through
+ the EC and therefore its node should be a child of an EC I2C tunnel node
+ (See google,cros-ec-i2c-tunnel.yaml).
+
+properties:
+ compatible:
+const: google,cros-ec-anx7688
+
+ reg:
+maxItems: 1
+description: I2C address of the device.
+
+ ports:
+$ref: /schemas/graph.yaml#/properties/ports
+
+properties:
+ port@0:
+$ref: /schemas/graph.yaml#/properties/port
+description: Video port for HDMI input.
+
+ port@1:
+$ref: /schemas/graph.yaml#/properties/port
+description: USB Type-c connector.
+
+required:
+ - port@0
+ - port@1
+
+required:
+ - compatible
+ - reg
+ - ports
+
+additionalProperties: false
+
+examples:
+ - |
+i2c_tunnel_b: i2c-tunnel1 {
+compatible = "google,cros-ec-i2c-tunnel";
+google,remote-bus = <1>;
+#address-cells = <1>;
+#size-cells = <0>;
+
+anx7688: anx7688@2c {
+compatible = "google,cros-ec-anx7688";
+reg = <0x2c>;
+
+ports {
+#address-cells = <1>;
+#size-cells = <0>;
+port@0 {
+reg = <0>;
+anx7688_in: endpoint {
+remote-endpoint = <_out>;
+};
+};
+port@1 {
+reg = <1>;
+anx7688_out: endpoint {
+remote-endpoint = <_connector>;
+};
+};
+};
+};
+};
+
--
2.17.1
[PATCH v6 RESEND 2/2] drm/bridge: Add ChromeOS EC ANX7688 bridge driver support
From: Enric Balletbo i Serra
This driver adds support for the ChromeOS EC ANX7688 HDMI to DP converter
For our use case, the only reason the Linux kernel driver is necessary is
to reject resolutions that require more bandwidth than what is available
on the DP side. DP bandwidth and lane count are reported by the bridge via
2 registers and, as far as we know, only chips that have a firmware
version greater than 0.85 support these two registers.
Signed-off-by: Nicolas Boichat
Signed-off-by: Hsin-Yi Wang
[The driver is OF only so should depends on CONFIG_OF]
Reported-by: kbuild test robot
Signed-off-by: Enric Balletbo i Serra
[convert to i2c driver, rename to cros_ec_anx7688, add err checks]
Signed-off-by: Dafna Hirschfeld
Reviewed-by: Laurent Pinchart
---
drivers/gpu/drm/bridge/Kconfig | 12 ++
drivers/gpu/drm/bridge/Makefile | 1 +
drivers/gpu/drm/bridge/cros-ec-anx7688.c | 191 +++
3 files changed, 204 insertions(+)
create mode 100644 drivers/gpu/drm/bridge/cros-ec-anx7688.c
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 630072f3ab59..7e9df06bae19 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -50,6 +50,18 @@ config DRM_CHRONTEL_CH7033
If in doubt, say "N".
+config DRM_CROS_EC_ANX7688
+ tristate "ChromeOS EC ANX7688 bridge"
+ depends on OF
+ select DRM_KMS_HELPER
+ select REGMAP_I2C
+ help
+ ChromeOS EC ANX7688 is an ultra-low power
+ 4K Ultra-HD (4096x2160p60) mobile HD transmitter
+ designed for ChromeOS devices. It converts HDMI
+ 2.0 to DisplayPort 1.3 Ultra-HD. It is connected
+ to the ChromeOS Embedded Controller.
+
config DRM_DISPLAY_CONNECTOR
tristate "Display connector support"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index b017ec182b8f..ae27ef374ef3 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
obj-$(CONFIG_DRM_CHIPONE_ICN6211) += chipone-icn6211.o
obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
+obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o
diff --git a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
new file mode 100644
index ..0f6d907432e3
--- /dev/null
+++ b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * CrOS EC ANX7688 HDMI->DP bridge driver
+ *
+ * Copyright 2020 Google LLC
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Register addresses */
+#define ANX7688_VENDOR_ID_REG 0x00
+#define ANX7688_DEVICE_ID_REG 0x02
+
+#define ANX7688_FW_VERSION_REG 0x80
+
+#define ANX7688_DP_BANDWIDTH_REG 0x85
+#define ANX7688_DP_LANE_COUNT_REG 0x86
+
+#define ANX7688_VENDOR_ID 0x1f29
+#define ANX7688_DEVICE_ID 0x7688
+
+/* First supported firmware version (0.85) */
+#define ANX7688_MINIMUM_FW_VERSION 0x0085
+
+static const struct regmap_config cros_ec_anx7688_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+struct cros_ec_anx7688 {
+ struct i2c_client *client;
+ struct regmap *regmap;
+ struct drm_bridge bridge;
+ bool filter;
+};
+
+static inline struct cros_ec_anx7688 *
+bridge_to_cros_ec_anx7688(struct drm_bridge *bridge)
+{
+ return container_of(bridge, struct cros_ec_anx7688, bridge);
+}
+
+static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
+ const struct drm_display_mode
*mode,
+ struct drm_display_mode
*adjusted_mode)
+{
+ struct cros_ec_anx7688 *anx = bridge_to_cros_ec_anx7688(bridge);
+ int totalbw, requiredbw;
+ u8 dpbw, lanecount;
+ u8 regs[2];
+ int ret;
+
+ if (!anx->filter)
+ return true;
+
+ /* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
+ ret = regmap_bulk_read(anx->regmap, ANX7688_DP_BANDWIDTH_REG, regs, 2);
+ if (ret < 0) {
+ DRM_ERROR("Failed to read bandwidth/lane count\n");
+ return false;
+ }
+ dpbw = regs[0];
+ lanecount = regs[1];
+
+ /* Maximum 0x19 bandwidth (6.75 Gbps Turbo mode), 2 lanes */
+ if (dpbw > 0x19 || lanecount > 2) {
+ DRM_ERROR("Invalid bandwidth/lane count (%02x/%d)\n", dpbw,
+ lanecount);
+ return false;
+ }
+
+ /* Compute available bandwidth (kHz) */
+ totalbw = dpbw * lanecount * 27 * 8 / 10;
+
+ /* Required bandwidth (8 bpc,
Re: [RFC PATCH 0/3] A drm_plane API to support HDR planes
On 2021-05-18 3:56 a.m., Pekka Paalanen wrote: > On Mon, 17 May 2021 15:39:03 -0400 > Vitaly Prosyak wrote: > >> On 2021-05-17 12:48 p.m., Sebastian Wick wrote: >>> On 2021-05-17 10:57, Pekka Paalanen wrote: On Fri, 14 May 2021 17:05:11 -0400 Harry Wentland wrote: > On 2021-04-27 10:50 a.m., Pekka Paalanen wrote: >> On Mon, 26 Apr 2021 13:38:49 -0400 >> Harry Wentland wrote: ... >>> ## Mastering Luminances >>> >>> Now we are able to use the PQ 2084 EOTF to define the luminance of >>> pixels in absolute terms. Unfortunately we're again presented with >>> physical limitations of the display technologies on the market > today. >>> Here are a few examples of luminance ranges of displays. >>> >>> | Display | Luminance range in nits | >>> | | --- | >>> | Typical PC display | 0.3 - 200 | >>> | Excellent LCD HDTV | 0.3 - 400 | >>> | HDR LCD w/ local dimming | 0.05 - 1,500 | >>> >>> Since no display can currently show the full 0.0005 to 10,000 nits >>> luminance range the display will need to tonemap the HDR content, > i.e >>> to fit the content within a display's capabilities. To assist with >>> tonemapping HDR content is usually accompanied with a metadata that >>> describes (among other things) the minimum and maximum mastering >>> luminance, i.e. the maximum and minimum luminance of the display > that >>> was used to master the HDR content. >>> >>> The HDR metadata is currently defined on the drm_connector via the >>> hdr_output_metadata blob property. >>> >>> It might be useful to define per-plane hdr metadata, as different >>> planes might have been mastered differently. >> >> I don't think this would directly help with the dynamic range > blending >> problem. You still need to establish the mapping between the optical >> values from two different EOTFs and dynamic ranges. Or can you know >> which optical values match the mastering display maximum and minimum >> luminances for not-PQ? >> > > My understanding of this is probably best illustrated by this example: > > Assume HDR was mastered on a display with a maximum white level of 500 > nits and played back on a display that supports a max white level of > 400 > nits. If you know the mastering white level of 500 you know that > this is > the maximum value you need to compress down to 400 nits, allowing > you to > use the full extent of the 400 nits panel. Right, but in the kernel, where do you get these nits values from? hdr_output_metadata blob is infoframe data to the monitor. I think this should be independent of the metadata used for color transformations in the display pipeline before the monitor. EDID may tell us the monitor HDR metadata, but again what is used in the color transformations should be independent, because EDIDs lie, lighting environments change, and users have different preferences. What about black levels? Do you want to do black level adjustment? How exactly should the compression work? Where do you map the mid-tones? What if the end user wants something different? >>> >>> I suspect that this is not about tone mapping at all. The use cases >>> listed always have the display in PQ mode and just assume that no >>> content exceeds the PQ limitations. Then you can simply bring all >>> content to the color space with a matrix multiplication and then map the >>> linear light content somewhere into the PQ range. Tone mapping is >>> performed in the display only. > > The use cases do use the word "desktop" though. Harry, could you expand > on this, are you seeking a design that is good for generic desktop > compositors too, or one that is more tailored to "embedded" video > player systems taking the most advantage of (potentially > fixed-function) hardware? > The goal is to enable this on a generic desktop, such as generic Wayland implementations or ChromeOS. We're not looking for a custom solution for some embedded systems, though the solution we end up with should obviously not prevent an implementation on embedded video players. > What matrix would one choose? Which render intent would it > correspond to? > > If you need to adapt different dynamic ranges into the blending dynamic > range, would a simple linear transformation really be enough? > >>> From a generic wayland compositor point of view this is uninteresting. >>> >> It a compositor's decision to provide or not the metadata property to >> the kernel. The metadata can be available from one or multiple clients >> or most likely not available at all. >> >> Compositors may put a display in HDR10 ( when PQ 2084 INV EOTF and TM >>
Re: [PATCH v8 1/8] mm: Remove special swap entry functions
On Tue, May 18, 2021 at 09:58:09PM +1000, Alistair Popple wrote:
> On Tuesday, 18 May 2021 12:17:32 PM AEST Peter Xu wrote:
> > On Wed, Apr 07, 2021 at 06:42:31PM +1000, Alistair Popple wrote:
> > > +static inline struct page *pfn_swap_entry_to_page(swp_entry_t entry)
> > > +{
> > > + struct page *p = pfn_to_page(swp_offset(entry));
> > > +
> > > + /*
> > > + * Any use of migration entries may only occur while the
> > > + * corresponding page is locked
> > > + */
> > > + BUG_ON(is_migration_entry(entry) && !PageLocked(p));
> > > +
> > > + return p;
> > > +}
> >
> > Would swap_pfn_entry_to_page() be slightly better?
> >
> > The thing is it's very easy to read pfn_*() as a function to take a pfn as
> > parameter...
> >
> > Since I'm also recently working on some swap-related new ptes [1], I'm
> > thinking whether we could name these swap entries as "swap XXX entries".
> > Say, "swap hwpoison entry", "swap pfn entry" (which is a superset of "swap
> > migration entry", "swap device exclusive entry", ...). That's where I came
> > with the above swap_pfn_entry_to_page(), then below will be naturally
> > is_swap_pfn_entry().
>
> Equally though "hwpoison swap entry", "pfn swap entry", "migration swap
> entry", etc. also makes sense (at least to me), but does that not fit in as
> well with your series? I haven't looked too deeply at your series but have
> been meaning to so thanks for the pointer.
Yeah it looks good too. It's just to avoid starting with "pfn_" I guess, so at
least we know that's something related to swap not one specific pfn.
I found the naming is important as e.g. in my series I introduced another idea
called "swap special pte" or "special swap pte" (yeah so indeed my naming is
not that coherent as well... :), then I noticed if without a good naming I'm
afraid we can get lost easier.
I have a description here in the commit message re the new swap special pte:
https://lore.kernel.org/lkml/20210427161317.50682-5-pet...@redhat.com/
In which the uffd-wp marker pte will be the first swap special pte. Feel free
to ignore the details too, just want to mention some context, while it should
be orthogonal to this series.
I think yet-another swap entry suites for my case too but it'll be a waste as
in that pte I don't need to keep pfn information, but only a marker (one single
bit would suffice), so I chose one single pte value (one for each arch, I only
defined that on x86 in my series which is a special pte with only one bit set)
to not pollute the swap entry address spaces.
>
> > No strong opinion as this is already a v8 series (and sorry to chim in this
> > late), just to raise this up.
>
> No worries, it's good timing as I was about to send a v9 which was just a
> rebase anyway. I am hoping to try and get this accepted for the next merge
> window but I will wait before sending v9 to see if anyone else has thoughts
> on
> the naming here.
>
> I don't have a particularly strong opinion either, and your justification is
> more thought than I gave to naming these originally so am happy to rename if
> it's more readable or fits better with your series.
I'll leave the decision to you, especially in case you still prefer the old
naming. Or feel free to wait too until someone else shares the thoughts so as
to avoid unnecessary rebase work.
Thanks,
--
Peter Xu
Re: [PATCH v4 1/3] drm/hyperv: Add DRM driver for hyperv synthetic video device
> > +
> > +static struct pci_driver hyperv_pci_driver = {
> > + .name = KBUILD_MODNAME,
> > + .id_table = hyperv_pci_tbl,
> > + .probe =hyperv_pci_probe,
> > + .remove = hyperv_pci_remove,
> > +};
>
> The PCI code doesn't do anything. Do you need this for gen1 to work
> corretly. If so, there should at least be a short comment. Why don't
> you
> call hyperv_setup_gen1() in the PCI probe function?
>
>
Thank you very much Thomas for the review. Yes, this is needed for gen1
to work. Regarding why hyperv_setup_gen1() is not called from PCI
probe, this is to maintain consistency with hyperv_fb and also
hv_driver probe method is called irrespective of gen1 or gen2. I will
add a comment explaining this.
Deepak
Re: [PATCH v7 13/16] drm/scheduler: Fix hang when sched_entity released
In a separate discussion with Daniel we once more iterated over the
dma_resv requirements and I came to the conclusion that this approach
here won't work reliable.
The problem is as following:
1. device A schedules some rendering with into a buffer and exports it
as DMA-buf.
2. device B imports the DMA-buf and wants to consume the rendering, for
the the fence of device A is replaced with a new operation.
3. device B is hot plugged and the new operation canceled/newer scheduled.
The problem is now that we can't do this since the operation of device A
is still running and by signaling our fences we run into the problem of
potential memory corruption.
Not sure how to handle that case. One possibility would be to wait for
all dependencies of unscheduled jobs before signaling their fences as
canceled.
Christian.
Am 12.05.21 um 16:26 schrieb Andrey Grodzovsky:
Problem: If scheduler is already stopped by the time sched_entity
is released and entity's job_queue not empty I encountred
a hang in drm_sched_entity_flush. This is because drm_sched_entity_is_idle
never becomes false.
Fix: In drm_sched_fini detach all sched_entities from the
scheduler's run queues. This will satisfy drm_sched_entity_is_idle.
Also wakeup all those processes stuck in sched_entity flushing
as the scheduler main thread which wakes them up is stopped by now.
v2:
Reverse order of drm_sched_rq_remove_entity and marking
s_entity as stopped to prevent reinserion back to rq due
to race.
v3:
Drop drm_sched_rq_remove_entity, only modify entity->stopped
and check for it in drm_sched_entity_is_idle
Signed-off-by: Andrey Grodzovsky
Reviewed-by: Christian König
---
drivers/gpu/drm/scheduler/sched_entity.c | 3 ++-
drivers/gpu/drm/scheduler/sched_main.c | 24
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 0249c7450188..2e93e881b65f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -116,7 +116,8 @@ static bool drm_sched_entity_is_idle(struct
drm_sched_entity *entity)
rmb(); /* for list_empty to work without lock */
if (list_empty(>list) ||
- spsc_queue_count(>job_queue) == 0)
+ spsc_queue_count(>job_queue) == 0 ||
+ entity->stopped)
return true;
return false;
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 8d1211e87101..a2a953693b45 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -898,9 +898,33 @@ EXPORT_SYMBOL(drm_sched_init);
*/
void drm_sched_fini(struct drm_gpu_scheduler *sched)
{
+ struct drm_sched_entity *s_entity;
+ int i;
+
if (sched->thread)
kthread_stop(sched->thread);
+ for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
+ struct drm_sched_rq *rq = >sched_rq[i];
+
+ if (!rq)
+ continue;
+
+ spin_lock(>lock);
+ list_for_each_entry(s_entity, >entities, list)
+ /*
+* Prevents reinsertion and marks job_queue as idle,
+* it will removed from rq in drm_sched_entity_fini
+* eventually
+*/
+ s_entity->stopped = true;
+ spin_unlock(>lock);
+
+ }
+
+ /* Wakeup everyone stuck in drm_sched_entity_flush for this scheduler */
+ wake_up_all(>job_scheduled);
+
/* Confirm no work left behind accessing device structures */
cancel_delayed_work_sync(>work_tdr);
Re: [PATCH] drm/amdgpu: Unmap all MMIO mappings
[Public]
Reviewed-by: Alex Deucher
From: Grodzovsky, Andrey
Sent: Tuesday, May 18, 2021 10:01 AM
To: dri-devel@lists.freedesktop.org ;
amd-...@lists.freedesktop.org ;
linux-...@vger.kernel.org ;
ckoenig.leichtzumer...@gmail.com ;
daniel.vet...@ffwll.ch ; Wentland, Harry
Cc: ppaala...@gmail.com ; Deucher, Alexander
; gre...@linuxfoundation.org
; helg...@kernel.org ;
Kuehling, Felix
Subject: Re: [PATCH] drm/amdgpu: Unmap all MMIO mappings
Ping
Andrey
On 2021-05-17 3:31 p.m., Andrey Grodzovsky wrote:
> Access to those must be prevented post pci_remove
>
> v6: Drop BOs list, unampping VRAM BAR is enough.
> v8:
> Add condition of xgmi.connected_to_cpu to MTTR
> handling and remove MTTR handling from the old place.
>
> Signed-off-by: Andrey Grodzovsky
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 26 +++---
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4
> 3 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index f7cca25c0fa0..8b50315d1fe1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3666,6 +3666,27 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>return r;
> }
>
> +static void amdgpu_device_unmap_mmio(struct amdgpu_device *adev)
> +{
> + /* Clear all CPU mappings pointing to this device */
> + unmap_mapping_range(adev->ddev.anon_inode->i_mapping, 0, 0, 1);
> +
> + /* Unmap all mapped bars - Doorbell, registers and VRAM */
> + amdgpu_device_doorbell_fini(adev);
> +
> + iounmap(adev->rmmio);
> + adev->rmmio = NULL;
> + if (adev->mman.aper_base_kaddr)
> + iounmap(adev->mman.aper_base_kaddr);
> + adev->mman.aper_base_kaddr = NULL;
> +
> + /* Memory manager related */
> + if (!adev->gmc.xgmi.connected_to_cpu) {
> + arch_phys_wc_del(adev->gmc.vram_mtrr);
> + arch_io_free_memtype_wc(adev->gmc.aper_base,
> adev->gmc.aper_size);
> + }
> +}
> +
> /**
>* amdgpu_device_fini - tear down the driver
>*
> @@ -3712,6 +3733,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>amdgpu_device_ip_fini_early(adev);
>
>amdgpu_gart_dummy_page_fini(adev);
> +
> + amdgpu_device_unmap_mmio(adev);
> }
>
> void amdgpu_device_fini_sw(struct amdgpu_device *adev)
> @@ -3739,9 +3762,6 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>}
>if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>vga_client_register(adev->pdev, NULL, NULL, NULL);
> - iounmap(adev->rmmio);
> - adev->rmmio = NULL;
> - amdgpu_device_doorbell_fini(adev);
>
>if (IS_ENABLED(CONFIG_PERF_EVENTS))
>amdgpu_pmu_fini(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 0adffcace326..8eabe3c9ad17 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1107,10 +1107,6 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
> void amdgpu_bo_fini(struct amdgpu_device *adev)
> {
>amdgpu_ttm_fini(adev);
> - if (!adev->gmc.xgmi.connected_to_cpu) {
> - arch_phys_wc_del(adev->gmc.vram_mtrr);
> - arch_io_free_memtype_wc(adev->gmc.aper_base,
> adev->gmc.aper_size);
> - }
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 0d54e70278ca..58ad2fecc9e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -1841,10 +1841,6 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
>amdgpu_bo_free_kernel(>mman.discovery_memory, NULL, NULL);
>amdgpu_ttm_fw_reserve_vram_fini(adev);
>
> - if (adev->mman.aper_base_kaddr)
> - iounmap(adev->mman.aper_base_kaddr);
> - adev->mman.aper_base_kaddr = NULL;
> -
>amdgpu_vram_mgr_fini(adev);
>amdgpu_gtt_mgr_fini(adev);
>ttm_range_man_fini(>mman.bdev, AMDGPU_PL_GDS);
>
Re: [PATCH] drm/amdgpu: Unmap all MMIO mappings
Ping
Andrey
On 2021-05-17 3:31 p.m., Andrey Grodzovsky wrote:
Access to those must be prevented post pci_remove
v6: Drop BOs list, unampping VRAM BAR is enough.
v8:
Add condition of xgmi.connected_to_cpu to MTTR
handling and remove MTTR handling from the old place.
Signed-off-by: Andrey Grodzovsky
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 26 +++---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4
3 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index f7cca25c0fa0..8b50315d1fe1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3666,6 +3666,27 @@ int amdgpu_device_init(struct amdgpu_device *adev,
return r;
}
+static void amdgpu_device_unmap_mmio(struct amdgpu_device *adev)
+{
+ /* Clear all CPU mappings pointing to this device */
+ unmap_mapping_range(adev->ddev.anon_inode->i_mapping, 0, 0, 1);
+
+ /* Unmap all mapped bars - Doorbell, registers and VRAM */
+ amdgpu_device_doorbell_fini(adev);
+
+ iounmap(adev->rmmio);
+ adev->rmmio = NULL;
+ if (adev->mman.aper_base_kaddr)
+ iounmap(adev->mman.aper_base_kaddr);
+ adev->mman.aper_base_kaddr = NULL;
+
+ /* Memory manager related */
+ if (!adev->gmc.xgmi.connected_to_cpu) {
+ arch_phys_wc_del(adev->gmc.vram_mtrr);
+ arch_io_free_memtype_wc(adev->gmc.aper_base,
adev->gmc.aper_size);
+ }
+}
+
/**
* amdgpu_device_fini - tear down the driver
*
@@ -3712,6 +3733,8 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
amdgpu_device_ip_fini_early(adev);
amdgpu_gart_dummy_page_fini(adev);
+
+ amdgpu_device_unmap_mmio(adev);
}
void amdgpu_device_fini_sw(struct amdgpu_device *adev)
@@ -3739,9 +3762,6 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
}
if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
vga_client_register(adev->pdev, NULL, NULL, NULL);
- iounmap(adev->rmmio);
- adev->rmmio = NULL;
- amdgpu_device_doorbell_fini(adev);
if (IS_ENABLED(CONFIG_PERF_EVENTS))
amdgpu_pmu_fini(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 0adffcace326..8eabe3c9ad17 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1107,10 +1107,6 @@ int amdgpu_bo_init(struct amdgpu_device *adev)
void amdgpu_bo_fini(struct amdgpu_device *adev)
{
amdgpu_ttm_fini(adev);
- if (!adev->gmc.xgmi.connected_to_cpu) {
- arch_phys_wc_del(adev->gmc.vram_mtrr);
- arch_io_free_memtype_wc(adev->gmc.aper_base,
adev->gmc.aper_size);
- }
}
/**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 0d54e70278ca..58ad2fecc9e3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1841,10 +1841,6 @@ void amdgpu_ttm_fini(struct amdgpu_device *adev)
amdgpu_bo_free_kernel(>mman.discovery_memory, NULL, NULL);
amdgpu_ttm_fw_reserve_vram_fini(adev);
- if (adev->mman.aper_base_kaddr)
- iounmap(adev->mman.aper_base_kaddr);
- adev->mman.aper_base_kaddr = NULL;
-
amdgpu_vram_mgr_fini(adev);
amdgpu_gtt_mgr_fini(adev);
ttm_range_man_fini(>mman.bdev, AMDGPU_PL_GDS);
Re: [PATCH v7 02/10] dt-bindings: display: simple: List hpd properties in panel-simple
Hi, On Tue, May 18, 2021 at 5:42 AM Rob Herring wrote: > > On Mon, May 17, 2021 at 3:09 PM Douglas Anderson > wrote: > > > > These are described in panel-common.yaml but if I don't list them in > > panel-simple then I get yells when running 'dt_binding_check' in a > > future patch. List them along with other properties that seem to be > > listed in panel-simple for similar reasons. > > If you have HPD, is it still a simple panel? I don't see this as an > omission because the use of these properties for simple panels was > never documented IIRC. I would say so. It is currently supported by panel-simple in Linux. Of course, you could make the argument that panel-simple is no longer really "simple" because of things like this... I guess I'd say this: I believe that the HPD properties eventually belong in the generic "edp-panel" that I'm still planning to post (and which will be based on this series). I justified that previously [1] by talking about the fact that there's a single timing diagram that (as far as I've been able to tell) is fairly universal in panel specs. It's a complicated timing diagram showing some two dozen timings (and includes HPD!), but if you support all the timings then you've supported pretty much all panels. IMO the original intent of "simple-panel" was to specify a panel that's just like all the other panels w/ a few parameters. NOTE: I'd also say that for nearly all eDP panels HPD is important, but in many designs HPD is handled "magically" and not specified in the device tree. This is because it goes to a dedicated location on the eDP controller / bridge chip. I added the HPD GPIO support (and no-hpd) to simple-panel because my bridge chip has a fairly useless HPD line and we don't use it. Even though the fact that we need the HPD specified like this is a function of our bridge chip, back in the day I was told that the property belonged in the panel and so that's where it lives. > Not saying we can't add them, but justify it as an addition, not just > fixing a warning. Sure, I'll beef up the commit message. > > Signed-off-by: Douglas Anderson > > --- > > I didn't spend tons of time digging to see if there was supposed to be > > a better way of doing this. If there is, feel free to yell. > > That's the right way to do it unless you want to allow all common > properties, then we'd use unevaluatedProperties instead of > additionalProperties. Ah, perfect. Thanks! [1] https://lore.kernel.org/r/CAD=FV=vzyompwqzzwdhjgh5cjjww_ecm-wqveivz-bdgxjp...@mail.gmail.com/ -Doug
Re: [RFC] Add DMA_RESV_USAGE flags
Am 18.05.21 um 15:26 schrieb Daniel Stone: On Tue, 18 May 2021 at 13:49, Christian König wrote: Am 18.05.21 um 07:59 schrieb Daniel Vetter: First step in fixing that is (and frankly was since years) to fix the amdgpu CS so winsys can pass along a bunch of flags about which CS should actually set the exclusive fence, so that you stop oversyncing so badly. Ofc old userspace needs to keep oversyncing forever, no way to fix that. Exactly that is what we don't want to do because the winsys has no idea when to sync and when not to sync. Hey, we're typing that out as fast as we can ... it's just that you keep reinventing sync primitives faster than we can ship support for them :P You can stop typing. We will even need that for backward compatibility. But yeah, not reinventing sync_file support with drm_syncobj would have helped :) Cheers, Christian.
Re: [PATCH AUTOSEL 5.12 5/5] tty: vt: always invoke vc->vc_sw->con_resize callback
On Tue, May 18, 2021 at 09:22:48AM -0400, Sasha Levin wrote: > On Tue, May 18, 2021 at 07:45:59AM +0200, Greg KH wrote: > > On Mon, May 17, 2021 at 06:35:24PM -0700, Linus Torvalds wrote: > > > On Mon, May 17, 2021 at 6:09 PM Sasha Levin wrote: > > > > > > > > From: Tetsuo Handa > > > > > > > > [ Upstream commit ffb324e6f874121f7dce5bdae5e05d02baae7269 ] > > > > > > So I think the commit is fine, and yes, it should be applied to > > > stable, but it's one of those "there were three different patches in > > > as many days to fix the problem, and this is the right one, but maybe > > > stable should hold off for a while to see that there aren't any > > > problem reports". > > > > > > I don't think there will be any problems from this, but while the > > > patch is tiny, it's conceptually quite a big change to something that > > > people haven't really touched for a long time. > > > > > > So use your own judgement, but it might be a good idea to wait a week > > > before backporting this to see if anything screams. > > > > I was going to wait a few weeks for this, and the other vt patches that > > were marked with cc: stable@ before queueing them up. > > I'll drop it from my queue then. Thanks!
