| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <06b399a14a0a42d7d9a49d546e2f499f03106bdf.camel@icenowy.me>
Date: Mon, 02 Sep 2024 21:39:24 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: Matt Coster <Matt.Coster@...tec.com>, "dri-devel@...ts.freedesktop.org"
<dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Cc: Frank Binns <Frank.Binns@...tec.com>, Maarten Lankhorst
<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] drm/imagination: properly support stopping LAYOUT_MARS
= 1 cores
在 2024-09-02星期一的 09:24 +0000，Matt Coster写道：
> On 11/08/2024 09:28, Icenowy Zheng wrote:
> > Some new Rogue GPU cores have an extra MARS power domain, which
> > controlls the power of the firmware core and allows the firmware
> > core to
> > power down most parts of the GPU.
> >
> > Adapt to this by ignoring power domains that should be powered down
> > by
> > the fiwmare and checking MARS idle status instead.
> >
> > The logic mimics RGXStop() function in the DDK kernel mode source
> > code.
> >
> > Tested on BXE-4-32 (36.50.54.182) with firmware build 6503725 OS
> > provided
> > by Imagination Technologies.
> >
> > Signed-off-by: Icenowy Zheng <uwu@...nowy.me>
>
> Hi Icenowy,
>
> Thanks for the patch! It's great to see people getting involved in
> getting this driver working on more platforms.
>
> > ---
> > .../gpu/drm/imagination/pvr_fw_startstop.c | 49 +++++++++++++--
> > ----
> > 1 file changed, 35 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> > b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> > index 159a4c3dd65b..4301a7aded64 100644
> > --- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> > +++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c
> > @@ -201,19 +201,28 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
> >
> > ~(ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN |
> >
> > ROGUE_CR_SIDEKICK_IDLE_SOCIF_EN |
> >
> > ROGUE_CR_SIDEKICK_IDLE_HOSTIF_EN);
> > + const u32 mars_idle_mask = ROGUE_CR_MARS_IDLE_CPU_EN |
> > +
> > ROGUE_CR_MARS_IDLE_MH_SYSARB0_EN;
> > bool skip_garten_idle = false;
> > + u32 mars = 0;
> > u32 reg_value;
> > int err;
> >
> > + if (PVR_HAS_FEATURE(pvr_dev, layout_mars))
> > + PVR_FEATURE_VALUE(pvr_dev, layout_mars, &mars);
> > +
>
> This check is unnecessary – the PVR_FEATURE_VALUE() macro already
> checks
> for the feature presence internally and doesn't change the output
> value
> if it's not present.
>
> > /*
> > * Wait for Sidekick/Jones to signal IDLE except for the
> > Garten Wrapper.
> > * For cores with the LAYOUT_MARS feature, SIDEKICK would
> > have been
> > * powered down by the FW.
> > */
> > - err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
> > sidekick_idle_mask,
> > - sidekick_idle_mask,
> > POLL_TIMEOUT_USEC);
> > - if (err)
> > - return err;
> > + if (mars) {
>
> I think you might have these conditionals backwards. This is saying
> we
> need to touch sidekick iff mars is present, but the comments say this
> is
> the other way around (mars takes care of powering sidekick, so we
> don't
> need to).
Thanks for this tip. I am thinking I got this wrong too, so I am going
to change this.
>
> Cheers,
> Matt
>
> > + err = pvr_cr_poll_reg32(pvr_dev,
> > ROGUE_CR_SIDEKICK_IDLE,
> > + sidekick_idle_mask,
> > + sidekick_idle_mask,
> > POLL_TIMEOUT_USEC);
> > + if (err)
> > + return err;
> > + }
> >
> > /* Unset MTS DM association with threads. */
> > pvr_cr_write32(pvr_dev,
> > ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC,
> > @@ -267,21 +276,27 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
> > * For cores with the LAYOUT_MARS feature, SLC would have
> > been powered
> > * down by the FW.
> > */
> > - err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
> > - ROGUE_CR_SLC_IDLE_MASKFULL,
> > - ROGUE_CR_SLC_IDLE_MASKFULL,
> > POLL_TIMEOUT_USEC);
> > - if (err)
> > - return err;
> > + if (mars) {
> > + err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SLC_IDLE,
> > + ROGUE_CR_SLC_IDLE_MASKFULL,
> > + ROGUE_CR_SLC_IDLE_MASKFULL,
> > + POLL_TIMEOUT_USEC);
> > + if (err)
> > + return err;
> > + }
> >
> > /*
> > * Wait for Sidekick/Jones to signal IDLE except for the
> > Garten Wrapper.
> > * For cores with the LAYOUT_MARS feature, SIDEKICK would
> > have been powered
> > * down by the FW.
> > */
> > - err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
> > sidekick_idle_mask,
> > - sidekick_idle_mask,
> > POLL_TIMEOUT_USEC);
> > - if (err)
> > - return err;
> > + if (mars) {
> > + err = pvr_cr_poll_reg32(pvr_dev,
> > ROGUE_CR_SIDEKICK_IDLE,
> > + sidekick_idle_mask,
> > + sidekick_idle_mask,
> > POLL_TIMEOUT_USEC);
> > + if (err)
> > + return err;
> > + }
> >
> > if (pvr_dev->fw_dev.processor_type ==
> > PVR_FW_PROCESSOR_TYPE_META) {
> > err = pvr_meta_cr_read32(pvr_dev,
> > META_CR_TxVECINT_BHALT, ®_value);
> > @@ -297,7 +312,13 @@ pvr_fw_stop(struct pvr_device *pvr_dev)
> > skip_garten_idle = true;
> > }
> >
> > - if (!skip_garten_idle) {
> > + if (mars) {
> > + err = pvr_cr_poll_reg32(pvr_dev,
> > ROGUE_CR_MARS_IDLE,
> > + mars_idle_mask,
> > mars_idle_mask,
> > + POLL_TIMEOUT_USEC);
> > + if (err)
> > + return err;
> > + } else if (!skip_garten_idle) {
> > err = pvr_cr_poll_reg32(pvr_dev,
> > ROGUE_CR_SIDEKICK_IDLE,
> > ROGUE_CR_SIDEKICK_IDLE_GART
> > EN_EN,
> > ROGUE_CR_SIDEKICK_IDLE_GART
> > EN_EN,
Powered by blists - more mailing lists