Re: [RFC v2 2/2] drm/i915/gmbus: Enable burst read

From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ramalingam C <ramalingam.c@intel.com>,
	intel-gfx@lists.freedesktop.org, daniel@ffwll.ch,
	chris@chris-wilson.co.uk, rodrigo.vivi@intel.com,
	ville.syrjala@linux.intel.com
Subject: Re: [RFC v2 2/2] drm/i915/gmbus: Enable burst read
Date: Fri, 13 Apr 2018 15:55:56 +0300	[thread overview]
Message-ID: <871sfjfeoz.fsf@intel.com> (raw)
In-Reply-To: <1523548876-22449-1-git-send-email-ramalingam.c@intel.com>

On Thu, 12 Apr 2018, Ramalingam C <ramalingam.c@intel.com> 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.
>
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_i2c.c | 38 ++++++++++++++++++++++++++++++++++----
>  2 files changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4f583da0cee9..0ef162ee9ce0 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 7e92c7934657..2ee9cf2effcd 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -364,12 +364,24 @@ 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)

I think you could throw out the burst_read parameter. See below.

>  {
> +	unsigned int bytes_af_override, size;
> +
> +	if (burst_read) {
> +		bytes_af_override = len - (((len / 256) - 1) * 256);
> +		size = bytes_af_override;
> +
> +		I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) |
> +			      GMBUS_BYTE_CNT_OVERRIDE));
> +	} else {
> +		size = len;
> +	}
> +
>  	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) {
> @@ -385,11 +397,23 @@ 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))
> +			I915_WRITE_FW(GMBUS0, (I915_READ_FW(GMBUS0) &
> +				      ~GMBUS_BYTE_CNT_OVERRIDE));
>  	}
>  
>  	return 0;
>  }
>  
> +static bool 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;
> +}

Name this HAS_GMBUS_BURST_READ() and put below HAS_GMBUS_IRQ() in
i915_drv.h.

> +
>  static int
>  gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  		u32 gmbus1_index)
> @@ -398,15 +422,21 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	unsigned int rx_size = msg->len;
>  	unsigned int len;
>  	int ret;
> +	bool burst_read = false;
> +
> +	if (rx_size > BXT_GMBUS_BYTE_COUNT_MAX)
> +		burst_read = gmbus_burst_read_supported(dev_priv);
>  
>  	do {
> -		if (INTEL_GEN(dev_priv) >= 9)
> +		if (burst_read)
> +			len = rx_size;
> +		else if (INTEL_GEN(dev_priv) >= 9)
>  			len = min(rx_size, BXT_GMBUS_BYTE_COUNT_MAX);
>  		else
>  			len = min(rx_size, GMBUS_BYTE_COUNT_MAX);

If you abstracted the max transfer length in the previous patch, I think
you could condence this into something like:

		if (HAS_GMBUS_BURST_READ(dev_priv))
                	len = rx_size;
                else
                	len = min(rx_size, gmbus_max_xfer_size(dev_priv))

You could throw away the burst_read variable and parameter altogether,
and have gmbus_xfer_read_chunk() decide internally if burst read is to
be used by:

	burst_read = len > gmbus_max_xfer_size(dev_priv);

(Potentially with a WARN_ON(burst_read && !HAS_GMBUS_BURST_READ()) if
you want to be paranoid.)

BR,
Jani.

>  
>  		ret = gmbus_xfer_read_chunk(dev_priv, msg->addr,
> -					    buf, len, gmbus1_index);
> +					    buf, len, gmbus1_index, burst_read);
>  		if (ret)
>  			return ret;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx