| Message ID | 20240712093243.2108456-1-victor.liu@nxp.com |
|---|---|
| Headers | show |
| Series | Add Freescale i.MX8qxp Display Controller support | expand |
On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote: > i.MX8qxp Display Controller display engine consists of all processing > units that operate in a display clock domain. Add minimal feature > support with FrameGen and TCon so that the engine can output display > timings. The display engine driver as a master binds FrameGen and > TCon drivers as components. While at it, the display engine driver > is a component to be bound with the upcoming DRM driver. Generic question: why do you need so many small subdrivers? Are they used to represent the flexibility of the pipeline? Can you instantiate these units directly from the de(?) driver and reference created structures without the need to create subdevices? > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v2: > * Use OF alias id to get instance id. > * Add dev member to struct dc_tc. > > drivers/gpu/drm/imx/Kconfig | 1 + > drivers/gpu/drm/imx/Makefile | 1 + > drivers/gpu/drm/imx/dc/Kconfig | 5 + > drivers/gpu/drm/imx/dc/Makefile | 5 + > drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++++++++++ > drivers/gpu/drm/imx/dc/dc-de.h | 62 ++++++ > drivers/gpu/drm/imx/dc/dc-drv.c | 32 +++ > drivers/gpu/drm/imx/dc/dc-drv.h | 24 +++ > drivers/gpu/drm/imx/dc/dc-fg.c | 366 ++++++++++++++++++++++++++++++++ > drivers/gpu/drm/imx/dc/dc-tc.c | 137 ++++++++++++ > 10 files changed, 784 insertions(+) > create mode 100644 drivers/gpu/drm/imx/dc/Kconfig > create mode 100644 drivers/gpu/drm/imx/dc/Makefile > create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h > create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h > create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c > > diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig > index 03535a15dd8f..3e8c6edbc17c 100644 > --- a/drivers/gpu/drm/imx/Kconfig > +++ b/drivers/gpu/drm/imx/Kconfig > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0-only > > +source "drivers/gpu/drm/imx/dc/Kconfig" > source "drivers/gpu/drm/imx/dcss/Kconfig" > source "drivers/gpu/drm/imx/ipuv3/Kconfig" > source "drivers/gpu/drm/imx/lcdc/Kconfig" > diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile > index 86f38e7c7422..c7b317640d71 100644 > --- a/drivers/gpu/drm/imx/Makefile > +++ b/drivers/gpu/drm/imx/Makefile > @@ -1,5 +1,6 @@ > # SPDX-License-Identifier: GPL-2.0 > > +obj-$(CONFIG_DRM_IMX8_DC) += dc/ > obj-$(CONFIG_DRM_IMX_DCSS) += dcss/ > obj-$(CONFIG_DRM_IMX) += ipuv3/ > obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/ > diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig > new file mode 100644 > index 000000000000..32d7471c49d0 > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/Kconfig > @@ -0,0 +1,5 @@ > +config DRM_IMX8_DC > + tristate "Freescale i.MX8 Display Controller Graphics" > + depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) > + help > + enable Freescale i.MX8 Display Controller(DC) graphics support > diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile > new file mode 100644 > index 000000000000..56de82d53d4d > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o > + > +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o > diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c > new file mode 100644 > index 000000000000..2c8268b76b08 > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-de.c > @@ -0,0 +1,151 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2024 NXP > + */ > + > +#include <linux/component.h> > +#include <linux/container_of.h> > +#include <linux/io.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > + > +#include <drm/drm_managed.h> > + > +#include "dc-de.h" > +#include "dc-drv.h" > + > +#define POLARITYCTRL 0xc > +#define POLEN_HIGH BIT(2) > + > +struct dc_de_priv { > + struct dc_de engine; > + void __iomem *reg_top; > +}; > + > +static inline struct dc_de_priv *to_de_priv(struct dc_de *de) > +{ > + return container_of(de, struct dc_de_priv, engine); > +} > + > +static inline void > +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value) > +{ > + struct dc_de_priv *priv = to_de_priv(de); > + > + writel(value, priv->reg_top + offset); Is there a point in this wrapper? Can you call writel directly? This question generally applies to the driver. I see a lot of small functions which can be inlined without losing the clarity. > +} > + > +static void dc_dec_init(struct dc_de *de) > +{ > + dc_dec_write(de, POLARITYCTRL, POLEN_HIGH); > +} > + > +static int dc_de_bind(struct device *dev, struct device *master, void *data) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct dc_drm_device *dc_drm = data; > + struct dc_de_priv *priv; > + int ret; > + > + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top"); > + if (IS_ERR(priv->reg_top)) > + return PTR_ERR(priv->reg_top); > + > + priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload"); > + if (priv->engine.irq_shdld < 0) > + return priv->engine.irq_shdld; > + > + priv->engine.irq_framecomplete = > + platform_get_irq_byname(pdev, "framecomplete"); > + if (priv->engine.irq_framecomplete < 0) > + return priv->engine.irq_framecomplete; > + > + priv->engine.irq_seqcomplete = > + platform_get_irq_byname(pdev, "seqcomplete"); > + if (priv->engine.irq_seqcomplete < 0) > + return priv->engine.irq_seqcomplete; > + > + priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine"); Is this alias documented somewhere? Is it Acked by DT maintainers? > + if (priv->engine.id < 0) { > + dev_err(dev, "failed to get alias id: %d\n", priv->engine.id); > + return priv->engine.id; > + } > + > + priv->engine.dev = dev; > + > + dev_set_drvdata(dev, priv); > + > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + > + dc_drm->de[priv->engine.id] = &priv->engine; > + > + return 0; > +} > +
On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: > i.MX8qxp Display Controller pixel engine consists of all processing > units that operate in the AXI bus clock domain. Add drivers for > ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as > well as a pixel engine driver, so that two displays with primary > planes can be supported. The pixel engine driver as a master binds > those unit drivers as components. While at it, the pixel engine > driver is a component to be bound with the upcoming DRM driver. Same question / comment: create subnodes directly, without going through the subdevices. A lot of small functions that would benefit being inlined. > +static int dc_cf_bind(struct device *dev, struct device *master, void *data) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct dc_drm_device *dc_drm = data; > + struct dc_pe *pe = dc_drm->pe; > + struct dc_cf_priv *priv; > + int id; > + > + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); > + if (IS_ERR(priv->reg_cfg)) > + return PTR_ERR(priv->reg_cfg); > + > + id = of_alias_get_id(dev->of_node, "dc0-constframe"); Is it documented? Acked? > + if (id < 0) { > + dev_err(dev, "failed to get alias id: %d\n", id); > + return id; > + } > +
On Fri, Jul 12, 2024 at 05:32:36PM GMT, Liu Ying wrote: > i.MX8qxp Display Controller(DC) is comprised of three main components that > include a blit engine for 2D graphics accelerations, display controller for > display output processing, as well as a command sequencer. Add kernel > mode setting support for the display controller part with two CRTCs and > two primary planes(backed by FetchLayer and FetchWarp respectively). The > registers of the display controller are accessed without command sequencer > involved, instead just by using CPU. The command sequencer is supposed to > be used by the blit engine. Generic comment: please consider moving dc_plane / dc_crtc defines to the source files and dropping the headers. > > Signed-off-by: Liu Ying <victor.liu@nxp.com> > --- > v2: > * Find next bridge from TCon's port. > * Drop drm/drm_module.h include from dc-drv.c. > > drivers/gpu/drm/imx/dc/Kconfig | 2 + > drivers/gpu/drm/imx/dc/Makefile | 5 +- > drivers/gpu/drm/imx/dc/dc-crtc.c | 578 ++++++++++++++++++++++++++++++ > drivers/gpu/drm/imx/dc/dc-crtc.h | 67 ++++ > drivers/gpu/drm/imx/dc/dc-de.h | 3 + > drivers/gpu/drm/imx/dc/dc-drv.c | 236 ++++++++++++ > drivers/gpu/drm/imx/dc/dc-drv.h | 21 ++ > drivers/gpu/drm/imx/dc/dc-kms.c | 143 ++++++++ > drivers/gpu/drm/imx/dc/dc-kms.h | 15 + > drivers/gpu/drm/imx/dc/dc-plane.c | 227 ++++++++++++ > drivers/gpu/drm/imx/dc/dc-plane.h | 37 ++ > 11 files changed, 1332 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.h > create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h > create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c > create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.h > > diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig > index b66b815fbdf1..dac0de009273 100644 > --- a/drivers/gpu/drm/imx/dc/Kconfig > +++ b/drivers/gpu/drm/imx/dc/Kconfig > @@ -1,6 +1,8 @@ > config DRM_IMX8_DC > tristate "Freescale i.MX8 Display Controller Graphics" > depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) > + select DRM_GEM_DMA_HELPER > + select DRM_KMS_HELPER > select GENERIC_IRQ_CHIP > help > enable Freescale i.MX8 Display Controller(DC) graphics support > diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile > index 1ce3e8a8db22..b9d33c074984 100644 > --- a/drivers/gpu/drm/imx/dc/Makefile > +++ b/drivers/gpu/drm/imx/dc/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > > -imx8-dc-drm-objs := dc-cf.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o dc-fu.o \ > - dc-fw.o dc-ic.o dc-lb.o dc-pe.o dc-tc.o > +imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o \ > + dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o \ > + dc-plane.o dc-tc.o > > obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o > diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c > new file mode 100644 > index 000000000000..e151e14a6677 > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c > @@ -0,0 +1,578 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2024 NXP > + */ > + > +#include <linux/completion.h> > +#include <linux/interrupt.h> > +#include <linux/irqreturn.h> > +#include <linux/pm_runtime.h> > +#include <linux/spinlock.h> > + > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_atomic_state_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_device.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_managed.h> > +#include <drm/drm_modes.h> > +#include <drm/drm_modeset_helper_vtables.h> > +#include <drm/drm_plane.h> > +#include <drm/drm_vblank.h> > + > +#include "dc-crtc.h" > +#include "dc-de.h" > +#include "dc-drv.h" > +#include "dc-pe.h" > +#include "dc-plane.h" > + > +#define DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(c) \ > +do { \ > + unsigned long ret; \ > + ret = wait_for_completion_timeout(&dc_crtc->c, HZ); \ > + if (ret == 0) \ > + dc_crtc_err(crtc, "%s: wait for " #c " timeout\n", \ > + __func__); \ > +} while (0) > + > +#define DC_CRTC_CHECK_FRAMEGEN_FIFO(fg) \ > +do { \ > + typeof(fg) _fg = (fg); \ > + if (dc_fg_secondary_requests_to_read_empty_fifo(_fg)) { \ > + dc_fg_secondary_clear_channel_status(_fg); \ > + dc_crtc_err(crtc, "%s: FrameGen FIFO empty\n", \ > + __func__); \ > + } \ > +} while (0) > + > +#define DC_CRTC_WAIT_FOR_FRAMEGEN_SECONDARY_SYNCUP(fg) \ > +do { \ > + if (dc_fg_wait_for_secondary_syncup(fg)) \ > + dc_crtc_err(crtc, \ > + "%s: FrameGen secondary channel isn't syncup\n",\ > + __func__); \ > +} while (0) > + > +static u32 dc_crtc_get_vblank_counter(struct drm_crtc *crtc) > +{ > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + > + return dc_fg_get_frame_index(dc_crtc->fg); > +} > + > +static int dc_crtc_enable_vblank(struct drm_crtc *crtc) > +{ > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + > + enable_irq(dc_crtc->irq_dec_framecomplete); > + > + return 0; > +} > + > +static void dc_crtc_disable_vblank(struct drm_crtc *crtc) > +{ > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + > + disable_irq_nosync(dc_crtc->irq_dec_framecomplete); > +} > + > +static irqreturn_t > +dc_crtc_dec_framecomplete_irq_handler(int irq, void *dev_id) > +{ > + struct dc_crtc *dc_crtc = dev_id; > + struct drm_crtc *crtc = &dc_crtc->base; > + unsigned long flags; > + > + drm_crtc_handle_vblank(crtc); > + > + spin_lock_irqsave(&crtc->dev->event_lock, flags); > + if (dc_crtc->event) { > + drm_crtc_send_vblank_event(crtc, dc_crtc->event); > + dc_crtc->event = NULL; > + drm_crtc_vblank_put(crtc); > + } > + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t dc_crtc_common_irq_handler(int irq, void *dev_id) > +{ > + struct dc_crtc *dc_crtc = dev_id; > + struct drm_crtc *crtc = &dc_crtc->base; > + > + if (irq == dc_crtc->irq_dec_seqcomplete) { > + complete(&dc_crtc->dec_seqcomplete_done); > + } else if (irq == dc_crtc->irq_dec_shdld) { > + complete(&dc_crtc->dec_shdld_done); > + } else if (irq == dc_crtc->irq_ed_cont_shdld) { > + complete(&dc_crtc->ed_cont_shdld_done); > + } else if (irq == dc_crtc->irq_ed_safe_shdld) { > + complete(&dc_crtc->ed_safe_shdld_done); > + } else { > + dc_crtc_err(crtc, "invalid CRTC irq(%u)\n", irq); > + return IRQ_NONE; And this is a counter-example to my previous questions. If you had 4 separate handlers, there would have been no need for the futile "invalid CRTC" error, because it would not be possible at all. > + } > + > + return IRQ_HANDLED; > +} > + > +static const struct drm_crtc_funcs dc_crtc_funcs = { > + .reset = drm_atomic_helper_crtc_reset, > + .destroy = drm_crtc_cleanup, > + .set_config = drm_atomic_helper_set_config, > + .page_flip = drm_atomic_helper_page_flip, > + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > + .get_vblank_counter = dc_crtc_get_vblank_counter, > + .enable_vblank = dc_crtc_enable_vblank, > + .disable_vblank = dc_crtc_disable_vblank, > + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp, > +}; > + > +static void dc_crtc_queue_state_event(struct drm_crtc_state *crtc_state) > +{ > + struct drm_crtc *crtc = crtc_state->crtc; > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (crtc_state->event) { > + WARN_ON(drm_crtc_vblank_get(crtc)); > + WARN_ON(dc_crtc->event); > + dc_crtc->event = crtc_state->event; > + crtc_state->event = NULL; > + } > + spin_unlock_irq(&crtc->dev->event_lock); > +} > + > +static enum drm_mode_status > +dc_crtc_check_clock(struct dc_crtc *dc_crtc, int clk_khz) > +{ > + return dc_fg_check_clock(dc_crtc->fg, clk_khz); > +} > + > +static enum drm_mode_status > +dc_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) > +{ > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + enum drm_mode_status status; > + > + status = dc_crtc_check_clock(dc_crtc, mode->clock); > + if (status != MODE_OK) > + return status; > + > + if (mode->crtc_clock > DC_FRAMEGEN_MAX_CLOCK_KHZ) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > +static int > +dc_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) > +{ > + struct drm_crtc_state *new_crtc_state = > + drm_atomic_get_new_crtc_state(state, crtc); > + struct drm_display_mode *adj = &new_crtc_state->adjusted_mode; > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + enum drm_mode_status status; > + > + status = dc_crtc_check_clock(dc_crtc, adj->clock); > + if (status != MODE_OK) > + return -EINVAL; > + > + return 0; > +} > + > +static void > +dc_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) > +{ > + struct drm_crtc_state *new_crtc_state = > + drm_atomic_get_new_crtc_state(state, crtc); > + struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev); > + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > + int idx, ret; > + > + if (!drm_atomic_crtc_needs_modeset(new_crtc_state) || > + !new_crtc_state->active) > + return; Why? Can it be called under such conditions? > + > + if (!drm_dev_enter(crtc->dev, &idx)) > + return; Can you please give an example of a driver using drm_dev_enter()/_exit() in DRM callbacks? > + > + /* request pixel engine power-on when CRTC starts to be active */ > + ret = pm_runtime_resume_and_get(dc_crtc->pe->dev); This function doesn't return an error. So if pm_runtime_resume_and_get() didn't increment the counter, corresponding pm_runtime_put() might cause an underflow. Instead void functions should use pm_runtime_get_sync() Also can any of the code running afterwards result in the unclocked exception if resume fails? > + if (ret) > + dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n", > + ret); > + > + atomic_inc(&dc_drm->pe_rpm_count); Why do you need a separate RPM count? RPM code already has one. > + > + drm_dev_exit(idx); > +} > + [...] > + > +static int > +dc_crtc_request_irq(struct dc_crtc *dc_crtc, struct device *dev, > + unsigned int irq, > + irqreturn_t (*irq_handler)(int irq, void *dev_id)) > +{ > + int ret; > + > + ret = request_irq(irq, irq_handler, IRQF_NO_AUTOEN, dev_name(dev), > + dc_crtc); > + if (ret < 0) > + dev_err(dev, "failed to request irq(%u): %d\n", irq, ret); > + > + return ret; > +} > + > +static int dc_crtc_request_irqs(struct drm_device *drm, struct dc_crtc *dc_crtc) > +{ > + struct { > + struct device *dev; > + unsigned int irq; > + irqreturn_t (*irq_handler)(int irq, void *dev_id); > + } irqs[] = { > + { > + dc_crtc->de->dev, > + dc_crtc->irq_dec_framecomplete, > + dc_crtc_dec_framecomplete_irq_handler, > + }, { > + dc_crtc->de->dev, > + dc_crtc->irq_dec_seqcomplete, > + dc_crtc_common_irq_handler, > + }, { > + dc_crtc->de->dev, > + dc_crtc->irq_dec_shdld, > + dc_crtc_common_irq_handler, > + }, { > + dc_crtc->ed_cont->dev, > + dc_crtc->irq_ed_cont_shdld, > + dc_crtc_common_irq_handler, > + }, { > + dc_crtc->ed_safe->dev, > + dc_crtc->irq_ed_safe_shdld, > + dc_crtc_common_irq_handler, > + }, > + }; > + struct drm_crtc *crtc = &dc_crtc->base; > + int i, ret; > + > + dc_crtc->irqs = drmm_kcalloc(drm, ARRAY_SIZE(irqs), > + sizeof(*dc_crtc->irqs), GFP_KERNEL); Using array would remove the necessity to call drmm_kcalloc here(). > + if (!dc_crtc->irqs) { > + dev_err(drm->dev, "failed to allocate CRTC%u irqs\n", > + crtc->index); > + return -ENOMEM; > + } > + > + for (i = 0; i < ARRAY_SIZE(irqs); i++) { > + struct dc_crtc_irq *irq = &dc_crtc->irqs[i]; > + > + ret = dc_crtc_request_irq(dc_crtc, irqs[i].dev, irqs[i].irq, > + irqs[i].irq_handler); > + if (ret) > + return ret; > + > + irq->dc_crtc = dc_crtc; > + irq->irq = irqs[i].irq; > + > + ret = drmm_add_action_or_reset(drm, dc_crtc_free_irq, irq); Can you use devm_request_irq() instead? > + if (ret) > + return ret; > + } > + > + return 0; > +} > + [...] > + > +static int dc_kms_init_encoder_per_crtc(struct dc_drm_device *dc_drm, > + int crtc_index) > +{ > + struct dc_crtc *dc_crtc = &dc_drm->dc_crtc[crtc_index]; > + struct drm_device *drm = &dc_drm->base; > + struct drm_crtc *crtc = &dc_crtc->base; > + struct drm_connector *connector; > + struct device *dev = drm->dev; > + struct drm_encoder *encoder; > + struct device_node *remote; > + struct drm_bridge *bridge; > + int ret = 0; > + > + remote = of_graph_get_remote_node(dc_crtc->de->tc->dev->of_node, 0, -1); > + if (!of_device_is_available(remote)) > + goto out; > + > + bridge = of_drm_find_bridge(remote); drm_of_find_panel_or_bridge() instead. > + if (!bridge) { > + ret = -EPROBE_DEFER; > + dev_err_probe(dev, ret, "failed to find bridge for CRTC%u\n", > + crtc->index); > + goto out; > + } > + > + encoder = &dc_drm->encoder[crtc_index]; > + ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); > + if (ret) { > + dev_err(dev, "failed to initialize encoder for CRTC%u: %d\n", > + crtc->index, ret); > + goto out; > + } > + > + encoder->possible_crtcs = drm_crtc_mask(crtc); > + > + ret = drm_bridge_attach(encoder, bridge, NULL, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > + if (ret) { > + dev_err(dev, > + "failed to attach bridge to encoder for CRTC%u: %d\n", > + crtc->index, ret); > + goto out; > + } > + > + connector = drm_bridge_connector_init(drm, encoder); > + if (IS_ERR(connector)) { > + ret = PTR_ERR(connector); > + dev_err(dev, "failed to init bridge connector for CRTC%u: %d\n", > + crtc->index, ret); > + goto out; > + } > + > + ret = drm_connector_attach_encoder(connector, encoder); > + if (ret) > + dev_err(dev, > + "failed to attach encoder to connector for CRTC%u: %d\n", > + crtc->index, ret); > + > +out: > + of_node_put(remote); > + return ret; > +} > + > +int dc_kms_init(struct dc_drm_device *dc_drm) > +{ > + struct drm_device *drm = &dc_drm->base; > + int ret, i; > + > + ret = drmm_mode_config_init(drm); > + if (ret) > + return ret; > + > + atomic_set(&dc_drm->pe_rpm_count, 0); > + > + drm->mode_config.min_width = 60; > + drm->mode_config.min_height = 60; > + drm->mode_config.max_width = 8192; > + drm->mode_config.max_height = 8192; > + drm->mode_config.funcs = &dc_drm_mode_config_funcs; > + > + drm->vblank_disable_immediate = true; > + drm->max_vblank_count = DC_FRAMEGEN_MAX_FRAME_INDEX; > + > + for (i = 0; i < DC_CRTCS; i++) { > + ret = dc_crtc_init(dc_drm, i); > + if (ret) > + return ret; > + > + ret = dc_kms_init_encoder_per_crtc(dc_drm, i); > + if (ret) > + return ret; > + } > + > + ret = drm_vblank_init(drm, DC_CRTCS); > + if (ret) { > + dev_err(drm->dev, "failed to init vblank support: %d\n", ret); > + return ret; > + } > + > + drm_mode_config_reset(drm); > + > + drm_kms_helper_poll_init(drm); > + > + return 0; > +} > + > +void dc_kms_uninit(struct dc_drm_device *dc_drm) > +{ > + drm_kms_helper_poll_fini(&dc_drm->base); > +} > diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h > new file mode 100644 > index 000000000000..4f66b11c106a > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-kms.h > @@ -0,0 +1,15 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright 2024 NXP > + */ > + > +#ifndef __DC_KMS_H__ > +#define __DC_KMS_H__ > + > +#include "dc-de.h" > + > +#define DC_CRTCS DC_DISPLAYS > +#define DC_ENCODERS DC_DISPLAYS > +#define DC_PRIMARYS DC_DISPLAYS If they are all equal, why do you need separate defines? > + > +#endif /* __DC_KMS_H__ */ > diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c > new file mode 100644 > index 000000000000..a49b043ca167 > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-plane.c > @@ -0,0 +1,227 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright 2024 NXP > + */ > + > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_atomic_state_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_fb_dma_helper.h> > +#include <drm/drm_fourcc.h> > +#include <drm/drm_framebuffer.h> > +#include <drm/drm_gem_atomic_helper.h> > +#include <drm/drm_plane_helper.h> > + > +#include "dc-drv.h" > +#include "dc-fu.h" > +#include "dc-plane.h" > + > +#define DC_PLANE_MAX_PITCH 0x10000 > +#define DC_PLANE_MAX_PIX_CNT 8192 > + > +static const uint32_t dc_plane_formats[] = { > + DRM_FORMAT_XRGB8888, > +}; > + > +static const struct drm_plane_funcs dc_plane_funcs = { > + .update_plane = drm_atomic_helper_update_plane, > + .disable_plane = drm_atomic_helper_disable_plane, > + .destroy = drm_plane_cleanup, > + .reset = drm_atomic_helper_plane_reset, > + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > +}; > + > +static int dc_plane_check_no_off_screen(struct drm_plane_state *state, > + struct drm_crtc_state *crtc_state) > +{ > + if (state->dst.x1 < 0 || state->dst.y1 < 0 || > + state->dst.x2 > crtc_state->adjusted_mode.hdisplay || > + state->dst.y2 > crtc_state->adjusted_mode.vdisplay) { > + dc_plane_dbg(state->plane, "no off screen\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int dc_plane_check_max_source_resolution(struct drm_plane_state *state) > +{ > + int src_h = drm_rect_height(&state->src) >> 16; > + int src_w = drm_rect_width(&state->src) >> 16; > + > + if (src_w > DC_PLANE_MAX_PIX_CNT || src_h > DC_PLANE_MAX_PIX_CNT) { > + dc_plane_dbg(state->plane, "invalid source resolution\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int dc_plane_check_fb(struct drm_plane_state *state) > +{ > + struct drm_framebuffer *fb = state->fb; > + dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0); > + > + /* base address alignment */ > + if (baseaddr & 0x3) { > + dc_plane_dbg(state->plane, "fb bad baddr alignment\n"); > + return -EINVAL; > + } > + > + /* pitches[0] range */ > + if (fb->pitches[0] > DC_PLANE_MAX_PITCH) { > + dc_plane_dbg(state->plane, "fb pitches[0] is out of range\n"); > + return -EINVAL; > + } > + > + /* pitches[0] alignment */ > + if (fb->pitches[0] & 0x3) { > + dc_plane_dbg(state->plane, "fb bad pitches[0] alignment\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int > +dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) > +{ > + struct drm_plane_state *plane_state = > + drm_atomic_get_new_plane_state(state, plane); > + struct drm_crtc_state *crtc_state; > + int ret; > + > + /* ok to disable */ > + if (!plane_state->fb) > + return 0; > + > + if (!plane_state->crtc) { > + dc_plane_dbg(plane, "no CRTC in plane state\n"); > + return -EINVAL; > + } > + > + crtc_state = > + drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > + if (WARN_ON(!crtc_state)) > + return -EINVAL; > + > + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state, > + DRM_PLANE_NO_SCALING, > + DRM_PLANE_NO_SCALING, > + true, false); > + if (ret) { > + dc_plane_dbg(plane, "failed to check plane state: %d\n", ret); > + return ret; > + } > + > + ret = dc_plane_check_no_off_screen(plane_state, crtc_state); > + if (ret) > + return ret; > + > + ret = dc_plane_check_max_source_resolution(plane_state); > + if (ret) > + return ret; > + > + return dc_plane_check_fb(plane_state); > +} > + > +static void > +dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) > +{ > + struct drm_plane_state *new_state = > + drm_atomic_get_new_plane_state(state, plane); > + struct dc_plane *dplane = to_dc_plane(plane); > + struct drm_framebuffer *fb = new_state->fb; > + const struct dc_fu_ops *fu_ops; > + struct dc_lb *lb = dplane->lb; > + struct dc_fu *fu = dplane->fu; > + dma_addr_t baseaddr; > + int src_w, src_h; > + int idx; > + > + if (!drm_dev_enter(plane->dev, &idx)) > + return; > + > + src_w = drm_rect_width(&new_state->src) >> 16; > + src_h = drm_rect_height(&new_state->src) >> 16; > + > + baseaddr = drm_fb_dma_get_gem_addr(fb, new_state, 0); > + > + fu_ops = dc_fu_get_ops(dplane->fu); > + > + fu_ops->set_layerblend(fu, lb); > + fu_ops->set_burstlength(fu, baseaddr); > + fu_ops->set_src_stride(fu, fb->pitches[0]); > + fu_ops->set_src_buf_dimensions(fu, src_w, src_h); > + fu_ops->set_fmt(fu, fb->format); > + fu_ops->set_framedimensions(fu, src_w, src_h); > + fu_ops->set_baseaddress(fu, baseaddr); > + fu_ops->enable_src_buf(fu); Are you expecting that these ops might change? Can you call the function directly? > + > + dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu)); > + > + dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf)); > + dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu)); > + dc_lb_mode(lb, LB_BLEND); > + dc_lb_blendcontrol(lb); > + dc_lb_position(lb, new_state->dst.x1, new_state->dst.y1); > + dc_lb_pec_clken(lb, CLKEN_AUTOMATIC); > + > + dc_plane_dbg(plane, "uses LayerBlend%u\n", dc_lb_get_id(lb)); > + > + /* set ExtDst's source to LayerBlend */ > + dc_ed_pec_src_sel(dplane->ed, dc_lb_get_link_id(lb)); > + > + drm_dev_exit(idx); > +} > + > +static void dc_plane_atomic_disable(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct dc_plane *dplane = to_dc_plane(plane); > + const struct dc_fu_ops *fu_ops; > + int idx; > + > + if (!drm_dev_enter(plane->dev, &idx)) > + return; > + > + /* disable fetchunit in shadow */ > + fu_ops = dc_fu_get_ops(dplane->fu); > + fu_ops->disable_src_buf(dplane->fu); > + > + /* set ExtDst's source to ConstFrame */ > + dc_ed_pec_src_sel(dplane->ed, dc_cf_get_link_id(dplane->cf)); > + > + drm_dev_exit(idx); > +} > + > +static const struct drm_plane_helper_funcs dc_plane_helper_funcs = { > + .atomic_check = dc_plane_atomic_check, > + .atomic_update = dc_plane_atomic_update, > + .atomic_disable = dc_plane_atomic_disable, > +}; > + > +int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane) > +{ > + struct drm_plane *plane = &dc_plane->base; > + int ret; > + > + ret = drm_universal_plane_init(&dc_drm->base, plane, 0, &dc_plane_funcs, > + dc_plane_formats, > + ARRAY_SIZE(dc_plane_formats), > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); > + if (ret) > + return ret; > + > + drm_plane_helper_add(plane, &dc_plane_helper_funcs); > + > + dc_plane->fu = dc_drm->pe->fu_disp[plane->index]; > + dc_plane->cf = dc_drm->pe->cf_cont[plane->index]; > + dc_plane->lb = dc_drm->pe->lb[plane->index]; > + dc_plane->ed = dc_drm->pe->ed_cont[plane->index]; > + > + return 0; > +} > diff --git a/drivers/gpu/drm/imx/dc/dc-plane.h b/drivers/gpu/drm/imx/dc/dc-plane.h > new file mode 100644 > index 000000000000..e72c3a7cb66f > --- /dev/null > +++ b/drivers/gpu/drm/imx/dc/dc-plane.h > @@ -0,0 +1,37 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright 2024 NXP > + */ > + > +#ifndef __DC_PLANE_H__ > +#define __DC_PLANE_H__ > + > +#include <linux/container_of.h> > + > +#include <drm/drm_plane.h> > +#include <drm/drm_print.h> > + > +#include "dc-fu.h" > +#include "dc-pe.h" > + > +#define dc_plane_dbg(plane, fmt, ...) \ > +do { \ > + typeof(plane) _plane = (plane); \ > + drm_dbg_kms(_plane->dev, "[PLANE:%d:%s] " fmt, \ > + _plane->base.id, _plane->name, ##__VA_ARGS__); \ > +} while (0) > + > +struct dc_plane { > + struct drm_plane base; > + struct dc_fu *fu; > + struct dc_cf *cf; > + struct dc_lb *lb; > + struct dc_ed *ed; > +}; > + > +static inline struct dc_plane *to_dc_plane(struct drm_plane *plane) > +{ > + return container_of(plane, struct dc_plane, base); > +} > + > +#endif /* __DC_PLANE_H__ */ > -- > 2.34.1 >
On 07/28/2024, Dmitry Baryshkov wrote: > On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote: >> i.MX8qxp Display Controller display engine consists of all processing >> units that operate in a display clock domain. Add minimal feature >> support with FrameGen and TCon so that the engine can output display >> timings. The display engine driver as a master binds FrameGen and >> TCon drivers as components. While at it, the display engine driver >> is a component to be bound with the upcoming DRM driver. > > Generic question: why do you need so many small subdrivers? Are they As we model processing units, interrupt controller, display engine and pixel engine as devices, relevant drivers are created to bind them. Maxime insisted on splitting the main display controller(the overall IP) into separate devices. Also, Rob asked me to document every processing units and the other sub-devices in v2. So, splitting the controller is kinda accepted from both DT PoV and DRM PoV. > used to represent the flexibility of the pipeline? Can you instantiate No. They are just used to bind the devices created from DT. > these units directly from the de(?) driver and reference created > structures without the need to create subdevices? Given the separated devices created from DT, I can't. > >> >> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> --- >> v2: >> * Use OF alias id to get instance id. >> * Add dev member to struct dc_tc. >> >> drivers/gpu/drm/imx/Kconfig | 1 + >> drivers/gpu/drm/imx/Makefile | 1 + >> drivers/gpu/drm/imx/dc/Kconfig | 5 + >> drivers/gpu/drm/imx/dc/Makefile | 5 + >> drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++++++++++ >> drivers/gpu/drm/imx/dc/dc-de.h | 62 ++++++ >> drivers/gpu/drm/imx/dc/dc-drv.c | 32 +++ >> drivers/gpu/drm/imx/dc/dc-drv.h | 24 +++ >> drivers/gpu/drm/imx/dc/dc-fg.c | 366 ++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/imx/dc/dc-tc.c | 137 ++++++++++++ >> 10 files changed, 784 insertions(+) >> create mode 100644 drivers/gpu/drm/imx/dc/Kconfig >> create mode 100644 drivers/gpu/drm/imx/dc/Makefile >> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h >> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h >> create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c >> >> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig >> index 03535a15dd8f..3e8c6edbc17c 100644 >> --- a/drivers/gpu/drm/imx/Kconfig >> +++ b/drivers/gpu/drm/imx/Kconfig >> @@ -1,5 +1,6 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> >> +source "drivers/gpu/drm/imx/dc/Kconfig" >> source "drivers/gpu/drm/imx/dcss/Kconfig" >> source "drivers/gpu/drm/imx/ipuv3/Kconfig" >> source "drivers/gpu/drm/imx/lcdc/Kconfig" >> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile >> index 86f38e7c7422..c7b317640d71 100644 >> --- a/drivers/gpu/drm/imx/Makefile >> +++ b/drivers/gpu/drm/imx/Makefile >> @@ -1,5 +1,6 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> +obj-$(CONFIG_DRM_IMX8_DC) += dc/ >> obj-$(CONFIG_DRM_IMX_DCSS) += dcss/ >> obj-$(CONFIG_DRM_IMX) += ipuv3/ >> obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/ >> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig >> new file mode 100644 >> index 000000000000..32d7471c49d0 >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/Kconfig >> @@ -0,0 +1,5 @@ >> +config DRM_IMX8_DC >> + tristate "Freescale i.MX8 Display Controller Graphics" >> + depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) >> + help >> + enable Freescale i.MX8 Display Controller(DC) graphics support >> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile >> new file mode 100644 >> index 000000000000..56de82d53d4d >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/Makefile >> @@ -0,0 +1,5 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> + >> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o >> + >> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o >> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c >> new file mode 100644 >> index 000000000000..2c8268b76b08 >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/dc-de.c >> @@ -0,0 +1,151 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright 2024 NXP >> + */ >> + >> +#include <linux/component.h> >> +#include <linux/container_of.h> >> +#include <linux/io.h> >> +#include <linux/mod_devicetable.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm.h> >> +#include <linux/pm_runtime.h> >> + >> +#include <drm/drm_managed.h> >> + >> +#include "dc-de.h" >> +#include "dc-drv.h" >> + >> +#define POLARITYCTRL 0xc >> +#define POLEN_HIGH BIT(2) >> + >> +struct dc_de_priv { >> + struct dc_de engine; >> + void __iomem *reg_top; >> +}; >> + >> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de) >> +{ >> + return container_of(de, struct dc_de_priv, engine); >> +} >> + >> +static inline void >> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value) >> +{ >> + struct dc_de_priv *priv = to_de_priv(de); >> + >> + writel(value, priv->reg_top + offset); > > Is there a point in this wrapper? Can you call writel directly? This At least, it helps finding read/write ops upon interested devices through 'git grep'. Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to have dc_*_read/write() helpers. > question generally applies to the driver. I see a lot of small functions > which can be inlined without losing the clarity. Can you please point out typical ones? > >> +} >> + >> +static void dc_dec_init(struct dc_de *de) >> +{ >> + dc_dec_write(de, POLARITYCTRL, POLEN_HIGH); >> +} >> + >> +static int dc_de_bind(struct device *dev, struct device *master, void *data) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct dc_drm_device *dc_drm = data; >> + struct dc_de_priv *priv; >> + int ret; >> + >> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top"); >> + if (IS_ERR(priv->reg_top)) >> + return PTR_ERR(priv->reg_top); >> + >> + priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload"); >> + if (priv->engine.irq_shdld < 0) >> + return priv->engine.irq_shdld; >> + >> + priv->engine.irq_framecomplete = >> + platform_get_irq_byname(pdev, "framecomplete"); >> + if (priv->engine.irq_framecomplete < 0) >> + return priv->engine.irq_framecomplete; >> + >> + priv->engine.irq_seqcomplete = >> + platform_get_irq_byname(pdev, "seqcomplete"); >> + if (priv->engine.irq_seqcomplete < 0) >> + return priv->engine.irq_seqcomplete; >> + >> + priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine"); > > Is this alias documented somewhere? Is it Acked by DT maintainers? I see aliases nodes in about 10 .yaml files as examples. If needed, I can add them to examples. Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only appropriate way to get instance id. In v1 review cycles, we've seen kinda 4 ways: 1) fsl,dc-*-id DT property Rejected by Krzystof. 2) OF alias 3) OF graph ports (Rob) This doesn't directly get instance id but just tell the connections. Since there are too many input/output options between some processing units, I hope we don't end up using this approach, as I mentioned in v1. It seems be difficult for display driver to handle those ports. VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the connections to display controllers, either. See brcm,bcm2835-hvs.yaml. 4) fsl,imx8qxp-dc-*{id} DT compatible string It doesn't seem necessary to add the id information to compatible string. > >> + if (priv->engine.id < 0) { >> + dev_err(dev, "failed to get alias id: %d\n", priv->engine.id); >> + return priv->engine.id; >> + } >> + >> + priv->engine.dev = dev; >> + >> + dev_set_drvdata(dev, priv); >> + >> + ret = devm_pm_runtime_enable(dev); >> + if (ret) >> + return ret; >> + >> + dc_drm->de[priv->engine.id] = &priv->engine; >> + >> + return 0; >> +} >> + >
On 07/28/2024, Dmitry Baryshkov wrote: > On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: >> i.MX8qxp Display Controller pixel engine consists of all processing >> units that operate in the AXI bus clock domain. Add drivers for >> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as >> well as a pixel engine driver, so that two displays with primary >> planes can be supported. The pixel engine driver as a master binds >> those unit drivers as components. While at it, the pixel engine >> driver is a component to be bound with the upcoming DRM driver. > > Same question / comment: create subnodes directly, without going > through the subdevices. A lot of small functions that would benefit > being inlined. Like I replied in patch 06/16, I can't create sub devices directly. Can you please point out typical ones for those small functions if the comment still stands? > >> +static int dc_cf_bind(struct device *dev, struct device *master, void *data) >> +{ >> + struct platform_device *pdev = to_platform_device(dev); >> + struct dc_drm_device *dc_drm = data; >> + struct dc_pe *pe = dc_drm->pe; >> + struct dc_cf_priv *priv; >> + int id; >> + >> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); >> + if (IS_ERR(priv->reg_cfg)) >> + return PTR_ERR(priv->reg_cfg); >> + >> + id = of_alias_get_id(dev->of_node, "dc0-constframe"); > > Is it documented? Acked? Like I replied in patch 06/16, I can add aliases nodes to examples, if needed. No Nak from DT maintainers I'd say, but I hope there will be direct Ack(s). > >> + if (id < 0) { >> + dev_err(dev, "failed to get alias id: %d\n", id); >> + return id; >> + } >> + >
On 30/07/2024 08:55, Liu Ying wrote: > On 07/28/2024, Dmitry Baryshkov wrote: >> On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: >>> i.MX8qxp Display Controller pixel engine consists of all processing >>> units that operate in the AXI bus clock domain. Add drivers for >>> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as >>> well as a pixel engine driver, so that two displays with primary >>> planes can be supported. The pixel engine driver as a master binds >>> those unit drivers as components. While at it, the pixel engine >>> driver is a component to be bound with the upcoming DRM driver. >> >> Same question / comment: create subnodes directly, without going >> through the subdevices. A lot of small functions that would benefit >> being inlined. > > Like I replied in patch 06/16, I can't create sub devices directly. > > Can you please point out typical ones for those small functions if > the comment still stands? > >> >>> +static int dc_cf_bind(struct device *dev, struct device *master, void *data) >>> +{ >>> + struct platform_device *pdev = to_platform_device(dev); >>> + struct dc_drm_device *dc_drm = data; >>> + struct dc_pe *pe = dc_drm->pe; >>> + struct dc_cf_priv *priv; >>> + int id; >>> + >>> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >>> + if (!priv) >>> + return -ENOMEM; >>> + >>> + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); >>> + if (IS_ERR(priv->reg_cfg)) >>> + return PTR_ERR(priv->reg_cfg); >>> + >>> + id = of_alias_get_id(dev->of_node, "dc0-constframe"); >> >> Is it documented? Acked? > > Like I replied in patch 06/16, I can add aliases nodes to examples, > if needed. > > No Nak from DT maintainers I'd say, but I hope there will be direct > Ack(s). > It was not Acked, because there was no documentation added for it. Anyway, naming is quite cryptic, e.g. "0" in "dc0" is quite confusing. Do you expect different aliases for dc1 or dc9? But anyway, aliases for sub-devices of pipeline look wrong. Best regards, Krzysztof
On 07/28/2024, Dmitry Baryshkov wrote: > On Fri, Jul 12, 2024 at 05:32:36PM GMT, Liu Ying wrote: >> i.MX8qxp Display Controller(DC) is comprised of three main components that >> include a blit engine for 2D graphics accelerations, display controller for >> display output processing, as well as a command sequencer. Add kernel >> mode setting support for the display controller part with two CRTCs and >> two primary planes(backed by FetchLayer and FetchWarp respectively). The >> registers of the display controller are accessed without command sequencer >> involved, instead just by using CPU. The command sequencer is supposed to >> be used by the blit engine. > > Generic comment: please consider moving dc_plane / dc_crtc defines to > the source files and dropping the headers. struct dc_crtc is referenced from dc-drv.h and dc-kms.c. struct dc_plane is referenced from dc-crtc.c and dc-drv.h. If no objections, I may drop dc-crtc.h and dc-plane.h, and move necessary stuff to dc-kms.h. > >> >> Signed-off-by: Liu Ying <victor.liu@nxp.com> >> --- >> v2: >> * Find next bridge from TCon's port. >> * Drop drm/drm_module.h include from dc-drv.c. >> >> drivers/gpu/drm/imx/dc/Kconfig | 2 + >> drivers/gpu/drm/imx/dc/Makefile | 5 +- >> drivers/gpu/drm/imx/dc/dc-crtc.c | 578 ++++++++++++++++++++++++++++++ >> drivers/gpu/drm/imx/dc/dc-crtc.h | 67 ++++ >> drivers/gpu/drm/imx/dc/dc-de.h | 3 + >> drivers/gpu/drm/imx/dc/dc-drv.c | 236 ++++++++++++ >> drivers/gpu/drm/imx/dc/dc-drv.h | 21 ++ >> drivers/gpu/drm/imx/dc/dc-kms.c | 143 ++++++++ >> drivers/gpu/drm/imx/dc/dc-kms.h | 15 + >> drivers/gpu/drm/imx/dc/dc-plane.c | 227 ++++++++++++ >> drivers/gpu/drm/imx/dc/dc-plane.h | 37 ++ >> 11 files changed, 1332 insertions(+), 2 deletions(-) >> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.h >> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h >> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c >> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.h >> >> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig >> index b66b815fbdf1..dac0de009273 100644 >> --- a/drivers/gpu/drm/imx/dc/Kconfig >> +++ b/drivers/gpu/drm/imx/dc/Kconfig >> @@ -1,6 +1,8 @@ >> config DRM_IMX8_DC >> tristate "Freescale i.MX8 Display Controller Graphics" >> depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) >> + select DRM_GEM_DMA_HELPER >> + select DRM_KMS_HELPER >> select GENERIC_IRQ_CHIP >> help >> enable Freescale i.MX8 Display Controller(DC) graphics support >> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile >> index 1ce3e8a8db22..b9d33c074984 100644 >> --- a/drivers/gpu/drm/imx/dc/Makefile >> +++ b/drivers/gpu/drm/imx/dc/Makefile >> @@ -1,6 +1,7 @@ >> # SPDX-License-Identifier: GPL-2.0 >> >> -imx8-dc-drm-objs := dc-cf.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o dc-fu.o \ >> - dc-fw.o dc-ic.o dc-lb.o dc-pe.o dc-tc.o >> +imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o \ >> + dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o \ >> + dc-plane.o dc-tc.o >> >> obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o >> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c >> new file mode 100644 >> index 000000000000..e151e14a6677 >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c >> @@ -0,0 +1,578 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright 2024 NXP >> + */ >> + >> +#include <linux/completion.h> >> +#include <linux/interrupt.h> >> +#include <linux/irqreturn.h> >> +#include <linux/pm_runtime.h> >> +#include <linux/spinlock.h> >> + >> +#include <drm/drm_atomic.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_atomic_state_helper.h> >> +#include <drm/drm_crtc.h> >> +#include <drm/drm_device.h> >> +#include <drm/drm_drv.h> >> +#include <drm/drm_managed.h> >> +#include <drm/drm_modes.h> >> +#include <drm/drm_modeset_helper_vtables.h> >> +#include <drm/drm_plane.h> >> +#include <drm/drm_vblank.h> >> + >> +#include "dc-crtc.h" >> +#include "dc-de.h" >> +#include "dc-drv.h" >> +#include "dc-pe.h" >> +#include "dc-plane.h" >> + >> +#define DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(c) \ >> +do { \ >> + unsigned long ret; \ >> + ret = wait_for_completion_timeout(&dc_crtc->c, HZ); \ >> + if (ret == 0) \ >> + dc_crtc_err(crtc, "%s: wait for " #c " timeout\n", \ >> + __func__); \ >> +} while (0) >> + >> +#define DC_CRTC_CHECK_FRAMEGEN_FIFO(fg) \ >> +do { \ >> + typeof(fg) _fg = (fg); \ >> + if (dc_fg_secondary_requests_to_read_empty_fifo(_fg)) { \ >> + dc_fg_secondary_clear_channel_status(_fg); \ >> + dc_crtc_err(crtc, "%s: FrameGen FIFO empty\n", \ >> + __func__); \ >> + } \ >> +} while (0) >> + >> +#define DC_CRTC_WAIT_FOR_FRAMEGEN_SECONDARY_SYNCUP(fg) \ >> +do { \ >> + if (dc_fg_wait_for_secondary_syncup(fg)) \ >> + dc_crtc_err(crtc, \ >> + "%s: FrameGen secondary channel isn't syncup\n",\ >> + __func__); \ >> +} while (0) >> + >> +static u32 dc_crtc_get_vblank_counter(struct drm_crtc *crtc) >> +{ >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + >> + return dc_fg_get_frame_index(dc_crtc->fg); >> +} >> + >> +static int dc_crtc_enable_vblank(struct drm_crtc *crtc) >> +{ >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + >> + enable_irq(dc_crtc->irq_dec_framecomplete); >> + >> + return 0; >> +} >> + >> +static void dc_crtc_disable_vblank(struct drm_crtc *crtc) >> +{ >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + >> + disable_irq_nosync(dc_crtc->irq_dec_framecomplete); >> +} >> + >> +static irqreturn_t >> +dc_crtc_dec_framecomplete_irq_handler(int irq, void *dev_id) >> +{ >> + struct dc_crtc *dc_crtc = dev_id; >> + struct drm_crtc *crtc = &dc_crtc->base; >> + unsigned long flags; >> + >> + drm_crtc_handle_vblank(crtc); >> + >> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >> + if (dc_crtc->event) { >> + drm_crtc_send_vblank_event(crtc, dc_crtc->event); >> + dc_crtc->event = NULL; >> + drm_crtc_vblank_put(crtc); >> + } >> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static irqreturn_t dc_crtc_common_irq_handler(int irq, void *dev_id) >> +{ >> + struct dc_crtc *dc_crtc = dev_id; >> + struct drm_crtc *crtc = &dc_crtc->base; >> + >> + if (irq == dc_crtc->irq_dec_seqcomplete) { >> + complete(&dc_crtc->dec_seqcomplete_done); >> + } else if (irq == dc_crtc->irq_dec_shdld) { >> + complete(&dc_crtc->dec_shdld_done); >> + } else if (irq == dc_crtc->irq_ed_cont_shdld) { >> + complete(&dc_crtc->ed_cont_shdld_done); >> + } else if (irq == dc_crtc->irq_ed_safe_shdld) { >> + complete(&dc_crtc->ed_safe_shdld_done); >> + } else { >> + dc_crtc_err(crtc, "invalid CRTC irq(%u)\n", irq); >> + return IRQ_NONE; > > And this is a counter-example to my previous questions. If you had 4 > separate handlers, there would have been no need for the futile "invalid > CRTC" error, because it would not be possible at all. Ok, will drop the else clause. Thanks. > >> + } >> + >> + return IRQ_HANDLED; >> +} >> + >> +static const struct drm_crtc_funcs dc_crtc_funcs = { >> + .reset = drm_atomic_helper_crtc_reset, >> + .destroy = drm_crtc_cleanup, >> + .set_config = drm_atomic_helper_set_config, >> + .page_flip = drm_atomic_helper_page_flip, >> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, >> + .get_vblank_counter = dc_crtc_get_vblank_counter, >> + .enable_vblank = dc_crtc_enable_vblank, >> + .disable_vblank = dc_crtc_disable_vblank, >> + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp, >> +}; >> + >> +static void dc_crtc_queue_state_event(struct drm_crtc_state *crtc_state) >> +{ >> + struct drm_crtc *crtc = crtc_state->crtc; >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + >> + spin_lock_irq(&crtc->dev->event_lock); >> + if (crtc_state->event) { >> + WARN_ON(drm_crtc_vblank_get(crtc)); >> + WARN_ON(dc_crtc->event); >> + dc_crtc->event = crtc_state->event; >> + crtc_state->event = NULL; >> + } >> + spin_unlock_irq(&crtc->dev->event_lock); >> +} >> + >> +static enum drm_mode_status >> +dc_crtc_check_clock(struct dc_crtc *dc_crtc, int clk_khz) >> +{ >> + return dc_fg_check_clock(dc_crtc->fg, clk_khz); >> +} >> + >> +static enum drm_mode_status >> +dc_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) >> +{ >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + enum drm_mode_status status; >> + >> + status = dc_crtc_check_clock(dc_crtc, mode->clock); >> + if (status != MODE_OK) >> + return status; >> + >> + if (mode->crtc_clock > DC_FRAMEGEN_MAX_CLOCK_KHZ) >> + return MODE_CLOCK_HIGH; >> + >> + return MODE_OK; >> +} >> + >> +static int >> +dc_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) >> +{ >> + struct drm_crtc_state *new_crtc_state = >> + drm_atomic_get_new_crtc_state(state, crtc); >> + struct drm_display_mode *adj = &new_crtc_state->adjusted_mode; >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + enum drm_mode_status status; >> + >> + status = dc_crtc_check_clock(dc_crtc, adj->clock); >> + if (status != MODE_OK) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static void >> +dc_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) >> +{ >> + struct drm_crtc_state *new_crtc_state = >> + drm_atomic_get_new_crtc_state(state, crtc); >> + struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev); >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >> + int idx, ret; >> + >> + if (!drm_atomic_crtc_needs_modeset(new_crtc_state) || >> + !new_crtc_state->active) >> + return; > > Why? Can it be called under such conditions? This is needed to make sure the balance of calling pm_runtime_resume_and_get(dc_crtc->pe->dev) from dc_crtc_atomic_begin() and calling pm_runtime_put(dc_crtc->pe->dev) from dc_crtc_atomic_disable(). pm_runtime_resume_and_get(dc_crtc->pe->dev) is called only when the CRTC is to be enabled with a modeset commit. > >> + >> + if (!drm_dev_enter(crtc->dev, &idx)) >> + return; > > Can you please give an example of a driver using drm_dev_enter()/_exit() > in DRM callbacks? vc4. BTW, this is required by Maxime, as noted in cover letter. > >> + >> + /* request pixel engine power-on when CRTC starts to be active */ >> + ret = pm_runtime_resume_and_get(dc_crtc->pe->dev); > > This function doesn't return an error. So if pm_runtime_resume_and_get() Kerneldoc of pm_runtime_resume_and_get() mentions error code. ' or a negative error code otherwise ' So, it may return an error. > didn't increment the counter, corresponding pm_runtime_put() might cause > an underflow. Instead void functions should use pm_runtime_get_sync() pm_runtime_resume_and_get() is called from dc_crtc_atomic_begin(), which is atomic considering the general DRM atomic KMS idea. So, if the call returns an error, the best we can do is to print the error out like this driver does IMO. The call should not fail in the first place due to the "atomic" sense, though it can fail in theory. pm_runtime_get_sync() may also return an error. And it's Kerneldoc kinda says pm_runtime_resume_and_get() is better. ' Consider using pm_runtime_resume_and_get() instead of it, especially if its return value is checked by the caller, as this is likely to result in cleaner code. ' > > Also can any of the code running afterwards result in the unclocked > exception if resume fails? Yes. But, it's all atomic anyway... > >> + if (ret) >> + dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n", >> + ret); >> + >> + atomic_inc(&dc_drm->pe_rpm_count); > > Why do you need a separate RPM count? RPM code already has one. If no objections, I will drop the count and call pm_runtime_active(dc_crtc->pe->dev) from dc_crtc_disable_at_unbind(). Thanks. > >> + >> + drm_dev_exit(idx); >> +} >> + > > > [...] > >> + >> +static int >> +dc_crtc_request_irq(struct dc_crtc *dc_crtc, struct device *dev, >> + unsigned int irq, >> + irqreturn_t (*irq_handler)(int irq, void *dev_id)) >> +{ >> + int ret; >> + >> + ret = request_irq(irq, irq_handler, IRQF_NO_AUTOEN, dev_name(dev), >> + dc_crtc); >> + if (ret < 0) >> + dev_err(dev, "failed to request irq(%u): %d\n", irq, ret); >> + >> + return ret; >> +} >> + >> +static int dc_crtc_request_irqs(struct drm_device *drm, struct dc_crtc *dc_crtc) >> +{ >> + struct { >> + struct device *dev; >> + unsigned int irq; >> + irqreturn_t (*irq_handler)(int irq, void *dev_id); >> + } irqs[] = { >> + { >> + dc_crtc->de->dev, >> + dc_crtc->irq_dec_framecomplete, >> + dc_crtc_dec_framecomplete_irq_handler, >> + }, { >> + dc_crtc->de->dev, >> + dc_crtc->irq_dec_seqcomplete, >> + dc_crtc_common_irq_handler, >> + }, { >> + dc_crtc->de->dev, >> + dc_crtc->irq_dec_shdld, >> + dc_crtc_common_irq_handler, >> + }, { >> + dc_crtc->ed_cont->dev, >> + dc_crtc->irq_ed_cont_shdld, >> + dc_crtc_common_irq_handler, >> + }, { >> + dc_crtc->ed_safe->dev, >> + dc_crtc->irq_ed_safe_shdld, >> + dc_crtc_common_irq_handler, >> + }, >> + }; >> + struct drm_crtc *crtc = &dc_crtc->base; >> + int i, ret; >> + >> + dc_crtc->irqs = drmm_kcalloc(drm, ARRAY_SIZE(irqs), >> + sizeof(*dc_crtc->irqs), GFP_KERNEL); > > Using array would remove the necessity to call drmm_kcalloc here(). Ok, I may use a macro to define the array size instead. #define DC_CRTC_IRQS 5 > >> + if (!dc_crtc->irqs) { >> + dev_err(drm->dev, "failed to allocate CRTC%u irqs\n", >> + crtc->index); >> + return -ENOMEM; >> + } >> + >> + for (i = 0; i < ARRAY_SIZE(irqs); i++) { >> + struct dc_crtc_irq *irq = &dc_crtc->irqs[i]; >> + >> + ret = dc_crtc_request_irq(dc_crtc, irqs[i].dev, irqs[i].irq, >> + irqs[i].irq_handler); >> + if (ret) >> + return ret; >> + >> + irq->dc_crtc = dc_crtc; >> + irq->irq = irqs[i].irq; >> + >> + ret = drmm_add_action_or_reset(drm, dc_crtc_free_irq, irq); > > Can you use devm_request_irq() instead? No. The requested irqs would be freed too late as devm_of_platform_populate() is called early from dc_probe(). They would be freed later than the time point where irq domain is removed from dc_ic_unbind(). That would cause a kernel Oops as I tried. > >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> + > > [...] > >> + >> +static int dc_kms_init_encoder_per_crtc(struct dc_drm_device *dc_drm, >> + int crtc_index) >> +{ >> + struct dc_crtc *dc_crtc = &dc_drm->dc_crtc[crtc_index]; >> + struct drm_device *drm = &dc_drm->base; >> + struct drm_crtc *crtc = &dc_crtc->base; >> + struct drm_connector *connector; >> + struct device *dev = drm->dev; >> + struct drm_encoder *encoder; >> + struct device_node *remote; >> + struct drm_bridge *bridge; >> + int ret = 0; >> + >> + remote = of_graph_get_remote_node(dc_crtc->de->tc->dev->of_node, 0, -1); >> + if (!of_device_is_available(remote)) >> + goto out; >> + >> + bridge = of_drm_find_bridge(remote); > > drm_of_find_panel_or_bridge() instead. Nope. The first bridge is always pixel combiner according to SoC design. It can't be a panel. > >> + if (!bridge) { >> + ret = -EPROBE_DEFER; >> + dev_err_probe(dev, ret, "failed to find bridge for CRTC%u\n", >> + crtc->index); >> + goto out; >> + } >> + >> + encoder = &dc_drm->encoder[crtc_index]; >> + ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); >> + if (ret) { >> + dev_err(dev, "failed to initialize encoder for CRTC%u: %d\n", >> + crtc->index, ret); >> + goto out; >> + } >> + >> + encoder->possible_crtcs = drm_crtc_mask(crtc); >> + >> + ret = drm_bridge_attach(encoder, bridge, NULL, >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); >> + if (ret) { >> + dev_err(dev, >> + "failed to attach bridge to encoder for CRTC%u: %d\n", >> + crtc->index, ret); >> + goto out; >> + } >> + >> + connector = drm_bridge_connector_init(drm, encoder); >> + if (IS_ERR(connector)) { >> + ret = PTR_ERR(connector); >> + dev_err(dev, "failed to init bridge connector for CRTC%u: %d\n", >> + crtc->index, ret); >> + goto out; >> + } >> + >> + ret = drm_connector_attach_encoder(connector, encoder); >> + if (ret) >> + dev_err(dev, >> + "failed to attach encoder to connector for CRTC%u: %d\n", >> + crtc->index, ret); >> + >> +out: >> + of_node_put(remote); >> + return ret; >> +} >> + >> +int dc_kms_init(struct dc_drm_device *dc_drm) >> +{ >> + struct drm_device *drm = &dc_drm->base; >> + int ret, i; >> + >> + ret = drmm_mode_config_init(drm); >> + if (ret) >> + return ret; >> + >> + atomic_set(&dc_drm->pe_rpm_count, 0); >> + >> + drm->mode_config.min_width = 60; >> + drm->mode_config.min_height = 60; >> + drm->mode_config.max_width = 8192; >> + drm->mode_config.max_height = 8192; >> + drm->mode_config.funcs = &dc_drm_mode_config_funcs; >> + >> + drm->vblank_disable_immediate = true; >> + drm->max_vblank_count = DC_FRAMEGEN_MAX_FRAME_INDEX; >> + >> + for (i = 0; i < DC_CRTCS; i++) { >> + ret = dc_crtc_init(dc_drm, i); >> + if (ret) >> + return ret; >> + >> + ret = dc_kms_init_encoder_per_crtc(dc_drm, i); >> + if (ret) >> + return ret; >> + } >> + >> + ret = drm_vblank_init(drm, DC_CRTCS); >> + if (ret) { >> + dev_err(drm->dev, "failed to init vblank support: %d\n", ret); >> + return ret; >> + } >> + >> + drm_mode_config_reset(drm); >> + >> + drm_kms_helper_poll_init(drm); >> + >> + return 0; >> +} >> + >> +void dc_kms_uninit(struct dc_drm_device *dc_drm) >> +{ >> + drm_kms_helper_poll_fini(&dc_drm->base); >> +} >> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h >> new file mode 100644 >> index 000000000000..4f66b11c106a >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/dc-kms.h >> @@ -0,0 +1,15 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright 2024 NXP >> + */ >> + >> +#ifndef __DC_KMS_H__ >> +#define __DC_KMS_H__ >> + >> +#include "dc-de.h" >> + >> +#define DC_CRTCS DC_DISPLAYS >> +#define DC_ENCODERS DC_DISPLAYS >> +#define DC_PRIMARYS DC_DISPLAYS > > If they are all equal, why do you need separate defines? Hmm, just for meaningful macro names to make code easy to read. > >> + >> +#endif /* __DC_KMS_H__ */ >> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c >> new file mode 100644 >> index 000000000000..a49b043ca167 >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/dc-plane.c >> @@ -0,0 +1,227 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright 2024 NXP >> + */ >> + >> +#include <drm/drm_atomic.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_atomic_state_helper.h> >> +#include <drm/drm_crtc.h> >> +#include <drm/drm_drv.h> >> +#include <drm/drm_fb_dma_helper.h> >> +#include <drm/drm_fourcc.h> >> +#include <drm/drm_framebuffer.h> >> +#include <drm/drm_gem_atomic_helper.h> >> +#include <drm/drm_plane_helper.h> >> + >> +#include "dc-drv.h" >> +#include "dc-fu.h" >> +#include "dc-plane.h" >> + >> +#define DC_PLANE_MAX_PITCH 0x10000 >> +#define DC_PLANE_MAX_PIX_CNT 8192 >> + >> +static const uint32_t dc_plane_formats[] = { >> + DRM_FORMAT_XRGB8888, >> +}; >> + >> +static const struct drm_plane_funcs dc_plane_funcs = { >> + .update_plane = drm_atomic_helper_update_plane, >> + .disable_plane = drm_atomic_helper_disable_plane, >> + .destroy = drm_plane_cleanup, >> + .reset = drm_atomic_helper_plane_reset, >> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, >> +}; >> + >> +static int dc_plane_check_no_off_screen(struct drm_plane_state *state, >> + struct drm_crtc_state *crtc_state) >> +{ >> + if (state->dst.x1 < 0 || state->dst.y1 < 0 || >> + state->dst.x2 > crtc_state->adjusted_mode.hdisplay || >> + state->dst.y2 > crtc_state->adjusted_mode.vdisplay) { >> + dc_plane_dbg(state->plane, "no off screen\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int dc_plane_check_max_source_resolution(struct drm_plane_state *state) >> +{ >> + int src_h = drm_rect_height(&state->src) >> 16; >> + int src_w = drm_rect_width(&state->src) >> 16; >> + >> + if (src_w > DC_PLANE_MAX_PIX_CNT || src_h > DC_PLANE_MAX_PIX_CNT) { >> + dc_plane_dbg(state->plane, "invalid source resolution\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int dc_plane_check_fb(struct drm_plane_state *state) >> +{ >> + struct drm_framebuffer *fb = state->fb; >> + dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0); >> + >> + /* base address alignment */ >> + if (baseaddr & 0x3) { >> + dc_plane_dbg(state->plane, "fb bad baddr alignment\n"); >> + return -EINVAL; >> + } >> + >> + /* pitches[0] range */ >> + if (fb->pitches[0] > DC_PLANE_MAX_PITCH) { >> + dc_plane_dbg(state->plane, "fb pitches[0] is out of range\n"); >> + return -EINVAL; >> + } >> + >> + /* pitches[0] alignment */ >> + if (fb->pitches[0] & 0x3) { >> + dc_plane_dbg(state->plane, "fb bad pitches[0] alignment\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +static int >> +dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) >> +{ >> + struct drm_plane_state *plane_state = >> + drm_atomic_get_new_plane_state(state, plane); >> + struct drm_crtc_state *crtc_state; >> + int ret; >> + >> + /* ok to disable */ >> + if (!plane_state->fb) >> + return 0; >> + >> + if (!plane_state->crtc) { >> + dc_plane_dbg(plane, "no CRTC in plane state\n"); >> + return -EINVAL; >> + } >> + >> + crtc_state = >> + drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >> + if (WARN_ON(!crtc_state)) >> + return -EINVAL; >> + >> + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state, >> + DRM_PLANE_NO_SCALING, >> + DRM_PLANE_NO_SCALING, >> + true, false); >> + if (ret) { >> + dc_plane_dbg(plane, "failed to check plane state: %d\n", ret); >> + return ret; >> + } >> + >> + ret = dc_plane_check_no_off_screen(plane_state, crtc_state); >> + if (ret) >> + return ret; >> + >> + ret = dc_plane_check_max_source_resolution(plane_state); >> + if (ret) >> + return ret; >> + >> + return dc_plane_check_fb(plane_state); >> +} >> + >> +static void >> +dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) >> +{ >> + struct drm_plane_state *new_state = >> + drm_atomic_get_new_plane_state(state, plane); >> + struct dc_plane *dplane = to_dc_plane(plane); >> + struct drm_framebuffer *fb = new_state->fb; >> + const struct dc_fu_ops *fu_ops; >> + struct dc_lb *lb = dplane->lb; >> + struct dc_fu *fu = dplane->fu; >> + dma_addr_t baseaddr; >> + int src_w, src_h; >> + int idx; >> + >> + if (!drm_dev_enter(plane->dev, &idx)) >> + return; >> + >> + src_w = drm_rect_width(&new_state->src) >> 16; >> + src_h = drm_rect_height(&new_state->src) >> 16; >> + >> + baseaddr = drm_fb_dma_get_gem_addr(fb, new_state, 0); >> + >> + fu_ops = dc_fu_get_ops(dplane->fu); >> + >> + fu_ops->set_layerblend(fu, lb); >> + fu_ops->set_burstlength(fu, baseaddr); >> + fu_ops->set_src_stride(fu, fb->pitches[0]); >> + fu_ops->set_src_buf_dimensions(fu, src_w, src_h); >> + fu_ops->set_fmt(fu, fb->format); >> + fu_ops->set_framedimensions(fu, src_w, src_h); >> + fu_ops->set_baseaddress(fu, baseaddr); >> + fu_ops->enable_src_buf(fu); > > Are you expecting that these ops might change? Can you call the function > directly? Looking at struct dc_fl and struct dc_fw, you may find struct dc_fu is the base class. These function calls take struct dc_fu as arguments so that derived instances may specify their implementations. So, I can't call their implementation functions directly. > >> + >> + dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu)); >> + >> + dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf)); >> + dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu)); >> + dc_lb_mode(lb, LB_BLEND); >> + dc_lb_blendcontrol(lb); >> + dc_lb_position(lb, new_state->dst.x1, new_state->dst.y1); >> + dc_lb_pec_clken(lb, CLKEN_AUTOMATIC); >> + >> + dc_plane_dbg(plane, "uses LayerBlend%u\n", dc_lb_get_id(lb)); >> + >> + /* set ExtDst's source to LayerBlend */ >> + dc_ed_pec_src_sel(dplane->ed, dc_lb_get_link_id(lb)); >> + >> + drm_dev_exit(idx); >> +} >> + >> +static void dc_plane_atomic_disable(struct drm_plane *plane, >> + struct drm_atomic_state *state) >> +{ >> + struct dc_plane *dplane = to_dc_plane(plane); >> + const struct dc_fu_ops *fu_ops; >> + int idx; >> + >> + if (!drm_dev_enter(plane->dev, &idx)) >> + return; >> + >> + /* disable fetchunit in shadow */ >> + fu_ops = dc_fu_get_ops(dplane->fu); >> + fu_ops->disable_src_buf(dplane->fu); >> + >> + /* set ExtDst's source to ConstFrame */ >> + dc_ed_pec_src_sel(dplane->ed, dc_cf_get_link_id(dplane->cf)); >> + >> + drm_dev_exit(idx); >> +} >> + >> +static const struct drm_plane_helper_funcs dc_plane_helper_funcs = { >> + .atomic_check = dc_plane_atomic_check, >> + .atomic_update = dc_plane_atomic_update, >> + .atomic_disable = dc_plane_atomic_disable, >> +}; >> + >> +int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane) >> +{ >> + struct drm_plane *plane = &dc_plane->base; >> + int ret; >> + >> + ret = drm_universal_plane_init(&dc_drm->base, plane, 0, &dc_plane_funcs, >> + dc_plane_formats, >> + ARRAY_SIZE(dc_plane_formats), >> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); >> + if (ret) >> + return ret; >> + >> + drm_plane_helper_add(plane, &dc_plane_helper_funcs); >> + >> + dc_plane->fu = dc_drm->pe->fu_disp[plane->index]; >> + dc_plane->cf = dc_drm->pe->cf_cont[plane->index]; >> + dc_plane->lb = dc_drm->pe->lb[plane->index]; >> + dc_plane->ed = dc_drm->pe->ed_cont[plane->index]; >> + >> + return 0; >> +} >> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.h b/drivers/gpu/drm/imx/dc/dc-plane.h >> new file mode 100644 >> index 000000000000..e72c3a7cb66f >> --- /dev/null >> +++ b/drivers/gpu/drm/imx/dc/dc-plane.h >> @@ -0,0 +1,37 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright 2024 NXP >> + */ >> + >> +#ifndef __DC_PLANE_H__ >> +#define __DC_PLANE_H__ >> + >> +#include <linux/container_of.h> >> + >> +#include <drm/drm_plane.h> >> +#include <drm/drm_print.h> >> + >> +#include "dc-fu.h" >> +#include "dc-pe.h" >> + >> +#define dc_plane_dbg(plane, fmt, ...) \ >> +do { \ >> + typeof(plane) _plane = (plane); \ >> + drm_dbg_kms(_plane->dev, "[PLANE:%d:%s] " fmt, \ >> + _plane->base.id, _plane->name, ##__VA_ARGS__); \ >> +} while (0) >> + >> +struct dc_plane { >> + struct drm_plane base; >> + struct dc_fu *fu; >> + struct dc_cf *cf; >> + struct dc_lb *lb; >> + struct dc_ed *ed; >> +}; >> + >> +static inline struct dc_plane *to_dc_plane(struct drm_plane *plane) >> +{ >> + return container_of(plane, struct dc_plane, base); >> +} >> + >> +#endif /* __DC_PLANE_H__ */ >> -- >> 2.34.1 >> >
On 07/30/2024, Krzysztof Kozlowski wrote: > On 30/07/2024 08:55, Liu Ying wrote: >> On 07/28/2024, Dmitry Baryshkov wrote: >>> On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: >>>> i.MX8qxp Display Controller pixel engine consists of all processing >>>> units that operate in the AXI bus clock domain. Add drivers for >>>> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as >>>> well as a pixel engine driver, so that two displays with primary >>>> planes can be supported. The pixel engine driver as a master binds >>>> those unit drivers as components. While at it, the pixel engine >>>> driver is a component to be bound with the upcoming DRM driver. >>> >>> Same question / comment: create subnodes directly, without going >>> through the subdevices. A lot of small functions that would benefit >>> being inlined. >> >> Like I replied in patch 06/16, I can't create sub devices directly. >> >> Can you please point out typical ones for those small functions if >> the comment still stands? >> >>> >>>> +static int dc_cf_bind(struct device *dev, struct device *master, void *data) >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct dc_drm_device *dc_drm = data; >>>> + struct dc_pe *pe = dc_drm->pe; >>>> + struct dc_cf_priv *priv; >>>> + int id; >>>> + >>>> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >>>> + if (!priv) >>>> + return -ENOMEM; >>>> + >>>> + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); >>>> + if (IS_ERR(priv->reg_cfg)) >>>> + return PTR_ERR(priv->reg_cfg); >>>> + >>>> + id = of_alias_get_id(dev->of_node, "dc0-constframe"); >>> >>> Is it documented? Acked? >> >> Like I replied in patch 06/16, I can add aliases nodes to examples, >> if needed. >> >> No Nak from DT maintainers I'd say, but I hope there will be direct >> Ack(s). >> > > It was not Acked, because there was no documentation added for it. I may add aliases nodes in examples in next version, if no objections. > Anyway, naming is quite cryptic, e.g. "0" in "dc0" is quite confusing. > Do you expect different aliases for dc1 or dc9? But anyway, aliases for Yes, I do. If the alias approach is used, DC instance ids need to be specified in aliases. > sub-devices of pipeline look wrong. Why? They are separate devices. Though I agree with Rob that aliases should be generic rather than vendor specific, it seems that there are some vendor specific aliases in upstream device trees. Any better way to specify the instance ids? OF graph ports? > > Best regards, > Krzysztof > >
On 30/07/2024 11:42, Liu Ying wrote: > On 07/30/2024, Krzysztof Kozlowski wrote: >> On 30/07/2024 08:55, Liu Ying wrote: >>> On 07/28/2024, Dmitry Baryshkov wrote: >>>> On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: >>>>> i.MX8qxp Display Controller pixel engine consists of all processing >>>>> units that operate in the AXI bus clock domain. Add drivers for >>>>> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as >>>>> well as a pixel engine driver, so that two displays with primary >>>>> planes can be supported. The pixel engine driver as a master binds >>>>> those unit drivers as components. While at it, the pixel engine >>>>> driver is a component to be bound with the upcoming DRM driver. >>>> >>>> Same question / comment: create subnodes directly, without going >>>> through the subdevices. A lot of small functions that would benefit >>>> being inlined. >>> >>> Like I replied in patch 06/16, I can't create sub devices directly. >>> >>> Can you please point out typical ones for those small functions if >>> the comment still stands? >>> >>>> >>>>> +static int dc_cf_bind(struct device *dev, struct device *master, void *data) >>>>> +{ >>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>> + struct dc_drm_device *dc_drm = data; >>>>> + struct dc_pe *pe = dc_drm->pe; >>>>> + struct dc_cf_priv *priv; >>>>> + int id; >>>>> + >>>>> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >>>>> + if (!priv) >>>>> + return -ENOMEM; >>>>> + >>>>> + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); >>>>> + if (IS_ERR(priv->reg_cfg)) >>>>> + return PTR_ERR(priv->reg_cfg); >>>>> + >>>>> + id = of_alias_get_id(dev->of_node, "dc0-constframe"); >>>> >>>> Is it documented? Acked? >>> >>> Like I replied in patch 06/16, I can add aliases nodes to examples, >>> if needed. >>> >>> No Nak from DT maintainers I'd say, but I hope there will be direct >>> Ack(s). >>> >> >> It was not Acked, because there was no documentation added for it. > > I may add aliases nodes in examples in next version, if no objections. Example is just example. It is not a documentation. You must explain it in the binding, e.g. description. > >> Anyway, naming is quite cryptic, e.g. "0" in "dc0" is quite confusing. >> Do you expect different aliases for dc1 or dc9? But anyway, aliases for > > Yes, I do. If the alias approach is used, DC instance ids need to be > specified in aliases. Really? Uh, that does not look good. I tend to like this binding less and less. Best regards, Krzysztof
On 07/30/2024, Krzysztof Kozlowski wrote: > On 30/07/2024 11:42, Liu Ying wrote: >> On 07/30/2024, Krzysztof Kozlowski wrote: >>> On 30/07/2024 08:55, Liu Ying wrote: >>>> On 07/28/2024, Dmitry Baryshkov wrote: >>>>> On Fri, Jul 12, 2024 at 05:32:34PM GMT, Liu Ying wrote: >>>>>> i.MX8qxp Display Controller pixel engine consists of all processing >>>>>> units that operate in the AXI bus clock domain. Add drivers for >>>>>> ConstFrame, ExtDst, FetchLayer, FetchWarp and LayerBlend units, as >>>>>> well as a pixel engine driver, so that two displays with primary >>>>>> planes can be supported. The pixel engine driver as a master binds >>>>>> those unit drivers as components. While at it, the pixel engine >>>>>> driver is a component to be bound with the upcoming DRM driver. >>>>> >>>>> Same question / comment: create subnodes directly, without going >>>>> through the subdevices. A lot of small functions that would benefit >>>>> being inlined. >>>> >>>> Like I replied in patch 06/16, I can't create sub devices directly. >>>> >>>> Can you please point out typical ones for those small functions if >>>> the comment still stands? >>>> >>>>> >>>>>> +static int dc_cf_bind(struct device *dev, struct device *master, void *data) >>>>>> +{ >>>>>> + struct platform_device *pdev = to_platform_device(dev); >>>>>> + struct dc_drm_device *dc_drm = data; >>>>>> + struct dc_pe *pe = dc_drm->pe; >>>>>> + struct dc_cf_priv *priv; >>>>>> + int id; >>>>>> + >>>>>> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >>>>>> + if (!priv) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + priv->reg_cfg = devm_platform_ioremap_resource_byname(pdev, "cfg"); >>>>>> + if (IS_ERR(priv->reg_cfg)) >>>>>> + return PTR_ERR(priv->reg_cfg); >>>>>> + >>>>>> + id = of_alias_get_id(dev->of_node, "dc0-constframe"); >>>>> >>>>> Is it documented? Acked? >>>> >>>> Like I replied in patch 06/16, I can add aliases nodes to examples, >>>> if needed. >>>> >>>> No Nak from DT maintainers I'd say, but I hope there will be direct >>>> Ack(s). >>>> >>> >>> It was not Acked, because there was no documentation added for it. >> >> I may add aliases nodes in examples in next version, if no objections. > > Example is just example. It is not a documentation. You must explain it > in the binding, e.g. description. Ok, I'll explain it in the dt-binding description in next version.
On Tue, Jul 30, 2024 at 02:25:41PM GMT, Liu Ying wrote: > On 07/28/2024, Dmitry Baryshkov wrote: > > On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote: > >> i.MX8qxp Display Controller display engine consists of all processing > >> units that operate in a display clock domain. Add minimal feature > >> support with FrameGen and TCon so that the engine can output display > >> timings. The display engine driver as a master binds FrameGen and > >> TCon drivers as components. While at it, the display engine driver > >> is a component to be bound with the upcoming DRM driver. > > > > Generic question: why do you need so many small subdrivers? Are they > > As we model processing units, interrupt controller, display engine > and pixel engine as devices, relevant drivers are created to bind > them. > > Maxime insisted on splitting the main display controller(the overall > IP) into separate devices. Also, Rob asked me to document every > processing units and the other sub-devices in v2. So, splitting the > controller is kinda accepted from both DT PoV and DRM PoV. I went back to the previous series, where Maxime commented on the "multiple devices glued together". With the split architecture it becomes even more strange, because now you have a separate IRQ controller which enumerates interrupts for all subdevices and then glues them back. If it was an actually split device, I'd have expected that each of subdevices has interrupts going to the external controller, without the glue controller. Or that the glue controller has a limited set of the externally-generated interrupts that it further splits into per-block IRQs. Could you please point out the patches that describe and implement the <&dc0_irqsteer> device? > > > used to represent the flexibility of the pipeline? Can you instantiate > > No. They are just used to bind the devices created from DT. > > > these units directly from the de(?) driver and reference created > > structures without the need to create subdevices? > > Given the separated devices created from DT, I can't. I'd allow Maxime to override me here, but I think that subblocks should not be described in DT, unless that is required to describe possible versatility in the display pipeline. > >> > >> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >> --- > >> v2: > >> * Use OF alias id to get instance id. > >> * Add dev member to struct dc_tc. > >> > >> drivers/gpu/drm/imx/Kconfig | 1 + > >> drivers/gpu/drm/imx/Makefile | 1 + > >> drivers/gpu/drm/imx/dc/Kconfig | 5 + > >> drivers/gpu/drm/imx/dc/Makefile | 5 + > >> drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++++++++++ > >> drivers/gpu/drm/imx/dc/dc-de.h | 62 ++++++ > >> drivers/gpu/drm/imx/dc/dc-drv.c | 32 +++ > >> drivers/gpu/drm/imx/dc/dc-drv.h | 24 +++ > >> drivers/gpu/drm/imx/dc/dc-fg.c | 366 ++++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/imx/dc/dc-tc.c | 137 ++++++++++++ > >> 10 files changed, 784 insertions(+) > >> create mode 100644 drivers/gpu/drm/imx/dc/Kconfig > >> create mode 100644 drivers/gpu/drm/imx/dc/Makefile > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c > >> > >> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig > >> index 03535a15dd8f..3e8c6edbc17c 100644 > >> --- a/drivers/gpu/drm/imx/Kconfig > >> +++ b/drivers/gpu/drm/imx/Kconfig > >> @@ -1,5 +1,6 @@ > >> # SPDX-License-Identifier: GPL-2.0-only > >> > >> +source "drivers/gpu/drm/imx/dc/Kconfig" > >> source "drivers/gpu/drm/imx/dcss/Kconfig" > >> source "drivers/gpu/drm/imx/ipuv3/Kconfig" > >> source "drivers/gpu/drm/imx/lcdc/Kconfig" > >> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile > >> index 86f38e7c7422..c7b317640d71 100644 > >> --- a/drivers/gpu/drm/imx/Makefile > >> +++ b/drivers/gpu/drm/imx/Makefile > >> @@ -1,5 +1,6 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> > >> +obj-$(CONFIG_DRM_IMX8_DC) += dc/ > >> obj-$(CONFIG_DRM_IMX_DCSS) += dcss/ > >> obj-$(CONFIG_DRM_IMX) += ipuv3/ > >> obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/ > >> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig > >> new file mode 100644 > >> index 000000000000..32d7471c49d0 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/Kconfig > >> @@ -0,0 +1,5 @@ > >> +config DRM_IMX8_DC > >> + tristate "Freescale i.MX8 Display Controller Graphics" > >> + depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) > >> + help > >> + enable Freescale i.MX8 Display Controller(DC) graphics support > >> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile > >> new file mode 100644 > >> index 000000000000..56de82d53d4d > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/Makefile > >> @@ -0,0 +1,5 @@ > >> +# SPDX-License-Identifier: GPL-2.0 > >> + > >> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o > >> + > >> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o > >> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c > >> new file mode 100644 > >> index 000000000000..2c8268b76b08 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/dc-de.c > >> @@ -0,0 +1,151 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Copyright 2024 NXP > >> + */ > >> + > >> +#include <linux/component.h> > >> +#include <linux/container_of.h> > >> +#include <linux/io.h> > >> +#include <linux/mod_devicetable.h> > >> +#include <linux/module.h> > >> +#include <linux/of.h> > >> +#include <linux/of_platform.h> > >> +#include <linux/platform_device.h> > >> +#include <linux/pm.h> > >> +#include <linux/pm_runtime.h> > >> + > >> +#include <drm/drm_managed.h> > >> + > >> +#include "dc-de.h" > >> +#include "dc-drv.h" > >> + > >> +#define POLARITYCTRL 0xc > >> +#define POLEN_HIGH BIT(2) > >> + > >> +struct dc_de_priv { > >> + struct dc_de engine; > >> + void __iomem *reg_top; > >> +}; > >> + > >> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de) > >> +{ > >> + return container_of(de, struct dc_de_priv, engine); > >> +} > >> + > >> +static inline void > >> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value) > >> +{ > >> + struct dc_de_priv *priv = to_de_priv(de); > >> + > >> + writel(value, priv->reg_top + offset); > > > > Is there a point in this wrapper? Can you call writel directly? This > > At least, it helps finding read/write ops upon interested devices through > 'git grep'. git grep writel also works. > > Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to > have dc_*_read/write() helpers. Please use regmap_update_bits instead of dc_*_write_mask. > > > question generally applies to the driver. I see a lot of small functions > > which can be inlined without losing the clarity. > > Can you please point out typical ones? dc_fg_enable_shden(), dc_fg_syncmode(), dc_dec_init() To provide an example, this is the code from dc_crtc_atomic_enable(). dc_fg_displaymode(dc_crtc->fg, FG_DM_SEC_ON_TOP); dc_fg_panic_displaymode(dc_crtc->fg, FG_DM_CONSTCOL); dc_fg_cfg_videomode(dc_crtc->fg, adj); the FG parameters are fixed here. I'd expect a single call from dc_dcrtc to dc_fg, which internally does all the settings. This removes a need to export low-level details to other modules. > > > > >> +} > >> + > >> +static void dc_dec_init(struct dc_de *de) > >> +{ > >> + dc_dec_write(de, POLARITYCTRL, POLEN_HIGH); > >> +} > >> + > >> +static int dc_de_bind(struct device *dev, struct device *master, void *data) > >> +{ > >> + struct platform_device *pdev = to_platform_device(dev); > >> + struct dc_drm_device *dc_drm = data; > >> + struct dc_de_priv *priv; > >> + int ret; > >> + > >> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); > >> + if (!priv) > >> + return -ENOMEM; > >> + > >> + priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top"); > >> + if (IS_ERR(priv->reg_top)) > >> + return PTR_ERR(priv->reg_top); > >> + > >> + priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload"); > >> + if (priv->engine.irq_shdld < 0) > >> + return priv->engine.irq_shdld; > >> + > >> + priv->engine.irq_framecomplete = > >> + platform_get_irq_byname(pdev, "framecomplete"); > >> + if (priv->engine.irq_framecomplete < 0) > >> + return priv->engine.irq_framecomplete; > >> + > >> + priv->engine.irq_seqcomplete = > >> + platform_get_irq_byname(pdev, "seqcomplete"); > >> + if (priv->engine.irq_seqcomplete < 0) > >> + return priv->engine.irq_seqcomplete; > >> + > >> + priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine"); > > > > Is this alias documented somewhere? Is it Acked by DT maintainers? > > I see aliases nodes in about 10 .yaml files as examples. > If needed, I can add them to examples. > > Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only > appropriate way to get instance id. In v1 review cycles, we've seen kinda > 4 ways: > > 1) fsl,dc-*-id DT property > Rejected by Krzystof. > > 2) OF alias > > 3) OF graph ports (Rob) > This doesn't directly get instance id but just tell the connections. > Since there are too many input/output options between some processing > units, I hope we don't end up using this approach, as I mentioned in v1. > It seems be difficult for display driver to handle those ports. > > VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the > connections to display controllers, either. See brcm,bcm2835-hvs.yaml. > > 4) fsl,imx8qxp-dc-*{id} DT compatible string > It doesn't seem necessary to add the id information to compatible string. For the similar issue we ended up hardcoding IO address / masks into the driver. This is far from being optimal (and I'd like to get away from it). If we were designing drm/msm from scratch now, we'd probably have used OF graph port IDs. > > > > >> + if (priv->engine.id < 0) { > >> + dev_err(dev, "failed to get alias id: %d\n", priv->engine.id); > >> + return priv->engine.id; > >> + } > >> + > >> + priv->engine.dev = dev; > >> + > >> + dev_set_drvdata(dev, priv); > >> + > >> + ret = devm_pm_runtime_enable(dev); > >> + if (ret) > >> + return ret; > >> + > >> + dc_drm->de[priv->engine.id] = &priv->engine; > >> + > >> + return 0; > >> +} > >> + > > > > -- > Regards, > Liu Ying >
On Tue, Jul 30, 2024 at 04:31:35PM GMT, Liu Ying wrote: > On 07/28/2024, Dmitry Baryshkov wrote: > > On Fri, Jul 12, 2024 at 05:32:36PM GMT, Liu Ying wrote: > >> i.MX8qxp Display Controller(DC) is comprised of three main components that > >> include a blit engine for 2D graphics accelerations, display controller for > >> display output processing, as well as a command sequencer. Add kernel > >> mode setting support for the display controller part with two CRTCs and > >> two primary planes(backed by FetchLayer and FetchWarp respectively). The > >> registers of the display controller are accessed without command sequencer > >> involved, instead just by using CPU. The command sequencer is supposed to > >> be used by the blit engine. > > > > Generic comment: please consider moving dc_plane / dc_crtc defines to > > the source files and dropping the headers. > > struct dc_crtc is referenced from dc-drv.h and dc-kms.c. > struct dc_plane is referenced from dc-crtc.c and dc-drv.h. > > If no objections, I may drop dc-crtc.h and dc-plane.h, > and move necessary stuff to dc-kms.h. > > > > >> > >> Signed-off-by: Liu Ying <victor.liu@nxp.com> > >> --- > >> v2: > >> * Find next bridge from TCon's port. > >> * Drop drm/drm_module.h include from dc-drv.c. > >> > >> drivers/gpu/drm/imx/dc/Kconfig | 2 + > >> drivers/gpu/drm/imx/dc/Makefile | 5 +- > >> drivers/gpu/drm/imx/dc/dc-crtc.c | 578 ++++++++++++++++++++++++++++++ > >> drivers/gpu/drm/imx/dc/dc-crtc.h | 67 ++++ > >> drivers/gpu/drm/imx/dc/dc-de.h | 3 + > >> drivers/gpu/drm/imx/dc/dc-drv.c | 236 ++++++++++++ > >> drivers/gpu/drm/imx/dc/dc-drv.h | 21 ++ > >> drivers/gpu/drm/imx/dc/dc-kms.c | 143 ++++++++ > >> drivers/gpu/drm/imx/dc/dc-kms.h | 15 + > >> drivers/gpu/drm/imx/dc/dc-plane.c | 227 ++++++++++++ > >> drivers/gpu/drm/imx/dc/dc-plane.h | 37 ++ > >> 11 files changed, 1332 insertions(+), 2 deletions(-) > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.h > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c > >> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.h > >> > >> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig > >> index b66b815fbdf1..dac0de009273 100644 > >> --- a/drivers/gpu/drm/imx/dc/Kconfig > >> +++ b/drivers/gpu/drm/imx/dc/Kconfig > >> @@ -1,6 +1,8 @@ > >> config DRM_IMX8_DC > >> tristate "Freescale i.MX8 Display Controller Graphics" > >> depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) > >> + select DRM_GEM_DMA_HELPER > >> + select DRM_KMS_HELPER > >> select GENERIC_IRQ_CHIP > >> help > >> enable Freescale i.MX8 Display Controller(DC) graphics support > >> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile > >> index 1ce3e8a8db22..b9d33c074984 100644 > >> --- a/drivers/gpu/drm/imx/dc/Makefile > >> +++ b/drivers/gpu/drm/imx/dc/Makefile > >> @@ -1,6 +1,7 @@ > >> # SPDX-License-Identifier: GPL-2.0 > >> > >> -imx8-dc-drm-objs := dc-cf.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o dc-fu.o \ > >> - dc-fw.o dc-ic.o dc-lb.o dc-pe.o dc-tc.o > >> +imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o \ > >> + dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o \ > >> + dc-plane.o dc-tc.o > >> > >> obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o > >> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c > >> new file mode 100644 > >> index 000000000000..e151e14a6677 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c > >> @@ -0,0 +1,578 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Copyright 2024 NXP > >> + */ > >> + > >> +#include <linux/completion.h> > >> +#include <linux/interrupt.h> > >> +#include <linux/irqreturn.h> > >> +#include <linux/pm_runtime.h> > >> +#include <linux/spinlock.h> > >> + > >> +#include <drm/drm_atomic.h> > >> +#include <drm/drm_atomic_helper.h> > >> +#include <drm/drm_atomic_state_helper.h> > >> +#include <drm/drm_crtc.h> > >> +#include <drm/drm_device.h> > >> +#include <drm/drm_drv.h> > >> +#include <drm/drm_managed.h> > >> +#include <drm/drm_modes.h> > >> +#include <drm/drm_modeset_helper_vtables.h> > >> +#include <drm/drm_plane.h> > >> +#include <drm/drm_vblank.h> > >> + > >> +#include "dc-crtc.h" > >> +#include "dc-de.h" > >> +#include "dc-drv.h" > >> +#include "dc-pe.h" > >> +#include "dc-plane.h" > >> + > >> +#define DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(c) \ > >> +do { \ > >> + unsigned long ret; \ > >> + ret = wait_for_completion_timeout(&dc_crtc->c, HZ); \ > >> + if (ret == 0) \ > >> + dc_crtc_err(crtc, "%s: wait for " #c " timeout\n", \ > >> + __func__); \ > >> +} while (0) > >> + > >> +#define DC_CRTC_CHECK_FRAMEGEN_FIFO(fg) \ > >> +do { \ > >> + typeof(fg) _fg = (fg); \ > >> + if (dc_fg_secondary_requests_to_read_empty_fifo(_fg)) { \ > >> + dc_fg_secondary_clear_channel_status(_fg); \ > >> + dc_crtc_err(crtc, "%s: FrameGen FIFO empty\n", \ > >> + __func__); \ > >> + } \ > >> +} while (0) > >> + > >> +#define DC_CRTC_WAIT_FOR_FRAMEGEN_SECONDARY_SYNCUP(fg) \ > >> +do { \ > >> + if (dc_fg_wait_for_secondary_syncup(fg)) \ > >> + dc_crtc_err(crtc, \ > >> + "%s: FrameGen secondary channel isn't syncup\n",\ > >> + __func__); \ > >> +} while (0) > >> + > >> +static u32 dc_crtc_get_vblank_counter(struct drm_crtc *crtc) > >> +{ > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + > >> + return dc_fg_get_frame_index(dc_crtc->fg); > >> +} > >> + > >> +static int dc_crtc_enable_vblank(struct drm_crtc *crtc) > >> +{ > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + > >> + enable_irq(dc_crtc->irq_dec_framecomplete); > >> + > >> + return 0; > >> +} > >> + > >> +static void dc_crtc_disable_vblank(struct drm_crtc *crtc) > >> +{ > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + > >> + disable_irq_nosync(dc_crtc->irq_dec_framecomplete); > >> +} > >> + > >> +static irqreturn_t > >> +dc_crtc_dec_framecomplete_irq_handler(int irq, void *dev_id) > >> +{ > >> + struct dc_crtc *dc_crtc = dev_id; > >> + struct drm_crtc *crtc = &dc_crtc->base; > >> + unsigned long flags; > >> + > >> + drm_crtc_handle_vblank(crtc); > >> + > >> + spin_lock_irqsave(&crtc->dev->event_lock, flags); > >> + if (dc_crtc->event) { > >> + drm_crtc_send_vblank_event(crtc, dc_crtc->event); > >> + dc_crtc->event = NULL; > >> + drm_crtc_vblank_put(crtc); > >> + } > >> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); > >> + > >> + return IRQ_HANDLED; > >> +} > >> + > >> +static irqreturn_t dc_crtc_common_irq_handler(int irq, void *dev_id) > >> +{ > >> + struct dc_crtc *dc_crtc = dev_id; > >> + struct drm_crtc *crtc = &dc_crtc->base; > >> + > >> + if (irq == dc_crtc->irq_dec_seqcomplete) { > >> + complete(&dc_crtc->dec_seqcomplete_done); > >> + } else if (irq == dc_crtc->irq_dec_shdld) { > >> + complete(&dc_crtc->dec_shdld_done); > >> + } else if (irq == dc_crtc->irq_ed_cont_shdld) { > >> + complete(&dc_crtc->ed_cont_shdld_done); > >> + } else if (irq == dc_crtc->irq_ed_safe_shdld) { > >> + complete(&dc_crtc->ed_safe_shdld_done); > >> + } else { > >> + dc_crtc_err(crtc, "invalid CRTC irq(%u)\n", irq); > >> + return IRQ_NONE; > > > > And this is a counter-example to my previous questions. If you had 4 > > separate handlers, there would have been no need for the futile "invalid > > CRTC" error, because it would not be possible at all. > > Ok, will drop the else clause. Thanks. > > > > >> + } > >> + > >> + return IRQ_HANDLED; > >> +} > >> + > >> +static const struct drm_crtc_funcs dc_crtc_funcs = { > >> + .reset = drm_atomic_helper_crtc_reset, > >> + .destroy = drm_crtc_cleanup, > >> + .set_config = drm_atomic_helper_set_config, > >> + .page_flip = drm_atomic_helper_page_flip, > >> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > >> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > >> + .get_vblank_counter = dc_crtc_get_vblank_counter, > >> + .enable_vblank = dc_crtc_enable_vblank, > >> + .disable_vblank = dc_crtc_disable_vblank, > >> + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp, > >> +}; > >> + > >> +static void dc_crtc_queue_state_event(struct drm_crtc_state *crtc_state) > >> +{ > >> + struct drm_crtc *crtc = crtc_state->crtc; > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + > >> + spin_lock_irq(&crtc->dev->event_lock); > >> + if (crtc_state->event) { > >> + WARN_ON(drm_crtc_vblank_get(crtc)); > >> + WARN_ON(dc_crtc->event); > >> + dc_crtc->event = crtc_state->event; > >> + crtc_state->event = NULL; > >> + } > >> + spin_unlock_irq(&crtc->dev->event_lock); > >> +} > >> + > >> +static enum drm_mode_status > >> +dc_crtc_check_clock(struct dc_crtc *dc_crtc, int clk_khz) > >> +{ > >> + return dc_fg_check_clock(dc_crtc->fg, clk_khz); > >> +} > >> + > >> +static enum drm_mode_status > >> +dc_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) > >> +{ > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + enum drm_mode_status status; > >> + > >> + status = dc_crtc_check_clock(dc_crtc, mode->clock); > >> + if (status != MODE_OK) > >> + return status; > >> + > >> + if (mode->crtc_clock > DC_FRAMEGEN_MAX_CLOCK_KHZ) > >> + return MODE_CLOCK_HIGH; > >> + > >> + return MODE_OK; > >> +} > >> + > >> +static int > >> +dc_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) > >> +{ > >> + struct drm_crtc_state *new_crtc_state = > >> + drm_atomic_get_new_crtc_state(state, crtc); > >> + struct drm_display_mode *adj = &new_crtc_state->adjusted_mode; > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + enum drm_mode_status status; > >> + > >> + status = dc_crtc_check_clock(dc_crtc, adj->clock); > >> + if (status != MODE_OK) > >> + return -EINVAL; > >> + > >> + return 0; > >> +} > >> + > >> +static void > >> +dc_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) > >> +{ > >> + struct drm_crtc_state *new_crtc_state = > >> + drm_atomic_get_new_crtc_state(state, crtc); > >> + struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev); > >> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); > >> + int idx, ret; > >> + > >> + if (!drm_atomic_crtc_needs_modeset(new_crtc_state) || > >> + !new_crtc_state->active) > >> + return; > > > > Why? Can it be called under such conditions? > > This is needed to make sure the balance of calling > pm_runtime_resume_and_get(dc_crtc->pe->dev) > from dc_crtc_atomic_begin() and calling > pm_runtime_put(dc_crtc->pe->dev) > from dc_crtc_atomic_disable(). > > pm_runtime_resume_and_get(dc_crtc->pe->dev) is called > only when the CRTC is to be enabled with a modeset > commit. > > > > >> + > >> + if (!drm_dev_enter(crtc->dev, &idx)) > >> + return; > > > > Can you please give an example of a driver using drm_dev_enter()/_exit() > > in DRM callbacks? > > vc4. > > BTW, this is required by Maxime, as noted in cover letter. > > > > >> + > >> + /* request pixel engine power-on when CRTC starts to be active */ > >> + ret = pm_runtime_resume_and_get(dc_crtc->pe->dev); > > > > This function doesn't return an error. So if pm_runtime_resume_and_get() > > Kerneldoc of pm_runtime_resume_and_get() mentions error code. > ' > or a negative error code otherwise > ' > So, it may return an error. > > > didn't increment the counter, corresponding pm_runtime_put() might cause > > an underflow. Instead void functions should use pm_runtime_get_sync() > > pm_runtime_resume_and_get() is called from dc_crtc_atomic_begin(), which > is atomic considering the general DRM atomic KMS idea. So, if the call > returns an error, the best we can do is to print the error out like > this driver does IMO. The call should not fail in the first place due > to the "atomic" sense, though it can fail in theory. > > pm_runtime_get_sync() may also return an error. And it's Kerneldoc kinda > says pm_runtime_resume_and_get() is better. > ' > Consider using pm_runtime_resume_and_get() instead of it, especially > if its return value is checked by the caller, as this is likely to result > in cleaner code. > ' > > > > > Also can any of the code running afterwards result in the unclocked > > exception if resume fails? > > Yes. But, it's all atomic anyway... > > > > >> + if (ret) > >> + dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n", > >> + ret); > >> + > >> + atomic_inc(&dc_drm->pe_rpm_count); > > > > Why do you need a separate RPM count? RPM code already has one. > > If no objections, I will drop the count and call > pm_runtime_active(dc_crtc->pe->dev) from dc_crtc_disable_at_unbind(). Why do you need the disable_at_unbind() thing? Doesn't shutdown take care of disabling it for you? > > Thanks. > > > > >> + > >> + drm_dev_exit(idx); > >> +} > >> + > > > > > > [...] > > > >> + > >> +static int > >> +dc_crtc_request_irq(struct dc_crtc *dc_crtc, struct device *dev, > >> + unsigned int irq, > >> + irqreturn_t (*irq_handler)(int irq, void *dev_id)) > >> +{ > >> + int ret; > >> + > >> + ret = request_irq(irq, irq_handler, IRQF_NO_AUTOEN, dev_name(dev), > >> + dc_crtc); > >> + if (ret < 0) > >> + dev_err(dev, "failed to request irq(%u): %d\n", irq, ret); > >> + > >> + return ret; > >> +} > >> + > >> +static int dc_crtc_request_irqs(struct drm_device *drm, struct dc_crtc *dc_crtc) > >> +{ > >> + struct { > >> + struct device *dev; > >> + unsigned int irq; > >> + irqreturn_t (*irq_handler)(int irq, void *dev_id); > >> + } irqs[] = { > >> + { > >> + dc_crtc->de->dev, > >> + dc_crtc->irq_dec_framecomplete, > >> + dc_crtc_dec_framecomplete_irq_handler, > >> + }, { > >> + dc_crtc->de->dev, > >> + dc_crtc->irq_dec_seqcomplete, > >> + dc_crtc_common_irq_handler, > >> + }, { > >> + dc_crtc->de->dev, > >> + dc_crtc->irq_dec_shdld, > >> + dc_crtc_common_irq_handler, > >> + }, { > >> + dc_crtc->ed_cont->dev, > >> + dc_crtc->irq_ed_cont_shdld, > >> + dc_crtc_common_irq_handler, > >> + }, { > >> + dc_crtc->ed_safe->dev, > >> + dc_crtc->irq_ed_safe_shdld, > >> + dc_crtc_common_irq_handler, > >> + }, > >> + }; > >> + struct drm_crtc *crtc = &dc_crtc->base; > >> + int i, ret; > >> + > >> + dc_crtc->irqs = drmm_kcalloc(drm, ARRAY_SIZE(irqs), > >> + sizeof(*dc_crtc->irqs), GFP_KERNEL); > > > > Using array would remove the necessity to call drmm_kcalloc here(). > > Ok, I may use a macro to define the array size instead. > > #define DC_CRTC_IRQS 5 Just embed the array into dc_crtc, no need for extra defines. > > > > > >> + if (!dc_crtc->irqs) { > >> + dev_err(drm->dev, "failed to allocate CRTC%u irqs\n", > >> + crtc->index); > >> + return -ENOMEM; > >> + } > >> + > >> + for (i = 0; i < ARRAY_SIZE(irqs); i++) { > >> + struct dc_crtc_irq *irq = &dc_crtc->irqs[i]; > >> + > >> + ret = dc_crtc_request_irq(dc_crtc, irqs[i].dev, irqs[i].irq, > >> + irqs[i].irq_handler); > >> + if (ret) > >> + return ret; > >> + > >> + irq->dc_crtc = dc_crtc; > >> + irq->irq = irqs[i].irq; > >> + > >> + ret = drmm_add_action_or_reset(drm, dc_crtc_free_irq, irq); > > > > Can you use devm_request_irq() instead? > > No. > > The requested irqs would be freed too late as devm_of_platform_populate() > is called early from dc_probe(). They would be freed later than the time > point where irq domain is removed from dc_ic_unbind(). That would cause > a kernel Oops as I tried. Ohh, you are using drmm here. Please don't use drmm for IRQ domains. > > > > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + return 0; > >> +} > >> + > > > > [...] > > > >> + > >> +static int dc_kms_init_encoder_per_crtc(struct dc_drm_device *dc_drm, > >> + int crtc_index) > >> +{ > >> + struct dc_crtc *dc_crtc = &dc_drm->dc_crtc[crtc_index]; > >> + struct drm_device *drm = &dc_drm->base; > >> + struct drm_crtc *crtc = &dc_crtc->base; > >> + struct drm_connector *connector; > >> + struct device *dev = drm->dev; > >> + struct drm_encoder *encoder; > >> + struct device_node *remote; > >> + struct drm_bridge *bridge; > >> + int ret = 0; > >> + > >> + remote = of_graph_get_remote_node(dc_crtc->de->tc->dev->of_node, 0, -1); > >> + if (!of_device_is_available(remote)) > >> + goto out; > >> + > >> + bridge = of_drm_find_bridge(remote); > > > > drm_of_find_panel_or_bridge() instead. > > Nope. > > The first bridge is always pixel combiner according to SoC design. > It can't be a panel. So pass NULL as a panel pointer. At least it saves you from calling of_graph_get_remote_node() manually. > >> + if (!bridge) { > >> + ret = -EPROBE_DEFER; > >> + dev_err_probe(dev, ret, "failed to find bridge for CRTC%u\n", > >> + crtc->index); > >> + goto out; > >> + } > >> + > >> + encoder = &dc_drm->encoder[crtc_index]; > >> + ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); > >> + if (ret) { > >> + dev_err(dev, "failed to initialize encoder for CRTC%u: %d\n", > >> + crtc->index, ret); > >> + goto out; > >> + } > >> + > >> + encoder->possible_crtcs = drm_crtc_mask(crtc); > >> + > >> + ret = drm_bridge_attach(encoder, bridge, NULL, > >> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > >> + if (ret) { > >> + dev_err(dev, > >> + "failed to attach bridge to encoder for CRTC%u: %d\n", > >> + crtc->index, ret); > >> + goto out; > >> + } > >> + > >> + connector = drm_bridge_connector_init(drm, encoder); > >> + if (IS_ERR(connector)) { > >> + ret = PTR_ERR(connector); > >> + dev_err(dev, "failed to init bridge connector for CRTC%u: %d\n", > >> + crtc->index, ret); > >> + goto out; > >> + } > >> + > >> + ret = drm_connector_attach_encoder(connector, encoder); > >> + if (ret) > >> + dev_err(dev, > >> + "failed to attach encoder to connector for CRTC%u: %d\n", > >> + crtc->index, ret); > >> + > >> +out: > >> + of_node_put(remote); > >> + return ret; > >> +} > >> + > >> +int dc_kms_init(struct dc_drm_device *dc_drm) > >> +{ > >> + struct drm_device *drm = &dc_drm->base; > >> + int ret, i; > >> + > >> + ret = drmm_mode_config_init(drm); > >> + if (ret) > >> + return ret; > >> + > >> + atomic_set(&dc_drm->pe_rpm_count, 0); > >> + > >> + drm->mode_config.min_width = 60; > >> + drm->mode_config.min_height = 60; > >> + drm->mode_config.max_width = 8192; > >> + drm->mode_config.max_height = 8192; > >> + drm->mode_config.funcs = &dc_drm_mode_config_funcs; > >> + > >> + drm->vblank_disable_immediate = true; > >> + drm->max_vblank_count = DC_FRAMEGEN_MAX_FRAME_INDEX; > >> + > >> + for (i = 0; i < DC_CRTCS; i++) { > >> + ret = dc_crtc_init(dc_drm, i); > >> + if (ret) > >> + return ret; > >> + > >> + ret = dc_kms_init_encoder_per_crtc(dc_drm, i); > >> + if (ret) > >> + return ret; > >> + } > >> + > >> + ret = drm_vblank_init(drm, DC_CRTCS); > >> + if (ret) { > >> + dev_err(drm->dev, "failed to init vblank support: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + drm_mode_config_reset(drm); > >> + > >> + drm_kms_helper_poll_init(drm); > >> + > >> + return 0; > >> +} > >> + > >> +void dc_kms_uninit(struct dc_drm_device *dc_drm) > >> +{ > >> + drm_kms_helper_poll_fini(&dc_drm->base); > >> +} > >> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h > >> new file mode 100644 > >> index 000000000000..4f66b11c106a > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/dc-kms.h > >> @@ -0,0 +1,15 @@ > >> +/* SPDX-License-Identifier: GPL-2.0+ */ > >> +/* > >> + * Copyright 2024 NXP > >> + */ > >> + > >> +#ifndef __DC_KMS_H__ > >> +#define __DC_KMS_H__ > >> + > >> +#include "dc-de.h" > >> + > >> +#define DC_CRTCS DC_DISPLAYS > >> +#define DC_ENCODERS DC_DISPLAYS > >> +#define DC_PRIMARYS DC_DISPLAYS > > > > If they are all equal, why do you need separate defines? > > Hmm, just for meaningful macro names to make code easy to read. From my POV it makes code harder to read, as the reviewer has to keep in mind that those values are all the same. > > > > >> + > >> +#endif /* __DC_KMS_H__ */ > >> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c > >> new file mode 100644 > >> index 000000000000..a49b043ca167 > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/dc-plane.c > >> @@ -0,0 +1,227 @@ > >> +// SPDX-License-Identifier: GPL-2.0+ > >> +/* > >> + * Copyright 2024 NXP > >> + */ > >> + > >> +#include <drm/drm_atomic.h> > >> +#include <drm/drm_atomic_helper.h> > >> +#include <drm/drm_atomic_state_helper.h> > >> +#include <drm/drm_crtc.h> > >> +#include <drm/drm_drv.h> > >> +#include <drm/drm_fb_dma_helper.h> > >> +#include <drm/drm_fourcc.h> > >> +#include <drm/drm_framebuffer.h> > >> +#include <drm/drm_gem_atomic_helper.h> > >> +#include <drm/drm_plane_helper.h> > >> + > >> +#include "dc-drv.h" > >> +#include "dc-fu.h" > >> +#include "dc-plane.h" > >> + > >> +#define DC_PLANE_MAX_PITCH 0x10000 > >> +#define DC_PLANE_MAX_PIX_CNT 8192 > >> + > >> +static const uint32_t dc_plane_formats[] = { > >> + DRM_FORMAT_XRGB8888, > >> +}; > >> + > >> +static const struct drm_plane_funcs dc_plane_funcs = { > >> + .update_plane = drm_atomic_helper_update_plane, > >> + .disable_plane = drm_atomic_helper_disable_plane, > >> + .destroy = drm_plane_cleanup, > >> + .reset = drm_atomic_helper_plane_reset, > >> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, > >> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, > >> +}; > >> + > >> +static int dc_plane_check_no_off_screen(struct drm_plane_state *state, > >> + struct drm_crtc_state *crtc_state) > >> +{ > >> + if (state->dst.x1 < 0 || state->dst.y1 < 0 || > >> + state->dst.x2 > crtc_state->adjusted_mode.hdisplay || > >> + state->dst.y2 > crtc_state->adjusted_mode.vdisplay) { > >> + dc_plane_dbg(state->plane, "no off screen\n"); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int dc_plane_check_max_source_resolution(struct drm_plane_state *state) > >> +{ > >> + int src_h = drm_rect_height(&state->src) >> 16; > >> + int src_w = drm_rect_width(&state->src) >> 16; > >> + > >> + if (src_w > DC_PLANE_MAX_PIX_CNT || src_h > DC_PLANE_MAX_PIX_CNT) { > >> + dc_plane_dbg(state->plane, "invalid source resolution\n"); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int dc_plane_check_fb(struct drm_plane_state *state) > >> +{ > >> + struct drm_framebuffer *fb = state->fb; > >> + dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0); > >> + > >> + /* base address alignment */ > >> + if (baseaddr & 0x3) { > >> + dc_plane_dbg(state->plane, "fb bad baddr alignment\n"); > >> + return -EINVAL; > >> + } > >> + > >> + /* pitches[0] range */ > >> + if (fb->pitches[0] > DC_PLANE_MAX_PITCH) { > >> + dc_plane_dbg(state->plane, "fb pitches[0] is out of range\n"); > >> + return -EINVAL; > >> + } > >> + > >> + /* pitches[0] alignment */ > >> + if (fb->pitches[0] & 0x3) { > >> + dc_plane_dbg(state->plane, "fb bad pitches[0] alignment\n"); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> +static int > >> +dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) > >> +{ > >> + struct drm_plane_state *plane_state = > >> + drm_atomic_get_new_plane_state(state, plane); > >> + struct drm_crtc_state *crtc_state; > >> + int ret; > >> + > >> + /* ok to disable */ > >> + if (!plane_state->fb) > >> + return 0; > >> + > >> + if (!plane_state->crtc) { > >> + dc_plane_dbg(plane, "no CRTC in plane state\n"); > >> + return -EINVAL; > >> + } > >> + > >> + crtc_state = > >> + drm_atomic_get_existing_crtc_state(state, plane_state->crtc); > >> + if (WARN_ON(!crtc_state)) > >> + return -EINVAL; > >> + > >> + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state, > >> + DRM_PLANE_NO_SCALING, > >> + DRM_PLANE_NO_SCALING, > >> + true, false); > >> + if (ret) { > >> + dc_plane_dbg(plane, "failed to check plane state: %d\n", ret); > >> + return ret; > >> + } > >> + > >> + ret = dc_plane_check_no_off_screen(plane_state, crtc_state); > >> + if (ret) > >> + return ret; > >> + > >> + ret = dc_plane_check_max_source_resolution(plane_state); > >> + if (ret) > >> + return ret; > >> + > >> + return dc_plane_check_fb(plane_state); > >> +} > >> + > >> +static void > >> +dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) > >> +{ > >> + struct drm_plane_state *new_state = > >> + drm_atomic_get_new_plane_state(state, plane); > >> + struct dc_plane *dplane = to_dc_plane(plane); > >> + struct drm_framebuffer *fb = new_state->fb; > >> + const struct dc_fu_ops *fu_ops; > >> + struct dc_lb *lb = dplane->lb; > >> + struct dc_fu *fu = dplane->fu; > >> + dma_addr_t baseaddr; > >> + int src_w, src_h; > >> + int idx; > >> + > >> + if (!drm_dev_enter(plane->dev, &idx)) > >> + return; > >> + > >> + src_w = drm_rect_width(&new_state->src) >> 16; > >> + src_h = drm_rect_height(&new_state->src) >> 16; > >> + > >> + baseaddr = drm_fb_dma_get_gem_addr(fb, new_state, 0); > >> + > >> + fu_ops = dc_fu_get_ops(dplane->fu); > >> + > >> + fu_ops->set_layerblend(fu, lb); > >> + fu_ops->set_burstlength(fu, baseaddr); > >> + fu_ops->set_src_stride(fu, fb->pitches[0]); > >> + fu_ops->set_src_buf_dimensions(fu, src_w, src_h); > >> + fu_ops->set_fmt(fu, fb->format); > >> + fu_ops->set_framedimensions(fu, src_w, src_h); > >> + fu_ops->set_baseaddress(fu, baseaddr); > >> + fu_ops->enable_src_buf(fu); > > > > Are you expecting that these ops might change? Can you call the function > > directly? > > Looking at struct dc_fl and struct dc_fw, you may find struct dc_fu > is the base class. These function calls take struct dc_fu as > arguments so that derived instances may specify their implementations. > > So, I can't call their implementation functions directly. Ack. > > > > >> + > >> + dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu)); > >> + > >> + dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf)); > >> + dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu)); > >> + dc_lb_mode(lb, LB_BLEND); > >> + dc_lb_blendcontrol(lb); > >> + dc_lb_position(lb, new_state->dst.x1, new_state->dst.y1); > >> + dc_lb_pec_clken(lb, CLKEN_AUTOMATIC); > >> + > >> + dc_plane_dbg(plane, "uses LayerBlend%u\n", dc_lb_get_id(lb)); > >> + > >> + /* set ExtDst's source to LayerBlend */ > >> + dc_ed_pec_src_sel(dplane->ed, dc_lb_get_link_id(lb)); > >> + > >> + drm_dev_exit(idx); > >> +} > >> + > >> +static void dc_plane_atomic_disable(struct drm_plane *plane, > >> + struct drm_atomic_state *state) > >> +{ > >> + struct dc_plane *dplane = to_dc_plane(plane); > >> + const struct dc_fu_ops *fu_ops; > >> + int idx; > >> + > >> + if (!drm_dev_enter(plane->dev, &idx)) > >> + return; > >> + > >> + /* disable fetchunit in shadow */ > >> + fu_ops = dc_fu_get_ops(dplane->fu); > >> + fu_ops->disable_src_buf(dplane->fu); > >> + > >> + /* set ExtDst's source to ConstFrame */ > >> + dc_ed_pec_src_sel(dplane->ed, dc_cf_get_link_id(dplane->cf)); > >> + > >> + drm_dev_exit(idx); > >> +} > >> + > >> +static const struct drm_plane_helper_funcs dc_plane_helper_funcs = { > >> + .atomic_check = dc_plane_atomic_check, > >> + .atomic_update = dc_plane_atomic_update, > >> + .atomic_disable = dc_plane_atomic_disable, > >> +}; > >> + > >> +int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane) > >> +{ > >> + struct drm_plane *plane = &dc_plane->base; > >> + int ret; > >> + > >> + ret = drm_universal_plane_init(&dc_drm->base, plane, 0, &dc_plane_funcs, > >> + dc_plane_formats, > >> + ARRAY_SIZE(dc_plane_formats), > >> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); > >> + if (ret) > >> + return ret; > >> + > >> + drm_plane_helper_add(plane, &dc_plane_helper_funcs); > >> + > >> + dc_plane->fu = dc_drm->pe->fu_disp[plane->index]; > >> + dc_plane->cf = dc_drm->pe->cf_cont[plane->index]; > >> + dc_plane->lb = dc_drm->pe->lb[plane->index]; > >> + dc_plane->ed = dc_drm->pe->ed_cont[plane->index]; > >> + > >> + return 0; > >> +} > >> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.h b/drivers/gpu/drm/imx/dc/dc-plane.h > >> new file mode 100644 > >> index 000000000000..e72c3a7cb66f > >> --- /dev/null > >> +++ b/drivers/gpu/drm/imx/dc/dc-plane.h > >> @@ -0,0 +1,37 @@ > >> +/* SPDX-License-Identifier: GPL-2.0+ */ > >> +/* > >> + * Copyright 2024 NXP > >> + */ > >> + > >> +#ifndef __DC_PLANE_H__ > >> +#define __DC_PLANE_H__ > >> + > >> +#include <linux/container_of.h> > >> + > >> +#include <drm/drm_plane.h> > >> +#include <drm/drm_print.h> > >> + > >> +#include "dc-fu.h" > >> +#include "dc-pe.h" > >> + > >> +#define dc_plane_dbg(plane, fmt, ...) \ > >> +do { \ > >> + typeof(plane) _plane = (plane); \ > >> + drm_dbg_kms(_plane->dev, "[PLANE:%d:%s] " fmt, \ > >> + _plane->base.id, _plane->name, ##__VA_ARGS__); \ > >> +} while (0) > >> + > >> +struct dc_plane { > >> + struct drm_plane base; > >> + struct dc_fu *fu; > >> + struct dc_cf *cf; > >> + struct dc_lb *lb; > >> + struct dc_ed *ed; > >> +}; > >> + > >> +static inline struct dc_plane *to_dc_plane(struct drm_plane *plane) > >> +{ > >> + return container_of(plane, struct dc_plane, base); > >> +} > >> + > >> +#endif /* __DC_PLANE_H__ */ > >> -- > >> 2.34.1 > >> > > > > -- > Regards, > Liu Ying >
On 07/31/2024, Dmitry Baryshkov wrote: > On Tue, Jul 30, 2024 at 02:25:41PM GMT, Liu Ying wrote: >> On 07/28/2024, Dmitry Baryshkov wrote: >>> On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote: >>>> i.MX8qxp Display Controller display engine consists of all processing >>>> units that operate in a display clock domain. Add minimal feature >>>> support with FrameGen and TCon so that the engine can output display >>>> timings. The display engine driver as a master binds FrameGen and >>>> TCon drivers as components. While at it, the display engine driver >>>> is a component to be bound with the upcoming DRM driver. >>> >>> Generic question: why do you need so many small subdrivers? Are they >> >> As we model processing units, interrupt controller, display engine >> and pixel engine as devices, relevant drivers are created to bind >> them. >> >> Maxime insisted on splitting the main display controller(the overall >> IP) into separate devices. Also, Rob asked me to document every >> processing units and the other sub-devices in v2. So, splitting the >> controller is kinda accepted from both DT PoV and DRM PoV. > > I went back to the previous series, where Maxime commented on the > "multiple devices glued together". With the split architecture it > becomes even more strange, because now you have a separate IRQ > controller which enumerates interrupts for all subdevices and then glues > them back. > > If it was an actually split device, I'd have expected that each of > subdevices has interrupts going to the external controller, without the > glue controller. Or that the glue controller has a limited set of the > externally-generated interrupts that it further splits into per-block > IRQs. People may have different opinions on whether the main display controller should be split into sub-devices or not, or even how it should be split, which looks quite subjective. Given this situation, I tend to follow the kind of agreement reached by Rob and Maxime that it should be split entirely and each processing unit should have a dt-binding doc. > > Could you please point out the patches that describe and implement the > <&dc0_irqsteer> device? drivers/irqchip/irq-imx-irqsteer.c Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.yaml You may find an irqsteer DT node in patch 13/16 in v2. > >> >>> used to represent the flexibility of the pipeline? Can you instantiate >> >> No. They are just used to bind the devices created from DT. >> >>> these units directly from the de(?) driver and reference created >>> structures without the need to create subdevices? >> >> Given the separated devices created from DT, I can't. > > I'd allow Maxime to override me here, but I think that subblocks should > not be described in DT, unless that is required to describe > possible versatility in the display pipeline. > >>>> >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >>>> --- >>>> v2: >>>> * Use OF alias id to get instance id. >>>> * Add dev member to struct dc_tc. >>>> >>>> drivers/gpu/drm/imx/Kconfig | 1 + >>>> drivers/gpu/drm/imx/Makefile | 1 + >>>> drivers/gpu/drm/imx/dc/Kconfig | 5 + >>>> drivers/gpu/drm/imx/dc/Makefile | 5 + >>>> drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++++++++++ >>>> drivers/gpu/drm/imx/dc/dc-de.h | 62 ++++++ >>>> drivers/gpu/drm/imx/dc/dc-drv.c | 32 +++ >>>> drivers/gpu/drm/imx/dc/dc-drv.h | 24 +++ >>>> drivers/gpu/drm/imx/dc/dc-fg.c | 366 ++++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/imx/dc/dc-tc.c | 137 ++++++++++++ >>>> 10 files changed, 784 insertions(+) >>>> create mode 100644 drivers/gpu/drm/imx/dc/Kconfig >>>> create mode 100644 drivers/gpu/drm/imx/dc/Makefile >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c >>>> >>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig >>>> index 03535a15dd8f..3e8c6edbc17c 100644 >>>> --- a/drivers/gpu/drm/imx/Kconfig >>>> +++ b/drivers/gpu/drm/imx/Kconfig >>>> @@ -1,5 +1,6 @@ >>>> # SPDX-License-Identifier: GPL-2.0-only >>>> >>>> +source "drivers/gpu/drm/imx/dc/Kconfig" >>>> source "drivers/gpu/drm/imx/dcss/Kconfig" >>>> source "drivers/gpu/drm/imx/ipuv3/Kconfig" >>>> source "drivers/gpu/drm/imx/lcdc/Kconfig" >>>> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile >>>> index 86f38e7c7422..c7b317640d71 100644 >>>> --- a/drivers/gpu/drm/imx/Makefile >>>> +++ b/drivers/gpu/drm/imx/Makefile >>>> @@ -1,5 +1,6 @@ >>>> # SPDX-License-Identifier: GPL-2.0 >>>> >>>> +obj-$(CONFIG_DRM_IMX8_DC) += dc/ >>>> obj-$(CONFIG_DRM_IMX_DCSS) += dcss/ >>>> obj-$(CONFIG_DRM_IMX) += ipuv3/ >>>> obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/ >>>> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig >>>> new file mode 100644 >>>> index 000000000000..32d7471c49d0 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/Kconfig >>>> @@ -0,0 +1,5 @@ >>>> +config DRM_IMX8_DC >>>> + tristate "Freescale i.MX8 Display Controller Graphics" >>>> + depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) >>>> + help >>>> + enable Freescale i.MX8 Display Controller(DC) graphics support >>>> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile >>>> new file mode 100644 >>>> index 000000000000..56de82d53d4d >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/Makefile >>>> @@ -0,0 +1,5 @@ >>>> +# SPDX-License-Identifier: GPL-2.0 >>>> + >>>> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o >>>> + >>>> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o >>>> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c >>>> new file mode 100644 >>>> index 000000000000..2c8268b76b08 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/dc-de.c >>>> @@ -0,0 +1,151 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +/* >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#include <linux/component.h> >>>> +#include <linux/container_of.h> >>>> +#include <linux/io.h> >>>> +#include <linux/mod_devicetable.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_platform.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/pm.h> >>>> +#include <linux/pm_runtime.h> >>>> + >>>> +#include <drm/drm_managed.h> >>>> + >>>> +#include "dc-de.h" >>>> +#include "dc-drv.h" >>>> + >>>> +#define POLARITYCTRL 0xc >>>> +#define POLEN_HIGH BIT(2) >>>> + >>>> +struct dc_de_priv { >>>> + struct dc_de engine; >>>> + void __iomem *reg_top; >>>> +}; >>>> + >>>> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de) >>>> +{ >>>> + return container_of(de, struct dc_de_priv, engine); >>>> +} >>>> + >>>> +static inline void >>>> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value) >>>> +{ >>>> + struct dc_de_priv *priv = to_de_priv(de); >>>> + >>>> + writel(value, priv->reg_top + offset); >>> >>> Is there a point in this wrapper? Can you call writel directly? This >> >> At least, it helps finding read/write ops upon interested devices through >> 'git grep'. > > git grep writel also works. I said "interested devices". For example, write ops upon LayerBlend can be easily found by 'git grep dc_lb_write'. 'git grep writel' is not enough to tell interested device. > >> >> Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to >> have dc_*_read/write() helpers. > > Please use regmap_update_bits instead of dc_*_write_mask. Then, you are suggesting to use both regmap_update_bits and writel. This doesn't look very consistent. Why not use regmap_write and regmap_update_bits? Anyway, this is a bit bike-shedding. If you don't have too strong opinion on this, I'd hope to keep the read/write ops as-is. > >> >>> question generally applies to the driver. I see a lot of small functions >>> which can be inlined without losing the clarity. >> >> Can you please point out typical ones? > > dc_fg_enable_shden(), dc_fg_syncmode(), dc_dec_init() I'll inline them and some others. > > To provide an example, this is the code from dc_crtc_atomic_enable(). > > dc_fg_displaymode(dc_crtc->fg, FG_DM_SEC_ON_TOP); > dc_fg_panic_displaymode(dc_crtc->fg, FG_DM_CONSTCOL); > dc_fg_cfg_videomode(dc_crtc->fg, adj); > > the FG parameters are fixed here. I'd expect a single call from dc_dcrtc > to dc_fg, which internally does all the settings. This removes a need to > export low-level details to other modules. The display modes set by dc_fg_displaymode() and dc_fg_panic_displaymode() are kinda key settings for KMS. IMHO, setting them in ->atomic_enable() makes maintenance and code reading a bit easier though with trivial and neglectable performance penalty since they are done for each mode setting. Anyway, since you asked, I'll move the two to dc_fg_init() and move some others where apply. > >> >>> >>>> +} >>>> + >>>> +static void dc_dec_init(struct dc_de *de) >>>> +{ >>>> + dc_dec_write(de, POLARITYCTRL, POLEN_HIGH); >>>> +} >>>> + >>>> +static int dc_de_bind(struct device *dev, struct device *master, void *data) >>>> +{ >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> + struct dc_drm_device *dc_drm = data; >>>> + struct dc_de_priv *priv; >>>> + int ret; >>>> + >>>> + priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL); >>>> + if (!priv) >>>> + return -ENOMEM; >>>> + >>>> + priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top"); >>>> + if (IS_ERR(priv->reg_top)) >>>> + return PTR_ERR(priv->reg_top); >>>> + >>>> + priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload"); >>>> + if (priv->engine.irq_shdld < 0) >>>> + return priv->engine.irq_shdld; >>>> + >>>> + priv->engine.irq_framecomplete = >>>> + platform_get_irq_byname(pdev, "framecomplete"); >>>> + if (priv->engine.irq_framecomplete < 0) >>>> + return priv->engine.irq_framecomplete; >>>> + >>>> + priv->engine.irq_seqcomplete = >>>> + platform_get_irq_byname(pdev, "seqcomplete"); >>>> + if (priv->engine.irq_seqcomplete < 0) >>>> + return priv->engine.irq_seqcomplete; >>>> + >>>> + priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine"); >>> >>> Is this alias documented somewhere? Is it Acked by DT maintainers? >> >> I see aliases nodes in about 10 .yaml files as examples. >> If needed, I can add them to examples. >> >> Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only >> appropriate way to get instance id. In v1 review cycles, we've seen kinda >> 4 ways: >> >> 1) fsl,dc-*-id DT property >> Rejected by Krzystof. >> >> 2) OF alias >> >> 3) OF graph ports (Rob) >> This doesn't directly get instance id but just tell the connections. >> Since there are too many input/output options between some processing >> units, I hope we don't end up using this approach, as I mentioned in v1. >> It seems be difficult for display driver to handle those ports. >> >> VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the >> connections to display controllers, either. See brcm,bcm2835-hvs.yaml. >> >> 4) fsl,imx8qxp-dc-*{id} DT compatible string >> It doesn't seem necessary to add the id information to compatible string. > > For the similar issue we ended up hardcoding IO address / masks into the > driver. This is far from being optimal (and I'd like to get away from I thought about using IO address to tell instance id in driver before v1, and chose not to do that since it's not straightforward and kinda abusing IO address. > it). If we were designing drm/msm from scratch now, we'd probably have used OF > graph port IDs. Don't know drm/msm and it's backing HW(s), so not sure how OF graph ports can bring benefits for it. For i.MX8 DC, it will be too complex for DT and display driver to use OF graph ports. > >> >>> >>>> + if (priv->engine.id < 0) { >>>> + dev_err(dev, "failed to get alias id: %d\n", priv->engine.id); >>>> + return priv->engine.id; >>>> + } >>>> + >>>> + priv->engine.dev = dev; >>>> + >>>> + dev_set_drvdata(dev, priv); >>>> + >>>> + ret = devm_pm_runtime_enable(dev); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + dc_drm->de[priv->engine.id] = &priv->engine; >>>> + >>>> + return 0; >>>> +} >>>> + >>> >> >> -- >> Regards, >> Liu Ying >> >
On 07/31/2024, Dmitry Baryshkov wrote: > On Tue, Jul 30, 2024 at 04:31:35PM GMT, Liu Ying wrote: >> On 07/28/2024, Dmitry Baryshkov wrote: >>> On Fri, Jul 12, 2024 at 05:32:36PM GMT, Liu Ying wrote: >>>> i.MX8qxp Display Controller(DC) is comprised of three main components that >>>> include a blit engine for 2D graphics accelerations, display controller for >>>> display output processing, as well as a command sequencer. Add kernel >>>> mode setting support for the display controller part with two CRTCs and >>>> two primary planes(backed by FetchLayer and FetchWarp respectively). The >>>> registers of the display controller are accessed without command sequencer >>>> involved, instead just by using CPU. The command sequencer is supposed to >>>> be used by the blit engine. >>> >>> Generic comment: please consider moving dc_plane / dc_crtc defines to >>> the source files and dropping the headers. >> >> struct dc_crtc is referenced from dc-drv.h and dc-kms.c. >> struct dc_plane is referenced from dc-crtc.c and dc-drv.h. >> >> If no objections, I may drop dc-crtc.h and dc-plane.h, >> and move necessary stuff to dc-kms.h. >> >>> >>>> >>>> Signed-off-by: Liu Ying <victor.liu@nxp.com> >>>> --- >>>> v2: >>>> * Find next bridge from TCon's port. >>>> * Drop drm/drm_module.h include from dc-drv.c. >>>> >>>> drivers/gpu/drm/imx/dc/Kconfig | 2 + >>>> drivers/gpu/drm/imx/dc/Makefile | 5 +- >>>> drivers/gpu/drm/imx/dc/dc-crtc.c | 578 ++++++++++++++++++++++++++++++ >>>> drivers/gpu/drm/imx/dc/dc-crtc.h | 67 ++++ >>>> drivers/gpu/drm/imx/dc/dc-de.h | 3 + >>>> drivers/gpu/drm/imx/dc/dc-drv.c | 236 ++++++++++++ >>>> drivers/gpu/drm/imx/dc/dc-drv.h | 21 ++ >>>> drivers/gpu/drm/imx/dc/dc-kms.c | 143 ++++++++ >>>> drivers/gpu/drm/imx/dc/dc-kms.h | 15 + >>>> drivers/gpu/drm/imx/dc/dc-plane.c | 227 ++++++++++++ >>>> drivers/gpu/drm/imx/dc/dc-plane.h | 37 ++ >>>> 11 files changed, 1332 insertions(+), 2 deletions(-) >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.h >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c >>>> create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.h >>>> >>>> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig >>>> index b66b815fbdf1..dac0de009273 100644 >>>> --- a/drivers/gpu/drm/imx/dc/Kconfig >>>> +++ b/drivers/gpu/drm/imx/dc/Kconfig >>>> @@ -1,6 +1,8 @@ >>>> config DRM_IMX8_DC >>>> tristate "Freescale i.MX8 Display Controller Graphics" >>>> depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST) >>>> + select DRM_GEM_DMA_HELPER >>>> + select DRM_KMS_HELPER >>>> select GENERIC_IRQ_CHIP >>>> help >>>> enable Freescale i.MX8 Display Controller(DC) graphics support >>>> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile >>>> index 1ce3e8a8db22..b9d33c074984 100644 >>>> --- a/drivers/gpu/drm/imx/dc/Makefile >>>> +++ b/drivers/gpu/drm/imx/dc/Makefile >>>> @@ -1,6 +1,7 @@ >>>> # SPDX-License-Identifier: GPL-2.0 >>>> >>>> -imx8-dc-drm-objs := dc-cf.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o dc-fu.o \ >>>> - dc-fw.o dc-ic.o dc-lb.o dc-pe.o dc-tc.o >>>> +imx8-dc-drm-objs := dc-cf.o dc-crtc.o dc-de.o dc-drv.o dc-ed.o dc-fg.o dc-fl.o \ >>>> + dc-fu.o dc-fw.o dc-ic.o dc-kms.o dc-lb.o dc-pe.o \ >>>> + dc-plane.o dc-tc.o >>>> >>>> obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o >>>> diff --git a/drivers/gpu/drm/imx/dc/dc-crtc.c b/drivers/gpu/drm/imx/dc/dc-crtc.c >>>> new file mode 100644 >>>> index 000000000000..e151e14a6677 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/dc-crtc.c >>>> @@ -0,0 +1,578 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +/* >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#include <linux/completion.h> >>>> +#include <linux/interrupt.h> >>>> +#include <linux/irqreturn.h> >>>> +#include <linux/pm_runtime.h> >>>> +#include <linux/spinlock.h> >>>> + >>>> +#include <drm/drm_atomic.h> >>>> +#include <drm/drm_atomic_helper.h> >>>> +#include <drm/drm_atomic_state_helper.h> >>>> +#include <drm/drm_crtc.h> >>>> +#include <drm/drm_device.h> >>>> +#include <drm/drm_drv.h> >>>> +#include <drm/drm_managed.h> >>>> +#include <drm/drm_modes.h> >>>> +#include <drm/drm_modeset_helper_vtables.h> >>>> +#include <drm/drm_plane.h> >>>> +#include <drm/drm_vblank.h> >>>> + >>>> +#include "dc-crtc.h" >>>> +#include "dc-de.h" >>>> +#include "dc-drv.h" >>>> +#include "dc-pe.h" >>>> +#include "dc-plane.h" >>>> + >>>> +#define DC_CRTC_WAIT_FOR_COMPLETION_TIMEOUT(c) \ >>>> +do { \ >>>> + unsigned long ret; \ >>>> + ret = wait_for_completion_timeout(&dc_crtc->c, HZ); \ >>>> + if (ret == 0) \ >>>> + dc_crtc_err(crtc, "%s: wait for " #c " timeout\n", \ >>>> + __func__); \ >>>> +} while (0) >>>> + >>>> +#define DC_CRTC_CHECK_FRAMEGEN_FIFO(fg) \ >>>> +do { \ >>>> + typeof(fg) _fg = (fg); \ >>>> + if (dc_fg_secondary_requests_to_read_empty_fifo(_fg)) { \ >>>> + dc_fg_secondary_clear_channel_status(_fg); \ >>>> + dc_crtc_err(crtc, "%s: FrameGen FIFO empty\n", \ >>>> + __func__); \ >>>> + } \ >>>> +} while (0) >>>> + >>>> +#define DC_CRTC_WAIT_FOR_FRAMEGEN_SECONDARY_SYNCUP(fg) \ >>>> +do { \ >>>> + if (dc_fg_wait_for_secondary_syncup(fg)) \ >>>> + dc_crtc_err(crtc, \ >>>> + "%s: FrameGen secondary channel isn't syncup\n",\ >>>> + __func__); \ >>>> +} while (0) >>>> + >>>> +static u32 dc_crtc_get_vblank_counter(struct drm_crtc *crtc) >>>> +{ >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + >>>> + return dc_fg_get_frame_index(dc_crtc->fg); >>>> +} >>>> + >>>> +static int dc_crtc_enable_vblank(struct drm_crtc *crtc) >>>> +{ >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + >>>> + enable_irq(dc_crtc->irq_dec_framecomplete); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void dc_crtc_disable_vblank(struct drm_crtc *crtc) >>>> +{ >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + >>>> + disable_irq_nosync(dc_crtc->irq_dec_framecomplete); >>>> +} >>>> + >>>> +static irqreturn_t >>>> +dc_crtc_dec_framecomplete_irq_handler(int irq, void *dev_id) >>>> +{ >>>> + struct dc_crtc *dc_crtc = dev_id; >>>> + struct drm_crtc *crtc = &dc_crtc->base; >>>> + unsigned long flags; >>>> + >>>> + drm_crtc_handle_vblank(crtc); >>>> + >>>> + spin_lock_irqsave(&crtc->dev->event_lock, flags); >>>> + if (dc_crtc->event) { >>>> + drm_crtc_send_vblank_event(crtc, dc_crtc->event); >>>> + dc_crtc->event = NULL; >>>> + drm_crtc_vblank_put(crtc); >>>> + } >>>> + spin_unlock_irqrestore(&crtc->dev->event_lock, flags); >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static irqreturn_t dc_crtc_common_irq_handler(int irq, void *dev_id) >>>> +{ >>>> + struct dc_crtc *dc_crtc = dev_id; >>>> + struct drm_crtc *crtc = &dc_crtc->base; >>>> + >>>> + if (irq == dc_crtc->irq_dec_seqcomplete) { >>>> + complete(&dc_crtc->dec_seqcomplete_done); >>>> + } else if (irq == dc_crtc->irq_dec_shdld) { >>>> + complete(&dc_crtc->dec_shdld_done); >>>> + } else if (irq == dc_crtc->irq_ed_cont_shdld) { >>>> + complete(&dc_crtc->ed_cont_shdld_done); >>>> + } else if (irq == dc_crtc->irq_ed_safe_shdld) { >>>> + complete(&dc_crtc->ed_safe_shdld_done); >>>> + } else { >>>> + dc_crtc_err(crtc, "invalid CRTC irq(%u)\n", irq); >>>> + return IRQ_NONE; >>> >>> And this is a counter-example to my previous questions. If you had 4 >>> separate handlers, there would have been no need for the futile "invalid >>> CRTC" error, because it would not be possible at all. >> >> Ok, will drop the else clause. Thanks. >> >>> >>>> + } >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static const struct drm_crtc_funcs dc_crtc_funcs = { >>>> + .reset = drm_atomic_helper_crtc_reset, >>>> + .destroy = drm_crtc_cleanup, >>>> + .set_config = drm_atomic_helper_set_config, >>>> + .page_flip = drm_atomic_helper_page_flip, >>>> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, >>>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, >>>> + .get_vblank_counter = dc_crtc_get_vblank_counter, >>>> + .enable_vblank = dc_crtc_enable_vblank, >>>> + .disable_vblank = dc_crtc_disable_vblank, >>>> + .get_vblank_timestamp = drm_crtc_vblank_helper_get_vblank_timestamp, >>>> +}; >>>> + >>>> +static void dc_crtc_queue_state_event(struct drm_crtc_state *crtc_state) >>>> +{ >>>> + struct drm_crtc *crtc = crtc_state->crtc; >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + >>>> + spin_lock_irq(&crtc->dev->event_lock); >>>> + if (crtc_state->event) { >>>> + WARN_ON(drm_crtc_vblank_get(crtc)); >>>> + WARN_ON(dc_crtc->event); >>>> + dc_crtc->event = crtc_state->event; >>>> + crtc_state->event = NULL; >>>> + } >>>> + spin_unlock_irq(&crtc->dev->event_lock); >>>> +} >>>> + >>>> +static enum drm_mode_status >>>> +dc_crtc_check_clock(struct dc_crtc *dc_crtc, int clk_khz) >>>> +{ >>>> + return dc_fg_check_clock(dc_crtc->fg, clk_khz); >>>> +} >>>> + >>>> +static enum drm_mode_status >>>> +dc_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode) >>>> +{ >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + enum drm_mode_status status; >>>> + >>>> + status = dc_crtc_check_clock(dc_crtc, mode->clock); >>>> + if (status != MODE_OK) >>>> + return status; >>>> + >>>> + if (mode->crtc_clock > DC_FRAMEGEN_MAX_CLOCK_KHZ) >>>> + return MODE_CLOCK_HIGH; >>>> + >>>> + return MODE_OK; >>>> +} >>>> + >>>> +static int >>>> +dc_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) >>>> +{ >>>> + struct drm_crtc_state *new_crtc_state = >>>> + drm_atomic_get_new_crtc_state(state, crtc); >>>> + struct drm_display_mode *adj = &new_crtc_state->adjusted_mode; >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + enum drm_mode_status status; >>>> + >>>> + status = dc_crtc_check_clock(dc_crtc, adj->clock); >>>> + if (status != MODE_OK) >>>> + return -EINVAL; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static void >>>> +dc_crtc_atomic_begin(struct drm_crtc *crtc, struct drm_atomic_state *state) >>>> +{ >>>> + struct drm_crtc_state *new_crtc_state = >>>> + drm_atomic_get_new_crtc_state(state, crtc); >>>> + struct dc_drm_device *dc_drm = to_dc_drm_device(crtc->dev); >>>> + struct dc_crtc *dc_crtc = to_dc_crtc(crtc); >>>> + int idx, ret; >>>> + >>>> + if (!drm_atomic_crtc_needs_modeset(new_crtc_state) || >>>> + !new_crtc_state->active) >>>> + return; >>> >>> Why? Can it be called under such conditions? >> >> This is needed to make sure the balance of calling >> pm_runtime_resume_and_get(dc_crtc->pe->dev) >> from dc_crtc_atomic_begin() and calling >> pm_runtime_put(dc_crtc->pe->dev) >> from dc_crtc_atomic_disable(). >> >> pm_runtime_resume_and_get(dc_crtc->pe->dev) is called >> only when the CRTC is to be enabled with a modeset >> commit. >> >>> >>>> + >>>> + if (!drm_dev_enter(crtc->dev, &idx)) >>>> + return; >>> >>> Can you please give an example of a driver using drm_dev_enter()/_exit() >>> in DRM callbacks? >> >> vc4. >> >> BTW, this is required by Maxime, as noted in cover letter. >> >>> >>>> + >>>> + /* request pixel engine power-on when CRTC starts to be active */ >>>> + ret = pm_runtime_resume_and_get(dc_crtc->pe->dev); >>> >>> This function doesn't return an error. So if pm_runtime_resume_and_get() >> >> Kerneldoc of pm_runtime_resume_and_get() mentions error code. >> ' >> or a negative error code otherwise >> ' >> So, it may return an error. >> >>> didn't increment the counter, corresponding pm_runtime_put() might cause >>> an underflow. Instead void functions should use pm_runtime_get_sync() >> >> pm_runtime_resume_and_get() is called from dc_crtc_atomic_begin(), which >> is atomic considering the general DRM atomic KMS idea. So, if the call >> returns an error, the best we can do is to print the error out like >> this driver does IMO. The call should not fail in the first place due >> to the "atomic" sense, though it can fail in theory. >> >> pm_runtime_get_sync() may also return an error. And it's Kerneldoc kinda >> says pm_runtime_resume_and_get() is better. >> ' >> Consider using pm_runtime_resume_and_get() instead of it, especially >> if its return value is checked by the caller, as this is likely to result >> in cleaner code. >> ' >> >>> >>> Also can any of the code running afterwards result in the unclocked >>> exception if resume fails? >> >> Yes. But, it's all atomic anyway... >> >>> >>>> + if (ret) >>>> + dc_crtc_err(crtc, "failed to get DC pixel engine RPM: %d\n", >>>> + ret); >>>> + >>>> + atomic_inc(&dc_drm->pe_rpm_count); >>> >>> Why do you need a separate RPM count? RPM code already has one. >> >> If no objections, I will drop the count and call >> pm_runtime_active(dc_crtc->pe->dev) from dc_crtc_disable_at_unbind(). > > Why do you need the disable_at_unbind() thing? Doesn't shutdown take > care of disabling it for you? It is used to balance PM runtime usage counter, because shutdown is called after drm_dev_unplug() which means pm_runtime_put() function calls in ->atomic_disable() are skipped due to drm_dev_enter(). > >> >> Thanks. >> >>> >>>> + >>>> + drm_dev_exit(idx); >>>> +} >>>> + >>> >>> >>> [...] >>> >>>> + >>>> +static int >>>> +dc_crtc_request_irq(struct dc_crtc *dc_crtc, struct device *dev, >>>> + unsigned int irq, >>>> + irqreturn_t (*irq_handler)(int irq, void *dev_id)) >>>> +{ >>>> + int ret; >>>> + >>>> + ret = request_irq(irq, irq_handler, IRQF_NO_AUTOEN, dev_name(dev), >>>> + dc_crtc); >>>> + if (ret < 0) >>>> + dev_err(dev, "failed to request irq(%u): %d\n", irq, ret); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static int dc_crtc_request_irqs(struct drm_device *drm, struct dc_crtc *dc_crtc) >>>> +{ >>>> + struct { >>>> + struct device *dev; >>>> + unsigned int irq; >>>> + irqreturn_t (*irq_handler)(int irq, void *dev_id); >>>> + } irqs[] = { >>>> + { >>>> + dc_crtc->de->dev, >>>> + dc_crtc->irq_dec_framecomplete, >>>> + dc_crtc_dec_framecomplete_irq_handler, >>>> + }, { >>>> + dc_crtc->de->dev, >>>> + dc_crtc->irq_dec_seqcomplete, >>>> + dc_crtc_common_irq_handler, >>>> + }, { >>>> + dc_crtc->de->dev, >>>> + dc_crtc->irq_dec_shdld, >>>> + dc_crtc_common_irq_handler, >>>> + }, { >>>> + dc_crtc->ed_cont->dev, >>>> + dc_crtc->irq_ed_cont_shdld, >>>> + dc_crtc_common_irq_handler, >>>> + }, { >>>> + dc_crtc->ed_safe->dev, >>>> + dc_crtc->irq_ed_safe_shdld, >>>> + dc_crtc_common_irq_handler, >>>> + }, >>>> + }; >>>> + struct drm_crtc *crtc = &dc_crtc->base; >>>> + int i, ret; >>>> + >>>> + dc_crtc->irqs = drmm_kcalloc(drm, ARRAY_SIZE(irqs), >>>> + sizeof(*dc_crtc->irqs), GFP_KERNEL); >>> >>> Using array would remove the necessity to call drmm_kcalloc here(). >> >> Ok, I may use a macro to define the array size instead. >> >> #define DC_CRTC_IRQS 5 > > Just embed the array into dc_crtc, no need for extra defines. The array sizes of dc_crtc->irqs[] and the local irqs[] are the same. Wouldn't it be straightforward to use macro DC_CRTC_IRQS to make sure they are the same? > >> >> >>> >>>> + if (!dc_crtc->irqs) { >>>> + dev_err(drm->dev, "failed to allocate CRTC%u irqs\n", >>>> + crtc->index); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) { >>>> + struct dc_crtc_irq *irq = &dc_crtc->irqs[i]; >>>> + >>>> + ret = dc_crtc_request_irq(dc_crtc, irqs[i].dev, irqs[i].irq, >>>> + irqs[i].irq_handler); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + irq->dc_crtc = dc_crtc; >>>> + irq->irq = irqs[i].irq; >>>> + >>>> + ret = drmm_add_action_or_reset(drm, dc_crtc_free_irq, irq); >>> >>> Can you use devm_request_irq() instead? >> >> No. >> >> The requested irqs would be freed too late as devm_of_platform_populate() >> is called early from dc_probe(). They would be freed later than the time >> point where irq domain is removed from dc_ic_unbind(). That would cause >> a kernel Oops as I tried. > > Ohh, you are using drmm here. Please don't use drmm for IRQ domains. Can you please tell the reason? Are you talking about the drmm_kzalloc() and drmm_kcalloc() function calls in dc_ic_bind()? > >> >>> >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>> >>> [...] >>> >>>> + >>>> +static int dc_kms_init_encoder_per_crtc(struct dc_drm_device *dc_drm, >>>> + int crtc_index) >>>> +{ >>>> + struct dc_crtc *dc_crtc = &dc_drm->dc_crtc[crtc_index]; >>>> + struct drm_device *drm = &dc_drm->base; >>>> + struct drm_crtc *crtc = &dc_crtc->base; >>>> + struct drm_connector *connector; >>>> + struct device *dev = drm->dev; >>>> + struct drm_encoder *encoder; >>>> + struct device_node *remote; >>>> + struct drm_bridge *bridge; >>>> + int ret = 0; >>>> + >>>> + remote = of_graph_get_remote_node(dc_crtc->de->tc->dev->of_node, 0, -1); >>>> + if (!of_device_is_available(remote)) >>>> + goto out; >>>> + >>>> + bridge = of_drm_find_bridge(remote); >>> >>> drm_of_find_panel_or_bridge() instead. >> >> Nope. >> >> The first bridge is always pixel combiner according to SoC design. >> It can't be a panel. > > So pass NULL as a panel pointer. At least it saves you from calling > of_graph_get_remote_node() manually. Kerneldoc of drm_of_find_panel_or_bridge() says it's deprecated and should not be used in new drivers. I'll use devm_drm_of_get_bridge(). > >>>> + if (!bridge) { >>>> + ret = -EPROBE_DEFER; >>>> + dev_err_probe(dev, ret, "failed to find bridge for CRTC%u\n", >>>> + crtc->index); >>>> + goto out; >>>> + } >>>> + >>>> + encoder = &dc_drm->encoder[crtc_index]; >>>> + ret = drm_simple_encoder_init(drm, encoder, DRM_MODE_ENCODER_NONE); >>>> + if (ret) { >>>> + dev_err(dev, "failed to initialize encoder for CRTC%u: %d\n", >>>> + crtc->index, ret); >>>> + goto out; >>>> + } >>>> + >>>> + encoder->possible_crtcs = drm_crtc_mask(crtc); >>>> + >>>> + ret = drm_bridge_attach(encoder, bridge, NULL, >>>> + DRM_BRIDGE_ATTACH_NO_CONNECTOR); >>>> + if (ret) { >>>> + dev_err(dev, >>>> + "failed to attach bridge to encoder for CRTC%u: %d\n", >>>> + crtc->index, ret); >>>> + goto out; >>>> + } >>>> + >>>> + connector = drm_bridge_connector_init(drm, encoder); >>>> + if (IS_ERR(connector)) { >>>> + ret = PTR_ERR(connector); >>>> + dev_err(dev, "failed to init bridge connector for CRTC%u: %d\n", >>>> + crtc->index, ret); >>>> + goto out; >>>> + } >>>> + >>>> + ret = drm_connector_attach_encoder(connector, encoder); >>>> + if (ret) >>>> + dev_err(dev, >>>> + "failed to attach encoder to connector for CRTC%u: %d\n", >>>> + crtc->index, ret); >>>> + >>>> +out: >>>> + of_node_put(remote); >>>> + return ret; >>>> +} >>>> + >>>> +int dc_kms_init(struct dc_drm_device *dc_drm) >>>> +{ >>>> + struct drm_device *drm = &dc_drm->base; >>>> + int ret, i; >>>> + >>>> + ret = drmm_mode_config_init(drm); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + atomic_set(&dc_drm->pe_rpm_count, 0); >>>> + >>>> + drm->mode_config.min_width = 60; >>>> + drm->mode_config.min_height = 60; >>>> + drm->mode_config.max_width = 8192; >>>> + drm->mode_config.max_height = 8192; >>>> + drm->mode_config.funcs = &dc_drm_mode_config_funcs; >>>> + >>>> + drm->vblank_disable_immediate = true; >>>> + drm->max_vblank_count = DC_FRAMEGEN_MAX_FRAME_INDEX; >>>> + >>>> + for (i = 0; i < DC_CRTCS; i++) { >>>> + ret = dc_crtc_init(dc_drm, i); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = dc_kms_init_encoder_per_crtc(dc_drm, i); >>>> + if (ret) >>>> + return ret; >>>> + } >>>> + >>>> + ret = drm_vblank_init(drm, DC_CRTCS); >>>> + if (ret) { >>>> + dev_err(drm->dev, "failed to init vblank support: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + drm_mode_config_reset(drm); >>>> + >>>> + drm_kms_helper_poll_init(drm); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +void dc_kms_uninit(struct dc_drm_device *dc_drm) >>>> +{ >>>> + drm_kms_helper_poll_fini(&dc_drm->base); >>>> +} >>>> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.h b/drivers/gpu/drm/imx/dc/dc-kms.h >>>> new file mode 100644 >>>> index 000000000000..4f66b11c106a >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/dc-kms.h >>>> @@ -0,0 +1,15 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0+ */ >>>> +/* >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#ifndef __DC_KMS_H__ >>>> +#define __DC_KMS_H__ >>>> + >>>> +#include "dc-de.h" >>>> + >>>> +#define DC_CRTCS DC_DISPLAYS >>>> +#define DC_ENCODERS DC_DISPLAYS >>>> +#define DC_PRIMARYS DC_DISPLAYS >>> >>> If they are all equal, why do you need separate defines? >> >> Hmm, just for meaningful macro names to make code easy to read. > > From my POV it makes code harder to read, as the reviewer has to keep in > mind that those values are all the same. Will only use DC_DISPLAYS and drop the rest. > >> >>> >>>> + >>>> +#endif /* __DC_KMS_H__ */ >>>> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.c b/drivers/gpu/drm/imx/dc/dc-plane.c >>>> new file mode 100644 >>>> index 000000000000..a49b043ca167 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/dc-plane.c >>>> @@ -0,0 +1,227 @@ >>>> +// SPDX-License-Identifier: GPL-2.0+ >>>> +/* >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#include <drm/drm_atomic.h> >>>> +#include <drm/drm_atomic_helper.h> >>>> +#include <drm/drm_atomic_state_helper.h> >>>> +#include <drm/drm_crtc.h> >>>> +#include <drm/drm_drv.h> >>>> +#include <drm/drm_fb_dma_helper.h> >>>> +#include <drm/drm_fourcc.h> >>>> +#include <drm/drm_framebuffer.h> >>>> +#include <drm/drm_gem_atomic_helper.h> >>>> +#include <drm/drm_plane_helper.h> >>>> + >>>> +#include "dc-drv.h" >>>> +#include "dc-fu.h" >>>> +#include "dc-plane.h" >>>> + >>>> +#define DC_PLANE_MAX_PITCH 0x10000 >>>> +#define DC_PLANE_MAX_PIX_CNT 8192 >>>> + >>>> +static const uint32_t dc_plane_formats[] = { >>>> + DRM_FORMAT_XRGB8888, >>>> +}; >>>> + >>>> +static const struct drm_plane_funcs dc_plane_funcs = { >>>> + .update_plane = drm_atomic_helper_update_plane, >>>> + .disable_plane = drm_atomic_helper_disable_plane, >>>> + .destroy = drm_plane_cleanup, >>>> + .reset = drm_atomic_helper_plane_reset, >>>> + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, >>>> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, >>>> +}; >>>> + >>>> +static int dc_plane_check_no_off_screen(struct drm_plane_state *state, >>>> + struct drm_crtc_state *crtc_state) >>>> +{ >>>> + if (state->dst.x1 < 0 || state->dst.y1 < 0 || >>>> + state->dst.x2 > crtc_state->adjusted_mode.hdisplay || >>>> + state->dst.y2 > crtc_state->adjusted_mode.vdisplay) { >>>> + dc_plane_dbg(state->plane, "no off screen\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int dc_plane_check_max_source_resolution(struct drm_plane_state *state) >>>> +{ >>>> + int src_h = drm_rect_height(&state->src) >> 16; >>>> + int src_w = drm_rect_width(&state->src) >> 16; >>>> + >>>> + if (src_w > DC_PLANE_MAX_PIX_CNT || src_h > DC_PLANE_MAX_PIX_CNT) { >>>> + dc_plane_dbg(state->plane, "invalid source resolution\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int dc_plane_check_fb(struct drm_plane_state *state) >>>> +{ >>>> + struct drm_framebuffer *fb = state->fb; >>>> + dma_addr_t baseaddr = drm_fb_dma_get_gem_addr(fb, state, 0); >>>> + >>>> + /* base address alignment */ >>>> + if (baseaddr & 0x3) { >>>> + dc_plane_dbg(state->plane, "fb bad baddr alignment\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* pitches[0] range */ >>>> + if (fb->pitches[0] > DC_PLANE_MAX_PITCH) { >>>> + dc_plane_dbg(state->plane, "fb pitches[0] is out of range\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* pitches[0] alignment */ >>>> + if (fb->pitches[0] & 0x3) { >>>> + dc_plane_dbg(state->plane, "fb bad pitches[0] alignment\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int >>>> +dc_plane_atomic_check(struct drm_plane *plane, struct drm_atomic_state *state) >>>> +{ >>>> + struct drm_plane_state *plane_state = >>>> + drm_atomic_get_new_plane_state(state, plane); >>>> + struct drm_crtc_state *crtc_state; >>>> + int ret; >>>> + >>>> + /* ok to disable */ >>>> + if (!plane_state->fb) >>>> + return 0; >>>> + >>>> + if (!plane_state->crtc) { >>>> + dc_plane_dbg(plane, "no CRTC in plane state\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + crtc_state = >>>> + drm_atomic_get_existing_crtc_state(state, plane_state->crtc); >>>> + if (WARN_ON(!crtc_state)) >>>> + return -EINVAL; >>>> + >>>> + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state, >>>> + DRM_PLANE_NO_SCALING, >>>> + DRM_PLANE_NO_SCALING, >>>> + true, false); >>>> + if (ret) { >>>> + dc_plane_dbg(plane, "failed to check plane state: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + ret = dc_plane_check_no_off_screen(plane_state, crtc_state); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + ret = dc_plane_check_max_source_resolution(plane_state); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + return dc_plane_check_fb(plane_state); >>>> +} >>>> + >>>> +static void >>>> +dc_plane_atomic_update(struct drm_plane *plane, struct drm_atomic_state *state) >>>> +{ >>>> + struct drm_plane_state *new_state = >>>> + drm_atomic_get_new_plane_state(state, plane); >>>> + struct dc_plane *dplane = to_dc_plane(plane); >>>> + struct drm_framebuffer *fb = new_state->fb; >>>> + const struct dc_fu_ops *fu_ops; >>>> + struct dc_lb *lb = dplane->lb; >>>> + struct dc_fu *fu = dplane->fu; >>>> + dma_addr_t baseaddr; >>>> + int src_w, src_h; >>>> + int idx; >>>> + >>>> + if (!drm_dev_enter(plane->dev, &idx)) >>>> + return; >>>> + >>>> + src_w = drm_rect_width(&new_state->src) >> 16; >>>> + src_h = drm_rect_height(&new_state->src) >> 16; >>>> + >>>> + baseaddr = drm_fb_dma_get_gem_addr(fb, new_state, 0); >>>> + >>>> + fu_ops = dc_fu_get_ops(dplane->fu); >>>> + >>>> + fu_ops->set_layerblend(fu, lb); >>>> + fu_ops->set_burstlength(fu, baseaddr); >>>> + fu_ops->set_src_stride(fu, fb->pitches[0]); >>>> + fu_ops->set_src_buf_dimensions(fu, src_w, src_h); >>>> + fu_ops->set_fmt(fu, fb->format); >>>> + fu_ops->set_framedimensions(fu, src_w, src_h); >>>> + fu_ops->set_baseaddress(fu, baseaddr); >>>> + fu_ops->enable_src_buf(fu); >>> >>> Are you expecting that these ops might change? Can you call the function >>> directly? >> >> Looking at struct dc_fl and struct dc_fw, you may find struct dc_fu >> is the base class. These function calls take struct dc_fu as >> arguments so that derived instances may specify their implementations. >> >> So, I can't call their implementation functions directly. > > Ack. Thanks. > >> >>> >>>> + >>>> + dc_plane_dbg(plane, "uses %s\n", fu_ops->get_name(fu)); >>>> + >>>> + dc_lb_pec_dynamic_prim_sel(lb, dc_cf_get_link_id(dplane->cf)); >>>> + dc_lb_pec_dynamic_sec_sel(lb, fu_ops->get_link_id(fu)); >>>> + dc_lb_mode(lb, LB_BLEND); >>>> + dc_lb_blendcontrol(lb); >>>> + dc_lb_position(lb, new_state->dst.x1, new_state->dst.y1); >>>> + dc_lb_pec_clken(lb, CLKEN_AUTOMATIC); >>>> + >>>> + dc_plane_dbg(plane, "uses LayerBlend%u\n", dc_lb_get_id(lb)); >>>> + >>>> + /* set ExtDst's source to LayerBlend */ >>>> + dc_ed_pec_src_sel(dplane->ed, dc_lb_get_link_id(lb)); >>>> + >>>> + drm_dev_exit(idx); >>>> +} >>>> + >>>> +static void dc_plane_atomic_disable(struct drm_plane *plane, >>>> + struct drm_atomic_state *state) >>>> +{ >>>> + struct dc_plane *dplane = to_dc_plane(plane); >>>> + const struct dc_fu_ops *fu_ops; >>>> + int idx; >>>> + >>>> + if (!drm_dev_enter(plane->dev, &idx)) >>>> + return; >>>> + >>>> + /* disable fetchunit in shadow */ >>>> + fu_ops = dc_fu_get_ops(dplane->fu); >>>> + fu_ops->disable_src_buf(dplane->fu); >>>> + >>>> + /* set ExtDst's source to ConstFrame */ >>>> + dc_ed_pec_src_sel(dplane->ed, dc_cf_get_link_id(dplane->cf)); >>>> + >>>> + drm_dev_exit(idx); >>>> +} >>>> + >>>> +static const struct drm_plane_helper_funcs dc_plane_helper_funcs = { >>>> + .atomic_check = dc_plane_atomic_check, >>>> + .atomic_update = dc_plane_atomic_update, >>>> + .atomic_disable = dc_plane_atomic_disable, >>>> +}; >>>> + >>>> +int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane) >>>> +{ >>>> + struct drm_plane *plane = &dc_plane->base; >>>> + int ret; >>>> + >>>> + ret = drm_universal_plane_init(&dc_drm->base, plane, 0, &dc_plane_funcs, >>>> + dc_plane_formats, >>>> + ARRAY_SIZE(dc_plane_formats), >>>> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + drm_plane_helper_add(plane, &dc_plane_helper_funcs); >>>> + >>>> + dc_plane->fu = dc_drm->pe->fu_disp[plane->index]; >>>> + dc_plane->cf = dc_drm->pe->cf_cont[plane->index]; >>>> + dc_plane->lb = dc_drm->pe->lb[plane->index]; >>>> + dc_plane->ed = dc_drm->pe->ed_cont[plane->index]; >>>> + >>>> + return 0; >>>> +} >>>> diff --git a/drivers/gpu/drm/imx/dc/dc-plane.h b/drivers/gpu/drm/imx/dc/dc-plane.h >>>> new file mode 100644 >>>> index 000000000000..e72c3a7cb66f >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/imx/dc/dc-plane.h >>>> @@ -0,0 +1,37 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0+ */ >>>> +/* >>>> + * Copyright 2024 NXP >>>> + */ >>>> + >>>> +#ifndef __DC_PLANE_H__ >>>> +#define __DC_PLANE_H__ >>>> + >>>> +#include <linux/container_of.h> >>>> + >>>> +#include <drm/drm_plane.h> >>>> +#include <drm/drm_print.h> >>>> + >>>> +#include "dc-fu.h" >>>> +#include "dc-pe.h" >>>> + >>>> +#define dc_plane_dbg(plane, fmt, ...) \ >>>> +do { \ >>>> + typeof(plane) _plane = (plane); \ >>>> + drm_dbg_kms(_plane->dev, "[PLANE:%d:%s] " fmt, \ >>>> + _plane->base.id, _plane->name, ##__VA_ARGS__); \ >>>> +} while (0) >>>> + >>>> +struct dc_plane { >>>> + struct drm_plane base; >>>> + struct dc_fu *fu; >>>> + struct dc_cf *cf; >>>> + struct dc_lb *lb; >>>> + struct dc_ed *ed; >>>> +}; >>>> + >>>> +static inline struct dc_plane *to_dc_plane(struct drm_plane *plane) >>>> +{ >>>> + return container_of(plane, struct dc_plane, base); >>>> +} >>>> + >>>> +#endif /* __DC_PLANE_H__ */ >>>> -- >>>> 2.34.1 >>>> >>> >> >> -- >> Regards, >> Liu Ying >> >
Hi, This patch series aims to add Freescale i.MX8qxp Display Controller support. The controller is comprised of three main components that include a blit engine for 2D graphics accelerations, display controller for display output processing, as well as a command sequencer. Previous patch series attempts to do that can be found at: https://patchwork.freedesktop.org/series/84524/ This series addresses Maxime's comments on the previous one: a. Split the display controller into multiple internal devices. 1) List display engine, pixel engine, interrupt controller and more as the controller's child devices. 2) List display engine and pixel engine's processing units as their child devices. b. Add minimal feature support. Only support two display pipelines with primary planes with XR24 fb, backed by two fetchunits. No fetchunit dynamic allocation logic(to be done when necessary). c. Use drm_dev_{enter, exit}(). Since this series changes a lot comparing to the previous one, I choose to send it with a new patch series, not a new version. To follow up i.MX8qxp TRM, I changed the controller name to "Display Controller" instead of the previous "DPU". "DPU" is only mentioned in the SoC block diagram and represents the whole display subsystem which includes the display controller and prefech engines, etc. With an additional patch[1] for simple-pm-bus.c, this series facilitates testing a LVDS panel on i.MX8qxp MEK. Please do NOT merge patch 11-16. [1] https://lkml.org/lkml/2023/1/25/120 v2: * Drop fsl,dc-*-id DT properties from fsl,imx8qxp-dc*.yaml. (Krzysztof) * Move port property from fsl,imx8qxp-dc-display-engine.yaml to fsl,imx8qxp-dc-tcon.yaml. (Krzysztof) * Drop unneeded "|" from fsl,imx8qxp-dc-intc.yaml. (Krzysztof) * Use generic pmu pattern property in fsl,imx8qxp-dc.yaml. (Krzysztof) * Fix register range size in fsl,imx8qxp-dc*.yaml. * Use OF alias id to get instance id from display driver. * Find next bridge from TCon's port from display driver. * Drop drm/drm_module.h include from dc-drv.c. * Improve file list in MAINTAINERS. (Frank) * Add entire i.MX8qxp display controller device tree for review. (Krzysztof) * Add MIPI/LVDS subsystems device tree and a DT overlay for imx8qxp MEK to test a LVDS panel as an example. (Francesco) Liu Ying (16): dt-bindings: display: imx: Add some i.MX8qxp Display Controller processing units dt-bindings: display: imx: Add i.MX8qxp Display Controller display engine dt-bindings: display: imx: Add i.MX8qxp Display Controller pixel engine dt-bindings: interrupt-controller: Add i.MX8qxp Display Controller interrupt controller dt-bindings: display: imx: Add i.MX8qxp Display Controller drm/imx: Add i.MX8qxp Display Controller display engine drm/imx: Add i.MX8qxp Display Controller pixel engine drm/imx: Add i.MX8qxp Display Controller interrupt controller drm/imx: Add i.MX8qxp Display Controller KMS MAINTAINERS: Add maintainer for i.MX8qxp Display Controller dt-bindings: phy: mixel,mipi-dsi-phy: Allow assigned-clock* properties dt-bindings: firmware: imx: Add SCU controlled display pixel link nodes arm64: dts: imx8qxp: Add display controller subsystem arm64: dts: imx8qxp: Add MIPI-LVDS combo subsystems arm64: dts: imx8qxp-mek: Enable display controller arm64: dts: imx8qxp-mek: Add MX8-DLVDS-LCD1 display module support .../imx/fsl,imx8qxp-dc-constframe.yaml | 44 ++ .../imx/fsl,imx8qxp-dc-display-engine.yaml | 152 +++++ .../display/imx/fsl,imx8qxp-dc-extdst.yaml | 72 +++ .../imx/fsl,imx8qxp-dc-fetchlayer.yaml | 30 + .../imx/fsl,imx8qxp-dc-fetchunit-common.yaml | 125 ++++ .../display/imx/fsl,imx8qxp-dc-fetchwarp.yaml | 30 + .../display/imx/fsl,imx8qxp-dc-framegen.yaml | 64 ++ .../imx/fsl,imx8qxp-dc-layerblend.yaml | 39 ++ .../imx/fsl,imx8qxp-dc-pixel-engine.yaml | 250 ++++++++ .../display/imx/fsl,imx8qxp-dc-tcon.yaml | 45 ++ .../bindings/display/imx/fsl,imx8qxp-dc.yaml | 236 +++++++ .../devicetree/bindings/firmware/fsl,scu.yaml | 20 + .../fsl,imx8qxp-dc-intc.yaml | 318 ++++++++++ .../bindings/phy/mixel,mipi-dsi-phy.yaml | 5 - MAINTAINERS | 8 + arch/arm64/boot/dts/freescale/Makefile | 4 + .../arm64/boot/dts/freescale/imx8-ss-dc0.dtsi | 408 +++++++++++++ .../imx8qxp-mek-mx8-dlvds-lcd1-lvds0-odd.dtso | 183 ++++++ arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 34 ++ .../boot/dts/freescale/imx8qxp-ss-dc.dtsi | 240 ++++++++ .../dts/freescale/imx8qxp-ss-mipi-lvds.dtsi | 437 +++++++++++++ arch/arm64/boot/dts/freescale/imx8qxp.dtsi | 28 +- drivers/gpu/drm/imx/Kconfig | 1 + drivers/gpu/drm/imx/Makefile | 1 + drivers/gpu/drm/imx/dc/Kconfig | 8 + drivers/gpu/drm/imx/dc/Makefile | 7 + drivers/gpu/drm/imx/dc/dc-cf.c | 157 +++++ drivers/gpu/drm/imx/dc/dc-crtc.c | 578 ++++++++++++++++++ drivers/gpu/drm/imx/dc/dc-crtc.h | 67 ++ drivers/gpu/drm/imx/dc/dc-de.c | 151 +++++ drivers/gpu/drm/imx/dc/dc-de.h | 65 ++ drivers/gpu/drm/imx/dc/dc-drv.c | 275 +++++++++ drivers/gpu/drm/imx/dc/dc-drv.h | 54 ++ drivers/gpu/drm/imx/dc/dc-ed.c | 266 ++++++++ drivers/gpu/drm/imx/dc/dc-fg.c | 366 +++++++++++ drivers/gpu/drm/imx/dc/dc-fl.c | 136 +++++ drivers/gpu/drm/imx/dc/dc-fu.c | 241 ++++++++ drivers/gpu/drm/imx/dc/dc-fu.h | 129 ++++ drivers/gpu/drm/imx/dc/dc-fw.c | 149 +++++ drivers/gpu/drm/imx/dc/dc-ic.c | 249 ++++++++ drivers/gpu/drm/imx/dc/dc-kms.c | 143 +++++ drivers/gpu/drm/imx/dc/dc-kms.h | 15 + drivers/gpu/drm/imx/dc/dc-lb.c | 300 +++++++++ drivers/gpu/drm/imx/dc/dc-pe.c | 140 +++++ drivers/gpu/drm/imx/dc/dc-pe.h | 91 +++ drivers/gpu/drm/imx/dc/dc-plane.c | 227 +++++++ drivers/gpu/drm/imx/dc/dc-plane.h | 37 ++ drivers/gpu/drm/imx/dc/dc-tc.c | 137 +++++ 48 files changed, 6756 insertions(+), 6 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-constframe.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-display-engine.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-extdst.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-fetchlayer.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-fetchunit-common.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-fetchwarp.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-framegen.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-layerblend.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-pixel-engine.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc-tcon.yaml create mode 100644 Documentation/devicetree/bindings/display/imx/fsl,imx8qxp-dc.yaml create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,imx8qxp-dc-intc.yaml create mode 100644 arch/arm64/boot/dts/freescale/imx8-ss-dc0.dtsi create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-mek-mx8-dlvds-lcd1-lvds0-odd.dtso create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ss-dc.dtsi create mode 100644 arch/arm64/boot/dts/freescale/imx8qxp-ss-mipi-lvds.dtsi create mode 100644 drivers/gpu/drm/imx/dc/Kconfig create mode 100644 drivers/gpu/drm/imx/dc/Makefile create mode 100644 drivers/gpu/drm/imx/dc/dc-cf.c create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.c create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.h create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h create mode 100644 drivers/gpu/drm/imx/dc/dc-ed.c create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c create mode 100644 drivers/gpu/drm/imx/dc/dc-fl.c create mode 100644 drivers/gpu/drm/imx/dc/dc-fu.c create mode 100644 drivers/gpu/drm/imx/dc/dc-fu.h create mode 100644 drivers/gpu/drm/imx/dc/dc-fw.c create mode 100644 drivers/gpu/drm/imx/dc/dc-ic.c create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.c create mode 100644 drivers/gpu/drm/imx/dc/dc-kms.h create mode 100644 drivers/gpu/drm/imx/dc/dc-lb.c create mode 100644 drivers/gpu/drm/imx/dc/dc-pe.c create mode 100644 drivers/gpu/drm/imx/dc/dc-pe.h create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.c create mode 100644 drivers/gpu/drm/imx/dc/dc-plane.h create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c