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

[Ramalingam C <ramalingam.c@xxxxxxxxx>]

On Thursday 05 April 2018 02:42 PM, Jani Nikula wrote:
> On Tue, 03 Apr 2018, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxx>
> > 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

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel