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.
