Message-ID: <20200104192026.GA21210@ravnborg.org>
Date:   Sat, 4 Jan 2020 20:20:26 +0100
From:   Sam Ravnborg <sam@...nborg.org>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>
Cc:     dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        David Airlie <airlied@...ux.ie>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH v4 2/3] drm: Add support for the LogiCVC display
 controller

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@...nborg.org>

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

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@...tlin.com>
> + */
> +
> +#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@...tlin.com>
> + */
> +
> +#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@...tlin.com>
> + */
> +
> +#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@...tlin.com>
> + */
> +
> +#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@...tlin.com>
> + */
> +
> +#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@...tlin.com>
> + */
> +
> +#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"
> +

Powered by blists - more mailing lists