On Thu, May 12, 2022 at 03:37:14PM -0700, Lucas De Marchi wrote:
> On Thu, May 12, 2022 at 10:47:46PM +0300, Imre Deak wrote:
> > On Thu, May 12, 2022 at 10:56:11AM -0700, Lucas De Marchi wrote:
> > > On Thu, May 12, 2022 at 12:37:05PM +0300, Imre Deak wrote:
> > > > Based on a bspec update the DMC firmware's flip queue handling events
> > > > need to be disabled before enabling DC5/6. i915 doesn't use the flip
> > > > queue feature atm, so disable it already after loading the firmware.
> > > > This removes some overhead of the event handler which runs at a 1 kHz
> > > > frequency.
> > > >
> > > > Bspec: 49193, 72486, 72487
> > > >
> > > > v2:
> > > > - Fix the DMC pipe A register offsets for GEN12.
> > > > - Disable the events on DG2 only on pipe A..D .
> > > >
> > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx> # v1
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_dmc.c | 89 ++++++++++++++++++-
> > > > drivers/gpu/drm/i915/display/intel_dmc_regs.h | 41 +++++++++
> > > > 2 files changed, 129 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > index 257cf662f9f4b..0ede8c86c6ccb 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> > > > @@ -244,9 +244,14 @@ struct stepping_info {
> > > > char substepping;
> > > > };
> > > >
> > > > +static bool intel_dmc_has_fw_payload(struct drm_i915_private *i915, int dmc_id)
> > >
> > > in several places, including this file, we are trying to keep the
> > > convention of not using intel_ prefix for non-exported functions.
> > >
> > > has_dmc_id_fw() here would read better IMO.
> >
> > Ok.
> >
> > > > +{
> > > > + return i915->dmc.dmc_info[dmc_id].payload;
> > > > +}
> > > > +
> > > > bool intel_dmc_has_payload(struct drm_i915_private *i915)
> > > > {
> > > > - return i915->dmc.dmc_info[DMC_FW_MAIN].payload;
> > > > + return intel_dmc_has_fw_payload(i915, DMC_FW_MAIN);
> > > > }
> > > >
> > > > static const struct stepping_info *
> > > > @@ -268,6 +273,81 @@ static void gen9_set_dc_state_debugmask(struct drm_i915_private *dev_priv)
> > > > intel_de_posting_read(dev_priv, DC_STATE_DEBUG);
> > > > }
> > > >
> > > > +static void
> > > > +disable_simple_flip_queue_event(struct drm_i915_private *i915,
> > > > + i915_reg_t ctl_reg, i915_reg_t htp_reg)
> > > > +{
> > > > + u32 event_ctl;
> > > > + u32 event_htp;
> > > > +
> > > > + event_ctl = intel_de_read(i915, ctl_reg);
> > > > + event_htp = intel_de_read(i915, htp_reg);
> > > > + if (event_ctl != (DMC_EVT_CTL_ENABLE |
> > > > + DMC_EVT_CTL_RECURRING |
> > > > + REG_FIELD_PREP(DMC_EVT_CTL_TYPE_MASK,
> > > > + DMC_EVT_CTL_TYPE_EDGE_0_1) |
> > > > + REG_FIELD_PREP(DMC_EVT_CTL_EVENT_ID_MASK,
> > > > + DMC_EVT_CTL_EVENT_ID_CLK_MSEC)) ||
> > > > + !event_htp) {
> > > > + drm_dbg_kms(&i915->drm,
> > > > + "Unexpected DMC event configuration (control %08x htp %08x)\n",
> > > > + event_ctl, event_htp);
> > > > + return;
> > > > + }
> > >
> > > why are we doing this if we just want to disable? If we will always keep
> > > it disabled, then just writing the right values would be simpler.
> >
> > The requirement to disable flip queues explicitly came only now,
> > somewhat as a surprise. Future firmware versions will disable it by
> > default, but it's not clear at which point and how the ABI will change
> > then. So I'd like to keep the above check for any such ABI change in
> > place at least until those plans get clarified to us.
>
> humn... It seems weird to log "Unexpected DMC event... " if we are
> expecting that in future it will be disabled. But since it's a debug
> message only, it's not a big thing.
>
> Later are you expecting to separate the an if condition like below?
>
> if (!(event_ctl & DMC_EVT_CTL_ENABLE)) {
> drm_dbg_kms(&i915->drm, "Skip flip queue disabling: already disabled\n");
> return;
> }
I think after noticing via the above debug message that a given
platform/fw revision changed the MMIO list so that it doesn't enable the
flip queue events, we should adjust the platform/fw revision checks to
not call the disabling function for those.
> > > > +
> > > > + intel_de_write(i915, ctl_reg,
> > > > + REG_FIELD_PREP(DMC_EVT_CTL_TYPE_MASK,
> > > > + DMC_EVT_CTL_TYPE_EDGE_0_1) |
> > > > + REG_FIELD_PREP(DMC_EVT_CTL_EVENT_ID_MASK,
> > > > + DMC_EVT_CTL_EVENT_ID_FALSE));
> > > > + intel_de_write(i915, htp_reg, 0);
> > >
> > > matches bspec 72487 and 72486. It looks like we are missing a disable
> > > sequence for ADL-P though. Is it a missing documentation or function
> > > below should be updated to do nothing on ADL-P?
> >
> > First I thought ADL-P doesn't use the same 1 kHz event as the other
> > platforms, hence no disable sequence is defined by bspec. But looking
> > again it also seems to use that just with a different event ID
> > ('1KHZ_FLIPQ' vs. 'CLK_MSEC', see bspec 67608). I filed a bspec ticket
> > and will add the disabling for ADLP as well once that gets clarified.
>
> ok
>
> > > > +}
> > > > +
> > > > +static bool
> > > > +get_simple_flip_queue_event_regs(struct drm_i915_private *i915, int dmc_id,
> > > > + i915_reg_t *ctl_reg, i915_reg_t *htp_reg)
> > > > +{
> > > > + switch (dmc_id) {
> > > > + case DMC_FW_MAIN:
> > > > + if (DISPLAY_VER(i915) == 12) {
> > >
> > > Shouldn't this be >= 12? but see comment above about ADL-P
> >
> > It's only D12 platforms that use one event handler running on the main
> > DMC for this, all later platforms use one event handler running on each
> > pipe DMC, and no flip queue handler running on the main DMC.
>
> ok, this is the PIPEMDC_* vs DMC_* on Display 13 vs 12.
>
>
> > > > + *ctl_reg = DMC_EVT_CTL(i915, dmc_id, 3);
> > > > + *htp_reg = DMC_EVT_HTP(i915, dmc_id, 3);
> > >
> > > For DG2 the sequence in bspec is:
> > >
> > > 1. Disable flip queue
> > > 2. PIPEDMC_EVT_CTL_2_A/B/C/D = 0x00030100
> > > 3. PIPEDMC_EVT_HTP_2_A/B/C/D = 0x00000000
> > >
> > > where did you get that for main dmc you need to write to CTL_3/HTP_3?
> >
> > Bspec/72486, and checking the fixed MMIOs in the TGL/RKL firmwares (for
> > the above 1kHz events).
>
> yeah, looks good. Kind of same issue I had reading the spec in the
> question above. That answers it.
>
> > > > +
> > > > + return true;
> > > > + }
> > > > + break;
> > > > + case DMC_FW_PIPEA ... DMC_FW_PIPED:
> > > > + if (IS_DG2(i915)) {
> > > > + *ctl_reg = DMC_EVT_CTL(i915, dmc_id, 2);
> > > > + *htp_reg = DMC_EVT_HTP(i915, dmc_id, 2);
> > > > +
> > > > + return true;
> > > > + }
> > > > + break;
> > > > + }
> > > > +
> > > > + return false;
> > > > +}
> > > > +
> > > > +static void
> > > > +disable_all_simple_flip_queue_events(struct drm_i915_private *i915)
> > > > +{
> > > > + int dmc_id;
> > > > +
> > > > + for (dmc_id = 0; dmc_id < DMC_FW_MAX; dmc_id++) {
> > > > + i915_reg_t ctl_reg;
> > > > + i915_reg_t htp_reg;
> > > > +
> > > > + if (!intel_dmc_has_fw_payload(i915, dmc_id))
> > > > + continue;
> > > > +
> > > > + if (!get_simple_flip_queue_event_regs(i915, dmc_id, &ctl_reg, &htp_reg))
> > > > + continue;
> > > > +
> > > > + disable_simple_flip_queue_event(i915, ctl_reg, htp_reg);
> > > > + }
> > > > +}
> > >
> > > it seems we are mixing "flip queue" and "simple flip queue". Maybe just
> > > remove "simple" as it doesn't add much information?
> >
> > The other variant is called batch flip queue, but can rename this to
> > flip_queue.
>
> My preference is to drop "simple", but feel free to keep it if you want.
>
> > > > +
> > > > /**
> > > > * intel_dmc_load_program() - write the firmware from memory to register.
> > > > * @dev_priv: i915 drm device.
> > > > @@ -308,6 +388,13 @@ void intel_dmc_load_program(struct drm_i915_private *dev_priv)
> > > > dev_priv->dmc.dc_state = 0;
> > > >
> > > > gen9_set_dc_state_debugmask(dev_priv);
> > > > +
> > > > + /*
> > > > + * Flip queue events need to be disabled before enabling DC5/6.
> > > > + * i915 doesn't use the flip queue feature, so disable it already
> > > > + * here.
> > > > + */
> > > > + disable_all_simple_flip_queue_events(dev_priv);
> > > > }
> > > >
> > > > void assert_dmc_loaded(struct drm_i915_private *i915)
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_regs.h b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
> > > > index d65e698832eb5..43d780148b196 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dmc_regs.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dmc_regs.h
> > > > @@ -10,6 +10,47 @@
> > > >
> > > > #define DMC_PROGRAM(addr, i) _MMIO((addr) + (i) * 4)
> > > > #define DMC_SSP_BASE_ADDR_GEN9 0x00002FC0
> > > > +
> > > > +#define _PIPEDMC_REG_MMIO_BASE_A_GEN13 0x5f000
> > > > +#define _PIPEDMC_REG_MMIO_BASE_A_GEN12 0x92000
> > >
> > > no _GEN12/_GEN13 suffix
> >
> > Why not? Is _ADLP/_TGL ok?
>
> We are not supposed to add GEN<x> anymore as that doesn't make sense for
> our gpus anymore. Versions are separated per IP like display, graphics,
> media. We went on an effort to rename everything last year. We still
> have some, but we should avoid adding more.
>
> yes, using the first platform that uses the register keeps with what's
> being used in i915_drv.h
Ok. Looks like flags use a suffix and regs/bases a prefix, so will use
ADLP_*/TGL_*.
> > > > +
> > > > +#define _PIPEDMC_REG_MMIO_BASE(i915, dmc_id) \
> > > > + ((DISPLAY_VER(i915) >= 13 ? _PIPEDMC_REG_MMIO_BASE_A_GEN13 : \
> > > > + _PIPEDMC_REG_MMIO_BASE_A_GEN12) + \
> > > > + 0x400 * ((dmc_id) - 1))
> > > > +
> > > > +#define _MAINDMC_REG_MMIO_BASE 0x8f000
> > > > +
> > > > +#define _DMC_REG_MMIO_BASE(i915, dmc_id) \
> > > > + ((dmc_id) == DMC_FW_MAIN ? _MAINDMC_REG_MMIO_BASE : \
> > > > + _PIPEDMC_REG_MMIO_BASE(i915, dmc_id))
> > > > +
> > > > +#define _DMC_REG(i915, dmc_id, reg) \
> > > > + ((reg) - _MAINDMC_REG_MMIO_BASE + _DMC_REG_MMIO_BASE(i915, dmc_id))
> > > > +
> > > > +#define _MAINDMC_EVT_HTP_0 0x8f004
> > > > +
> > > > +#define DMC_EVT_HTP(i915, dmc_id, handler) \
> > > > + _MMIO(_DMC_REG(i915, dmc_id, _MAINDMC_EVT_HTP_0) + 4 * (handler))
> > > > +
> > > > +#define _MAINDMC_EVT_CTL_0 0x8f034
> > >
> > > if we were to follow the spec names, we'd rather name
> > > _DMC_* for main fw
> > > _PIPEDMC_* for others.
> >
> > Ok.
> >
> > > > +
> > > > +#define DMC_EVT_CTL(i915, dmc_id, handler) \
> > > > + _MMIO(_DMC_REG(i915, dmc_id, _MAINDMC_EVT_CTL_0) + 4 * (handler))
> > >
> > > s/handler/offset/?
> >
> > There are a number of event handlers each main/pipe DMC can run,
> > each of which is configured via its control and HTP register. 'handler'
> > refers to such handler instances.
>
> makes sense, it was my lack of understanding on this part of the spec.
>
> > > It seems we have to massage the macros everywhere to handle pipe vs
> > > main. Given get_simple_flip_queue_event_regs() already handle them
> > > separate, I think it would be simpler to just split the macros on DMC_*
> > > vs PIPEDMC_*, which would be more inline with the spec too.
> >
> > There are cases where it's practical being able to loop through all main
> > and pipe DMC registers in one go, for instance for clearing all
> > event control and HTP registers when reloading the firmware.
>
> humn, I have the impression in most cases the code is cleaner handling
> the MAIN DMC separate from the loop on the pipe DMCs. But if you want to
> keep them together on the macro, I'm fine.
I meant for instance the still to-be-added init from bspec/49193
"Sequence to reload DMC Firmware". It could be just
for_each_dmc_id
for_each_handler
init_evt_ctl(dmc_id, handler)
init_evt_htp(dmc_id, handler)
instead of having two separate loops for both the MAIN DMC and pipe DMCs.
> > > Maybe the most important question:
> > >
> > > with this patch + the patch to load DMC on DG2, do we get the DC5
> > > transition to work? It'd be good to submit both together so we can
> > > ensure it does.
> >
> > I think disabling flip queues is the correct thing even w/o considering
> > DC5. But yes, on DG2 it happens to fix that. The idea was to merge this
> > patch first to get separate CI results and follow up with a change to
> > enable DC5 on DG2.
>
> The problem from my pov is avoid merging something just to realize later
> there is something wrong we didn't find in the review and having to fix
> it up on top.
We did a bunch of both manual and full CI run testing on DG2 with this
patch and enabling DC5. I can send a trybot patchset with both and check
that result too before merging this one.
> Anyway, code here looks correct. Feel free to add
>
> Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>
> on the next version.
Thanks.
> thanks
> Lucas De Marchi
>
> >
> > > Lucas De Marchi
> > >
> > > > +
> > > > +#define DMC_EVT_CTL_ENABLE REG_BIT(31)
> > > > +#define DMC_EVT_CTL_RECURRING REG_BIT(30)
> > > > +#define DMC_EVT_CTL_TYPE_MASK REG_GENMASK(17, 16)
> > > > +#define DMC_EVT_CTL_TYPE_LEVEL_0 0
> > > > +#define DMC_EVT_CTL_TYPE_LEVEL_1 1
> > > > +#define DMC_EVT_CTL_TYPE_EDGE_1_0 2
> > > > +#define DMC_EVT_CTL_TYPE_EDGE_0_1 3
> > > > +
> > > > +#define DMC_EVT_CTL_EVENT_ID_MASK REG_GENMASK(15, 8)
> > > > +#define DMC_EVT_CTL_EVENT_ID_FALSE 0x01
> > > > +/* An event handler scheduled to run at a 1 kHz frequency. */
> > > > +#define DMC_EVT_CTL_EVENT_ID_CLK_MSEC 0xbf
> > > > +
> > > > #define DMC_HTP_ADDR_SKL 0x00500034
> > > > #define DMC_SSP_BASE _MMIO(0x8F074)
> > > > #define DMC_HTP_SKL _MMIO(0x8F004)
> > > > --
> > > > 2.30.2
> > > >
