Dear Daniel,
On Wed, 18 Mar 2015 12:24:40 +0000 Daniel Stone daniel@fooishbar.org wrote:
Hi, Some feedback comments - most of these are not unique to your 5433 DECON driver but endemic throughout Exynos, so I don't blame you for them - but they should be fixed anyway.
On 18 March 2015 at 08:16, Hyungwon Hwang human.hwang@samsung.com wrote:
+static void decon_dpms_on(struct exynos_decon *decon) +{
int ret;int i;for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {ret = clk_prepare_enable(decon->clks[i]);if (ret < 0)goto err;}set_bit(BIT_CLKS_ENABLED, &decon->enabled);Do you really not even need to set a control register?
+static void decon_commit(struct exynos_drm_crtc *crtc) +{
struct exynos_decon *decon = crtc->ctx;struct drm_display_mode *mode = &crtc->base.mode;u32 val;/* enable clock gate */val = CMU_CLKGAGE_MODE_SFR_F | CMU_CLKGAGE_MODE_MEM_F;writel(val, decon->addr + DECON_CMU);/* lcd on and use command if */val = VIDOUT_LCD_ON;if (decon->i80_if)val |= VIDOUT_COMMAND_IF;elseval |= VIDOUT_RGB_IF;writel(val, decon->addr + DECON_VIDOUTCON0);This seems much more likely to be DPMS, no?
[...]/* enable output and display signal */val = VIDCON0_ENVID | VIDCON0_ENVID_F;writel(val, decon->addr + DECON_VIDCON0);As does this.
Have you tested DPMS on/off, without enabling/disabling the CRTC first? Does it work?
Yes. I misunderstanded the behavior of the driver. I will send the fixed version for working DPMS. Thanks.
+static void decon_win_mode_set(struct exynos_drm_crtc *crtc,
struct exynos_drm_plane *plane)+{
struct exynos_decon *decon = crtc->ctx;struct decon_reg_data *reg_data;unsigned int bytes_per_pixel = plane->bpp >> 3;unsigned int val_h;unsigned int val_l;unsigned int win;dma_addr_t addr;u32 val = 0;if (!plane) {DRM_ERROR("plane is NULL\n");return;}win = plane->zpos;if (win == DEFAULT_ZPOS)win = 0;if (win < 0 || win >= 5)return;It would be nice to have a #define for the largest-supported window number.
Yes. That would be better.
reg_data = &decon->reg_data[win];switch (plane->pixel_format) {case DRM_FORMAT_XRGB1555:val |= WINCONx_BPPMODE_16BPP_I1555;val |= WINCONx_HAWSWP_F;val |= WINCONx_BURSTLEN_16WORD;break;case DRM_FORMAT_RGB565:val |= WINCONx_BPPMODE_16BPP_565;val |= WINCONx_HAWSWP_F;val |= WINCONx_BURSTLEN_16WORD;break;case DRM_FORMAT_XRGB8888:val |= WINCONx_BPPMODE_24BPP_888;val |= WINCONx_WSWP_F;val |= WINCONx_BURSTLEN_16WORD;break;case DRM_FORMAT_ARGB8888:val |= WINCONx_BPPMODE_32BPP_A8888;val |= WINCONx_WSWP_F | WINCONx_BLD_PIX_F |WINCONx_ALPHA_SEL_F;
val |= WINCONx_BURSTLEN_16WORD;break;default:Please remove the 'default' case. If you get here with a format you don't know how to configure, then it is a bug and should be fixed: the plane should never advertise a format that it cannot support.
Yes. I agree.
val |= WINCONx_BPPMODE_24BPP_888;val |= WINCONx_WSWP_F;val |= WINCONx_BURSTLEN_16WORD;break;}reg_data->wincon = val;reg_data->vidosd_a = COORDINATE_X(plane->crtc_x) |COORDINATE_Y(plane->crtc_y);reg_data->vidosd_b =COORDINATE_X(plane->crtc_x + plane->crtc_width - 1)|
COORDINATE_Y(plane->crtc_y + plane->crtc_height -1);
reg_data->vidosd_c = VIDOSD_Wx_ALPHA_R_F(0x0) |VIDOSD_Wx_ALPHA_G_F(0x0) |VIDOSD_Wx_ALPHA_B_F(0x0);reg_data->vidosd_d = VIDOSD_Wx_ALPHA_R_F(0x0) |VIDOSD_Wx_ALPHA_G_F(0x0) |VIDOSD_Wx_ALPHA_B_F(0x0);addr = plane->dma_addr[0];addr += plane->fb_width * plane->fb_y * bytes_per_pixel;Replace plane->fb_width * bytes_per_pixel by plane->fb_pitch please, and set plane->fb_pitch from exynos_drm_plane->pitch. See this patch: https://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg42861.html
You should be able to test this case, either by making a specialised userspace program which has a larger pitch with garbage values in the padding (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa473780(v=vs.85).a...), or by testing a resolution where width*bytespp is not a multiple of 4, e.g. 1366x768.
Yes. I agree.
addr += plane->fb_x * bytes_per_pixel;reg_data->vidw_add0 = addr;addr += plane->fb_width * plane->crtc_height *bytes_per_pixel;
Again, replace fb_width*bytes_per_pixel with fb_pitch.
reg_data->vidw_add1 = addr;val_h = (plane->fb_width - plane->crtc_width) *bytes_per_pixel;
val_h = plane->fb_pitch - (plane->crtc_width * bytes_per_pixel);
+static void decon_win_commit(struct exynos_drm_crtc *crtc, int zpos) +{
struct exynos_decon *decon = crtc->ctx;struct decon_reg_data *reg_data;unsigned int win = zpos;u32 val;if (win == DEFAULT_ZPOS)win = 0;if (win < 0 || win >= 5)return;reg_data = &decon->reg_data[win];/* shadow update disable */val = readl(decon->addr + DECON_SHADOWCON);val |= SHADOWCON_Wx_PROTECT(win);writel(val, decon->addr + DECON_SHADOWCON);writel(reg_data->vidosd_a, decon->addr +DECON_VIDOSDxA(win));
writel(reg_data->vidosd_b, decon->addr +DECON_VIDOSDxB(win));
writel(reg_data->vidosd_c, decon->addr +DECON_VIDOSDxC(win));
writel(reg_data->vidosd_d, decon->addr +DECON_VIDOSDxD(win));
writel(reg_data->vidw_add0, decon->addr +DECON_VIDW0xADD0B0(win));
writel(reg_data->vidw_add1, decon->addr +DECON_VIDW0xADD1B0(win));
writel(reg_data->vidw_add2, decon->addr +DECON_VIDW0xADD2(win)); +
/* window enable */val = reg_data->wincon;val |= WINCONx_ENWIN_F;writel(val, decon->addr + DECON_WINCONx(win));/* shadow update enable */val = readl(decon->addr + DECON_SHADOWCON);val &= ~SHADOWCON_Wx_PROTECT(win);writel(val, decon->addr + DECON_SHADOWCON);/* standalone update */val = readl(decon->addr + DECON_UPDATE);val |= STANDALONE_UPDATE_F;writel(val, decon->addr + DECON_UPDATE);if (decon->i80_if)atomic_set(&decon->win_updated, 1);+}
+static void decon_win_disable(struct exynos_drm_crtc *crtc, int zpos) +{
struct exynos_decon *decon = crtc->ctx;struct decon_reg_data *reg_data;unsigned int win = zpos;u32 val;if (win == DEFAULT_ZPOS)win = 0;if (win < 0 || win >= 5)return;reg_data = &decon->reg_data[win];/* shadow update disable */val = readl(decon->addr + DECON_SHADOWCON);val |= SHADOWCON_Wx_PROTECT(win);writel(val, decon->addr + DECON_SHADOWCON);/* window disable */val = reg_data->wincon;val &= ~WINCONx_ENWIN_F;writel(val, decon->addr + DECON_WINCONx(win));/* shadow update enable */val = readl(decon->addr + DECON_SHADOWCON);val &= ~SHADOWCON_Wx_PROTECT(win);writel(val, decon->addr + DECON_SHADOWCON);/* standalone update */val = readl(decon->addr + DECON_UPDATE);val |= STANDALONE_UPDATE_F;writel(val, decon->addr + DECON_UPDATE);+}
+void decon_te_irq_handler(struct exynos_drm_crtc *crtc) +{
struct exynos_decon *decon = crtc->ctx;u32 val;if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))return;if (atomic_add_unless(&decon->win_updated, -1, 0)) {/* trigger */val = readl(decon->addr + DECON_TRIGCON);val |= TRIGCON_SWTRIGCMD;writel(val, decon->addr + DECON_TRIGCON);}+}
+static struct exynos_drm_crtc_ops decon_crtc_ops = {
.dpms = decon_dpms,.enable_vblank = decon_enable_vblank,.disable_vblank = decon_disable_vblank,.commit = decon_commit,.win_mode_set = decon_win_mode_set,.win_commit = decon_win_commit,.win_disable = decon_win_disable,.te_handler = decon_te_irq_handler,+};
+static int decon_bind(struct device *dev, struct device *master, void *data) +{
struct exynos_decon *decon = dev_get_drvdata(dev);struct drm_device *drm_dev = data;struct exynos_drm_private *priv = drm_dev->dev_private;decon->drm_dev = drm_dev;decon->pipe = priv->pipe++;decon->crtc = exynos_drm_crtc_create(drm_dev, decon->pipe,EXYNOS_DISPLAY_TYPE_LCD, &decon_crtc_ops,decon);
if (IS_ERR(decon->crtc))return PTR_ERR(decon->crtc);Should failing to register the CRTC also decrement priv->pipe?
Yes. That's right.
return 0;+}
+static void decon_unbind(struct device *dev, struct device *master, void *data) +{
struct exynos_decon *decon = dev_get_drvdata(dev);decon_dpms(decon->crtc, DRM_MODE_DPMS_OFF);+}
As above, it seems like DPMS will not do enough here.
+static const struct component_ops decon_component_ops = {
.bind = decon_bind,.unbind = decon_unbind,+};
+static irqreturn_t decon_vsync_irq_handler(int irq, void *dev_id) +{
struct exynos_decon *decon = dev_id;u32 val;if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))goto out;val = readl(decon->addr + DECON_VIDINTCON1);if (val & VIDINTCON1_INTFRMPEND) {if (decon->drm_dev)drm_handle_vblank(decon->drm_dev,decon->pipe);
If decon->drm_dev is ever NULL, it sounds like something has gone very seriously wrong.
decon->drm_dev will not be NULL at any time. So I will remove the if statement.
/* clear */writel(VIDINTCON1_INTFRMPEND, decon->addr +DECON_VIDINTCON1);
}+out:
return IRQ_HANDLED;+}
+static irqreturn_t decon_lcd_sys_irq_handler(int irq, void *dev_id) +{
struct exynos_decon *decon = dev_id;u32 val;if (!test_bit(BIT_CLKS_ENABLED, &decon->enabled))goto out;val = readl(decon->addr + DECON_VIDINTCON1);if (val & VIDINTCON1_INTFRMDONEPEND) {if (decon->drm_dev)exynos_drm_crtc_finish_pageflip(decon->drm_dev,
decon->pipe);
Ditto - when would drm_dev be NULL?
Ditto.
/* clear */writel(VIDINTCON1_INTFRMDONEPEND,decon->addr + DECON_VIDINTCON1);}+out:
return IRQ_HANDLED;+}
+static int exynos5433_decon_probe(struct platform_device *pdev) +{
struct device *dev = &pdev->dev;struct exynos_decon *decon;struct resource *res;int ret;int i;decon = devm_kzalloc(dev, sizeof(*decon), GFP_KERNEL);if (!decon)return -ENOMEM;decon->dev = dev;if (of_get_child_by_name(dev->of_node, "i80-if-timings"))decon->i80_if = true;for (i = 0; i < ARRAY_SIZE(decon_clks_name); i++) {struct clk *clk;clk = devm_clk_get(decon->dev, decon_clks_name[i]);if (IS_ERR(clk))return PTR_ERR(clk);decon->clks[i] = clk;}res = platform_get_resource(pdev, IORESOURCE_MEM, 0);if (!res) {dev_err(dev, "cannot find IO resource\n");return -ENXIO;}decon->addr = devm_ioremap_resource(dev, res);if (IS_ERR(decon->addr)) {dev_err(dev, "ioremap failed\n");return PTR_ERR(decon->addr);}res = platform_get_resource_byname(pdev, IORESOURCE_IRQ,decon->i80_if ? "lcd_sys" : "vsync");if (!res) {dev_err(dev, "cannot find IRQ resource\n");return -ENXIO;}ret = devm_request_irq(dev, res->start, decon->i80_if ?decon_lcd_sys_irq_handler :decon_vsync_irq_handler, 0,
"drm_decon", decon);if (ret < 0) {dev_err(dev, "lcd_sys irq request failed\n");return ret;}ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,EXYNOS_DISPLAY_TYPE_LCD);if (ret < 0)return ret;platform_set_drvdata(pdev, decon);ret = component_add(dev, &decon_component_ops);if (ret < 0) {exynos_drm_component_del(dev,EXYNOS_DEVICE_TYPE_CRTC);
return ret;}return 0;+}
+static int exynos5433_decon_remove(struct platform_device *pdev) +{
component_del(&pdev->dev, &decon_component_ops);exynos_drm_component_del(&pdev->dev,EXYNOS_DEVICE_TYPE_CRTC); +
return 0;+}
+static const struct of_device_id exynos5433_decon_driver_dt_match[] = {
{ .compatible = "samsung,exynos5433-decon" },{},+}; +MODULE_DEVICE_TABLE(of, exynos5433_decon_driver_dt_match);
+struct platform_driver exynos5433_decon_driver = {
.probe = exynos5433_decon_probe,.remove = exynos5433_decon_remove,.driver = {.name = "exynos5433-decon",.owner = THIS_MODULE,.of_match_table = exynos5433_decon_driver_dt_match,},+}; diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index 98a239a..1fa0dd0 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -556,6 +556,9 @@ static struct platform_driver *const exynos_drm_kms_drivers[] = { #ifdef CONFIG_DRM_EXYNOS_FIMD &fimd_driver, #endif +#ifdef CONFIG_DRM_EXYNOS5433_DECON
&exynos5433_decon_driver,+#endif #ifdef CONFIG_DRM_EXYNOS7_DECON &decon_driver, #endif diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 9afd390..40996d8 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -344,6 +344,7 @@ void exynos_drm_component_del(struct device *dev, enum exynos_drm_device_type dev_type);
extern struct platform_driver fimd_driver; +extern struct platform_driver exynos5433_decon_driver; extern struct platform_driver decon_driver; extern struct platform_driver dp_driver; extern struct platform_driver dsi_driver; diff --git a/drivers/gpu/drm/exynos/regs-exynos5433-decon.h b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h new file mode 100644 index 0000000..9e7f851 --- /dev/null +++ b/drivers/gpu/drm/exynos/regs-exynos5433-decon.h @@ -0,0 +1,163 @@ +/*
- Copyright (C) 2014 Samsung Electronics Co.Ltd
- 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 Foundationr
- */
+#ifndef EXYNOS_REGS_DECON_H +#define EXYNOS_REGS_DECON_H
+/* Exynos543X DECON */ +#define DECON_VIDCON0 0x0000 +#define DECON_VIDOUTCON0 0x0010 +#define DECON_WINCONx(n) (0x0020 + ((n) * 4)) +#define DECON_VIDOSDxH(n) (0x0080 + ((n) * 4)) +#define DECON_SHADOWCON 0x00A0 +#define DECON_VIDOSDxA(n) (0x00B0 + ((n) * 0x20)) +#define DECON_VIDOSDxB(n) (0x00B4 + ((n) * 0x20)) +#define DECON_VIDOSDxC(n) (0x00B8 + ((n) * 0x20)) +#define DECON_VIDOSDxD(n) (0x00BC + ((n) * 0x20)) +#define DECON_VIDOSDxE(n) (0x00C0 + ((n) * 0x20)) +#define DECON_VIDW0xADD0B0(n) (0x0150 + ((n) * 0x10)) +#define DECON_VIDW0xADD0B1(n) (0x0154 + ((n) * 0x10)) +#define DECON_VIDW0xADD0B2(n) (0x0158 + ((n) * 0x10)) +#define DECON_VIDW0xADD1B0(n) (0x01A0 + ((n) * 0x10)) +#define DECON_VIDW0xADD1B1(n) (0x01A4 + ((n) * 0x10)) +#define DECON_VIDW0xADD1B2(n) (0x01A8 + ((n) * 0x10)) +#define DECON_VIDW0xADD2(n) (0x0200 + ((n) * 4)) +#define DECON_LOCALxSIZE(n) (0x0214 + ((n) * 4))change-booting-mode.sh --update +#define DECON_VIDINTCON0 0x0220 +#define DECON_VIDINTCON1 0x0224 +#define DECON_WxKEYCON0(n) (0x0230 + ((n - 1) * 8)) +#define DECON_WxKEYCON1(n) (0x0234 + ((n - 1) * 8)) +#define DECON_WxKEYALPHA(n) (0x0250 + ((n - 1) * 4)) +#define DECON_WINxMAP(n) (0x0270 + ((n) * 4)) +#define DECON_QOSLUT07_00 0x02C0 +#define DECON_QOSLUT15_08 0x02C4 +#define DECON_QOSCTRL 0x02C8 +#define DECON_BLENDERQx(n) (0x0300 + ((n - 1) * 4)) +#define DECON_BLENDCON 0x0310 +#define DECON_OPE_VIDW0xADD0(n) (0x0400 + ((n) * 4)) +#define DECON_OPE_VIDW0xADD1(n) (0x0414 + ((n) * 4)) +#define DECON_FRAMEFIFO_REG7 0x051C +#define DECON_FRAMEFIFO_REG8 0x0520 +#define DECON_FRAMEFIFO_STATUS 0x0524 +#define DECON_CMU 0x1404 +#define DECON_UPDATE 0x1410 +#define DECON_UPDATE_SCHEME 0x1438 +#define DECON_VIDCON1 0x2000 +#define DECON_VIDCON2 0x2004 +#define DECON_VIDCON3 0x2008 +#define DECON_VIDCON4 0x200C +#define DECON_VIDTCON2 0x2028 +#define DECON_FRAME_SIZE 0x2038 +#define DECON_LINECNT_OP_THRESHOLD 0x203C +#define DECON_TRIGCON 0x2040 +#define DECON_TRIGSKIP 0x2050 +#define DECON_CRCRDATA 0x20B0 +#define DECON_CRCCTRL 0x20B4 + +/* Exynos5430 DECON */ +#define DECON_VIDTCON0 0x2020 +#define DECON_VIDTCON1 0x2024
+/* Exynos5433 DECON */ +#define DECON_VIDTCON00 0x2010 +#define DECON_VIDTCON01 0x2014 +#define DECON_VIDTCON10 0x2018 +#define DECON_VIDTCON11 0x201C
+/* Exynos543X DECON Internal */ +#define DECON_W013DSTREOCON 0x0320 +#define DECON_W233DSTREOCON 0x0324 +#define DECON_FRAMEFIFO_REG0 0x0500 +#define DECON_ENHANCER_CTRL 0x2100
+/* Exynos543X DECON TV */ +#define DECON_VCLKCON0 0x0014 +#define DECON_VIDINTCON2 0x0228 +#define DECON_VIDINTCON3 0x022C
Yes, my impression was that DECON supported both internal and external, i.e. would replace both FIMD/Mixer. This patch only supports the internal interface: are you planning to add support for the external/TV/HDMI interface as well? If so, how does this work with the DT format you have defined? Should there maybe separate decon-int and decon-tv subnodes, each of which create one CRTC? The Android tree seems to rely on static configuration, but presumably we would need a much more nuanced approach.
If so, it seems like the first start would be to add a basic DECON binding which covered the entire controller, and then as a next step add support for decon-int as a subdevice of this.
As I know, they are physically different devices even though they have similar names and similar operations. If it is right, I am not sure that congregating them in one DT node is right.
I really appreciate your review. Thanks.
Cheers, Daniel
Best regards, Hyungwon Hwang