Hi Hyun,
(CC'ing Daniel, with a question for him below)
On Fri, Sep 27, 2019 at 05:04:57PM -0700, Hyun Kwon wrote:
> 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.
Right. We may not want to drop them completely, but I don't think
introducing them for this driver is the right thing to do. It's usually
better to start with two drivers and create helpers out of them instead
of creating helpers up-front and making the drivers fit them.
I can certainly help with the helpers and the other drivers that use
them, but based on what I've seen, there's no code that exposes multiple
CRTCs at the moment, so helpers for this are likely not needed. Or did I
miss something ?
> > - 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.
Doesn't fbdev always perform a mode set, and thus override this ?
> > - 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.
Even if the firmware handles this incorrectly, I think the workaround
should be in the clock driver, not in the dpsub driver. I can try to
handle this on top of the dpsub series. It will require moving to
programmable clocks first though, the upstream ZynqMP DT sources use
fixed clocks.
> > - 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.
I think we should have TPG and live inputs support, but it was only half
implemented. It's best, I believe, given how large the driver already
is, to drop the half implementation now and then add a full
implementation on top. If anyone can provide me with guidance regarding
priorities between those items, I can start working on them. For live
inputs and async updates I would also need to know about the use cases.
> > - 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.
I think it will have to go in a defconfig, as far as I know there's no
way to do a select DRM_FBDEV_OVERALLOC=200
> > + 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 '|'.
I like it better with | at the beginning of the line :-) As this is a
matter of style, and you authored the driver, I will change it.
> > + 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.
There are color space conversion properties, I think they would be a
good match for this. If properties are missing we need to add them, but
in that case we will also need to show a userspace implementation that
makes use of them. TPG may be a bit of a grey area, as I'm not sure in
what userspace it could fit. Possibly igt ? Daniel, what's your opinion
about test pattern generator properties ? Should we perhaps expose them
through sysfs instead, similarly to how CRC works ?
> > +
> > +/**
> > + * 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.
You're right, I'll rephrase that as "DMA alignment constraint (must be a
power of 2)" and will rename the field to dma_align.
> > + */
> > +struct zynqmp_dpsub {
> > + struct drm_device *drm;
> > + struct device *dev;
> > +
> > + struct clk *apb_clk;
>
> The description is missing for this one.
Oops, I'll fix that.
> > +
> > + 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.
The alignment constraint needs to be enforced in
zynqmp_dpsub_dumb_create() and zynqmp_dpsub_fb_create(), implemented in
zynqmp_dpsub.c, which models the top-level DRM/KMS device. The value is
retrieved from the DPDMA engine, only visible to zynqmp_disp.c, which
models the display controller part of the subsystem (and thus the
planes, CRTC, encoder and connector). This is why I've added this field
here, to communicate between the two parts.
This being said, maybe we should merge zynqmp_dpsub.c and zynqmp_disp.c,
and merge struct zynqmp_disp with struct zynqmp_dpsub. Doing so would, I
think, simplify the code a little bit by removing a logical split that
doesn't really match the device. If you agree with this, then the align
member could be removed. I would however prefer doing so on top of this
series.
> > +};
>
> I skimmed through, and I only have minor comments as above. Thanks for
> clean-up!
You're welcome :-)
> 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.
Ideally there should be no difference between upstream and the Xilinx
trees :-) An upstream-first approach makes those issues go away. Of
course that may conflict with Xilinx's schedule, so some middle ground
usually needs to be found.
As noted above, I'd like to discuss priorities and use cases for the
missing features, and I will then work on them.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
