[Intel-gfx] [PATCH v3 36/40] drm/i915: Implement gmbus burst read
Ramalingam C
ramalingam.c at intel.com
Thu Apr 5 13:44:22 UTC 2018
On Thursday 05 April 2018 02:42 PM, Jani Nikula wrote:
> On Tue, 03 Apr 2018, Daniel Vetter <daniel at 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?
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 <ramalingam.c at 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?
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_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
>>>
More information about the Intel-gfx
mailing list