Re: [PATCH v2 06/10] drm/hisilicon: Add vblank feature

[Xinliang Liu <xinliang.liu@linaro.org>]

On 30 November 2015 at 15:54, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sat, Nov 28, 2015 at 06:39:01PM +0800, Xinliang Liu wrote:
>> Add vblank handle for ADE.
>>
>> Signed-off-by: Xinliang Liu <xinliang.liu@linaro.org>
>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Signed-off-by: Andy Green <andy.green@linaro.org>
>> ---
>>  drivers/gpu/drm/hisilicon/hisi_drm_ade.c | 78 ++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/hisilicon/hisi_drm_ade.h | 16 +++++++
>>  drivers/gpu/drm/hisilicon/hisi_drm_drv.c | 19 +++++++-
>>  3 files changed, 112 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/hisilicon/hisi_drm_ade.h
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hisi_drm_ade.c b/drivers/gpu/drm/hisilicon/hisi_drm_ade.c
>> index b0976c3..acb11e7 100644
>> --- a/drivers/gpu/drm/hisilicon/hisi_drm_ade.c
>> +++ b/drivers/gpu/drm/hisilicon/hisi_drm_ade.c
>> @@ -267,7 +267,79 @@ static void ade_power_down(struct ade_hw_ctx *ctx)
>>       ctx->power_on = false;
>>  }
>>
>> +static struct drm_crtc *hisi_get_crtc_from_index(struct drm_device *dev,
>> +                                              unsigned int index)
>
> Ugly that you had to add this, but unfortunately necessary :( Fixing up
> the drm vblank hooks so that they deal with struct drm_crtc directly is
> somewhere on my todo. But drm_irq.c is still a bit a mess, so this is some
> ways off still.
>
> What might be possible as a follow-up cleanup though is to add
> vblank_enable and vblank_disable functions to struct
> drm_crtc_helper_funcs. And then provide this code here to map from int
> index to struct drm_crtc * as helpers in a new drm_vblank_helper.c file.
> That might be a good intermediate step.

I would like to have a try to make this intermediate step patch and
send out for review soon.

> But nothing that needs to be done before merging hisilicon, that's for
> sure.
>
> One more comment below.
>
>> +{
>> +     unsigned int index_tmp = 0;
>> +     struct drm_crtc *crtc;
>> +
>> +     list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>> +             if (index_tmp == index)
>> +                     return crtc;
>> +
>> +             index_tmp++;
>> +     }
>> +
>> +     WARN_ON(true);
>> +}
>> +
>> +int ade_enable_vblank(struct drm_device *dev, int crtc_index)
>> +{
>> +     struct drm_crtc *crtc = hisi_get_crtc_from_index(dev, crtc_index);
>> +     struct ade_crtc *acrtc = to_ade_crtc(crtc);
>> +     struct ade_hw_ctx *ctx = acrtc->ctx;
>> +     void __iomem *base = ctx->base;
>> +     u32 intr_en;
>> +
>> +     DRM_INFO("enable_vblank enter.\n");
>> +     if (!ctx->power_on)
>> +             (void)ade_power_up(ctx);
>> +
>> +     intr_en = readl(base + LDI_INT_EN);
>> +     intr_en |= LDI_ISR_FRAME_END_INT;
>> +     writel(intr_en, base + LDI_INT_EN);
>>
>> +     return 0;
>> +}
>> +
>> +void ade_disable_vblank(struct drm_device *dev, int crtc_index)
>> +{
>> +     struct drm_crtc *crtc = hisi_get_crtc_from_index(dev, crtc_index);
>> +     struct ade_crtc *acrtc = to_ade_crtc(crtc);
>> +     struct ade_hw_ctx *ctx = acrtc->ctx;
>> +     void __iomem *base = ctx->base;
>> +     u32 intr_en;
>> +
>> +     DRM_INFO("disable_vblank enter.\n");
>> +     if (!ctx->power_on) {
>> +             DRM_ERROR("power is down! vblank disable fail\n");
>> +             return;
>> +     }
>> +     intr_en = readl(base + LDI_INT_EN);
>> +     intr_en &= ~LDI_ISR_FRAME_END_INT;
>> +     writel(intr_en, base + LDI_INT_EN);
>> +}
>> +
>> +static irqreturn_t ade_irq_handler(int irq, void *data)
>> +{
>> +     struct ade_crtc *acrtc = data;
>> +     struct ade_hw_ctx *ctx = acrtc->ctx;
>> +     struct drm_crtc *crtc = &acrtc->base;
>> +     struct drm_device *dev = crtc->dev;
>> +     void __iomem *base = ctx->base;
>> +     u32 status;
>> +
>> +     status = readl(base + LDI_MSK_INT);
>> +     /* DRM_INFO("LDI IRQ: status=0x%X\n",status); */
>> +
>> +     /* vblank irq */
>> +     if (status & LDI_ISR_FRAME_END_INT) {
>> +             writel(LDI_ISR_FRAME_END_INT, base + LDI_INT_CLR);
>> +             drm_handle_vblank(dev, drm_crtc_index(crtc));
>
> drm_crtc_handle_vblank please. At least when calling into the vblank code
> drivers don't have to do the struct drm_crtc -> int index conversion any
> more. Please make sure you do that everywhere, in case I've missed one.

will use drm_crtc_handle_vblank in v3.

Thanks,
-xinliang

>
> Cheers, Daniel
>
>> +     }
>> +
>> +     return IRQ_HANDLED;
>> +}
>>
>>  /*
>>   * set modules' reset mode: by software or hardware
>> @@ -858,6 +930,12 @@ static int ade_bind(struct device *dev, struct device *master, void *data)
>>       if (ret)
>>               return ret;
>>
>> +     /* vblank irq init */
>> +     ret = request_irq(ctx->irq, ade_irq_handler, DRIVER_IRQ_SHARED,
>> +                       drm_dev->driver->name, acrtc);
>> +     if (ret)
>> +             return ret;
>> +
>>       return 0;
>>  }
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hisi_drm_ade.h b/drivers/gpu/drm/hisilicon/hisi_drm_ade.h
>> new file mode 100644
>> index 0000000..d1d7b5d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hisi_drm_ade.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * Copyright (c) 2014-2015 Hisilicon Limited.
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __HISI_DRM_ADE_H__
>> +#define __HISI_DRM_ADE_H__
>> +
>> +int ade_enable_vblank(struct drm_device *dev, int crtc_index);
>> +void ade_disable_vblank(struct drm_device *dev, int crtc_index);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/hisilicon/hisi_drm_drv.c b/drivers/gpu/drm/hisilicon/hisi_drm_drv.c
>> index d0eca80..13f59aa 100644
>> --- a/drivers/gpu/drm/hisilicon/hisi_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hisi_drm_drv.c
>> @@ -21,6 +21,7 @@
>>  #include <drm/drm_fb_cma_helper.h>
>>  #include <drm/drm_atomic_helper.h>
>>
>> +#include "hisi_drm_ade.h"
>>  #include "hisi_drm_drv.h"
>>
>>  #define DRIVER_NAME  "hisi-drm"
>> @@ -29,6 +30,7 @@ static int hisi_drm_unload(struct drm_device *dev)
>>  {
>>       struct hisi_drm_private *priv = dev->dev_private;
>>
>> +     drm_vblank_cleanup(dev);
>>       drm_mode_config_cleanup(dev);
>>       devm_kfree(dev->dev, priv);
>>       dev->dev_private = NULL;
>> @@ -76,11 +78,22 @@ static int hisi_drm_load(struct drm_device *dev, unsigned long flags)
>>               goto err_mode_config_cleanup;
>>       }
>>
>> +     /* vblank init */
>> +     ret = drm_vblank_init(dev, dev->mode_config.num_crtc);
>> +     if (ret) {
>> +             DRM_ERROR("failed to initialize vblank.\n");
>> +             goto err_unbind_all;
>> +     }
>> +     /* with irq_enabled = true, we can use the vblank feature. */
>> +     dev->irq_enabled = true;
>> +
>>       /* reset all the states of crtc/plane/encoder/connector */
>>       drm_mode_config_reset(dev);
>>
>>       return 0;
>>
>> +err_unbind_all:
>> +     component_unbind_all(dev->dev, dev);
>>  err_mode_config_cleanup:
>>       drm_mode_config_cleanup(dev);
>>       devm_kfree(dev->dev, priv);
>> @@ -126,7 +139,7 @@ static int hisi_gem_cma_dumb_create(struct drm_file *file,
>>
>>  static struct drm_driver hisi_drm_driver = {
>>       .driver_features        = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
>> -                               DRIVER_ATOMIC,
>> +                               DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
>>       .load                   = hisi_drm_load,
>>       .unload                 = hisi_drm_unload,
>>       .fops                   = &hisi_drm_fops,
>> @@ -148,6 +161,10 @@ static struct drm_driver hisi_drm_driver = {
>>       .gem_prime_vunmap       = drm_gem_cma_prime_vunmap,
>>       .gem_prime_mmap         = drm_gem_cma_prime_mmap,
>>
>> +     .get_vblank_counter     = drm_vblank_count,
>> +     .enable_vblank          = ade_enable_vblank,
>> +     .disable_vblank         = ade_disable_vblank,
>> +
>>       .name                   = "hisi",
>>       .desc                   = "Hisilicon SoCs' DRM Driver",
>>       .date                   = "20150718",
>> --
>> 1.9.1
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel