From: jsg Date: Mon, 6 Mar 2023 02:38:01 +0000 (+0000) Subject: drm/amd/display: Fix race condition in DPIA AUX transfer X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=224ad95353c870e0fca93f818b7d49e140557843;p=openbsd drm/amd/display: Fix race condition in DPIA AUX transfer From Stylon Wang 075e2099c32cf4486b27266d2aecf61e95499ea4 in linux-6.1.y/6.1.15 ead08b95fa50f40618c72b93a849c4ae30c9cd50 in mainline linux --- diff --git a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.c index 39e2b5cbdc3..e08b0556d99 100644 --- a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -147,14 +147,6 @@ MODULE_FIRMWARE(FIRMWARE_NAVI12_DMCU); /* Number of bytes in PSP footer for firmware. */ #define PSP_FOOTER_BYTES 0x100 -/* - * DMUB Async to Sync Mechanism Status - */ -#define DMUB_ASYNC_TO_SYNC_ACCESS_FAIL 1 -#define DMUB_ASYNC_TO_SYNC_ACCESS_TIMEOUT 2 -#define DMUB_ASYNC_TO_SYNC_ACCESS_SUCCESS 3 -#define DMUB_ASYNC_TO_SYNC_ACCESS_INVALID 4 - /** * DOC: overview * @@ -1458,6 +1450,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev) memset(&init_params, 0, sizeof(init_params)); #endif + rw_init(&adev->dm.dpia_aux_lock, "dmdpia"); rw_init(&adev->dm.dc_lock, "dmdc"); rw_init(&adev->dm.audio_lock, "dmaud"); mtx_init(&adev->dm.vblank_lock, IPL_TTY); @@ -1818,6 +1811,7 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev) mutex_destroy(&adev->dm.audio_lock); mutex_destroy(&adev->dm.dc_lock); + mutex_destroy(&adev->dm.dpia_aux_lock); return; } @@ -10205,91 +10199,92 @@ uint32_t dm_read_reg_func(const struct dc_context *ctx, uint32_t address, return value; } -static int amdgpu_dm_set_dmub_async_sync_status(bool is_cmd_aux, - struct dc_context *ctx, - uint8_t status_type, - uint32_t *operation_result) +int amdgpu_dm_process_dmub_aux_transfer_sync( + struct dc_context *ctx, + unsigned int link_index, + struct aux_payload *payload, + enum aux_return_code_type *operation_result) { struct amdgpu_device *adev = ctx->driver_context; - int return_status = -1; struct dmub_notification *p_notify = adev->dm.dmub_notify; + int ret = -1; - if (is_cmd_aux) { - if (status_type == DMUB_ASYNC_TO_SYNC_ACCESS_SUCCESS) { - return_status = p_notify->aux_reply.length; - *operation_result = p_notify->result; - } else if (status_type == DMUB_ASYNC_TO_SYNC_ACCESS_TIMEOUT) { - *operation_result = AUX_RET_ERROR_TIMEOUT; - } else if (status_type == DMUB_ASYNC_TO_SYNC_ACCESS_FAIL) { - *operation_result = AUX_RET_ERROR_ENGINE_ACQUIRE; - } else if (status_type == DMUB_ASYNC_TO_SYNC_ACCESS_INVALID) { - *operation_result = AUX_RET_ERROR_INVALID_REPLY; - } else { - *operation_result = AUX_RET_ERROR_UNKNOWN; + mutex_lock(&adev->dm.dpia_aux_lock); + if (!dc_process_dmub_aux_transfer_async(ctx->dc, link_index, payload)) { + *operation_result = AUX_RET_ERROR_ENGINE_ACQUIRE; + goto out; + } + + if (!wait_for_completion_timeout(&adev->dm.dmub_aux_transfer_done, 10 * HZ)) { + DRM_ERROR("wait_for_completion_timeout timeout!"); + *operation_result = AUX_RET_ERROR_TIMEOUT; + goto out; + } + + if (p_notify->result != AUX_RET_SUCCESS) { + /* + * Transient states before tunneling is enabled could + * lead to this error. We can ignore this for now. + */ + if (p_notify->result != AUX_RET_ERROR_PROTOCOL_ERROR) { + DRM_WARN("DPIA AUX failed on 0x%x(%d), error %d\n", + payload->address, payload->length, + p_notify->result); } - } else { - if (status_type == DMUB_ASYNC_TO_SYNC_ACCESS_SUCCESS) { - return_status = 0; - *operation_result = p_notify->sc_status; - } else { - *operation_result = SET_CONFIG_UNKNOWN_ERROR; + *operation_result = AUX_RET_ERROR_INVALID_REPLY; + goto out; + } + + + payload->reply[0] = adev->dm.dmub_notify->aux_reply.command; + if (!payload->write && p_notify->aux_reply.length && + (payload->reply[0] == AUX_TRANSACTION_REPLY_AUX_ACK)) { + + if (payload->length != p_notify->aux_reply.length) { + DRM_WARN("invalid read length %d from DPIA AUX 0x%x(%d)!\n", + p_notify->aux_reply.length, + payload->address, payload->length); + *operation_result = AUX_RET_ERROR_INVALID_REPLY; + goto out; } + + memcpy(payload->data, p_notify->aux_reply.data, + p_notify->aux_reply.length); } - return return_status; + /* success */ + ret = p_notify->aux_reply.length; + *operation_result = p_notify->result; +out: + mutex_unlock(&adev->dm.dpia_aux_lock); + return ret; } -int amdgpu_dm_process_dmub_aux_transfer_sync(bool is_cmd_aux, struct dc_context *ctx, - unsigned int link_index, void *cmd_payload, void *operation_result) +int amdgpu_dm_process_dmub_set_config_sync( + struct dc_context *ctx, + unsigned int link_index, + struct set_config_cmd_payload *payload, + enum set_config_status *operation_result) { struct amdgpu_device *adev = ctx->driver_context; - int ret = 0; + bool is_cmd_complete; + int ret; - if (is_cmd_aux) { - dc_process_dmub_aux_transfer_async(ctx->dc, - link_index, (struct aux_payload *)cmd_payload); - } else if (dc_process_dmub_set_config_async(ctx->dc, link_index, - (struct set_config_cmd_payload *)cmd_payload, - adev->dm.dmub_notify)) { - return amdgpu_dm_set_dmub_async_sync_status(is_cmd_aux, - ctx, DMUB_ASYNC_TO_SYNC_ACCESS_SUCCESS, - (uint32_t *)operation_result); - } + mutex_lock(&adev->dm.dpia_aux_lock); + is_cmd_complete = dc_process_dmub_set_config_async(ctx->dc, + link_index, payload, adev->dm.dmub_notify); - ret = wait_for_completion_timeout(&adev->dm.dmub_aux_transfer_done, 10 * HZ); - if (ret == 0) { + if (is_cmd_complete || wait_for_completion_timeout(&adev->dm.dmub_aux_transfer_done, 10 * HZ)) { + ret = 0; + *operation_result = adev->dm.dmub_notify->sc_status; + } else { DRM_ERROR("wait_for_completion_timeout timeout!"); - return amdgpu_dm_set_dmub_async_sync_status(is_cmd_aux, - ctx, DMUB_ASYNC_TO_SYNC_ACCESS_TIMEOUT, - (uint32_t *)operation_result); - } - - if (is_cmd_aux) { - if (adev->dm.dmub_notify->result == AUX_RET_SUCCESS) { - struct aux_payload *payload = (struct aux_payload *)cmd_payload; - - payload->reply[0] = adev->dm.dmub_notify->aux_reply.command; - if (!payload->write && adev->dm.dmub_notify->aux_reply.length && - payload->reply[0] == AUX_TRANSACTION_REPLY_AUX_ACK) { - - if (payload->length != adev->dm.dmub_notify->aux_reply.length) { - DRM_WARN("invalid read from DPIA AUX %x(%d) got length %d!\n", - payload->address, payload->length, - adev->dm.dmub_notify->aux_reply.length); - return amdgpu_dm_set_dmub_async_sync_status(is_cmd_aux, ctx, - DMUB_ASYNC_TO_SYNC_ACCESS_INVALID, - (uint32_t *)operation_result); - } - - memcpy(payload->data, adev->dm.dmub_notify->aux_reply.data, - adev->dm.dmub_notify->aux_reply.length); - } - } + ret = -1; + *operation_result = SET_CONFIG_UNKNOWN_ERROR; } - return amdgpu_dm_set_dmub_async_sync_status(is_cmd_aux, - ctx, DMUB_ASYNC_TO_SYNC_ACCESS_SUCCESS, - (uint32_t *)operation_result); + mutex_unlock(&adev->dm.dpia_aux_lock); + return ret; } /* diff --git a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.h index 518e3c90061..4780ac4b53b 100644 --- a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -59,7 +59,9 @@ #include "signal_types.h" #include "amdgpu_dm_crc.h" struct aux_payload; +struct set_config_cmd_payload; enum aux_return_code_type; +enum set_config_status; /* Forward declarations */ struct amdgpu_device; @@ -549,6 +551,13 @@ struct amdgpu_display_manager { * occurred on certain intel platform */ bool aux_hpd_discon_quirk; + + /** + * @dpia_aux_lock: + * + * Guards access to DPIA AUX + */ + struct rwlock dpia_aux_lock; }; enum dsc_clock_force_state { @@ -792,9 +801,11 @@ void amdgpu_dm_update_connector_after_detect( extern const struct drm_encoder_helper_funcs amdgpu_dm_encoder_helper_funcs; -int amdgpu_dm_process_dmub_aux_transfer_sync(bool is_cmd_aux, - struct dc_context *ctx, unsigned int link_index, - void *payload, void *operation_result); +int amdgpu_dm_process_dmub_aux_transfer_sync(struct dc_context *ctx, unsigned int link_index, + struct aux_payload *payload, enum aux_return_code_type *operation_result); + +int amdgpu_dm_process_dmub_set_config_sync(struct dc_context *ctx, unsigned int link_index, + struct set_config_cmd_payload *payload, enum set_config_status *operation_result); bool check_seamless_boot_capability(struct amdgpu_device *adev); diff --git a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 74d1599b10f..422c76a874c 100644 --- a/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/sys/dev/pci/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -844,9 +844,8 @@ int dm_helper_dmub_aux_transfer_sync( struct aux_payload *payload, enum aux_return_code_type *operation_result) { - return amdgpu_dm_process_dmub_aux_transfer_sync(true, ctx, - link->link_index, (void *)payload, - (void *)operation_result); + return amdgpu_dm_process_dmub_aux_transfer_sync(ctx, link->link_index, payload, + operation_result); } int dm_helpers_dmub_set_config_sync(struct dc_context *ctx, @@ -854,9 +853,8 @@ int dm_helpers_dmub_set_config_sync(struct dc_context *ctx, struct set_config_cmd_payload *payload, enum set_config_status *operation_result) { - return amdgpu_dm_process_dmub_aux_transfer_sync(false, ctx, - link->link_index, (void *)payload, - (void *)operation_result); + return amdgpu_dm_process_dmub_set_config_sync(ctx, link->link_index, payload, + operation_result); } void dm_set_dcn_clocks(struct dc_context *ctx, struct dc_clocks *clks)