Message-ID: <57CF1282.3000607@rock-chips.com>
Date:   Wed, 7 Sep 2016 03:01:22 +0800
From:   hl <hl@...k-chips.com>
To:     Sean Paul <seanpaul@...gle.com>
Cc:     Heiko Stübner <heiko@...ech.de>,
        ??? <myungjoo.ham@...sung.com>,
        姚智情 <mark.yao@...k-chips.com>,
        cw00.choi@...sung.com, Dave Airlie <airlied@...ux.ie>,
        Michael Turquette <mturquette@...libre.com>,
        dbasehore@...omium.org, Stephen Boyd <sboyd@...eaurora.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-rockchip@...ts.infradead.org,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Linux ARM Kernel <linux-arm-kernel@...ts.infradead.org>,
        Jon Medhurst <tixy@...aro.org>, typ@...k-chips.com,
        sudeep.holla@....com, Mark Rutland <mark.rutland@....com>,
        linux-pm@...r.kernel.org,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: Re: [PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver

Hi

On 2016年09月07日 02:55, Sean Paul wrote:
> On Tue, Sep 6, 2016 at 2:15 PM, hl <hl@...k-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@...k-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@...k-chips.com>
>>>> Reviewed-by: Chanwoo Choi <cw00.choi@...sung.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
>>
>>
>
>

-- 
Lin Huang

Powered by blists - more mailing lists