Hi Sam,
On Wed 04 Nov 20, 22:22, Sam Ravnborg wrote:
> Hi Paul.
>
> A few comments in the following. I did not find time to read all of the
> driver.
Thanks for taking a look at 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.
I agree that it looks ugly and starting with += doesn't really make sense.
But personally, I find that repeating the variable name doesn't look quite
nice either. I think I'll settle for something like:
logicvc-drm-y = logicvc_crtc.o \
logicvc_drm.o \
...
logicvc_of.o
>
> > +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.
Understood, I'll change that in the next revision!
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
