Thanks for explanations Jani and Daniel. Feel free to use Reviewe-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> On Tue, Mar 18, 2014 at 7:54 AM, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > On Mon, 17 Mar 2014, 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. > > I'm confused, what are you referring to? drm_dp_dpcd_access() in > drm_dp_helper.c has the retry loop now. > > I think you're mixing this with the intel_dp_dpcd_read_wake() wrapper, > but that ends up in drm_dp_dpcd_access() as well. > >> 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. > > Ditto, we store ack >>= 4 into msg->reply which gets checked in > drm_dp_dpcd_access(). > > BR, > Jani. > >> 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); >>> 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 > > -- > Jani Nikula, Intel Open Source Technology Center -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx