diff mbox

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

Message ID 1523955333-8587-3-git-send-email-ramalingam.c@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ramalingam C April 17, 2018, 8:55 a.m. UTC
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(-)

Comments

Ville Syrjälä April 17, 2018, 6:42 p.m. UTC | #1
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
Ramalingam C April 18, 2018, 11:18 a.m. UTC | #2
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
diff mbox

Patch

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);