[PATCH v3 0/2] GMBUS changes

[PATCH v3 0/2] GMBUS changes

[Ramalingam C]

I am not aware if there is a reason for restricting the Bytes per GMBUS
WR/RD to 256 at present. But HW has 9Bits for Total Byte count for a
single read or Write cycle. Means we can extend a cycle of RD/WR to
511Bytes.

At present nothing much as ROI, as most of the usecases are for less
than 256Bytes. On GLK tested for 300Bytes on single normal read, found
to be working fine.

First patch does it. But I have restricted the extension to Gen9 onwards,
as I am not sure about the legacy platforms.

And second patch is enabling the burst read for all GMBUS read of more
than 511Bytes, on supported platforms. Basically this Burst read is
enabled in HW for HDCP2.2 compliance requirement. Instead of enabling
the burst read only for HDCP on special API this patch enables it for
all GMBUS read of >511Bytes, on capable platforms.

Changes in V3:
  --Avoided the flag for burst read notification [Jani]
  --Runtime Detection of the need for burst_read [Jani]
  --Simplified the calculation for bytes_af_burst.

Ramalingam C (2):
  drm/i915/gmbus: Increase the Bytes per Rd/Wr Op
  drm/i915/gmbus: Enable burst read

 drivers/gpu/drm/i915/i915_drv.h  |  3 +++
 drivers/gpu/drm/i915/i915_reg.h  |  2 ++
 drivers/gpu/drm/i915/intel_i2c.c | 31 ++++++++++++++++++++++++++++---
 3 files changed, 33 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Ramalingam C]

From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
511Bytes instead of previous 256Bytes used in SW.

This change allows the max bytes per op upto 511Bytes from Gen9 onwards.

v2:
  No Change.
v3:
  Inline function for max_xfer_size and renaming of the macro.[Jani]

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 475cac07d3e6..be6114a0e8ab 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3013,6 +3013,7 @@ enum i915_power_well_id {
 #define   GMBUS_CYCLE_STOP	(4<<25)
 #define   GMBUS_BYTE_COUNT_SHIFT 16
 #define   GMBUS_BYTE_COUNT_MAX   256U
+#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
 #define   GMBUS_SLAVE_INDEX_SHIFT 8
 #define   GMBUS_SLAVE_ADDR_SHIFT 1
 #define   GMBUS_SLAVE_READ	(1<<0)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e6875509bcd9..4367827d7661 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+static inline
+unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
+{
+	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
+		GMBUS_BYTE_COUNT_MAX;
+}
+
 static int
 gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
 		      unsigned short addr, u8 *buf, unsigned int len,
@@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	int ret;
 
 	do {
-		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
+		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
 
 		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
 					    buf, len, gmbus1_index);
@@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	int ret;
 
 	do {
-		len = min(tx_size, GMBUS_BYTE_COUNT_MAX);
+		len = min(tx_size, gmbus_max_xfer_size(dev_priv));
 
 		ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len,
 					     gmbus1_index);
-- 
2.7.4

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

[PATCH v3 2/2] drm/i915/gmbus: Enable burst read

[Ramalingam C]

Support for Burst read in HW is added for HDCP2.2 compliance
requirement.

This patch enables the burst read for all the gmbus read of more than
511Bytes, on capable platforms.

v2:
  Extra line is removed.
v3:
  Macro is added for detecting the BURST_READ Support [Jani]
  Runtime detection of the need for burst_read [Jani]

Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  3 +++
 drivers/gpu/drm/i915/i915_reg.h  |  1 +
 drivers/gpu/drm/i915/intel_i2c.c | 22 ++++++++++++++++++++--
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5373b171bb96..9cddcaa3efb2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2548,6 +2548,9 @@ intel_info(const struct drm_i915_private *dev_priv)
  */
 #define HAS_AUX_IRQ(dev_priv)   true
 #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
+#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
+					IS_GEMINILAKE(dev_priv) || \
+					IS_KABYLAKE(dev_priv))
 
 /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
  * rows, which changed the alignment requirements and fence programming.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index be6114a0e8ab..8b4e6363c7d2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2984,6 +2984,7 @@ enum i915_power_well_id {
 #define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
 #define   GMBUS_RATE_1MHZ	(3<<8) /* reserved on Pineview */
 #define   GMBUS_HOLD_EXT	(1<<7) /* 300ns hold time, rsvd on Pineview */
+#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
 #define   GMBUS_PIN_DISABLED	0
 #define   GMBUS_PIN_SSC		1
 #define   GMBUS_PIN_VGADDC	2
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 4367827d7661..cb260f667cfa 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -373,10 +373,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
 		      unsigned short addr, u8 *buf, unsigned int len,
 		      u32 gmbus1_index)
 {
+	unsigned int bytes_af_override = 0, size = len;
+	bool burst_read = len > gmbus_max_xfer_size(dev_priv);
+
+	if (burst_read) {
+		bytes_af_override = (len % 256) + 256;
+		size = bytes_af_override;
+
+		I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
+			      GMBUS_BYTE_CNT_OVERRIDE));
+	}
+
 	I915_WRITE_FW(GMBUS1,
 		      gmbus1_index |
 		      GMBUS_CYCLE_WAIT |
-		      (len << GMBUS_BYTE_COUNT_SHIFT) |
+		      (size << GMBUS_BYTE_COUNT_SHIFT) |
 		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
 	while (len) {
@@ -392,6 +403,10 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
 			*buf++ = val & 0xff;
 			val >>= 8;
 		} while (--len && ++loop < 4);
+
+		if (burst_read && len == (bytes_af_override - 4))
+			I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
+				      ~GMBUS_BYTE_CNT_OVERRIDE));
 	}
 
 	return 0;
@@ -407,7 +422,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	int ret;
 
 	do {
-		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
+		if (HAS_GMBUS_BURST_READ(dev_priv))
+			len = rx_size;
+		else
+			len = min(rx_size, gmbus_max_xfer_size(dev_priv));
 
 		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
 					    buf, len, gmbus1_index);
-- 
2.7.4

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

✗ Fi.CI.CHECKPATCH: warning for GMBUS changes (rev3)

[Patchwork]

== Series Details ==

Series: GMBUS changes (rev3)
URL   : https://patchwork.freedesktop.org/series/41632/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
16f60217eb21 drm/i915/gmbus: Increase the Bytes per Rd/Wr Op
bcedea2a9483 drm/i915/gmbus: Enable burst read
-:28: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'dev_priv' - possible side-effects?
#28: FILE: drivers/gpu/drm/i915/i915_drv.h:2554:
+#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
+					IS_GEMINILAKE(dev_priv) || \
+					IS_KABYLAKE(dev_priv))

-:42: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
#42: FILE: drivers/gpu/drm/i915/i915_reg.h:2999:
+#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
                                     ^

total: 0 errors, 0 warnings, 2 checks, 59 lines checked

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

✗ Fi.CI.SPARSE: warning for GMBUS changes (rev3)

[Patchwork]

== Series Details ==

Series: GMBUS changes (rev3)
URL   : https://patchwork.freedesktop.org/series/41632/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Commit: drm/i915/gmbus: Increase the Bytes per Rd/Wr Op
-O:drivers/gpu/drm/i915/intel_i2c.c:403:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_i2c.c:465:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:410:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:410:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:472:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:472:23: warning: expression using sizeof(void)

Commit: drm/i915/gmbus: Enable burst read
-O:drivers/gpu/drm/i915/intel_i2c.c:410:23: warning: expression using sizeof(void)
-O:drivers/gpu/drm/i915/intel_i2c.c:410:23: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:428:31: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_i2c.c:428:31: warning: expression using sizeof(void)
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3655:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3658:16: warning: expression using sizeof(void)

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

✓ Fi.CI.BAT: success for GMBUS changes (rev3)

[Patchwork]

== Series Details ==

Series: GMBUS changes (rev3)
URL   : https://patchwork.freedesktop.org/series/41632/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4059 -> Patchwork_8703 =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8703 need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8703, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41632/revisions/3/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8703:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_gttfill@basic:
      fi-pnv-d510:        PASS -> SKIP

    
== Known issues ==

  Here are the changes found in Patchwork_8703 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#105128)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-ivb-3520m:       PASS -> DMESG-WARN (fdo#106084) +1

    
    ==== Possible fixes ====

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-elk-e7500:       INCOMPLETE (fdo#103989) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS

    
  fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (36 -> 33) ==

  Missing    (3): fi-ctg-p8600 fi-ilk-m540 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4059 -> Patchwork_8703

  CI_DRM_4059: c1645edc253f2b52a8c94565a75b479a6782e75f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4435: ddbe5a4d8bb1780ecf07f72e815062d3bce8ff71 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8703: bcedea2a9483baceeda55a73119ea1c49d4a1a0d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4435: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

bcedea2a9483 drm/i915/gmbus: Enable burst read
16f60217eb21 drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8703/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.CHECKPATCH: warning for GMBUS changes (rev3)

[Jani Nikula]

On Tue, 17 Apr 2018, Patchwork <patchwork@emeril.freedesktop.org> wrote:
> == Series Details ==
>
> Series: GMBUS changes (rev3)
> URL   : https://patchwork.freedesktop.org/series/41632/
> State : warning
>
> == Summary ==
>
> $ dim checkpatch origin/drm-tip
> 16f60217eb21 drm/i915/gmbus: Increase the Bytes per Rd/Wr Op
> bcedea2a9483 drm/i915/gmbus: Enable burst read
> -:28: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'dev_priv' - possible side-effects?
> #28: FILE: drivers/gpu/drm/i915/i915_drv.h:2554:
> +#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
> +					IS_GEMINILAKE(dev_priv) || \
> +					IS_KABYLAKE(dev_priv))
>
> -:42: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
> #42: FILE: drivers/gpu/drm/i915/i915_reg.h:2999:
> +#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
>                                      ^

Both can be ignored.

>
> total: 0 errors, 0 warnings, 2 checks, 59 lines checked
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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.IGT: success for GMBUS changes (rev3)

[Patchwork]

== Series Details ==

Series: GMBUS changes (rev3)
URL   : https://patchwork.freedesktop.org/series/41632/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4059_full -> Patchwork_8703_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_8703_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_8703_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41632/revisions/3/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_8703_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-vebox:
      shard-kbl:          PASS -> SKIP +1

    igt@kms_frontbuffer_tracking@fbc-rgb101010-draw-blt:
      shard-snb:          PASS -> SKIP +1

    
== Known issues ==

  Here are the changes found in Patchwork_8703_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume-context:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_draw_crc@draw-method-rgb565-mmap-wc-xtiled:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +2

    igt@kms_flip@2x-dpms-vs-vblank-race:
      shard-hsw:          PASS -> FAIL (fdo#103060)

    igt@kms_flip@plain-flip-fb-recreate:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-apl:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_flip@modeset-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602


== Participating hosts (6 -> 4) ==

  Missing    (2): shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4059 -> Patchwork_8703

  CI_DRM_4059: c1645edc253f2b52a8c94565a75b479a6782e75f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4435: ddbe5a4d8bb1780ecf07f72e815062d3bce8ff71 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8703: bcedea2a9483baceeda55a73119ea1c49d4a1a0d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4435: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8703/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Ville Syrjälä]

On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote:
> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
> 511Bytes instead of previous 256Bytes used in SW.
> 
> This change allows the max bytes per op upto 511Bytes from Gen9 onwards.
> 
> v2:
>   No Change.
> v3:
>   Inline function for max_xfer_size and renaming of the macro.[Jani]
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 475cac07d3e6..be6114a0e8ab 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3013,6 +3013,7 @@ enum i915_power_well_id {
>  #define   GMBUS_CYCLE_STOP	(4<<25)
>  #define   GMBUS_BYTE_COUNT_SHIFT 16
>  #define   GMBUS_BYTE_COUNT_MAX   256U
> +#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
>  #define   GMBUS_SLAVE_INDEX_SHIFT 8
>  #define   GMBUS_SLAVE_ADDR_SHIFT 1
>  #define   GMBUS_SLAVE_READ	(1<<0)
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index e6875509bcd9..4367827d7661 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +static inline
> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
> +{
> +	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
> +		GMBUS_BYTE_COUNT_MAX;

Hmm. You sure about this 256 limit on older HW? The spec does sort of
say that 0-256 is the valid range, but the SPT+ docs still have that
same text, and the register has always had 9 bits for byte count. I
don't see any statements saying that they changed this in any way for
SPT. It only talks about >511 bytes needing the special treatment.

If we do this the I think you should just drop the defines and put the
raw numbers into this function. The extra indirection just makes life
harder. Also pointless parens around the GEN>9 check.

> +}
> +
>  static int
>  gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>  		      unsigned short addr, u8 *buf, unsigned int len,
> @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	int ret;
>  
>  	do {
> -		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
> +		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>  
>  		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>  					    buf, len, gmbus1_index);
> @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	int ret;
>  
>  	do {
> -		len = min(tx_size, GMBUS_BYTE_COUNT_MAX);
> +		len = min(tx_size, gmbus_max_xfer_size(dev_priv));
>  
>  		ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len,
>  					     gmbus1_index);
> -- 
> 2.7.4

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

Re: [PATCH v3 2/2] drm/i915/gmbus: Enable burst read

[Ville Syrjälä]

On Tue, Apr 17, 2018 at 02:25:33PM +0530, Ramalingam C wrote:
> Support for Burst read in HW is added for HDCP2.2 compliance
> requirement.
> 
> This patch enables the burst read for all the gmbus read of more than
> 511Bytes, on capable platforms.
> 
> v2:
>   Extra line is removed.
> v3:
>   Macro is added for detecting the BURST_READ Support [Jani]
>   Runtime detection of the need for burst_read [Jani]
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +++
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_i2c.c | 22 ++++++++++++++++++++--
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5373b171bb96..9cddcaa3efb2 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2548,6 +2548,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>   */
>  #define HAS_AUX_IRQ(dev_priv)   true
>  #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
> +#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
> +					IS_GEMINILAKE(dev_priv) || \
> +					IS_KABYLAKE(dev_priv))

Spec says "Not available until KBLPCH-H A0, SPT-LP D1, SPT-H E1"
Not quite sure if those two statements are equivalenet or not.

>  
>  /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
>   * rows, which changed the alignment requirements and fence programming.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index be6114a0e8ab..8b4e6363c7d2 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2984,6 +2984,7 @@ enum i915_power_well_id {
>  #define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
>  #define   GMBUS_RATE_1MHZ	(3<<8) /* reserved on Pineview */
>  #define   GMBUS_HOLD_EXT	(1<<7) /* 300ns hold time, rsvd on Pineview */
> +#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
>  #define   GMBUS_PIN_DISABLED	0
>  #define   GMBUS_PIN_SSC		1
>  #define   GMBUS_PIN_VGADDC	2
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 4367827d7661..cb260f667cfa 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -373,10 +373,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>  		      unsigned short addr, u8 *buf, unsigned int len,
>  		      u32 gmbus1_index)
>  {
> +	unsigned int bytes_af_override = 0, size = len;

whatis af?

> +	bool burst_read = len > gmbus_max_xfer_size(dev_priv);
> +
> +	if (burst_read) {
> +		bytes_af_override = (len % 256) + 256;

The spec has a special case for 512 for some reason. Would be nice to
find out why it's there, and why there is no special case listed for
other larger multiples of 256.

No need for () around the %.

> +		size = bytes_af_override;

Why two variables for the same thing?

> +
> +		I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
> +			      GMBUS_BYTE_CNT_OVERRIDE));

useless parens

We could probably avoid the rmw by passing in the gmbus0
value from the caller.

> +	}
> +
>  	I915_WRITE_FW(GMBUS1,
>  		      gmbus1_index |
>  		      GMBUS_CYCLE_WAIT |
> -		      (len << GMBUS_BYTE_COUNT_SHIFT) |
> +		      (size << GMBUS_BYTE_COUNT_SHIFT) |
>  		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
>  	while (len) {
> @@ -392,6 +403,10 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>  			*buf++ = val & 0xff;
>  			val >>= 8;
>  		} while (--len && ++loop < 4);
> +
> +		if (burst_read && len == (bytes_af_override - 4))

more useless parens

> +			I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
> +				      ~GMBUS_BYTE_CNT_OVERRIDE));

and some more

>  	}
>  
>  	return 0;
> @@ -407,7 +422,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	int ret;
>  
>  	do {
> -		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
> +		if (HAS_GMBUS_BURST_READ(dev_priv))
> +			len = rx_size;
> +		else
> +			len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>  
>  		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>  					    buf, len, gmbus1_index);
> -- 
> 2.7.4

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

Re: [PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Ramalingam C]

On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote:
> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote:
>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
>> 511Bytes instead of previous 256Bytes used in SW.
>>
>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards.
>>
>> v2:
>>    No Change.
>> v3:
>>    Inline function for max_xfer_size and renaming of the macro.[Jani]
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>   drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 475cac07d3e6..be6114a0e8ab 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id {
>>   #define   GMBUS_CYCLE_STOP	(4<<25)
>>   #define   GMBUS_BYTE_COUNT_SHIFT 16
>>   #define   GMBUS_BYTE_COUNT_MAX   256U
>> +#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
>>   #define   GMBUS_SLAVE_INDEX_SHIFT 8
>>   #define   GMBUS_SLAVE_ADDR_SHIFT 1
>>   #define   GMBUS_SLAVE_READ	(1<<0)
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index e6875509bcd9..4367827d7661 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>   	return ret;
>>   }
>>   
>> +static inline
>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
>> +{
>> +	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
>> +		GMBUS_BYTE_COUNT_MAX;
> Hmm. You sure about this 256 limit on older HW? The spec does sort of
> say that 0-256 is the valid range, but the SPT+ docs still have that
> same text, and the register has always had 9 bits for byte count. I
> don't see any statements saying that they changed this in any way for
> SPT. It only talks about >511 bytes needing the special treatment.
>
> If we do this the I think you should just drop the defines and put the
> raw numbers into this function. The extra indirection just makes life
> harder. Also pointless parens around the GEN>9 check.
Even I couldn't get any place where BSpec says 256Bytes is the limit for 
any platform. Everywhere 9bits are used.
And when I cross verified with other OS usage 511Bytes is used as limit 
across all platforms.

Just to be cautious for not breaking any older platforms out in linux 
world, I limited the extension of the limit to the known
and easily testable platforms at my desk (Gen9+)

Do you suggest we should apply 511Bytes as max limit for all platforms?
Do we have any means to test this new limit on all supported legacy 
platforms?

Except enabling the full potential of the HW in SW, I dont see any ROI 
here as most of the GMBUS reqs are <256Bytes.
Only in case of HDCP2.2 we need single read cycle for 538Bytes.

we have couple of options here: Please share your opinion to choose one 
of them.
1. Just dont change the upper limit for RD/WR. Keep it as it is at 
256Bytes. Anyway no user demands it.
2. As per HW capability, Change the upper limit for RD/WR to 511Bytes 
for all platforms. This is needs the functional verification on all 
legacy plat supported.
3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say 
Gen9+.

--Ram
>
>> +}
>> +
>>   static int
>>   gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>   		      unsigned short addr, u8 *buf, unsigned int len,
>> @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>   	int ret;
>>   
>>   	do {
>> -		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
>> +		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>>   
>>   		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>>   					    buf, len, gmbus1_index);
>> @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>   	int ret;
>>   
>>   	do {
>> -		len = min(tx_size, GMBUS_BYTE_COUNT_MAX);
>> +		len = min(tx_size, gmbus_max_xfer_size(dev_priv));
>>   
>>   		ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len,
>>   					     gmbus1_index);
>> -- 
>> 2.7.4

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

Re: [PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Jani Nikula]

On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote:
> On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote:
>> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote:
>>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
>>> 511Bytes instead of previous 256Bytes used in SW.
>>>
>>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards.
>>>
>>> v2:
>>>    No Change.
>>> v3:
>>>    Inline function for max_xfer_size and renaming of the macro.[Jani]
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>>   drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
>>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 475cac07d3e6..be6114a0e8ab 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id {
>>>   #define   GMBUS_CYCLE_STOP	(4<<25)
>>>   #define   GMBUS_BYTE_COUNT_SHIFT 16
>>>   #define   GMBUS_BYTE_COUNT_MAX   256U
>>> +#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
>>>   #define   GMBUS_SLAVE_INDEX_SHIFT 8
>>>   #define   GMBUS_SLAVE_ADDR_SHIFT 1
>>>   #define   GMBUS_SLAVE_READ	(1<<0)
>>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>>> index e6875509bcd9..4367827d7661 100644
>>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>>   	return ret;
>>>   }
>>>   
>>> +static inline
>>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
>>> +{
>>> +	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
>>> +		GMBUS_BYTE_COUNT_MAX;
>> Hmm. You sure about this 256 limit on older HW? The spec does sort of
>> say that 0-256 is the valid range, but the SPT+ docs still have that
>> same text, and the register has always had 9 bits for byte count. I
>> don't see any statements saying that they changed this in any way for
>> SPT. It only talks about >511 bytes needing the special treatment.
>>
>> If we do this the I think you should just drop the defines and put the
>> raw numbers into this function. The extra indirection just makes life
>> harder. Also pointless parens around the GEN>9 check.
> Even I couldn't get any place where BSpec says 256Bytes is the limit for 
> any platform. Everywhere 9bits are used.
> And when I cross verified with other OS usage 511Bytes is used as limit 
> across all platforms.
>
> Just to be cautious for not breaking any older platforms out in linux 
> world, I limited the extension of the limit to the known
> and easily testable platforms at my desk (Gen9+)
>
> Do you suggest we should apply 511Bytes as max limit for all platforms?
> Do we have any means to test this new limit on all supported legacy 
> platforms?
>
> Except enabling the full potential of the HW in SW, I dont see any ROI 
> here as most of the GMBUS reqs are <256Bytes.
> Only in case of HDCP2.2 we need single read cycle for 538Bytes.
>
> we have couple of options here: Please share your opinion to choose one 
> of them.
> 1. Just dont change the upper limit for RD/WR. Keep it as it is at 
> 256Bytes. Anyway no user demands it.
> 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes 
> for all platforms. This is needs the functional verification on all 
> legacy plat supported.
> 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say 
> Gen9+.

Please let's not change the limit for old platforms for absolutely no
gain. And if Ville insists anyway, let's leave that as a separate
follow-up change that can easily be reverted later.

I might consider using the 511 limit only for platforms that
HAS_GMBUS_BURST_READ too.

The original limit seems to have been added in 9535c4757b88 ("drm/i915:
cope with large i2c transfers") citing "the specs". Any recollection
anyone? Chris?

BR,
Jani.


>
> --Ram
>>
>>> +}
>>> +
>>>   static int
>>>   gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>>   		      unsigned short addr, u8 *buf, unsigned int len,
>>> @@ -400,7 +407,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>>   	int ret;
>>>   
>>>   	do {
>>> -		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
>>> +		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>>>   
>>>   		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>>>   					    buf, len, gmbus1_index);
>>> @@ -462,7 +469,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>>   	int ret;
>>>   
>>>   	do {
>>> -		len = min(tx_size, GMBUS_BYTE_COUNT_MAX);
>>> +		len = min(tx_size, gmbus_max_xfer_size(dev_priv));
>>>   
>>>   		ret = gmbus_xfer_write_chunk(dev_priv, msg->addr, buf, len,
>>>   					     gmbus1_index);
>>> -- 
>>> 2.7.4
>

-- 
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 v3 2/2] drm/i915/gmbus: Enable burst read

[Ramalingam C]

[-- Attachment #1.1: Type: text/plain, Size: 4991 bytes --]

Thanks ville for reviewing this change.

On Wednesday 18 April 2018 12:12 AM, Ville Syrjälä wrote:
> On Tue, Apr 17, 2018 at 02:25:33PM +0530, Ramalingam C wrote:
>> Support for Burst read in HW is added for HDCP2.2 compliance
>> requirement.
>>
>> This patch enables the burst read for all the gmbus read of more than
>> 511Bytes, on capable platforms.
>>
>> v2:
>>    Extra line is removed.
>> v3:
>>    Macro is added for detecting the BURST_READ Support [Jani]
>>    Runtime detection of the need for burst_read [Jani]
>>
>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h  |  3 +++
>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>   drivers/gpu/drm/i915/intel_i2c.c | 22 ++++++++++++++++++++--
>>   3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 5373b171bb96..9cddcaa3efb2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2548,6 +2548,9 @@ intel_info(const struct drm_i915_private *dev_priv)
>>    */
>>   #define HAS_AUX_IRQ(dev_priv)   true
>>   #define HAS_GMBUS_IRQ(dev_priv) (INTEL_GEN(dev_priv) >= 4)
>> +#define HAS_GMBUS_BURST_READ(dev_priv) (INTEL_GEN(dev_priv) >= 10 || \
>> +					IS_GEMINILAKE(dev_priv) || \
>> +					IS_KABYLAKE(dev_priv))
> Spec says "Not available until KBLPCH-H A0, SPT-LP D1, SPT-H E1"
> Not quite sure if those two statements are equivalenet or not.
>
>>   
>>   /* With the 945 and later, Y tiling got adjusted so that it was 32 128-byte
>>    * rows, which changed the alignment requirements and fence programming.
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index be6114a0e8ab..8b4e6363c7d2 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -2984,6 +2984,7 @@ enum i915_power_well_id {
>>   #define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
>>   #define   GMBUS_RATE_1MHZ	(3<<8) /* reserved on Pineview */
>>   #define   GMBUS_HOLD_EXT	(1<<7) /* 300ns hold time, rsvd on Pineview */
>> +#define   GMBUS_BYTE_CNT_OVERRIDE (1<<6)
>>   #define   GMBUS_PIN_DISABLED	0
>>   #define   GMBUS_PIN_SSC		1
>>   #define   GMBUS_PIN_VGADDC	2
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index 4367827d7661..cb260f667cfa 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -373,10 +373,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>   		      unsigned short addr, u8 *buf, unsigned int len,
>>   		      u32 gmbus1_index)
>>   {
>> +	unsigned int bytes_af_override = 0, size = len;
> whatis af?
stands for bytes after override. Bytes that will be read after resetting 
the
Byte Count Override bit. we can use size for the purpose of 
bytes_af_override also.
Which is feed to HW total_byte_count fields.
>
>> +	bool burst_read = len > gmbus_max_xfer_size(dev_priv);
>> +
>> +	if (burst_read) {
>> +		bytes_af_override = (len % 256) + 256;
> The spec has a special case for 512 for some reason. Would be nice to
> find out why it's there, and why there is no special case listed for
> other larger multiples of 256.
Sent a mail for clarification from HW team. But I hope that could be 
iterative fix, on top of this. Need not block this!?
>
> No need for () around the %.
>
>> +		size = bytes_af_override;
> Why two variables for the same thing?
will fix it. My bad.
>
>> +
>> +		I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
>> +			      GMBUS_BYTE_CNT_OVERRIDE));
> useless parens
>
> We could probably avoid the rmw by passing in the gmbus0
> value from the caller.
Is that fine to add another parameter throughout 
do_gmbus_xfer-->gmbus_index_xfer-->
gmbus_xfer_read-->gmbus_xfer_read_chunk to avoid the reg read?

--Ram
>
>> +	}
>> +
>>   	I915_WRITE_FW(GMBUS1,
>>   		      gmbus1_index |
>>   		      GMBUS_CYCLE_WAIT |
>> -		      (len << GMBUS_BYTE_COUNT_SHIFT) |
>> +		      (size << GMBUS_BYTE_COUNT_SHIFT) |
>>   		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
>>   		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
>>   	while (len) {
>> @@ -392,6 +403,10 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>   			*buf++ = val & 0xff;
>>   			val >>= 8;
>>   		} while (--len && ++loop < 4);
>> +
>> +		if (burst_read && len == (bytes_af_override - 4))
> more useless parens
>
>> +			I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
>> +				      ~GMBUS_BYTE_CNT_OVERRIDE));
> and some more
>
>>   	}
>>   
>>   	return 0;
>> @@ -407,7 +422,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>   	int ret;
>>   
>>   	do {
>> -		len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>> +		if (HAS_GMBUS_BURST_READ(dev_priv))
>> +			len = rx_size;
>> +		else
>> +			len = min(rx_size, gmbus_max_xfer_size(dev_priv));
>>   
>>   		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>>   					    buf, len, gmbus1_index);
>> -- 
>> 2.7.4


[-- Attachment #1.2: Type: text/html, Size: 6591 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Ville Syrjälä]

On Wed, Apr 18, 2018 at 09:20:23AM +0300, Jani Nikula wrote:
> On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote:
> > On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote:
> >> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote:
> >>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
> >>> 511Bytes instead of previous 256Bytes used in SW.
> >>>
> >>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards.
> >>>
> >>> v2:
> >>>    No Change.
> >>> v3:
> >>>    Inline function for max_xfer_size and renaming of the macro.[Jani]
> >>>
> >>> Cc: Jani Nikula <jani.nikula@intel.com>
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> >>> ---
> >>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
> >>>   drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
> >>>   2 files changed, 10 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>> index 475cac07d3e6..be6114a0e8ab 100644
> >>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id {
> >>>   #define   GMBUS_CYCLE_STOP	(4<<25)
> >>>   #define   GMBUS_BYTE_COUNT_SHIFT 16
> >>>   #define   GMBUS_BYTE_COUNT_MAX   256U
> >>> +#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
> >>>   #define   GMBUS_SLAVE_INDEX_SHIFT 8
> >>>   #define   GMBUS_SLAVE_ADDR_SHIFT 1
> >>>   #define   GMBUS_SLAVE_READ	(1<<0)
> >>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> >>> index e6875509bcd9..4367827d7661 100644
> >>> --- a/drivers/gpu/drm/i915/intel_i2c.c
> >>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> >>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
> >>>   	return ret;
> >>>   }
> >>>   
> >>> +static inline
> >>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
> >>> +{
> >>> +	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
> >>> +		GMBUS_BYTE_COUNT_MAX;
> >> Hmm. You sure about this 256 limit on older HW? The spec does sort of
> >> say that 0-256 is the valid range, but the SPT+ docs still have that
> >> same text, and the register has always had 9 bits for byte count. I
> >> don't see any statements saying that they changed this in any way for
> >> SPT. It only talks about >511 bytes needing the special treatment.
> >>
> >> If we do this the I think you should just drop the defines and put the
> >> raw numbers into this function. The extra indirection just makes life
> >> harder. Also pointless parens around the GEN>9 check.
> > Even I couldn't get any place where BSpec says 256Bytes is the limit for 
> > any platform. Everywhere 9bits are used.
> > And when I cross verified with other OS usage 511Bytes is used as limit 
> > across all platforms.
> >
> > Just to be cautious for not breaking any older platforms out in linux 
> > world, I limited the extension of the limit to the known
> > and easily testable platforms at my desk (Gen9+)
> >
> > Do you suggest we should apply 511Bytes as max limit for all platforms?
> > Do we have any means to test this new limit on all supported legacy 
> > platforms?
> >
> > Except enabling the full potential of the HW in SW, I dont see any ROI 
> > here as most of the GMBUS reqs are <256Bytes.
> > Only in case of HDCP2.2 we need single read cycle for 538Bytes.
> >
> > we have couple of options here: Please share your opinion to choose one 
> > of them.
> > 1. Just dont change the upper limit for RD/WR. Keep it as it is at 
> > 256Bytes. Anyway no user demands it.
> > 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes 
> > for all platforms. This is needs the functional verification on all 
> > legacy plat supported.
> > 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say 
> > Gen9+.
> 
> Please let's not change the limit for old platforms for absolutely no
> gain. And if Ville insists anyway, let's leave that as a separate
> follow-up change that can easily be reverted later.
> 
> I might consider using the 511 limit only for platforms that
> HAS_GMBUS_BURST_READ too.
> 
> The original limit seems to have been added in 9535c4757b88 ("drm/i915:
> cope with large i2c transfers") citing "the specs". Any recollection
> anyone? Chris?

"Duble buffered data register and a 9 bit counter support 0 byte to 256
Byte transfers."
has always been in the spec (and still is for SPT+).

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

Re: [PATCH v3 1/2] drm/i915/gmbus: Increase the Bytes per Rd/Wr Op

[Ramalingam C]

On Wednesday 18 April 2018 08:47 PM, Ville Syrjälä wrote:
> On Wed, Apr 18, 2018 at 09:20:23AM +0300, Jani Nikula wrote:
>> On Wed, 18 Apr 2018, Ramalingam C <ramalingam.c@intel.com> wrote:
>>> On Tuesday 17 April 2018 11:39 PM, Ville Syrjälä wrote:
>>>> On Tue, Apr 17, 2018 at 02:25:32PM +0530, Ramalingam C wrote:
>>>>> >From Gen9 onwards Bspec says HW supports Max Bytes per single RD/WR op is
>>>>> 511Bytes instead of previous 256Bytes used in SW.
>>>>>
>>>>> This change allows the max bytes per op upto 511Bytes from Gen9 onwards.
>>>>>
>>>>> v2:
>>>>>     No Change.
>>>>> v3:
>>>>>     Inline function for max_xfer_size and renaming of the macro.[Jani]
>>>>>
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>>>>    drivers/gpu/drm/i915/intel_i2c.c | 11 +++++++++--
>>>>>    2 files changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>> index 475cac07d3e6..be6114a0e8ab 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>> @@ -3013,6 +3013,7 @@ enum i915_power_well_id {
>>>>>    #define   GMBUS_CYCLE_STOP	(4<<25)
>>>>>    #define   GMBUS_BYTE_COUNT_SHIFT 16
>>>>>    #define   GMBUS_BYTE_COUNT_MAX   256U
>>>>> +#define   GEN9_GMBUS_BYTE_COUNT_MAX 511U
>>>>>    #define   GMBUS_SLAVE_INDEX_SHIFT 8
>>>>>    #define   GMBUS_SLAVE_ADDR_SHIFT 1
>>>>>    #define   GMBUS_SLAVE_READ	(1<<0)
>>>>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>>>>> index e6875509bcd9..4367827d7661 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>>>>> @@ -361,6 +361,13 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>>>>    	return ret;
>>>>>    }
>>>>>    
>>>>> +static inline
>>>>> +unsigned int gmbus_max_xfer_size(struct drm_i915_private *dev_priv)
>>>>> +{
>>>>> +	return (INTEL_GEN(dev_priv) >= 9) ? GEN9_GMBUS_BYTE_COUNT_MAX :
>>>>> +		GMBUS_BYTE_COUNT_MAX;
>>>> Hmm. You sure about this 256 limit on older HW? The spec does sort of
>>>> say that 0-256 is the valid range, but the SPT+ docs still have that
>>>> same text, and the register has always had 9 bits for byte count. I
>>>> don't see any statements saying that they changed this in any way for
>>>> SPT. It only talks about >511 bytes needing the special treatment.
>>>>
>>>> If we do this the I think you should just drop the defines and put the
>>>> raw numbers into this function. The extra indirection just makes life
>>>> harder. Also pointless parens around the GEN>9 check.
>>> Even I couldn't get any place where BSpec says 256Bytes is the limit for
>>> any platform. Everywhere 9bits are used.
>>> And when I cross verified with other OS usage 511Bytes is used as limit
>>> across all platforms.
>>>
>>> Just to be cautious for not breaking any older platforms out in linux
>>> world, I limited the extension of the limit to the known
>>> and easily testable platforms at my desk (Gen9+)
>>>
>>> Do you suggest we should apply 511Bytes as max limit for all platforms?
>>> Do we have any means to test this new limit on all supported legacy
>>> platforms?
>>>
>>> Except enabling the full potential of the HW in SW, I dont see any ROI
>>> here as most of the GMBUS reqs are <256Bytes.
>>> Only in case of HDCP2.2 we need single read cycle for 538Bytes.
>>>
>>> we have couple of options here: Please share your opinion to choose one
>>> of them.
>>> 1. Just dont change the upper limit for RD/WR. Keep it as it is at
>>> 256Bytes. Anyway no user demands it.
>>> 2. As per HW capability, Change the upper limit for RD/WR to 511Bytes
>>> for all platforms. This is needs the functional verification on all
>>> legacy plat supported.
>>> 3. Change the upper limit for RD/WR to 511Bytes for newer platforms, say
>>> Gen9+.
>> Please let's not change the limit for old platforms for absolutely no
>> gain. And if Ville insists anyway, let's leave that as a separate
>> follow-up change that can easily be reverted later.
>>
>> I might consider using the 511 limit only for platforms that
>> HAS_GMBUS_BURST_READ too.
>>
>> The original limit seems to have been added in 9535c4757b88 ("drm/i915:
>> cope with large i2c transfers") citing "the specs". Any recollection
>> anyone? Chris?
> "Duble buffered data register and a 9 bit counter support 0 byte to 256
> Byte transfers."
> has always been in the spec (and still is for SPT+).
Seems like gap in Bspec. Just today HW team has clarified that all 
platforms support 511Bytes for single Gmbus read and write cycle.

--Ram
>

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