Hi Laurent,
Thanks for the patch.
On Wed, 2019-09-25 at 16:55:42 -0700, Laurent Pinchart wrote:
> From: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
>
> The Xilinx ZynqMP SoC has a hardened display pipeline named DisplayPort
> Subsystem. It includes a buffer manager, a video pipeline renderer
> (blender), an audio mixer and a DisplayPort source controller
> (transmitter). The DMA engine the provide data to the buffer manager, as
> well as the DisplayPort PHYs that drive the lanes, are external to the
> subsystem and interfaced using the DMA engine and PHY APIs respectively.
>
> This driver supports the DisplayPort Subsystem and implements
>
> - Two planes, for graphics and video
> - One CRTC that supports alpha blending
> - One encoder for the DisplayPort transmitter
> - One connector for an external monitor
>
> It currently doesn't support
>
> - Color keying
> - Test pattern generation
> - Audio
> - Live input from the Programmable Logic (FPGA)
> - Output to the Programmable Logic (FPGA)
>
> Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> Notable changes since v8:
>
> - Rebase on top of v5.3 and update to API changes
> - Cleanup #include statements
> - Use the mode config suspend/resume helpers
> - Remove unused code (macros, fields, functions, ...)
> - Sort headers and forward declarations alphabetically
> - Use more standard DRM/KMS helpers (suspend/resume, fbdev,
> DEFINE_DRM_GEM_CMA_FOPS)
> - Drop support for half-implemented or unsupported features (sound DT
> bindings parsing, interface with PL)
> - Remove the xlnx drm helpers
This will have impact on other downstream drivers, as this is shared layer
in xlnx tree.
> - Remove some module parameters
The default format module parameter is required to change the fbdev
format. The fbdev seems widely used in many applications, especially
along with graphics stack.
> - Reorganize code in sections, with better separation of operations
> - Better separation of the encoder from the CRTC
> - Drop the component framework
> - Various kerneldoc reworks, with renames of structures, fields and
> functions
> - Fix vblank and page flip event handling
> - Remove hack to disable clocks at startup
Hoepfully, it's fixed in firmware. Previously it wasn't guaranteed
that the clock is disabled at bootup, because it was one of design
parameters. Then if the clock rate changed when enabled, it resulted
in incorrect rate.
> - Update copyright headers
> - Move checks from atomic commit time to atomic check time
> - Remove partial support for the TPG
> - Remove partial support for live inputs
> - Remove async update support
All above 3 are actually requested features / fixes.
> - Clean up DP TX initialisation and cleanup
> ---
> MAINTAINERS | 9 +
> drivers/gpu/drm/Kconfig | 2 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/xlnx/Kconfig | 13 +
> drivers/gpu/drm/xlnx/Makefile | 2 +
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 1678 +++++++++++++++++++++++
> drivers/gpu/drm/xlnx/zynqmp_disp.h | 43 +
> drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 201 +++
> drivers/gpu/drm/xlnx/zynqmp_dp.c | 1677 ++++++++++++++++++++++
> drivers/gpu/drm/xlnx/zynqmp_dp.h | 29 +
> drivers/gpu/drm/xlnx/zynqmp_dpsub.c | 331 +++++
> drivers/gpu/drm/xlnx/zynqmp_dpsub.h | 48 +
> 12 files changed, 4034 insertions(+)
> create mode 100644 drivers/gpu/drm/xlnx/Kconfig
> create mode 100644 drivers/gpu/drm/xlnx/Makefile
> create mode 100644 drivers/gpu/drm/xlnx/zynqmp_disp.c
> create mode 100644 drivers/gpu/drm/xlnx/zynqmp_disp.h
> create mode 100644 drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> create mode 100644 drivers/gpu/drm/xlnx/zynqmp_dp.c
[snip]
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f0d2ee35794..eb91cfdac13a 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -120,3 +120,4 @@ obj-$(CONFIG_DRM_LIMA) += lima/
> obj-$(CONFIG_DRM_PANFROST) += panfrost/
> obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
> obj-$(CONFIG_DRM_MCDE) += mcde/
> +obj-y += xlnx/
> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> new file mode 100644
> index 000000000000..aa6cd889bd11
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/Kconfig
> @@ -0,0 +1,13 @@
> +config DRM_ZYNQMP_DPSUB
> + tristate "ZynqMP DisplayPort Controller Driver"
> + depends on ARCH_ZYNQMP || COMPILE_TEST
> + depends on COMMON_CLK && DRM && OF
> + select DMA_ENGINE
> + select DRM_GEM_CMA_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_KMS_HELPER
> + select GENERIC_PHY
Can we enforce the DRM_FBDEV_OVERALLOC=200 for this driver somehow? Or should
it be done in the defconfig? This is required, especially when used by the GPU
driver to swap buffers.
> + help
> + This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> + this option if you have a Xilinx ZynqMP SoC with DisplayPort
> + subsystem.
> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
[snip]
> +
> +/**
> + * zynqmp_disp_avbuf_enable_channels - Enable buffer channels
> + * @avbuf: Audio/video buffer manager
> + *
> + * Enable all (video and audio) buffer channels.
> + */
> +static void zynqmp_disp_avbuf_enable_channels(struct zynqmp_disp_avbuf *avbuf)
> +{
> + unsigned int i;
> + u32 val;
> +
> + val = ZYNQMP_DISP_AV_BUF_CHBUF_EN
> + | (ZYNQMP_DISP_AV_BUF_CHBUF_BURST_LEN_MAX <<
Nit. The line better end with an operator '|'.
> + ZYNQMP_DISP_AV_BUF_CHBUF_BURST_LEN_SHIFT);
> +
> + for (i = 0; i < ZYNQMP_DISP_AV_BUF_NUM_VID_GFX_BUFFERS; i++)
> + zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_CHBUF(i),
> + val);
> +
> + val = ZYNQMP_DISP_AV_BUF_CHBUF_EN
> + | (ZYNQMP_DISP_AV_BUF_CHBUF_BURST_LEN_AUD_MAX <<
Here too.
> + ZYNQMP_DISP_AV_BUF_CHBUF_BURST_LEN_SHIFT);
> +
> + for (; i < ZYNQMP_DISP_AV_BUF_NUM_BUFFERS; i++)
> + zynqmp_disp_avbuf_write(avbuf, ZYNQMP_DISP_AV_BUF_CHBUF(i),
> + val);
> +}
> +
[snip]
> +
> +/* -----------------------------------------------------------------------------
> + * Blender (Video Pipeline)
> + */
> +
> +static void zynqmp_disp_blend_write(struct zynqmp_disp_blend *blend,
> + int reg, u32 val)
> +{
> + writel(val, blend->base + reg);
> +}
> +
> +/*
> + * Colorspace conversion matrices.
> + *
> + * Hardcode RGB <-> YUV conversion to full-range SDTV for now.
> + */
> +static const u16 csc_zero_matrix[] = {
> + 0x0, 0x0, 0x0,
> + 0x0, 0x0, 0x0,
> + 0x0, 0x0, 0x0
> +};
> +
> +static const u16 csc_identity_matrix[] = {
> + 0x1000, 0x0, 0x0,
> + 0x0, 0x1000, 0x0,
> + 0x0, 0x0, 0x1000
> +};
> +
> +static const u32 csc_zero_offsets[] = {
> + 0, 0, 0
> +};
> +
> +static const u16 csc_rgb_to_sdtv_matrix[] = {
> + 0x4c9, 0x864, 0x1d3,
> + 0x7d4d, 0x7ab3, 0x800,
> + 0x800, 0x794d, 0x7eb3
> +};
> +
> +static const u32 csc_rgb_to_sdtv_offsets[] = {
> + 0x0, 0x8000000, 0x8000000
> +};
> +
> +static const u16 csc_sdtv_to_rgb_matrix[] = {
> + 0x1000, 0x166f, 0x0,
> + 0x1000, 0x7483, 0x7a7f,
> + 0x1000, 0x0, 0x1c5a
> +};
> +
> +static const u32 csc_sdtv_to_rgb_offsets[] = {
> + 0x0, 0x1800, 0x1800
> +};
Just a question. These are all programable parameters. If we want to allow users to
change these, what'd be the best way? The drm properties seems getting more strict
controlled, and there are a few more, ex TPG / alpha /,,, that are not included in
this set.
> +
> +/**
> + * zynqmp_disp_blend_set_output_format - Set the output format of the blender
> + * @blend: Blender object
> + * @format: Output format
> + *
> + * Set the output format of the blender to @format.
> + */
> +static void zynqmp_disp_blend_set_output_format(struct zynqmp_disp_blend *blend,
> + enum zynqmp_dpsub_format format)
> +{
> + static const unsigned int blend_output_fmts[] = {
> + [ZYNQMP_DPSUB_FORMAT_RGB] = ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_RGB,
> + [ZYNQMP_DPSUB_FORMAT_YCRCB444] = ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_YCBCR444,
> + [ZYNQMP_DPSUB_FORMAT_YCRCB422] = ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_YCBCR422
> + | ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_EN_DOWNSAMPLE,
> + [ZYNQMP_DPSUB_FORMAT_YONLY] = ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_YONLY,
> + };
> +
> + u32 fmt = blend_output_fmts[format];
[snip]
> +}
> +
> +/**
> + * zynqmp_disp_blend_layer_enable - Enable a layer
> + * @blend: Blender object
> + * @layer: The layer
> + */
> +static void zynqmp_disp_blend_layer_enable(struct zynqmp_disp_blend *blend,
> + struct zynqmp_disp_layer *layer)
> +{
> + const u16 *coeffs;
> + const u32 *offsets;
> + u32 val;
> +
> + val = (layer->drm_fmt->is_yuv ?
> + 0 : ZYNQMP_DISP_V_BLEND_LAYER_CONTROL_RGB)
> + | (layer->drm_fmt->hsub > 1 ?
Ditto for '|'.
> + ZYNQMP_DISP_V_BLEND_LAYER_CONTROL_EN_US : 0);
> +
> + zynqmp_disp_blend_write(blend,
> + ZYNQMP_DISP_V_BLEND_LAYER_CONTROL(layer->id),
[snip]
> +}
> +
> +static int zynqmp_dpsub_drm_init(struct zynqmp_dpsub *dpsub)
> +{
> + struct drm_device *drm;
> + int ret;
> +
> + drm = drm_dev_alloc(&zynqmp_dpsub_drm_driver, dpsub->dev);
> + if (IS_ERR(drm))
> + return PTR_ERR(drm);
> +
> + dpsub->drm = drm;
> + drm->dev_private = dpsub;
> +
> + /* Initialize mode config, vblank and the KMS poll helper. */
> + drm_mode_config_init(drm);
> +
> + drm->mode_config.funcs = &zynqmp_dpsub_mode_config_funcs;
[snip]
> +};
> +
> +module_platform_driver(zynqmp_dpsub_driver);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("ZynqMP DP Subsystem Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_dpsub.h b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
> new file mode 100644
> index 000000000000..d4e0613d2f82
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/zynqmp_dpsub.h
> @@ -0,0 +1,48 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * ZynqMP DPSUB Subsystem Driver
> + *
> + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> + *
> + * Authors:
> + * - Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + * - Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> + */
> +
> +#ifndef _ZYNQMP_DPSUB_H_
> +#define _ZYNQMP_DPSUB_H_
> +
> +struct clk;
> +struct device;
> +struct drm_device;
> +struct zynqmp_disp;
> +struct zynqmp_dp;
> +
> +enum zynqmp_dpsub_format {
> + ZYNQMP_DPSUB_FORMAT_RGB,
> + ZYNQMP_DPSUB_FORMAT_YCRCB444,
> + ZYNQMP_DPSUB_FORMAT_YCRCB422,
> + ZYNQMP_DPSUB_FORMAT_YONLY,
> +};
> +
> +/**
> + * struct zynqmp_dpsub - ZynqMP DisplayPort Subsystem
> + * @drm: The DRM/KMS device
> + * @dev: The physical device
> + * @disp: The display controller
> + * @dp: The DisplayPort controller
> + * @align: Buffer pitch alignment constraint (must be a power of 2)
This is a little misleading. The alingment limitation is from DMA.
The dedicated DMA requires 256 byte alignment, for example.
> + */
> +struct zynqmp_dpsub {
> + struct drm_device *drm;
> + struct device *dev;
> +
> + struct clk *apb_clk;
The description is missing for this one.
> +
> + struct zynqmp_disp *disp;
> + struct zynqmp_dp *dp;
> +
> + unsigned int align;
I don't think this variable is needed, but maybe it's simpler with this.
> +};
I skimmed through, and I only have minor comments as above. Thanks for
clean-up!
As also noted above and briefly discussed offline, since a lot of codes were
trimmed out in this set, it'd be helpful to know how to bring those back
properly, so the upstream one can stabilize in reasonable timeframe. Also,
this is related to how it should be maintained between upstream and xilinx
trees. We can discuss details offline as well.
Thanks,
-hyun
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
