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

[Ramalingam C <ramalingam.c@intel.com>]

[-- 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