[v10,5/5] drm/rockchip: Add dmc notifier in vop driver
diff mbox

Message ID 1473051971-5500-6-git-send-email-hl@rock-chips.com
State New
Headers show

Commit Message

huang lin Sept. 5, 2016, 5:06 a.m. UTC
when in ddr frequency scaling process, vop can not do enable or
disable operation, since in dcf we check vop clock to see whether
vop work. If vop work, dcf do ddr frequency scaling when vop
in vblank status, and we need to read vop register to check whether
vop go into vblank status. If vop not work, dcf can do ddr frequency
any time. So when do ddr frequency scaling, you disabled or enable
vop, there may two bad thing happen: 1, the panel flicker(when vop from
disable status change to enable). 2, kernel hang (when vop from enable
status change to disable, dcf need to read vblank status, but if you disable
vop clock, it can not get the status, it will lead soc dead) So we need
register to devfreq notifier, and we can get the dmc status. Also, when
there have two vop enabled, we need to disable dmc, since dcf only base
on one vop vblank time, so the other panel will flicker when do ddr
frequency scaling.

Signed-off-by: Lin Huang <hl@rock-chips.com>
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
---
Changes in v10:
- None

Changes in v9:
- None

Changes in v8:
- None

Changes in v7:
- None

Changes in v6:
- fix a build error

Changes in v5:
- improve some nits

Changes in v4:
- register notifier to devfreq_register_notifier
- use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
- when two vop enable, disable dmc
- when two vop back to one vop, enable dmc

Changes in v3:
- when do vop eanble/disable, dmc will wait until it finish

Changes in v2:
- None

Changes in v1:
- use wait_event instead usleep

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

Comments

kbuild test robot Sept. 6, 2016, 3:40 a.m. UTC | #1
Hi Lin,

[auto build test ERROR on next-20160825]
[cannot apply to rockchip/for-next devfreq/for-rafael linus/master v4.8-rc5 v4.8-rc4 v4.8-rc3 v4.8-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Lin-Huang/rk3399-support-ddr-frequency-scaling/20160905-131110
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1124:16: note: (near initialization for 'vop_crtc_helper_funcs.mode_fixup')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1125:18: error: initializer element is not constant
     .atomic_flush = vop_crtc_atomic_flush,
                     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1125:18: note: (near initialization for 'vop_crtc_helper_funcs.atomic_flush')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1126:18: error: initializer element is not constant
     .atomic_begin = vop_crtc_atomic_begin,
                     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1126:18: note: (near initialization for 'vop_crtc_helper_funcs.atomic_begin')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1129:13: error: invalid storage class for function 'vop_crtc_destroy'
    static void vop_crtc_destroy(struct drm_crtc *crtc)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1134:13: error: invalid storage class for function 'vop_crtc_reset'
    static void vop_crtc_reset(struct drm_crtc *crtc)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1145:31: error: invalid storage class for function 'vop_crtc_duplicate_state'
    static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
                                  ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1157:13: error: invalid storage class for function 'vop_crtc_destroy_state'
    static void vop_crtc_destroy_state(struct drm_crtc *crtc,
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1169:13: error: initializer element is not constant
     .destroy = vop_crtc_destroy,
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1169:13: note: (near initialization for 'vop_crtc_funcs.destroy')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1170:11: error: initializer element is not constant
     .reset = vop_crtc_reset,
              ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1170:11: note: (near initialization for 'vop_crtc_funcs.reset')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1171:28: error: initializer element is not constant
     .atomic_duplicate_state = vop_crtc_duplicate_state,
                               ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1171:28: note: (near initialization for 'vop_crtc_funcs.atomic_duplicate_state')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1172:26: error: initializer element is not constant
     .atomic_destroy_state = vop_crtc_destroy_state,
                             ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1172:26: note: (near initialization for 'vop_crtc_funcs.atomic_destroy_state')
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1175:13: error: invalid storage class for function 'vop_win_pending_is_complete'
    static bool vop_win_pending_is_complete(struct vop_win *vop_win)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1187:13: error: invalid storage class for function 'vop_handle_vblank'
    static void vop_handle_vblank(struct vop *vop)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1213:20: error: invalid storage class for function 'vop_isr'
    static irqreturn_t vop_isr(int irq, void *data)
                       ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1259:12: error: invalid storage class for function 'vop_create_crtc'
    static int vop_create_crtc(struct vop *vop)
               ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1359:13: error: invalid storage class for function 'vop_destroy_crtc'
    static void vop_destroy_crtc(struct vop *vop)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1387:12: error: invalid storage class for function 'vop_initial'
    static int vop_initial(struct vop *vop)
               ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1487:13: error: invalid storage class for function 'vop_win_init'
    static void vop_win_init(struct vop *vop)
                ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1501:12: error: invalid storage class for function 'vop_bind'
    static int vop_bind(struct device *dev, struct device *master, void *data)
               ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1596:13: error: invalid storage class for function 'vop_unbind'
    static void vop_unbind(struct device *dev, struct device *master, void *data)
                ^
   In file included from include/linux/linkage.h:6:0,
                    from include/linux/kernel.h:6,
                    from include/linux/list.h:8,
                    from include/linux/agp_backend.h:33,
                    from include/drm/drmP.h:35,
                    from drivers/gpu/drm/rockchip/rockchip_drm_vop.c:16:
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:19: error: extern declaration of 'vop_component_ops' follows declaration with no linkage
    EXPORT_SYMBOL_GPL(vop_component_ops);
                      ^
   include/linux/export.h:57:21: note: in definition of macro '___EXPORT_SYMBOL'
     extern typeof(sym) sym;     \
                        ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1604:28: note: previous definition of 'vop_component_ops' was here
    const struct component_ops vop_component_ops = {
                               ^
   In file included from include/linux/linkage.h:6:0,
                    from include/linux/kernel.h:6,
                    from include/linux/list.h:8,
                    from include/linux/agp_backend.h:33,
                    from include/drm/drmP.h:35,
                    from drivers/gpu/drm/rockchip/rockchip_drm_vop.c:16:
   include/linux/export.h:63:25: warning: '__used__' attribute ignored [-Wattributes]
     __visible const struct kernel_symbol __ksymtab_##sym \
                            ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
>> include/linux/export.h:63:39: error: section attribute cannot be specified for local variables
     __visible const struct kernel_symbol __ksymtab_##sym \
                                          ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
   include/linux/export.h:63:25: warning: 'externally_visible' attribute have effect only on public objects [-Wattributes]
     __visible const struct kernel_symbol __ksymtab_##sym \
                            ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
   include/linux/export.h:63:39: error: declaration of '__ksymtab_vop_component_ops' with no linkage follows extern declaration
     __visible const struct kernel_symbol __ksymtab_##sym \
                                          ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
   include/linux/export.h:62:36: note: previous declaration of '__ksymtab_vop_component_ops' was here
     extern const struct kernel_symbol __ksymtab_##sym; \
                                       ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
>> include/linux/export.h:63:25: error: expected declaration or statement at end of input
     __visible const struct kernel_symbol __ksymtab_##sym \
                            ^
   include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^
   include/linux/export.h:101:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "_gpl")
     ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:1608:1: note: in expansion of macro 'EXPORT_SYMBOL_GPL'
    EXPORT_SYMBOL_GPL(vop_component_ops);
    ^
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c: At top level:
   drivers/gpu/drm/rockchip/rockchip_drm_vop.c:557:13: warning: 'vop_crtc_disable' defined but not used [-Wunused-function]
    static void vop_crtc_disable(struct drm_crtc *crtc)
                ^

vim +63 include/linux/export.h

f5016932 Paul Gortmaker  2011-05-23  57  	extern typeof(sym) sym;					\
f5016932 Paul Gortmaker  2011-05-23  58  	__CRC_SYMBOL(sym, sec)					\
f5016932 Paul Gortmaker  2011-05-23  59  	static const char __kstrtab_##sym[]			\
f5016932 Paul Gortmaker  2011-05-23  60  	__attribute__((section("__ksymtab_strings"), aligned(1))) \
b92021b0 Rusty Russell   2013-03-15  61  	= VMLINUX_SYMBOL_STR(sym);				\
7b4ec8dd Johannes Berg   2014-01-16  62  	extern const struct kernel_symbol __ksymtab_##sym;	\
e0f244c6 Andi Kleen      2013-10-23 @63  	__visible const struct kernel_symbol __ksymtab_##sym	\
f5016932 Paul Gortmaker  2011-05-23  64  	__used							\
f5016932 Paul Gortmaker  2011-05-23  65  	__attribute__((section("___ksymtab" sec "+" #sym), unused))	\
f5016932 Paul Gortmaker  2011-05-23  66  	= { (unsigned long)&sym, __kstrtab_##sym }
f5016932 Paul Gortmaker  2011-05-23  67  
c1a95fda Nicolas Pitre   2016-01-22  68  #if defined(__KSYM_DEPS__)
c1a95fda Nicolas Pitre   2016-01-22  69  
c1a95fda Nicolas Pitre   2016-01-22  70  /*
c1a95fda Nicolas Pitre   2016-01-22  71   * For fine grained build dependencies, we want to tell the build system
c1a95fda Nicolas Pitre   2016-01-22  72   * about each possible exported symbol even if they're not actually exported.
c1a95fda Nicolas Pitre   2016-01-22  73   * We use a string pattern that is unlikely to be valid code that the build
c1a95fda Nicolas Pitre   2016-01-22  74   * system filters out from the preprocessor output (see ksym_dep_filter
c1a95fda Nicolas Pitre   2016-01-22  75   * in scripts/Kbuild.include).
c1a95fda Nicolas Pitre   2016-01-22  76   */
c1a95fda Nicolas Pitre   2016-01-22  77  #define __EXPORT_SYMBOL(sym, sec)	=== __KSYM_##sym ===
c1a95fda Nicolas Pitre   2016-01-22  78  
c1a95fda Nicolas Pitre   2016-01-22  79  #elif defined(CONFIG_TRIM_UNUSED_KSYMS)
f2355416 Nicolas Pitre   2016-01-22  80  
f2355416 Nicolas Pitre   2016-01-22  81  #include <linux/kconfig.h>
f2355416 Nicolas Pitre   2016-01-22  82  #include <generated/autoksyms.h>
f2355416 Nicolas Pitre   2016-01-22  83  
f2355416 Nicolas Pitre   2016-01-22  84  #define __EXPORT_SYMBOL(sym, sec)				\
6023d236 Masahiro Yamada 2016-06-14  85  	__cond_export_sym(sym, sec, __is_defined(__KSYM_##sym))
f2355416 Nicolas Pitre   2016-01-22  86  #define __cond_export_sym(sym, sec, conf)			\
f2355416 Nicolas Pitre   2016-01-22  87  	___cond_export_sym(sym, sec, conf)
f2355416 Nicolas Pitre   2016-01-22  88  #define ___cond_export_sym(sym, sec, enabled)			\
f2355416 Nicolas Pitre   2016-01-22  89  	__cond_export_sym_##enabled(sym, sec)
f2355416 Nicolas Pitre   2016-01-22  90  #define __cond_export_sym_1(sym, sec) ___EXPORT_SYMBOL(sym, sec)
f2355416 Nicolas Pitre   2016-01-22  91  #define __cond_export_sym_0(sym, sec) /* nothing */
f2355416 Nicolas Pitre   2016-01-22  92  
f2355416 Nicolas Pitre   2016-01-22  93  #else
f2355416 Nicolas Pitre   2016-01-22 @94  #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
f2355416 Nicolas Pitre   2016-01-22  95  #endif
f2355416 Nicolas Pitre   2016-01-22  96  
f5016932 Paul Gortmaker  2011-05-23  97  #define EXPORT_SYMBOL(sym)					\

:::::: The code at line 63 was first introduced by commit
:::::: e0f244c63fc9d192dfd399cc2677bbdca61994b1 asmlinkage, module: Make ksymtab and kcrctab symbols and __this_module __visible

:::::: TO: Andi Kleen <ak@linux.intel.com>
:::::: CC: Rusty Russell <rusty@rustcorp.com.au>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Sean Paul Sept. 6, 2016, 5:18 p.m. UTC | #2
On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
> when in ddr frequency scaling process, vop can not do enable or
> disable operation, since in dcf we check vop clock to see whether
> vop work. If vop work, dcf do ddr frequency scaling when vop
> in vblank status, and we need to read vop register to check whether
> vop go into vblank status. If vop not work, dcf can do ddr frequency
> any time. So when do ddr frequency scaling, you disabled or enable
> vop, there may two bad thing happen: 1, the panel flicker(when vop from
> disable status change to enable). 2, kernel hang (when vop from enable
> status change to disable, dcf need to read vblank status, but if you disable
> vop clock, it can not get the status, it will lead soc dead) So we need
> register to devfreq notifier, and we can get the dmc status. Also, when
> there have two vop enabled, we need to disable dmc, since dcf only base
> on one vop vblank time, so the other panel will flicker when do ddr
> frequency scaling.
>
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
> ---
> Changes in v10:
> - None
>
> Changes in v9:
> - None
>
> Changes in v8:
> - None
>
> Changes in v7:
> - None
>
> Changes in v6:
> - fix a build error
>
> Changes in v5:
> - improve some nits
>
> Changes in v4:
> - register notifier to devfreq_register_notifier
> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
> - when two vop enable, disable dmc
> - when two vop back to one vop, enable dmc
>
> Changes in v3:
> - when do vop eanble/disable, dmc will wait until it finish
>
> Changes in v2:
> - None
>
> Changes in v1:
> - use wait_event instead usleep
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index efbc41a..a73f3aa 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -19,6 +19,8 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_plane_helper.h>
>
> +#include <linux/devfreq.h>
> +#include <linux/devfreq-event.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -118,6 +120,13 @@ struct vop {
>
>         const struct vop_data *data;
>
> +       struct devfreq *devfreq;
> +       struct devfreq_event_dev *devfreq_event_dev;
> +       struct notifier_block dmc_nb;
> +       int dmc_in_process;
> +       int vop_switch_status;
> +       wait_queue_head_t wait_dmc_queue;
> +       wait_queue_head_t wait_vop_switch_queue;
>         uint32_t *regsbak;
>         void __iomem *regs;
>
> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>  }
>
> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
> +                     void *data)
> +{
> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
> +
> +       if (event == DEVFREQ_PRECHANGE) {
> +               /*
> +                * check if vop in enable or disable process,
> +                * if yes, wait until it finishes, use 200ms as
> +                * timeout.
> +                */
> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
> +                                       !vop->vop_switch_status, HZ / 5))
> +                       dev_warn(vop->dev,
> +                                "Timeout waiting for vop swtich status\n");
> +               vop->dmc_in_process = 1;
> +       } else if (event == DEVFREQ_POSTCHANGE) {
> +               vop->dmc_in_process = 0;
> +               wake_up(&vop->wait_dmc_queue);
> +       }
> +
> +       return NOTIFY_OK;
> +}
> +
>  static int vop_enable(struct drm_crtc *crtc)
>  {
>         struct vop *vop = to_vop(crtc);
> +       int num_enabled_crtc = 0;
>         int ret;
>
> +       /*
> +        * if in dmc scaling frequency process, wait until it finishes
> +        * use 200ms as timeout time.
> +        */
> +       if (!wait_event_timeout(vop->wait_dmc_queue,
> +                               !vop->dmc_in_process, HZ / 5))
> +               dev_warn(vop->dev,
> +                        "Timeout waiting for dmc when vop enable\n");
> +


This wait_event_timeout code is terribly racey (same goes above and below).


> +       vop->vop_switch_status = 1;
> +
>         ret = pm_runtime_get_sync(vop->dev);
>         if (ret < 0) {
>                 dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>
>         drm_crtc_vblank_on(crtc);
>
> +       vop->vop_switch_status = 0;
> +       wake_up(&vop->wait_vop_switch_queue);
> +
> +       /* check how many VOPs in use now */
> +       drm_for_each_crtc(crtc, vop->drm_dev) {
> +               if (crtc->state->enable)

I think you really want to check active, instead of enable.

> +                       num_enabled_crtc++;
> +       }
> +
> +       /* if enable two vop, need to disable dmc */
> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
> +               if (vop->devfreq_event_dev)
> +                       devfreq_event_disable_edev(vop->devfreq_event_dev);
> +               devfreq_suspend_device(vop->devfreq);
> +       }

This really feels like something that should be handled somewhere
else. I don't fully understand how this works, but it seems like this
dependency should be handled where it actually matters, rather than
building in a seemingly arbitrary restriction in the vop driver.

Even if this is the best place for it, this needs to be refactored to
eliminate the races that exist now.

Sean

>         return 0;
>
>  err_disable_aclk:
> @@ -489,17 +549,31 @@ err_disable_hclk:
>         clk_disable(vop->hclk);
>  err_put_pm_runtime:
>         pm_runtime_put_sync(vop->dev);
> +       vop->vop_switch_status = 0;
> +       wake_up(&vop->wait_vop_switch_queue);
>         return ret;
>  }
>
>  static void vop_crtc_disable(struct drm_crtc *crtc)
>  {
>         struct vop *vop = to_vop(crtc);
> +       int num_enabled_crtc = 0;
>         int i;
>
>         WARN_ON(vop->event);
>
>         /*
> +        * if in dmc scaling frequency process, wait until it finish
> +        * use 200ms as timeout time.
> +        */
> +       if (!wait_event_timeout(vop->wait_dmc_queue,
> +                               !vop->dmc_in_process, HZ / 5))
> +               dev_warn(vop->dev,
> +                        "Timeout waiting for dmc when vop disable\n");
> +
> +       vop->vop_switch_status = 1;
> +
> +       /*
>          * We need to make sure that all windows are disabled before we
>          * disable that crtc. Otherwise we might try to scan from a destroyed
>          * buffer later.
> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>                 spin_unlock_irq(&crtc->dev->event_lock);
>
>                 crtc->state->event = NULL;
> +
> +       vop->vop_switch_status = 0;
> +       wake_up(&vop->wait_vop_switch_queue);
> +
> +       /* check how many VOPs in use now */
> +       drm_for_each_crtc(crtc, vop->drm_dev) {
> +               if (crtc->state->enable)
> +                       num_enabled_crtc++;
> +       }
> +
> +       /*
> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
> +        * change to 1 vop enabled  need to enable dmc again.
> +        */
> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
> +               if (vop->devfreq_event_dev)
> +                       devfreq_event_enable_edev(vop->devfreq_event_dev);
> +               devfreq_resume_device(vop->devfreq);
>         }
>  }
>
> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>         struct drm_device *drm_dev = data;
>         struct vop *vop;
>         struct resource *res;
> +       struct devfreq *devfreq;
> +       struct devfreq_event_dev *event_dev;
>         size_t alloc_size;
>         int ret, irq;
>
> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>                 return ret;
>
>         pm_runtime_enable(&pdev->dev);
> +
> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
> +       vop->vop_switch_status = 0;
> +       init_waitqueue_head(&vop->wait_dmc_queue);
> +       vop->dmc_in_process = 0;
> +
> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
> +       if (IS_ERR(devfreq))
> +               goto out;
> +
> +       vop->devfreq = devfreq;
> +       vop->dmc_nb.notifier_call = dmc_notify;
> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
> +                                 DEVFREQ_TRANSITION_NOTIFIER);
> +
> +       event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
> +                                                     0);
> +       if (IS_ERR(event_dev))
> +               goto out;
> +
> +       vop->devfreq_event_dev = event_dev;
> +out:
>         return 0;
>  }
>
> --
> 2.6.6
>
huang lin Sept. 6, 2016, 6:15 p.m. UTC | #3
Hi Sean,

On 2016年09月07日 01:18, Sean Paul wrote:
> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>> when in ddr frequency scaling process, vop can not do enable or
>> disable operation, since in dcf we check vop clock to see whether
>> vop work. If vop work, dcf do ddr frequency scaling when vop
>> in vblank status, and we need to read vop register to check whether
>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>> any time. So when do ddr frequency scaling, you disabled or enable
>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>> disable status change to enable). 2, kernel hang (when vop from enable
>> status change to disable, dcf need to read vblank status, but if you disable
>> vop clock, it can not get the status, it will lead soc dead) So we need
>> register to devfreq notifier, and we can get the dmc status. Also, when
>> there have two vop enabled, we need to disable dmc, since dcf only base
>> on one vop vblank time, so the other panel will flicker when do ddr
>> frequency scaling.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> Changes in v10:
>> - None
>>
>> Changes in v9:
>> - None
>>
>> Changes in v8:
>> - None
>>
>> Changes in v7:
>> - None
>>
>> Changes in v6:
>> - fix a build error
>>
>> Changes in v5:
>> - improve some nits
>>
>> Changes in v4:
>> - register notifier to devfreq_register_notifier
>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>> - when two vop enable, disable dmc
>> - when two vop back to one vop, enable dmc
>>
>> Changes in v3:
>> - when do vop eanble/disable, dmc will wait until it finish
>>
>> Changes in v2:
>> - None
>>
>> Changes in v1:
>> - use wait_event instead usleep
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
>>   1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index efbc41a..a73f3aa 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,8 @@
>>   #include <drm/drm_crtc_helper.h>
>>   #include <drm/drm_plane_helper.h>
>>
>> +#include <linux/devfreq.h>
>> +#include <linux/devfreq-event.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> @@ -118,6 +120,13 @@ struct vop {
>>
>>          const struct vop_data *data;
>>
>> +       struct devfreq *devfreq;
>> +       struct devfreq_event_dev *devfreq_event_dev;
>> +       struct notifier_block dmc_nb;
>> +       int dmc_in_process;
>> +       int vop_switch_status;
>> +       wait_queue_head_t wait_dmc_queue;
>> +       wait_queue_head_t wait_vop_switch_queue;
>>          uint32_t *regsbak;
>>          void __iomem *regs;
>>
>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>>   }
>>
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> +                     void *data)
>> +{
>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>> +
>> +       if (event == DEVFREQ_PRECHANGE) {
>> +               /*
>> +                * check if vop in enable or disable process,
>> +                * if yes, wait until it finishes, use 200ms as
>> +                * timeout.
>> +                */
>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>> +                                       !vop->vop_switch_status, HZ / 5))
>> +                       dev_warn(vop->dev,
>> +                                "Timeout waiting for vop swtich status\n");
>> +               vop->dmc_in_process = 1;
>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>> +               vop->dmc_in_process = 0;
>> +               wake_up(&vop->wait_dmc_queue);
>> +       }
>> +
>> +       return NOTIFY_OK;
>> +}
>> +
>>   static int vop_enable(struct drm_crtc *crtc)
>>   {
>>          struct vop *vop = to_vop(crtc);
>> +       int num_enabled_crtc = 0;
>>          int ret;
>>
>> +       /*
>> +        * if in dmc scaling frequency process, wait until it finishes
>> +        * use 200ms as timeout time.
>> +        */
>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>> +                               !vop->dmc_in_process, HZ / 5))
>> +               dev_warn(vop->dev,
>> +                        "Timeout waiting for dmc when vop enable\n");
>> +
>
> This wait_event_timeout code is terribly racey (same goes above and below).
   No i use the vop_switch_status and dmc_in_progress to handle the vop 
and dmc racey,
    I assume this can fix the racey now. I do not better a idea now.
>
>
>> +       vop->vop_switch_status = 1;
>> +
>>          ret = pm_runtime_get_sync(vop->dev);
>>          if (ret < 0) {
>>                  dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>
>>          drm_crtc_vblank_on(crtc);
>>
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>> +
>> +       /* check how many VOPs in use now */
>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>> +               if (crtc->state->enable)
> I think you really want to check active, instead of enable.
Okay, i will check it, thanks.
>
>> +                       num_enabled_crtc++;
>> +       }
>> +
>> +       /* if enable two vop, need to disable dmc */
>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>> +               if (vop->devfreq_event_dev)
>> +                       devfreq_event_disable_edev(vop->devfreq_event_dev);
>> +               devfreq_suspend_device(vop->devfreq);
>> +       }
> This really feels like something that should be handled somewhere
> else. I don't fully understand how this works, but it seems like this
> dependency should be handled where it actually matters, rather than
> building in a seemingly arbitrary restriction in the vop driver.
>
> Even if this is the best place for it, this needs to be refactored to
> eliminate the races that exist now.
>
> Sean
>
>>          return 0;
>>
>>   err_disable_aclk:
>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>          clk_disable(vop->hclk);
>>   err_put_pm_runtime:
>>          pm_runtime_put_sync(vop->dev);
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>>          return ret;
>>   }
>>
>>   static void vop_crtc_disable(struct drm_crtc *crtc)
>>   {
>>          struct vop *vop = to_vop(crtc);
>> +       int num_enabled_crtc = 0;
>>          int i;
>>
>>          WARN_ON(vop->event);
>>
>>          /*
>> +        * if in dmc scaling frequency process, wait until it finish
>> +        * use 200ms as timeout time.
>> +        */
>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>> +                               !vop->dmc_in_process, HZ / 5))
>> +               dev_warn(vop->dev,
>> +                        "Timeout waiting for dmc when vop disable\n");
>> +
>> +       vop->vop_switch_status = 1;
>> +
>> +       /*
>>           * We need to make sure that all windows are disabled before we
>>           * disable that crtc. Otherwise we might try to scan from a destroyed
>>           * buffer later.
>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>                  spin_unlock_irq(&crtc->dev->event_lock);
>>
>>                  crtc->state->event = NULL;
>> +
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>> +
>> +       /* check how many VOPs in use now */
>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>> +               if (crtc->state->enable)
>> +                       num_enabled_crtc++;
>> +       }
>> +
>> +       /*
>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>> +        * change to 1 vop enabled  need to enable dmc again.
>> +        */
>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>> +               if (vop->devfreq_event_dev)
>> +                       devfreq_event_enable_edev(vop->devfreq_event_dev);
>> +               devfreq_resume_device(vop->devfreq);
>>          }
>>   }
>>
>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>          struct drm_device *drm_dev = data;
>>          struct vop *vop;
>>          struct resource *res;
>> +       struct devfreq *devfreq;
>> +       struct devfreq_event_dev *event_dev;
>>          size_t alloc_size;
>>          int ret, irq;
>>
>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>                  return ret;
>>
>>          pm_runtime_enable(&pdev->dev);
>> +
>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>> +       vop->vop_switch_status = 0;
>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>> +       vop->dmc_in_process = 0;
>> +
>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>> +       if (IS_ERR(devfreq))
>> +               goto out;
>> +
>> +       vop->devfreq = devfreq;
>> +       vop->dmc_nb.notifier_call = dmc_notify;
>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>> +
>> +       event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>> +                                                     0);
>> +       if (IS_ERR(event_dev))
>> +               goto out;
>> +
>> +       vop->devfreq_event_dev = event_dev;
>> +out:
>>          return 0;
>>   }
>>
>> --
>> 2.6.6
>>
>
>
Sean Paul Sept. 6, 2016, 6:55 p.m. UTC | #4
On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@rock-chips.com> wrote:
> Hi Sean,
>
>
> On 2016年09月07日 01:18, Sean Paul wrote:
>>
>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>
>>> when in ddr frequency scaling process, vop can not do enable or
>>> disable operation, since in dcf we check vop clock to see whether
>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>> in vblank status, and we need to read vop register to check whether
>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>> any time. So when do ddr frequency scaling, you disabled or enable
>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>> disable status change to enable). 2, kernel hang (when vop from enable
>>> status change to disable, dcf need to read vblank status, but if you
>>> disable
>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>> on one vop vblank time, so the other panel will flicker when do ddr
>>> frequency scaling.
>>>
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>> Changes in v10:
>>> - None
>>>
>>> Changes in v9:
>>> - None
>>>
>>> Changes in v8:
>>> - None
>>>
>>> Changes in v7:
>>> - None
>>>
>>> Changes in v6:
>>> - fix a build error
>>>
>>> Changes in v5:
>>> - improve some nits
>>>
>>> Changes in v4:
>>> - register notifier to devfreq_register_notifier
>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>> - when two vop enable, disable dmc
>>> - when two vop back to one vop, enable dmc
>>>
>>> Changes in v3:
>>> - when do vop eanble/disable, dmc will wait until it finish
>>>
>>> Changes in v2:
>>> - None
>>>
>>> Changes in v1:
>>> - use wait_event instead usleep
>>>
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>> ++++++++++++++++++++++++++++
>>>   1 file changed, 116 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index efbc41a..a73f3aa 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -19,6 +19,8 @@
>>>   #include <drm/drm_crtc_helper.h>
>>>   #include <drm/drm_plane_helper.h>
>>>
>>> +#include <linux/devfreq.h>
>>> +#include <linux/devfreq-event.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   #include <linux/platform_device.h>
>>> @@ -118,6 +120,13 @@ struct vop {
>>>
>>>          const struct vop_data *data;
>>>
>>> +       struct devfreq *devfreq;
>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>> +       struct notifier_block dmc_nb;
>>> +       int dmc_in_process;
>>> +       int vop_switch_status;
>>> +       wait_queue_head_t wait_dmc_queue;
>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>          uint32_t *regsbak;
>>>          void __iomem *regs;
>>>
>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>> vop *vop)
>>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>   }
>>>
>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>> +                     void *data)
>>> +{
>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>> +
>>> +       if (event == DEVFREQ_PRECHANGE) {
>>> +               /*
>>> +                * check if vop in enable or disable process,
>>> +                * if yes, wait until it finishes, use 200ms as
>>> +                * timeout.
>>> +                */
>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>> +                                       !vop->vop_switch_status, HZ / 5))
>>> +                       dev_warn(vop->dev,
>>> +                                "Timeout waiting for vop swtich
>>> status\n");
>>> +               vop->dmc_in_process = 1;
>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>> +               vop->dmc_in_process = 0;
>>> +               wake_up(&vop->wait_dmc_queue);
>>> +       }
>>> +
>>> +       return NOTIFY_OK;
>>> +}
>>> +
>>>   static int vop_enable(struct drm_crtc *crtc)
>>>   {
>>>          struct vop *vop = to_vop(crtc);
>>> +       int num_enabled_crtc = 0;
>>>          int ret;
>>>
>>> +       /*
>>> +        * if in dmc scaling frequency process, wait until it finishes
>>> +        * use 200ms as timeout time.
>>> +        */
>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>> +                               !vop->dmc_in_process, HZ / 5))
>>> +               dev_warn(vop->dev,
>>> +                        "Timeout waiting for dmc when vop enable\n");
>>> +
>>
>>
>> This wait_event_timeout code is terribly racey (same goes above and
>> below).
>
>   No i use the vop_switch_status and dmc_in_progress to handle the vop and
> dmc racey,


So what happens if dmc_in_progress becomes 1 right here, or anywhere
during vop_enable()?

Sean




>    I assume this can fix the racey now. I do not better a idea now.
>>
>>
>>
>>> +       vop->vop_switch_status = 1;
>>> +
>>>          ret = pm_runtime_get_sync(vop->dev);
>>>          if (ret < 0) {
>>>                  dev_err(vop->dev, "failed to get pm runtime: %d\n",
>>> ret);
>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>
>>>          drm_crtc_vblank_on(crtc);
>>>
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>> +
>>> +       /* check how many VOPs in use now */
>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>> +               if (crtc->state->enable)
>>
>> I think you really want to check active, instead of enable.
>
> Okay, i will check it, thanks.
>
>>
>>> +                       num_enabled_crtc++;
>>> +       }
>>> +
>>> +       /* if enable two vop, need to disable dmc */
>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>> +               if (vop->devfreq_event_dev)
>>> +
>>> devfreq_event_disable_edev(vop->devfreq_event_dev);
>>> +               devfreq_suspend_device(vop->devfreq);
>>> +       }
>>
>> This really feels like something that should be handled somewhere
>> else. I don't fully understand how this works, but it seems like this
>> dependency should be handled where it actually matters, rather than
>> building in a seemingly arbitrary restriction in the vop driver.
>>
>> Even if this is the best place for it, this needs to be refactored to
>> eliminate the races that exist now.
>>
>> Sean
>>
>>>          return 0;
>>>
>>>   err_disable_aclk:
>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>          clk_disable(vop->hclk);
>>>   err_put_pm_runtime:
>>>          pm_runtime_put_sync(vop->dev);
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>          return ret;
>>>   }
>>>
>>>   static void vop_crtc_disable(struct drm_crtc *crtc)
>>>   {
>>>          struct vop *vop = to_vop(crtc);
>>> +       int num_enabled_crtc = 0;
>>>          int i;
>>>
>>>          WARN_ON(vop->event);
>>>
>>>          /*
>>> +        * if in dmc scaling frequency process, wait until it finish
>>> +        * use 200ms as timeout time.
>>> +        */
>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>> +                               !vop->dmc_in_process, HZ / 5))
>>> +               dev_warn(vop->dev,
>>> +                        "Timeout waiting for dmc when vop disable\n");
>>> +
>>> +       vop->vop_switch_status = 1;
>>> +
>>> +       /*
>>>           * We need to make sure that all windows are disabled before we
>>>           * disable that crtc. Otherwise we might try to scan from a
>>> destroyed
>>>           * buffer later.
>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>>                  spin_unlock_irq(&crtc->dev->event_lock);
>>>
>>>                  crtc->state->event = NULL;
>>> +
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>> +
>>> +       /* check how many VOPs in use now */
>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>> +               if (crtc->state->enable)
>>> +                       num_enabled_crtc++;
>>> +       }
>>> +
>>> +       /*
>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>> +        * change to 1 vop enabled  need to enable dmc again.
>>> +        */
>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>> +               if (vop->devfreq_event_dev)
>>> +
>>> devfreq_event_enable_edev(vop->devfreq_event_dev);
>>> +               devfreq_resume_device(vop->devfreq);
>>>          }
>>>   }
>>>
>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct
>>> device *master, void *data)
>>>          struct drm_device *drm_dev = data;
>>>          struct vop *vop;
>>>          struct resource *res;
>>> +       struct devfreq *devfreq;
>>> +       struct devfreq_event_dev *event_dev;
>>>          size_t alloc_size;
>>>          int ret, irq;
>>>
>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct
>>> device *master, void *data)
>>>                  return ret;
>>>
>>>          pm_runtime_enable(&pdev->dev);
>>> +
>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>> +       vop->vop_switch_status = 0;
>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>> +       vop->dmc_in_process = 0;
>>> +
>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>> +       if (IS_ERR(devfreq))
>>> +               goto out;
>>> +
>>> +       vop->devfreq = devfreq;
>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>> +
>>> +       event_dev =
>>> devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>> +                                                     0);
>>> +       if (IS_ERR(event_dev))
>>> +               goto out;
>>> +
>>> +       vop->devfreq_event_dev = event_dev;
>>> +out:
>>>          return 0;
>>>   }
>>>
>>> --
>>> 2.6.6
>>>
>>
>>
>
> --
> Lin Huang
>
>
huang lin Sept. 6, 2016, 7:01 p.m. UTC | #5
Hi

On 2016年09月07日 02:55, Sean Paul wrote:
> On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@rock-chips.com> wrote:
>> Hi Sean,
>>
>>
>> On 2016年09月07日 01:18, Sean Paul wrote:
>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>> when in ddr frequency scaling process, vop can not do enable or
>>>> disable operation, since in dcf we check vop clock to see whether
>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>> in vblank status, and we need to read vop register to check whether
>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>> status change to disable, dcf need to read vblank status, but if you
>>>> disable
>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>> frequency scaling.
>>>>
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>> Changes in v10:
>>>> - None
>>>>
>>>> Changes in v9:
>>>> - None
>>>>
>>>> Changes in v8:
>>>> - None
>>>>
>>>> Changes in v7:
>>>> - None
>>>>
>>>> Changes in v6:
>>>> - fix a build error
>>>>
>>>> Changes in v5:
>>>> - improve some nits
>>>>
>>>> Changes in v4:
>>>> - register notifier to devfreq_register_notifier
>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>> - when two vop enable, disable dmc
>>>> - when two vop back to one vop, enable dmc
>>>>
>>>> Changes in v3:
>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>
>>>> Changes in v2:
>>>> - None
>>>>
>>>> Changes in v1:
>>>> - use wait_event instead usleep
>>>>
>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>>> ++++++++++++++++++++++++++++
>>>>    1 file changed, 116 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> index efbc41a..a73f3aa 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> @@ -19,6 +19,8 @@
>>>>    #include <drm/drm_crtc_helper.h>
>>>>    #include <drm/drm_plane_helper.h>
>>>>
>>>> +#include <linux/devfreq.h>
>>>> +#include <linux/devfreq-event.h>
>>>>    #include <linux/kernel.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/platform_device.h>
>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>
>>>>           const struct vop_data *data;
>>>>
>>>> +       struct devfreq *devfreq;
>>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>>> +       struct notifier_block dmc_nb;
>>>> +       int dmc_in_process;
>>>> +       int vop_switch_status;
>>>> +       wait_queue_head_t wait_dmc_queue;
>>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>>           uint32_t *regsbak;
>>>>           void __iomem *regs;
>>>>
>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>>> vop *vop)
>>>>           spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>>    }
>>>>
>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>> +                     void *data)
>>>> +{
>>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>> +
>>>> +       if (event == DEVFREQ_PRECHANGE) {
>>>> +               /*
>>>> +                * check if vop in enable or disable process,
>>>> +                * if yes, wait until it finishes, use 200ms as
>>>> +                * timeout.
>>>> +                */
>>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>> +                                       !vop->vop_switch_status, HZ / 5))
>>>> +                       dev_warn(vop->dev,
>>>> +                                "Timeout waiting for vop swtich
>>>> status\n");
>>>> +               vop->dmc_in_process = 1;
>>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>>> +               vop->dmc_in_process = 0;
>>>> +               wake_up(&vop->wait_dmc_queue);
>>>> +       }
>>>> +
>>>> +       return NOTIFY_OK;
>>>> +}
>>>> +
>>>>    static int vop_enable(struct drm_crtc *crtc)
>>>>    {
>>>>           struct vop *vop = to_vop(crtc);
>>>> +       int num_enabled_crtc = 0;
>>>>           int ret;
>>>>
>>>> +       /*
>>>> +        * if in dmc scaling frequency process, wait until it finishes
>>>> +        * use 200ms as timeout time.
>>>> +        */
>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>> +               dev_warn(vop->dev,
>>>> +                        "Timeout waiting for dmc when vop enable\n");
>>>> +
>>>
>>> This wait_event_timeout code is terribly racey (same goes above and
>>> below).
>>    No i use the vop_switch_status and dmc_in_progress to handle the vop and
>> dmc racey,
>
> So what happens if dmc_in_progress becomes 1 right here, or anywhere
> during vop_enable()?
In dmc_notify(), i will check the vop_switch_status, so in the 
vop_enable() and vop_crtc_disable(),
the dmc_in_progress can not change value.
> Sean
>
>
>
>
>>     I assume this can fix the racey now. I do not better a idea now.
>>>
>>>
>>>> +       vop->vop_switch_status = 1;
>>>> +
>>>>           ret = pm_runtime_get_sync(vop->dev);
>>>>           if (ret < 0) {
>>>>                   dev_err(vop->dev, "failed to get pm runtime: %d\n",
>>>> ret);
>>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>>
>>>>           drm_crtc_vblank_on(crtc);
>>>>
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>> +
>>>> +       /* check how many VOPs in use now */
>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>> +               if (crtc->state->enable)
>>> I think you really want to check active, instead of enable.
>> Okay, i will check it, thanks.
>>
>>>> +                       num_enabled_crtc++;
>>>> +       }
>>>> +
>>>> +       /* if enable two vop, need to disable dmc */
>>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>>> +               if (vop->devfreq_event_dev)
>>>> +
>>>> devfreq_event_disable_edev(vop->devfreq_event_dev);
>>>> +               devfreq_suspend_device(vop->devfreq);
>>>> +       }
>>> This really feels like something that should be handled somewhere
>>> else. I don't fully understand how this works, but it seems like this
>>> dependency should be handled where it actually matters, rather than
>>> building in a seemingly arbitrary restriction in the vop driver.
>>>
>>> Even if this is the best place for it, this needs to be refactored to
>>> eliminate the races that exist now.
>>>
>>> Sean
>>>
>>>>           return 0;
>>>>
>>>>    err_disable_aclk:
>>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>>           clk_disable(vop->hclk);
>>>>    err_put_pm_runtime:
>>>>           pm_runtime_put_sync(vop->dev);
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>           return ret;
>>>>    }
>>>>
>>>>    static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>    {
>>>>           struct vop *vop = to_vop(crtc);
>>>> +       int num_enabled_crtc = 0;
>>>>           int i;
>>>>
>>>>           WARN_ON(vop->event);
>>>>
>>>>           /*
>>>> +        * if in dmc scaling frequency process, wait until it finish
>>>> +        * use 200ms as timeout time.
>>>> +        */
>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>> +               dev_warn(vop->dev,
>>>> +                        "Timeout waiting for dmc when vop disable\n");
>>>> +
>>>> +       vop->vop_switch_status = 1;
>>>> +
>>>> +       /*
>>>>            * We need to make sure that all windows are disabled before we
>>>>            * disable that crtc. Otherwise we might try to scan from a
>>>> destroyed
>>>>            * buffer later.
>>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>                   spin_unlock_irq(&crtc->dev->event_lock);
>>>>
>>>>                   crtc->state->event = NULL;
>>>> +
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>> +
>>>> +       /* check how many VOPs in use now */
>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>> +               if (crtc->state->enable)
>>>> +                       num_enabled_crtc++;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>>> +        * change to 1 vop enabled  need to enable dmc again.
>>>> +        */
>>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>>> +               if (vop->devfreq_event_dev)
>>>> +
>>>> devfreq_event_enable_edev(vop->devfreq_event_dev);
>>>> +               devfreq_resume_device(vop->devfreq);
>>>>           }
>>>>    }
>>>>
>>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct
>>>> device *master, void *data)
>>>>           struct drm_device *drm_dev = data;
>>>>           struct vop *vop;
>>>>           struct resource *res;
>>>> +       struct devfreq *devfreq;
>>>> +       struct devfreq_event_dev *event_dev;
>>>>           size_t alloc_size;
>>>>           int ret, irq;
>>>>
>>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct
>>>> device *master, void *data)
>>>>                   return ret;
>>>>
>>>>           pm_runtime_enable(&pdev->dev);
>>>> +
>>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>>> +       vop->vop_switch_status = 0;
>>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>>> +       vop->dmc_in_process = 0;
>>>> +
>>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>> +       if (IS_ERR(devfreq))
>>>> +               goto out;
>>>> +
>>>> +       vop->devfreq = devfreq;
>>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>>> +
>>>> +       event_dev =
>>>> devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>>> +                                                     0);
>>>> +       if (IS_ERR(event_dev))
>>>> +               goto out;
>>>> +
>>>> +       vop->devfreq_event_dev = event_dev;
>>>> +out:
>>>>           return 0;
>>>>    }
>>>>
>>>> --
>>>> 2.6.6
>>>>
>>>
>> --
>> Lin Huang
>>
>>
>
>
Sean Paul Sept. 6, 2016, 7:07 p.m. UTC | #6
On Tue, Sep 6, 2016 at 3:01 PM, hl <hl@rock-chips.com> wrote:
> Hi
>
>
> On 2016年09月07日 02:55, Sean Paul wrote:
>>
>> On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@rock-chips.com> wrote:
>>>
>>> Hi Sean,
>>>
>>>
>>> On 2016年09月07日 01:18, Sean Paul wrote:
>>>>
>>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>>>
>>>>> when in ddr frequency scaling process, vop can not do enable or
>>>>> disable operation, since in dcf we check vop clock to see whether
>>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>>> in vblank status, and we need to read vop register to check whether
>>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>>> status change to disable, dcf need to read vblank status, but if you
>>>>> disable
>>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>>> frequency scaling.
>>>>>
>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> ---
>>>>> Changes in v10:
>>>>> - None
>>>>>
>>>>> Changes in v9:
>>>>> - None
>>>>>
>>>>> Changes in v8:
>>>>> - None
>>>>>
>>>>> Changes in v7:
>>>>> - None
>>>>>
>>>>> Changes in v6:
>>>>> - fix a build error
>>>>>
>>>>> Changes in v5:
>>>>> - improve some nits
>>>>>
>>>>> Changes in v4:
>>>>> - register notifier to devfreq_register_notifier
>>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>>> - when two vop enable, disable dmc
>>>>> - when two vop back to one vop, enable dmc
>>>>>
>>>>> Changes in v3:
>>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>>
>>>>> Changes in v2:
>>>>> - None
>>>>>
>>>>> Changes in v1:
>>>>> - use wait_event instead usleep
>>>>>
>>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>>>> ++++++++++++++++++++++++++++
>>>>>    1 file changed, 116 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> index efbc41a..a73f3aa 100644
>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>> @@ -19,6 +19,8 @@
>>>>>    #include <drm/drm_crtc_helper.h>
>>>>>    #include <drm/drm_plane_helper.h>
>>>>>
>>>>> +#include <linux/devfreq.h>
>>>>> +#include <linux/devfreq-event.h>
>>>>>    #include <linux/kernel.h>
>>>>>    #include <linux/module.h>
>>>>>    #include <linux/platform_device.h>
>>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>>
>>>>>           const struct vop_data *data;
>>>>>
>>>>> +       struct devfreq *devfreq;
>>>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>>>> +       struct notifier_block dmc_nb;
>>>>> +       int dmc_in_process;
>>>>> +       int vop_switch_status;
>>>>> +       wait_queue_head_t wait_dmc_queue;
>>>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>>>           uint32_t *regsbak;
>>>>>           void __iomem *regs;
>>>>>
>>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>>>> vop *vop)
>>>>>           spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>>>    }
>>>>>
>>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>>> +                     void *data)
>>>>> +{
>>>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>>> +
>>>>> +       if (event == DEVFREQ_PRECHANGE) {
>>>>> +               /*
>>>>> +                * check if vop in enable or disable process,
>>>>> +                * if yes, wait until it finishes, use 200ms as
>>>>> +                * timeout.
>>>>> +                */
>>>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>>> +                                       !vop->vop_switch_status, HZ /
>>>>> 5))
>>>>> +                       dev_warn(vop->dev,
>>>>> +                                "Timeout waiting for vop swtich
>>>>> status\n");
>>>>> +               vop->dmc_in_process = 1;
>>>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>>>> +               vop->dmc_in_process = 0;
>>>>> +               wake_up(&vop->wait_dmc_queue);
>>>>> +       }
>>>>> +
>>>>> +       return NOTIFY_OK;
>>>>> +}
>>>>> +
>>>>>    static int vop_enable(struct drm_crtc *crtc)
>>>>>    {
>>>>>           struct vop *vop = to_vop(crtc);
>>>>> +       int num_enabled_crtc = 0;
>>>>>           int ret;
>>>>>
>>>>> +       /*
>>>>> +        * if in dmc scaling frequency process, wait until it finishes
>>>>> +        * use 200ms as timeout time.
>>>>> +        */
>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>> +               dev_warn(vop->dev,
>>>>> +                        "Timeout waiting for dmc when vop enable\n");
>>>>> +
>>>>
>>>>
>>>> This wait_event_timeout code is terribly racey (same goes above and
>>>> below).
>>>
>>>    No i use the vop_switch_status and dmc_in_progress to handle the vop
>>> and
>>> dmc racey,
>>
>>
>> So what happens if dmc_in_progress becomes 1 right here, or anywhere
>> during vop_enable()?
>
> In dmc_notify(), i will check the vop_switch_status, so in the vop_enable()
> and vop_crtc_disable(),
> the dmc_in_progress can not change value.
>


THREAD 1 - wait_event_timeout (vop_switch_status)
THREAD 2 - wait_event_timeout(wait_dmc_queue)
THREAD 2 - vop_switch_status = 1
THREAD 1 - dmc_in_progress = 1

Sean


>> Sean
>>
>>
>>
>>
>>>     I assume this can fix the racey now. I do not better a idea now.
>>>>
>>>>
>>>>
>>>>> +       vop->vop_switch_status = 1;
>>>>> +
>>>>>           ret = pm_runtime_get_sync(vop->dev);
>>>>>           if (ret < 0) {
>>>>>                   dev_err(vop->dev, "failed to get pm runtime: %d\n",
>>>>> ret);
>>>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>>>
>>>>>           drm_crtc_vblank_on(crtc);
>>>>>
>>>>> +       vop->vop_switch_status = 0;
>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>> +
>>>>> +       /* check how many VOPs in use now */
>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>> +               if (crtc->state->enable)
>>>>
>>>> I think you really want to check active, instead of enable.
>>>
>>> Okay, i will check it, thanks.
>>>
>>>>> +                       num_enabled_crtc++;
>>>>> +       }
>>>>> +
>>>>> +       /* if enable two vop, need to disable dmc */
>>>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>>>> +               if (vop->devfreq_event_dev)
>>>>> +
>>>>> devfreq_event_disable_edev(vop->devfreq_event_dev);
>>>>> +               devfreq_suspend_device(vop->devfreq);
>>>>> +       }
>>>>
>>>> This really feels like something that should be handled somewhere
>>>> else. I don't fully understand how this works, but it seems like this
>>>> dependency should be handled where it actually matters, rather than
>>>> building in a seemingly arbitrary restriction in the vop driver.
>>>>
>>>> Even if this is the best place for it, this needs to be refactored to
>>>> eliminate the races that exist now.
>>>>
>>>> Sean
>>>>
>>>>>           return 0;
>>>>>
>>>>>    err_disable_aclk:
>>>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>>>           clk_disable(vop->hclk);
>>>>>    err_put_pm_runtime:
>>>>>           pm_runtime_put_sync(vop->dev);
>>>>> +       vop->vop_switch_status = 0;
>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>           return ret;
>>>>>    }
>>>>>
>>>>>    static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>>    {
>>>>>           struct vop *vop = to_vop(crtc);
>>>>> +       int num_enabled_crtc = 0;
>>>>>           int i;
>>>>>
>>>>>           WARN_ON(vop->event);
>>>>>
>>>>>           /*
>>>>> +        * if in dmc scaling frequency process, wait until it finish
>>>>> +        * use 200ms as timeout time.
>>>>> +        */
>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>> +               dev_warn(vop->dev,
>>>>> +                        "Timeout waiting for dmc when vop disable\n");
>>>>> +
>>>>> +       vop->vop_switch_status = 1;
>>>>> +
>>>>> +       /*
>>>>>            * We need to make sure that all windows are disabled before
>>>>> we
>>>>>            * disable that crtc. Otherwise we might try to scan from a
>>>>> destroyed
>>>>>            * buffer later.
>>>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc
>>>>> *crtc)
>>>>>                   spin_unlock_irq(&crtc->dev->event_lock);
>>>>>
>>>>>                   crtc->state->event = NULL;
>>>>> +
>>>>> +       vop->vop_switch_status = 0;
>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>> +
>>>>> +       /* check how many VOPs in use now */
>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>> +               if (crtc->state->enable)
>>>>> +                       num_enabled_crtc++;
>>>>> +       }
>>>>> +
>>>>> +       /*
>>>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>>>> +        * change to 1 vop enabled  need to enable dmc again.
>>>>> +        */
>>>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>>>> +               if (vop->devfreq_event_dev)
>>>>> +
>>>>> devfreq_event_enable_edev(vop->devfreq_event_dev);
>>>>> +               devfreq_resume_device(vop->devfreq);
>>>>>           }
>>>>>    }
>>>>>
>>>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct
>>>>> device *master, void *data)
>>>>>           struct drm_device *drm_dev = data;
>>>>>           struct vop *vop;
>>>>>           struct resource *res;
>>>>> +       struct devfreq *devfreq;
>>>>> +       struct devfreq_event_dev *event_dev;
>>>>>           size_t alloc_size;
>>>>>           int ret, irq;
>>>>>
>>>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct
>>>>> device *master, void *data)
>>>>>                   return ret;
>>>>>
>>>>>           pm_runtime_enable(&pdev->dev);
>>>>> +
>>>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>>>> +       vop->vop_switch_status = 0;
>>>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>>>> +       vop->dmc_in_process = 0;
>>>>> +
>>>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>> +       if (IS_ERR(devfreq))
>>>>> +               goto out;
>>>>> +
>>>>> +       vop->devfreq = devfreq;
>>>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>>>> +
>>>>> +       event_dev =
>>>>> devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>>>> +                                                     0);
>>>>> +       if (IS_ERR(event_dev))
>>>>> +               goto out;
>>>>> +
>>>>> +       vop->devfreq_event_dev = event_dev;
>>>>> +out:
>>>>>           return 0;
>>>>>    }
>>>>>
>>>>> --
>>>>> 2.6.6
>>>>>
>>>>
>>> --
>>> Lin Huang
>>>
>>>
>>
>>
>
> --
> Lin Huang
>
>
dbasehore . Sept. 7, 2016, 6:07 p.m. UTC | #7
On Tue, Sep 6, 2016 at 12:07 PM, Sean Paul <seanpaul@google.com> wrote:
> On Tue, Sep 6, 2016 at 3:01 PM, hl <hl@rock-chips.com> wrote:
>> Hi
>>
>>
>> On 2016年09月07日 02:55, Sean Paul wrote:
>>>
>>> On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@rock-chips.com> wrote:
>>>>
>>>> Hi Sean,
>>>>
>>>>
>>>> On 2016年09月07日 01:18, Sean Paul wrote:
>>>>>
>>>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>>>>
>>>>>> when in ddr frequency scaling process, vop can not do enable or
>>>>>> disable operation, since in dcf we check vop clock to see whether
>>>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>>>> in vblank status, and we need to read vop register to check whether
>>>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>>>> status change to disable, dcf need to read vblank status, but if you
>>>>>> disable
>>>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>>>> frequency scaling.
>>>>>>
>>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>> ---
>>>>>> Changes in v10:
>>>>>> - None
>>>>>>
>>>>>> Changes in v9:
>>>>>> - None
>>>>>>
>>>>>> Changes in v8:
>>>>>> - None
>>>>>>
>>>>>> Changes in v7:
>>>>>> - None
>>>>>>
>>>>>> Changes in v6:
>>>>>> - fix a build error
>>>>>>
>>>>>> Changes in v5:
>>>>>> - improve some nits
>>>>>>
>>>>>> Changes in v4:
>>>>>> - register notifier to devfreq_register_notifier
>>>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>>>> - when two vop enable, disable dmc
>>>>>> - when two vop back to one vop, enable dmc
>>>>>>
>>>>>> Changes in v3:
>>>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>>>
>>>>>> Changes in v2:
>>>>>> - None
>>>>>>
>>>>>> Changes in v1:
>>>>>> - use wait_event instead usleep
>>>>>>
>>>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>>>>> ++++++++++++++++++++++++++++
>>>>>>    1 file changed, 116 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> index efbc41a..a73f3aa 100644
>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>> @@ -19,6 +19,8 @@
>>>>>>    #include <drm/drm_crtc_helper.h>
>>>>>>    #include <drm/drm_plane_helper.h>
>>>>>>
>>>>>> +#include <linux/devfreq.h>
>>>>>> +#include <linux/devfreq-event.h>
>>>>>>    #include <linux/kernel.h>
>>>>>>    #include <linux/module.h>
>>>>>>    #include <linux/platform_device.h>
>>>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>>>
>>>>>>           const struct vop_data *data;
>>>>>>
>>>>>> +       struct devfreq *devfreq;
>>>>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>>>>> +       struct notifier_block dmc_nb;
>>>>>> +       int dmc_in_process;
>>>>>> +       int vop_switch_status;
>>>>>> +       wait_queue_head_t wait_dmc_queue;
>>>>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>>>>           uint32_t *regsbak;
>>>>>>           void __iomem *regs;
>>>>>>
>>>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>>>>> vop *vop)
>>>>>>           spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>>>>    }
>>>>>>
>>>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>>>> +                     void *data)
>>>>>> +{
>>>>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>>>> +
>>>>>> +       if (event == DEVFREQ_PRECHANGE) {
>>>>>> +               /*
>>>>>> +                * check if vop in enable or disable process,
>>>>>> +                * if yes, wait until it finishes, use 200ms as
>>>>>> +                * timeout.
>>>>>> +                */
>>>>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>>>> +                                       !vop->vop_switch_status, HZ /
>>>>>> 5))
>>>>>> +                       dev_warn(vop->dev,
>>>>>> +                                "Timeout waiting for vop swtich
>>>>>> status\n");
>>>>>> +               vop->dmc_in_process = 1;
>>>>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>>>>> +               vop->dmc_in_process = 0;
>>>>>> +               wake_up(&vop->wait_dmc_queue);
>>>>>> +       }
>>>>>> +
>>>>>> +       return NOTIFY_OK;
>>>>>> +}
>>>>>> +
>>>>>>    static int vop_enable(struct drm_crtc *crtc)
>>>>>>    {
>>>>>>           struct vop *vop = to_vop(crtc);
>>>>>> +       int num_enabled_crtc = 0;
>>>>>>           int ret;
>>>>>>
>>>>>> +       /*
>>>>>> +        * if in dmc scaling frequency process, wait until it finishes
>>>>>> +        * use 200ms as timeout time.
>>>>>> +        */
>>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>>> +               dev_warn(vop->dev,
>>>>>> +                        "Timeout waiting for dmc when vop enable\n");
>>>>>> +
>>>>>
>>>>>
>>>>> This wait_event_timeout code is terribly racey (same goes above and
>>>>> below).
>>>>
>>>>    No i use the vop_switch_status and dmc_in_progress to handle the vop
>>>> and
>>>> dmc racey,
>>>
>>>
>>> So what happens if dmc_in_progress becomes 1 right here, or anywhere
>>> during vop_enable()?
>>
>> In dmc_notify(), i will check the vop_switch_status, so in the vop_enable()
>> and vop_crtc_disable(),
>> the dmc_in_progress can not change value.
>>
>
>
> THREAD 1 - wait_event_timeout (vop_switch_status)
> THREAD 2 - wait_event_timeout(wait_dmc_queue)
> THREAD 2 - vop_switch_status = 1
> THREAD 1 - dmc_in_progress = 1
>
> Sean
>

I've thought about how to handle these timeout conditions, and it
might be best to get rid of them. These would actually be error
conditions, and we don't plan to hit them.

Also, if something goes wrong with DMC frequency transitions, we are
likely out of luck and going to panic anyways.

>
>>> Sean
>>>
>>>
>>>
>>>
>>>>     I assume this can fix the racey now. I do not better a idea now.
>>>>>
>>>>>
>>>>>
>>>>>> +       vop->vop_switch_status = 1;
>>>>>> +
>>>>>>           ret = pm_runtime_get_sync(vop->dev);
>>>>>>           if (ret < 0) {
>>>>>>                   dev_err(vop->dev, "failed to get pm runtime: %d\n",
>>>>>> ret);
>>>>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>>>>
>>>>>>           drm_crtc_vblank_on(crtc);
>>>>>>
>>>>>> +       vop->vop_switch_status = 0;
>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>> +
>>>>>> +       /* check how many VOPs in use now */
>>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>>> +               if (crtc->state->enable)
>>>>>
>>>>> I think you really want to check active, instead of enable.
>>>>
>>>> Okay, i will check it, thanks.
>>>>
>>>>>> +                       num_enabled_crtc++;
>>>>>> +       }
>>>>>> +
>>>>>> +       /* if enable two vop, need to disable dmc */
>>>>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>>>>> +               if (vop->devfreq_event_dev)
>>>>>> +
>>>>>> devfreq_event_disable_edev(vop->devfreq_event_dev);
>>>>>> +               devfreq_suspend_device(vop->devfreq);
>>>>>> +       }
>>>>>
>>>>> This really feels like something that should be handled somewhere
>>>>> else. I don't fully understand how this works, but it seems like this
>>>>> dependency should be handled where it actually matters, rather than
>>>>> building in a seemingly arbitrary restriction in the vop driver.
>>>>>
>>>>> Even if this is the best place for it, this needs to be refactored to
>>>>> eliminate the races that exist now.
>>>>>
>>>>> Sean
>>>>>
>>>>>>           return 0;
>>>>>>
>>>>>>    err_disable_aclk:
>>>>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>>>>           clk_disable(vop->hclk);
>>>>>>    err_put_pm_runtime:
>>>>>>           pm_runtime_put_sync(vop->dev);
>>>>>> +       vop->vop_switch_status = 0;
>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>>           return ret;
>>>>>>    }
>>>>>>
>>>>>>    static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>>>    {
>>>>>>           struct vop *vop = to_vop(crtc);
>>>>>> +       int num_enabled_crtc = 0;
>>>>>>           int i;
>>>>>>
>>>>>>           WARN_ON(vop->event);
>>>>>>
>>>>>>           /*
>>>>>> +        * if in dmc scaling frequency process, wait until it finish
>>>>>> +        * use 200ms as timeout time.
>>>>>> +        */
>>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>>> +               dev_warn(vop->dev,
>>>>>> +                        "Timeout waiting for dmc when vop disable\n");
>>>>>> +
>>>>>> +       vop->vop_switch_status = 1;
>>>>>> +
>>>>>> +       /*
>>>>>>            * We need to make sure that all windows are disabled before
>>>>>> we
>>>>>>            * disable that crtc. Otherwise we might try to scan from a
>>>>>> destroyed
>>>>>>            * buffer later.
>>>>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc
>>>>>> *crtc)
>>>>>>                   spin_unlock_irq(&crtc->dev->event_lock);
>>>>>>
>>>>>>                   crtc->state->event = NULL;
>>>>>> +
>>>>>> +       vop->vop_switch_status = 0;
>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>> +
>>>>>> +       /* check how many VOPs in use now */
>>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>>> +               if (crtc->state->enable)
>>>>>> +                       num_enabled_crtc++;
>>>>>> +       }
>>>>>> +
>>>>>> +       /*
>>>>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>>>>> +        * change to 1 vop enabled  need to enable dmc again.
>>>>>> +        */
>>>>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>>>>> +               if (vop->devfreq_event_dev)
>>>>>> +
>>>>>> devfreq_event_enable_edev(vop->devfreq_event_dev);
>>>>>> +               devfreq_resume_device(vop->devfreq);
>>>>>>           }
>>>>>>    }
>>>>>>
>>>>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct
>>>>>> device *master, void *data)
>>>>>>           struct drm_device *drm_dev = data;
>>>>>>           struct vop *vop;
>>>>>>           struct resource *res;
>>>>>> +       struct devfreq *devfreq;
>>>>>> +       struct devfreq_event_dev *event_dev;
>>>>>>           size_t alloc_size;
>>>>>>           int ret, irq;
>>>>>>
>>>>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct
>>>>>> device *master, void *data)
>>>>>>                   return ret;
>>>>>>
>>>>>>           pm_runtime_enable(&pdev->dev);
>>>>>> +
>>>>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>>>>> +       vop->vop_switch_status = 0;
>>>>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>>>>> +       vop->dmc_in_process = 0;
>>>>>> +
>>>>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>>> +       if (IS_ERR(devfreq))
>>>>>> +               goto out;
>>>>>> +
>>>>>> +       vop->devfreq = devfreq;
>>>>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>>>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>>>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>>>>> +
>>>>>> +       event_dev =
>>>>>> devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>>>>> +                                                     0);
>>>>>> +       if (IS_ERR(event_dev))
>>>>>> +               goto out;
>>>>>> +
>>>>>> +       vop->devfreq_event_dev = event_dev;
>>>>>> +out:
>>>>>>           return 0;
>>>>>>    }
>>>>>>
>>>>>> --
>>>>>> 2.6.6
>>>>>>
>>>>>
>>>> --
>>>> Lin Huang
>>>>
>>>>
>>>
>>>
>>
>> --
>> Lin Huang
>>
>>
dbasehore . Sept. 7, 2016, 6:13 p.m. UTC | #8
On Tue, Sep 6, 2016 at 10:18 AM, Sean Paul <seanpaul@google.com> wrote:
> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>> when in ddr frequency scaling process, vop can not do enable or
>> disable operation, since in dcf we check vop clock to see whether
>> vop work. If vop work, dcf do ddr frequency scaling when vop
>> in vblank status, and we need to read vop register to check whether
>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>> any time. So when do ddr frequency scaling, you disabled or enable
>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>> disable status change to enable). 2, kernel hang (when vop from enable
>> status change to disable, dcf need to read vblank status, but if you disable
>> vop clock, it can not get the status, it will lead soc dead) So we need
>> register to devfreq notifier, and we can get the dmc status. Also, when
>> there have two vop enabled, we need to disable dmc, since dcf only base
>> on one vop vblank time, so the other panel will flicker when do ddr
>> frequency scaling.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> ---
>> Changes in v10:
>> - None
>>
>> Changes in v9:
>> - None
>>
>> Changes in v8:
>> - None
>>
>> Changes in v7:
>> - None
>>
>> Changes in v6:
>> - fix a build error
>>
>> Changes in v5:
>> - improve some nits
>>
>> Changes in v4:
>> - register notifier to devfreq_register_notifier
>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>> - when two vop enable, disable dmc
>> - when two vop back to one vop, enable dmc
>>
>> Changes in v3:
>> - when do vop eanble/disable, dmc will wait until it finish
>>
>> Changes in v2:
>> - None
>>
>> Changes in v1:
>> - use wait_event instead usleep
>>
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
>>  1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index efbc41a..a73f3aa 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -19,6 +19,8 @@
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_plane_helper.h>
>>
>> +#include <linux/devfreq.h>
>> +#include <linux/devfreq-event.h>
>>  #include <linux/kernel.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> @@ -118,6 +120,13 @@ struct vop {
>>
>>         const struct vop_data *data;
>>
>> +       struct devfreq *devfreq;
>> +       struct devfreq_event_dev *devfreq_event_dev;
>> +       struct notifier_block dmc_nb;
>> +       int dmc_in_process;
>> +       int vop_switch_status;
>> +       wait_queue_head_t wait_dmc_queue;
>> +       wait_queue_head_t wait_vop_switch_queue;
>>         uint32_t *regsbak;
>>         void __iomem *regs;
>>
>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>>  }
>>
>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>> +                     void *data)
>> +{
>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>> +
>> +       if (event == DEVFREQ_PRECHANGE) {
>> +               /*
>> +                * check if vop in enable or disable process,
>> +                * if yes, wait until it finishes, use 200ms as
>> +                * timeout.
>> +                */
>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>> +                                       !vop->vop_switch_status, HZ / 5))
>> +                       dev_warn(vop->dev,
>> +                                "Timeout waiting for vop swtich status\n");
>> +               vop->dmc_in_process = 1;
>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>> +               vop->dmc_in_process = 0;
>> +               wake_up(&vop->wait_dmc_queue);
>> +       }
>> +
>> +       return NOTIFY_OK;
>> +}
>> +
>>  static int vop_enable(struct drm_crtc *crtc)
>>  {
>>         struct vop *vop = to_vop(crtc);
>> +       int num_enabled_crtc = 0;
>>         int ret;
>>
>> +       /*
>> +        * if in dmc scaling frequency process, wait until it finishes
>> +        * use 200ms as timeout time.
>> +        */
>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>> +                               !vop->dmc_in_process, HZ / 5))
>> +               dev_warn(vop->dev,
>> +                        "Timeout waiting for dmc when vop enable\n");
>> +
>
>
> This wait_event_timeout code is terribly racey (same goes above and below).
>
>
>> +       vop->vop_switch_status = 1;
>> +
>>         ret = pm_runtime_get_sync(vop->dev);
>>         if (ret < 0) {
>>                 dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>
>>         drm_crtc_vblank_on(crtc);
>>
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>> +
>> +       /* check how many VOPs in use now */
>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>> +               if (crtc->state->enable)
>
> I think you really want to check active, instead of enable.
>
>> +                       num_enabled_crtc++;
>> +       }
>> +
>> +       /* if enable two vop, need to disable dmc */
>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>> +               if (vop->devfreq_event_dev)
>> +                       devfreq_event_disable_edev(vop->devfreq_event_dev);
>> +               devfreq_suspend_device(vop->devfreq);
>> +       }
>
> This really feels like something that should be handled somewhere
> else. I don't fully understand how this works, but it seems like this
> dependency should be handled where it actually matters, rather than
> building in a seemingly arbitrary restriction in the vop driver.
>
> Even if this is the best place for it, this needs to be refactored to
> eliminate the races that exist now.
>
> Sean

DMC transitions can't happen when there is more than one display. You
would have to synchronize with more than one vblank on the DCF (which
actually changes the DDR frequency), and that's probably not going to
happen.

If this code should be done somewhere else, it needs to still be able
to check the number of displays and avoid race conditions with
displays getting enabled or disabled. I don't know enough about DRM to
know if that's possible outside of this code.

>
>>         return 0;
>>
>>  err_disable_aclk:
>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>         clk_disable(vop->hclk);
>>  err_put_pm_runtime:
>>         pm_runtime_put_sync(vop->dev);
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>>         return ret;
>>  }
>>
>>  static void vop_crtc_disable(struct drm_crtc *crtc)
>>  {
>>         struct vop *vop = to_vop(crtc);
>> +       int num_enabled_crtc = 0;
>>         int i;
>>
>>         WARN_ON(vop->event);
>>
>>         /*
>> +        * if in dmc scaling frequency process, wait until it finish
>> +        * use 200ms as timeout time.
>> +        */
>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>> +                               !vop->dmc_in_process, HZ / 5))
>> +               dev_warn(vop->dev,
>> +                        "Timeout waiting for dmc when vop disable\n");
>> +
>> +       vop->vop_switch_status = 1;
>> +
>> +       /*
>>          * We need to make sure that all windows are disabled before we
>>          * disable that crtc. Otherwise we might try to scan from a destroyed
>>          * buffer later.
>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>                 spin_unlock_irq(&crtc->dev->event_lock);
>>
>>                 crtc->state->event = NULL;
>> +
>> +       vop->vop_switch_status = 0;
>> +       wake_up(&vop->wait_vop_switch_queue);
>> +
>> +       /* check how many VOPs in use now */
>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>> +               if (crtc->state->enable)
>> +                       num_enabled_crtc++;
>> +       }
>> +
>> +       /*
>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>> +        * change to 1 vop enabled  need to enable dmc again.
>> +        */
>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>> +               if (vop->devfreq_event_dev)
>> +                       devfreq_event_enable_edev(vop->devfreq_event_dev);
>> +               devfreq_resume_device(vop->devfreq);
>>         }
>>  }
>>
>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>         struct drm_device *drm_dev = data;
>>         struct vop *vop;
>>         struct resource *res;
>> +       struct devfreq *devfreq;
>> +       struct devfreq_event_dev *event_dev;
>>         size_t alloc_size;
>>         int ret, irq;
>>
>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>                 return ret;
>>
>>         pm_runtime_enable(&pdev->dev);
>> +
>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>> +       vop->vop_switch_status = 0;
>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>> +       vop->dmc_in_process = 0;
>> +
>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>> +       if (IS_ERR(devfreq))
>> +               goto out;
>> +
>> +       vop->devfreq = devfreq;
>> +       vop->dmc_nb.notifier_call = dmc_notify;
>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>> +
>> +       event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>> +                                                     0);
>> +       if (IS_ERR(event_dev))
>> +               goto out;
>> +
>> +       vop->devfreq_event_dev = event_dev;
>> +out:
>>         return 0;
>>  }
>>
>> --
>> 2.6.6
>>
Sean Paul Sept. 7, 2016, 6:27 p.m. UTC | #9
On Wed, Sep 7, 2016 at 2:07 PM, dbasehore . <dbasehore@chromium.org> wrote:
> On Tue, Sep 6, 2016 at 12:07 PM, Sean Paul <seanpaul@google.com> wrote:
>> On Tue, Sep 6, 2016 at 3:01 PM, hl <hl@rock-chips.com> wrote:
>>> Hi
>>>
>>>
>>> On 2016年09月07日 02:55, Sean Paul wrote:
>>>>
>>>> On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@rock-chips.com> wrote:
>>>>>
>>>>> Hi Sean,
>>>>>
>>>>>
>>>>> On 2016年09月07日 01:18, Sean Paul wrote:
>>>>>>
>>>>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>>>>>
>>>>>>> when in ddr frequency scaling process, vop can not do enable or
>>>>>>> disable operation, since in dcf we check vop clock to see whether
>>>>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>>>>> in vblank status, and we need to read vop register to check whether
>>>>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>>>>> status change to disable, dcf need to read vblank status, but if you
>>>>>>> disable
>>>>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>>>>> frequency scaling.
>>>>>>>
>>>>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>>>>> ---
>>>>>>> Changes in v10:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes in v9:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes in v8:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes in v7:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes in v6:
>>>>>>> - fix a build error
>>>>>>>
>>>>>>> Changes in v5:
>>>>>>> - improve some nits
>>>>>>>
>>>>>>> Changes in v4:
>>>>>>> - register notifier to devfreq_register_notifier
>>>>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>>>>> - when two vop enable, disable dmc
>>>>>>> - when two vop back to one vop, enable dmc
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - None
>>>>>>>
>>>>>>> Changes in v1:
>>>>>>> - use wait_event instead usleep
>>>>>>>
>>>>>>>    drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116
>>>>>>> ++++++++++++++++++++++++++++
>>>>>>>    1 file changed, 116 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>>> index efbc41a..a73f3aa 100644
>>>>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>>>>> @@ -19,6 +19,8 @@
>>>>>>>    #include <drm/drm_crtc_helper.h>
>>>>>>>    #include <drm/drm_plane_helper.h>
>>>>>>>
>>>>>>> +#include <linux/devfreq.h>
>>>>>>> +#include <linux/devfreq-event.h>
>>>>>>>    #include <linux/kernel.h>
>>>>>>>    #include <linux/module.h>
>>>>>>>    #include <linux/platform_device.h>
>>>>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>>>>
>>>>>>>           const struct vop_data *data;
>>>>>>>
>>>>>>> +       struct devfreq *devfreq;
>>>>>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>>>>>> +       struct notifier_block dmc_nb;
>>>>>>> +       int dmc_in_process;
>>>>>>> +       int vop_switch_status;
>>>>>>> +       wait_queue_head_t wait_dmc_queue;
>>>>>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>>>>>           uint32_t *regsbak;
>>>>>>>           void __iomem *regs;
>>>>>>>
>>>>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct
>>>>>>> vop *vop)
>>>>>>>           spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>>>>>    }
>>>>>>>
>>>>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>>>>> +                     void *data)
>>>>>>> +{
>>>>>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>>>>> +
>>>>>>> +       if (event == DEVFREQ_PRECHANGE) {
>>>>>>> +               /*
>>>>>>> +                * check if vop in enable or disable process,
>>>>>>> +                * if yes, wait until it finishes, use 200ms as
>>>>>>> +                * timeout.
>>>>>>> +                */
>>>>>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>>>>> +                                       !vop->vop_switch_status, HZ /
>>>>>>> 5))
>>>>>>> +                       dev_warn(vop->dev,
>>>>>>> +                                "Timeout waiting for vop swtich
>>>>>>> status\n");
>>>>>>> +               vop->dmc_in_process = 1;
>>>>>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>>>>>> +               vop->dmc_in_process = 0;
>>>>>>> +               wake_up(&vop->wait_dmc_queue);
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       return NOTIFY_OK;
>>>>>>> +}
>>>>>>> +
>>>>>>>    static int vop_enable(struct drm_crtc *crtc)
>>>>>>>    {
>>>>>>>           struct vop *vop = to_vop(crtc);
>>>>>>> +       int num_enabled_crtc = 0;
>>>>>>>           int ret;
>>>>>>>
>>>>>>> +       /*
>>>>>>> +        * if in dmc scaling frequency process, wait until it finishes
>>>>>>> +        * use 200ms as timeout time.
>>>>>>> +        */
>>>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>>>> +               dev_warn(vop->dev,
>>>>>>> +                        "Timeout waiting for dmc when vop enable\n");
>>>>>>> +
>>>>>>
>>>>>>
>>>>>> This wait_event_timeout code is terribly racey (same goes above and
>>>>>> below).
>>>>>
>>>>>    No i use the vop_switch_status and dmc_in_progress to handle the vop
>>>>> and
>>>>> dmc racey,
>>>>
>>>>
>>>> So what happens if dmc_in_progress becomes 1 right here, or anywhere
>>>> during vop_enable()?
>>>
>>> In dmc_notify(), i will check the vop_switch_status, so in the vop_enable()
>>> and vop_crtc_disable(),
>>> the dmc_in_progress can not change value.
>>>
>>
>>
>> THREAD 1 - wait_event_timeout (vop_switch_status)
>> THREAD 2 - wait_event_timeout(wait_dmc_queue)
>> THREAD 2 - vop_switch_status = 1
>> THREAD 1 - dmc_in_progress = 1
>>
>> Sean
>>
>
> I've thought about how to handle these timeout conditions, and it
> might be best to get rid of them. These would actually be error
> conditions, and we don't plan to hit them.
>
> Also, if something goes wrong with DMC frequency transitions, we are
> likely out of luck and going to panic anyways.

Yeah, I'm not sure if the timeouts are recoverable (makes sense that
they're not), but this code has several different variations of the
race I described above.

I'm working on something a little more buttoned down to hopefully sync
things up better.

Sean


>
>>
>>>> Sean
>>>>
>>>>
>>>>
>>>>
>>>>>     I assume this can fix the racey now. I do not better a idea now.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> +       vop->vop_switch_status = 1;
>>>>>>> +
>>>>>>>           ret = pm_runtime_get_sync(vop->dev);
>>>>>>>           if (ret < 0) {
>>>>>>>                   dev_err(vop->dev, "failed to get pm runtime: %d\n",
>>>>>>> ret);
>>>>>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>>>>>
>>>>>>>           drm_crtc_vblank_on(crtc);
>>>>>>>
>>>>>>> +       vop->vop_switch_status = 0;
>>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>>> +
>>>>>>> +       /* check how many VOPs in use now */
>>>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>>>> +               if (crtc->state->enable)
>>>>>>
>>>>>> I think you really want to check active, instead of enable.
>>>>>
>>>>> Okay, i will check it, thanks.
>>>>>
>>>>>>> +                       num_enabled_crtc++;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       /* if enable two vop, need to disable dmc */
>>>>>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>>>>>> +               if (vop->devfreq_event_dev)
>>>>>>> +
>>>>>>> devfreq_event_disable_edev(vop->devfreq_event_dev);
>>>>>>> +               devfreq_suspend_device(vop->devfreq);
>>>>>>> +       }
>>>>>>
>>>>>> This really feels like something that should be handled somewhere
>>>>>> else. I don't fully understand how this works, but it seems like this
>>>>>> dependency should be handled where it actually matters, rather than
>>>>>> building in a seemingly arbitrary restriction in the vop driver.
>>>>>>
>>>>>> Even if this is the best place for it, this needs to be refactored to
>>>>>> eliminate the races that exist now.
>>>>>>
>>>>>> Sean
>>>>>>
>>>>>>>           return 0;
>>>>>>>
>>>>>>>    err_disable_aclk:
>>>>>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>>>>>           clk_disable(vop->hclk);
>>>>>>>    err_put_pm_runtime:
>>>>>>>           pm_runtime_put_sync(vop->dev);
>>>>>>> +       vop->vop_switch_status = 0;
>>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>>>           return ret;
>>>>>>>    }
>>>>>>>
>>>>>>>    static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>>>>    {
>>>>>>>           struct vop *vop = to_vop(crtc);
>>>>>>> +       int num_enabled_crtc = 0;
>>>>>>>           int i;
>>>>>>>
>>>>>>>           WARN_ON(vop->event);
>>>>>>>
>>>>>>>           /*
>>>>>>> +        * if in dmc scaling frequency process, wait until it finish
>>>>>>> +        * use 200ms as timeout time.
>>>>>>> +        */
>>>>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>>>>> +               dev_warn(vop->dev,
>>>>>>> +                        "Timeout waiting for dmc when vop disable\n");
>>>>>>> +
>>>>>>> +       vop->vop_switch_status = 1;
>>>>>>> +
>>>>>>> +       /*
>>>>>>>            * We need to make sure that all windows are disabled before
>>>>>>> we
>>>>>>>            * disable that crtc. Otherwise we might try to scan from a
>>>>>>> destroyed
>>>>>>>            * buffer later.
>>>>>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc
>>>>>>> *crtc)
>>>>>>>                   spin_unlock_irq(&crtc->dev->event_lock);
>>>>>>>
>>>>>>>                   crtc->state->event = NULL;
>>>>>>> +
>>>>>>> +       vop->vop_switch_status = 0;
>>>>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>>>> +
>>>>>>> +       /* check how many VOPs in use now */
>>>>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>>>>> +               if (crtc->state->enable)
>>>>>>> +                       num_enabled_crtc++;
>>>>>>> +       }
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>>>>>> +        * change to 1 vop enabled  need to enable dmc again.
>>>>>>> +        */
>>>>>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>>>>>> +               if (vop->devfreq_event_dev)
>>>>>>> +
>>>>>>> devfreq_event_enable_edev(vop->devfreq_event_dev);
>>>>>>> +               devfreq_resume_device(vop->devfreq);
>>>>>>>           }
>>>>>>>    }
>>>>>>>
>>>>>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct
>>>>>>> device *master, void *data)
>>>>>>>           struct drm_device *drm_dev = data;
>>>>>>>           struct vop *vop;
>>>>>>>           struct resource *res;
>>>>>>> +       struct devfreq *devfreq;
>>>>>>> +       struct devfreq_event_dev *event_dev;
>>>>>>>           size_t alloc_size;
>>>>>>>           int ret, irq;
>>>>>>>
>>>>>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct
>>>>>>> device *master, void *data)
>>>>>>>                   return ret;
>>>>>>>
>>>>>>>           pm_runtime_enable(&pdev->dev);
>>>>>>> +
>>>>>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>>>>>> +       vop->vop_switch_status = 0;
>>>>>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>>>>>> +       vop->dmc_in_process = 0;
>>>>>>> +
>>>>>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>>>>> +       if (IS_ERR(devfreq))
>>>>>>> +               goto out;
>>>>>>> +
>>>>>>> +       vop->devfreq = devfreq;
>>>>>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>>>>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>>>>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>>>>>> +
>>>>>>> +       event_dev =
>>>>>>> devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>>>>>> +                                                     0);
>>>>>>> +       if (IS_ERR(event_dev))
>>>>>>> +               goto out;
>>>>>>> +
>>>>>>> +       vop->devfreq_event_dev = event_dev;
>>>>>>> +out:
>>>>>>>           return 0;
>>>>>>>    }
>>>>>>>
>>>>>>> --
>>>>>>> 2.6.6
>>>>>>>
>>>>>>
>>>>> --
>>>>> Lin Huang
>>>>>
>>>>>
>>>>
>>>>
>>>
>>> --
>>> Lin Huang
>>>
>>>
Sean Paul Sept. 7, 2016, 8:31 p.m. UTC | #10
On Wed, Sep 7, 2016 at 2:13 PM, dbasehore . <dbasehore@chromium.org> wrote:
> On Tue, Sep 6, 2016 at 10:18 AM, Sean Paul <seanpaul@google.com> wrote:
>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>> when in ddr frequency scaling process, vop can not do enable or
>>> disable operation, since in dcf we check vop clock to see whether
>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>> in vblank status, and we need to read vop register to check whether
>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>> any time. So when do ddr frequency scaling, you disabled or enable
>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>> disable status change to enable). 2, kernel hang (when vop from enable
>>> status change to disable, dcf need to read vblank status, but if you disable
>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>> on one vop vblank time, so the other panel will flicker when do ddr
>>> frequency scaling.
>>>
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>> ---
>>> Changes in v10:
>>> - None
>>>
>>> Changes in v9:
>>> - None
>>>
>>> Changes in v8:
>>> - None
>>>
>>> Changes in v7:
>>> - None
>>>
>>> Changes in v6:
>>> - fix a build error
>>>
>>> Changes in v5:
>>> - improve some nits
>>>
>>> Changes in v4:
>>> - register notifier to devfreq_register_notifier
>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>> - when two vop enable, disable dmc
>>> - when two vop back to one vop, enable dmc
>>>
>>> Changes in v3:
>>> - when do vop eanble/disable, dmc will wait until it finish
>>>
>>> Changes in v2:
>>> - None
>>>
>>> Changes in v1:
>>> - use wait_event instead usleep
>>>
>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
>>>  1 file changed, 116 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index efbc41a..a73f3aa 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -19,6 +19,8 @@
>>>  #include <drm/drm_crtc_helper.h>
>>>  #include <drm/drm_plane_helper.h>
>>>
>>> +#include <linux/devfreq.h>
>>> +#include <linux/devfreq-event.h>
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>> @@ -118,6 +120,13 @@ struct vop {
>>>
>>>         const struct vop_data *data;
>>>
>>> +       struct devfreq *devfreq;
>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>> +       struct notifier_block dmc_nb;
>>> +       int dmc_in_process;
>>> +       int vop_switch_status;
>>> +       wait_queue_head_t wait_dmc_queue;
>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>         uint32_t *regsbak;
>>>         void __iomem *regs;
>>>
>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>>>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>  }
>>>
>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>> +                     void *data)
>>> +{
>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>> +
>>> +       if (event == DEVFREQ_PRECHANGE) {
>>> +               /*
>>> +                * check if vop in enable or disable process,
>>> +                * if yes, wait until it finishes, use 200ms as
>>> +                * timeout.
>>> +                */
>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>> +                                       !vop->vop_switch_status, HZ / 5))
>>> +                       dev_warn(vop->dev,
>>> +                                "Timeout waiting for vop swtich status\n");
>>> +               vop->dmc_in_process = 1;
>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>> +               vop->dmc_in_process = 0;
>>> +               wake_up(&vop->wait_dmc_queue);
>>> +       }
>>> +
>>> +       return NOTIFY_OK;
>>> +}
>>> +
>>>  static int vop_enable(struct drm_crtc *crtc)
>>>  {
>>>         struct vop *vop = to_vop(crtc);
>>> +       int num_enabled_crtc = 0;
>>>         int ret;
>>>
>>> +       /*
>>> +        * if in dmc scaling frequency process, wait until it finishes
>>> +        * use 200ms as timeout time.
>>> +        */
>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>> +                               !vop->dmc_in_process, HZ / 5))
>>> +               dev_warn(vop->dev,
>>> +                        "Timeout waiting for dmc when vop enable\n");
>>> +
>>
>>
>> This wait_event_timeout code is terribly racey (same goes above and below).
>>
>>
>>> +       vop->vop_switch_status = 1;
>>> +
>>>         ret = pm_runtime_get_sync(vop->dev);
>>>         if (ret < 0) {
>>>                 dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>
>>>         drm_crtc_vblank_on(crtc);
>>>
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>> +
>>> +       /* check how many VOPs in use now */
>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>> +               if (crtc->state->enable)
>>
>> I think you really want to check active, instead of enable.
>>
>>> +                       num_enabled_crtc++;
>>> +       }
>>> +
>>> +       /* if enable two vop, need to disable dmc */
>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>> +               if (vop->devfreq_event_dev)
>>> +                       devfreq_event_disable_edev(vop->devfreq_event_dev);
>>> +               devfreq_suspend_device(vop->devfreq);
>>> +       }
>>
>> This really feels like something that should be handled somewhere
>> else. I don't fully understand how this works, but it seems like this
>> dependency should be handled where it actually matters, rather than
>> building in a seemingly arbitrary restriction in the vop driver.
>>
>> Even if this is the best place for it, this needs to be refactored to
>> eliminate the races that exist now.
>>
>> Sean
>
> DMC transitions can't happen when there is more than one display. You
> would have to synchronize with more than one vblank on the DCF (which
> actually changes the DDR frequency), and that's probably not going to
> happen.
>
> If this code should be done somewhere else, it needs to still be able
> to check the number of displays and avoid race conditions with
> displays getting enabled or disabled. I don't know enough about DRM to
> know if that's possible outside of this code.
>

Yeah, so I suppose that's another race, since we aren't necessarily
guaranteed to be enabling/disabling the crtcs in the order this patch
assumes.

I think we can probably handle this better by doing this up a level
where we have a more global view of which vops are enabled/disabled
and the order in which they will be processed.

I'm working on this now, stay tuned.

Sean



>>
>>>         return 0;
>>>
>>>  err_disable_aclk:
>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>         clk_disable(vop->hclk);
>>>  err_put_pm_runtime:
>>>         pm_runtime_put_sync(vop->dev);
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>         return ret;
>>>  }
>>>
>>>  static void vop_crtc_disable(struct drm_crtc *crtc)
>>>  {
>>>         struct vop *vop = to_vop(crtc);
>>> +       int num_enabled_crtc = 0;
>>>         int i;
>>>
>>>         WARN_ON(vop->event);
>>>
>>>         /*
>>> +        * if in dmc scaling frequency process, wait until it finish
>>> +        * use 200ms as timeout time.
>>> +        */
>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>> +                               !vop->dmc_in_process, HZ / 5))
>>> +               dev_warn(vop->dev,
>>> +                        "Timeout waiting for dmc when vop disable\n");
>>> +
>>> +       vop->vop_switch_status = 1;
>>> +
>>> +       /*
>>>          * We need to make sure that all windows are disabled before we
>>>          * disable that crtc. Otherwise we might try to scan from a destroyed
>>>          * buffer later.
>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>>                 spin_unlock_irq(&crtc->dev->event_lock);
>>>
>>>                 crtc->state->event = NULL;
>>> +
>>> +       vop->vop_switch_status = 0;
>>> +       wake_up(&vop->wait_vop_switch_queue);
>>> +
>>> +       /* check how many VOPs in use now */
>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>> +               if (crtc->state->enable)
>>> +                       num_enabled_crtc++;
>>> +       }
>>> +
>>> +       /*
>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>> +        * change to 1 vop enabled  need to enable dmc again.
>>> +        */
>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>> +               if (vop->devfreq_event_dev)
>>> +                       devfreq_event_enable_edev(vop->devfreq_event_dev);
>>> +               devfreq_resume_device(vop->devfreq);
>>>         }
>>>  }
>>>
>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>>         struct drm_device *drm_dev = data;
>>>         struct vop *vop;
>>>         struct resource *res;
>>> +       struct devfreq *devfreq;
>>> +       struct devfreq_event_dev *event_dev;
>>>         size_t alloc_size;
>>>         int ret, irq;
>>>
>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>>                 return ret;
>>>
>>>         pm_runtime_enable(&pdev->dev);
>>> +
>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>> +       vop->vop_switch_status = 0;
>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>> +       vop->dmc_in_process = 0;
>>> +
>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>> +       if (IS_ERR(devfreq))
>>> +               goto out;
>>> +
>>> +       vop->devfreq = devfreq;
>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>> +
>>> +       event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>> +                                                     0);
>>> +       if (IS_ERR(event_dev))
>>> +               goto out;
>>> +
>>> +       vop->devfreq_event_dev = event_dev;
>>> +out:
>>>         return 0;
>>>  }
>>>
>>> --
>>> 2.6.6
>>>
dbasehore . Sept. 7, 2016, 10:21 p.m. UTC | #11
On Wed, Sep 7, 2016 at 1:31 PM, Sean Paul <seanpaul@google.com> wrote:
> On Wed, Sep 7, 2016 at 2:13 PM, dbasehore . <dbasehore@chromium.org> wrote:
>> On Tue, Sep 6, 2016 at 10:18 AM, Sean Paul <seanpaul@google.com> wrote:
>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang <hl@rock-chips.com> wrote:
>>>> when in ddr frequency scaling process, vop can not do enable or
>>>> disable operation, since in dcf we check vop clock to see whether
>>>> vop work. If vop work, dcf do ddr frequency scaling when vop
>>>> in vblank status, and we need to read vop register to check whether
>>>> vop go into vblank status. If vop not work, dcf can do ddr frequency
>>>> any time. So when do ddr frequency scaling, you disabled or enable
>>>> vop, there may two bad thing happen: 1, the panel flicker(when vop from
>>>> disable status change to enable). 2, kernel hang (when vop from enable
>>>> status change to disable, dcf need to read vblank status, but if you disable
>>>> vop clock, it can not get the status, it will lead soc dead) So we need
>>>> register to devfreq notifier, and we can get the dmc status. Also, when
>>>> there have two vop enabled, we need to disable dmc, since dcf only base
>>>> on one vop vblank time, so the other panel will flicker when do ddr
>>>> frequency scaling.
>>>>
>>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>> ---
>>>> Changes in v10:
>>>> - None
>>>>
>>>> Changes in v9:
>>>> - None
>>>>
>>>> Changes in v8:
>>>> - None
>>>>
>>>> Changes in v7:
>>>> - None
>>>>
>>>> Changes in v6:
>>>> - fix a build error
>>>>
>>>> Changes in v5:
>>>> - improve some nits
>>>>
>>>> Changes in v4:
>>>> - register notifier to devfreq_register_notifier
>>>> - use DEVFREQ_PRECHANGE and DEVFREQ_POSTCHANGE to get dmc status
>>>> - when two vop enable, disable dmc
>>>> - when two vop back to one vop, enable dmc
>>>>
>>>> Changes in v3:
>>>> - when do vop eanble/disable, dmc will wait until it finish
>>>>
>>>> Changes in v2:
>>>> - None
>>>>
>>>> Changes in v1:
>>>> - use wait_event instead usleep
>>>>
>>>>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 116 ++++++++++++++++++++++++++++
>>>>  1 file changed, 116 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> index efbc41a..a73f3aa 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>>> @@ -19,6 +19,8 @@
>>>>  #include <drm/drm_crtc_helper.h>
>>>>  #include <drm/drm_plane_helper.h>
>>>>
>>>> +#include <linux/devfreq.h>
>>>> +#include <linux/devfreq-event.h>
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/module.h>
>>>>  #include <linux/platform_device.h>
>>>> @@ -118,6 +120,13 @@ struct vop {
>>>>
>>>>         const struct vop_data *data;
>>>>
>>>> +       struct devfreq *devfreq;
>>>> +       struct devfreq_event_dev *devfreq_event_dev;
>>>> +       struct notifier_block dmc_nb;
>>>> +       int dmc_in_process;
>>>> +       int vop_switch_status;
>>>> +       wait_queue_head_t wait_dmc_queue;
>>>> +       wait_queue_head_t wait_vop_switch_queue;
>>>>         uint32_t *regsbak;
>>>>         void __iomem *regs;
>>>>
>>>> @@ -428,11 +437,47 @@ static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
>>>>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>>  }
>>>>
>>>> +static int dmc_notify(struct notifier_block *nb, unsigned long event,
>>>> +                     void *data)
>>>> +{
>>>> +       struct vop *vop = container_of(nb, struct vop, dmc_nb);
>>>> +
>>>> +       if (event == DEVFREQ_PRECHANGE) {
>>>> +               /*
>>>> +                * check if vop in enable or disable process,
>>>> +                * if yes, wait until it finishes, use 200ms as
>>>> +                * timeout.
>>>> +                */
>>>> +               if (!wait_event_timeout(vop->wait_vop_switch_queue,
>>>> +                                       !vop->vop_switch_status, HZ / 5))
>>>> +                       dev_warn(vop->dev,
>>>> +                                "Timeout waiting for vop swtich status\n");
>>>> +               vop->dmc_in_process = 1;
>>>> +       } else if (event == DEVFREQ_POSTCHANGE) {
>>>> +               vop->dmc_in_process = 0;
>>>> +               wake_up(&vop->wait_dmc_queue);
>>>> +       }
>>>> +
>>>> +       return NOTIFY_OK;
>>>> +}
>>>> +
>>>>  static int vop_enable(struct drm_crtc *crtc)
>>>>  {
>>>>         struct vop *vop = to_vop(crtc);
>>>> +       int num_enabled_crtc = 0;
>>>>         int ret;
>>>>
>>>> +       /*
>>>> +        * if in dmc scaling frequency process, wait until it finishes
>>>> +        * use 200ms as timeout time.
>>>> +        */
>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>> +               dev_warn(vop->dev,
>>>> +                        "Timeout waiting for dmc when vop enable\n");
>>>> +
>>>
>>>
>>> This wait_event_timeout code is terribly racey (same goes above and below).
>>>
>>>
>>>> +       vop->vop_switch_status = 1;
>>>> +
>>>>         ret = pm_runtime_get_sync(vop->dev);
>>>>         if (ret < 0) {
>>>>                 dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
>>>> @@ -479,6 +524,21 @@ static int vop_enable(struct drm_crtc *crtc)
>>>>
>>>>         drm_crtc_vblank_on(crtc);
>>>>
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>> +
>>>> +       /* check how many VOPs in use now */
>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>> +               if (crtc->state->enable)
>>>
>>> I think you really want to check active, instead of enable.
>>>
>>>> +                       num_enabled_crtc++;
>>>> +       }
>>>> +
>>>> +       /* if enable two vop, need to disable dmc */
>>>> +       if ((num_enabled_crtc > 1) && vop->devfreq) {
>>>> +               if (vop->devfreq_event_dev)
>>>> +                       devfreq_event_disable_edev(vop->devfreq_event_dev);
>>>> +               devfreq_suspend_device(vop->devfreq);
>>>> +       }
>>>
>>> This really feels like something that should be handled somewhere
>>> else. I don't fully understand how this works, but it seems like this
>>> dependency should be handled where it actually matters, rather than
>>> building in a seemingly arbitrary restriction in the vop driver.
>>>
>>> Even if this is the best place for it, this needs to be refactored to
>>> eliminate the races that exist now.
>>>
>>> Sean
>>
>> DMC transitions can't happen when there is more than one display. You
>> would have to synchronize with more than one vblank on the DCF (which
>> actually changes the DDR frequency), and that's probably not going to
>> happen.
>>
>> If this code should be done somewhere else, it needs to still be able
>> to check the number of displays and avoid race conditions with
>> displays getting enabled or disabled. I don't know enough about DRM to
>> know if that's possible outside of this code.
>>
>
> Yeah, so I suppose that's another race, since we aren't necessarily
> guaranteed to be enabling/disabling the crtcs in the order this patch
> assumes.
>
> I think we can probably handle this better by doing this up a level
> where we have a more global view of which vops are enabled/disabled
> and the order in which they will be processed.
>
> I'm working on this now, stay tuned.
>
> Sean
>

Can there be concurrent calls to vop enable/disable for different
displays? If so, we could also hit a race between calculating
num_enabled_crtc and calling devfreq_suspend_device. That section
might have to be protected by a lock.

>
>
>>>
>>>>         return 0;
>>>>
>>>>  err_disable_aclk:
>>>> @@ -489,17 +549,31 @@ err_disable_hclk:
>>>>         clk_disable(vop->hclk);
>>>>  err_put_pm_runtime:
>>>>         pm_runtime_put_sync(vop->dev);
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>>         return ret;
>>>>  }
>>>>
>>>>  static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>  {
>>>>         struct vop *vop = to_vop(crtc);
>>>> +       int num_enabled_crtc = 0;
>>>>         int i;
>>>>
>>>>         WARN_ON(vop->event);
>>>>
>>>>         /*
>>>> +        * if in dmc scaling frequency process, wait until it finish
>>>> +        * use 200ms as timeout time.
>>>> +        */
>>>> +       if (!wait_event_timeout(vop->wait_dmc_queue,
>>>> +                               !vop->dmc_in_process, HZ / 5))
>>>> +               dev_warn(vop->dev,
>>>> +                        "Timeout waiting for dmc when vop disable\n");
>>>> +
>>>> +       vop->vop_switch_status = 1;
>>>> +
>>>> +       /*
>>>>          * We need to make sure that all windows are disabled before we
>>>>          * disable that crtc. Otherwise we might try to scan from a destroyed
>>>>          * buffer later.
>>>> @@ -555,6 +629,24 @@ static void vop_crtc_disable(struct drm_crtc *crtc)
>>>>                 spin_unlock_irq(&crtc->dev->event_lock);
>>>>
>>>>                 crtc->state->event = NULL;
>>>> +
>>>> +       vop->vop_switch_status = 0;
>>>> +       wake_up(&vop->wait_vop_switch_queue);
>>>> +
>>>> +       /* check how many VOPs in use now */
>>>> +       drm_for_each_crtc(crtc, vop->drm_dev) {
>>>> +               if (crtc->state->enable)
>>>> +                       num_enabled_crtc++;
>>>> +       }
>>>> +
>>>> +       /*
>>>> +        * if num_enabled_crtc = 1 now, it means 2 vop enabled
>>>> +        * change to 1 vop enabled  need to enable dmc again.
>>>> +        */
>>>> +       if ((num_enabled_crtc == 1) && vop->devfreq) {
>>>> +               if (vop->devfreq_event_dev)
>>>> +                       devfreq_event_enable_edev(vop->devfreq_event_dev);
>>>> +               devfreq_resume_device(vop->devfreq);

This is the section I'm referring to.

>>>>         }
>>>>  }
>>>>
>>>> @@ -1413,6 +1505,8 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>>>         struct drm_device *drm_dev = data;
>>>>         struct vop *vop;
>>>>         struct resource *res;
>>>> +       struct devfreq *devfreq;
>>>> +       struct devfreq_event_dev *event_dev;
>>>>         size_t alloc_size;
>>>>         int ret, irq;
>>>>
>>>> @@ -1474,6 +1568,28 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>>>                 return ret;
>>>>
>>>>         pm_runtime_enable(&pdev->dev);
>>>> +
>>>> +       init_waitqueue_head(&vop->wait_vop_switch_queue);
>>>> +       vop->vop_switch_status = 0;
>>>> +       init_waitqueue_head(&vop->wait_dmc_queue);
>>>> +       vop->dmc_in_process = 0;
>>>> +
>>>> +       devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
>>>> +       if (IS_ERR(devfreq))
>>>> +               goto out;
>>>> +
>>>> +       vop->devfreq = devfreq;
>>>> +       vop->dmc_nb.notifier_call = dmc_notify;
>>>> +       devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
>>>> +                                 DEVFREQ_TRANSITION_NOTIFIER);
>>>> +
>>>> +       event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
>>>> +                                                     0);
>>>> +       if (IS_ERR(event_dev))
>>>> +               goto out;
>>>> +
>>>> +       vop->devfreq_event_dev = event_dev;
>>>> +out:
>>>>         return 0;
>>>>  }
>>>>
>>>> --
>>>> 2.6.6
>>>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index efbc41a..a73f3aa 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -19,6 +19,8 @@ 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_plane_helper.h>
 
+#include <linux/devfreq.h>
+#include <linux/devfreq-event.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -118,6 +120,13 @@  struct vop {
 
 	const struct vop_data *data;
 
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *devfreq_event_dev;
+	struct notifier_block dmc_nb;
+	int dmc_in_process;
+	int vop_switch_status;
+	wait_queue_head_t wait_dmc_queue;
+	wait_queue_head_t wait_vop_switch_queue;
 	uint32_t *regsbak;
 	void __iomem *regs;
 
@@ -428,11 +437,47 @@  static void vop_dsp_hold_valid_irq_disable(struct vop *vop)
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 }
 
+static int dmc_notify(struct notifier_block *nb, unsigned long event,
+		      void *data)
+{
+	struct vop *vop = container_of(nb, struct vop, dmc_nb);
+
+	if (event == DEVFREQ_PRECHANGE) {
+		/*
+		 * check if vop in enable or disable process,
+		 * if yes, wait until it finishes, use 200ms as
+		 * timeout.
+		 */
+		if (!wait_event_timeout(vop->wait_vop_switch_queue,
+					!vop->vop_switch_status, HZ / 5))
+			dev_warn(vop->dev,
+				 "Timeout waiting for vop swtich status\n");
+		vop->dmc_in_process = 1;
+	} else if (event == DEVFREQ_POSTCHANGE) {
+		vop->dmc_in_process = 0;
+		wake_up(&vop->wait_dmc_queue);
+	}
+
+	return NOTIFY_OK;
+}
+
 static int vop_enable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	int num_enabled_crtc = 0;
 	int ret;
 
+	/*
+	 * if in dmc scaling frequency process, wait until it finishes
+	 * use 200ms as timeout time.
+	 */
+	if (!wait_event_timeout(vop->wait_dmc_queue,
+				!vop->dmc_in_process, HZ / 5))
+		dev_warn(vop->dev,
+			 "Timeout waiting for dmc when vop enable\n");
+
+	vop->vop_switch_status = 1;
+
 	ret = pm_runtime_get_sync(vop->dev);
 	if (ret < 0) {
 		dev_err(vop->dev, "failed to get pm runtime: %d\n", ret);
@@ -479,6 +524,21 @@  static int vop_enable(struct drm_crtc *crtc)
 
 	drm_crtc_vblank_on(crtc);
 
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
+
+	/* check how many VOPs in use now */
+	drm_for_each_crtc(crtc, vop->drm_dev) {
+		if (crtc->state->enable)
+			num_enabled_crtc++;
+	}
+
+	/* if enable two vop, need to disable dmc */
+	if ((num_enabled_crtc > 1) && vop->devfreq) {
+		if (vop->devfreq_event_dev)
+			devfreq_event_disable_edev(vop->devfreq_event_dev);
+		devfreq_suspend_device(vop->devfreq);
+	}
 	return 0;
 
 err_disable_aclk:
@@ -489,17 +549,31 @@  err_disable_hclk:
 	clk_disable(vop->hclk);
 err_put_pm_runtime:
 	pm_runtime_put_sync(vop->dev);
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
 	return ret;
 }
 
 static void vop_crtc_disable(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
+	int num_enabled_crtc = 0;
 	int i;
 
 	WARN_ON(vop->event);
 
 	/*
+	 * if in dmc scaling frequency process, wait until it finish
+	 * use 200ms as timeout time.
+	 */
+	if (!wait_event_timeout(vop->wait_dmc_queue,
+				!vop->dmc_in_process, HZ / 5))
+		dev_warn(vop->dev,
+			 "Timeout waiting for dmc when vop disable\n");
+
+	vop->vop_switch_status = 1;
+
+	/*
 	 * We need to make sure that all windows are disabled before we
 	 * disable that crtc. Otherwise we might try to scan from a destroyed
 	 * buffer later.
@@ -555,6 +629,24 @@  static void vop_crtc_disable(struct drm_crtc *crtc)
 		spin_unlock_irq(&crtc->dev->event_lock);
 
 		crtc->state->event = NULL;
+
+	vop->vop_switch_status = 0;
+	wake_up(&vop->wait_vop_switch_queue);
+
+	/* check how many VOPs in use now */
+	drm_for_each_crtc(crtc, vop->drm_dev) {
+		if (crtc->state->enable)
+			num_enabled_crtc++;
+	}
+
+	/*
+	 * if num_enabled_crtc = 1 now, it means 2 vop enabled
+	 * change to 1 vop enabled  need to enable dmc again.
+	 */
+	if ((num_enabled_crtc == 1) && vop->devfreq) {
+		if (vop->devfreq_event_dev)
+			devfreq_event_enable_edev(vop->devfreq_event_dev);
+		devfreq_resume_device(vop->devfreq);
 	}
 }
 
@@ -1413,6 +1505,8 @@  static int vop_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm_dev = data;
 	struct vop *vop;
 	struct resource *res;
+	struct devfreq *devfreq;
+	struct devfreq_event_dev *event_dev;
 	size_t alloc_size;
 	int ret, irq;
 
@@ -1474,6 +1568,28 @@  static int vop_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 
 	pm_runtime_enable(&pdev->dev);
+
+	init_waitqueue_head(&vop->wait_vop_switch_queue);
+	vop->vop_switch_status = 0;
+	init_waitqueue_head(&vop->wait_dmc_queue);
+	vop->dmc_in_process = 0;
+
+	devfreq = devfreq_get_devfreq_by_phandle(dev, 0);
+	if (IS_ERR(devfreq))
+		goto out;
+
+	vop->devfreq = devfreq;
+	vop->dmc_nb.notifier_call = dmc_notify;
+	devfreq_register_notifier(vop->devfreq, &vop->dmc_nb,
+				  DEVFREQ_TRANSITION_NOTIFIER);
+
+	event_dev = devfreq_event_get_edev_by_phandle(vop->devfreq->dev.parent,
+						      0);
+	if (IS_ERR(event_dev))
+		goto out;
+
+	vop->devfreq_event_dev = event_dev;
+out:
 	return 0;
 }