Quoting Daniele Ceraolo Spurio (2019-06-18 19:40:40) > > > On 6/18/19 3:22 AM, Chris Wilson wrote: > > Quoting Daniele Ceraolo Spurio (2019-06-17 19:09:33) > >> We always call some of the setup/cleanup functions for forcewake, even > >> if the feature is not actually available. Skipping these operations if > >> forcewake is not available saves us some operations on older gens and > >> prepares us for having a forcewake-less display uncore. > >> The suspend/resume functions have also been renamed to clearly indicate > >> that they only operate on forcewake status. > >> > >> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> > >> --- > >> drivers/gpu/drm/i915/i915_drv.c | 15 +-- > >> drivers/gpu/drm/i915/intel_uncore.c | 147 +++++++++++++++++----------- > >> drivers/gpu/drm/i915/intel_uncore.h | 8 +- > >> 3 files changed, 101 insertions(+), 69 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > >> index d113296cbe34..95b36fe17f99 100644 > >> --- a/drivers/gpu/drm/i915/i915_drv.c > >> +++ b/drivers/gpu/drm/i915/i915_drv.c > >> @@ -996,7 +996,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv) > >> > >> intel_device_info_init_mmio(dev_priv); > >> > >> - intel_uncore_prune_mmio_domains(&dev_priv->uncore); > >> + intel_uncore_prune_forcewake_domains(&dev_priv->uncore); > > > > The _prune_ was the exceptional case... > > > >> intel_uc_init_mmio(dev_priv); > >> > >> @@ -2152,7 +2152,7 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation) > >> > >> i915_gem_suspend_late(dev_priv); > >> > >> - intel_uncore_suspend(&dev_priv->uncore); > >> + intel_uncore_forcewake_suspend(&dev_priv->uncore); > > > > But are you sure you want to delegate all the fw control to i915_drv.c, > > and not keep this as a call into intel_uncore_suspend() ? It is meant to > > be telling the uncore functionality that it is time to suspend, and it > > has to work out what to save. > > > > I am not sold on this yet. (One day this will just be a bunch of async > > tasks plugged into signals with ordering determined by their > > self-declared dependency tree. One day.) > > > > So sell me on why we want an uncore_forcewake object, as currently you > > are proposing a intel_uncore_suspend_forcewake(). > > -Chris > > > > My aim was to make it clear that this call will not be required for > display_uncore since there is nothing to do on suspend/resume if there > is no forcewake (and you're right, intel_uncore_suspend_forcewake is a > better naming). Ultimately I'd like to transition the GT uncore inside > intel_gt and call this inside intel_gt_suspend(). However, I don't mind > keeping the current naming and calling it for display uncore as well to > be more generic, there are checks everywhere so the overhead should me > minimal. What's your preference? I think, for the time being a sketch like i915_drm_suspend: intel_uncore_suspend(i915->gt.uncore) intel_uncore_suspend(i915->display.uncore) is preferable. Rather than pulling the knowlegde that we need only suspend the gt.uncore because it has forcewake into the high level i915_drm_suspend. A couple of ifs or a vfunc go a long way to keeping as simple as it can be (and no simpler). At the i915_drm_suspend level it is hard enough to manage a list of components and the correct order in which to call them :) -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
