Re: [PATCH 0/7] modeset rework prep patches

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sun, 12 Aug 2012 22:01:02 +0200, Daniel Vetter <daniel@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@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