Hi Pankaj,
On Mon, Dec 1, 2014 at 1:36 PM, Pankaj Dubey pankaj.dubey@samsung.com wrote:
Hi Ajay,
On 28 November 2014 at 16:45, Ajay Kumar ajaykumar.rs@samsung.com 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 akshua@gmail.com Signed-off-by: Ajay Kumar ajaykumar.rs@samsung.com
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 = &ctx->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;elseval &= ~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 = &ctx->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 = &pdev->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 = &decon_manager_ops;ret = exynos_drm_component_add(dev, EXYNOS_DEVICE_TYPE_CRTC,ctx->manager.type);if (ret)return ret;ctx->dev = dev;ctx->suspended = true;ctx->pclk = devm_clk_get(dev, "pclk_decon0");if (IS_ERR(ctx->pclk)) {dev_err(dev, "failed to get bus clock pclk\n");ret = PTR_ERR(ctx->pclk);goto err_del_component;}ctx->aclk = devm_clk_get(dev, "aclk_decon0");if (IS_ERR(ctx->aclk)) {dev_err(dev, "failed to get bus clock aclk\n");ret = PTR_ERR(ctx->aclk);goto err_del_component;}ctx->eclk = devm_clk_get(dev, "decon0_eclk");if (IS_ERR(ctx->eclk)) {dev_err(dev, "failed to get eclock\n");ret = PTR_ERR(ctx->eclk);goto err_del_component;}ctx->vclk = devm_clk_get(dev, "decon0_vclk");if (IS_ERR(ctx->vclk)) {dev_err(dev, "failed to get vclock\n");ret = PTR_ERR(ctx->vclk);goto err_del_component;}ctx->regs = of_iomap(dev->of_node, 0);This is OK, but we should handle unmapping of ctx->regs in error conditions below.
Right, I will fix this. iounmap should also happen in decon_remove( ).
if (IS_ERR(ctx->regs)) {ret = PTR_ERR(ctx->regs);goto err_del_component;}res = platform_get_resource_byname(pdev, IORESOURCE_IRQ, "vsync");if (!res) {dev_err(dev, "irq request failed.\n");ret = -ENXIO;goto err_del_component;}ret = devm_request_irq(dev, res->start, decon_irq_handler,0, "drm_decon", ctx);if (ret) {dev_err(dev, "irq request failed.\n");goto err_del_component;}init_waitqueue_head(&ctx->wait_vsync_queue);atomic_set(&ctx->wait_vsync_event, 0);platform_set_drvdata(pdev, ctx);ctx->display = exynos_dpi_probe(dev);if (IS_ERR(ctx->display)) {ret = PTR_ERR(ctx->display);goto err_del_component;}pm_runtime_enable(dev);ret = component_add(dev, &decon_component_ops);if (ret)goto err_disable_pm_runtime;return ret;+err_disable_pm_runtime:
pm_runtime_disable(dev);+err_del_component:
exynos_drm_component_del(dev, EXYNOS_DEVICE_TYPE_CRTC);return ret;+}
+static int decon_remove(struct platform_device *pdev) +{
pm_runtime_disable(&pdev->dev);component_del(&pdev->dev, &decon_component_ops);exynos_drm_component_del(&pdev->dev, EXYNOS_DEVICE_TYPE_CRTC);return 0;+}
+struct platform_driver decon_driver = {
.probe = decon_probe,.remove = decon_remove,.driver = {.name = "exynos-decon",.owner = THIS_MODULE,This is no more required.
Ok.
Ajay