[PATCH V2] drm/exynos: Add DECON driver
Hi Pankaj,
On Mon, Dec 1, 2014 at 1:36 PM, Pankaj Dubey
wrote:
> Hi Ajay,
>
> On 28 November 2014 at 16:45, Ajay Kumar wrote:
>> This series is based on exynos-drm-next branch of Inki Dae's tree at:
>> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>>
>> DECON(Display and Enhancement Controller) is the new IP
>> in exynos7 SOC for generating video signals using pixel data.
>>
>> DECON driver can be used to drive 2 different interfaces on Exynos7:
>> DECON-INT(video controller) and DECON-EXT(Mixer for HDMI)
>>
>> The existing FIMD driver code was used as a template to create
>> DECON driver. Only DECON-INT is supported as of now, and
>> DECON-EXT support will be added later.
>>
>> Signed-off-by: Akshu Agrawal
>> Signed-off-by: Ajay Kumar
>> ---
>> Changes since V1:
>> -- Address comments from Pankaj and do few cleanups.
>>
>
> Thanks, but still I can see this needs modification, please see my comments:
>
>> .../devicetree/bindings/video/exynos7-decon.txt| 67 ++
>> drivers/gpu/drm/exynos/Kconfig | 13 +-
>> drivers/gpu/drm/exynos/Makefile|1 +
>> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037
>>
>> drivers/gpu/drm/exynos/exynos_drm_drv.c|4 +
>> drivers/gpu/drm/exynos/exynos_drm_drv.h|1 +
>> include/video/exynos7_decon.h | 346 +++
>> 7 files changed, 1466 insertions(+), 3 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt
>> create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> create mode 100644 include/video/exynos7_decon.h
>>
>
> [snip]
>
>> +static int decon_mgr_initialize(struct exynos_drm_manager *mgr,
>> + struct drm_device *drm_dev)
>> +{
>> + struct decon_context *ctx = mgr_to_decon(mgr);
>> + struct exynos_drm_private *priv = drm_dev->dev_private;
>> + int ret;
>> +
>> + mgr->drm_dev = drm_dev;
>> + mgr->pipe = ctx->pipe = priv->pipe++;
>> +
>
> Do we really need 'pipe' in exynos_drm_manager and decon_context both?
Will fix this.
>> + /* attach this sub driver to iommu mapping if supported. */
>> + if (is_drm_iommu_supported(mgr->drm_dev)) {
>
> [snip]
>
>> +static void decon_win_mode_set(struct exynos_drm_manager *mgr,
>> + struct exynos_drm_overlay *overlay)
>> +{
>> + struct decon_context *ctx = mgr_to_decon(mgr);
>> + struct decon_win_data *win_data;
>> + int win, padding;
>> +
>> + if (!overlay) {
>> + DRM_ERROR("overlay is NULL\n");
>> + return;
>> + }
>> +
>> + win = overlay->zpos;
>> + if (win == DEFAULT_ZPOS)
>> + win = ctx->default_win;
>> +
>> + if (win < 0 || win >= WINDOWS_NR)
>> + return;
>> +
>> +
>> + win_data = >win_data[win];
>> +
>
> As I mentioned in V1, since these 5 lines are getting repeating better
> we move it in one static function. It will help in reducing code
> footprint.
I tried this, the code readability is gone. I think its better to
leave this as it is.
>
> [snip]
>
>> +
>> +/**
>> + * shadow_protect_win() - disable updating values from shadow registers at
>> vsync
>> + *
>> + * @win: window to protect registers for
>> + * @protect: 1 to protect (disable updates)
>> + */
>> +static void decon_shadow_protect_win(struct decon_context *ctx,
>> + int win, bool
>> protect)
>> +{
>> + u32 reg, bits, val;
>> +
>> + reg = SHADOWCON;
>
> How about using SHADOWCON directly instead of using local variable?
Ok.
>> + bits = SHADOWCON_WINx_PROTECT(win);
>> +
>> + val = readl(ctx->regs + reg);
>> + if (protect)
>> + val |= bits;
>> + else
>> + val &= ~bits;
>> + writel(val, ctx->regs + reg);
>> +}
>> +
>> +static void decon_win_commit(struct exynos_drm_manager *mgr, int zpos)
>> +{
>> + struct decon_context *ctx = mgr_to_decon(mgr);
>> + struct decon_win_data *win_data;
>> + int win = zpos;
>> + unsigned long val, alpha, blendeq;
>> + unsigned int last_x;
>> + unsigned int last_y;
>> +
>> + if (ctx->suspended)
>> + return;
>> +
>> + if (win == DEFAULT_ZPOS)
>> + win = ctx->default_win;
>> +
>> + if (win < 0 || win >= WINDOWS_NR)
>> + return;
>> +
>> + win_data = >win_data[win];
>> +
>> + /* If suspended, enable this on resume */
>> + if (ctx->suspended) {
>> + win_data->resume = true;
>> + return;
>> + }
>> +
>> + /*
>
> [snip]
>
>> +static int decon_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = >dev;
>> + struct decon_context *ctx;
>> + struct resource *res;
>> + int ret = -EINVAL;
>> +
>> + if (!dev->of_node)
>> +
[PATCH V2] drm/exynos: Add DECON driver
Hi Ajay,
On 28 November 2014 at 16:45, Ajay Kumar wrote:
> This series is based on exynos-drm-next branch of Inki Dae's tree at:
> git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
>
> DECON(Display and Enhancement Controller) is the new IP
> in exynos7 SOC for generating video signals using pixel data.
>
> DECON driver can be used to drive 2 different interfaces on Exynos7:
> DECON-INT(video controller) and DECON-EXT(Mixer for HDMI)
>
> The existing FIMD driver code was used as a template to create
> DECON driver. Only DECON-INT is supported as of now, and
> DECON-EXT support will be added later.
>
> Signed-off-by: Akshu Agrawal
> Signed-off-by: Ajay Kumar
> ---
> Changes since V1:
> -- Address comments from Pankaj and do few cleanups.
>
Thanks, but still I can see this needs modification, please see my comments:
> .../devicetree/bindings/video/exynos7-decon.txt| 67 ++
> drivers/gpu/drm/exynos/Kconfig | 13 +-
> drivers/gpu/drm/exynos/Makefile|1 +
> drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037
>
> drivers/gpu/drm/exynos/exynos_drm_drv.c|4 +
> drivers/gpu/drm/exynos/exynos_drm_drv.h|1 +
> include/video/exynos7_decon.h | 346 +++
> 7 files changed, 1466 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt
> create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c
> create mode 100644 include/video/exynos7_decon.h
>
[snip]
> +static int decon_mgr_initialize(struct exynos_drm_manager *mgr,
> + struct drm_device *drm_dev)
> +{
> + struct decon_context *ctx = mgr_to_decon(mgr);
> + struct exynos_drm_private *priv = drm_dev->dev_private;
> + int ret;
> +
> + mgr->drm_dev = drm_dev;
> + mgr->pipe = ctx->pipe = priv->pipe++;
> +
Do we really need 'pipe' in exynos_drm_manager and decon_context both?
> + /* attach this sub driver to iommu mapping if supported. */
> + if (is_drm_iommu_supported(mgr->drm_dev)) {
[snip]
> +static void decon_win_mode_set(struct exynos_drm_manager *mgr,
> + struct exynos_drm_overlay *overlay)
> +{
> + struct decon_context *ctx = mgr_to_decon(mgr);
> + struct decon_win_data *win_data;
> + int win, padding;
> +
> + if (!overlay) {
> + DRM_ERROR("overlay is NULL\n");
> + return;
> + }
> +
> + win = overlay->zpos;
> + if (win == DEFAULT_ZPOS)
> + win = ctx->default_win;
> +
> + if (win < 0 || win >= WINDOWS_NR)
> + return;
> +
> +
> + win_data = >win_data[win];
> +
As I mentioned in V1, since these 5 lines are getting repeating better
we move it in one static function. It will help in reducing code
footprint.
[snip]
> +
> +/**
> + * shadow_protect_win() - disable updating values from shadow registers at
> vsync
> + *
> + * @win: window to protect registers for
> + * @protect: 1 to protect (disable updates)
> + */
> +static void decon_shadow_protect_win(struct decon_context *ctx,
> + int win, bool protect)
> +{
> + u32 reg, bits, val;
> +
> + reg = SHADOWCON;
How about using SHADOWCON directly instead of using local variable?
> + bits = SHADOWCON_WINx_PROTECT(win);
> +
> + val = readl(ctx->regs + reg);
> + if (protect)
> + val |= bits;
> + else
> + val &= ~bits;
> + writel(val, ctx->regs + reg);
> +}
> +
> +static void decon_win_commit(struct exynos_drm_manager *mgr, int zpos)
> +{
> + struct decon_context *ctx = mgr_to_decon(mgr);
> + struct decon_win_data *win_data;
> + int win = zpos;
> + unsigned long val, alpha, blendeq;
> + unsigned int last_x;
> + unsigned int last_y;
> +
> + if (ctx->suspended)
> + return;
> +
> + if (win == DEFAULT_ZPOS)
> + win = ctx->default_win;
> +
> + if (win < 0 || win >= WINDOWS_NR)
> + return;
> +
> + win_data = >win_data[win];
> +
> + /* If suspended, enable this on resume */
> + if (ctx->suspended) {
> + win_data->resume = true;
> + return;
> + }
> +
> + /*
[snip]
> +static int decon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct decon_context *ctx;
> + struct resource *res;
> + int ret = -EINVAL;
> +
> + if (!dev->of_node)
> + return -ENODEV;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->manager.type = EXYNOS_DISPLAY_TYPE_LCD;
> + ctx->manager.ops = _manager_ops;
> +
> + ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,
> +
[PATCH V2] drm/exynos: Add DECON driver
This series is based on exynos-drm-next branch of Inki Dae's tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git
DECON(Display and Enhancement Controller) is the new IP
in exynos7 SOC for generating video signals using pixel data.
DECON driver can be used to drive 2 different interfaces on Exynos7:
DECON-INT(video controller) and DECON-EXT(Mixer for HDMI)
The existing FIMD driver code was used as a template to create
DECON driver. Only DECON-INT is supported as of now, and
DECON-EXT support will be added later.
Signed-off-by: Akshu Agrawal
Signed-off-by: Ajay Kumar
---
Changes since V1:
-- Address comments from Pankaj and do few cleanups.
.../devicetree/bindings/video/exynos7-decon.txt| 67 ++
drivers/gpu/drm/exynos/Kconfig | 13 +-
drivers/gpu/drm/exynos/Makefile|1 +
drivers/gpu/drm/exynos/exynos7_drm_decon.c | 1037
drivers/gpu/drm/exynos/exynos_drm_drv.c|4 +
drivers/gpu/drm/exynos/exynos_drm_drv.h|1 +
include/video/exynos7_decon.h | 346 +++
7 files changed, 1466 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/video/exynos7-decon.txt
create mode 100644 drivers/gpu/drm/exynos/exynos7_drm_decon.c
create mode 100644 include/video/exynos7_decon.h
diff --git a/Documentation/devicetree/bindings/video/exynos7-decon.txt
b/Documentation/devicetree/bindings/video/exynos7-decon.txt
new file mode 100644
index 000..14db519
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/exynos7-decon.txt
@@ -0,0 +1,67 @@
+Device-Tree bindings for Samsung Exynos7 SoC display controller (DECON)
+
+DECON (Display and Enhancement Controller) is the Display Controller for the
+Exynos7 series of SoCs which transfers the image data from a video memory
+buffer to an external LCD interface.
+
+Required properties:
+- compatible: value should be "samsung,exynos7-decon";
+
+- reg: physical base address and length of the DECON registers set.
+
+- interrupt-parent: should be the phandle of the decon controller's
+ parent interrupt controller.
+
+- interrupts: should contain a list of all DECON IP block interrupts in the
+order: FIFO Level, VSYNC, LCD_SYSTEM. The interrupt specifier
+format depends on the interrupt controller used.
+
+- interrupt-names: should contain the interrupt names: "fifo", "vsync",
+ "lcd_sys", in the same order as they were listed in the interrupts
+property.
+
+- pinctrl-0: pin control group to be used for this controller.
+
+- pinctrl-names: must contain a "default" entry.
+
+- clocks: must include clock specifiers corresponding to entries in the
+ clock-names property.
+
+- clock-names: list of clock names sorted in the same order as the clocks
+ property. Must contain "pclk_decon0", "aclk_decon0",
+ "decon0_eclk", "decon0_vclk".
+
+Optional Properties:
+- samsung,power-domain: a phandle to DECON power domain node.
+- display-timings: timing settings for FIMD, as described in document [1].
+ Can be used in case timings cannot be provided otherwise
+ or to override timings provided by the panel.
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+
+Example:
+
+SoC specific DT entry:
+
+ decon at 1393 {
+ compatible = "samsung,exynos7-decon";
+ interrupt-parent = <>;
+ reg = <0x1393 0x1000>;
+ interrupt-names = "lcd_sys", "vsync", "fifo";
+ interrupts = <0 188 0>, <0 189 0>, <0 190 0>;
+ clocks = <_disp PCLK_DECON_INT>,
+<_disp ACLK_DECON_INT>,
+<_disp SCLK_DECON_INT_ECLK>,
+<_disp SCLK_DECON_INT_EXTCLKPLL>;
+ clock-names = "pclk_decon0", "aclk_decon0", "decon0_eclk",
+ "decon0_vclk";
+ status = "disabled";
+ };
+
+Board specific DT entry:
+
+ decon at 1393 {
+ pinctrl-0 = <_clk _out>;
+ pinctrl-names = "default";
+ status = "okay";
+ };
diff --git a/drivers/gpu/drm/exynos/Kconfig b/drivers/gpu/drm/exynos/Kconfig
index 7f9f6f9..d3434cb 100644
--- a/drivers/gpu/drm/exynos/Kconfig
+++ b/drivers/gpu/drm/exynos/Kconfig
@@ -32,9 +32,16 @@ config DRM_EXYNOS_FIMD
help
Choose this option if you want to use Exynos FIMD for DRM.
+config DRM_EXYNOS_DECON
+ bool "Exynos DRM DECON"
+ depends on DRM_EXYNOS
+ select FB_MODE_HELPERS
+ help
+ Choose this option if you want to use Exynos DECON for DRM.
+
config DRM_EXYNOS_DPI
bool "EXYNOS DRM parallel output support"
- depends on DRM_EXYNOS_FIMD
+ depends on (DRM_EXYNOS_FIMD || DRM_EXYNOS_DECON)
select DRM_PANEL
default n
help
@@ -42,7
