From: Dominik Kaszewski <[email protected]> [Why] link_dpms.c issues I2C writes during HDMI link enablement. Current implementation contains a lot of duplicated code with copy-paste errors.
[How] * Refactor common logic into helper functions. * Invert logic with early returns to decrease indentation. * Sequence writes by looping over data arrays. * Fix write_i2c_retimer_setting is_over_340mhz checking reg_settings instead of reg_settings_6g in the i2c_reg_index <= 0x20 check. Reviewed-by: Wenjing Liu <[email protected]> Signed-off-by: Dominik Kaszewski <[email protected]> Signed-off-by: Ray Wu <[email protected]> --- .../gpu/drm/amd/display/dc/link/link_dpms.c | 475 ++++++------------ 1 file changed, 165 insertions(+), 310 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c index 91742bde4dc2..befbe005af68 100644 --- a/drivers/gpu/drm/amd/display/dc/link/link_dpms.c +++ b/drivers/gpu/drm/amd/display/dc/link/link_dpms.c @@ -320,344 +320,203 @@ static bool get_ext_hdmi_settings(struct pipe_ctx *pipe_ctx, return result; } -static bool write_i2c(struct pipe_ctx *pipe_ctx, - uint8_t address, uint8_t *buffer, uint32_t length) +static bool write_i2c( + const struct dc_link *link, + uint8_t address, + uint8_t *buffer, + uint32_t length +) { - struct i2c_command cmd = {0}; - struct i2c_payload payload = {0}; + struct i2c_payload payload = { + .write = true, + .address = address, + .length = length, + .data = buffer, + }; + struct i2c_command cmd = { + .payloads = &payload, + .number_of_payloads = 1, + .engine = I2C_COMMAND_ENGINE_DEFAULT, + .speed = link->ctx->dc->caps.i2c_speed_in_khz, + }; + + return dm_helpers_submit_i2c(link->ctx, link, &cmd); +} - memset(&payload, 0, sizeof(payload)); - memset(&cmd, 0, sizeof(cmd)); +static bool write_i2c_retimer_offset_value( + const struct dc_link *link, + uint8_t address, + uint8_t offset, + uint8_t value +) +{ + DC_LOGGER_INIT(link->ctx->logger); + uint8_t buffer[] = { offset, value }; + const bool success = write_i2c(link, address, buffer, sizeof(buffer)); + + RETIMER_REDRIVER_INFO( + "Retimer write, address: 0x%x, offset: 0x%x, value: 0x%x, success: %d\n", + address, offset, value, success + ); + return success; +} - cmd.number_of_payloads = 1; - cmd.engine = I2C_COMMAND_ENGINE_DEFAULT; - cmd.speed = pipe_ctx->stream->ctx->dc->caps.i2c_speed_in_khz; +static bool write_i2c_retimer_vga( + const struct dc_link *link, + uint8_t address +) +{ + DC_LOGGER_INIT(link->ctx->logger); + const uint8_t vga_data[][2] = { + { 0xFF, 0x01 }, + { 0x00, 0x23 }, + { 0xFF, 0x00 }, + }; + + for (size_t i = 0; i < ARRAY_SIZE(vga_data); i++) { + if (!write_i2c_retimer_offset_value(link, address, vga_data[i][0], vga_data[i][1])) { + DC_LOG_ERROR("Set retimer failed, vga index: %zu\n", i); + return false; + } + } + return true; +} - payload.address = address; - payload.data = buffer; - payload.length = length; - payload.write = true; - cmd.payloads = &payload; +static bool write_i2c_retimer_byte( + const struct dc_link *link, + uint8_t address, + uint8_t index, + uint8_t value +) +{ + DC_LOGGER_INIT(link->ctx->logger); + const uint8_t apply_rx_tx_change = 0x4; - if (dm_helpers_submit_i2c(pipe_ctx->stream->ctx, - pipe_ctx->stream->link, &cmd)) + if (index > 0x20) return true; - return false; + if (!write_i2c_retimer_offset_value(link, address, index, value)) { + DC_LOG_ERROR("Set retimer failed, 3g index: 0x%x, value: 0x%x\n", index, value); + return false; + } + + // Based on DP159 specs, APPLY_RX_TX_CHANGE bit in 0x0A + // needs to be set to 1 on every 0x0A-0x0C write. + if (0x0A <= index && index <= 0x0C) { + uint8_t offset = 0x0A; + + // Query current value from offset 0x0A + if (index == 0x0A) { + // Just written correct value, so no need to read it back + } else { + if (!link_query_ddc_data( + link->ddc, address, &offset, 1, &value, 1 + )) { + DC_LOG_ERROR("Set retimer failed, link_query_ddc_data\n"); + return false; + } + } + + value |= apply_rx_tx_change; + if (!write_i2c_retimer_offset_value(link, address, offset, value)) { + DC_LOG_ERROR("Set retimer failed, 3g offset: 0x%x, value: 0x%x\n", offset, value); + return false; + } + } + return true; } -static void write_i2c_retimer_setting( - struct pipe_ctx *pipe_ctx, +static bool write_i2c_retimer_setting( + const struct dc_link *link, bool is_vga_mode, bool is_over_340mhz, struct ext_hdmi_settings *settings) { - uint8_t slave_address = (settings->slv_addr >> 1); - uint8_t buffer[2]; - const uint8_t apply_rx_tx_change = 0x4; - uint8_t offset = 0xA; - uint8_t value = 0; - int i = 0; - bool i2c_success = false; - DC_LOGGER_INIT(pipe_ctx->stream->ctx->logger); - - memset(&buffer, 0, sizeof(buffer)); - - /* Start Ext-Hdmi programming*/ - - for (i = 0; i < settings->reg_num; i++) { - /* Apply 3G settings */ - if (settings->reg_settings[i].i2c_reg_index <= 0x20) { - - buffer[0] = settings->reg_settings[i].i2c_reg_index; - buffer[1] = settings->reg_settings[i].i2c_reg_val; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val= 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); + DC_LOGGER_INIT(link->ctx->logger); + const uint8_t address = settings->slv_addr >> 1; - if (!i2c_success) - goto i2c_write_fail; + for (size_t i = 0; i < settings->reg_num; i++) { + const uint8_t index = settings->reg_settings[i].i2c_reg_index; + uint8_t value = settings->reg_settings[i].i2c_reg_val; - /* Based on DP159 specs, APPLY_RX_TX_CHANGE bit in 0x0A - * needs to be set to 1 on every 0xA-0xC write. - */ - if (settings->reg_settings[i].i2c_reg_index == 0xA || - settings->reg_settings[i].i2c_reg_index == 0xB || - settings->reg_settings[i].i2c_reg_index == 0xC) { - - /* Query current value from offset 0xA */ - if (settings->reg_settings[i].i2c_reg_index == 0xA) - value = settings->reg_settings[i].i2c_reg_val; - else { - i2c_success = - link_query_ddc_data( - pipe_ctx->stream->link->ddc, - slave_address, &offset, 1, &value, 1); - if (!i2c_success) - goto i2c_write_fail; - } - - buffer[0] = offset; - /* Set APPLY_RX_TX_CHANGE bit to 1 */ - buffer[1] = value | apply_rx_tx_change; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - } + if (!write_i2c_retimer_byte(link, address, index, value)) { + DC_LOG_ERROR("Set retimer failed, index: %zu\n", i); + return false; } } - /* Apply 3G settings */ if (is_over_340mhz) { - for (i = 0; i < settings->reg_num_6g; i++) { - /* Apply 3G settings */ - if (settings->reg_settings[i].i2c_reg_index <= 0x20) { - - buffer[0] = settings->reg_settings_6g[i].i2c_reg_index; - buffer[1] = settings->reg_settings_6g[i].i2c_reg_val; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("above 340Mhz: retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - - if (!i2c_success) - goto i2c_write_fail; - - /* Based on DP159 specs, APPLY_RX_TX_CHANGE bit in 0x0A - * needs to be set to 1 on every 0xA-0xC write. - */ - if (settings->reg_settings_6g[i].i2c_reg_index == 0xA || - settings->reg_settings_6g[i].i2c_reg_index == 0xB || - settings->reg_settings_6g[i].i2c_reg_index == 0xC) { - - /* Query current value from offset 0xA */ - if (settings->reg_settings_6g[i].i2c_reg_index == 0xA) - value = settings->reg_settings_6g[i].i2c_reg_val; - else { - i2c_success = - link_query_ddc_data( - pipe_ctx->stream->link->ddc, - slave_address, &offset, 1, &value, 1); - if (!i2c_success) - goto i2c_write_fail; - } + for (size_t i = 0; i < settings->reg_num_6g; i++) { + const uint8_t index = settings->reg_settings_6g[i].i2c_reg_index; + uint8_t value = settings->reg_settings_6g[i].i2c_reg_val; - buffer[0] = offset; - /* Set APPLY_RX_TX_CHANGE bit to 1 */ - buffer[1] = value | apply_rx_tx_change; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - } + if (!write_i2c_retimer_byte(link, address, index, value)) { + DC_LOG_ERROR("Set retimer failed, 6g index: %zu\n", i); + return false; } } } if (is_vga_mode) { - /* Program additional settings if using 640x480 resolution */ - - /* Write offset 0xFF to 0x01 */ - buffer[0] = 0xff; - buffer[1] = 0x01; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x00 to 0x23 */ - buffer[0] = 0x00; - buffer[1] = 0x23; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0xff to 0x00 */ - buffer[0] = 0xff; - buffer[1] = 0x00; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - } - - return; - -i2c_write_fail: - DC_LOG_DEBUG("Set retimer failed"); + return write_i2c_retimer_vga(link, address); + } + return true; } -static void write_i2c_default_retimer_setting( - struct pipe_ctx *pipe_ctx, +static bool write_i2c_default_retimer_setting( + const struct dc_link *link, bool is_vga_mode, bool is_over_340mhz) { - uint8_t slave_address = (0xBA >> 1); - uint8_t buffer[2]; - bool i2c_success = false; - DC_LOGGER_INIT(pipe_ctx->stream->ctx->logger); + const uint8_t address = 0xBA >> 1; - memset(&buffer, 0, sizeof(buffer)); - - /* Program Slave Address for tuning single integrity */ - /* Write offset 0x0A to 0x13 */ - buffer[0] = 0x0A; - buffer[1] = 0x13; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer writes default setting to slave_address = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x0A to 0x17 */ - buffer[0] = 0x0A; - buffer[1] = 0x17; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x0B to 0xDA or 0xD8 */ - buffer[0] = 0x0B; - buffer[1] = is_over_340mhz ? 0xDA : 0xD8; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x0A to 0x17 */ - buffer[0] = 0x0A; - buffer[1] = 0x17; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val= 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x0C to 0x1D or 0x91 */ - buffer[0] = 0x0C; - buffer[1] = is_over_340mhz ? 0x1D : 0x91; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x0A to 0x17 */ - buffer[0] = 0x0A; - buffer[1] = 0x17; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; + DC_LOGGER_INIT(link->ctx->logger); + const uint8_t data[][2] = { + { 0x0A, 0x13 }, + { 0x0A, 0x17 }, + { 0x0B, is_over_340mhz ? 0xDA : 0xD8 }, + { 0x0A, 0x17 }, + { 0x0C, is_over_340mhz ? 0x1D : 0x91 }, + { 0x0A, 0x17 }, + }; + + for (size_t i = 0; i < ARRAY_SIZE(data); i++) { + if (!write_i2c_retimer_offset_value(link, address, data[i][0], data[i][1])) { + DC_LOG_ERROR("Set default retimer failed, index: %zu\n", i); + return false; + } + } if (is_vga_mode) { - /* Program additional settings if using 640x480 resolution */ - - /* Write offset 0xFF to 0x01 */ - buffer[0] = 0xff; - buffer[1] = 0x01; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0x00 to 0x23 */ - buffer[0] = 0x00; - buffer[1] = 0x23; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write to slave_addr = 0x%x,\ - offset = 0x%x, reg_val= 0x%x, i2c_success = %d\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - - /* Write offset 0xff to 0x00 */ - buffer[0] = 0xff; - buffer[1] = 0x00; - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("retimer write default setting to slave_addr = 0x%x,\ - offset = 0x%x, reg_val= 0x%x, i2c_success = %d end here\n", - slave_address, buffer[0], buffer[1], i2c_success?1:0); - if (!i2c_success) - goto i2c_write_fail; - } - - return; - -i2c_write_fail: - DC_LOG_DEBUG("Set default retimer failed"); + return write_i2c_retimer_vga(link, address); + } + return true; } -static void write_i2c_redriver_setting( - struct pipe_ctx *pipe_ctx, +static bool write_i2c_redriver_setting( + const struct dc_link *link, bool is_over_340mhz) { - uint8_t slave_address = (0xF0 >> 1); - uint8_t buffer[16]; - bool i2c_success = false; - DC_LOGGER_INIT(pipe_ctx->stream->ctx->logger); - - memset(&buffer, 0, sizeof(buffer)); - - // Program Slave Address for tuning single integrity - buffer[3] = 0x4E; - buffer[4] = 0x4E; - buffer[5] = 0x4E; - buffer[6] = is_over_340mhz ? 0x4E : 0x4A; - - i2c_success = write_i2c(pipe_ctx, slave_address, - buffer, sizeof(buffer)); - RETIMER_REDRIVER_INFO("redriver write 0 to all 16 reg offset expect following:\n\ - \t slave_addr = 0x%x, offset[3] = 0x%x, offset[4] = 0x%x,\ - offset[5] = 0x%x,offset[6] is_over_340mhz = 0x%x,\ - i2c_success = %d\n", - slave_address, buffer[3], buffer[4], buffer[5], buffer[6], i2c_success?1:0); - - if (!i2c_success) - DC_LOG_DEBUG("Set redriver failed"); + DC_LOGGER_INIT(link->ctx->logger); + const uint8_t address = 0xF0 >> 1; + uint8_t buffer[16] = { + [3] = 0x4E, + [4] = 0x4E, + [5] = 0x4E, + [6] = is_over_340mhz ? 0x4E : 0x4A, + }; + + const bool success = write_i2c(link, address, buffer, sizeof(buffer)); + + RETIMER_REDRIVER_INFO( + "Redriver write, address: 0x%x, buffer: { [3]: 0x%x, 0x%x, 0x%x, 0x%x }, success: %d\n", + address, buffer[3], buffer[4], buffer[5], buffer[6], success + ); + + if (!success) + DC_LOG_ERROR("Set redriver failed"); + return success; } static void update_psp_stream_config(struct pipe_ctx *pipe_ctx, bool dpms_off) @@ -1934,15 +1793,13 @@ static void enable_link_hdmi(struct pipe_ctx *pipe_ctx) eng_id = pipe_ctx->stream_res.stream_enc->id; if (get_ext_hdmi_settings(pipe_ctx, eng_id, &settings)) { - write_i2c_retimer_setting(pipe_ctx, - is_vga_mode, is_over_340mhz, &settings); + write_i2c_retimer_setting(link, is_vga_mode, is_over_340mhz, &settings); } else { - write_i2c_default_retimer_setting(pipe_ctx, - is_vga_mode, is_over_340mhz); + write_i2c_default_retimer_setting(link, is_vga_mode, is_over_340mhz); } } else if (masked_chip_caps == AMD_EXT_DISPLAY_PATH_CAPS__HDMI20_PI3EQX1204) { /* PI3EQX1204, Redriver settings */ - write_i2c_redriver_setting(pipe_ctx, is_over_340mhz); + write_i2c_redriver_setting(link, is_over_340mhz); } } @@ -2353,14 +2210,12 @@ void link_set_dpms_off(struct pipe_ctx *pipe_ctx) if (masked_chip_caps == AMD_EXT_DISPLAY_PATH_CAPS__HDMI20_TISN65DP159RSBT) { /* DP159, Retimer settings */ if (get_ext_hdmi_settings(pipe_ctx, eng_id, &settings)) - write_i2c_retimer_setting(pipe_ctx, - false, false, &settings); + write_i2c_retimer_setting(link, false, false, &settings); else - write_i2c_default_retimer_setting(pipe_ctx, - false, false); + write_i2c_default_retimer_setting(link, false, false); } else if (masked_chip_caps == AMD_EXT_DISPLAY_PATH_CAPS__HDMI20_PI3EQX1204) { /* PI3EQX1204, Redriver settings */ - write_i2c_redriver_setting(pipe_ctx, false); + write_i2c_redriver_setting(link, false); } } -- 2.43.0
