From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938621AbcIFTIl (ORCPT ); Tue, 6 Sep 2016 15:08:41 -0400 Received: from mail-yw0-f176.google.com ([209.85.161.176]:34041 "EHLO mail-yw0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965041AbcIFTIX (ORCPT ); Tue, 6 Sep 2016 15:08:23 -0400 MIME-Version: 1.0 In-Reply-To: <57CF1282.3000607@rock-chips.com> References: <1473051971-5500-1-git-send-email-hl@rock-chips.com> <1473051971-5500-6-git-send-email-hl@rock-chips.com> <57CF07C4.30508@rock-chips.com> <57CF1282.3000607@rock-chips.com> From: Sean Paul Date: Tue, 6 Sep 2016 15:07:56 -0400 Message-ID: Subject: Re: [PATCH v10 5/5] drm/rockchip: Add dmc notifier in vop driver To: hl Cc: =?UTF-8?Q?Heiko_St=C3=BCbner?= , "???" , =?UTF-8?B?5aea5pm65oOF?= , cw00.choi@samsung.com, Dave Airlie , Michael Turquette , dbasehore@chromium.org, Stephen Boyd , Linux Kernel Mailing List , dri-devel , Douglas Anderson , linux-rockchip@lists.infradead.org, Kyungmin Park , Linux ARM Kernel , Jon Medhurst , typ@rock-chips.com, sudeep.holla@arm.com, Mark Rutland , linux-pm@vger.kernel.org, "devicetree@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u86J8iB9011323 On Tue, Sep 6, 2016 at 3:01 PM, hl wrote: > Hi > > > On 2016年09月07日 02:55, Sean Paul wrote: >> >> On Tue, Sep 6, 2016 at 2:15 PM, hl wrote: >>> >>> Hi Sean, >>> >>> >>> On 2016年09月07日 01:18, Sean Paul wrote: >>>> >>>> On Mon, Sep 5, 2016 at 1:06 AM, Lin Huang 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 >>>>> Reviewed-by: Chanwoo Choi >>>>> --- >>>>> 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 >>>>> #include >>>>> >>>>> +#include >>>>> +#include >>>>> #include >>>>> #include >>>>> #include >>>>> @@ -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 > >