Hi Paul.
Good looking driver. Well structured in a number of relevant files.
A few comments in the following.
Some parts I fail to follow - due to my lack of DRM knowledge.
So all in all - only trivial comments.
With these fixed you can add:
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
Sam
On Tue, Dec 03, 2019 at 04:06:05PM +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 needs an entry.
Will this driver be supported in drm-misc?
If yes, then yoy need to apply for write access (if you do not have it).
> diff --git a/drivers/gpu/drm/logicvc/Kconfig b/drivers/gpu/drm/logicvc/Kconfig
> new file mode 100644
> index 000000000000..34dacabbb49a
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/Kconfig
> @@ -0,0 +1,8 @@
> +config DRM_LOGICVC
> + tristate "LogiCVC DRM"
> + depends on DRM
> + 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
The driver, as far as I can tell, required OF.
So a depends on OF seems missing.
> diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.c b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> new file mode 100644
> index 000000000000..2c07f4594c32
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
Empty line between the include blocks.
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_vblank.h>
> +
> +#include "logicvc_crtc.h"
> +#include "logicvc_drm.h"
> +#include "logicvc_interface.h"
> +#include "logicvc_layer.h"
> +#include "logicvc_regs.h"
All sorted within each block - good.
> +
> +#define logicvc_crtc(c) \
> + container_of(c, struct logicvc_crtc, drm_crtc)
> +
> +static int logicvc_crtc_atomic_check(struct drm_crtc *drm_crtc,
> + struct drm_crtc_state *state)
> +{
> + struct drm_display_mode *mode = &state->adjusted_mode;
> +
> + if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +
> +void logicvc_crtc_vblank_handler(struct logicvc_drm *logicvc)
> +{
> + struct logicvc_crtc *crtc = logicvc->crtc;
> + struct drm_device *drm = logicvc->drm;
> + unsigned long flags;
> +
> + if (!crtc)
> + return;
> +
> + drm_crtc_handle_vblank(&crtc->drm_crtc);
> +
> + spin_lock_irqsave(&drm->event_lock, flags);
> + if (crtc->event) {
> + drm_crtc_send_vblank_event(&crtc->drm_crtc, crtc->event);
> + drm_crtc_vblank_put(&crtc->drm_crtc);
> + crtc->event = NULL;
> + }
> + spin_unlock_irqrestore(&drm->event_lock, flags);
In the other cases you have spin_* inside the block.
No nbeed to take the spinlock if crtc->event equals NULL
> +}
> +
> +int logicvc_crtc_init(struct logicvc_drm *logicvc)
> +{
> + struct device_node *of_node = logicvc->drm->dev->of_node;
> + struct logicvc_crtc *crtc;
> + struct logicvc_layer *layer_primary;
> + int ret;
> +
> + crtc = devm_kzalloc(logicvc->drm->dev, sizeof(*crtc), GFP_KERNEL);
> + if (!crtc)
> + return -ENOMEM;
> +
> + crtc->logicvc = logicvc;
> +
> + layer_primary = logicvc_layer_get_primary(logicvc);
> + if (!layer_primary) {
> + DRM_ERROR("Failed to get primary layer\n");
Please use drm_err(logicvc->drm, "...");
This goes for all uses of DRM_ where you have a drm_device available.
> + return -EINVAL;
> + }
> +
> + ret = drm_crtc_init_with_planes(logicvc->drm, &crtc->drm_crtc,
> + &layer_primary->drm_plane, NULL,
> + &logicvc_crtc_funcs, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to initalize CRTC\n");
> + return ret;
> + }
> +
> + drm_crtc_helper_add(&crtc->drm_crtc, &logicvc_crtc_helper_funcs);
> +
> + crtc->drm_crtc.port = of_graph_get_port_by_id(of_node, 1);
> +
> + logicvc->crtc = crtc;
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/logicvc/logicvc_crtc.h b/drivers/gpu/drm/logicvc/logicvc_crtc.h
> new file mode 100644
> index 000000000000..dfa55c377ac3
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_crtc.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#ifndef _LOGICVC_CRTC_H_
> +#define _LOGICVC_CRTC_H_
> +
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_vblank.h>
Use a forward declaration of drm_pending_vblank_event,
then you can drop the above include.
> +
> +struct logicvc_drm;
> +
> +struct logicvc_crtc {
> + struct logicvc_drm *logicvc;
> +
> + struct drm_crtc drm_crtc;
> + struct drm_pending_vblank_event *event;
> +};
> +
> +void logicvc_crtc_vblank_handler(struct logicvc_drm *logicvc);
> +int logicvc_crtc_init(struct logicvc_drm *logicvc);
> +
> +#endif
> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
> new file mode 100644
> index 000000000000..c9a1a6b38692
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
> @@ -0,0 +1,467 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <drm/drm_atomic_helper.h>
Add empty lines between include blocks (one line up)..
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_print.h>
> +
> +#include "logicvc_crtc.h"
> +#include "logicvc_drm.h"
> +#include "logicvc_interface.h"
> +#include "logicvc_mode.h"
> +#include "logicvc_layer.h"
> +#include "logicvc_of.h"
> +#include "logicvc_regs.h"
> +
> +DEFINE_DRM_GEM_CMA_FOPS(logicvc_drm_fops);
> +
> +static int logicvc_drm_gem_cma_dumb_create(struct drm_file *file_priv,
> + struct drm_device *drm,
> + struct drm_mode_create_dumb *args)
> +{
> + struct logicvc_drm *logicvc = drm->dev_private;
> +
> + /* Stride is always fixed to its configuration value. */
> + args->pitch = logicvc->config.row_stride * DIV_ROUND_UP(args->bpp, 8);
> +
> + return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
> +}
> +
> +static struct drm_driver logicvc_drm_driver = {
> + .driver_features = DRIVER_GEM | DRIVER_MODESET |
> + DRIVER_ATOMIC,
> +
> + .fops = &logicvc_drm_fops,
> + .name = "logicvc-drm",
> + .desc = "Xylon LogiCVC DRM driver",
> + .date = "20190319",
2020?
> + .major = 1,
> + .minor = 0,
> +
> + DRM_GEM_CMA_VMAP_DRIVER_OPS,
> + .dumb_create = logicvc_drm_gem_cma_dumb_create,
> +};
> +
> new file mode 100644
> index 000000000000..cc728d5a4aee
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#ifndef _LOGICVC_DRM_H_
> +#define _LOGICVC_DRM_H_
> +
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <drm/drm_device.h>
> +
> +#define LOGICVC_DISPLAY_INTERFACE_RGB 0
> +#define LOGICVC_DISPLAY_INTERFACE_ITU656 1
> +#define LOGICVC_DISPLAY_INTERFACE_LVDS_4BITS 2
> +#define LOGICVC_DISPLAY_INTERFACE_LVDS_4BITS_CAMERA 3
> +#define LOGICVC_DISPLAY_INTERFACE_LVDS_3BITS 4
> +#define LOGICVC_DISPLAY_INTERFACE_DVI 5
> +
> +#define LOGICVC_DISPLAY_COLORSPACE_RGB 0
> +#define LOGICVC_DISPLAY_COLORSPACE_YUV422 1
> +#define LOGICVC_DISPLAY_COLORSPACE_YUV444 2
> +
> +struct logicvc_crtc;
> +struct logicvc_interface;
> +
> +struct logicvc_drm_config {
> + u32 display_interface;
> + u32 display_colorspace;
> + u32 display_depth;
> + u32 row_stride;
> + bool dithering;
> + bool background_layer;
> + bool layers_configurable;
> + u32 layers_count;
> +};
> +
> +struct logicvc_drm_caps {
> + bool layer_address;
> +};
> +
> +struct logicvc_drm {
> + const struct logicvc_drm_caps *caps;
> + struct logicvc_drm_config config;
> + struct drm_device *drm;
Modern drm drivers are expected to embed drm_device.
See example in drm_drv.c
> + phys_addr_t reserved_mem_base;
> + struct regmap *regmap;
> +
> + struct clk *vclk;
> + struct clk *vclk2;
> + struct clk *lvdsclk;
> + struct clk *lvdsclkn;
> +
> + struct list_head layers_list;
> + struct logicvc_crtc *crtc;
> + struct logicvc_interface *interface;
> +};
> +
> +#endif
> diff --git a/drivers/gpu/drm/logicvc/logicvc_interface.c b/drivers/gpu/drm/logicvc/logicvc_interface.c
> new file mode 100644
> index 000000000000..792b1fc2a5f7
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_interface.c
> @@ -0,0 +1,235 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#include <linux/types.h>
> +#include <drm/drm_atomic_helper.h>
Empty line
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "logicvc_crtc.h"
> +#include "logicvc_drm.h"
> +#include "logicvc_interface.h"
> +#include "logicvc_regs.h"
> +
> +static int logicvc_connector_get_modes(struct drm_connector *drm_connector)
> +{
> + struct logicvc_interface *interface =
> + logicvc_interface_from_drm_connector(drm_connector);
> +
> + if (interface->drm_panel)
> + return drm_panel_get_modes(interface->drm_panel);
In drm-misc-next this call takes a connector argument.
> +
> + return 0;
> +}
> +
> +static const struct drm_connector_helper_funcs logicvc_connector_helper_funcs = {
> + .get_modes = logicvc_connector_get_modes,
> +};
> +
> +static void logicvc_connector_destroy(struct drm_connector *drm_connector)
> +{
> + struct logicvc_interface *interface =
> + logicvc_interface_from_drm_connector(drm_connector);
> +
> + if (interface->drm_panel)
> + drm_panel_detach(interface->drm_panel);
> +
> + drm_connector_cleanup(drm_connector);
> +}
> +
> +static const struct drm_connector_funcs logicvc_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = logicvc_connector_destroy,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +int logicvc_interface_init(struct logicvc_drm *logicvc)
> +{
> + struct logicvc_interface *interface;
> + struct device_node *of_node = logicvc->drm->dev->of_node;
> + int encoder_type = logicvc_interface_encoder_type(logicvc);
> + int connector_type = logicvc_interface_connector_type(logicvc);
The connector is today part of drm_panel, and should not be decided by
the display driver.
> + bool native_connector = logicvc_interface_native_connector(logicvc);
> + int ret;
> +
> + interface = devm_kzalloc(logicvc->drm->dev, sizeof(*interface),
> + GFP_KERNEL);
> + if (!interface) {
> + ret = -ENOMEM;
> + goto error_early;
> + }
> +
> + interface->logicvc = logicvc;
> +
> + ret = drm_of_find_panel_or_bridge(of_node, 1, 0, &interface->drm_panel,
> + &interface->drm_bridge);
> + if (ret == -EPROBE_DEFER)
> + goto error_early;
> +
> + ret = drm_encoder_init(logicvc->drm, &interface->drm_encoder,
> + &logicvc_encoder_funcs, encoder_type, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to initalize encoder\n");
> + goto error_early;
> + }
> +
> + drm_encoder_helper_add(&interface->drm_encoder,
> + &logicvc_encoder_helper_funcs);
> +
> + if (native_connector || interface->drm_panel) {
> + ret = drm_connector_init(logicvc->drm,
> + &interface->drm_connector,
> + &logicvc_connector_funcs,
> + connector_type);
> + if (ret) {
> + DRM_ERROR("Failed to initalize connector\n");
> + goto error_encoder;
> + }
> +
> + drm_connector_helper_add(&interface->drm_connector,
> + &logicvc_connector_helper_funcs);
> +
> + ret = drm_connector_attach_encoder(&interface->drm_connector,
> + &interface->drm_encoder);
> + if (ret) {
> + DRM_ERROR("Failed to attach connector to encoder\n");
> + goto error_encoder;
> + }
> + }
> +
> + if (interface->drm_panel) {
> + ret = drm_panel_attach(interface->drm_panel,
> + &interface->drm_connector);
> + if (ret) {
> + DRM_ERROR("Failed to attach panel to connector\n");
> + goto error_encoder;
> + }
> + }
> +
> + if (interface->drm_bridge) {
> + ret = drm_bridge_attach(&interface->drm_encoder,
> + interface->drm_bridge, NULL);
> + if (ret) {
> + DRM_ERROR("Failed to attach bridge to encoder\n");
> + goto error_encoder;
> + }
> + }
> +
> + logicvc->interface = interface;
> +
> + return 0;
> +
> +error_encoder:
> + drm_encoder_cleanup(&interface->drm_encoder);
> +
> +error_early:
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/logicvc/logicvc_layer.c b/drivers/gpu/drm/logicvc/logicvc_layer.c
> new file mode 100644
> index 000000000000..c7b092d592ad
> --- /dev/null
> +++ b/drivers/gpu/drm/logicvc/logicvc_layer.c
> @@ -0,0 +1,594 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Bootlin
> + * Author: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/types.h>
> +#include <drm/drm_atomic.h>
Empty lines between blocks of include
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_plane.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_print.h>
> +
> +#include "logicvc_crtc.h"
> +#include "logicvc_drm.h"
> +#include "logicvc_layer.h"
> +#include "logicvc_of.h"
> +#include "logicvc_regs.h"
> +
