| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150714104913.GI12465@ulmo.nvidia.com>
Date: Tue, 14 Jul 2015 12:49:14 +0200
From: Thierry Reding <thierry.reding@...il.com>
To: Jianwei Wang <jianwei.wang@...escale.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
airlied@...ux.ie, daniel.vetter@...ll.ch, mark.yao@...k-chips.com,
scottwood@...escale.com, Alison Wang <b18965@...escale.com>,
Xiubo Li <lixiubo@...s.chinamobile.com>,
Jianwei Wang <b52261@...escale.com>
Subject: Re: [PATCH v9 1/4] drm/layerscape: Add Freescale DCU DRM driver
On Mon, Jul 13, 2015 at 06:51:29PM +0800, Jianwei Wang wrote:
> This patch add support for Two Dimensional Animation and Compositing
> Engine (2D-ACE) on the Freescale SoCs.
>
> 2D-ACE is a Freescale display controller. 2D-ACE describes
> the functionality of the module extremely well its name is a value
> that cannot be used as a token in programming languages.
> Instead the valid token "DCU" is used to tag the register names and
> function names.
>
> The Display Controller Unit (DCU) module is a system master that
> fetches graphics stored in internal or external memory and displays
> them on a TFT LCD panel. A wide range of panel sizes is supported
> and the timing of the interface signals is highly configurable.
> Graphics are read directly from memory and then blended in real-time,
> which allows for dynamic content creation with minimal CPU
> intervention.
Is this for some non-i.MX SoC?
> The features:
> (1) Full RGB888 output to TFT LCD panel.
> (2) For the current LCD panel, WQVGA "480x272" is supported.
Would be more useful to describe the actual capabilities of the display
controller rather than what's supported by the panel that you happened
to have attached to it.
> (3) Blending of each pixel using up to 4 source layers
> dependent on size of panel.
> (4) Each graphic layer can be placed with one pixel resolution
> in either axis.
> (5) Each graphic layer support RGB565 and RGB888 direct colors
> without alpha
> channel and BGRA8888 BGRA4444 ARGB1555 direct colors with an
> alpha channel and YUV422 format.
> (6) Each graphic layer support alpha blending with 8-bit
> resolution.
>
> This is a simplified version, only one primary plane, one
> framebuffer, one crtc, one connector and one encoder for TFT
> LCD panel.
>
> Signed-off-by: Alison Wang <b18965@...escale.com>
> Signed-off-by: Xiubo Li <lixiubo@...s.chinamobile.com>
> Signed-off-by: Jianwei Wang <b52261@...escale.com>
> Acked-by: Daniel Vetter <daniel.vetter@...ll.ch>
> Reviewed-by: Alison Wang <alison.wang@...escale.com>
[...]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6761318..fffb8c9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3404,6 +3404,15 @@ S: Maintained
> F: drivers/gpu/drm/imx/
> F: Documentation/devicetree/bindings/drm/imx/
>
> +DRM DRIVERS FOR FREESCALE DCU
> +M: Jianwei Wang <jianwei.wang@...escale.com>
> +M: Alison Wang <alison.wang@...escale.com>
> +L: dri-devel@...ts.freedesktop.org
> +S: Supported
> +F: drivers/gpu/drm/fsl-dcu/
> +F: Documentation/devicetree/bindings/drm/fsl-dcu/
> +F: Documentation/devicetree/bindings/panel/nec,nl4827hc19_05b.txt
This binding has got nothing to do with the display driver, so it
shouldn't be listed here.
Also, please use tabs consistently (the above two lines use spaces).
> DRM DRIVERS FOR NVIDIA TEGRA
> M: Thierry Reding <thierry.reding@...il.com>
> M: Terje Bergström <tbergstrom@...dia.com>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index c46ca31..9cfd14e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -231,6 +231,8 @@ source "drivers/gpu/drm/virtio/Kconfig"
>
> source "drivers/gpu/drm/msm/Kconfig"
>
> +source "drivers/gpu/drm/fsl-dcu/Kconfig"
> +
As mentioned in a different email, I'm somewhat annoyed by the random
placement of these source statements. But we can probably clean that up
in one go and insist on proper ordering when there is one.
> diff --git a/drivers/gpu/drm/fsl-dcu/Makefile b/drivers/gpu/drm/fsl-dcu/Makefile
> new file mode 100644
> index 0000000..336b4a6
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/Makefile
> @@ -0,0 +1,7 @@
> +fsl-dcu-drm-y := fsl_dcu_drm_drv.o \
> + fsl_dcu_drm_kms.o \
> + fsl_dcu_drm_connector.o \
> + fsl_dcu_drm_plane.o \
> + fsl_dcu_drm_crtc.o \
> + fsl_dcu_drm_fbdev.o
I don't get what kind of indentation this is supposed to be. Either
align everything with the first object file, or use a single tab rather
than a tab followed by a couple of spaces.
> +obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu-drm.o
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c
> new file mode 100644
> index 0000000..41dd1d0
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_connector.c
> @@ -0,0 +1,187 @@
> +/*
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + *
> + * Freescale DCU drm device driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/backlight.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_panel.h>
> +
> +#include "fsl_dcu_drm_drv.h"
> +#include "fsl_dcu_drm_connector.h"
> +
> +static void fsl_dcu_drm_encoder_disable(struct drm_encoder *encoder)
> +{
> +}
> +
> +static int
> +fsl_dcu_drm_encoder_atomic_check(struct drm_encoder *encoder,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + return 0;
> +}
> +
> +static void fsl_dcu_drm_encoder_destroy(struct drm_encoder *encoder)
> +{
> + drm_encoder_cleanup(encoder);
> +}
> +
> +static void fsl_dcu_drm_encoder_enable(struct drm_encoder *encoder)
> +{
> +}
> +
> +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> + .enable = fsl_dcu_drm_encoder_enable,
> + .disable = fsl_dcu_drm_encoder_disable,
> + .atomic_check = fsl_dcu_drm_encoder_atomic_check,
> +};
> +
> +static const struct drm_encoder_funcs encoder_funcs = {
> + .destroy = fsl_dcu_drm_encoder_destroy,
> +};
For ease of maintenance and review, you might want to reorder your
function definitions to match the structure layout and put them closer
together.
The same applies to other places in this file.
> +int fsl_dcu_drm_encoder_create(struct fsl_dcu_drm_device *fsl_dev,
> + struct drm_crtc *crtc)
> +{
> + struct drm_encoder *encoder = &fsl_dev->encoder;
> + int ret;
> +
> + encoder->possible_crtcs = 1;
> + ret = drm_encoder_init(fsl_dev->ddev, encoder, &encoder_funcs,
> + DRM_MODE_ENCODER_LVDS);
> + if (ret < 0)
> + return ret;
> +
> + drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> + encoder->crtc = crtc;
I think you're supposed to let the DRM core make this association.
> +
> + return 0;
> +}
> +
> +#define to_fsl_dcu_connector(connector) \
> + container_of(connector, struct fsl_dcu_drm_connector, connector)
> +
It's common to put this near the definition of the structure that the
upcast is for. It's also preferred (though not by everyone it seems) to
make this a static inline function rather than a #define.
> +static int fsl_dcu_drm_connector_get_modes(struct drm_connector *connector)
> +{
> + struct fsl_dcu_drm_connector *fsl_connector;
> + int num_modes = 0;
> +
> + fsl_connector = to_fsl_dcu_connector(connector);
> + if (fsl_connector->panel && fsl_connector->panel->funcs &&
> + fsl_connector->panel->funcs->get_modes)
> + num_modes = fsl_connector->panel->funcs->get_modes
> + (fsl_connector->panel);
You could perhaps use some temporary variables to make this more
readable.
> +int fsl_dcu_drm_connector_create(struct fsl_dcu_drm_device *fsl_dev,
> + struct drm_encoder *encoder)
> +{
> + struct drm_connector *connector = &fsl_dev->connector.connector;
> + struct device_node *panel_node;
> + int ret;
> +
> + fsl_dev->connector.encoder = encoder;
Why do you need this? The DRM core should set this for you when setting
the initial configuration.
> +
> + connector->display_info.width_mm = 0;
> + connector->display_info.height_mm = 0;
Why do you need to zero these out?
> + ret = drm_connector_init(fsl_dev->ddev, connector,
> + &fsl_dcu_drm_connector_funcs,
> + DRM_MODE_CONNECTOR_LVDS);
> + if (ret < 0)
> + return ret;
> +
> + connector->dpms = DRM_MODE_DPMS_OFF;
This looks like it really belongs in the ->reset() callback.
> + drm_connector_helper_add(connector, &connector_helper_funcs);
> + ret = drm_connector_register(connector);
> + if (ret < 0)
> + goto err_cleanup;
> +
> + ret = drm_mode_connector_attach_encoder(connector, encoder);
> + if (ret < 0)
> + goto err_sysfs;
> +
> + connector->encoder = encoder;
You did this before already, why repeat it? Why even do it in the first
place? Does this somehow not work if you omit this explicit assignment?
> + drm_object_property_set_value
> + (&connector->base, fsl_dev->ddev->mode_config.dpms_property,
> + DRM_MODE_DPMS_OFF);
This is badly aligned. "&connector->base," should go on the first line
and the second line (all subsequent ones, really) should align with it.
> +
> + panel_node = of_parse_phandle(fsl_dev->np, "panel", 0);
This isn't a standard property, so technically you'd need to prefix it
with a vendor prefix.
> + if (panel_node) {
> + fsl_dev->connector.panel = of_drm_find_panel(panel_node);
> + if (!fsl_dev->connector.panel) {
> + of_node_put(panel_node);
> + ret = -EPROBE_DEFER;
> + goto err_sysfs;
> + }
> + }
> + of_node_put(panel_node);
You're leaking the OF node reference on -EPROBE_DEFER.
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_crtc.c
[...]
> +#include <linux/regmap.h>
> +#include <linux/clk.h>
You should keep these sorted alphabetically, that'll save you headaches
later on.
> +static void fsl_dcu_drm_crtc_mode_set_nofb(struct drm_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->dev;
> + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> + struct drm_display_mode *mode = &crtc->state->mode;
> + uint32_t hbp, hfp, hsw, vbp, vfp, vsw, div, index;
> + int err;
> +
> + DBG(": set mode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x",
> + mode->base.id, mode->name,
> + mode->vrefresh, mode->clock,
> + mode->hdisplay, mode->hsync_start,
> + mode->hsync_end, mode->htotal,
> + mode->vdisplay, mode->vsync_start,
> + mode->vsync_end, mode->vtotal,
> + mode->type, mode->flags);
The KMS helpers already output this line, don't they?
> + index = drm_crtc_index(crtc);
> + div = (uint32_t)clk_get_rate(fsl_dev->clk) / mode->clock / 1000;
You should probably not cast the clk_get_rate() return value. You risk
running into problems if you ever need to support rates higher than 4
GHz. I'll grant you that it's unlikely to happen any time soon, but it
shouldn't be a reason not to write future-proof code.
> +
> + /* Configure timings: */
> + hbp = mode->htotal - mode->hsync_end;
> + hfp = mode->hsync_start - mode->hdisplay;
> + hsw = mode->hsync_end - mode->hsync_start;
> + vbp = mode->vtotal - mode->vsync_end;
> + vfp = mode->vsync_start - mode->vdisplay;
> + vsw = mode->vsync_end - mode->vsync_start;
> +
> + err = regmap_write(fsl_dev->regmap, DCU_HSYN_PARA,
> + DCU_HSYN_PARA_BP(hbp) |
> + DCU_HSYN_PARA_PW(hsw) |
> + DCU_HSYN_PARA_FP(hfp));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_VSYN_PARA,
> + DCU_VSYN_PARA_BP(vbp) |
> + DCU_VSYN_PARA_PW(vsw) |
> + DCU_VSYN_PARA_FP(vfp));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_DISP_SIZE,
> + DCU_DISP_SIZE_DELTA_Y(mode->vdisplay) |
> + DCU_DISP_SIZE_DELTA_X(mode->hdisplay));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_DIV_RATIO, div);
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> + DCU_UPDATE_MODE_READREG);
> + if (err)
> + goto set_failed;
> + return;
> +set_failed:
> + dev_err(dev->dev, "set DCU register failed\n");
> +}
[...]
> +static void fsl_dcu_drm_crtc_enable(struct drm_crtc *crtc)
> +{
> +}
> +
> +static int fsl_dcu_drm_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_crtc_state *state)
> +{
> + return 0;
> +}
> +
> +static void fsl_dcu_drm_crtc_atomic_begin(struct drm_crtc *crtc)
> +{
> +}
> +
> +static void fsl_dcu_drm_crtc_atomic_flush(struct drm_crtc *crtc)
> +{
> +}
> +
> +static void fsl_dcu_drm_disable_crtc(struct drm_crtc *crtc)
> +{
> +}
Really? Then how does the CRTC get enabled or disabled, I wonder?
> +int fsl_dcu_drm_crtc_create(struct fsl_dcu_drm_device *fsl_dev)
> +{
> + struct drm_plane *primary;
> + struct drm_crtc *crtc = &fsl_dev->crtc;
> + int i, ret;
i could be unsigned int.
> +
> + primary = fsl_dcu_drm_primary_create_plane(fsl_dev->ddev);
> + ret = drm_crtc_init_with_planes(fsl_dev->ddev, crtc, primary, NULL,
> + &fsl_dcu_drm_crtc_funcs);
> + if (ret < 0)
> + return ret;
> +
> + drm_crtc_helper_add(crtc, &fsl_dcu_drm_crtc_helper_funcs);
> +
> + for (i = 0; i < DCU_TOTAL_LAYER_NUM; i++) {
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_1(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_2(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_3(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_4(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_5(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_6(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_7(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_8(i), 0);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_9(i), 0);
> + if (ret)
> + goto init_failed;
This is highly repetitive, perhaps make a DCU_CTRLDESCLN(x, y) instead
and use nested loops here?
> + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) {
> + ret = regmap_write(fsl_dev->regmap,
> + DCU_CTRLDESCLN_10(i), 0);
> + if (ret)
> + goto init_failed;
> + }
> + }
Might be better to introduce some sort of per-SoC capability structure
to parameterize, so that you can avoid this sort of check on the OF
node's compatible string. You typically do that by setting the OF match
table's entries' .data field to the instance of the structure for the
particular SoC or hardware block.
> + ret = regmap_write(fsl_dev->regmap, DCU_SYN_POL,
> + DCU_SYN_POL_INV_VS_LOW | DCU_SYN_POL_INV_HS_LOW);
> + if (ret)
> + goto init_failed;
This looks like it should be part of the ->mode_set_nofb()
implementation and depend on parameters of the mode.
> + ret = regmap_write(fsl_dev->regmap, DCU_BGND, DCU_BGND_R(0) |
> + DCU_BGND_G(0) | DCU_BGND_B(0));
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_DCU_MODE,
> + DCU_MODE_BLEND_ITER(1) | DCU_MODE_RASTER_EN);
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_THRESHOLD,
> + DCU_THRESHOLD_LS_BF_VS(BF_VS_VAL) |
> + DCU_THRESHOLD_OUT_BUF_HIGH(BUF_MAX_VAL) |
> + DCU_THRESHOLD_OUT_BUF_LOW(BUF_MIN_VAL));
> + if (ret)
> + goto init_failed;
> + ret = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> + DCU_MODE_DCU_MODE_MASK,
> + DCU_MODE_DCU_MODE(DCU_MODE_OFF));
> + if (ret)
> + goto init_failed;
> + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> + DCU_UPDATE_MODE_READREG);
> + if (ret)
> + goto init_failed;
> +
> + return 0;
> +init_failed:
> + dev_err(fsl_dev->dev, "init DCU register failed\n");
> + return ret;
> +}
[...]
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
[...]
> +static int fsl_dcu_unload(struct drm_device *dev)
> +{
> + drm_mode_config_cleanup(dev);
> + drm_vblank_cleanup(dev);
> + drm_irq_uninstall(dev);
> +
> + dev->dev_private = NULL;
> +
> + return 0;
> +}
> +
> +static struct regmap_config fsl_dcu_regmap_config = {
static const
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> +};
> +
> +static int fsl_dcu_bypass_tcon(struct fsl_dcu_drm_device *fsl_dev,
> + struct device_node *np)
> +{
> + struct device_node *tcon_np;
> + struct platform_device *pdev;
> + struct clk *tcon_clk;
> + struct resource *res;
> + void __iomem *base;
> + int ret;
> +
> + tcon_np = of_parse_phandle(np, "tcon-controller", 0);
> + if (!tcon_np)
> + return -EINVAL;
> +
> + pdev = of_find_device_by_node(tcon_np);
> + if (!pdev)
> + return -EINVAL;
> +
> + tcon_clk = devm_clk_get(&pdev->dev, "tcon");
> + if (IS_ERR(tcon_clk))
> + return PTR_ERR(tcon_clk);
> + ret = clk_prepare(tcon_clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to prepare tcon clk\n");
> + return ret;
> + }
> + ret = clk_enable(tcon_clk);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to enable tcon clk\n");
> + clk_unprepare(tcon_clk);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res)
> + return -ENODEV;
> +
> + base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + fsl_dev->tcon_regmap = devm_regmap_init_mmio(&pdev->dev,
> + base, &fsl_dcu_regmap_config);
> + if (IS_ERR(fsl_dev->tcon_regmap)) {
> + dev_err(&pdev->dev, "regmap init failed\n");
> + return PTR_ERR(fsl_dev->tcon_regmap);
> + }
> +
> + ret = regmap_write(fsl_dev->tcon_regmap,
> + TCON_CTRL1, TCON_BYPASS_ENABLE);
> + if (ret) {
> + dev_err(&pdev->dev, "set TCON_CTRL1 failed\n");
> + return ret;
> + }
> + return 0;
> +}
Erm... no. You should have a proper TCON driver to take care of this. If
it's only used by the display driver, the TCON driver could be part of
the DRM driver.
But as it is, you're side-stepping the driver model and are leaving
resource crumbs all over the place that noone will get a chance to clean
up.
> +
> +static void dcu_pixclk_enable(void)
> +{
> + struct regmap *scfg_regmap;
> +
> + scfg_regmap = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
> + if (IS_ERR(scfg_regmap)) {
> + pr_err("No syscfg phandle specified\n");
> + return;
> + }
> +
> + regmap_write(scfg_regmap, SCFG_PIXCLKCR, PXCK_ENABLE);
> +}
This should be a clock driver. Chances are that you're going to need to
change the pixel clock frequency at some point, in which case you're
going to need a proper pixel clock anyway.
That said, you already have a "dcu" clock that you use. According to the
modeset code it's some kind of parent of the pixel clock (you derive a
divider based on the "dcu" clock frequency and the mode clock). You
should really model this properly as a clock tree.
> +static int fsl_dcu_drm_irq_init(struct drm_device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev->dev);
> + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> + unsigned int int_mask;
> + int ret;
> +
> + ret = drm_irq_install(dev, platform_get_irq(pdev, 0));
> + if (ret < 0)
> + dev_err(&pdev->dev, "failed to install IRQ handler\n");
If you think you'll ever need to support multiple interrupts, I'd
suggest you don't use drm_irq_install() but rather set up the interrupt
yourself. It's not difficult to do and will give you some more
flexibility in turn.
> + dev->irq_enabled = true;
> + dev->vblank_disable_allowed = true;
> +
> + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0);
> + if (ret)
> + dev_err(&pdev->dev, "set DCU_INT_STATUS failed\n");
> + ret = regmap_read(fsl_dev->regmap, DCU_INT_MASK, &int_mask);
> + if (ret)
> + dev_err(&pdev->dev, "read DCU_INT_MASK failed\n");
> + ret = regmap_write(fsl_dev->regmap, DCU_INT_MASK, int_mask &
> + ~DCU_INT_MASK_VBLANK);
> + if (ret)
> + dev_err(&pdev->dev, "set DCU_INT_MASK failed\n");
> + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> + DCU_UPDATE_MODE_READREG);
What's this DCU_UPDATE_MODE_READREG bit?
> +static int fsl_dcu_load(struct drm_device *ddev, unsigned long flags)
> +{
> + struct device *dev = ddev->dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct fsl_dcu_drm_device *fsl_dev;
> + struct resource *res;
> + void __iomem *base;
> + int ret;
> +
> + fsl_dev = devm_kzalloc(dev, sizeof(*fsl_dev), GFP_KERNEL);
> + if (!fsl_dev)
> + return -ENOMEM;
> +
> + fsl_dev->dev = dev;
> + fsl_dev->ddev = ddev;
> + fsl_dev->np = dev->of_node;
> + ddev->dev_private = fsl_dev;
> + dev_set_drvdata(dev, fsl_dev);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "could not get memory IO resource\n");
> + return -ENODEV;
> + }
> +
> + base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(base)) {
> + ret = PTR_ERR(base);
> + return ret;
> + }
> +
> + fsl_dev->clk = devm_clk_get(dev, "dcu");
> + if (IS_ERR(fsl_dev->clk)) {
> + ret = PTR_ERR(fsl_dev->clk);
> + dev_err(dev, "failed to get dcu clock\n");
> + return ret;
> + }
> + ret = clk_prepare(fsl_dev->clk);
> + if (ret < 0) {
> + dev_err(dev, "failed to prepare dcu clk\n");
> + return ret;
> + }
> + ret = clk_enable(fsl_dev->clk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable dcu clk\n");
> + clk_unprepare(fsl_dev->clk);
> + return ret;
> + }
> +
> + fsl_dev->regmap = devm_regmap_init_mmio(dev, base,
> + &fsl_dcu_regmap_config);
> + if (IS_ERR(fsl_dev->regmap)) {
> + dev_err(dev, "regmap init failed\n");
> + return PTR_ERR(fsl_dev->regmap);
> + }
> +
> + /* Put TCON in bypass mode, so the input signals from DCU are passed
> + * through TCON unchanged */
> + fsl_dcu_bypass_tcon(fsl_dev, fsl_dev->np);
> +
> + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu"))
> + dcu_pixclk_enable();
All of the above really belongs in ->probe(). If you do that you don't
need to involve anything of DRM until after all resources have been
successfully claimed. Also it allows you to get rid of the ugly upcast
from struct device to struct platform_device, because ->probe() will
directly give you a platform device.
> + ret = fsl_dcu_drm_modeset_init(fsl_dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to initialize mode setting\n");
> + return ret;
> + }
> +
> + ret = drm_vblank_init(ddev, ddev->mode_config.num_crtc);
> + if (ret < 0) {
> + dev_err(dev, "failed to initialize vblank\n");
> + goto done;
> + }
> +
> + ret = fsl_dcu_drm_irq_init(ddev);
> + if (ret < 0)
> + goto done;
> +
> + fsl_dcu_fbdev_init(ddev);
> +
> + return 0;
> +done:
> + if (ret)
> + fsl_dcu_unload(ddev);
> +
> + return ret;
> +}
> +
> +static void fsl_dcu_drm_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> +}
> +
> +static irqreturn_t fsl_dcu_drm_irq(int irq, void *arg)
> +{
> + struct drm_device *dev = arg;
> + struct fsl_dcu_drm_device *fsl_dev = dev->dev_private;
> + unsigned int int_status;
> + int ret;
> +
> + regmap_read(fsl_dev->regmap, DCU_INT_STATUS, &int_status);
No error checking here?
> + if (int_status & DCU_INT_STATUS_VBLANK)
> + drm_handle_vblank(dev, 0);
> +
> + ret = regmap_write(fsl_dev->regmap, DCU_INT_STATUS, 0xffffffff);
> + if (ret)
> + dev_err(dev->dev, "set DCU_INT_STATUS failed\n");
> + ret = regmap_write(fsl_dev->regmap, DCU_UPDATE_MODE,
> + DCU_UPDATE_MODE_READREG);
> + if (ret)
> + dev_err(dev->dev, "set DCU_UPDATE_MODE failed\n");
Here it is again. Is this some sort of manual trigger for screen
updates?
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int fsl_dcu_drm_enable_vblank(struct drm_device *dev, int crtc)
> +{
> + return 0;
> +}
> +
> +static void fsl_dcu_drm_disable_vblank(struct drm_device *dev, int crtc)
> +{
> +}
You do have a way to enable and disable VBLANK, why don't you use it?
> +static struct drm_driver fsl_dcu_drm_driver = {
> + .driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET
> + | DRIVER_PRIME | DRIVER_ATOMIC,
> + .load = fsl_dcu_load,
> + .unload = fsl_dcu_unload,
> + .preclose = fsl_dcu_drm_preclose,
> + .irq_handler = fsl_dcu_drm_irq,
> + .get_vblank_counter = drm_vblank_count,
> + .enable_vblank = fsl_dcu_drm_enable_vblank,
> + .disable_vblank = fsl_dcu_drm_disable_vblank,
> + .gem_free_object = drm_gem_cma_free_object,
> + .gem_vm_ops = &drm_gem_cma_vm_ops,
> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> + .gem_prime_import = drm_gem_prime_import,
> + .gem_prime_export = drm_gem_prime_export,
> + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
> + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> + .gem_prime_vmap = drm_gem_cma_prime_vmap,
> + .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
> + .gem_prime_mmap = drm_gem_cma_prime_mmap,
> + .dumb_create = drm_gem_cma_dumb_create,
> + .dumb_map_offset = drm_gem_cma_dumb_map_offset,
> + .dumb_destroy = drm_gem_dumb_destroy,
> + .fops = &fsl_dcu_drm_fops,
> + .name = "fsl-dcu-drm",
> + .desc = "Freescale DCU DRM",
> + .date = "20150213",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +static void dcu_pixclk_disable(void)
> +{
> + struct regmap *scfg_regmap;
> +
> + scfg_regmap = syscon_regmap_lookup_by_compatible("fsl,ls1021a-scfg");
> + if (IS_ERR(scfg_regmap)) {
> + pr_err("No syscfg phandle specified\n");
> + return;
> + }
> +
> + regmap_write(scfg_regmap, SCFG_PIXCLKCR, PXCK_DISABLE);
> +}
Again, if this was modelled as a clock, you could hide all this
integration glue from the display driver.
> +static int fsl_dcu_drm_pm_suspend(struct device *dev)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> +
> + if (!fsl_dev)
> + return 0;
> +
> + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu"))
> + dcu_pixclk_disable();
> +
> + drm_kms_helper_poll_disable(fsl_dev->ddev);
> + regcache_cache_only(fsl_dev->regmap, true);
> + regcache_mark_dirty(fsl_dev->regmap);
> + clk_disable(fsl_dev->clk);
Why not unprepare the clock at the same time?
> +
> + if (fsl_dev->tcon_regmap) {
> + regcache_cache_only(fsl_dev->tcon_regmap, true);
> + regcache_mark_dirty(fsl_dev->tcon_regmap);
> + clk_disable(fsl_dev->tcon_clk);
Same here.
> + }
> +
> + return 0;
> +}
> +
> +static int fsl_dcu_drm_pm_resume(struct device *dev)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!fsl_dev)
> + return 0;
> +
> + /* Enable clocks and restore all registers */
> + if (fsl_dev->tcon_regmap) {
> + ret = clk_enable(fsl_dev->tcon_clk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable tcon clk\n");
> + clk_unprepare(fsl_dev->tcon_clk);
> + return ret;
> + }
> + regcache_cache_only(fsl_dev->tcon_regmap, false);
> + regcache_sync(fsl_dev->tcon_regmap);
> + }
> +
> + ret = clk_enable(fsl_dev->clk);
> + if (ret < 0) {
> + dev_err(dev, "failed to enable dcu clk\n");
> + clk_unprepare(fsl_dev->clk);
> + return ret;
> + }
> +
> + drm_kms_helper_poll_enable(fsl_dev->ddev);
> + regcache_cache_only(fsl_dev->regmap, false);
> + regcache_sync(fsl_dev->regmap);
> +
> + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu"))
> + dcu_pixclk_enable();
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops fsl_dcu_drm_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(fsl_dcu_drm_pm_suspend, fsl_dcu_drm_pm_resume)
> +};
> +
> +static int fsl_dcu_drm_probe(struct platform_device *pdev)
> +{
> + struct drm_driver *driver = &fsl_dcu_drm_driver;
> + struct drm_device *drm;
> + int err;
> +
> + drm = drm_dev_alloc(driver, &pdev->dev);
> + if (!drm)
> + return -ENOMEM;
> +
> + drm_dev_set_unique(drm, dev_name(&pdev->dev));
> +
> + err = drm_dev_register(drm, 0);
> + if (err < 0)
> + goto unref;
> +
> + DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n", driver->name,
> + driver->major, driver->minor, driver->patchlevel,
> + driver->date, drm->primary->index);
> +
> + return 0;
> +
> +unref:
> + drm_dev_unref(drm);
> + return err;
> +}
> +
> +static int fsl_dcu_drm_remove(struct platform_device *pdev)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = platform_get_drvdata(pdev);
> +
> + drm_put_dev(fsl_dev->ddev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id fsl_dcu_of_match[] = {
> + { .compatible = "fsl,ls1021a-dcu", },
> + { .compatible = "fsl,vf610-dcu", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, fsl_dcu_of_match);
> +
> +static struct platform_driver fsl_dcu_drm_platform_driver = {
> + .probe = fsl_dcu_drm_probe,
> + .remove = fsl_dcu_drm_remove,
> + .driver = {
> + .name = "fsl,dcu",
I don't think it's typical to use "vendor-prefix," in platform driver
names. Perhaps a more typical name would be "fsl-dcu".
> + .pm = &fsl_dcu_drm_pm_ops,
> + .of_match_table = fsl_dcu_of_match,
> + },
> +};
> +
> +module_platform_driver(fsl_dcu_drm_platform_driver);
> +
> +MODULE_DESCRIPTION("Freescale DCU DRM Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.h
[...]
> +#define DCU_DISP_SIZE 0x0018
> +#define DCU_DISP_SIZE_DELTA_Y(x) ((x) << 16)
> +/*Regisiter value 1/16 of horizontal resolution*/
> +#define DCU_DISP_SIZE_DELTA_X(x) ((x) >> 4)
Does this mean the display controller only supports horizontal
resolutions that are a multiple of 16?
> +#ifdef CONFIG_SOC_VF610
> +#define DCU_TOTAL_LAYER_NUM 64
> +#define DCU_LAYER_NUM_MAX 6
> +#else
> +#define DCU_TOTAL_LAYER_NUM 16
> +#define DCU_LAYER_NUM_MAX 4
> +#endif
Should this not be a runtime decision so that the same driver can run on
VF610 and LS1021A platforms?
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c
[...]
> new file mode 100644
> index 0000000..f8ef0e1
> --- /dev/null
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_fbdev.c
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright 2015 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
The rest of your driver is GPL v2 (or later), this file isn't. Pick one.
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c
[...]
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <linux/regmap.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +
> +#include "fsl_dcu_drm_drv.h"
> +#include "fsl_dcu_drm_kms.h"
> +#include "fsl_dcu_drm_plane.h"
> +
> +#define to_fsl_dcu_plane(plane) \
> + container_of(plane, struct fsl_dcu_drm_plane, plane)
> +
> +static int
> +fsl_dcu_drm_plane_prepare_fb(struct drm_plane *plane,
> + struct drm_framebuffer *fb,
> + const struct drm_plane_state *new_state)
> +{
> + return 0;
> +}
> +
> +static void
> +fsl_dcu_drm_plane_cleanup_fb(struct drm_plane *plane,
> + struct drm_framebuffer *fb,
> + const struct drm_plane_state *new_state)
> +{
> +}
> +
> +static int fsl_dcu_drm_plane_atomic_check(struct drm_plane *plane,
> + struct drm_plane_state *state)
> +{
> + return 0;
> +}
> +
> +static void fsl_dcu_drm_plane_atomic_disable(struct drm_plane *plane,
> + struct drm_plane_state *old_state)
> +{
> +}
This really should disable the plane. Perhaps clear DCU_CTRLDESCLN_4_EN
here?
> +void fsl_dcu_drm_plane_atomic_update(struct drm_plane *plane,
> + struct drm_plane_state *old_state)
> +{
> + struct fsl_dcu_drm_device *fsl_dev = plane->dev->dev_private;
> + struct drm_plane_state *state = plane->state;
> + struct drm_framebuffer *fb = plane->state->fb;
> + struct drm_gem_cma_object *gem;
> + struct fsl_dcu_drm_plane *fsl_plane = to_fsl_dcu_plane(plane);
> + u32 index, alpha, bpp;
> + int err;
> +
> + if (!fb)
> + return;
> +
> + index = fsl_plane->index;
> + gem = drm_fb_cma_get_gem_obj(fb, 0);
> +
> + switch (fb->pixel_format) {
> + case DRM_FORMAT_RGB565:
> + bpp = FSL_DCU_RGB565;
> + alpha = 0xff;
> + break;
> + case DRM_FORMAT_RGB888:
> + bpp = FSL_DCU_RGB888;
> + alpha = 0xff;
> + break;
> + case DRM_FORMAT_ARGB8888:
> + bpp = FSL_DCU_ARGB8888;
> + alpha = 0xff;
> + break;
> + case DRM_FORMAT_BGRA4444:
> + bpp = FSL_DCU_ARGB4444;
> + alpha = 0xff;
> + break;
> + case DRM_FORMAT_ARGB1555:
> + bpp = FSL_DCU_ARGB1555;
> + alpha = 0xff;
> + break;
> + case DRM_FORMAT_YUV422:
> + bpp = FSL_DCU_YUV422;
> + alpha = 0xff;
> + break;
> + default:
> + return;
> + }
You should do this in ->atomic_check() already so that you can guarantee
that the selected format can be set. The DRM core should already check
it for you, but silently aborting here still doesn't seem like the right
solution.
> +
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_1(index),
> + DCU_CTRLDESCLN_1_HEIGHT(state->crtc_h) |
> + DCU_CTRLDESCLN_1_WIDTH(state->crtc_w));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_2(index),
> + DCU_CTRLDESCLN_2_POSY(state->crtc_y) |
> + DCU_CTRLDESCLN_2_POSX(state->crtc_x));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap,
> + DCU_CTRLDESCLN_3(index), gem->paddr);
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_4(index),
> + DCU_CTRLDESCLN_4_EN |
> + DCU_CTRLDESCLN_4_TRANS(alpha) |
> + DCU_CTRLDESCLN_4_BPP(bpp) |
> + DCU_CTRLDESCLN_4_AB(0));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_5(index),
> + DCU_CTRLDESCLN_5_CKMAX_R(0xFF) |
> + DCU_CTRLDESCLN_5_CKMAX_G(0xFF) |
> + DCU_CTRLDESCLN_5_CKMAX_B(0xFF));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_6(index),
> + DCU_CTRLDESCLN_6_CKMIN_R(0) |
> + DCU_CTRLDESCLN_6_CKMIN_G(0) |
> + DCU_CTRLDESCLN_6_CKMIN_B(0));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_7(index), 0);
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_8(index),
> + DCU_CTRLDESCLN_8_FG_FCOLOR(0));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_9(index),
> + DCU_CTRLDESCLN_9_BG_BCOLOR(0));
> + if (err)
> + goto set_failed;
> + if (of_device_is_compatible(fsl_dev->np, "fsl,ls1021a-dcu")) {
> + err = regmap_write(fsl_dev->regmap, DCU_CTRLDESCLN_10(index),
> + DCU_CTRLDESCLN_10_POST_SKIP(0) |
> + DCU_CTRLDESCLN_10_PRE_SKIP(0));
> + if (err)
> + goto set_failed;
> + }
> + err = regmap_update_bits(fsl_dev->regmap, DCU_DCU_MODE,
> + DCU_MODE_DCU_MODE_MASK,
> + DCU_MODE_DCU_MODE(DCU_MODE_NORMAL));
> + if (err)
> + goto set_failed;
> + err = regmap_write(fsl_dev->regmap,
> + DCU_UPDATE_MODE, DCU_UPDATE_MODE_READREG);
> + if (err)
> + goto set_failed;
> + return;
> +
> +set_failed:
> + dev_err(fsl_dev->dev, "set DCU register failed\n");
> +}
> +
> +int fsl_dcu_drm_plane_disable(struct drm_plane *plane)
> +{
> + return 0;
> +}
> +
> +void fsl_dcu_drm_plane_destroy(struct drm_plane *plane)
> +{
> + fsl_dcu_drm_plane_disable(plane);
Hmm? This function doesn't do anything, so why not just drop it?
> + drm_plane_cleanup(plane);
> +}
Also this function and ->atomic_update() should be static. Perhaps make
it a habit of running your build tests with C=1 occasionally to get
notified of this kind of error.
Thierry
Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)
Powered by blists - more mailing lists