On Tue, Feb 07, 2023 at 12:05:43PM +0200, Jani Nikula wrote:
> [...]
> >> > I agree with making struct intel_dmc internal to intel_dmc.c. Since DC
> >> > state is a functionality provided by the firmware (except of DC9), an
> >> > alternative would be to move/add get/set/assert etc. DC state functions
> >> > to intel_dmc.c instead and call these from intel_display_power*.c.
> >>
> >> That was actually the first patch I wrote but didn't send. There were a
> >> few reasons I switched to this one:
> >>
> >> First, it adds a dependency between power well and dmc initialization.
> >
> > Do you mean the dependency that is there already?: taking some power ref
> > and keeping the whole runtime PM functionality disabled until the
> > firmware load completes. This is based on an earlier decision not to
> > support runtime PM w/o DMC.
>
> intel_power_domains_init() is called very early. It adjusts
> allowed_dc_mask and target_dc_state.
>
> intel_dmc_ucode_init() is called later, and depends on
> intel_power_domains_init() in the way you describe above.
>
> If intel_dmc_ucode_init() starts allocating intel_dmc dynamically, it
> needs to exist before intel_power_domains_init() modifies
> allowed_dc_mask and target_dc_state.
Ok, I missed all the above.
> It's an interdependency that would need to be broken up somehow.
>
> >> Second, it's a bunch of boilerplate, six get/set functions altogether,
> >> and all of them checking for dmc init in patch 3.
> >>
> >> Third, it still seems funny to have these members stored in intel_dmc,
> >> accessed via intel_dmc.c, but intel_dmc.c not actually using them for
> >> anything internally. It's only the power well code that uses
> >> them. Should more of the DC state handling be moved to intel_dmc.c then?
> >
> > I thought that any functions accessing the DC_STATE_EN register would be
> > moved as well (besides the state checkers you refer to). I wasn't sure
> > if my suggestion makes sense without actually seeing the end result; I
> > think we can also regard the DC_STATE_EN register as a more general
> > display PM HW interface leaving that in intel_display_power* (since DC9
> > which isn't a DMC thing is also toggled via it) and leave only the
> > firmware loading/initialization in intel_dmc.c as in your RFC.
>
> Yeah, I don't know. *lots of shrugging* :)
In the end I think it was wrong on my side to regard the DC_STATE_EN HW
i/f as a DMC internal thing. Rather it is for a display wide runtime PM
control. So yes, moving the DC state fields to intel_display_power.c
makes sense, as well as making the DMC FW internals local to
intel_dmc.c.
Could you follow up with the actual patches?
> >> BR,
> >> Jani.
> >>
> >> >> gen9_set_dc_state_debugmask(dev_priv);
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> index 88eae74dbcf2..da8ba246013e 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> >> >> @@ -40,9 +40,6 @@ struct intel_dmc {
> >> >> bool present;
> >> >> } dmc_info[DMC_FW_MAX];
> >> >>
> >> >> - u32 dc_state;
> >> >> - u32 target_dc_state;
> >> >> - u32 allowed_dc_mask;
> >> >> intel_wakeref_t wakeref;
> >> >> };
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> index 2954759e9d12..cf13580af34a 100644
> >> >> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> >> >> @@ -702,6 +702,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >> >> {
> >> >> const u32 crtc_vdisplay = crtc_state->uapi.adjusted_mode.crtc_vdisplay;
> >> >> struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> >> + struct i915_power_domains *power_domains = &dev_priv->display.power.domains;
> >> >> u32 exit_scanlines;
> >> >>
> >> >> /*
> >> >> @@ -718,7 +719,7 @@ tgl_dc3co_exitline_compute_config(struct intel_dp *intel_dp,
> >> >> if (crtc_state->enable_psr2_sel_fetch)
> >> >> return;
> >> >>
> >> >> - if (!(dev_priv->display.dmc.allowed_dc_mask & DC_STATE_EN_DC3CO))
> >> >> + if (!(power_domains->allowed_dc_mask & DC_STATE_EN_DC3CO))
> >> >> return;
> >> >>
> >> >> if (!dc3co_is_pipe_port_compatible(intel_dp, crtc_state))
> >> >> --
> >> >> 2.34.1
> >> >>
> >>
> >> --
> >> Jani Nikula, Intel Open Source Graphics Center
>
> --
> Jani Nikula, Intel Open Source Graphics Center
