[Intel-gfx] [PATCH v6 2/8] drm/i915: Use cached cdclk value

[Damien Lespiau]

On Mon, Jun 15, 2015 at 01:40:24PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/15/2015 01:14 PM, Damien Lespiau wrote:
> >On Mon, Jun 15, 2015 at 01:54:40PM +0200, Daniel Vetter wrote:
> >>On Wed, Jun 03, 2015 at 03:45:08PM +0300, Mika Kahola wrote:
> >>>From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>
> >>>Rather than reading out the current cdclk value use the cached value we
> >>>have tucked away in dev_priv.
> >>>
> >>>v2: Rebased to the latest
> >>>v3: Rebased to the latest
> >>>v4: Fix for patch style problems
> >>>
> >>>Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>Signed-off-by: Mika Kahola <mika.kahola at intel.com>
> >>
> >>This patch needs to be extended to also cover the recently added
> >>skl_max_scale. Tvrtko has recently written a patch to add some checks to
> >>that code too, would be good to resurrect that too. Chandra can help with
> >>any questions wrt the skl scaler code.
> >
> >Not quite I'm afraid. The CDCLK used in skl_max_scale() has to be part
> >of the atomic state, even bumping CDCLK if possible/needed.
> >
> >If you use the cached cdclk in skl_max_scale(), it won't do the right
> >thing when CDCLK is off (ie cached frew is the fallback 24Mhz ref clock)
> >and we try to do the first modeset before waking up the display.
> >
> >I filed a bug about it already to track it:
> >
> >   https://bugs.freedesktop.org/show_bug.cgi?id=90874
> 
> I know nothing about these specific clocks, but FWIW, my patch was only
> about enabling new platforms - making skl_max_scale more robust in cases
> where clock querying does not yet work correctly.
> 
> My reasoning was based on a comment from Ville that one of those two clocks
> must never be lower than the other.
> 
> So it sounded reasonable to ignore such cases ie. assume no scaling is
> possible and allow a normal (unscaled) modeset to succeed rather than fail
> it and display nothing.

So to be more specific, I believe this is because we detect CDCLK as
being "disabled" or on the ref clock in simulation?

Generally speaking, it's questionable if we want to work around such
limitations in the code like that, I'd rather go for defaulting to max
CDCLK in simulation.

In this particular case, we really shouldn't get cdclk < crtc_clock at
this point, I'd expect the cdclk we use (probably part of the atomic
state) to be bumped to cover crtc_clock prior to plane checks (See
Marteen's [PATCH v3 19/19] drm/i915: Make cdclk part of the atomic
state.), I guess we could add a WARN_ON(cdclk < crtc_clock) in
skl_max_scale() to ensure that's indeed the case?

-- 
Damien