On Tue, Apr 04, 2017 at 01:46:48PM -0400, Sean Paul wrote:
> > +static int zx_vga_register(struct drm_device *drm, struct zx_vga *vga)
> > +{
> > + struct drm_encoder *encoder = &vga->encoder;
> > + struct drm_connector *connector = &vga->connector;
> > +
> > + encoder->possible_crtcs = VOU_CRTC_MASK;
> > +
> > + drm_encoder_init(drm, encoder, &zx_vga_encoder_funcs,
> > + DRM_MODE_ENCODER_DAC, NULL);
> > + drm_encoder_helper_add(encoder, &zx_vga_encoder_helper_funcs);
> > +
> > + vga->connector.polled = DRM_CONNECTOR_POLL_HPD;
> > +
> > + drm_connector_init(drm, connector, &zx_vga_connector_funcs,
> > + DRM_MODE_CONNECTOR_VGA);
> > + drm_connector_helper_add(connector, &zx_vga_connector_helper_funcs);
> > +
> > + drm_mode_connector_attach_encoder(connector, encoder);
>
> You're missing a couple of return value checks in this function.
Okay. I will add return checks in the new version.
>
> > +
> > + return 0;
> > +}
<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) {
> > + /*
> > + * We should ideally set up VGA_DETECT_SEL_HAS_DEVICE bit here
> > + * for unplug detection, but doing so will stop DDC bus from
> > + * reading EDID later on. It looks like a HW limitation, and we
> > + * work around it by defering the bit setup to .get_modes hook
> > + * after EDID read succeeds.
>
> get_modes resets VGA_AUTO_DETECT_SEL before and after reading edid, so is it
> really necessary to defer setting it here?
Okay, the comment in the code seems confusing. Since we need to reset
VGA_AUTO_DETECT_SEL bits to switch DDC bus from device detecting to EDID
reading, VGA_DETECT_SEL_HAS_DEVICE will need to be set after reading
EDID anyway. I will improve the comment.
>
> > + */
> > + vga->connected = true;
> > + return IRQ_WAKE_THREAD;
> > + }
> > +
> > + if (status & VGA_DEVICE_DISCONNECTED) {
> > + zx_writel(vga->mmio + VGA_AUTO_DETECT_SEL,
> > + VGA_DETECT_SEL_NO_DEVICE);
>
> This races with get_modes, you should serialize access to this register.
Good catch. I will add lock for it.
Shawn
>
> > + vga->connected = false;
> > + return IRQ_WAKE_THREAD;
> > + }
> > +
> > + if (status & VGA_TRANS_DONE) {
> > + complete(&vga->complete);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + return IRQ_NONE;
> > +}
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
