Hi Laurent,
On Fri, 2019-11-08 at 09:13:25 -0800, Laurent Pinchart wrote:
> 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 ?
You are correct. Initially there was requirement to support that, but i don't
see anything coming up that needs multiple CRTCs. So we can put it aside for
now. Meanwhile we can confirm if it has to be really supported even in
the future.
>
> > > - 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 ?
>
As far as I remember, the emulated fbdev format is picked at initiliazation
time based on bpp / depth, and never changes after all. Maybe it's just me
not knowing how to do. The command line mode may work.
> > > - 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.
>
Ah makes sense. I don't know why I didn't think it that way. There may be
some complications to fix in clock driver, such as some clocks shouldn't
be disabled, and it's not easy to which ones? We'll figure it out!
> > > - 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.
In my view, async update is high priority. There are many applications and
use cases relying on it.
>
> > > - 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.
Thanks! I have been thinking it's recommended style, but I guess I was wrong.
Then, I'm fine either way too.
>
> > > + 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.
>
Got it.
> > > +};
> >
> > 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.
Agreed. It just sometimes gets difficult to spend continuous effort, and we
are working to improve. One missing item is the changes related to Xilinx
specific formats. Those can be discussed offline as some other folks may have
better answers on some.
Thanks,
-hyun
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
