This patchset addresses a couple of issues with the i915 gmbus implementation: * fixes misassigned pin port pair for HDMI-D * fixes write transactions when they are the only transaction requested (including large >4-byte writes) by terminating every transaction with a WAIT cycle. * returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled i2c transaction failures * optimizes the typical read transaction case by using the INDEX cycle * turns on the GMBUS interrupt whenever possible to dramatically improve throughput by eliminating the very slow polling loop.
The patchset should apply cleanly onto linus/master. It is inspired by, but completely supercedes, a similar patch submitted recently by Benson Leung (bleung@chromium.org). I'd be happy to rebase on a different tree if necessary.
v2 of the patchset address review comments from Chris Wilson (thanks for the review, Chris!), and adds the interrupt patch. There weren't any review comments for patches 5, 7, or 8 of the first set. Hopefully they will get more love the second time around :).
Daniel Kurtz (10): drm/i915/intel_i2c: cleanup drm/i915/intel_i2c: assign HDMI port D to pin pair 6 drm/i915/intel_i2c: refactor using intel_gmbus_get_adapter drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments drm/i915/intel_i2c: add locking around i2c algorithm accesses drm/i915/intel_i2c: return -ENXIO for device NAK drm/i915/intel_i2c: use WAIT cycle, not STOP drm/i915/intel_i2c: use INDEX cycles for i2c read transactions drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop drm/i915/intel_i2c: enable gmbus interrupts
drivers/gpu/drm/i915/i915_drv.h | 7 +- drivers/gpu/drm/i915/i915_irq.c | 22 +++- drivers/gpu/drm/i915/i915_reg.h | 6 +- drivers/gpu/drm/i915/intel_bios.c | 11 +- drivers/gpu/drm/i915/intel_crt.c | 13 +- drivers/gpu/drm/i915/intel_drv.h | 9 ++- drivers/gpu/drm/i915/intel_dvo.c | 4 +- drivers/gpu/drm/i915/intel_hdmi.c | 29 ++--- drivers/gpu/drm/i915/intel_i2c.c | 236 ++++++++++++++++++++++++++++-------- drivers/gpu/drm/i915/intel_lvds.c | 3 +- drivers/gpu/drm/i915/intel_modes.c | 6 +- drivers/gpu/drm/i915/intel_sdvo.c | 10 +- 12 files changed, 257 insertions(+), 99 deletions(-)
80 col, spaces around operators and other basic cleanup.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index d30cccc..5e413c4 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -253,11 +253,13 @@ gmbus_xfer(struct i2c_adapter *adapter, (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset); do { u32 val, loop = 0;
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) goto clear_err; @@ -282,10 +284,12 @@ gmbus_xfer(struct i2c_adapter *adapter, (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset);
while (len) { - if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) goto clear_err; @@ -296,11 +300,14 @@ gmbus_xfer(struct i2c_adapter *adapter, } while (--len && ++loop < 4);
I915_WRITE(GMBUS3 + reg_offset, val); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset); } }
- if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) + if (i + 1 < num && + wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), + 50)) goto timeout; if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) goto clear_err;
According to i915 documentation [1], "Port D" (DP/HDMI Port D) is actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b). Pin pair 7 is a reserved pair.
[1] Documentation for [DevSNB+] and [DevIBX], as found on http://intellinuxgraphics.org
Note: the "reserved" and "disabled" pairs do not actually map to a physical pair of pins, nor GPIO regs and shouldn't be initialized or used. Fixing this is left for a later patch.
This bug has not been noticed for two reasons: 1) "gmbus" mode is currently disabled - all transfers are actually using "bit-bang" mode which uses the GPIO port 5 (the "HDMI/DPD CTLDATA/CLK" pair), at register 0x5024 (defined as GPIOF i915_reg.h). Since this is the correct pair of pins for HDMI1, transfers succeed.
2) Even if gmbus mode is re-enabled, the first attempted transaction will fail because it tries to use the wrong ("Reserved") pin pair. However, the driver immediately falls back again to the bit-bang method, which correctly uses GPIOF, so again, transfers succeed.
However, if gmbus mode is re-enabled and the GPIO fall-back mode is disabled, then reading an attached monitor's EDID fail.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_reg.h | 6 +++--- drivers/gpu/drm/i915/intel_i2c.c | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 03c53fc..56af0df 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -697,9 +697,9 @@ #define GMBUS_PORT_PANEL 3 #define GMBUS_PORT_DPC 4 /* HDMIC */ #define GMBUS_PORT_DPB 5 /* SDVO, HDMIB */ - /* 6 reserved */ -#define GMBUS_PORT_DPD 7 /* HDMID */ -#define GMBUS_NUM_PORTS 8 +#define GMBUS_PORT_DPD 6 /* HDMID */ +#define GMBUS_PORT_RESERVED 7 /* 7 reserved */ +#define GMBUS_NUM_PORTS 8 #define GMBUS1 0x5104 /* command/status */ #define GMBUS_SW_CLR_INT (1<<31) #define GMBUS_SW_RDY (1<<30) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 5e413c4..1135e3e 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -157,8 +157,8 @@ intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin) GPIOC, GPIOD, GPIOE, - 0, GPIOF, + 0, }; struct intel_gpio *gpio;
@@ -175,7 +175,7 @@ intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin) gpio->dev_priv = dev_priv;
snprintf(gpio->adapter.name, sizeof(gpio->adapter.name), - "i915 GPIO%c", "?BACDE?F"[pin]); + "i915 GPIO%c", "?BACDEF?"[pin]); gpio->adapter.owner = THIS_MODULE; gpio->adapter.algo_data = &gpio->algo; gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev; @@ -376,8 +376,8 @@ int intel_setup_gmbus(struct drm_device *dev) "panel", "dpc", "dpb", - "reserved", "dpd", + "reserved", }; struct drm_i915_private *dev_priv = dev->dev_private; int ret, i;
Instead of letting other modules directly access the ->gmbus array, introduce a new API, intel_gmbus_get_adapter(), to lookup an i2c_adapter for a given gmbus pin pair identifier. This API enables later refactoring of the gmbus pin pair list.
Note: It is critical that intel_setup_gmbus() gets called before intel_gmbus_get_adapter().
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 4 +++- drivers/gpu/drm/i915/intel_bios.c | 11 +++++++---- drivers/gpu/drm/i915/intel_crt.c | 13 ++++++------- drivers/gpu/drm/i915/intel_drv.h | 3 ++- drivers/gpu/drm/i915/intel_dvo.c | 4 ++-- drivers/gpu/drm/i915/intel_hdmi.c | 29 ++++++++++++++--------------- drivers/gpu/drm/i915/intel_i2c.c | 7 +++++++ drivers/gpu/drm/i915/intel_lvds.c | 3 ++- drivers/gpu/drm/i915/intel_modes.c | 6 +++--- drivers/gpu/drm/i915/intel_sdvo.c | 10 ++++------ 10 files changed, 50 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9689ca3..9a81e48 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -391,7 +391,7 @@ typedef struct drm_i915_private {
struct notifier_block lid_notifier;
- int crt_ddc_pin; + struct i2c_adapter *crt_ddc; struct drm_i915_fence_reg fence_regs[I915_MAX_NUM_FENCES]; /* assume 965 */ int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */ int num_fence_regs; /* 8 on pre-965, 16 otherwise */ @@ -1274,6 +1274,8 @@ extern int i915_restore_state(struct drm_device *dev); /* intel_i2c.c */ extern int intel_setup_gmbus(struct drm_device *dev); extern void intel_teardown_gmbus(struct drm_device *dev); +extern struct i2c_adapter *intel_gmbus_get_adapter( + struct drm_i915_private *dev_priv, unsigned pin); extern void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed); extern void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit); extern inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index 63880e2..00e751f 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -330,12 +330,14 @@ parse_general_definitions(struct drm_i915_private *dev_priv, u16 block_size = get_blocksize(general); if (block_size >= sizeof(*general)) { int bus_pin = general->crt_ddc_gmbus_pin; + struct i2c_adapter *i2c; DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin); - if (bus_pin >= 1 && bus_pin <= 6) - dev_priv->crt_ddc_pin = bus_pin; + i2c = intel_gmbus_get_adapter(dev_priv, bus_pin); + if (i2c) + dev_priv->crt_ddc = i2c; } else { DRM_DEBUG_KMS("BDB_GD too small (%d). Invalid.\n", - block_size); + block_size); } } } @@ -599,7 +601,8 @@ init_vbt_defaults(struct drm_i915_private *dev_priv) { struct drm_device *dev = dev_priv->dev;
- dev_priv->crt_ddc_pin = GMBUS_PORT_VGADDC; + dev_priv->crt_ddc = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_VGADDC);
/* LFP panel data */ dev_priv->lvds_dither = 1; diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index dd729d4..02f6fc1 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -275,12 +275,11 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) if (crt->base.type != INTEL_OUTPUT_ANALOG) return false;
- if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) { + if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc)) { struct edid *edid; bool is_digital = false;
- edid = drm_get_edid(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + edid = drm_get_edid(connector, dev_priv->crt_ddc); /* * This may be a DVI-I connector with a shared DDC * link between analog and digital outputs, so we @@ -484,14 +483,14 @@ static int intel_crt_get_modes(struct drm_connector *connector) struct drm_i915_private *dev_priv = dev->dev_private; int ret;
- ret = intel_ddc_get_modes(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + ret = intel_ddc_get_modes(connector, dev_priv->crt_ddc); if (ret || !IS_G4X(dev)) return ret;
/* Try to probe digital port for output in DVI-I -> VGA mode. */ - return intel_ddc_get_modes(connector, - &dev_priv->gmbus[GMBUS_PORT_DPB].adapter); + return intel_ddc_get_modes( + connector, + intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB)); }
static int intel_crt_set_property(struct drm_connector *connector, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1348705..7538f2e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -285,7 +285,8 @@ struct intel_fbc_work { };
int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); -extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus); +extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, + struct i2c_adapter *adapter);
extern void intel_attach_force_audio_property(struct drm_connector *connector); extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index 6eda1b5..6ff1095 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -244,7 +244,7 @@ static int intel_dvo_get_modes(struct drm_connector *connector) * that's not the case. */ intel_ddc_get_modes(connector, - &dev_priv->gmbus[GMBUS_PORT_DPC].adapter); + intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPC)); if (!list_empty(&connector->probed_modes)) return 1;
@@ -387,7 +387,7 @@ void intel_dvo_init(struct drm_device *dev) * It appears that everything is on GPIOE except for panels * on i830 laptops, which are on GPIOB (DVOA). */ - i2c = &dev_priv->gmbus[gpio].adapter; + i2c = intel_gmbus_get_adapter(dev_priv, gpio);
intel_dvo->dev = *dvo; if (!dvo->dev_ops->init(&intel_dvo->dev, i2c)) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 64541f7..a5844d7 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -40,7 +40,7 @@ struct intel_hdmi { struct intel_encoder base; u32 sdvox_reg; - int ddc_bus; + struct i2c_adapter *ddc_bus; uint32_t color_range; bool has_hdmi_sink; bool has_audio; @@ -327,14 +327,12 @@ static enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private; struct edid *edid; enum drm_connector_status status = connector_status_disconnected;
intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; - edid = drm_get_edid(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + edid = drm_get_edid(connector, intel_hdmi->ddc_bus);
if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) { @@ -357,26 +355,22 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) static int intel_hdmi_get_modes(struct drm_connector *connector) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private;
/* We should parse the EDID data and find out if it's an HDMI sink so * we can send audio to it. */
- return intel_ddc_get_modes(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + return intel_ddc_get_modes(connector, intel_hdmi->ddc_bus); }
static bool intel_hdmi_detect_audio(struct drm_connector *connector) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct drm_i915_private *dev_priv = connector->dev->dev_private; struct edid *edid; bool has_audio = false;
- edid = drm_get_edid(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + edid = drm_get_edid(connector, intel_hdmi->ddc_bus); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) has_audio = drm_detect_monitor_audio(edid); @@ -521,23 +515,28 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg) /* Set up the DDC bus. */ if (sdvox_reg == SDVOB) { intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT); - intel_hdmi->ddc_bus = GMBUS_PORT_DPB; + intel_hdmi->ddc_bus = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_DPB); dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS; } else if (sdvox_reg == SDVOC) { intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT); - intel_hdmi->ddc_bus = GMBUS_PORT_DPC; + intel_hdmi->ddc_bus = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_DPC); dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS; } else if (sdvox_reg == HDMIB) { intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT); - intel_hdmi->ddc_bus = GMBUS_PORT_DPB; + intel_hdmi->ddc_bus = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_DPB); dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS; } else if (sdvox_reg == HDMIC) { intel_encoder->clone_mask = (1 << INTEL_HDMIE_CLONE_BIT); - intel_hdmi->ddc_bus = GMBUS_PORT_DPC; + intel_hdmi->ddc_bus = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_DPC); dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS; } else if (sdvox_reg == HDMID) { intel_encoder->clone_mask = (1 << INTEL_HDMIF_CLONE_BIT); - intel_hdmi->ddc_bus = GMBUS_PORT_DPD; + intel_hdmi->ddc_bus = intel_gmbus_get_adapter(dev_priv, + GMBUS_PORT_DPD); dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS; }
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 1135e3e..87a3abf 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -426,6 +426,13 @@ err: return ret; }
+struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, + unsigned pin) +{ + BUG_ON(pin >= GMBUS_NUM_PORTS); + return &dev_priv->gmbus[pin].adapter; +} + void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed) { struct intel_gmbus *bus = to_intel_gmbus(adapter); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index aa84832..24c73d7 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -948,7 +948,8 @@ bool intel_lvds_init(struct drm_device *dev) * preferred mode is the right one. */ intel_lvds->edid = drm_get_edid(connector, - &dev_priv->gmbus[pin].adapter); + intel_gmbus_get_adapter(dev_priv, + pin)); if (intel_lvds->edid) { if (drm_add_edid_modes(connector, intel_lvds->edid)) { diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index be2c6fe..095c1ae 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -35,9 +35,9 @@ * intel_ddc_probe * */ -bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) +bool intel_ddc_probe(struct intel_encoder *intel_encoder, + struct i2c_adapter *ddc_bus) { - struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; u8 out_buf[] = { 0x0, 0x0}; u8 buf[2]; struct i2c_msg msgs[] = { @@ -55,7 +55,7 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) } };
- return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2; + return i2c_transfer(ddc_bus, msgs, 2) == 2; }
/** diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index e334ec3..dc1c8df 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1251,9 +1251,7 @@ static struct edid * intel_sdvo_get_analog_edid(struct drm_connector *connector) { struct drm_i915_private *dev_priv = connector->dev->dev_private; - - return drm_get_edid(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + return drm_get_edid(connector, dev_priv->crt_ddc); }
enum drm_connector_status @@ -1921,12 +1919,12 @@ intel_sdvo_select_i2c_bus(struct drm_i915_private *dev_priv, if (mapping->initialized) pin = mapping->i2c_pin;
- if (pin < GMBUS_NUM_PORTS) { - sdvo->i2c = &dev_priv->gmbus[pin].adapter; + if (pin <= GMBUS_PORT_DPD) { + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, pin); intel_gmbus_set_speed(sdvo->i2c, GMBUS_RATE_1MHZ); intel_gmbus_force_bit(sdvo->i2c, true); } else { - sdvo->i2c = &dev_priv->gmbus[GMBUS_PORT_DPB].adapter; + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB); } }
There is no "disabled" port 0. So, don't even try to initialize/scan it, etc. This saves a bit of time when initializing the driver, since it avoids a 50ms timeout waiting for a device to respond on a port that doesn't even exist.
Similarly, don't initialize the "reserved" port 7, either.
Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_reg.h | 1 - drivers/gpu/drm/i915/intel_i2c.c | 64 +++++++++++++++++-------------------- 2 files changed, 29 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 56af0df..89cace2 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -699,7 +699,6 @@ #define GMBUS_PORT_DPB 5 /* SDVO, HDMIB */ #define GMBUS_PORT_DPD 6 /* HDMID */ #define GMBUS_PORT_RESERVED 7 /* 7 reserved */ -#define GMBUS_NUM_PORTS 8 #define GMBUS1 0x5104 /* command/status */ #define GMBUS_SW_CLR_INT (1<<31) #define GMBUS_SW_RDY (1<<30) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 87a3abf..f53f525 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -35,6 +35,20 @@ #include "i915_drm.h" #include "i915_drv.h"
+struct gmbus_port { + const char *name; + int reg; +}; + +static const struct gmbus_port gmbus_ports[] = { + { "ssc", GPIOB }, + { "vga", GPIOA }, + { "panel", GPIOC }, + { "dpc", GPIOD }, + { "dpb", GPIOE }, + { "dpd", GPIOF }, +}; + /* Intel GPIO access functions */
#define I2C_RISEFALL_TIME 20 @@ -150,32 +164,22 @@ static void set_data(void *data, int state_high) static struct i2c_adapter * intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin) { - static const int map_pin_to_reg[] = { - 0, - GPIOB, - GPIOA, - GPIOC, - GPIOD, - GPIOE, - GPIOF, - 0, - }; struct intel_gpio *gpio;
- if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) - return NULL; + pin -= 1; /* NB: -1 to map pin pair to gmbus array index */ + BUG_ON(pin >= ARRAY_SIZE(gmbus_ports));
gpio = kzalloc(sizeof(struct intel_gpio), GFP_KERNEL); if (gpio == NULL) return NULL;
- gpio->reg = map_pin_to_reg[pin]; + gpio->reg = gmbus_ports[pin].reg; if (HAS_PCH_SPLIT(dev_priv->dev)) gpio->reg += PCH_GPIOA - GPIOA; gpio->dev_priv = dev_priv;
snprintf(gpio->adapter.name, sizeof(gpio->adapter.name), - "i915 GPIO%c", "?BACDEF?"[pin]); + "i915 GPIO%c", "BACDEF"[pin]); gpio->adapter.owner = THIS_MODULE; gpio->adapter.algo_data = &gpio->algo; gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev; @@ -369,33 +373,22 @@ static const struct i2c_algorithm gmbus_algorithm = { */ int intel_setup_gmbus(struct drm_device *dev) { - static const char *names[GMBUS_NUM_PORTS] = { - "disabled", - "ssc", - "vga", - "panel", - "dpc", - "dpb", - "dpd", - "reserved", - }; struct drm_i915_private *dev_priv = dev->dev_private; int ret, i;
- dev_priv->gmbus = kcalloc(sizeof(struct intel_gmbus), GMBUS_NUM_PORTS, - GFP_KERNEL); + dev_priv->gmbus = kcalloc(sizeof(struct intel_gmbus), + ARRAY_SIZE(gmbus_ports), GFP_KERNEL); if (dev_priv->gmbus == NULL) return -ENOMEM;
- for (i = 0; i < GMBUS_NUM_PORTS; i++) { + for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i]; + u32 port = i + 1; /* +1 to map gmbus index to pin pair */
bus->adapter.owner = THIS_MODULE; bus->adapter.class = I2C_CLASS_DDC; - snprintf(bus->adapter.name, - sizeof(bus->adapter.name), - "i915 gmbus %s", - names[i]); + snprintf(bus->adapter.name, sizeof(bus->adapter.name), + "i915 gmbus %s", gmbus_ports[i].name);
bus->adapter.dev.parent = &dev->pdev->dev; bus->adapter.algo_data = dev_priv; @@ -406,10 +399,10 @@ int intel_setup_gmbus(struct drm_device *dev) goto err;
/* By default use a conservative clock rate */ - bus->reg0 = i | GMBUS_RATE_100KHZ; + bus->reg0 = port | GMBUS_RATE_100KHZ;
/* XXX force bit banging until GMBUS is fully debugged */ - bus->force_bit = intel_gpio_create(dev_priv, i); + bus->force_bit = intel_gpio_create(dev_priv, port); }
intel_i2c_reset(dev_priv->dev); @@ -429,7 +422,8 @@ err: struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, unsigned pin) { - BUG_ON(pin >= GMBUS_NUM_PORTS); + pin -= 1; /* NB: -1 to map pin pair to gmbus array index */ + BUG_ON(pin >= ARRAY_SIZE(gmbus_ports)); return &dev_priv->gmbus[pin].adapter; }
@@ -467,7 +461,7 @@ void intel_teardown_gmbus(struct drm_device *dev) if (dev_priv->gmbus == NULL) return;
- for (i = 0; i < GMBUS_NUM_PORTS; i++) { + for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i]; if (bus->force_bit) { i2c_del_adapter(bus->force_bit);
The i915 has multiple i2c adapters. However, they all share a single single set of i2c control registers (algorithm). Thus, different threads trying to access different adapters could interfere with each other.
Note: different threads trying to access the same channel is already handled in the i2c-core using the i2c adapter lock.
This patch adds a mutex to serialize access to the gmbus_xfer routine. Note: the same mutex serializes both bit banged and native xfers.
Signed-off-by: Yufeng Shen miletus@chromium.org Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_i2c.c | 13 +++++++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9a81e48..68aef65 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -301,6 +301,7 @@ typedef struct drm_i915_private { struct i2c_adapter *force_bit; u32 reg0; } *gmbus; + struct mutex gmbus_mutex;
struct pci_dev *bridge_dev; struct intel_ring_buffer ring[I915_NUM_RINGS]; diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index f53f525..232b7cb 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -212,6 +212,8 @@ intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv, adapter); int ret;
+ mutex_lock(&dev_priv->gmbus_mutex); + intel_i2c_reset(dev_priv->dev);
intel_i2c_quirk_set(dev_priv, true); @@ -225,6 +227,8 @@ intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv, set_clock(gpio, 1); intel_i2c_quirk_set(dev_priv, false);
+ mutex_unlock(&dev_priv->gmbus_mutex); + return ret; }
@@ -243,6 +247,8 @@ gmbus_xfer(struct i2c_adapter *adapter, return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+ mutex_lock(&dev_priv->gmbus_mutex); + reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
I915_WRITE(GMBUS0 + reg_offset, bus->reg0); @@ -332,6 +338,9 @@ done: * start of the next xfer, till then let it sleep. */ I915_WRITE(GMBUS0 + reg_offset, 0); + + mutex_unlock(&dev_priv->gmbus_mutex); + return i;
timeout: @@ -339,6 +348,8 @@ timeout: bus->reg0 & 0xff, bus->adapter.name); I915_WRITE(GMBUS0 + reg_offset, 0);
+ mutex_unlock(&dev_priv->gmbus_mutex); + /* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff); if (!bus->force_bit) @@ -381,6 +392,8 @@ int intel_setup_gmbus(struct drm_device *dev) if (dev_priv->gmbus == NULL) return -ENOMEM;
+ mutex_init(&dev_priv->gmbus_mutex); + for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i]; u32 port = i + 1; /* +1 to map gmbus index to pin pair */
Return -ENXIO if a device NAKs a transaction.
Note: We should return -ETIMEDOUT, too if the transaction times out, however, that error path is currently handled by the 'bit-bang fallback'.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 232b7cb..151b828 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -242,6 +242,7 @@ gmbus_xfer(struct i2c_adapter *adapter, adapter); struct drm_i915_private *dev_priv = adapter->algo_data; int i, reg_offset; + int ret = 0;
if (bus->force_bit) return intel_i2c_quirk_xfer(dev_priv, @@ -333,6 +334,15 @@ clear_err: I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); I915_WRITE(GMBUS1 + reg_offset, 0);
+ /* + * If no ACK is received during the address phase of a transaction, + * the adapter must report -ENXIO. + * It is not clear what to return if no ACK is received at other times. + * So, we always return -ENXIO in all NAK cases, to ensure we send + * it at least during the one case that is specified. + */ + ret = -ENXIO; + done: /* Mark the GMBUS interface as disabled. We will re-enable it at the * start of the next xfer, till then let it sleep. @@ -341,7 +351,7 @@ done:
mutex_unlock(&dev_priv->gmbus_mutex);
- return i; + return ret ?: i;
timeout: DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence.
Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle.
Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle.
This patch also takes advantage (in the write path) of the double-buffered GMBUS3 data register by writing two 4-byte words before the first wait for HW_RDY.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 42 +++++++++++++++++++++++++------------ 1 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 151b828..b79a181 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -260,7 +260,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
if (msgs[i].flags & I2C_M_RD) { I915_WRITE(GMBUS1 + reg_offset, - GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) | + GMBUS_CYCLE_WAIT | (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY); @@ -272,7 +272,8 @@ gmbus_xfer(struct i2c_adapter *adapter, (GMBUS_SATOER | GMBUS_HW_RDY), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) + if (I915_READ(GMBUS2 + reg_offset) & + GMBUS_SATOER) goto clear_err;
val = I915_READ(GMBUS3 + reg_offset); @@ -291,20 +292,13 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS3 + reg_offset, val); I915_WRITE(GMBUS1 + reg_offset, - (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) | + GMBUS_CYCLE_WAIT | (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); POSTING_READ(GMBUS2 + reg_offset);
while (len) { - if (wait_for(I915_READ(GMBUS2 + reg_offset) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50)) - goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) - goto clear_err; - val = loop = 0; do { val |= *buf++ << (8 * loop); @@ -312,11 +306,18 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS3 + reg_offset, val); POSTING_READ(GMBUS2 + reg_offset); + + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) + goto timeout; + if (I915_READ(GMBUS2 + reg_offset) & + GMBUS_SATOER) + goto clear_err; } }
- if (i + 1 < num && - wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) goto timeout; @@ -344,9 +345,22 @@ clear_err: ret = -ENXIO;
done: - /* Mark the GMBUS interface as disabled. We will re-enable it at the - * start of the next xfer, till then let it sleep. + if (I915_READ(GMBUS2 + reg_offset) & GMBUS_HW_WAIT_PHASE) { + I915_WRITE(GMBUS1 + reg_offset, + GMBUS_CYCLE_STOP | GMBUS_SW_RDY); + POSTING_READ(GMBUS2 + reg_offset); + } + + /* Mark the GMBUS interface as disabled after waiting for idle. + * We will re-enable it at the start of the next xfer, + * till then let it sleep. */ + if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, + 10)) { + DRM_INFO("GMBUS [%s] timed out waiting for IDLE\n", + adapter->name); + ret = -ETIMEDOUT; + } I915_WRITE(GMBUS0 + reg_offset, 0);
mutex_unlock(&dev_priv->gmbus_mutex);
It is very common for an i2c device to require a small 1 or 2 byte write followed by a read. For example, when reading from an i2c EEPROM it is common to write and address, offset or index followed by a reading some values.
The i915 gmbus controller provides a special "INDEX" cycle for performing such a small write followed by a read. The INDEX can be either one or two bytes long. The advantage of using such a cycle is that the CPU has slightly less work to do once the read with INDEX cycle is started.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index b79a181..61fe317 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -255,12 +255,40 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
for (i = 0; i < num; i++) { - u16 len = msgs[i].len; - u8 *buf = msgs[i].buf; + u16 len; + u8 *buf; + u32 gmbus5 = 0; + u32 gmbus1_index = 0; + + /* + * The gmbus controller can combine a 1 or 2 byte write with a + * read that immediately follows it by using an "INDEX" cycle. + */ + if (i + 1 < num && + !(msgs[i].flags & I2C_M_RD) && + (msgs[i + 1].flags & I2C_M_RD) && + msgs[i].len <= 2) { + if (msgs[i].len == 2) + gmbus5 = GMBUS_2BYTE_INDEX_EN | + msgs[i].buf[1] | + (msgs[i].buf[0] << 8); + if (msgs[i].len == 1) + gmbus1_index = GMBUS_CYCLE_INDEX | + (msgs[i].buf[0] << + GMBUS_SLAVE_INDEX_SHIFT); + i += 1; /* set i to the index of the read xfer */ + } + + len = msgs[i].len; + buf = msgs[i].buf; + + /* GMBUS5 holds 16-bit index, but must be 0 if not used */ + I915_WRITE(GMBUS5 + reg_offset, gmbus5);
if (msgs[i].flags & I2C_M_RD) { I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | + gmbus1_index | (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY);
Save the GMBUS2 value read while polling for state changes, and then reuse this value when determining for which reason the loops were exited. This is a small optimization which saves a couple of bus accesses for memory mapped IO registers.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org Reviewed-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_i2c.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 61fe317..2c372d3 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -243,6 +243,7 @@ gmbus_xfer(struct i2c_adapter *adapter, struct drm_i915_private *dev_priv = adapter->algo_data; int i, reg_offset; int ret = 0; + u32 gmbus2 = 0;
if (bus->force_bit) return intel_i2c_quirk_xfer(dev_priv, @@ -296,12 +297,12 @@ gmbus_xfer(struct i2c_adapter *adapter, do { u32 val, loop = 0;
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & - GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err;
val = I915_READ(GMBUS3 + reg_offset); @@ -335,21 +336,21 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS3 + reg_offset, val); POSTING_READ(GMBUS2 + reg_offset);
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & - GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err; } }
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err; }
@@ -373,7 +374,7 @@ clear_err: ret = -ENXIO;
done: - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_HW_WAIT_PHASE) { + if (gmbus2 & GMBUS_HW_WAIT_PHASE) { I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY); POSTING_READ(GMBUS2 + reg_offset);
Instead of polling for gmbus state changes, use the corresponding gmbus interrupts, when possible.
There are still some cases where using the GMBUS interrupts is not possible. For instance, this patch only enables the interrupt for ironlake (+ sandy bridge), and ivybridge. It does not enable them for the older i915 path through the driver. Also, there are cases where a gmbus transaction may be requested before irqs have even been enabled.
For this reason, the older polling loop is left in place, as a backup.
Also, since the interrupts can be enabled/disabled completely asynchronously from any active gmbus transactions, changing the interrupt enable is protected by the gmbus transaction mutex.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 22 +++++++++- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_drv.h | 6 +++ drivers/gpu/drm/i915/intel_i2c.c | 82 ++++++++++++++++++++++++++++++++------ 5 files changed, 97 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 68aef65..156ad6d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -302,6 +302,8 @@ typedef struct drm_i915_private { u32 reg0; } *gmbus; struct mutex gmbus_mutex; + wait_queue_head_t gmbus_waitq; + bool gmbus_irq_enabled;
struct pci_dev *bridge_dev; struct intel_ring_buffer ring[I915_NUM_RINGS]; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 5bd4361..12a2259 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -432,13 +432,23 @@ static void pch_irq_handler(struct drm_device *dev)
pch_iir = I915_READ(SDEIIR);
+ if (HAS_PCH_CPT(dev)) { + if (pch_iir & SDE_GMBUS_CPT) { + DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n"); + intel_gmbus_irq(dev); + } + return; + } + if (pch_iir & SDE_AUDIO_POWER_MASK) DRM_DEBUG_DRIVER("PCH audio power change on port %d\n", (pch_iir & SDE_AUDIO_POWER_MASK) >> SDE_AUDIO_POWER_SHIFT);
- if (pch_iir & SDE_GMBUS) + if (pch_iir & SDE_GMBUS) { DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n"); + intel_gmbus_irq(dev); + }
if (pch_iir & SDE_AUDIO_HDCP_MASK) DRM_DEBUG_DRIVER("PCH HDCP audio interrupt\n"); @@ -1847,13 +1857,15 @@ static int ironlake_irq_postinstall(struct drm_device *dev) hotplug_mask = (SDE_CRT_HOTPLUG_CPT | SDE_PORTB_HOTPLUG_CPT | SDE_PORTC_HOTPLUG_CPT | - SDE_PORTD_HOTPLUG_CPT); + SDE_PORTD_HOTPLUG_CPT | + SDE_GMBUS_CPT); } else { hotplug_mask = (SDE_CRT_HOTPLUG | SDE_PORTB_HOTPLUG | SDE_PORTC_HOTPLUG | SDE_PORTD_HOTPLUG | - SDE_AUX_MASK); + SDE_AUX_MASK | + SDE_GMBUS); }
dev_priv->pch_irq_mask = ~hotplug_mask; @@ -1863,6 +1875,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev) I915_WRITE(SDEIER, hotplug_mask); POSTING_READ(SDEIER);
+ intel_gmbus_irq_enable(dev); ironlake_enable_pch_hotplug(dev);
if (IS_IRONLAKE_M(dev)) { @@ -1922,6 +1935,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) I915_WRITE(SDEIER, hotplug_mask); POSTING_READ(SDEIER);
+ intel_gmbus_irq_enable(dev); ironlake_enable_pch_hotplug(dev);
return 0; @@ -2049,6 +2063,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev) I915_WRITE(GTIER, 0x0); I915_WRITE(GTIIR, I915_READ(GTIIR));
+ intel_gmbus_irq_disable(dev); + I915_WRITE(SDEIMR, 0xffffffff); I915_WRITE(SDEIER, 0x0); I915_WRITE(SDEIIR, I915_READ(SDEIIR)); diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 89cace2..94333e8 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3080,6 +3080,7 @@ #define SDE_TRANSA_FIFO_UNDER (1 << 0) #define SDE_TRANS_MASK (0x3f) /* CPT */ +#define SDE_GMBUS_CPT (1 << 17) #define SDE_CRT_HOTPLUG_CPT (1 << 19) #define SDE_PORTD_HOTPLUG_CPT (1 << 23) #define SDE_PORTC_HOTPLUG_CPT (1 << 22) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7538f2e..7c91354 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -47,6 +47,8 @@
#define wait_for(COND, MS) _wait_for(COND, MS, 1) #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0) +#define wait_irq(WQ, COND, MS) \ + (wait_event_timeout(WQ, COND, msecs_to_jiffies(MS)) == 0)
#define KHz(x) (1000*x) #define MHz(x) KHz(1000*x) @@ -408,6 +410,10 @@ extern void intel_write_eld(struct drm_encoder *encoder, struct drm_display_mode *mode); extern void intel_cpt_verify_modeset(struct drm_device *dev, int pipe);
+extern void intel_gmbus_irq(struct drm_device *dev); +extern void intel_gmbus_irq_enable(struct drm_device *dev); +extern void intel_gmbus_irq_disable(struct drm_device *dev); + /* For use by IVB LP watermark workaround in intel_sprite.c */ extern void sandybridge_update_wm(struct drm_device *dev); extern void intel_update_sprite_watermarks(struct drm_device *dev, int pipe, diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 2c372d3..520112c 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -231,6 +231,10 @@ intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv,
return ret; } +#define gmbus_wait(DEV_PRIV, COND, MS) \ + (((DEV_PRIV)->gmbus_irq_enabled) ? \ + wait_irq((DEV_PRIV)->gmbus_waitq, (COND), (MS)) : \ + wait_for((COND), (MS)))
static int gmbus_xfer(struct i2c_adapter *adapter, @@ -244,6 +248,7 @@ gmbus_xfer(struct i2c_adapter *adapter, int i, reg_offset; int ret = 0; u32 gmbus2 = 0; + u32 g2_addr;
if (bus->force_bit) return intel_i2c_quirk_xfer(dev_priv, @@ -252,6 +257,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0; + g2_addr = GMBUS2 + reg_offset;
I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
@@ -287,6 +293,10 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS5 + reg_offset, gmbus5);
if (msgs[i].flags & I2C_M_RD) { + I915_WRITE(GMBUS4 + reg_offset, + GMBUS_SLAVE_TIMEOUT_EN | GMBUS_NAK_EN | + ((len > 4) ? GMBUS_HW_RDY_EN : + GMBUS_HW_WAIT_EN)); I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | gmbus1_index | @@ -297,14 +307,20 @@ gmbus_xfer(struct i2c_adapter *adapter, do { u32 val, loop = 0;
- if (wait_for((gmbus2 = I915_READ(GMBUS2 + - reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50)) + if (gmbus_wait(dev_priv, + (gmbus2 = I915_READ(g2_addr)) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; if (gmbus2 & GMBUS_SATOER) goto clear_err;
+ I915_WRITE(GMBUS4 + reg_offset, + GMBUS_SLAVE_TIMEOUT_EN | + GMBUS_NAK_EN | + ((len > 4) ? GMBUS_HW_RDY_EN : + GMBUS_HW_WAIT_EN)); + val = I915_READ(GMBUS3 + reg_offset); do { *buf++ = val & 0xff; @@ -319,6 +335,11 @@ gmbus_xfer(struct i2c_adapter *adapter, val |= *buf++ << (8 * loop); } while (--len && ++loop < 4);
+ I915_WRITE(GMBUS4 + reg_offset, + GMBUS_SLAVE_TIMEOUT_EN | + GMBUS_NAK_EN | + ((len > 4) ? GMBUS_HW_RDY_EN : + GMBUS_HW_WAIT_EN)); I915_WRITE(GMBUS3 + reg_offset, val); I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | @@ -333,22 +354,29 @@ gmbus_xfer(struct i2c_adapter *adapter, val |= *buf++ << (8 * loop); } while (--len && ++loop < 4);
+ I915_WRITE(GMBUS4 + reg_offset, + GMBUS_SLAVE_TIMEOUT_EN | + GMBUS_NAK_EN | + ((len > 0) ? GMBUS_HW_RDY_EN : + GMBUS_HW_WAIT_EN)); + I915_WRITE(GMBUS3 + reg_offset, val); POSTING_READ(GMBUS2 + reg_offset);
- if (wait_for((gmbus2 = I915_READ(GMBUS2 + - reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50)) + if (gmbus_wait(dev_priv, + (gmbus2 = I915_READ(g2_addr)) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; if (gmbus2 & GMBUS_SATOER) goto clear_err; } }
- if (wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & - (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), - 50)) + if (gmbus_wait(dev_priv, + (gmbus2 = I915_READ(GMBUS2 + reg_offset)) & + (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), + 50)) goto timeout; if (gmbus2 & GMBUS_SATOER) goto clear_err; @@ -375,6 +403,7 @@ clear_err:
done: if (gmbus2 & GMBUS_HW_WAIT_PHASE) { + I915_WRITE(GMBUS4 + reg_offset, GMBUS_IDLE_EN); I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY); POSTING_READ(GMBUS2 + reg_offset); @@ -384,8 +413,8 @@ done: * We will re-enable it at the start of the next xfer, * till then let it sleep. */ - if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, - 10)) { + if (gmbus_wait(dev_priv, + (I915_READ(g2_addr) & GMBUS_ACTIVE) == 0, 10)) { DRM_INFO("GMBUS [%s] timed out waiting for IDLE\n", adapter->name); ret = -ETIMEDOUT; @@ -431,6 +460,32 @@ static const struct i2c_algorithm gmbus_algorithm = { .functionality = gmbus_func };
+void intel_gmbus_irq_enable(struct drm_device *drm_device) +{ + struct drm_i915_private *dev_priv = drm_device->dev_private; + + /* Delay enabling irq until current gmbus transaction has finished */ + mutex_lock(&dev_priv->gmbus_mutex); + dev_priv->gmbus_irq_enabled = true; + mutex_unlock(&dev_priv->gmbus_mutex); +} + +void intel_gmbus_irq_disable(struct drm_device *drm_device) +{ + struct drm_i915_private *dev_priv = drm_device->dev_private; + + /* Delay disabling irq until current gmbus transaction has finished */ + mutex_lock(&dev_priv->gmbus_mutex); + dev_priv->gmbus_irq_enabled = false; + mutex_unlock(&dev_priv->gmbus_mutex); +} + +void intel_gmbus_irq(struct drm_device *drm_device) +{ + struct drm_i915_private *dev_priv = drm_device->dev_private; + wake_up(&dev_priv->gmbus_waitq); +} + /** * intel_gmbus_setup - instantiate all Intel i2c GMBuses * @dev: DRM device @@ -445,6 +500,7 @@ int intel_setup_gmbus(struct drm_device *dev) if (dev_priv->gmbus == NULL) return -ENOMEM;
+ init_waitqueue_head(&dev_priv->gmbus_waitq); mutex_init(&dev_priv->gmbus_mutex);
for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) {
On Sat, Mar 10, 2012 at 02:48:14AM +0800, Daniel Kurtz wrote:
This patchset addresses a couple of issues with the i915 gmbus implementation:
- fixes misassigned pin port pair for HDMI-D
- fixes write transactions when they are the only transaction requested (including large >4-byte writes) by terminating every transaction with a WAIT cycle.
- returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled i2c transaction failures
- optimizes the typical read transaction case by using the INDEX cycle
- turns on the GMBUS interrupt whenever possible to dramatically improve throughput by eliminating the very slow polling loop.
The patchset should apply cleanly onto linus/master. It is inspired by, but completely supercedes, a similar patch submitted recently by Benson Leung (bleung@chromium.org). I'd be happy to rebase on a different tree if necessary.
v2 of the patchset address review comments from Chris Wilson (thanks for the review, Chris!), and adds the interrupt patch. There weren't any review comments for patches 5, 7, or 8 of the first set. Hopefully they will get more love the second time around :).
Oops, still catching up - I've commented on the first thread. Please resubmit on top of drm-intel-next-queued and I'll have a look. Also, please cc me so I don't miss them.
Thanks, Daniel
On Sun, Mar 18, 2012 at 07:22:05PM +0100, Daniel Vetter wrote:
On Sat, Mar 10, 2012 at 02:48:14AM +0800, Daniel Kurtz wrote:
This patchset addresses a couple of issues with the i915 gmbus implementation:
- fixes misassigned pin port pair for HDMI-D
- fixes write transactions when they are the only transaction requested (including large >4-byte writes) by terminating every transaction with a WAIT cycle.
- returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled i2c transaction failures
- optimizes the typical read transaction case by using the INDEX cycle
- turns on the GMBUS interrupt whenever possible to dramatically improve throughput by eliminating the very slow polling loop.
The patchset should apply cleanly onto linus/master. It is inspired by, but completely supercedes, a similar patch submitted recently by Benson Leung (bleung@chromium.org). I'd be happy to rebase on a different tree if necessary.
v2 of the patchset address review comments from Chris Wilson (thanks for the review, Chris!), and adds the interrupt patch. There weren't any review comments for patches 5, 7, or 8 of the first set. Hopefully they will get more love the second time around :).
Oops, still catching up - I've commented on the first thread. Please resubmit on top of drm-intel-next-queued and I'll have a look. Also, please cc me so I don't miss them.
I've pushed aout a few more changes to intel_i2c.c in my drm-intel-next-queued branch. Generally I like these patches, so please rebase and resubmit.
Thanks, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Kurtz -
Daniel Vetter