| Message ID | 1476272687-15070-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > GMBUS is part of the display engine, and thus has no need for > forcewake. Let's not bother trying to grab it then. > > I don't recall if the display engine suffers from system hangs > due to multiple accesses to the same "cacheline" in mmio space. > I hope not since we're no longer protected by the uncore lock > since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for > the entire GMBUS transaction") Only applies to concurrent access to the same cacheline, in this case should be serialised by the mutex around the gmbus xfer. > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: David Weinehall <david.weinehall@linux.intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_i2c.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > index 79aab9ad6faa..49c7824a4c29 100644 > --- a/drivers/gpu/drm/i915/intel_i2c.c > +++ b/drivers/gpu/drm/i915/intel_i2c.c > @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) > struct intel_gmbus, > adapter); > struct drm_i915_private *dev_priv = bus->dev_priv; > - const unsigned int fw = > - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, > - FW_REG_READ | FW_REG_WRITE); > int i = 0, inc, try = 0; > int ret = 0; I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, FW_REG_READ | FW_REG_WRITE)); ? Would be good to test the fw handling as well. -Chris
On Wed, Oct 12, 2016 at 12:58:34PM +0100, Chris Wilson wrote: > On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > GMBUS is part of the display engine, and thus has no need for > > forcewake. Let's not bother trying to grab it then. > > > > I don't recall if the display engine suffers from system hangs > > due to multiple accesses to the same "cacheline" in mmio space. > > I hope not since we're no longer protected by the uncore lock > > since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for > > the entire GMBUS transaction") > > Only applies to concurrent access to the same cacheline, in this case > should be serialised by the mutex around the gmbus xfer. Hmm. Yeah, I suppose there shouldn't be unrelated stuff nearby. Haven't double checked though. > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: David Weinehall <david.weinehall@linux.intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_i2c.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > > index 79aab9ad6faa..49c7824a4c29 100644 > > --- a/drivers/gpu/drm/i915/intel_i2c.c > > +++ b/drivers/gpu/drm/i915/intel_i2c.c > > @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) > > struct intel_gmbus, > > adapter); > > struct drm_i915_private *dev_priv = bus->dev_priv; > > - const unsigned int fw = > > - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, > > - FW_REG_READ | FW_REG_WRITE); > > int i = 0, inc, try = 0; > > int ret = 0; > > I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, > FW_REG_READ | > FW_REG_WRITE)); > > ? Would be good to test the fw handling as well. Not sure I'd want to sprinkle forcewake testing into modeset code.
On Wed, Oct 12, 2016 at 03:39:47PM +0300, Ville Syrjälä wrote: > On Wed, Oct 12, 2016 at 12:58:34PM +0100, Chris Wilson wrote: > > On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > GMBUS is part of the display engine, and thus has no need for > > > forcewake. Let's not bother trying to grab it then. > > > > > > I don't recall if the display engine suffers from system hangs > > > due to multiple accesses to the same "cacheline" in mmio space. > > > I hope not since we're no longer protected by the uncore lock > > > since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for > > > the entire GMBUS transaction") > > > > Only applies to concurrent access to the same cacheline, in this case > > should be serialised by the mutex around the gmbus xfer. > > Hmm. Yeah, I suppose there shouldn't be unrelated stuff nearby. Haven't > double checked though. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: David Weinehall <david.weinehall@linux.intel.com> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_i2c.c | 5 ----- > > > 1 file changed, 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > > > index 79aab9ad6faa..49c7824a4c29 100644 > > > --- a/drivers/gpu/drm/i915/intel_i2c.c > > > +++ b/drivers/gpu/drm/i915/intel_i2c.c > > > @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) > > > struct intel_gmbus, > > > adapter); > > > struct drm_i915_private *dev_priv = bus->dev_priv; > > > - const unsigned int fw = > > > - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, > > > - FW_REG_READ | FW_REG_WRITE); > > > int i = 0, inc, try = 0; > > > int ret = 0; > > > > I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, > > FW_REG_READ | > > FW_REG_WRITE)); > > > > ? Would be good to test the fw handling as well. > > Not sure I'd want to sprinkle forcewake testing into modeset code. You never use registers here? ;) Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, Oct 12, 2016 at 01:49:20PM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: GMBUS don't need no forcewake > URL : https://patchwork.freedesktop.org/series/13641/ > State : warning > > == Summary == > > Series 13641v1 drm/i915: GMBUS don't need no forcewake > https://patchwork.freedesktop.org/api/1.0/series/13641/revisions/1/mbox/ > > Test drv_module_reload_basic: > pass -> DMESG-WARN (fi-ilk-650) [ 44.582062] [drm:intel_enable_pipe [i915]] enabling pipe B [ 44.582480] [drm:ironlake_fdi_link_train [i915]] FDI_RX_IIR 0x100 [ 44.582505] [drm:ironlake_fdi_link_train [i915]] FDI train 1 done. [ 44.582684] [drm:ironlake_fdi_link_train [i915]] FDI_RX_IIR 0x600 [ 44.582708] [drm:ironlake_fdi_link_train [i915]] FDI train 2 done. [ 44.582732] [drm:ironlake_fdi_link_train [i915]] FDI train done [ 44.582758] [drm:intel_enable_shared_dpll [i915]] enable PCH DPLL B (active 2, on? 0) for crtc 30 [ 44.582781] [drm:intel_enable_shared_dpll [i915]] enabling PCH DPLL B [ 44.584207] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS1 [ 44.585526] [drm:intel_dp_set_signal_levels [i915]] Using signal levels 00000000 [ 44.585551] [drm:intel_dp_set_signal_levels [i915]] Using vswing level 0 [ 44.585574] [drm:intel_dp_set_signal_levels [i915]] Using pre-emphasis level 0 [ 44.585599] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS1 [ 44.586310] [drm:intel_dp_start_link_train [i915]] clock recovery OK [ 44.586344] [drm:intel_dp_program_link_training_pattern [i915]] Using DP training pattern TPS2 [ 44.587380] [drm:intel_dp_start_link_train [i915]] Channel EQ done. DP Training successful [ 44.587587] [drm:intel_enable_dp [i915]] Enabling DP audio on pipe B [ 44.587614] [drm:intel_audio_codec_enable [i915]] ELD on [CONNECTOR:40:DP-1], [ENCODER:39:DP C] [ 44.587639] [drm:ilk_audio_codec_enable [i915]] Enable audio codec on port C, pipe B, 36 bytes ELD [ 44.604682] [drm:intel_pch_fifo_underrun_irq_handler [i915]] *ERROR* PCH transcoder B FIFO underrun so this is the non-link retraining case, which could be audio related (or not). https://bugs.freedesktop.org/show_bug.cgi?id=98251 Patch pushe to dinq, thanks for the review. > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > dmesg-warn -> PASS (fi-byt-j1900) > Test vgem_basic: > Subgroup unload: > skip -> PASS (fi-hsw-4770) > > fi-bdw-5557u total:248 pass:232 dwarn:0 dfail:0 fail:0 skip:16 > fi-bsw-n3050 total:248 pass:205 dwarn:0 dfail:0 fail:0 skip:43 > fi-bxt-t5700 total:248 pass:217 dwarn:0 dfail:0 fail:0 skip:31 > fi-byt-j1900 total:248 pass:214 dwarn:1 dfail:0 fail:1 skip:32 > fi-byt-n2820 total:248 pass:211 dwarn:0 dfail:0 fail:1 skip:36 > fi-hsw-4770 total:248 pass:225 dwarn:0 dfail:0 fail:0 skip:23 > fi-hsw-4770r total:248 pass:225 dwarn:0 dfail:0 fail:0 skip:23 > fi-ilk-650 total:248 pass:184 dwarn:1 dfail:0 fail:2 skip:61 > fi-ivb-3520m total:248 pass:222 dwarn:0 dfail:0 fail:0 skip:26 > fi-ivb-3770 total:248 pass:222 dwarn:0 dfail:0 fail:0 skip:26 > fi-kbl-7200u total:248 pass:223 dwarn:0 dfail:0 fail:0 skip:25 > fi-skl-6260u total:248 pass:233 dwarn:0 dfail:0 fail:0 skip:15 > fi-skl-6700hq total:248 pass:225 dwarn:0 dfail:0 fail:0 skip:23 > fi-skl-6700k total:248 pass:222 dwarn:1 dfail:0 fail:0 skip:25 > fi-skl-6770hq total:248 pass:231 dwarn:1 dfail:0 fail:1 skip:15 > fi-snb-2520m total:248 pass:211 dwarn:0 dfail:0 fail:0 skip:37 > fi-snb-2600 total:248 pass:210 dwarn:0 dfail:0 fail:0 skip:38 > > Results at /archive/results/CI_IGT_test/Patchwork_2685/ > > 46271d41e30090d7fc996e8f5abde6a59f51038b drm-intel-nightly: 2016y-10m-12d-11h-06m-41s UTC integration manifest > 3d9fd0b drm/i915: GMBUS don't need no forcewake
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 79aab9ad6faa..49c7824a4c29 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) struct intel_gmbus, adapter); struct drm_i915_private *dev_priv = bus->dev_priv; - const unsigned int fw = - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0, - FW_REG_READ | FW_REG_WRITE); int i = 0, inc, try = 0; int ret = 0; - intel_uncore_forcewake_get(dev_priv, fw); retry: I915_WRITE_FW(GMBUS0, bus->reg0); @@ -576,7 +572,6 @@ timeout: ret = -EAGAIN; out: - intel_uncore_forcewake_put(dev_priv, fw); return ret; }