From: Liu Ying <victor.liu@nxp.com>
To: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
Cc: linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, p.zabel@pengutronix.de,
airlied@linux.ie, daniel@ffwll.ch, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de,
festevam@gmail.com, linux-imx@nxp.com, robh+dt@kernel.org,
maarten.lankhorst@linux.intel.com, mripard@kernel.org,
tzimmermann@suse.de, guido.gunther@puri.sm
Subject: Re: [PATCH v6 5/6] drm/imx: Introduce i.MX8qm/qxp DPU DRM
Date: Tue, 26 Jan 2021 13:20:32 +0800 [thread overview]
Message-ID: <9233b33c789a2b207a437df9abcff1dd7fd89b16.camel@nxp.com> (raw)
In-Reply-To: <20210125134806.w77bdrx2wbb4kirz@fsr-ub1864-141>
On Mon, 2021-01-25 at 15:48 +0200, Laurentiu Palcu wrote:
> Hi Liu Ying,
>
> Just some minor comments below.
>
> On Thu, Jan 21, 2021 at 03:14:22PM +0800, Liu Ying wrote:
> > This patch introduces i.MX8qm/qxp Display Processing Unit(DPU) DRM support.
> >
> > DPU is comprised of two main components that include a blit engine for
> > 2D graphics accelerations(with composition support) and a display controller
> > for display output processing, as well as a command sequencer. Outside of
> > DPU, optional prefetch engines, a.k.a, Prefetch Resolve Gasket(PRG) and
> > Display Prefetch Resolve(DPR), can fetch data from memory prior to some DPU
> > fetchunits of blit engine and display controller. The prefetch engines
> > support reading linear formats and resolving Vivante GPU tile formats.
> >
> > This patch adds kernel modesetting support for the display controller part.
> > The driver supports two CRTCs per display controller, planes backed by
> > four fetchunits(decode0/1, fetchlayer, fetchwarp), fetchunit allocation
> > logic for the two CRTCs, prefetch engines(with tile resolving supported),
> > plane upscaling/deinterlacing/yuv2rgb CSC/alpha blending and CRTC gamma
> > correction. The registers of the controller is accessed without command
> > sequencer involved, instead just by using CPU.
> >
> > Reference manual can be found at:
> > https://www.nxp.com/webapp/Download?colCode=IMX8DQXPRM
> >
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> > v5->v6:
> > * Do not use macros where possible. (Laurentiu)
> > * Break dpu_plane_atomic_check() into some smaller functions. (Laurentiu)
> > * Address some minor comments from Laurentiu.
> > * Add dpu_crtc_err() helper marco to tell dmesg which CRTC generates error.
> > * Drop calling dev_set_drvdata() from dpu_drm_bind/unbind() as it is done
> > in dpu_drm_probe().
> > * Some trivial tweaks.
> >
> > v4->v5:
> > * Rebase up onto the latest drm-misc-next branch and remove the hook to
> > drm_atomic_helper_legacy_gamma_set(), because it was dropped by the newly
> > landed commit 'drm: automatic legacy gamma support'.
> > * Remove a redundant blank line from dpu_plane_atomic_update().
> >
> > v3->v4:
> > * No change.
> >
> > v2->v3:
> > * Fix build warnings Reported-by: kernel test robot <lkp@intel.com>.
> > * Drop build dependency on IMX_SCU, as dummy SCU functions have been added in
> > header files by the patch 'firmware: imx: add dummy functions' which has
> > landed in linux-next/master branch.
> >
> > v1->v2:
> > * Add compatible for i.MX8qm DPU, as this is tested with i.MX8qm LVDS displays.
> > (Laurentiu)
> > * Fix PRG burst size and stride. (Laurentiu)
> > * Put 'ports' OF node to fix the bail-out logic in dpu_drm_probe(). (Laurentiu)
> >
> > drivers/gpu/drm/imx/Kconfig | 1 +
> > drivers/gpu/drm/imx/Makefile | 1 +
> > drivers/gpu/drm/imx/dpu/Kconfig | 10 +
> > drivers/gpu/drm/imx/dpu/Makefile | 10 +
> > drivers/gpu/drm/imx/dpu/dpu-constframe.c | 171 +++++
> > drivers/gpu/drm/imx/dpu/dpu-core.c | 1094 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-crtc.c | 967 +++++++++++++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-crtc.h | 66 ++
> > drivers/gpu/drm/imx/dpu/dpu-disengcfg.c | 117 +++
> > drivers/gpu/drm/imx/dpu/dpu-dprc.c | 718 +++++++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-dprc.h | 40 ++
> > drivers/gpu/drm/imx/dpu/dpu-drv.c | 292 ++++++++
> > drivers/gpu/drm/imx/dpu/dpu-drv.h | 28 +
> > drivers/gpu/drm/imx/dpu/dpu-extdst.c | 299 ++++++++
> > drivers/gpu/drm/imx/dpu/dpu-fetchdecode.c | 294 ++++++++
> > drivers/gpu/drm/imx/dpu/dpu-fetcheco.c | 224 ++++++
> > drivers/gpu/drm/imx/dpu/dpu-fetchlayer.c | 154 ++++
> > drivers/gpu/drm/imx/dpu/dpu-fetchunit.c | 609 ++++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-fetchunit.h | 191 +++++
> > drivers/gpu/drm/imx/dpu/dpu-fetchwarp.c | 250 +++++++
> > drivers/gpu/drm/imx/dpu/dpu-framegen.c | 395 +++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-gammacor.c | 223 ++++++
> > drivers/gpu/drm/imx/dpu/dpu-hscaler.c | 275 ++++++++
> > drivers/gpu/drm/imx/dpu/dpu-kms.c | 540 ++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-kms.h | 23 +
> > drivers/gpu/drm/imx/dpu/dpu-layerblend.c | 348 +++++++++
> > drivers/gpu/drm/imx/dpu/dpu-plane.c | 799 +++++++++++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-plane.h | 56 ++
> > drivers/gpu/drm/imx/dpu/dpu-prg.c | 433 ++++++++++++
> > drivers/gpu/drm/imx/dpu/dpu-prg.h | 45 ++
> > drivers/gpu/drm/imx/dpu/dpu-prv.h | 233 ++++++
> > drivers/gpu/drm/imx/dpu/dpu-tcon.c | 250 +++++++
> > drivers/gpu/drm/imx/dpu/dpu-vscaler.c | 308 ++++++++
> > drivers/gpu/drm/imx/dpu/dpu.h | 385 ++++++++++
> > 34 files changed, 9849 insertions(+)
> > create mode 100644 drivers/gpu/drm/imx/dpu/Kconfig
> > create mode 100644 drivers/gpu/drm/imx/dpu/Makefile
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-constframe.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-core.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-crtc.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-crtc.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-disengcfg.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-dprc.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-dprc.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-drv.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-drv.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-extdst.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetchdecode.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetcheco.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetchlayer.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetchunit.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetchunit.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-fetchwarp.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-framegen.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-gammacor.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-hscaler.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-kms.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-kms.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-layerblend.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-plane.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-plane.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-prg.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-prg.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-prv.h
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-tcon.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu-vscaler.c
> > create mode 100644 drivers/gpu/drm/imx/dpu/dpu.h
> >
>
> [...]
>
> > diff --git a/drivers/gpu/drm/imx/dpu/dpu-core.c b/drivers/gpu/drm/imx/dpu/dpu-core.c
> > new file mode 100644
> > index 00000000..7dab6cc
> > --- /dev/null
> > +++ b/drivers/gpu/drm/imx/dpu/dpu-core.c
>
> [...]
>
> > +static int dpu_get_irqs(struct platform_device *pdev, struct dpu_soc *dpu)
> > +{
> > + unsigned int i, j;
> > +
> > + /* do not get the reserved irq */
> > + for (i = 0, j = 0; i < DPU_IRQ_COUNT - 1; i++, j++) {
> > + if (i == DPU_IRQ_RESERVED)
> > + j++;
> > +
> > + dpu->irq[j] = platform_get_irq(pdev, i);
> > + if (dpu->irq[j] < 0) {
> > + dev_err_probe(dpu->dev, dpu->irq[j],
> > + "failed to get irq\n");
> > + return dpu->irq[i];
>
> I think you want 'return dpu->irq[j]'.
Good catch.
>
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> [...]
>
> > +static const struct dpu_irq_handler_map {
> > + void (*handler)(struct irq_desc *desc);
> > +} dpu_irq_handler_maps[DPU_IRQ_COUNT] = {
> > + {}, /* 0 */
> > + {}, /* 1 */
> > + {}, /* 2 */
> > + {dpu_extdst0_shdload_irq_handler}, /* 3 */
> > + {}, /* 4 */
> > + {}, /* 5 */
> > + {dpu_extdst4_shdload_irq_handler}, /* 6 */
> > + {}, /* 7 */
> > + {}, /* 8 */
> > + {dpu_extdst1_shdload_irq_handler}, /* 9 */
> > + {}, /* 10 */
> > + {}, /* 11 */
> > + {dpu_extdst5_shdload_irq_handler}, /* 12 */
> > + {}, /* 13 */
> > + {}, /* 14 */
> > + {dpu_disengcfg_shdload0_irq_handler}, /* 15 */
> > + {dpu_disengcfg_framecomplete0_irq_handler}, /* 16 */
> > + {dpu_disengcfg_seqcomplete0_irq_handler}, /* 17 */
> > + {}, /* 18 */
> > + {}, /* 19 */
> > + {}, /* 20 */
> > + {}, /* 21 */
> > + {}, /* 22 */
> > + {}, /* 23 */
> > + {}, /* 24 */
> > + {dpu_disengcfg_shdload1_irq_handler}, /* 25 */
> > + {dpu_disengcfg_framecomplete1_irq_handler}, /* 26 */
> > + {dpu_disengcfg_seqcomplete1_irq_handler}, /* 27 */
> > + {}, /* 28 */
> > + {}, /* 29 */
> > + {}, /* 30 */
> > + {}, /* 31 */
> > + {}, /* 32 */
> > + {}, /* 33 */
> > + {}, /* 34 */
> > + {/* reserved */}, /* 35 */
> > + {}, /* 36 */
> > + {}, /* 37 */
> > + {}, /* 38 */
> > + {}, /* 39 */
> > + {}, /* 40 */
> > + {}, /* 41 */
> > + {}, /* 42 */
> > + {}, /* 43 */
> > + {}, /* 44 */
> > + {}, /* 45 */
> > + {}, /* 46 */
> > + {}, /* 47 */
> > + {}, /* 48 */
> > +};
>
> Why not make this an array of pointers to functions. Do we need a struct?
> Something like:
>
> static void (* const dpu_irq_handler[DPU_IRQ_COUNT])(struct irq_desc *) = {
> [3] = dpu_extdst0_shdload_irq_handler,
> [6] = dpu_extdst4_shdload_irq_handler,
> ...
> };
Alright, will use the function array.
>
> [...]
>
> > +static int
> > +dpu_get_fetchunits_for_plane_grp(struct dpu_soc *dpu,
> > + const struct dpu_units *us,
> > + struct dpu_fetchunit ***fu,
> > + unsigned int *cnt,
> > + struct dpu_fetchunit *
> > + (*get)(struct dpu_soc *dpu,
> > + unsigned int id))
> > +{
> > + unsigned int fu_cnt = 0;
> > + int i, j, ret;
> > +
> > + for (i = 0; i < us->cnt; i++) {
> > + if (us->types[i] == DPU_DISP)
> > + fu_cnt++;
> > + }
> > +
> > + *cnt = fu_cnt;
> > +
> > + *fu = devm_kcalloc(dpu->dev, fu_cnt, sizeof(**fu), GFP_KERNEL);
> > + if (!(*fu))
> > + return -ENOMEM;
> > +
> > + for (i = 0, j = 0; i < us->cnt; i++) {
> > + if (us->types[i] != DPU_DISP)
> > + continue;
> > +
> > + (*fu)[j] = (*get)(dpu, us->ids[i]);
>
> You can also call get() directly. No need to dereference function
> pointers.
Will do.
>
> > + if (IS_ERR((*fu)[j])) {
> > + ret = PTR_ERR((*fu)[j]);
> > + dev_err(dpu->dev, "failed to get %s%d: %d\n",
> > + us->name, us->ids[i], ret);
> > + return ret;
> > + }
> > + j++;
> > + }
> > +
> > + return 0;
> > +}
>
> [...]
>
> > +static void
> > +dpu_put_fetchunits_for_plane_grp(struct dpu_fetchunit ***fu,
> > + unsigned int *cnt,
> > + void (*put)(struct dpu_fetchunit *fu))
> > +{
> > + int i;
> > +
> > + for (i = 0; i < *cnt; i++)
> > + (*put)((*fu)[i]);
>
> Same here, you can call put() directly.
Will do.
Thanks,
Liu Ying
next prev parent reply other threads:[~2021-01-26 22:58 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-21 7:14 [PATCH v6 0/6] drm/imx: Introduce i.MX8qm/qxp DPU DRM Liu Ying
2021-01-21 7:14 ` [PATCH v6 1/6] dt-bindings: display: imx: Add i.MX8qxp/qm DPU binding Liu Ying
2021-01-24 22:32 ` Rob Herring
2021-01-21 7:14 ` [PATCH v6 2/6] dt-bindings: display: imx: Add i.MX8qxp/qm PRG binding Liu Ying
2021-01-21 7:14 ` [PATCH v6 3/6] dt-bindings: display: imx: Add i.MX8qxp/qm DPR channel binding Liu Ying
2021-01-21 7:14 ` [PATCH v6 4/6] drm/atomic: Avoid unused-but-set-variable warning on for_each_old_plane_in_state Liu Ying
2021-01-21 7:14 ` [PATCH v6 5/6] drm/imx: Introduce i.MX8qm/qxp DPU DRM Liu Ying
2021-01-25 13:48 ` Laurentiu Palcu
2021-01-26 5:20 ` Liu Ying [this message]
2021-01-21 7:14 ` [PATCH v6 6/6] MAINTAINERS: add maintainer for i.MX8qxp DPU DRM driver Liu Ying
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9233b33c789a2b207a437df9abcff1dd7fd89b16.camel@nxp.com \
--to=victor.liu@nxp.com \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=festevam@gmail.com \
--cc=guido.gunther@puri.sm \
--cc=kernel@pengutronix.de \
--cc=laurentiu.palcu@oss.nxp.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=shawnguo@kernel.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).