Re: [PATCH 1/2] drm/i915: kill intel_resume_prepare()

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 28, 2014 at 04:47:14PM +0200, Imre Deak wrote:
> On Tue, 2014-10-28 at 11:43 -0200, Paulo Zanoni wrote:
> > 2014-10-28 11:12 GMT-02:00 Imre Deak <imre.deak@intel.com>:
> > > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> Because, really, the abstraction is not working for us. It is nice for
> > >> VLV, but doesn't add anything useful on SNB/HSW/BDW. We want to change
> > >> this code due to a recently-discovered bug, but we can't seem to find
> > >> a nice solution that repects the current abstraction. So let's kill
> > >> intel_resume_prepare() and its friends, and add an equivalent
> > >> implementation to both its callers.
> > >>
> > >> Also, look at the diffstat!
> > >
> > > The reason for intel_resume_prepare() and intel_suspend_complete() was
> > > to contain platform dependent code in those and to share parts between
> > > the system and runtime suspend code, see the discussion at [1].
> > 
> > Well, IMHO we are just pretending to "share" the code paths between S3
> > and RPM because we still have the "bool rpm_resume" argument on the
> > current code. IMHO this is one of the biggest reasons why the code
> > became so complex: take a look at the second version of my patch, it
> > added even more "if (rpm_resume)" checks.
> 
> Yes, but that's one more thing we could try to remove at one point. For
> example intel_init_pch_refclk() in snb_resume_prepare() could be just as
> well called for both paths.
> 
> > I know that adding more
> > IS_VLV checks is not cool, but the S3 and RPM code paths are very
> > different here, and the VLV code even requires a really-weird
> > ordering. I _really_ think the code becomes much easier to
> > read/understand/modify by just killing intel_resume_prepare().
> > 
> > > I still
> > > think this is a good idea, but I admit we need to work on it more, by
> > > sharing more between the two paths. So for example instead of doing this
> > > revert now I would consider calling intel_uncore_early_sanitize() for
> > > both system and runtime resume.
> > 
> > That still wouldn't solve the ordering problem we have on S3. My goal
> > is just to fix a very simple bug with our function ordering...
> 
> I think it would by having in intel_resume_prepare():
> 
> if (IS_VALLEYVIEW(dev_priv))
> 	ret = vlv_resume_prepare(dev_priv, rpm_resume);
> intel_uncore_early_sanitize(dev,true);
> if (IS_HASWELL(dev_priv) ||IS_BROADWELL(dev_priv))
> 	hsw_disable_pc8(dev_priv);
> 
> and call it for both system and runtime resume.
> 
> > > But if that's not feasible and you want to go ahead with the removal
> > > then please also remove intel_suspend_complete(), leaving it in would be
> > > confusing imo.
> > 
> > I don't think so. It doesn't have all the "bool rpm_resume" confusion,
> > there's no function ordering issues with it, and both the S3 and RPM
> > versions are actually identical. I don't think killing it is relevant
> > to the bug we're trying to fix here, and I also don't think that
> > removing t would be an improvement to the code base.
> 
> It's confusing to call vlv_resume_prepare(), hsw_disable_pc8() from
> i915_drm_resume_early() directly and have to jump over some wrappers to
> find the corresponding vlv_suspend_complete(), hsw_enable_pc8() in
> i915_drm_suspend_complete(). But this is somewhat bikeshedding, so
> either way feel free to add on both patches:
> Reviewed-by: Imre Deak <imre.deak@intel.com>

Yeah that first attempt at unifying rpm and s3 didn't really pan out at
all. I guess we need to retry.

Meanwhile merged both patches to dinq.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx