On Fri, Apr 07, 2017 at 11:02:33AM +0800, Shawn Guo wrote:
> On Thu, Apr 06, 2017 at 01:12:51PM -0400, Sean Paul wrote:
> > On Thu, Apr 06, 2017 at 11:01:10PM +0800, Shawn Guo wrote:
> > > +static int zx_vga_connector_get_modes(struct drm_connector *connector)
> > > +{
> > > + struct zx_vga *vga = to_zx_vga(connector);
> > > + unsigned long flags;
> > > + struct edid *edid;
> > > + int ret;
> > > +
> > > + /*
> > > + * Clear both detection bits to switch I2C bus from device
> > > + * detecting to EDID reading.
> > > + */
> > > + spin_lock_irqsave(&vga->lock, flags);
> > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0);
> > > + spin_unlock_irqrestore(&vga->lock, flags);
> > > +
> > > + edid = drm_get_edid(connector, &vga->ddc->adap);
> > > + if (!edid)
> > > + return 0;
> > > +
> > > + /*
> > > + * As edid reading succeeds, device must be connected, so we set
> > > + * up detection bit for unplug interrupt here.
> > > + */
> > > + spin_lock_irqsave(&vga->lock, flags);
> > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE);
> > > + spin_unlock_irqrestore(&vga->lock, flags);
> > > +
> > > + drm_mode_connector_update_edid_property(connector, edid);
> > > + ret = drm_add_edid_modes(connector, edid);
> > > + kfree(edid);
> > > +
> > > + return ret;
> > > +}
>
> <snip>
>
> > > +static irqreturn_t zx_vga_irq_handler(int irq, void *dev_id)
> > > +{
> > > + struct zx_vga *vga = dev_id;
> > > + u32 status;
> > > +
> > > + status = zx_readl(vga->mmio + VGA_I2C_STATUS);
> > > +
> > > + /* Clear interrupt status */
> > > + zx_writel_mask(vga->mmio + VGA_I2C_STATUS, VGA_CLEAR_IRQ,
> > > + VGA_CLEAR_IRQ);
> > > +
> > > + if (status & VGA_DEVICE_CONNECTED) {
> > > + /*
> > > + * Since VGA_DETECT_SEL bits need to be reset for switching DDC
> > > + * bus from device detection to EDID read, rather than setting
> > > + * up HAS_DEVICE bit here, we need to do that in .get_modes
> > > + * hook for unplug detecting after EDID read succeeds.
> > > + */
> > > + vga->connected = true;
> > > + return IRQ_WAKE_THREAD;
> > > + }
> > > +
> > > + if (status & VGA_DEVICE_DISCONNECTED) {
> > > + spin_lock(&vga->lock);
> > > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL,
> > > + VGA_DETECT_SEL_NO_DEVICE);
> > > + spin_unlock(&vga->lock);
> >
> > I think you still have the race here. If you get a disconnect between get_edid
> > successfully finishing, and resetting the DETECT_SEL register, you will end up
> > with the device being disconnected and DETECT_SEL == VGA_DETECT_SEL_HAS_DEVICE.
> >
> > In order to close this, you'll need to hold the lock across the edid read.
>
> We cannot hold a spin lock across the EDID read procedure, which does
> sleep.
Yeah, this is a pretty common problem. We usually use a mutex in conjunction
with a work function to handle this type of thing.
>
> When you suggested to have a lock for DETECT_SEL register access, I
> thought we are accessing it in a read-modify-write way and thus agreed
> to add a lock. However, I just found that it's not the case actually.
> All the accesses to the register are single direct write.
>
> And here is my understanding about the condition you described above.
> Once DETECT_SEL register gets reset (both bits cleared), the hardware
> switches DDC bus for EDID read, and hotplug detection will stop working
> right away (this is how ZTE hardware works). That said, if we get a
> disconnect before drm_get_edid() successfully finishes, two points:
>
> - The irq handler will not be triggered, so it will not get a chance to
> update DETECT_SEL register.
Ah, this was the missing piece I needed. I hadn't realized that the first
VGA_AUTO_DETECT_SEL write in get_modes disabled the irq.
> - The drm_get_edid() fails, and .get_modes returns with both detect bits
> kept cleared.
>
> I think what we can do better in this case is that we should set the
> device state into disconnected, before returning from .get_modes,
> something like the code below. But still, I do not see we need a lock
> for that.
Yep, agreed. Can you also please add to your comment in the !edid case,
something like:
* Locking is not required here since the only other place to write this register
* (and connected var) is the irq handler. The irq handler is disabled because
* we've cleared AUTO_DETECT_SEL above.
Thanks for walking me through this.
Sean
>
> Shawn
>
> static int zx_vga_connector_get_modes(struct drm_connector *connector)
> {
> struct zx_vga *vga = to_zx_vga(connector);
> unsigned long flags;
> struct edid *edid;
> int ret;
>
> /*
> * Clear both detection bits to switch I2C bus from device
> * detecting to EDID reading.
> */
> zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, 0);
>
> edid = drm_get_edid(connector, &vga->ddc->adap);
> if (!edid) {
> /*
> * If EDID reading fails, we set the device state into
> * disconnected.
> */
> zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL,
> VGA_DETECT_SEL_NO_DEVICE);
> vga->connected = false;
> return 0;
> }
>
> /*
> * As edid reading succeeds, device must be connected, so we set
> * up detection bit for unplug interrupt here.
> */
> zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL, VGA_DETECT_SEL_HAS_DEVICE);
>
> drm_mode_connector_update_edid_property(connector, edid);
> ret = drm_add_edid_modes(connector, edid);
> kfree(edid);
>
> return ret;
> }
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
