On Sun, 12 Aug 2012 22:01:02 +0200, Daniel Vetter <daniel at ffwll.ch> wrote: > On Sun, Aug 12, 2012 at 08:47:56PM +0100, Chris Wilson wrote: > > On Sun, 12 Aug 2012 19:27:07 +0200, Daniel Vetter <daniel.vetter at ffwll.ch> wrote: > > > drm/i915: prepare load-detect pipe code for dpms changes > > > > It is not immediately obvious from the function that there is a > > relationship between the connector and intel_encoder. If we derived the > > encoder from the connector in that function, the reviewer's life gets a > > little easier. As it stands the code looks correct and rightly removes > > some internal details. > > Hm, good point. I'll add a patch on top that drops the intel_encoder > argument (since it's redudant, all callers get it with > intel_attached_encoder). Or better if I squash it together with this one? I'll buy it either way, with a slight preference to having it as two steps. > > > drm/i915: simplify dvo dpms interface > > > > This just looks like churn for churn's sake? The changes look correct. > > We don't bother with anything else than dpms on/off states in most of the > modeset code (even for crt newer hw drops the intermediate states). Hence > the new interfaces have only enable/disable functions at the encoder/crtc > level. I've figured it looks odd if we keep the full dpms interface for > dvo. But since it's rather independant churn I've moved it into this > odds bits series. The full fledged dpms mode isn't going to completely disappear thanks to the CRT dinosaur. I just wonder if we can achieve the same simplification by recognising that all non-zero dpms modes are off, e.g. s/dpms_mode/powersave/ if (powersave) switch_off() else switch_on() -Chris -- Chris Wilson, Intel Open Source Technology Centre
