[Intel-gfx] [PATCH 2/6] drm/i915: get mode clock when reading the pipe config v5

[Jesse Barnes]

On Thu, 23 May 2013 17:30:06 +0200
Daniel Vetter <daniel at ffwll.ch> wrote:

> On Tue, May 14, 2013 at 05:08:27PM -0700, Jesse Barnes wrote:
> > We need this for comparing modes between configuration changes.
> > 
> > v2: try harder to calulate non-simple pixel clocks (Daniel)
> >     call get_clock after getting the encoder config, needed for pixel multiply
> >     (Jesse)
> > v3: drop get_clock now that the pixel_multiply has been moved into
> >     get_pipe_config
> > v4: re-add get_clock; we need to get the pixel multiplier in the
> >     encoder, so need to calculate the clock value after the encoder's
> >     get_config is called
> > v5: drop hsw clock_get, still needs to be written
> > 
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> 
> Two things:
> - the pixel_multiplier should be a separate patch, and I think we should
>   add pipe_config check support for it. Maybe if we pick a default value
>   of 1 (both for the get_config hw state readout and the compute config
>   stuff in the modeset code) that should also simplify a bit since only
>   SDVO uses it atm. We still need to implement pixel multiplier stuff for
>   native hdmi ports ...

Yeah, I'm ambivalent about it.  I like it called out in the encoder
funcs just because the setting belongs there, but since only SDVO uses
it, it does seem wasteful.  Maybe I'm just being hopeful about the HDMI
side of things landing soon. :)

> - For the clock readout code I think we should be able to have pipe config
>   compare support (with adjusted_mode->clock), with a bit of fuzz at
>   least. Not on current dinq, but with my cleanup to give
>   adjusted_mode->clock saner semantics:
> 
>   http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/21750

So far I haven't needed the fuzz, but I'm not opposed to adding it as
needed.

> - There's no reason imo for the new ->get_clock vfunc, it splits exactly
>   the same as the existing ->get_pipe_config. So could simply be included
>   there.

Except that it depends on the results of the encoder->get_config
callback... that's the only reason I split it.  If you have ideas about
how to merge it back that's ok with me.

> With those three adjustements I think this is ready to go. For the
> follow-up patches though I think we should guard them with a fastboot
> module option.

Yeah, the main one there is the compute_config.  We can hide that
behind "boot_me_faster" or some such.

-- 
Jesse Barnes, Intel Open Source Technology Center