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 X-Spam-Level: X-Spam-Status: No, score=-10.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51A4AC43460 for ; Thu, 8 Apr 2021 23:16:33 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E19866115C for ; Thu, 8 Apr 2021 23:16:32 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E19866115C Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=somainline.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A55106E40D; Thu, 8 Apr 2021 23:16:31 +0000 (UTC) X-Greylist: delayed 101073 seconds by postgrey-1.36 at gabe; Thu, 08 Apr 2021 23:16:30 UTC Received: from relay01.th.seeweb.it (relay01.th.seeweb.it [5.144.164.162]) by gabe.freedesktop.org (Postfix) with ESMTPS id 779CF6E40D for ; Thu, 8 Apr 2021 23:16:30 +0000 (UTC) Received: from mail-vs1-f44.google.com (mail-vs1-f44.google.com [209.85.217.44]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by m-r1.th.seeweb.it (Postfix) with ESMTPSA id 1DBAA1FAF2; Fri, 9 Apr 2021 01:16:27 +0200 (CEST) Received: by mail-vs1-f44.google.com with SMTP id h20so2030576vsu.1; Thu, 08 Apr 2021 16:16:27 -0700 (PDT) X-Gm-Message-State: AOAM531mhwwacsLqMv6TukdjVx9+Z2hjRQNoGPyyOpHeLM1GjFPxA/62 uMUZNix8gztr3aDDQ5RuS7JUyn4+VsUS+rMwRqY= X-Google-Smtp-Source: ABdhPJw6EYN1JMtoqLeAzxdZC79DvwurUARvz2v73JW8EL5WAz2/XfMweZwCJD4zHKDB52Cr44AIVeAzm2ovWTShR8s= X-Received: by 2002:a67:7751:: with SMTP id s78mr6096451vsc.3.1617923786067; Thu, 08 Apr 2021 16:16:26 -0700 (PDT) MIME-Version: 1.0 References: <20210406214726.131534-1-marijn.suijten@somainline.org> <20210406214726.131534-2-marijn.suijten@somainline.org> <6413863d04df9743e2d7e81beff5c3e8@codeaurora.org> <04860f05-f79f-de0b-13d1-aba85065b4da@somainline.org> In-Reply-To: From: AngeloGioacchino Del Regno Date: Fri, 9 Apr 2021 01:16:13 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [Freedreno] [PATCH 1/3] drm/msm/mdp5: Configure PP_SYNC_HEIGHT to double the vtotal To: Rob Clark X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sean Paul , Sai Prakash Ranjan , David Airlie , linux-arm-msm , Konrad Dybcio , Linux Kernel Mailing List , Abhinav Kumar , Martin Botka , dri-devel , AngeloGioacchino Del Regno , Marijn Suijten , phone-devel@vger.kernel.org, freedreno , ~postmarketos/upstreaming@lists.sr.ht Content-Type: multipart/mixed; boundary="===============1642146574==" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --===============1642146574== Content-Type: multipart/alternative; boundary="0000000000008ad23305bf7e3c14" --0000000000008ad23305bf7e3c14 Content-Type: text/plain; charset="UTF-8" Il gio 8 apr 2021, 21:05 Rob Clark ha scritto: > On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino Del Regno > wrote: > > > > Il 07/04/21 20:19, abhinavk@codeaurora.org ha scritto: > > > Hi Marijn > > > > > > On 2021-04-06 14:47, Marijn Suijten wrote: > > >> Leaving this at a close-to-maximum register value 0xFFF0 means it > takes > > >> very long for the MDSS to generate a software vsync interrupt when the > > >> hardware TE interrupt doesn't arrive. Configuring this to double the > > >> vtotal (like some downstream kernels) leads to a frame to take at most > > >> twice before the vsync signal, until hardware TE comes up. > > >> > > >> In this case the hardware interrupt responsible for providing this > > >> signal - "disp-te" gpio - is not hooked up to the mdp5 vsync/pp logic > at > > >> all. This solves severe panel update issues observed on at least the > > >> Xperia Loire and Tone series, until said gpio is properly hooked up to > > >> an irq. > > > > > > The reason the CONFIG_HEIGHT was at such a high value is to make sure > that > > > we always get the TE only from the panel vsync and not false positives > > > coming > > > from the tear check logic itself. > > > > > > When you say that disp-te gpio is not hooked up, is it something > > > incorrect with > > > the schematic OR panel is not generating the TE correctly? > > > > > > > Sometimes, some panels aren't getting correctly configured by the > > OEM/ODM in the first place: especially when porting devices from > > downstream to upstream, developers often get in a situation in which > > their TE line is either misconfigured or the DriverIC is not configured > > to raise V-Sync interrupts. > > Please remember: some DDICs need a "commands sequence" to enable > > generating the TE interrupts, sometimes this is not standard, and > > sometimes OEMs/ODMs are not even doing that in their downstream code > > (but instead they work around it in creative ways "for reasons", even > > though their DDIC supports indeed sending TE events). > > > > This mostly happens when bringing up devices that have autorefresh > > enabled from the bootloader (when the bootloader sets up the splash > > screen) by using simple-panel as a (hopefully) temporary solution to get > > through the initial stages of porting. > > > > We are not trying to cover cases related to incorrect schematics or > > hardware mistakes here, as the fix for that - as you know - is to just > > fix your hardware. > > What we're trying to do here is to stop freezes and, in some cases, > > lockups, other than false positives making the developer go offroad when > > the platform shows that something is wrong during early porting. > > > > Also, sometimes, some DDICs will not generate TE interrupts when > > expected... in these cases we get a PP timeout and a MDP5 recovery: this > > is totally avoidable if we rely on the 2*vtotal, as we wouldn't get > > through the very time consuming task of recovering the entire MDP. > > > > Of course, if something is wrong in the MDP and the block really needs > > recovery, this "trick" won't save anyone and the recovery will anyway be > > triggered, as the PP-done will anyway timeout. > > So, is this (mostly) a workaround due to TE not wired up? In which > case I think it is ok, but maybe should have a comment about the > interaction with TE? > Mostly, yes. > Currently I have this patch in msm-next-staging but I guess we need to > decide in the next day or so whether to drop it or smash in a comment? > > BR, > -R > Marijn, can you please urgently throw a comment in, reminding that these timers are interacting with TE and send a fast V2? Cheers, - Angelo > > >> > > >> Suggested-by: AngeloGioacchino Del Regno > > >> > > >> Signed-off-by: Marijn Suijten > > >> Reviewed-by: AngeloGioacchino Del Regno > > >> > > >> --- > > >> drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > > >> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > > >> index ff2c1d583c79..2d5ac03dbc17 100644 > > >> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > > >> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c > > >> @@ -51,7 +51,7 @@ static int pingpong_tearcheck_setup(struct > > >> drm_encoder *encoder, > > >> > > >> mdp5_write(mdp5_kms, REG_MDP5_PP_SYNC_CONFIG_VSYNC(pp_id), cfg); > > >> mdp5_write(mdp5_kms, > > >> - REG_MDP5_PP_SYNC_CONFIG_HEIGHT(pp_id), 0xfff0); > > >> + REG_MDP5_PP_SYNC_CONFIG_HEIGHT(pp_id), (2 * mode->vtotal)); > > >> mdp5_write(mdp5_kms, > > >> REG_MDP5_PP_VSYNC_INIT_VAL(pp_id), mode->vdisplay); > > >> mdp5_write(mdp5_kms, REG_MDP5_PP_RD_PTR_IRQ(pp_id), > > >> mode->vdisplay + 1); > --0000000000008ad23305bf7e3c14 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

Il gio 8 apr 2021, 21:05 Rob Clark <robdclark@gmail.com> ha scritto:
On Wed, Apr 7, 2021 at 12:11 PM AngeloGioacchino D= el Regno
<angelogioacchino.delregno@somainline.org> = wrote:
>
> Il 07/04/21 20:19, abhinavk@codeaurora.org ha scritto:
> > Hi Marijn
> >
> > On 2021-04-06 14:47, Marijn Suijten wrote:
> >> Leaving this at a close-to-maximum register value 0xFFF0 mean= s it takes
> >> very long for the MDSS to generate a software vsync interrupt= when the
> >> hardware TE interrupt doesn't arrive.=C2=A0 Configuring t= his to double the
> >> vtotal (like some downstream kernels) leads to a frame to tak= e at most
> >> twice before the vsync signal, until hardware TE comes up. > >>
> >> In this case the hardware interrupt responsible for providing= this
> >> signal - "disp-te" gpio - is not hooked up to the m= dp5 vsync/pp logic at
> >> all.=C2=A0 This solves severe panel update issues observed on= at least the
> >> Xperia Loire and Tone series, until said gpio is properly hoo= ked up to
> >> an irq.
> >
> > The reason the CONFIG_HEIGHT was at such a high value is to make = sure that
> > we always get the TE only from the panel vsync and not false posi= tives
> > coming
> > from the tear check logic itself.
> >
> > When you say that disp-te gpio is not hooked up, is it something<= br> > > incorrect with
> > the schematic OR panel is not generating the TE correctly?
> >
>
> Sometimes, some panels aren't getting correctly configured by the<= br> > OEM/ODM in the first place: especially when porting devices from
> downstream to upstream, developers often get in a situation in which > their TE line is either misconfigured or the DriverIC is not configure= d
> to raise V-Sync interrupts.
> Please remember: some DDICs need a "commands sequence" to en= able
> generating the TE interrupts, sometimes this is not standard, and
> sometimes OEMs/ODMs are not even doing that in their downstream code > (but instead they work around it in creative ways "for reasons&qu= ot;, even
> though their DDIC supports indeed sending TE events).
>
> This mostly happens when bringing up devices that have autorefresh
> enabled from the bootloader (when the bootloader sets up the splash > screen) by using simple-panel as a (hopefully) temporary solution to g= et
> through the initial stages of porting.
>
> We are not trying to cover cases related to incorrect schematics or > hardware mistakes here, as the fix for that - as you know - is to just=
> fix your hardware.
> What we're trying to do here is to stop freezes and, in some cases= ,
> lockups, other than false positives making the developer go offroad wh= en
> the platform shows that something is wrong during early porting.
>
> Also, sometimes, some DDICs will not generate TE interrupts when
> expected... in these cases we get a PP timeout and a MDP5 recovery: th= is
> is totally avoidable if we rely on the 2*vtotal, as we wouldn't ge= t
> through the very time consuming task of recovering the entire MDP.
>
> Of course, if something is wrong in the MDP and the block really needs=
> recovery, this "trick" won't save anyone and the recover= y will anyway be
> triggered, as the PP-done will anyway timeout.

So, is this (mostly) a workaround due to TE not wired up?=C2=A0 In which case I think it is ok, but maybe should have a comment about the
interaction with TE?

Mostly, yes.=C2=A0


Currently I have this patch in msm-next-staging but I guess we need to
decide in the next day or so whether to drop it or smash in a comment?

BR,
-R

Marijn, can you please urgently throw a comment= in, reminding that these timers are interacting with TE and send a fast V2= ?=C2=A0

Cheers,=C2=A0
=C2=A0- Angelo


> >>
> >> Suggested-by: AngeloGioacchino Del Regno
> >> <angelogioacchino.delregno@somainlin= e.org>
> >> Signed-off-by: Marijn Suijten <marijn.suijten@s= omainline.org>
> >> Reviewed-by: AngeloGioacchino Del Regno
> >> <angelogioacchino.delregno@somainlin= e.org>
> >> ---
> >>=C2=A0 drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c | 2 +-=
> >>=C2=A0 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c=
> >> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> >> index ff2c1d583c79..2d5ac03dbc17 100644
> >> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> >> @@ -51,7 +51,7 @@ static int pingpong_tearcheck_setup(struct<= br> > >> drm_encoder *encoder,
> >>
> >>=C2=A0 =C2=A0 =C2=A0 mdp5_write(mdp5_kms, REG_MDP5_PP_SYNC_CON= FIG_VSYNC(pp_id), cfg);
> >>=C2=A0 =C2=A0 =C2=A0 mdp5_write(mdp5_kms,
> >> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 REG_MDP5_PP_SYNC_CONFIG_HEIGHT(p= p_id), 0xfff0);
> >> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 REG_MDP5_PP_SYNC_CONFIG_HEIGHT(p= p_id), (2 * mode->vtotal));
> >>=C2=A0 =C2=A0 =C2=A0 mdp5_write(mdp5_kms,
> >>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 REG_MDP5_PP_VSYNC_INIT_VAL(= pp_id), mode->vdisplay);
> >>=C2=A0 =C2=A0 =C2=A0 mdp5_write(mdp5_kms, REG_MDP5_PP_RD_PTR_I= RQ(pp_id),
> >> mode->vdisplay + 1);
--0000000000008ad23305bf7e3c14-- --===============1642146574== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1642146574==--