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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
