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

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@bootlin.com Reviewed-by: Maxime Ripard mripard@kernel.org

---

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@bootlin.com +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@ideasonboard.com L: dri-devel@lists.freedesktop.org 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@bootlin.com
• */

...

+

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