Re: [PATCH v6 5/6] drm/imx: Introduce i.MX8qm/qxp DPU DRM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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]'.

> +		}
> +	}
> +
> +	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,
	...
};

[...]

> +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.

> +		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.

> +
> +	*cnt = 0;
> +}

Thanks,
laurentiu
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux