Re: [PATCH v3 36/40] drm/i915: Implement gmbus burst read

2018-04-05 Thread Ramalingam C 



On Thursday 05 April 2018 02:42 PM, Jani Nikula wrote:

On Tue, 03 Apr 2018, Daniel Vetter  wrote:

On Tue, Apr 03, 2018 at 07:27:49PM +0530, Ramalingam C wrote:

Implements a interface for single burst read of data that is larger
than 512 Bytes through gmbus.

Where does 512 come from? Current code chunks at GMBUS_BYTE_COUNT_MAX
i.e. 256 bytes. Should GMBUS_BYTE_COUNT_MAX be platform specific?
Actually I am not sure why 256 is the max limit for gmbus read/write 
when HW allows upto 511Bytes. 24:16 9bits of GMBUS is Total byte count. 
So max value could be 511Bytes.
256 is choosen because of some practical reason or any legacy platform 
capability?



HDCP2.2 spec expects HDCP2.2 transmitter to read 522Bytes of HDCP
receiver certificates in single burst read. On gmbus, to read more
than 511Bytes, HW provides a workaround for burst read.

This patch passes the burst read request through gmbus read functions.
And implements the sequence of enabling and disabling the burst read.

v2:
   No Changes.
v3:
   No Changes.

Signed-off-by: Ramalingam C 

Why only enable this burst_read mode for hdcp, and not for all i2c
transactions? Seems to unecessarily complicate the code, since it requires
that you pass burst_read through the entire call chain. For other changes
we've done for hdcp (like enabling the read/write mode and other stuff)
we've enabled it for all i2c transactions. That also means more testing,
since it will be used even when HDCP is not in use.

Agreed.

Shouldn't the decision to use burst read be based on the length to be
read? More than 256 bytes, use burst, otherwise not. Or are there
functional differences or benefits to using burst mode for shorter
reads?
Busrt read is enabled on few latest platforms (from KBL+ for HDCP2.2 
compliance requirement,

where split I2C read of a msg(534Bytes) is not accepted)

If there is no objection, extend the single normal rd/wr to 511Bytes 
from 256bytes.
And any read for more than 511 Bytes use the Burst read on capable 
platforms.


Though personally I dont believe it will give much of optimization, as 
burst read also brings in some
extra programming requirements. But might avoid extra driver entry point 
for GMBUS like intel_gmbus_burst_read().




I guess I'd still prepare to add some burst field to struct intel_gmbus,
not unlike force_bit.

Side note, I would prefer significant changes to basic plumbing like
gmbus be highlighted better. Please at least prefix the hdcp patches
with "drm/i915/hdcp" and gmbus with "drm/i915/gmbus". The patch at hand
is kind of hidden in the middle of a large topical series, and would
deserve to be looked at by people who aren't necessarily all that into
hdcp.

Sure.

--Ram



BR,
Jani.



-Daniel


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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6e740f6fe33f..72534a1e544b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3688,6 +3688,8 @@ extern void intel_teardown_gmbus(struct drm_i915_private 
*dev_priv);
  extern bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
 unsigned int pin);
  extern int intel_gmbus_output_aksv(struct i2c_adapter *adapter);
+extern int intel_gmbus_burst_read(struct i2c_adapter *adapter,
+ unsigned int offset, void *buf, size_t size);
  
  extern struct i2c_adapter *

  intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, unsigned int pin);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f04ad3c15abd..56979bc4e9d8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3123,6 +3123,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_VGADDC2
@@ -3150,8 +3151,10 @@ enum i915_power_well_id {
  #define   GMBUS_CYCLE_WAIT(1<<25)
  #define   GMBUS_CYCLE_INDEX   (2<<25)
  #define   GMBUS_CYCLE_STOP(4<<25)
+#define   GMBUS_CYCLE_MASK (7<<25)
  #define   GMBUS_BYTE_COUNT_SHIFT 16
  #define   GMBUS_BYTE_COUNT_MAX   256U
+#define   GMBUS_BYTE_COUNT_HW_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..dcb2be0d54ee 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -364,21

Re: [PATCH v3 36/40] drm/i915: Implement gmbus burst read

2018-04-05 Thread Jani Nikula 
On Tue, 03 Apr 2018, Daniel Vetter  wrote:
> On Tue, Apr 03, 2018 at 07:27:49PM +0530, Ramalingam C wrote:
>> Implements a interface for single burst read of data that is larger
>> than 512 Bytes through gmbus.

Where does 512 come from? Current code chunks at GMBUS_BYTE_COUNT_MAX
i.e. 256 bytes. Should GMBUS_BYTE_COUNT_MAX be platform specific?

>> 
>> HDCP2.2 spec expects HDCP2.2 transmitter to read 522Bytes of HDCP
>> receiver certificates in single burst read. On gmbus, to read more
>> than 511Bytes, HW provides a workaround for burst read.
>> 
>> This patch passes the burst read request through gmbus read functions.
>> And implements the sequence of enabling and disabling the burst read.
>> 
>> v2:
>>   No Changes.
>> v3:
>>   No Changes.
>> 
>> Signed-off-by: Ramalingam C 
>
> Why only enable this burst_read mode for hdcp, and not for all i2c
> transactions? Seems to unecessarily complicate the code, since it requires
> that you pass burst_read through the entire call chain. For other changes
> we've done for hdcp (like enabling the read/write mode and other stuff)
> we've enabled it for all i2c transactions. That also means more testing,
> since it will be used even when HDCP is not in use.

Agreed.

Shouldn't the decision to use burst read be based on the length to be
read? More than 256 bytes, use burst, otherwise not. Or are there
functional differences or benefits to using burst mode for shorter
reads?

I guess I'd still prepare to add some burst field to struct intel_gmbus,
not unlike force_bit.

Side note, I would prefer significant changes to basic plumbing like
gmbus be highlighted better. Please at least prefix the hdcp patches
with "drm/i915/hdcp" and gmbus with "drm/i915/gmbus". The patch at hand
is kind of hidden in the middle of a large topical series, and would
deserve to be looked at by people who aren't necessarily all that into
hdcp.


BR,
Jani.


> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |   2 +
>>  drivers/gpu/drm/i915/i915_reg.h  |   3 +
>>  drivers/gpu/drm/i915/intel_i2c.c | 124 
>> +--
>>  3 files changed, 112 insertions(+), 17 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 6e740f6fe33f..72534a1e544b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3688,6 +3688,8 @@ extern void intel_teardown_gmbus(struct 
>> drm_i915_private *dev_priv);
>>  extern bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
>>   unsigned int pin);
>>  extern int intel_gmbus_output_aksv(struct i2c_adapter *adapter);
>> +extern int intel_gmbus_burst_read(struct i2c_adapter *adapter,
>> +  unsigned int offset, void *buf, size_t size);
>>  
>>  extern struct i2c_adapter *
>>  intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, unsigned int 
>> pin);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index f04ad3c15abd..56979bc4e9d8 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3123,6 +3123,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_DISABLED0
>>  #define   GMBUS_PIN_SSC 1
>>  #define   GMBUS_PIN_VGADDC  2
>> @@ -3150,8 +3151,10 @@ enum i915_power_well_id {
>>  #define   GMBUS_CYCLE_WAIT  (1<<25)
>>  #define   GMBUS_CYCLE_INDEX (2<<25)
>>  #define   GMBUS_CYCLE_STOP  (4<<25)
>> +#define   GMBUS_CYCLE_MASK  (7<<25)
>>  #define   GMBUS_BYTE_COUNT_SHIFT 16
>>  #define   GMBUS_BYTE_COUNT_MAX   256U
>> +#define   GMBUS_BYTE_COUNT_HW_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..dcb2be0d54ee 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -364,21 +364,30 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>>  static int
>>  gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>>unsigned short addr, u8 *buf, unsigned int len,
>> -  u32 gmbus1_index)
>> +  u32 gmbus1_index, bool burst_read)
>>  {
>> +unsigned int size = len;
>> +int ret;
>> +
>> +if (burst_read) {
>> +/* Seq to enable Burst Read */
>> +I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
>> +  GMBUS_BYTE_CNT_OVERRIDE));
>> +size = GMBUS_BYTE_COUNT_HW_MAX;
>> +}
>> +
>>  I915_WRITE_FW(GMBUS1,
>>gmbus1_index |
>>

Re: [PATCH v3 36/40] drm/i915: Implement gmbus burst read

2018-04-03 Thread Daniel Vetter 
On Tue, Apr 03, 2018 at 07:27:49PM +0530, Ramalingam C wrote:
> Implements a interface for single burst read of data that is larger
> than 512 Bytes through gmbus.
> 
> HDCP2.2 spec expects HDCP2.2 transmitter to read 522Bytes of HDCP
> receiver certificates in single burst read. On gmbus, to read more
> than 511Bytes, HW provides a workaround for burst read.
> 
> This patch passes the burst read request through gmbus read functions.
> And implements the sequence of enabling and disabling the burst read.
> 
> v2:
>   No Changes.
> v3:
>   No Changes.
> 
> Signed-off-by: Ramalingam C 

Why only enable this burst_read mode for hdcp, and not for all i2c
transactions? Seems to unecessarily complicate the code, since it requires
that you pass burst_read through the entire call chain. For other changes
we've done for hdcp (like enabling the read/write mode and other stuff)
we've enabled it for all i2c transactions. That also means more testing,
since it will be used even when HDCP is not in use.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   2 +
>  drivers/gpu/drm/i915/i915_reg.h  |   3 +
>  drivers/gpu/drm/i915/intel_i2c.c | 124 
> +--
>  3 files changed, 112 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6e740f6fe33f..72534a1e544b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3688,6 +3688,8 @@ extern void intel_teardown_gmbus(struct 
> drm_i915_private *dev_priv);
>  extern bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
>unsigned int pin);
>  extern int intel_gmbus_output_aksv(struct i2c_adapter *adapter);
> +extern int intel_gmbus_burst_read(struct i2c_adapter *adapter,
> +   unsigned int offset, void *buf, size_t size);
>  
>  extern struct i2c_adapter *
>  intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, unsigned int pin);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f04ad3c15abd..56979bc4e9d8 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3123,6 +3123,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
> @@ -3150,8 +3151,10 @@ enum i915_power_well_id {
>  #define   GMBUS_CYCLE_WAIT   (1<<25)
>  #define   GMBUS_CYCLE_INDEX  (2<<25)
>  #define   GMBUS_CYCLE_STOP   (4<<25)
> +#define   GMBUS_CYCLE_MASK   (7<<25)
>  #define   GMBUS_BYTE_COUNT_SHIFT 16
>  #define   GMBUS_BYTE_COUNT_MAX   256U
> +#define   GMBUS_BYTE_COUNT_HW_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..dcb2be0d54ee 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -364,21 +364,30 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
>  static int
>  gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
> unsigned short addr, u8 *buf, unsigned int len,
> -   u32 gmbus1_index)
> +   u32 gmbus1_index, bool burst_read)
>  {
> + unsigned int size = len;
> + int ret;
> +
> + if (burst_read) {
> + /* Seq to enable Burst Read */
> + I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
> +   GMBUS_BYTE_CNT_OVERRIDE));
> + size = GMBUS_BYTE_COUNT_HW_MAX;
> + }
> +
>   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) {
> - int ret;
>   u32 val, loop = 0;
>  
>   ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
>   if (ret)
> - return ret;
> + goto exit;
>  
>   val = I915_READ_FW(GMBUS3);
>   do {
> @@ -387,12 +396,29 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>   } while (--len && ++loop < 4);
>   }
>  
> - return 0;
> +exit:
> + if (burst_read) {
> +
> + /* Seq to disable the Burst Read */
> + I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
> +   ~GMBUS_BYTE_CNT_OVERRIDE));
> +

[PATCH v3 36/40] drm/i915: Implement gmbus burst read

2018-04-03 Thread Ramalingam C 
Implements a interface for single burst read of data that is larger
than 512 Bytes through gmbus.

HDCP2.2 spec expects HDCP2.2 transmitter to read 522Bytes of HDCP
receiver certificates in single burst read. On gmbus, to read more
than 511Bytes, HW provides a workaround for burst read.

This patch passes the burst read request through gmbus read functions.
And implements the sequence of enabling and disabling the burst read.

v2:
  No Changes.
v3:
  No Changes.

Signed-off-by: Ramalingam C 
---
 drivers/gpu/drm/i915/i915_drv.h  |   2 +
 drivers/gpu/drm/i915/i915_reg.h  |   3 +
 drivers/gpu/drm/i915/intel_i2c.c | 124 +--
 3 files changed, 112 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6e740f6fe33f..72534a1e544b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3688,6 +3688,8 @@ extern void intel_teardown_gmbus(struct drm_i915_private 
*dev_priv);
 extern bool intel_gmbus_is_valid_pin(struct drm_i915_private *dev_priv,
 unsigned int pin);
 extern int intel_gmbus_output_aksv(struct i2c_adapter *adapter);
+extern int intel_gmbus_burst_read(struct i2c_adapter *adapter,
+ unsigned int offset, void *buf, size_t size);
 
 extern struct i2c_adapter *
 intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, unsigned int pin);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f04ad3c15abd..56979bc4e9d8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3123,6 +3123,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_SSC1
 #define   GMBUS_PIN_VGADDC 2
@@ -3150,8 +3151,10 @@ enum i915_power_well_id {
 #define   GMBUS_CYCLE_WAIT (1<<25)
 #define   GMBUS_CYCLE_INDEX(2<<25)
 #define   GMBUS_CYCLE_STOP (4<<25)
+#define   GMBUS_CYCLE_MASK (7<<25)
 #define   GMBUS_BYTE_COUNT_SHIFT 16
 #define   GMBUS_BYTE_COUNT_MAX   256U
+#define   GMBUS_BYTE_COUNT_HW_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..dcb2be0d54ee 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -364,21 +364,30 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
 static int
 gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
  unsigned short addr, u8 *buf, unsigned int len,
- u32 gmbus1_index)
+ u32 gmbus1_index, bool burst_read)
 {
+   unsigned int size = len;
+   int ret;
+
+   if (burst_read) {
+   /* Seq to enable Burst Read */
+   I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
+ GMBUS_BYTE_CNT_OVERRIDE));
+   size = GMBUS_BYTE_COUNT_HW_MAX;
+   }
+
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) {
-   int ret;
u32 val, loop = 0;
 
ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
if (ret)
-   return ret;
+   goto exit;
 
val = I915_READ_FW(GMBUS3);
do {
@@ -387,12 +396,29 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
} while (--len && ++loop < 4);
}
 
-   return 0;
+exit:
+   if (burst_read) {
+
+   /* Seq to disable the Burst Read */
+   I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
+ ~GMBUS_BYTE_CNT_OVERRIDE));
+   I915_WRITE_FW(GMBUS1, (I915_READ_FW(GMBUS1) &
+ ~GMBUS_CYCLE_MASK) | GMBUS_CYCLE_STOP);
+
+   /*
+* On Burst read disable, GMBUS need more time to settle
+* down to Idle State.
+*/
+   ret = intel_wait_for_register_fw(dev_priv, GMBUS2,
+GMBUS_ACTIVE, 0, 50);
+   }
+
+   return ret;
 }
 
 static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-   u32 gmbus1_index)
+   u32 gmbus1_index, bool burst_read)
 {