From patchwork Sat Aug 29 22:37:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Vladimir Zapolskiy X-Patchwork-Id: 7095771 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 80A45BEEC1 for ; Sat, 29 Aug 2015 22:38:04 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0B61320737 for ; Sat, 29 Aug 2015 22:38:03 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 1D2AE20735 for ; Sat, 29 Aug 2015 22:38:01 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id CC8F66EA7B; Sat, 29 Aug 2015 15:37:58 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from relay1.mentorg.com (relay1.mentorg.com [192.94.38.131]) by gabe.freedesktop.org (Postfix) with ESMTPS id 113E56EA7B for ; Sat, 29 Aug 2015 15:37:58 -0700 (PDT) Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1ZVokw-0006gV-A2 from Vladimir_Zapolskiy@mentor.com ; Sat, 29 Aug 2015 15:37:50 -0700 Received: from [137.202.108.169] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server (TLS) id 14.3.224.2; Sat, 29 Aug 2015 23:37:27 +0100 Subject: Re: [PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support To: Russell King - ARM Linux References: <1431952341-20823-1-git-send-email-vladimir_zapolskiy@mentor.com> <1431952341-20823-3-git-send-email-vladimir_zapolskiy@mentor.com> <20150813225616.GA6912@n2100.arm.linux.org.uk> From: Vladimir Zapolskiy Message-ID: <55E23426.9010306@mentor.com> Date: Sun, 30 Aug 2015 01:37:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Icedove/38.1.0 MIME-Version: 1.0 In-Reply-To: <20150813225616.GA6912@n2100.arm.linux.org.uk> X-Originating-IP: [137.202.0.76] Cc: Fabio Estevam , dri-devel@lists.freedesktop.org, Yakir Yang , Andy Yan X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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 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);