Hi David, Philipp, Andy, Russell,

On 19.05.2015 17:39, Andy Yan wrote:

Hi Vladimir, Thanks for you patch.

On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:

...

I2CM_ADDRESS became a MESS, fix it, also change guarding define to __DW_HDMI_H__ , since the driver is not IMX specific.

Signed-off-by: Vladimir Zapolskiy vladimir_zapolskiy@mentor.com

Acked-by: Andy Yan <andy.yan@rock-chips.com>

...

---

drivers/gpu/drm/bridge/dw_hdmi.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h index 175dbc8..ee7f7ed 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.h +++ b/drivers/gpu/drm/bridge/dw_hdmi.h @@ -7,8 +7,8 @@

• (at your option) any later version.

*/

-#ifndef __IMX_HDMI_H__ -#define __IMX_HDMI_H__ +#ifndef __DW_HDMI_H__ +#define __DW_HDMI_H__

/* Identification Registers */ #define HDMI_DESIGN_ID 0x0000 @@ -525,7 +525,7 @@

/* I2C Master Registers (E-DDC) */ #define HDMI_I2CM_SLAVE 0x7E00 -#define HDMI_I2CMESS 0x7E01 +#define HDMI_I2CM_ADDRESS 0x7E01 #define HDMI_I2CM_DATAO 0x7E02 #define HDMI_I2CM_DATAI 0x7E03 #define HDMI_I2CM_OPERATION 0x7E04 @@ -1031,4 +1031,4 @@ enum { HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0, };

-#endif /* __IMX_HDMI_H__ */ +#endif /* __DW_HDMI_H__ */

what would be the next action regarding these two patches? If review is done, should they go to drm-dwhdmi-devel or drm-next ?

-- With best wishes, Vladimir

Hi Russell,

On 14.08.2015 01:56, Russell King - ARM Linux wrote:

On Mon, May 18, 2015 at 03:32:21PM +0300, Vladimir Zapolskiy wrote:

...

+/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */ +#define HDMI_IH_I2CM_STAT0_ERROR BIT(0) +#define HDMI_IH_I2CM_STAT0_DONE BIT(1)

+/* HDMI_I2CM_OPERATION register bits */ +#define HDMI_I2CM_OPERATION_READ BIT(0) +#define HDMI_I2CM_OPERATION_READ_EXT BIT(1) +#define HDMI_I2CM_OPERATION_WRITE BIT(4)

+/* HDMI_I2CM_INT register bits */ +#define HDMI_I2CM_INT_DONE_MASK BIT(2) +#define HDMI_I2CM_INT_DONE_POL BIT(3)

+/* HDMI_I2CM_CTLINT register bits */ +#define HDMI_I2CM_CTLINT_ARB_MASK BIT(2) +#define HDMI_I2CM_CTLINT_ARB_POL BIT(3) +#define HDMI_I2CM_CTLINT_NAC_MASK BIT(6) +#define HDMI_I2CM_CTLINT_NAC_POL BIT(7)

Please put these definitions in dw_hdmi.h

okay.

...

#define HDMI_EDID_LEN 512

#define RGB 0 @@ -102,6 +124,19 @@ struct hdmi_data_info { struct hdmi_vmode video_mode; };

+struct dw_hdmi_i2c {

• struct i2c_adapter adap;
• spinlock_t lock;
• struct completion cmp_r;
• struct completion cmp_w;
• u8 stat;
• u8 operation_reg;
• u8 slave_reg;
• bool is_regaddr;

+};

struct dw_hdmi { struct drm_connector connector; struct drm_encoder *encoder; @@ -111,6 +146,7 @@ struct dw_hdmi { struct device *dev; struct clk *isfr_clk; struct clk *iahb_clk;

• struct dw_hdmi_i2c *i2c;

  struct hdmi_data_info hdmi_data; const struct dw_hdmi_plat_data *plat_data;

@@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, hdmi_modb(hdmi, data << shift, mask, reg); }

+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) +{

• unsigned long flags;
• spin_lock_irqsave(&hdmi->i2c->lock, flags);
• /* Set Fast Mode speed */
• hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
• /* Software reset */
• hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
• /* Set done, not acknowledged and arbitration interrupt polarities */
• hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
• hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,

•     HDMI_I2CM_CTLINT);
  

• /* Clear DONE and ERROR interrupts */
• hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,

•     HDMI_IH_I2CM_STAT0);
  

• /* Mute DONE and ERROR interrupts */
• hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,

•     HDMI_IH_MUTE_I2CM_STAT0);
  

• spin_unlock_irqrestore(&hdmi->i2c->lock, flags);

What exactly is this spinlock protecting against with the above code?

On second inspection I don't see a need of locking here.

...

+}

+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,

• 	    unsigned char *buf, int length)
  

+{

• int stat;
• unsigned long flags;
• struct dw_hdmi_i2c *i2c = hdmi->i2c;
• spin_lock_irqsave(&i2c->lock, flags);
• i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
• if (!i2c->is_regaddr) {

• dev_dbg(hdmi->dev, "set read register address to 0\n");
  

• i2c->slave_reg = 0x00;
  

• i2c->is_regaddr = true;
  

• }
• while (length--) {

• reinit_completion(&i2c->cmp_r);
  

If you're re-initialising the completion on every byte, why do you need separate completions for the read path and the write path?

If a single completion can be used, you then don't have to store the operation register value in struct dw_hdmi_i2c either.

Correct, I had only one completion and no i2c->operation_reg in v1, will revert the added complexity to match the previous version http://lists.freedesktop.org/archives/dri-devel/2015-May/082887.html

...

• i2c->stat = 0;
  

What use does zeroing this have? You don't read it, except after you've had a successful completion, which implies that the interrupt handler must have written to it.

I agree, it can be removed.

...

• hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
  

• hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
  

• spin_unlock_irqrestore(&i2c->lock, flags);
  

• stat = wait_for_completion_interruptible_timeout(&i2c->cmp_r,
  

• 						 HZ / 10);
  

• if (!stat)
  

• 	return -ETIMEDOUT;
  

• if (stat < 0)
  

• 	return stat;
  

Can a DDC read/write really be interrupted and restarted correctly? Won't this restart the transaction from the very beginning? Have you validated that all code paths calling into here can cope with -ERESTARTSYS?

If you look at drm_do_probe_ddc_edid() for example, it will retry the transfer if you return -ERESTARTSYS here, but as the signal has not been handled, wait_for_completion_interruptible_timeout() will immediately return -ERESTARTSYS again... and again... and again.

For me it sounds like an incomplete error condition checking in drm_do_probe_ddc_edid(). There are 5 retries defined to complete I2C transfer,

i2c_transfer() may return -EOPNOTSUPP, -EAGAIN or any adap->algo->master_xfer() value.

I've checked source code of the last 10 alphabetically sorted I2C bus drivers from i2c/busses/* with .master_xfer in algo, I have found that the following error codes may be reported:

i2c-sirf.c -EAGAIN on timeout i2c-st.c -ETIMEDOUT on timeout, -EBUSY, -EIO, -EAGAIN i2c-stu300.c -ERESTARTSYS (interrupted), -ETIMEDOUT, -EINVAL, -EIO i2c-tegra.c -ETIMEDOUT, -EBUSY, -EINVAL, -EREMOTEIO, -EIO i2c-tiny-usb.c -ENOMEM, -EREMOTEIO i2c-viperboard.c -EREMOTEIO, -EPROTO i2c-wmt.c -ETIMEDOUT, -EIO, -EBUSY i2c-xiic.c -ETIMEDOUT, -EIO, -EBUSY i2c-xlp9xx.c -ETIMEDOUT, -EIO i2c-xlr.c -ETIMEDOUT, -EIO

Seems that only 1 of 10 bus drivers is supported correctly by drm_do_probe_ddc_edid().

Each time will queue another operation _without_ waiting for the last one to complete.

I agree it may happen, what would be the best way to avoid this kind of problem in your opinion? May be convert wait_for_completion_interruptible_timeout() to wait_for_completion_timeout() ?

In any case I would propose to add

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7087da3..93cb1cd 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1242,6 +1242,9 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", adapter->name); break; + } else if (ret < 0 && ret != -EAGAIN) { + DRM_ERROR("I2C transfer error: %d\n", ret); + break; } } while (ret != xfers && --retries);

Or due to the statistics it might be better to use -ETIMEDOUT instead of -EAGAIN above...

...

• spin_lock_irqsave(&i2c->lock, flags);
  

• /* Check for error condition on the bus */
  

• if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
  

• 	spin_unlock_irqrestore(&i2c->lock, flags);
  

• 	return -EIO;
  

• }
  

• *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
  

• }
• spin_unlock_irqrestore(&i2c->lock, flags);

I'd be very surprised if you need the spinlocks in the above code. You'll see the update to i2c->stat after the completion, becuase wait_for_completion*() is in the same class as the other event-waiting functions, and contains barriers which will ensure that you will not read i2c->stat before you see the completion even on SMP platforms.

I have no objections and I will remove this spinlock.

The initial reason for the spinlock was to atomically update pair of HDMI_I2CM_ADDRESS and HDMI_I2CM_OPERATION registers, but if dw_hdmi_i2c_read() and dw_hdmi_i2c_write() are serialized (e.g. by serializing dw_hdmi_i2c_xfer()) the spinlock is not needed then.

...

• return 0;

+}

...

...

+static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,

• 	    struct i2c_msg *msgs, int num)
  

+{

• struct dw_hdmi *hdmi = i2c_get_adapdata(adap);
• struct dw_hdmi_i2c *i2c = hdmi->i2c;
• int i, ret;
• u8 addr;
• unsigned long flags;
• dev_dbg(hdmi->dev, "xfer: num: %d, addr: 0x%x\n", num, msgs[0].addr);
• spin_lock_irqsave(&i2c->lock, flags);
• hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0);
• /* Set slave device address from the first transaction */
• addr = msgs[0].addr;
• hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE);
• /* Set slave device register address on transfer */
• i2c->is_regaddr = false;
• spin_unlock_irqrestore(&i2c->lock, flags);
• for (i = 0; i < num; i++) {

• dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: 0x%x\n",
  

• 	i + 1, num, msgs[i].len, msgs[i].flags);
  

• if (msgs[i].addr != addr) {
  

• 	dev_warn(hdmi->dev,
  

• 		 "unsupported transaction, changed slave address\n");
  

• 	ret = -EOPNOTSUPP;
  

• 	break;
  

• }
  

Probably ought to validate that before starting any transaction?

...

• if (msgs[i].len == 0) {
  

• 	dev_dbg(hdmi->dev,
  

• 		 "unsupported transaction %d/%d, no data\n",
  

• 		 i + 1, num);
  

• 	ret = -EOPNOTSUPP;
  

• 	break;
  

• }
  

Ditto.

Do you mean split the cycle to loop over message array for validation purpose and then attempt to send messages iff all of them are valid? Probably it makes sense, since the expected length of a message array is small, I'll implement it.

It is worth to mention that the message array from drm_do_probe_ddc_edid() discussed above won't pass this validation, if a monitor has more than 1 extension blocks, the number is taken from the Extension Flag 0x7E. This case should be handled separately using unimplemented in my change "I2C Master Interface Extended Read Mode", unfortunately I don't have such monitor to add/test this feature of DW HDMI. Probably I can fake a monitor and read EDID with multiple Extension Blocks from AT24 EEPROM, but I would prefer to postpone this change, hopefully most of HDMI monitors are supported by this version.

...

• if (msgs[i].flags & I2C_M_RD)
  

• 	ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
  

• else
  

• 	ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
  

• if (ret < 0)
  

• 	break;
  

• }
• if (!ret)

• ret = num;
  

• spin_lock_irqsave(&i2c->lock, flags);
• /* Mute DONE and ERROR interrupts */
• hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,

•     HDMI_IH_MUTE_I2CM_STAT0);
  

• spin_unlock_irqrestore(&i2c->lock, flags);

What exactly is this spinlock protecting us from? A single write to a register is always atomic.

Will remove it.

On the other hand I think of adding a mutex to serialize dw_hdmi_i2c_xfer() calls.

...

• return ret;

+}

+static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter) +{

• return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

+}

+static struct i2c_algorithm dw_hdmi_algorithm = {

const?

Certainly.

...

• .master_xfer = dw_hdmi_i2c_xfer,
• .functionality = dw_hdmi_i2c_func,

+};

+static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi) +{

• struct i2c_adapter *adap;
• int ret;
• hdmi->i2c = devm_kzalloc(hdmi->dev, sizeof(*hdmi->i2c), GFP_KERNEL);
• if (!hdmi->i2c)

• return ERR_PTR(-ENOMEM);
  

I'd much prefer: struct dw_hdmi_i2c *i2c;

i2c = devm_kzalloc(...);

...

• spin_lock_init(&hdmi->i2c->lock);
• init_completion(&hdmi->i2c->cmp_r);
• init_completion(&hdmi->i2c->cmp_w);
• adap = &hdmi->i2c->adap;
• adap->class = I2C_CLASS_DDC;
• adap->owner = THIS_MODULE;
• adap->dev.parent = hdmi->dev;
• adap->algo = &dw_hdmi_algorithm;
• strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name));
• i2c_set_adapdata(adap, hdmi);
• ret = i2c_add_adapter(adap);
• if (ret) {

• dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
  

• devm_kfree(hdmi->dev, hdmi->i2c);
  

• hdmi->i2c = NULL;
  

• return ERR_PTR(ret);
  

• }

hdmi->i2c = i2c;

rather than having to remember to clear out hdmi->i2c in error paths.

No objections, the resulting code should be slightly simpler.

...

• dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name);
• return adap;

+}

static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts, unsigned int n) { @@ -1466,16 +1745,47 @@ struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .mode_fixup = dw_hdmi_bridge_mode_fixup, };

+static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) +{

• struct dw_hdmi_i2c *i2c = hdmi->i2c;
• unsigned long flags;
• spin_lock_irqsave(&i2c->lock, flags);
• i2c->stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
• if (!i2c->stat) {

• spin_unlock_irqrestore(&i2c->lock, flags);
  

• return IRQ_NONE;
  

• }
• hdmi_writeb(hdmi, i2c->stat, HDMI_IH_I2CM_STAT0);
• if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)

• complete(&i2c->cmp_r);
  

• else

• complete(&i2c->cmp_w);
  

• spin_unlock_irqrestore(&i2c->lock, flags);

Again, what function does this spinlock perform? Wouldn't:

unsigned int stat;

stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0); if (stat == 0) return IRQ_NONE;

/* Clear interrupts */ hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0);

i2c->stat = stat;

if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ) complete(&i2c->cmp_r); else complete(&i2c->cmp_w);

be fine, or maybe with the spinlock around the assignment to i2c->stat and this point if you need the assignment and completion to be atomic?

Note that the write to 'i2c->stat' would be atomic in itself anyway.

In any case, complete() implies a write memory barrier, so even on SMP systems, the write to i2c->stat will be visible before the completion "completes". So, I don't think you need any locking here.

Nice trick with a local variable, I agree it is safe to remove the spinlock from the handler.

Thanks.

Thank you for review, I'll send the updates. Please check my proposals regarding drm_do_probe_ddc_edid(),

-- With best wishes, Vladimir