RE: [PATCH] drm/amdgpu: update GC golden setting for dimgrey_cavefish
[AMD Official Use Only - Internal Distribution Only] Reviewed-by: Jiansong Chen -Original Message- From: Zhou1, Tao Sent: Friday, November 27, 2020 12:28 PM To: Chen, Jiansong (Simon) ; Gui, Jack ; Zhang, Hawking ; amd-gfx@lists.freedesktop.org Cc: Zhou1, Tao Subject: [PATCH] drm/amdgpu: update GC golden setting for dimgrey_cavefish Update GC golden setting for dimgrey_cavefish. Signed-off-by: Tao Zhou --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 841d39eb62d9..ffbda6680a68 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -3266,6 +3266,7 @@ static const struct soc15_reg_golden golden_settings_gc_10_3_vangogh[] = static const struct soc15_reg_golden golden_settings_gc_10_3_4[] = { +SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_CS_CLK_CTRL, 0x7800, +0x78000100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_RA0_CLK_CTRL, 0x3000, 0x3100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_RA1_CLK_CTRL, 0x7e00, 0x7e000100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCPF_GCR_CNTL, 0x0007, 0xc000), -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: update GC golden setting for dimgrey_cavefish
Update GC golden setting for dimgrey_cavefish. Signed-off-by: Tao Zhou --- drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c index 841d39eb62d9..ffbda6680a68 100644 --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c @@ -3266,6 +3266,7 @@ static const struct soc15_reg_golden golden_settings_gc_10_3_vangogh[] = static const struct soc15_reg_golden golden_settings_gc_10_3_4[] = { + SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_CS_CLK_CTRL, 0x7800, 0x78000100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_RA0_CLK_CTRL, 0x3000, 0x3100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCGTT_SPI_RA1_CLK_CTRL, 0x7e00, 0x7e000100), SOC15_REG_GOLDEN_VALUE(GC, 0, mmCPF_GCR_CNTL, 0x0007, 0xc000), -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Extends Tune min clk for MPO for RV
Friendly ping. Please help review. Regards Pratik On 25/11/20 10:02 am, Pratik Vishwakarma wrote: On 25/11/20 1:38 am, Harry Wentland wrote: On 2020-11-24 10:55 a.m., Pratik Vishwakarma wrote: [Why] Changes in video resolution during playback cause dispclk to ramp higher but sets incompatile fclk and dcfclk values for MPO. [How] Check for MPO and set proper min clk values for this case also. This was missed during previous patch. Signed-off-by: Pratik Vishwakarma --- .../display/dc/clk_mgr/dcn10/rv1_clk_mgr.c | 19 --- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c index 75b8240ed059..ed087a9e73bb 100644 --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn10/rv1_clk_mgr.c @@ -275,9 +275,22 @@ static void rv1_update_clocks(struct clk_mgr *clk_mgr_base, if (pp_smu->set_hard_min_fclk_by_freq && pp_smu->set_hard_min_dcfclk_by_freq && pp_smu->set_min_deep_sleep_dcfclk) { - pp_smu->set_hard_min_fclk_by_freq(_smu->pp_smu, new_clocks->fclk_khz / 1000); - pp_smu->set_hard_min_dcfclk_by_freq(_smu->pp_smu, new_clocks->dcfclk_khz / 1000); - pp_smu->set_min_deep_sleep_dcfclk(_smu->pp_smu, (new_clocks->dcfclk_deep_sleep_khz + 999) / 1000); + // Only increase clocks when display is active and MPO is enabled Why do we want to only do this when MPO is enabled? Harry Hi Harry, When MPO is enabled and system moves to lower clock state, clock values are not sufficient and we see flash lines across entire screen. This issue is not observed when MPO is disabled or not active. Regards, Pratik + if (display_count && is_mpo_enabled(context)) { + pp_smu->set_hard_min_fclk_by_freq(_smu->pp_smu, + ((new_clocks->fclk_khz / 1000) * 101) / 100); + pp_smu->set_hard_min_dcfclk_by_freq(_smu->pp_smu, + ((new_clocks->dcfclk_khz / 1000) * 101) / 100); + pp_smu->set_min_deep_sleep_dcfclk(_smu->pp_smu, + (new_clocks->dcfclk_deep_sleep_khz + 999) / 1000); + } else { + pp_smu->set_hard_min_fclk_by_freq(_smu->pp_smu, + new_clocks->fclk_khz / 1000); + pp_smu->set_hard_min_dcfclk_by_freq(_smu->pp_smu, + new_clocks->dcfclk_khz / 1000); + pp_smu->set_min_deep_sleep_dcfclk(_smu->pp_smu, + (new_clocks->dcfclk_deep_sleep_khz + 999) / 1000); + } } } ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [PATCH] drm/amdgpu: skip power profile switch in sriov
[AMD Official Use Only - Internal Distribution Only] Acked-by: Evan Quan -Original Message- From: amd-gfx On Behalf Of Jingwen Chen Sent: Thursday, November 26, 2020 4:38 PM To: amd-gfx@lists.freedesktop.org; Zhao, Jiange Cc: Chen, JingWen Subject: [PATCH] drm/amdgpu: skip power profile switch in sriov power profile switch in vcn need to send SetWorkLoad msg to smu, which is not supported in sriov. Signed-off-by: Jingwen Chen --- drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c index 17a45baff638..8fb12afe3c96 100644 --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c @@ -1168,6 +1168,9 @@ int amdgpu_dpm_switch_power_profile(struct amdgpu_device *adev, { int ret = 0; +if (amdgpu_sriov_vf(adev)) +return 0; + if (is_support_sw_smu(adev)) ret = smu_switch_power_profile(>smu, type, en); else if (adev->powerplay.pp_funcs && -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cevan.quan%40amd.com%7Ca24d4843e0a84a161aaa08d891e69af0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637419766916626849%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=uJk3bchf%2Fen1BTWQXiCN4Dcb3iRR%2FdZ%2FqwQ9%2Fgebo%2BQ%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v3] drm/amd/display: turn DPMS off on mst connector unplug
[Why] Set dpms off on the MST connector that was unplugged, for the side effect of releasing some references held through deallocation of mst payload. Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 31 ++- drivers/gpu/drm/amd/display/dc/core/dc.c | 13 drivers/gpu/drm/amd/display/dc/dc_stream.h| 1 + 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index e213246e3f04..9966679d29e7 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1984,6 +1984,32 @@ static void dm_gpureset_commit_state(struct dc_state *dc_state, return; } +static void dm_set_dpms_off(struct dc_link *link) +{ + struct dc_stream_state *stream_state; + struct amdgpu_dm_connector *aconnector = link->priv; + struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev); + struct dc_stream_update stream_update = {0}; + bool dpms_off = true; + + stream_update.dpms_off = _off; + + mutex_lock(>dm.dc_lock); + stream_state = dc_stream_find_from_link(link); + + if (stream_state == NULL) { + dm_error("Error finding stream state associated with link!\n"); + mutex_unlock(>dm.dc_lock); + return; + } + + stream_update.stream = stream_state; + dc_commit_updates_for_stream(stream_state->ctx->dc, NULL, 0, +stream_state, _update, +stream_state->ctx->dc->current_state); + mutex_unlock(>dm.dc_lock); +} + static int dm_resume(void *handle) { struct amdgpu_device *adev = handle; @@ -2434,8 +2460,11 @@ static void handle_hpd_irq(void *param) drm_kms_helper_hotplug_event(dev); } else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { - amdgpu_dm_update_connector_after_detect(aconnector); + if (new_connection_type == dc_connection_none && + aconnector->dc_link->type == dc_connection_none) + dm_set_dpms_off(aconnector->dc_link); + amdgpu_dm_update_connector_after_detect(aconnector); drm_modeset_lock_all(dev); dm_restore_drm_connector_state(dev, connector); diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 903353389edb..58eb0d69873a 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2798,6 +2798,19 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i) return NULL; } +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link) +{ + uint8_t i; + struct dc_context *ctx = link->ctx; + + for (i = 0; i < ctx->dc->current_state->stream_count; i++) { + if (ctx->dc->current_state->streams[i]->link == link) + return ctx->dc->current_state->streams[i]; + } + + return NULL; +} + enum dc_irq_source dc_interrupt_to_irq_source( struct dc *dc, uint32_t src_id, diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h index bf090afc2f70..b7910976b81a 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h @@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct dc_stream_state *stream); uint8_t dc_get_current_stream_count(struct dc *dc); struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i); +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link); /* * Return the current frame counter. -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2] drm/amd/display: turn DPMS off on mst connector unplug
On 2020-11-26 4:45 p.m., Aurabindo Pillai wrote: [Why] Set dpms off on the MST connector that was unplugged, for the side effect of releasing some references held through deallocation of mst payload. Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++- drivers/gpu/drm/amd/display/dc/core/dc.c | 17 ++ drivers/gpu/drm/amd/display/dc/dc_stream.h| 1 + 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index e213246e3f04..6203cbf3ee33 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1984,6 +1984,34 @@ static void dm_gpureset_commit_state(struct dc_state *dc_state, return; } +static void dm_set_dpms_off(struct dc_link *link) +{ + struct dc_stream_state *stream_state; + struct amdgpu_dm_connector *aconnector = link->priv; + struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev); + struct { + struct dc_surface_update surface_updates[MAX_SURFACES]; Let's remove the bundle and drop the surface_updates here. A surface_count of 0 should be sufficient to guard against NULL surface_updates array. + struct dc_stream_update stream_update; + } bundle = {0}; + bool dpms_off = true; + + bundle.stream_update.dpms_off = _off; + + mutex_lock(>dm.dc_lock); + stream_state = dc_stream_find_from_link(link); + mutex_unlock(>dm.dc_lock); This needs to be move under dc_commit_updates_for_stream(). It's not safe to call dc_commit_updates_for_stream while unlocked since it modifies global state. + + if (stream_state == NULL) { + dm_error("Error finding stream state associated with link!\n"); + return; + } + + bundle.stream_update.stream = stream_state; + dc_commit_updates_for_stream(stream_state->ctx->dc, bundle.surface_updates, 0, +stream_state, _update, +stream_state->ctx->dc->current_state); +} + static int dm_resume(void *handle) { struct amdgpu_device *adev = handle; @@ -2434,8 +2462,11 @@ static void handle_hpd_irq(void *param) drm_kms_helper_hotplug_event(dev); } else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { - amdgpu_dm_update_connector_after_detect(aconnector); + if (new_connection_type == dc_connection_none && + aconnector->dc_link->type == dc_connection_none) + dm_set_dpms_off(aconnector->dc_link); + amdgpu_dm_update_connector_after_detect(aconnector); drm_modeset_lock_all(dev); dm_restore_drm_connector_state(dev, connector); diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 903353389edb..7a2b2802faa2 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2798,6 +2798,23 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i) return NULL; } +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link) +{ + uint8_t i; + struct dc_context *ctx = link->ctx; + + for (i = 0; i< MAX_PIPES; i++) { + if (ctx->dc->current_state->streams[i] == NULL) + continue; Drop the NULL check above and change MAX_PIPES to dc->current_state->stream_count. Regards, Nicholas Kazlauskas + + if (ctx->dc->current_state->streams[i]->link == link) { + return ctx->dc->current_state->streams[i]; + } + } + + return NULL; +} + enum dc_irq_source dc_interrupt_to_irq_source( struct dc *dc, uint32_t src_id, diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h index bf090afc2f70..b7910976b81a 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h @@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct dc_stream_state *stream); uint8_t dc_get_current_stream_count(struct dc *dc); struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i); +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link); /* * Return the current frame counter. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: turn DPMS off on mst connector unplug
On 2020-11-26 3:11 p.m., Kazlauskas, Nicholas wrote: > On 2020-11-26 2:50 p.m., Aurabindo Pillai wrote: >> [Why] >> >> Set dpms off on the MST connector that was unplugged, for the side >> effect of >> releasing some references held through deallocation of mst payload. >> >> Signed-off-by: Aurabindo Pillai >> Signed-off-by: Eryk Brol >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++- >> 1 file changed, 62 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index e213246e3f04..fc984cf6e316 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct >> dc_state *dc_state, >> return; >> } >> +static void dm_set_dpms_off(struct dc_link *link) >> +{ >> + struct { >> + struct dc_surface_update surface_updates[MAX_SURFACES]; >> + struct dc_stream_update stream_update; >> + } * bundle; >> + struct dc_stream_state *stream_state; >> + struct dc_context *dc_ctx = link->ctx; >> + int i; >> + >> + bundle = kzalloc(sizeof(*bundle), GFP_KERNEL); >> + >> + if (!bundle) { >> + dm_error("Failed to allocate update bundle\n"); >> + return; >> + } >> + >> + bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL); > > You don't need to allocate memory for the bool. You can just use a local > here, DC doesn't need it after the call ends. > > I think the same should apply to the surface updates as well. I'm not > entirely sure how much stack the bundle takes up for a single stream > here but it should be small enough that we can leave it on the stack. > >> + >> + if (!bundle->stream_update.dpms_off) { >> + dm_error("Failed to allocate update bundle\n"); >> + goto cleanup; >> + } >> + >> + *bundle->stream_update.dpms_off = true; >> + >> + for (i = 0; i< MAX_PIPES; i++) { >> + if (dc_ctx->dc->current_state->streams[i] == NULL) >> + continue; >> + >> + if (dc_ctx->dc->current_state->streams[i]->link == link) { >> + stream_state = dc_ctx->dc->current_state->streams[i]; >> + goto link_found; >> + } >> + } > > We shouldn't be reading from dc->current_state directly in DM, it's > essentially private state. > > I think we should actually have a new helper here in dc_stream.h that's > like: > > struct dc_stream_state *dc_stream_find_from_link(const struct dc_link > *link); > > to replace this block of code. > > Note that any time we touch current_state we also need to be locking - > it looks like this function is missing the appropriate calls to: > > mutex_lock(>dm.dc_lock); > mutex_unlock(>dm.dc_lock); > > Regards, > Nicholas Kazlauskas > > Thank you for the review. Sent a v2 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH v2] drm/amd/display: turn DPMS off on mst connector unplug
[Why] Set dpms off on the MST connector that was unplugged, for the side effect of releasing some references held through deallocation of mst payload. Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 ++- drivers/gpu/drm/amd/display/dc/core/dc.c | 17 ++ drivers/gpu/drm/amd/display/dc/dc_stream.h| 1 + 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index e213246e3f04..6203cbf3ee33 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1984,6 +1984,34 @@ static void dm_gpureset_commit_state(struct dc_state *dc_state, return; } +static void dm_set_dpms_off(struct dc_link *link) +{ + struct dc_stream_state *stream_state; + struct amdgpu_dm_connector *aconnector = link->priv; + struct amdgpu_device *adev = drm_to_adev(aconnector->base.dev); + struct { + struct dc_surface_update surface_updates[MAX_SURFACES]; + struct dc_stream_update stream_update; + } bundle = {0}; + bool dpms_off = true; + + bundle.stream_update.dpms_off = _off; + + mutex_lock(>dm.dc_lock); + stream_state = dc_stream_find_from_link(link); + mutex_unlock(>dm.dc_lock); + + if (stream_state == NULL) { + dm_error("Error finding stream state associated with link!\n"); + return; + } + + bundle.stream_update.stream = stream_state; + dc_commit_updates_for_stream(stream_state->ctx->dc, bundle.surface_updates, 0, +stream_state, _update, +stream_state->ctx->dc->current_state); +} + static int dm_resume(void *handle) { struct amdgpu_device *adev = handle; @@ -2434,8 +2462,11 @@ static void handle_hpd_irq(void *param) drm_kms_helper_hotplug_event(dev); } else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { - amdgpu_dm_update_connector_after_detect(aconnector); + if (new_connection_type == dc_connection_none && + aconnector->dc_link->type == dc_connection_none) + dm_set_dpms_off(aconnector->dc_link); + amdgpu_dm_update_connector_after_detect(aconnector); drm_modeset_lock_all(dev); dm_restore_drm_connector_state(dev, connector); diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c b/drivers/gpu/drm/amd/display/dc/core/dc.c index 903353389edb..7a2b2802faa2 100644 --- a/drivers/gpu/drm/amd/display/dc/core/dc.c +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c @@ -2798,6 +2798,23 @@ struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i) return NULL; } +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link) +{ + uint8_t i; + struct dc_context *ctx = link->ctx; + + for (i = 0; i< MAX_PIPES; i++) { + if (ctx->dc->current_state->streams[i] == NULL) + continue; + + if (ctx->dc->current_state->streams[i]->link == link) { + return ctx->dc->current_state->streams[i]; + } + } + + return NULL; +} + enum dc_irq_source dc_interrupt_to_irq_source( struct dc *dc, uint32_t src_id, diff --git a/drivers/gpu/drm/amd/display/dc/dc_stream.h b/drivers/gpu/drm/amd/display/dc/dc_stream.h index bf090afc2f70..b7910976b81a 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_stream.h +++ b/drivers/gpu/drm/amd/display/dc/dc_stream.h @@ -292,6 +292,7 @@ void dc_stream_log(const struct dc *dc, const struct dc_stream_state *stream); uint8_t dc_get_current_stream_count(struct dc *dc); struct dc_stream_state *dc_get_stream_at_index(struct dc *dc, uint8_t i); +struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link); /* * Return the current frame counter. -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: turn DPMS off on mst connector unplug
On 2020-11-26 2:50 p.m., Aurabindo Pillai wrote: [Why] Set dpms off on the MST connector that was unplugged, for the side effect of releasing some references held through deallocation of mst payload. Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index e213246e3f04..fc984cf6e316 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct dc_state *dc_state, return; } +static void dm_set_dpms_off(struct dc_link *link) +{ + struct { + struct dc_surface_update surface_updates[MAX_SURFACES]; + struct dc_stream_update stream_update; + } * bundle; + struct dc_stream_state *stream_state; + struct dc_context *dc_ctx = link->ctx; + int i; + + bundle = kzalloc(sizeof(*bundle), GFP_KERNEL); + + if (!bundle) { + dm_error("Failed to allocate update bundle\n"); + return; + } + + bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL); You don't need to allocate memory for the bool. You can just use a local here, DC doesn't need it after the call ends. I think the same should apply to the surface updates as well. I'm not entirely sure how much stack the bundle takes up for a single stream here but it should be small enough that we can leave it on the stack. + + if (!bundle->stream_update.dpms_off) { + dm_error("Failed to allocate update bundle\n"); + goto cleanup; + } + + *bundle->stream_update.dpms_off = true; + + for (i = 0; i< MAX_PIPES; i++) { + if (dc_ctx->dc->current_state->streams[i] == NULL) + continue; + + if (dc_ctx->dc->current_state->streams[i]->link == link) { + stream_state = dc_ctx->dc->current_state->streams[i]; + goto link_found; + } + } We shouldn't be reading from dc->current_state directly in DM, it's essentially private state. I think we should actually have a new helper here in dc_stream.h that's like: struct dc_stream_state *dc_stream_find_from_link(const struct dc_link *link); to replace this block of code. Note that any time we touch current_state we also need to be locking - it looks like this function is missing the appropriate calls to: mutex_lock(>dm.dc_lock); mutex_unlock(>dm.dc_lock); Regards, Nicholas Kazlauskas + + dm_error("Cannot find link associated with the stream to be disabled\n"); + return; + +link_found: + if (stream_state == NULL) { + dm_error("Stream state associated with the link is NULL\n"); + return; + } + + bundle->stream_update.stream = stream_state; + + dc_commit_updates_for_stream(stream_state->ctx->dc, bundle->surface_updates, 0, +stream_state, >stream_update, +stream_state->ctx->dc->current_state); + + kfree(bundle->stream_update.dpms_off); +cleanup: + kfree(bundle); + + return; +} + static int dm_resume(void *handle) { struct amdgpu_device *adev = handle; @@ -2434,8 +2492,11 @@ static void handle_hpd_irq(void *param) drm_kms_helper_hotplug_event(dev); } else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { - amdgpu_dm_update_connector_after_detect(aconnector); + if (new_connection_type == dc_connection_none && + aconnector->dc_link->type == dc_connection_none) + dm_set_dpms_off(aconnector->dc_link); + amdgpu_dm_update_connector_after_detect(aconnector); drm_modeset_lock_all(dev); dm_restore_drm_connector_state(dev, connector); ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: turn DPMS off on mst connector unplug
[Why] Set dpms off on the MST connector that was unplugged, for the side effect of releasing some references held through deallocation of mst payload. Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 63 ++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index e213246e3f04..fc984cf6e316 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -1984,6 +1984,64 @@ static void dm_gpureset_commit_state(struct dc_state *dc_state, return; } +static void dm_set_dpms_off(struct dc_link *link) +{ + struct { + struct dc_surface_update surface_updates[MAX_SURFACES]; + struct dc_stream_update stream_update; + } * bundle; + struct dc_stream_state *stream_state; + struct dc_context *dc_ctx = link->ctx; + int i; + + bundle = kzalloc(sizeof(*bundle), GFP_KERNEL); + + if (!bundle) { + dm_error("Failed to allocate update bundle\n"); + return; + } + + bundle->stream_update.dpms_off = kzalloc(sizeof(bool), GFP_KERNEL); + + if (!bundle->stream_update.dpms_off) { + dm_error("Failed to allocate update bundle\n"); + goto cleanup; + } + + *bundle->stream_update.dpms_off = true; + + for (i = 0; i< MAX_PIPES; i++) { + if (dc_ctx->dc->current_state->streams[i] == NULL) + continue; + + if (dc_ctx->dc->current_state->streams[i]->link == link) { + stream_state = dc_ctx->dc->current_state->streams[i]; + goto link_found; + } + } + + dm_error("Cannot find link associated with the stream to be disabled\n"); + return; + +link_found: + if (stream_state == NULL) { + dm_error("Stream state associated with the link is NULL\n"); + return; + } + + bundle->stream_update.stream = stream_state; + + dc_commit_updates_for_stream(stream_state->ctx->dc, bundle->surface_updates, 0, +stream_state, >stream_update, +stream_state->ctx->dc->current_state); + + kfree(bundle->stream_update.dpms_off); +cleanup: + kfree(bundle); + + return; +} + static int dm_resume(void *handle) { struct amdgpu_device *adev = handle; @@ -2434,8 +2492,11 @@ static void handle_hpd_irq(void *param) drm_kms_helper_hotplug_event(dev); } else if (dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD)) { - amdgpu_dm_update_connector_after_detect(aconnector); + if (new_connection_type == dc_connection_none && + aconnector->dc_link->type == dc_connection_none) + dm_set_dpms_off(aconnector->dc_link); + amdgpu_dm_update_connector_after_detect(aconnector); drm_modeset_lock_all(dev); dm_restore_drm_connector_state(dev, connector); -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: fix check order in radeon_bo_move
On Wed, Nov 25, 2020 at 3:34 PM Christian König wrote: > > Reorder the code to fix checking if blitting is available. Might be good to explain why blitting might not be available, e.g. suspend/resume and or chip death and stuff like that. > Signed-off-by: Christian König Needs Fixes: 28a68f828266 ("drm/radeon/ttm: use multihop") Btw $ dim fixes [sha1] generates that for you plus nice cc list of offenders. With the Fixes line added: Reviewed-by: Daniel Vetter At least I'm hanging onto the illusion that I understand what you did here :-) -Daniel > --- > drivers/gpu/drm/radeon/radeon_ttm.c | 54 + > 1 file changed, 24 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c > b/drivers/gpu/drm/radeon/radeon_ttm.c > index 0ca381b95d3d..2b598141225f 100644 > --- a/drivers/gpu/drm/radeon/radeon_ttm.c > +++ b/drivers/gpu/drm/radeon/radeon_ttm.c > @@ -216,27 +216,15 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, > bool evict, > struct ttm_resource *old_mem = >mem; > int r; > > - if ((old_mem->mem_type == TTM_PL_SYSTEM && > -new_mem->mem_type == TTM_PL_VRAM) || > - (old_mem->mem_type == TTM_PL_VRAM && > -new_mem->mem_type == TTM_PL_SYSTEM)) { > - hop->fpfn = 0; > - hop->lpfn = 0; > - hop->mem_type = TTM_PL_TT; > - hop->flags = 0; > - return -EMULTIHOP; > - } > - > if (new_mem->mem_type == TTM_PL_TT) { > r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); > if (r) > return r; > } > - radeon_bo_move_notify(bo, evict, new_mem); > > r = ttm_bo_wait_ctx(bo, ctx); > if (r) > - goto fail; > + return r; > > /* Can't move a pinned BO */ > rbo = container_of(bo, struct radeon_bo, tbo); > @@ -246,12 +234,12 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, > bool evict, > rdev = radeon_get_rdev(bo->bdev); > if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) { > ttm_bo_move_null(bo, new_mem); > - return 0; > + goto out; > } > if (old_mem->mem_type == TTM_PL_SYSTEM && > new_mem->mem_type == TTM_PL_TT) { > ttm_bo_move_null(bo, new_mem); > - return 0; > + goto out; > } > > if (old_mem->mem_type == TTM_PL_TT && > @@ -259,31 +247,37 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, > bool evict, > radeon_ttm_tt_unbind(bo->bdev, bo->ttm); > ttm_resource_free(bo, >mem); > ttm_bo_assign_mem(bo, new_mem); > - return 0; > + goto out; > } > - if (!rdev->ring[radeon_copy_ring_index(rdev)].ready || > - rdev->asic->copy.copy == NULL) { > - /* use memcpy */ > - goto memcpy; > + if (rdev->ring[radeon_copy_ring_index(rdev)].ready && > + rdev->asic->copy.copy != NULL) { > + if ((old_mem->mem_type == TTM_PL_SYSTEM && > +new_mem->mem_type == TTM_PL_VRAM) || > + (old_mem->mem_type == TTM_PL_VRAM && > +new_mem->mem_type == TTM_PL_SYSTEM)) { > + hop->fpfn = 0; > + hop->lpfn = 0; > + hop->mem_type = TTM_PL_TT; > + hop->flags = 0; > + return -EMULTIHOP; > + } > + > + r = radeon_move_blit(bo, evict, new_mem, old_mem); > + } else { > + r = -ENODEV; > } > > - r = radeon_move_blit(bo, evict, new_mem, old_mem); > if (r) { > -memcpy: > r = ttm_bo_move_memcpy(bo, ctx, new_mem); > - if (r) { > - goto fail; > - } > + if (r) > + return r; > } > > +out: > /* update statistics */ > atomic64_add((u64)bo->num_pages << PAGE_SHIFT, > >num_bytes_moved); > + radeon_bo_move_notify(bo, evict, new_mem); > return 0; > -fail: > - swap(*new_mem, bo->mem); > - radeon_bo_move_notify(bo, false, new_mem); > - swap(*new_mem, bo->mem); > - return r; > } > > static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct > ttm_resource *mem) > -- > 2.25.1 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Thu, Nov 26, 2020 at 4:28 PM Geert Uytterhoeven wrote: > > Hi Miguel, > > On Thu, Nov 26, 2020 at 3:54 PM Miguel Ojeda > wrote: > > On Wed, Nov 25, 2020 at 11:44 PM Edward Cree wrote: > > > To make the intent clear, you have to first be certain that you > > > understand the intent; otherwise by adding either a break or a > > > fallthrough to suppress the warning you are just destroying the > > > information that "the intent of this code is unknown". > > > > If you don't know what the intent of your own code is, then you > > *already* have a problem in your hands. > > The maintainer is not necessarily the owner/author of the code, and > thus may not know the intent of the code. > > > > or does it flag up code > > > that can be mindlessly "fixed" (in which case the warning is > > > worthless)? Proponents in this thread seem to be trying to > > > have it both ways. > > > > A warning is not worthless just because you can mindlessly fix it. > > There are many counterexamples, e.g. many > > checkpatch/lint/lang-format/indentation warnings, functional ones like > > the `if (a = b)` warning... > > BTW, you cannot mindlessly fix the latter, as you cannot know if > "(a == b)" or "((a = b))" was intended, without understanding the code > (and the (possibly unavailable) data sheet, and the hardware, ...). > to allow assignments in if statements was clearly a mistake and if you need outside information to understand the code, your code is the issue already. > P.S. So far I've stayed out of this thread, as I like it if the compiler > flags possible mistakes. After all I was the one fixing new > "may be used uninitialized" warnings thrown up by gcc-4.1, until > (a bit later than) support for that compiler was removed... > > 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 > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect
On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote: > On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote: >> [Why] >> Recent changes to upstream mst code remove the callback which >> cleared the internal state for mst. Move the missing functionality >> that was previously called through the destroy call back for mst >> connector >> destroy >> >> Signed-off-by: Aurabindo Pillai >> Signed-off-by: Eryk Brol >> --- >> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +-- >> drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- >> 2 files changed, 21 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> index b7d7ec3ba00d..d8b0f07deaf2 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr( >> void dm_helpers_dp_mst_stop_top_mgr( >> struct dc_context *ctx, >> - const struct dc_link *link) >> + struct dc_link *link) >> { >> struct amdgpu_dm_connector *aconnector = link->priv; >> + uint8_t i; >> if (!aconnector) { >> DRM_ERROR("Failed to find connector for link!"); >> @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr( >> DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n", >> aconnector, aconnector->base.base.id); >> - if (aconnector->mst_mgr.mst_state == true) >> + if (aconnector->mst_mgr.mst_state == true) { >> drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false); >> + >> + for (i = 0; i < MAX_SINKS_PER_LINK; i++) { >> + if (link->remote_sinks[i] == NULL) >> + continue; >> + >> + if (link->remote_sinks[i]->sink_signal == >> + SIGNAL_TYPE_DISPLAY_PORT_MST) { >> + dc_link_remove_remote_sink(link, link->remote_sinks[i]); > > In general I think this patch looks fine, and you can have the: > > Reviewed-by: Nicholas Kazlauskas > > But I think that this loop is redundant - dc_link_remove_remote_sink > should be removing all the remote sinks. Not sure if remote_sinks can > start at an index other than 0 though. dc_link_remove_remote_sink() will only remove one sink. It returns as soon as it is done removing the sink we asked for. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: fix check order in radeon_bo_move
Ping, Dave this is another fix for the Multihop patch set. Without it radeon is completely broken on drm-misc-next. Thanks, Christian. Am 25.11.20 um 15:34 schrieb Christian König: Reorder the code to fix checking if blitting is available. Signed-off-by: Christian König --- drivers/gpu/drm/radeon/radeon_ttm.c | 54 + 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 0ca381b95d3d..2b598141225f 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -216,27 +216,15 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, struct ttm_resource *old_mem = >mem; int r; - if ((old_mem->mem_type == TTM_PL_SYSTEM && -new_mem->mem_type == TTM_PL_VRAM) || - (old_mem->mem_type == TTM_PL_VRAM && -new_mem->mem_type == TTM_PL_SYSTEM)) { - hop->fpfn = 0; - hop->lpfn = 0; - hop->mem_type = TTM_PL_TT; - hop->flags = 0; - return -EMULTIHOP; - } - if (new_mem->mem_type == TTM_PL_TT) { r = radeon_ttm_tt_bind(bo->bdev, bo->ttm, new_mem); if (r) return r; } - radeon_bo_move_notify(bo, evict, new_mem); r = ttm_bo_wait_ctx(bo, ctx); if (r) - goto fail; + return r; /* Can't move a pinned BO */ rbo = container_of(bo, struct radeon_bo, tbo); @@ -246,12 +234,12 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, rdev = radeon_get_rdev(bo->bdev); if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) { ttm_bo_move_null(bo, new_mem); - return 0; + goto out; } if (old_mem->mem_type == TTM_PL_SYSTEM && new_mem->mem_type == TTM_PL_TT) { ttm_bo_move_null(bo, new_mem); - return 0; + goto out; } if (old_mem->mem_type == TTM_PL_TT && @@ -259,31 +247,37 @@ static int radeon_bo_move(struct ttm_buffer_object *bo, bool evict, radeon_ttm_tt_unbind(bo->bdev, bo->ttm); ttm_resource_free(bo, >mem); ttm_bo_assign_mem(bo, new_mem); - return 0; + goto out; } - if (!rdev->ring[radeon_copy_ring_index(rdev)].ready || - rdev->asic->copy.copy == NULL) { - /* use memcpy */ - goto memcpy; + if (rdev->ring[radeon_copy_ring_index(rdev)].ready && + rdev->asic->copy.copy != NULL) { + if ((old_mem->mem_type == TTM_PL_SYSTEM && +new_mem->mem_type == TTM_PL_VRAM) || + (old_mem->mem_type == TTM_PL_VRAM && +new_mem->mem_type == TTM_PL_SYSTEM)) { + hop->fpfn = 0; + hop->lpfn = 0; + hop->mem_type = TTM_PL_TT; + hop->flags = 0; + return -EMULTIHOP; + } + + r = radeon_move_blit(bo, evict, new_mem, old_mem); + } else { + r = -ENODEV; } - r = radeon_move_blit(bo, evict, new_mem, old_mem); if (r) { -memcpy: r = ttm_bo_move_memcpy(bo, ctx, new_mem); - if (r) { - goto fail; - } + if (r) + return r; } +out: /* update statistics */ atomic64_add((u64)bo->num_pages << PAGE_SHIFT, >num_bytes_moved); + radeon_bo_move_notify(bo, evict, new_mem); return 0; -fail: - swap(*new_mem, bo->mem); - radeon_bo_move_notify(bo, false, new_mem); - swap(*new_mem, bo->mem); - return r; } static int radeon_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_resource *mem) ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 3/6] drm/scheduler: Job timeout handler returns status
On 11/24/20 10:17 PM, Luben Tuikov wrote: The job timeout handler now returns status indicating back to the DRM layer whether the job was successfully cancelled or whether more time should be given to the job to complete. Signed-off-by: Luben Tuikov --- drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 -- include/drm/gpu_scheduler.h | 13 ++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index ff48101bab55..81b73790ecc6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -28,7 +28,7 @@ #include "amdgpu.h" #include "amdgpu_trace.h" -static void amdgpu_job_timedout(struct drm_sched_job *s_job) +static int amdgpu_job_timedout(struct drm_sched_job *s_job) { struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched); struct amdgpu_job *job = to_amdgpu_job(s_job); @@ -41,7 +41,7 @@ static void amdgpu_job_timedout(struct drm_sched_job *s_job) amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) { DRM_ERROR("ring %s timeout, but soft recovered\n", s_job->sched->name); - return; + return 0; } amdgpu_vm_get_task_info(ring->adev, job->pasid, ); @@ -53,10 +53,12 @@ static void amdgpu_job_timedout(struct drm_sched_job *s_job) if (amdgpu_device_should_recover_gpu(ring->adev)) { amdgpu_device_gpu_recover(ring->adev, job); + return 0; For amdgpu specifically - not that amdgpu_device_gpu_recover returns a value which is 0 for successful GPU reset meaning we reset the GPU and resubmitted to HW the job that triggered the timeout to HW (guilty). It means the job is still should be considered part of pending list and so a non zero value should be returned. I think only if we reset the GPU and don't submit back the guilty job then it can be considered 'aborted' - but I don't think we even do this. Andrey } else { drm_sched_suspend_timeout(>sched); if (amdgpu_sriov_vf(adev)) adev->virt.tdr_debug = true; + return 1; } } diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 2e0c368e19f6..61f7121e1c19 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -230,10 +230,17 @@ struct drm_sched_backend_ops { struct dma_fence *(*run_job)(struct drm_sched_job *sched_job); /** - * @timedout_job: Called when a job has taken too long to execute, - * to trigger GPU recovery. +* @timedout_job: Called when a job has taken too long to execute, +* to trigger GPU recovery. +* +* Return 0, if the job has been aborted successfully and will +* never be heard of from the device. Return non-zero if the +* job wasn't able to be aborted, i.e. if more time should be +* given to this job. The result is not "bool" as this +* function is not a predicate, although its result may seem +* as one. */ - void (*timedout_job)(struct drm_sched_job *sched_job); + int (*timedout_job)(struct drm_sched_job *sched_job); /** * @free_job: Called once the job's finished fence has been signaled ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect
On 2020-11-26 9:35 a.m., Kazlauskas, Nicholas wrote: On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote: [Why] Recent changes to upstream mst code remove the callback which cleared the internal state for mst. Move the missing functionality that was previously called through the destroy call back for mst connector destroy Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +-- drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index b7d7ec3ba00d..d8b0f07deaf2 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link) + struct dc_link *link) { struct amdgpu_dm_connector *aconnector = link->priv; + uint8_t i; if (!aconnector) { DRM_ERROR("Failed to find connector for link!"); @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr( DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n", aconnector, aconnector->base.base.id); - if (aconnector->mst_mgr.mst_state == true) + if (aconnector->mst_mgr.mst_state == true) { drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false); + + for (i = 0; i < MAX_SINKS_PER_LINK; i++) { + if (link->remote_sinks[i] == NULL) + continue; + + if (link->remote_sinks[i]->sink_signal == + SIGNAL_TYPE_DISPLAY_PORT_MST) { + dc_link_remove_remote_sink(link, link->remote_sinks[i]); In general I think this patch looks fine, and you can have the: Reviewed-by: Nicholas Kazlauskas But I think that this loop is redundant - dc_link_remove_remote_sink should be removing all the remote sinks. Not sure if remote_sinks can start at an index other than 0 though. I'd prefer we clean this up if it's redundant. Otherwise redundant code stays around forever and makes future debug more cumbersome. Harry Regards, Nicholas Kazlauskas + + if (aconnector->dc_sink) { + dc_sink_release(aconnector->dc_sink); + aconnector->dc_sink = NULL; + aconnector->dc_link->cur_link_settings.lane_count = 0; + } + } + } + } > } bool dm_helpers_dp_read_dpcd( diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h index b2cd8491c707..07e349b1067b 100644 --- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h +++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h @@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link); + struct dc_link *link); /** * OS specific aux read callback. */ ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7CHARRY.WENTLAND%40amd.com%7C7576866a05154b66cdb208d8921898da%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C63741998164920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=sm7jvbxW%2BgEUevWgjACydnjuiE7SgtunMBUIpLT99r4%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect
On 2020-11-26 9:31 a.m., Aurabindo Pillai wrote: [Why] Recent changes to upstream mst code remove the callback which cleared the internal state for mst. Move the missing functionality that was previously called through the destroy call back for mst connector destroy Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +-- drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index b7d7ec3ba00d..d8b0f07deaf2 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link) + struct dc_link *link) { struct amdgpu_dm_connector *aconnector = link->priv; + uint8_t i; if (!aconnector) { DRM_ERROR("Failed to find connector for link!"); @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr( DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n", aconnector, aconnector->base.base.id); - if (aconnector->mst_mgr.mst_state == true) + if (aconnector->mst_mgr.mst_state == true) { drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false); + + for (i = 0; i < MAX_SINKS_PER_LINK; i++) { + if (link->remote_sinks[i] == NULL) + continue; + + if (link->remote_sinks[i]->sink_signal == + SIGNAL_TYPE_DISPLAY_PORT_MST) { + dc_link_remove_remote_sink(link, link->remote_sinks[i]); In general I think this patch looks fine, and you can have the: Reviewed-by: Nicholas Kazlauskas But I think that this loop is redundant - dc_link_remove_remote_sink should be removing all the remote sinks. Not sure if remote_sinks can start at an index other than 0 though. Regards, Nicholas Kazlauskas + + if (aconnector->dc_sink) { + dc_sink_release(aconnector->dc_sink); + aconnector->dc_sink = NULL; + aconnector->dc_link->cur_link_settings.lane_count = 0; + } + } + } + } > } bool dm_helpers_dp_read_dpcd( diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h index b2cd8491c707..07e349b1067b 100644 --- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h +++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h @@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link); + struct dc_link *link); /** * OS specific aux read callback. */ ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amd/display: Clear dc remote sinks on MST disconnect
[Why] Recent changes to upstream mst code remove the callback which cleared the internal state for mst. Move the missing functionality that was previously called through the destroy call back for mst connector destroy Signed-off-by: Aurabindo Pillai Signed-off-by: Eryk Brol --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 22 +-- drivers/gpu/drm/amd/display/dc/dm_helpers.h | 2 +- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index b7d7ec3ba00d..d8b0f07deaf2 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -418,9 +418,10 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link) + struct dc_link *link) { struct amdgpu_dm_connector *aconnector = link->priv; + uint8_t i; if (!aconnector) { DRM_ERROR("Failed to find connector for link!"); @@ -430,8 +431,25 @@ void dm_helpers_dp_mst_stop_top_mgr( DRM_INFO("DM_MST: stopping TM on aconnector: %p [id: %d]\n", aconnector, aconnector->base.base.id); - if (aconnector->mst_mgr.mst_state == true) + if (aconnector->mst_mgr.mst_state == true) { drm_dp_mst_topology_mgr_set_mst(>mst_mgr, false); + + for (i = 0; i < MAX_SINKS_PER_LINK; i++) { + if (link->remote_sinks[i] == NULL) + continue; + + if (link->remote_sinks[i]->sink_signal == + SIGNAL_TYPE_DISPLAY_PORT_MST) { + dc_link_remove_remote_sink(link, link->remote_sinks[i]); + + if (aconnector->dc_sink) { + dc_sink_release(aconnector->dc_sink); + aconnector->dc_sink = NULL; + aconnector->dc_link->cur_link_settings.lane_count = 0; + } + } + } + } } bool dm_helpers_dp_read_dpcd( diff --git a/drivers/gpu/drm/amd/display/dc/dm_helpers.h b/drivers/gpu/drm/amd/display/dc/dm_helpers.h index b2cd8491c707..07e349b1067b 100644 --- a/drivers/gpu/drm/amd/display/dc/dm_helpers.h +++ b/drivers/gpu/drm/amd/display/dc/dm_helpers.h @@ -113,7 +113,7 @@ bool dm_helpers_dp_mst_start_top_mgr( void dm_helpers_dp_mst_stop_top_mgr( struct dc_context *ctx, - const struct dc_link *link); + struct dc_link *link); /** * OS specific aux read callback. */ -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Am 26.11.20 um 13:14 schrieb Thomas Zimmermann: Hi Am 26.11.20 um 13:08 schrieb Christian König: [...] Yeah, I remember a bug report about frequent page-table modifications wrt to vram helpers. So we implemented the lazy unmapping / vmap caching, as suggested by Christian back them. My guess is that anything TTM-based can use a similar pattern. Christian probably knows the corner cases. My memory is failing me, what corner case are you talking about? Haha! :D What I meant was that if there were corner cases, you'd know about them. From your comment, I guess there are none. Ah, ok :) I was wondering for a moment if I have missed something. Cheers, Christian. Best regards Thomas Christian. CMA seems obviously working correctly already. For SHMEM, I'd have to figure out the reference counting of the pages involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could be moved into the free callback, plus a few other modifications. Best regards Thomas But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking place, so I guess the reservation lock could be acquired/release in drm_client_buffer_vmap/vunmap(), or a separate pair of DRM client functions could do the locking. Given how this "do the right locking" is a can of worms (and I think it's worse than what you dug out already) I think the fb_helper.lock hack is perfectly good enough. I'm also somewhat worried that starting to use dma_resv lock in generic code, while many helpers/drivers still have their hand-rolled locking, will make conversion over to dma_resv needlessly more complicated. -Daniel Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394 Please note that the reservation lock you need to take here is part of the GEM object. Usually we design things in the way that the code needs to take a lock which protects an object, then do some operations with the object and then release the lock again. Having in the lock inside the operation can be done as well, but returning with it is kind of unusual design. Sorry for the noob questions. I'm still trying to understand the implications of acquiring these locks. Well this is the reservation lock of the GEM object we are talking about here. We need to take that for a couple of different operations, vmap/vunmap doesn't sound like a special case to me. Regards, Christian. Best regards Thomas ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Hi Am 26.11.20 um 13:08 schrieb Christian König: [...] Yeah, I remember a bug report about frequent page-table modifications wrt to vram helpers. So we implemented the lazy unmapping / vmap caching, as suggested by Christian back them. My guess is that anything TTM-based can use a similar pattern. Christian probably knows the corner cases. My memory is failing me, what corner case are you talking about? Haha! :D What I meant was that if there were corner cases, you'd know about them. From your comment, I guess there are none. Best regards Thomas Christian. CMA seems obviously working correctly already. For SHMEM, I'd have to figure out the reference counting of the pages involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could be moved into the free callback, plus a few other modifications. Best regards Thomas But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking place, so I guess the reservation lock could be acquired/release in drm_client_buffer_vmap/vunmap(), or a separate pair of DRM client functions could do the locking. Given how this "do the right locking" is a can of worms (and I think it's worse than what you dug out already) I think the fb_helper.lock hack is perfectly good enough. I'm also somewhat worried that starting to use dma_resv lock in generic code, while many helpers/drivers still have their hand-rolled locking, will make conversion over to dma_resv needlessly more complicated. -Daniel Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394 Please note that the reservation lock you need to take here is part of the GEM object. Usually we design things in the way that the code needs to take a lock which protects an object, then do some operations with the object and then release the lock again. Having in the lock inside the operation can be done as well, but returning with it is kind of unusual design. Sorry for the noob questions. I'm still trying to understand the implications of acquiring these locks. Well this is the reservation lock of the GEM object we are talking about here. We need to take that for a couple of different operations, vmap/vunmap doesn't sound like a special case to me. Regards, Christian. Best regards Thomas ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_0x680DC11D530B7A23.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature ___
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Am 26.11.20 um 12:59 schrieb Thomas Zimmermann: Hi Am 26.11.20 um 12:04 schrieb Daniel Vetter: On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann wrote: Hi Am 25.11.20 um 17:32 schrieb Daniel Vetter: [...] I guess full locking is required :-/ I'm not exactly sure how to make this happen with the current plethora of helpers ... I think we need an _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. I think we might be able to get away without new callbacks. I looked through the sources that implement and use vmap. All the implementations are without taking resv locks. For locking, we can wrap them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() that locks and vmaps should make this easy. In terms of implementation, only vram helpers do a pin+map in their vmap code. And as I mentioned before, this is actually wrong. The pattern dates back to when the code was still in individual drivers. It's time to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. Finally, there aren't that many users of vmap. A few drivers use it while blitting framebuffers into HW buffers and ast does some permanent mapping of the cursor BO. All this is trivial to turn into small pairs of lock+vmap and vunmap+unlock. That leaves generic fbdev. The shadow buffer is also trivial to fix, as outlined during this discussion. The code for fbdev in hardware buffers is a special case. It vmaps the buffer during initialization and only vunmaps it during shutdown. As this has worked so far without locking, I'd leave it as it is and put a big comment next to is. Hardware fbdev buffers is only required by few drivers; namely those that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. We should consider to make the fbdev shadow buffer the default and have drivers opt-in for the hardware buffer, if they need it. And then document that if the callers of the _locked version wants a permanent mapping, it also needs to pin it. Plus I guess ideally implement the unlocked/permanent versions in terms of that, so that drivers only have to implement one or the other. For my understanding, pinning is only done in prepare_fb code. And ast pins its cursor BOs into vram. We should document to hols vmap/vunmap only for time and cover them with resv locks. Pinning is for cases where the hardware requires buffers in a special location, but does not protect against concurrent threat. I think those are the implicit rules already. I updated the radeon patchset, where all this appears to be working well. Hm yeah if you want to do the full change, then that works out too. It's just a pile of work. But if we can finish off with an dma_resv_assert_locked in dma_buf_vmap/vunmap, then I think that's ok. It does mean that exporters must implement vmap caching, or performance will be terrible. So quite some update for the dma-buf docs. Yeah, I remember a bug report about frequent page-table modifications wrt to vram helpers. So we implemented the lazy unmapping / vmap caching, as suggested by Christian back them. My guess is that anything TTM-based can use a similar pattern. Christian probably knows the corner cases. My memory is failing me, what corner case are you talking about? Christian. CMA seems obviously working correctly already. For SHMEM, I'd have to figure out the reference counting of the pages involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could be moved into the free callback, plus a few other modifications. Best regards Thomas But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Hi Am 26.11.20 um 12:04 schrieb Daniel Vetter: On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann wrote: Hi Am 25.11.20 um 17:32 schrieb Daniel Vetter: [...] I guess full locking is required :-/ I'm not exactly sure how to make this happen with the current plethora of helpers ... I think we need an _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. I think we might be able to get away without new callbacks. I looked through the sources that implement and use vmap. All the implementations are without taking resv locks. For locking, we can wrap them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() that locks and vmaps should make this easy. In terms of implementation, only vram helpers do a pin+map in their vmap code. And as I mentioned before, this is actually wrong. The pattern dates back to when the code was still in individual drivers. It's time to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. Finally, there aren't that many users of vmap. A few drivers use it while blitting framebuffers into HW buffers and ast does some permanent mapping of the cursor BO. All this is trivial to turn into small pairs of lock+vmap and vunmap+unlock. That leaves generic fbdev. The shadow buffer is also trivial to fix, as outlined during this discussion. The code for fbdev in hardware buffers is a special case. It vmaps the buffer during initialization and only vunmaps it during shutdown. As this has worked so far without locking, I'd leave it as it is and put a big comment next to is. Hardware fbdev buffers is only required by few drivers; namely those that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. We should consider to make the fbdev shadow buffer the default and have drivers opt-in for the hardware buffer, if they need it. And then document that if the callers of the _locked version wants a permanent mapping, it also needs to pin it. Plus I guess ideally implement the unlocked/permanent versions in terms of that, so that drivers only have to implement one or the other. For my understanding, pinning is only done in prepare_fb code. And ast pins its cursor BOs into vram. We should document to hols vmap/vunmap only for time and cover them with resv locks. Pinning is for cases where the hardware requires buffers in a special location, but does not protect against concurrent threat. I think those are the implicit rules already. I updated the radeon patchset, where all this appears to be working well. Hm yeah if you want to do the full change, then that works out too. It's just a pile of work. But if we can finish off with an dma_resv_assert_locked in dma_buf_vmap/vunmap, then I think that's ok. It does mean that exporters must implement vmap caching, or performance will be terrible. So quite some update for the dma-buf docs. Yeah, I remember a bug report about frequent page-table modifications wrt to vram helpers. So we implemented the lazy unmapping / vmap caching, as suggested by Christian back them. My guess is that anything TTM-based can use a similar pattern. Christian probably knows the corner cases. CMA seems obviously working correctly already. For SHMEM, I'd have to figure out the reference counting of the pages involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could be moved into the free callback, plus a few other modifications. Best regards Thomas But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking place, so I guess the reservation lock could be acquired/release in drm_client_buffer_vmap/vunmap(), or a separate pair of DRM client functions could do the
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Hi Am 26.11.20 um 12:28 schrieb Christian König: Am 26.11.20 um 12:04 schrieb Daniel Vetter: On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann wrote: Hi Am 25.11.20 um 17:32 schrieb Daniel Vetter: [...] I guess full locking is required :-/ I'm not exactly sure how to make this happen with the current plethora of helpers ... I think we need an _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. I think we might be able to get away without new callbacks. I looked through the sources that implement and use vmap. All the implementations are without taking resv locks. For locking, we can wrap them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() that locks and vmaps should make this easy. In terms of implementation, only vram helpers do a pin+map in their vmap code. And as I mentioned before, this is actually wrong. The pattern dates back to when the code was still in individual drivers. It's time to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. Finally, there aren't that many users of vmap. A few drivers use it while blitting framebuffers into HW buffers and ast does some permanent mapping of the cursor BO. All this is trivial to turn into small pairs of lock+vmap and vunmap+unlock. That leaves generic fbdev. The shadow buffer is also trivial to fix, as outlined during this discussion. The code for fbdev in hardware buffers is a special case. It vmaps the buffer during initialization and only vunmaps it during shutdown. As this has worked so far without locking, I'd leave it as it is and put a big comment next to is. Please keep in mind that you only need to grab the lock if the buffer is not pinned otherwise. In other words when we are scanning out from the BO it is guaranteed that it can't move around. Maybe this makes the case here easier to handle. The fbdev code is already fragile. If no shadow FB is selected, the hardware BO is vmapped, but never pinned; if only for the reason that there's no useful generic interface to do this. So we cannot lock for longer periods, but it's also not pinned either. This really only work with a few drivers that use CMA helpers, where BOs don't move. Best regards Thomas Hardware fbdev buffers is only required by few drivers; namely those that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. We should consider to make the fbdev shadow buffer the default and have drivers opt-in for the hardware buffer, if they need it. And then document that if the callers of the _locked version wants a permanent mapping, it also needs to pin it. Plus I guess ideally implement the unlocked/permanent versions in terms of that, so that drivers only have to implement one or the other. For my understanding, pinning is only done in prepare_fb code. And ast pins its cursor BOs into vram. We should document to hols vmap/vunmap only for time and cover them with resv locks. Pinning is for cases where the hardware requires buffers in a special location, but does not protect against concurrent threat. I think those are the implicit rules already. I updated the radeon patchset, where all this appears to be working well. Hm yeah if you want to do the full change, then that works out too. It's just a pile of work. But if we can finish off with an dma_resv_assert_locked in dma_buf_vmap/vunmap, then I think that's ok. It does mean that exporters must implement vmap caching, or performance will be terrible. So quite some update for the dma-buf docs. That's one possibility, but I think we should keep the ability to use pin+vmap instead of lock+vmap. Regards, Christian. But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Am 26.11.20 um 12:04 schrieb Daniel Vetter: On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann wrote: Hi Am 25.11.20 um 17:32 schrieb Daniel Vetter: [...] I guess full locking is required :-/ I'm not exactly sure how to make this happen with the current plethora of helpers ... I think we need an _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. I think we might be able to get away without new callbacks. I looked through the sources that implement and use vmap. All the implementations are without taking resv locks. For locking, we can wrap them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() that locks and vmaps should make this easy. In terms of implementation, only vram helpers do a pin+map in their vmap code. And as I mentioned before, this is actually wrong. The pattern dates back to when the code was still in individual drivers. It's time to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. Finally, there aren't that many users of vmap. A few drivers use it while blitting framebuffers into HW buffers and ast does some permanent mapping of the cursor BO. All this is trivial to turn into small pairs of lock+vmap and vunmap+unlock. That leaves generic fbdev. The shadow buffer is also trivial to fix, as outlined during this discussion. The code for fbdev in hardware buffers is a special case. It vmaps the buffer during initialization and only vunmaps it during shutdown. As this has worked so far without locking, I'd leave it as it is and put a big comment next to is. Please keep in mind that you only need to grab the lock if the buffer is not pinned otherwise. In other words when we are scanning out from the BO it is guaranteed that it can't move around. Maybe this makes the case here easier to handle. Hardware fbdev buffers is only required by few drivers; namely those that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. We should consider to make the fbdev shadow buffer the default and have drivers opt-in for the hardware buffer, if they need it. And then document that if the callers of the _locked version wants a permanent mapping, it also needs to pin it. Plus I guess ideally implement the unlocked/permanent versions in terms of that, so that drivers only have to implement one or the other. For my understanding, pinning is only done in prepare_fb code. And ast pins its cursor BOs into vram. We should document to hols vmap/vunmap only for time and cover them with resv locks. Pinning is for cases where the hardware requires buffers in a special location, but does not protect against concurrent threat. I think those are the implicit rules already. I updated the radeon patchset, where all this appears to be working well. Hm yeah if you want to do the full change, then that works out too. It's just a pile of work. But if we can finish off with an dma_resv_assert_locked in dma_buf_vmap/vunmap, then I think that's ok. It does mean that exporters must implement vmap caching, or performance will be terrible. So quite some update for the dma-buf docs. That's one possibility, but I think we should keep the ability to use pin+vmap instead of lock+vmap. Regards, Christian. But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking place, so I guess the reservation lock could be acquired/release in drm_client_buffer_vmap/vunmap(), or a separate pair of DRM client functions could do the locking. Given how this "do the right locking" is a can of worms (and I think it's worse than what you dug out already) I think the fb_helper.lock hack is perfectly good enough. I'm also somewhat worried that
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
On Thu, Nov 26, 2020 at 11:15 AM Thomas Zimmermann wrote: > > Hi > > Am 25.11.20 um 17:32 schrieb Daniel Vetter: > > [...] > > I guess full locking is required :-/ I'm not exactly sure how to make this > > happen with the current plethora of helpers ... I think we need an > > _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. > > I think we might be able to get away without new callbacks. > > I looked through the sources that implement and use vmap. All the > implementations are without taking resv locks. For locking, we can wrap > them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() > that locks and vmaps should make this easy. > > In terms of implementation, only vram helpers do a pin+map in their vmap > code. And as I mentioned before, this is actually wrong. The pattern > dates back to when the code was still in individual drivers. It's time > to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. > > Finally, there aren't that many users of vmap. A few drivers use it > while blitting framebuffers into HW buffers and ast does some permanent > mapping of the cursor BO. All this is trivial to turn into small pairs > of lock+vmap and vunmap+unlock. > > That leaves generic fbdev. The shadow buffer is also trivial to fix, as > outlined during this discussion. > > The code for fbdev in hardware buffers is a special case. It vmaps the > buffer during initialization and only vunmaps it during shutdown. As > this has worked so far without locking, I'd leave it as it is and put a > big comment next to is. > > Hardware fbdev buffers is only required by few drivers; namely those > that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. > We should consider to make the fbdev shadow buffer the default and have > drivers opt-in for the hardware buffer, if they need it. > > > > > And then document that if the callers of the _locked version wants a > > permanent mapping, it also needs to pin it. Plus I guess ideally implement > > the unlocked/permanent versions in terms of that, so that drivers only > > have to implement one or the other. > > For my understanding, pinning is only done in prepare_fb code. And ast > pins its cursor BOs into vram. We should document to hols vmap/vunmap > only for time and cover them with resv locks. Pinning is for cases where > the hardware requires buffers in a special location, but does not > protect against concurrent threat. I think those are the implicit rules > already. > > I updated the radeon patchset, where all this appears to be working well. Hm yeah if you want to do the full change, then that works out too. It's just a pile of work. But if we can finish off with an dma_resv_assert_locked in dma_buf_vmap/vunmap, then I think that's ok. It does mean that exporters must implement vmap caching, or performance will be terrible. So quite some update for the dma-buf docs. But if you're willing to do all that conversion of callers, then of course I'm not stopping you. Not at all, it's great to see that kind of maze untangled. -Daniel > > Best regards > Thomas > > > > > That should give us at least some way forward to gradually conver all the > > drivers and helpers over to dma_resv locking. > > -Daniel > > > >> The pin count is currently maintained by the vmap implementation in vram > >> helpers. Calling vmap is an implicit pin; calling vunmap is an implicit > >> unpin. This prevents eviction in the damage worker. But now I was told than > >> pinning is only for BOs that are controlled by userspace and internal users > >> should acquire the resv lock. So vram helpers have to be fixed, actually. > >> > >> In vram helpers, unmapping does not mean eviction. The unmap operation only > >> marks the BO as unmappable. The real unmap happens when the eviction takes > >> place. This avoids many modifications to the page tables. IOW an unpinned, > >> unmapped BO will remain in VRAM until the memory is actually needed. > >> > >> Best regards > >> Thomas > >> > >>> > >>> So I'm still not seeing how this can go boom. > >>> > >>> Now long term it'd be nice to cut everything over to dma_resv locking, but > >>> the issue there is that beyond ttm, none of the helpers (and few of the > >>> drivers) use dma_resv. So this is a fairly big uphill battle. Quick > >>> interim fix seems like the right solution to me. > >>> -Daniel > >>> > > Regards, > Christian. > > > > > Best regards > > Thomas > > > >> > >>> There's no recursion taking place, so I guess the reservation > >>> lock could be > >>> acquired/release in drm_client_buffer_vmap/vunmap(), or a > >>> separate pair of > >>> DRM client functions could do the locking. > >> > >> Given how this "do the right locking" is a can of worms (and I think > >> it's > >> worse than what you dug out already) I think the fb_helper.lock hack is > >> perfectly good enough. > >> > >> I'm also somewhat worried that
Re: [PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed
Hi Am 25.11.20 um 17:32 schrieb Daniel Vetter: [...] I guess full locking is required :-/ I'm not exactly sure how to make this happen with the current plethora of helpers ... I think we need an _locked version of vmap/vunmap callbacks in drm_gem_object_funcs. I think we might be able to get away without new callbacks. I looked through the sources that implement and use vmap. All the implementations are without taking resv locks. For locking, we can wrap them in lock/unlock pairs. Having something like drm_gem_vmap_unlocked() that locks and vmaps should make this easy. In terms of implementation, only vram helpers do a pin+map in their vmap code. And as I mentioned before, this is actually wrong. The pattern dates back to when the code was still in individual drivers. It's time to clean this up. Vram helpers can use drm_gem_ttm_vmap() instead. Finally, there aren't that many users of vmap. A few drivers use it while blitting framebuffers into HW buffers and ast does some permanent mapping of the cursor BO. All this is trivial to turn into small pairs of lock+vmap and vunmap+unlock. That leaves generic fbdev. The shadow buffer is also trivial to fix, as outlined during this discussion. The code for fbdev in hardware buffers is a special case. It vmaps the buffer during initialization and only vunmaps it during shutdown. As this has worked so far without locking, I'd leave it as it is and put a big comment next to is. Hardware fbdev buffers is only required by few drivers; namely those that require the CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM config option to work. We should consider to make the fbdev shadow buffer the default and have drivers opt-in for the hardware buffer, if they need it. And then document that if the callers of the _locked version wants a permanent mapping, it also needs to pin it. Plus I guess ideally implement the unlocked/permanent versions in terms of that, so that drivers only have to implement one or the other. For my understanding, pinning is only done in prepare_fb code. And ast pins its cursor BOs into vram. We should document to hols vmap/vunmap only for time and cover them with resv locks. Pinning is for cases where the hardware requires buffers in a special location, but does not protect against concurrent threat. I think those are the implicit rules already. I updated the radeon patchset, where all this appears to be working well. Best regards Thomas That should give us at least some way forward to gradually conver all the drivers and helpers over to dma_resv locking. -Daniel The pin count is currently maintained by the vmap implementation in vram helpers. Calling vmap is an implicit pin; calling vunmap is an implicit unpin. This prevents eviction in the damage worker. But now I was told than pinning is only for BOs that are controlled by userspace and internal users should acquire the resv lock. So vram helpers have to be fixed, actually. In vram helpers, unmapping does not mean eviction. The unmap operation only marks the BO as unmappable. The real unmap happens when the eviction takes place. This avoids many modifications to the page tables. IOW an unpinned, unmapped BO will remain in VRAM until the memory is actually needed. Best regards Thomas So I'm still not seeing how this can go boom. Now long term it'd be nice to cut everything over to dma_resv locking, but the issue there is that beyond ttm, none of the helpers (and few of the drivers) use dma_resv. So this is a fairly big uphill battle. Quick interim fix seems like the right solution to me. -Daniel Regards, Christian. Best regards Thomas There's no recursion taking place, so I guess the reservation lock could be acquired/release in drm_client_buffer_vmap/vunmap(), or a separate pair of DRM client functions could do the locking. Given how this "do the right locking" is a can of worms (and I think it's worse than what you dug out already) I think the fb_helper.lock hack is perfectly good enough. I'm also somewhat worried that starting to use dma_resv lock in generic code, while many helpers/drivers still have their hand-rolled locking, will make conversion over to dma_resv needlessly more complicated. -Daniel Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394 Please note that the reservation lock you need to take here is part of the GEM object. Usually we design things in the way that the code needs to take a lock which protects an object, then do some operations with the object and then release the lock again. Having in the lock inside the operation can be done as well, but returning with it is kind of unusual design. Sorry for the noob questions. I'm still trying to understand the implications of acquiring these locks. Well this is the reservation lock of the GEM object we are talking about here. We need to take that for a couple of
RE: [PATCH] drm/amdgpu: skip power profile switch in sriov
[AMD Public Use] Reviewed-by: Jiange Zhao -Original Message- From: Jingwen Chen Sent: Thursday, November 26, 2020 4:38 PM To: amd-gfx@lists.freedesktop.org; Zhao, Jiange Cc: Chen, JingWen Subject: [PATCH] drm/amdgpu: skip power profile switch in sriov power profile switch in vcn need to send SetWorkLoad msg to smu, which is not supported in sriov. Signed-off-by: Jingwen Chen --- drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c index 17a45baff638..8fb12afe3c96 100644 --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c @@ -1168,6 +1168,9 @@ int amdgpu_dpm_switch_power_profile(struct amdgpu_device *adev, { int ret = 0; + if (amdgpu_sriov_vf(adev)) + return 0; + if (is_support_sw_smu(adev)) ret = smu_switch_power_profile(>smu, type, en); else if (adev->powerplay.pp_funcs && -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: skip power profile switch in sriov
power profile switch in vcn need to send SetWorkLoad msg to smu, which is not supported in sriov. Signed-off-by: Jingwen Chen --- drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c index 17a45baff638..8fb12afe3c96 100644 --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c @@ -1168,6 +1168,9 @@ int amdgpu_dpm_switch_power_profile(struct amdgpu_device *adev, { int ret = 0; + if (amdgpu_sriov_vf(adev)) + return 0; + if (is_support_sw_smu(adev)) ret = smu_switch_power_profile(>smu, type, en); else if (adev->powerplay.pp_funcs && -- 2.25.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > On Wed, Nov 25, 2020 at 1:33 PM Finn Thain wrote: > > > > Or do you think that a codebase can somehow satisfy multiple checkers > > and their divergent interpretations of the language spec? > > Have we found any cases yet that are divergent? I don't think so. You mean, aside from -Wimplicit-fallthrough? I'm glad you asked. How about -Wincompatible-pointer-types and -Wframe-larger-than? All of the following files have been affected by divergent diagnostics produced by clang and gcc. arch/arm64/include/asm/neon-intrinsics.h arch/powerpc/xmon/Makefile drivers/gpu/drm/i915/Makefile drivers/gpu/drm/i915/i915_utils.h drivers/staging/media/atomisp/pci/atomisp_subdev.c fs/ext4/super.c include/trace/events/qla.h net/mac80211/rate.c tools/lib/string.c tools/perf/util/setup.py tools/scripts/Makefile.include And if I searched for 'smatch' or 'coverity' instead of 'clang' I'd probably find more divergence. Here are some of the relevant commits. 0738c8b5915c7eaf1e6007b441008e8f3b460443 9c87156cce5a63735d1218f0096a65c50a7a32aa babaab2f473817f173a2d08e410c25abf5ed0f6b 065e5e559555e2f100bc95792a8ef1b609bbe130 93f56de259376d7e4fff2b2d104082e1fa66e237 6c4798d3f08b81c2c52936b10e0fa872590c96ae b7a313d84e853049062011d78cb04b6decd12f5c 093b75ef5995ea35d7f6bdb6c7b32a42a1999813 And before you object, "but -Wconstant-logical-operand is a clang-only warning! it can't be divergent with gcc!", consider that the special cases added to deal with clang-only warnings have to be removed when gcc catches up, which is more churn. Now multiply that by the number of checkers you care about. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > On Wed, Nov 25, 2020 at 1:33 PM Finn Thain > wrote: > > > > Or do you think that a codebase can somehow satisfy multiple checkers > > and their divergent interpretations of the language spec? > > Have we found any cases yet that are divergent? I don't think so. There are many implementations, so I think you are guaranteed to find more divergence if you look. That's because the spec is full of language like this: "implementations are encouraged not to emit a diagnostic" and "implementations are encouraged to issue a diagnostic". Some implementations will decide to not emit (under the premise that vast amounts of existing code would have to get patched until the compiler goes quiet) whereas other implementations will decide to emit (under the premise that the author is doing the checking and not the janitor or the packager). > It sounds to me like GCC's cases it warns for is a subset of Clang's. > Having additional coverage with Clang then should ensure coverage for > both. > If that claim were true, the solution would be simple. (It's not.) For the benefit of projects that enable -Werror and projects that nominated gcc as their preferred compiler, clang would simply need a flag to enable conformance with gcc by suppressing those additional warnings that clang would normally produce. This simple solution is, of course, completely unworkable, since it would force clang to copy some portion of gcc's logic (rewritten under LLVM's unique license) and then to track future changes to that portion of gcc indefinitely. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On 24/11/2020 21:25, Kees Cook wrote: > I still think this isn't right -- it's a case statement that runs off > the end without an explicit flow control determination. Proves too much — for instance case foo: case bar: thing; break; doesn't require a fallthrough; after case foo:, and afaik no-one is suggesting it should. Yet it, too, is "a case statement that runs off the end without an explicit flow control determination". -ed ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, Nov 24, 2020 at 11:05:35PM -0800, James Bottomley wrote: > Now, what we have seems to be about 6 cases (at least what's been shown > in this thread) where a missing break would cause potentially user > visible issues. That means the value of this isn't zero, but it's not > a no-brainer massive win either. That's why I think asking what we've > invested vs the return isn't a useless exercise. The number is much higher[1]. If it were 6 in the entire history of the kernel, I would agree with you. :) Some were fixed _before_ Gustavo's effort too, which I also count towards the idea of "this is a dangerous weakness in C, and now we have stopped it forever." > But the broader point I'm making is just because the compiler people > come up with a shiny new warning doesn't necessarily mean the problem > it's detecting is one that causes us actual problems in the code base. > I'd really be happier if we had a theory about what classes of CVE or > bug we could eliminate before we embrace the next new warning. But we did! It was long ago justified and documented[2], and even links to the CWE[3] for it. This wasn't random joy over discovering a new warning we could turn on, this was turning on a warning that the compiler folks finally gave us to handle an entire class of flaws. If we need to update the code-base to address it not a useful debate -- that was settled already, even if you're only discovering it now. :P. This last patch set is about finishing that work for Clang, which is correctly even more strict than GCC. -Kees [1] https://outflux.net/slides/2019/lss/kspp.pdf calls out specific numbers (about 6.5% of the patches fixed missing breaks): v4.19: 3 of 129 v4.20: 2 of 59 v5.0: 3 of 56 v5.1: 10 of 100 v5.2: 6 of 71 v5.3: 7 of 69 And in the history of the kernel, it's been an ongoing source of flaws: $ l --no-merges | grep -i 'missing break' | wc -l 185 The frequency of such errors being "naturally" found was pretty steady until the static checkers started warning, and then it was on the rise, but the full effort flushed the rest out, and now it's dropped to almost zero: 1 v2.6.12 3 v2.6.16.28 1 v2.6.17 1 v2.6.19 2 v2.6.21 1 v2.6.22 3 v2.6.24 3 v2.6.29 1 v2.6.32 1 v2.6.33 1 v2.6.35 4 v2.6.36 3 v2.6.38 2 v2.6.39 7 v3.0 2 v3.1 2 v3.2 2 v3.3 3 v3.4 1 v3.5 8 v3.6 7 v3.7 3 v3.8 6 v3.9 3 v3.10 2 v3.11 5 v3.12 5 v3.13 2 v3.14 4 v3.15 2 v3.16 3 v3.17 2 v3.18 2 v3.19 1 v4.0 2 v4.1 5 v4.2 4 v4.5 5 v4.7 6 v4.8 1 v4.9 3 v4.10 2 v4.11 6 v4.12 3 v4.13 2 v4.14 5 v4.15 2 v4.16 7 v4.18 2 v4.19 6 v4.20 3 v5.0 12 v5.1 3 v5.2 4 v5.3 2 v5.4 1 v5.8 And the reason it's fully zero, is because we still have the cases we're cleaning up right now. Even this last one from v5.8 is specifically of the same type this series addresses: case 4: color_index = TrueCModeIndex; + break; default: return; } [2] https://www.kernel.org/doc/html/latest/process/deprecated.html#implicit-switch-case-fall-through All switch/case blocks must end in one of: break; fallthrough; continue; goto ; return [expression]; [3] https://cwe.mitre.org/data/definitions/484.html -- Kees Cook ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, 25 Nov 2020, Nick Desaulniers wrote: > So developers and distributions using Clang can't have > -Wimplicit-fallthrough enabled because GCC is less strict (which has > been shown in this thread to lead to bugs)? We'd like to have nice > things too, you know. > Apparently the GCC developers don't want you to have "nice things" either. Do you think that the kernel should drop gcc in favour of clang? Or do you think that a codebase can somehow satisfy multiple checkers and their divergent interpretations of the language spec? > This is not a shiny new warning; it's already on for GCC and has existed > in both compilers for multiple releases. > Perhaps you're referring to the compiler feature that lead to the ill-fated, tree-wide /* fallthrough */ patch series. When the ink dries on the C23 language spec and the implementations figure out how to interpret it then sure, enforce the warning for new code -- the cost/benefit analysis is straight forward. However, the case for patching existing mature code is another story. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On 25/11/2020 00:32, Miguel Ojeda wrote: > I have said *authoring* lines of *this* kind takes a minute per line. > Specifically: lines fixing the fallthrough warning mechanically and > repeatedly where the compiler tells you to, and doing so full-time for > a month. > It is useful since it makes intent clear. To make the intent clear, you have to first be certain that you understand the intent; otherwise by adding either a break or a fallthrough to suppress the warning you are just destroying the information that "the intent of this code is unknown". Figuring out the intent of a piece of unfamiliar code takes more than 1 minute; just because case foo: thing; case bar: break; produces identical code to case foo: thing; break; case bar: break; doesn't mean that *either* is correct — maybe the author meant to write case foo: return thing; case bar: break; and by inserting that break you've destroyed the marker that would direct someone who knew what the code was about to look at that point in the code and spot the problem. Thus, you *always* have to look at more than just the immediate mechanical context of the code, to make a proper judgement that yes, this was the intent. If you think that that sort of thing can be done in an *average* time of one minute, then I hope you stay away from code I'm responsible for! One minute would be an optimistic target for code that, as the maintainer, one is already somewhat familiar with. For code that you're seeing for the first time, as is usually the case with the people doing these mechanical fix-a-warning patches, it's completely unrealistic. A warning is only useful because it makes you *think* about the code. If you suppress the warning without doing that thinking, then you made the warning useless; and if the warning made you think about code that didn't *need* it, then the warning was useless from the start. So make your mind up: does Clang's stricter -Wimplicit-fallthrough flag up code that needs thought (in which case the fixes take effort both to author and to review) or does it flag up code that can be mindlessly "fixed" (in which case the warning is worthless)? Proponents in this thread seem to be trying to have it both ways. -ed ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [Intel-wired-lan] [PATCH 000/141] Fix fall-through warnings for Clang
On Wed, Nov 25, 2020 at 5:24 PM Jakub Kicinski wrote: > > And just to spell it out, > > case ENUM_VALUE1: > bla(); > break; > case ENUM_VALUE2: > bla(); > default: > break; > > is a fairly idiomatic way of indicating that not all values of the enum > are expected to be handled by the switch statement. It looks like a benign typo to me -- `ENUM_VALUE2` does not follow the same pattern like `ENUM_VALUE1`. To me, the presence of the `default` is what indicates (explicitly) that not everything is handled. > Applying a real patch set and then getting a few follow ups the next day > for trivial coding things like fallthrough missing or static missing, > just because I didn't have the full range of compilers to check with > before applying makes me feel pretty shitty, like I'm not doing a good > job. YMMV. The number of compilers, checkers, static analyzers, tests, etc. we use keeps going up. That, indeed, means maintainers will miss more things (unless maintainers do more work than before). But catching bugs before they happen is *not* a bad thing. Perhaps we could encourage more rebasing in -next (while still giving credit to bots and testers) to avoid having many fixing commits afterwards, but that is orthogonal. I really don't think we should encourage the feeling that a maintainer is doing a bad job if they don't catch everything on their reviews. Any review is worth it. Maintainers, in the end, are just the "guaranteed" reviewers that decide when the code looks reasonable enough. They should definitely not feel pressured to be perfect. Cheers, Miguel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 5/6] drm/amdgpu: Don't hardcode thread name length
Am 25.11.20 um 18:01 schrieb Luben Tuikov: On 2020-11-25 04:55, Christian König wrote: Am 25.11.20 um 04:17 schrieb Luben Tuikov: Introduce a macro DRM_THREAD_NAME_LEN and use that to define ring name size, instead of hardcoding it to 16. Signed-off-by: Luben Tuikov --- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 2 +- include/drm/gpu_scheduler.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 7112137689db..bbd46c6dec65 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -230,7 +230,7 @@ struct amdgpu_ring { unsignedwptr_offs; unsignedfence_offs; uint64_tcurrent_ctx; - charname[16]; + charname[DRM_THREAD_NAME_LEN]; u32 trail_seq; unsignedtrail_fence_offs; u64 trail_fence_gpu_addr; diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 61f7121e1c19..3a5686c3b5e9 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -30,6 +30,8 @@ #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000) +#define DRM_THREAD_NAME_LEN TASK_COMM_LEN + The thread name is an amdgpu specific thing. I don't think we should have that in the scheduler. I need it in DRM when creating the done thread from the name of the main scheduler thread. Since DRM creates threads, the main scheduler thread and the done thread, it would be good to have a preliminary limit to the name string. And why do you use TASK_COMM_LEN here? That is completely unrelated stuff. If you trace down into the kernel, TASK_COMM_LEN seems to be used in snprintf() when naming a kernel thread, and its value is 16--same as the one used in amdgpu. Oh, that's new to me. In my memory this name was used as a filename in debugfs only. So the size of the name string transitions from amdgpu to DRM to kernel proper, where amdgpu and kernel proper set it to max 16, but DRM doesn't give it a limit. Sure, I can remove it from DRM, and just use a local limit when snprintf() the name when creating a tread, possibly using TASK_COMM_LEN. (That's in the next patch.) Yeah, just use TASK_COMM_LEN directly where appropriate. Regards, Christian. Would that be better? I can do that in v2 of this patchset. Thanks, Luben Regards, Christian. struct drm_gpu_scheduler; struct drm_sched_rq; ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx