[PATCH 0/4] drm/i915: GMBUS fixes and whatnot

[PATCH 0/4] drm/i915: GMBUS fixes and whatnot

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

While I was frobbing around with DP++ dongles I noticed that out
GMBUS bit-banging fallbacks weren't working as I was expecting, so
after a bit of digging around I came up wit these patches.

Ville Syrjälä (4):
  drm/i915: Actually retry with bit-banging after GMBUS timeout
  drm/i915: Protect force_bit with gmbus_mutex
  drm/i915: Restore GMBUS operation after a failed bit-banging fallback
  drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS

 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_i2c.c | 25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Actually retry with bit-banging after GMBUS timeout

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

After the GMBUS transfer times out, we set force_bit=1 and
return -EAGAIN expecting the i2c core to call the .master_xfer
hook again so that we will retry the same transfer via bit-banging.
This is in case the gmbus hardware is somehow faulty.

Unfortunately we left adapter->retries to 0, meaning the i2c core
didn't actually do the retry. Let's tell the core we want one retry
when we return -EAGAIN.

Note that i2c-algo-bit also uses this retry count for some internal
retries, so we'll end up increasing those a bit as well.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Fixes: bffce907d640 ("drm/i915: abstract i2c bit banging fallback in gmbus xfer")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index deb8282c26d8..52fbe530fc9e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -664,6 +664,12 @@ int intel_setup_gmbus(struct drm_device *dev)
 
 		bus->adapter.algo = &gmbus_algorithm;
 
+		/*
+		 * We wish to retry with bit banging
+		 * after a timed out GMBUS attempt.
+		 */
+		bus->adapter.retries = 1;
+
 		/* By default use a conservative clock rate */
 		bus->reg0 = pin | GMBUS_RATE_100KHZ;
 
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Protect force_bit with gmbus_mutex

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Extend the protection of gmbus_mutex around the force_bit
RMW in intel_gmbus_force_bit(), in case someone gets the
idea of calling it from a separate thread while there's
other stuff happening on the same bus.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 52fbe530fc9e..7bf8a485e18f 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -718,11 +718,16 @@ void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
 void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
 {
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
+	struct drm_i915_private *dev_priv = bus->dev_priv;
+
+	mutex_lock(&dev_priv->gmbus_mutex);
 
 	bus->force_bit += force_bit ? 1 : -1;
 	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
 		      force_bit ? "en" : "dis", adapter->name,
 		      bus->force_bit);
+
+	mutex_unlock(&dev_priv->gmbus_mutex);
 }
 
 void intel_teardown_gmbus(struct drm_device *dev)
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Restore GMBUS operation after a failed bit-banging fallback

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When the GMBUS based i2c transfer times out, we try to fall back to
bit-banging and retry the operation that way. However if the bit-banging
attempt also fails, we should probably go back to the GMBUS method for
the next attempt. Maybe there simply wasn't anyone one the bus at this
time.

There's also a bit of a mess going on with the force_bit handling.
It's supposed to be a ref count actually, and it is as far as
intel_gmbus_force_bit() is concerned. But it's treated as just a
flag by the timeout based bit-banging fallback. I suppose that's
fine since we should never end up in the timeout fallback case
if force_bit was already non-zero. However now that we want to restore
things back to where they were after the bit-banging attempt failed,
we're going to have to do things a bit differently to avoid clobbering
the force_bit count as set up by intel_gmbus_force_bit(). So let's
dedicate the high bit as a flag for the low level timeout based fallback
and treat the rest of the bits as a ref count just as before.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_i2c.c | 10 +++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f37ac120a29d..2348fea59592 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1043,6 +1043,7 @@ struct intel_fbc_work;
 
 struct intel_gmbus {
 	struct i2c_adapter adapter;
+#define GMBUS_FORCE_BIT_RETRY (1U << 31)
 	u32 force_bit;
 	u32 reg0;
 	i915_reg_t gpio_reg;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 7bf8a485e18f..5d4b3604afd2 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -579,7 +579,6 @@ timeout:
 	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
 	 * instead. Use EAGAIN to have i2c core retry.
 	 */
-	bus->force_bit = 1;
 	ret = -EAGAIN;
 
 out:
@@ -597,10 +596,15 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 	mutex_lock(&dev_priv->gmbus_mutex);
 
-	if (bus->force_bit)
+	if (bus->force_bit) {
 		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
-	else
+		if (ret < 0)
+			bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
+	} else {
 		ret = do_gmbus_xfer(adapter, msgs, num);
+		if (ret == -EAGAIN)
+			bus->force_bit |= GMBUS_FORCE_BIT_RETRY;
+	}
 
 	mutex_unlock(&dev_priv->gmbus_mutex);
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

There's no real reason the user should care that we're about to fall
back to bitbanging, so let's change the message from DRM_INFO to
DRM_DEBUG_KMS.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 5d4b3604afd2..edeb74c94d88 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -571,8 +571,8 @@ clear_err:
 	goto out;
 
 timeout:
-	DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
-		 bus->adapter.name, bus->reg0 & 0xff);
+	DRM_DEBUG_KMS("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
+		      bus->adapter.name, bus->reg0 & 0xff);
 	I915_WRITE(GMBUS0, 0);
 
 	/*
-- 
2.4.10

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Actually retry with bit-banging after GMBUS timeout

[Jani Nikula]

On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> After the GMBUS transfer times out, we set force_bit=1 and
> return -EAGAIN expecting the i2c core to call the .master_xfer
> hook again so that we will retry the same transfer via bit-banging.
> This is in case the gmbus hardware is somehow faulty.
>
> Unfortunately we left adapter->retries to 0, meaning the i2c core
> didn't actually do the retry. Let's tell the core we want one retry
> when we return -EAGAIN.
>
> Note that i2c-algo-bit also uses this retry count for some internal
> retries, so we'll end up increasing those a bit as well.

I think I must have confused this with the dp aux i2c adapter retries
which does get initialized to 3. Meh.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Fixes: bffce907d640 ("drm/i915: abstract i2c bit banging fallback in gmbus xfer")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index deb8282c26d8..52fbe530fc9e 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -664,6 +664,12 @@ int intel_setup_gmbus(struct drm_device *dev)
>  
>  		bus->adapter.algo = &gmbus_algorithm;
>  
> +		/*
> +		 * We wish to retry with bit banging
> +		 * after a timed out GMBUS attempt.
> +		 */
> +		bus->adapter.retries = 1;
> +
>  		/* By default use a conservative clock rate */
>  		bus->reg0 = pin | GMBUS_RATE_100KHZ;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for drm/i915: GMBUS fixes and whatnot

[Patchwork]

== Series Details ==

Series: drm/i915: GMBUS fixes and whatnot
URL   : https://patchwork.freedesktop.org/series/4178/
State : warning

== Summary ==

Series 4178v1 drm/i915: GMBUS fixes and whatnot
http://patchwork.freedesktop.org/api/1.0/series/4178/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (hsw-gt2)
                fail       -> PASS       (hsw-brixbox)
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (hsw-gt2)
                pass       -> DMESG-WARN (hsw-brixbox)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> DMESG-WARN (snb-x220t)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (hsw-brixbox)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-fail -> FAIL       (snb-x220t)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (snb-x220t)

bdw-nuci7        total:183  pass:172  dwarn:0   dfail:0   fail:0   skip:11 
bsw-nuc-2        total:183  pass:149  dwarn:0   dfail:0   fail:0   skip:34 
byt-nuc          total:183  pass:152  dwarn:0   dfail:0   fail:0   skip:31 
hsw-brixbox      total:183  pass:162  dwarn:2   dfail:0   fail:0   skip:19 
hsw-gt2          total:183  pass:169  dwarn:0   dfail:0   fail:0   skip:14 
ivb-t430s        total:183  pass:162  dwarn:0   dfail:0   fail:0   skip:21 
skl-i5k-2        total:183  pass:163  dwarn:0   dfail:0   fail:0   skip:20 
snb-dellxps      total:183  pass:153  dwarn:1   dfail:0   fail:0   skip:29 
snb-x220t        total:183  pass:153  dwarn:1   dfail:0   fail:1   skip:28 

Results at /archive/results/CI_IGT_test/Patchwork_1537/

dd7b01270c8b2048c35b3c336431c50c05a07718 drm-intel-nightly: 2016y-03m-07d-18h-03m-57s UTC integration manifest
52f3fbed204983eeefcfd2e6bcdfd95bcfd6a324 drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS
3a2a906288c099bc6a8ddf81092da3423b6065c9 drm/i915: Restore GMBUS operation after a failed bit-banging fallback
770fe6a15dbf7330b964ddd3db44c6068b607669 drm/i915: Protect force_bit with gmbus_mutex
4aa7bf0b8885ef5bd55280c51bd0d48822d02c2e drm/i915: Actually retry with bit-banging after GMBUS timeout

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: warning for drm/i915: GMBUS fixes and whatnot

[Ville Syrjälä]

On Tue, Mar 08, 2016 at 07:25:05AM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915: GMBUS fixes and whatnot
> URL   : https://patchwork.freedesktop.org/series/4178/
> State : warning
> 
> == Summary ==
> 
> Series 4178v1 drm/i915: GMBUS fixes and whatnot
> http://patchwork.freedesktop.org/api/1.0/series/4178/revisions/1/mbox/
> 
> Test kms_flip:
>         Subgroup basic-flip-vs-wf_vblank:
>                 fail       -> PASS       (hsw-gt2)
>                 fail       -> PASS       (hsw-brixbox)
>         Subgroup basic-plain-flip:
>                 dmesg-warn -> PASS       (hsw-gt2)
>                 pass       -> DMESG-WARN (hsw-brixbox)

[  276.675359]  [<ffffffffa0193a8c>] hsw_write32+0x1dc/0x270 [i915]
[  276.675387]  [<ffffffffa013cb88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]

> Test kms_frontbuffer_tracking:
>         Subgroup basic:
>                 pass       -> DMESG-WARN (snb-x220t)

[  209.900547]  [<ffffffffa037d0cc>] gen6_write32+0x1dc/0x270 [i915]
[  209.900584]  [<ffffffffa0326b88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]

> Test kms_pipe_crc_basic:
>         Subgroup nonblocking-crc-pipe-b:
>                 pass       -> DMESG-WARN (hsw-brixbox)

[  301.672605]  [<ffffffffa0193acb>] hsw_write32+0x21b/0x270 [i915]
[  301.672614]  [<ffffffffa013cb88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]

>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> PASS       (bsw-nuc-2)
> Test pm_rpm:
>         Subgroup basic-pci-d3-state:
>                 dmesg-fail -> FAIL       (snb-x220t)

dmesg ping pong
[   89.058704]  [<ffffffffa022a0cc>] gen6_write32+0x1dc/0x270 [i915]
[   89.058777]  [<ffffffffa01d3b88>] _ilk_disable_lp_wm+0x98/0xd0 [i915]

So all are
https://bugs.freedesktop.org/show_bug.cgi?id=94349

>         Subgroup basic-rte:
>                 dmesg-warn -> PASS       (snb-x220t)
> 
> bdw-nuci7        total:183  pass:172  dwarn:0   dfail:0   fail:0   skip:11 
> bsw-nuc-2        total:183  pass:149  dwarn:0   dfail:0   fail:0   skip:34 
> byt-nuc          total:183  pass:152  dwarn:0   dfail:0   fail:0   skip:31 
> hsw-brixbox      total:183  pass:162  dwarn:2   dfail:0   fail:0   skip:19 
> hsw-gt2          total:183  pass:169  dwarn:0   dfail:0   fail:0   skip:14 
> ivb-t430s        total:183  pass:162  dwarn:0   dfail:0   fail:0   skip:21 
> skl-i5k-2        total:183  pass:163  dwarn:0   dfail:0   fail:0   skip:20 
> snb-dellxps      total:183  pass:153  dwarn:1   dfail:0   fail:0   skip:29 
> snb-x220t        total:183  pass:153  dwarn:1   dfail:0   fail:1   skip:28 
> 
> Results at /archive/results/CI_IGT_test/Patchwork_1537/
> 
> dd7b01270c8b2048c35b3c336431c50c05a07718 drm-intel-nightly: 2016y-03m-07d-18h-03m-57s UTC integration manifest
> 52f3fbed204983eeefcfd2e6bcdfd95bcfd6a324 drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS
> 3a2a906288c099bc6a8ddf81092da3423b6065c9 drm/i915: Restore GMBUS operation after a failed bit-banging fallback
> 770fe6a15dbf7330b964ddd3db44c6068b607669 drm/i915: Protect force_bit with gmbus_mutex
> 4aa7bf0b8885ef5bd55280c51bd0d48822d02c2e drm/i915: Actually retry with bit-banging after GMBUS timeout

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Actually retry with bit-banging after GMBUS timeout

[Ville Syrjälä]

On Mon, Mar 07, 2016 at 07:06:37PM +0200, Jani Nikula wrote:
> On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > After the GMBUS transfer times out, we set force_bit=1 and
> > return -EAGAIN expecting the i2c core to call the .master_xfer
> > hook again so that we will retry the same transfer via bit-banging.
> > This is in case the gmbus hardware is somehow faulty.
> >
> > Unfortunately we left adapter->retries to 0, meaning the i2c core
> > didn't actually do the retry. Let's tell the core we want one retry
> > when we return -EAGAIN.
> >
> > Note that i2c-algo-bit also uses this retry count for some internal
> > retries, so we'll end up increasing those a bit as well.
> 
> I think I must have confused this with the dp aux i2c adapter retries
> which does get initialized to 3. Meh.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Patch pushed to dinq. Thanks for the review.

> 
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Fixes: bffce907d640 ("drm/i915: abstract i2c bit banging fallback in gmbus xfer")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_i2c.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index deb8282c26d8..52fbe530fc9e 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -664,6 +664,12 @@ int intel_setup_gmbus(struct drm_device *dev)
> >  
> >  		bus->adapter.algo = &gmbus_algorithm;
> >  
> > +		/*
> > +		 * We wish to retry with bit banging
> > +		 * after a timed out GMBUS attempt.
> > +		 */
> > +		bus->adapter.retries = 1;
> > +
> >  		/* By default use a conservative clock rate */
> >  		bus->reg0 = pin | GMBUS_RATE_100KHZ;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS

[Ville Syrjälä]

On Mon, Mar 07, 2016 at 05:57:00PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> There's no real reason the user should care that we're about to fall
> back to bitbanging, so let's change the message from DRM_INFO to
> DRM_DEBUG_KMS.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94890

> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 5d4b3604afd2..edeb74c94d88 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -571,8 +571,8 @@ clear_err:
>  	goto out;
>  
>  timeout:
> -	DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
> -		 bus->adapter.name, bus->reg0 & 0xff);
> +	DRM_DEBUG_KMS("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
> +		      bus->adapter.name, bus->reg0 & 0xff);
>  	I915_WRITE(GMBUS0, 0);
>  
>  	/*
> -- 
> 2.4.10

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS

[Chris Wilson]

On Mon, Apr 11, 2016 at 10:29:29AM +0300, Ville Syrjälä wrote:
> On Mon, Mar 07, 2016 at 05:57:00PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > There's no real reason the user should care that we're about to fall
> > back to bitbanging, so let's change the message from DRM_INFO to
> > DRM_DEBUG_KMS.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94890

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

KMS or DRIVER though? GMBUS is now independent of KMS, so probably worth
keeping it separate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Protect force_bit with gmbus_mutex

[Jani Nikula]

On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Extend the protection of gmbus_mutex around the force_bit
> RMW in intel_gmbus_force_bit(), in case someone gets the
> idea of calling it from a separate thread while there's
> other stuff happening on the same bus.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

intel_gmbus_is_forced_bit() usage in intel_crt_get_edid() seems a bit
suspect still, but this is a step forward.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 52fbe530fc9e..7bf8a485e18f 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -718,11 +718,16 @@ void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
>  void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>  {
>  	struct intel_gmbus *bus = to_intel_gmbus(adapter);
> +	struct drm_i915_private *dev_priv = bus->dev_priv;
> +
> +	mutex_lock(&dev_priv->gmbus_mutex);
>  
>  	bus->force_bit += force_bit ? 1 : -1;
>  	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
>  		      force_bit ? "en" : "dis", adapter->name,
>  		      bus->force_bit);
> +
> +	mutex_unlock(&dev_priv->gmbus_mutex);
>  }
>  
>  void intel_teardown_gmbus(struct drm_device *dev)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Restore GMBUS operation after a failed bit-banging fallback

[Jani Nikula]

On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When the GMBUS based i2c transfer times out, we try to fall back to
> bit-banging and retry the operation that way. However if the bit-banging
> attempt also fails, we should probably go back to the GMBUS method for
> the next attempt. Maybe there simply wasn't anyone one the bus at this
> time.
>
> There's also a bit of a mess going on with the force_bit handling.
> It's supposed to be a ref count actually, and it is as far as
> intel_gmbus_force_bit() is concerned. But it's treated as just a
> flag by the timeout based bit-banging fallback. I suppose that's
> fine since we should never end up in the timeout fallback case
> if force_bit was already non-zero. However now that we want to restore
> things back to where they were after the bit-banging attempt failed,
> we're going to have to do things a bit differently to avoid clobbering
> the force_bit count as set up by intel_gmbus_force_bit(). So let's
> dedicate the high bit as a flag for the low level timeout based fallback
> and treat the rest of the bits as a ref count just as before.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_i2c.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f37ac120a29d..2348fea59592 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_fbc_work;
>  
>  struct intel_gmbus {
>  	struct i2c_adapter adapter;
> +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>  	u32 force_bit;
>  	u32 reg0;
>  	i915_reg_t gpio_reg;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 7bf8a485e18f..5d4b3604afd2 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -579,7 +579,6 @@ timeout:
>  	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
>  	 * instead. Use EAGAIN to have i2c core retry.
>  	 */
> -	bus->force_bit = 1;
>  	ret = -EAGAIN;
>  
>  out:
> @@ -597,10 +596,15 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  	mutex_lock(&dev_priv->gmbus_mutex);
>  
> -	if (bus->force_bit)
> +	if (bus->force_bit) {
>  		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> -	else
> +		if (ret < 0)
> +			bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
> +	} else {
>  		ret = do_gmbus_xfer(adapter, msgs, num);
> +		if (ret == -EAGAIN)
> +			bus->force_bit |= GMBUS_FORCE_BIT_RETRY;

Hmm, would this all be simpler if we did the first bit-banging retry
here ourselves after all, and set ->force_bit only if bit-banging
succeeds after gmbus -EAGAIN? I think moving the retry out of
do_gmbus_xfer() was the right thing to do to, but maybe I went too far
by pushing it all the way to i2c core?

Anyway, this patch looks good, but it's just a bit subtle with the
-EAGAIN and one retry and all.

Up to you.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	}
>  
>  	mutex_unlock(&dev_priv->gmbus_mutex);
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Make GMBUS timeout message DRM_DEBUG_KMS

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 09:19:40AM +0100, Chris Wilson wrote:
> On Mon, Apr 11, 2016 at 10:29:29AM +0300, Ville Syrjälä wrote:
> > On Mon, Mar 07, 2016 at 05:57:00PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > There's no real reason the user should care that we're about to fall
> > > back to bitbanging, so let's change the message from DRM_INFO to
> > > DRM_DEBUG_KMS.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94890
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> KMS or DRIVER though? GMBUS is now independent of KMS, so probably worth
> keeping it separate.

All the other debug message in the gmbus code are KMS. Keeping to the
party line seems best to me.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Restore GMBUS operation after a failed bit-banging fallback

[Ville Syrjälä]

On Mon, Apr 11, 2016 at 12:50:44PM +0300, Jani Nikula wrote:
> On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > When the GMBUS based i2c transfer times out, we try to fall back to
> > bit-banging and retry the operation that way. However if the bit-banging
> > attempt also fails, we should probably go back to the GMBUS method for
> > the next attempt. Maybe there simply wasn't anyone one the bus at this
> > time.
> >
> > There's also a bit of a mess going on with the force_bit handling.
> > It's supposed to be a ref count actually, and it is as far as
> > intel_gmbus_force_bit() is concerned. But it's treated as just a
> > flag by the timeout based bit-banging fallback. I suppose that's
> > fine since we should never end up in the timeout fallback case
> > if force_bit was already non-zero. However now that we want to restore
> > things back to where they were after the bit-banging attempt failed,
> > we're going to have to do things a bit differently to avoid clobbering
> > the force_bit count as set up by intel_gmbus_force_bit(). So let's
> > dedicate the high bit as a flag for the low level timeout based fallback
> > and treat the rest of the bits as a ref count just as before.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 +
> >  drivers/gpu/drm/i915/intel_i2c.c | 10 +++++++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index f37ac120a29d..2348fea59592 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1043,6 +1043,7 @@ struct intel_fbc_work;
> >  
> >  struct intel_gmbus {
> >  	struct i2c_adapter adapter;
> > +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
> >  	u32 force_bit;
> >  	u32 reg0;
> >  	i915_reg_t gpio_reg;
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index 7bf8a485e18f..5d4b3604afd2 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -579,7 +579,6 @@ timeout:
> >  	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> >  	 * instead. Use EAGAIN to have i2c core retry.
> >  	 */
> > -	bus->force_bit = 1;
> >  	ret = -EAGAIN;
> >  
> >  out:
> > @@ -597,10 +596,15 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> >  	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >  	mutex_lock(&dev_priv->gmbus_mutex);
> >  
> > -	if (bus->force_bit)
> > +	if (bus->force_bit) {
> >  		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> > -	else
> > +		if (ret < 0)
> > +			bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
> > +	} else {
> >  		ret = do_gmbus_xfer(adapter, msgs, num);
> > +		if (ret == -EAGAIN)
> > +			bus->force_bit |= GMBUS_FORCE_BIT_RETRY;
> 
> Hmm, would this all be simpler if we did the first bit-banging retry
> here ourselves after all, and set ->force_bit only if bit-banging
> succeeds after gmbus -EAGAIN? I think moving the retry out of
> do_gmbus_xfer() was the right thing to do to, but maybe I went too far
> by pushing it all the way to i2c core?
> 
> Anyway, this patch looks good, but it's just a bit subtle with the
> -EAGAIN and one retry and all.
> 
> Up to you.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

I left it alone for now, if for no other reason than to avoid
having to redo any testing right now. We can always revisit it later.

Rest of the series pushed to dinq. Thanks for the reviews.

> 
> > +	}
> >  
> >  	mutex_unlock(&dev_priv->gmbus_mutex);
> >  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx