Re: [PATCH v7 2/3] drm: Add support for the LogiCVC display controller

[Sam Ravnborg]

Hi Paul.

A few comments in the following. I did not find time to read all of the
driver.

	Sam

On Mon, Nov 02, 2020 at 04:53:07PM +0100, Paul Kocialkowski wrote:
> Introduces a driver for the LogiCVC display controller, a programmable
> logic controller optimized for use in Xilinx Zynq-7000 SoCs and other
> Xilinx FPGAs. The controller is mostly configured at logic synthesis
> time so only a subset of configuration is left for the driver to
> handle.
> 
> The following features are implemented and tested:
> - LVDS 4-bit interface;
> - RGB565 pixel formats;
> - Multiple layers and hardware composition;
> - Layer-wide alpha mode;
> 
> The following features are implemented but untested:
> - Other RGB pixel formats;
> - Layer framebuffer configuration for version 4;
> - Lowest-layer used as background color;
> - Per-pixel alpha mode.
> 
> The following features are not implemented:
> - YUV pixel formats;
> - DVI, LVDS 3-bit, ITU656 and camera link interfaces;
> - External parallel input for layer;
> - Color-keying;
> - LUT-based alpha modes.
> 
> Additional implementation-specific notes:
> - Panels are only enabled after the first page flip to avoid flashing a
>   white screen.
> - Depth used in context of the LogiCVC driver only counts color components
>   to match the definition of the synthesis parameters.
> 
> Support is implemented for both version 3 and 4 of the controller.
> 
> With version 3, framebuffers are stored in a dedicated contiguous
> memory area, with a base address hardcoded for each layer. This requires
> using a dedicated CMA pool registered at the base address and tweaking a
> few offset-related registers to try to use any buffer allocated from
> the pool. This is done on a best-effort basis to have the hardware cope
> with the DRM framebuffer allocation model and there is no guarantee
> that each buffer allocated by GEM CMA can be used for any layer.
> In particular, buffers allocated below the base address for a layer are
> guaranteed not to be configurable for that layer. See the implementation of
> logicvc_layer_buffer_find_setup for specifics.
> 
> Version 4 allows configuring each buffer address directly, which
> guarantees that any buffer can be configured.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>
> ---
>  MAINTAINERS                                 |   6 +
>  drivers/gpu/drm/Kconfig                     |   2 +
>  drivers/gpu/drm/Makefile                    |   1 +
>  drivers/gpu/drm/logicvc/Kconfig             |   9 +
>  drivers/gpu/drm/logicvc/Makefile            |   4 +
>  drivers/gpu/drm/logicvc/logicvc_crtc.c      | 277 +++++++++
>  drivers/gpu/drm/logicvc/logicvc_crtc.h      |  21 +
>  drivers/gpu/drm/logicvc/logicvc_drm.c       | 472 +++++++++++++++
>  drivers/gpu/drm/logicvc/logicvc_drm.h       |  64 ++
>  drivers/gpu/drm/logicvc/logicvc_interface.c | 224 +++++++
>  drivers/gpu/drm/logicvc/logicvc_interface.h |  30 +
>  drivers/gpu/drm/logicvc/logicvc_layer.c     | 615 ++++++++++++++++++++
>  drivers/gpu/drm/logicvc/logicvc_layer.h     |  64 ++
>  drivers/gpu/drm/logicvc/logicvc_mode.c      | 101 ++++
>  drivers/gpu/drm/logicvc/logicvc_mode.h      |  15 +
>  drivers/gpu/drm/logicvc/logicvc_of.c        | 197 +++++++
>  drivers/gpu/drm/logicvc/logicvc_of.h        |  46 ++
>  drivers/gpu/drm/logicvc/logicvc_regs.h      |  88 +++
>  18 files changed, 2236 insertions(+)
>  create mode 100644 drivers/gpu/drm/logicvc/Kconfig
>  create mode 100644 drivers/gpu/drm/logicvc/Makefile
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_crtc.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_crtc.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_drm.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_drm.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_interface.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_interface.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_layer.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_layer.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_mode.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_mode.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_of.c
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_of.h
>  create mode 100644 drivers/gpu/drm/logicvc/logicvc_regs.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 71e29dc0ab9d..9c4c5edef0ba 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5522,6 +5522,12 @@ S:	Orphan / Obsolete
>  F:	drivers/gpu/drm/i810/
>  F:	include/uapi/drm/i810_drm.h
>  
> +DRM DRIVER FOR LOGICVC DISPLAY CONTROLLER
> +M:	Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +S:	Supported
> +F:	drivers/gpu/drm/logicvc/
> +
>  DRM DRIVER FOR LVDS PANELS
>  M:	Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>  L:	dri-devel@xxxxxxxxxxxxxxxxxxxxx
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 64376dd298ed..7b280056207f 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -352,6 +352,8 @@ source "drivers/gpu/drm/arc/Kconfig"
>  
>  source "drivers/gpu/drm/hisilicon/Kconfig"
>  
> +source "drivers/gpu/drm/logicvc/Kconfig"
> +
>  source "drivers/gpu/drm/mediatek/Kconfig"
>  
>  source "drivers/gpu/drm/zte/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 81569009f884..29fbb7cd9570 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -102,6 +102,7 @@ obj-$(CONFIG_DRM_STM) += stm/
>  obj-$(CONFIG_DRM_STI) += sti/
>  obj-y 			+= imx/
>  obj-$(CONFIG_DRM_INGENIC) += ingenic/
> +obj-$(CONFIG_DRM_LOGICVC) += logicvc/
>  obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>  obj-$(CONFIG_DRM_MESON)	+= meson/
>  obj-y			+= i2c/
> diff --git a/drivers/gpu/drm/logicvc/Kconfig b/drivers/gpu/drm/logicvc/Kconfig
> new file mode 100644
> index 000000000000..300b2be07385
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/Kconfig
> @@ -0,0 +1,9 @@
> +config DRM_LOGICVC
> +	tristate "LogiCVC DRM"
> +	depends on DRM
> +	depends on OF || COMPILE_TEST
> +	select DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
> +	select DRM_GEM_CMA_HELPER
> +	help
> +	  DRM display driver for the logiCVC programmable logic block from Xylon
> diff --git a/drivers/gpu/drm/logicvc/Makefile b/drivers/gpu/drm/logicvc/Makefile
> new file mode 100644
> index 000000000000..c09531fbd6ad
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/Makefile
> @@ -0,0 +1,4 @@
> +logicvc-drm-y += logicvc_crtc.o logicvc_drm.o logicvc_interface.o \
> +		 logicvc_layer.o logicvc_mode.o logicvc_of.o
> +
Even after maintaining kbuild for several years and reading far too many
Makefile I still dislike the use of '\' to break long assignments.

logicvc-drm-y := logicvc_crtc.o
logicvc-drm-y += logicvc_drm.o
logicvc-drm-y += logicvc_interface.o
logicvc-drm-y += logicvc_layer.o
logicvc-drm-y += logicvc_mode.o
logicvc-drm-y += logicvc_of.o

Or if this is too much repeated the shorter:
logicvc-drm-y := logicvc_crtc.o logicvc_drm.o logicvc_interface.o
logicvc-drm-y += logicvc_layer.o logicvc_mode.o logicvc_of.o

Also note that the first stement is an assingnment and not an addition.

It is a personal thing - so feel free to ignore.



> +obj-$(CONFIG_DRM_LOGICVC) += logicvc-drm.o
> diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.c b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> new file mode 100644
> index 000000000000..75e6a47a7724
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
...

 +
> +int logicvc_crtc_init(struct logicvc_drm *logicvc)
> +{
> +	struct drm_device *drm_dev = &logicvc->drm_dev;
> +	struct device *dev = drm_dev->dev;
> +	struct device_node *of_node = dev->of_node;
> +	struct logicvc_crtc *crtc;
> +	struct logicvc_layer *layer_primary;
> +	int ret;
> +
> +	crtc = devm_kzalloc(dev, sizeof(*crtc), GFP_KERNEL);
> +	if (!crtc)
> +		return -ENOMEM;
> +
> +	layer_primary = logicvc_layer_get_primary(logicvc);
> +	if (!layer_primary) {
> +		DRM_ERROR("Failed to get primary layer\n");
> +		return -EINVAL;
> +	}
Please use drm_err(logicvc->drm, "...") and friends all over the file.
DRM_DEV_ERROR() and friends are deprecated.
If you have no drm_device use whatever.