Re: [PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver

[Sean Paul <seanpaul@google.com>]

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
>