Re: [PATCH V2] drm/exynos: Add DECON driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Pankaj,

On Mon, Dec 1, 2014 at 1:36 PM, Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx> wrote:
> Hi Ajay,
>
> On 28 November 2014 at 16:45, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> 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@xxxxxxxxx>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>
>> ---
>> 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;
>> +       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 = &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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux