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

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

[Matthias Kaehlcke]

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
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++) {
-- 
2.22.0.657.g960e92d24f-goog

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

[Doug Anderson]

Hi,

On Mon, Jul 22, 2019 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> 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
> 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;
> +

In theory we could also solve this by detecting (in other parts of the
function) the bad multi-byte read/write operations.  That would maybe
be more generic (AKA it would more properly handle random userspace
tools fiddling with i2c-dev) but add complexity to the code.

...possibly a better long-term solution is to just totally stop
emulating a regular i2c adapter when the hardware just doesn't support
that.  In theory we could remove the "drm_get_edid()" call in
dw_hdmi_connector_get_modes() and replace it with a direct call to
drm_do_get_edid() if we're using the built-in adapter.  Possibly
"tda998x_drv.c" would be a good reference.  If we did that, we could
remove all the weird / hacky i2c code in this driver.


Since the bigger cleanup seems like a bit much to ask, I'm OK with
this as a stopgap to at least prevent userspace tools from getting
confused.  Thus:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

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

[Matthias Kaehlcke]

On Mon, Jul 22, 2019 at 01:12:40PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jul 22, 2019 at 11:19 AM Matthias Kaehlcke <mka@chromium.org> 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
> > 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;
> > +
> 
> In theory we could also solve this by detecting (in other parts of the
> function) the bad multi-byte read/write operations.  That would maybe
> be more generic (AKA it would more properly handle random userspace
> tools fiddling with i2c-dev) but add complexity to the code.

But how do you know it's an unsupported operation, and not the driver
knowing the wacky limitations doing something valid.

E.g. you get the sequence:

0x01 0x0a 0x0b 0x0c 0x0d

This could be interpreted as "send the above bytes to the slave" or
as "send 0x0a to address 0x01, 0x0b to 0x02, 0x0c to 0x03 and 0x0d to
0x04" (at least by this driver ;-) . The latter could be intended.

> ...possibly a better long-term solution is to just totally stop
> emulating a regular i2c adapter when the hardware just doesn't support
> that.  In theory we could remove the "drm_get_edid()" call in
> dw_hdmi_connector_get_modes() and replace it with a direct call to
> drm_do_get_edid() if we're using the built-in adapter.  Possibly
> "tda998x_drv.c" would be a good reference.  If we did that, we could
> remove all the weird / hacky i2c code in this driver.

yes, that would be another and probably better option than trying to
detect unsupported transaction.

> Since the bigger cleanup seems like a bit much to ask, I'm OK with
> this as a stopgap to at least prevent userspace tools from getting
> confused.  Thus:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!

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

[Sean Paul]

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?

Sean

> 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++) {
> -- 
> 2.22.0.657.g960e92d24f-goog
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Doug Anderson]

Hi,

On Mon, Jul 22, 2019 at 1:24 PM Sean Paul <sean@poorly.run> 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?

It's not quite that easy, I think.  Specifically a 1-byte transfer
isn't really a 1-byte transfer.

It always sticks this on the wire for a 1-byte write:

Start
Slave address (7 bits) + write (1 bit)
(wait ack)
Register address
1 byte of data
wait for ack
Stop

...or for a 1-byte read:

Start
Slave address (7 bits) + write (1 bit)
(wait ack)
Register address
(wait ack)
Repeated Start (1 bit)
Slave address (7 bits) + read (1 bit)
(read 1 byte of data)
Ack
Stop

Putting more than one of those in a row is not the same thing as just
doing a whole bunch of reads or a whole bunch of writes with no "stop"
in between.

As far as I could find out about DDC/CI it's part of the spec to _not_
send the stop between the reads / writes.


-Doug

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

[Matthias Kaehlcke]

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++) {
> 

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

[Sean Paul]

On Mon, Jul 22, 2019 at 02:02:07PM -0700, Matthias Kaehlcke wrote:
> 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").

Thanks for the detailed answers (both you and Doug)!

This change looks good to me, but I'll leave it to a dw-hdmi expert to apply. So
fwiw,

Reviewed-by: Sean Paul <sean@poorly.run>


> 
> > > 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++) {
> > 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Neil Armstrong]

Hi,

On 25/07/2019 19:49, Sean Paul wrote:
> On Mon, Jul 22, 2019 at 02:02:07PM -0700, Matthias Kaehlcke wrote:
>> 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").
> 
> Thanks for the detailed answers (both you and Doug)!
> 
> This change looks good to me, but I'll leave it to a dw-hdmi expert to apply. So
> fwiw,

I'm not qualified as a dw-hdmi expert but until the internal i2c controller
is exposed as a "standard" i2c adapter (which is a valuable feature),
blacklisting a fixed address is wrong, and we should detect invalid/malformed
transactions instead that doesn't fit in the HW model OR really stop emulating
an i2c adapter.

Moving to drm_do_get_edid() would need to entirely rewrite or refactor communication
code to handle the SCDC transactions, since they use an i2c adapter...

Neil

> 
> Reviewed-by: Sean Paul <sean@poorly.run>
> 
> 
>>
>>>> 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++) {
>>>
> 

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

[Sean Paul]

On Tue, Jul 30, 2019 at 03:38:23PM +0200, Neil Armstrong wrote:
> Hi,
> 
> On 25/07/2019 19:49, Sean Paul wrote:
> > On Mon, Jul 22, 2019 at 02:02:07PM -0700, Matthias Kaehlcke wrote:
> >> 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").
> > 
> > Thanks for the detailed answers (both you and Doug)!
> > 
> > This change looks good to me, but I'll leave it to a dw-hdmi expert to apply. So
> > fwiw,
> 
> I'm not qualified as a dw-hdmi expert but until the internal i2c controller
> is exposed as a "standard" i2c adapter (which is a valuable feature),
> blacklisting a fixed address is wrong, and we should detect invalid/malformed
> transactions instead that doesn't fit in the HW model OR really stop emulating
> an i2c adapter.

I think we all agree on this (and Doug mentioned it upthread). That said, the
driver is currently returning successful status and garbage data. I think that's
objectively worse than returning an error, and this patch really doesn't
prevent us from doing it right in the future.

If the code wasn't already upstream, I agree we should pivot to the correct
solution. But unless someone volunteers to fix this the right way, I don't have
a problem with this patch for now.

Sean

> 
> Moving to drm_do_get_edid() would need to entirely rewrite or refactor communication
> code to handle the SCDC transactions, since they use an i2c adapter...
> 
> Neil
> 
> > 
> > Reviewed-by: Sean Paul <sean@poorly.run>
> > 
> > 
> >>
> >>>> 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++) {
> >>>
> > 
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[Neil Armstrong]

On 30/07/2019 18:14, Sean Paul wrote:
> On Tue, Jul 30, 2019 at 03:38:23PM +0200, Neil Armstrong wrote:
>> Hi,
>>
>> On 25/07/2019 19:49, Sean Paul wrote:
>>> On Mon, Jul 22, 2019 at 02:02:07PM -0700, Matthias Kaehlcke wrote:
>>>> 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").
>>>
>>> Thanks for the detailed answers (both you and Doug)!
>>>
>>> This change looks good to me, but I'll leave it to a dw-hdmi expert to apply. So
>>> fwiw,
>>
>> I'm not qualified as a dw-hdmi expert but until the internal i2c controller
>> is exposed as a "standard" i2c adapter (which is a valuable feature),
>> blacklisting a fixed address is wrong, and we should detect invalid/malformed
>> transactions instead that doesn't fit in the HW model OR really stop emulating
>> an i2c adapter.
> 
> I think we all agree on this (and Doug mentioned it upthread). That said, the
> driver is currently returning successful status and garbage data. I think that's
> objectively worse than returning an error, and this patch really doesn't
> prevent us from doing it right in the future.
> 
> If the code wasn't already upstream, I agree we should pivot to the correct
> solution. But unless someone volunteers to fix this the right way, I don't have
> a problem with this patch for now.

Acked with an updated comment, example :
+		/*
+		 * The internal I2C controller does not support the multi-byte
+		 * read and write operations needed for DDC/CI.
+		 * TOFIX: Blacklist the DDC/CI address until we filter out
+		 * unsupported I2C operations.
+		 */

Neil

> 
> Sean
> 
>>
>> Moving to drm_do_get_edid() would need to entirely rewrite or refactor communication
>> code to handle the SCDC transactions, since they use an i2c adapter...
>>
>> Neil
>>
>>>
>>> Reviewed-by: Sean Paul <sean@poorly.run>
>>>
>>>
>>>>
>>>>>> 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++) {
>>>>>
>>>
>>
> 

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

[Matthias Kaehlcke]

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
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>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Sean Paul <sean@poorly.run>
Acked-by: Neil Armstrong <narmstrong@baylibre.com>
---
Sorry for the delay with sending v2, I completely forgot about this patch ...

Changes in v2:
- updated comment with 'TOFIX' entry as requested by Neil
- added Neil's 'Acked-by' tag

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 52d220a70362..ac24bceaf415 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -41,6 +41,7 @@
 
 #include <media/cec-notifier.h>
 
+#define DDC_CI_ADDR		0x37
 #define DDC_SEGMENT_ADDR	0x30
 
 #define HDMI_EDID_LEN		512
@@ -439,6 +440,15 @@ 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.
+		 * TOFIX: Blacklist the DDC/CI address until we filter out
+		 * unsupported I2C operations.
+		 */
+		return -EOPNOTSUPP;
+
 	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
 
 	for (i = 0; i < num; i++) {
-- 
2.23.0.444.g18eeb5a265-goog

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

[Neil Armstrong]

Hi Matthias,

On 02/10/2019 21:44, 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
> 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>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Sean Paul <sean@poorly.run>
> Acked-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> Sorry for the delay with sending v2, I completely forgot about this patch ...
> 
> Changes in v2:
> - updated comment with 'TOFIX' entry as requested by Neil
> - added Neil's 'Acked-by' tag
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 52d220a70362..ac24bceaf415 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -41,6 +41,7 @@
>  
>  #include <media/cec-notifier.h>
>  
> +#define DDC_CI_ADDR		0x37
>  #define DDC_SEGMENT_ADDR	0x30
>  
>  #define HDMI_EDID_LEN		512
> @@ -439,6 +440,15 @@ 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.
> +		 * TOFIX: Blacklist the DDC/CI address until we filter out
> +		 * unsupported I2C operations.
> +		 */
> +		return -EOPNOTSUPP;
> +
>  	dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr);
>  
>  	for (i = 0; i < num; i++) {
> 

Applying to drm-misc-next

Thanks !
Neil