On Mon, Mar 17, 2014 at 11:08 AM, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote: > On Fri, Mar 14, 2014 at 11:51 AM, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >> Functionality remains largely the same as before. >> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 257 +++++++++++++++++--------------------- >> drivers/gpu/drm/i915/intel_drv.h | 1 + >> 2 files changed, 116 insertions(+), 142 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 22134edc03dd..24cbf4bd36c4 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -575,97 +575,77 @@ out: >> return ret; >> } >> >> -/* Write data to the aux channel in native mode */ >> -static int >> -intel_dp_aux_native_write(struct intel_dp *intel_dp, >> - uint16_t address, uint8_t *send, int send_bytes) >> +#define HEADER_SIZE 4 >> +static ssize_t >> +intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) >> { >> + struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux); >> + uint8_t txbuf[20], rxbuf[20]; >> + size_t txsize, rxsize; >> int ret; >> - uint8_t msg[20]; >> - int msg_bytes; >> - uint8_t ack; >> - int retry; >> >> - if (WARN_ON(send_bytes > 16)) >> - return -E2BIG; >> + txbuf[0] = msg->request << 4; >> + txbuf[1] = msg->address >> 8; >> + txbuf[2] = msg->address & 0xff; >> + txbuf[3] = msg->size - 1; >> >> - intel_dp_check_edp(intel_dp); >> - msg[0] = DP_AUX_NATIVE_WRITE << 4; >> - msg[1] = address >> 8; >> - msg[2] = address & 0xff; >> - msg[3] = send_bytes - 1; >> - memcpy(&msg[4], send, send_bytes); >> - msg_bytes = send_bytes + 4; >> - for (retry = 0; retry < 7; retry++) { > > we were retrying 7 times always, but now we are now retrying only 3 > times or even none. > Couldn't it trigger some new bug or regression? > >> - ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, &ack, 1); >> - if (ret < 0) >> - return ret; >> - ack >>= 4; >> - if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) >> - return send_bytes; >> - else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER) >> - usleep_range(400, 500); > > we are also not checking this ack x defer (here nor in native_write) > replies anymore. > Don't we need it anymore? why? > >> - else >> - return -EIO; >> - } >> + switch (msg->request & ~DP_AUX_I2C_MOT) { >> + case DP_AUX_NATIVE_WRITE: >> + case DP_AUX_I2C_WRITE: >> + txsize = HEADER_SIZE + msg->size; >> + rxsize = 1; >> >> - DRM_ERROR("too many retries, giving up\n"); >> - return -EIO; >> -} >> + if (WARN_ON(txsize > 20)) >> + return -E2BIG; >> >> -/* Write a single byte to the aux channel in native mode */ >> -static int >> -intel_dp_aux_native_write_1(struct intel_dp *intel_dp, >> - uint16_t address, uint8_t byte) >> -{ >> - return intel_dp_aux_native_write(intel_dp, address, &byte, 1); >> -} >> + memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size); >> >> -/* read bytes from a native aux channel */ >> -static int >> -intel_dp_aux_native_read(struct intel_dp *intel_dp, >> - uint16_t address, uint8_t *recv, int recv_bytes) >> -{ >> - uint8_t msg[4]; >> - int msg_bytes; >> - uint8_t reply[20]; >> - int reply_bytes; >> - uint8_t ack; >> - int ret; >> - int retry; >> + ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize); >> + if (ret > 0) { >> + msg->reply = rxbuf[0] >> 4; >> >> - if (WARN_ON(recv_bytes > 19)) >> - return -E2BIG; >> + /* Return payload size. */ >> + ret = msg->size; >> + } >> + break; >> >> - intel_dp_check_edp(intel_dp); >> - msg[0] = DP_AUX_NATIVE_READ << 4; >> - msg[1] = address >> 8; >> - msg[2] = address & 0xff; >> - msg[3] = recv_bytes - 1; >> + case DP_AUX_NATIVE_READ: >> + case DP_AUX_I2C_READ: >> + txsize = HEADER_SIZE; >> + rxsize = msg->size + 1; >> >> - msg_bytes = 4; >> - reply_bytes = recv_bytes + 1; >> + if (WARN_ON(rxsize > 20)) >> + return -E2BIG; >> >> - for (retry = 0; retry < 7; retry++) { >> - ret = intel_dp_aux_ch(intel_dp, msg, msg_bytes, >> - reply, reply_bytes); >> - if (ret == 0) >> - return -EPROTO; >> - if (ret < 0) >> - return ret; >> - ack = reply[0] >> 4; >> - if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_ACK) { >> - memcpy(recv, reply + 1, ret - 1); >> - return ret - 1; >> + ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize); >> + if (ret > 0) { >> + msg->reply = rxbuf[0] >> 4; >> + /* >> + * Assume happy day, and copy the data. The caller is >> + * expected to check msg->reply before touching it. >> + * >> + * Return payload size. >> + */ >> + ret--; >> + memcpy(msg->buffer, rxbuf + 1, ret); >> } >> - else if ((ack & DP_AUX_NATIVE_REPLY_MASK) == DP_AUX_NATIVE_REPLY_DEFER) >> - usleep_range(400, 500); >> - else >> - return -EIO; >> + break; >> + >> + default: >> + ret = -EINVAL; >> + break; >> } >> >> - DRM_ERROR("too many retries, giving up\n"); >> - return -EIO; >> + return ret; >> +} >> + >> +static void >> +intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector *connector) >> +{ >> + struct drm_device *dev = intel_dp_to_dev(intel_dp); >> + >> + intel_dp->aux.dev = dev->dev; >> + intel_dp->aux.transfer = intel_dp_aux_transfer; >> } >> >> static int >> @@ -1472,8 +1452,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) >> return; >> >> if (mode != DRM_MODE_DPMS_ON) { >> - ret = intel_dp_aux_native_write_1(intel_dp, DP_SET_POWER, >> - DP_SET_POWER_D3); >> + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, >> + DP_SET_POWER_D3); >> if (ret != 1) >> DRM_DEBUG_DRIVER("failed to write sink power state\n"); >> } else { >> @@ -1482,9 +1462,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) >> * time to wake up. >> */ >> for (i = 0; i < 3; i++) { >> - ret = intel_dp_aux_native_write_1(intel_dp, >> - DP_SET_POWER, >> - DP_SET_POWER_D0); >> + ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER, >> + DP_SET_POWER_D0); >> if (ret == 1) >> break; >> msleep(1); >> @@ -1708,13 +1687,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp) >> >> /* Enable PSR in sink */ >> if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) >> - intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> - DP_PSR_ENABLE & >> - ~DP_PSR_MAIN_LINK_ACTIVE); >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, >> + DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE); >> else >> - intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG, >> - DP_PSR_ENABLE | >> - DP_PSR_MAIN_LINK_ACTIVE); >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, >> + DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE); >> >> /* Setup AUX registers */ >> I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND); >> @@ -2026,26 +2003,25 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder) >> /* >> * Native read with retry for link status and receiver capability reads for >> * cases where the sink may still be asleep. >> + * >> + * Sinks are *supposed* to come up within 1ms from an off state, but we're also >> + * supposed to retry 3 times per the spec. >> */ >> -static bool >> -intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address, >> - uint8_t *recv, int recv_bytes) >> +static ssize_t >> +intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset, >> + void *buffer, size_t size) >> { >> - int ret, i; >> + ssize_t ret; >> + int i; >> >> - /* >> - * Sinks are *supposed* to come up within 1ms from an off state, >> - * but we're also supposed to retry 3 times per the spec. >> - */ >> for (i = 0; i < 3; i++) { >> - ret = intel_dp_aux_native_read(intel_dp, address, recv, >> - recv_bytes); >> - if (ret == recv_bytes) >> - return true; >> + ret = drm_dp_dpcd_read(aux, offset, buffer, size); >> + if (ret == size) >> + return ret; >> msleep(1); >> } >> >> - return false; >> + return ret; >> } >> >> /* >> @@ -2055,10 +2031,10 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address, >> static bool >> intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]) >> { >> - return intel_dp_aux_native_read_retry(intel_dp, >> - DP_LANE0_1_STATUS, >> - link_status, >> - DP_LINK_STATUS_SIZE); >> + return intel_dp_dpcd_read_wake(&intel_dp->aux, >> + DP_LANE0_1_STATUS, >> + link_status, >> + DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE; >> } >> >> /* >> @@ -2572,8 +2548,8 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, >> len = intel_dp->lane_count + 1; >> } >> >> - ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_PATTERN_SET, >> - buf, len); >> + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, >> + buf, len); >> >> return ret == len; >> } >> @@ -2602,9 +2578,8 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, >> I915_WRITE(intel_dp->output_reg, *DP); >> POSTING_READ(intel_dp->output_reg); >> >> - ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET, >> - intel_dp->train_set, >> - intel_dp->lane_count); >> + ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET, >> + intel_dp->train_set, intel_dp->lane_count); >> >> return ret == intel_dp->lane_count; >> } >> @@ -2660,11 +2635,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) >> link_config[1] = intel_dp->lane_count; >> if (drm_dp_enhanced_frame_cap(intel_dp->dpcd)) >> link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN; >> - intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET, link_config, 2); >> + drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2); >> >> link_config[0] = 0; >> link_config[1] = DP_SET_ANSI_8B10B; >> - intel_dp_aux_native_write(intel_dp, DP_DOWNSPREAD_CTRL, link_config, 2); >> + drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2); >> >> DP |= DP_PORT_EN; >> >> @@ -2907,8 +2882,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >> >> char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; >> >> - if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, >> - sizeof(intel_dp->dpcd)) == 0) >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd, >> + sizeof(intel_dp->dpcd)) < 0) >> return false; /* aux transfer failed */ >> >> hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd), >> @@ -2921,9 +2896,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >> /* Check if the panel supports PSR */ >> memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); >> if (is_edp(intel_dp)) { >> - intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT, >> - intel_dp->psr_dpcd, >> - sizeof(intel_dp->psr_dpcd)); >> + intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT, >> + intel_dp->psr_dpcd, >> + sizeof(intel_dp->psr_dpcd)); >> if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) { >> dev_priv->psr.sink_support = true; >> DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); >> @@ -2945,9 +2920,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >> if (intel_dp->dpcd[DP_DPCD_REV] == 0x10) >> return true; /* no per-port downstream info */ >> >> - if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0, >> - intel_dp->downstream_ports, >> - DP_MAX_DOWNSTREAM_PORTS) == 0) >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0, >> + intel_dp->downstream_ports, >> + DP_MAX_DOWNSTREAM_PORTS) < 0) >> return false; /* downstream port status fetch failed */ >> >> return true; >> @@ -2963,11 +2938,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp) >> >> edp_panel_vdd_on(intel_dp); >> >> - if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3)) >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3) >> DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n", >> buf[0], buf[1], buf[2]); >> >> - if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3)) >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3) >> DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n", >> buf[0], buf[1], buf[2]); >> >> @@ -2982,46 +2957,40 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) >> to_intel_crtc(intel_dig_port->base.base.crtc); >> u8 buf[1]; >> >> - if (!intel_dp_aux_native_read(intel_dp, DP_TEST_SINK_MISC, buf, 1)) >> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, buf) < 0); wrong ";" here.. >> return -EAGAIN; >> >> if (!(buf[0] & DP_TEST_CRC_SUPPORTED)) >> return -ENOTTY; >> >> - if (!intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, >> - DP_TEST_SINK_START)) >> + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, >> + DP_TEST_SINK_START) < 0) >> return -EAGAIN; >> >> /* Wait 2 vblanks to be sure we will have the correct CRC value */ >> intel_wait_for_vblank(dev, intel_crtc->pipe); >> intel_wait_for_vblank(dev, intel_crtc->pipe); >> >> - if (!intel_dp_aux_native_read(intel_dp, DP_TEST_CRC_R_CR, crc, 6)) >> + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) >> return -EAGAIN; >> >> - intel_dp_aux_native_write_1(intel_dp, DP_TEST_SINK, 0); >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); >> return 0; >> } >> >> static bool >> intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector) >> { >> - int ret; >> - >> - ret = intel_dp_aux_native_read_retry(intel_dp, >> - DP_DEVICE_SERVICE_IRQ_VECTOR, >> - sink_irq_vector, 1); >> - if (!ret) >> - return false; >> - >> - return true; >> + return intel_dp_dpcd_read_wake(&intel_dp->aux, >> + DP_DEVICE_SERVICE_IRQ_VECTOR, >> + sink_irq_vector, 1) == 1; >> } >> >> static void >> intel_dp_handle_test_request(struct intel_dp *intel_dp) >> { >> /* NAK by default */ >> - intel_dp_aux_native_write_1(intel_dp, DP_TEST_RESPONSE, DP_TEST_NAK); >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_RESPONSE, DP_TEST_NAK); >> } >> >> /* >> @@ -3060,9 +3029,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) >> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && >> intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) { >> /* Clear interrupt source */ >> - intel_dp_aux_native_write_1(intel_dp, >> - DP_DEVICE_SERVICE_IRQ_VECTOR, >> - sink_irq_vector); >> + drm_dp_dpcd_writeb(&intel_dp->aux, >> + DP_DEVICE_SERVICE_IRQ_VECTOR, >> + sink_irq_vector); >> >> if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) >> intel_dp_handle_test_request(intel_dp); >> @@ -3097,9 +3066,11 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) >> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && >> intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { >> uint8_t reg; >> - if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT, >> - ®, 1)) >> + >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >> + ®, 1) < 0) >> return connector_status_unknown; >> + >> return DP_GET_SINK_COUNT(reg) ? connector_status_connected >> : connector_status_disconnected; >> } >> @@ -3925,6 +3896,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, >> intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); >> } >> >> + intel_dp_aux_init(intel_dp, intel_connector); >> + >> error = intel_dp_i2c_init(intel_dp, intel_connector, name); >> WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n", >> error, port_name(port)); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 2546cae0b4f0..561af4b013b1 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -491,6 +491,7 @@ struct intel_dp { >> uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS]; >> struct i2c_adapter adapter; >> struct i2c_algo_dp_aux_data algo; >> + struct drm_dp_aux aux; >> uint8_t train_set[4]; >> int panel_power_up_delay; >> int panel_power_down_delay; >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx