Message-ID: <CAA8EJpoUa9y9nfrrDf47aLs5zr4XKSghySXNLnCGwDyYdOvMog@mail.gmail.com>
Date: Mon, 16 Dec 2024 11:44:13 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Liu Ying <victor.liu@....com>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org, 
	p.zabel@...gutronix.de, maarten.lankhorst@...ux.intel.com, mripard@...nel.org, 
	tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, shawnguo@...nel.org, 
	s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com, 
	tglx@...utronix.de, vkoul@...nel.org, kishon@...nel.org, aisheng.dong@....com, 
	agx@...xcpu.org, francesco@...cini.it, frank.li@....com, 
	u.kleine-koenig@...libre.com
Subject: Re: [PATCH v6 12/19] drm/imx: Add i.MX8qxp Display Controller KMS

On Mon, 16 Dec 2024 at 08:28, Liu Ying <victor.liu@....com> wrote:
>
> On 12/13/2024, Dmitry Baryshkov wrote:
> > On Fri, 13 Dec 2024 at 08:06, Liu Ying <victor.liu@....com> wrote:
> >>
> >> On 12/12/2024, Dmitry Baryshkov wrote:
> >>> On Wed, Dec 11, 2024 at 03:43:20PM +0800, Liu Ying wrote:
> >>>> On 12/10/2024, Dmitry Baryshkov wrote:
> >>>>> On Mon, Dec 09, 2024 at 11:39:16AM +0800, 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.
> >>>>>>
> >>>>>> Signed-off-by: Liu Ying <victor.liu@....com>
> >>>>>> ---
> >>>>>> v6:
> >>>>>> * No change.
> >>>>>>
> >>>>>> v5:
> >>>>>> * Replace .remove_new with .remove in dc-drv.c. (Uwe)
> >>>>>>
> >>>>>> v4:
> >>>>>> * Move dc_fg_displaymode(), dc_fg_panic_displaymode() and dc_lb_blendcontrol()
> >>>>>>   function calls from KMS routine to initialization stage. (Dmitry)
> >>>>>> * Drop dc-crtc.h and dc-plane.h header files and move relevant defines to
> >>>>>>   appropriate .h header files or .c source files. (Dmitry)
> >>>>>> * Drop futile "else" clause from dc_crtc_common_irq_handler(). (Dmitry)
> >>>>>> * Drop dc_drm->pe_rpm_count. (Dmitry)
> >>>>>> * Drop DC_{CRTCS,ENCODERS,PRIMARYS} macros and only use DC_DISPLAYS. (Dmitry)
> >>>>>> * Drop drmm_kcalloc() function call to allocate an array for storing IRQs.
> >>>>>>   Instead, put it in struct dc_crtc.  (Dmitry)
> >>>>>> * Call devm_request_irq() to request IRQs, instead of using drmm action.
> >>>>>>   (Dmitry)
> >>>>>> * Call devm_drm_of_get_bridge() to find the next bridge. (Dmitry)
> >>>>>> * Select DRM_CLIENT_SELECTION due to rebase.
> >>>>>> * Select the missing DRM_DISPLAY_HELPER and DRM_BRIDGE_CONNECTOR.
> >>>>>> * Use DRM_FBDEV_DMA_DRIVER_OPS due to rebase.
> >>>>>> * Replace drm_fbdev_dma_setup() with drm_client_setup_with_fourcc() due to
> >>>>>>   rebase.
> >>>>>> * Replace drmm_add_action_or_reset() with devm_add_action_or_reset() to
> >>>>>>   register dc_drm_component_unbind_all() action.
> >>>>>> * Request interrupts in dc_crtc_post_init() after encoder initialization to
> >>>>>>   make sure next bridge is found first.
> >>>>>>
> >>>>>> v3:
> >>>>>> * No change.
> >>>>>>
> >>>>>> v2:
> >>>>>> * Find next bridge from TCon's port.
> >>>>>> * Drop drm/drm_module.h include from dc-drv.c.
> >>>>>>
> >>>>>>  drivers/gpu/drm/imx/dc/Kconfig    |   5 +
> >>>>>>  drivers/gpu/drm/imx/dc/Makefile   |   5 +-
> >>>>>>  drivers/gpu/drm/imx/dc/dc-crtc.c  | 558 ++++++++++++++++++++++++++++++
> >>>>>>  drivers/gpu/drm/imx/dc/dc-de.h    |   3 +
> >>>>>>  drivers/gpu/drm/imx/dc/dc-drv.c   | 244 +++++++++++++
> >>>>>>  drivers/gpu/drm/imx/dc/dc-drv.h   |  19 +
> >>>>>>  drivers/gpu/drm/imx/dc/dc-kms.c   | 143 ++++++++
> >>>>>>  drivers/gpu/drm/imx/dc/dc-kms.h   |  58 ++++
> >>>>>>  drivers/gpu/drm/imx/dc/dc-plane.c | 241 +++++++++++++
> >>>>>>  9 files changed, 1274 insertions(+), 2 deletions(-)
> >>>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-crtc.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-plane.c
> >>>>>>

> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>> +static const struct component_master_ops dc_drm_ops = {
> >>>>>> +  .bind = dc_drm_bind,
> >>>>>> +  .unbind = dc_drm_unbind,
> >>>>>> +};
> >>>>>> +
> >>>>>> +static int dc_probe(struct platform_device *pdev)
> >>>>>> +{
> >>>>>> +  struct component_match *match = NULL;
> >>>>>> +  struct dc_priv *priv;
> >>>>>> +  int ret;
> >>>>>> +
> >>>>>> +  priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> >>>>>> +  if (!priv)
> >>>>>> +          return -ENOMEM;
> >>>>>> +
> >>>>>> +  priv->clk_cfg = devm_clk_get(&pdev->dev, NULL);
> >>>>>> +  if (IS_ERR(priv->clk_cfg))
> >>>>>> +          return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk_cfg),
> >>>>>> +                               "failed to get cfg clock\n");
> >>>>>> +
> >>>>>> +  dev_set_drvdata(&pdev->dev, priv);
> >>>>>> +
> >>>>>> +  ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> >>>>>> +  if (ret)
> >>>>>> +          return ret;
> >>>>>> +
> >>>>>> +  ret = devm_pm_runtime_enable(&pdev->dev);
> >>>>>> +  if (ret)
> >>>>>> +          return ret;
> >>>>>> +
> >>>>>> +  ret = devm_of_platform_populate(&pdev->dev);
> >>>>>> +  if (ret)
> >>>>>> +          return ret;
> >>>>>> +
> >>>>>> +  dc_add_components(&pdev->dev, &match);
> >>>>>> +
> >>>>>> +  ret = component_master_add_with_match(&pdev->dev, &dc_drm_ops, match);
> >>>>>> +  if (ret)
> >>>>>> +          return dev_err_probe(&pdev->dev, ret,
> >>>>>> +                               "failed to add component master\n");
> >>>>>> +
> >>>>>> +  return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void dc_remove(struct platform_device *pdev)
> >>>>>> +{
> >>>>>> +  component_master_del(&pdev->dev, &dc_drm_ops);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int dc_runtime_suspend(struct device *dev)
> >>>>>> +{
> >>>>>> +  struct dc_priv *priv = dev_get_drvdata(dev);
> >>>>>> +
> >>>>>> +  clk_disable_unprepare(priv->clk_cfg);
> >>>>>> +
> >>>>>> +  return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int dc_runtime_resume(struct device *dev)
> >>>>>> +{
> >>>>>> +  struct dc_priv *priv = dev_get_drvdata(dev);
> >>>>>> +  int ret;
> >>>>>> +
> >>>>>> +  ret = clk_prepare_enable(priv->clk_cfg);
> >>>>>> +  if (ret)
> >>>>>> +          dev_err(dev, "failed to enable cfg clock: %d\n", ret);
> >>>>>> +
> >>>>>> +  return ret;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int dc_suspend(struct device *dev)
> >>>>>> +{
> >>>>>> +  struct dc_priv *priv = dev_get_drvdata(dev);
> >>>>>> +
> >>>>>> +  return drm_mode_config_helper_suspend(priv->drm);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int dc_resume(struct device *dev)
> >>>>>> +{
> >>>>>> +  struct dc_priv *priv = dev_get_drvdata(dev);
> >>>>>> +
> >>>>>> +  return drm_mode_config_helper_resume(priv->drm);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void dc_shutdown(struct platform_device *pdev)
> >>>>>> +{
> >>>>>> +  struct dc_priv *priv = dev_get_drvdata(&pdev->dev);
> >>>>>> +
> >>>>>> +  drm_atomic_helper_shutdown(priv->drm);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static const struct dev_pm_ops dc_pm_ops = {
> >>>>>> +  RUNTIME_PM_OPS(dc_runtime_suspend, dc_runtime_resume, NULL)
> >>>>>> +  SYSTEM_SLEEP_PM_OPS(dc_suspend, dc_resume)
> >>>>>> +};
> >>>>>> +
> >>>>>> +static const struct of_device_id dc_dt_ids[] = {
> >>>>>> +  { .compatible = "fsl,imx8qxp-dc", },
> >>>>>> +  { /* sentinel */ }
> >>>>>> +};
> >>>>>> +MODULE_DEVICE_TABLE(of, dc_dt_ids);
> >>>>>> +
> >>>>>> +static struct platform_driver dc_driver = {
> >>>>>> +  .probe = dc_probe,
> >>>>>> +  .remove = dc_remove,
> >>>>>> +  .shutdown = dc_shutdown,
> >>>>>> +  .driver = {
> >>>>>> +          .name = "imx8-dc",
> >>>>>> +          .of_match_table = dc_dt_ids,
> >>>>>> +          .pm = pm_sleep_ptr(&dc_pm_ops),
> >>>>>> +  },
> >>>>>> +};
> >>>>>> +
> >>>>>>  static struct platform_driver * const dc_drivers[] = {
> >>>>>>    &dc_cf_driver,
> >>>>>>    &dc_de_driver,
> >>>>>> @@ -19,6 +262,7 @@ static struct platform_driver * const dc_drivers[] = {
> >>>>>>    &dc_lb_driver,
> >>>>>>    &dc_pe_driver,
> >>>>>>    &dc_tc_driver,
> >>>>>> +  &dc_driver,
> >>>>>>  };
> >>>>>>
> >>>>>>  static int __init dc_drm_init(void)
> >>>>>> diff --git a/drivers/gpu/drm/imx/dc/dc-drv.h b/drivers/gpu/drm/imx/dc/dc-drv.h
> >>>>>> index 3b11f4862c6c..39a771a13933 100644
> >>>>>> --- a/drivers/gpu/drm/imx/dc/dc-drv.h
> >>>>>> +++ b/drivers/gpu/drm/imx/dc/dc-drv.h
> >>>>>> @@ -6,19 +6,38 @@
> >>>>>>  #ifndef __DC_DRV_H__
> >>>>>>  #define __DC_DRV_H__
> >>>>>>
> >>>>>> +#include <linux/container_of.h>
> >>>>>>  #include <linux/platform_device.h>
> >>>>>>
> >>>>>>  #include <drm/drm_device.h>
> >>>>>> +#include <drm/drm_encoder.h>
> >>>>>>
> >>>>>>  #include "dc-de.h"
> >>>>>> +#include "dc-kms.h"
> >>>>>>  #include "dc-pe.h"
> >>>>>>
> >>>>>>  struct dc_drm_device {
> >>>>>>    struct drm_device base;
> >>>>>> +  struct dc_crtc dc_crtc[DC_DISPLAYS];
> >>>>>> +  struct dc_plane dc_primary[DC_DISPLAYS];
> >>>>>> +  struct drm_encoder encoder[DC_DISPLAYS];
> >>>>>>    struct dc_de *de[DC_DISPLAYS];
> >>>>>>    struct dc_pe *pe;
> >>>>>>  };
> >>>>>>
> >>>>>> +static inline struct dc_drm_device *to_dc_drm_device(struct drm_device *drm)
> >>>>>> +{
> >>>>>> +  return container_of(drm, struct dc_drm_device, base);
> >>>>>> +}
> >>>>>> +
> >>>>>> +int dc_crtc_init(struct dc_drm_device *dc_drm, int crtc_index);
> >>>>>> +int dc_crtc_post_init(struct dc_drm_device *dc_drm, int crtc_index);
> >>>>>> +
> >>>>>> +int dc_kms_init(struct dc_drm_device *dc_drm);
> >>>>>> +void dc_kms_uninit(struct dc_drm_device *dc_drm);
> >>>>>> +
> >>>>>> +int dc_plane_init(struct dc_drm_device *dc_drm, struct dc_plane *dc_plane);
> >>>>>> +
> >>>>>>  extern struct platform_driver dc_cf_driver;
> >>>>>>  extern struct platform_driver dc_ed_driver;
> >>>>>>  extern struct platform_driver dc_de_driver;
> >>>>>> diff --git a/drivers/gpu/drm/imx/dc/dc-kms.c b/drivers/gpu/drm/imx/dc/dc-kms.c
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..2b18aa37a4a8
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/gpu/drm/imx/dc/dc-kms.c
> >>>>>> @@ -0,0 +1,143 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>>> +/*
> >>>>>> + * Copyright 2024 NXP
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include <linux/of.h>
> >>>>>> +#include <linux/of_graph.h>
> >>>>>> +
> >>>>>> +#include <drm/drm_atomic_helper.h>
> >>>>>> +#include <drm/drm_bridge.h>
> >>>>>> +#include <drm/drm_bridge_connector.h>
> >>>>>> +#include <drm/drm_connector.h>
> >>>>>> +#include <drm/drm_crtc.h>
> >>>>>> +#include <drm/drm_device.h>
> >>>>>> +#include <drm/drm_encoder.h>
> >>>>>> +#include <drm/drm_gem_framebuffer_helper.h>
> >>>>>> +#include <drm/drm_mode_config.h>
> >>>>>> +#include <drm/drm_print.h>
> >>>>>> +#include <drm/drm_probe_helper.h>
> >>>>>> +#include <drm/drm_simple_kms_helper.h>
> >>>>>> +#include <drm/drm_vblank.h>
> >>>>>> +
> >>>>>> +#include "dc-de.h"
> >>>>>> +#include "dc-drv.h"
> >>>>>> +#include "dc-kms.h"
> >>>>>> +
> >>>>>> +static const struct drm_mode_config_funcs dc_drm_mode_config_funcs = {
> >>>>>> +  .fb_create = drm_gem_fb_create,
> >>>>>> +  .atomic_check = drm_atomic_helper_check,
> >>>>>> +  .atomic_commit = drm_atomic_helper_commit,
> >>>>>> +};
> >>>>>> +
> >>>>>> +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 drm_bridge *bridge;
> >>>>>> +  int ret;
> >>>>>> +
> >>>>>> +  bridge = devm_drm_of_get_bridge(dev, dc_crtc->de->tc->dev->of_node,
> >>>>>> +                                  0, 0);
> >>>>>> +  if (IS_ERR(bridge)) {
> >>>>>> +          ret = PTR_ERR(bridge);
> >>>>>> +          if (ret == -ENODEV)
> >>>>>> +                  return 0;
> >>>>>> +
> >>>>>> +          return dev_err_probe(dev, ret,
> >>>>>> +                               "failed to find bridge for CRTC%u\n",
> >>>>>> +                               crtc->index);
> >>>>>> +  }
> >>>>>> +
> >>>>>> +  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);
> >>>>>> +          return ret;
> >>>>>> +  }
> >>>>>> +
> >>>>>> +  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);
> >>>>>> +          return ret;
> >>>>>> +  }
> >>>>>> +
> >>>>>> +  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);
> >>>>>> +          return ret;
> >>>>>> +  }
> >>>>>> +
> >>>>>> +  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);
> >>>>>> +
> >>>>>> +  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;
> >>>>>> +
> >>>>>> +  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_DISPLAYS; 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;
> >>>>>> +  }
> >>>>>> +
> >>>>>> +  for (i = 0; i < DC_DISPLAYS; i++) {
> >>>>>> +          ret = dc_crtc_post_init(dc_drm, i);
> >>>>>
> >>>>> Can you use .late_register for this?
> >>>>
> >>>> Kerneldoc of struct drm_crtc_funcs::late_register says it's used to register
> >>>> additional userspace interfaces like debugfs interfaces. And, it seems that
> >>>> everyone implementing this uses it to add debugfs interfaces. So, it will
> >>>> kind of abuse it to do CRTC post initialization.
> >>>
> >>> Why can't they be requested earlier then?
> >>
> >> If I request them earlier in dc_crtc_init(), then they cannot be freed by
> >> devm_irq_release() when devm_drm_of_get_bridge() called by
> >> dc_kms_init_encoder_per_crtc() returns -EPROBE_DEFER(which means failing
> >> to find the first DRM bridge for the CRTC).  Why can't they be freed by
> >> devm_irq_release()?  Because they are requested by the devices of ExtDsts
> >> and Display Engines and their drivers are not removed during the probe
> >> deferral dance.  Furthermore, -EPROBE_DEFER won't be returned after
> >> dc_crtc_post_init() since the later called drm_vblank_init() doesn't
> >> return -EPROBE_DEFER anyway, so it's fine to call dc_crtc_post_init() here.
> >>
> >> I met the irq free issue on my i.MX8qxp MEK board before, i.e., -EBUSY is
> >> returned when requesting them again, so it's tested.
> >
> > A typical solution is to request all resources before binding the
> > device as a component. Don't tell me that your interrupt controller is
> > another component of the DRM device :-)
>
> The IRQ handlers are _highly_ related to the CRTC driver(especially the
> dc_crtc_dec_framecomplete_irq_handler() where vblank is handled), so maybe
> it's more appropriate to request the IRQs and implement the IRQ handlers in
> dc-crtc.c instead of doing them in dc-{de,ed}.c. No?

And CRTCs don't exist before master_bind(). Ack.

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists