| Message ID | 20240811082813.245871-1-uwu@icenowy.me (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/imagination: properly support stopping LAYOUT_MARS = 1 cores | expand |
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@icenowy.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). 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_GARTEN_EN, > ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
在 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@icenowy.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,
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); + /* * 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; + } /* 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_GARTEN_EN, ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN,
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@icenowy.me> --- .../gpu/drm/imagination/pvr_fw_startstop.c | 49 +++++++++++++------ 1 file changed, 35 insertions(+), 14 deletions(-)