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@xxxxxxx> > > --- > > 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@xxxxxxxxx>. > > * 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
