[Intel-gfx] [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 at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Ramalingam C <ramalingam.c at 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