Hi Hyun
Thank you for the patch.
On Sat, Jul 07, 2018 at 07:05:34PM -0700, 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 pipeline is available on the chip. Furthermore,
> 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.kwon@xxxxxxxxxx>
> Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> v7
> - Unbind as component in shutdown
> - Register release callback for master device
> v6
> - Fix the function desc for pipeline calls
> - Rebase on drm-misc-next
> - Fix typos in documentation
> - Match types for return and internal variables
> - Remove use of 'tmp' variables
> - Protect any crtc list iteration with mutex
> - Remove a check for drm device in crtc unregistration
> - Split the crtc ops as a separate struct to constify
> - Embed xlnx_crtc_helper into xlnx_drm
> - Move to_xlnx_crtc macro close to xlnx_crtc
> - Remove unneeded include
> - Replace custom vres module param with CONFIG_DRM_FBDEV_OVERALLOC
> - Rename crtc to crtc_helper to make it clearer
> - Use 'DRM device' instead of 'DRM core'
> - Remove unused function, xlnx_get_format()
> - Use device instead of platform device for the logical master
> - Inline xlnx_of_component_probe()
> - Use of_get_parent()
> - Remove the port binding handling in the driver
> - Do complete forward-declarations in headers
> - Constify all function pointers
> - Use the default ioctl from fb helper
> - Return the minimum pitch always
> - Clean up the duplicate license paragraphs
> - Get common bits for dma mask instead of minimum value
> - Remove dummy function declaration from header
> - Fix a typo in the commit message with some re-organization
> 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 | 142 +++++++++++++
> drivers/gpu/drm/xlnx/xlnx_crtc.h | 83 ++++++++
> drivers/gpu/drm/xlnx/xlnx_drv.c | 433 +++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/xlnx/xlnx_drv.h | 23 +++
> drivers/gpu/drm/xlnx/xlnx_fb.c | 249 ++++++++++++++++++++++
> drivers/gpu/drm/xlnx/xlnx_fb.h | 27 +++
> drivers/gpu/drm/xlnx/xlnx_gem.c | 36 ++++
> drivers/gpu/drm/xlnx/xlnx_gem.h | 21 ++
> 13 files changed, 1040 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..06f1d70
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_crtc.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM crtc driver
> + *
> + * Copyright (C) 2017 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + */
> +
> +#include <drm/drmP.h>
drmP.h is deprecated, you should instead include the broken-down DRM
headers (and the required other Linux headers). This holds true for the
other files in this series, and for all the other patches.
> +#include "xlnx_crtc.h"
> +#include "xlnx_drv.h"
> +
> +/*
> + * Overview
> + * --------
> + *
> + * The Xilinx CRTC layer is to enable the custom interface to CRTC drivers.
> + * The interface is used by Xilinx DRM driver where it needs CRTC
> + * functionality. CRTC drivers should attach the desired callbacks
> + * to struct xlnx_crtc and register the xlnx_crtc with corresponding
> + * drm_device. It's highly recommended CRTC drivers register all callbacks
> + * even though many of them are optional.
> + * The CRTC helper simply walks through the registered CRTC device,
> + * and calls the callbacks.
> + */
> +
> +unsigned int xlnx_crtc_helper_get_align(struct xlnx_crtc_helper *helper)
How about renaming get_align to get_alignment, here and everywhere else ?
> +{
> + struct xlnx_crtc *crtc;
> + unsigned int align = 1;
> +
> + mutex_lock(&helper->lock);
> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> + if (crtc->ops->get_align)
> + align = ALIGN(align, crtc->ops->get_align(crtc));
> + }
> + mutex_unlock(&helper->lock);
> +
> + return align;
> +}
> +
> +u64 xlnx_crtc_helper_get_dma_mask(struct xlnx_crtc_helper *helper)
> +{
> + struct xlnx_crtc *crtc;
> + u64 mask = DMA_BIT_MASK(sizeof(dma_addr_t) * 8);
> +
> + mutex_lock(&helper->lock);
> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> + if (crtc->ops->get_dma_mask)
> + mask &= crtc->ops->get_dma_mask(crtc);
> + }
> + mutex_unlock(&helper->lock);
> +
> + return mask;
> +}
> +
> +int xlnx_crtc_helper_get_max_width(struct xlnx_crtc_helper *helper)
Would it make sense to merge the get_max_width and get_max_height
functions into a get_max_size function, given that they are both used in
a single location ?
> +{
> + struct xlnx_crtc *crtc;
> + int width = S32_MAX;
> +
> + mutex_lock(&helper->lock);
> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> + if (crtc->ops->get_max_width)
> + width = min(width, crtc->ops->get_max_width(crtc));
> + }
> + mutex_unlock(&helper->lock);
> +
> + return width;
> +}
> +
> +int xlnx_crtc_helper_get_max_height(struct xlnx_crtc_helper *helper)
> +{
> + struct xlnx_crtc *crtc;
> + int height = S32_MAX;
> +
> + mutex_lock(&helper->lock);
> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> + if (crtc->ops->get_max_height)
> + height = min(height, crtc->ops->get_max_height(crtc));
> + }
> + mutex_unlock(&helper->lock);
> +
> + return height;
> +}
> +
> +u32 xlnx_crtc_helper_get_format(struct xlnx_crtc_helper *helper)
> +{
> + struct xlnx_crtc *crtc;
> + u32 result = 0, format;
> +
> + mutex_lock(&helper->lock);
> + list_for_each_entry(crtc, &helper->xlnx_crtcs, list) {
> + if (crtc->ops->get_format) {
> + format = crtc->ops->get_format(crtc);
> + if (result && result != format) {
> + mutex_unlock(&helper->lock);
> + return 0;
> + }
> + result = format;
> + }
> + }
> + mutex_unlock(&helper->lock);
> +
> + return result;
> +}
> +
> +void xlnx_crtc_helper_init(struct xlnx_crtc_helper *helper)
> +{
> + INIT_LIST_HEAD(&helper->xlnx_crtcs);
> + mutex_init(&helper->lock);
> +}
> +
> +void xlnx_crtc_helper_fini(struct xlnx_crtc_helper *helper)
> +{
> + if (WARN_ON(!list_empty(&helper->xlnx_crtcs)))
> + return;
I wouldn't return early, we're likely screwed anyway if this happens, so
I think you can as well destroy the mutex.
> +
> + mutex_destroy(&helper->lock);
> +}
> +
> +void xlnx_crtc_register(struct drm_device *drm, struct xlnx_crtc *crtc)
> +{
> + struct xlnx_crtc_helper *helper = xlnx_get_crtc_helper(drm);
Do we really need xlnx_get_crtc_helper() ? Can't this function access
xlnx_drm directly ? I understand you may not want to expose the
internals of xlnx_drm to the xlnx_crtc implementations, but I don't see
a reason not to expose it to xlnx_crtc.c.
> +
> + mutex_lock(&helper->lock);
> + list_add_tail(&crtc->list, &helper->xlnx_crtcs);
> + mutex_unlock(&helper->lock);
> +}
> +EXPORT_SYMBOL_GPL(xlnx_crtc_register);
> +
> +void xlnx_crtc_unregister(struct drm_device *drm, struct xlnx_crtc *crtc)
> +{
> + struct xlnx_crtc_helper *helper = xlnx_get_crtc_helper(drm);
> +
> + mutex_lock(&helper->lock);
> + list_del(&crtc->list);
> + mutex_unlock(&helper->lock);
> +}
> +EXPORT_SYMBOL_GPL(xlnx_crtc_unregister);
> diff --git a/drivers/gpu/drm/xlnx/xlnx_crtc.h b/drivers/gpu/drm/xlnx/xlnx_crtc.h
> new file mode 100644
> index 0000000..a6b5f59
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_crtc.h
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM crtc header
> + *
> + * Copyright (C) 2017 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + */
> +
> +#ifndef _XLNX_CRTC_H_
> +#define _XLNX_CRTC_H_
> +
> +#include <drm/drm_crtc.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +struct drm_device;
> +struct xlnx_crtc;
> +
> +/**
> + * struct xlnx_crtc_ops - Xilinx CRTC operations
> + * @get_align: Get the alignment requirement of CRTC device
> + * @get_dma_mask: Get the dma mask of CRTC device
> + * @get_max_width: Get the maximum supported width
> + * @get_max_height: Get the maximum supported height
> + * @get_format: Get the current format of CRTC device
"current format" seems to imply that the format can change, while I
don't think you support this. Is that correct ? If so I would make that
clear in the field description.
> + */
> +struct xlnx_crtc_ops {
> + unsigned int (*get_align)(struct xlnx_crtc *crtc);
> + u64 (*get_dma_mask)(struct xlnx_crtc *crtc);
> + int (*get_max_width)(struct xlnx_crtc *crtc);
> + int (*get_max_height)(struct xlnx_crtc *crtc);
> + u32 (*get_format)(struct xlnx_crtc *crtc);
Couldn't we replace all those operations with corresponding integer
fields in struct xlnx_crtc ? They seem to all be fixed.
> +};
> +
> +/**
> + * struct xlnx_crtc - Xilinx CRTC device
> + * @crtc: DRM CRTC device
> + * @list: list node for Xilinx CRTC device list
> + * @ops: Xilinx CRTC ops
> + */
> +struct xlnx_crtc {
> + struct drm_crtc crtc;
> + struct list_head list;
> + const struct xlnx_crtc_ops *ops;
> +};
> +
> +static inline struct xlnx_crtc *to_xlnx_crtc(struct drm_crtc *crtc)
> +{
> + return container_of(crtc, struct xlnx_crtc, crtc);
> +}
> +
> +/*
> + * Helper functions: used within Xlnx DRM
> + */
> +
> +/**
> + * struct xlnx_crtc_helper - Xilinx CRTC helper
> + * @xlnx_crtcs: list of Xilinx CRTC devices
> + * @lock: lock to protect @xlnx_crtcs
> + */
> +struct xlnx_crtc_helper {
> + struct list_head xlnx_crtcs;
> + struct mutex lock; /* lock for @xlnx_crtcs */
> +};
If you access the internals of xlnx_drm in xlnx_crtc.c as advised above,
you can inline the two fields in struct xlnx_drm and remove this
structure. I think it will simplify the code.
> +unsigned int xlnx_crtc_helper_get_align(struct xlnx_crtc_helper *helper);
> +u64 xlnx_crtc_helper_get_dma_mask(struct xlnx_crtc_helper *helper);
> +int xlnx_crtc_helper_get_max_width(struct xlnx_crtc_helper *helper);
> +int xlnx_crtc_helper_get_max_height(struct xlnx_crtc_helper *helper);
> +u32 xlnx_crtc_helper_get_format(struct xlnx_crtc_helper *helper);
> +
> +void xlnx_crtc_helper_init(struct xlnx_crtc_helper *helper);
> +void xlnx_crtc_helper_fini(struct xlnx_crtc_helper *helper);
> +
> +/*
> + * CRTC registration: used by other sub-driver modules
> + */
> +
> +void xlnx_crtc_register(struct drm_device *drm, struct xlnx_crtc *crtc);
> +void xlnx_crtc_unregister(struct drm_device *drm, struct xlnx_crtc *crtc);
> +
> +#endif /* _XLNX_CRTC_H_ */
> diff --git a/drivers/gpu/drm/xlnx/xlnx_drv.c b/drivers/gpu/drm/xlnx/xlnx_drv.c
> new file mode 100644
> index 0000000..5ad2dc2
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_drv.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM KMS Driver
> + *
> + * Copyright (C) 2013 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_of.h>
> +
> +#include <linux/component.h>
> +#include <linux/device.h>
> +#include <linux/dma-buf.h>
> +#include <linux/module.h>
> +#include <linux/of_graph.h>
> +
> +#include "xlnx_crtc.h"
> +#include "xlnx_fb.h"
> +#include "xlnx_gem.h"
> +
> +#define DRIVER_NAME "xlnx"
> +#define DRIVER_DESC "Xilinx DRM KMS Driver"
> +#define DRIVER_DATE "20130509"
> +#define DRIVER_MAJOR 1
> +#define DRIVER_MINOR 0
> +
> +/**
> + * struct xlnx_drm - Xilinx DRM private data
> + * @drm: DRM device
> + * @crtc_helper: Helper to access Xilinx CRTCs
> + * @fb: DRM fb helper
> + * @master: logical master device for pipeline
> + * @suspend_state: atomic state for suspend / resume
> + */
> +struct xlnx_drm {
> + struct drm_device *drm;
> + struct xlnx_crtc_helper crtc_helper;
> + struct drm_fb_helper *fb;
> + struct device *master;
> + struct drm_atomic_state *suspend_state;
> +};
> +
> +/**
> + * xlnx_get_crtc_helper - Return the crtc helper instance
> + * @drm: DRM device
> + *
> + * Return: the crtc helper instance
> + */
> +struct xlnx_crtc_helper *xlnx_get_crtc_helper(struct drm_device *drm)
> +{
> + struct xlnx_drm *xlnx_drm = drm->dev_private;
> +
> + return &xlnx_drm->crtc_helper;
> +}
> +
> +/**
> + * xlnx_get_align - Return the align requirement through CRTC helper
> + * @drm: DRM device
> + *
> + * Return: the alignment requirement
> + */
> +unsigned int xlnx_get_align(struct drm_device *drm)
This is a very generic name, would it make sense to rename the function
to xlnx_drm_get_align() to avoid name collisions ? It could make sense
to rename other functions accordingly to standardize on the xlnx_drm_
prefix (at least when there's no other prefix such as xlnx_crtc_).
> +{
> + struct xlnx_drm *xlnx_drm = drm->dev_private;
> +
> + return xlnx_crtc_helper_get_align(&xlnx_drm->crtc_helper);
> +}
> +
> +static void xlnx_output_poll_changed(struct drm_device *drm)
> +{
> + struct xlnx_drm *xlnx_drm = drm->dev_private;
> +
> + if (xlnx_drm->fb)
> + drm_fb_helper_hotplug_event(xlnx_drm->fb);
> +}
> +
> +static const struct drm_mode_config_funcs xlnx_mode_config_funcs = {
> + .fb_create = xlnx_fb_create,
> + .output_poll_changed = xlnx_output_poll_changed,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static void xlnx_mode_config_init(struct drm_device *drm)
> +{
> + struct xlnx_drm *xlnx_drm = drm->dev_private;
> + struct xlnx_crtc_helper *crtc_helper = &xlnx_drm->crtc_helper;
> +
> + drm->mode_config.min_width = 0;
> + drm->mode_config.min_height = 0;
> + drm->mode_config.max_width =
> + xlnx_crtc_helper_get_max_width(crtc_helper);
> + drm->mode_config.max_height =
> + xlnx_crtc_helper_get_max_height(crtc_helper);
This restricts the framebuffer size to the maximum CRTC size, which
prevents creating a single framebuffer that spans multiple CRTCs. Isn't
that a problem ?
> +}
> +
> +static void xlnx_lastclose(struct drm_device *drm)
> +{
> + struct xlnx_drm *xlnx_drm = drm->dev_private;
> +
> + if (xlnx_drm->fb)
> + drm_fb_helper_restore_fbdev_mode_unlocked(xlnx_drm->fb);
> +}
> +
> +static const struct file_operations xlnx_fops = {
> + .owner = THIS_MODULE,
> + .open = drm_open,
> + .release = drm_release,
> + .unlocked_ioctl = drm_ioctl,
> + .mmap = drm_gem_cma_mmap,
> + .poll = drm_poll,
> + .read = drm_read,
> +#ifdef CONFIG_COMPAT
> + .compat_ioctl = drm_compat_ioctl,
> +#endif
> + .llseek = noop_llseek,
> +};
> +
> +static struct drm_driver xlnx_drm_driver = {
> + .driver_features = DRIVER_MODESET | DRIVER_GEM |
> + DRIVER_ATOMIC | DRIVER_PRIME,
> + .lastclose = xlnx_lastclose,
> +
> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> + .gem_prime_export = drm_gem_prime_export,
> + .gem_prime_import = drm_gem_prime_import,
> + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> + .gem_prime_vmap = drm_gem_cma_prime_vmap,
> + .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> + .gem_prime_mmap = drm_gem_cma_prime_mmap,
> + .gem_free_object = drm_gem_cma_free_object,
> + .gem_vm_ops = &drm_gem_cma_vm_ops,
> + .dumb_create = xlnx_gem_cma_dumb_create,
> + .dumb_destroy = drm_gem_dumb_destroy,
> +
> + .fops = &xlnx_fops,
> +
> + .name = DRIVER_NAME,
> + .desc = DRIVER_DESC,
> + .date = DRIVER_DATE,
> + .major = DRIVER_MAJOR,
> + .minor = DRIVER_MINOR,
> +};
> +
> +static int xlnx_bind(struct device *master)
> +{
> + struct xlnx_drm *xlnx_drm;
> + struct drm_device *drm;
> + const struct drm_format_info *info;
> + int ret;
> + u32 format;
> +
> + drm = drm_dev_alloc(&xlnx_drm_driver, master->parent);
> + if (IS_ERR(drm))
> + return PTR_ERR(drm);
> +
> + xlnx_drm = devm_kzalloc(drm->dev, sizeof(*xlnx_drm), GFP_KERNEL);
> + if (!xlnx_drm) {
> + ret = -ENOMEM;
> + goto err_drm;
> + }
> +
> + drm_mode_config_init(drm);
> + drm->mode_config.funcs = &xlnx_mode_config_funcs;
> +
> + ret = drm_vblank_init(drm, 1);
> + if (ret) {
> + dev_err(master->parent, "failed to initialize vblank\n");
> + goto err_xlnx_drm;
> + }
> +
> + drm->irq_enabled = 1;
> + drm->dev_private = xlnx_drm;
> + xlnx_drm->drm = drm;
> + xlnx_drm->master = master;
> + drm_kms_helper_poll_init(drm);
> + dev_set_drvdata(master, xlnx_drm);
> +
> + xlnx_crtc_helper_init(&xlnx_drm->crtc_helper);
> +
> + ret = component_bind_all(master, drm);
> + if (ret)
> + goto err_crtc;
> +
> + xlnx_mode_config_init(drm);
> + drm_mode_config_reset(drm);
> + dma_set_mask(drm->dev,
> + xlnx_crtc_helper_get_dma_mask(&xlnx_drm->crtc_helper));
> +
> + format = xlnx_crtc_helper_get_format(&xlnx_drm->crtc_helper);
> + info = drm_format_info(format);
> + if (info && info->depth && info->cpp[0]) {
> + unsigned int align;
> +
> + align = xlnx_crtc_helper_get_align(&xlnx_drm->crtc_helper);
> + xlnx_drm->fb = xlnx_fb_init(drm, info->cpp[0] * 8, 1, align);
> + if (IS_ERR(xlnx_drm->fb)) {
> + dev_err(master->parent,
> + "failed to initialize drm fb\n");
> + xlnx_drm->fb = NULL;
> + }
> + } else {
> + /* fbdev emulation is optional */
> + dev_info(master->parent, "fbdev is not initialized\n");
> + }
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret < 0)
> + goto err_fb;
> +
> + return 0;
> +
> +err_fb:
> + if (xlnx_drm->fb)
> + xlnx_fb_fini(xlnx_drm->fb);
> + component_unbind_all(drm->dev, drm);
> +err_crtc:
> + xlnx_crtc_helper_fini(&xlnx_drm->crtc_helper);
> +err_xlnx_drm:
> + drm_mode_config_cleanup(drm);
> +err_drm:
> + drm_dev_unref(drm);
> + return ret;
> +}
> +
> +static void xlnx_unbind(struct device *master)
> +{
> + struct xlnx_drm *xlnx_drm = dev_get_drvdata(master);
> + struct drm_device *drm = xlnx_drm->drm;
> +
> + drm_dev_unregister(drm);
> + if (xlnx_drm->fb)
> + xlnx_fb_fini(xlnx_drm->fb);
> + component_unbind_all(master, drm);
> + xlnx_crtc_helper_fini(&xlnx_drm->crtc_helper);
> + drm_kms_helper_poll_fini(drm);
> + drm_mode_config_cleanup(drm);
> + drm_dev_unref(drm);
> +}
> +
> +static const struct component_master_ops xlnx_master_ops = {
> + .bind = xlnx_bind,
> + .unbind = xlnx_unbind,
> +};
> +
> +static int xlnx_compare_of(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +static int xlnx_probe(struct device *master_dev)
> +{
> + struct device *dev = master_dev->parent;
> + struct component_match *match = NULL;
> +
> + if (!dev->of_node)
> + return -EINVAL;
> +
> + component_match_add(master_dev, &match, xlnx_compare_of, dev->of_node);
> +
> + return component_master_add_with_match(master_dev, &xlnx_master_ops,
> + match);
I'm not sure to understand what you're doing here. You're registering a
component master for the xlnx-drm device with a single component match, for the
parent of the xlnx-drm device. The xlnx-drm device is registered with a call to
xlnx_drm_pipeline_init(), which is this series is called from
zynqmp_dpsub_probe() only, and you register the corresponding component in the
same function. This seems very convoluted to me, can't this be implemented in a
much simpler way ? I think we could just initialize the zynqmp_disp and
zynqmp_dp synchronously from the zynqmp_dpsub probe function without needing
the component framework at all. Do you envision the zynqmp_disp and zynqmp_dp
parts to be reused in other display pipelines, or will they always be combined
together in zynqmp_dpsub ?
I would also say that if the component is a parent of the component master,
you're likely doing something wrong. Component are supposed to represent pieces
of the whole DRM device, and as such should be children of the master, or
sometimes possibly unrelated devices.
> +}
> +
> +static int xlnx_remove(struct device *dev)
> +{
> + component_master_del(dev, &xlnx_master_ops);
> + return 0;
> +}
> +
> +static void xlnx_shutdown(struct device *dev)
> +{
> + component_master_del(dev, &xlnx_master_ops);
> +}
> +
> +static int __maybe_unused xlnx_pm_suspend(struct device *dev)
> +{
> + struct xlnx_drm *xlnx_drm = dev_get_drvdata(dev);
> + struct drm_device *drm = xlnx_drm->drm;
> +
> + drm_kms_helper_poll_disable(drm);
> +
> + xlnx_drm->suspend_state = drm_atomic_helper_suspend(drm);
> + if (IS_ERR(xlnx_drm->suspend_state)) {
> + drm_kms_helper_poll_enable(drm);
> + return PTR_ERR(xlnx_drm->suspend_state);
> + }
You can use the drm_mode_config_helper_suspend() and
drm_mode_config_helper_resume() functions, removing the need to track
suspend_state manually in xlnx_drm.
> +
> + return 0;
> +}
> +
> +static int __maybe_unused xlnx_pm_resume(struct device *dev)
> +{
> + struct xlnx_drm *xlnx_drm = dev_get_drvdata(dev);
> + struct drm_device *drm = xlnx_drm->drm;
> +
> + drm_atomic_helper_resume(drm, xlnx_drm->suspend_state);
> + drm_kms_helper_poll_enable(drm);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops xlnx_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(xlnx_pm_suspend, xlnx_pm_resume)
> +};
> +
> +static int xlnx_drv_bus_match(struct device *dev, struct device_driver *drv)
> +{
> + return !strncmp(dev_name(dev), drv->name, strlen(drv->name));
> +}
> +
> +struct bus_type xlnx_driver_bus_type = {
> + .name = "xlnx-drm-bus",
> + .match = &xlnx_drv_bus_match,
> +};
Why do you need a custom bus ? Can't you just use platform devices with
a platform_driver and remove all the code below ? Is there a reason not
to do so ?
> +static struct device_driver xlnx_driver = {
> + .probe = xlnx_probe,
> + .remove = xlnx_remove,
> + .shutdown = xlnx_shutdown,
> + .name = "xlnx-drm",
> + .pm = &xlnx_pm_ops,
> + .bus = &xlnx_driver_bus_type,
> + .owner = THIS_MODULE,
> +};
> +
> +/* bitmap for master id */
> +static u32 xlnx_master_ids = GENMASK(31, 0);
> +
> +static void xlnx_master_release(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +/**
> + * xlnx_drm_pipeline_init - Initialize the drm pipeline for the device
> + * @dev: The device to initialize the drm pipeline device
> + *
> + * This function initializes the drm pipeline device, struct drm_device,
> + * on @dev by creating a logical master device. The logical device acts
> + * as a master device to bind slave devices and represents the entire
> + * pipeline.
> + * The logical master uses the port bindings of the calling device to
> + * figure out the pipeline topology.
> + *
> + * Return: the logical master device if the drm device is initialized
> + * on @dev. Error code otherwise.
> + */
> +struct device *xlnx_drm_pipeline_init(struct device *dev)
> +{
> + struct device *master;
> + int id, ret;
> +
> + id = ffs(xlnx_master_ids);
> + if (!id)
> + return ERR_PTR(-ENOSPC);
> +
> + master = kzalloc(sizeof(*master), GFP_KERNEL);
> + if (!master)
> + return ERR_PTR(-ENOMEM);
> + device_initialize(master);
> + master->parent = dev;
> + master->bus = &xlnx_driver_bus_type;
> + master->release = xlnx_master_release;
> +
> + ret = dev_set_name(master, "xlnx-drm.%d", id);
> + if (ret)
> + goto err_kfree;
> +
> + ret = device_add(master);
> + if (ret)
> + goto err_kfree;
> +
> + xlnx_master_ids &= ~BIT(master->id);
> + return master;
> +
> +err_kfree:
> + kfree(master);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(xlnx_drm_pipeline_init);
> +
> +/**
> + * xlnx_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 device *master)
> +{
> + xlnx_master_ids |= BIT(master->id);
> + device_unregister(master);
> +}
> +EXPORT_SYMBOL_GPL(xlnx_drm_pipeline_exit);
> +
> +static int __init xlnx_drm_drv_init(void)
> +{
> + int ret;
> +
> + ret = bus_register(&xlnx_driver_bus_type);
> + if (ret)
> + return ret;
> +
> + ret = driver_register(&xlnx_driver);
> + if (ret) {
> + bus_unregister(&xlnx_driver_bus_type);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit xlnx_drm_drv_exit(void)
> +{
> + bus_unregister(&xlnx_driver_bus_type);
> + driver_unregister(&xlnx_driver);
> +}
> +
> +module_init(xlnx_drm_drv_init);
> +module_exit(xlnx_drm_drv_exit);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("Xilinx DRM KMS Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/xlnx/xlnx_drv.h b/drivers/gpu/drm/xlnx/xlnx_drv.h
> new file mode 100644
> index 0000000..b38ff2a
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_drv.h
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM KMS Header for Xilinx
> + *
> + * Copyright (C) 2013 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyunk@xxxxxxxxxx>
> + */
> +
> +#ifndef _XLNX_DRV_H_
> +#define _XLNX_DRV_H_
> +
> +struct device;
> +struct drm_device;
> +struct xlnx_crtc_helper;
> +
> +struct device *xlnx_drm_pipeline_init(struct device *parent);
> +void xlnx_drm_pipeline_exit(struct device *pipeline);
> +
> +unsigned int xlnx_get_align(struct drm_device *drm);
> +struct xlnx_crtc_helper *xlnx_get_crtc_helper(struct drm_device *drm);
> +
> +#endif /* _XLNX_DRV_H_ */
> diff --git a/drivers/gpu/drm/xlnx/xlnx_fb.c b/drivers/gpu/drm/xlnx/xlnx_fb.c
> new file mode 100644
> index 0000000..8e8ab14
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_fb.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM KMS Framebuffer helper
> + *
> + * Copyright (C) 2015 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + *
> + * Based on drm_fb_cma_helper.c
> + *
> + * Copyright (C) 2012 Analog Device Inc.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +
> +#include "xlnx_drv.h"
> +#include "xlnx_fb.h"
> +
> +#define XLNX_MAX_PLANES 4
This isn't used.
> +struct xlnx_fbdev {
> + struct drm_fb_helper fb_helper;
> + struct drm_framebuffer *fb;
> + unsigned int align;
> +};
> +
> +static inline struct xlnx_fbdev *to_fbdev(struct drm_fb_helper *fb_helper)
> +{
> + return container_of(fb_helper, struct xlnx_fbdev, fb_helper);
> +}
> +
> +static const struct drm_framebuffer_funcs xlnx_fb_funcs = {
> + .destroy = drm_gem_fb_destroy,
> + .create_handle = drm_gem_fb_create_handle,
> +};
> +
> +static struct fb_ops xlnx_fbdev_ops = {
> + .owner = THIS_MODULE,
> + .fb_fillrect = sys_fillrect,
> + .fb_copyarea = sys_copyarea,
> + .fb_imageblit = sys_imageblit,
> + DRM_FB_HELPER_DEFAULT_OPS,
> +};
> +
> +/**
> + * xlnx_fbdev_create - Create the fbdev with a framebuffer
> + * @fb_helper: fb helper structure
> + * @size: framebuffer size info
> + *
> + * This function is based on drm_fbdev_cma_create().
The fbdev compatibility layer has been improved in DRM, is there a way
you could use drm_fbdev_generic_setup() instead of reimplementing fbdev
create yourself ?
> + * Return: 0 if successful, or the error code.
> + */
> +static int xlnx_fbdev_create(struct drm_fb_helper *fb_helper,
> + struct drm_fb_helper_surface_size *size)
> +{
> + struct xlnx_fbdev *fbdev = to_fbdev(fb_helper);
> + struct drm_device *drm = fb_helper->dev;
> + struct drm_gem_cma_object *obj;
> + struct drm_framebuffer *fb;
> + unsigned int bytes_per_pixel;
> + unsigned long offset;
> + struct fb_info *fbi;
> + size_t bytes;
> + int ret;
> +
> + dev_dbg(drm->dev, "surface width(%d), height(%d) and bpp(%d)\n",
> + size->surface_width, size->surface_height, size->surface_bpp);
> +
> + bytes_per_pixel = DIV_ROUND_UP(size->surface_bpp, 8);
> + bytes = ALIGN(size->surface_width * bytes_per_pixel, fbdev->align);
> + bytes *= size->surface_height;
> +
> + obj = drm_gem_cma_create(drm, bytes);
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + fbi = framebuffer_alloc(0, drm->dev);
> + if (!fbi) {
> + dev_err(drm->dev, "Failed to allocate framebuffer info.\n");
> + ret = -ENOMEM;
> + goto err_drm_gem_cma_free_object;
> + }
> +
> + fbdev->fb = drm_gem_fbdev_fb_create(drm, size, fbdev->align, &obj->base,
> + &xlnx_fb_funcs);
As xlnx_fb_funcs is identical to drm_gem_fb_funcs you can pass NULL as
the last parameter to this function.
> + if (IS_ERR(fbdev->fb)) {
> + dev_err(drm->dev, "Failed to allocate DRM framebuffer.\n");
> + ret = PTR_ERR(fbdev->fb);
> + goto err_framebuffer_release;
> + }
> +
> + fb = fbdev->fb;
> + fb_helper->fb = fb;
> + fb_helper->fbdev = fbi;
> + fbi->par = fb_helper;
> + fbi->flags = FBINFO_FLAG_DEFAULT;
> + fbi->fbops = &xlnx_fbdev_ops;
> +
> + ret = fb_alloc_cmap(&fbi->cmap, 256, 0);
> + if (ret) {
> + dev_err(drm->dev, "Failed to allocate color map.\n");
> + goto err_fb_destroy;
> + }
> +
> + drm_fb_helper_fill_fix(fbi, fb->pitches[0], fb->format->depth);
> + drm_fb_helper_fill_var(fbi, fb_helper, size->fb_width, size->fb_height);
> +
> + offset = fbi->var.xoffset * bytes_per_pixel;
> + offset += fbi->var.yoffset * fb->pitches[0];
> +
> + drm->mode_config.fb_base = (resource_size_t)obj->paddr;
> + fbi->screen_base = (char __iomem *)(obj->vaddr + offset);
> + fbi->fix.smem_start = (unsigned long)(obj->paddr + offset);
> + fbi->screen_size = bytes;
> + fbi->fix.smem_len = bytes;
> +
> + return 0;
> +
> +err_fb_destroy:
> + drm_framebuffer_unregister_private(fb);
> + drm_gem_fb_destroy(fb);
> +err_framebuffer_release:
> + framebuffer_release(fbi);
> +err_drm_gem_cma_free_object:
> + drm_gem_cma_free_object(&obj->base);
> + return ret;
> +}
> +
> +static const struct drm_fb_helper_funcs xlnx_fb_helper_funcs = {
> + .fb_probe = xlnx_fbdev_create,
> +};
> +
> +/**
> + * xlnx_fb_init - Allocate and initializes the Xilinx framebuffer
Unless I'm mistaken this function deals with fbdev compatibility, not
drm_framuffer. I would s/fb/fbdev/ for clarity. Ideally the function
should be removed completely and the driver should use
drm_fbdev_generic_setup() instead.
> + * @drm: DRM device
> + * @preferred_bpp: preferred bits per pixel for the device
> + * @max_conn_count: maximum number of connectors
> + * @align: alignment value for pitch
> + *
> + * This function is based on drm_fbdev_cma_init().
> + *
> + * Return: a newly allocated drm_fb_helper struct or a ERR_PTR.
> + */
> +struct drm_fb_helper *
> +xlnx_fb_init(struct drm_device *drm, int preferred_bpp,
> + unsigned int max_conn_count, unsigned int align)
> +{
> + struct xlnx_fbdev *fbdev;
> + struct drm_fb_helper *fb_helper;
> + int ret;
> +
> + fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> + if (!fbdev)
> + return ERR_PTR(-ENOMEM);
> +
> + fbdev->align = align;
> + fb_helper = &fbdev->fb_helper;
> + drm_fb_helper_prepare(drm, fb_helper, &xlnx_fb_helper_funcs);
> +
> + ret = drm_fb_helper_init(drm, fb_helper, max_conn_count);
> + if (ret < 0) {
> + dev_err(drm->dev, "Failed to initialize drm fb helper.\n");
> + goto err_free;
> + }
> +
> + ret = drm_fb_helper_single_add_all_connectors(fb_helper);
> + if (ret < 0) {
> + dev_err(drm->dev, "Failed to add connectors.\n");
> + goto err_drm_fb_helper_fini;
> + }
> +
> + ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp);
> + if (ret < 0) {
> + dev_err(drm->dev, "Failed to set initial hw configuration.\n");
> + goto err_drm_fb_helper_fini;
> + }
> +
> + return fb_helper;
> +
> +err_drm_fb_helper_fini:
> + drm_fb_helper_fini(fb_helper);
> +err_free:
> + kfree(fbdev);
> + return ERR_PTR(ret);
> +}
> +
> +/**
> + * xlnx_fbdev_defio_fini - Free the defio fb
> + * @fbi: fb_info struct
> + *
> + * This function is based on drm_fbdev_cma_defio_fini().
> + */
> +static void xlnx_fbdev_defio_fini(struct fb_info *fbi)
> +{
> + if (!fbi->fbdefio)
> + return;
> +
> + fb_deferred_io_cleanup(fbi);
> + kfree(fbi->fbdefio);
> + kfree(fbi->fbops);
> +}
> +
> +/**
> + * xlnx_fbdev_fini - Free the Xilinx framebuffer
> + * @fb_helper: drm_fb_helper struct
> + *
> + * This function is based on drm_fbdev_cma_fini().
> + */
> +void xlnx_fb_fini(struct drm_fb_helper *fb_helper)
> +{
> + struct xlnx_fbdev *fbdev = to_fbdev(fb_helper);
> +
> + drm_fb_helper_unregister_fbi(&fbdev->fb_helper);
> + if (fbdev->fb_helper.fbdev)
> + xlnx_fbdev_defio_fini(fbdev->fb_helper.fbdev);
> +
> + if (fbdev->fb_helper.fb)
> + drm_framebuffer_remove(fbdev->fb_helper.fb);
> +
> + drm_fb_helper_fini(&fbdev->fb_helper);
> + kfree(fbdev);
> +}
> +
> +/**
> + * xlnx_fb_create - (struct drm_mode_config_funcs *)->fb_create callback
> + * @drm: DRM device
> + * @file_priv: drm file private data
> + * @mode_cmd: mode command for fb creation
> + *
> + * This functions creates a drm_framebuffer with xlnx_fb_funcs for given mode
> + * @mode_cmd. This functions is intended to be used for the fb_create callback
> + * function of drm_mode_config_funcs.
I'd say "to be used as the drm_mode_config_funcs->fb_create() handler".
> + * Return: a drm_framebuffer object if successful, or
> + * ERR_PTR from drm_gem_fb_create_with_funcs().
> + */
> +struct drm_framebuffer *
> +xlnx_fb_create(struct drm_device *drm, struct drm_file *file_priv,
> + const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> + return drm_gem_fb_create_with_funcs(drm, file_priv, mode_cmd,
> + &xlnx_fb_funcs);
> +}
You can replace xlnx_fb_create() with drm_gem_fb_create() as xlnx_fb_funcs is
identical to drm_gem_fb_funcs.
Unless there's a reason that would prevent usage of
drm_fbdev_generic_setup(), this file can be removed completely :-) The
fb pointer in xlnx_drm could then be removed too.
> diff --git a/drivers/gpu/drm/xlnx/xlnx_fb.h b/drivers/gpu/drm/xlnx/xlnx_fb.h
> new file mode 100644
> index 0000000..e41738e
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_fb.h
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM KMS Framebuffer helper header
> + *
> + * Copyright (C) 2015 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + */
> +
> +#ifndef _XLNX_FB_H_
> +#define _XLNX_FB_H_
> +
> +struct drm_device;
> +struct drm_file;
> +struct drm_fb_helper;
b comes before i in alphabetical order :-)
> +struct drm_framebuffer;
> +struct drm_mode_fb_cmd2;
> +
> +struct drm_framebuffer *
> +xlnx_fb_create(struct drm_device *drm, struct drm_file *file_priv,
> + const struct drm_mode_fb_cmd2 *mode_cmd);
> +struct drm_fb_helper *
> +xlnx_fb_init(struct drm_device *drm, int preferred_bpp,
> + unsigned int max_conn_count, unsigned int align);
> +void xlnx_fb_fini(struct drm_fb_helper *fb_helper);
> +
> +#endif /* _XLNX_FB_H_ */
> diff --git a/drivers/gpu/drm/xlnx/xlnx_gem.c b/drivers/gpu/drm/xlnx/xlnx_gem.c
> new file mode 100644
> index 0000000..6748dc1
> --- /dev/null
> +++ b/drivers/gpu/drm/xlnx/xlnx_gem.c
> @@ -0,0 +1,36 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Xilinx DRM KMS GEM helper
> + *
> + * Copyright (C) 2015 - 2018 Xilinx, Inc.
> + *
> + * Author: Hyun Woo Kwon <hyun.kwon@xxxxxxxxxx>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_gem_cma_helper.h>
> +
> +#include "xlnx_drv.h"
> +#include "xlnx_gem.h"
> +
> +/*
> + * xlnx_gem_cma_dumb_create - (struct drm_driver)->dumb_create callback
> + * @file_priv: drm_file object
> + * @drm: DRM object
> + * @args: info for dumb scanout buffer creation
> + *
> + * This function is for dumb_create callback of drm_driver struct. Simply
> + * it wraps around drm_gem_cma_dumb_create() and sets the pitch value
> + * by retrieving the value from the device.
This isn't entirely accurate, the function doesn't retrieve a value from
the device. How about the following ?
This function is the drm_driver.dumb_create() callback handler. It wraps
the DRM GEM CMA helper implementation and computes the pitch based on the
alignment constraints retrieved from the CRTC driver.
> + * Return: The return value from drm_gem_cma_dumb_create()
> + */
> +int xlnx_gem_cma_dumb_create(struct drm_file *file_priv, struct drm_device *drm,
> + struct drm_mode_create_dumb *args)
> +{
> + unsigned int pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +
> + args->pitch = ALIGN(pitch, xlnx_get_align(drm));
> +
> + return drm_gem_cma_dumb_create_internal(file_priv, drm, args);
> +}
--
Regards,
Laurent Pinchart
