From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EB8E0C433F5 for ; Tue, 28 Sep 2021 01:33:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D00F26120E for ; Tue, 28 Sep 2021 01:33:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238430AbhI1Be6 (ORCPT ); Mon, 27 Sep 2021 21:34:58 -0400 Received: from m43-7.mailgun.net ([69.72.43.7]:38673 "EHLO m43-7.mailgun.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238428AbhI1Be6 (ORCPT ); Mon, 27 Sep 2021 21:34:58 -0400 DKIM-Signature: a=rsa-sha256; v=1; c=relaxed/relaxed; d=mg.codeaurora.org; q=dns/txt; s=smtp; t=1632792799; h=Message-ID: References: In-Reply-To: Subject: Cc: To: From: Date: Content-Transfer-Encoding: Content-Type: MIME-Version: Sender; bh=9gcoDqJsrt5Dtswh7Yn7D8hyS/1K39L+H/Oj3ZbCDO8=; b=OGkpGKTsVd5H4WFmLscpNyXY1OEenXqKvwizGZTyGYnHYlY1hMFn4o0G4f+esUivffMiY2LQ U5QRlgKBDkHjWJ4P9sbysh7xEvmKNbW4jc3HyDScYmg4C6sRQt+V3ChxtvFI2XnhEUJ/S3oU 91W5IpTLYQH5bqHJ506PNq0fQ/o= X-Mailgun-Sending-Ip: 69.72.43.7 X-Mailgun-Sid: WyI1MzIzYiIsICJsaW51eC1hcm0tbXNtQHZnZXIua2VybmVsLm9yZyIsICJiZTllNGEiXQ== Received: from smtp.codeaurora.org (ec2-35-166-182-171.us-west-2.compute.amazonaws.com [35.166.182.171]) by smtp-out-n06.prod.us-east-1.postgun.com with SMTP id 615270d7713d5d6f960695e6 (version=TLS1.2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256); Tue, 28 Sep 2021 01:33:11 GMT Sender: abhinavk=codeaurora.org@mg.codeaurora.org Received: by smtp.codeaurora.org (Postfix, from userid 1001) id 137CFC43616; Tue, 28 Sep 2021 01:33:10 +0000 (UTC) Received: from mail.codeaurora.org (localhost.localdomain [127.0.0.1]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: abhinavk) by smtp.codeaurora.org (Postfix) with ESMTPSA id 6FECFC4338F; Tue, 28 Sep 2021 01:33:09 +0000 (UTC) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Mon, 27 Sep 2021 18:33:09 -0700 From: abhinavk@codeaurora.org To: Dmitry Baryshkov Cc: Bjorn Andersson , Rob Clark , Sean Paul , Jonathan Marek , Stephen Boyd , David Airlie , Daniel Vetter , "open list:DRM DRIVER FOR MSM ADRENO GPU" , "open list:DRM DRIVER FOR MSM ADRENO GPU" , freedreno , David Heidelberg Subject: Re: [Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host In-Reply-To: <45473d71-5986-7299-336a-ed498160fce4@linaro.org> References: <20210921162258.1858223-1-dmitry.baryshkov@linaro.org> <0c275df228a1925e43a4dc59ceeab6b7@codeaurora.org> <8c1e44cf44f917d38fa7133b869047b0@codeaurora.org> <7512b299-106f-2ffa-6d4f-46dc195abb84@linaro.org> <8060e6fd83d521ed14785ea66386337b@codeaurora.org> <1ebb9efd461e9a84027ea63f7141a208@codeaurora.org> <45473d71-5986-7299-336a-ed498160fce4@linaro.org> Message-ID: <2a64ac4f1ef77d2e525486d411c9142b@codeaurora.org> X-Sender: abhinavk@codeaurora.org User-Agent: Roundcube Webmail/1.3.9 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 2021-09-27 18:29, Dmitry Baryshkov wrote: > On 28/09/2021 04:19, abhinavk@codeaurora.org wrote: >> On 2021-09-27 18:06, Dmitry Baryshkov wrote: >>> On Tue, 28 Sept 2021 at 03:22, wrote: >>>> >>>> On 2021-09-25 12:43, Dmitry Baryshkov wrote: >>>> > On 21/09/2021 23:52, abhinavk@codeaurora.org wrote: >>>> >> On 2021-09-21 10:47, Dmitry Baryshkov wrote: >>>> >>> Hi, >>>> >>> >>>> >>> On Tue, 21 Sept 2021 at 20:01, wrote: >>>> >>>> >>>> >>>> On 2021-09-21 09:22, Dmitry Baryshkov wrote: >>>> >>>> > The DSI host might be left in some state by the bootloader. If this >>>> >>>> > state generates an IRQ, it might hang the system by holding the >>>> >>>> > interrupt line before the driver sets up the DSI host to the known >>>> >>>> > state. >>>> >>>> > >>>> >>>> > Move the request/free_irq calls into msm_dsi_host_power_on/_off calls, >>>> >>>> > so that we can be sure that the interrupt is delivered when the host is >>>> >>>> > in the known state. >>>> >>>> > >>>> >>>> > Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support") >>>> >>>> > Signed-off-by: Dmitry Baryshkov >>>> >>>> >>>> >>>> This is a valid change and we have seen interrupt storms in >>>> >>>> downstream >>>> >>>> happening >>>> >>>> when like you said the bootloader leaves the DSI host in unknown >>>> >>>> state. >>>> >>>> Just one question below. >>>> >>>> >>>> >>>> > --- >>>> >>>> >  drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++++++++++++--------- >>>> >>>> >  1 file changed, 12 insertions(+), 9 deletions(-) >>>> >>>> > >>>> >>>> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> >>>> > b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> >>>> > index e269df285136..cd842347a6b1 100644 >>>> >>>> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> >>>> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c >>>> >>>> > @@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct >>>> >>>> > mipi_dsi_host *host, >>>> >>>> >               return ret; >>>> >>>> >       } >>>> >>>> > >>>> >>>> > -     ret = devm_request_irq(&pdev->dev, msm_host->irq, >>>> >>>> > -                     dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >>>> >>>> > -                     "dsi_isr", msm_host); >>>> >>>> > -     if (ret < 0) { >>>> >>>> > -             DRM_DEV_ERROR(&pdev->dev, "failed to request IRQ%u: %d\n", >>>> >>>> > -                             msm_host->irq, ret); >>>> >>>> > -             return ret; >>>> >>>> > -     } >>>> >>>> > - >>>> >>>> >       msm_host->dev = dev; >>>> >>>> >       ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K); >>>> >>>> >       if (ret) { >>>> >>>> > @@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct mipi_dsi_host >>>> >>>> > *host, >>>> >>>> >       if (msm_host->disp_en_gpio) >>>> >>>> >               gpiod_set_value(msm_host->disp_en_gpio, 1); >>>> >>>> > >>>> >>>> > +     ret = devm_request_irq(&msm_host->pdev->dev, msm_host->irq, >>>> >>>> > +                     dsi_host_irq, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >>>> >>>> > +                     "dsi_isr", msm_host); >>>> >>>> > +     if (ret < 0) { >>>> >>>> > +             DRM_DEV_ERROR(&msm_host->pdev->dev, "failed to request IRQ%u: %d\n", >>>> >>>> > +                             msm_host->irq, ret); >>>> >>>> > +             return ret; >>>> >>>> > +     } >>>> >>>> > + >>>> >>>> > + >>>> >>>> >>>> >>>> Do you want to move this to msm_dsi_host_enable()? >>>> >>>> So without the controller being enabled it is still in unknown >>>> >>>> state? >>>> >>> >>>> >>> msm_dsi_host_power_on() reconfigures the host registers, so the state >>>> >>> is known at the end of the power_on(). >>>> >>> >>>> >>>> Also do you want to do this after dsi0 and dsi1 are initialized to >>>> >>>> account for >>>> >>>> dual dsi cases? >>>> >>> >>>> >>> I don't think this should matter. The host won't generate 'extra' >>>> >>> interrupts in such case, will it? >>>> >>> >>>> >> We have seen cases where misconfiguration has caused interrupts to >>>> >> storm only >>>> >> on one DSI in some cases. So yes, I would prefer this is done after >>>> >> both are >>>> >> configured. >>>> > >>>> > I've checked. The power_on is called from dsi_mgr_bridge_pre_enable() >>>> > when both DSI hosts should be bound. >>>> >>>> DSI being bound is enough? I thought the issue we are trying to >>>> address >>>> is that >>>> we need to have called msm_dsi_host_power_on() for both the hosts so >>>> that both are >>>> put in the known state before requesting the irq. >>>> >>>> OR in other words move the irq_enable() to below location. >>>> >>>> 341 static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) >>>> 342 { >>>> ******************************** >>>> 364     ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], >>>> is_bonded_dsi, msm_dsi->phy); >>>> 365     if (ret) { >>>> 366             pr_err("%s: power on host %d failed, %d\n", >>>> __func__, id, ret); >>>> 367             goto host_on_fail; >>>> 368     } >>>> 369 >>>> 370     if (is_bonded_dsi && msm_dsi1) { >>>> 371             ret = msm_dsi_host_power_on(msm_dsi1->host, >>>> 372                             &phy_shared_timings[DSI_1], >>>> is_bonded_dsi, msm_dsi1->phy); >>>> 373             if (ret) { >>>> 374                     pr_err("%s: power on host1 failed, %d\n", >>>> 375                                                     __func__, >>>> ret); >>>> 376                     goto host1_on_fail; >>>> 377             } >>>> 378     } >>>> >>>> < move the irq enable here > >>>> ********************************** >>> >>> Ah, I see your point. What about moving to msm_dsi_host_enable() >>> then? >> >> Yes, I had suggested this a few replies ago. But only at the dsi_msgr >> we know if DSI1 is also done. >> So you can do it right after it in below location? >> >> 427     if (is_dual_dsi && msm_dsi1) { >> 428         ret = msm_dsi_host_enable(msm_dsi1->host); >> 429         if (ret) { >> 430             pr_err("%s: enable host1 failed, %d\n", __func__, >> ret); >> 431             goto host1_en_fail; >> 432         } >> 433     } >> >> > > If there is DSI1, it was also powered on/programmed at the time of > msm_dsi_host_enable, so enabling IRQs from it should be safe. Do you > see any pitfalls from enabling the irq from that function? Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 are powered on But for DSI1, we will enable it when its powered ON but not enabled. Hence i thought its better this way. > >> >>> >>>> >>>> >       msm_host->power_on = true; >>>> >>>> >       mutex_unlock(&msm_host->dev_mutex); >>>> >>>> > >>>> >>>> > @@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct mipi_dsi_host >>>> >>>> > *host) >>>> >>>> >               goto unlock_ret; >>>> >>>> >       } >>>> >>>> > >>>> >>>> > +     devm_free_irq(&msm_host->pdev->dev, msm_host->irq, msm_host); >>>> >>>> > + >>>> >>>> >       dsi_ctrl_config(msm_host, false, NULL, NULL); >>>> >>>> > >>>> >>>> >       if (msm_host->disp_en_gpio)