Hi Hyun,
On Thursday, 22 February 2018 04:50:42 EET Hyun Kwon wrote:
> On Wed, 2018-02-21 at 15:17:25 -0800, Laurent Pinchart wrote:
> > On Wednesday, 7 February 2018 03:36:36 EET Hyun Kwon wrote:
> >> Xilinx has various platforms for display, where users can create
> >> using multiple IPs in the programmable FPGA fabric, or where
> >> some hardened piepline is available on the chip. Furthermore,
> >
> > s/piepline/pipeline/
> >
> >> hardened pipeline can also interact with soft logics in FPGA.
> >>
> >> The Xilinx DRM KMS module is to integrate multiple subdevices and
> >> to represent the entire pipeline as a single DRM device. The module
> >> includes helper (ex, framebuffer and gem helpers) and
> >> glue logic (ex, crtc interface) functions.
> >>
> >> Signed-off-by: Hyun Kwon <hyun.k...@xilinx.com>
> >> Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> >> ---
> >> v5
> >> - Redefine xlnx_pipeline_init()
> >> v4
> >> - Fix a bug in of graph binding handling
> >> - Remove vblank callbacks from xlnx_crtc
> >> - Remove the dt binding. This module becomes more like a library.
> >> - Rephrase the commit message
> >> v3
> >> - Add Laurent as a maintainer
> >> - Fix multiple-reference on gem objects
> >> v2
> >> - Change the SPDX identifier format
> >> - Merge patches(crtc, gem, fb) into single one
> >> v2 of xlnx_drv
> >> - Rename kms to display in xlnx_drv
> >> - Replace some xlnx specific fb helper with common helpers in xlnx_drv
> >> - Don't set the commit tail callback in xlnx_drv
> >> - Support 'ports' graph binding in xlnx_drv
> >> v2 of xlnx_fb
> >> - Remove wrappers in xlnx_fb
> >> - Replace some functions with drm core helpers in xlnx_fb
> >> ---
> >> ---
> >>
> >> MAINTAINERS | 9 +
> >> drivers/gpu/drm/Kconfig | 2 +
> >> drivers/gpu/drm/Makefile | 1 +
> >> drivers/gpu/drm/xlnx/Kconfig | 12 +
> >> drivers/gpu/drm/xlnx/Makefile | 2 +
> >> drivers/gpu/drm/xlnx/xlnx_crtc.c | 177 ++++++++++++++
> >> drivers/gpu/drm/xlnx/xlnx_crtc.h | 70 ++++++
> >> drivers/gpu/drm/xlnx/xlnx_drv.c | 501 +++++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/xlnx/xlnx_drv.h | 33 +++
> >> drivers/gpu/drm/xlnx/xlnx_fb.c | 298 +++++++++++++++++++++++
> >> drivers/gpu/drm/xlnx/xlnx_fb.h | 33 +++
> >> drivers/gpu/drm/xlnx/xlnx_gem.c | 47 ++++
> >> drivers/gpu/drm/xlnx/xlnx_gem.h | 26 ++
> >> 13 files changed, 1211 insertions(+)
> >> create mode 100644 drivers/gpu/drm/xlnx/Kconfig
> >> create mode 100644 drivers/gpu/drm/xlnx/Makefile
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_crtc.c
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_crtc.h
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_drv.c
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_drv.h
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_fb.c
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_fb.h
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_gem.c
> >> create mode 100644 drivers/gpu/drm/xlnx/xlnx_gem.h
[snip]
> >> diff --git a/drivers/gpu/drm/xlnx/xlnx_crtc.c
> >> b/drivers/gpu/drm/xlnx/xlnx_crtc.c new file mode 100644
> >> index 0000000..de83905
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xlnx/xlnx_crtc.c
[snip]
> >> +uint32_t xlnx_crtc_helper_get_format(struct xlnx_crtc_helper *helper)
> >
> > You can use the u32 type within the kernel.
> >
> >> +{
> >> + struct xlnx_crtc *crtc;
> >> + u32 format = 0, tmp;
> >> +
> >> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> >> + if (crtc->get_format) {
> >> + tmp = crtc->get_format(crtc);
> >> + if (format && format != tmp)
> >> + return 0;
> >> + format = tmp;
> >
> > Same comments regarding the tmp variable and the list locking issue.
> >
> >> + }
> >> + }
> >> +
> >> + return format;
> >
> > Does this mean that your CRTCs support a single format each only ?
Does it ? :-)
> >> +}
[snip]
> >> diff --git a/drivers/gpu/drm/xlnx/xlnx_drv.c
> >> b/drivers/gpu/drm/xlnx/xlnx_drv.c new file mode 100644
> >> index 0000000..8f0e357
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xlnx/xlnx_drv.c
[snip]
> >> +static int xlnx_of_component_probe(struct device *master_dev,
> >> + int (*compare_of)(struct device *, void *),
> >> + const struct component_master_ops *m_ops)
> >
> > As there's a single user of this function, do you need to pass compare_of
> > as a parameter ? Couldn't you move xlnx_compare_of() above and use it
> > directly ?
> >
> >> +{
> >> + struct device *dev = master_dev->parent;
> >> + struct device_node *ep, *port, *remote, *parent;
> >> + struct component_match *match = NULL;
> >> + int i;
> >
> > i is never negative, you can make it unsigned.
> >
> >> + if (!dev->of_node)
> >> + return -EINVAL;
> >> +
> >> + component_match_add(master_dev, &match, compare_of, dev->of_node);
> >> +
> >> + for (i = 0; ; i++) {
> >> + port = of_parse_phandle(dev->of_node, "ports", i);
> >
> > I don't see the ports property documented in the DT bindings in patch 2/5.
> > I assume that's because it has been removed in v4 as stated in the
> > changelog :-) You shouldn't make use of it then. I've skipped reviewing
> > the logic for the rest of this function as I need to see the
> > corresponding DT bindings.
>
> Good catch! I needed some discussion on this. :-) This logic is actually
> intended for future extention and used for pipelines with multiple
> components (ex, this driver with soft IPs, or multiple soft IPs), where the
> dt describes its own topology of the pipeline. Some part of this logic is
> actively used with downstream drivers, but I can simplify this logic
> (remove logic not needed by this set) and revert back when it's actually
> needed.
>
> Then as you commented, the dt binding for this module is gone, so the
> behavior of this logic is documented in the function description, treating
> the function as a helper. So my expectation is that the corresponding
> driver that calls this function should document the dt binding in its own.
> And the DP driver in this set doesn't support such binding without any soft
> IP drivers, thus it doesn't document it as of now.
I suspected so :-) I think the topology should indeed be described in DT in
that case. However, looking at the code (as that's my only source of
information given the lack of DT bindings documentation), it seems that you
plan to use a ports property that contains a list of phandles to components. I
believe we should instead use the OF graph bindings to model the pipeline in
DT, which would require rewriting this function. If you want to merge this
series with DT bindings for more complex pipelines I would thus drop this
code.
I have related comments on the ZynqMP DP subsystem bindings, I'll reply to
patch 2/5.
> >> + if (!port)
> >> + break;
> >> +
> >> + parent = port->parent;
> >> + if (!of_node_cmp(parent->name, "ports"))
> >> + parent = parent->parent;
> >> + parent = of_node_get(parent);
> >
> > I've recently found out that dereferencing a device_node parent directly
> > is prone to race conditions. You should use of_get_parent() instead (or
> > of_get_next_parent() as appropriate to make device_node reference handling
> > easier).
> >
> >> +
> >> + if (!of_device_is_available(parent)) {
> >> + of_node_put(parent);
> >> + of_node_put(port);
> >> + continue;
> >> + }
> >> +
> >> + component_match_add(master_dev, &match, compare_of, parent);
> >> + of_node_put(parent);
> >> + of_node_put(port);
> >> + }
> >> +
> >> + parent = dev->of_node;
> >> + for (i = 0; ; i++) {
> >> + parent = of_node_get(parent);
> >> + if (!of_device_is_available(parent)) {
> >> + of_node_put(parent);
> >> + continue;
> >> + }
> >> +
> >> + for_each_endpoint_of_node(parent, ep) {
> >> + remote = of_graph_get_remote_port_parent(ep);
> >> + if (!remote || !of_device_is_available(remote) ||
> >> + remote == dev->of_node) {
> >> + of_node_put(remote);
> >> + continue;
> >> + } else if (!of_device_is_available(remote->parent)) {
> >> + dev_warn(dev, "parent dev of %s unavailable\n",
> >> + remote->full_name);
> >> + of_node_put(remote);
> >> + continue;
> >> + }
> >> + component_match_add(master_dev, &match, compare_of,
> >> + remote);
> >> + of_node_put(remote);
> >> + }
> >> + of_node_put(parent);
> >> +
> >> + port = of_parse_phandle(dev->of_node, "ports", i);
> >> + if (!port)
> >> + break;
> >> +
> >> + parent = port->parent;
> >> + if (!of_node_cmp(parent->name, "ports"))
> >> + parent = parent->parent;
> >> + of_node_put(port);
> >> + }
> >> +
> >> + return component_master_add_with_match(master_dev, m_ops, match);
> >> +}
[snip]
> >> +/* bitmap for master id */
> >> +static u32 xlnx_master_ids = GENMASK(31, 0);
> >> +
> >> +/**
> >> + * xilinx_drm_pipeline_init - Initialize the drm pipeline for the
> >> device
> >> + * @pdev: The platform device to initialize the drm pipeline device
> >> + *
> >> + * This function initializes the drm pipeline device, struct
> >> drm_device,
> >> + * on @pdev by creating a logical master platform device. The logical
> >> platform
> >> + * device acts as a master device to bind slave devices and represents
> >> + * the entire pipeline.
> >
> > I'm sorry to ask this question so late, but couldn't you do without that
> > platform device ? I don't really see what it brings you.
>
> It is fine without this in current set where there is only one component,
> but it doesn't scale with complex pipelines where soft and hardened IP
> drivers can be mixed. There can be cases where multiple master devices
> exist in a single pipeline, and this is intended to unifiy and simplify the
> model by creating a logical master and allowing all drivers to be bound as
> regular components.
In order to understand how you intend this to work with more complex
pipelines, could you give me a real life example that can serve as a basis for
discussions ?
> >> + * The logical master uses the port bindings of the calling device to
> >> + * figure out the pipeline topology.
> >> + *
> >> + * Return: the logical master platform device if the drm device is
> >> initialized
> >> + * on @pdev. Error code otherwise.
> >> + */
> >> +struct platform_device *xlnx_drm_pipeline_init(struct platform_device
> >> *pdev)
> >> +{
> >> + struct platform_device *master;
> >> + int id, ret;
> >> +
> >
> > Is there any risk of a race between concurrent calls to this function, or
> > to this function and xlnx_drm_pipeline_exit() ?
> >
> >> + id = ffs(xlnx_master_ids);
> >> + if (!id)
> >> + return ERR_PTR(-ENOSPC);
> >> +
> >> + master = platform_device_alloc("xlnx-drm", id - 1);
> >> + if (!master)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + master->dev.parent = &pdev->dev;
> >> + ret = platform_device_add(master);
> >> + if (ret)
> >> + goto err_out;
> >
> > As this is the only location where you jump to the error path I would
> > inline it.
> >
> >> +
> >> + WARN_ON(master->id != id - 1);
> >> + xlnx_master_ids &= ~BIT(master->id);
> >> + return master;
> >> +
> >> +err_out:
> >> + platform_device_unregister(master);
> >> + return ERR_PTR(ret);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xlnx_drm_pipeline_init);
> >> +
> >> +/**
> >> + * xilinx_drm_pipeline_exit - Release the drm pipeline for the device
> >> + * @master: The master pipeline device to release
> >> + *
> >> + * Release the logical pipeline device returned by
> >> xlnx_drm_pipeline_init().
> >> + */
> >> +void xlnx_drm_pipeline_exit(struct platform_device *master)
> >> +{
> >> + xlnx_master_ids |= BIT(master->id);
> >> + platform_device_unregister(master);
> >> +}
> >> +EXPORT_SYMBOL_GPL(xlnx_drm_pipeline_exit);
[snip]
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
