Re: [PATCH 3/8] drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 19, 2012 at 03:11:33PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/12 Daniel Vetter <daniel@ffwll.ch>:
> > On Wed, Sep 12, 2012 at 10:06:31AM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Hm, I think we should extract the same code from i9xx_crtc_set_mode, too
> > and share it in a common intel_set_pipe_timings. Their almost identical
> > safe for:
> > - vsyncshift is only gen4+
> 
> This is easy to solve.
> 
> > - source position handling is a bit different, but I think it'd be
> >   semantically clearer if we leave that out of set_pipe_timings. Imo that
> >   belongs to the panel fitter settings, which are currently splattered all
> >   over. Meh.
> 
> Well, the PIPESRC register is described inside the "pipe timings"
> documentation section, so I think it should be inside the
> set_pipe_timings function.
> 
> I actually implemented your suggestion locally and the only real
> problem is that on i9xx_crtc_mode_set we currently write to DSPSIZE
> and DSPPOS before writing to PIPESRC, so to make the code look good we
> have 2 options:
> 1 - Write to DSPPOS and DSPSIZE before writing all the timing registers
> 2 - Write to DSPPOS and DSPSIZE after writing all the timing registers
> 
> In both cases we are changing the writing order. I looked at the
> documentation and it seems we should be writing to the plane registers
> only after setting the pipe registers, so maybe solution 2 is the
> correct. The problem is that yes, we are changing the behavior and I
> don't even have such machines to test.
> 
> So, how do we proceed here? Want the version, keep the old one, or do
> something entirely different?

I guess a quick patch to move around the DSP* regs down and run it on a
few gen2/3 machines. Then move things around if it doesn't blow up. Since
I'm travelling I think you need to volunteer Chris for the testing ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch