Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Apr 2021 11:09:08 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Kevin Tang <kevin3.tang@gmail.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Orson Zhai <orsonzhai@gmail.com>,
        Chunyan Zhang <zhang.lyra@gmail.com>,
        "Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
        ML dri-devel <dri-devel@lists.freedesktop.org>,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v4 4/6] drm/sprd: add Unisoc's drm display controller
 driver
Message-ID: <20210415090908.lmu372lzzjb5hz2b@gilmour>
References: <20210222132822.7830-1-kevin3.tang@gmail.com>
 <20210222132822.7830-5-kevin3.tang@gmail.com>
 <20210324111019.og6d3w47swjim2mq@gilmour>
 <CAFPSGXZ3DjKt87Kc=wc9YKVzTjkQ38Ok6HnHm+VEdqXyHv54Eg@mail.gmail.com>
 <20210407104538.cvssdck26rejrfye@gilmour>
 <CAFPSGXa3xsxmfVquN_pTyBJ4+kL4jQAj6sK+86G3SA2OhB7Jtg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="iqjdlfj2zil34vaj"
Content-Disposition: inline
In-Reply-To: <CAFPSGXa3xsxmfVquN_pTyBJ4+kL4jQAj6sK+86G3SA2OhB7Jtg@mail.gmail.com>
Precedence: bulk


--iqjdlfj2zil34vaj
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 =3D 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 wou=
ld
> > > > have the interrupts enabled all the time, but disabled in your devi=
ce?
> > > >
> > > It seems better to put in enable_vblank, i will try and test it... Th=
ks
> > >
> > >   And I would assume that you would
> > > have the interrupts enabled all the time, but disabled in your device?
> > > [kevin]I don=E2=80=99t 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 proble=
ms.

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

Maxime

--iqjdlfj2zil34vaj
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYHgCtAAKCRDj7w1vZxhR
xXBaAQC3ACb0Go3hl2khk93hUI2vLUrRqFpmr9tlUyb7hqba7QD+NkO+fFD5Ue+7
9aQgbCXUIw4o3IFlfY8J7s5gjehvog8=
=JU1P
-----END PGP SIGNATURE-----

--iqjdlfj2zil34vaj--