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@xxxxxxxxx>: > > > On Mon, 2014-10-27 at 17:54 -0200, Paulo Zanoni wrote: > > >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > > >> > > >> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
