Re: BUG: drm_crtc_helper_set_config does not work
On Fri, Mar 9, 2012 at 3:15 AM, Maarten Maathuis madman2...@gmail.com wrote: On Fri, Mar 9, 2012 at 2:50 AM, Yufeng Shen mile...@chromium.org wrote: So a simple case of disabling a CRTC and then re-enabling it. Disabling: CRTC X is originally connected to output Y 1. XRRSetCrtcConfig() is called to disable CRTC X 2. drm_helper_connector_dpms() then gets called on the connector connected to CRTC X, in the function, the connector-encoder is set to DPMS OFF, and the encoder-crtc set to DPMS OFF but the encoder-crtc is still CRTC X Re-enabling: 3. XRRSetCrtcConfig() is called to to enable CRTC X on output Y 4. drm_crtc_helper_set_config () is now called, suppose fb is not changed, mode is not changed, encoder is not changed and the encoder-crtc is not changed, then we have mode_changed == false and the mode is not reset So the CRTC X won't be connected to output Y. What I don't understand the code is that why XRRSetCrtcConfig() ends up only calling drm_helper_connector_dpms() ? I think it should do more than that, at least it should set the related encoder-crtc = NULL any comments ? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel As far as i know DPMS designed is a simple switch, meaning that the mode remains active, so if you do DPMS ON it should come back. If for some reason the driver is disconnecting the encoder from the crtc, then it is the drivers responsibility to reconnect it when DPMS ON happens. But don't take my word for it, others may have more/other information. Yes, I was thinking ideally DPMS call should enable/disable DPMS state, and XRRSetCrtcConfig call should enable/disable CRTC. But it seems that, when using XRRSetCrtcConfig to disable CRTC, the CRTC itself is disabled, the DPMS is disabled (DPMS OFF), and then when using XRRSetCrtcConfig to re-enable the CRTC, it only checks whether the CRTC that connected to the encoder is changed (whether a new crtc is assigned), but does not check whether the CRTC itself is previously disabled or not. So it considers the encoder is connected to the same CRTC and does no thing to re-enable the CRTC. -- Far away from the primal instinct, the song seems to fade away, the river get wider between your thoughts and the things we do and say. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
BUG: drm_crtc_helper_set_config does not work
On Fri, Mar 9, 2012 at 3:15 AM, Maarten Maathuis wrote: > On Fri, Mar 9, 2012 at 2:50 AM, Yufeng Shen wrote: >> So a simple case of ?disabling a CRTC and then re-enabling it. >> >> Disabling: >> >> CRTC X is originally connected to output Y >> >> 1. XRRSetCrtcConfig() ? ?is called to disable CRTC ?X >> >> 2. drm_helper_connector_dpms() ? then gets called on the connector >> connected to CRTC X, >> ? ?in the function, >> ? ?the ?connector->encoder ?is set to DPMS OFF, ? and the >> encoder->crtc ?set to DPMS OFF >> ? ?but the ?encoder->crtc ? is still CRTC X >> >> Re-enabling: >> >> 3. XRRSetCrtcConfig() ? ?is called to to enable CRTC ?X ?on output Y >> >> 4. drm_crtc_helper_set_config () ?is now called, >> suppose ? fb is not changed, ?mode is not changed, encoder is not changed >> and the ? encoder->crtc ?is not changed, >> then we have ? mode_changed == false ? ?and ?the mode is not reset >> >> So the CRTC X won't be connected to output Y. >> >> What I don't understand the code is that why ?XRRSetCrtcConfig() ?ends >> up only calling ?drm_helper_connector_dpms() ?? >> I think it should do more than that, at least it should set the >> related ?encoder->crtc = NULL >> >> any comments ? >> ___ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > As far as i know DPMS designed is a simple switch, meaning that the > mode remains active, so if you do DPMS ON it should come back. If for > some reason the driver is disconnecting the encoder from the crtc, > then it is the drivers responsibility to reconnect it when DPMS ON > happens. > > But don't take my word for it, others may have more/other information. > Yes, I was thinking ideally DPMS call should enable/disable DPMS state, and XRRSetCrtcConfig call should enable/disable CRTC. But it seems that, when using XRRSetCrtcConfig to disable CRTC, the CRTC itself is disabled, the DPMS is disabled (DPMS OFF), and then when using XRRSetCrtcConfig to re-enable the CRTC, it only checks whether the CRTC that connected to the encoder is changed (whether a new crtc is assigned), but does not check whether the CRTC itself is previously disabled or not. So it considers the encoder is connected to the same CRTC and does no thing to re-enable the CRTC. > -- > Far away from the primal instinct, the song seems to fade away, the > river get wider between your thoughts and the things we do and say.
BUG: drm_crtc_helper_set_config does not work
So a simple case of disabling a CRTC and then re-enabling it. Disabling: CRTC X is originally connected to output Y 1. XRRSetCrtcConfig()is called to disable CRTC X 2. drm_helper_connector_dpms() then gets called on the connector connected to CRTC X, in the function, the connector->encoder is set to DPMS OFF, and the encoder->crtc set to DPMS OFF but the encoder->crtc is still CRTC X Re-enabling: 3. XRRSetCrtcConfig()is called to to enable CRTC X on output Y 4. drm_crtc_helper_set_config () is now called, suppose fb is not changed, mode is not changed, encoder is not changed and the encoder->crtc is not changed, then we have mode_changed == falseand the mode is not reset So the CRTC X won't be connected to output Y. What I don't understand the code is that why XRRSetCrtcConfig() ends up only calling drm_helper_connector_dpms() ? I think it should do more than that, at least it should set the related encoder->crtc = NULL any comments ?
BUG: drm_crtc_helper_set_config does not work
So a simple case of disabling a CRTC and then re-enabling it. Disabling: CRTC X is originally connected to output Y 1. XRRSetCrtcConfig()is called to disable CRTC X 2. drm_helper_connector_dpms() then gets called on the connector connected to CRTC X, in the function, the connector-encoder is set to DPMS OFF, and the encoder-crtc set to DPMS OFF but the encoder-crtc is still CRTC X Re-enabling: 3. XRRSetCrtcConfig()is called to to enable CRTC X on output Y 4. drm_crtc_helper_set_config () is now called, suppose fb is not changed, mode is not changed, encoder is not changed and the encoder-crtc is not changed, then we have mode_changed == falseand the mode is not reset So the CRTC X won't be connected to output Y. What I don't understand the code is that why XRRSetCrtcConfig() ends up only calling drm_helper_connector_dpms() ? I think it should do more than that, at least it should set the related encoder-crtc = NULL any comments ? ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Fix race condition in accessing GMBUS
Moved gmbus_mutex below intel_gmbus and added comments.
Rebased to drm-intel-next-queued.
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen
---
drivers/gpu/drm/i915/i915_drv.h |8
drivers/gpu/drm/i915/intel_i2c.c | 24 +---
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 563d24e..40027de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,6 +313,14 @@ typedef struct drm_i915_private {
u32 reg0;
} *gmbus;
+ /** Multiple intel_gmbus i2c adapters are actually sharing the same
+ underlying GMBUS controller. i2c core protects concurrent use of
+ the same i2c adapter but is not aware of the concurrent use of the
+ underlying GMBUS controller. gmbus_mutex is used to prevent the race
+ condition from the perspective of GMBUS controller.
+ */
+ struct mutex gmbus_mutex;
+
struct pci_dev *bridge_dev;
struct intel_ring_buffer ring[I915_NUM_RINGS];
uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30..fc75d71 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,15 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter->algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus->force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(_priv->gmbus_mutex);
+
+ if (bus->force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus->force_bit, msgs, num);
+ goto out;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -321,7 +325,8 @@ done:
* start of the next xfer, till then let it sleep.
*/
I915_WRITE(GMBUS0 + reg_offset, 0);
- return i;
+ ret = i;
+ goto out;
timeout:
DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d
[%s]\n",
@@ -331,9 +336,12 @@ timeout:
/* 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)
- return -ENOMEM;
-
- return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+out:
+ mutex_unlock(_priv->gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -380,6 +388,8 @@ int intel_setup_gmbus(struct drm_device *dev)
if (dev_priv->gmbus == NULL)
return -ENOMEM;
+ mutex_init(_priv->gmbus_mutex);
+
for (i = 0; i < GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = _priv->gmbus[i];
--
1.7.3.1
[PATCH] [PATCH] drm/i915: Fix race condition in accessing GMBUS
Hi Ben, So I2C core does protect multiple access to one adapter, but I2C core does not protect the case where multiple adapters share the same underlying device. GMBUS has 7 different pin pairs and each pair is registered as an I2C adapter. I2C core can serialize the access to one pin pair, say that the LVDS port , or the DDC port. But when there are I2C transactions on both LVDS and DDC ports, since they share the same GMBUS registers, there will be race condition. Does this make sense to you ? --- Yufeng Shen On Mon, Feb 13, 2012 at 4:04 AM, Ben Widawsky wrote: > On Fri, Feb 10, 2012 at 12:50:01PM -0500, Yufeng Shen wrote: >> GMBUS has several ports and each has it's own corresponding >> I2C adpater. When multiple I2C adapters call gmbus_xfer() at >> the same time there is a race condition in using the underlying >> GMBUS controller. Fixing this by adding a mutex lock when calling >> gmbus_xfer(). >> >> Signed-off-by: Yufeng Shen > > I do not see the race. All the i2c transfers should be protected > correctly by the i2c core, or else I think we haven't registered our > device properly. Could you give an example of when/how this can happen? > > Ben
Re: [PATCH] [PATCH] drm/i915: Fix race condition in accessing GMBUS
Hi Ben, So I2C core does protect multiple access to one adapter, but I2C core does not protect the case where multiple adapters share the same underlying device. GMBUS has 7 different pin pairs and each pair is registered as an I2C adapter. I2C core can serialize the access to one pin pair, say that the LVDS port , or the DDC port. But when there are I2C transactions on both LVDS and DDC ports, since they share the same GMBUS registers, there will be race condition. Does this make sense to you ? --- Yufeng Shen On Mon, Feb 13, 2012 at 4:04 AM, Ben Widawsky b...@bwidawsk.net wrote: On Fri, Feb 10, 2012 at 12:50:01PM -0500, Yufeng Shen wrote: GMBUS has several ports and each has it's own corresponding I2C adpater. When multiple I2C adapters call gmbus_xfer() at the same time there is a race condition in using the underlying GMBUS controller. Fixing this by adding a mutex lock when calling gmbus_xfer(). Signed-off-by: Yufeng Shen mile...@chromium.org I do not see the race. All the i2c transfers should be protected correctly by the i2c core, or else I think we haven't registered our device properly. Could you give an example of when/how this can happen? Ben ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Fix race condition in accessing GMBUS
Moved gmbus_mutex below intel_gmbus and added comments.
Rebased to drm-intel-next-queued.
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen mile...@chromium.org
---
drivers/gpu/drm/i915/i915_drv.h |8
drivers/gpu/drm/i915/intel_i2c.c | 24 +---
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 563d24e..40027de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -313,6 +313,14 @@ typedef struct drm_i915_private {
u32 reg0;
} *gmbus;
+ /** Multiple intel_gmbus i2c adapters are actually sharing the same
+ underlying GMBUS controller. i2c core protects concurrent use of
+ the same i2c adapter but is not aware of the concurrent use of the
+ underlying GMBUS controller. gmbus_mutex is used to prevent the race
+ condition from the perspective of GMBUS controller.
+ */
+ struct mutex gmbus_mutex;
+
struct pci_dev *bridge_dev;
struct intel_ring_buffer ring[I915_NUM_RINGS];
uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30..fc75d71 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,15 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter-algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus-force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(dev_priv-gmbus_mutex);
+
+ if (bus-force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus-force_bit, msgs, num);
+ goto out;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv-dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -321,7 +325,8 @@ done:
* start of the next xfer, till then let it sleep.
*/
I915_WRITE(GMBUS0 + reg_offset, 0);
- return i;
+ ret = i;
+ goto out;
timeout:
DRM_INFO(GMBUS timed out, falling back to bit banging on pin %d
[%s]\n,
@@ -331,9 +336,12 @@ timeout:
/* 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)
- return -ENOMEM;
-
- return intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
+out:
+ mutex_unlock(dev_priv-gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -380,6 +388,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 GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = dev_priv-gmbus[i];
--
1.7.3.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] [PATCH] drm/i915: Fix race condition in accessing GMBUS
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen mile...@chromium.org
---
drivers/gpu/drm/i915/i915_drv.h |2 ++
drivers/gpu/drm/i915/intel_i2c.c | 23 +--
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 559fb6f..4ed9fd9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -722,6 +722,8 @@ typedef struct drm_i915_private {
u8 corr;
spinlock_t *mchdev_lock;
+ struct mutex gmbus_mutex;
+
enum no_fbc_reason no_fbc_reason;
struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d98cee6..42569b1 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -232,11 +232,15 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter-algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus-force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(dev_priv-gmbus_mutex);
+
+ if (bus-force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus-force_bit, msgs, num);
+ goto out;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv-dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -320,7 +324,8 @@ done:
* start of the next xfer, till then let it sleep.
*/
I915_WRITE(GMBUS0 + reg_offset, 0);
- return i;
+ ret = i;
+ goto out;
timeout:
DRM_INFO(GMBUS timed out, falling back to bit banging on pin %d
[%s]\n,
@@ -330,9 +335,13 @@ timeout:
/* 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)
- return -ENOMEM;
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
- return intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
+out:
+ mutex_unlock(dev_priv-gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -379,6 +388,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 GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = dev_priv-gmbus[i];
--
1.7.3.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] [PATCH] drm/i915: Fix race condition in accessing GMBUS
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen
---
drivers/gpu/drm/i915/i915_drv.h |2 ++
drivers/gpu/drm/i915/intel_i2c.c | 23 +--
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 559fb6f..4ed9fd9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -722,6 +722,8 @@ typedef struct drm_i915_private {
u8 corr;
spinlock_t *mchdev_lock;
+ struct mutex gmbus_mutex;
+
enum no_fbc_reason no_fbc_reason;
struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d98cee6..42569b1 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -232,11 +232,15 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter->algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus->force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(_priv->gmbus_mutex);
+
+ if (bus->force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus->force_bit, msgs, num);
+ goto out;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -320,7 +324,8 @@ done:
* start of the next xfer, till then let it sleep.
*/
I915_WRITE(GMBUS0 + reg_offset, 0);
- return i;
+ ret = i;
+ goto out;
timeout:
DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d
[%s]\n",
@@ -330,9 +335,13 @@ timeout:
/* 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)
- return -ENOMEM;
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
- return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+out:
+ mutex_unlock(_priv->gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -379,6 +388,8 @@ int intel_setup_gmbus(struct drm_device *dev)
if (dev_priv->gmbus == NULL)
return -ENOMEM;
+ mutex_init(_priv->gmbus_mutex);
+
for (i = 0; i < GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = _priv->gmbus[i];
--
1.7.3.4
[PATCH] drm/i915: Fix race condition in accessing GMBUS
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen
---
drivers/gpu/drm/i915/i915_drv.h |2 ++
drivers/gpu/drm/i915/intel_i2c.c | 21 -
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9689ca3..d0f826e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -733,6 +733,8 @@ typedef struct drm_i915_private {
u8 corr;
spinlock_t *mchdev_lock;
+ struct mutex gmbus_mutex;
+
enum no_fbc_reason no_fbc_reason;
struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30..24fafdc 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,16 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter->algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus->force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(_priv->gmbus_mutex);
+
+ if (bus->force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus->force_bit, msgs, num);
+ mutex_unlock(_priv->gmbus_mutex);
+ return ret;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -321,6 +326,7 @@ done:
* start of the next xfer, till then let it sleep.
*/
I915_WRITE(GMBUS0 + reg_offset, 0);
+ mutex_unlock(_priv->gmbus_mutex);
return i;
timeout:
@@ -331,9 +337,12 @@ timeout:
/* 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)
- return -ENOMEM;
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
- return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs, num);
+ mutex_unlock(_priv->gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -380,6 +389,8 @@ int intel_setup_gmbus(struct drm_device *dev)
if (dev_priv->gmbus == NULL)
return -ENOMEM;
+ mutex_init(_priv->gmbus_mutex);
+
for (i = 0; i < GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = _priv->gmbus[i];
--
1.7.3.1
[PATCH] drm/i915: Fix race condition in accessing GMBUS
GMBUS has several ports and each has it's own corresponding
I2C adpater. When multiple I2C adapters call gmbus_xfer() at
the same time there is a race condition in using the underlying
GMBUS controller. Fixing this by adding a mutex lock when calling
gmbus_xfer().
Signed-off-by: Yufeng Shen mile...@chromium.org
---
drivers/gpu/drm/i915/i915_drv.h |2 ++
drivers/gpu/drm/i915/intel_i2c.c | 21 -
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9689ca3..d0f826e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -733,6 +733,8 @@ typedef struct drm_i915_private {
u8 corr;
spinlock_t *mchdev_lock;
+ struct mutex gmbus_mutex;
+
enum no_fbc_reason no_fbc_reason;
struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30..24fafdc 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -233,11 +233,16 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = adapter-algo_data;
- int i, reg_offset;
+ int i, reg_offset, ret;
- if (bus-force_bit)
- return intel_i2c_quirk_xfer(dev_priv,
+ mutex_lock(dev_priv-gmbus_mutex);
+
+ if (bus-force_bit) {
+ ret = intel_i2c_quirk_xfer(dev_priv,
bus-force_bit, msgs, num);
+ mutex_unlock(dev_priv-gmbus_mutex);
+ return ret;
+ }
reg_offset = HAS_PCH_SPLIT(dev_priv-dev) ? PCH_GMBUS0 - GMBUS0 : 0;
@@ -321,6 +326,7 @@ 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:
@@ -331,9 +337,12 @@ timeout:
/* 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)
- return -ENOMEM;
+ ret = -ENOMEM;
+ else
+ ret = intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
- return intel_i2c_quirk_xfer(dev_priv, bus-force_bit, msgs, num);
+ mutex_unlock(dev_priv-gmbus_mutex);
+ return ret;
}
static u32 gmbus_func(struct i2c_adapter *adapter)
@@ -380,6 +389,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 GMBUS_NUM_PORTS; i++) {
struct intel_gmbus *bus = dev_priv-gmbus[i];
--
1.7.3.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
