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

[Daniel Vetter <daniel@ffwll.ch>]

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:
> > Hi all,
> > 
> > I've noticed that a few prep patches of the modeset rework series haven't been
> > merged nor reviewed yet, so I've split them out in this resend. Mostly concern
> > really old gen2 stuff (dvo + pipe A quirk), but little patches in other areas.
> > 
> > Comments&review highly welcome.
> > 
> > Thanks, Daniel
> > 
> > Daniel Vetter (7):
> >   drm/i915: add missing gen2 pipe A quirk entries
> 
> I remain dubious whether the 855gm entry is genuine.

Ok, since we don't have a bug to support this, I'll drop it again.

> >   drm/i915/ns2501: kill pll A enabling hack
> >   drm/i915: rip out the overlay pipe A workaround
> 
> Look fine and a welcome reduction in code + confusion.
> 
> >   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?

> >   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.

> >   drm/i915: kill a few unused things in dev_priv
> 
> +1
> 
> >   drm/i915: extract ironlake_fdi_pll_disable
> 
> Smaller more descriptive functions, what is not to like.
> 
> 2-7: Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> You can have an r-b for 1 if you drop the 855gm chunk. Then get someone
> else to a-b the 855gm entry :)

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48