Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller driver

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

Hi,

On Fri, Apr 09, 2021 at 09:35:07PM +0800, Kevin Tang wrote:
> > > > > +     }
> > > > > +
> > > > > +     return MODE_OK;
> > > > > +}
> > > > > +
> > > > > +static void sprd_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > > +                                struct drm_atomic_state *state)
> > > > > +{
> > > > > +     struct sprd_dpu *dpu = to_sprd_crtc(crtc);
> > > > > +
> > > > > +     sprd_dpu_init(dpu);
> > > > > +
> > > > > +     sprd_dpi_init(dpu);
> > > > > +
> > > > > +     enable_irq(dpu->ctx.irq);
> > > >
> > > > Shouldn't this be in enable_vblank? And I would assume that you would
> > > > have the interrupts enabled all the time, but disabled in your device?
> > > >
> > > It seems better to put in enable_vblank, i will try and test it... Thks
> > >
> > >   And I would assume that you would
> > > have the interrupts enabled all the time, but disabled in your device?
> > > [kevin]I don’t quite understand this, can you help me explain it in
> > > detail?
> >
> > You seem to have a register that enables and disables the interrupt in
> > that device. The way we usually deal with them in this case is just to
> > call request_irq in your bind/probe with the interrupts enabled at the
> > controller level, and mask them when needed at the device level by
> > clearing / setting that bit.
> >
>  Yeah, we have display controller interrupts setting and clear register.
> But the interrupts all been enabled in bootloader(eg, lk or uboot),
> if the interrupt handler is active in the probe/bind phase by request_irq,
> but the whole display pipeline is not ready, there maybe have some problems.

It's fairly common to clear / ack the interrupts from the device before
calling request_irq precisely to avoid that issue.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]