Re: [PATCH v2] drm/bridge: dw-hdmi: Refuse DDC/CI transfers on the internal I2C controller

From: Matthias Kaehlcke <mka@chromium.org>
To: Sean Paul <sean@poorly.run>
Cc: Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Jose Abreu <Jose.Abreu@synopsys.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Douglas Anderson <dianders@chromium.org>,
	Adam Jackson <ajax@redhat.com>
Subject: Re: [PATCH v2] drm/bridge: dw-hdmi: Refuse DDC/CI transfers on the internal I2C controller
Date: Mon, 22 Jul 2019 14:02:07 -0700	[thread overview]
Message-ID: <20190722210207.GZ250418@google.com> (raw)
In-Reply-To: <20190722202426.GL104440@art_vandelay>

On Mon, Jul 22, 2019 at 04:24:26PM -0400, Sean Paul wrote:
> On Mon, Jul 22, 2019 at 11:19:45AM -0700, Matthias Kaehlcke wrote:
> > The DDC/CI protocol involves sending a multi-byte request to the
> > display via I2C, which is typically followed by a multi-byte
> > response. The internal I2C controller only allows single byte
> > reads/writes or reads of 8 sequential bytes, hence DDC/CI is not
> > supported when the internal I2C controller is used. The I2C
> 
> This is very likely a stupid question, but I didn't see an answer for it, so
> I'll just ask :)
> 
> If the controller supports xfers of 8 bytes and 1 bytes, could you just split
> up any of these transactions into len/8+len%8 transactions?

The controller interprets all transfers to be register accesses. It is
not possible to just send the sequence '0x0a 0x0b 0x0c' as three byte
transfers, the controller expects an address for each byte and
(supposedly) sends it over the wire, which typically isn't what you
want.

Also the 8-byte reads only seem to be supported in certain
configurations ("when the DWC_HDMI_TX_20 parameter is enabled").

> > transfers complete without errors, however the data in the response
> > is garbage. Abort transfers to/from slave address 0x37 (DDC) with
> > -EOPNOTSUPP, to make it evident that the communication is failing.
> > 
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > Changes in v2:
> > - changed DDC_I2C_ADDR to DDC_CI_ADDR
> > ---
> >  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > index 045b1b13fd0e..28933629f3c7 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> > @@ -35,6 +35,7 @@
> >  
> >  #include <media/cec-notifier.h>
> >  
> > +#define DDC_CI_ADDR		0x37
> >  #define DDC_SEGMENT_ADDR	0x30
> >  
> >  #define HDMI_EDID_LEN		512
> > @@ -322,6 +323,13 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
> >  	u8 addr = msgs[0].addr;
> >  	int i, ret = 0;
> >  
> > +	if (addr == DDC_CI_ADDR)
> > +		/*
> > +		 * The internal I2C controller does not support the multi-byte
> > +		 * read and write operations needed for DDC/CI.
> > +		 */
> > +		return -EOPNOTSUPP;
> > +
> >  	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
> >  
> >  	for (i = 0; i < num; i++) {
> 