[PATCH v2 4/4] drm: zte: add VGA driver support

[Sean Paul]

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