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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 03 Apr 2018, Daniel Vetter <daniel@ffwll.ch> 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 <ramalingam.c@intel.com>
>
> 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_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));
>> +		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)
>>  {
>>  	u8 *buf = msg->buf;
>>  	unsigned int rx_size = msg->len;
>> @@ -400,10 +426,13 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>  	int ret;
>>  
>>  	do {
>> -		len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
>> +		if (burst_read)
>> +			len = rx_size;
>> +		else
>> +			len = min(rx_size, GMBUS_BYTE_COUNT_MAX);
>>  
>> -		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
>> -					    buf, len, gmbus1_index);
>> +		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr, buf, len,
>> +					    gmbus1_index, burst_read);
>>  		if (ret)
>>  			return ret;
>>  
>> @@ -491,7 +520,8 @@ gmbus_is_index_xfer(struct i2c_msg *msgs, int i, int num)
>>  }
>>  
>>  static int
>> -gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>> +gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs,
>> +		 bool burst_read)
>>  {
>>  	u32 gmbus1_index = 0;
>>  	u32 gmbus5 = 0;
>> @@ -509,7 +539,8 @@ gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>>  		I915_WRITE_FW(GMBUS5, gmbus5);
>>  
>>  	if (msgs[1].flags & I2C_M_RD)
>> -		ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
>> +		ret = gmbus_xfer_read(dev_priv, &msgs[1],
>> +				      gmbus1_index, burst_read);
>>  	else
>>  		ret = gmbus_xfer_write(dev_priv, &msgs[1], gmbus1_index);
>>  
>> @@ -522,7 +553,7 @@ gmbus_index_xfer(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>>  
>>  static int
>>  do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num,
>> -	      u32 gmbus0_source)
>> +	      u32 gmbus0_source, bool burst_read)
>>  {
>>  	struct intel_gmbus *bus = container_of(adapter,
>>  					       struct intel_gmbus,
>> @@ -544,15 +575,20 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num,
>>  	for (; i < num; i += inc) {
>>  		inc = 1;
>>  		if (gmbus_is_index_xfer(msgs, i, num)) {
>> -			ret = gmbus_index_xfer(dev_priv, &msgs[i]);
>> +			ret = gmbus_index_xfer(dev_priv, &msgs[i], burst_read);
>>  			inc = 2; /* an index transmission is two msgs */
>>  		} else if (msgs[i].flags & I2C_M_RD) {
>> -			ret = gmbus_xfer_read(dev_priv, &msgs[i], 0);
>> +			ret = gmbus_xfer_read(dev_priv, &msgs[i],
>> +					      0, burst_read);
>>  		} else {
>>  			ret = gmbus_xfer_write(dev_priv, &msgs[i], 0);
>>  		}
>>  
>> -		if (!ret)
>> +		/*
>> +		 * Burst read Sequence ends with STOP. So Dont expect
>> +		 * HW wait phase.
>> +		 */
>> +		if (!ret && !burst_read)
>>  			ret = gmbus_wait(dev_priv,
>>  					 GMBUS_HW_WAIT_PHASE, GMBUS_HW_WAIT_EN);
>>  		if (ret == -ETIMEDOUT)
>> @@ -664,7 +700,7 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>>  		if (ret < 0)
>>  			bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
>>  	} else {
>> -		ret = do_gmbus_xfer(adapter, msgs, num, 0);
>> +		ret = do_gmbus_xfer(adapter, msgs, num, 0, false);
>>  		if (ret == -EAGAIN)
>>  			bus->force_bit |= GMBUS_FORCE_BIT_RETRY;
>>  	}
>> @@ -705,7 +741,8 @@ int intel_gmbus_output_aksv(struct i2c_adapter *adapter)
>>  	 * pass the i2c command, and tell GMBUS to use the HW-provided value
>>  	 * instead of sourcing GMBUS3 for the data.
>>  	 */
>> -	ret = do_gmbus_xfer(adapter, msgs, ARRAY_SIZE(msgs), GMBUS_AKSV_SELECT);
>> +	ret = do_gmbus_xfer(adapter, msgs, ARRAY_SIZE(msgs),
>> +			    GMBUS_AKSV_SELECT, false);
>>  
>>  	mutex_unlock(&dev_priv->gmbus_mutex);
>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>> @@ -713,6 +750,59 @@ int intel_gmbus_output_aksv(struct i2c_adapter *adapter)
>>  	return ret;
>>  }
>>  
>> +static inline
>> +bool intel_gmbus_burst_read_supported(struct drm_i915_private *dev_priv)
>> +{
>> +	if (INTEL_GEN(dev_priv) > 10 || IS_GEMINILAKE(dev_priv) ||
>> +	    IS_KABYLAKE(dev_priv))
>> +		return true;
>> +	return false;
>> +}
>> +
>> +int intel_gmbus_burst_read(struct i2c_adapter *adapter, unsigned int offset,
>> +			   void *buf, size_t size)
>> +{
>> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
>> +					       adapter);
>> +	struct drm_i915_private *dev_priv = bus->dev_priv;
>> +	int ret;
>> +	u8 start = offset & 0xff;
>> +	struct i2c_msg msgs[] = {
>> +		{
>> +			.addr = DRM_HDCP_DDC_ADDR,
>> +			.flags = 0,
>> +			.len = 1,
>> +			.buf = &start,
>> +		},
>> +		{
>> +			.addr = DRM_HDCP_DDC_ADDR,
>> +			.flags = I2C_M_RD,
>> +			.len = size,
>> +			.buf = buf,
>> +		}
>> +	};
>> +
>> +	if (!intel_gmbus_burst_read_supported(dev_priv))
>> +		return -EINVAL;
>> +
>> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>> +	mutex_lock(&dev_priv->gmbus_mutex);
>> +
>> +	/*
>> +	 * In order to read the complete length(More than GMBus Limit) of data,
>> +	 * in burst mode, implement the Workaround supported in HW.
>> +	 */
>> +	ret = do_gmbus_xfer(adapter, msgs, ARRAY_SIZE(msgs), 0, true);
>> +
>> +	mutex_unlock(&dev_priv->gmbus_mutex);
>> +	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>> +
>> +	if (ret == ARRAY_SIZE(msgs))
>> +		return 0;
>> +
>> +	return ret >= 0 ? -EIO : ret;
>> +}
>> +
>>  static u32 gmbus_func(struct i2c_adapter *adapter)
>>  {
>>  	return i2c_bit_algo.functionality(adapter) &
>> -- 
>> 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